Pass key_information on SessionKeysChange message

Pass key_information on SessionKeysChange message

The event now includes a list of key ids and their associated
status.

BUG=428384
TEST=existing EME tests pass

Committed: https://crrev.com/3d50dc21e899735f9c99596fc182552c6871c318
Cr-Commit-Position: refs/heads/master@{#310543}

[jrummell, 2015-01-02 21:16:00]

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

[jrummell, 2015-01-02 21:16:02]

PTAL.

[xhwang, 2015-01-02 21:55:08]

Thanks! Here are my first round of comments.

https://codereview.chromium.org/833963003/diff/20001/media/base/cdm_key_infor...
File media/base/cdm_key_information.h (right):

https://codereview.chromium.org/833963003/diff/20001/media/base/cdm_key_infor...
media/base/cdm_key_information.h:1: // Copyright 2015 The Chromium Authors. All
rights reserved.
Happy new year!

https://codereview.chromium.org/833963003/diff/20001/media/base/cdm_key_infor...
media/base/cdm_key_information.h:16: class MEDIA_EXPORT CdmKeyInformation {
It seems this can be a struct, then you don't need the accessors.

https://codereview.chromium.org/833963003/diff/20001/media/base/cdm_key_infor...
media/base/cdm_key_information.h:31: uint32 system_code);
See comment below, you don't need a version of ctor that takes uint8*/uint32.
Then you might just have a ctor that takes std::vector<uint8> directly.

https://codereview.chromium.org/833963003/diff/20001/media/base/cdm_key_infor...
media/base/cdm_key_information.h:32: virtual ~CdmKeyInformation();
It seems this is not a base class - no need to use "virtual" here.

https://codereview.chromium.org/833963003/diff/20001/media/base/cdm_key_infor...
media/base/cdm_key_information.h:36: uint32 SystemCode() const { return
system_code_; }
For accessor, use key_id()/status()/system_code().

https://codereview.chromium.org/833963003/diff/20001/media/base/cdm_key_infor...
media/base/cdm_key_information.h:41: uint32 system_code_;
If you have only one ctor that takes std::vector<uint8>, you can make all these
three members const.

https://codereview.chromium.org/833963003/diff/20001/media/base/media_keys.h
File media/base/media_keys.h (right):

https://codereview.chromium.org/833963003/diff/20001/media/base/media_keys.h#...
media/base/media_keys.h:31: typedef std::vector<CdmKeyInformation>
CdmKeyInformationVector;
Is it worth using ScopedVector here? If not, add a comment in CdmKeyInformation
about why we make it copyable. (e.g. it needs to be used in a STL container...)

https://codereview.chromium.org/833963003/diff/20001/media/base/media_keys.h#...
media/base/media_keys.h:31: typedef std::vector<CdmKeyInformation>
CdmKeyInformationVector;
Throughout this CL, how about
s/CdmKeyInformationVector/CdmKeysInfo/
s/key_information/keys_info
?

KeysInfo is consistent with KeysChange. Also, "keys_info" makes it clear that we
have info for multiple keys.

https://codereview.chromium.org/833963003/diff/20001/media/cdm/aes_decryptor_...
File media/cdm/aes_decryptor_unittest.cc (right):

https://codereview.chromium.org/833963003/diff/20001/media/cdm/aes_decryptor_...
media/cdm/aes_decryptor_unittest.cc:412: CdmKeyInformationVector
useable_key_ids_;
s/useable/usable?

In chromium, usable is used 1919 times and useable is used 40 times.

https://codereview.chromium.org/833963003/diff/20001/media/mojo/services/medi...
File media/mojo/services/media_type_converters.cc (right):

https://codereview.chromium.org/833963003/diff/20001/media/mojo/services/medi...
media/mojo/services/media_type_converters.cc:454: media::CdmKeyInformation
If you use ScopedVector, you can return scoped_ptr<CdmKeyInformation> here.

https://codereview.chromium.org/833963003/diff/20001/media/mojo/services/medi...
media/mojo/services/media_type_converters.cc:459: input->key_id.size(),
Since input->key_id is an array, you should be able to input->storage() to get
the std::vector directly.

[jrummell, 2015-01-05 22:18:00]

Updated.

https://codereview.chromium.org/833963003/diff/20001/media/base/cdm_key_infor...
File media/base/cdm_key_information.h (right):

https://codereview.chromium.org/833963003/diff/20001/media/base/cdm_key_infor...
media/base/cdm_key_information.h:1: // Copyright 2015 The Chromium Authors. All
rights reserved.
On 2015/01/02 21:55:07, xhwang wrote:
> Happy new year!

Thanks. Same to you.

https://codereview.chromium.org/833963003/diff/20001/media/base/cdm_key_infor...
media/base/cdm_key_information.h:16: class MEDIA_EXPORT CdmKeyInformation {
On 2015/01/02 21:55:07, xhwang wrote:
> It seems this can be a struct, then you don't need the accessors.

Done.

https://codereview.chromium.org/833963003/diff/20001/media/base/cdm_key_infor...
media/base/cdm_key_information.h:31: uint32 system_code);
On 2015/01/02 21:55:07, xhwang wrote:
> See comment below, you don't need a version of ctor that takes uint8*/uint32.
> Then you might just have a ctor that takes std::vector<uint8> directly.

Simplified to a struct.

https://codereview.chromium.org/833963003/diff/20001/media/base/cdm_key_infor...
media/base/cdm_key_information.h:32: virtual ~CdmKeyInformation();
On 2015/01/02 21:55:07, xhwang wrote:
> It seems this is not a base class - no need to use "virtual" here.

Done.

https://codereview.chromium.org/833963003/diff/20001/media/base/cdm_key_infor...
media/base/cdm_key_information.h:36: uint32 SystemCode() const { return
system_code_; }
On 2015/01/02 21:55:07, xhwang wrote:
> For accessor, use key_id()/status()/system_code().

Not needed now that this is a struct.

https://codereview.chromium.org/833963003/diff/20001/media/base/cdm_key_infor...
media/base/cdm_key_information.h:41: uint32 system_code_;
On 2015/01/02 21:55:07, xhwang wrote:
> If you have only one ctor that takes std::vector<uint8>, you can make all
these
> three members const.

Now a struct.

https://codereview.chromium.org/833963003/diff/20001/media/base/media_keys.h
File media/base/media_keys.h (right):

https://codereview.chromium.org/833963003/diff/20001/media/base/media_keys.h#...
media/base/media_keys.h:31: typedef std::vector<CdmKeyInformation>
CdmKeyInformationVector;
On 2015/01/02 21:55:07, xhwang wrote:
> Throughout this CL, how about
> s/CdmKeyInformationVector/CdmKeysInfo/
> s/key_information/keys_info
> ?
> 
> KeysInfo is consistent with KeysChange. Also, "keys_info" makes it clear that
we
> have info for multiple keys.

Done.

https://codereview.chromium.org/833963003/diff/20001/media/base/media_keys.h#...
media/base/media_keys.h:31: typedef std::vector<CdmKeyInformation>
CdmKeyInformationVector;
On 2015/01/02 21:55:07, xhwang wrote:
> Is it worth using ScopedVector here? If not, add a comment in
CdmKeyInformation
> about why we make it copyable. (e.g. it needs to be used in a STL
container...)

Done.

https://codereview.chromium.org/833963003/diff/20001/media/cdm/aes_decryptor_...
File media/cdm/aes_decryptor_unittest.cc (right):

https://codereview.chromium.org/833963003/diff/20001/media/cdm/aes_decryptor_...
media/cdm/aes_decryptor_unittest.cc:412: CdmKeyInformationVector
useable_key_ids_;
On 2015/01/02 21:55:07, xhwang wrote:
> s/useable/usable?
> 
> In chromium, usable is used 1919 times and useable is used 40 times.

Done.

https://codereview.chromium.org/833963003/diff/20001/media/mojo/services/medi...
File media/mojo/services/media_type_converters.cc (right):

https://codereview.chromium.org/833963003/diff/20001/media/mojo/services/medi...
media/mojo/services/media_type_converters.cc:454: media::CdmKeyInformation
On 2015/01/02 21:55:08, xhwang wrote:
> If you use ScopedVector, you can return scoped_ptr<CdmKeyInformation> here.

Done.

https://codereview.chromium.org/833963003/diff/20001/media/mojo/services/medi...
media/mojo/services/media_type_converters.cc:459: input->key_id.size(),
On 2015/01/02 21:55:08, xhwang wrote:
> Since input->key_id is an array, you should be able to input->storage() to get
> the std::vector directly.

Done.

[xhwang, 2015-01-05 22:53:15]

Thanks! I just have a few minor comments.

https://codereview.chromium.org/833963003/diff/40001/media/base/cdm_key_infor...
File media/base/cdm_key_information.cc (right):

https://codereview.chromium.org/833963003/diff/40001/media/base/cdm_key_infor...
media/base/cdm_key_information.cc:9: CdmKeyInformation::CdmKeyInformation() {
Initialize member variables.

https://codereview.chromium.org/833963003/diff/40001/media/base/media_keys.h
File media/base/media_keys.h (right):

https://codereview.chromium.org/833963003/diff/40001/media/base/media_keys.h#...
media/base/media_keys.h:143: const CdmKeysInfo& keys_info)> SessionKeysChangeCB;
Typically we pass movable objects (e.g. scoped_ptr and ScopedVector) by value
and use Pass().

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

https://codereview.chromium.org/833963003/diff/40001/media/cdm/aes_decryptor....
media/cdm/aes_decryptor.cc:340: key_info->key_id = key_id_vector;
You can avoid the extra copy here by using 

key_info->key_id.assign(item.first.begin(), item.first.end());

https://codereview.chromium.org/833963003/diff/40001/media/cdm/aes_decryptor_...
File media/cdm/aes_decryptor_unittest.cc (right):

https://codereview.chromium.org/833963003/diff/40001/media/cdm/aes_decryptor_...
media/cdm/aes_decryptor_unittest.cc:314: void SaveUsableKeys(Unused, Unused,
const CdmKeysInfo& usable_key_ids) {
If you pass CdmKeysInfo by value, you can probably just use Pass() and transfer
the ownership over. Will that be simpler?

https://codereview.chromium.org/833963003/diff/40001/media/cdm/aes_decryptor_...
media/cdm/aes_decryptor_unittest.cc:321:
usable_key_ids_.push_back(new_key_id.release());
It seems here we save ALL key IDs regardless of it's status, i.e. even for
non-usable key IDs. So "Usable" in the function name doesn't seem accurate.

How about we rename SaveUsableKeys to something like SaveKeysInfo?

https://codereview.chromium.org/833963003/diff/40001/media/cdm/aes_decryptor_...
media/cdm/aes_decryptor_unittest.cc:345: bool
UsableKeyIdsContains(std::vector<uint8> expected) {
Ditto about "Usable".

https://codereview.chromium.org/833963003/diff/40001/media/cdm/aes_decryptor_...
media/cdm/aes_decryptor_unittest.cc:425: CdmKeysInfo usable_key_ids_;
ditto about "usable".

https://codereview.chromium.org/833963003/diff/40001/media/mojo/services/medi...
File media/mojo/services/media_type_converters.h (right):

https://codereview.chromium.org/833963003/diff/40001/media/mojo/services/medi...
media/mojo/services/media_type_converters.h:76: static CdmKeyInformationPtr
Convert(media::CdmKeyInformation* input);
For input-only param we should use const-ref per style guide.

[jrummell, 2015-01-06 02:36:25]

Updated.

https://codereview.chromium.org/833963003/diff/40001/media/base/cdm_key_infor...
File media/base/cdm_key_information.cc (right):

https://codereview.chromium.org/833963003/diff/40001/media/base/cdm_key_infor...
media/base/cdm_key_information.cc:9: CdmKeyInformation::CdmKeyInformation() {
On 2015/01/05 22:53:15, xhwang wrote:
> Initialize member variables.

Done.

https://codereview.chromium.org/833963003/diff/40001/media/base/media_keys.h
File media/base/media_keys.h (right):

https://codereview.chromium.org/833963003/diff/40001/media/base/media_keys.h#...
media/base/media_keys.h:143: const CdmKeysInfo& keys_info)> SessionKeysChangeCB;
On 2015/01/05 22:53:15, xhwang wrote:
> Typically we pass movable objects (e.g. scoped_ptr and ScopedVector) by value
> and use Pass().

Done.

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

https://codereview.chromium.org/833963003/diff/40001/media/cdm/aes_decryptor....
media/cdm/aes_decryptor.cc:340: key_info->key_id = key_id_vector;
On 2015/01/05 22:53:15, xhwang wrote:
> You can avoid the extra copy here by using 
> 
> key_info->key_id.assign(item.first.begin(), item.first.end());

Done.

https://codereview.chromium.org/833963003/diff/40001/media/cdm/aes_decryptor_...
File media/cdm/aes_decryptor_unittest.cc (right):

https://codereview.chromium.org/833963003/diff/40001/media/cdm/aes_decryptor_...
media/cdm/aes_decryptor_unittest.cc:314: void SaveUsableKeys(Unused, Unused,
const CdmKeysInfo& usable_key_ids) {
On 2015/01/05 22:53:15, xhwang wrote:
> If you pass CdmKeysInfo by value, you can probably just use Pass() and
transfer
> the ownership over. Will that be simpler?

MOCK functions don't like ScopedVector, so changed to have a proper method for
OnSessionKeysChange() and a mock'd method OnSessionKeysChangeCalled().

https://codereview.chromium.org/833963003/diff/40001/media/cdm/aes_decryptor_...
media/cdm/aes_decryptor_unittest.cc:321:
usable_key_ids_.push_back(new_key_id.release());
On 2015/01/05 22:53:15, xhwang wrote:
> It seems here we save ALL key IDs regardless of it's status, i.e. even for
> non-usable key IDs. So "Usable" in the function name doesn't seem accurate.
> 
> How about we rename SaveUsableKeys to something like SaveKeysInfo?

Removed.

https://codereview.chromium.org/833963003/diff/40001/media/cdm/aes_decryptor_...
media/cdm/aes_decryptor_unittest.cc:345: bool
UsableKeyIdsContains(std::vector<uint8> expected) {
On 2015/01/05 22:53:15, xhwang wrote:
> Ditto about "Usable".

Done.

https://codereview.chromium.org/833963003/diff/40001/media/cdm/aes_decryptor_...
media/cdm/aes_decryptor_unittest.cc:425: CdmKeysInfo usable_key_ids_;
On 2015/01/05 22:53:15, xhwang wrote:
> ditto about "usable".

Done.

https://codereview.chromium.org/833963003/diff/40001/media/mojo/services/medi...
File media/mojo/services/media_type_converters.h (right):

https://codereview.chromium.org/833963003/diff/40001/media/mojo/services/medi...
media/mojo/services/media_type_converters.h:76: static CdmKeyInformationPtr
Convert(media::CdmKeyInformation* input);
On 2015/01/05 22:53:15, xhwang wrote:
> For input-only param we should use const-ref per style guide.

Done.

[xhwang, 2015-01-06 02:46:21]

LG, just have two minor comments.

https://codereview.chromium.org/833963003/diff/60001/media/base/cdm_key_infor...
File media/base/cdm_key_information.cc (right):

https://codereview.chromium.org/833963003/diff/60001/media/base/cdm_key_infor...
media/base/cdm_key_information.cc:9: CdmKeyInformation::CdmKeyInformation() :
status(USABLE), system_code(0) {
Since we don't have valid key_id yet, USABLE doesn't really make sense. How
about initialize the |status| to INTERNAL_ERROR?

https://codereview.chromium.org/833963003/diff/60001/media/mojo/services/mojo...
File media/mojo/services/mojo_cdm_service.cc (right):

https://codereview.chromium.org/833963003/diff/60001/media/mojo/services/mojo...
media/mojo/services/mojo_cdm_service.cc:118: for (media::CdmKeyInformation* key
: keys_info)
Will it work if you do:

for (const auto& key : keys_info) ?

[jrummell, 2015-01-06 17:37:03]

jrummell@chromium.org changed reviewers:
+ dmichael@chromium.org

[jrummell, 2015-01-06 17:37:05]

Updated.

+dmichael for OWNERS review of content/renderer/pepper/* (1 file)

https://codereview.chromium.org/833963003/diff/60001/media/base/cdm_key_infor...
File media/base/cdm_key_information.cc (right):

https://codereview.chromium.org/833963003/diff/60001/media/base/cdm_key_infor...
media/base/cdm_key_information.cc:9: CdmKeyInformation::CdmKeyInformation() :
status(USABLE), system_code(0) {
On 2015/01/06 02:46:21, xhwang wrote:
> Since we don't have valid key_id yet, USABLE doesn't really make sense. How
> about initialize the |status| to INTERNAL_ERROR?

Done.

https://codereview.chromium.org/833963003/diff/60001/media/mojo/services/mojo...
File media/mojo/services/mojo_cdm_service.cc (right):

https://codereview.chromium.org/833963003/diff/60001/media/mojo/services/mojo...
media/mojo/services/mojo_cdm_service.cc:118: for (media::CdmKeyInformation* key
: keys_info)
On 2015/01/06 02:46:21, xhwang wrote:
> Will it work if you do:
> 
> for (const auto& key : keys_info) ?

It does, but I thought using *key on the next line would be confusing. Done.

[xhwang, 2015-01-06 18:21:53]

lgtm!

[jrummell, 2015-01-07 23:07:15]

jrummell@chromium.org changed reviewers:
+ teravest@chromium.org

[jrummell, 2015-01-07 23:07:15]

+teravest for OWNERS review of minor changes in
content/renderer/pepper/content_decryptor_delegate.*, since dmichael is
backlogged.

[teravest, 2015-01-08 14:56:50]

lgtm

[jrummell, 2015-01-08 17:48:28]

The CQ bit was checked by jrummell@chromium.org

[commit-bot: I haz the power, 2015-01-08 17:49:43]

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

[commit-bot: I haz the power, 2015-01-08 18:43:35]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-01-08 18:44:57]

Patchset 5 (id:??) landed as
https://crrev.com/3d50dc21e899735f9c99596fc182552c6871c318
Cr-Commit-Position: refs/heads/master@{#310543}