Fix the wrong usages of CSSSelectorList::selectorUsesXXX() functions

Fix the wrong usages of CSSSelectorList::selectorUsesXXX() functions

It looks that a wrong `selectorIndex` argument is passed to
CSSSelectorList::selectorUsesDeepCombinatorOrShadowPseudo(), and similar functions.

- The clients are passing the index of a *selector* in *selectors*, in the meaning used in the spec.
- The member functions are using this parameter as the index of  CSSSelecorList::m_selectorArray.

They are totally different if a selector list has more than one selectors.

This CL moved all functions to CSSSelector to prevent the wrong usage from happening
in the future. It is error-prone that CSSSelectorList has these functions.

This CL also fixes bug 643316.

BUG=643316
Committed: https://crrev.com/78be6354d3b13dff165e2715957bab3a6d158098
Cr-Commit-Position: refs/heads/master@{#416214}

[hayato, 2016-09-02 06:13:41]

The CQ bit was checked by hayato@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-02 06:13:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hayato, 2016-09-02 06:25:43]

Description was changed from

==========
Fix the broken CSSSelectorList::usesXXXX()

BUG=643316
==========

to

==========
Fix the wrong usage of CSSSelectorList::selectorUsesXXX() functions

It looks that the wrong `selectorIndex` argument is passed to
CSSSelectorList::selectorUsesDeepCombinatorOrShadowPseudo(), and similar
functions,
based on the wrong assumption.

- The clients are passing the index of a selector in selectors.
- The member functions are using this parameter as the index of a CSSSelector in
the CSSSelecorList.

This CL fixes that, and moves all functions to CSSSelector because
it is error-prone that CSSSelectorList has these functions.

BUG=643316
==========

[hayato, 2016-09-02 06:26:05]

Description was changed from

==========
Fix the wrong usage of CSSSelectorList::selectorUsesXXX() functions

It looks that the wrong `selectorIndex` argument is passed to
CSSSelectorList::selectorUsesDeepCombinatorOrShadowPseudo(), and similar
functions,
based on the wrong assumption.

- The clients are passing the index of a selector in selectors.
- The member functions are using this parameter as the index of a CSSSelector in
the CSSSelecorList.

This CL fixes that, and moves all functions to CSSSelector because
it is error-prone that CSSSelectorList has these functions.

BUG=643316
==========

to

==========
Fix the wrong usage of CSSSelectorList::selectorUsesXXX() functions

It looks that the wrong `selectorIndex` argument is passed to
CSSSelectorList::selectorUsesDeepCombinatorOrShadowPseudo(), and similar
functions,
based on the wrong assumption.

- The clients are passing the index of a selector in selectors.
- The member functions are using this parameter as the index of a CSSSelector in
the CSSSelecorList.

This CL fixes that, and moves all functions to CSSSelector because
it is error-prone that CSSSelectorList has these functions.

BUG=643316
==========

[hayato, 2016-09-02 06:26:41]

Description was changed from

==========
Fix the wrong usage of CSSSelectorList::selectorUsesXXX() functions

It looks that the wrong `selectorIndex` argument is passed to
CSSSelectorList::selectorUsesDeepCombinatorOrShadowPseudo(), and similar
functions,
based on the wrong assumption.

- The clients are passing the index of a selector in selectors.
- The member functions are using this parameter as the index of a CSSSelector in
the CSSSelecorList.

This CL fixes that, and moves all functions to CSSSelector because
it is error-prone that CSSSelectorList has these functions.

BUG=643316
==========

to

==========
Fix the wrong usage of CSSSelectorList::selectorUsesXXX() functions

It looks that a wrong `selectorIndex` argument is passed to
CSSSelectorList::selectorUsesDeepCombinatorOrShadowPseudo(), and similar
functions.

- The clients are passing the index of a selector in selectors.
- The member functions are using this parameter as the index of a CSSSelector in
the CSSSelecorList.

This CL fixes that, and moves all functions to CSSSelector because
it is error-prone that CSSSelectorList has these functions.

