[webkit-reviews] review granted: [Bug 43783] Make CSS Style Selector non-recursive : [Attachment 66201] test-included

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted 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 66201: test-included
https://bugs.webkit.org/attachment.cgi?id=66201&action=review

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

> WebCore/css/CSSStyleSelector.cpp:1922
> +    CallState(State state_, CSSSelector* selector_, Element* element_, bool
isSubSelector_, bool encounteredLink_, RenderStyle* elementStyle_, RenderStyle*
elementParentStyle_)
> +	   : state(state_)
> +	   , selector(selector_)
> +	   , element(element_)
> +	   , isSubSelector(isSubSelector_)
> +	   , encounteredLink(encounteredLink_)
> +	   , elementStyle(elementStyle_)
> +	   , elementParentStyle(elementParentStyle_)
> +    {
> +    }

There is no reason for all those trailing underscores. The C++ language works
fine with initializer arguments named the same as the data members, so there’s
no reason to have this strangeness.

> WebCore/css/CSSStyleSelector.cpp:1953
> +    // Therefore we have to maintain a call stack by ourselves so that we
can check selector iteratively.

This should say "check selectors iteratively", with an "s".

> WebCore/css/CSSStyleSelector.cpp:2064
> +	       default:
> +		   return false;

It’s unfortunate we have a default case, because including it turns off gcc’s
warning that is given when we forget to handle an enum value.

> WebCore/css/CSSStyleSelector.cpp:2070
> +	       while (true) {
> +		   if (callStack.isEmpty())
> +		       return false;

A more traditional way to write this is:

    while (!callStack.isEmpty()) {
	// loop body
    }
    return false;

That seems better than using an infinite loop.

> WebCore/css/CSSStyleSelector.cpp:2107
> +		   default:
> +		       ASSERT_NOT_REACHED();

It’s unfortunate we have a default case, because including it turns off gcc’s
warning that is given when we forget to handle an enum value.

> WebCore/css/CSSStyleSelector.h:234
> +	       struct CallState;
> +	       class CallStack;

I’m not sure these two need to be CSSStyleSelector members. I think it would be
better if they were private to the implementation file and not mentioned in the
header at all. We could give them slightly more specific names and do that. I
guess either way is OK.