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

[bugzilla-daemon at webkit.org]

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





--- Comment #9 from Antti Koivisto <koivisto at iki.fi>  2011-10-27 00:47:06 PST ---
(In reply to comment #3)
> (From update of attachment 112550 [details])
> 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?

As Hyatt suggested, I just added all properties to the switch instead (with some feature ifdefs).

> > 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?

I don't know any good way to do that. Switched to unsigned.

> 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.

True. Will leave this as a future excercise.

> > 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)).

Done.

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

Done.

> > 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.

Made these free functions, just had to make them friends too.

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

Done.

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