BUG=643316
==========

[hayato, 2016-09-02 06:27:41]

hayato@chromium.org changed reviewers:
+ kochi@chromium.org, rune@opera.com, tkent@chromium.org

[hayato, 2016-09-02 06:27:42]

PTAL

[hayato, 2016-09-02 06:30:47]

Add a test

[hayato, 2016-09-02 06:30:48]

The CQ bit was checked by hayato@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-02 06:31:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hayato, 2016-09-02 06:31:43]

https://codereview.chromium.org/2306903002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSSelectorList.cpp (left):

https://codereview.chromium.org/2306903002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSSelectorList.cpp:145: static bool
forEachSelector(const Functor& functor, const CSSSelectorList* selectorList)
This template is not used.

[hayato, 2016-09-02 06:48:02]

Description was changed from

==========
Fix the wrong usage of CSSSelectorList::selectorUsesXXX() functions

It looks that a wrong `selectorIndex` argument is passed to
CSSSelectorList::selectorUsesDeepCombinatorOrShadowPseudo(), and similar
functions.

- The clients are passing the index of a selector in selectors.
- The member functions are using this parameter as the index of a CSSSelector in
the CSSSelecorList.

This CL fixes that, and moves all functions to CSSSelector because
it is error-prone that CSSSelectorList has these functions.

BUG=643316
==========

to

==========
Fix the wrong usages of CSSSelectorList::selectorUsesXXX() functions

It looks that a wrong `selectorIndex` argument is passed to
CSSSelectorList::selectorUsesDeepCombinatorOrShadowPseudo(), and similar
functions.

- The clients are passing the index of a selector in selectors.
- The member functions are using this parameter as the index of a CSSSelector in
the CSSSelecorList.

This CL fixes that, and moves all functions to CSSSelector because
it is error-prone that CSSSelectorList has these functions.

BUG=643316
==========

[hayato, 2016-09-02 06:48:38]

Description was changed from

==========
Fix the wrong usages of CSSSelectorList::selectorUsesXXX() functions

It looks that a wrong `selectorIndex` argument is passed to
CSSSelectorList::selectorUsesDeepCombinatorOrShadowPseudo(), and similar
functions.

- The clients are passing the index of a selector in selectors.
- The member functions are using this parameter as the index of a CSSSelector in
the CSSSelecorList.

This CL fixes that, and moves all functions to CSSSelector because
it is error-prone that CSSSelectorList has these functions.

BUG=643316
==========

to

==========
Fix the wrong usages of CSSSelectorList::selectorUsesXXX() functions

It looks that a wrong `selectorIndex` argument is passed to
CSSSelectorList::selectorUsesDeepCombinatorOrShadowPseudo(), and similar
functions.

- The clients are passing the index of a *selector* in *selectors*, in the sense
of the spec.
- The member functions are using this parameter as the index of a CSSSelector in
the CSSSelecorList.

This CL fixes that, and moves all functions to CSSSelector because
it is error-prone that CSSSelectorList has these functions.

BUG=643316
==========

[hayato, 2016-09-02 06:49:31]

Description was changed from

==========
Fix the wrong usages of CSSSelectorList::selectorUsesXXX() functions

It looks that a wrong `selectorIndex` argument is passed to
CSSSelectorList::selectorUsesDeepCombinatorOrShadowPseudo(), and similar
functions.

- The clients are passing the index of a *selector* in *selectors*, in the sense
of the spec.
- The member functions are using this parameter as the index of a CSSSelector in
the CSSSelecorList.

This CL fixes that, and moves all functions to CSSSelector because
it is error-prone that CSSSelectorList has these functions.

BUG=643316
==========

to

==========
Fix the wrong usages of CSSSelectorList::selectorUsesXXX() functions

It looks that a wrong `selectorIndex` argument is passed to
CSSSelectorList::selectorUsesDeepCombinatorOrShadowPseudo(), and similar
functions.

