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

Issue 29633003: Avoid style sharing with mis-matched descendant selectors (Closed)

Created:
7 years, 2 months ago by leviw_travelin_and_unemployed
Modified:
7 years, 1 month ago
Reviewers:
esprehn, trchen
CC:
blink-reviews, dglazkov+blink, apavlov+blink_chromium.org, darktears
Visibility:
Public.

Description

Avoid style sharing with mis-matched descendant selectors Switching to an LRU for finding elements for style sharing resulted in elements sharing that don't share a direct grandparent. Because of this, the code to check that elements can share style didn't take descendant selectors into account. This patch breaks descendant selectors out and resolves them as part of checking that elements' styles can be shared. BUG=305502

Patch Set 1 #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+68 lines, -3 lines) Patch
A LayoutTests/fast/css/style-sharing-with-descendant-selectors.html View 1 chunk +17 lines, -0 lines 0 comments Download
A LayoutTests/fast/css/style-sharing-with-descendant-selectors-expected.html View 1 chunk +9 lines, -0 lines 0 comments Download
M Source/core/css/RuleFeature.h View 1 chunk +1 line, -0 lines 1 comment Download
M Source/core/css/RuleSet.cpp View 2 chunks +7 lines, -1 line 0 comments Download
M Source/core/css/resolver/ScopedStyleResolver.h View 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/css/resolver/ScopedStyleResolver.cpp View 1 chunk +8 lines, -0 lines 0 comments Download
M Source/core/css/resolver/SharedStyleFinder.h View 2 chunks +4 lines, -1 line 0 comments Download
M Source/core/css/resolver/SharedStyleFinder.cpp View 2 chunks +18 lines, -0 lines 2 comments Download
M Source/core/css/resolver/StyleResolver.h View 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/css/resolver/StyleResolver.cpp View 2 chunks +2 lines, -1 line 0 comments Download

Messages

Total messages: 2 (0 generated)
leviw_travelin_and_unemployed
I haven't written a description yet, but I wrote a different solution for the descendant ...
7 years, 2 months ago (2013-10-20 03:01:58 UTC) #1
esprehn
7 years, 2 months ago (2013-10-21 19:19:07 UTC) #2
I don't think this will work, you're making us match every rule in the entire
sheet against each element in the middle of style sharing.

Storing a Vector that's the size of the sheet also sounds bad. Almost every rule
contains a descendant selector in practice.

https://codereview.chromium.org/29633003/diff/1/Source/core/css/RuleFeature.h
File Source/core/css/RuleFeature.h (right):

https://codereview.chromium.org/29633003/diff/1/Source/core/css/RuleFeature.h...
Source/core/css/RuleFeature.h:83: Vector<RuleFeature> descendantRules;
Most rules are descendant rules, this Vector is going to be nearly the size of
the stylesheet itself.

https://codereview.chromium.org/29633003/diff/1/Source/core/css/resolver/Shar...
File Source/core/css/resolver/SharedStyleFinder.cpp (right):

https://codereview.chromium.org/29633003/diff/1/Source/core/css/resolver/Shar...
Source/core/css/resolver/SharedStyleFinder.cpp:273: if
(!candidateMatchesDescendantRules(context, element))
The hotest part of style sharing is matching the rulesets at the end. Matching
against almost the entire sheet (since nearly every rule is a descendant rule)
is going to be very very slow.

https://codereview.chromium.org/29633003/diff/1/Source/core/css/resolver/Shar...
Source/core/css/resolver/SharedStyleFinder.cpp:330: return
collector.matchedResult().ranges == candidateCollector.matchedResult().ranges;
Matching rules like this is really expensive.

Powered by Google App Engine
This is Rietveld 408576698