[webkit-reviews] review denied: [Bug 135200] compile window-inactive and fullscreen pseudo classes : [Attachment 235358] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Alex Christensen
<achristensen at apple.com>'s request for review:
Bug 135200: compile window-inactive and fullscreen pseudo classes
https://bugs.webkit.org/show_bug.cgi?id=135200

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

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=235358&action=review


Awesome!

r- because I have serious doubt about PseudoClassAnyLink, the patch looks good
otherwise.

> Source/WebCore/css/SelectorChecker.cpp:484
> +	   } else if (selector->pseudoClassType() ==
CSSSelector::PseudoClassWindowInactive)
> +	       return isWindowInactive(element);

Can't you juste remove this code? This is handled properly by the switch()
below.

> Source/WebCore/cssjit/SelectorCompiler.cpp:409
> +	       return FunctionType::SimpleSelectorChecker;

Indentation issue here.

> Source/WebCore/cssjit/SelectorCompiler.cpp:415
> +	       return FunctionType::SimpleSelectorChecker;

ditto.

> Source/WebCore/cssjit/SelectorCompiler.cpp:-417
> -	   fragment.pseudoClasses.add(CSSSelector::PseudoClassLink);
> -	   return FunctionType::SimpleSelectorChecker;
> -

This does not look right. There is no handler for PseudoClassAnyLink, it is
aliased with the handler of PseudoClassLink.

> Source/WebCore/cssjit/SelectorCompiler.cpp:573
> +	   return FunctionType::CannotCompile;

I think CannotMatchAnything would make sense.