[Clank] Load origin id before create MediaDrmBridge

[Clank] Load origin id before create MediaDrmBridge

Async Create MediaDrmBridge. When persistent license support is enabled,
we need to load origin id from persistent storage before create
MediaDrmStorage.

BUG=493521
TEST=Test page, MediaDrmBridgeTest

Review-Url: https://codereview.chromium.org/2823513002
Cr-Commit-Position: refs/heads/master@{#488784}
Committed: https://chromium.googlesource.com/chromium/src/+/bf16605671a4a735f6ee83668130d6a8bd39a12d

[yucliu1, 2017-04-13 22:44:42]

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

[commit-bot: I haz the power, 2017-04-13 22:45:19]

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-04-14 00:26:42]

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

[commit-bot: I haz the power, 2017-04-14 00:26:43]

Dry run: This issue passed the CQ dry run.

[yucliu1, 2017-04-14 01:30:35]

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

[commit-bot: I haz the power, 2017-04-14 01:30:55]

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-04-14 01:35:04]

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

[commit-bot: I haz the power, 2017-04-14 01:35:04]

Dry run: This issue passed the CQ dry run.

[yucliu1, 2017-04-14 20:13:26]

Description was changed from

==========
[Clank] Load origin id before create MediaDrmBridge

Async Create MediaDrmBridge. When persistent license support is enabled,
we need to load origin id from persistent storage before create
MediaDrmStorage.

BUG=493521
TEST=Test page, CQ
==========

to

==========
[Clank] Load origin id before create MediaDrmBridge

Async Create MediaDrmBridge. When persistent license support is enabled,
we need to load origin id from persistent storage before create
MediaDrmStorage.

BUG=493521
TEST=Test page, MediaDrmBridgeTest
==========

[yucliu1, 2017-04-14 20:13:58]

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

[xhwang, 2017-04-14 20:31:33]

Thanks! I am busy on a few other projects so I'll take a look next week.

[xhwang, 2017-05-03 06:00:56]

sorry for the delay. looking pretty good. I only have a few comments.

https://codereview.chromium.org/2823513002/diff/20001/chrome/browser/media/an...
File chrome/browser/media/android/cdm/media_drm_credential_manager.cc (right):

https://codereview.chromium.org/2823513002/diff/20001/chrome/browser/media/an...
chrome/browser/media/android/cdm/media_drm_credential_manager.cc:100:
&MediaDrmCredentialManager::OnMediaDrmCreated, base::Unretained(this),
This is not new code. But I wonder how is base::Unretained safe here?

https://codereview.chromium.org/2823513002/diff/20001/chrome/browser/media/an...
chrome/browser/media/android/cdm/media_drm_credential_manager.cc:107:
scoped_refptr<media::MediaDrmBridge> cdm) {
Do you need to set media_drm_bridge_ here? e.g.
media_drm_bridge_ = cdm?

https://codereview.chromium.org/2823513002/diff/20001/media/base/android/andr...
File media/base/android/android_cdm_factory.cc (right):

https://codereview.chromium.org/2823513002/diff/20001/media/base/android/andr...
media/base/android/android_cdm_factory.cc:106: security_level,
bound_cdm_created_cb));
nit: no need to use bound_cdm_created_cb here since the callback will always be
returned asynchronously.

https://codereview.chromium.org/2823513002/diff/20001/media/base/android/medi...
File media/base/android/media_drm_bridge.h (right):

https://codereview.chromium.org/2823513002/diff/20001/media/base/android/medi...
media/base/android/media_drm_bridge.h:70: using MediaDrmCreatedCB =
nit: how about just CreationCB? Or it needs to be MediaDrmBridgeCreatedCB, which
is too long...

https://codereview.chromium.org/2823513002/diff/20001/media/base/android/medi...
media/base/android/media_drm_bridge.h:108: MediaDrmCreatedCB
bound_cdm_created_cb);
ditto, just "CreationCB creation_cb"? "bound" is really an impl detail of the
caller.

https://codereview.chromium.org/2823513002/diff/20001/media/base/android/medi...
File media/base/android/media_drm_storage_bridge.cc (right):

https://codereview.chromium.org/2823513002/diff/20001/media/base/android/medi...
media/base/android/media_drm_storage_bridge.cc:48: impl_->Initialize(origin);
Initalize() should be a method that takes a callback to return an origin ID. The
ID should be generated the first time Initialize() is called, and stored in the
storage. This is because we could have two MediaDrmBridge on the same origin,
calling Initialize() trying to retrieve the origin ID, and the returned ID
should be consistent.

As you have a TODO here, we can do it in the next CL.

[yucliu1, 2017-05-03 06:33:49]

Comments only.

