[webkit-reviews] review denied: [Bug 135293] CSS JIT: Implement :visited pseudo class : [Attachment 238719] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 135293: CSS JIT: Implement :visited pseudo class
https://bugs.webkit.org/show_bug.cgi?id=135293

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

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


The part about :visited in top level selectors look good to me.

There is code related to :not() on which I would like more information:
    Wouldn't it be possible to handle :not(:link) with the generic path by
adding discarding the :not() and adding :visited to the fragment's pseudo
classes?

I r- to do a second round with your explanations for :not().

Your changes to StackAllocator looks correct. Feel free to land that separately
as reviewed by me.

> Source/WebCore/ChangeLog:8
> +	   This patch implements CSS JIT for :visited. And maake :not(:link)
JIT-ed.

Typo: maake

> Source/WebCore/ChangeLog:23
> +	   Edge cases are tested by the existing tests.
> +	   :not(:link)
> +	       fast/history/link-inside-not.html
> +	   :not(:visited)
> +	       fast/history/visited-inside-not.html
> +	   :-webkit-any(:link)
> +	       fast/history/link-inside-any.html
> +	   :-webkit-any(:visited)
> +	       fast/history/visited-inside-any.html

Mentioning this in the changelog is absolutely awesome, I wish more authors did
that.

> Source/WebCore/cssjit/SelectorCompiler.cpp:616
> +	       // And when :visited is inside :not (:not(:visited)), it returns
CannotMatchAnything and it always passes this filter.

This comment is not necessary, the explanation in PseudoClassVisited is enough.


> Source/WebCore/cssjit/SelectorCompiler.cpp:630
> +		   ASSERT_WITH_MESSAGE(selectorContext !=
SelectorContext::QuerySelector, "When visitedMatchEnabled is true, selector
context is never QuerySelector.");

The message repeats the condition IMHO. Maybe: "QuerySelector should never
match :visited link because it would be a privacy issue." or something like
that?

> Source/WebCore/cssjit/SelectorCompiler.cpp:723
> +    bool visitedMatchEnabled = true;
> +    if (selectorContext == SelectorContext::QuerySelector)
> +	   visitedMatchEnabled = false;

Could be:
bool visitedMatchEnabled = selectorContext != SelectorContext::QuerySelector;

> Source/WebCore/cssjit/SelectorCompiler.cpp:1431
> +    // Remember the stack base of the temporary variables.

This comment is superfluous, the code below is clear.

> Source/WebCore/cssjit/SelectorCompiler.cpp:1460
> +    // Invalidate temporaryStackBase if there's no temporary variables.
> +    if (temporaryStackBase == m_stackAllocator.stackTop())
> +	   temporaryStackBase = StackAllocator::StackReference();

Instead of this, couldn't we check "temporaryStackBase ==
m_stackAllocator.stackTop()" instead of temporaryStackBase.isValid() when
discarding the stack?

> Source/WebCore/cssjit/SelectorCompiler.cpp:1558
> +void SelectorCodeGenerator::generateRightmostTreeWalker(Assembler::JumpList&
failureCases, const SelectorFragment& fragment)
> +{
> +    generateElementMatching(failureCases, failureCases, fragment);
> +}
> +

Why do you need this?