Bug 82169 - [Shadow DOM] Implement a '::distributed()' pseudo element.
: [Shadow DOM] Implement a '::distributed()' pseudo element.
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: CSS
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Hayato Ito
: WebExposed
Depends on: 101048 100832 106879
Blocks: 97282
  Show dependency treegraph
 
Reported: 2012-03-25 23:07 PDT by Hajime Morrita
Modified: 2013-02-13 23:53 PST (History)
23 users (show)

See Also:


Attachments
WIP. update a parser. (8.52 KB, patch)
2012-10-23 01:44 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff
WIP. A lot of debug code. (42.71 KB, patch)
2012-11-02 07:14 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff
WIP. No crash. (41.15 KB, patch)
2012-11-07 16:31 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
WIP. Clean up and update a test. (28.38 KB, patch)
2012-11-21 00:57 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
Ready for review. Let me file another bugs later. (32.68 KB, patch)
2012-11-21 03:25 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
guard by SHADOW_DOM flag (31.52 KB, patch)
2012-11-21 22:38 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
Update. CSSSelectorList is now correctly divided into each RuleSet. (31.76 KB, patch)
2012-11-28 03:12 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
Add test. A select reference combinator is used in selecor list. (33.94 KB, patch)
2012-11-29 03:55 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
Addressed comments (34.55 KB, patch)
2012-11-29 05:24 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
Fix a mac build. (34.66 KB, patch)
2012-11-29 18:23 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
Minor update. (34.60 KB, patch)
2012-11-29 19:51 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
Implement a ::distributed pseudo elements (45.04 KB, patch)
2012-12-20 03:38 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
Add more tests. Update ChangeLog. Make it compilable without SHADOW_DOM flag. Support Reprojection. (49.61 KB, patch)
2013-01-06 21:22 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
Remove rule_bison.py from the previous patch. (49.42 KB, patch)
2013-01-06 21:27 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
Update ChangeLog. Check scopeOfLastElement using isDescendantof (49.35 KB, patch)
2013-01-07 23:32 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
Add a test for ::distributed() used in scoped style. (51.41 KB, patch)
2013-01-08 00:00 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
factor addStyleRule. (51.92 KB, patch)
2013-01-08 20:30 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
iteration. A lot of refinements. (59.52 KB, patch)
2013-01-09 22:14 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
use enum CrossBoundary instead of scopeOfLastElement. Reuse scope. (60.22 KB, patch)
2013-01-10 00:05 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
Remove DISTRIBUTED from RenderStyleConstants.h (59.23 KB, patch)
2013-01-10 00:24 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
Refactor StyleResolver so that a class ShadowDistributedRules can be separataed. (69.74 KB, patch)
2013-01-11 00:37 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
Update a ChangeLog (69.99 KB, patch)
2013-01-11 01:19 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
iter (69.24 KB, patch)
2013-01-14 22:08 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
Rebased on a factored patch. (59.92 KB, patch)
2013-01-16 01:31 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
Addressed. We need to update the URL in the ChangeLog once the spec is updated. (59.96 KB, patch)
2013-01-16 20:47 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
Rebased. (61.12 KB, patch)
2013-02-12 00:13 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
Rabased again. (62.22 KB, patch)
2013-02-12 23:47 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
Patch for landing (63.50 KB, patch)
2013-02-13 23:24 PST, Hayato Ito
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hajime Morrita 2012-03-25 23:07:50 PDT
http://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html#selecting-nodes-distributed-to-insertion-points
We need CSS4 combinar syntax to support this.
Comment 1 Hayato Ito 2012-10-22 22:35:17 PDT
Let me try to implement this.
Comment 2 Hayato Ito 2012-10-23 01:44:00 PDT
Created attachment 170083 [details]
WIP. update a parser.
Comment 3 Hayato Ito 2012-11-02 06:06:47 PDT
Let me explain the current status and the stragety of implementation what's in my mind.

This is the first CSS rule which crosses a shadow boundary. Current CSS implementation assumes every final target is applied in current scope. But shadow '/select/' reference combinator is defined in shadow subtree but the node who is matched in the rule is outside of the shadow subtree. This is the first case in the history of CSS, I guess.
That makes the implementation too hard.

To overcome this, I am now thinking the following approach:

In parsing CSS rule:
 - Done (in the WIP patch).

After parsing:
 - When adding a rule which includes a '/select/' reference combinator, we'll add that to the global scope. ie. Make StyleResolver have m_selectReferenceSelectorCombnatorRuleSet.
 - Preprocess a selector if it includes a '/select/' reference combinator.

    For example, suppose we have the following tree and shadow subtrees:

  - H ---------[SR1]
    - A          - C ---------------------------[SR2]
      - B          - D                            - F
                      - E (<content selct=A>)       - G (<content select=D)

  Also suppose a selector, 'F > G /select/ D > E /select/ A > B', is defined in the shadow DOM subtree of SR2.

  In this case, we'll add a psuedo combinator, which is used only internally, at the left-most position of the selector like:

    SR2 x F > G /select/ D > E /select/ A > B

  The benefit of this pre-processing is we can check that F is defined in the SR2 scope easily.
  Note that things are not so simple. We have to check SR2's applyAuthorStyle also.

When matching:
 - Try to match each CSS Selector right-to-left as usual.
 - If we encounter a '/select' reference combinator, we have to check the all *midpoint* insertion points to where a node is distributed either directly or indirectly.
   Continue matching for each recursively. If one of them matches, the selector matches.
Comment 4 Hayato Ito 2012-11-02 07:14:17 PDT
Created attachment 172054 [details]
WIP. A lot of debug code.
Comment 5 Hayato Ito 2012-11-06 00:38:56 PST
It seems the WIP patch contains a issue of life cycle of *something*. Maybe it's StyleRule, I guess.
It causes a crash in V8 garbage collection when reloading.

Let me continue to investigate the cause.

