[webkit-reviews] review granted: [Bug 135733] CSS JIT: support :scope : [Attachment 236252] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Aug 8 14:54:45 PDT 2014


Benjamin Poulain <benjamin at webkit.org> has granted Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 135733: CSS JIT: support :scope
https://bugs.webkit.org/show_bug.cgi?id=135733

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

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


Great patch. Some nitpick below:

> Source/WebCore/ChangeLog:17
> +	   * cssjit/SelectorCompiler.cpp:

addFlagsToElementStyleFromContext() should get an ASSERT(m_selectorContext ==
SelectorContext::QuerySelector).

Maybe shouldUseRenderStyleFromCheckingContext() could get one too. It is not
really incorrect to call that for QuerySelector but that is suspicious if that
happen. You would need to refactor some test functions.

> Source/WebCore/cssjit/SelectorCompiler.cpp:2971
> +    RELEASE_ASSERT(m_selectorContext == SelectorContext::QuerySelector);

This could be a regular assertion. Your code below handle the null case
correctly if that were to happen.

> Source/WebCore/cssjit/SelectorCompiler.cpp:2985
> +    Assembler::Jump successCase;
> +    {
> +	   LocalRegister checkingContext(m_registerAllocator);
> +	   loadCheckingContext(checkingContext);
> +
> +	   Assembler::Jump scopeIsNull =
m_assembler.branchTestPtr(Assembler::Zero, Assembler::Address(checkingContext,
OBJECT_OFFSETOF(CheckingContext, scope)));
> +	   successCase = m_assembler.branchPtr(Assembler::Equal,
Assembler::Address(checkingContext, OBJECT_OFFSETOF(CheckingContext, scope)),
elementAddressRegister);
> +	   failureCases.append(m_assembler.jump());
> +
> +	   scopeIsNull.link(&m_assembler);
> +    }
> +    generateElementIsRoot(failureCases);
> +    successCase.link(&m_assembler);

Alternative idea to write this with fewer jumps:

1) Reserve register "scope".
2) Load "checkingContext, OBJECT_OFFSETOF(CheckingContext, scope)" into
"scope".
3) Check for nullity, if null, jump to [5]
4) Load the root in scope with getDocument().
5) Compare scope and elementAddressRegister, fail if they are not equal.

Please put the update for review if you do this.

> Source/WebCore/cssjit/SelectorCompiler.cpp:3002
>      // When fragment doesn't have a pseudo element, there's no need to mark
the pseudo element style.
>      if (!fragment.pseudoElementSelector)
>	   return;

Ooops, loadCheckingContext() should have been after that if() :)

> LayoutTests/ChangeLog:13
> +	   * fast/selectors/querySelector-scope-filtered-root.html: Added.

Good idea to test that!


More information about the webkit-reviews mailing list