Bug 75946

Spec is here. http://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html

Created attachment 123257 [details] Patch

Comment on attachment 123257 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123257&action=review The approach looks OK. Added some comments inline. > Source/WebCore/html/shadow/ContentSelectorQuery.cpp:48 > parser.parseSelector(element->select(), element->document(), m_selectorList); Is there any way to detect syntax error? > Source/WebCore/html/shadow/ContentSelectorQuery.cpp:57 > + if (m_contentElement->select().isNull() || m_contentElement->select().isEmpty()) It looks we don't need this - this check was done in the constructor. > Source/WebCore/html/shadow/ContentSelectorQuery.cpp:118 > + for (size_t i = 0; i < sizeof(allowedPseudoType) / sizeof(allowedPseudoType[0]); ++i) { Why not switch? > Source/WebCore/html/shadow/ContentSelectorQuery.h:60 > + bool m_isValidSelector; Looks something is wrong.... This should be a property of SelectorDataList or CSSSelectorList? or should validateSelectorList() be a method of SelectorDataList? I'm not sure. but ownership around this class looks strange. > Source/WebCore/html/shadow/HTMLContentElement.h:65 > + virtual bool hasValidSelect() const; isSelectValid? > Source/WebCore/testing/Internals.cpp:153 > + if (!contentElement->isContentElement()) { Please check null-ity of contentElement > LayoutTests/fast/dom/shadow/content-selector-query.html:44 > +var dataOfInvalidCases = [ Could you add some syntactically invalid cases? There are, for example, forbidden symbols. We also need partially valid case which has both syntactically valid and invalid selectors.

Created attachment 123519 [details] Patch

(In reply to comment #3) > (From update of attachment 123257 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=123257&action=review > > The approach looks OK. Added some comments inline. > > > Source/WebCore/html/shadow/ContentSelectorQuery.cpp:48 > > parser.parseSelector(element->select(), element->document(), m_selectorList); > > Is there any way to detect syntax error? Ah... I have added a code to check m_selectorList->first() is empty or not. Empty means the CSSSelector fails parsing it. > > > Source/WebCore/html/shadow/ContentSelectorQuery.cpp:57 > > + if (m_contentElement->select().isNull() || m_contentElement->select().isEmpty()) > > It looks we don't need this - this check was done in the constructor. Done. > > > Source/WebCore/html/shadow/ContentSelectorQuery.cpp:118 > > + for (size_t i = 0; i < sizeof(allowedPseudoType) / sizeof(allowedPseudoType[0]); ++i) { > > Why not switch? Done. > > > Source/WebCore/html/shadow/ContentSelectorQuery.h:60 > > + bool m_isValidSelector; > > Looks something is wrong.... This should be a property of SelectorDataList or CSSSelectorList? > or should validateSelectorList() be a method of SelectorDataList? > I'm not sure. but ownership around this class looks strange. Hmm... Since SelectorDataList and CSSSelectorList are borrowed from CSSParser. The selector query is specific for HTMLContentElement, I don't think CSSParser related objects have it. So I've added m_isValidSelector there. I leave this code as is in Patch 2. > > > Source/WebCore/html/shadow/HTMLContentElement.h:65 > > + virtual bool hasValidSelect() const; > > isSelectValid? Done. > > > Source/WebCore/testing/Internals.cpp:153 > > + if (!contentElement->isContentElement()) { > > Please check null-ity of contentElement Done. > > > LayoutTests/fast/dom/shadow/content-selector-query.html:44 > > +var dataOfInvalidCases = [ > > Could you add some syntactically invalid cases? > There are, for example, forbidden symbols. > We also need partially valid case which has both syntactically valid and invalid selectors. Yes. I've added several (mainly syntax-check) test cases, which contains failing cases in my previous patch.

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

Comment on attachment 123519 [details] Patch Attachment 123519 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11253828 New failing tests: fast/dom/shadow/shadow-contents-fallback.html fast/html/details-add-child-2.html fast/html/details-replace-text.html fast/dom/shadow/content-selector-query.html fast/html/details-open2.html fast/html/details-remove-child-2.html fast/dom/shadow/shadow-contents-select.html

(In reply to comment #7) > (From update of attachment 123519 [details]) > Attachment 123519 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/11253828 > > New failing tests: > fast/dom/shadow/shadow-contents-fallback.html > fast/html/details-add-child-2.html > fast/html/details-replace-text.html > fast/dom/shadow/content-selector-query.html > fast/html/details-open2.html > fast/html/details-remove-child-2.html > fast/dom/shadow/shadow-contents-select.html Hmm... Let me try fix them.

Created attachment 123524 [details] Patch

(In reply to comment #8) > (In reply to comment #7) > > (From update of attachment 123519 [details] [details]) > > Attachment 123519 [details] [details] did not pass chromium-ews (chromium-xvfb): > > Output: http://queues.webkit.org/results/11253828 > > > > New failing tests: > > fast/dom/shadow/shadow-contents-fallback.html > > fast/html/details-add-child-2.html > > fast/html/details-replace-text.html > > fast/dom/shadow/content-selector-query.html > > fast/html/details-open2.html > > fast/html/details-remove-child-2.html > > fast/dom/shadow/shadow-contents-select.html > > Hmm... Let me try fix them. Sorry, I did a very stupid mistake. Fixed it in the latest patch.

Comment on attachment 123524 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123524&action=review Basically looks OK. Regarding to m_isValidSelector, I feel it as symptom of missing "optional" class like boost::optional. Could you consider to add it to WTF when you have time? Of couse it's a different topic from this bug. > Source/WebCore/html/shadow/ContentSelectorQuery.cpp:99 > + default: Don't use default clause if we have all enums in the case statements. Then compilers can catch possible missing case.

Created attachment 123694 [details] Patch

Comment on attachment 123694 [details] Patch Clearing flags on attachment: 123694 Committed r106225: <http://trac.webkit.org/changeset/106225>

All reviewed patches have been landed. Closing bug.