Bug 77299 - Reduce non-CSSOM API of CSSStyleDeclaration
Summary: Reduce non-CSSOM API of CSSStyleDeclaration
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 77745
  Show dependency treegraph
 
Reported: 2012-01-29 08:36 PST by Antti Koivisto
Modified: 2012-02-07 12:41 PST (History)
7 users (show)

See Also:


Attachments
patch (55.13 KB, patch)
2012-01-29 10:46 PST, Antti Koivisto
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
try to fix chromium build (59.06 KB, patch)
2012-01-29 12:32 PST, Antti Koivisto
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
another (59.14 KB, patch)
2012-01-29 13:16 PST, Antti Koivisto
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
try to fix qt build (61.86 KB, patch)
2012-01-30 00:21 PST, Antti Koivisto
no flags Details | Formatted Diff | Diff
updated to tot (61.39 KB, patch)
2012-01-30 00:34 PST, Antti Koivisto
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
another one (69.21 KB, patch)
2012-01-30 03:48 PST, Antti Koivisto
kling: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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!