[webkit-reviews] review denied: [Bug 75302] ShadowContentElement should be able to use query. : [Attachment 121828] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Jan 11 18:02:49 PST 2012
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.
More information about the webkit-reviews
mailing list