EME: Address TODOs related to the removal of prefixed EME

EME: Address TODOs related to the removal of prefixed EME

BUG=249976
Committed: https://crrev.com/82f226ceed72d9783807709cdb2075e788615852
Cr-Commit-Position: refs/heads/master@{#412563}

[ddorwin, 2016-06-23 01:06:45]

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

[ddorwin, 2016-06-23 01:06:47]

jrummell, sandersd, xhwang: Please review your respective TODOs that I've
addressed.

xhwang: Please sanity check everything.

I have commented in the base version (of PS3) what I think each TODO meant
and/or why we have addressed the issu.

https://codereview.chromium.org/1920433003/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/attestation/platform_verification_flow.cc (left):

https://codereview.chromium.org/1920433003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/attestation/platform_verification_flow.cc:212: //
TODO(xhwang): Change to DCHECK when prefixed EME support is removed.
See
https://codereview.chromium.org/1001723002/diff/20001/chrome/browser/chromeos....

https://codereview.chromium.org/1920433003/diff/40001/content/renderer/media/...
File content/renderer/media/cdm/render_cdm_factory.cc (left):

https://codereview.chromium.org/1920433003/diff/40001/content/renderer/media/...
content/renderer/media/cdm/render_cdm_factory.cc:62: // TODO(sandersd): Address
this now that prefixed EME has been removed.
We passed `true` from ProxyDecryptor::CreateMediaKeys().

https://codereview.chromium.org/1920433003/diff/40001/media/base/android/java...
File media/base/android/java/src/org/chromium/media/MediaDrmBridge.java (left):

https://codereview.chromium.org/1920433003/diff/40001/media/base/android/java...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:755: //
TODO(xhwang): DCHECK this now that prefixed EME is deprecated.
Sessiond IDs are handled internally now, so we should never have an ID for a
non-existent session.

https://codereview.chromium.org/1920433003/diff/40001/media/cdm/aes_decryptor.cc
File media/cdm/aes_decryptor.cc (left):

https://codereview.chromium.org/1920433003/diff/40001/media/cdm/aes_decryptor...
media/cdm/aes_decryptor.cc:271: // TODO(jrummell): Since unprefixed will never
send NULL, remove this check
I think we could have removed this before when we converted from a pointer to a
vector.

https://codereview.chromium.org/1920433003/diff/40001/media/cdm/aes_decryptor...
File media/cdm/aes_decryptor_unittest.cc (left):

https://codereview.chromium.org/1920433003/diff/40001/media/cdm/aes_decryptor...
media/cdm/aes_decryptor_unittest.cc:769: // TODO(jrummell): Clean this up when
the prefixed API is removed.
I'm not sure what this meant other than all the other changes in this file and
the previous one.

[xhwang, 2016-06-23 05:04:53]

Thanks for closing the loops! LGTM

[jrummell, 2016-06-23 17:20:16]

lgtm

https://codereview.chromium.org/1920433003/diff/40001/media/cdm/aes_decryptor...
File media/cdm/aes_decryptor_unittest.cc (left):

https://codereview.chromium.org/1920433003/diff/40001/media/cdm/aes_decryptor...
media/cdm/aes_decryptor_unittest.cc:769: // TODO(jrummell): Clean this up when
the prefixed API is removed.
On 2016/06/23 01:06:47, ddorwin wrote:
> I'm not sure what this meant other than all the other changes in this file and
> the previous one.

When I added the comment the test was simply RemoveSession(expect(RESOLVED)).
Looks like various changes after that left the comment, but it does apply to the
RemoveSession() changes you made above.

[sandersd (OOO until July 31), 2016-06-23 18:35:08]

lgtm

[ddorwin, 2016-06-23 20:40:50]

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

[ddorwin, 2016-06-23 20:40:51]

dkrahn: Please OWNERS review the first file.

jrummell: Please review the updates to the aes_decryptor* files.

[Darren Krahn, 2016-06-24 01:11:15]

lgtm

[jrummell, 2016-06-25 00:39:38]

aes_decryptor changes lgtm

https://codereview.chromium.org/1920433003/diff/60001/media/cdm/aes_decryptor...
File media/cdm/aes_decryptor_unittest.cc (right):

https://codereview.chromium.org/1920433003/diff/60001/media/cdm/aes_decryptor...
media/cdm/aes_decryptor_unittest.cc:515: init_data.resize(512);
Should limits::kMaxKeyIdLength be used here (and +1 below)?

[ddorwin, 2016-08-11 20:26:24]

Patchset #7 (id:120001) has been deleted

[ddorwin, 2016-08-11 20:28:32]

jrummell, PTAL at the change I had to make for the
ECKEncryptedMediaTest.LoadLoadableSession failure.

https://codereview.chromium.org/1920433003/diff/60001/media/cdm/aes_decryptor...
File media/cdm/aes_decryptor_unittest.cc (right):

https://codereview.chromium.org/1920433003/diff/60001/media/cdm/aes_decryptor...
media/cdm/aes_decryptor_unittest.cc:515: init_data.resize(512);
On 2016/06/25 00:39:38, jrummell wrote:
> Should limits::kMaxKeyIdLength be used here (and +1 below)?

Tests should avoid using the same constants as the code they are testing. That
way, you'll know when the behavior changes. If someone changed the constant to 1
MB, we'd want the test to catch that. This may be unlikely in this case, but
consider a constant that was based on other constants or a change that might
make sense in one case and the author might not realize it affects the code
under this specific test.

https://codereview.chromium.org/1920433003/diff/140001/media/cdm/ppapi/extern...
File media/cdm/ppapi/external_clear_key/clear_key_cdm.cc (right):

https://codereview.chromium.org/1920433003/diff/140001/media/cdm/ppapi/extern...
media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:338: // AesDecryptor does
not support loading, but we need a real session so create
Is this comment accurate?

https://codereview.chromium.org/1920433003/diff/140001/media/cdm/ppapi/extern...
media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:342:
std::vector<uint8_t>(16), std::move(promise));
Made this 128 (random) bits. 0 bytes failed the check in aes_decryptor.cc.

