[webkit-reviews] review granted: [Bug 45507] Improve the local{SharedStyle, CousinList} algorithm : [Attachment 67282] Proposed fix: Use a budgeted algorithm that simplifies the code and is a slightly better trade-off (without the style issues)

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Julien Chaffraix
<jchaffraix at webkit.org>'s request for review:
Bug 45507: Improve the local{SharedStyle,CousinList} algorithm
https://bugs.webkit.org/show_bug.cgi?id=45507

Attachment 67282: Proposed fix: Use a budgeted algorithm that simplifies the
code and is a slightly better trade-off (without the style issues)
https://bugs.webkit.org/attachment.cgi?id=67282&action=review

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

> WebCore/css/CSSStyleSelector.cpp:949
> +Node* CSSStyleSelector::locateCousinList(Element* parent, unsigned
&visitedNodesCount)

The "&" goes next to the type rather than next to the variable name.

> WebCore/css/CSSStyleSelector.cpp:951
> +    if (visitedNodesCount >= (cStyleSearchThreshold *
cStyleSearchLevelThreshold))

The parentheses here are not helpful. Please remove them.

> WebCore/css/CSSStyleSelector.cpp:1076
> +    unsigned visitedNodesCount = 0;

The phrase “visited nodes count” is not good grammar, but “visited node count”
would be.

> WebCore/css/CSSStyleSelector.cpp:1079
> +    // FIXME: We should use the ElementTraversal API instead of this
for-loop but we need to test the performance implication of this change.

I’m glad that you would test performance before making the change, but I don’t
agree with the FIXME. Just because element traversal is a public DOM API
doesn’t mean it’s a good idea to use it inside the engine. With both
previousSibling and isElementNode implemented as efficient inline functions,
it’s almost certainly better to call code that can do this inline. If you’d
like to come back here and deploy some kind of fast implementation of element
traversal, that’s OK, but I do not think it deserves a FIXME comment.

> WebCore/css/CSSStyleSelector.h:113
> +	   Node* locateCousinList(Element* parent, unsigned &visitedNodes);

Please move the & next to the type.

The name visitedNodes is not good. This variable does not hold nodes, so it
should not be named “visited nodes”. I think visitedNodeCount is an acceptable
name.