Closed Bug 1301426 Opened 3 years ago Closed 3 years ago

Mochitests for promise-based HTMLMediaElement::play()

Categories

(Core :: Audio/Video: Playback, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: kaku, Assigned: kaku)

References

Details

Attachments

(19 files)

58 bytes, text/x-review-board-request
jwwang
: review+
Details
58 bytes, text/x-review-board-request
jwwang
: review+
Details
58 bytes, text/x-review-board-request
jwwang
: review+
Details
58 bytes, text/x-review-board-request
jwwang
: review+
Details
58 bytes, text/x-review-board-request
jwwang
: review+
Details
58 bytes, text/x-review-board-request
jwwang
: review+
Details
58 bytes, text/x-review-board-request
jwwang
: review+
Details
58 bytes, text/x-review-board-request
jwwang
: review+
Details
58 bytes, text/x-review-board-request
jwwang
: review+
Details
58 bytes, text/x-review-board-request
jwwang
: review+
Details
58 bytes, text/x-review-board-request
jwwang
: review+
Details
58 bytes, text/x-review-board-request
jwwang
: review+
Details
58 bytes, text/x-review-board-request
jwwang
: review+
Details
58 bytes, text/x-review-board-request
jwwang
: review+
Details
58 bytes, text/x-review-board-request
jwwang
: review+
Details
58 bytes, text/x-review-board-request
jwwang
: review+
Details
58 bytes, text/x-review-board-request
jwwang
: review+
Details
58 bytes, text/x-review-board-request
jwwang
: review+
Details
58 bytes, text/x-review-board-request
jwwang
: review+
Details
Follow bug 1244768 comment 71.
Depends on: 1244768
QA Contact: kaku
Assignee: nobody → kaku
QA Contact: kaku
Comment on attachment 8789512 [details]
Bug 1301426 part 1 - implement helper functions;

https://reviewboard.mozilla.org/r/77694/#review76156
Attachment #8789512 - Flags: review?(jwwang) → review+
Comment on attachment 8789513 [details]
Bug 1301426 part 2 - test case 1;

https://reviewboard.mozilla.org/r/77696/#review76158
Attachment #8789513 - Flags: review?(jwwang) → review+
Comment on attachment 8789514 [details]
Bug 1301426 part 3 - test case 2;

https://reviewboard.mozilla.org/r/77698/#review76160
Attachment #8789514 - Flags: review?(jwwang) → review+
Comment on attachment 8789515 [details]
Bug 1301426 part 4 - test case 3;

https://reviewboard.mozilla.org/r/77700/#review76162

