[Webkit-unassigned] [Bug 137418] Implement Element.closest() API

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=137418


Benjamin Poulain <benjamin at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #239279|review?                     |review-
               Flag|                            |




--- Comment #5 from Benjamin Poulain <benjamin at webkit.org>  2014-10-04 12:12:46 PST ---
(From update of attachment 239279)
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("^_^")');

:)

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.