Bug 82169 - [Shadow DOM] Implement a '::distributed()' pseudo element.
: [Shadow DOM] Implement a '::distributed()' pseudo element.
Status: RESOLVED FIXED
: WebKit
CSS
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
: WebExposed
: 100832 101048 106879
: 97282
  Show dependency treegraph
 
Reported: 2012-03-25 23:07 PST by
Modified: 2013-02-13 23:53 PST (History)


Attachments
WIP. update a parser. (8.52 KB, patch)
2012-10-23 01:44 PST, Hayato Ito
no flags Review Patch | Details | Formatted Diff | Diff
WIP. A lot of debug code. (42.71 KB, patch)
2012-11-02 07:14 PST, Hayato Ito
no flags Review Patch | Details | Formatted Diff | Diff
WIP. No crash. (41.15 KB, patch)
2012-11-07 16:31 PST, Hayato Ito
no flags Review Patch | 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 Review Patch | 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 Review Patch | Details | Formatted Diff | Diff
guard by SHADOW_DOM flag (31.52 KB, patch)
2012-11-21 22:38 PST, Hayato Ito
no flags Review Patch | 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 Review Patch | 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 Review Patch | Details | Formatted Diff | Diff
Addressed comments (34.55 KB, patch)
2012-11-29 05:24 PST, Hayato Ito
no flags Review Patch | Details | Formatted Diff | Diff
Fix a mac build. (34.66 KB, patch)
2012-11-29 18:23 PST, Hayato Ito
no flags Review Patch | Details | Formatted Diff | Diff
Minor update. (34.60 KB, patch)
2012-11-29 19:51 PST, Hayato Ito
no flags Review Patch | Details | Formatted Diff | Diff
Implement a ::distributed pseudo elements (45.04 KB, patch)
2012-12-20 03:38 PST, Hayato Ito
no flags Review Patch | 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 Review Patch | 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 Review Patch | Details | Formatted Diff | Diff
Update ChangeLog. Check scopeOfLastElement using isDescendantof (49.35 KB, patch)
2013-01-07 23:32 PST, Hayato Ito
no flags Review Patch | 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 Review Patch | Details | Formatted Diff | Diff
factor addStyleRule. (51.92 KB, patch)
2013-01-08 20:30 PST, Hayato Ito
no flags Review Patch | Details | Formatted Diff | Diff
iteration. A lot of refinements. (59.52 KB, patch)
2013-01-09 22:14 PST, Hayato Ito
no flags Review Patch | 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 Review Patch | Details | Formatted Diff | Diff
Remove DISTRIBUTED from RenderStyleConstants.h (59.23 KB, patch)
2013-01-10 00:24 PST, Hayato Ito
no flags Review Patch | 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 Review Patch | Details | Formatted Diff | Diff
Update a ChangeLog (69.99 KB, patch)
2013-01-11 01:19 PST, Hayato Ito
no flags Review Patch | Details | Formatted Diff | Diff
iter (69.24 KB, patch)
2013-01-14 22:08 PST, Hayato Ito
no flags Review Patch | Details | Formatted Diff | Diff
Rebased on a factored patch. (59.92 KB, patch)
2013-01-16 01:31 PST, Hayato Ito
no flags Review Patch | 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 Review Patch | Details | Formatted Diff | Diff
Rebased. (61.12 KB, patch)
2013-02-12 00:13 PST, Hayato Ito
no flags Review Patch | Details | Formatted Diff | Diff
Rabased again. (62.22 KB, patch)
2013-02-12 23:47 PST, Hayato Ito
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (63.50 KB, patch)
2013-02-13 23:24 PST, Hayato Ito
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


