[Webkit-unassigned] [Bug 70931] Matched declaration cache

[bugzilla-daemon at webkit.org]

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


Darin Adler <darin at apple.com> changed:

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




--- Comment #3 from Darin Adler <darin at apple.com>  2011-10-26 09:47:48 PST ---
(From update of attachment 112550)
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.

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