media: Create MediaCrypto upon MediaDrmBridge creation.

media: Create MediaCrypto upon MediaDrmBridge creation.

- Create MediaCrypto right after MediaDrmBridge construction or after
  provisioning is successfully completed.
- Do not store mMediaCrypto in Java.

Tests:
- MediaCrypto creation on provisioned device.
- MediaCrypto creation on unprovisioned device with successful provisioning.
- MediaCrypto creation on unprovisioned device with failed provisioning (so null
  MediaCrypto was notified).

BUG=564246
TEST=See above.

Committed: https://crrev.com/4a1a996e31209104d3eabda56234c55596c0d1a6
Cr-Commit-Position: refs/heads/master@{#368438}

[xhwang, 2015-12-18 22:42:05]

Description was changed from

==========
media: Create MediaCrypto upon MediaDrmBridge construction.

Tests:
- MediaCrypto creation on provisioned device.
- MediaCrypto creation on unprovisioned device with successful provisioning.
- MediaCrypto creation on unprovisioned device with failed provisioning (so null
  MediaCrypto was notified).

BUG=564246
TEST=See above.
==========

to

==========
media: Create MediaCrypto upon MediaDrmBridge creation.

Tests:
- MediaCrypto creation on provisioned device.
- MediaCrypto creation on unprovisioned device with successful provisioning.
- MediaCrypto creation on unprovisioned device with failed provisioning (so null
  MediaCrypto was notified).

BUG=564246
TEST=See above.
==========

[xhwang, 2015-12-18 22:42:05]

xhwang@chromium.org changed reviewers:
+ qinmin@chromium.org, timav@chromium.org

[xhwang, 2015-12-18 22:46:08]

Description was changed from

==========
media: Create MediaCrypto upon MediaDrmBridge creation.

Tests:
- MediaCrypto creation on provisioned device.
- MediaCrypto creation on unprovisioned device with successful provisioning.
- MediaCrypto creation on unprovisioned device with failed provisioning (so null
  MediaCrypto was notified).

BUG=564246
TEST=See above.
==========

to

==========
media: Create MediaCrypto upon MediaDrmBridge creation.

- Add allowsSession to distinguish between normal and sessionless
MediaDrmBridge.
- Try to create MediaCrypto right after MediaDrmBridge construction if
"allowsSession".
- Do not store mMediaCrypto in Java.

Tests:
- MediaCrypto creation on provisioned device.
- MediaCrypto creation on unprovisioned device with successful provisioning.
- MediaCrypto creation on unprovisioned device with failed provisioning (so null
  MediaCrypto was notified).

BUG=564246
TEST=See above.
==========

[xhwang, 2015-12-18 22:47:14]

timav / qinmin: I am not super happy with the follow of this CL, but this is the
best I can get. I guess I am not super familiar with how to deal with
exceptions.

Any suggestions are welcome. PTAL!

[Tima Vaisburd, 2015-12-22 00:16:22]

https://chromiumcodereview.appspot.com/1540033002/diff/1/media/base/android/j...
File media/base/android/java/src/org/chromium/media/MediaDrmBridge.java (right):

https://chromiumcodereview.appspot.com/1540033002/diff/1/media/base/android/j...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:246: if
(MediaCrypto.isCryptoSchemeSupported(mSchemeUUID)) {
create() already calls MediaDrm.isCryptoSchemeSupported(), is this call
redundant? This is MediaCrypto and not MediaDrm, but both methods are static...

https://chromiumcodereview.appspot.com/1540033002/diff/1/media/base/android/j...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:344: if
(allowsSession && !mediaDrmBridge.createMediaCrypto()) {
[Q]: Do I get it right that if device has been provisioned and we cannot create
the media crypto, we will fail MediaDrmBridge creation; whereas if the device
has not been provisioned, then we will create MediaDrmBridge, provision the
device, and then if the MediaCrypto creation fails, report an error by
onMediaCryptoReady()?

https://chromiumcodereview.appspot.com/1540033002/diff/1/media/base/android/j...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:792:
release();
Does this release() call onMediaCryptoReady(null) even for mAllowSession ==
false, in other words, when a caller is not interested in onMediaCryptoReady()?

[xhwang, 2016-01-07 22:29:28]

comments addressed

[xhwang, 2016-01-07 22:31:20]

PTAL again!

https://chromiumcodereview.appspot.com/1540033002/diff/1/media/base/android/j...
File media/base/android/java/src/org/chromium/media/MediaDrmBridge.java (right):

https://chromiumcodereview.appspot.com/1540033002/diff/1/media/base/android/j...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:246: if
(MediaCrypto.isCryptoSchemeSupported(mSchemeUUID)) {
On 2015/12/22 00:16:21, Tima Vaisburd wrote:
> create() already calls MediaDrm.isCryptoSchemeSupported(), is this call
> redundant? This is MediaCrypto and not MediaDrm, but both methods are
static...

It "should be" redundant, but there's no such guarantee based on Android
documentation. I'd like to err on the safer side.

https://chromiumcodereview.appspot.com/1540033002/diff/1/media/base/android/j...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:344: if
(allowsSession && !mediaDrmBridge.createMediaCrypto()) {
On 2015/12/22 00:16:21, Tima Vaisburd wrote:
> [Q]: Do I get it right that if device has been provisioned and we cannot
create
> the media crypto, we will fail MediaDrmBridge creation; whereas if the device
> has not been provisioned, then we will create MediaDrmBridge, provision the
> device, and then if the MediaCrypto creation fails, report an error by
> onMediaCryptoReady()?

Correct. I see your worries about having different ways to report the same
error. But on the other side, I am kinda following the idea of "fail-fast" [1].
We have to handle creation failure at the caller anyways so this "fragmentation"
doesn't make the caller more "fragmented".

[1] https://en.wikipedia.org/wiki/Fail-fast

https://chromiumcodereview.appspot.com/1540033002/diff/1/media/base/android/j...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:792:
release();
On 2015/12/22 00:16:21, Tima Vaisburd wrote:
> Does this release() call onMediaCryptoReady(null) even for mAllowSession ==
> false, in other words, when a caller is not interested in
onMediaCryptoReady()?

Good point. Updated release(). As I said, I don't like the current
implementation but don't have better ideas right now (at least for M49).
Suggestions are welcome!

[xhwang, 2016-01-07 22:35:26]

BTW, now I don't really like mAllowsSession. How about we don't differentiate
normal mode and sessionless mode in the Java code? The only issue would be that
we will be creating MediaCrypto when users try to reset credentials. But as
discussed offline, MediaCrypto creation is fairly cheap. Also, I don't think
"credential resetting" is used very often. Maybe it's not worth complicating our
code only for this minor saving?

WDYT?

[Tima Vaisburd, 2016-01-07 22:38:50]

On 2016/01/07 22:35:26, xhwang wrote:
> BTW, now I don't really like mAllowsSession. How about we don't differentiate
> normal mode and sessionless mode in the Java code? The only issue would be
that
> we will be creating MediaCrypto when users try to reset credentials. But as
> discussed offline, MediaCrypto creation is fairly cheap. Also, I don't think
> "credential resetting" is used very often. Maybe it's not worth complicating
our
> code only for this minor saving?
> 
> WDYT?

I agree

[xhwang, 2016-01-08 00:18:20]

Updated. PTAL again!

[xhwang, 2016-01-08 00:19:30]

Description was changed from

==========
media: Create MediaCrypto upon MediaDrmBridge creation.

- Add allowsSession to distinguish between normal and sessionless
MediaDrmBridge.
- Try to create MediaCrypto right after MediaDrmBridge construction if
"allowsSession".
- Do not store mMediaCrypto in Java.

Tests:
- MediaCrypto creation on provisioned device.
- MediaCrypto creation on unprovisioned device with successful provisioning.
- MediaCrypto creation on unprovisioned device with failed provisioning (so null
  MediaCrypto was notified).

BUG=564246
TEST=See above.
==========

to

==========
media: Create MediaCrypto upon MediaDrmBridge creation.

- Create MediaCrypto right after MediaDrmBridge construction or after
  provisioning is successfully completed.
- Do not store mMediaCrypto in Java.

Tests:
- MediaCrypto creation on provisioned device.
- MediaCrypto creation on unprovisioned device with successful provisioning.
- MediaCrypto creation on unprovisioned device with failed provisioning (so null
  MediaCrypto was notified).

BUG=564246
TEST=See above.
==========

[xhwang, 2016-01-08 06:24:34]

rebase only

[xhwang, 2016-01-08 07:15:29]

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

[commit-bot: I haz the power, 2016-01-08 07:15:37]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1540033002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1540033002/60001

[commit-bot: I haz the power, 2016-01-08 08:33:50]

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

[commit-bot: I haz the power, 2016-01-08 08:33:51]

Dry run: This issue passed the CQ dry run.

[Tima Vaisburd, 2016-01-08 19:38:03]

lgtm % nits

https://chromiumcodereview.appspot.com/1540033002/diff/60001/media/base/andro...
File media/base/android/java/src/org/chromium/media/MediaDrmBridge.java (right):

https://chromiumcodereview.appspot.com/1540033002/diff/60001/media/base/andro...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:229:
Log.e(TAG, "Device not provisioned", e);
nit: This is not an error: device is not provisioned yet, we need to do
provisioninig. I'd change Log.e() to Log.i() or Log.d().

https://chromiumcodereview.appspot.com/1540033002/diff/60001/media/base/andro...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:357: 
nit: assert mMediaDrm != null; ?

[xhwang, 2016-01-08 20:05:20]

comments addressed

[xhwang, 2016-01-08 20:05:49]

qinmin: Do you also want to take a look?

https://chromiumcodereview.appspot.com/1540033002/diff/60001/media/base/andro...
File media/base/android/java/src/org/chromium/media/MediaDrmBridge.java (right):

https://chromiumcodereview.appspot.com/1540033002/diff/60001/media/base/andro...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:229:
Log.e(TAG, "Device not provisioned", e);
On 2016/01/08 19:38:03, Tima Vaisburd wrote:
> nit: This is not an error: device is not provisioned yet, we need to do
> provisioninig. I'd change Log.e() to Log.i() or Log.d().

Done.

https://chromiumcodereview.appspot.com/1540033002/diff/60001/media/base/andro...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:357: 
On 2016/01/08 19:38:03, Tima Vaisburd wrote:
> nit: assert mMediaDrm != null; ?

Done.

[qinmin, 2016-01-08 20:25:45]

lgtm % comments

https://chromiumcodereview.appspot.com/1540033002/diff/80001/media/base/andro...
File media/base/android/java/src/org/chromium/media/MediaDrmBridge.java (right):

https://chromiumcodereview.appspot.com/1540033002/diff/80001/media/base/andro...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:341: return
null;
this can be moved to the above line as they fit the character limit

https://chromiumcodereview.appspot.com/1540033002/diff/80001/media/base/andro...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:787: if
(!success) {
I think you can merge this if and the if block below:


success = success && (mMediaCryptoSession != null || createMediaCrypto());

if (!success) {
  release();
  return;
}

[xhwang, 2016-01-08 20:33:57]

comments addressed

[xhwang, 2016-01-08 20:34:01]

https://chromiumcodereview.appspot.com/1540033002/diff/80001/media/base/andro...
File media/base/android/java/src/org/chromium/media/MediaDrmBridge.java (right):

https://chromiumcodereview.appspot.com/1540033002/diff/80001/media/base/andro...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:341: return
null;
On 2016/01/08 20:25:45, qinmin wrote:
> this can be moved to the above line as they fit the character limit

git cl format likes the current style :)

https://chromiumcodereview.appspot.com/1540033002/diff/80001/media/base/andro...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:787: if
(!success) {
On 2016/01/08 20:25:45, qinmin wrote:
> I think you can merge this if and the if block below:
> 
> 
> success = success && (mMediaCryptoSession != null || createMediaCrypto());
> 
> if (!success) {
>   release();
>   return;
> }

Done.

[xhwang, 2016-01-08 20:34:05]

The CQ bit was checked by xhwang@chromium.org

[xhwang, 2016-01-08 20:34:05]

The patchset sent to the CQ was uploaded after l-g-t-m from timav@chromium.org,
qinmin@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1540033002/#ps100001 (title: "comments
addressed")

[commit-bot: I haz the power, 2016-01-08 20:34:38]

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

[commit-bot: I haz the power, 2016-01-08 21:46:10]

Description was changed from

==========
media: Create MediaCrypto upon MediaDrmBridge creation.

- Create MediaCrypto right after MediaDrmBridge construction or after
  provisioning is successfully completed.
- Do not store mMediaCrypto in Java.

Tests:
- MediaCrypto creation on provisioned device.
- MediaCrypto creation on unprovisioned device with successful provisioning.
- MediaCrypto creation on unprovisioned device with failed provisioning (so null
  MediaCrypto was notified).

BUG=564246
TEST=See above.
==========

to

==========
media: Create MediaCrypto upon MediaDrmBridge creation.

- Create MediaCrypto right after MediaDrmBridge construction or after
  provisioning is successfully completed.
- Do not store mMediaCrypto in Java.

Tests:
- MediaCrypto creation on provisioned device.
- MediaCrypto creation on unprovisioned device with successful provisioning.
- MediaCrypto creation on unprovisioned device with failed provisioning (so null
  MediaCrypto was notified).

BUG=564246
TEST=See above.
==========

[commit-bot: I haz the power, 2016-01-08 21:46:11]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-01-08 21:47:03]

Description was changed from

==========
media: Create MediaCrypto upon MediaDrmBridge creation.

- Create MediaCrypto right after MediaDrmBridge construction or after
  provisioning is successfully completed.
- Do not store mMediaCrypto in Java.

Tests:
- MediaCrypto creation on provisioned device.
- MediaCrypto creation on unprovisioned device with successful provisioning.
- MediaCrypto creation on unprovisioned device with failed provisioning (so null
  MediaCrypto was notified).

BUG=564246
TEST=See above.
==========

to

==========
media: Create MediaCrypto upon MediaDrmBridge creation.

- Create MediaCrypto right after MediaDrmBridge construction or after
  provisioning is successfully completed.
- Do not store mMediaCrypto in Java.

Tests:
- MediaCrypto creation on provisioned device.
- MediaCrypto creation on unprovisioned device with successful provisioning.
- MediaCrypto creation on unprovisioned device with failed provisioning (so null
  MediaCrypto was notified).

BUG=564246
TEST=See above.

Committed: https://crrev.com/4a1a996e31209104d3eabda56234c55596c0d1a6
Cr-Commit-Position: refs/heads/master@{#368438}
==========

[commit-bot: I haz the power, 2016-01-08 21:47:04]

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