[webkit-reviews] review granted: [Bug 82169] [Shadow DOM] Implement a '::distributed()' pseudo element. : [Attachment 188023] Rabased again.

[bugzilla-daemon at webkit.org]

Dimitri Glazkov (Google) <dglazkov at chromium.org> has granted Hayato Ito
<hayato at chromium.org>'s request for review:
Bug 82169: [Shadow DOM] Implement a '::distributed()' pseudo element.
https://bugs.webkit.org/show_bug.cgi?id=82169

Attachment 188023: Rabased again.
https://bugs.webkit.org/attachment.cgi?id=188023&action=review

------- Additional Comments from Dimitri Glazkov (Google)
<dglazkov at chromium.org>
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.