| Summary: | Split remaining CSSRules into internal and CSSOM types | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Antti Koivisto <koivisto> | ||||||||||||||||||||||
| Component: | CSS | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||
| Severity: | Normal | CC: | darin, dglazkov, gustavo, kling, macpherson, menard, webkit.review.bot, xan.lopez | ||||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||||||||
| Bug Blocks: | 77745 | ||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||
|
Description
Antti Koivisto
2012-03-30 06:27:19 PDT
Created attachment 134800 [details]
for bots
Created attachment 134814 [details]
rebased
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 on attachment 134814 [details] rebased Attachment 134814 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12253301 Comment on attachment 134814 [details] rebased Attachment 134814 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12267256 Comment on attachment 134814 [details] rebased Attachment 134814 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12266323 Comment on attachment 134814 [details] rebased Attachment 134814 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/12253312 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 on attachment 134814 [details] rebased Attachment 134814 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12247323 Comment on attachment 134814 [details] rebased Attachment 134814 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12288273 Comment on attachment 134814 [details] rebased Attachment 134814 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12266336 Created attachment 134833 [details]
try to fix build
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 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 Created attachment 134853 [details]
another one
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 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 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
Created attachment 135091 [details]
patch
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 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 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 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 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. (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. (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. (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. (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. Created attachment 135105 [details]
updated patch
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 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 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
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.
Created attachment 135152 [details]
rebased
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.
trac.webkit.org/changeset/112923 This has caused bug bug 89240. |