media: Resolve pending SetCdm() when WebMediaPlayerImpl is destructed

media: Resolve pending SetCdm() when WebMediaPlayerImpl is destructed

When WebMediaPlayerImpl is destructed, if there is a pending SetCdm()
call, we should resolve the promise so that the CDM can still be set on
the HTMLMediaElement. If we load() again, that CDM will be used as the
initial CDM for the new WebMediaPlayerImpl created.

BUG=693813
TEST=Added a new layout test.

Review-Url: https://codereview.chromium.org/2703993002
Cr-Commit-Position: refs/heads/master@{#452905}
Committed: https://chromium.googlesource.com/chromium/src/+/5113973bc4260b94300d81502935d5ebab57a1d0

[xhwang, 2017-02-18 07:24:57]

The CQ bit was checked by xhwang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-18 07:25:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-18 09:26:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-18 09:26:48]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[xhwang, 2017-02-18 18:08:31]

xhwang@chromium.org changed reviewers:
+ ddorwin@chromium.org, jrummell@chromium.org

[xhwang, 2017-02-18 18:08:31]

xhwang@chromium.org changed required reviewers:
+ jrummell@chromium.org

[xhwang, 2017-02-18 18:08:32]

PTAL

[xhwang, 2017-02-18 18:14:52]

I added a layout test, but it's hard to repro the original issue given this is a
race condition. So I am not sure how much value this test is adding.
Nevertheless, the test is a valid test from API's perspective.

PTAL

[xhwang, 2017-02-18 18:15:06]

The CQ bit was checked by xhwang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-18 18:15:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-18 20:08:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-18 20:08:40]

Dry run: This issue passed the CQ dry run.

[jrummell, 2017-02-22 00:36:48]

lgtm

https://codereview.chromium.org/2703993002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-reset-src-during-setmediakeys.html
(right):

https://codereview.chromium.org/2703993002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-reset-src-during-setmediakeys.html:42:
if (encryptedEventIndex == 2) {
Maybe assert that video.mediaKeys == null?

https://codereview.chromium.org/2703993002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-reset-src-during-setmediakeys.html:44:
video.setMediaKeys(null);
I thought this was bad behavior (setting it again while one is being processed).
is it even necessary?

https://codereview.chromium.org/2703993002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-reset-src-during-setmediakeys.html:48:
// Finished second video.
assert_true(video.mediaKeys === mediaKeys)?

https://codereview.chromium.org/2703993002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-reset-src-during-setmediakeys.html:65:
});
Probably needs a .catch(function(error) { forceTestFailureFromPromise(test,
error); }) here, just in case the promise fails (or the asserts throw).

[xhwang, 2017-02-24 17:36:50]

comments addressed

[xhwang, 2017-02-24 17:37:03]

The CQ bit was checked by xhwang@chromium.org to run a CQ dry run

[xhwang, 2017-02-24 17:37:09]

https://codereview.chromium.org/2703993002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-reset-src-during-setmediakeys.html
(right):

https://codereview.chromium.org/2703993002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-reset-src-during-setmediakeys.html:42:
if (encryptedEventIndex == 2) {
On 2017/02/22 00:36:48, jrummell wrote:
> Maybe assert that video.mediaKeys == null?

Done.

https://codereview.chromium.org/2703993002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-reset-src-during-setmediakeys.html:44:
video.setMediaKeys(null);
On 2017/02/22 00:36:48, jrummell wrote:
> I thought this was bad behavior (setting it again while one is being
processed).
> is it even necessary?

Removed.

https://codereview.chromium.org/2703993002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-reset-src-during-setmediakeys.html:48:
// Finished second video.
On 2017/02/22 00:36:48, jrummell wrote:
> assert_true(video.mediaKeys === mediaKeys)?

This is not necessarily true. Imagine setMediaKeys() takes very long time, and
load is very quick, by the time we get here, setMediaKeys() may not finish yet.

https://codereview.chromium.org/2703993002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/encrypted-media/encrypted-media-reset-src-during-setmediakeys.html:65:
});
On 2017/02/22 00:36:48, jrummell wrote:
> Probably needs a .catch(function(error) { forceTestFailureFromPromise(test,
> error); }) here, just in case the promise fails (or the asserts throw).

Done.

[commit-bot: I haz the power, 2017-02-24 17:37:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xhwang, 2017-02-24 17:38:25]

The CQ bit was unchecked by xhwang@chromium.org

[xhwang, 2017-02-24 17:38:25]

The CQ bit was checked by xhwang@chromium.org

[xhwang, 2017-02-24 17:38:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
jrummell@chromium.org
Link to the patchset: https://codereview.chromium.org/2703993002/#ps40001
(title: "comments addressed")

[commit-bot: I haz the power, 2017-02-24 17:38:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-24 19:36:13]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487957905656720,
"parent_rev": "030f8344b34bbfe3d7d73501eca197e2ed2114ef", "commit_rev":
"5113973bc4260b94300d81502935d5ebab57a1d0"}

[commit-bot: I haz the power, 2017-02-24 19:37:00]

Description was changed from

==========
media: Resolve pending SetCdm() when WebMediaPlayerImpl is destructed

When WebMediaPlayerImpl is destructed, if there is a pending SetCdm()
call, we should resolve the promise so that the CDM can still be set on
the HTMLMediaElement. If we load() again, that CDM will be used as the
initial CDM for the new WebMediaPlayerImpl created.

BUG=693813
TEST=Added a new layout test.
==========

to

==========
media: Resolve pending SetCdm() when WebMediaPlayerImpl is destructed

When WebMediaPlayerImpl is destructed, if there is a pending SetCdm()
call, we should resolve the promise so that the CDM can still be set on
the HTMLMediaElement. If we load() again, that CDM will be used as the
initial CDM for the new WebMediaPlayerImpl created.

BUG=693813
TEST=Added a new layout test.

Review-Url: https://codereview.chromium.org/2703993002
Cr-Commit-Position: refs/heads/master@{#452905}
Committed:
https://chromium.googlesource.com/chromium/src/+/5113973bc4260b94300d81502935...
==========

[commit-bot: I haz the power, 2017-02-24 19:37:01]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/5113973bc4260b94300d81502935...