[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 #13 from Shinya Kawanaka <shinyak at chromium.org>  2012-01-06 02:14:55 PST ---
(In reply to comment #11)
> (From update of attachment 121234 [details])
> 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?

Hmm...
I'd like to split search query matcher and ShadowContentElement.
So I've removed matchesQuery from ShadowContentElement and changed ShadowContentSelectorQuery a bit.

> 
> > 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?

Done.

> 
> > 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.

Done.

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

Sorry, this is a mistake.

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

I've searched several examples of value struct. They do not use m_.
So I leave this as is.

> 
> > 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?

Done.

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