[webkit-reviews] review granted: [Bug 137427] Element.matches()'s argument is not supposed to be optional : [Attachment 239288] Patch

Sat Oct 4 20:46:57 PDT 2014

Chris Dumez <cdumez at apple.com> has granted Benjamin Poulain
<benjamin at webkit.org>'s request for review:
Bug 137427: Element.matches()'s argument is not supposed to be optional
https://bugs.webkit.org/show_bug.cgi?id=137427

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

------- Additional Comments from Chris Dumez <cdumez at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=239288&action=review

r=me % nits.

> Source/WebCore/ChangeLog:8
> +	   The argument was marked as optional, it is not supposed to:

nit: Maybe we should mention in the changeling that you are updating
webkitMatchesSelector() as well?

> Source/WebCore/dom/Element.idl:125
> +    [ImplementedAs=matches, RaisesException] boolean
webkitMatchesSelector(DOMString selectors);

No test coverage for this one? Maybe in the one for matches() ?

> LayoutTests/fast/dom/SelectorAPI/closest-definition.html:11
> +shouldBeUndefined('Element.matches');

You likely mean closest, not matches

> LayoutTests/fast/dom/SelectorAPI/matches-definition.html:2
> +<html>

We don't really need all these tags.
<!doctype html>
<script src="../../../resources/js-test-pre.js"></script>
<script>...</script>
<script src="../../../resources/js-test-post.js"></script>

> LayoutTests/fast/dom/SelectorAPI/matches-null-undefined.html:14
> +shouldThrow('document.getElementsByTagName("undefined")[0].matches()');

nit: How about specifying the exception as second parameter to shouldThrow?
(same below and other tests). I personally like that we make sure we throw the
right exception.