[webkit-reviews] review denied: [Bug 75302] ShadowContentElement should be able to use query. : [Attachment 121828] 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 75302: ShadowContentElement should be able to use query.
https://bugs.webkit.org/show_bug.cgi?id=75302

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

------- Additional Comments from MORITA Hajime <morrita at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=121828&action=review


> Source/WebCore/dom/SelectorQuery.cpp:47
> +    m_selectors.clear();

Do we need clear() ? I guess we can just assert empty()-ness.

> Source/WebCore/dom/SelectorQuery.cpp:82
>  bool SelectorQuery::canUseIdLookup() const
>      // we would need to sort the results. For now, just traverse the
document in that case.

It looks canUseIdLookUp() can be moved to SelectorDataList.

> Source/WebCore/dom/SelectorQuery.cpp:-100
> -		   if (m_selectorChecker.checkSelector(m_selectors[i].selector,
element, m_selectors[i].isFastCheckable)) {

I guess this check function can be encapsulated inside SelectorDataList so that
we can remove an per-item accessor.

> Source/WebCore/dom/SelectorQuery.h:48
> +    CSSSelector* selector(int nth) const { return m_selectors[nth].selector;
}

selectorAt() ?

> Source/WebCore/dom/ShadowContentSelectorQuery.cpp:72
> +bool ShadowContentSelectorQuery::matches(Node* targetNode, CSSSelector*
selector, bool isFastCheckable) const

I hope this can be part of SelectorDataList to hide per-element accessor.

> Source/WebCore/dom/ShadowContentSelectorQuery.cpp:76
> +    if (!targetNode->isElementNode())

This check can be done by the caller.

> Source/WebCore/html/HTMLDetailsElement.cpp:68
> +const AtomicString DetailsSummaryElement::summaryQuerySelector =
"summary:first-of-type";

Please make this inside some (possibly static) method to avoid static
initializers.

> Source/WebCore/html/HTMLSummaryElement.cpp:46
> +	   : ShadowContentElement(HTMLNames::divTag, document, WTF::emptyAtom)

You don't need WTF::. It is in using.