[jrummell, 2016-08-12 19:24:25]

Still lgtm.

https://codereview.chromium.org/1920433003/diff/60001/media/cdm/aes_decryptor...
File media/cdm/aes_decryptor_unittest.cc (right):

https://codereview.chromium.org/1920433003/diff/60001/media/cdm/aes_decryptor...
media/cdm/aes_decryptor_unittest.cc:515: init_data.resize(512);
On 2016/08/11 20:28:32, ddorwin wrote:
> On 2016/06/25 00:39:38, jrummell wrote:
> > Should limits::kMaxKeyIdLength be used here (and +1 below)?
> 
> Tests should avoid using the same constants as the code they are testing. That
> way, you'll know when the behavior changes. If someone changed the constant to
1
> MB, we'd want the test to catch that. This may be unlikely in this case, but
> consider a constant that was based on other constants or a change that might
> make sense in one case and the author might not realize it affects the code
> under this specific test.

Acknowledged.

https://codereview.chromium.org/1920433003/diff/140001/media/cdm/ppapi/extern...
File media/cdm/ppapi/external_clear_key/clear_key_cdm.cc (right):

https://codereview.chromium.org/1920433003/diff/140001/media/cdm/ppapi/extern...
media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:338: // AesDecryptor does
not support loading, but we need a real session so create
On 2016/08/11 20:28:32, ddorwin wrote:
> Is this comment accurate?

How about "AesDecryptor does not support loading, so create a temporary session
with fake initData to represent it."?

https://codereview.chromium.org/1920433003/diff/140001/media/cdm/ppapi/extern...
media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:342:
std::vector<uint8_t>(16), std::move(promise));
On 2016/08/11 20:28:32, ddorwin wrote:
> Made this 128 (random) bits. 0 bytes failed the check in aes_decryptor.cc.

The keyId used is kLoadableSessionKeyId. So it could be used here, but it is
ignored in OnSessionMessage. However, it might be more explicit if that key was
used here (since that is the intent).

[ddorwin, 2016-08-12 21:40:39]

Thanks for the suggestion.

https://codereview.chromium.org/1920433003/diff/140001/media/cdm/ppapi/extern...
File media/cdm/ppapi/external_clear_key/clear_key_cdm.cc (right):

https://codereview.chromium.org/1920433003/diff/140001/media/cdm/ppapi/extern...
media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:338: // AesDecryptor does
not support loading, but we need a real session so create
On 2016/08/12 19:24:25, jrummell wrote:
> On 2016/08/11 20:28:32, ddorwin wrote:
> > Is this comment accurate?
> 
> How about "AesDecryptor does not support loading, so create a temporary
session
> with fake initData to represent it."?

Done.

