Invalidation set features recognized left of adjacent.

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

[rune, 2014-07-07 19:06:40]

PTAL

[chrishtr, 2014-07-09 16:21:33]

lgtm

Sorry for the delay, there was a 4-day weekend and then I forgot about the CL
temporarily. Thanks for reminding me.

[esprehn, 2014-07-09 16:28:24]

Are you sure this is correct? If we don't check for descendant selectors then
doing .a + .b will put an invalidation set on ".a" and not do a
SubtreeStyleChange which will then make it not match .b?

[rune, 2014-07-09 17:36:29]

On 2014/07/09 16:28:24, esprehn wrote:
> Are you sure this is correct? If we don't check for descendant selectors then
> doing .a + .b will put an invalidation set on ".a" and not do a
> SubtreeStyleChange which will then make it not match .b?

This is the invalidationSetModeForSelector() method which checks if we should
use try add invalidation set features at all. foundIdent is to be set to true if
a feature added to the invalidation set is found in the rightmost compound
selector, so any combinator should stop setting foundIdent. The descendant vs
adjacent combinator behavior is done in the addFeaturesToInvalidationSets
method.

[chrishtr, 2014-07-09 17:37:37]

Rune do we have sufficient test coverage for cases like this?

[rune, 2014-07-09 17:53:56]

On 2014/07/09 17:37:37, chrishtr wrote:
> Rune do we have sufficient test coverage for cases like this?

for "... + *", apparently not.

These were the selectors with adjacent combinators in fast/css/invalidation
before this fix:

.c5 > .c4 ~ .c3 .c2 + .c1
#t1 div + .test
.a2 div + #b2
.a2 span + div

Should probably go through to cover various cases more extensively.

[rune, 2014-07-10 15:10:06]

On 2014/07/09 17:53:56, rune (futhark.irc - CET) wrote:
> On 2014/07/09 17:37:37, chrishtr wrote:
> > Rune do we have sufficient test coverage for cases like this?
> 
> for "... + *", apparently not.
> 
> These were the selectors with adjacent combinators in fast/css/invalidation
> before this fix:
> 
> .c5 > .c4 ~ .c3 .c2 + .c1
> #t1 div + .test
> .a2 div + #b2
> .a2 span + div
> 
> Should probably go through to cover various cases more extensively.

Looking at this, I found another issue:

https://code.google.com/p/chromium/issues/detail?id=392852

[rune, 2014-07-10 21:21:23]

Added more tests.

[rune, 2014-07-10 21:23:13]

On 2014/07/10 15:10:06, rune (futhark.irc - CET) wrote:

> Looking at this, I found another issue:
> 
> https://code.google.com/p/chromium/issues/detail?id=392852

Up for review here:

https://codereview.chromium.org/383833002/

[rune, 2014-07-11 22:27:17]

Any more issues?

[chrishtr, 2014-07-12 00:11:59]

https://codereview.chromium.org/365063005/diff/20001/LayoutTests/fast/css/inv...
File LayoutTests/fast/css/invalidation/class-sibling-universal.html (right):

https://codereview.chromium.org/365063005/diff/20001/LayoutTests/fast/css/inv...
LayoutTests/fast/css/invalidation/class-sibling-universal.html:5: .t1 .sibling +
*,
Please add one more test like what Elliott suggested, such as a simple .a + .b
{} selector, to test behavior of the adjacent operator in the right-most
compound selector. Also maybe .a + * {} ?

[rune, 2014-08-04 20:17:14]

https://codereview.chromium.org/365063005/diff/20001/LayoutTests/fast/css/inv...
File LayoutTests/fast/css/invalidation/class-sibling-universal.html (right):

https://codereview.chromium.org/365063005/diff/20001/LayoutTests/fast/css/inv...
LayoutTests/fast/css/invalidation/class-sibling-universal.html:5: .t1 .sibling +
*,
On 2014/07/12 00:11:59, chrishtr wrote:
> Please add one more test like what Elliott suggested, such as a simple .a + .b
> {} selector, to test behavior of the adjacent operator in the right-most
> compound selector. Also maybe .a + * {} ?

