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

Issue 365063005: Invalidation set features recognized left of adjacent. (Closed)

Created:
6 years, 5 months ago by rune
Modified:
6 years, 4 months ago
Reviewers:
chrishtr, esprehn
CC:
darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-css, dglazkov+blink, ed+blinkwatch_opera.com, rwlbuis, rune+blink
Project:
blink
Visibility:
Public.

Description

Invalidation set features recognized left of adjacent. Only recognize features (foundIdent) up until we see a relation different from SubSelector. After we allowed selectors with adjacent combinators to use invalidation sets, we started to acknowledge id/class/attribute to the left of adjacent combinators as features of the rightmost compound selector because we only looked for descendant type of selectors. For instance, the class 'b' below were recognized as an invalidation set feature in this selector: ".a .b + *". R=esprehn@chromium.org,chrishtr@chromium.org BUG=391244, 391193 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=180500

Patch Set 1 #

Patch Set 2 : Added more tests #

Total comments: 2

Patch Set 3 : Added two more tests. #

Total comments: 4

Patch Set 4 : Rebased #

Patch Set 5 : Extra test and documentation #

Total comments: 2

Patch Set 6 : Re-structured documentation #

Patch Set 7 : Rebased #

Unified diffs Side-by-side diffs Delta from patch set Stats (+138 lines, -21 lines) Patch
A LayoutTests/fast/css/invalidation/class-sibling-universal.html View 1 2 3 4 1 chunk +114 lines, -0 lines 0 comments Download
M Source/core/css/RuleFeature.cpp View 1 2 3 4 5 6 3 chunks +24 lines, -21 lines 0 comments Download

Messages

Total messages: 25 (0 generated)
rune
6 years, 5 months ago (2014-07-03 22:24:46 UTC) #1
rune
PTAL
6 years, 5 months ago (2014-07-07 19:06:40 UTC) #2
rune
6 years, 5 months ago (2014-07-09 09:41:00 UTC) #3
chrishtr
lgtm Sorry for the delay, there was a 4-day weekend and then I forgot about ...
6 years, 5 months ago (2014-07-09 16:21:33 UTC) #4
esprehn
Are you sure this is correct? If we don't check for descendant selectors then doing ...
6 years, 5 months ago (2014-07-09 16:28:24 UTC) #5
rune
On 2014/07/09 16:28:24, esprehn wrote: > Are you sure this is correct? If we don't ...
6 years, 5 months ago (2014-07-09 17:36:29 UTC) #6
chrishtr
Rune do we have sufficient test coverage for cases like this?
6 years, 5 months ago (2014-07-09 17:37:37 UTC) #7
rune
On 2014/07/09 17:37:37, chrishtr wrote: > Rune do we have sufficient test coverage for cases ...
6 years, 5 months ago (2014-07-09 17:53:56 UTC) #8
rune
On 2014/07/09 17:53:56, rune (futhark.irc - CET) wrote: > On 2014/07/09 17:37:37, chrishtr wrote: > ...
6 years, 5 months ago (2014-07-10 15:10:06 UTC) #9
rune
Added more tests.
6 years, 5 months ago (2014-07-10 21:21:23 UTC) #10
rune
On 2014/07/10 15:10:06, rune (futhark.irc - CET) wrote: > Looking at this, I found another ...
6 years, 5 months ago (2014-07-10 21:23:13 UTC) #11
rune
Any more issues?
6 years, 5 months ago (2014-07-11 22:27:17 UTC) #12
chrishtr
https://codereview.chromium.org/365063005/diff/20001/LayoutTests/fast/css/invalidation/class-sibling-universal.html File LayoutTests/fast/css/invalidation/class-sibling-universal.html (right): https://codereview.chromium.org/365063005/diff/20001/LayoutTests/fast/css/invalidation/class-sibling-universal.html#newcode5 LayoutTests/fast/css/invalidation/class-sibling-universal.html:5: .t1 .sibling + *, Please add one more test ...
6 years, 5 months ago (2014-07-12 00:11:59 UTC) #13
rune
https://codereview.chromium.org/365063005/diff/20001/LayoutTests/fast/css/invalidation/class-sibling-universal.html File LayoutTests/fast/css/invalidation/class-sibling-universal.html (right): https://codereview.chromium.org/365063005/diff/20001/LayoutTests/fast/css/invalidation/class-sibling-universal.html#newcode5 LayoutTests/fast/css/invalidation/class-sibling-universal.html:5: .t1 .sibling + *, On 2014/07/12 00:11:59, chrishtr wrote: ...
6 years, 4 months ago (2014-08-04 20:17:14 UTC) #14
chrishtr
https://codereview.chromium.org/365063005/diff/40001/LayoutTests/fast/css/invalidation/class-sibling-universal.html File LayoutTests/fast/css/invalidation/class-sibling-universal.html (right): https://codereview.chromium.org/365063005/diff/40001/LayoutTests/fast/css/invalidation/class-sibling-universal.html#newcode8 LayoutTests/fast/css/invalidation/class-sibling-universal.html:8: .t4 + .sibling, please add .t6 ~ .sibling also. ...
6 years, 4 months ago (2014-08-06 02:29:43 UTC) #15
rune
https://codereview.chromium.org/365063005/diff/40001/LayoutTests/fast/css/invalidation/class-sibling-universal.html File LayoutTests/fast/css/invalidation/class-sibling-universal.html (right): https://codereview.chromium.org/365063005/diff/40001/LayoutTests/fast/css/invalidation/class-sibling-universal.html#newcode8 LayoutTests/fast/css/invalidation/class-sibling-universal.html:8: .t4 + .sibling, On 2014/08/06 02:29:43, chrishtr wrote: > ...
6 years, 4 months ago (2014-08-07 10:06:45 UTC) #16
chrishtr
lgtm
6 years, 4 months ago (2014-08-07 15:42:15 UTC) #17
rune
On 2014/08/07 15:42:15, chrishtr wrote: > lgtm Thanks. Needs owner review.
6 years, 4 months ago (2014-08-08 07:43:14 UTC) #18
rune
6 years, 4 months ago (2014-08-13 07:45:56 UTC) #19
esprehn
I had a concern this system is becoming too complicated, that's usually a sign of ...
6 years, 4 months ago (2014-08-16 07:58:34 UTC) #20
rune
https://codereview.chromium.org/365063005/diff/80001/Source/core/css/RuleFeature.cpp File Source/core/css/RuleFeature.cpp (right): https://codereview.chromium.org/365063005/diff/80001/Source/core/css/RuleFeature.cpp#newcode284 Source/core/css/RuleFeature.cpp:284: // sibling subtree recalc in ContainerNode::checkForChildrenAdjacentRuleChanges. On 2014/08/16 07:58:34, ...
6 years, 4 months ago (2014-08-18 21:13:07 UTC) #21
rune
The CQ bit was checked by rune@opera.com
6 years, 4 months ago (2014-08-18 21:24:31 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rune@opera.com/365063005/120001
6 years, 4 months ago (2014-08-18 21:25:21 UTC) #23
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: mac_blink_compile_dbg on tryserver.blink ...
6 years, 4 months ago (2014-08-18 22:25:12 UTC) #24
commit-bot: I haz the power
6 years, 4 months ago (2014-08-18 23:08:19 UTC) #25
Message was sent while issue was closed.
Committed patchset #7 (120001) as 180500

Powered by Google App Engine
This is Rietveld 408576698