Bug 77299

Summary: Reduce non-CSSOM API of CSSStyleDeclaration
Product: WebKit Reporter: Antti Koivisto <koivisto>
Component: CSSAssignee: 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 Flags
patch
webkit-ews: commit-queue-
try to fix chromium build
webkit-ews: commit-queue-
another
webkit-ews: commit-queue-
try to fix qt build
none
updated to tot
webkit-ews: commit-queue-
another one kling: review+

Description Antti Koivisto 2012-01-29 08:36:03 PST
CSSStyleDeclaration should expose CSSOM API only. Subclasses should expose the internal API only. This will move us closer to being able to split the CSSOM API from the CSS implementation.
Comment 1 Antti Koivisto 2012-01-29 10:46:23 PST
Created attachment 124466 [details]
patch
Comment 2 Early Warning System Bot 2012-01-29 11:06:07 PST
Comment on attachment 124466 [details]
patch

Attachment 124466 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11369330
Comment 3 WebKit Review Bot 2012-01-29 11:16:49 PST
Comment on attachment 124466 [details]
patch

Attachment 124466 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11360662
Comment 4 Antti Koivisto 2012-01-29 12:32:12 PST
Created attachment 124467 [details]
try to fix chromium build
Comment 5 WebKit Review Bot 2012-01-29 12:35:58 PST
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 6 Early Warning System Bot 2012-01-29 12:51:41 PST
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 7 WebKit Review Bot 2012-01-29 13:03:31 PST
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
Comment 8 Antti Koivisto 2012-01-29 13:16:35 PST
Created attachment 124468 [details]
another
Comment 9 Early Warning System Bot 2012-01-29 13:34:32 PST
Comment on attachment 124468 [details]
another

Attachment 124468 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11368348
Comment 10 Darin Adler 2012-01-29 19:23:13 PST
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.
Comment 11 Antti Koivisto 2012-01-30 00:21:19 PST
Created attachment 124503 [details]
try to fix qt build
Comment 12 Antti Koivisto 2012-01-30 00:34:20 PST
Created attachment 124504 [details]
updated to tot
Comment 13 Early Warning System Bot 2012-01-30 01:55:29 PST
Comment on attachment 124504 [details]
updated to tot

Attachment 124504 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11371073
Comment 14 Antti Koivisto 2012-01-30 03:48:05 PST
Created attachment 124518 [details]
another one

Another qt fix attempt
Removed unnecessary exception code taking setProperty/removeProperty versions from CSSMutableStyleDeclaration
Comment 15 Nikolas Zimmermann 2012-01-30 04:09:02 PST
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.
Comment 16 Antti Koivisto 2012-01-30 04:11:58 PST
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 17 Andreas Kling 2012-01-30 08:41:19 PST
Comment on attachment 124518 [details]
another one

r=me, I agree that we should refactor and cleanup separately.
Comment 18 Antti Koivisto 2012-01-30 08:48:00 PST
http://trac.webkit.org/changeset/106247
Comment 19 Darin Adler 2012-02-07 12:41:55 PST
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!