https://codereview.chromium.org/2823513002/diff/20001/chrome/browser/media/an...
File chrome/browser/media/android/cdm/media_drm_credential_manager.cc (right):

https://codereview.chromium.org/2823513002/diff/20001/chrome/browser/media/an...
chrome/browser/media/android/cdm/media_drm_credential_manager.cc:100:
&MediaDrmCredentialManager::OnMediaDrmCreated, base::Unretained(this),
On 2017/05/03 06:00:55, xhwang wrote:
> This is not new code. But I wonder how is base::Unretained safe here?

I don't know actually. I'm just copying the old behavior. Let me check.

https://codereview.chromium.org/2823513002/diff/20001/chrome/browser/media/an...
chrome/browser/media/android/cdm/media_drm_credential_manager.cc:107:
scoped_refptr<media::MediaDrmBridge> cdm) {
On 2017/05/03 06:00:55, xhwang wrote:
> Do you need to set media_drm_bridge_ here? e.g.
> media_drm_bridge_ = cdm?

Yes. Good catch.

https://codereview.chromium.org/2823513002/diff/20001/media/base/android/andr...
File media/base/android/android_cdm_factory.cc (right):

https://codereview.chromium.org/2823513002/diff/20001/media/base/android/andr...
media/base/android/android_cdm_factory.cc:106: security_level,
bound_cdm_created_cb));
On 2017/05/03 06:00:55, xhwang wrote:
> nit: no need to use bound_cdm_created_cb here since the callback will always
be
> returned asynchronously.

The callback should be bound in some cases, e.g. other key systems. I can move
this logic to MediaDrmBridge::Create to remove unnecessary task post.

https://codereview.chromium.org/2823513002/diff/20001/media/base/android/medi...
File media/base/android/media_drm_storage_bridge.cc (right):

https://codereview.chromium.org/2823513002/diff/20001/media/base/android/medi...
media/base/android/media_drm_storage_bridge.cc:48: impl_->Initialize(origin);
On 2017/05/03 06:00:56, xhwang wrote:
> Initalize() should be a method that takes a callback to return an origin ID.
The
> ID should be generated the first time Initialize() is called, and stored in
the
> storage. This is because we could have two MediaDrmBridge on the same origin,
> calling Initialize() trying to retrieve the origin ID, and the returned ID
> should be consistent.
> 
> As you have a TODO here, we can do it in the next CL.

Yes, I mean "return origin id asynchronously in a callback" actually.

[yucliu1, 2017-05-10 20:41:38]

Updated, PTAL

https://codereview.chromium.org/2823513002/diff/20001/chrome/browser/media/an...
File chrome/browser/media/android/cdm/media_drm_credential_manager.cc (right):

https://codereview.chromium.org/2823513002/diff/20001/chrome/browser/media/an...
chrome/browser/media/android/cdm/media_drm_credential_manager.cc:100:
&MediaDrmCredentialManager::OnMediaDrmCreated, base::Unretained(this),
On 2017/05/03 06:33:49, yucliu1 wrote:
> On 2017/05/03 06:00:55, xhwang wrote:
> > This is not new code. But I wonder how is base::Unretained safe here?
> 
> I don't know actually. I'm just copying the old behavior. Let me check.

It's safe because it's a singleton.

https://codereview.chromium.org/2823513002/diff/20001/chrome/browser/media/an...
chrome/browser/media/android/cdm/media_drm_credential_manager.cc:107:
scoped_refptr<media::MediaDrmBridge> cdm) {
On 2017/05/03 06:33:49, yucliu1 wrote:
> On 2017/05/03 06:00:55, xhwang wrote:
> > Do you need to set media_drm_bridge_ here? e.g.
> > media_drm_bridge_ = cdm?
> 
> Yes. Good catch.

Done.

https://codereview.chromium.org/2823513002/diff/20001/media/base/android/andr...
File media/base/android/android_cdm_factory.cc (right):

https://codereview.chromium.org/2823513002/diff/20001/media/base/android/andr...
media/base/android/android_cdm_factory.cc:106: security_level,
bound_cdm_created_cb));
On 2017/05/03 06:33:49, yucliu1 wrote:
> On 2017/05/03 06:00:55, xhwang wrote:
> > nit: no need to use bound_cdm_created_cb here since the callback will always
> be
> > returned asynchronously.
> 
> The callback should be bound in some cases, e.g. other key systems. I can move
> this logic to MediaDrmBridge::Create to remove unnecessary task post.

After looking at the logic inside MediaDrmBridge, it's not clear to move the
logic inside, because it may replicate the logic in many layers. So let's keep
the 'bound' here.

https://codereview.chromium.org/2823513002/diff/20001/media/base/android/medi...
File media/base/android/media_drm_bridge.h (right):

