Bug 75302

Created attachment 120658 [details] Patch

I've tried to implement the proposed interface.

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

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

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

Created attachment 121223 [details] Patch

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

Created attachment 121229 [details] Patch

Created attachment 121234 [details] Patch

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

Created attachment 121411 [details] Patch

(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 on attachment 121411 [details] Patch Attachment 121411 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11144296

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

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

Created attachment 121775 [details] Patch

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

Created attachment 121828 [details] Patch

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

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.

Created attachment 122168 [details] Patch

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.

Created attachment 122179 [details] Patch

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

Created attachment 122188 [details] Patch

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

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

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

Created attachment 122189 [details] Patch

Comment on attachment 122189 [details] Patch Clearing flags on attachment: 122189 Committed r104805: <http://trac.webkit.org/changeset/104805>

All reviewed patches have been landed. Closing bug.

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.

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

Created attachment 122353 [details] Patch

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

Created attachment 122402 [details] Patch

Comment on attachment 122402 [details] Patch let's try again...

Comment on attachment 122402 [details] Patch Clearing flags on attachment: 122402 Committed r104919: <http://trac.webkit.org/changeset/104919>

All reviewed patches have been landed. Closing bug.