[webkit-reviews] review granted: [Bug 75948] Matched declaration cache should support mapped attribute declarations. : [Attachment 121817] Patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted Andreas Kling <kling at webkit.org>'s
request for review:
Bug 75948: Matched declaration cache should support mapped attribute
declarations.
https://bugs.webkit.org/show_bug.cgi?id=75948

Attachment 121817: Patch
https://bugs.webkit.org/attachment.cgi?id=121817&action=review

------- Additional Comments from Antti Koivisto <koivisto at iki.fi>
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.