Summary: | Reduce non-CSSOM API of CSSStyleDeclaration | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Antti Koivisto <koivisto> | ||||||||||||||
Component: | CSS | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | abarth, darin, dglazkov, japhet, kling, macpherson, webkit.review.bot | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Bug Depends on: | |||||||||||||||||
Bug Blocks: | 77745 | ||||||||||||||||
Attachments: |
|
Description
Antti Koivisto
2012-01-29 08:36:03 PST
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! |