media: CdmFactory creates CDM (MediaKeys) asynchronously.

media: CdmFactory creates CDM (MediaKeys) asynchronously.

This CL fixes the EME stack down to CdmFactory. The real aync CDM creation
will be fixed in a separate CL.

For unprefixed EME, since it's promise based, async creation of CDM fits easily.

For prefixed EME, GenerateKeyRequest() can be called multiple times without
waiting for the real CDM to be created/loaded. ProxyDecryptor is modified to
handle this case.

BUG=469003
TEST=All existing tests pass.

Committed: https://crrev.com/9bd8c733478345d0331fcdfd79e0bc61e22be68a
Cr-Commit-Position: refs/heads/master@{#324942}

[xhwang, 2015-04-09 00:37:55]

rebase only

[xhwang, 2015-04-09 00:42:41]

Patchset #2 (id:20001) has been deleted

[xhwang, 2015-04-09 00:54:58]

rebase only

[xhwang, 2015-04-09 04:53:08]

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

[xhwang, 2015-04-09 04:53:09]

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

[xhwang, 2015-04-09 04:53:09]

I still need to fix Android. But please take a look first to make sure I am on
the right path. Thanks!

[ddorwin, 2015-04-10 00:59:43]

Reviewed everything except proxy_decryptor.

https://codereview.chromium.org/1070853004/diff/40001/content/renderer/media/...
File content/renderer/media/crypto/render_cdm_factory.cc (right):

https://codereview.chromium.org/1070853004/diff/40001/content/renderer/media/...
content/renderer/media/crypto/render_cdm_factory.cc:59: FROM_HERE,
base::Bind(cdm_created_cb, base::Passed(&cdm)));
nit: Does nullptr not work here? (It's a bit weird to pass a real value even
though we can see it's invalid above. Also, there is an (unlikely)  refactoring
risk.)

https://codereview.chromium.org/1070853004/diff/40001/media/blink/cdm_session...
File media/blink/cdm_session_adapter.cc (right):

https://codereview.chromium.org/1070853004/diff/40001/media/blink/cdm_session...
media/blink/cdm_session_adapter.cc:38: // prevent |this| from being desctructed.
Thanks for documenting this.
Can you further explain why we need to prevent that?

https://codereview.chromium.org/1070853004/diff/40001/media/blink/cdm_session...
media/blink/cdm_session_adapter.cc:132: "Failed to create CDM.");
s/CDM/{MediaKeys|CDM instance}/

https://codereview.chromium.org/1070853004/diff/40001/media/blink/encrypted_m...
File media/blink/encrypted_media_player_support.cc (right):

https://codereview.chromium.org/1070853004/diff/40001/media/blink/encrypted_m...
media/blink/encrypted_media_player_support.cc:173: // We do not support run-time
switching between key systems for now.
Be sure this passes layout tests. I think there was some test that triggered a
corner case in wGKR() calls.

https://codereview.chromium.org/1070853004/diff/40001/media/blink/encrypted_m...
media/blink/encrypted_media_player_support.cc:182:
proxy_decryptor_->GenerateKeyRequest(init_data_type, init_data,
Can this no longer fail? Is everything in an Initialize() call now (all the way
into the CdmAdapter)?

https://codereview.chromium.org/1070853004/diff/40001/media/blink/encrypted_m...
File media/blink/encrypted_media_player_support.h (right):

https://codereview.chromium.org/1070853004/diff/40001/media/blink/encrypted_m...
media/blink/encrypted_media_player_support.h:42: // |set_cdm_context_cb| is
called when the real CDM is created successfully.
"real"? Is there a fake/stub/temporary one?

"CDM instance"?

https://codereview.chromium.org/1070853004/diff/40001/media/cdm/default_cdm_f...
File media/cdm/default_cdm_factory.cc (right):

https://codereview.chromium.org/1070853004/diff/40001/media/cdm/default_cdm_f...
media/cdm/default_cdm_factory.cc:40: FROM_HERE, base::Bind(cdm_created_cb,
base::Passed(&cdm)));
ditto on |cdm|.

Also, perhaps fail at line 36 (and before |cdm| is declared), then do success
her. WDYT?

[xhwang, 2015-04-10 17:40:28]

comments addressed

[xhwang, 2015-04-10 17:41:17]

PTAL again.

https://codereview.chromium.org/1070853004/diff/40001/content/renderer/media/...
File content/renderer/media/crypto/render_cdm_factory.cc (right):

