Bug 75948

Summary: Matched declaration cache should support mapped attribute declarations.
Product: WebKit Reporter: Andreas Kling <kling>
Component: CSSAssignee: Andreas Kling <kling>
Status: RESOLVED FIXED    
Severity: Normal CC: macpherson, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch koivisto: review+

Description Andreas Kling 2012-01-10 02:21:11 PST
CSSStyleSelector's matched decl cache currently doesn't support mapped attribute declarations due to lifetime differences.
Comment 1 Andreas Kling 2012-01-10 02:47:20 PST
Created attachment 121817 [details]
Patch
Comment 2 Antti Koivisto 2012-01-10 03:06:29 PST
Comment on attachment 121817 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121817&action=review

r=me, nice!

> Source/WebCore/css/CSSStyleSelector.cpp:139
> +static unsigned gCacheAdditionsBetweenSweeps = 100;

A more complete name would be better. And drop the g. (matchedDeclarationCacheAdditionsBetweenSweeps)

> Source/WebCore/css/CSSStyleSelector.cpp:335
> +    , m_cacheAdditionsSinceLastSweep(0)

Use more complete name here too. It is not clear which cache is being referred.

> Source/WebCore/css/CSSStyleSelector.cpp:493
> +    // FIXME: This could be improved/replaced by some kind of invalidation mechanism.
> +    // Look for cache entries containing a style declaration with a single ref and remove them.
> +    // This may happen when an element attribute mutation causes it to swap out its Attribute::decl()
> +    // for another CSSMappedAttributeDeclaration, potentially leaving this cache with the last ref.

FIXME could be dropped as it is not clear if that would be any better (the alternative strategy could still be mentioned)

> Source/WebCore/css/CSSStyleSelector.cpp:504
> +    for (; it != end; ++it) {
> +        for (size_t i = 0; i < it->second.matchedStyleDeclarations.size(); ++i) {
> +            if (it->second.matchedStyleDeclarations[i].styleDeclaration->hasOneRef()) {
> +                toRemove.append(it->first);
> +                break;
> +            }
> +        }
> +    }

This might be less confusing if you assigned it->first and it->second to variables.

> Source/WebCore/css/CSSStyleSelector.h:348
> -    struct MatchedStyleDeclaration {
> +    class MatchedStyleDeclaration {

Better keep it struct since it has all public members.

> Source/WebCore/css/CSSStyleSelector.h:349
> +    public:

No need for public with struct.

> Source/WebCore/css/CSSStyleSelector.h:372
> +    unsigned m_cacheAdditionsSinceLastSweep;

Maybe move this field next to the cache field.
Comment 3 Andreas Kling 2012-01-10 03:35:44 PST
Committed r104550: <http://trac.webkit.org/changeset/104550>