Bug 77299

Created attachment 124466 [details] patch

Comment on attachment 124466 [details] patch Attachment 124466 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11369330

Comment on attachment 124466 [details] patch Attachment 124466 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11360662

Created attachment 124467 [details] try to fix chromium build

Attachment 124467 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/ChangeLog:16: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5] Total errors found: 1 in 25 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 124467 [details] try to fix chromium build Attachment 124467 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11359664

Comment on attachment 124467 [details] try to fix chromium build Attachment 124467 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11366361

Created attachment 124468 [details] another

Comment on attachment 124468 [details] another Attachment 124468 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11368348

Qt failures: 1) In qwebelement.cpp because QWebElement::styleProperty uses StyledElement::style() instead of StyledElement:: inlineStyleDecl() or StyledElement::ensureInlineStyleDecl(). 2) In DumpRenderTreeSupportQt.cpp because DumpRenderTreeSupportQt::computedStyleIncludingVisitedInfo calls some functions on CSSComputedStyleDeclaration that are not used elsewhere to convert it to a QVariantMap. I am not sure why computedStyleIncludingVisitedInfo returns a QVariantMap, since the version in Mac WebKit returns a CSSComputedStyleDeclaration to JavaScript directly, which is better. The best thing to do with computedStyleIncludingVisitedInfo would be to move it out of the DumpRenderTree implementations and into the internals object now that we have that.

Created attachment 124503 [details] try to fix qt build

Created attachment 124504 [details] updated to tot

Comment on attachment 124504 [details] updated to tot Attachment 124504 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11371073

Created attachment 124518 [details] another one Another qt fix attempt Removed unnecessary exception code taking setProperty/removeProperty versions from CSSMutableStyleDeclaration

Comment on attachment 124518 [details] another one View in context: https://bugs.webkit.org/attachment.cgi?id=124518&action=review First round of comments, not changing r? state yet. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2569 > + for (unsigned i = 0; i < length; i++) { nitpick: ++i > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2571 > + RefPtr<CSSValue> value = getPropertyCSSValue(set[i]); > + if (value) This can be turned into one line. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2582 > + if (!propertyID) > + return 0; > + return getPropertyCSSValue(propertyID); If the common case is property is found (which I assume), I'd swap these. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2589 > + if (!propertyID) > + return String(); Ditto. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2596 > + return ""; return emptyString(), it's shared and more efficient. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2601 > + return ""; Ditto. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2617 > + return String(); Ditto. > Source/WebCore/css/CSSMutableStyleDeclaration.cpp:1052 > + if (!propertyID) > + return 0; Same swapping possible. > Source/WebCore/css/CSSMutableStyleDeclaration.cpp:1060 > + if (!propertyID) > + return String(); Ditto + use emptyString(). > Source/WebCore/css/CSSMutableStyleDeclaration.cpp:1069 > + if (!propertyID) > + return String(); > + return getPropertyPriority(propertyID) ? "important" : ""; Ditto + use emptyString(). > Source/WebCore/css/CSSMutableStyleDeclaration.cpp:1080 > + if (!propertyID) > + return String(); > + int shorthandID = getPropertyShorthand(propertyID); > + if (!shorthandID) > + return String(); > + return getPropertyName(static_cast<CSSPropertyID>(shorthandID)); Ditto + use emptyString(). > Source/WebCore/css/CSSMutableStyleDeclaration.cpp:1086 > + if (!propertyID) etc, .. not listing the others. > Source/WebCore/css/CSSMutableStyleDeclaration.cpp:1147 > + { Is the extra scope intentional? I don't see it's needed. > Source/WebCore/css/CSSMutableStyleDeclaration.cpp:1158 > + // FIXME: This should use mass removal. > + for (unsigned i = 0; i < propertiesToRemove.size(); i++) > + removeProperty(propertiesToRemove[i]); How about adding removeProperty(Vector<int>&) and moving the FIXME into that method? > Source/WebCore/editing/Editor.cpp:2754 > + style->setProperty(CSSPropertyWordWrap, "break-word", false); While you're at it, how about using getValueName(CSSValueBreakWord) instead of hard-coding it here? > Source/WebCore/editing/Editor.cpp:2755 > + style->setProperty(CSSPropertyWebkitNbspMode, "space", false); Ditto, for all other props.

As mentioned in the IRC, there is huge amount of additional refactoring that could be done. This is not meant to be an end state. The patch is already so large it is unwieldy.

Comment on attachment 124518 [details] another one r=me, I agree that we should refactor and cleanup separately.

Comment on attachment 124518 [details] another one View in context: https://bugs.webkit.org/attachment.cgi?id=124518&action=review >> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2582 >> + if (!propertyID) >> + return 0; >> + return getPropertyCSSValue(propertyID); > > If the common case is property is found (which I assume), I'd swap these. We like early return for error cases. The comment implies we think that compilers run an if statement faster if it is true; that is not correct. The style here should not be changed!