Bug 82728 – Split remaining CSSRules into internal and CSSOM types

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

[Antti Koivisto]

Complete the move to internal CSS rule types.

[Antti Koivisto]

Created attachment 134800 [details]
for bots

[Antti Koivisto]

Created attachment 134814 [details]
rebased

[WebKit Review Bot]

Attachment 134814 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/WebCore.xcodeproj/project.p..." exit_code: 1
Source/WebCore/css/CSSParser.h:259:  The parameter name "marginBox" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/css/CSSFontSelector.cpp:48:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/css/StyleRule.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/css/StyleRule.h:109:  The parameter name "properties" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/css/WebKitCSSKeyframesRule.h:74:  The parameter name "name" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/css/CSSStyleSheet.h:147:  The return type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/css/CSSStyleSheet.cpp:120:  The return type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/css/CSSStyleSheet.cpp:126:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/css/CSSStyleSheet.cpp:203:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/WebCore/css/CSSStyleSheet.cpp:232:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/css/CSSStyleSheet.cpp:302:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 11 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 134814 [details]
rebased

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

[Build Bot]

Comment on attachment 134814 [details]
rebased

Attachment 134814 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12267256

[Early Warning System Bot]

Comment on attachment 134814 [details]
rebased

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

[Gyuyoung Kim]

Comment on attachment 134814 [details]
rebased

Attachment 134814 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12253312

[Andreas Kling]

Comment on attachment 134814 [details]
rebased

View in context: https://bugs.webkit.org/attachment.cgi?id=134814&action=review

Dumping IRC whine for posterity.

> Source/WebCore/bindings/js/JSDOMBinding.h:204
>      inline void* root(StyleSheet* styleSheet)
>      {
> +        if (styleSheet->ownerNode())
> +            return root(styleSheet->ownerNode());
> +        return styleSheet;
> +    }

This looks like it will do the wrong thing if the StyleSheet* passed points to a CSSStyleSheet object.

> Source/WebCore/css/StyleRule.h:56
> +    bool isKeyframeRule() const { return false; }

This should be removed. Calling it would indicate a bug, as it can only ever return false.

[WebKit Review Bot]

Comment on attachment 134814 [details]
rebased

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

[Build Bot]

Comment on attachment 134814 [details]
rebased

Attachment 134814 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12288273

[Gustavo Noronha (kov)]

Comment on attachment 134814 [details]
rebased

Attachment 134814 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12266336

[Antti Koivisto]

Created attachment 134833 [details]
try to fix build

[WebKit Review Bot]

Attachment 134833 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/WebCore.xcodeproj/project.p..." exit_code: 1
Source/WebCore/css/CSSStyleSheet.cpp:127:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/css/CSSStyleSheet.cpp:232:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/css/CSSStyleSheet.cpp:302:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 3 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 134833 [details]
try to fix build

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

[Antti Koivisto]

Created attachment 134853 [details]
another one

[WebKit Review Bot]

Attachment 134853 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/WebCore.xcodeproj/project.p..." exit_code: 1
Source/WebCore/css/CSSStyleSheet.cpp:127:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/css/CSSStyleSheet.cpp:232:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/css/CSSStyleSheet.cpp:302:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 3 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 134853 [details]
another one

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

New failing tests:
http/tests/inspector/extensions-headers.html
accessibility/aria-disabled.html
http/tests/inspector/extensions-ignore-cache.html
http/tests/inspector/console-xhr-logging.html
http/tests/inspector/console-xhr-logging-async.html
http/tests/inspector/console-cd.html
http/tests/inspector/console-cd-completions.html
http/tests/inspector/injected-script-for-origin.html
http/tests/inspector/console-cross-origin-iframe-logging.html
http/tests/inspector/network-preflight-options.html
http/tests/inspector/extensions-network-redirect.html
http/tests/inspector/console-websocket-error.html
http/tests/inspector/modify-cross-domain-rule.html
http/tests/inspector/change-iframe-src.html
http/tests/inspector/compiler-script-mapping.html
http/tests/inspector/extensions-useragent.html
http/tests/inspector/compiler-source-mapping-debug.html
http/tests/inspector/inspect-iframe-from-different-domain.html
http/tests/inspector/console-resource-errors.html
http/tests/inspector/inspect-element.html

[WebKit Review Bot]

Created attachment 134873 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Antti Koivisto]

Created attachment 135091 [details]
patch

[WebKit Review Bot]

Attachment 135091 [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/css/CSSStyleSheet.cpp:127:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/css/CSSStyleSheet.cpp:233:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/css/CSSStyleSheet.cpp:303:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 3 in 36 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexis Menard (darktears)]

Comment on attachment 135091 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=135091&action=review

> Source/WebCore/ChangeLog:27
> +        StyleRuleBase is refounted. 

Typo here.

> Source/WebCore/ChangeLog:29
> +        The CSSOM objects contain the public API functions and their implemenations (almost) nothing else. Unlike the 

Typo again. Is that beer?

> Source/WebCore/bindings/js/JSDOMBinding.h:198
> +    

Whitespaces.

[Alexis Menard (darktears)]

Comment on attachment 135091 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=135091&action=review

> Source/WebCore/css/CSSImportRule.cpp:180
> +    

Whitespaces no?

> Source/WebCore/css/CSSImportRule.cpp:186
> +    

Whitespaces no?

[Andreas Kling]

Comment on attachment 135091 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=135091&action=review

Love it.
Would be nice to get EWS input though.

> Source/WebCore/ChangeLog:27
> +        StyleRuleBase is refounted. 

Typo, refcounted.

