Bug 101180 - [Shadow] ElementShadow should have RuleFeatureSet for select attribute selectors.
Summary: [Shadow] ElementShadow should have RuleFeatureSet for select attribute select...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Shinya Kawanaka
URL:
Keywords:
Depends on: 101543 101692
Blocks: 100451
  Show dependency treegraph
 
Reported: 2012-11-04 22:18 PST by Shinya Kawanaka
Modified: 2012-11-11 19:40 PST (History)
8 users (show)

See Also:


Attachments
WIP (17.60 KB, patch)
2012-11-07 03:42 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
WIP (46.02 KB, patch)
2012-11-08 01:36 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
WIP (46.01 KB, patch)
2012-11-08 01:41 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
WIP (46.25 KB, patch)
2012-11-08 02:25 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
WIP (34.39 KB, patch)
2012-11-08 20:18 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (29.44 KB, patch)
2012-11-08 23:11 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Shinya Kawanaka 2012-11-04 22:18:55 PST
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.
Comment 1 Shinya Kawanaka 2012-11-07 03:42:04 PST
Created attachment 172753 [details]
WIP
Comment 2 Build Bot 2012-11-07 03:52:51 PST
Comment on attachment 172753 [details]
WIP

Attachment 172753 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14763153
Comment 3 Build Bot 2012-11-07 05:31:54 PST
Comment on attachment 172753 [details]
WIP

Attachment 172753 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14757360
Comment 4 Dimitri Glazkov (Google) 2012-11-07 09:04:03 PST
Comment on attachment 172753 [details]
WIP

Really excited about this direction. Being smart about when to redistribute is awesome.
Comment 5 Shinya Kawanaka 2012-11-08 01:36:15 PST
Created attachment 172957 [details]
WIP
Comment 6 Shinya Kawanaka 2012-11-08 01:37:33 PST
This is almost reviewable, though it contains a patch for Bug 101543.
Comment 7 Shinya Kawanaka 2012-11-08 01:41:22 PST
Created attachment 172961 [details]
WIP
Comment 8 Build Bot 2012-11-08 02:03:50 PST
Comment on attachment 172961 [details]
WIP

Attachment 172961 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14763482
Comment 9 Build Bot 2012-11-08 02:23:33 PST
Comment on attachment 172961 [details]
WIP

Attachment 172961 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14771088
Comment 10 Shinya Kawanaka 2012-11-08 02:25:29 PST
Created attachment 172967 [details]
WIP
Comment 11 Build Bot 2012-11-08 05:34:08 PST
Comment on attachment 172967 [details]
WIP

Attachment 172967 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14757769
Comment 12 Dimitri Glazkov (Google) 2012-11-08 08:47:39 PST
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.
Comment 13 Shinya Kawanaka 2012-11-08 20:18:09 PST
Created attachment 173185 [details]
WIP
Comment 14 Shinya Kawanaka 2012-11-08 20:19:19 PST
(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).
Comment 15 Shinya Kawanaka 2012-11-08 23:06:21 PST
not Bug 100738 but Bug 101543.
Comment 16 Shinya Kawanaka 2012-11-08 23:11:16 PST
Created attachment 173213 [details]
Patch
Comment 17 Dimitri Glazkov (Google) 2012-11-09 09:10:45 PST
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.
Comment 18 Shinya Kawanaka 2012-11-10 00:27:16 PST
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()...
Comment 19 Shinya Kawanaka 2012-11-10 01:59:08 PST
(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.
Comment 20 Shinya Kawanaka 2012-11-11 17:56:37 PST
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...
Comment 21 Shinya Kawanaka 2012-11-11 18:11:00 PST
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"]
Comment 22 WebKit Review Bot 2012-11-11 19:40:35 PST
Comment on attachment 173213 [details]
Patch

Clearing flags on attachment: 173213

Committed r134184: <http://trac.webkit.org/changeset/134184>
Comment 23 WebKit Review Bot 2012-11-11 19:40:40 PST
All reviewed patches have been landed.  Closing bug.