[Webkit-unassigned] [Bug 75302] ShadowContentElement should be able to use query.

[bugzilla-daemon at webkit.org]

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





--- Comment #11 from Dominic Cooney <dominicc at chromium.org>  2012-01-05 20:01:55 PST ---
(From update of attachment 121234)
View in context: https://bugs.webkit.org/attachment.cgi?id=121234&action=review

> Source/WebCore/dom/ShadowContentElement.cpp:81
> +bool ShadowContentElement::matchesQuery(const ShadowContentSelectorQuery& query, Node* node)

This factoring looks wrong to me:
• The responsibility for defining whether a query matches is split between ShadowContentElement::matchesQuery method and ShadowContentSelectorQuery::matches.
• The ShadowContentElement defines the query, but stores it in an string; then ShadowInclusionsSelector::select is responsible for parsing the query. It doesn’t seem right. Why is the representation of the query split this way?

> Source/WebCore/dom/ShadowContentElement.h:53
> +    void setSelect(const String& select) { m_select = select; }

Since the content element doesn’t handle updates, maybe it is safer to remove this method for now?

> Source/WebCore/dom/ShadowContentSelectorQuery.cpp:43
> +    parser.parseSelector(querySelector, document, m_selectorList);

This allows selectors not permitted by the Shadow DOM spec, right? If so you definitely need a FIXME here to tighten this up later.

> Source/WebCore/dom/ShadowContentSelectorQuery.cpp:51
> +    if (!node)

This is one indication of why it is bad to split matching across two places… this test is now duplicated.

> Source/WebCore/dom/ShadowContentSelectorQuery.cpp:55
> +    fprintf(stderr, "selectorCount = %d\n", (int) selectorCount);

Remove logging.

> Source/WebCore/dom/ShadowContentSelectorQuery.cpp:72
> +    // return m_selectorChecker.checkSelector(selectorData.selector, element, selectorData.isFastCheckable);

??

> Source/WebCore/dom/ShadowContentSelectorQuery.h:61
> +        CSSSelector* selector;

m_ for members

> LayoutTests/fast/dom/shadow/shadow-contents-select-expected.txt:1
> +PASS

This will be hard to debug when it fails because the output is so terse. Consider using a reftest?

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