https://codereview.chromium.org/1920433003/diff/140001/media/cdm/ppapi/extern...
media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:342:
std::vector<uint8_t>(16), std::move(promise));
On 2016/08/12 19:24:25, jrummell wrote:
> On 2016/08/11 20:28:32, ddorwin wrote:
> > Made this 128 (random) bits. 0 bytes failed the check in aes_decryptor.cc.
> 
> The keyId used is kLoadableSessionKeyId. So it could be used here, but it is
> ignored in OnSessionMessage. However, it might be more explicit if that key
was
> used here (since that is the intent).

Done.

[ddorwin, 2016-08-12 21:40:43]

The CQ bit was checked by ddorwin@chromium.org

[ddorwin, 2016-08-12 21:40:44]

The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org,
sandersd@chromium.org, dkrahn@chromium.org, jrummell@chromium.org
Link to the patchset: https://codereview.chromium.org/1920433003/#ps160001
(title: "review feedback")

[commit-bot: I haz the power, 2016-08-12 21:41:11]

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, 2016-08-12 22:05:53]

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

[commit-bot: I haz the power, 2016-08-12 22:05:55]

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

[ddorwin, 2016-08-15 22:36:14]

The CQ bit was checked by ddorwin@chromium.org

[ddorwin, 2016-08-15 22:36:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
jrummell@chromium.org, xhwang@chromium.org, dkrahn@chromium.org,
sandersd@chromium.org
Link to the patchset: https://codereview.chromium.org/1920433003/#ps180001
(title: "build fix")

[commit-bot: I haz the power, 2016-08-15 22:36:36]

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, 2016-08-15 23:20:14]

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

[commit-bot: I haz the power, 2016-08-15 23:20:15]

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

[ddorwin, 2016-08-16 19:44:23]

https://codereview.chromium.org/1920433003/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/attestation/platform_verification_flow.cc (right):

https://codereview.chromium.org/1920433003/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/attestation/platform_verification_flow.cc:219:
NOTREACHED() << "Platform verification not supported in the current mode.";
This caused the PlatformVerificationFlowTest.UnsupportedMode test to fail.
Instead, we'll log the error and not assert.

[ddorwin, 2016-08-16 19:44:28]

The CQ bit was checked by ddorwin@chromium.org

[ddorwin, 2016-08-16 19:44:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
jrummell@chromium.org, xhwang@chromium.org, dkrahn@chromium.org,
sandersd@chromium.org
Link to the patchset: https://codereview.chromium.org/1920433003/#ps200001
(title: "Fix PlatformVerificationFlowTest.UnsupportedMode")

[commit-bot: I haz the power, 2016-08-16 19:44:59]

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, 2016-08-16 21:09:24]

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

[commit-bot: I haz the power, 2016-08-16 21:09:25]

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

[ddorwin, 2016-08-16 23:26:02]

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

[ddorwin, 2016-08-16 23:26:22]

Updated to account for https://crbug.com/616166 and re-enable the test.

[commit-bot: I haz the power, 2016-08-16 23:26:33]

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, 2016-08-17 00:48:23]

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

[commit-bot: I haz the power, 2016-08-17 00:48:24]

Dry run: This issue passed the CQ dry run.

[ddorwin, 2016-08-17 16:47:16]

The CQ bit was checked by ddorwin@chromium.org

[ddorwin, 2016-08-17 16:47:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
jrummell@chromium.org, xhwang@chromium.org, dkrahn@chromium.org,
sandersd@chromium.org
Link to the patchset: https://codereview.chromium.org/1920433003/#ps240001
(title: "Update per crbug.com/616166: Remove NOTREACHED and re-enable test.")

[commit-bot: I haz the power, 2016-08-17 16:47: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, 2016-08-17 16:52:08]

Committed patchset #12 (id:240001)

[commit-bot: I haz the power, 2016-08-17 16:55:32]

Description was changed from

==========
EME: Address TODOs related to the removal of prefixed EME

BUG=249976
==========

to

==========
EME: Address TODOs related to the removal of prefixed EME

BUG=249976

Committed: https://crrev.com/82f226ceed72d9783807709cdb2075e788615852
Cr-Commit-Position: refs/heads/master@{#412563}
==========

[commit-bot: I haz the power, 2016-08-17 16:55:34]

Patchset 12 (id:??) landed as
https://crrev.com/82f226ceed72d9783807709cdb2075e788615852
Cr-Commit-Position: refs/heads/master@{#412563}