Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(877)

Issue 1703893002: Don't add rule feature data for rules which may never match. (Closed)

Created:
4 years, 10 months ago by rune
Modified:
4 years, 10 months ago
Reviewers:
kochi, Eric Willigers
CC:
chromium-reviews, blink-reviews-css, dglazkov+blink, apavlov+blink_chromium.org, darktears, blink-reviews, rwlbuis
Base URL:
https://chromium.googlesource.com/chromium/src.git@uncommon-attr-20160216
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Don't add rule feature data for rules which may never match. :host and :host-context must always be in the rightmost compound with no other simple selectors except a succeding pseudo element in order to match as the host element is feature-less in that context. It's however not an invalid selector according to the CSS Scoping spec, so we shouldn't drop it at parse time. We could potentially skip adding other selectors to rulesets as well including selectors like: :hover:not(:hover), div:not(div), ::content and ::slotted not in shadow trees, etc. R=kochi@chromium.org,ericwilligers@chromium.org BUG=489481 Committed: https://crrev.com/6c4b4fbb5192fb3624d428172cdf916013bc5a2e Cr-Commit-Position: refs/heads/master@{#376116}

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+170 lines, -94 lines) Patch
M third_party/WebKit/Source/core/css/CSSSelector.h View 2 chunks +0 lines, -17 lines 0 comments Download
M third_party/WebKit/Source/core/css/RuleFeature.h View 2 chunks +4 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/css/RuleFeature.cpp View 1 chunk +56 lines, -21 lines 0 comments Download
M third_party/WebKit/Source/core/css/RuleFeatureSetTest.cpp View 16 chunks +107 lines, -48 lines 0 comments Download
M third_party/WebKit/Source/core/css/RuleSet.cpp View 2 chunks +3 lines, -6 lines 0 comments Download

Depends on Patchset:

Messages

Total messages: 12 (4 generated)
rune
See dependency to other CL.
4 years, 10 months ago (2016-02-17 13:28:26 UTC) #1
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1703893002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1703893002/1
4 years, 10 months ago (2016-02-18 06:26:20 UTC) #3
kochi
Sorry for the delay in reviewing, understanding which rules to reject for :host/:host-context() was tricky ...
4 years, 10 months ago (2016-02-18 06:56:05 UTC) #4
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 10 months ago (2016-02-18 07:36:24 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1703893002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1703893002/1
4 years, 10 months ago (2016-02-18 07:39:34 UTC) #8
commit-bot: I haz the power
Committed patchset #1 (id:1)
4 years, 10 months ago (2016-02-18 07:44:49 UTC) #9
commit-bot: I haz the power
Patchset 1 (id:??) landed as https://crrev.com/6c4b4fbb5192fb3624d428172cdf916013bc5a2e Cr-Commit-Position: refs/heads/master@{#376116}
4 years, 10 months ago (2016-02-18 07:46:08 UTC) #11
esprehn
4 years, 10 months ago (2016-02-18 07:58:31 UTC) #12
Message was sent while issue was closed.
Ideally this pre match would be outside RuleFeature so querySelector could use
it too and we could add simplify all the host matching logic in SelectorChecker.

Powered by Google App Engine
This is Rietveld 408576698