[Clank] Implement MediaDrmStorageBridge with MediaDrmStorage

[Clank] Implement MediaDrmStorageBridge with MediaDrmStorage

MediaDrmStorageBridge will lazy initialize MediaDrmStorage and forward
functions to MediaDrmStorage.

BUG=493521
TEST=Test page, Shaka demo page.

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

[yucliu1, 2017-04-07 00:05:36]

Description was changed from

==========
[Clank] Implement MediaDrmStorageBridge with MediaDrmStorage

MediaDrmStorageBridge will lazy initialize MediaDrmStorage and forward
functions to MediaDrmStorage.

BUG=493521
TEST=Test page, Shaka demo page.
==========

to

==========
[Clank] Implement MediaDrmStorageBridge with MediaDrmStorage

MediaDrmStorageBridge will lazy initialize MediaDrmStorage and forward
functions to MediaDrmStorage.

BUG=493521
TEST=Test page, Shaka demo page.
==========

[yucliu1, 2017-04-07 00:05:36]

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

[xhwang, 2017-04-07 05:47:35]

Looking pretty good. I only have a few comments.

https://codereview.chromium.org/2805803002/diff/1/media/base/android/media_dr...
File media/base/android/media_drm_storage_bridge.cc (right):

https://codereview.chromium.org/2805803002/diff/1/media/base/android/media_dr...
media/base/android/media_drm_storage_bridge.cc:89:
base::BindOnce(&OnSessionDataLoaded,
Is it safe to call OnSessionDataLoaded() and RunAndroidBoolCallback() after
|this| is destroyed? Imagine the callback is posted into the run loop, then
MediaDrmBridge is destroyed, then the task is run. Is this okay?

https://codereview.chromium.org/2805803002/diff/1/media/base/android/media_dr...
media/base/android/media_drm_storage_bridge.cc:134:
DCHECK(!create_storage_cb_.is_null());
nit: DCHECK(create_storage_cb);

https://codereview.chromium.org/2805803002/diff/1/media/base/android/media_dr...
media/base/android/media_drm_storage_bridge.cc:149: std::move(task).Run();
What's the purpose of PostCancellableTask and  RunCancellableTask? Is it so that
you can use weak_factory_.GetWeakPtr() so that the callback will be cancelled
when |this| is destructed?

If so, probably add a comment.

Or, maybe we can add a 

virtual base::WeakPtr<MediaDrmStorage> GetWeakPtr() = 0;

in MediaDrmStorage interface. Then you can use it to bind the task. Will that
work for you?

https://codereview.chromium.org/2805803002/diff/1/media/base/android/media_dr...
File media/base/android/media_drm_storage_bridge.h (right):

https://codereview.chromium.org/2805803002/diff/1/media/base/android/media_dr...
media/base/android/media_drm_storage_bridge.h:29:
base::OnceCallback<std::unique_ptr<MediaDrmStorage>()>;
Can you reuse the CreateStorageCB?

https://cs.chromium.org/chromium/src/media/base/android/media_drm_storage.h

Feel free to change that one to OnceCallback.

[yucliu1, 2017-04-07 18:53:10]

https://codereview.chromium.org/2805803002/diff/1/media/base/android/media_dr...
File media/base/android/media_drm_storage_bridge.cc (right):

https://codereview.chromium.org/2805803002/diff/1/media/base/android/media_dr...
media/base/android/media_drm_storage_bridge.cc:89:
base::BindOnce(&OnSessionDataLoaded,
On 2017/04/07 05:47:35, xhwang_slow wrote:
> Is it safe to call OnSessionDataLoaded() and RunAndroidBoolCallback() after
> |this| is destroyed? Imagine the callback is posted into the run loop, then
> MediaDrmBridge is destroyed, then the task is run. Is this okay?

Changed to WeakPtr<MediaDrmStorage>.

https://codereview.chromium.org/2805803002/diff/1/media/base/android/media_dr...
media/base/android/media_drm_storage_bridge.cc:134:
DCHECK(!create_storage_cb_.is_null());
On 2017/04/07 05:47:35, xhwang_slow wrote:
> nit: DCHECK(create_storage_cb);

Done.

https://codereview.chromium.org/2805803002/diff/1/media/base/android/media_dr...
media/base/android/media_drm_storage_bridge.cc:149: std::move(task).Run();
On 2017/04/07 05:47:35, xhwang_slow wrote:
> What's the purpose of PostCancellableTask and  RunCancellableTask? Is it so
that
> you can use weak_factory_.GetWeakPtr() so that the callback will be cancelled
> when |this| is destructed?
> 
> If so, probably add a comment.
> 
> Or, maybe we can add a 
> 
> virtual base::WeakPtr<MediaDrmStorage> GetWeakPtr() = 0;
> 
> in MediaDrmStorage interface. Then you can use it to bind the task. Will that
> work for you?

Add SupportsWeakPtr to MediaDrmStorage, it's redundant for subclass to implement
their own weak ptr. It's the easiest way to get rid of this.

https://codereview.chromium.org/2805803002/diff/1/media/base/android/media_dr...
File media/base/android/media_drm_storage_bridge.h (right):

https://codereview.chromium.org/2805803002/diff/1/media/base/android/media_dr...
media/base/android/media_drm_storage_bridge.h:29:
base::OnceCallback<std::unique_ptr<MediaDrmStorage>()>;
On 2017/04/07 05:47:35, xhwang_slow wrote:
> Can you reuse the CreateStorageCB?
> 
> https://cs.chromium.org/chromium/src/media/base/android/media_drm_storage.h
> 
> Feel free to change that one to OnceCallback.

Re-use CreateStorageCB. AndroidCdmFactory need to pass create_storage_cb to
different MediaDrmBridge, OnceCallback doesn't work for that case.

[xhwang, 2017-04-07 19:12:52]

lgtm with one question

https://codereview.chromium.org/2805803002/diff/1/media/base/android/media_dr...
File media/base/android/media_drm_storage_bridge.cc (right):

https://codereview.chromium.org/2805803002/diff/1/media/base/android/media_dr...
media/base/android/media_drm_storage_bridge.cc:89:
base::BindOnce(&OnSessionDataLoaded,
On 2017/04/07 18:53:10, yucliu1 wrote:
> On 2017/04/07 05:47:35, xhwang_slow wrote:
> > Is it safe to call OnSessionDataLoaded() and RunAndroidBoolCallback() after
> > |this| is destroyed? Imagine the callback is posted into the run loop, then
> > MediaDrmBridge is destroyed, then the task is run. Is this okay?
> 
> Changed to WeakPtr<MediaDrmStorage>.

Right. But OnSessionDataLoaded() isn't bounded using weakptr, is it okay?

[yucliu1, 2017-04-07 19:30:19]

https://codereview.chromium.org/2805803002/diff/1/media/base/android/media_dr...
File media/base/android/media_drm_storage_bridge.cc (right):

https://codereview.chromium.org/2805803002/diff/1/media/base/android/media_dr...
media/base/android/media_drm_storage_bridge.cc:89:
base::BindOnce(&OnSessionDataLoaded,
On 2017/04/07 19:12:52, xhwang_slow wrote:
> On 2017/04/07 18:53:10, yucliu1 wrote:
> > On 2017/04/07 05:47:35, xhwang_slow wrote:
> > > Is it safe to call OnSessionDataLoaded() and RunAndroidBoolCallback()
after
> > > |this| is destroyed? Imagine the callback is posted into the run loop,
then
> > > MediaDrmBridge is destroyed, then the task is run. Is this okay?
> > 
> > Changed to WeakPtr<MediaDrmStorage>.
> 
> Right. But OnSessionDataLoaded() isn't bounded using weakptr, is it okay?

I think it's safe to do this. If MediaDrmStorage is deleted, it won't call any
of the callbacks passed to it. And these functions don't take pointers to those
objects, so it should be safe to do this. What do you think?

[xhwang, 2017-04-07 21:34:43]

https://codereview.chromium.org/2805803002/diff/1/media/base/android/media_dr...
File media/base/android/media_drm_storage_bridge.cc (right):

https://codereview.chromium.org/2805803002/diff/1/media/base/android/media_dr...
media/base/android/media_drm_storage_bridge.cc:89:
base::BindOnce(&OnSessionDataLoaded,
On 2017/04/07 19:30:19, yucliu1 wrote:
> On 2017/04/07 19:12:52, xhwang_slow wrote:
> > On 2017/04/07 18:53:10, yucliu1 wrote:
> > > On 2017/04/07 05:47:35, xhwang_slow wrote:
> > > > Is it safe to call OnSessionDataLoaded() and RunAndroidBoolCallback()
> after
> > > > |this| is destroyed? Imagine the callback is posted into the run loop,
> then
> > > > MediaDrmBridge is destroyed, then the task is run. Is this okay?
> > > 
> > > Changed to WeakPtr<MediaDrmStorage>.
> > 
> > Right. But OnSessionDataLoaded() isn't bounded using weakptr, is it okay?
> 
> I think it's safe to do this. If MediaDrmStorage is deleted, it won't call any
> of the callbacks passed to it. And these functions don't take pointers to
those
> objects, so it should be safe to do this. What do you think?

The problem is that OnSessionDataLoaded() is not bound by a weak pointer, and
the bound callback could be posted so it outlives MediaDrmStorage. Either you
need to make sure calling OnSessionDataLoaded() is okay after |this| is
destoryed, or you can make OnSessionDataLoaded a class member method so that you
can use the weakptr of |this| to prevent it from being called after |this| is
destructed.

[yucliu1, 2017-04-08 04:04:11]

Bind callback to WeakPtr

[xhwang, 2017-04-08 04:23:54]

lgtm++

[yucliu1, 2017-04-09 04:23:47]

The CQ bit was checked by yucliu@chromium.org

[commit-bot: I haz the power, 2017-04-09 04:24:01]

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-09 04:45:32]

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

[commit-bot: I haz the power, 2017-04-09 04:45:33]

Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[yucliu1, 2017-04-09 18:15:51]

Add default copy copy constructor for SessionData to pass checker.

[yucliu1, 2017-04-09 18:15:56]

The CQ bit was checked by yucliu@chromium.org

[yucliu1, 2017-04-09 18:15:56]

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/2805803002/#ps60001
(title: "copy ctor")

[commit-bot: I haz the power, 2017-04-09 18:16:13]

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

[yucliu1, 2017-04-09 18:18:37]

The CQ bit was unchecked by yucliu@chromium.org

[yucliu1, 2017-04-09 18:23:32]

The CQ bit was checked by yucliu@chromium.org

[yucliu1, 2017-04-09 18:23:32]

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/2805803002/#ps80001
(title: "fix lint")

[commit-bot: I haz the power, 2017-04-09 18:23:43]

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-09 18:40:25]

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

[commit-bot: I haz the power, 2017-04-09 18:40:26]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[yucliu1, 2017-04-09 18:43:00]

The CQ bit was checked by yucliu@chromium.org

[yucliu1, 2017-04-09 18:43:01]

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/2805803002/#ps100001
(title: "Rebase")

[commit-bot: I haz the power, 2017-04-09 18:43:14]

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-09 18:59:40]

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

[commit-bot: I haz the power, 2017-04-09 18:59:41]

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_...)

[yucliu1, 2017-04-09 19:39:16]

The CQ bit was checked by yucliu@chromium.org

[commit-bot: I haz the power, 2017-04-09 19:39:27]

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-09 20:02:12]

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

[commit-bot: I haz the power, 2017-04-09 20:02:13]

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_...)

[yucliu1, 2017-04-09 22:36:55]

The CQ bit was checked by yucliu@chromium.org

[commit-bot: I haz the power, 2017-04-09 22:37:01]

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-09 23:37:19]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1491777415948940,
"parent_rev": "7be38d0566e6d753b65d4cf522dba17e4af41a5d", "commit_rev":
"ea2b51465a6fad6d53324a7238c4407e572dab97"}

[commit-bot: I haz the power, 2017-04-09 23:38:16]

Description was changed from

==========
[Clank] Implement MediaDrmStorageBridge with MediaDrmStorage

MediaDrmStorageBridge will lazy initialize MediaDrmStorage and forward
functions to MediaDrmStorage.

BUG=493521
TEST=Test page, Shaka demo page.
==========

to

==========
[Clank] Implement MediaDrmStorageBridge with MediaDrmStorage

MediaDrmStorageBridge will lazy initialize MediaDrmStorage and forward
functions to MediaDrmStorage.

BUG=493521
TEST=Test page, Shaka demo page.

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

[commit-bot: I haz the power, 2017-04-09 23:38:17]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/ea2b51465a6fad6d53324a7238c4...