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

Issue 1282243002: Prepare for multiple !important author ranges. (Closed)

Created:
5 years, 4 months ago by rune
Modified:
5 years, 4 months ago
Reviewers:
Timothy Loh, kochi
CC:
blink-reviews, blink-reviews-style_chromium.org, blink-reviews-css, dglazkov+blink, apavlov+blink_chromium.org, darktears, rwlbuis
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

Prepare for multiple !important author ranges. This CL is split out from [1] where we support multiple author origins in terms of shadow dom scopes. The vector of MatchedProperties contains matched declarations from all origins in the order in which they should be applied. For !important declarations, they are applied in the same order within the same origin, but the order of the origins are different as the cascading order is: - UA important - Author important - Author - UA We used to have two ranges, one for UA and one for author to accomplish this. However, with the normal/!important cascading order for declarations from different shadow doms, we need multiple author ranges: - UA important - Author important scope n ... - Author important scope 1 - Author scope 1 ... - Author scope n - UA We introduce an ImportantAuthorRangeIterator to iterate through the author ranges and add iterator-style iteration for each range through the MatchedPropertiesRange class. As the applyMatchedProperties code is hot, I've run the blink_perf.css tests locally. Some improved by 2-4%, some became worse by similar percentages. [1] https://codereview.chromium.org/1224673002/ BUG=487125 TEST=fast/css Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=200697 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=200818

Patch Set 1 #

Patch Set 2 : ready for review. #

Patch Set 3 : Re-added documentation #

Patch Set 4 : Fixed incorrect assertion #

Total comments: 8

Patch Set 5 : Added unit tests for MatchResult and fixed review issues. #

Patch Set 6 : Another issue #

Patch Set 7 : Rebased #

Patch Set 8 : Oilpan fix in unit test #

Patch Set 9 : Signed/unsigned issue #

Patch Set 10 : CORE_EXPORT needed for unit tests #

Patch Set 11 : Missing call to finishAddingAuthorRulesForTreeScope. #

Patch Set 12 : Missing STACK_ALLOCATED() #

Unified diffs Side-by-side diffs Delta from patch set Stats (+317 lines, -50 lines) Patch
M Source/core/core.gypi View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/css/ElementRuleCollector.h View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download
M Source/core/css/ElementRuleCollector.cpp View 2 chunks +1 line, -4 lines 0 comments Download
M Source/core/css/resolver/MatchResult.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +89 lines, -11 lines 0 comments Download
M Source/core/css/resolver/MatchResult.cpp View 1 2 3 4 5 1 chunk +17 lines, -2 lines 0 comments Download
A Source/core/css/resolver/MatchResultTest.cpp View 1 2 3 4 5 6 7 8 1 chunk +173 lines, -0 lines 0 comments Download
M Source/core/css/resolver/MatchedPropertiesCache.h View 2 chunks +3 lines, -3 lines 0 comments Download
M Source/core/css/resolver/MatchedPropertiesCache.cpp View 4 chunks +7 lines, -7 lines 0 comments Download
M Source/core/css/resolver/StyleResolver.h View 1 chunk +1 line, -1 line 0 comments Download
M Source/core/css/resolver/StyleResolver.cpp View 1 2 3 4 5 6 7 8 9 10 9 chunks +23 lines, -22 lines 0 comments Download

Messages

Total messages: 47 (20 generated)
rune
5 years, 4 months ago (2015-08-10 22:59:12 UTC) #2
kochi
On 2015/08/10 22:59:12, rune wrote: Is this okay to review? I applied patch set 3, ...
5 years, 4 months ago (2015-08-11 06:49:37 UTC) #3
rune
On 2015/08/11 06:49:37, Takayoshi Kochi wrote: > On 2015/08/10 22:59:12, rune wrote: > > Is ...
5 years, 4 months ago (2015-08-11 09:28:46 UTC) #4
kochi
Thanks for splitting the CL! It made my life a lot easier. This still doesn't ...
5 years, 4 months ago (2015-08-11 11:07:35 UTC) #5
rune
On 2015/08/11 11:07:35, Takayoshi Kochi wrote: > Thanks for splitting the CL! > It made ...
5 years, 4 months ago (2015-08-11 11:44:56 UTC) #6
rune
https://codereview.chromium.org/1282243002/diff/60001/Source/core/css/resolver/MatchResult.h File Source/core/css/resolver/MatchResult.h (right): https://codereview.chromium.org/1282243002/diff/60001/Source/core/css/resolver/MatchResult.h#newcode91 Source/core/css/resolver/MatchResult.h:91: void uaRulesFinished(); On 2015/08/11 11:07:35, Takayoshi Kochi wrote: > ...
5 years, 4 months ago (2015-08-12 01:13:26 UTC) #7
kochi
On 2015/08/11 11:44:56, rune wrote: > On 2015/08/11 11:07:35, Takayoshi Kochi wrote: > > Thanks ...
5 years, 4 months ago (2015-08-12 08:53:00 UTC) #8
kochi
lgtm Thanks for adding the unittest.
5 years, 4 months ago (2015-08-12 08:59:03 UTC) #9
rune
On 2015/08/12 08:59:03, Takayoshi Kochi wrote: > lgtm > > Thanks for adding the unittest. ...
5 years, 4 months ago (2015-08-13 11:04:06 UTC) #10
rune
On 2015/08/13 11:04:06, rune wrote: > On 2015/08/12 08:59:03, Takayoshi Kochi wrote: > > lgtm ...
5 years, 4 months ago (2015-08-16 21:55:05 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1282243002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1282243002/140001
5 years, 4 months ago (2015-08-17 07:43:28 UTC) #14
commit-bot: I haz the power
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_compile_dbg_ng/builds/85883)
5 years, 4 months ago (2015-08-17 07:53:22 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1282243002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1282243002/160001
5 years, 4 months ago (2015-08-17 08:54:34 UTC) #19
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_compile_dbg_32_ng/builds/85540)
5 years, 4 months ago (2015-08-17 09:11:36 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1282243002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1282243002/180001
5 years, 4 months ago (2015-08-17 09:56:28 UTC) #24
commit-bot: I haz the power
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/99709)
5 years, 4 months ago (2015-08-17 11:33:43 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1282243002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1282243002/180001
5 years, 4 months ago (2015-08-17 12:28:51 UTC) #28
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/94007)
5 years, 4 months ago (2015-08-17 13:37:50 UTC) #30
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1282243002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1282243002/180001
5 years, 4 months ago (2015-08-17 19:12:01 UTC) #32
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/99173)
5 years, 4 months ago (2015-08-17 20:30:31 UTC) #34
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1282243002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1282243002/200001
5 years, 4 months ago (2015-08-17 22:03:22 UTC) #37
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/99313)
5 years, 4 months ago (2015-08-18 00:51:55 UTC) #39
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1282243002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1282243002/200001
5 years, 4 months ago (2015-08-18 02:43:24 UTC) #41
commit-bot: I haz the power
Committed patchset #11 (id:200001) as https://src.chromium.org/viewvc/blink?view=rev&revision=200697
5 years, 4 months ago (2015-08-18 04:41:20 UTC) #42
hans
A revert of this CL (patchset #11 id:200001) has been created in https://codereview.chromium.org/1303513002/ by hans@chromium.org. ...
5 years, 4 months ago (2015-08-18 14:58:30 UTC) #43
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1282243002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1282243002/220001
5 years, 4 months ago (2015-08-19 09:09:28 UTC) #46
commit-bot: I haz the power
5 years, 4 months ago (2015-08-19 11:52:26 UTC) #47
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200818

Powered by Google App Engine
This is Rietveld 408576698