[webkit-reviews] review denied: [Bug 87078] Make CSSParser::filteredProperties() O(n) instead of O(n^2) and improve readability. : [Attachment 144251] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Luke Macpherson
<macpherson at chromium.org>'s request for review:
Bug 87078: Make CSSParser::filteredProperties() O(n) instead of O(n^2) and
improve readability.
https://bugs.webkit.org/show_bug.cgi?id=87078

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

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


review- because of the VectorTraits mistake; the other comments are optional
ideas for improvement

> Source/WebCore/ChangeLog:13
> +	   3) Remove O(n^2) behavior caused by scanning for and removing
previously encountered definitions of each property.
> +	      This was quite bad because we would remove early in the vector
causing a move of all subsequent entries, then
> +	      add at the end to keep the order correct.

Given that we’re removing this, we should create a regression test to
demonstrate it, perhaps with Ojan’s performance regression test system (in
LayoutTests/perf). If not, the change log should say why not.

> Source/WebCore/css/CSSParser.cpp:1185
> +    size_t unused = m_parsedProperties.size();

Typically it’s better to have variable names be noun phrases rather than
adjective phrases. So we need a noun to go after “unused”. Maybe unusedCount is
a good choice.

> Source/WebCore/css/CSSParser.cpp:1186
> +    results.resize(unused);

It’s slightly more efficient to initialize the vector with this size rather
than calling resize explicitly. Easy to do by just passing this number to the
vector constructor.

> Source/WebCore/css/CSSParser.cpp:1189
> +    for (int important = 1; important >= 0; --important) {

While this will work just fine, I think it’s unconventional and so not all that
readable.

A more readable alternative would be to put the inner loop in an inline
function and call it twice, once for important and once for non-important
properties.

> Source/WebCore/css/CSSParser.cpp:1203
> +    if (unused)
> +	   results.remove(0, unused);

It’s probably OK to leave out the if statement; it’s not needed for correctness
since remove(0, 0) will do nothing, so we should only keep it if we think it’s
a significant optimization.

> Source/WebCore/css/CSSProperty.h:76
> +namespace WTF {
> +template <> struct VectorTraits<WebCore::CSSProperty> :
SimpleClassVectorTraits { };
> +}

This is not quite correct. It will set canCompareWithMemcmp to true, and that
should be false. Probably the best way to do this would be to inherit from
VectorTraitsBase<false, WebCore::CSSProperty> and set canInitializeWithMemset
and canMoveWithMemcpy to true. Inheriting from SimpleClassVectorTraits and
setting canCompareWithMemcmp to false would be OK for now, but is less
future-proof we add additional traits.