Bug 77299 – Reduce non-CSSOM API of CSSStyleDeclaration

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Antti Koivisto]

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.

[Antti Koivisto]

Created attachment 124466 [details]
patch

[Early Warning System Bot]

Comment on attachment 124466 [details]
patch

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

[WebKit Review Bot]

Comment on attachment 124466 [details]
patch

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

[Antti Koivisto]

Created attachment 124467 [details]
try to fix chromium build

[WebKit Review Bot]

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.

[Early Warning System Bot]

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

[WebKit Review Bot]

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

[Antti Koivisto]

Created attachment 124468 [details]
another

[Early Warning System Bot]

Comment on attachment 124468 [details]
another

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

[Darin Adler]

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.

[Antti Koivisto]

Created attachment 124503 [details]
try to fix qt build

[Antti Koivisto]

Created attachment 124504 [details]
updated to tot

[Early Warning System Bot]

Comment on attachment 124504 [details]
updated to tot

Attachment 124504 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11371073

[Antti Koivisto]

Created attachment 124518 [details]
another one

Another qt fix attempt
Removed unnecessary exception code taking setProperty/removeProperty versions from CSSMutableStyleDeclaration

[Nikolas Zimmermann]

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.

[Antti Koivisto]

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.

[Andreas Kling]

Comment on attachment 124518 [details]
another one

r=me, I agree that we should refactor and cleanup separately.

[Antti Koivisto]

http://trac.webkit.org/changeset/106247

[Darin Adler]

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!