https://codereview.chromium.org/2823513002/diff/20001/media/base/android/medi...
media/base/android/media_drm_bridge.h:70: using MediaDrmCreatedCB =
On 2017/05/03 06:00:55, xhwang wrote:
> nit: how about just CreationCB? Or it needs to be MediaDrmBridgeCreatedCB,
which
> is too long...

Done.

https://codereview.chromium.org/2823513002/diff/20001/media/base/android/medi...
media/base/android/media_drm_bridge.h:108: MediaDrmCreatedCB
bound_cdm_created_cb);
On 2017/05/03 06:00:56, xhwang wrote:
> ditto, just "CreationCB creation_cb"? "bound" is really an impl detail of the
> caller.

Done.

[jrummell, 2017-05-10 22:38:13]

Looks good.

https://codereview.chromium.org/2823513002/diff/80001/chrome/browser/media/an...
File chrome/browser/media/android/cdm/media_drm_credential_manager.cc (right):

https://codereview.chromium.org/2823513002/diff/80001/chrome/browser/media/an...
chrome/browser/media/android/cdm/media_drm_credential_manager.cc:115:
media_drm_bridge_ = cdm;
Won't this create a second reference to the object, with the original being
unref'd at the end of this function? Would it be better to use
media_drm_bridge_.swap(cdm)?

[yucliu1, 2017-05-10 22:42:44]

https://codereview.chromium.org/2823513002/diff/80001/chrome/browser/media/an...
File chrome/browser/media/android/cdm/media_drm_credential_manager.cc (right):

https://codereview.chromium.org/2823513002/diff/80001/chrome/browser/media/an...
chrome/browser/media/android/cdm/media_drm_credential_manager.cc:115:
media_drm_bridge_ = cdm;
On 2017/05/10 22:38:12, jrummell wrote:
> Won't this create a second reference to the object, with the original being
> unref'd at the end of this function? Would it be better to use
> media_drm_bridge_.swap(cdm)?

Correct. I'll use std::move.

[yucliu1, 2017-05-16 21:54:59]

https://codereview.chromium.org/2823513002/diff/80001/chrome/browser/media/an...
File chrome/browser/media/android/cdm/media_drm_credential_manager.cc (right):

https://codereview.chromium.org/2823513002/diff/80001/chrome/browser/media/an...
chrome/browser/media/android/cdm/media_drm_credential_manager.cc:115:
media_drm_bridge_ = cdm;
On 2017/05/10 22:42:44, yucliu1 wrote:
> On 2017/05/10 22:38:12, jrummell wrote:
> > Won't this create a second reference to the object, with the original being
> > unref'd at the end of this function? Would it be better to use
> > media_drm_bridge_.swap(cdm)?
> 
> Correct. I'll use std::move.

Done.

[xhwang, 2017-05-16 21:56:18]

Sorry for the delay. I was totally distracted by other tasks. I'll take a look
this week.

[yucliu1, 2017-07-05 22:12:26]

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

[commit-bot: I haz the power, 2017-07-05 22:12:51]

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-07-06 01:57:01]

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

[commit-bot: I haz the power, 2017-07-06 01:57:02]

Dry run: This issue passed the CQ dry run.

[xhwang, 2017-07-18 06:40:14]

Sorry again for the delay. I am picking this up this week.

[xhwang, 2017-07-21 18:18:27]

Sorry again for the delay. LGTM with nits/questions.

https://codereview.chromium.org/2823513002/diff/160001/media/base/android/med...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/2823513002/diff/160001/media/base/android/med...
media/base/android/media_drm_bridge.cc:344:
std::move(created_cb).Run(std::move(media_drm_bridge));
Add a comment why we want to return the CDM asynchronously even though we can
return it synchronously here, e.g. make the callback binding easier.

https://codereview.chromium.org/2823513002/diff/160001/media/base/android/med...
media/base/android/media_drm_bridge.cc:865: !storage_->origin_id().empty();
Add a comment about when the origin_id could be empty.

https://codereview.chromium.org/2823513002/diff/160001/media/base/android/med...
File media/base/android/media_drm_bridge.h (right):

https://codereview.chromium.org/2823513002/diff/160001/media/base/android/med...
media/base/android/media_drm_bridge.h:299: // origin.
What does this exactly mean?

https://codereview.chromium.org/2823513002/diff/160001/media/base/android/med...
File media/base/android/media_drm_storage_bridge.h (right):

https://codereview.chromium.org/2823513002/diff/160001/media/base/android/med...
media/base/android/media_drm_storage_bridge.h:37: // this function isn't called,
all the other functions will fail.
"This function must be called before any other function is called.", then DCHECK
in the code.

