[webkit-reviews] review granted: [Bug 135293] CSS JIT: Implement :visited pseudo class : [Attachment 239709] 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 135293: CSS JIT: Implement :visited pseudo class
https://bugs.webkit.org/show_bug.cgi?id=135293

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

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


Looks good to me. Sorry I changed my mind so many time on the processing of
:visited. Thanks for keeping up with the updates.

It believe we should revisit our options after :matches() is implemented,
including multiple matching.

>> Source/WebCore/cssjit/SelectorCompiler.cpp:1042
>> +	// Allocate the stack references for the last visited element and the
start element.
>> +	if (visitedMode == VisitedMode::Visited)
>> +	    rootBacktrackingMemoryRequirement.stackCount += 2;
> 
> When the visited mode is Visited, we allocate 2 more stack references in the
root fragment.
> In the current simplified semantics, visited is only effective on the root
fragments.

I would put this branch outside computeBacktrackingMemoryRequirements().

Those two registers are unaffected by backtracking. Having the check in
generateSelectorChecker() would also put the branch in the same scope where the
stack references are assigned.

> Source/WebCore/cssjit/SelectorCompiler.cpp:1561
> +    if (m_visitedMode == VisitedMode::Visited) {
> +	   m_lastVisitedElement = m_backtrackingStack.takeLast();
> +	   m_startElement = m_backtrackingStack.takeLast();
> +	   unsigned offsetToStartElement =
m_stackAllocator.offsetToStackReference(m_startElement);
> +	   m_assembler.storePtr(elementAddressRegister,
Assembler::Address(Assembler::stackPointerRegister, offsetToStartElement));
> +	   unsigned offsetToLastVisitedElement =
m_stackAllocator.offsetToStackReference(m_lastVisitedElement);
> +	   m_assembler.storePtr(Assembler::TrustedImmPtr(nullptr),
Assembler::Address(Assembler::stackPointerRegister,
offsetToLastVisitedElement));
> +    }
> +

I would prefer this allocation to be more explicit.

First, allocate allocateUninitialized(). Then take two out of the vector for
:visited. The assign the remaining vector to m_backtrackingStack.

> LayoutTests/TestExpectations:221
> -fast/history/link-inside-not.html [ Failure ]
> +# fast/history/link-inside-not.html [ Failure ]

Let's remove those lines entirely.