- The clients are passing the index of a *selector* in *selectors*, in the
meaning used in the spec.
- The member functions are using this parameter as the index of a CSSSelector in
the CSSSelecorList.

This CL fixes that, and moves all functions to CSSSelector because
it is error-prone that CSSSelectorList has these functions.

BUG=643316
==========

[hayato, 2016-09-02 06:50:56]

Description was changed from

==========
Fix the wrong usages of CSSSelectorList::selectorUsesXXX() functions

It looks that a wrong `selectorIndex` argument is passed to
CSSSelectorList::selectorUsesDeepCombinatorOrShadowPseudo(), and similar
functions.

- The clients are passing the index of a *selector* in *selectors*, in the
meaning used in the spec.
- The member functions are using this parameter as the index of a CSSSelector in
the CSSSelecorList.

This CL fixes that, and moves all functions to CSSSelector because
it is error-prone that CSSSelectorList has these functions.

BUG=643316
==========

to

==========
Fix the wrong usages of CSSSelectorList::selectorUsesXXX() functions

It looks that a wrong `selectorIndex` argument is passed to
CSSSelectorList::selectorUsesDeepCombinatorOrShadowPseudo(), and similar
functions.

- The clients are passing the index of a *selector* in *selectors*, in the
meaning used in the spec.
- The member functions are using this parameter as the index of 
CSSSelecorList::m_selectorArray.

They are totally different.

This CL fixes that, and moves all functions to CSSSelector because
it is error-prone that CSSSelectorList has these functions.

BUG=643316
==========

[hayato, 2016-09-02 06:51:23]

Description was changed from

==========
Fix the wrong usages of CSSSelectorList::selectorUsesXXX() functions

It looks that a wrong `selectorIndex` argument is passed to
CSSSelectorList::selectorUsesDeepCombinatorOrShadowPseudo(), and similar
functions.

- The clients are passing the index of a *selector* in *selectors*, in the
meaning used in the spec.
- The member functions are using this parameter as the index of 
CSSSelecorList::m_selectorArray.

They are totally different.

This CL fixes that, and moves all functions to CSSSelector because
it is error-prone that CSSSelectorList has these functions.

BUG=643316
==========

to

==========
Fix the wrong usages of CSSSelectorList::selectorUsesXXX() functions

It looks that a wrong `selectorIndex` argument is passed to
CSSSelectorList::selectorUsesDeepCombinatorOrShadowPseudo(), and similar
functions.

- The clients are passing the index of a *selector* in *selectors*, in the
meaning used in the spec.
- The member functions are using this parameter as the index of 
CSSSelecorList::m_selectorArray.

They are totally different if a selector list has more than one selectors.

This CL fixes that, and moves all functions to CSSSelector because
it is error-prone that CSSSelectorList has these functions.

BUG=643316
==========

[kochi, 2016-09-02 06:52:52]

nice find and nice refactoring.
lgtm % nits

Does this fix the case that the linked bug reports?
If so, could you improve the CL description about
where specifically used wrong argument (SelectorQuery.cpp
only?) and what went wrong?

https://codereview.chromium.org/2306903002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSSelector.cpp (right):

https://codereview.chromium.org/2306903002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSSelector.cpp:950: }, *this);
Wrong indent?
Please be consistent with other cases.

https://codereview.chromium.org/2306903002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/SelectorQuery.cpp (right):

https://codereview.chromium.org/2306903002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/SelectorQuery.cpp:116: unsigned index = 0;
You no longer need this index.