::: dom/media/test/test_play_promise_3.html:35
(Diff revision 1)
> +      (msg) => { ok(true, msg); manager.finished(token); },
> +      (msg) => { ok(false, msg); manager.finished(token); }
> +    );
> +  });
> +
> +  once(element, "canplaythrough").then(() => {

We should just call play() without waiting for 'canplaythrough', right? I can't see how waiting for 'canplaythrough' is relevant to this test case.
Attachment #8789515 - Flags: review?(jwwang) → review+
Comment on attachment 8789516 [details]
Bug 1301426 part 5 - test case 4;

https://reviewboard.mozilla.org/r/77702/#review76164

::: dom/media/test/test_play_promise_4.html:26
(Diff revision 1)
> +
> +  let element = document.createElement(getMajorMimeType(test.type));
> +  element.src = getNotSupportedFile(test.name);
> +  invokePlayShouldBeRejectedWithError("playNotSupportedContent", element, token, "NotSupportedError")
> +  .then(
> +    (msg) => { ok(true, msg); manager.finished(token); },

It is kinda confusing that this promise is resolved when the play promise is rejected. It would be clearer to call play() directly (without using the helper function) and check the resolved/rejected values in this test case.
Attachment #8789516 - Flags: review?(jwwang) → review+
Comment on attachment 8789517 [details]
Bug 1301426 part 6 - test case 5;

https://reviewboard.mozilla.org/r/77704/#review76166

::: dom/media/test/test_play_promise_5.html:28
(Diff revision 1)
> +  element.src = getNotSupportedFile(test.name);
> +  once(element, "error").then(() => {
> +    ok(element.error.code == MediaError.MEDIA_ERR_SRC_NOT_SUPPORTED);
> +    invokePlayShouldBeRejectedWithError("playWithErrorAlreadySet", element, token, "NotSupportedError")
> +    .then(
> +      (msg) => { ok(true, msg); manager.finished(token); },

The same issue as the previosu test case.
Attachment #8789517 - Flags: review?(jwwang) → review+
Comment on attachment 8789518 [details]
Bug 1301426 part 7 - test case 6;

https://reviewboard.mozilla.org/r/77706/#review76168
Attachment #8789518 - Flags: review?(jwwang) → review+
Comment on attachment 8789519 [details]
Bug 1301426 part 8 - test case 7;

https://reviewboard.mozilla.org/r/77708/#review76170
Attachment #8789519 - Flags: review?(jwwang) → review+
Comment on attachment 8789520 [details]
Bug 1301426 part 9 - test case 8;

https://reviewboard.mozilla.org/r/77710/#review76172

::: dom/media/test/test_play_promise_8.html:14
(Diff revision 1)
> +</head>
> +<body>
> +<pre id="test">
> +
> +<script>
> +// Case: invlke play() and the pause() on an element that already has enough data to play

and 'then' pause()
Attachment #8789520 - Flags: review?(jwwang) → review+
Comment on attachment 8789521 [details]
Bug 1301426 part 10 - test case 9;

https://reviewboard.mozilla.org/r/77712/#review76174

::: dom/media/test/test_play_promise_9.html:14
(Diff revision 1)
> +</head>
> +<body>
> +<pre id="test">
> +
> +<script>
> +// Case: invlke play() and the pause() on an element that deoen't have enough data to play.

and 'then' pause()
Attachment #8789521 - Flags: review?(jwwang) → review+
Comment on attachment 8789522 [details]
Bug 1301426 part 11 - test case 10;

https://reviewboard.mozilla.org/r/77714/#review76176
Attachment #8789522 - Flags: review?(jwwang) → review+
Comment on attachment 8789523 [details]
Bug 1301426 part 12 - test case 11;

https://reviewboard.mozilla.org/r/77716/#review76178
Attachment #8789523 - Flags: review?(jwwang) → review+
Comment on attachment 8789524 [details]
Bug 1301426 part 13 - test case 12;

https://reviewboard.mozilla.org/r/77718/#review76180
Attachment #8789524 - Flags: review?(jwwang) → review+
Comment on attachment 8789525 [details]
Bug 1301426 part 14 - test case 13;

https://reviewboard.mozilla.org/r/77720/#review76182
Attachment #8789525 - Flags: review?(jwwang) → review+
Comment on attachment 8789526 [details]
Bug 1301426 part 15 - test case 14;

https://reviewboard.mozilla.org/r/77722/#review76184

::: dom/media/test/test_play_promise_14.html:33
(Diff revision 1)
> +  .then(
> +    (msg) => { ok(true, msg); manager.finished(token); },
> +    (msg) => { ok(false, msg); manager.finished(token); }
> +  );
> +
> +  once(element, "play").then(() => {

How can we make sure the 'play' handler happens before the play promise is resolved? Is it because the play promise won't be resolved until we assign a valid media resource to it?

::: dom/media/test/test_play_promise_14.html:34
(Diff revision 1)
> +    (msg) => { ok(true, msg); manager.finished(token); },
> +    (msg) => { ok(false, msg); manager.finished(token); }
> +  );
> +
> +  once(element, "play").then(() => {
> +    // The resouce selection algorithm has been done -> set the networkState to be NETWORK_EMPTY.

Add a comment to note that the networkState becomes NETWORK_EMPTY because there is no valid resource to load.
Attachment #8789526 - Flags: review?(jwwang) → review+
Comment on attachment 8789527 [details]
Bug 1301426 part 16 - test case 15;

https://reviewboard.mozilla.org/r/77724/#review76186
Attachment #8789527 - Flags: review?(jwwang) → review+
Comment on attachment 8789528 [details]
Bug 1301426 part 17 - test case 16;

https://reviewboard.mozilla.org/r/77726/#review76188
Attachment #8789528 - Flags: review?(jwwang) → review+
Comment on attachment 8789529 [details]
Bug 1301426 part 18 - test case 17;

https://reviewboard.mozilla.org/r/77728/#review76190
Attachment #8789529 - Flags: review?(jwwang) → review+
Comment on attachment 8789526 [details]
Bug 1301426 part 15 - test case 14;

https://reviewboard.mozilla.org/r/77722/#review76184

> How can we make sure the 'play' handler happens before the play promise is resolved? Is it because the play promise won't be resolved until we assign a valid media resource to it?

For a valid and successful play() operation, the order of resulting events are always "play" -> "playing" -> "promise resolving". 

> Is it because the play promise won't be resolved until we assign a valid media resource to it?

This is also right. Invoke play() on an element without soruce still dispatch the 'play' event but not the 'playing' event, of course not resoving the promise.
Comment on attachment 8814037 [details]
Bug 1301426 part 19 - test case 18;

https://reviewboard.mozilla.org/r/95352/#review96382
Attachment #8814037 - Flags: review?(jwwang) → review+
Depends on: 1321497
Try looks good! Thanks for reviews!
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3022d045808d
part 1 - implement helper functions; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/fe6a6213ad78
part 2 - test case 1; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/8c29980a4c22
part 3 - test case 2; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/d3b6ec3f3c6a
part 4 - test case 3; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/8946df884a6f
part 5 - test case 4; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/b1c958a86d87
part 6 - test case 5; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/094826abc0e7
part 7 - test case 6; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/6caab9b2dbeb
part 8 - test case 7; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/c444d693373e
part 9 - test case 8; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/95b156f55a2d
part 10 - test case 9; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/29d8085fd77d
part 11 - test case 10; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/7c73bc8bc144
part 12 - test case 11; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/876048fb7c8f
part 13 - test case 12; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/cb5d7ef529bd
part 14 - test case 13; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/209ee261ba6c
part 15 - test case 14; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/d59d582c6dfc
part 16 - test case 15; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/f3d6fedbddab
part 17 - test case 16; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/e6a5e973ffef
part 18 - test case 17; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/a7ffc4485fa5
part 19 - test case 18; r=jwwang
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.