[webkit-reviews] review granted: [Bug 135733] CSS JIT: support :scope : [Attachment 236323] 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 135733: CSS JIT: support :scope
https://bugs.webkit.org/show_bug.cgi?id=135733

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

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


Brilliant work as usual. Some minor comments below.

>> Source/WebCore/cssjit/SelectorCompiler.cpp:1345
>> +	if (m_selectorContext != SelectorContext::QuerySelector &&
m_functionType == FunctionType::SelectorCheckerWithCheckingContext) {
> 
> Since it is clear that the pseudo element has no effect when the context is
QuerySelector, I wrote this guard here.
> Is it preferable to pushing this guard into
`generateRequestedPseudoElementEqualsToSelectorPseudoElement`?

IMHO, this code is clear with this check...

> Source/WebCore/cssjit/SelectorCompiler.cpp:1398
> +    if (m_selectorContext != SelectorContext::QuerySelector &&
m_functionType == FunctionType::SelectorCheckerWithCheckingContext) {

...and it is consistent with the counterpart here.

> Source/WebCore/cssjit/SelectorCompiler.cpp:1642
> +    ASSERT(m_functionType ==
FunctionType::SelectorCheckerWithCheckingContext);

A RELEASE_ASSERT() could be better here to guard any invalid access to
m_checkingContextStackReference.

> Source/WebCore/cssjit/SelectorCompiler.cpp:1827
> +    ASSERT(m_selectorContext != SelectorContext::QuerySelector);
> +

In WebKit we generally avoid ASSERT that repeat a early return.

What you can do is use ASSERT_WITH_MESSAGE to explain the early return.
E.g.: ASSERT_WITH_MESSAGE(m_selectorContext != SelectorContext::QuerySelector,
"We should never mark the tree from QuerySelector because that would cause
useless style invalidation.")

...or you can remove this assertion.

> Source/WebCore/cssjit/SelectorCompiler.cpp:1848
> +    ASSERT(m_selectorContext != SelectorContext::QuerySelector);
> +

ditto.

> Source/WebCore/cssjit/SelectorCompiler.cpp:2444
> +    ASSERT(m_selectorContext != SelectorContext::QuerySelector);

ditto.

> Source/WebCore/cssjit/SelectorCompiler.cpp:2446
> +    if (shouldUseRenderStyleFromCheckingContext(m_selectorContext,
fragment)) {

This actually looks quite a bit better with your early return.

> Source/WebCore/cssjit/SelectorCompiler.cpp:2479
> +    ASSERT(m_selectorContext != SelectorContext::QuerySelector);

Same comment as above regarding the assertion.

> Source/WebCore/cssjit/SelectorCompiler.cpp:2601
> +    ASSERT(m_selectorContext != SelectorContext::QuerySelector);

ditto.

> Source/WebCore/cssjit/SelectorCompiler.cpp:2673
> +    ASSERT(m_selectorContext != SelectorContext::QuerySelector);

ditto.

> Source/WebCore/cssjit/SelectorCompiler.cpp:2957
> +    ASSERT_WITH_MESSAGE(m_selectorContext != SelectorContext::QuerySelector,
"When the fragment has pseudo element, the selector becomres
CannotMatchAnything for QuerySelector and this test function is not called.");

Typo: "becomres"