Bug 85598

Created attachment 140198 [details] patch

Attachment 140198 [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:183: 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: 1 in 23 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 140198 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=140198&action=review Attack angle looks sane. As you said yourself on IRC, this needs better test coverage though. > Source/WebCore/css/CSSStyleSheet.cpp:177 > +StyleRuleBase* StyleSheetInternal::ruleAt(unsigned index) const Perhaps this function could share some code with StyleSheetInternal::createChildRuleCSSOMWrapper().

Created attachment 140241 [details] updated patch - Use RAII instead of explicit willMutate/didMutate pairs - Remove StyleSheetInternal::createChildRuleCSSOMWrapper in favor of just using ruleAt() Some tests coming up.

Attachment 140241 [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:597: 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: 1 in 23 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 140242 [details] updated patch2

Attachment 140242 [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:597: 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: 1 in 23 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 140396 [details] now with tests

Attachment 140396 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1 Source/WebCore/css/CSSStyleSheet.cpp:599: 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: 1 in 27 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 140396 [details] now with tests View in context: https://bugs.webkit.org/attachment.cgi?id=140396&action=review It seems really easy to forget to do one of these “will mutate” or “did mutate” calls. Is there some way to make it easy to catch that kind of mistake when people change this code in the future? > Source/WebCore/css/CSSPageRule.cpp:68 > - Document* doc = 0; > - if (CSSStyleSheet* styleSheet = parentStyleSheet()) > - doc = styleSheet->ownerDocument(); > - if (!doc) > - return; > Left a stray blank line here. > Source/WebCore/css/CSSRule.h:27 > +#include "ExceptionCode.h" Why change to this instead of the forward-declaration typedef? It seems the code was already doing things the conventional way. > Source/WebCore/css/CSSStyleSheet.h:222 > + class RuleMutation { This object itself is not “a mutation” so I think we need a different name. Maybe RuleMutationGuard or RuleMutationScope. Also should use WTF_NONCOPYABLE on this class. Should the constructor and destructor be inlined? I wouldn’t want to without evidence that the code was performance critical, but the functions are short and just turn around and call one other function, which is what makes me think about that. What will happen if we forget to use one of these? Is there something we can do to make it assert if we forget to set up one of these guards around mutation? > Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp:160 > + didMutate(true); The “true” here is confusing. We normally avoid boolean constants in places like this in new code. > Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp:379 > + m_propertySet->deref(); > + m_propertySet = propertySet; > + m_propertySet->ref(); Why isn’t m_propertySet a RefPtr? > Source/WebCore/css/PropertySetCSSStyleDeclaration.h:105 > + virtual void didMutate(bool changes) OVERRIDE; I don’t understand what “changes” means here as the name of a boolean argument. It sounds like the name of an array, not a boolean. Maybe if it’s false it means there was actually no change? > LayoutTests/http/tests/css/shared-stylesheet-mutation.html:55 > + shouldBe("window.getComputedStyle(testElement, null).getPropertyValue('background-color')", No need for the “window.” here.

http://trac.webkit.org/changeset/116235 with the requested fixes and improved test coverage.

(In reply to comment #10) > (From update of attachment 140396 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=140396&action=review > > It seems really easy to forget to do one of these “will mutate” or “did mutate” calls. Is there some way to make it easy to catch that kind of mistake when people change this code in the future? If you don't invoke didMutateRules then the mutation will not be reflected in rendering. Basic testing should pick this up. In didMutateRules we ASSERT(m_internal->isMutable()) and mutable bit is only set by willMutateRules. This should help some. Generally we should avoid adding new mutating CSSOM APIs (by not having them into standards in the first place). > > Source/WebCore/css/CSSRule.h:27 > > +#include "ExceptionCode.h" > > Why change to this instead of the forward-declaration typedef? It seems the code was already doing things the conventional way. I had impression ExceptionCode.h was trivial (but it seems to be including some other header with a FIXME) and I didn't like having the type defined in multiple places (typedef is not exactly forward-declaration). If the idea is to always redefine ExceptionCode in the header, why is it defined in ExceptionCode.h at all? > > Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp:379 > > + m_propertySet->deref(); > > + m_propertySet = propertySet; > > + m_propertySet->ref(); > > Why isn’t m_propertySet a RefPtr? PropertySetCSSStyleDeclaration has two subclasses, one of which uses forwarding refcounting scheme. This can be fixed when InlineCSSStyleDeclaration is switched to conventional refcounting.

(In reply to comment #12) > If the idea is to always redefine ExceptionCode in the header, why is it defined in ExceptionCode.h at all? The design has been to use typedef int ExceptionCode in files that need only the type, and ExceptionCode.h in files that need constants for DOM exception codes. Analogous to using class Node when we need only the class, and Node.h when we need to call functions on an object. We have about a hundred different headers that do the "typedef int ExceptionCode" thing and about two hundred more that use ExceptionCode.h. I’m not married to it. We could change the design. But that’s how it’s been for a long time, and I don’t think we should start moving away from the typedef as we are making other changes to a header unless we make a conscious decision to do so across the project.

This causes a large number of tests crash on chromium debug bots due to an assertion. Sample flakiness dashboard: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=fast%2Fjs%2Fmozilla%2Feval%2Fexhaustive-fun-normalcaller-direct-strictcode.html%2Cfast%2Fjs%2Fmozilla%2Feval%2Fexhaustive-fun-normalcaller-indirect-strictcode.html%2Cfast%2Fjs%2Fmozilla%2Feval%2Fexhaustive-fun-strictcaller-direct-strictcode.html%2Cfast%2Fjs%2Fmozilla%2Feval%2Fexhaustive-fun-strictcaller-indirect-strictcode.html%2Cfast%2Fjs%2Fmozilla%2Feval%2Fexhaustive-global-normalcaller-direct-strictcode.html%2Cfast%2Fjs%2Fmozilla%2Feval%2Fexhaustive-global-normalcaller-indirect-strictcode.html%2Cfast%2Fjs%2Fmozilla%2Feval%2Fexhaustive-global-strictcaller-direct-strictcode.html%2Cfast%2Fjs%2Fmozilla%2Feval%2Fexhaustive-global-strictcaller-indirect-strictcode.html%2Cfast%2Fjs%2Fmozilla%2Fstrict%2F10.4.3.html%2Cfast%2Fjs%2Fmozilla%2Fstrict%2F11.1.5.html%2Cfast%2Fjs%2Fmozilla%2Fstrict%2F11.13.1.html%2Cfast%2Fjs%2Fmozilla%2Fstrict%2F11.3.2.html%2Cfast%2Fjs%2Fmozilla%2Fstrict%2F11.4.4.html%2Cfast%2Fjs%2Fmozilla%2Fstrict%2F12.14.1.html%2Cfast%2Fjs%2Fmozilla%2Fstrict%2F12.2.1.html%2Cfast%2Fjs%2Fmozilla%2Fstrict%2F15.3.4.5.html%2Cfast%2Fjs%2Fmozilla%2Fstrict%2F15.3.5.2.html%2Cfast%2Fjs%2Fmozilla%2Fstrict%2F15.4.4.8.html%2Cfast%2Fjs%2Fmozilla%2Fstrict%2F8.12.7.html%2Cfast%2Fjs%2Fmozilla%2Fstrict%2F8.7.2.html Assertion: STDERR: ASSERTION FAILED: root->m_clients.size() == 1 STDERR: /Volumes/data/b/build/slave/webkit-mac-latest-dbg/build/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../css/CSSStyleSheet.cpp(409) : WebCore::Node *WebCore::StyleSheetInternal::singleOwnerNode() const

Re-opened since this is blocked by 85773

http://trac.webkit.org/changeset/116281 should fix the assert when scoped stylesheets are enabled.

Apparently the rollout already silently landed.

Re-landed in http://trac.webkit.org/changeset/116291.