[webkit-reviews] review granted: [Bug 70931] Matched declaration cache : [Attachment 112550] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Antti Koivisto <koivisto at iki.fi>'s
request for review:
Bug 70931: Matched declaration cache
https://bugs.webkit.org/show_bug.cgi?id=70931

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=112550&action=review


> Source/WebCore/css/CSSProperty.cpp:384
> +    COMPILE_ASSERT(349 == numCSSProperties,
_when_adding_new_properties_add_the_inherited_here_and_bump_the_property_count)
;

Looks like this assertion is failing on various platforms. Maybe the number of
properties varies based on feature defines?

> Source/WebCore/css/CSSProperty.h:73
> +    signed m_id : 14;
> +    signed m_shorthandID : 14; // If this property was set as part of a
shorthand, gives the shorthand.

Is there a good way to do a runtime check to assert we have enough bits here?
Does this really need to be signed rather than unsigned?

> Source/WebCore/css/CSSStyleSelector.cpp:2153
> +    unsigned hash = 0;
> +    for (unsigned i = 0; i < size; ++i) {
> +	   unsigned ptrHash =
PtrHash<CSSMutableStyleDeclaration*>::hash(declarations[i].styleDeclaration);
> +	   ptrHash ^= IntHash<unsigned>::hash(declarations[i].linkMatchType);
> +	   // Make the position matter.
> +	   hash ^= (ptrHash << i) | (ptrHash >> (32 - i));
> +    }
> +    return hash;

I think a better way to compute this hash is to just compute a hash across each
bit of data using the algorithm like StringHasher::hashMemory. Combining
multiple hashes into a new one is typically not as good as that function. I
believe that would be both faster and a better algorithm than this. The
function that is needed to make StringHasher usable for this would be
StringHasher::addMemory that would let you add things other than characters to
a hash. Should be really easy to write.

> Source/WebCore/css/CSSStyleSelector.cpp:2186
> +	   if (!(m_matchedDecls[i] == cacheItem.matchedStyleDeclarations[i]))

I think this would be easier to read with (a != b) rather than (!(a == b)).

> Source/WebCore/css/CSSStyleSelector.cpp:2189
> +    if (!(cacheItem.matchResult == matchResult))

Ditto.

> Source/WebCore/css/CSSStyleSelector.cpp:2264
> +    if (m_style->unique() || m_style->hasAppearance() ||
(m_style->styleType() != NOPSEUDO && m_parentStyle->unique()) 
> +	   || m_style->zoom() != RenderStyle::initialZoom())
> +	   return;

You could break this up into two or more return statements to avoid having the
awkwardly broken long line.

> Source/WebCore/css/CSSStyleSelector.h:230
> +	   bool operator==(const MatchResult&) const;

Usually as a matter of default C++ style operator== should be a free function,
not a member function. In cases where type conversions are possible the free
function allows type conversions on the left side. But I guess that might be a
little tricky to do inside a class definition.

We should add an operator != that just calls operator== and does the ! part.

> Source/WebCore/css/CSSStyleSelector.h:333
> +	   bool operator==(const MatchedStyleDeclaration&) const;

Same as above.