Changes to support CDM_7

Changes to support CDM_7

Add support for CDM_7 and update Pepper to support the updated
interface.

Changes:
- Rename CreateSession() to CreateSessionAndGenerateRequest()
  with reordered parameter list.
- Add |session_type| to LoadSession().
- OnSessionMessage() adds |message_type|, removes |destination_url|.
- OnSessionKeysChange() adds |key_information|.
- Remove GetUsableKeyIds()
- Remove OnPromiseResolvedWithKeyIds()
- Remove OnSessionReady().

BUG=428384
TEST=existing EME test cases pass

Committed: https://crrev.com/a68c98fe5d09235cdfff39176964e10ac2074a6b
Cr-Commit-Position: refs/heads/master@{#310615}

[jrummell, 2014-12-17 00:25:44]

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

[jrummell, 2014-12-17 00:25:44]

PTAL.

[ddorwin, 2014-12-17 01:01:15]

I reviewed the interfaces and some other parts of the code. Please update then
get sandersd to review.

https://codereview.chromium.org/811923002/diff/1/content/renderer/pepper/cont...
File content/renderer/pepper/content_decryptor_delegate.cc (right):

https://codereview.chromium.org/811923002/diff/1/content/renderer/pepper/cont...
content/renderer/pepper/content_decryptor_delegate.cc:264: return
PP_SESSIONTYPE_PERSISTENT_LICENSE;
Add the new type too. I guess a TODO for now.

https://codereview.chromium.org/811923002/diff/1/content/renderer/pepper/cont...
content/renderer/pepper/content_decryptor_delegate.cc:397: pp_instance_,
promise_id, PP_SESSIONTYPE_TEMPORARY,
PP_SESSIONTYPE_PERSISTENT_LICENSE

https://codereview.chromium.org/811923002/diff/1/media/cdm/ppapi/cdm_adapter.cc
File media/cdm/ppapi/cdm_adapter.cc (right):

https://codereview.chromium.org/811923002/diff/1/media/cdm/ppapi/cdm_adapter....
media/cdm/ppapi/cdm_adapter.cc:725: // cdm::Host_6 only.
We should probably note where and how we provide "degraded" support. In this
case, all messages appear to be license requests.

Perhaps we should use kLicenseRenewal if the url is not empty?

https://codereview.chromium.org/811923002/diff/1/media/cdm/ppapi/cdm_adapter....
media/cdm/ppapi/cdm_adapter.cc:1149: cdm_->OnQueryOutputProtectionStatus(0, 0,
PP_ERROR_NOTSUPPORTED);
PP_NOTREACHED()?

https://codereview.chromium.org/811923002/diff/1/media/cdm/ppapi/cdm_adapter.h
File media/cdm/ppapi/cdm_adapter.h (right):

https://codereview.chromium.org/811923002/diff/1/media/cdm/ppapi/cdm_adapter....
media/cdm/ppapi/cdm_adapter.h:119: void OnSessionUsableKeysChange(const char*
session_id,
Drop "Usable". I guess we'll need to update CDM.h.

https://codereview.chromium.org/811923002/diff/1/media/cdm/ppapi/cdm_adapter....
media/cdm/ppapi/cdm_adapter.h:185: const std::string& web_session_id,
Still using "web_".

Note: This isn't consistently removed throughout the CL. It probably would have
been better to leave it and remove it throughout the entire stack in a separate
CL so that reviewers would see less diffs.

https://codereview.chromium.org/811923002/diff/1/media/cdm/ppapi/cdm_adapter....
media/cdm/ppapi/cdm_adapter.h:187: void SendSessionUsableKeysChangeInternal(
Drop Usable

https://codereview.chromium.org/811923002/diff/1/media/cdm/ppapi/external_cle...
File media/cdm/ppapi/external_clear_key/clear_key_cdm.cc (right):

https://codereview.chromium.org/811923002/diff/1/media/cdm/ppapi/external_cle...
media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:160: case
cdm::kPersistentKeyRelease:
TODO.

It might be better to have a separate return statement  so we can see which is
incorrect. At some point, we should reorder these to be in enum value.

https://codereview.chromium.org/811923002/diff/1/ppapi/api/private/ppb_conten...
File ppapi/api/private/ppb_content_decryptor_private.idl (right):

https://codereview.chromium.org/811923002/diff/1/ppapi/api/private/ppb_conten...
ppapi/api/private/ppb_content_decryptor_private.idl:111: *
<code>PP_VARTYPE_ARRAYBUFFER</code> that are the session's usable key IDs.
Update this line
Specifically, not just "usable" keys. And the type might be wrong too.

https://codereview.chromium.org/811923002/diff/1/ppapi/api/private/ppb_conten...
ppapi/api/private/ppb_content_decryptor_private.idl:135: /**
Wasn't SessionReady used for prefixed's keyadded event? If so, do we have
another way of triggering that event?

[jrummell, 2014-12-17 19:17:19]

Updated.

https://codereview.chromium.org/811923002/diff/1/content/renderer/pepper/cont...
File content/renderer/pepper/content_decryptor_delegate.cc (right):

https://codereview.chromium.org/811923002/diff/1/content/renderer/pepper/cont...
content/renderer/pepper/content_decryptor_delegate.cc:264: return
PP_SESSIONTYPE_PERSISTENT_LICENSE;
On 2014/12/17 01:01:14, ddorwin wrote:
> Add the new type too. I guess a TODO for now.

Done.

https://codereview.chromium.org/811923002/diff/1/content/renderer/pepper/cont...
content/renderer/pepper/content_decryptor_delegate.cc:397: pp_instance_,
promise_id, PP_SESSIONTYPE_TEMPORARY,
On 2014/12/17 01:01:15, ddorwin wrote:
> PP_SESSIONTYPE_PERSISTENT_LICENSE

Done.

https://codereview.chromium.org/811923002/diff/1/media/cdm/ppapi/cdm_adapter.cc
File media/cdm/ppapi/cdm_adapter.cc (right):

https://codereview.chromium.org/811923002/diff/1/media/cdm/ppapi/cdm_adapter....
media/cdm/ppapi/cdm_adapter.cc:725: // cdm::Host_6 only.
On 2014/12/17 01:01:15, ddorwin wrote:
> We should probably note where and how we provide "degraded" support. In this
> case, all messages appear to be license requests.
> 
> Perhaps we should use kLicenseRenewal if the url is not empty?

Done.

https://codereview.chromium.org/811923002/diff/1/media/cdm/ppapi/cdm_adapter....
media/cdm/ppapi/cdm_adapter.cc:1149: cdm_->OnQueryOutputProtectionStatus(0, 0,
PP_ERROR_NOTSUPPORTED);
On 2014/12/17 01:01:15, ddorwin wrote:
> PP_NOTREACHED()?

Since returning 0,0 is specified as the result if OP is not allowed, I would
think that CDMs can call this on non-CrOS systems. Is that not the case?

https://codereview.chromium.org/811923002/diff/1/media/cdm/ppapi/cdm_adapter.h
File media/cdm/ppapi/cdm_adapter.h (right):

https://codereview.chromium.org/811923002/diff/1/media/cdm/ppapi/cdm_adapter....
media/cdm/ppapi/cdm_adapter.h:119: void OnSessionUsableKeysChange(const char*
session_id,
On 2014/12/17 01:01:15, ddorwin wrote:
> Drop "Usable". I guess we'll need to update CDM.h.

Done. I'll update the DEPS once the CDM.h change is checked in.

https://codereview.chromium.org/811923002/diff/1/media/cdm/ppapi/cdm_adapter....
media/cdm/ppapi/cdm_adapter.h:185: const std::string& web_session_id,
On 2014/12/17 01:01:15, ddorwin wrote:
> Still using "web_".
> 
> Note: This isn't consistently removed throughout the CL. It probably would
have
> been better to leave it and remove it throughout the entire stack in a
separate
> CL so that reviewers would see less diffs.

Acknowledged.

https://codereview.chromium.org/811923002/diff/1/media/cdm/ppapi/cdm_adapter....
media/cdm/ppapi/cdm_adapter.h:187: void SendSessionUsableKeysChangeInternal(
On 2014/12/17 01:01:15, ddorwin wrote:
> Drop Usable

Done.

https://codereview.chromium.org/811923002/diff/1/media/cdm/ppapi/external_cle...
File media/cdm/ppapi/external_clear_key/clear_key_cdm.cc (right):

https://codereview.chromium.org/811923002/diff/1/media/cdm/ppapi/external_cle...
media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:160: case
cdm::kPersistentKeyRelease:
On 2014/12/17 01:01:15, ddorwin wrote:
> TODO.
> 
> It might be better to have a separate return statement  so we can see which is
> incorrect. At some point, we should reorder these to be in enum value.

Done.

https://codereview.chromium.org/811923002/diff/1/ppapi/api/private/ppb_conten...
File ppapi/api/private/ppb_content_decryptor_private.idl (right):

https://codereview.chromium.org/811923002/diff/1/ppapi/api/private/ppb_conten...
ppapi/api/private/ppb_content_decryptor_private.idl:111: *
<code>PP_VARTYPE_ARRAYBUFFER</code> that are the session's usable key IDs.
On 2014/12/17 01:01:15, ddorwin wrote:
> Update this line
> Specifically, not just "usable" keys. And the type might be wrong too.

Done.

https://codereview.chromium.org/811923002/diff/1/ppapi/api/private/ppb_conten...
ppapi/api/private/ppb_content_decryptor_private.idl:135: /**
On 2014/12/17 01:01:15, ddorwin wrote:
> Wasn't SessionReady used for prefixed's keyadded event? If so, do we have
> another way of triggering that event?

Neither CDM_6 nor CDM_7 generate the ready event. Currently ready is generated
for the prefixed API when the update() promise is resolved
(https://code.google.com/p/chromium/codesearch#chromium/src/media/cdm/proxy_de...).

[sandersd (OOO until July 31), 2014-12-17 21:14:07]

lgtm

https://codereview.chromium.org/811923002/diff/20001/content/renderer/pepper/...
File content/renderer/pepper/content_decryptor_delegate.cc (right):

https://codereview.chromium.org/811923002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/content_decryptor_delegate.cc:325: // TODO(jrummell):
Remove |session_ready_cb| and |session_keys_change_cb|.
When?

https://codereview.chromium.org/811923002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/content_decryptor_delegate.cc:376: // TODO(jrummell):
Rename method and reorder parameter list.
Rename to what?

https://codereview.chromium.org/811923002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/content_decryptor_delegate.cc:446: // TODO(jrummell):
Remove this method.
When?

https://codereview.chromium.org/811923002/diff/20001/media/cdm/ppapi/cdm_adap...
File media/cdm/ppapi/cdm_adapter.cc (right):

https://codereview.chromium.org/811923002/diff/20001/media/cdm/ppapi/cdm_adap...
media/cdm/ppapi/cdm_adapter.cc:27: const uint32_t kOutputProtectionCheckFailed =
1;  // must be non-zero.
Define kOutputProtectionCheckSucceeded as well?

https://codereview.chromium.org/811923002/diff/20001/media/cdm/ppapi/cdm_adap...
media/cdm/ppapi/cdm_adapter.cc:754: PP_KeyInformation next_key;
'= {0}' to avoid the memset?

https://codereview.chromium.org/811923002/diff/20001/media/cdm/ppapi/cdm_adap...
media/cdm/ppapi/cdm_adapter.cc:765: memcpy(&next_key.key_id, key_info.key_id,
key_info.key_id_size);
Shouldn't need & to copy into an array. It doesn't make a difference to
memcpy(), so I'm not sure which is more technically correct.

[jrummell, 2014-12-17 22:57:39]

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

[jrummell, 2014-12-17 22:57:40]

+dmichael@ for all the Pepper code
+dcheng@ for OWNERS review of ppapi/proxy/ppapi_messages.h

https://codereview.chromium.org/811923002/diff/20001/content/renderer/pepper/...
File content/renderer/pepper/content_decryptor_delegate.cc (right):

https://codereview.chromium.org/811923002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/content_decryptor_delegate.cc:325: // TODO(jrummell):
Remove |session_ready_cb| and |session_keys_change_cb|.
On 2014/12/17 21:14:07, sandersd wrote:
> When?

Next CL when these changes pass up through media::MediaKeys.

https://codereview.chromium.org/811923002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/content_decryptor_delegate.cc:376: // TODO(jrummell):
Rename method and reorder parameter list.
On 2014/12/17 21:14:07, sandersd wrote:
> Rename to what?

Updated.

https://codereview.chromium.org/811923002/diff/20001/content/renderer/pepper/...
content/renderer/pepper/content_decryptor_delegate.cc:446: // TODO(jrummell):
Remove this method.
On 2014/12/17 21:14:07, sandersd wrote:
> When?

When media::MediaKeys no longer specifies it.

https://codereview.chromium.org/811923002/diff/20001/media/cdm/ppapi/cdm_adap...
File media/cdm/ppapi/cdm_adapter.cc (right):

https://codereview.chromium.org/811923002/diff/20001/media/cdm/ppapi/cdm_adap...
media/cdm/ppapi/cdm_adapter.cc:27: const uint32_t kOutputProtectionCheckFailed =
1;  // must be non-zero.
On 2014/12/17 21:14:07, sandersd wrote:
> Define kOutputProtectionCheckSucceeded as well?

Removed due to CDM_7 changes.

https://codereview.chromium.org/811923002/diff/20001/media/cdm/ppapi/cdm_adap...
media/cdm/ppapi/cdm_adapter.cc:754: PP_KeyInformation next_key;
On 2014/12/17 21:14:07, sandersd wrote:
> '= {0}' to avoid the memset?

Done (although I think it should be {}).

https://codereview.chromium.org/811923002/diff/20001/media/cdm/ppapi/cdm_adap...
media/cdm/ppapi/cdm_adapter.cc:765: memcpy(&next_key.key_id, key_info.key_id,
key_info.key_id_size);
On 2014/12/17 21:14:07, sandersd wrote:
> Shouldn't need & to copy into an array. It doesn't make a difference to
> memcpy(), so I'm not sure which is more technically correct.

Done.

[ddorwin, 2014-12-17 23:13:44]

Interfaces LGTM with nits. Thanks.

https://codereview.chromium.org/811923002/diff/40001/media/cdm/ppapi/cdm_adap...
File media/cdm/ppapi/cdm_adapter.cc (right):

https://codereview.chromium.org/811923002/diff/40001/media/cdm/ppapi/cdm_adap...
media/cdm/ppapi/cdm_adapter.cc:1278: CDM_DLOG() << __FUNCTION__ << " failed,
result = " << result;
nit: Log first.

https://codereview.chromium.org/811923002/diff/40001/media/cdm/ppapi/cdm_wrap...
File media/cdm/ppapi/cdm_wrapper.h (right):

https://codereview.chromium.org/811923002/diff/40001/media/cdm/ppapi/cdm_wrap...
media/cdm/ppapi/cdm_wrapper.h:265:
cdm_->OnQueryOutputProtectionStatus(link_mask, output_protection_mask);
Should we DCHECK(result==SUCCESS | mask == 0 && maks == 0)?

[jrummell, 2014-12-18 19:27:41]

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

[jrummell, 2014-12-18 19:27:42]

+isherman@ for OWNERS review of histogram.xml

[jrummell, 2014-12-18 20:26:20]

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

[jrummell, 2014-12-18 20:26:21]

+bbudge@ for OWNERS review of pepper code since dmicheal@ is out

[dcheng, 2014-12-18 22:35:02]

dcheng@chromium.org changed reviewers:
+ dmichael@chromium.org
- bbudge@chromium.org

[dcheng, 2014-12-18 22:35:04]

For future reviews, it would be helpful for cleanup-only changes, such as the
virtual and default enum cleanups, to be in a separate patch.

https://codereview.chromium.org/811923002/diff/40001/media/cdm/ppapi/cdm_adap...
File media/cdm/ppapi/cdm_adapter.cc (right):

https://codereview.chromium.org/811923002/diff/40001/media/cdm/ppapi/cdm_adap...
media/cdm/ppapi/cdm_adapter.cc:735: cdm::MessageType message_type =
(destination_url_size)
Nit: > 0

https://codereview.chromium.org/811923002/diff/40001/media/cdm/ppapi/cdm_adap...
media/cdm/ppapi/cdm_adapter.cc:812: std::string(error_message,
error_message_size))));
By my count, this string gets created 3x =P

Not much we can do atm though. You could make SessionError take a StringPiece,
but I don't really know that that'd make this much nicer looking.

https://codereview.chromium.org/811923002/diff/40001/ppapi/cpp/private/conten...
File ppapi/cpp/private/content_decryptor_private.cc (right):

https://codereview.chromium.org/811923002/diff/40001/ppapi/cpp/private/conten...
ppapi/cpp/private/content_decryptor_private.cc:331: auto entry =
key_information[i];
Isn't this a copy? Qualify this with const auto&.

https://codereview.chromium.org/811923002/diff/40001/ppapi/cpp/private/conten...
ppapi/cpp/private/content_decryptor_private.cc:332: pp::VarArrayBuffer
array_buffer(sizeof(PP_KeyInformation));
You could use sizeof(entry) here to make this "future" proof, in case the
std::vector<> ever holds something that's not a PP_KeyInformation. I don't know
if that's a common pattern in ppapi code though.

https://codereview.chromium.org/811923002/diff/40001/ppapi/proxy/ppb_instance...
File ppapi/proxy/ppb_instance_proxy.cc (right):

https://codereview.chromium.org/811923002/diff/40001/ppapi/proxy/ppb_instance...
ppapi/proxy/ppb_instance_proxy.cc:1336: sizeof(PP_KeyInformation), &info));
Ditto: sizeof(info) instead of sizeof(PP_KeyInformation).

[Ilya Sherman, 2014-12-18 22:39:06]

histograms.xml lgtm

[jrummell, 2014-12-18 23:47:59]

Updated.

Sorry there is also a rebase as I needed to get the latest for another CL. It
only affects DEPS and pepper_plugin_instance_impl.cc, which have no changes in
this PS.

https://codereview.chromium.org/811923002/diff/40001/media/cdm/ppapi/cdm_adap...
File media/cdm/ppapi/cdm_adapter.cc (right):

https://codereview.chromium.org/811923002/diff/40001/media/cdm/ppapi/cdm_adap...
media/cdm/ppapi/cdm_adapter.cc:735: cdm::MessageType message_type =
(destination_url_size)
On 2014/12/18 22:35:03, dcheng wrote:
> Nit: > 0

Done.

https://codereview.chromium.org/811923002/diff/40001/media/cdm/ppapi/cdm_adap...
media/cdm/ppapi/cdm_adapter.cc:812: std::string(error_message,
error_message_size))));
On 2014/12/18 22:35:03, dcheng wrote:
> By my count, this string gets created 3x =P
> 
> Not much we can do atm though. You could make SessionError take a StringPiece,
> but I don't really know that that'd make this much nicer looking.

Acknowledged.

https://codereview.chromium.org/811923002/diff/40001/media/cdm/ppapi/cdm_adap...
media/cdm/ppapi/cdm_adapter.cc:1278: CDM_DLOG() << __FUNCTION__ << " failed,
result = " << result;
On 2014/12/17 23:13:44, ddorwin wrote:
> nit: Log first.

Done.

https://codereview.chromium.org/811923002/diff/40001/media/cdm/ppapi/cdm_wrap...
File media/cdm/ppapi/cdm_wrapper.h (right):

https://codereview.chromium.org/811923002/diff/40001/media/cdm/ppapi/cdm_wrap...
media/cdm/ppapi/cdm_wrapper.h:265:
cdm_->OnQueryOutputProtectionStatus(link_mask, output_protection_mask);
On 2014/12/17 23:13:44, ddorwin wrote:
> Should we DCHECK(result==SUCCESS | mask == 0 && maks == 0)?

Good idea, since we state that if result != SUCCESS, the values are undefined.
Done as code.

https://codereview.chromium.org/811923002/diff/40001/ppapi/cpp/private/conten...
File ppapi/cpp/private/content_decryptor_private.cc (right):

https://codereview.chromium.org/811923002/diff/40001/ppapi/cpp/private/conten...
ppapi/cpp/private/content_decryptor_private.cc:331: auto entry =
key_information[i];
On 2014/12/18 22:35:03, dcheng wrote:
> Isn't this a copy? Qualify this with const auto&.

Done.

https://codereview.chromium.org/811923002/diff/40001/ppapi/cpp/private/conten...
ppapi/cpp/private/content_decryptor_private.cc:332: pp::VarArrayBuffer
array_buffer(sizeof(PP_KeyInformation));
On 2014/12/18 22:35:03, dcheng wrote:
> You could use sizeof(entry) here to make this "future" proof, in case the
> std::vector<> ever holds something that's not a PP_KeyInformation. I don't
know
> if that's a common pattern in ppapi code though.

Done.

https://codereview.chromium.org/811923002/diff/40001/ppapi/proxy/ppb_instance...
File ppapi/proxy/ppb_instance_proxy.cc (right):

https://codereview.chromium.org/811923002/diff/40001/ppapi/proxy/ppb_instance...
ppapi/proxy/ppb_instance_proxy.cc:1336: sizeof(PP_KeyInformation), &info));
On 2014/12/18 22:35:03, dcheng wrote:
> Ditto: sizeof(info) instead of sizeof(PP_KeyInformation).

Done.

[bbudge, 2014-12-19 18:22:51]

bbudge@chromium.org changed reviewers:
+ bbudge@chromium.org

[bbudge, 2014-12-19 18:22:52]

Just a single comment. Deferring to dmichael for the ppapi review.

https://codereview.chromium.org/811923002/diff/40001/ppapi/api/private/ppp_co...
File ppapi/api/private/ppp_content_decryptor_private.idl (right):

https://codereview.chromium.org/811923002/diff/40001/ppapi/api/private/ppp_co...
ppapi/api/private/ppp_content_decryptor_private.idl:49: * Creates a session.
<code>init_data_type</code> contains the MIME type of
Could you add some commenting here about what 'GenerateRequest' means?

[dmichael (off chromium), 2014-12-19 18:50:04]

Sorry for being slow.

https://codereview.chromium.org/811923002/diff/40001/ppapi/c/private/pp_conte...
File ppapi/c/private/pp_content_decryptor.h (right):

https://codereview.chromium.org/811923002/diff/40001/ppapi/c/private/pp_conte...
ppapi/c/private/pp_content_decryptor.h:484: } PP_MessageType;
Could you do a slightly more specific name, like PP_CDM_MessageType?

https://codereview.chromium.org/811923002/diff/40001/ppapi/c/private/pp_conte...
ppapi/c/private/pp_content_decryptor.h:495: } PP_KeyStatus;
ditto

https://codereview.chromium.org/811923002/diff/60001/content/renderer/pepper/...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/811923002/diff/60001/content/renderer/pepper/...
content/renderer/pepper/pepper_plugin_instance_impl.cc:2431: web_session_id_var,
has_additional_usable_key, key_information_var);
If you haven't run git cl format, please do...  I didn't think it would format
this line this way anymore.

https://codereview.chromium.org/811923002/diff/60001/ppapi/proxy/ppb_instance...
File ppapi/proxy/ppb_instance_proxy.cc (right):

https://codereview.chromium.org/811923002/diff/60001/ppapi/proxy/ppb_instance...
ppapi/proxy/ppb_instance_proxy.cc:627: std::vector<std::string>
serialized_key_info;
nit: Would it be worth reserve(), since you know the length?

https://codereview.chromium.org/811923002/diff/60001/ppapi/proxy/ppb_instance...
ppapi/proxy/ppb_instance_proxy.cc:633: continue;
I know CDM is a little different, but we generally assume that plugins can give
us bad data. Is it going to be OK with everything downstream if you leave out an
entry from the vector? Or would it be better to bail out completely (i.e.,
return)?

https://codereview.chromium.org/811923002/diff/60001/ppapi/proxy/ppb_instance...
ppapi/proxy/ppb_instance_proxy.cc:641: continue;
same question

https://codereview.chromium.org/811923002/diff/60001/ppapi/proxy/ppb_instance...
ppapi/proxy/ppb_instance_proxy.cc:650:
serialized_key_info.push_back(serialized_info);
So why are you doing your own serialization? Can't you just use
SerializedVarSendInput?

[jrummell, 2014-12-20 00:23:09]

Updated.

https://codereview.chromium.org/811923002/diff/40001/ppapi/api/private/ppp_co...
File ppapi/api/private/ppp_content_decryptor_private.idl (right):

https://codereview.chromium.org/811923002/diff/40001/ppapi/api/private/ppp_co...
ppapi/api/private/ppp_content_decryptor_private.idl:49: * Creates a session.
<code>init_data_type</code> contains the MIME type of
On 2014/12/19 18:22:52, bbudge wrote:
> Could you add some commenting here about what 'GenerateRequest' means?

Done.

https://codereview.chromium.org/811923002/diff/40001/ppapi/c/private/pp_conte...
File ppapi/c/private/pp_content_decryptor.h (right):

https://codereview.chromium.org/811923002/diff/40001/ppapi/c/private/pp_conte...
ppapi/c/private/pp_content_decryptor.h:484: } PP_MessageType;
On 2014/12/19 18:50:04, dmichael (OOO until Jan 2nd) wrote:
> Could you do a slightly more specific name, like PP_CDM_MessageType?

Done.

https://codereview.chromium.org/811923002/diff/40001/ppapi/c/private/pp_conte...
ppapi/c/private/pp_content_decryptor.h:495: } PP_KeyStatus;
On 2014/12/19 18:50:04, dmichael (OOO until Jan 2nd) wrote:
> ditto

Done.

https://codereview.chromium.org/811923002/diff/60001/content/renderer/pepper/...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/811923002/diff/60001/content/renderer/pepper/...
content/renderer/pepper/pepper_plugin_instance_impl.cc:2431: web_session_id_var,
has_additional_usable_key, key_information_var);
On 2014/12/19 18:50:04, dmichael (OOO until Jan 2nd) wrote:
> If you haven't run git cl format, please do...  I didn't think it would format
> this line this way anymore.

This is what clang-format did.

https://codereview.chromium.org/811923002/diff/60001/ppapi/proxy/ppb_instance...
File ppapi/proxy/ppb_instance_proxy.cc (right):

https://codereview.chromium.org/811923002/diff/60001/ppapi/proxy/ppb_instance...
ppapi/proxy/ppb_instance_proxy.cc:627: std::vector<std::string>
serialized_key_info;
On 2014/12/19 18:50:04, dmichael (OOO until Jan 2nd) wrote:
> nit: Would it be worth reserve(), since you know the length?

Done.

https://codereview.chromium.org/811923002/diff/60001/ppapi/proxy/ppb_instance...
ppapi/proxy/ppb_instance_proxy.cc:633: continue;
On 2014/12/19 18:50:04, dmichael (OOO until Jan 2nd) wrote:
> I know CDM is a little different, but we generally assume that plugins can
give
> us bad data. Is it going to be OK with everything downstream if you leave out
an
> entry from the vector? Or would it be better to bail out completely (i.e.,
> return)?

Done. I figured that returning partial data would be OK, but if the data is
messed up, we might as well quit rather than trying to interpret what comes
later.

https://codereview.chromium.org/811923002/diff/60001/ppapi/proxy/ppb_instance...
ppapi/proxy/ppb_instance_proxy.cc:641: continue;
On 2014/12/19 18:50:04, dmichael (OOO until Jan 2nd) wrote:
> same question

Done.

https://codereview.chromium.org/811923002/diff/60001/ppapi/proxy/ppb_instance...
ppapi/proxy/ppb_instance_proxy.cc:650:
serialized_key_info.push_back(serialized_info);
On 2014/12/19 18:50:04, dmichael (OOO until Jan 2nd) wrote:
> So why are you doing your own serialization? Can't you just use
> SerializedVarSendInput?

I just copied what was done elsewhere (e.g. DeliverBlock). I just tried
SerializedVarSendInput(), but new_key is not a PP_VAR, just a struct.

[jrummell, 2015-01-06 00:07:16]

rebase (and a way to get this back on everybody's schedule now that the
Christmas break is over).

[dmichael (off chromium), 2015-01-07 23:05:50]

https://codereview.chromium.org/811923002/diff/60001/ppapi/proxy/ppb_instance...
File ppapi/proxy/ppb_instance_proxy.cc (right):

https://codereview.chromium.org/811923002/diff/60001/ppapi/proxy/ppb_instance...
ppapi/proxy/ppb_instance_proxy.cc:650:
serialized_key_info.push_back(serialized_info);
On 2014/12/20 00:23:09, jrummell wrote:
> On 2014/12/19 18:50:04, dmichael (OOO until Jan 2nd) wrote:
> > So why are you doing your own serialization? Can't you just use
> > SerializedVarSendInput?
> 
> I just copied what was done elsewhere (e.g. DeliverBlock). I just tried
> SerializedVarSendInput(), but new_key is not a PP_VAR, just a struct.
But key_information_var is a PP_Var. Honestly, the API taking a PP_Var but
expecting a very particular value is kind of strange. Why is the argument to the
API not just a PP_KeyInformation array and size?

[jrummell, 2015-01-08 03:12:29]

Updated to pass an array. PTAL.

Also includes a rebase.

https://codereview.chromium.org/811923002/diff/60001/ppapi/proxy/ppb_instance...
File ppapi/proxy/ppb_instance_proxy.cc (right):

https://codereview.chromium.org/811923002/diff/60001/ppapi/proxy/ppb_instance...
ppapi/proxy/ppb_instance_proxy.cc:650:
serialized_key_info.push_back(serialized_info);
On 2015/01/07 23:05:50, dmichael (OOO until Jan 2nd) wrote:
> On 2014/12/20 00:23:09, jrummell wrote:
> > On 2014/12/19 18:50:04, dmichael (OOO until Jan 2nd) wrote:
> > > So why are you doing your own serialization? Can't you just use
> > > SerializedVarSendInput?
> > 
> > I just copied what was done elsewhere (e.g. DeliverBlock). I just tried
> > SerializedVarSendInput(), but new_key is not a PP_VAR, just a struct.
> But key_information_var is a PP_Var. Honestly, the API taking a PP_Var but
> expecting a very particular value is kind of strange. Why is the argument to
the
> API not just a PP_KeyInformation array and size?

Done.

[dmichael (off chromium), 2015-01-08 18:23:18]

lgtm, thanks!

https://codereview.chromium.org/811923002/diff/120001/ppapi/proxy/ppapi_param...
File ppapi/proxy/ppapi_param_traits.cc (right):

https://codereview.chromium.org/811923002/diff/120001/ppapi/proxy/ppapi_param...
ppapi/proxy/ppapi_param_traits.cc:107: WriteParam(m, p.system_code);
Did you try using the IPC_STRUCT_TRAITS_BEGIN/END stuff like this?
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/ppapi_...
I'm not certain it works with a fixed array like key_id, but *if* it does, it
would be simpler. (I'm guessing you tried that and it didn't work, though, in
which case ParamTraits is appropriate).

[jrummell, 2015-01-08 18:40:50]

ping dcheng@ for OWNERS review of ppapi/proxy/ppapi_messages.h

https://codereview.chromium.org/811923002/diff/120001/ppapi/proxy/ppapi_param...
File ppapi/proxy/ppapi_param_traits.cc (right):

https://codereview.chromium.org/811923002/diff/120001/ppapi/proxy/ppapi_param...
ppapi/proxy/ppapi_param_traits.cc:107: WriteParam(m, p.system_code);
On 2015/01/08 18:23:18, dmichael wrote:
> Did you try using the IPC_STRUCT_TRAITS_BEGIN/END stuff like this?
>
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/ppapi_...
> I'm not certain it works with a fixed array like key_id, but *if* it does, it
> would be simpler. (I'm guessing you tried that and it didn't work, though, in
> which case ParamTraits is appropriate).

I did try it, but got compile errors with ParamTraits<uint8_t[512]>. Tried
char[512] too, in case it was supported, but it failed too.

[dcheng, 2015-01-08 19:12:31]

lgtm with comments addressed

Sorry, I've been reviewing this for some time but the diff is quite large. If
it's possible, breaking up patches like this into smaller chunks in the future
(and not including cleanup changes) will make reviews go much more quickly.

https://codereview.chromium.org/811923002/diff/120001/media/cdm/ppapi/cdm_ada...
File media/cdm/ppapi/cdm_adapter.cc (right):

https://codereview.chromium.org/811923002/diff/120001/media/cdm/ppapi/cdm_ada...
media/cdm/ppapi/cdm_adapter.cc:722: std::vector<uint8>(message, message +
message_size)));
uint8_t

https://codereview.chromium.org/811923002/diff/120001/media/cdm/ppapi/cdm_ada...
media/cdm/ppapi/cdm_adapter.cc:846: const std::vector<uint8>& message) {
uint8_t

https://codereview.chromium.org/811923002/diff/120001/media/cdm/ppapi/cdm_ada...
File media/cdm/ppapi/cdm_adapter.h (right):

https://codereview.chromium.org/811923002/diff/120001/media/cdm/ppapi/cdm_ada...
media/cdm/ppapi/cdm_adapter.h:182: const std::vector<uint8>& message);
uint8_t for consistency?

https://codereview.chromium.org/811923002/diff/120001/media/cdm/ppapi/cdm_ada...
media/cdm/ppapi/cdm_adapter.h:190: std::vector<PP_KeyInformation> key_info);
Pass by const reference.

https://codereview.chromium.org/811923002/diff/120001/ppapi/proxy/ppapi_param...
File ppapi/proxy/ppapi_param_traits.cc (right):

https://codereview.chromium.org/811923002/diff/120001/ppapi/proxy/ppapi_param...
ppapi/proxy/ppapi_param_traits.cc:114: uint32 size;
uint32_t

https://codereview.chromium.org/811923002/diff/120001/ppapi/proxy/ppapi_param...
ppapi/proxy/ppapi_param_traits.cc:131: uint32 system_code;
Here too.

https://codereview.chromium.org/811923002/diff/120001/ppapi/proxy/ppb_instanc...
File ppapi/proxy/ppb_instance_proxy.cc (right):

https://codereview.chromium.org/811923002/diff/120001/ppapi/proxy/ppb_instanc...
ppapi/proxy/ppb_instance_proxy.cc:1307: key_information.empty() ? NULL :
&key_information[0]);
I think you can use vector_as_array() here.

https://codereview.chromium.org/811923002/diff/120001/ppapi/proxy/ppb_instanc...
ppapi/proxy/ppb_instance_proxy.cc:1307: key_information.empty() ? NULL :
&key_information[0]);
It might be slightly clearer to say vector_as_array(&key_information) here.

[jrummell, 2015-01-08 20:12:12]

Thanks for the reviews.

https://codereview.chromium.org/811923002/diff/120001/media/cdm/ppapi/cdm_ada...
File media/cdm/ppapi/cdm_adapter.cc (right):

https://codereview.chromium.org/811923002/diff/120001/media/cdm/ppapi/cdm_ada...
media/cdm/ppapi/cdm_adapter.cc:722: std::vector<uint8>(message, message +
message_size)));
On 2015/01/08 19:12:30, dcheng wrote:
> uint8_t

Done.

https://codereview.chromium.org/811923002/diff/120001/media/cdm/ppapi/cdm_ada...
media/cdm/ppapi/cdm_adapter.cc:846: const std::vector<uint8>& message) {
On 2015/01/08 19:12:30, dcheng wrote:
> uint8_t

Done.

https://codereview.chromium.org/811923002/diff/120001/media/cdm/ppapi/cdm_ada...
File media/cdm/ppapi/cdm_adapter.h (right):

https://codereview.chromium.org/811923002/diff/120001/media/cdm/ppapi/cdm_ada...
media/cdm/ppapi/cdm_adapter.h:182: const std::vector<uint8>& message);
On 2015/01/08 19:12:30, dcheng wrote:
> uint8_t for consistency?

Done.

https://codereview.chromium.org/811923002/diff/120001/media/cdm/ppapi/cdm_ada...
media/cdm/ppapi/cdm_adapter.h:190: std::vector<PP_KeyInformation> key_info);
On 2015/01/08 19:12:30, dcheng wrote:
> Pass by const reference.

Done.

https://codereview.chromium.org/811923002/diff/120001/ppapi/proxy/ppapi_param...
File ppapi/proxy/ppapi_param_traits.cc (right):

https://codereview.chromium.org/811923002/diff/120001/ppapi/proxy/ppapi_param...
ppapi/proxy/ppapi_param_traits.cc:114: uint32 size;
On 2015/01/08 19:12:31, dcheng wrote:
> uint32_t

Done.

https://codereview.chromium.org/811923002/diff/120001/ppapi/proxy/ppapi_param...
ppapi/proxy/ppapi_param_traits.cc:131: uint32 system_code;
On 2015/01/08 19:12:31, dcheng wrote:
> Here too.

Done.

https://codereview.chromium.org/811923002/diff/120001/ppapi/proxy/ppb_instanc...
File ppapi/proxy/ppb_instance_proxy.cc (right):

https://codereview.chromium.org/811923002/diff/120001/ppapi/proxy/ppb_instanc...
ppapi/proxy/ppb_instance_proxy.cc:1307: key_information.empty() ? NULL :
&key_information[0]);
On 2015/01/08 19:12:31, dcheng wrote:
> It might be slightly clearer to say vector_as_array(&key_information) here.

Done.

https://codereview.chromium.org/811923002/diff/120001/ppapi/proxy/ppb_instanc...
ppapi/proxy/ppb_instance_proxy.cc:1307: key_information.empty() ? NULL :
&key_information[0]);
On 2015/01/08 19:12:31, dcheng wrote:
> I think you can use vector_as_array() here.

Done.

[jrummell, 2015-01-08 20:12:32]

The CQ bit was checked by jrummell@chromium.org

[commit-bot: I haz the power, 2015-01-08 20:14:35]

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

[commit-bot: I haz the power, 2015-01-08 20:56:35]

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

[commit-bot: I haz the power, 2015-01-08 20:56:37]

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

[jrummell, 2015-01-08 21:30:10]

The CQ bit was checked by jrummell@chromium.org

[commit-bot: I haz the power, 2015-01-08 21:31:29]

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

[commit-bot: I haz the power, 2015-01-08 22:36:35]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-01-08 22:37:23]

Patchset 9 (id:??) landed as
https://crrev.com/a68c98fe5d09235cdfff39176964e10ac2074a6b
Cr-Commit-Position: refs/heads/master@{#310615}