Element.matches()'s argument is not supposed to be optional
Created attachment 239288 [details] Patch
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.
(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?
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.
Created attachment 239303 [details] Patch
(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?
(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.
Comment on attachment 239303 [details] Patch Clearing flags on attachment: 239303 Committed r174334: <http://trac.webkit.org/changeset/174334>
All reviewed patches have been landed. Closing bug.