[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


MORITA Hajime <morrita at google.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #123257|review?                     |review-
               Flag|                            |




--- Comment #3 from MORITA Hajime <morrita at google.com>  2012-01-20 08:18:21 PST ---
(From update of attachment 123257)
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.

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