(In reply to comment #4)
> Created an attachment (id=172054) [details]
> WIP. A lot of debug code.
Comment 6 Hayato Ito 2012-11-07 16:31:00 PST
Created attachment 172889 [details]
WIP. No crash.
Comment 7 Hayato Ito 2012-11-21 00:57:27 PST
Created attachment 175365 [details]
WIP. Clean up and update a test.
Comment 8 Hayato Ito 2012-11-21 03:25:56 PST
Created attachment 175404 [details]
Ready for review. Let me file another bugs later.
Comment 9 Ojan Vafai 2012-11-21 09:34:49 PST
This should be announced on webkit-dev first as it's a new feature.

Was this discussed in a standards body somewhere? I'm not sure "select" is the best term for this. It's very generic for something so specific.

I'm not opposed to implementing it. I'd just like to make sure we ship something that won't change on us.
Comment 10 Antti Koivisto 2012-11-21 11:07:58 PST
Comment on attachment 175404 [details]
Ready for review. Let me file another bugs later.

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

I agree with Ojan, a webkit-dev post would be a good idea. The standardization status of this is also unclear to me. How much review has this received?

> Source/WebCore/css/CSSGrammar.y.in:1012
> +reference_combinator:
> +  '/' IDENT '/' maybe_space {
> +        CSSParserString& str = $2;
> +        if (parser->m_context.isHTMLDocument)
> +            str.lower();
> +        $$ = str;
> +  }
> +  ;

Please put this feature fully behind a feature flag, ENABLE(SHADOW_DOM) probably.
Comment 11 Hayato Ito 2012-11-21 19:57:30 PST
I agree. Let me announce this in webkit-dev.

(In reply to comment #9)
> This should be announced on webkit-dev first as it's a new feature.
> 
> Was this discussed in a standards body somewhere? I'm not sure "select" is the best term for this. It's very generic for something so specific.

The term of 'select' is chosen since it's attribute name of <content> element.
There has been some discussions about this reference combinator in W3C's Shadow DOM specification's bugzilla. The current spec is the conclusion of the discussions.

> 
> I'm not opposed to implementing it. I'd just like to make sure we ship something that won't change on us.
Comment 12 Hayato Ito 2012-11-21 19:58:58 PST
Comment on attachment 175404 [details]
Ready for review. Let me file another bugs later.

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

>> Source/WebCore/css/CSSGrammar.y.in:1012
>> +  ;
> 
> Please put this feature fully behind a feature flag, ENABLE(SHADOW_DOM) probably.

I agree. Let me update the patch so that this feature is fully behind a SHADOW_DOM flag.
Comment 13 Ojan Vafai 2012-11-21 22:13:31 PST
(In reply to comment #11)
> The term of 'select' is chosen since it's attribute name of <content> element.
> There has been some discussions about this reference combinator in W3C's Shadow DOM specification's bugzilla. The current spec is the conclusion of the discussions.

I see. No objection from me on this.
Comment 14 Hayato Ito 2012-11-21 22:38:51 PST
Created attachment 175593 [details]
guard by SHADOW_DOM flag
Comment 15 Hayato Ito 2012-11-28 03:12:03 PST
Created attachment 176437 [details]
Update. CSSSelectorList is now correctly divided into each RuleSet.
Comment 16 Hayato Ito 2012-11-28 04:06:44 PST
I announced that in webkit-dev a week ago and there is no concerns so far.
https://lists.webkit.org/pipermail/webkit-dev/2012-November/022950.html

I think it's okay to be reviewed.

(In reply to comment #10)
> (From update of attachment 175404 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=175404&action=review
> 
> I agree with Ojan, a webkit-dev post would be a good idea. The standardization status of this is also unclear to me. How much review has this received?
> 
> > Source/WebCore/css/CSSGrammar.y.in:1012
> > +reference_combinator:
> > +  '/' IDENT '/' maybe_space {
> > +        CSSParserString& str = $2;
> > +        if (parser->m_context.isHTMLDocument)
> > +            str.lower();
> > +        $$ = str;
> > +  }
> > +  ;
> 
> Please put this feature fully behind a feature flag, ENABLE(SHADOW_DOM) probably.
Comment 17 Dimitri Glazkov (Google) 2012-11-28 09:10:03 PST
(In reply to comment #16)
> I announced that in webkit-dev a week ago and there is no concerns so far.
> https://lists.webkit.org/pipermail/webkit-dev/2012-November/022950.html
> 
> I think it's okay to be reviewed.

Tab started a thread on www-style to make double-darn sure that this is the right approach. Can we give it a couple of days and ensure that we're using the right combinator?

http://lists.w3.org/Archives/Public/www-style/2012Nov/thread.html#msg428
Comment 18 Hayato Ito 2012-11-29 03:55:53 PST
Created attachment 176698 [details]
Add test. A select reference combinator is used in selecor list.
Comment 19 Hayato Ito 2012-11-29 03:57:05 PST
Yeah, let me keep watch on that.

(In reply to comment #17)
> (In reply to comment #16)
> > I announced that in webkit-dev a week ago and there is no concerns so far.
> > https://lists.webkit.org/pipermail/webkit-dev/2012-November/022950.html
> > 
> > I think it's okay to be reviewed.
> 
> Tab started a thread on www-style to make double-darn sure that this is the right approach. Can we give it a couple of days and ensure that we're using the right combinator?
> 
> http://lists.w3.org/Archives/Public/www-style/2012Nov/thread.html#msg428
Comment 20 Hajime Morrita 2012-11-29 04:08:29 PST
Comment on attachment 176698 [details]
Add test. A select reference combinator is used in selecor list.

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

> Source/WebCore/css/CSSSelector.h:265
> +            QualifiedName m_reference; // used for reference combinator

Could you hide this behind the ifdef? Having empty/dummy API for non-SHADOW_DOM port is OK, but allocating extra memory might not.

> Source/WebCore/css/CSSSelectorList.cpp:219
> +        return selector->isReferenceCombinator() && selector->reference() == "select";

Let's define static QualifiedName instead of comparing against string literal.

> Source/WebCore/css/RuleSet.cpp:286
> +#if ENABLE(SHADOW_DOM)

Do you thins it is good idea to move this ifdef in StyleResolver::addStyleRule() ? I guess the ifdef might be narrower without hurting simplicity much.

> Source/WebCore/css/SelectorChecker.cpp:593
> +            if (context.selector->reference() != "select")

Ditto.
Comment 21 Shinya Kawanaka 2012-11-29 04:46:09 PST
Comment on attachment 176698 [details]
Add test. A select reference combinator is used in selecor list.

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

> Source/WebCore/css/CSSGrammar.y.in:1115
> +        fprintf(stderr, "Reference Combinator!\n");

Let's remove this.

> Source/WebCore/css/SelectorChecker.cpp:596
> +            Vector<InsertionPoint*> insertionPoints;

Let's use Vector<InsertionPoint*, 8> or something so as not to allocate heap memory much.
Comment 22 Hayato Ito 2012-11-29 05:24:28 PST
Created attachment 176709 [details]
Addressed comments
Comment 23 Hayato Ito 2012-11-29 05:26:32 PST
Comment on attachment 176698 [details]
Add test. A select reference combinator is used in selecor list.

Thank you for the review.

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

>> Source/WebCore/css/CSSGrammar.y.in:1115
>> +        fprintf(stderr, "Reference Combinator!\n");
> 
> Let's remove this.

Done.

>> Source/WebCore/css/CSSSelector.h:265
>> +            QualifiedName m_reference; // used for reference combinator
> 
> Could you hide this behind the ifdef? Having empty/dummy API for non-SHADOW_DOM port is OK, but allocating extra memory might not.

Done.

>> Source/WebCore/css/CSSSelectorList.cpp:219
>> +        return selector->isReferenceCombinator() && selector->reference() == "select";
> 
> Let's define static QualifiedName instead of comparing against string literal.

Let me use HTMLNames::selectAttr.

>> Source/WebCore/css/RuleSet.cpp:286
>> +#if ENABLE(SHADOW_DOM)
> 
> Do you thins it is good idea to move this ifdef in StyleResolver::addStyleRule() ? I guess the ifdef might be narrower without hurting simplicity much.

Done. That's moved.

>> Source/WebCore/css/SelectorChecker.cpp:593
>> +            if (context.selector->reference() != "select")
> 
> Ditto.

Done.

>> Source/WebCore/css/SelectorChecker.cpp:596
>> +            Vector<InsertionPoint*> insertionPoints;
> 
> Let's use Vector<InsertionPoint*, 8> or something so as not to allocate heap memory much.

Done.
Comment 24 Build Bot 2012-11-29 13:50:39 PST
Comment on attachment 176709 [details]
Addressed comments

Attachment 176709 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15058006
Comment 25 Hayato Ito 2012-11-29 18:23:18 PST
Created attachment 176878 [details]
Fix a mac build.
Comment 26 Hayato Ito 2012-11-29 19:51:14 PST
Created attachment 176892 [details]
Minor update.
Comment 27 Hayato Ito 2012-12-11 05:20:41 PST
The bug in the spec side was updated:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=19684
Comment 28 Hayato Ito 2012-12-20 03:37:07 PST
I've changed the subject of this issue, respecting the recent discussion on the spec.

(In reply to comment #27)
> The bug in the spec side was updated:
> https://www.w3.org/Bugs/Public/show_bug.cgi?id=19684
Comment 29 Hayato Ito 2012-12-20 03:38:48 PST
Created attachment 180316 [details]
Implement a ::distributed pseudo elements
Comment 30 Hayato Ito 2012-12-20 03:41:00 PST
I am aware that we have to update the spec at first, but early feedbacks are welcome for the patch.

(In reply to comment #29)
> Created an attachment (id=180316) [details]
> Implement a ::distributed pseudo elements
Comment 31 Early Warning System Bot 2012-12-20 03:49:15 PST
Comment on attachment 180316 [details]
Implement a ::distributed pseudo elements

Attachment 180316 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15418917
Comment 32 Early Warning System Bot 2012-12-20 03:50:06 PST
Comment on attachment 180316 [details]
Implement a ::distributed pseudo elements

Attachment 180316 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15449035
Comment 33 Build Bot 2012-12-20 04:55:26 PST
Comment on attachment 180316 [details]
Implement a ::distributed pseudo elements

Attachment 180316 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15446071
Comment 34 Dimitri Glazkov (Google) 2012-12-20 11:11:28 PST
Comment on attachment 180316 [details]
Implement a ::distributed pseudo elements

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

> Source/WebCore/ChangeLog:17
> +        This patch implements a select reference combinator as the first
> +        reference combinator implemented in WebKit.  Since WebKit has not
> +        supported reference combinators so far, CSS grammer and parser are
> +        updated to support the new syntax used in Selector Level4's
> +        reference combinators.

This is not necessary anymore, right?

> Source/WebCore/html/shadow/InsertionPoint.h:183
> +// FIXME: Collects only HTMLContentElement since a select reference combinator can be
> +// applied to only <content> elements.

What is here to fix? It's unclear from the comment. Better yet, provide a link to a bug.
Comment 35 Build Bot 2012-12-20 17:20:33 PST
Comment on attachment 180316 [details]
Implement a ::distributed pseudo elements

Attachment 180316 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15457208
Comment 36 Hayato Ito 2013-01-06 21:22:31 PST
Created attachment 181474 [details]
Add more tests. Update ChangeLog. Make it compilable without SHADOW_DOM flag. Support Reprojection.
Comment 37 Hayato Ito 2013-01-06 21:25:19 PST
Thank you for the review.

(In reply to comment #34)
> (From update of attachment 180316 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180316&action=review
> 
> > Source/WebCore/ChangeLog:17
> > +        This patch implements a select reference combinator as the first
> > +        reference combinator implemented in WebKit.  Since WebKit has not
> > +        supported reference combinators so far, CSS grammer and parser are
> > +        updated to support the new syntax used in Selector Level4's
> > +        reference combinators.
> 
> This is not necessary anymore, right?

I've updated ChangeLog.

> 
> > Source/WebCore/html/shadow/InsertionPoint.h:183
> > +// FIXME: Collects only HTMLContentElement since a select reference combinator can be
> > +// applied to only <content> elements.
> 
> What is here to fix? It's unclear from the comment. Better yet, provide a link to a bug.

This FIXME comment is valid only for select' reference combinator.
I've removed the FIXME comment.
Comment 38 Hayato Ito 2013-01-06 21:27:53 PST
Created attachment 181476 [details]
Remove rule_bison.py from the previous patch.
Comment 39 Build Bot 2013-01-07 03:03:15 PST
Comment on attachment 181476 [details]
Remove rule_bison.py from the previous patch.

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

New failing tests:
scrollbars/scrollbar-orientation.html
scrollbars/disabled-scrollbar.html
inspector/styles/region-style-crash.html
fast/forms/textarea-metrics.html
scrollbars/scrollbar-buttons.html
scrollbars/scrollbars-on-positioned-content.html
scrollbars/overflow-scrollbar-combinations.html
scrollbars/basic-scrollbar.html
scrollbars/scrollbar-drag-thumb-with-large-content.html
Comment 40 Dimitri Glazkov (Google) 2013-01-07 16:35:22 PST
Comment on attachment 181476 [details]
Remove rule_bison.py from the previous patch.

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

> Source/WebCore/ChangeLog:25
> +        of each RuleSet mapped by a scope where a '::distributed()' pseudo
> +        element is originally defined. This scope is passed to

it's not a pseudo element, right? It's a function.

> Source/WebCore/ChangeLog:27
> +        exists in the scope where the rule is originally defined.

I think using the word "last" here is disorienting, since it's hard to understand what "last" means in this context. If anything, it's "first", since the selectors are resolved backwards.

> Source/WebCore/css/CSSParser.cpp:10608
> +CSSParserSelector* CSSParser::updateSpecifiersWithElementName(const AtomicString& namespacePrefix, const AtomicString& elementName, CSSParserSelector* specifiers)

The name of the function is now misleading. You're not only updating specifiers, but also occasionally change the specifiers.

> Source/WebCore/css/CSSParser.cpp:10629
> +        end->setTagHistory(elementNameSelector.release());
> +        end->setRelation(CSSSelector::ShadowDistributed);

Is this necessary? Can we not just store these like we do in -webkit-any?

> Source/WebCore/css/CSSSelector.h:80
> +            ShadowDistributed,

I am not convinced we need to treat "distributed" function as a relation.

> Source/WebCore/css/SelectorChecker.cpp:289
> +            return context.scopeOfLastElement->treeScope() == context.element->treeScope() ? SelectorMatches : SelectorFailsLocally;

Why do you store the element, when all you need is its scope?

> Source/WebCore/css/StyleResolver.cpp:454
> +void StyleResolver::addStyleRule(StyleRule* rule, ContainerNode* scope, RuleSet* ruleSet, AddRuleFlags addRuleFlags)

Ugh, what a nasty detour back into StyleResolver. Is this just because we need to populate the map?
Comment 41 WebKit Review Bot 2013-01-07 18:49:24 PST
Comment on attachment 181476 [details]
Remove rule_bison.py from the previous patch.

Attachment 181476 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15737930

New failing tests:
platform/chromium/compositing/layout-width-change.html
platform/chromium/compositing/render-surface-alpha-blending.html
fast/text-autosizing/constrained-then-position-absolute-ancestors.html
platform/chromium/compositing/3d-corners.html
platform/chromium/compositing/video-frame-size-change.html
platform/chromium/compositing/perpendicular-layer-sorting.html
platform/chromium/compositing/child-layer-3d-sorting.html
platform/chromium/compositing/accelerated-drawing/svg-filters.html
fast/css/sticky/sticky-top-zoomed.html
fast/scrolling/scrollbar-tickmarks-styled.html
compositing/scrollbar-painting.html
fast/repaint/fixed-right-in-page-scale.html
platform/chromium/compositing/accelerated-drawing/alpha.html
fast/events/scale-and-scroll-window.html
fast/loader/text-document-wrapping.html
compositing/overflow/clip-content-under-overflow-controls.html
fast/events/scale-and-scroll-body.html
fast/forms/textarea-metrics.html
fast/text-autosizing/constrained-and-overflow-scroll-ancestor.html
platform/chromium/compositing/backface-visibility-transformed.html
fast/frames/transparent-scrollbar.html
fast/repaint/fixed-right-bottom-in-page-scale.html
fast/events/scale-and-scroll-iframe-body.html
fast/events/scale-and-scroll-iframe-window.html
fast/loader/javascript-url-in-object.html
fast/repaint/fixed-in-page-scale.html
platform/chromium/compositing/img-layer-grow.html
Comment 42 WebKit Review Bot 2013-01-07 19:53:58 PST
Comment on attachment 181476 [details]
Remove rule_bison.py from the previous patch.

Attachment 181476 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15741880

New failing tests:
fast/text-autosizing/constrained-then-position-absolute-ancestors.html
fast/forms/textarea-metrics.html
fast/css/sticky/sticky-top-zoomed.html
fast/scrolling/scrollbar-tickmarks-styled.html
compositing/scrollbar-painting.html
fast/repaint/fixed-right-in-page-scale.html
platform/chromium/virtual/gpu/compositedscrolling/scrollbars/scrollbars-on-positioned-content.html
fast/events/scale-and-scroll-window.html
fast/loader/text-document-wrapping.html
fast/text/international/hindi-spacing.html
compositing/overflow/clip-content-under-overflow-controls.html
fast/events/scale-and-scroll-iframe-body.html
fast/text-autosizing/constrained-and-overflow-scroll-ancestor.html
fast/frames/transparent-scrollbar.html
fast/repaint/fixed-right-bottom-in-page-scale.html
fast/events/scale-and-scroll-body.html
fast/events/scale-and-scroll-iframe-window.html
platform/chromium/virtual/gpu/compositedscrolling/scrollbars/listbox-scrollbar-combinations.html
fast/repaint/fixed-in-page-scale.html
platform/chromium/virtual/gpu/compositedscrolling/scrollbars/basic-scrollbar.html
platform/chromium/virtual/gpu/compositedscrolling/scrollbars/overflow-scrollbar-combinations.html
platform/chromium/virtual/gpu/compositedscrolling/scrollbars/scrollbar-orientation.html
fast/loader/javascript-url-in-object.html
platform/chromium/virtual/gpu/compositedscrolling/scrollbars/scrollbar-buttons.html
platform/chromium/virtual/gpu/compositedscrolling/scrollbars/custom-scrollbar-table-cell.html
platform/chromium/virtual/gpu/compositedscrolling/overflow/clip-content-under-overflow-controls.html
platform/chromium/virtual/gpu/compositedscrolling/scrollbars/disabled-scrollbar.html
Comment 43 Hayato Ito 2013-01-07 23:12:30 PST
Comment on attachment 181476 [details]
Remove rule_bison.py from the previous patch.

Thank you for the review. I don't have a new patch yet. Let me reply your comments for now.


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

>> Source/WebCore/ChangeLog:25
>> +        element is originally defined. This scope is passed to
> 
> it's not a pseudo element, right? It's a function.

'::distributed(..)' seems the first pseudo element which can take parameters.

I think we should follow the same naming convention which is used for pseudo classes.
http://dev.w3.org/csswg/selectors4/#pseudo-classes

We should call it 'a functional pseudo element', shouldn't we?

>> Source/WebCore/ChangeLog:27
>> +        exists in the scope where the rule is originally defined.
> 
> I think using the word "last" here is disorienting, since it's hard to understand what "last" means in this context. If anything, it's "first", since the selectors are resolved backwards.

That's the 'last' element actually in that sense. That's the left-most element in the selector. Using 'lastly' instead of 'last' makes sense? Let me update the ChangeLog.

The scope is passed to SelectorChecker and is used to check whether the lastly matched element
exists in the scope where the rule is originally defined.

>> Source/WebCore/css/CSSParser.cpp:10608
>> +CSSParserSelector* CSSParser::updateSpecifiersWithElementName(const AtomicString& namespacePrefix, const AtomicString& elementName, CSSParserSelector* specifiers)
> 
> The name of the function is now misleading. You're not only updating specifiers, but also occasionally change the specifiers.

Hmm.  Could you clarify a difference between update and change in this context?
I am not confident how we should name this function.

ShadowDecentRelation seems to do the similar things to what I added to this function already.

>> Source/WebCore/css/CSSParser.cpp:10629
>> +        end->setRelation(CSSSelector::ShadowDistributed);
> 
> Is this necessary? Can we not just store these like we do in -webkit-any?

As long as we use Relation, there is no such 'generic' relation which we can reuse with a parameter of '-webkit-any-xx',

The current enum Relation  is defined as:

        enum Relation {
            Descendant = 0,
            Child,
            DirectAdjacent,
            IndirectAdjacent,
            SubSelector,
            ShadowDescendant,
            ShadowDistributed,
        };


Does this answer makes sense? Are you suggesting we should not use Relation here?

>> Source/WebCore/css/CSSSelector.h:80
>> +            ShadowDistributed,
> 
> I am not convinced we need to treat "distributed" function as a relation.

The makes an implementation much simpler. Let me explain. Let's assume the following selector is given:

  A > content::distributed(B > content(distributed::(C > D)))

In this selector, D is the element which should be matched and should be checked at first. If we convert this selector to a selector which uses a relation internally like:

  A > content (distributed>) B > content (distributed>) C > D

SelectorChecker::checkSelector can receive 'D' at first. It's right-most element. We cau re-use the current implementation of SelectorChecker (right-to-left, backwards) in this case.

If we don't convert '::distributed()' to the relation, SelectorChecker::checkSelector will receive a 'content' (in 'A > content') as the first element which should be checked.
I am afraid that makes an implementation of checking complicated. We have to retrieve the inner-most element, 'D', somehow, and start matching from 'D'. And we have to go backword from 'selector given as a parameter' to a insertion point somehow.

I've abandonded this idea at early stage since converting it to the relation makes an implementation much simpler.

>> Source/WebCore/css/SelectorChecker.cpp:289
>> +            return context.scopeOfLastElement->treeScope() == context.element->treeScope() ? SelectorMatches : SelectorFailsLocally;
> 
> Why do you store the element, when all you need is its scope?

This is a due to a historical reason in this patch.  As FIXME says, this part is WIP. I have to fix this expression, line 289.
To check precisely, we need an element instead of treeScope here. I have not implemented that yet.

For example,

<shadow root>
  <div class='A'
    <div class='B'>
      <content></content>
    </div>
  </div>
  <div id='C'>
    <div id='D'>
      <style scoped>
        .A  .B content distributed::(*) { color: green }
      </style>
   </div>
 </div>

I've not tried, but I guess this style rule matches wrongly. Since in this expression,

> return context.scopeOfLastElement->treeScope() == context.element->treeScope() ? SelectorMatches : SelectorFailsLocally

context.scopeOfLastElement is 'D' and context.elemetn is 'A'. The treeScopes of both are same. So we need to store ContainerNode, 'D', instead of its treeScope. Let me try to implement checking correctly using a given element in the following patch.

>> Source/WebCore/css/StyleResolver.cpp:454
>> +void StyleResolver::addStyleRule(StyleRule* rule, ContainerNode* scope, RuleSet* ruleSet, AddRuleFlags addRuleFlags)
> 
> Ugh, what a nasty detour back into StyleResolver. Is this just because we need to populate the map?

I feel the same thing.  Agreed that's ugly. 

> Is this just because we need to populate the map?

Yes. That's the reason. I am hoping we have a better way.
Comment 44 Hayato Ito 2013-01-07 23:32:28 PST
Created attachment 181656 [details]
Update ChangeLog. Check scopeOfLastElement using isDescendantof
Comment 45 Hayato Ito 2013-01-08 00:00:25 PST
Created attachment 181659 [details]
Add a test for ::distributed() used in scoped style.
Comment 46 Build Bot 2013-01-08 01:39:45 PST
Comment on attachment 181659 [details]
Add a test for ::distributed() used in scoped style.

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

New failing tests:
scrollbars/scrollbar-orientation.html
scrollbars/disabled-scrollbar.html
inspector/styles/region-style-crash.html
fast/forms/textarea-metrics.html
scrollbars/scrollbar-drag-thumb-with-large-content.html
scrollbars/scrollbars-on-positioned-content.html
scrollbars/overflow-scrollbar-combinations.html
scrollbars/basic-scrollbar.html
scrollbars/scrollbar-buttons.html
Comment 47 Antti Koivisto 2013-01-08 06:32:25 PST
Comment on attachment 181659 [details]
Add a test for ::distributed() used in scoped style.

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

> Source/WebCore/css/RuleSet.cpp:291
>          if (rule->isStyleRule())
> -            addStyleRule(static_cast<StyleRule*>(rule), addRuleFlags);
> +            resolver->addStyleRule(static_cast<StyleRule*>(rule), const_cast<ContainerNode*>(scope), this, addRuleFlags);

RuleSet calls to StyleResolver::addStyleRule which then calls back to RuleSet::addStyleRule. This is bit of spaghetti. Please factor differently (for example by adding accessors for getting m_shadowDistributedRuleSetMap here).

> Source/WebCore/css/SelectorChecker.cpp:394
> +
> +    case CSSSelector::ShadowDistributed:

Please use SHADOW_DOM guards consistently.
Comment 48 Hayato Ito 2013-01-08 20:30:03 PST
Created attachment 181830 [details]
factor addStyleRule.
Comment 49 Hayato Ito 2013-01-08 20:34:34 PST
Comment on attachment 181659 [details]
Add a test for ::distributed() used in scoped style.

Thank you for the review.

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

>> Source/WebCore/css/RuleSet.cpp:291
>> +            resolver->addStyleRule(static_cast<StyleRule*>(rule), const_cast<ContainerNode*>(scope), this, addRuleFlags);
> 
> RuleSet calls to StyleResolver::addStyleRule which then calls back to RuleSet::addStyleRule. This is bit of spaghetti. Please factor differently (for example by adding accessors for getting m_shadowDistributedRuleSetMap here).

Done.

I've removed StyleResolver::addStyleRule. Most of code came back to RuleSet::addRulesFromSheet.
I've introduced a StyleResolver::addShadowDistributedRule() which is called from there.

>> Source/WebCore/css/SelectorChecker.cpp:394
>> +    case CSSSelector::ShadowDistributed:
> 
> Please use SHADOW_DOM guards consistently.

Done.
Comment 50 Early Warning System Bot 2013-01-08 20:37:42 PST
Comment on attachment 181830 [details]
factor addStyleRule.

Attachment 181830 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15754564
Comment 51 Early Warning System Bot 2013-01-08 20:38:56 PST
Comment on attachment 181830 [details]
factor addStyleRule.

Attachment 181830 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15771312
Comment 52 Hayato Ito 2013-01-08 20:40:27 PST
Let me fix a build without SHADOW_DOM flag.

(In reply to comment #50)
> (From update of attachment 181830 [details])
> Attachment 181830 [details] did not pass qt-ews (qt):
> Output: http://queues.webkit.org/results/15754564
Comment 53 Build Bot 2013-01-08 20:51:59 PST
Comment on attachment 181830 [details]
factor addStyleRule.

Attachment 181830 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15757360
Comment 54 WebKit Review Bot 2013-01-08 20:59:05 PST
Comment on attachment 181830 [details]
factor addStyleRule.

Attachment 181830 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15775198
Comment 55 EFL EWS Bot 2013-01-08 21:02:45 PST
Comment on attachment 181830 [details]
factor addStyleRule.

Attachment 181830 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15758388
Comment 56 Build Bot 2013-01-08 21:08:58 PST
Comment on attachment 181830 [details]
factor addStyleRule.

Attachment 181830 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15755414
Comment 57 Peter Beverloo (cr-android ews) 2013-01-08 21:12:26 PST
Comment on attachment 181830 [details]
factor addStyleRule.

Attachment 181830 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/15756419
Comment 58 Dominic Cooney 2013-01-08 21:29:08 PST
Comment on attachment 181830 [details]
factor addStyleRule.

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

> Source/WebCore/ChangeLog:8
> +        Implements a '::distributed()' pseudo element.

Is this a pseudo-element or a pseudo-class?

> Source/WebCore/ChangeLog:11
> +        http://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html#selecting-nodes-distributed-to-insertion-points

There seems to be a gap between this spec—/select/—and what you’ve implemented here—::distributed(). Who is responsible for resolving this inconsistency?

> Source/WebCore/ChangeLog:13
> +        A '::distributed()' pseudo element is the first CSS rule which

Great explanation of why this is special.

> Source/WebCore/ChangeLog:26
> +        SelectorChecker and is used to check whether the lastly matched element

I think you should give an example in concrete syntax to make this clearer (and avoid "lastly".) I think you mean

A::distributed(B)

This scope is passed to SelectorChecker and is used to check whether A exists in the scope where the rule is defined.

> Source/WebCore/ChangeLog:31
> +        from a distrirbuted pseudo element to a ShadowDistributed is done

Spelling distributed

> Source/WebCore/css/CSSParser.cpp:9504
> +        else if (isEqualToCSSIdentifier(name, "distributed"))

Should this be webkit- prefixed for now?

> Source/WebCore/css/CSSParser.cpp:10608
> +CSSParserSelector* CSSParser::updateSpecifiersWithElementName(const AtomicString& namespacePrefix, const AtomicString& elementName, CSSParserSelector* specifiers)

Would it be less error-prone to leave this returning void and make it take CSSParserSelector**?

Perhaps this should be renamed, for example

rewriteSpecifiersWtihElementName

> Source/WebCore/css/CSSParserValues.h:194
> +    bool isPseudoDistributedElement() const { return m_selector->isDistributedPseudoElement(); }

It seems needlessly inconsistent to have isPseudoDistributedElement and isDistributedPseudoElement.

> Source/WebCore/css/CSSSelector.h:318
> +

Nit: Delete this blank line.

> Source/WebCore/css/StyleResolver.cpp:368
> +    m_shadowDistributedRuleSetMap.clear();

Is this enough to stop m_shadowDistributedRuleSetMap growing unboundedly in common uses of web components? For example, if we add and remove a lot of components over time, will that reset styles and cause this map to shrink?

Who owns the key to the map? What stops the key outliving the actual node?

> Source/WebCore/html/shadow/InsertionPoint.h:182
> +inline void collectAllMidpointInsertionPoints(const Node* projectedNode, Vector<InsertionPoint*, 8>& results)

What does 'Midpoint' mean here? Maybe intermediate is a better word? Maybe collectAllInsertionPoints is even clearer?

> Source/WebCore/html/shadow/InsertionPoint.h:184
> +    InsertionPoint* insertionPoint = 0;

Why do you need this variable? Just use the local variable scoped to the if.

> LayoutTests/fast/dom/shadow/distributed-pseudo-element-nested.html:10
> +shadowStyle.innerHTML = 'div content::distributed(div content::distributed(div.green)) { color: green; }';

You should include a test where the thing directly to the left of ::distributed is neither content nor shadow, and does match (eg *) and does not match (eg span or some other non-insertion-point.)
Comment 59 Dimitri Glazkov (Google) 2013-01-09 11:30:29 PST
Comment on attachment 181830 [details]
factor addStyleRule.

All these ifdefs and extra gobs of code in StyleResolver make me super-sad. There has got to be a better way.
Comment 60 Dimitri Glazkov (Google) 2013-01-09 13:12:54 PST
Comment on attachment 181476 [details]
Remove rule_bison.py from the previous patch.

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

>>> Source/WebCore/ChangeLog:27
>>> +        exists in the scope where the rule is originally defined.
>> 
>> I think using the word "last" here is disorienting, since it's hard to understand what "last" means in this context. If anything, it's "first", since the selectors are resolved backwards.
> 
> That's the 'last' element actually in that sense. That's the left-most element in the selector. Using 'lastly' instead of 'last' makes sense? Let me update the ChangeLog.
> 
> The scope is passed to SelectorChecker and is used to check whether the lastly matched element
> exists in the scope where the rule is originally defined.

lastly sounds awkward.

>>> Source/WebCore/css/CSSParser.cpp:10608
>>> +CSSParserSelector* CSSParser::updateSpecifiersWithElementName(const AtomicString& namespacePrefix, const AtomicString& elementName, CSSParserSelector* specifiers)
>> 
>> The name of the function is now misleading. You're not only updating specifiers, but also occasionally change the specifiers.
> 
> Hmm.  Could you clarify a difference between update and change in this context?
> I am not confident how we should name this function.
> 
> ShadowDecentRelation seems to do the similar things to what I added to this function already.

True, though ShadowDescendantRelation doesn't turn around and return a whole different selector to replace the one that's being processed as $$.

>>> Source/WebCore/css/CSSParser.cpp:10629
>>> +        end->setRelation(CSSSelector::ShadowDistributed);
>> 
>> Is this necessary? Can we not just store these like we do in -webkit-any?
> 
> As long as we use Relation, there is no such 'generic' relation which we can reuse with a parameter of '-webkit-any-xx',
> 
> The current enum Relation  is defined as:
> 
>         enum Relation {
>             Descendant = 0,
>             Child,
>             DirectAdjacent,
>             IndirectAdjacent,
>             SubSelector,
>             ShadowDescendant,
>             ShadowDistributed,
>         };
> 
> 
> Does this answer makes sense? Are you suggesting we should not use Relation here?

I was suggesting not using relation, but now I realize I need to first fully comprehend whether it's possible.

> Source/WebCore/css/CSSParserValues.h:194
> +    bool isPseudoDistributedElement() const { return m_selector->isDistributedPseudoElement(); }

Do we need this anywhere? I wonder if the "::distributed(" is not the name of the function, but rather the name of the relation.

>>> Source/WebCore/css/CSSSelector.h:80
>>> +            ShadowDistributed,
>> 
>> I am not convinced we need to treat "distributed" function as a relation.
> 
> The makes an implementation much simpler. Let me explain. Let's assume the following selector is given:
> 
>   A > content::distributed(B > content(distributed::(C > D)))
> 
> In this selector, D is the element which should be matched and should be checked at first. If we convert this selector to a selector which uses a relation internally like:
> 
>   A > content (distributed>) B > content (distributed>) C > D
> 
> SelectorChecker::checkSelector can receive 'D' at first. It's right-most element. We cau re-use the current implementation of SelectorChecker (right-to-left, backwards) in this case.
> 
> If we don't convert '::distributed()' to the relation, SelectorChecker::checkSelector will receive a 'content' (in 'A > content') as the first element which should be checked.
> I am afraid that makes an implementation of checking complicated. We have to retrieve the inner-most element, 'D', somehow, and start matching from 'D'. And we have to go backword from 'selector given as a parameter' to a insertion point somehow.
> 
> I've abandonded this idea at early stage since converting it to the relation makes an implementation much simpler.

I understand. We are introducing a whole new type of CSS functions into WebKit, the CSS pseudo element functions. Per discussion with Tab, this is just a first one (lucky us).

>>> Source/WebCore/css/SelectorChecker.cpp:289
>>> +            return context.scopeOfLastElement->treeScope() == context.element->treeScope() ? SelectorMatches : SelectorFailsLocally;
>> 
>> Why do you store the element, when all you need is its scope?
> 
> This is a due to a historical reason in this patch.  As FIXME says, this part is WIP. I have to fix this expression, line 289.
> To check precisely, we need an element instead of treeScope here. I have not implemented that yet.
> 
> For example,
> 
> <shadow root>
>   <div class='A'
>     <div class='B'>
>       <content></content>
>     </div>
>   </div>
>   <div id='C'>
>     <div id='D'>
>       <style scoped>
>         .A  .B content distributed::(*) { color: green }
>       </style>
>    </div>
>  </div>
> 
> I've not tried, but I guess this style rule matches wrongly. Since in this expression,

I see. You really need to expand on this. Also, we should have a type that's different from ContainerNode to denote a scoping element. Even if it's a typedef.

> Source/WebCore/css/StyleResolver.cpp:838
> +        collectMatchingRules(it->value.get(), result.ranges.firstAuthorRule, result.ranges.lastAuthorRule, options);

Ideally, the data structure that deals with ::distributed rules should be completely factored out of the StyleResolver and its logic encapsulated. This collectMatchingRules call is what makes it difficult.

I am wondering if it's time to decouple collecting rules for an element into a separate class from StyleResolver, and then allow passing around its instance to collect rules. This should enable creating ::distributed rules class.
Comment 61 Dimitri Glazkov (Google) 2013-01-09 13:15:35 PST
Comment on attachment 181830 [details]
factor addStyleRule.

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

>> Source/WebCore/ChangeLog:13
>> +        A '::distributed()' pseudo element is the first CSS rule which
> 
> Great explanation of why this is special.

It's also the first-ever pseudo element function in WebKit.

>> Source/WebCore/css/CSSParser.cpp:10608
>> +CSSParserSelector* CSSParser::updateSpecifiersWithElementName(const AtomicString& namespacePrefix, const AtomicString& elementName, CSSParserSelector* specifiers)
> 
> Would it be less error-prone to leave this returning void and make it take CSSParserSelector**?
> 
> Perhaps this should be renamed, for example
> 
> rewriteSpecifiersWtihElementName

It seems to me this whole bit of machinery needs to be first cleaned up and named according to purpose. Stuffing more stuff in it just looks bad.

> Source/WebCore/css/SelectorChecker.h:79
> +        const ContainerNode* scopeOfLastElement;

I am still unhappy about the name. If I have to think about what this means, imagine what a person who didn't write the Shadow DOM spec would. We must do better.
Comment 62 Hayato Ito 2013-01-09 22:14:56 PST
Created attachment 182063 [details]
iteration. A lot of refinements.
Comment 63 Hayato Ito 2013-01-09 22:45:05 PST
Thank you for the review. Let me reply to only the comments I've addressed.
There still remains to-do things.

(In reply to comment #58)
> (From update of attachment 181830 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=181830&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Implements a '::distributed()' pseudo element.
> 
> Is this a pseudo-element or a pseudo-class?

As discussed in another threads, it should be a pseudo element.

> 
> > Source/WebCore/ChangeLog:11
> > +        http://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html#selecting-nodes-distributed-to-insertion-points
> 
> There seems to be a gap between this spec—/select/—and what you’ve implemented here—::distributed(). Who is responsible for resolving this inconsistency?
> 
> > Source/WebCore/ChangeLog:13
> > +        A '::distributed()' pseudo element is the first CSS rule which
> 
> Great explanation of why this is special.
> 
> > Source/WebCore/ChangeLog:26
> > +        SelectorChecker and is used to check whether the lastly matched element
> 
> I think you should give an example in concrete syntax to make this clearer (and avoid "lastly".) I think you mean
> 
> A::distributed(B)
> 
> This scope is passed to SelectorChecker and is used to check whether A exists in the scope where the rule is defined.

I've updated the ChangeLog.

> 
> > Source/WebCore/ChangeLog:31
> > +        from a distrirbuted pseudo element to a ShadowDistributed is done
> 
> Spelling distributed

Done.

> 
> > Source/WebCore/css/CSSParser.cpp:9504
> > +        else if (isEqualToCSSIdentifier(name, "distributed"))
> 
> Should this be webkit- prefixed for now?

Done. Now it's a '-webkit-distributed::(...)'

> 
> > Source/WebCore/css/CSSParser.cpp:10608
> > +CSSParserSelector* CSSParser::updateSpecifiersWithElementName(const AtomicString& namespacePrefix, const AtomicString& elementName, CSSParserSelector* specifiers)
> 
> Would it be less error-prone to leave this returning void and make it take CSSParserSelector**?

I don't think it's worth introducing CSSParseSelector** here. The current code is not so match bad so far and be consistent with updateSpecifiers() which also returns CSSParserSelector*.

> 
> Perhaps this should be renamed, for example
> 
> rewriteSpecifiersWtihElementName

Agreed on renaming. Done.


> 
> > Source/WebCore/css/CSSParserValues.h:194
> > +    bool isPseudoDistributedElement() const { return m_selector->isDistributedPseudoElement(); }
> 
> It seems needlessly inconsistent to have isPseudoDistributedElement and isDistributedPseudoElement.

Done.

> 
> > Source/WebCore/css/CSSSelector.h:318
> > +
> 
> Nit: Delete this blank line.

Done.

> 
> > Source/WebCore/css/StyleResolver.cpp:368
> > +    m_shadowDistributedRuleSetMap.clear();
> 
> Is this enough to stop m_shadowDistributedRuleSetMap growing unboundedly in common uses of web components? For example, if we add and remove a lot of components over time, will that reset styles and cause this map to shrink?
> 
> Who owns the key to the map? What stops the key outliving the actual node?

At the first glance, if a node of scope where style is defined is deleted, style node is also deleted as well. So eventually StyleResolver's reset() will be called.

Let me investigate that.

> 
> > Source/WebCore/html/shadow/InsertionPoint.h:182
> > +inline void collectAllMidpointInsertionPoints(const Node* projectedNode, Vector<InsertionPoint*, 8>& results)
> 
> What does 'Midpoint' mean here? Maybe intermediate is a better word? Maybe collectAllInsertionPoints is even clearer?

Done. Renamed it to collectAllInsertionPoints.

> 
> > Source/WebCore/html/shadow/InsertionPoint.h:184
> > +    InsertionPoint* insertionPoint = 0;
> 
> Why do you need this variable? Just use the local variable scoped to the if.

Right. This is a mistake. Done.

> 
> > LayoutTests/fast/dom/shadow/distributed-pseudo-element-nested.html:10
> > +shadowStyle.innerHTML = 'div content::distributed(div content::distributed(div.green)) { color: green; }';
> 
> You should include a test where the thing directly to the left of ::distributed is neither content nor shadow, and does match (eg *) and does not match (eg span or some other non-insertion-point.)

Done. I've added a few more tests.
Comment 64 Hayato Ito 2013-01-09 23:11:49 PST
Thank you for the review.

(In reply to comment #60)
> (From update of attachment 181476 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=181476&action=review
> 
> >>> Source/WebCore/ChangeLog:27
> >>> +        exists in the scope where the rule is originally defined.
> >> 
> >> I think using the word "last" here is disorienting, since it's hard to understand what "last" means in this context. If anything, it's "first", since the selectors are resolved backwards.
> > 
> > That's the 'last' element actually in that sense. That's the left-most element in the selector. Using 'lastly' instead of 'last' makes sense? Let me update the ChangeLog.
> > 
> > The scope is passed to SelectorChecker and is used to check whether the lastly matched element
> > exists in the scope where the rule is originally defined.
> 
> lastly sounds awkward.

I've updated the ChangeLog.

> 
> >>> Source/WebCore/css/CSSParser.cpp:10608
> >>> +CSSParserSelector* CSSParser::updateSpecifiersWithElementName(const AtomicString& namespacePrefix, const AtomicString& elementName, CSSParserSelector* specifiers)
> >> 
> >> The name of the function is now misleading. You're not only updating specifiers, but also occasionally change the specifiers.
> > 
> > Hmm.  Could you clarify a difference between update and change in this context?
> > I am not confident how we should name this function.
> > 
> > ShadowDecentRelation seems to do the similar things to what I added to this function already.
> 
> True, though ShadowDescendantRelation doesn't turn around and return a whole different selector to replace the one that's being processed as $$.
> 
> >>> Source/WebCore/css/CSSParser.cpp:10629
> >>> +        end->setRelation(CSSSelector::ShadowDistributed);
> >> 
> >> Is this necessary? Can we not just store these like we do in -webkit-any?
> > 
> > As long as we use Relation, there is no such 'generic' relation which we can reuse with a parameter of '-webkit-any-xx',
> > 
> > The current enum Relation  is defined as:
> > 
> >         enum Relation {
> >             Descendant = 0,
> >             Child,
> >             DirectAdjacent,
> >             IndirectAdjacent,
> >             SubSelector,
> >             ShadowDescendant,
> >             ShadowDistributed,
> >         };
> > 
> > 
> > Does this answer makes sense? Are you suggesting we should not use Relation here?
> 
> I was suggesting not using relation, but now I realize I need to first fully comprehend whether it's possible.

Okay. I think using relation is the best way to represent this *relation*.

> 
> > Source/WebCore/css/CSSParserValues.h:194
> > +    bool isPseudoDistributedElement() const { return m_selector->isDistributedPseudoElement(); }
> 
> Do we need this anywhere? I wonder if the "::distributed(" is not the name of the function, but rather the name of the relation.

It's used in parsing stage. After paring '::distributed() as a functional pseudo element, it is transformed to the relation.
The ShadowDistributed relation does not have a name (as a string) internally.

> 
> >>> Source/WebCore/css/CSSSelector.h:80
> >>> +            ShadowDistributed,
> >> 
> >> I am not convinced we need to treat "distributed" function as a relation.
> > 
> > The makes an implementation much simpler. Let me explain. Let's assume the following selector is given:
> > 
> >   A > content::distributed(B > content(distributed::(C > D)))
> > 
> > In this selector, D is the element which should be matched and should be checked at first. If we convert this selector to a selector which uses a relation internally like:
> > 
> >   A > content (distributed>) B > content (distributed>) C > D
> > 
> > SelectorChecker::checkSelector can receive 'D' at first. It's right-most element. We cau re-use the current implementation of SelectorChecker (right-to-left, backwards) in this case.
> > 
> > If we don't convert '::distributed()' to the relation, SelectorChecker::checkSelector will receive a 'content' (in 'A > content') as the first element which should be checked.
> > I am afraid that makes an implementation of checking complicated. We have to retrieve the inner-most element, 'D', somehow, and start matching from 'D'. And we have to go backword from 'selector given as a parameter' to a insertion point somehow.
> > 
> > I've abandonded this idea at early stage since converting it to the relation makes an implementation much simpler.
> 
> I understand. We are introducing a whole new type of CSS functions into WebKit, the CSS pseudo element functions. Per discussion with Tab, this is just a first one (lucky us).

Yay. Good luck to us :)

> 
> >>> Source/WebCore/css/SelectorChecker.cpp:289
> >>> +            return context.scopeOfLastElement->treeScope() == context.element->treeScope() ? SelectorMatches : SelectorFailsLocally;
> >> 
> >> Why do you store the element, when all you need is its scope?
> > 
> > This is a due to a historical reason in this patch.  As FIXME says, this part is WIP. I have to fix this expression, line 289.
> > To check precisely, we need an element instead of treeScope here. I have not implemented that yet.
> > 
> > For example,
> > 
> > <shadow root>
> >   <div class='A'
> >     <div class='B'>
> >       <content></content>
> >     </div>
> >   </div>
> >   <div id='C'>
> >     <div id='D'>
> >       <style scoped>
> >         .A  .B content distributed::(*) { color: green }
> >       </style>
> >    </div>
> >  </div>
> > 
> > I've not tried, but I guess this style rule matches wrongly. Since in this expression,
x
> 
> I see. You really need to expand on this. Also, we should have a type that's different from ContainerNode to denote a scoping element. Even if it's a typedef.

I've fixed this, though it's still ContainerNode.

> 
> > Source/WebCore/css/StyleResolver.cpp:838
> > +        collectMatchingRules(it->value.get(), result.ranges.firstAuthorRule, result.ranges.lastAuthorRule, options);
> 
> Ideally, the data structure that deals with ::distributed rules should be completely factored out of the StyleResolver and its logic encapsulated. This collectMatchingRules call is what makes it difficult.
> 
> I am wondering if it's time to decouple collecting rules for an element into a separate class from StyleResolver, and then allow passing around its instance to collect rules. This should enable creating ::distributed rules class.

Let me try what you are suggesting. I've never tried that.
Comment 65 Hayato Ito 2013-01-09 23:14:45 PST
Thank you for the review again.

(In reply to comment #61)
> (From update of attachment 181830 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=181830&action=review
> 
> >> Source/WebCore/ChangeLog:13
> >> +        A '::distributed()' pseudo element is the first CSS rule which
> > 
> > Great explanation of why this is special.
> 
> It's also the first-ever pseudo element function in WebKit.
> 
> >> Source/WebCore/css/CSSParser.cpp:10608
> >> +CSSParserSelector* CSSParser::updateSpecifiersWithElementName(const AtomicString& namespacePrefix, const AtomicString& elementName, CSSParserSelector* specifiers)
> > 
> > Would it be less error-prone to leave this returning void and make it take CSSParserSelector**?
> > 
> > Perhaps this should be renamed, for example
> > 
> > rewriteSpecifiersWtihElementName
> 
> It seems to me this whole bit of machinery needs to be first cleaned up and named according to purpose. Stuffing more stuff in it just looks bad.
> 
> > Source/WebCore/css/SelectorChecker.h:79
> > +        const ContainerNode* scopeOfLastElement;
> 
> I am still unhappy about the name. If I have to think about what this means, imagine what a person who didn't write the Shadow DOM spec would. We must do better.

Okay. Let me think a better name. That's my bad.
Comment 66 Build Bot 2013-01-10 00:03:08 PST
Comment on attachment 182063 [details]
iteration. A lot of refinements.

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

New failing tests:
scrollbars/scrollbar-orientation.html
scrollbars/disabled-scrollbar.html
inspector/styles/region-style-crash.html
fast/forms/textarea-metrics.html
scrollbars/scrollbar-buttons.html
scrollbars/scrollbars-on-positioned-content.html
scrollbars/overflow-scrollbar-combinations.html
scrollbars/basic-scrollbar.html
scrollbars/scrollbar-drag-thumb-with-large-content.html
Comment 67 Hayato Ito 2013-01-10 00:05:44 PST
Created attachment 182077 [details]
use enum CrossBoundary instead of scopeOfLastElement. Reuse scope.
Comment 68 Hayato Ito 2013-01-10 00:10:04 PST
I've removed the usage of scopeOfLastElement. Instead of it, I've decided to use enum, CrossBoundary.
We can use an existing scope parameter to remember *scopeOfLastElement* since they are never mutually used.

(In reply to comment #67)
> Created an attachment (id=182077) [details]
> use enum CrossBoundary instead of scopeOfLastElement. Reuse scope.
Comment 69 Hayato Ito 2013-01-10 00:24:31 PST
Created attachment 182079 [details]
Remove DISTRIBUTED from RenderStyleConstants.h
Comment 70 Hayato Ito 2013-01-10 20:58:42 PST
(In reply to comment #64)

> > 
> > > Source/WebCore/css/StyleResolver.cpp:838
> > > +        collectMatchingRules(it->value.get(), result.ranges.firstAuthorRule, result.ranges.lastAuthorRule, options);
> > 
> > Ideally, the data structure that deals with ::distributed rules should be completely factored out of the StyleResolver and its logic encapsulated. This collectMatchingRules call is what makes it difficult.
> > 
> > I am wondering if it's time to decouple collecting rules for an element into a separate class from StyleResolver, and then allow passing around its instance to collect rules. This should enable creating ::distributed rules class.
> 
> Let me try what you are suggesting. I've never tried that.

I've tried that, but it turned out that it requires non-trivial refactoring. I think that should be done as another fix from a wider perspective. Maybe soon after this patch is landed?
Comment 71 Dimitri Glazkov (Google) 2013-01-10 21:06:04 PST
(In reply to comment #70)

> I've tried that, but it turned out that it requires non-trivial refactoring. I think that should be done as another fix from a wider perspective. Maybe soon after this patch is landed?

There's no great hurry to land this patch. Let's do this right.
Comment 72 Hayato Ito 2013-01-10 21:07:26 PST
I'm okay to land this patch after refactoring. I think It depends on the urgency of the need of this feature for Web developers.

(In reply to comment #70)
> (In reply to comment #64)
> 
> > > 
> > > > Source/WebCore/css/StyleResolver.cpp:838
> > > > +        collectMatchingRules(it->value.get(), result.ranges.firstAuthorRule, result.ranges.lastAuthorRule, options);
> > > 
> > > Ideally, the data structure that deals with ::distributed rules should be completely factored out of the StyleResolver and its logic encapsulated. This collectMatchingRules call is what makes it difficult.
> > > 
> > > I am wondering if it's time to decouple collecting rules for an element into a separate class from StyleResolver, and then allow passing around its instance to collect rules. This should enable creating ::distributed rules class.
> > 
> > Let me try what you are suggesting. I've never tried that.
> 
> I've tried that, but it turned out that it requires non-trivial refactoring. I think that should be done as another fix from a wider perspective. Maybe soon after this patch is landed?
Comment 73 Hayato Ito 2013-01-10 21:21:59 PST
A). One benefit of landing this before refactoring is that it forces those who will refactor StyleResolver to consider how these cross-boundary CSS rules should be implemented in their refactoring process.

B). Of course, landing this before refactoring will increase a maximum value of current StyleResolver's spaghetti a bit.

I don't have a strong opinion, but A is worth than B, isn't it?


(In reply to comment #72)
> I'm okay to land this patch after refactoring. I think It depends on the urgency of the need of this feature for Web developers.
> 
> (In reply to comment #70)
> > (In reply to comment #64)
> > 
> > > > 
> > > > > Source/WebCore/css/StyleResolver.cpp:838
> > > > > +        collectMatchingRules(it->value.get(), result.ranges.firstAuthorRule, result.ranges.lastAuthorRule, options);
> > > > 
> > > > Ideally, the data structure that deals with ::distributed rules should be completely factored out of the StyleResolver and its logic encapsulated. This collectMatchingRules call is what makes it difficult.
> > > > 
> > > > I am wondering if it's time to decouple collecting rules for an element into a separate class from StyleResolver, and then allow passing around its instance to collect rules. This should enable creating ::distributed rules class.
> > > 
> > > Let me try what you are suggesting. I've never tried that.
> > 
> > I've tried that, but it turned out that it requires non-trivial refactoring. I think that should be done as another fix from a wider perspective. Maybe soon after this patch is landed?
Comment 74 Dimitri Glazkov (Google) 2013-01-10 21:45:00 PST
(In reply to comment #73)
> A). One benefit of landing this before refactoring is that it forces those who will refactor StyleResolver to consider how these cross-boundary CSS rules should be implemented in their refactoring process.

That's true, but this is a classic paying back the technical debt scenario. You swipe the card, and then some time later "those who will refactor StyleResolver" will have to put up the cash.

What I am suggesting is that we treat StyleResolver refactoring as a yak standing in font of us. And there's no way around shaving it.
Comment 75 Hayato Ito 2013-01-10 22:00:18 PST
Okay.  I understand it. But I'd like to note that I meant that landing this before refactoring will be beneficial for us, who will refactor, instead of technical debt.

We can get benefits from landed layout tests written for '::distributed()' in refactoring process. That will give us a confidence in refactoring process. I think how to implement crossing shadow boundary rules should be considered altogether in our refactoring process.

(In reply to comment #74)
> (In reply to comment #73)
> > A). One benefit of landing this before refactoring is that it forces those who will refactor StyleResolver to consider how these cross-boundary CSS rules should be implemented in their refactoring process.
> 
> That's true, but this is a classic paying back the technical debt scenario. You swipe the card, and then some time later "those who will refactor StyleResolver" will have to put up the cash.
> 
> What I am suggesting is that we treat StyleResolver refactoring as a yak standing in font of us. And there's no way around shaving it.
Comment 76 Dimitri Glazkov (Google) 2013-01-10 22:27:05 PST
(In reply to comment #75)
> We can get benefits from landed layout tests written for '::distributed()' in refactoring process. That will give us a confidence in refactoring process. I think how to implement crossing shadow boundary rules should be considered altogether in our refactoring process.

That is actually a pretty good point.
Comment 77 Hayato Ito 2013-01-11 00:37:43 PST
Created attachment 182283 [details]
Refactor StyleResolver so that a class ShadowDistributedRules can be separataed.
Comment 78 Hayato Ito 2013-01-11 00:39:40 PST
I've done a small refactoring for StyleResolver so that we can have a ShadowDistributedRules as a separated class from StyleResolver.

- Introduces a MatchRequest class, which replaces a MatchOptions.
- Let ShadowDistributedRules return a set of MatchRequest so that we don't have to call StyleResolver::collectMatchRules() from there.

WDYT?

(In reply to comment #77)
> Created an attachment (id=182283) [details]
> Refactor StyleResolver so that a class ShadowDistributedRules can be separataed.
Comment 79 Hayato Ito 2013-01-11 01:19:40 PST
Created attachment 182293 [details]
Update a ChangeLog
Comment 80 Dimitri Glazkov (Google) 2013-01-11 09:25:38 PST
Comment on attachment 182293 [details]
Update a ChangeLog

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

Now we're cooking with gas! This looks interesting. It seems the first yak to shave is to introduce MatchRequest.

> Source/WebCore/css/StyleResolver.cpp:262
> +void ShadowDistributedRules::addShadowDistributedRule(StyleRule* rule, size_t selectorIndex, ContainerNode* scope, AddRuleFlags addRuleFlags)

addRule

> Source/WebCore/css/StyleResolver.cpp:479
> +void StyleResolver::addShadowDistributedRule(StyleRule* rule, size_t selectorIndex, ContainerNode* scope, AddRuleFlags addRuleFlags)
> +{
> +    m_shadowDistributedRules.addShadowDistributedRule(rule, selectorIndex, scope, addRuleFlags);
> +}

This is not needed anymore. RuleSet can add directly: styleResolver.shadowDistributedRules().addRule(...)

> Source/WebCore/css/StyleResolver.cpp:850
> +    m_shadowDistributedRules.collectMatchRequests(includeEmptyRules, matchRequests);
> +    for (size_t i = 0; i < matchRequests.size(); ++i)
> +        collectMatchingRules(matchRequests[i], result.ranges.firstAuthorRule, result.ranges.lastAuthorRule);

This is where stuff falls apart somewhat. If there was a RuleCollector class factored out of StyleResolver, you could just give it to ShadowDistributedRules, so that it could use it directly, instead of queueing up requests.

> Source/WebCore/css/StyleResolver.h:134
> +class MatchRequest {

MatchRequest looks like the right data structure here.

> Source/WebCore/css/StyleResolver.h:145
> +    MatchRequest updated(RuleSet* newRuleSet) const { return MatchRequest(newRuleSet, scope, includeEmptyRules, crossBoundary); }

This sounds like a copy constructor-style thing. "updated" is awkward.
Comment 81 Hayato Ito 2013-01-14 22:08:19 PST
Created attachment 182702 [details]
iter
Comment 82 Hayato Ito 2013-01-14 22:14:50 PST
Comment on attachment 182293 [details]
Update a ChangeLog

Thank you for the review.

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

>> Source/WebCore/css/StyleResolver.cpp:262
>> +void ShadowDistributedRules::addShadowDistributedRule(StyleRule* rule, size_t selectorIndex, ContainerNode* scope, AddRuleFlags addRuleFlags)
> 
> addRule

Done.

>> Source/WebCore/css/StyleResolver.cpp:479
>> +}
> 
> This is not needed anymore. RuleSet can add directly: styleResolver.shadowDistributedRules().addRule(...)

Okay. It's like a WebKit style. Done.

>> Source/WebCore/css/StyleResolver.cpp:850
>> +        collectMatchingRules(matchRequests[i], result.ranges.firstAuthorRule, result.ranges.lastAuthorRule);
> 
> This is where stuff falls apart somewhat. If there was a RuleCollector class factored out of StyleResolver, you could just give it to ShadowDistributedRules, so that it could use it directly, instead of queueing up requests.

Actually I've tried that before sending the patch and decided not to include that in the patch :). It requires non-trivial refactoring since collectMatchingXXX families depend on a lot of member variables in StyleResolver and these variables seems to be used in other function in the StyleResolver.

Let me retry that. I am aware that it is worth trying.

>> Source/WebCore/css/StyleResolver.h:145
>> +    MatchRequest updated(RuleSet* newRuleSet) const { return MatchRequest(newRuleSet, scope, includeEmptyRules, crossBoundary); }
> 
> This sounds like a copy constructor-style thing. "updated" is awkward.

Okay. I've renamed it to 'cloneWith'.
Comment 83 Early Warning System Bot 2013-01-14 22:22:10 PST
Comment on attachment 182702 [details]
iter

Attachment 182702 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15875825
Comment 84 Build Bot 2013-01-14 22:22:32 PST
Comment on attachment 182702 [details]
iter

Attachment 182702 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15845814
Comment 85 Early Warning System Bot 2013-01-14 22:23:43 PST
Comment on attachment 182702 [details]
iter

Attachment 182702 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15867767
Comment 86 Hayato Ito 2013-01-14 22:27:08 PST
Before proceeding further refactoring for StyleResolver, I am feeling that this patch might become big.

So let me separate this patch into small patches, one of which only introduces MatchRequest in another bug.

(In reply to comment #82)
> (From update of attachment 182293 [details])
> Thank you for the review.
> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=182293&action=review
> 
> >> Source/WebCore/css/StyleResolver.cpp:262
> >> +void ShadowDistributedRules::addShadowDistributedRule(StyleRule* rule, size_t selectorIndex, ContainerNode* scope, AddRuleFlags addRuleFlags)
> > 
> > addRule
> 
> Done.
> 
> >> Source/WebCore/css/StyleResolver.cpp:479
> >> +}
> > 
> > This is not needed anymore. RuleSet can add directly: styleResolver.shadowDistributedRules().addRule(...)
> 
> Okay. It's like a WebKit style. Done.
> 
> >> Source/WebCore/css/StyleResolver.cpp:850
> >> +        collectMatchingRules(matchRequests[i], result.ranges.firstAuthorRule, result.ranges.lastAuthorRule);
> > 
> > This is where stuff falls apart somewhat. If there was a RuleCollector class factored out of StyleResolver, you could just give it to ShadowDistributedRules, so that it could use it directly, instead of queueing up requests.
> 
> Actually I've tried that before sending the patch and decided not to include that in the patch :). It requires non-trivial refactoring since collectMatchingXXX families depend on a lot of member variables in StyleResolver and these variables seems to be used in other function in the StyleResolver.
> 
> Let me retry that. I am aware that it is worth trying.
> 
> >> Source/WebCore/css/StyleResolver.h:145
> >> +    MatchRequest updated(RuleSet* newRuleSet) const { return MatchRequest(newRuleSet, scope, includeEmptyRules, crossBoundary); }
> > 
> > This sounds like a copy constructor-style thing. "updated" is awkward.
> 
> Okay. I've renamed it to 'cloneWith'.
Comment 87 Build Bot 2013-01-14 22:40:05 PST
Comment on attachment 182702 [details]
iter

Attachment 182702 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15875829
Comment 88 Hayato Ito 2013-01-15 01:30:53 PST
Filed as a https://bugs.webkit.org/show_bug.cgi?id=106879.

(In reply to comment #86)
> Before proceeding further refactoring for StyleResolver, I am feeling that this patch might become big.
> 
> So let me separate this patch into small patches, one of which only introduces MatchRequest in another bug.
> 
> (In reply to comment #82)
> > (From update of attachment 182293 [details] [details])
> > Thank you for the review.
> > 
> > View in context: https://bugs.webkit.org/attachment.cgi?id=182293&action=review
> > 
> > >> Source/WebCore/css/StyleResolver.cpp:262
> > >> +void ShadowDistributedRules::addShadowDistributedRule(StyleRule* rule, size_t selectorIndex, ContainerNode* scope, AddRuleFlags addRuleFlags)
> > > 
> > > addRule
> > 
> > Done.
> > 
> > >> Source/WebCore/css/StyleResolver.cpp:479
> > >> +}
> > > 
> > > This is not needed anymore. RuleSet can add directly: styleResolver.shadowDistributedRules().addRule(...)
> > 
> > Okay. It's like a WebKit style. Done.
> > 
> > >> Source/WebCore/css/StyleResolver.cpp:850
> > >> +        collectMatchingRules(matchRequests[i], result.ranges.firstAuthorRule, result.ranges.lastAuthorRule);
> > > 
> > > This is where stuff falls apart somewhat. If there was a RuleCollector class factored out of StyleResolver, you could just give it to ShadowDistributedRules, so that it could use it directly, instead of queueing up requests.
> > 
> > Actually I've tried that before sending the patch and decided not to include that in the patch :). It requires non-trivial refactoring since collectMatchingXXX families depend on a lot of member variables in StyleResolver and these variables seems to be used in other function in the StyleResolver.
> > 
> > Let me retry that. I am aware that it is worth trying.
> > 
> > >> Source/WebCore/css/StyleResolver.h:145
> > >> +    MatchRequest updated(RuleSet* newRuleSet) const { return MatchRequest(newRuleSet, scope, includeEmptyRules, crossBoundary); }
> > > 
> > > This sounds like a copy constructor-style thing. "updated" is awkward.
> > 
> > Okay. I've renamed it to 'cloneWith'.
Comment 89 Dimitri Glazkov (Google) 2013-01-15 09:10:32 PST
Comment on attachment 182702 [details]
iter

removing review flag, since this is just an iteration.
Comment 90 Hayato Ito 2013-01-16 01:31:04 PST
Created attachment 182936 [details]
Rebased on a factored patch.
Comment 91 Dominic Cooney 2013-01-16 08:17:26 PST
Comment on attachment 182936 [details]
Rebased on a factored patch.

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

> Source/WebCore/ChangeLog:11
> +        # FIXME: Update URL after the spec is updated.

Assuming this will be fixed when the spec is updated?

> Source/WebCore/ChangeLog:35
> +        pseudo element which takes parameter.  It's also the first CSS

parameter => a parameter

> Source/WebCore/ChangeLog:37
> +        tree of its shadow host. That means a '::distributed()' pseudo

What about @host? It also crosses the boundary.

> Source/WebCore/ChangeLog:61
> +

Excellent change description. Very clear and easy to understand.

> Source/WebCore/css/SelectorChecker.cpp:286
> +        if (context.crossBoundary == CrossesBoundary) {

Both crossBoundary and CrossesBoundary sound like booleans; maybe the field should be behaviorAtBoundary, or even behaviorAtParentScopeBoundary to be really specific.

I guess this naming was set in an earlier patch. What was the review feedback? Would you consider revisiting the naming?

> Source/WebCore/css/StyleResolver.h:226
> +

Two blank lines?

> Source/WebCore/html/shadow/InsertionPoint.h:164
> +inline void collectAllInsertionPoints(const Node* projectedNode, Vector<InsertionPoint*, 8>& results)

Thank you for working on renaming this.

Now I’m concerned that the meaning of this method is only clear when you see the parameter name projectedNode. If you just see a callsite:

collectAllInsertionPoints(element, &result)

I would guess it means something like collect all of the insertion points that are children or descendants of this node.

What about collectInsertionPointsWhichProject(node, &result)? Not great. Do you have any ideas?

Is there a precise difference in meaning between distributed (ie the child of a host) and projected (anything presented via an insertion point)?

> LayoutTests/fast/dom/shadow/distributed-pseudo-element-expected.html:1
> +<!DOCTYPE html>

Excellent use of reftests.

> LayoutTests/fast/dom/shadow/distributed-pseudo-element-match-all.html:20
> +                        createDOM('div', {'class': 'hello'},

So A::distributed(B) actually has a kind of implicit descendant selector, A::distributed(* B)?

> LayoutTests/fast/dom/shadow/distributed-pseudo-element.html:1
> +<!DOCTYPE html>

Could we have tests with chained and nested ::distributed? eg

A::distributed(B)::distributed(C) (what does it mean? What does it do?)

A::distributed(B::distributed(C))
Comment 92 Hayato Ito 2013-01-16 20:42:53 PST
Comment on attachment 182936 [details]
Rebased on a factored patch.

Thank you for the review. Let me upload new patch soon, which also updates the ChangeLog.

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

>> Source/WebCore/css/SelectorChecker.cpp:286
>> +        if (context.crossBoundary == CrossesBoundary) {
> 
> Both crossBoundary and CrossesBoundary sound like booleans; maybe the field should be behaviorAtBoundary, or even behaviorAtParentScopeBoundary to be really specific.
> 
> I guess this naming was set in an earlier patch. What was the review feedback? Would you consider revisiting the naming?

No review for this in an earlier patch.

'behaviorAtBoundary' sounds good. 'behaviorAtParentScopeBoundary' sounds unnatural in this context for me.
I've renamed the field to 'behaviorAtBoundary' and enum itself to BehaviorAtBoundary.

>> Source/WebCore/css/StyleResolver.h:226
>> +
> 
> Two blank lines?

Done.

>> Source/WebCore/html/shadow/InsertionPoint.h:164
>> +inline void collectAllInsertionPoints(const Node* projectedNode, Vector<InsertionPoint*, 8>& results)
> 
> Thank you for working on renaming this.
> 
> Now I’m concerned that the meaning of this method is only clear when you see the parameter name projectedNode. If you just see a callsite:
> 
> collectAllInsertionPoints(element, &result)
> 
> I would guess it means something like collect all of the insertion points that are children or descendants of this node.
> 
> What about collectInsertionPointsWhichProject(node, &result)? Not great. Do you have any ideas?
> 
> Is there a precise difference in meaning between distributed (ie the child of a host) and projected (anything presented via an insertion point)?

Okay. Let's rename it.

The spec says:
https://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html#reprojection
> The effect of a node being distributed into more than one insertion point is called reprojection.

So I've renamed it to the 'collectInsertionPointsWhereNodeIsDistributed(node, &results)'. We can say 'NodeIsDistributed' into more than one insertion point in this context.

>> LayoutTests/fast/dom/shadow/distributed-pseudo-element-match-all.html:20
>> +                        createDOM('div', {'class': 'hello'},
> 
> So A::distributed(B) actually has a kind of implicit descendant selector, A::distributed(* B)?

Yes. That's an intentional behavior.
A distributed-pseudo-element-match-descendant.html tests that.

>> LayoutTests/fast/dom/shadow/distributed-pseudo-element.html:1
>> +<!DOCTYPE html>
> 
> Could we have tests with chained and nested ::distributed? eg
> 
> A::distributed(B)::distributed(C) (what does it mean? What does it do?)
> 
> A::distributed(B::distributed(C))

A nested case is already covered by a distributed-pseudo-element-nested.html.

A *chained* case, A::distributed(B)::distributed(C), is not allowed according to the selector spec.
http://dev.w3.org/csswg/selectors4/#pseudo-elements
> A future version of this specification may allow multiple pseudo-elements per selector.

So I'd like to leave the behavior in this case 'undefined' for a while.
Comment 93 Hayato Ito 2013-01-16 20:47:14 PST
Created attachment 183111 [details]
Addressed. We need to update the URL in the ChangeLog once the spec is updated.
Comment 94 Dimitri Glazkov (Google) 2013-01-23 10:04:11 PST
*** Bug 107654 has been marked as a duplicate of this bug. ***
Comment 95 Dimitri Glazkov (Google) 2013-02-11 11:10:29 PST
I wonder how much smaller this patch has gotten in light of all the recent refactorings?
Comment 96 Hayato Ito 2013-02-11 20:00:50 PST
I don't expect this patch get any benefits from the recent refactorings.

I guess there isn't any meaningful difference between landing this patch before and after refactoring.

(In reply to comment #95)
> I wonder how much smaller this patch has gotten in light of all the recent refactorings?
Comment 97 Dimitri Glazkov (Google) 2013-02-11 20:09:25 PST
(In reply to comment #96)
> I don't expect this patch get any benefits from the recent refactorings.
> 
> I guess there isn't any meaningful difference between landing this patch before and after refactoring.

Ok. Can you rebase and let me look at it?
Comment 98 Hayato Ito 2013-02-11 20:14:12 PST
Sure. Let me rebase.

(In reply to comment #97)
> (In reply to comment #96)
> > I don't expect this patch get any benefits from the recent refactorings.
> > 
> > I guess there isn't any meaningful difference between landing this patch before and after refactoring.
> 
> Ok. Can you rebase and let me look at it?
Comment 99 Hayato Ito 2013-02-12 00:13:58 PST
Created attachment 187789 [details]
Rebased.
Comment 100 Dimitri Glazkov (Google) 2013-02-12 11:10:05 PST
Comment on attachment 187789 [details]
Rebased.

The rebase didn't work? All EWSes are purple.
Comment 101 Hayato Ito 2013-02-12 23:47:39 PST
Created attachment 188023 [details]
Rabased again.
Comment 102 Hayato Ito 2013-02-12 23:49:21 PST
I rebased again. This rebase was not simple one since DocumentRuleSets had been introduced.
ShadowDistributedRules have moved from StyleResolver to DocumentRuleSets.

(In reply to comment #101)
> Created an attachment (id=188023) [details]
> Rabased again.
Comment 103 Dimitri Glazkov (Google) 2013-02-13 20:14:39 PST
Comment on attachment 188023 [details]
Rabased again.

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

I've made peace with this patch.

> Source/WebCore/ChangeLog:50
> +        scope is 'D'.  The scope is used to check whether the left-most

Wait... isn't the scope [A's ShadowRoot] here? This is not a scoped style.

> Source/WebCore/ChangeLog:75
> +        * css/CSSGrammar.y.in:

If you're not enumerating changes in each file, probably don't need this generated list.

> Source/WebCore/css/SelectorChecker.cpp:288
> +            return context.scope->contains(context.element) ? SelectorMatches : SelectorFailsLocally;

Linear ancestor search, ugh. We can get away with TreeScope comparisons, except for cases where the scope is created by <style scoped>. We should consider optimizing this somehow.

> Source/WebCore/css/StyleResolver.cpp:597
> +    m_ruleSets.shadowDistributedRules().collectMatchRequests(includeEmptyRules, matchRequests);
> +    for (size_t i = 0; i < matchRequests.size(); ++i)
> +        collectMatchingRules(matchRequests[i], ruleRange);

This is about the only place in the patch where I wish we could do better. In my crazy imagination, the collector of rules is decoupled from StyleResolver and could be passed to various actors, but I understand this is probably not feasible.

> Source/WebCore/css/StyleResolver.h:139
> +class MatchRequest {

This didn't need to move up in the file anymore. Probably would make the diff look better.
Comment 104 Hayato Ito 2013-02-13 21:20:07 PST
Comment on attachment 188023 [details]
Rabased again.

Thank you for the review. Let me land this patch after addressing the comments.
Since StyleResolver has still been hot recently, it might be good timing to land this.

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

> Source/WebCore/ChangeLog:12
> +        http://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html#selecting-nodes-distributed-to-insertion-points

Let me update the URL to the W3C's bug's one: https://www.w3.org/Bugs/Public/show_bug.cgi?id=19684

>> Source/WebCore/ChangeLog:50
>> +        scope is 'D'.  The scope is used to check whether the left-most
> 
> Wait... isn't the scope [A's ShadowRoot] here? This is not a scoped style.

Right. The example is not a scoped style. Fixed.

>> Source/WebCore/ChangeLog:75
>> +        * css/CSSGrammar.y.in:
> 
> If you're not enumerating changes in each file, probably don't need this generated list.

Since this patch is becoming stable, let me enumerate changes in each file briefly.

>> Source/WebCore/css/SelectorChecker.cpp:288
>> +            return context.scope->contains(context.element) ? SelectorMatches : SelectorFailsLocally;
> 
> Linear ancestor search, ugh. We can get away with TreeScope comparisons, except for cases where the scope is created by <style scoped>. We should consider optimizing this somehow.

Good point. I am sure we can improve this.
Let me improve this in another bug since this code path is hit only when '::distributed' is used.

>> Source/WebCore/css/StyleResolver.cpp:597
>> +        collectMatchingRules(matchRequests[i], ruleRange);
> 
> This is about the only place in the patch where I wish we could do better. In my crazy imagination, the collector of rules is decoupled from StyleResolver and could be passed to various actors, but I understand this is probably not feasible.

Agreed. I felt the same thing.
We can do better if we factor StyleResolver further. Let me think further how we can factor StyleResolver.

>> Source/WebCore/css/StyleResolver.h:139
>> +class MatchRequest {
> 
> This didn't need to move up in the file anymore. Probably would make the diff look better.

Unfortunately, ShadowDistributedRules::collectMatchRequests() in DocumentRuleSets.h needs MatchRequest.
Therefore, MatchRequest needs to be in a top level class in WebCore namespace. I don't think this is an ideal situation, though.
Comment 105 Hayato Ito 2013-02-13 23:24:10 PST
Created attachment 188267 [details]
Patch for landing
Comment 106 WebKit Review Bot 2013-02-13 23:53:24 PST
Comment on attachment 188267 [details]
Patch for landing

Clearing flags on attachment: 188267

Committed r142855: <http://trac.webkit.org/changeset/142855>
Comment 107 WebKit Review Bot 2013-02-13 23:53:38 PST
All reviewed patches have been landed.  Closing bug.