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

Issue 726503003: Single code path for updating invalidation sets. (Closed)

Created:
6 years, 1 month ago by rune
Modified:
6 years, 1 month ago
Reviewers:
chrishtr
CC:
darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-css, dglazkov+blink, ed+blinkwatch_opera.com, rwlbuis
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Project:
blink
Visibility:
Public.

Description

Single code path for updating invalidation sets. Allow all selectors to update invalidation sets using updateInvalidationSets and never fall back to a separate wholeSubtree loop in collectFeaturesFromSelector. The method for finding the InvalidationSetMode is dropped completely. With this change we are now at a point were a whole subtree invalidation is only necessary for these cases: 1. There are no recognized features in the rightmost compound selector - A universal selector: ".a *" - Only contains pseudo classes or elements that are not considered features: ".a ::before", ".a :first-child", etc. 2. The invalidation set component is immediately left of an adjacent combinator: ".a + .b", ".a ~ .b" 3. The selector has a ::first-letter or ::first-line pseudo These are currently depending on style recalc or layout below the element they are pseudo elements for. 3. There is a :host-context() in the rightmost compound selector :host-context sub-selectors match ascendants of the :host element for which we are resolving style, so ":host-context(.a)" is effectively the same as ".a :host" which falls under no recognized features in the rightmost compound in point 1. Points 1, 3, and 4 are now handled through InvalidationSetFeatures::forceSubtree set as a result of feature extraction. Point 2 is handled by updating InvalidationSetFeatures::adjacent as we progress past selector combinators. RuleFeaturesSet::collectFeaturesFromSelector is now back to how it was before we introduced invalidation sets (without the feature hash sets). Now, it only counts adjacent combinators and sets a flag when finding ::first-line, so it is possible to squash it into the same method as updateInvalidationSets() to have a single pass over the selector when finding rule features (if we want to do that). The lowered expected node count in the :host-context test is due to the fact that we only fall back to subtree invalidation when present in the rightmost compound. ::first-letter/::first-line are always in the rightmost compound, and :host-context() doesn't need subtree invalidation when in other compounds. ":host-context(.a) .b" is effectively ".a :host .b". R=chrishtr@chromium.org BUG=335247 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=185354

Patch Set 1 #

Patch Set 2 : Fixed rebase issue #

Patch Set 3 : Assertion fail, missing HostContext #

Total comments: 6
Unified diffs Side-by-side diffs Delta from patch set Stats (+78 lines, -127 lines) Patch
M LayoutTests/fast/css/invalidation/targeted-class-host-context.html View 1 chunk +1 line, -1 line 0 comments Download
M Source/core/css/RuleFeature.h View 3 chunks +12 lines, -13 lines 0 comments Download
M Source/core/css/RuleFeature.cpp View 1 2 9 chunks +65 lines, -113 lines 6 comments Download

Messages

Total messages: 8 (1 generated)
rune
6 years, 1 month ago (2014-11-13 14:44:11 UTC) #1
rune
Failure in last try-job doesn't look like my fault. Ready for review.
6 years, 1 month ago (2014-11-13 16:43:16 UTC) #2
chrishtr
https://codereview.chromium.org/726503003/diff/40001/Source/core/css/RuleFeature.cpp File Source/core/css/RuleFeature.cpp (right): https://codereview.chromium.org/726503003/diff/40001/Source/core/css/RuleFeature.cpp#newcode171 Source/core/css/RuleFeature.cpp:171: || pseudo == CSSSelector::PseudoHostContext This one is sort of ...
6 years, 1 month ago (2014-11-13 23:01:11 UTC) #3
rune
https://codereview.chromium.org/726503003/diff/40001/Source/core/css/RuleFeature.cpp File Source/core/css/RuleFeature.cpp (right): https://codereview.chromium.org/726503003/diff/40001/Source/core/css/RuleFeature.cpp#newcode171 Source/core/css/RuleFeature.cpp:171: || pseudo == CSSSelector::PseudoHostContext On 2014/11/13 23:01:11, chrishtr wrote: ...
6 years, 1 month ago (2014-11-14 00:04:54 UTC) #4
chrishtr
lgtm
6 years, 1 month ago (2014-11-14 01:07:14 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/726503003/40001
6 years, 1 month ago (2014-11-14 06:44:06 UTC) #7
commit-bot: I haz the power
6 years, 1 month ago (2014-11-14 08:23:41 UTC) #8
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as 185354

Powered by Google App Engine
This is Rietveld 408576698