https://codereview.chromium.org/2823513002/diff/160001/media/base/android/med...
media/base/android/media_drm_storage_bridge.h:40: base::OnceClosure on_init);
nit: s/on_init/init_cb

https://codereview.chromium.org/2823513002/diff/160001/media/base/android/med...
media/base/android/media_drm_storage_bridge.h:86: // Random generated ID for
origin.
nit: Randomly

[yucliu1, 2017-07-21 20:24:10]

https://codereview.chromium.org/2823513002/diff/160001/media/base/android/med...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/2823513002/diff/160001/media/base/android/med...
media/base/android/media_drm_bridge.cc:344:
std::move(created_cb).Run(std::move(media_drm_bridge));
On 2017/07/21 18:18:26, xhwang wrote:
> Add a comment why we want to return the CDM asynchronously even though we can
> return it synchronously here, e.g. make the callback binding easier.

Done.

https://codereview.chromium.org/2823513002/diff/160001/media/base/android/med...
media/base/android/media_drm_bridge.cc:865: !storage_->origin_id().empty();
On 2017/07/21 18:18:26, xhwang wrote:
> Add a comment about when the origin_id could be empty.

Done.

https://codereview.chromium.org/2823513002/diff/160001/media/base/android/med...
File media/base/android/media_drm_bridge.h (right):

https://codereview.chromium.org/2823513002/diff/160001/media/base/android/med...
media/base/android/media_drm_bridge.h:299: // origin.
On 2017/07/21 18:18:26, xhwang wrote:
> What does this exactly mean?

Forget the reason for this. Removed.

https://codereview.chromium.org/2823513002/diff/160001/media/base/android/med...
File media/base/android/media_drm_storage_bridge.h (right):

https://codereview.chromium.org/2823513002/diff/160001/media/base/android/med...
media/base/android/media_drm_storage_bridge.h:37: // this function isn't called,
all the other functions will fail.
On 2017/07/21 18:18:26, xhwang wrote:
> "This function must be called before any other function is called.", then
DCHECK
> in the code.

Already DCHECK(impl_) which is assigned in Initialize.

https://codereview.chromium.org/2823513002/diff/160001/media/base/android/med...
media/base/android/media_drm_storage_bridge.h:40: base::OnceClosure on_init);
On 2017/07/21 18:18:27, xhwang wrote:
> nit: s/on_init/init_cb

Done.

https://codereview.chromium.org/2823513002/diff/160001/media/base/android/med...
media/base/android/media_drm_storage_bridge.h:86: // Random generated ID for
origin.
On 2017/07/21 18:18:26, xhwang wrote:
> nit: Randomly

Done.

[yucliu1, 2017-07-21 20:24:17]

The CQ bit was checked by yucliu@chromium.org

[yucliu1, 2017-07-21 20:24:18]

The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/2823513002/#ps180001
(title: "Rebase + Nit")

[commit-bot: I haz the power, 2017-07-21 20:24:26]

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-07-21 22:24:57]

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

[commit-bot: I haz the power, 2017-07-21 22:24:59]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_TIMED_OUT, build has not
started yet; builder either lacks capacity or does not exist (misspelled?))
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_TIMED_OUT, build
has not started yet; builder either lacks capacity or does not exist
(misspelled?))
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_TIMED_OUT,
build has not started yet; builder either lacks capacity or does not exist
(misspelled?))

[yucliu1, 2017-07-21 22:37:25]

The CQ bit was checked by yucliu@chromium.org

[commit-bot: I haz the power, 2017-07-21 22:37:34]

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-07-21 23:19:38]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1500676645799240,
"parent_rev": "7a2ac6c75aaeea4e3a2546b82cd358b4088cf036", "commit_rev":
"bf16605671a4a735f6ee83668130d6a8bd39a12d"}

[commit-bot: I haz the power, 2017-07-21 23:19:52]

Description was changed from

==========
[Clank] Load origin id before create MediaDrmBridge

Async Create MediaDrmBridge. When persistent license support is enabled,
we need to load origin id from persistent storage before create
MediaDrmStorage.

BUG=493521
TEST=Test page, MediaDrmBridgeTest
==========

to

==========
[Clank] Load origin id before create MediaDrmBridge

Async Create MediaDrmBridge. When persistent license support is enabled,
we need to load origin id from persistent storage before create
MediaDrmStorage.

BUG=493521
TEST=Test page, MediaDrmBridgeTest

Review-Url: https://codereview.chromium.org/2823513002
Cr-Commit-Position: refs/heads/master@{#488784}
Committed:
https://chromium.googlesource.com/chromium/src/+/bf16605671a4a735f6ee83668130...
==========

[commit-bot: I haz the power, 2017-07-21 23:19:55]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/bf16605671a4a735f6ee83668130...