[webkit-reviews] review denied: [Bug 137418] Implement Element.closest() API : [Attachment 239279] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Dhi Aurrahman
<diorahman at rockybars.com>'s request for review:
Bug 137418: Implement Element.closest() API
https://bugs.webkit.org/show_bug.cgi?id=137418

Attachment 239279: Patch
https://bugs.webkit.org/attachment.cgi?id=239279&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
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("^_^")');

:)