RESOLVED FIXED 137427
Element.matches()'s argument is not supposed to be optional 
https://bugs.webkit.org/show_bug.cgi?id=137427
Summary Element.matches()'s argument is not supposed to be optional
Benjamin Poulain
Reported 2014-10-04 20:24:24 PDT
Element.matches()'s argument is not supposed to be optional
Attachments
Patch (7.68 KB, patch)
2014-10-04 20:26 PDT, Benjamin Poulain 		no flags
DetailsFormatted DiffDiff
Patch (14.41 KB, patch)
2014-10-05 16:45 PDT, Benjamin Poulain 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Benjamin Poulain
Comment 1 2014-10-04 20:26:53 PDT
Created attachment 239288 [details] Patch
Chris Dumez
Comment 2 2014-10-04 20:46:55 PDT
Comment on attachment 239288 [details] Patch 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.
Benjamin Poulain
Comment 3 2014-10-05 16:37:51 PDT
(In reply to comment #2) > 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> I am updating the patch. I don't really understand this comment. What do you mean?
Chris Dumez
Comment 4 2014-10-05 16:42:54 PDT
Comment on attachment 239288 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=239288&action=review >> 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> I have been told by several reviewers before that we should omit the tags / sections if they're not needed, to keep the tests short. Here all <html>, <head> and <body> are not strictly needed. Technically, you just need the doctype and the scripts.
Benjamin Poulain
Comment 5 2014-10-05 16:45:09 PDT
Created attachment 239303 [details] Patch
Benjamin Poulain
Comment 6 2014-10-05 16:48:06 PDT
(In reply to comment #4) > (From update of attachment 239288 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=239288&action=review > > >> 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> > > I have been told by several reviewers before that we should omit the tags / sections if they're not needed, to keep the tests short. Here all <html>, <head> and <body> are not strictly needed. Technically, you just need the doctype and the scripts. I find this an odd advice. Our tests should use style that are the close to real use cases. Some variation is good but we should not force a minimalistic markup. This does not apply to JS tests obviously. Who complains about that?
Chris Dumez
Comment 7 2014-10-05 16:51:42 PDT
(In reply to comment #6) > (In reply to comment #4) > > (From update of attachment 239288 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=239288&action=review > > > > >> 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> > > > > I have been told by several reviewers before that we should omit the tags / sections if they're not needed, to keep the tests short. Here all <html>, <head> and <body> are not strictly needed. Technically, you just need the doctype and the scripts. > > I find this an odd advice. Our tests should use style that are the close to real use cases. Some variation is good but we should not force a minimalistic markup. > > This does not apply to JS tests obviously. > > Who complains about that? Well, I don't mind either way :) Your call. I am sure I heard this advice from Blink reviewers. On WebKit side, it may have been rniwa@ but I am not 100% sure.
Benjamin Poulain
Comment 8 2014-10-05 17:41:17 PDT
Comment on attachment 239303 [details] Patch Clearing flags on attachment: 239303 Committed r174334: <http://trac.webkit.org/changeset/174334>
Benjamin Poulain
Comment 9 2014-10-05 17:41:24 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.