Separate method for adding features to an invalidation set.

Separate method for adding features to an invalidation set.

Tried to improve readability. Also re-arranged the for-loop a bit.

This CL is part of splitting up [1].

[1] https://codereview.chromium.org/639433002

R=chrishtr@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183633

[chrishtr, 2014-10-13 15:44:31]

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

https://codereview.chromium.org/652703002/diff/1/Source/core/css/RuleFeature....
Source/core/css/RuleFeature.cpp:322: if (current->relation() ==
CSSSelector::SubSelector)
Why is this continue needed here? Is it because we need to avoid setting
adjacent to false after setting it to true earlier?

https://codereview.chromium.org/652703002/diff/1/Source/core/css/RuleFeature....
Source/core/css/RuleFeature.cpp:325: if (current->isShadowSelector())
Is isShadowSelector() checked in?

[rune, 2014-10-13 21:42:57]

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

https://codereview.chromium.org/652703002/diff/1/Source/core/css/RuleFeature....
Source/core/css/RuleFeature.cpp:322: if (current->relation() ==
CSSSelector::SubSelector)
On 2014/10/13 15:44:31, chrishtr wrote:
> Why is this continue needed here? Is it because we need to avoid setting
> adjacent to false after setting it to true earlier?

Yes, without this "continue", we'd have to check for != SubSelector before
setting adjacent. We can keep the old code. I thought this looked clearer.

https://codereview.chromium.org/652703002/diff/1/Source/core/css/RuleFeature....
Source/core/css/RuleFeature.cpp:325: if (current->isShadowSelector())
On 2014/10/13 15:44:31, chrishtr wrote:
> Is isShadowSelector() checked in?

I don't understand.

[chrishtr, 2014-10-13 22:00:25]

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

https://codereview.chromium.org/652703002/diff/1/Source/core/css/RuleFeature....
Source/core/css/RuleFeature.cpp:322: if (current->relation() ==
CSSSelector::SubSelector)
On 2014/10/13 at 21:42:57, rune wrote:
> On 2014/10/13 15:44:31, chrishtr wrote:
> > Why is this continue needed here? Is it because we need to avoid setting
> > adjacent to false after setting it to true earlier?
> 
> Yes, without this "continue", we'd have to check for != SubSelector before
setting adjacent. We can keep the old code. I thought this looked clearer.

Let me put it in a different way: what would be the appropriate comment to put
before 322 to explain to readers why we short-circuit for subselector? That
would be a nice comment to add. (I actually don't have it in my head right now
either, so want to know.)

https://codereview.chromium.org/652703002/diff/1/Source/core/css/RuleFeature....
Source/core/css/RuleFeature.cpp:325: if (current->isShadowSelector())
On 2014/10/13 at 21:42:57, rune wrote:
> On 2014/10/13 15:44:31, chrishtr wrote:
> > Is isShadowSelector() checked in?
> 
> I don't understand.

Nevermind. I tried codesearching for this method earlier to understand it and
failed to find it. Can find it now.

[rune, 2014-10-13 22:08:09]

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

https://codereview.chromium.org/652703002/diff/1/Source/core/css/RuleFeature....
Source/core/css/RuleFeature.cpp:322: if (current->relation() ==
CSSSelector::SubSelector)
On 2014/10/13 22:00:25, chrishtr wrote:
> On 2014/10/13 at 21:42:57, rune wrote:
> > On 2014/10/13 15:44:31, chrishtr wrote:
> > > Why is this continue needed here? Is it because we need to avoid setting
> > > adjacent to false after setting it to true earlier?
> > 
> > Yes, without this "continue", we'd have to check for != SubSelector before
> setting adjacent. We can keep the old code. I thought this looked clearer.
> 
> Let me put it in a different way: what would be the appropriate comment to put
> before 322 to explain to readers why we short-circuit for subselector? That
> would be a nice comment to add. (I actually don't have it in my head right now
> either, so want to know.)

Something like:

// No combinator to update flags for.

adjacent can be set unconditionally, since it's flipping true/false for all
combinators, but not for subselectors.

[chrishtr, 2014-10-13 22:10:46]

lgtm

[chrishtr, 2014-10-13 22:10:55]

Thanks.

[rune, 2014-10-13 23:05:16]

On 2014/10/13 at 22:10:46, chrishtr wrote:
> lgtm

Did you want me to add the comment?

[chrishtr, 2014-10-13 23:07:24]

Not necessary.

[rune, 2014-10-13 23:18:30]

The CQ bit was checked by rune@opera.com

[commit-bot: I haz the power, 2014-10-13 23:19:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/652703002/1

[commit-bot: I haz the power, 2014-10-14 00:33:13]

Committed patchset #1 (id:1) as 183633