Bug 75302 - ShadowContentElement should be able to use query.
Summary: ShadowContentElement should be able to use query.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 76180
Blocks: 75301 75306
  Show dependency treegraph
 
Reported: 2011-12-28 03:24 PST by Shinya Kawanaka
Modified: 2012-01-13 05:05 PST (History)
10 users (show)

See Also:


Attachments
Patch (27.56 KB, patch)
2011-12-28 05:42 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (35.36 KB, patch)
2012-01-04 23:40 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (36.12 KB, patch)
2012-01-05 00:22 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (35.11 KB, patch)
2012-01-05 01:02 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (26.20 KB, patch)
2012-01-06 02:11 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (34.23 KB, patch)
2012-01-09 18:03 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (40.02 KB, patch)
2012-01-10 05:02 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (44.90 KB, patch)
2012-01-11 21:57 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (44.95 KB, patch)
2012-01-12 00:26 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (44.96 KB, patch)
2012-01-12 01:26 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (45.01 KB, patch)
2012-01-12 01:36 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (46.90 KB, patch)
2012-01-12 18:02 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (38.82 KB, patch)
2012-01-13 02:26 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Shinya Kawanaka 2011-12-28 03:24:55 PST
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.
Comment 1 Shinya Kawanaka 2011-12-28 05:42:00 PST
Created attachment 120658 [details]
Patch
Comment 2 Shinya Kawanaka 2011-12-28 05:42:50 PST
I've tried to implement the proposed interface.
Comment 3 Early Warning System Bot 2011-12-28 05:48:42 PST
Comment on attachment 120658 [details]
Patch

Attachment 120658 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11035613
Comment 4 Collabora GTK+ EWS bot 2011-12-28 05:52:26 PST
Comment on attachment 120658 [details]
Patch

Attachment 120658 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11037619
Comment 5 Gyuyoung Kim 2011-12-28 05:56:45 PST
Comment on attachment 120658 [details]
Patch

Attachment 120658 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11037622
Comment 6 Shinya Kawanaka 2012-01-04 23:40:22 PST
Created attachment 121223 [details]
Patch
Comment 7 Early Warning System Bot 2012-01-04 23:45:31 PST
Comment on attachment 121223 [details]
Patch

Attachment 121223 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11120131
Comment 8 Shinya Kawanaka 2012-01-05 00:22:07 PST
Created attachment 121229 [details]
Patch
Comment 9 Shinya Kawanaka 2012-01-05 01:02:57 PST
Created attachment 121234 [details]
Patch
Comment 10 Shinya Kawanaka 2012-01-05 18:16:56 PST
(In reply to comment #9)
> Created an attachment (id=121234) [details]
> Patch

OK, it builds on all platforms. It's time to review this?
Comment 11 Dominic Cooney 2012-01-05 20:01:55 PST
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?
Comment 12 Shinya Kawanaka 2012-01-06 02:11:41 PST
Created attachment 121411 [details]
Patch
Comment 13 Shinya Kawanaka 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.
Comment 14 Early Warning System Bot 2012-01-06 02:19:11 PST
Comment on attachment 121411 [details]
Patch

Attachment 121411 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11144296
Comment 15 Gyuyoung Kim 2012-01-06 02:20:04 PST
Comment on attachment 121411 [details]
Patch

Attachment 121411 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11145309
Comment 16 Collabora GTK+ EWS bot 2012-01-06 04:26:31 PST
Comment on attachment 121411 [details]
Patch

Attachment 121411 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11142347
Comment 17 Shinya Kawanaka 2012-01-09 18:03:54 PST
Created attachment 121775 [details]
Patch
Comment 18 Dimitri Glazkov (Google) 2012-01-09 19:43:28 PST
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.
Comment 19 Hajime Morrita 2012-01-09 19:45:28 PST
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
> +2012-01-06  Shinya Kawanaka  <shinyak@google.com>

Dup.
Comment 20 Shinya Kawanaka 2012-01-10 05:02:40 PST
Created attachment 121828 [details]
Patch
Comment 21 Collabora GTK+ EWS bot 2012-01-10 05:30:30 PST
Comment on attachment 121828 [details]
Patch

Attachment 121828 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11198026
Comment 22 Hajime Morrita 2012-01-11 18:02:49 PST
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.
Comment 23 Shinya Kawanaka 2012-01-11 21:57:29 PST
Created attachment 122168 [details]
Patch
Comment 24 Hajime Morrita 2012-01-11 23:22:35 PST
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.
Comment 25 Shinya Kawanaka 2012-01-12 00:26:32 PST
Created attachment 122179 [details]
Patch
Comment 26 Shinya Kawanaka 2012-01-12 00:58:44 PST
(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.
Comment 27 Shinya Kawanaka 2012-01-12 01:26:04 PST
Created attachment 122188 [details]
Patch
Comment 28 Gyuyoung Kim 2012-01-12 01:31:09 PST
Comment on attachment 122188 [details]
Patch

Attachment 122188 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11222295
Comment 29 Early Warning System Bot 2012-01-12 01:33:37 PST
Comment on attachment 122188 [details]
Patch

Attachment 122188 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11212355
Comment 30 Gustavo Noronha (kov) 2012-01-12 01:36:27 PST
Comment on attachment 122188 [details]
Patch

Attachment 122188 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11118318
Comment 31 Shinya Kawanaka 2012-01-12 01:36:44 PST
Created attachment 122189 [details]
Patch
Comment 32 WebKit Review Bot 2012-01-12 03:15:54 PST
Comment on attachment 122189 [details]
Patch

Clearing flags on attachment: 122189

Committed r104805: <http://trac.webkit.org/changeset/104805>
Comment 33 WebKit Review Bot 2012-01-12 03:16:01 PST
All reviewed patches have been landed.  Closing bug.
Comment 34 Vsevolod Vlasov 2012-01-12 08:16:14 PST
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.
Comment 35 Hajime Morrita 2012-01-12 17:44:00 PST
(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.
Comment 36 Shinya Kawanaka 2012-01-12 18:02:32 PST
Created attachment 122353 [details]
Patch
Comment 37 Shinya Kawanaka 2012-01-12 18:03:28 PST
(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.
Comment 38 Shinya Kawanaka 2012-01-13 02:26:35 PST
Created attachment 122402 [details]
Patch
Comment 39 Hajime Morrita 2012-01-13 03:48:16 PST
Comment on attachment 122402 [details]
Patch

let's try again...
Comment 40 WebKit Review Bot 2012-01-13 05:05:12 PST
Comment on attachment 122402 [details]
Patch

Clearing flags on attachment: 122402

Committed r104919: <http://trac.webkit.org/changeset/104919>
Comment 41 WebKit Review Bot 2012-01-13 05:05:20 PST
All reviewed patches have been landed.  Closing bug.