[webkit-reviews] review granted: [Bug 133314] CSS JIT: add support for the "not" pseudo class : [Attachment 232343] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has granted Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 133314: CSS JIT: add support for the "not" pseudo class
https://bugs.webkit.org/show_bug.cgi?id=133314

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

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


Great patch

> Source/WebCore/cssjit/SelectorCompiler.cpp:158
>      Vector<std::pair<int, int>> nthChildfilters;

Ooops,I forgot to capitalize Filters here :|

> Source/WebCore/cssjit/SelectorCompiler.cpp:233
> +    void generateElementIsNotMatchedToNegation(Assembler::JumpList&
failureCases, const SelectorFragment&);

I realize the name "generateElementIsNotMatchedToNegation" is confusing. What
do you think of generateElementMatchesNotPseudoClass?

> Source/WebCore/cssjit/SelectorCompiler.cpp:1659
> -    generateElementAttributesMatching(failureCases, elementDataAddress,
fragment);
> +	   generateElementAttributesMatching(failureCases, elementDataAddress,
fragment);

Oops.

> Source/WebCore/cssjit/SelectorCompiler.cpp:2560
> +    Assembler::JumpList localFailureCases;

This could be moved in the for() loop.

> LayoutTests/ChangeLog:23
> +	   * fast/selectors/not-active-hover-quirks-expected.txt: Added.
> +	   * fast/selectors/not-active-hover-quirks.html: Added.
> +	   * fast/selectors/not-active-hover-strict-expected.txt: Added.
> +	   * fast/selectors/not-active-hover-strict.html: Added.
> +	   * fast/selectors/pseudo-class-not-expected.txt: Added.
> +	   * fast/selectors/pseudo-class-not.html: Added.
> +	   * fast/selectors/resources/not-hover-active-quirks-utility.js:
Added.
> +	   (testQuerySelector):
> +	   (test):
> +	   * fast/selectors/resources/not-hover-active-strict-utility.js:
Added.
> +	   (testQuerySelector):
> +	   (test):

Wow, that's a lot of tests :)

> LayoutTests/fast/selectors/not-active-hover-quirks.html:15
> +description('Test the :not(:active) and :not(:hover) selector when the
document is in quirks mode. To test manually, ensure that not move the cursor
over the green rectangle and press the gray START div until the test is
finished.');

Typo here: "ensure that not move".