Summary: | [mediacontrols] Improve media controls testing helpers | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dean Jackson <dino> | ||||
Component: | Media | Assignee: | Dean Jackson <dino> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | webkit-bug-importer | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | WebKit Nightly Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Dean Jackson
2015-09-01 17:40:30 PDT
Created attachment 260396 [details]
Patch
Comment on attachment 260396 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=260396&action=review Very nice! > LayoutTests/media/controls/basic.html:9 > + var tester = new ControlsTest(); > + tester.whenReady(runTests); > + tester.start(); Can you make this even more modern-looking with something like: var tester = new ControlsTest() .whenReady(runTests) .start(); :-) > LayoutTests/media/controls/controls-test-helpers.js:11 > + this.eventType = eventType || "canplaythrough"; Nit: maybe 'eventTrigger' instead of 'eventType'? > LayoutTests/media/controls/controls-test-helpers.js:66 > + var textMsg = document.createTextNode(msg); > + this.console.appendChild(textMsg); > + var br = document.createElement("br"); > + this.console.appendChild(br); Nit: don't really need the local variables. > LayoutTests/media/controls/controls-test-helpers.js:73 > + this.logMessage(""); > + this.logMessage(msg); > + this.logMessage(""); Nit: "this.logMessage('<br>' + msg + '<br>') would be more concise. > LayoutTests/media/controls/showControlsButton-expected.txt:12 > +EVENT: canplaythrough > +PASS: We are using the apple idiom > + > +Get the button that shows controls while in Voice Over > + > +PASS: Name is 'Show Controls' > +PASS: Is hidden It would be nice to have a label for the first test section as well. Maybe have the constructor take a label and have start() call startNewSection()?. Comment on attachment 260396 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=260396&action=review >> LayoutTests/media/controls/basic.html:9 >> + tester.start(); > > Can you make this even more modern-looking with something like: > > var tester = new ControlsTest() > .whenReady(runTests) > .start(); > > :-) Done! >> LayoutTests/media/controls/controls-test-helpers.js:11 >> + this.eventType = eventType || "canplaythrough"; > > Nit: maybe 'eventTrigger' instead of 'eventType'? Changed! >> LayoutTests/media/controls/controls-test-helpers.js:66 >> + this.console.appendChild(br); > > Nit: don't really need the local variables. Removed! >> LayoutTests/media/controls/controls-test-helpers.js:73 >> + this.logMessage(""); > > Nit: "this.logMessage('<br>' + msg + '<br>') would be more concise. That doesn't quite work because we append a text node (rather than innerHTML). I cleaned it up a bit though. >> LayoutTests/media/controls/showControlsButton-expected.txt:12 >> +PASS: Is hidden > > It would be nice to have a label for the first test section as well. Maybe have the constructor take a label and have start() call startNewSection()?. Done. The start() method now takes an optional label. Committed r189254: <http://trac.webkit.org/changeset/189254> |