Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
383833002:
All selectors in :-webkit-any must have an invalidation set feature. (Closed)
|
Created:
6 years, 5 months ago by rune Modified:
6 years, 4 months ago CC:
darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-css, dglazkov+blink, ed+blinkwatch_opera.com, rwlbuis, rune+blink Base URL:
svn://svn.chromium.org/blink/trunk Project:
blink Visibility:
Public. |
DescriptionAll selectors in :-webkit-any must have an invalidation set feature.
In order to avoid subtree style recalc for a :-webkit-any selector in the
rightmost compound selector, all of its selectors must contain an
invalidation set feature. In terms of invalidation sets, these selectors
are all universal in the rightmost compound:
.x :-webkit-any(*)
.x :-webkit-any(.y, *)
.x :-webkit-any(.y, :hover)
The fix is to set foundIdent to true when all selectors in the selector
list have found one.
BUG=392852
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=180566
Patch Set 1 #
Total comments: 2
Patch Set 2 : Removed condition from other patch #
Total comments: 2
Patch Set 3 : Documentation #
Total comments: 3
Patch Set 4 : More documentation #
Total comments: 3
Patch Set 5 : Rebased. #Patch Set 6 : Avoid eventSender #
Messages
Total messages: 22 (0 generated)
This CL is based on: https://codereview.chromium.org/365063005/
https://codereview.chromium.org/383833002/diff/1/Source/core/css/RuleFeature.cpp File Source/core/css/RuleFeature.cpp (right): https://codereview.chromium.org/383833002/diff/1/Source/core/css/RuleFeature.... Source/core/css/RuleFeature.cpp:138: if (hostMode == UseLocalStyleChange || component->pseudoType() == CSSSelector::PseudoNot) The PseudoNot check sneaked its way in from another patch. Will remove.
https://codereview.chromium.org/383833002/diff/1/Source/core/css/RuleFeature.cpp File Source/core/css/RuleFeature.cpp (right): https://codereview.chromium.org/383833002/diff/1/Source/core/css/RuleFeature.... Source/core/css/RuleFeature.cpp:138: if (hostMode == UseLocalStyleChange || component->pseudoType() == CSSSelector::PseudoNot) On 2014/07/10 21:49:20, rune (futhark.irc - CET) wrote: > The PseudoNot check sneaked its way in from another patch. Will remove. Done.
https://codereview.chromium.org/383833002/diff/20001/Source/core/css/RuleFeat... File Source/core/css/RuleFeature.cpp (right): https://codereview.chromium.org/383833002/diff/20001/Source/core/css/RuleFeat... Source/core/css/RuleFeature.cpp:138: if (hostMode == UseLocalStyleChange) What does foundUniversal have to do with UseLocalStyleChange? At the very least this needs a big comment.
https://codereview.chromium.org/383833002/diff/20001/Source/core/css/RuleFeat... File Source/core/css/RuleFeature.cpp (right): https://codereview.chromium.org/383833002/diff/20001/Source/core/css/RuleFeat... Source/core/css/RuleFeature.cpp:138: if (hostMode == UseLocalStyleChange) On 2014/07/10 23:26:49, chrishtr wrote: > What does foundUniversal have to do with UseLocalStyleChange? At the very least > this needs > a big comment. Added documentation and renamed hostMode to subSelectorMode. UseLocalStyleChange means foundIdent was false when returning from invalidationSetModeForSelector, hence no invalidation set features, and universal in that sense. Would it be better to call it something like foundSubSelectorWithoutFeatures instead?
https://codereview.chromium.org/383833002/diff/30001/Source/core/css/RuleFeat... File Source/core/css/RuleFeature.cpp (right): https://codereview.chromium.org/383833002/diff/30001/Source/core/css/RuleFeat... Source/core/css/RuleFeature.cpp:148: return foundCombinator ? UseLocalStyleChange : UseSubtreeStyleChange; Please explain in the comment why it is sometimes allowed to return UseLocalStyleChange.
https://codereview.chromium.org/383833002/diff/30001/Source/core/css/RuleFeat... File Source/core/css/RuleFeature.cpp (right): https://codereview.chromium.org/383833002/diff/30001/Source/core/css/RuleFeat... Source/core/css/RuleFeature.cpp:148: return foundCombinator ? UseLocalStyleChange : UseSubtreeStyleChange; On 2014/07/11 18:14:08, chrishtr wrote: > Please explain in the comment why it is sometimes allowed to return > UseLocalStyleChange. I have to investigate why the _UseSubtreeStyleChange_ is still needed. The returned value from this method is the input to the collectFeaturesFromSelector method which does the fallback for adding invalidation set features. UseLocalStyleChange there means just create invalidation sets for the features found in the rightmost compound to do a LocalStyleChange when any of those features change, but start to use SubtreeStyleChange once we go past the first combinator. So, if the features in the rightmost compound are ok, we can clearly UseLocalStyleChange. I can't think of what type of selector found in the rightmost compound selector will require a SubtreeStyleChange for any of its features.
Added documentation, and a FIXME for investigating if UseSubtreeStyleChange is ever needed in rightmost compound. https://codereview.chromium.org/383833002/diff/30001/Source/core/css/RuleFeat... File Source/core/css/RuleFeature.cpp (right): https://codereview.chromium.org/383833002/diff/30001/Source/core/css/RuleFeat... Source/core/css/RuleFeature.cpp:148: return foundCombinator ? UseLocalStyleChange : UseSubtreeStyleChange; On 2014/07/11 18:14:08, chrishtr wrote: > Please explain in the comment why it is sometimes allowed to return > UseLocalStyleChange. Done.
lgtm
@esprehn Needs owner review.
https://codereview.chromium.org/383833002/diff/50001/Source/core/css/RuleFeat... File Source/core/css/RuleFeature.cpp (right): https://codereview.chromium.org/383833002/diff/50001/Source/core/css/RuleFeat... Source/core/css/RuleFeature.cpp:153: // changing features when present in the rightmost compound selector? Yes, but only for :host-context() since :host-context() has an implicit descendant selector. We can get rid of the whole mode thing if we handle the implicit descendant combinator for :host-context() in collectFeaturesFromSelector instead. But that's for another CL.
lgtm https://codereview.chromium.org/383833002/diff/50001/LayoutTests/fast/css/inv... File LayoutTests/fast/css/invalidation/webkit-any-universal.html (right): https://codereview.chromium.org/383833002/diff/50001/LayoutTests/fast/css/inv... LayoutTests/fast/css/invalidation/webkit-any-universal.html:43: assert_true(!!window.eventSender, "This test only works with eventSender present"); You could use :focus and then not depend on event sender.
The CQ bit was checked by rune@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rune@opera.com/383833002/90001
https://codereview.chromium.org/383833002/diff/50001/LayoutTests/fast/css/inv... File LayoutTests/fast/css/invalidation/webkit-any-universal.html (right): https://codereview.chromium.org/383833002/diff/50001/LayoutTests/fast/css/inv... LayoutTests/fast/css/invalidation/webkit-any-universal.html:43: assert_true(!!window.eventSender, "This test only works with eventSender present"); On 2014/08/16 07:55:10, esprehn wrote: > You could use :focus and then not depend on event sender. Done.
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/23438)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/23450)
The CQ bit was checked by rune@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rune@opera.com/383833002/90001
Message was sent while issue was closed.
Committed patchset #6 (90001) as 180566 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
