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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 2283933003: Don't cache matched properties for elements without a flat-tree parent. (Closed)

Created:
4 years, 3 months ago by rune
Modified:
4 years, 3 months ago
Reviewers:
andersr, esprehn, hayato
CC:
darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-css, blink-reviews-style_chromium.org, chromium-reviews, dglazkov+blink, rwlbuis
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Don't cache matched properties for elements without a flat-tree parent. When there's not flat tree parent for the element we are computing style for, setHasExplicitlyInheritedProperties will not be set on the ComputedStyle during property application. Without that flag set correctly we allowed to add to the matched properties cache even though we shouldn't. There are some open questions here: * How should body -> html propagation work when html has a shadow tree? * Do children of a shadow host have a computed style at all when not distributed/slotted? It should be noted that attachShadow is not allowed on the <html> element. That is why the test case uses a v0 shadow tree. R=andersr@opera.com,esprehn@chromium.org,hayato@chromium.org BUG=636500 Committed: https://crrev.com/fd0e0cc88c441a51c4840945f83442281fbac773 Cr-Commit-Position: refs/heads/master@{#416200}

Patch Set 1 #

Patch Set 2 : Trying to mend test failure. #

Total comments: 2

Patch Set 3 : Pass StyleResolverState #

Unified diffs Side-by-side diffs Delta from patch set Stats (+27 lines, -5 lines) Patch
A third_party/WebKit/LayoutTests/fast/css/cache/matched-properties-cache-no-flat-tree-parent.html View 1 1 chunk +17 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/css/resolver/MatchedPropertiesCache.h View 1 2 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/css/resolver/MatchedPropertiesCache.cpp View 1 2 2 chunks +7 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp View 1 2 2 chunks +2 lines, -2 lines 0 comments Download

Messages

Total messages: 22 (11 generated)
rune
ptal
4 years, 3 months ago (2016-08-26 23:17:34 UTC) #3
esprehn
You don't need to use a v0 shadow tree, just swap the <html> documentElement for ...
4 years, 3 months ago (2016-08-26 23:21:17 UTC) #4
hayato
> * Do children of a shadow host have a computed style at all when ...
4 years, 3 months ago (2016-08-29 02:28:18 UTC) #7
rune
On 2016/08/26 23:21:17, esprehn wrote: > You don't need to use a v0 shadow tree, ...
4 years, 3 months ago (2016-08-29 09:19:50 UTC) #8
hayato
https://codereview.chromium.org/2283933003/diff/20001/third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp File third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp (right): https://codereview.chromium.org/2283933003/diff/20001/third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp#newcode1536 third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp:1536: if (cachedMatchedProperties && MatchedPropertiesCache::isCacheable(*state.style(), *state.parentStyle(), state.parentNode())) { Could you ...
4 years, 3 months ago (2016-08-30 02:01:05 UTC) #13
esprehn
This seems reasonable to me, but I'd just pass the state instead. :) lgtm if ...
4 years, 3 months ago (2016-08-30 21:37:10 UTC) #14
rune
On 2016/08/30 02:01:05, hayato wrote: > https://codereview.chromium.org/2283933003/diff/20001/third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp > File third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp (right): > > https://codereview.chromium.org/2283933003/diff/20001/third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp#newcode1536 > ...
4 years, 3 months ago (2016-09-01 17:26:57 UTC) #15
hayato
On 2016/09/01 at 17:26:57, rune wrote: > The current case for body is that it's ...
4 years, 3 months ago (2016-09-02 01:47:03 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2283933003/40001
4 years, 3 months ago (2016-09-02 04:47:13 UTC) #19
commit-bot: I haz the power
Committed patchset #3 (id:40001)
4 years, 3 months ago (2016-09-02 06:25:33 UTC) #20
commit-bot: I haz the power
4 years, 3 months ago (2016-09-02 06:27:33 UTC) #22
Message was sent while issue was closed. 
Patchset 3 (id:??) landed as
https://crrev.com/fd0e0cc88c441a51c4840945f83442281fbac773
Cr-Commit-Position: refs/heads/master@{#416200}

Powered by Google App Engine
This is Rietveld 408576698