[webkit-reviews] review granted: [Bug 85598] Share stylesheet data structures between documents : [Attachment 140396] now with tests

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Antti Koivisto <koivisto at iki.fi>'s
request for review:
Bug 85598: Share stylesheet data structures between documents
https://bugs.webkit.org/show_bug.cgi?id=85598

Attachment 140396: now with tests
https://bugs.webkit.org/attachment.cgi?id=140396&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.