[Webkit-unassigned] [Bug 132057] CSS JIT: Backtracking with current matching element support for Child fragment.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=132057





--- Comment #16 from Yusuke Suzuki <utatane.tea at gmail.com>  2014-04-25 02:23:39 PST ---
(From update of attachment 230071)
View in context: https://bugs.webkit.org/attachment.cgi?id=230071&action=review

Thank you for your review!
I'll upload a revised patch.

>> Source/WebCore/cssjit/SelectorCompiler.cpp:114
>> +static const unsigned InvalidHeight = std::numeric_limits<unsigned>::max();
> 
> We do not capitalize the first letter of constant in WebKit.

I've fixed.

>> Source/WebCore/cssjit/SelectorCompiler.cpp:120
>> +        , matchingOtherBacktrackingAction(BacktrackingAction::NoBacktracking)
> 
> maybe matchingOtherBacktrackingAction->matchingPostTagNameBacktrackingAction?
> To make it more explicit that tag name must go first.

Renaming to `matchingPostTagNameBacktrackingAction` looks nice.

>> Source/WebCore/cssjit/SelectorCompiler.cpp:177
>> +    void generateWalkToParentElementWithoutElementCheck(Assembler::RegisterID targetRegister);
> 
> Without the element check, that would simply be generateWalkToParentNode().

You're right. To align with C++ code `ContainerNode* parent = parentNode()`, `generateWalkToParentNode` is better.
I've renamed.

>> Source/WebCore/cssjit/SelectorCompiler.cpp:680
>> +enum TagNameEquality {
> 
> Let's use a typed enum with "enum class".

Wow! I don't know we can use C++11 enum class in WebKit.
I've changed to use "enum class".

>> Source/WebCore/cssjit/SelectorCompiler.cpp:702
>> +        // inverted lhs never matches on rhs.
> 
> This comment could be on a single line.
> 
> I believe this comment is useful but it should be in equalTagNamePatterns(). In equalTagNames(), we do not have the context of inverted/non-inverted.

Move this comment to `equalTagNamePatterns`.

>> Source/WebCore/cssjit/SelectorCompiler.cpp:731
>> +static inline unsigned computeBacktrackingStartHeightFromDescendant(const TagNameList& tagNames)
> 
> This function could have a comment explaining it finds the largest matching prefix.

Right. Added comment.

>> Source/WebCore/cssjit/SelectorCompiler.cpp:733
>> +    ASSERT(!tagNames.isEmpty());
> 
> You can use RELEASE_ASSERT here.
> 
> I try to be very aggressive with release assertion in this compiler to avoid any security issue. We can afford it because we compile very little compared to how many time the compiled code is executed.

Ah, I've got it. Changed to RELEASE_ASSERT.

>> Source/WebCore/cssjit/SelectorCompiler.cpp:735
>> +    unsigned size = tagNames.size();
> 
> size -> subsetSize?
> largestPrefixSize?

Changed to largestPrefixSize.

>> Source/WebCore/cssjit/SelectorCompiler.cpp:744
>> +        }
> 
> It would be nice to simplify this loop somehow.
> 
> Maybe rename amount to "offsetToLargestPrefix"?
> Put "tagNames.size() - 1" in a temporary "lastIndex"?
> 
> It would be nice to put "tagNames.size() - 1 - i" and "(tagNames.size() - 1 - i) - amount" in named temporary too. Not sure what would be a good name.

Changed. Introducing lastIndex, offsetToLargestPrefix and currentIndex.

In this loop, we check prefix matches to tagNames with the reverse order.

e.g.
TagNames [ p , div, div,  p ]
Index      3    2    1    0

And we create prefix from this, such as
Prefix   [ div, div, p ]
Index       2    1   0

And we need to check this prefix matches to TagNames with the reverse order (from index 3 to 1 in TagNames)
TagNames [ p , div, div,  p ]
T-Index    3    2    1    0
Prefix   [div, div,  p  ]
P-Index    2    1    0

In this code, currentIndex changes from 3 to 1, currentIndex - offsetToLargestPrefix changes from 2 to 0.

>> Source/WebCore/cssjit/SelectorCompiler.cpp:752
>> +{
> 
> I think this function could start with
> 
> if (!hasDescendantRelationOnTheRight)
>     return;
> 
> The descendant backtracking height is only relevant inside backtracking chains, and a chain can only start with a descendant.
> 
> 
> Maybe rename previousChildFragment to previousChildFragmentInBacktrackingChain or previousChildFragmentInDescendantBacktrackingChain?

Right! I've fixed this.

>> Source/WebCore/cssjit/SelectorCompiler.cpp:753
>> +    TagNamePattern pattern;
> 
> Let's make 2 local declaration in the block where it is used.

Yes. Moved it.

>> Source/WebCore/cssjit/SelectorCompiler.cpp:765
>> +            // tagName is not matched
> 
> WebKit comments should be sentences, you need the period at the end.

Changed comment contents.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.