Bug 101180 – [Shadow] ElementShadow should have RuleFeatureSet for select attribute selectors.

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Shinya Kawanaka]

This is a preparation for https://bugs.webkit.org/show_bug.cgi?id=100451

When attribute is changed, we would like to consult with FeatureRuleSet whether we have to invalidate distribution or not.

[Shinya Kawanaka]

Created attachment 172753 [details]
WIP

[Build Bot]

Comment on attachment 172753 [details]
WIP

Attachment 172753 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14763153

[Build Bot]

Comment on attachment 172753 [details]
WIP

Attachment 172753 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14757360

[Dimitri Glazkov (Google)]

Comment on attachment 172753 [details]
WIP

Really excited about this direction. Being smart about when to redistribute is awesome.

[Shinya Kawanaka]

Created attachment 172957 [details]
WIP

[Shinya Kawanaka]

This is almost reviewable, though it contains a patch for Bug 101543.

[Shinya Kawanaka]

Created attachment 172961 [details]
WIP

[Build Bot]

Comment on attachment 172961 [details]
WIP

Attachment 172961 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14763482

[Build Bot]

Comment on attachment 172961 [details]
WIP

Attachment 172961 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14771088

[Shinya Kawanaka]

Created attachment 172967 [details]
WIP

[Build Bot]

Comment on attachment 172967 [details]
WIP

Attachment 172967 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14757769

[Dimitri Glazkov (Google)]

Comment on attachment 172967 [details]
WIP

This looks great! May I suggest 3 patches?
1) refactor RuleFeature/RuleSet to expose collectFeaturesFromSelector
2) refactor HTMLContentElement to move in selector validation
3) everything else.

[Shinya Kawanaka]

Created attachment 173185 [details]
WIP

[Shinya Kawanaka]

(In reply to comment #12)
> (From update of attachment 172967 [details])
> This looks great! May I suggest 3 patches?
> 1) refactor RuleFeature/RuleSet to expose collectFeaturesFromSelector
> 2) refactor HTMLContentElement to move in selector validation
> 3) everything else.

Yes.
(1) will be done in Bug 101692
(2) has been done in Bug 100738
(3) is here!

The previous WIP patch contains (1) and (3).

[Shinya Kawanaka]

not Bug 100738 but Bug 101543.

[Shinya Kawanaka]

Created attachment 173213 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 173213 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173213&action=review

> Source/WebCore/dom/ElementShadow.cpp:237
> +        for (Node* node = root->firstChild(); node; node = node->traverseNextNode()) {

Should this not only collect from children, not all descendants?

> Source/WebCore/dom/ElementShadow.cpp:246
> +        for (Node* node = root->firstChild(); node; node = node->traverseNextNode()) {

Ditto.

[Shinya Kawanaka]

Comment on attachment 173213 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173213&action=review

>> Source/WebCore/dom/ElementShadow.cpp:237
>> +        for (Node* node = root->firstChild(); node; node = node->traverseNextNode()) {
> 
> Should this not only collect from children, not all descendants?

It should collect all descendants, right? 
I'm using traverseNextNode() instead of nextSibling()...

[Shinya Kawanaka]

(In reply to comment #18)
> (From update of attachment 173213 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=173213&action=review
> 
> >> Source/WebCore/dom/ElementShadow.cpp:237
> >> +        for (Node* node = root->firstChild(); node; node = node->traverseNextNode()) {
> > 
> > Should this not only collect from children, not all descendants?
> 
> It should collect all descendants, right? 
> I'm using traverseNextNode() instead of nextSibling()...

Ah, I see.. We don't need collect all descendants. Only direct children are required.

[Shinya Kawanaka]

Comment on attachment 173213 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173213&action=review

>>>> Source/WebCore/dom/ElementShadow.cpp:237
>>>> +        for (Node* node = root->firstChild(); node; node = node->traverseNextNode()) {
>>> 
>>> Should this not only collect from children, not all descendants?
>> 
>> It should collect all descendants, right? 
>> I'm using traverseNextNode() instead of nextSibling()...
> 
> Ah, I see.. We don't need collect all descendants. Only direct children are required.

Oops. This is not correct. We should collect all elements having ElementShadow. So this code should be right...

[Shinya Kawanaka]

Example:

When className of div[class="foo"] is changed, we have to invalidate distribution.

div -------------------------- ShadowRoot
 | - div[class="foo"]              |- div ------------------------------ ShadowRoot
                                   |- div
                                       |- div ----- ShadowRoot
                                       |- shadow        |-content[select=".foo"]

[WebKit Review Bot]

Comment on attachment 173213 [details]
Patch

Clearing flags on attachment: 173213

Committed r134184: <http://trac.webkit.org/changeset/134184>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.