Bug 148697 – [mediacontrols] Improve media controls testing helpers

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Dean Jackson]

[mediacontrols] Improve media controls testing helpers

[Radar WebKit Bug Importer]

<rdar://problem/22530876>

[Dean Jackson]

Created attachment 260396 [details]
Patch

[Eric Carlson]

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()?.

[Dean Jackson]

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.

[Dean Jackson]

Committed r189254: <http://trac.webkit.org/changeset/189254>