Bug 82728 - Split remaining CSSRules into internal and CSSOM types
Summary: Split remaining CSSRules into internal and CSSOM types
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-03-30 06:27 PDT by Antti Koivisto
Modified: 2012-06-18 14:50 PDT (History)
8 users (show)

See Also:


Attachments
for bots (108.82 KB, patch)
2012-03-30 06:30 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
rebased (108.94 KB, patch)
2012-03-30 07:40 PDT, Antti Koivisto
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
try to fix build (111.52 KB, patch)
2012-03-30 10:02 PDT, Antti Koivisto
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
another one (109.71 KB, patch)
2012-03-30 11:58 PDT, Antti Koivisto
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (275.26 KB, application/zip)
2012-03-30 13:55 PDT, WebKit Review Bot
no flags Details
patch (122.52 KB, patch)
2012-04-02 07:04 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
updated patch (122.25 KB, patch)
2012-04-02 08:24 PDT, Antti Koivisto
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (7.18 MB, application/zip)
2012-04-02 09:43 PDT, WebKit Review Bot
no flags Details
fix the crash in inspector tests (124.69 KB, patch)
2012-04-02 12:03 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
rebased (124.67 KB, patch)
2012-04-02 12:21 PDT, 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-03-30 06:27:19 PDT
Complete the move to internal CSS rule types.
Comment 1 Antti Koivisto 2012-03-30 06:30:54 PDT
Created attachment 134800 [details]
for bots
Comment 2 Antti Koivisto 2012-03-30 07:40:24 PDT
Created attachment 134814 [details]
rebased
Comment 3 WebKit Review Bot 2012-03-30 07:42:43 PDT
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.
Comment 4 Early Warning System Bot 2012-03-30 07:51:00 PDT
Comment on attachment 134814 [details]
rebased

Attachment 134814 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12253301
Comment 5 Build Bot 2012-03-30 07:52:18 PDT
Comment on attachment 134814 [details]
rebased

Attachment 134814 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12267256
Comment 6 Early Warning System Bot 2012-03-30 07:52:51 PDT
Comment on attachment 134814 [details]
rebased

Attachment 134814 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12266323
Comment 7 Gyuyoung Kim 2012-03-30 08:10:10 PDT
Comment on attachment 134814 [details]
rebased

Attachment 134814 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12253312
Comment 8 Andreas Kling 2012-03-30 08:21:48 PDT
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.
Comment 9 WebKit Review Bot 2012-03-30 08:41:17 PDT
Comment on attachment 134814 [details]
rebased

Attachment 134814 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12247323
Comment 10 Build Bot 2012-03-30 09:04:31 PDT
Comment on attachment 134814 [details]
rebased

Attachment 134814 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12288273
Comment 11 Gustavo Noronha (kov) 2012-03-30 09:10:13 PDT
Comment on attachment 134814 [details]
rebased

Attachment 134814 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12266336
Comment 12 Antti Koivisto 2012-03-30 10:02:58 PDT
Created attachment 134833 [details]
try to fix build
Comment 13 WebKit Review Bot 2012-03-30 10:06:09 PDT
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.
Comment 14 WebKit Review Bot 2012-03-30 10:41:53 PDT
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
Comment 15 Antti Koivisto 2012-03-30 11:58:29 PDT
Created attachment 134853 [details]
another one
Comment 16 WebKit Review Bot 2012-03-30 12:01:33 PDT
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.
Comment 17 WebKit Review Bot 2012-03-30 13:55:46 PDT
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
Comment 18 WebKit Review Bot 2012-03-30 13:55:50 PDT
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
Comment 19 Antti Koivisto 2012-04-02 07:04:01 PDT
Created attachment 135091 [details]
patch
Comment 20 WebKit Review Bot 2012-04-02 07:07:42 PDT
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.
Comment 21 Alexis Menard (darktears) 2012-04-02 07:13:02 PDT
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.
Comment 22 Alexis Menard (darktears) 2012-04-02 07:37:12 PDT
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?
Comment 23 Andreas Kling 2012-04-02 07:48:36 PDT
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.
Comment 24 Alexis Menard (darktears) 2012-04-02 07:54:40 PDT
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.
Comment 25 Andreas Kling 2012-04-02 08:09:10 PDT
(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.
Comment 26 Antti Koivisto 2012-04-02 08:19:17 PDT
(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.
Comment 27 Antti Koivisto 2012-04-02 08:19:58 PDT
(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.
Comment 28 Alexis Menard (darktears) 2012-04-02 08:23:01 PDT
(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.
Comment 29 Antti Koivisto 2012-04-02 08:24:58 PDT
Created attachment 135105 [details]
updated patch
Comment 30 WebKit Review Bot 2012-04-02 08:28:51 PDT
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.
Comment 31 WebKit Review Bot 2012-04-02 09:43:43 PDT
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
Comment 32 WebKit Review Bot 2012-04-02 09:43:50 PDT
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
Comment 33 Antti Koivisto 2012-04-02 12:03:23 PDT
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.
Comment 34 Antti Koivisto 2012-04-02 12:21:39 PDT
Created attachment 135152 [details]
rebased
Comment 35 WebKit Review Bot 2012-04-02 12:25:43 PDT
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.
Comment 36 Antti Koivisto 2012-04-02 12:51:50 PDT
trac.webkit.org/changeset/112923
Comment 37 Alexey Proskuryakov 2012-06-18 14:50:37 PDT
This has caused bug bug 89240.