> Source/WebCore/css/CSSImportRule.h:57
>      // to avoid adding a vptr to CSSImportRule.

This comment should be updated to say StyleRuleImport.

> Source/WebCore/css/CSSMediaRule.cpp:45
> +    for (unsigned i = 0; i < m_childRuleCSSOMWrappers.size(); ++i) {

Official Antti Coding Style dictates that size() should be kept in a separate variable.

> Source/WebCore/css/CSSMediaRule.h:58
> +    mutable Vector<RefPtr<CSSRule> > m_childRuleCSSOMWrappers;

This would be more memory-efficient as OwnPtr<Vector<RefPtr<CSSRule>>> as CSSOM wrappers are rarely constructed.

> Source/WebCore/css/CSSStyleSheet.cpp:493
> +     for (unsigned i = 0; i < m_importRules.size(); ++i) {

Antti Coding Style!

> Source/WebCore/css/CSSStyleSheet.h:69
> +    virtual CSSStyleSheet* parentStyleSheet() const;

OVERRIDE.

> Source/WebCore/css/StyleRule.h:83
> +    signed m_sourceLine : 28;

We should make this unsigned at some point, the default value is 0, and there is no need for negative values AFAICT.

> Source/WebCore/css/WebKitCSSKeyframesRule.cpp:74
> +    for (unsigned i = 0; i < m_keyframes.size(); ++i) {

Antti Coding Style!

> Source/WebCore/css/WebKitCSSKeyframesRule.h:56
> +    String name() const { return m_name; }    
> +    void setName(const String& name) { m_name = AtomicString(name); }

These should probably pass AtomicString around rather than String.

> Source/WebCore/css/WebKitCSSRegionRule.cpp:49
> +    for (unsigned i = 0; i < m_childRuleCSSOMWrappers.size(); ++i) {

Antti Coding Style.

[Alexis Menard (darktears)]

Comment on attachment 135091 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=135091&action=review

> Source/WebCore/css/CSSStyleSheet.cpp:95
> +    , m_cssParserMode(ownerRule ? ownerRule->parentStyleSheet()->cssParserMode() : CSSStrictMode)

Is this always a safe call? StyleRuleImport constructor always take a stylesheet as a parameter but clearParentStyleSheet is called in various places. Is there a scenario where the importRule will not have a parentStyleSheet?

> Source/WebCore/css/CSSStyleSheet.cpp:175
> +            m_childRuleCSSOMWrappers[i]->setParentStyleSheet(0);

Seems like the indentation is wrong.

[Andreas Kling]

(In reply to comment #24)
> (From update of attachment 135091 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=135091&action=review
> 
> > Source/WebCore/css/CSSStyleSheet.cpp:95
> > +    , m_cssParserMode(ownerRule ? ownerRule->parentStyleSheet()->cssParserMode() : CSSStrictMode)
> 
> Is this always a safe call? StyleRuleImport constructor always take a stylesheet as a parameter but clearParentStyleSheet is called in various places. Is there a scenario where the importRule will not have a parentStyleSheet?

Good point. If you remove the @import rule from its containing style sheet before the call to CachedCSSStyleSheet::checkNotify() happens, you could possibly have an unparented @import rule. We should keep the null check that the old version of this code had.

[Antti Koivisto]

(In reply to comment #23)
> This would be more memory-efficient as OwnPtr<Vector<RefPtr<CSSRule>>> as CSSOM wrappers are rarely constructed.

The are bunch of improvements like that that can be done but better leave those for future patches.

[Antti Koivisto]

(In reply to comment #24)
> Is this always a safe call? StyleRuleImport constructor always take a stylesheet as a parameter but clearParentStyleSheet is called in various places. Is there a scenario where the importRule will not have a parentStyleSheet?

Added null check.

> > Source/WebCore/css/CSSStyleSheet.cpp:175
> > +            m_childRuleCSSOMWrappers[i]->setParentStyleSheet(0);
> 
> Seems like the indentation is wrong.

I don't see it.

[Alexis Menard (darktears)]

(In reply to comment #27)
> (In reply to comment #24)
> > Is this always a safe call? StyleRuleImport constructor always take a stylesheet as a parameter but clearParentStyleSheet is called in various places. Is there a scenario where the importRule will not have a parentStyleSheet?
> 
> Added null check.
> 
> > > Source/WebCore/css/CSSStyleSheet.cpp:175
> > > +            m_childRuleCSSOMWrappers[i]->setParentStyleSheet(0);
> > 
> > Seems like the indentation is wrong.
> 
> I don't see it.

Cool then maybe the review tool was confused.

[Antti Koivisto]

Created attachment 135105 [details]
updated patch

[WebKit Review Bot]

Attachment 135105 [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/css/CSSStyleSheet.cpp:127:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/css/CSSStyleSheet.cpp:233:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/css/CSSStyleSheet.cpp:303:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 3 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 135105 [details]
updated patch

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

New failing tests:
http/tests/inspector/modify-cross-domain-rule.html

[WebKit Review Bot]

Created attachment 135121 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Antti Koivisto]

Created attachment 135148 [details]
fix the crash in inspector tests

Use item()/length() for iterating collections instead of CSSRuleList from cssRules(). The latter is security checked for CSSStyleSheet and may return null.

[Antti Koivisto]

Created attachment 135152 [details]
rebased

[WebKit Review Bot]

Attachment 135152 [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/css/CSSStyleSheet.cpp:127:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/css/CSSStyleSheet.cpp:233:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/css/CSSStyleSheet.cpp:303:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 3 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antti Koivisto]

trac.webkit.org/changeset/112923

[Alexey Proskuryakov]

This has caused bug bug 89240.