https://codereview.chromium.org/1070853004/diff/40001/content/renderer/media/...
content/renderer/media/crypto/render_cdm_factory.cc:59: FROM_HERE,
base::Bind(cdm_created_cb, base::Passed(&cdm)));
On 2015/04/10 00:59:43, ddorwin wrote:
> nit: Does nullptr not work here? (It's a bit weird to pass a real value even
> though we can see it's invalid above. Also, there is an (unlikely) 
refactoring
> risk.)

Done.

https://codereview.chromium.org/1070853004/diff/40001/media/blink/cdm_session...
File media/blink/cdm_session_adapter.cc (right):

https://codereview.chromium.org/1070853004/diff/40001/media/blink/cdm_session...
media/blink/cdm_session_adapter.cc:38: // prevent |this| from being desctructed.
On 2015/04/10 00:59:43, ddorwin wrote:
> Thanks for documenting this.
> Can you further explain why we need to prevent that?

Done.

https://codereview.chromium.org/1070853004/diff/40001/media/blink/cdm_session...
media/blink/cdm_session_adapter.cc:132: "Failed to create CDM.");
On 2015/04/10 00:59:43, ddorwin wrote:
> s/CDM/{MediaKeys|CDM instance}/

Done.

https://codereview.chromium.org/1070853004/diff/40001/media/blink/encrypted_m...
File media/blink/encrypted_media_player_support.cc (right):

https://codereview.chromium.org/1070853004/diff/40001/media/blink/encrypted_m...
media/blink/encrypted_media_player_support.cc:173: // We do not support run-time
switching between key systems for now.
On 2015/04/10 00:59:43, ddorwin wrote:
> Be sure this passes layout tests. I think there was some test that triggered a
> corner case in wGKR() calls.

This has always been the case and just confirmed that all layout tests are still
passing.

The only behavior change is:

- Previously when CDM initialization failed, we reset |current_key_system_| (but
we don't reset |proxy_decryptor_|), so that if GKR is called again after the
failure, we'll call InitializeCDM() again on the same |proxy_decryptor_| which
might succeed in theory.

- Now since CDM creation is async, if we want to do the same, we have to reset
the |current_key_system_| in a callback. But it becomes more complex if a second
GKR is called immediately after the first one, before the callback returns. And
we have to have this case in both EMPS and WMPA... Since this is only about a
corner case for a to-be-deprecated feature, I chose a simper solution: if CDM
instance creation failed, all following GKR calls will fail.

WDYT?

https://codereview.chromium.org/1070853004/diff/40001/media/blink/encrypted_m...
media/blink/encrypted_media_player_support.cc:182:
proxy_decryptor_->GenerateKeyRequest(init_data_type, init_data,
On 2015/04/10 00:59:43, ddorwin wrote:
> Can this no longer fail? Is everything in an Initialize() call now (all the
way
> into the CdmAdapter)?

Since we added RequestPermission, this path is already async. So it cannot fail
synchronously.

https://codereview.chromium.org/1070853004/diff/40001/media/blink/encrypted_m...
File media/blink/encrypted_media_player_support.h (right):

https://codereview.chromium.org/1070853004/diff/40001/media/blink/encrypted_m...
media/blink/encrypted_media_player_support.h:42: // |set_cdm_context_cb| is
called when the real CDM is created successfully.
On 2015/04/10 00:59:43, ddorwin wrote:
> "real"? Is there a fake/stub/temporary one?
> 
> "CDM instance"?

Done.

https://codereview.chromium.org/1070853004/diff/40001/media/cdm/default_cdm_f...
File media/cdm/default_cdm_factory.cc (right):

https://codereview.chromium.org/1070853004/diff/40001/media/cdm/default_cdm_f...
media/cdm/default_cdm_factory.cc:40: FROM_HERE, base::Bind(cdm_created_cb,
base::Passed(&cdm)));
On 2015/04/10 00:59:43, ddorwin wrote:
> ditto on |cdm|.
> 
> Also, perhaps fail at line 36 (and before |cdm| is declared), then do success
> her. WDYT?

Done.

[ddorwin, 2015-04-11 02:41:48]

LGTM with comments.

https://codereview.chromium.org/1070853004/diff/80001/media/cdm/proxy_decrypt...
File media/cdm/proxy_decryptor.cc (right):

https://codereview.chromium.org/1070853004/diff/80001/media/cdm/proxy_decrypt...
media/cdm/proxy_decryptor.cc:121: 
DCHECK(media_keys_);

Line 116 assumes it.
Oh, actually, this is somehow handled at line 143. Hmm. Should even new calls go
through there?

https://codereview.chromium.org/1070853004/diff/80001/media/cdm/proxy_decrypt...
media/cdm/proxy_decryptor.cc:232: "CDM creation failed.");
Is this the correct message? Can we not DCHECK because WMPI handles this?

