[Webkit-unassigned] [Bug 85598] Share stylesheet data structures between documents

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=85598


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #140396|review?                     |review+
               Flag|                            |




--- Comment #10 from Darin Adler <darin at apple.com>  2012-05-05 17:20:05 PST ---
(From update of attachment 140396)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.