[Webkit-unassigned] [Bug 75946] The query selector for HTMLContentElement should follow the shadow dom spec.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=75946





--- Comment #5 from Shinya Kawanaka <shinyak at chromium.org>  2012-01-22 21:58:35 PST ---
(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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.