Bug 163232 - Stop copying author shadow pseudo rules into shadow tree style resolver
Summary: Stop copying author shadow pseudo rules into shadow tree style resolver
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-10-10 12:01 PDT by Antti Koivisto
Modified: 2016-10-11 03:59 PDT (History)
2 users (show)

See Also:


Attachments
patch (6.91 KB, patch)
2016-10-10 12:37 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (6.92 KB, patch)
2016-10-10 12:42 PDT, Antti Koivisto
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-yosemite (903.42 KB, application/zip)
2016-10-10 13:28 PDT, Build Bot
no flags Details
patch (9.41 KB, patch)
2016-10-10 13:46 PDT, Antti Koivisto
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antti Koivisto 2016-10-10 12:01:19 PDT
Instead do a lookup from the document author style during style resolution. This is simpler.
Comment 1 Antti Koivisto 2016-10-10 12:37:21 PDT
Created attachment 291141 [details]
patch
Comment 2 Antti Koivisto 2016-10-10 12:42:20 PDT
Created attachment 291144 [details]
patch
Comment 3 Build Bot 2016-10-10 13:28:49 PDT
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
Comment 4 Build Bot 2016-10-10 13:28:52 PDT
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
Comment 5 Antti Koivisto 2016-10-10 13:46:53 PDT
Created attachment 291155 [details]
patch
Comment 6 Darin Adler 2016-10-10 14:02:19 PDT
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?
Comment 7 Antti Koivisto 2016-10-11 03:24:27 PDT
> 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.
Comment 8 Antti Koivisto 2016-10-11 03:59:19 PDT
https://trac.webkit.org/r207077