Bug 85598 - Share stylesheet data structures between documents
Summary: Share stylesheet data structures between documents
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: 85773
Blocks: 77745
  Show dependency treegraph
 
Reported: 2012-05-04 03:49 PDT by Antti Koivisto
Modified: 2012-05-07 02:56 PDT (History)
11 users (show)

See Also:


Attachments
patch (38.76 KB, patch)
2012-05-04 05:42 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
updated patch (41.73 KB, patch)
2012-05-04 08:28 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
updated patch2 (41.74 KB, patch)
2012-05-04 08:36 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
now with tests (55.43 KB, patch)
2012-05-05 06:07 PDT, Antti Koivisto
darin: 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-05-04 03:49:26 PDT
We currently make a copy of the data structures when restoring a cached stylesheet. We could share the data until someone uses CSSOM to modify to sheet.
Comment 1 Antti Koivisto 2012-05-04 05:42:54 PDT
Created attachment 140198 [details]
patch
Comment 2 WebKit Review Bot 2012-05-04 05:46:42 PDT
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 3 Andreas Kling 2012-05-04 06:17:54 PDT
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().
Comment 4 Antti Koivisto 2012-05-04 08:28:24 PDT
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.
Comment 5 WebKit Review Bot 2012-05-04 08:32:56 PDT
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.
Comment 6 Antti Koivisto 2012-05-04 08:36:21 PDT
Created attachment 140242 [details]
updated patch2
Comment 7 WebKit Review Bot 2012-05-04 08:38:44 PDT
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.
Comment 8 Antti Koivisto 2012-05-05 06:07:51 PDT
Created attachment 140396 [details]
now with tests
Comment 9 WebKit Review Bot 2012-05-05 06:09:54 PDT
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 10 Darin Adler 2012-05-05 17:20:05 PDT
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.
Comment 11 Antti Koivisto 2012-05-06 03:29:29 PDT
http://trac.webkit.org/changeset/116235

with the requested fixes and improved test coverage.
Comment 12 Antti Koivisto 2012-05-06 05:54:52 PDT
(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.
Comment 13 Darin Adler 2012-05-06 16:53:37 PDT
(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.
Comment 14 Andrey Kosyakov 2012-05-06 22:07:31 PDT
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
Comment 15 WebKit Review Bot 2012-05-06 22:39:42 PDT
Re-opened since this is blocked by 85773
Comment 16 Antti Koivisto 2012-05-07 01:14:48 PDT
http://trac.webkit.org/changeset/116281 should fix the assert when scoped stylesheets are enabled.
Comment 17 Antti Koivisto 2012-05-07 01:47:30 PDT
Apparently the rollout already silently landed.
Comment 18 Antti Koivisto 2012-05-07 02:56:21 PDT
Re-landed in http://trac.webkit.org/changeset/116291.