Bug 163232 – Stop copying author shadow pseudo rules into shadow tree style resolver

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Antti Koivisto]

Instead do a lookup from the document author style during style resolution. This is simpler.

[Antti Koivisto]

Created attachment 291141 [details]
patch

[Antti Koivisto]

Created attachment 291144 [details]
patch

[Build Bot]

Comment on attachment 291144 [details]
patch

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

New failing tests:
media/track/track-css-matching-lang.html
media/track/track-css-property-whitelist.html
media/track/track-css-cue-lifetime.html
media/track/track-css-matching.html
media/track/track-css-matching-timestamps.html

[Build Bot]

Created attachment 291152 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Antti Koivisto]

Created attachment 291155 [details]
patch

[Darin Adler]

Comment on attachment 291155 [details]
patch

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

> Source/WebCore/css/ElementRuleCollector.cpp:229
> +    auto& shadowRoot = *m_element.containingShadowRoot();

What guarantees this is non-null?

> Source/WebCore/css/ElementRuleCollector.cpp:233
> +    auto& hostAuthorRules = Style::Scope::forNode(*shadowRoot.host()).resolver().ruleSets().authorStyle();

What guarantees shadowRoot.host() is non-null?

> Source/WebCore/css/ElementRuleCollector.cpp:234
> +    MatchRequest hostAuthorRequest(&hostAuthorRules, matchRequest.includeEmptyRules);

Anders and I would write this in this new coding style:

    MatchRequest hostAuthorRequest { &hostAuthorRules, matchRequest.includeEmptyRules };

Has the benefit of not looking anything like a function call and another benefit is that it won’t do implicit type conversions like silently converting an int to a boolean.

> Source/WebCore/css/ElementRuleCollector.cpp:296
> +    auto& rules = *matchRequest.ruleSet;

What guarantees ruleSet is non-null?

[Antti Koivisto]

> What guarantees this is non-null?

The call site tests we are in a shadow tree. Added an assert.

> What guarantees shadowRoot.host() is non-null?

We should never be resolving style for a disconnected shadow tree.

> Anders and I would write this in this new coding style:
> 
>     MatchRequest hostAuthorRequest { &hostAuthorRules,
> matchRequest.includeEmptyRules };
> 
> Has the benefit of not looking anything like a function call and another
> benefit is that it won’t do implicit type conversions like silently
> converting an int to a boolean.

I'm not sure I understand the first point. Invoking a constructor is a function call so looking like one doesn't seem so wrong to me.

> What guarantees ruleSet is non-null?

Call sites assert or set it explicitly.

[Antti Koivisto]

https://trac.webkit.org/r207077