Reject pseudo class inside :not()

Reject pseudo class inside :not()

The current behavior, pseudo classes inside :not() work when they used in the <content select=””> context,  however, it does not satisfy the matching criteria.
This patch reject them.


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

[yurina1, 2014-09-08 11:18:40]

yurina@chromium.org changed reviewers:
+ hayato@chromium.org, yanagawa@chromium.org

[yurina1, 2014-09-09 00:50:31]

Hi hayato-san,
Could you review this patch?

[hayato, 2014-09-09 05:58:32]

Thank you for the contribution.

https://codereview.chromium.org/547363002/diff/1/LayoutTests/fast/dom/shadow/...
File LayoutTests/fast/dom/shadow/content-selector-query.html (right):

https://codereview.chromium.org/547363002/diff/1/LayoutTests/fast/dom/shadow/...
LayoutTests/fast/dom/shadow/content-selector-query.html:54: 'div:first-of-type,
div', 'div, div:first-of-type', 'div:first-of-type, div:last-of-type',
'div:not(:not(div))', 'div:not(:hover)',
You might want to remove a space at the end of the line.

https://codereview.chromium.org/547363002/diff/1/LayoutTests/fast/dom/shadow/...
LayoutTests/fast/dom/shadow/content-selector-query.html:54: 'div:first-of-type,
div', 'div, div:first-of-type', 'div:first-of-type, div:last-of-type',
'div:not(:not(div))', 'div:not(:hover)',
Could you add a test where a selector which is not a simple selector is used
inside of the argument of `:not()`?

For example, 'div:not(div div)', 'div:not(div div:not)' and so on.
I am afraid that your code can't handle these cases. Could you try that?

[yurina1, 2014-09-09 06:36:12]

Patchset #2 (id:20001) has been deleted

[yurina1, 2014-09-09 06:47:50]

Thank you for the comment. I added some tests.

[hayato, 2014-09-10 06:29:11]

LGTM.

Congrats for the first patch. You can land this patch.

https://codereview.chromium.org/547363002/diff/40001/Source/core/html/HTMLCon...
File Source/core/html/HTMLContentElement.cpp (right):

https://codereview.chromium.org/547363002/diff/40001/Source/core/html/HTMLCon...
Source/core/html/HTMLContentElement.cpp:86: const CSSSelector* subSelector =
selector.selectorList()->first();
It looks that current implementation of ':not()' can take only a simple
selector. Therefore, it is enough to check the first(). I understand.

According to the latest spec, http://dev.w3.org/csswg/selectors4/#negation,
':not()' can take a selector list as an argument. That means we should update
the code around here in the future if we support a selector list in an argument
of ':not()'.

I think the layout test you added will catch this kind of errors in the future.
I think we can land this patch as is.

https://codereview.chromium.org/547363002/diff/40001/Source/core/html/HTMLCon...
Source/core/html/HTMLContentElement.cpp:89: return selector.match() ==
CSSSelector::PseudoClass && selector.pseudoType() != CSSSelector::PseudoNot;
You can remove `selector.pseudoType() != CSSSelector::PseudoNot` from here
because this is always satisfied thanks to the previous `if ` statement (line
85).

[yurina1, 2014-09-10 06:40:18]

The CQ bit was checked by yurina@chromium.org

[yurina1, 2014-09-10 06:40:19]

The CQ bit was unchecked by yurina@chromium.org

[yurina1, 2014-09-10 11:02:53]

https://codereview.chromium.org/547363002/diff/40001/Source/core/html/HTMLCon...
File Source/core/html/HTMLContentElement.cpp (right):

https://codereview.chromium.org/547363002/diff/40001/Source/core/html/HTMLCon...
Source/core/html/HTMLContentElement.cpp:89: return selector.match() ==
CSSSelector::PseudoClass && selector.pseudoType() != CSSSelector::PseudoNot;
On 2014/09/10 06:29:11, hayato wrote:
> You can remove `selector.pseudoType() != CSSSelector::PseudoNot` from here
> because this is always satisfied thanks to the previous `if ` statement (line
> 85).

Done.

[yurina1, 2014-09-10 11:02:58]

The CQ bit was checked by yurina@chromium.org

[commit-bot: I haz the power, 2014-09-10 11:03:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yurina@chromium.org/547363002/60001

[commit-bot: I haz the power, 2014-09-10 11:06:38]

Committed patchset #3 (id:60001) as 181726