Bug 75302 – ShadowContentElement should be able to use query.

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Shinya Kawanaka]

To support various element based on ShadowContentElement, it would be better that ShadowContentElement can use some queries to determine which elements are injected in it.

[Shinya Kawanaka]

Created attachment 120658 [details]
Patch

[Shinya Kawanaka]

I've tried to implement the proposed interface.

[Early Warning System Bot]

Comment on attachment 120658 [details]
Patch

Attachment 120658 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11035613

[Collabora GTK+ EWS bot]

Comment on attachment 120658 [details]
Patch

Attachment 120658 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11037619

[Gyuyoung Kim]

Comment on attachment 120658 [details]
Patch

Attachment 120658 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11037622

[Shinya Kawanaka]

Created attachment 121223 [details]
Patch

[Early Warning System Bot]

Comment on attachment 121223 [details]
Patch

Attachment 121223 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11120131

[Shinya Kawanaka]

Created attachment 121229 [details]
Patch

[Shinya Kawanaka]

Created attachment 121234 [details]
Patch

[Shinya Kawanaka]

(In reply to comment #9)
> Created an attachment (id=121234) [details]
> Patch

OK, it builds on all platforms. It's time to review this?

[Dominic Cooney]

Comment on attachment 121234 [details]
Patch

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?

[Shinya Kawanaka]

Created attachment 121411 [details]
Patch

[Shinya Kawanaka]

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

[Early Warning System Bot]

Comment on attachment 121411 [details]
Patch

Attachment 121411 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11144296

[Gyuyoung Kim]

Comment on attachment 121411 [details]
Patch

Attachment 121411 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11145309

[Collabora GTK+ EWS bot]

Comment on attachment 121411 [details]
Patch

Attachment 121411 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11142347

[Shinya Kawanaka]

Created attachment 121775 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 121775 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121775&action=review

Nifty.

> Source/WebCore/dom/ShadowContentElement.cpp:46
> +    , m_select(select)

And that's it? What about changing attribute value? Shouldn't that also reset m_select?

> Source/WebCore/dom/ShadowContentElement.h:46
> +    static PassRefPtr<ShadowContentElement> create(Document*, const String& select = String());

Can we just eliminate the implicit param? It seem too clever.

> Source/WebCore/dom/ShadowContentElement.h:57
> +    // FIXME: Currently this constructor accepts wider query than shadow dom spec.

File a bug and list it here.

[Hajime Morrita]

Comment on attachment 121775 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121775&action=review

Hi, thanks for attacking this. This is what I hope to have!
Let us iterate for a few round.

> Source/WebCore/ChangeLog:41
> +

Please add an entry to dom/DOMAllInOne.cpp to make the Windows port happy.

> Source/WebCore/dom/ShadowContentElement.h:46
> +    static PassRefPtr<ShadowContentElement> create(Document*, const String& select = String());

Please use AtomicString instead of String. It is what Attribute class uses.

> Source/WebCore/dom/ShadowContentElement.h:52
> +    const String& select() { return m_select; }

This should be implemented using getAttribute() because it should be also represented as an HTML attribute.

> Source/WebCore/dom/ShadowContentElement.h:59
> +    ShadowContentElement(const QualifiedName&, Document*, const String& select = String());

Ditto for AtomicString.

> Source/WebCore/dom/ShadowContentElement.h:67
> +    String m_select;

So we shouldn't need this.

> Source/WebCore/dom/ShadowContentSelectorQuery.cpp:37
> +    : m_matchesAll(element->select().isEmpty())

It looks we don' t need this because it's derivable from m_contentEleent.

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

This check can be done in matches(Node*).

> Source/WebCore/dom/ShadowContentSelectorQuery.h:37
> +#include <wtf/HashSet.h>

Do we need this...

> Source/WebCore/dom/ShadowContentSelectorQuery.h:38
> +#include <wtf/RefCounted.h>

and this?

> Source/WebCore/dom/ShadowContentSelectorQuery.h:50
> +    ShadowContentSelectorQuery(ShadowContentElement*);

Please make this explicit.

> Source/WebCore/dom/ShadowContentSelectorQuery.h:55
> +    struct SelectorData {

Let's extract SelectorData from SelectorQuery and share the code. 
I hope there are some reasonable way to extract stuff from SelectorQuery without sacrificing performance...

> Source/WebCore/dom/ShadowContentSelectorQuery.h:66
> +    bool canUseIdLookup(const SelectorData&, Document*) const;

It looks we aren't using this.

> Source/WebCore/html/HTMLDetailsElement.cpp:45
> +        : ShadowContentElement(HTMLNames::divTag, document, "")

Please use WTF::emptyAtom().

> Source/WebCore/html/HTMLDetailsElement.cpp:61
> +        : ShadowContentElement(HTMLNames::divTag, document, "summary:first-of-type")

Please use pre-defined static AtomicString instance instead of C string literal to prevent an extra heap allocation.

> Source/WebCore/testing/Internals.cpp:137
> +    return elem.release();

Why do we need these two step?

> LayoutTests/ChangeLog:13
> [phone]  Shinya Kawanaka  <[e-mail]>

Dup.

[Shinya Kawanaka]

Created attachment 121828 [details]
Patch

[Collabora GTK+ EWS bot]

Comment on attachment 121828 [details]
Patch

Attachment 121828 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11198026

[Hajime Morrita]

Comment on attachment 121828 [details]
Patch

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.

[Shinya Kawanaka]

Created attachment 122168 [details]
Patch

[Hajime Morrita]

Comment on attachment 122168 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122168&action=review

> Source/WebCore/dom/SelectorQuery.h:49
> +    bool matches(const SelectorChecker&, Node*) const;

You can make this Element* to remove assertion.

[Shinya Kawanaka]

Created attachment 122179 [details]
Patch

[Shinya Kawanaka]

(In reply to comment #24)
> (From update of attachment 122168 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=122168&action=review
> 
> > Source/WebCore/dom/SelectorQuery.h:49
> > +    bool matches(const SelectorChecker&, Node*) const;
> 
> You can make this Element* to remove assertion.

Done.

[Shinya Kawanaka]

Created attachment 122188 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 122188 [details]
Patch

Attachment 122188 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11222295

[Early Warning System Bot]

Comment on attachment 122188 [details]
Patch

Attachment 122188 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11212355

[Gustavo Noronha (kov)]

Comment on attachment 122188 [details]
Patch

Attachment 122188 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11118318

[Shinya Kawanaka]

Created attachment 122189 [details]
Patch

[WebKit Review Bot]

Comment on attachment 122189 [details]
Patch

Clearing flags on attachment: 122189

Committed r104805: <http://trac.webkit.org/changeset/104805>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Vsevolod Vlasov]

Looks like this patch caused apple win bots to fail compilation.

Build log: http://build.webkit.org/builders/Windows%20Release%20%28Build%29/builds/25748/steps/compile-webkit/logs/stdio

5>Linking...
5>   Creating library C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\lib\WebKit.lib and object C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\lib\WebKit.exp
5>WebKit.exp : error LNK2001: unresolved external symbol "public: static class WTF::PassRefPtr<class WebCore::ShadowContentElement> __cdecl WebCore::ShadowContentElement::create(class WebCore::Document *)" (?create@ShadowContentElement@WebCore@@SA?AV?$PassRefPtr@VShadowContentElement@WebCore@@@WTF@@PAVDocument@2@@Z)
5>C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\bin\WebKit.dll : fatal error LNK1120: 1 unresolved externals
5>Build log was saved at "file://C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\obj\WebKit\BuildLog.htm"
5>WebKit - 2 error(s), 0 warning(s)

Rolled out: Committed r104827: <http://trac.webkit.org/changeset/104827>

Reopening.

[Hajime Morrita]

(In reply to comment #34)
> Looks like this patch caused apple win bots to fail compilation.
> 
Well, actually the ews bot also went red...
Thanks for caring this.

[Shinya Kawanaka]

Created attachment 122353 [details]
Patch

[Shinya Kawanaka]

(In reply to comment #35)
> (In reply to comment #34)
> > Looks like this patch caused apple win bots to fail compilation.
> > 
> Well, actually the ews bot also went red...
> Thanks for caring this.

Thank you for caring this.
Let me try again.

[Shinya Kawanaka]

Created attachment 122402 [details]
Patch

[Hajime Morrita]

Comment on attachment 122402 [details]
Patch

let's try again...

[WebKit Review Bot]

Comment on attachment 122402 [details]
Patch

Clearing flags on attachment: 122402

Committed r104919: <http://trac.webkit.org/changeset/104919>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.