https://codereview.chromium.org/2306903002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/SelectorQuery.cpp:117: for (const
CSSSelector* selector = selectorList.first(); selector; selector =
CSSSelectorList::next(*selector), ++index) {
Remove '++index'

[hayato, 2016-09-02 07:07:57]

Description was changed from

==========
Fix the wrong usages of CSSSelectorList::selectorUsesXXX() functions

It looks that a wrong `selectorIndex` argument is passed to
CSSSelectorList::selectorUsesDeepCombinatorOrShadowPseudo(), and similar
functions.

- The clients are passing the index of a *selector* in *selectors*, in the
meaning used in the spec.
- The member functions are using this parameter as the index of 
CSSSelecorList::m_selectorArray.

They are totally different if a selector list has more than one selectors.

This CL fixes that, and moves all functions to CSSSelector because
it is error-prone that CSSSelectorList has these functions.

BUG=643316
==========

to

==========
Fix the wrong usages of CSSSelectorList::selectorUsesXXX() functions

It looks that a wrong `selectorIndex` argument is passed to
CSSSelectorList::selectorUsesDeepCombinatorOrShadowPseudo(), and similar
functions.

- The clients are passing the index of a *selector* in *selectors*, in the
meaning used in the spec.
- The member functions are using this parameter as the index of 
CSSSelecorList::m_selectorArray.

They are totally different if a selector list has more than one selectors.

This CL moved all functions to CSSSelector to prevent the wrong usage from
happening
in the future. It is error-prone that CSSSelectorList has these functions.

This CL also fixes bug 643316.

BUG=643316
==========

[hayato, 2016-09-02 07:10:12]

Fix

[hayato, 2016-09-02 07:10:14]

The CQ bit was checked by hayato@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-02 07:10:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hayato, 2016-09-02 07:11:29]

> Does this fix the case that the linked bug reports?

Yes.

> If so, could you improve the CL description about
where specifically used wrong argument (SelectorQuery.cpp
only?) and what went wrong?

Only SelectorQuery.cpp. I have updated the description.

https://codereview.chromium.org/2306903002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSSelector.cpp (right):

https://codereview.chromium.org/2306903002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSSelector.cpp:950: }, *this);
Done

https://codereview.chromium.org/2306903002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/SelectorQuery.cpp (right):

https://codereview.chromium.org/2306903002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/SelectorQuery.cpp:116: unsigned index = 0;
Done

https://codereview.chromium.org/2306903002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/SelectorQuery.cpp:117: for (const
CSSSelector* selector = selectorList.first(); selector; selector =
CSSSelectorList::next(*selector), ++index) {
Done

[hayato, 2016-09-02 07:12:00]

The CQ bit was unchecked by hayato@chromium.org

[hayato, 2016-09-02 07:12:04]

The CQ bit was checked by hayato@chromium.org

[hayato, 2016-09-02 07:12:05]

The patchset sent to the CQ was uploaded after l-g-t-m from kochi@chromium.org
Link to the patchset: https://codereview.chromium.org/2306903002/#ps40001
(title: "Fix")

[commit-bot: I haz the power, 2016-09-02 07:12:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hayato, 2016-09-02 07:16:33]

Note: It looks I forgot to upload a layout test in the previous patch set. The
latest patch set includes it.

[kochi, 2016-09-02 07:20:19]

Adding a test case makes sense very much, now it is clear
that this CL fixes the case for querySelector() which has
multiple selectors.

lgtm

[commit-bot: I haz the power, 2016-09-02 08:43:43]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-02 08:45:34]

Description was changed from

==========
Fix the wrong usages of CSSSelectorList::selectorUsesXXX() functions

It looks that a wrong `selectorIndex` argument is passed to
CSSSelectorList::selectorUsesDeepCombinatorOrShadowPseudo(), and similar
functions.

- The clients are passing the index of a *selector* in *selectors*, in the
meaning used in the spec.
- The member functions are using this parameter as the index of 
CSSSelecorList::m_selectorArray.

They are totally different if a selector list has more than one selectors.

This CL moved all functions to CSSSelector to prevent the wrong usage from
happening
in the future. It is error-prone that CSSSelectorList has these functions.

This CL also fixes bug 643316.

BUG=643316
==========

to

==========
Fix the wrong usages of CSSSelectorList::selectorUsesXXX() functions

It looks that a wrong `selectorIndex` argument is passed to
CSSSelectorList::selectorUsesDeepCombinatorOrShadowPseudo(), and similar
functions.

