[webkit-reviews] review granted: [Bug 77528] <style scoped>: Implement scoped selector matching in the slow path : [Attachment 126478] Patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted Roland Steiner
<rolandsteiner at chromium.org>'s request for review:
Bug 77528: <style scoped>: Implement scoped selector matching in the slow path
https://bugs.webkit.org/show_bug.cgi?id=77528

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

------- Additional Comments from Antti Koivisto <koivisto at iki.fi>
View in context: https://bugs.webkit.org/attachment.cgi?id=126478&action=review


r=me, but consider the comments

> Source/WebCore/css/CSSStyleSelector.cpp:176
> +    RuleData(CSSStyleRule*, CSSSelector*, unsigned position, bool
canUseFastChecking = true);

Something like canUseFastCheckSelector would be a better name here and
elsewhere (so it is clear what is being checked)

> Source/WebCore/css/CSSStyleSelector.h:283
> +#if ENABLE(STYLE_SCOPED)
> +	   MatchOptions(bool includeEmptyRules, const Element* scope = 0) :
scope(scope), includeEmptyRules(includeEmptyRules) { }
> +	   const Element* scope;
> +	   bool includeEmptyRules;
> +#else
> +	   MatchOptions(bool includeEmptyRules) :
includeEmptyRules(includeEmptyRules) { }
> +	   bool includeEmptyRules;
> +#endif

I think you can remove #if ENABLE(STYLE_SCOPED) here and just default the scope
to null.

> Source/WebCore/css/SelectorChecker.cpp:490
> -	   for (; nextContext.element; nextContext.element =
nextContext.element->parentElement()) {
> +	   for (;;) {
> +	       if (nextContext.element == nextContext.scope)
> +		   return SelectorFailsCompletely;
> +	       nextContext.element = nextContext.element->parentElement();
> +	       if (!nextContext.element)
> +		   return SelectorFailsCompletely;

You can keep the for loop as is if you do the following...

> Source/WebCore/css/SelectorChecker.cpp:498
>      case CSSSelector::Child:
> +	   if (nextContext.element == nextContext.scope)
> +	       return SelectorFailsCompletely;

This test (current element is the scope) is repeated for all cases except
SubSelector. I think it could be moved before the nextContext initialization
with a relation != SubSelector test.