Bug 75948 - Matched declaration cache should support mapped attribute declarations.
Summary: Matched declaration cache should support mapped attribute declarations.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-10 02:21 PST by Andreas Kling
Modified: 2012-01-10 03:35 PST (History)
2 users (show)

See Also:


Attachments
Patch (8.03 KB, patch)
2012-01-10 02:47 PST, Andreas Kling
koivisto: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>