------- Comment #1 From 2012-10-22 22:35:17 PST -------
Let me try to implement this.
------- Comment #2 From 2012-10-23 01:44:00 PST -------
Created an attachment (id=170083) [details]
WIP. update a parser.
------- Comment #3 From 2012-11-02 06:06:47 PST -------
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 From 2012-11-02 07:14:17 PST -------
Created an attachment (id=172054) [details]
WIP. A lot of debug code.
------- Comment #5 From 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] [details]
> WIP. A lot of debug code.
------- Comment #6 From 2012-11-07 16:31:00 PST -------
Created an attachment (id=172889) [details]
WIP. No crash.
------- Comment #7 From 2012-11-21 00:57:27 PST -------
Created an attachment (id=175365) [details]
WIP. Clean up and update a test.
------- Comment #8 From 2012-11-21 03:25:56 PST -------
Created an attachment (id=175404) [details]
Ready for review. Let me file another bugs later.
------- Comment #9 From 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 From 2012-11-21 11:07:58 PST -------
(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 #11 From 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 From 2012-11-21 19:58:58 PST -------
(From update of attachment 175404 [details])
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 From 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 From 2012-11-21 22:38:51 PST -------
Created an attachment (id=175593) [details]
guard by SHADOW_DOM flag
------- Comment #15 From 2012-11-28 03:12:03 PST -------
Created an attachment (id=176437) [details]
Update. CSSSelectorList is now correctly divided into each RuleSet.
------- Comment #16 From 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] [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 From 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 From 2012-11-29 03:55:53 PST -------
Created an attachment (id=176698) [details]
Add test. A select reference combinator is used in selecor list.
------- Comment #19 From 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 From 2012-11-29 04:08:29 PST -------
(From update of attachment 176698 [details])
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 From 2012-11-29 04:46:09 PST -------
(From update of attachment 176698 [details])
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 From 2012-11-29 05:24:28 PST -------
Created an attachment (id=176709) [details]
Addressed comments
------- Comment #23 From 2012-11-29 05:26:32 PST -------
(From update of attachment 176698 [details])
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 From 2012-11-29 13:50:39 PST -------
(From update of attachment 176709 [details])
Attachment 176709 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15058006
------- Comment #25 From 2012-11-29 18:23:18 PST -------
Created an attachment (id=176878) [details]
Fix a mac build.
------- Comment #26 From 2012-11-29 19:51:14 PST -------
Created an attachment (id=176892) [details]
Minor update.
------- Comment #27 From 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 From 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 From 2012-12-20 03:38:48 PST -------
Created an attachment (id=180316) [details]
Implement a ::distributed pseudo elements
------- Comment #30 From 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] [details]
> Implement a ::distributed pseudo elements
------- Comment #31 From 2012-12-20 03:49:15 PST -------
(From update of attachment 180316 [details])
Attachment 180316 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15418917
------- Comment #32 From 2012-12-20 03:50:06 PST -------
(From update of attachment 180316 [details])
Attachment 180316 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15449035
------- Comment #33 From 2012-12-20 04:55:26 PST -------
(From update of attachment 180316 [details])
Attachment 180316 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15446071
------- Comment #34 From 2012-12-20 11:11:28 PST -------
(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?

> 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 From 2012-12-20 17:20:33 PST -------
(From update of attachment 180316 [details])
Attachment 180316 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15457208
------- Comment #36 From 2013-01-06 21:22:31 PST -------
Created an attachment (id=181474) [details]
Add more tests. Update ChangeLog. Make it compilable without SHADOW_DOM flag. Support Reprojection.
------- Comment #37 From 2013-01-06 21:25:19 PST -------
Thank you for the review.

(In reply to comment #34)
> (From update of attachment 180316 [details] [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 From 2013-01-06 21:27:53 PST -------
Created an attachment (id=181476) [details]
Remove rule_bison.py from the previous patch.
------- Comment #39 From 2013-01-07 03:03:15 PST -------
(From update of attachment 181476 [details])
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 From 2013-01-07 16:35:22 PST -------
(From update of attachment 181476 [details])
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 From 2013-01-07 18:49:24 PST -------
(From update of attachment 181476 [details])
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 From 2013-01-07 19:53:58 PST -------
(From update of attachment 181476 [details])
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 From 2013-01-07 23:12:30 PST -------
(From update of attachment 181476 [details])
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 From 2013-01-07 23:32:28 PST -------
Created an attachment (id=181656) [details]
Update ChangeLog. Check scopeOfLastElement using isDescendantof
------- Comment #45 From 2013-01-08 00:00:25 PST -------
Created an attachment (id=181659) [details]
Add a test for ::distributed() used in scoped style.
------- Comment #46 From 2013-01-08 01:39:45 PST -------
(From update of attachment 181659 [details])
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 From 2013-01-08 06:32:25 PST -------
(From update of attachment 181659 [details])
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 From 2013-01-08 20:30:03 PST -------
Created an attachment (id=181830) [details]
factor addStyleRule.
------- Comment #49 From 2013-01-08 20:34:34 PST -------
(From update of attachment 181659 [details])
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 From 2013-01-08 20:37:42 PST -------
(From update of attachment 181830 [details])
Attachment 181830 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15754564
------- Comment #51 From 2013-01-08 20:38:56 PST -------
(From update of attachment 181830 [details])
Attachment 181830 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15771312
------- Comment #52 From 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] [details])
> Attachment 181830 [details] [details] did not pass qt-ews (qt):
> Output: http://queues.webkit.org/results/15754564
------- Comment #53 From 2013-01-08 20:51:59 PST -------
(From update of attachment 181830 [details])
Attachment 181830 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15757360
------- Comment #54 From 2013-01-08 20:59:05 PST -------
(From update of attachment 181830 [details])
Attachment 181830 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15775198
------- Comment #55 From 2013-01-08 21:02:45 PST -------
(From update of attachment 181830 [details])
Attachment 181830 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15758388
------- Comment #56 From 2013-01-08 21:08:58 PST -------
(From update of attachment 181830 [details])
Attachment 181830 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15755414
------- Comment #57 From 2013-01-08 21:12:26 PST -------
(From update of attachment 181830 [details])
Attachment 181830 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/15756419
------- Comment #58 From 2013-01-08 21:29:08 PST -------
(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?

> 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 From 2013-01-09 11:30:29 PST -------
(From update of attachment 181830 [details])
All these ifdefs and extra gobs of code in StyleResolver make me super-sad. There has got to be a better way.
------- Comment #60 From 2013-01-09 13:12:54 PST -------
(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.

>>> 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 From 2013-01-09 13:15:35 PST -------
(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.
------- Comment #62 From 2013-01-09 22:14:56 PST -------
Created an attachment (id=182063) [details]
iteration. A lot of refinements.
------- Comment #63 From 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] [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 From 2013-01-09 23:11:49 PST -------
Thank you for the review.

(In reply to comment #60)
> (From update of attachment 181476 [details] [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 From 2013-01-09 23:14:45 PST -------
Thank you for the review again.

(In reply to comment #61)
> (From update of attachment 181830 [details] [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 From 2013-01-10 00:03:08 PST -------
(From update of attachment 182063 [details])
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 From 2013-01-10 00:05:44 PST -------
Created an attachment (id=182077) [details]
use enum CrossBoundary instead of scopeOfLastElement. Reuse scope.
------- Comment #68 From 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] [details]
> use enum CrossBoundary instead of scopeOfLastElement. Reuse scope.
------- Comment #69 From 2013-01-10 00:24:31 PST -------
Created an attachment (id=182079) [details]
Remove DISTRIBUTED from RenderStyleConstants.h
------- Comment #70 From 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 From 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 From 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 From 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 From 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 From 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 From 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 From 2013-01-11 00:37:43 PST -------
Created an attachment (id=182283) [details]
Refactor StyleResolver so that a class ShadowDistributedRules can be separataed.
------- Comment #78 From 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] [details]
> Refactor StyleResolver so that a class ShadowDistributedRules can be separataed.
------- Comment #79 From 2013-01-11 01:19:40 PST -------
Created an attachment (id=182293) [details]
Update a ChangeLog
------- Comment #80 From 2013-01-11 09:25:38 PST -------
(From update of attachment 182293 [details])
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 From 2013-01-14 22:08:19 PST -------
Created an attachment (id=182702) [details]
iter
------- Comment #82 From 2013-01-14 22:14:50 PST -------
(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 #83 From 2013-01-14 22:22:10 PST -------
(From update of attachment 182702 [details])
Attachment 182702 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15875825
------- Comment #84 From 2013-01-14 22:22:32 PST -------
(From update of attachment 182702 [details])
Attachment 182702 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15845814
------- Comment #85 From 2013-01-14 22:23:43 PST -------
(From update of attachment 182702 [details])
Attachment 182702 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15867767
------- Comment #86 From 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] [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 From 2013-01-14 22:40:05 PST -------
(From update of attachment 182702 [details])
Attachment 182702 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15875829
------- Comment #88 From 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] [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 From 2013-01-15 09:10:32 PST -------
(From update of attachment 182702 [details])
removing review flag, since this is just an iteration.
------- Comment #90 From 2013-01-16 01:31:04 PST -------
Created an attachment (id=182936) [details]
Rebased on a factored patch.
------- Comment #91 From 2013-01-16 08:17:26 PST -------
(From update of attachment 182936 [details])
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 From 2013-01-16 20:42:53 PST -------
(From update of attachment 182936 [details])
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 From 2013-01-16 20:47:14 PST -------
Created an attachment (id=183111) [details]
Addressed. We need to update the URL in the ChangeLog once the spec is updated.
------- Comment #94 From 2013-01-23 10:04:11 PST -------
*** Bug 107654 has been marked as a duplicate of this bug. ***
------- Comment #95 From 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 From 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 From 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 From 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 From 2013-02-12 00:13:58 PST -------
Created an attachment (id=187789) [details]
Rebased.
------- Comment #100 From 2013-02-12 11:10:05 PST -------
(From update of attachment 187789 [details])
The rebase didn't work? All EWSes are purple.
------- Comment #101 From 2013-02-12 23:47:39 PST -------
Created an attachment (id=188023) [details]
Rabased again.
------- Comment #102 From 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] [details]
> Rabased again.
------- Comment #103 From 2013-02-13 20:14:39 PST -------
(From update of attachment 188023 [details])
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 From 2013-02-13 21:20:07 PST -------
(From update of attachment 188023 [details])
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 From 2013-02-13 23:24:10 PST -------
Created an attachment (id=188267) [details]
Patch for landing
------- Comment #106 From 2013-02-13 23:53:24 PST -------
(From update of attachment 188267 [details])
Clearing flags on attachment: 188267

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