https://codereview.chromium.org/1070853004/diff/80001/media/cdm/proxy_decrypt...
media/cdm/proxy_decryptor.cc:285: OnLegacySessionError(std::string(),
MediaKeys::INVALID_STATE_ERROR, 0,
ditto

https://codereview.chromium.org/1070853004/diff/80001/media/cdm/proxy_decrypt...
File media/cdm/proxy_decryptor.h (right):

https://codereview.chromium.org/1070853004/diff/80001/media/cdm/proxy_decrypt...
media/cdm/proxy_decryptor.h:122: EmeInitDataType init_data_type;
nit: consts?

[xhwang, 2015-04-13 18:56:11]

ddorwin: Let me know if you comments are addressed....

https://codereview.chromium.org/1070853004/diff/80001/media/cdm/proxy_decrypt...
File media/cdm/proxy_decryptor.cc (right):

https://codereview.chromium.org/1070853004/diff/80001/media/cdm/proxy_decrypt...
media/cdm/proxy_decryptor.cc:121: 
On 2015/04/11 02:41:48, ddorwin wrote:
> DCHECK(media_keys_);
> 
> Line 116 assumes it.
> Oh, actually, this is somehow handled at line 143. Hmm. Should even new calls
go
> through there?

media_keys_ will be null here if previous CDM creation failed.

The check is put on l.143 because we are calling GKRInternal in two places now
(here and OnCdmCreated).

https://codereview.chromium.org/1070853004/diff/80001/media/cdm/proxy_decrypt...
media/cdm/proxy_decryptor.cc:232: "CDM creation failed.");
On 2015/04/11 02:41:48, ddorwin wrote:
> Is this the correct message? Can we not DCHECK because WMPI handles this?

Updated message.

WMPI (actually EncryptedMediaPlayerSupport) doesn't handle this because it don't
reset |current_key_system_| anymore.

https://codereview.chromium.org/1070853004/diff/80001/media/cdm/proxy_decrypt...
media/cdm/proxy_decryptor.cc:285: OnLegacySessionError(std::string(),
MediaKeys::INVALID_STATE_ERROR, 0,
On 2015/04/11 02:41:48, ddorwin wrote:
> ditto

Done.

https://codereview.chromium.org/1070853004/diff/80001/media/cdm/proxy_decrypt...
File media/cdm/proxy_decryptor.h (right):

https://codereview.chromium.org/1070853004/diff/80001/media/cdm/proxy_decrypt...
media/cdm/proxy_decryptor.h:122: EmeInitDataType init_data_type;
On 2015/04/11 02:41:48, ddorwin wrote:
> nit: consts?

Done.

[ddorwin, 2015-04-13 19:02:07]

lgtm

[xhwang, 2015-04-13 19:03:33]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2015-04-13 19:04:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1070853004/100001

[commit-bot: I haz the power, 2015-04-13 19:23:14]

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

[commit-bot: I haz the power, 2015-04-13 19:23:15]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[xhwang, 2015-04-13 19:59:15]

Patchset #6 (id:120001) has been deleted

[xhwang, 2015-04-13 20:46:20]

https://codereview.chromium.org/1070853004/diff/80001/media/cdm/proxy_decrypt...
File media/cdm/proxy_decryptor.h (right):

https://codereview.chromium.org/1070853004/diff/80001/media/cdm/proxy_decrypt...
media/cdm/proxy_decryptor.h:122: EmeInitDataType init_data_type;
On 2015/04/13 18:56:10, xhwang wrote:
> On 2015/04/11 02:41:48, ddorwin wrote:
> > nit: consts?
> 
> Done.

Actually I need this to be non-const here because I need the copy constructor to
push a PendingGenerateKeyRequestData into a vector. Switch to use ScopedVector
to avoid this issue.

[xhwang, 2015-04-13 20:46:32]

The CQ bit was checked by xhwang@chromium.org

[xhwang, 2015-04-13 20:46:32]

The patchset sent to the CQ was uploaded after l-g-t-m from ddorwin@chromium.org
Link to the patchset: https://codereview.chromium.org/1070853004/#ps140001
(title: "use ScopedVector")

[commit-bot: I haz the power, 2015-04-13 20:47:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1070853004/140001

[commit-bot: I haz the power, 2015-04-13 23:28:03]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2015-04-13 23:28:59]

Patchset 6 (id:??) landed as
https://crrev.com/9bd8c733478345d0331fcdfd79e0bc61e22be68a
Cr-Commit-Position: refs/heads/master@{#324942}