Done.

[chrishtr, 2014-08-06 02:29:43]

https://codereview.chromium.org/365063005/diff/40001/LayoutTests/fast/css/inv...
File LayoutTests/fast/css/invalidation/class-sibling-universal.html (right):

https://codereview.chromium.org/365063005/diff/40001/LayoutTests/fast/css/inv...
LayoutTests/fast/css/invalidation/class-sibling-universal.html:8: .t4 +
.sibling,
please add .t6 ~ .sibling also.

https://codereview.chromium.org/365063005/diff/40001/Source/core/css/RuleFeat...
File Source/core/css/RuleFeature.cpp (right):

https://codereview.chromium.org/365063005/diff/40001/Source/core/css/RuleFeat...
Source/core/css/RuleFeature.cpp:223: features.wholeSubtree = true;
My understanding is that wholeSubtree is necessary here because of this line of
code?

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Please document if so.

[rune, 2014-08-07 10:06:45]

https://codereview.chromium.org/365063005/diff/40001/LayoutTests/fast/css/inv...
File LayoutTests/fast/css/invalidation/class-sibling-universal.html (right):

https://codereview.chromium.org/365063005/diff/40001/LayoutTests/fast/css/inv...
LayoutTests/fast/css/invalidation/class-sibling-universal.html:8: .t4 +
.sibling,
On 2014/08/06 02:29:43, chrishtr wrote:
> please add .t6 ~ .sibling also.

Done.

https://codereview.chromium.org/365063005/diff/40001/Source/core/css/RuleFeat...
File Source/core/css/RuleFeature.cpp (right):

https://codereview.chromium.org/365063005/diff/40001/Source/core/css/RuleFeat...
Source/core/css/RuleFeature.cpp:223: features.wholeSubtree = true;
On 2014/08/06 02:29:43, chrishtr wrote:
> My understanding is that wholeSubtree is necessary here because of this line
of
> code?
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> Please document if so.

Done.

[chrishtr, 2014-08-07 15:42:15]

lgtm

[rune, 2014-08-08 07:43:14]

On 2014/08/07 15:42:15, chrishtr wrote:
> lgtm

Thanks. Needs owner review.

[esprehn, 2014-08-16 07:58:34]

I had a concern this system is becoming too complicated, that's usually a sign
of what's happening when we start leaving big comments inside the functions. How
can we make this simpler?

lgtm

https://codereview.chromium.org/365063005/diff/80001/Source/core/css/RuleFeat...
File Source/core/css/RuleFeature.cpp (right):

https://codereview.chromium.org/365063005/diff/80001/Source/core/css/RuleFeat...
Source/core/css/RuleFeature.cpp:284: // sibling subtree recalc in
ContainerNode::checkForChildrenAdjacentRuleChanges.
Not a fan of the copy pasta comments. Can you just put a comment above the
functions and explain what they do instead?

[rune, 2014-08-18 21:13:07]

https://codereview.chromium.org/365063005/diff/80001/Source/core/css/RuleFeat...
File Source/core/css/RuleFeature.cpp (right):

https://codereview.chromium.org/365063005/diff/80001/Source/core/css/RuleFeat...
Source/core/css/RuleFeature.cpp:284: // sibling subtree recalc in
ContainerNode::checkForChildrenAdjacentRuleChanges.
On 2014/08/16 07:58:34, esprehn wrote:
> Not a fan of the copy pasta comments. Can you just put a comment above the
> functions and explain what they do instead?

Done.

[rune, 2014-08-18 21:24:31]

The CQ bit was checked by rune@opera.com

[commit-bot: I haz the power, 2014-08-18 21:25:21]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rune@opera.com/365063005/120001

[commit-bot: I haz the power, 2014-08-18 22:25:12]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)

[commit-bot: I haz the power, 2014-08-18 23:08:19]

Committed patchset #7 (120001) as 180500