[webkit-reviews] review denied: [Bug 82169] [Shadow DOM] Implement a '::distributed()' pseudo element. : [Attachment 182293] Update a ChangeLog

[bugzilla-daemon at webkit.org]

Dimitri Glazkov (Google) <dglazkov at chromium.org> has denied 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 182293: Update a ChangeLog
https://bugs.webkit.org/attachment.cgi?id=182293&action=review

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