[webkit-reviews] review denied: [Bug 75946] The query selector for HTMLContentElement should follow the shadow dom spec. : [Attachment 123257] Patch

[bugzilla-daemon at webkit.org]

MORITA Hajime <morrita at google.com> has denied Shinya Kawanaka
<shinyak at chromium.org>'s request for review:
Bug 75946: The query selector for HTMLContentElement should follow the shadow
dom spec.
https://bugs.webkit.org/show_bug.cgi?id=75946

Attachment 123257: Patch
https://bugs.webkit.org/attachment.cgi?id=123257&action=review

------- Additional Comments from MORITA Hajime <morrita at google.com>
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.