[webkit-reviews] review denied: [Bug 43783] Make CSS Style Selector non-recursive : [Attachment 64069] make-it-iterative

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Hayato Ito <hayato at chromium.org>'s
request for review:
Bug 43783: Make CSS Style Selector non-recursive
https://bugs.webkit.org/show_bug.cgi?id=43783

Attachment 64069: make-it-iterative
https://bugs.webkit.org/attachment.cgi?id=64069&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
This looks like a good idea. We need to measure if it makes things faster or
slower. Also, can we construct a test case demonstrating that the old algorithm
runs out of stack space if nested too deeply, or is that not practical?

> +class CSSStyleSelector::SelectorChecker::CallState {

If this is going to have public data members, lets have it be a struct and not
use the "m_" prefix. Or make the data members private.

> +    State m_state;
> +    CSSSelector* m_sel;

Lets spell it out: selector, not sel.

> +    Element* m_e;

Lets spell it out: element, not e.

> +    CallState(State state, CSSSelector* sel, Element* e, bool isSubSelector,
bool encounteredLink, RenderStyle* elementStyle, RenderStyle*
elementParentStyle)
> +	       : m_state(state)

Not sure why this is indented 8 spaces.

> +    void push(CallState state)
> +    {
> +	   m_stack.append(state);
> +    }

Cuts down on copying if this takes a const CallState&.

> +private:
> +    Vector<CallState> m_stack;

It may improve performance to use Vector<CallState, 20> here or some other
number that's usually larger than the typical depth of the stack.

Is there a way to make this patch have a smaller diff, by doing enough of this
to change the indentation level separately? All those lines of code marked
different that are not different make the patch too hard to read.

Normally, we do not use "const bool".

I'm going to say review- for this patch, but it looks like a step in the right
direction.