- The clients are passing the index of a *selector* in *selectors*, in the
meaning used in the spec.
- The member functions are using this parameter as the index of 
CSSSelecorList::m_selectorArray.

They are totally different if a selector list has more than one selectors.

This CL moved all functions to CSSSelector to prevent the wrong usage from
happening
in the future. It is error-prone that CSSSelectorList has these functions.

This CL also fixes bug 643316.

BUG=643316

Committed: https://crrev.com/78be6354d3b13dff165e2715957bab3a6d158098
Cr-Commit-Position: refs/heads/master@{#416214}
==========

[commit-bot: I haz the power, 2016-09-02 08:45:36]

Patchset 3 (id:??) landed as
https://crrev.com/78be6354d3b13dff165e2715957bab3a6d158098
Cr-Commit-Position: refs/heads/master@{#416214}

[rune, 2016-09-02 09:14:32]

https://codereview.chromium.org/2306903002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSSelector.cpp (right):

https://codereview.chromium.org/2306903002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSSelector.cpp:916: static bool
forEachTagSelector(const Functor& functor, const CSSSelector& selector)
This is a weird name. It sounds like it's looking at "Tag" (type?) selectors
only. But it's in fact looking at every simple selector.

https://codereview.chromium.org/2306903002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/RuleSet.cpp (left):

https://codereview.chromium.org/2306903002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/RuleSet.cpp:260: for (size_t selectorIndex =
0; selectorIndex != kNotFound; selectorIndex =
selectorList.indexOfNextSelectorAfter(selectorIndex)) {
SelectorList::next() would be a better choice, I think.
indexOfNextSelectorAfter() calls next(), and is a convenience method for the
RuleData code which stores the selectorIndex.

[hayato, 2016-09-05 02:36:11]

https://codereview.chromium.org/2306903002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSSelector.cpp (right):

https://codereview.chromium.org/2306903002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSSelector.cpp:916: static bool
forEachTagSelector(const Functor& functor, const CSSSelector& selector)
On 2016/09/02 at 09:14:31, rune wrote:
> This is a weird name. It sounds like it's looking at "Tag" (type?) selectors
only. But it's in fact looking at every simple selector.

I think this name is coming from CSSSelector::m_tagHistory. Using a simple
selector here might be confusing here because what it does is traversing
CSSSelector's internal structures, which is slightly different for traversing
each simple selector.

Anyway, let me rename this to forEachTagHistory since it reflects more
accurately.

https://codereview.chromium.org/2306903002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/RuleSet.cpp (left):

https://codereview.chromium.org/2306903002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/RuleSet.cpp:260: for (size_t selectorIndex =
0; selectorIndex != kNotFound; selectorIndex =
selectorList.indexOfNextSelectorAfter(selectorIndex)) {
On 2016/09/02 at 09:14:31, rune wrote:
> SelectorList::next() would be a better choice, I think.
indexOfNextSelectorAfter() calls next(), and is a convenience method for the
RuleData code which stores the selectorIndex.

Yeah, but we still need a selectorIndex here because we call addRule here. Let
me try to use next() here anyway in another CL.

[rune, 2016-09-05 07:46:39]

https://codereview.chromium.org/2306903002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/RuleSet.cpp (left):

https://codereview.chromium.org/2306903002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/RuleSet.cpp:260: for (size_t selectorIndex =
0; selectorIndex != kNotFound; selectorIndex =
selectorList.indexOfNextSelectorAfter(selectorIndex)) {
On 2016/09/05 02:36:11, hayato wrote:
> On 2016/09/02 at 09:14:31, rune wrote:
> > SelectorList::next() would be a better choice, I think.
> indexOfNextSelectorAfter() calls next(), and is a convenience method for the
> RuleData code which stores the selectorIndex.
> 
> Yeah, but we still need a selectorIndex here because we call addRule here. Let
> me try to use next() here anyway in another CL.

Ah. I didn't notice. Just leave it like this then.