Implement closest() API
Created attachment 239265 [details] Patch
Comment on attachment 239265 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=239265&action=review review- because of the bad cast > Source/WebCore/dom/Element.cpp:2329 > + SelectorQuery* selectorQuery = document().selectorQueryForString(selector, ec); > + if (selectorQuery) > + return selectorQuery->closest(*this); > + return nullptr; WebKit coding style prefers early return, meaning: if (!selectorQuery) return nullptr; > Source/WebCore/dom/SelectorQuery.cpp:128 > + Element* currentNode = (Element *) &rootNode; The formatting here isn’t right, but more importantly this is a bad cast if the root node is a document node. That needs to be fixed. > Source/WebCore/dom/SelectorQuery.cpp:151 > + unsigned selectorCount = m_selectors.size(); > + for (unsigned i = 0; i < selectorCount; ++i) { > + Element* closestElement = selectorClosest(m_selectors[i], targetElement, targetElement); This should use a modern for loop: for (auto& selector : m_selectors) { if (Element* closestElement = selectorClosest(selector, targetElement, targetElement)) return closestElement; }
Comment on attachment 239265 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=239265&action=review The patch is in good shape. There is a bug not mentioned by Darin or I but I would prefer you to discover it through tests. Good test coverage is extremely important to validate a feature. > LayoutTests/fast/selectors/closest-general.html:43 > +shouldBe('theTarget.closest("#theTarget")', "theTarget"); > +shouldBe('theTarget.closest("ancestor")', "ancestor"); > +shouldBeNull('theTarget.closest("hihi")', "ancestor"); You need a few more test cases to be sure everything works properly. First, you need to test all the combinators: a b a+b a>b a~b Ideally, you should test a use case that finds an ancestor, and one that does not. The other kind of use case you need to test is coma separated selectors. For example: a, b b, a Finally, it would be good to have negative and positive cases for the big kind of matching: #id .class [attribute] tagname > LayoutTests/fast/selectors/closest-general.html:50 > +shouldBeTrue('failOnEmptyArguments()'); > +shouldBeTrue('failOnEmptyStringArgument()'); You should use shouldThrow() to test the exceptions. There are other cases to cover here, invalid selectors for example ".123", " ", ")", "()", "}", etc, etc.
Created attachment 239279 [details] Patch
Comment on attachment 239279 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=239279&action=review The tests are great. You should look into one more improvement to your algorithm: Currently, the algorithm has to evaluate each selectors for all ancestors before knowing which one matches. For example, given a selector "foo, bar", if "foo" is not one of the ancestor, all the parents need to be evaluated for "foo" before looking for "bar". What you could do instead is go up the parents and test for both "foo" and "bar" at each step. > Source/WebCore/ChangeLog:5 > + In the changelog, authors generally explain their change. (see the other changelogs for an example). Here your patch is pretty self explanatory so you do not necessarily need to explain it in the changelog. It would be good to add a link to the specification you are implementing though. > Source/WebCore/dom/Element.cpp:2328 > + SelectorQuery* selectorQuery = document().selectorQueryForString(selector, ec); > + if (selectorQuery) > + return selectorQuery->closest(*this); You can scope this one in the if() if (SelectorQuery* selectorQuery = document().selectorQueryForString(selector, ec)) return selectorQuery->closest(*this); > LayoutTests/ChangeLog:9 > + * fast/selectors/closest-general-expected.txt: Added. > + * fast/selectors/closest-general.html: Added. The new tests are great, very wide coverage. > LayoutTests/fast/selectors/closest-general.html:21 > + </ancestor> The indentation is wrong here. > LayoutTests/fast/selectors/closest-general.html:116 > +shouldThrow('theTarget.closest("^_^")'); :)
Created attachment 239285 [details] Patch
Comment on attachment 239285 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=239285&action=review > Source/WebCore/dom/SelectorQuery.cpp:149 > + Element* candidateElement = selectorClosest(selector, *currentNode, *currentNode); Shouldn't the scope be targetElement instead of currentNode? > LayoutTests/fast/selectors/closest-scope.html:24 > +shouldBe('theTarget.closest("body>:scope")', 'theTarget'); Let's add a test for the :scope bug. theTarget.closest('body:scope') should be null.
Created attachment 239286 [details] Patch
Comment on attachment 239286 [details] Patch lgtm
Comment on attachment 239286 [details] Patch Clearing flags on attachment: 239286 Committed r174324: <http://trac.webkit.org/changeset/174324>
All reviewed patches have been landed. Closing bug.