The query selector for HTMLContentElement should follow the shadow dom spec.

• Component: DOM
• Status: RESOLVED FIXED

[Shinya Kawanaka]

The 'select' attribute introduced in the patch for Bug 75302 will accept wider query than shadow dom spec.
The query format should obey the spec.

[Shinya Kawanaka]

Spec is here. http://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html

[Shinya Kawanaka]

Created attachment 123257 [details]
Patch

[Hajime Morrita]

Comment on attachment 123257 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=123257&action=review

The approach looks OK. Added some comments inline.

> Source/WebCore/html/shadow/ContentSelectorQuery.cpp:48
>      parser.parseSelector(element->select(), element->document(), m_selectorList);

Is there any way to detect syntax error?

> Source/WebCore/html/shadow/ContentSelectorQuery.cpp:57
> +    if (m_contentElement->select().isNull() || m_contentElement->select().isEmpty())

It looks we don't need this - this check was done in the constructor.

> Source/WebCore/html/shadow/ContentSelectorQuery.cpp:118
> +    for (size_t i = 0; i < sizeof(allowedPseudoType) / sizeof(allowedPseudoType[0]); ++i) {

Why not switch?

> Source/WebCore/html/shadow/ContentSelectorQuery.h:60
> +    bool m_isValidSelector;

Looks something is wrong.... This should be a property of SelectorDataList or CSSSelectorList?
or should validateSelectorList() be a method of SelectorDataList?
I'm not sure. but ownership around this class looks strange.

> Source/WebCore/html/shadow/HTMLContentElement.h:65
> +    virtual bool hasValidSelect() const;

isSelectValid?

> Source/WebCore/testing/Internals.cpp:153
> +    if (!contentElement->isContentElement()) {

Please check null-ity of contentElement

> LayoutTests/fast/dom/shadow/content-selector-query.html:44
> +var dataOfInvalidCases = [

Could you add some syntactically invalid cases?
There are, for example, forbidden symbols.
We also need partially valid case which has both syntactically valid and invalid selectors.

[Shinya Kawanaka]

Created attachment 123519 [details]
Patch

[Shinya Kawanaka]

(In reply to comment #3)
> (From update of attachment 123257 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123257&action=review
> 
> The approach looks OK. Added some comments inline.
> 
> > Source/WebCore/html/shadow/ContentSelectorQuery.cpp:48
> >      parser.parseSelector(element->select(), element->document(), m_selectorList);
> 
> Is there any way to detect syntax error?

Ah... I have added a code to check m_selectorList->first() is empty or not.
Empty means the CSSSelector fails parsing it.

> 
> > Source/WebCore/html/shadow/ContentSelectorQuery.cpp:57
> > +    if (m_contentElement->select().isNull() || m_contentElement->select().isEmpty())
> 
> It looks we don't need this - this check was done in the constructor.

Done.

> 
> > Source/WebCore/html/shadow/ContentSelectorQuery.cpp:118
> > +    for (size_t i = 0; i < sizeof(allowedPseudoType) / sizeof(allowedPseudoType[0]); ++i) {
> 
> Why not switch?

Done.

> 
> > Source/WebCore/html/shadow/ContentSelectorQuery.h:60
> > +    bool m_isValidSelector;
> 
> Looks something is wrong.... This should be a property of SelectorDataList or CSSSelectorList?
> or should validateSelectorList() be a method of SelectorDataList?
> I'm not sure. but ownership around this class looks strange.

Hmm...
Since SelectorDataList and CSSSelectorList are borrowed from CSSParser.
The selector query is specific for HTMLContentElement, I don't think CSSParser related objects have it.
So I've added m_isValidSelector there.

I leave this code as is in Patch 2.

> 
> > Source/WebCore/html/shadow/HTMLContentElement.h:65
> > +    virtual bool hasValidSelect() const;
> 
> isSelectValid?

Done.

> 
> > Source/WebCore/testing/Internals.cpp:153
> > +    if (!contentElement->isContentElement()) {
> 
> Please check null-ity of contentElement

Done.
> 
> > LayoutTests/fast/dom/shadow/content-selector-query.html:44
> > +var dataOfInvalidCases = [
> 
> Could you add some syntactically invalid cases?
> There are, for example, forbidden symbols.
> We also need partially valid case which has both syntactically valid and invalid selectors.

Yes. I've added several (mainly syntax-check) test cases, which contains failing cases in my previous patch.

[Early Warning System Bot]

Comment on attachment 123519 [details]
Patch

Attachment 123519 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11253823

[WebKit Review Bot]

Comment on attachment 123519 [details]
Patch

Attachment 123519 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11253828

New failing tests:
fast/dom/shadow/shadow-contents-fallback.html
fast/html/details-add-child-2.html
fast/html/details-replace-text.html
fast/dom/shadow/content-selector-query.html
fast/html/details-open2.html
fast/html/details-remove-child-2.html
fast/dom/shadow/shadow-contents-select.html

[Shinya Kawanaka]

(In reply to comment #7)
> (From update of attachment 123519 [details])
> Attachment 123519 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/11253828
> 
> New failing tests:
> fast/dom/shadow/shadow-contents-fallback.html
> fast/html/details-add-child-2.html
> fast/html/details-replace-text.html
> fast/dom/shadow/content-selector-query.html
> fast/html/details-open2.html
> fast/html/details-remove-child-2.html
> fast/dom/shadow/shadow-contents-select.html

Hmm... Let me try fix them.

[Shinya Kawanaka]

Created attachment 123524 [details]
Patch

[Shinya Kawanaka]

(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 123519 [details] [details])
> > Attachment 123519 [details] [details] did not pass chromium-ews (chromium-xvfb):
> > Output: http://queues.webkit.org/results/11253828
> > 
> > New failing tests:
> > fast/dom/shadow/shadow-contents-fallback.html
> > fast/html/details-add-child-2.html
> > fast/html/details-replace-text.html
> > fast/dom/shadow/content-selector-query.html
> > fast/html/details-open2.html
> > fast/html/details-remove-child-2.html
> > fast/dom/shadow/shadow-contents-select.html
> 
> Hmm... Let me try fix them.

Sorry, I did a very stupid mistake. Fixed it in the latest patch.

[Hajime Morrita]

Comment on attachment 123524 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=123524&action=review

Basically looks OK. Regarding to m_isValidSelector, I feel it as symptom of missing "optional" class like boost::optional. Could you consider to add it to WTF when you have time? Of couse it's a different topic from this bug.

> Source/WebCore/html/shadow/ContentSelectorQuery.cpp:99
> +    default:

Don't use default clause if we have all enums in the case statements. 
Then compilers can catch possible missing case.

[Shinya Kawanaka]

Created attachment 123694 [details]
Patch

[WebKit Review Bot]

Comment on attachment 123694 [details]
Patch

Clearing flags on attachment: 123694

Committed r106225: <http://trac.webkit.org/changeset/106225>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.