Rename EME WD call parameters

Rename EME WD call parameters

Rename parameters on EME WD calls to be consistent.

BUG=224786
TEST=compiles

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=239698

[jrummell, 2013-12-04 19:57:21]

PTAL. Lots of files, but mostly trivial renames.

[ddorwin, 2013-12-05 00:44:52]

Thanks!

https://codereview.chromium.org/105383002/diff/1/content/browser/media/androi...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/105383002/diff/1/content/browser/media/androi...
content/browser/media/android/browser_media_player_manager.cc:401: routing_id(),
media_keys_id, session_id, error_code, system_code));
This doesn't seem like the right formatting.

https://codereview.chromium.org/105383002/diff/1/content/common/media/media_p...
File content/common/media/media_player_messages_android.h (right):

https://codereview.chromium.org/105383002/diff/1/content/common/media/media_p...
content/common/media/media_player_messages_android.h:322:
IPC_MESSAGE_ROUTED3(MediaKeysMsg_SetSessionId,
Are these all being renamed when they are extracted?

https://codereview.chromium.org/105383002/diff/1/content/renderer/media/crypt...
File content/renderer/media/crypto/proxy_decryptor.h (right):

https://codereview.chromium.org/105383002/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/proxy_decryptor.h:48: // web session ID rather
than a session ID.
s/a/the internal/

https://codereview.chromium.org/105383002/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/proxy_decryptor.h:89: // This is session_id <->
web_session_id map.
remove "This is ". (It's missing an article anyway.)

https://codereview.chromium.org/105383002/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/proxy_decryptor.h:108: uint32
LookupReferenceId(const std::string& web_session_id);
LookupSessionId

https://codereview.chromium.org/105383002/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/proxy_decryptor.h:113: const std::string&
LookupSessionId(uint32 session_id);
Lookup*Web*SessionId

https://codereview.chromium.org/105383002/diff/1/content/renderer/media/webco...
File content/renderer/media/webcontentdecryptionmodule_impl.cc (right):

https://codereview.chromium.org/105383002/diff/1/content/renderer/media/webco...
content/renderer/media/webcontentdecryptionmodule_impl.cc:23: // Forwards the
reference ID-based callbacks of the MediaKeys interface to the
update

https://codereview.chromium.org/105383002/diff/1/content/renderer/media/webco...
content/renderer/media/webcontentdecryptionmodule_impl.cc:25: class
ReferenceIdAdapter {
Rename. TODO?

https://codereview.chromium.org/105383002/diff/1/content/renderer/media/webco...
File content/renderer/media/webcontentdecryptionmodulesession_impl.cc (right):

https://codereview.chromium.org/105383002/diff/1/content/renderer/media/webco...
content/renderer/media/webcontentdecryptionmodulesession_impl.cc:73:
web_session_id_ = web_session_id;
If we don't need the accessor, only sessionId() uses this. We can probably just
convert this to a WebString here. That also makes it even more obvious what it
is.

https://codereview.chromium.org/105383002/diff/1/content/renderer/media/webco...
File content/renderer/media/webcontentdecryptionmodulesession_impl.h (right):

https://codereview.chromium.org/105383002/diff/1/content/renderer/media/webco...
content/renderer/media/webcontentdecryptionmodulesession_impl.h:42: const
std::string& web_session_id() const { return web_session_id_; }
Because this is so confusing, I think we should add (somewhat duplicate)
comments to these accessors as well.

https://codereview.chromium.org/105383002/diff/1/content/renderer/media/webco...
content/renderer/media/webcontentdecryptionmodulesession_impl.h:42: const
std::string& web_session_id() const { return web_session_id_; }
Is anyone using this? If not, remove it. (This is really a duplicate of
sessionId().)

https://codereview.chromium.org/105383002/diff/1/content/renderer/media/webco...
content/renderer/media/webcontentdecryptionmodulesession_impl.h:44: uint32
session_id() const { return session_id_; }
I'd prefer to not have this public session_id(), but webcdm_impl.cc uses it.

We might be able to address this by having the ReferenceIdAdapter generate the
IDs and pass them to this class's constructor. Maybe we'll do that in a separate
CL. TODO?

https://codereview.chromium.org/105383002/diff/1/content/renderer/media/webco...
content/renderer/media/webcontentdecryptionmodulesession_impl.h:61: // Session
ID is the user visible ID for this session generated by the CDM.
Update.
Also, s/user /app-/

https://codereview.chromium.org/105383002/diff/1/content/renderer/media/webco...
content/renderer/media/webcontentdecryptionmodulesession_impl.h:65: // Reference
ID is used to uniquely track this object so that CDM callbacks
update

https://codereview.chromium.org/105383002/diff/1/media/base/android/media_drm...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/105383002/diff/1/media/base/android/media_drm...
media/base/android/media_drm_bridge.cc:318: std::string web_session_id =
ConvertJavaStringToUTF8(env, j_session_id);
Is MediaDrm passing us sessionId instead of "reference ID"? So, this will be the
last place where we don't have "reference ID", even after eliminating the
prefixed API?

https://codereview.chromium.org/105383002/diff/1/media/base/android/media_drm...
File media/base/android/media_drm_bridge.h (right):

https://codereview.chromium.org/105383002/diff/1/media/base/android/media_drm...
media/base/android/media_drm_bridge.h:89: void OnKeyAdded(JNIEnv* env, jobject,
jstring j_session_id);
We should comment on j_session_id being a web_session_id. And maybe even rename
it. See comment in .cc file.
ditto below.

https://codereview.chromium.org/105383002/diff/1/media/base/android/media_drm...
media/base/android/media_drm_bridge.h:124: uint32_t DetermineReferenceId(const
std::string& web_session_id);
Update these function names too. (If it simplifies things, you can do this as a
separate Android-only CL.)

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

https://codereview.chromium.org/105383002/diff/1/media/cdm/aes_decryptor.cc#n...
media/cdm/aes_decryptor.cc:164: std::string
session_id_string(base::UintToString(next_session_id_++));
web_?

https://codereview.chromium.org/105383002/diff/1/media/cdm/aes_decryptor.h
File media/cdm/aes_decryptor.h (right):

https://codereview.chromium.org/105383002/diff/1/media/cdm/aes_decryptor.h#ne...
media/cdm/aes_decryptor.h:119: // TODO(xhwang): Make session ID more strictly
defined if needed:
I think we can delete this. There were no more strict requirements. (Maybe in
your other CL, which is more related.)

https://codereview.chromium.org/105383002/diff/1/media/cdm/aes_decryptor.h#ne...
media/cdm/aes_decryptor.h:121: static uint32 next_session_id_;
next_web_...?

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

https://codereview.chromium.org/105383002/diff/1/media/cdm/ppapi/cdm_adapter....
media/cdm/ppapi/cdm_adapter.cc:539: uint32_t session_reference_id =
cdm_->DetermineReferenceId(session_id_str);
Is the naming here intentional?

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

https://codereview.chromium.org/105383002/diff/1/media/cdm/ppapi/cdm_wrapper....
media/cdm/ppapi/cdm_wrapper.h:96: virtual uint32_t DetermineReferenceId(const
std::string& web_session_id) = 0;
Update names of these functions.

https://codereview.chromium.org/105383002/diff/1/media/cdm/ppapi/cdm_wrapper....
media/cdm/ppapi/cdm_wrapper.h:231: uint32_t session_reference_id =
current_key_request_session_id_;
intentional?

https://codereview.chromium.org/105383002/diff/1/media/filters/pipeline_integ...
File media/filters/pipeline_integration_test.cc (right):

https://codereview.chromium.org/105383002/diff/1/media/filters/pipeline_integ...
media/filters/pipeline_integration_test.cc:83: virtual void SetSession(uint32
session_id,
Should we rename these? At least to On..., right?

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

https://codereview.chromium.org/105383002/diff/1/ppapi/api/private/ppb_conten...
ppapi/api/private/ppb_content_decryptor_private.idl:29: * @param[in] session_id
A reference for the session for which the CDM
An identifier...?
Identifies the session...?

https://codereview.chromium.org/105383002/diff/1/ppapi/cpp/private/content_de...
File ppapi/cpp/private/content_decryptor_private.cc (right):

https://codereview.chromium.org/105383002/diff/1/ppapi/cpp/private/content_de...
ppapi/cpp/private/content_decryptor_private.cc:60: ->CreateSession(session_id,
type_var.AsString(), init_data_array_buffer);
Weird break. Did a tool do this?

[xhwang, 2013-12-05 18:51:37]

Thanks!

FYI, you can develop this CL on top of:
https://codereview.chromium.org/100323004/
which will save you some rebasing time.

ddrowin/jrummell: We use session_id and web_session_id a lot in general
comments, shall we use session ID and web session ID instead? I guess it's not a
big deal :)

https://codereview.chromium.org/105383002/diff/1/content/renderer/media/crypt...
File content/renderer/media/crypto/proxy_decryptor.cc (right):

https://codereview.chromium.org/105383002/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/proxy_decryptor.cc:26: const uint32
INVALID_SESSION_ID = 0;
We use kCamelCase for consts.

https://codereview.chromium.org/105383002/diff/1/content/renderer/media/crypt...
File content/renderer/media/crypto/proxy_decryptor.h (right):

https://codereview.chromium.org/105383002/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/proxy_decryptor.h:40: // ID. Callers of this class
expect web session IDs in the responses.
Can you briefly explain what a session ID is and what a web session ID is?

https://codereview.chromium.org/105383002/diff/1/content/renderer/media/webco...
File content/renderer/media/webcontentdecryptionmodulesession_impl.cc (right):

https://codereview.chromium.org/105383002/diff/1/content/renderer/media/webco...
content/renderer/media/webcontentdecryptionmodulesession_impl.cc:14: static
const uint32 kStartingReferenceId = 1;
no need to have static here.

https://codereview.chromium.org/105383002/diff/1/media/base/android/media_drm...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/105383002/diff/1/media/base/android/media_drm...
media/base/android/media_drm_bridge.cc:318: std::string web_session_id =
ConvertJavaStringToUTF8(env, j_session_id);
On 2013/12/05 00:44:52, ddorwin wrote:
> Is MediaDrm passing us sessionId instead of "reference ID"? So, this will be
the
> last place where we don't have "reference ID", even after eliminating the
> prefixed API?

I'll fix MediaDrmBridge so that it'll pass back a "reference ID" (aka, the new
integer sessionId).

https://codereview.chromium.org/105383002/diff/1/media/base/android/media_drm...
File media/base/android/media_drm_bridge.h (right):

https://codereview.chromium.org/105383002/diff/1/media/base/android/media_drm...
media/base/android/media_drm_bridge.h:128: const std::string&
LookupSessionId(uint32_t session_id);
s/SessionId/WebSessionId/

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

https://codereview.chromium.org/105383002/diff/1/media/base/media_keys.h#newc...
media/base/media_keys.h:40: const static uint32 kInvalidReferenceId = 0;
update

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

https://codereview.chromium.org/105383002/diff/1/media/cdm/aes_decryptor.cc#n...
media/cdm/aes_decryptor.cc:173: session_message_cb_.Run(session_id, message,
std::string());
just double check, do we have a DCHECK that created_cb must be fired before
message_cb somewhere?

https://codereview.chromium.org/105383002/diff/1/media/cdm/aes_decryptor_unit...
File media/cdm/aes_decryptor_unittest.cc (right):

https://codereview.chromium.org/105383002/diff/1/media/cdm/aes_decryptor_unit...
media/cdm/aes_decryptor_unittest.cc:198:
session_id_(MediaKeys::kInvalidReferenceId),
update kInvalidReferencdId

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

https://codereview.chromium.org/105383002/diff/1/media/cdm/ppapi/cdm_wrapper....
media/cdm/ppapi/cdm_wrapper.h:106: // Map between session_id and web_session_id.
Move this comment above 103.

[jrummell, 2013-12-06 23:42:34]

Updated based on your comments. Will rebase once 100323004 lands.

https://codereview.chromium.org/105383002/diff/1/content/browser/media/androi...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/105383002/diff/1/content/browser/media/androi...
content/browser/media/android/browser_media_player_manager.cc:401: routing_id(),
media_keys_id, session_id, error_code, system_code));
On 2013/12/05 00:44:52, ddorwin wrote:
> This doesn't seem like the right formatting.

It's what clang-format does with it.

https://codereview.chromium.org/105383002/diff/1/content/common/media/media_p...
File content/common/media/media_player_messages_android.h (right):

https://codereview.chromium.org/105383002/diff/1/content/common/media/media_p...
content/common/media/media_player_messages_android.h:322:
IPC_MESSAGE_ROUTED3(MediaKeysMsg_SetSessionId,
On 2013/12/05 00:44:52, ddorwin wrote:
> Are these all being renamed when they are extracted?

Updated in https://codereview.chromium.org/100323004/, so I'll rebase once that
is in.

https://codereview.chromium.org/105383002/diff/1/content/renderer/media/crypt...
File content/renderer/media/crypto/proxy_decryptor.cc (right):

https://codereview.chromium.org/105383002/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/proxy_decryptor.cc:26: const uint32
INVALID_SESSION_ID = 0;
On 2013/12/05 18:51:37, xhwang wrote:
> We use kCamelCase for consts.

Done.

https://codereview.chromium.org/105383002/diff/1/content/renderer/media/crypt...
File content/renderer/media/crypto/proxy_decryptor.h (right):

https://codereview.chromium.org/105383002/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/proxy_decryptor.h:40: // ID. Callers of this class
expect web session IDs in the responses.
On 2013/12/05 18:51:37, xhwang wrote:
> Can you briefly explain what a session ID is and what a web session ID is?

Done.

https://codereview.chromium.org/105383002/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/proxy_decryptor.h:48: // web session ID rather
than a session ID.
On 2013/12/05 00:44:52, ddorwin wrote:
> s/a/the internal/

Done.

https://codereview.chromium.org/105383002/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/proxy_decryptor.h:89: // This is session_id <->
web_session_id map.
On 2013/12/05 00:44:52, ddorwin wrote:
> remove "This is ". (It's missing an article anyway.)

Done.

https://codereview.chromium.org/105383002/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/proxy_decryptor.h:108: uint32
LookupReferenceId(const std::string& web_session_id);
On 2013/12/05 00:44:52, ddorwin wrote:
> LookupSessionId

Done.

https://codereview.chromium.org/105383002/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/proxy_decryptor.h:113: const std::string&
LookupSessionId(uint32 session_id);
On 2013/12/05 00:44:52, ddorwin wrote:
> Lookup*Web*SessionId

Done.

https://codereview.chromium.org/105383002/diff/1/content/renderer/media/webco...
File content/renderer/media/webcontentdecryptionmodule_impl.cc (right):

https://codereview.chromium.org/105383002/diff/1/content/renderer/media/webco...
content/renderer/media/webcontentdecryptionmodule_impl.cc:23: // Forwards the
reference ID-based callbacks of the MediaKeys interface to the
On 2013/12/05 00:44:52, ddorwin wrote:
> update

Done.

https://codereview.chromium.org/105383002/diff/1/content/renderer/media/webco...
content/renderer/media/webcontentdecryptionmodule_impl.cc:25: class
ReferenceIdAdapter {
On 2013/12/05 00:44:52, ddorwin wrote:
> Rename. TODO?

Implemented.

https://codereview.chromium.org/105383002/diff/1/content/renderer/media/webco...
File content/renderer/media/webcontentdecryptionmodulesession_impl.cc (right):

https://codereview.chromium.org/105383002/diff/1/content/renderer/media/webco...
content/renderer/media/webcontentdecryptionmodulesession_impl.cc:14: static
const uint32 kStartingReferenceId = 1;
On 2013/12/05 18:51:37, xhwang wrote:
> no need to have static here.

Done.

https://codereview.chromium.org/105383002/diff/1/content/renderer/media/webco...
content/renderer/media/webcontentdecryptionmodulesession_impl.cc:73:
web_session_id_ = web_session_id;
On 2013/12/05 00:44:52, ddorwin wrote:
> If we don't need the accessor, only sessionId() uses this. We can probably
just
> convert this to a WebString here. That also makes it even more obvious what it
> is.

Done.

https://codereview.chromium.org/105383002/diff/1/content/renderer/media/webco...
File content/renderer/media/webcontentdecryptionmodulesession_impl.h (right):

https://codereview.chromium.org/105383002/diff/1/content/renderer/media/webco...
content/renderer/media/webcontentdecryptionmodulesession_impl.h:42: const
std::string& web_session_id() const { return web_session_id_; }
On 2013/12/05 00:44:52, ddorwin wrote:
> Is anyone using this? If not, remove it. (This is really a duplicate of
> sessionId().)

Not used, so removed.

https://codereview.chromium.org/105383002/diff/1/content/renderer/media/webco...
content/renderer/media/webcontentdecryptionmodulesession_impl.h:42: const
std::string& web_session_id() const { return web_session_id_; }
On 2013/12/05 00:44:52, ddorwin wrote:
> Because this is so confusing, I think we should add (somewhat duplicate)
> comments to these accessors as well.

No longer used.

https://codereview.chromium.org/105383002/diff/1/content/renderer/media/webco...
content/renderer/media/webcontentdecryptionmodulesession_impl.h:44: uint32
session_id() const { return session_id_; }
On 2013/12/05 00:44:52, ddorwin wrote:
> I'd prefer to not have this public session_id(), but webcdm_impl.cc uses it.
> 
> We might be able to address this by having the ReferenceIdAdapter generate the
> IDs and pass them to this class's constructor. Maybe we'll do that in a
separate
> CL. TODO?

Implemented.

https://codereview.chromium.org/105383002/diff/1/content/renderer/media/webco...
content/renderer/media/webcontentdecryptionmodulesession_impl.h:61: // Session
ID is the user visible ID for this session generated by the CDM.
On 2013/12/05 00:44:52, ddorwin wrote:
> Update.
> Also, s/user /app-/

Done.

https://codereview.chromium.org/105383002/diff/1/content/renderer/media/webco...
content/renderer/media/webcontentdecryptionmodulesession_impl.h:65: // Reference
ID is used to uniquely track this object so that CDM callbacks
On 2013/12/05 00:44:52, ddorwin wrote:
> update

Done.

https://codereview.chromium.org/105383002/diff/1/media/base/android/media_drm...
File media/base/android/media_drm_bridge.h (right):

https://codereview.chromium.org/105383002/diff/1/media/base/android/media_drm...
media/base/android/media_drm_bridge.h:124: uint32_t DetermineReferenceId(const
std::string& web_session_id);
On 2013/12/05 00:44:52, ddorwin wrote:
> Update these function names too. (If it simplifies things, you can do this as
a
> separate Android-only CL.)

Done.

https://codereview.chromium.org/105383002/diff/1/media/base/android/media_drm...
media/base/android/media_drm_bridge.h:128: const std::string&
LookupSessionId(uint32_t session_id);
On 2013/12/05 18:51:37, xhwang wrote:
> s/SessionId/WebSessionId/

Done.

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

https://codereview.chromium.org/105383002/diff/1/media/base/media_keys.h#newc...
media/base/media_keys.h:40: const static uint32 kInvalidReferenceId = 0;
On 2013/12/05 18:51:37, xhwang wrote:
> update

Done.

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

https://codereview.chromium.org/105383002/diff/1/media/cdm/aes_decryptor.cc#n...
media/cdm/aes_decryptor.cc:164: std::string
session_id_string(base::UintToString(next_session_id_++));
On 2013/12/05 00:44:52, ddorwin wrote:
> web_?

Done.

https://codereview.chromium.org/105383002/diff/1/media/cdm/aes_decryptor.cc#n...
media/cdm/aes_decryptor.cc:173: session_message_cb_.Run(session_id, message,
std::string());
On 2013/12/05 18:51:37, xhwang wrote:
> just double check, do we have a DCHECK that created_cb must be fired before
> message_cb somewhere?

SessionMessage will fail when it tries to convert session_id to web_session_id
on the way to the app. There is a DCHECK there.

https://codereview.chromium.org/105383002/diff/1/media/cdm/aes_decryptor.h
File media/cdm/aes_decryptor.h (right):

https://codereview.chromium.org/105383002/diff/1/media/cdm/aes_decryptor.h#ne...
media/cdm/aes_decryptor.h:119: // TODO(xhwang): Make session ID more strictly
defined if needed:
On 2013/12/05 00:44:52, ddorwin wrote:
> I think we can delete this. There were no more strict requirements. (Maybe in
> your other CL, which is more related.)

Done.

https://codereview.chromium.org/105383002/diff/1/media/cdm/aes_decryptor.h#ne...
media/cdm/aes_decryptor.h:121: static uint32 next_session_id_;
On 2013/12/05 00:44:52, ddorwin wrote:
> next_web_...?

Done.

https://codereview.chromium.org/105383002/diff/1/media/cdm/aes_decryptor_unit...
File media/cdm/aes_decryptor_unittest.cc (right):

https://codereview.chromium.org/105383002/diff/1/media/cdm/aes_decryptor_unit...
media/cdm/aes_decryptor_unittest.cc:198:
session_id_(MediaKeys::kInvalidReferenceId),
On 2013/12/05 18:51:37, xhwang wrote:
> update kInvalidReferencdId

Done.

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

https://codereview.chromium.org/105383002/diff/1/media/cdm/ppapi/cdm_adapter....
media/cdm/ppapi/cdm_adapter.cc:539: uint32_t session_reference_id =
cdm_->DetermineReferenceId(session_id_str);
On 2013/12/05 00:44:52, ddorwin wrote:
> Is the naming here intentional?

Yes. The old CDM APIs pass around the string as "session_id", not
"web_session_id", so I wanted to keep the parameters as specified.

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

https://codereview.chromium.org/105383002/diff/1/media/cdm/ppapi/cdm_wrapper....
media/cdm/ppapi/cdm_wrapper.h:96: virtual uint32_t DetermineReferenceId(const
std::string& web_session_id) = 0;
On 2013/12/05 00:44:52, ddorwin wrote:
> Update names of these functions.

Done.

https://codereview.chromium.org/105383002/diff/1/media/cdm/ppapi/cdm_wrapper....
media/cdm/ppapi/cdm_wrapper.h:106: // Map between session_id and web_session_id.
On 2013/12/05 18:51:37, xhwang wrote:
> Move this comment above 103.

Done.

https://codereview.chromium.org/105383002/diff/1/media/cdm/ppapi/cdm_wrapper....
media/cdm/ppapi/cdm_wrapper.h:231: uint32_t session_reference_id =
current_key_request_session_id_;
On 2013/12/05 00:44:52, ddorwin wrote:
> intentional?

Updated since the method parameter is web_session_id.

https://codereview.chromium.org/105383002/diff/1/media/filters/pipeline_integ...
File media/filters/pipeline_integration_test.cc (right):

https://codereview.chromium.org/105383002/diff/1/media/filters/pipeline_integ...
media/filters/pipeline_integration_test.cc:83: virtual void SetSession(uint32
session_id,
On 2013/12/05 00:44:52, ddorwin wrote:
> Should we rename these? At least to On..., right?

Updated to match full names.

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

https://codereview.chromium.org/105383002/diff/1/ppapi/api/private/ppb_conten...
ppapi/api/private/ppb_content_decryptor_private.idl:29: * @param[in] session_id
A reference for the session for which the CDM
On 2013/12/05 00:44:52, ddorwin wrote:
> An identifier...?
> Identifies the session...?

Done.

https://codereview.chromium.org/105383002/diff/1/ppapi/cpp/private/content_de...
File ppapi/cpp/private/content_decryptor_private.cc (right):

https://codereview.chromium.org/105383002/diff/1/ppapi/cpp/private/content_de...
ppapi/cpp/private/content_decryptor_private.cc:60: ->CreateSession(session_id,
type_var.AsString(), init_data_array_buffer);
On 2013/12/05 00:44:52, ddorwin wrote:
> Weird break. Did a tool do this?

clang-format.

[ddorwin, 2013-12-09 18:45:51]

LGTM with a couple minor changes.

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

https://codereview.chromium.org/105383002/diff/1/media/cdm/ppapi/cdm_adapter....
media/cdm/ppapi/cdm_adapter.cc:539: uint32_t session_reference_id =
cdm_->DetermineReferenceId(session_id_str);
On 2013/12/06 23:42:35, jrummell wrote:
> On 2013/12/05 00:44:52, ddorwin wrote:
> > Is the naming here intentional?
> 
> Yes. The old CDM APIs pass around the string as "session_id", not
> "web_session_id", so I wanted to keep the parameters as specified.

So we can rename it when we remove CDM 1 & 2?

https://codereview.chromium.org/105383002/diff/20001/content/renderer/media/w...
File content/renderer/media/webcontentdecryptionmodule_impl.cc (right):

https://codereview.chromium.org/105383002/diff/20001/content/renderer/media/w...
content/renderer/media/webcontentdecryptionmodule_impl.cc:73:
COMPILE_ASSERT(kStartingSessionId != media::MediaKeys::kInvalidSessionId,
maybe > since we're going to be incrementing - we wouldn't want to go through
it.

https://codereview.chromium.org/105383002/diff/20001/content/renderer/media/w...
content/renderer/media/webcontentdecryptionmodule_impl.cc:118: return
++next_session_id_;
This means 2 will be the first session ID. Is there a reason you used prefix ++?

[jrummell, 2013-12-09 21:01:17]

Updated.

+palmer@ for message security review
+dmichael@ for ppapi messages

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

https://codereview.chromium.org/105383002/diff/1/media/cdm/ppapi/cdm_adapter....
media/cdm/ppapi/cdm_adapter.cc:539: uint32_t session_reference_id =
cdm_->DetermineReferenceId(session_id_str);
On 2013/12/09 18:45:52, ddorwin wrote:
> On 2013/12/06 23:42:35, jrummell wrote:
> > On 2013/12/05 00:44:52, ddorwin wrote:
> > > Is the naming here intentional?
> > 
> > Yes. The old CDM APIs pass around the string as "session_id", not
> > "web_session_id", so I wanted to keep the parameters as specified.
> 
> So we can rename it when we remove CDM 1 & 2?

It will be removed, since SendKeyMessage() and SendKeyError() aren't in CDM_3.

https://codereview.chromium.org/105383002/diff/20001/content/renderer/media/w...
File content/renderer/media/webcontentdecryptionmodule_impl.cc (right):

https://codereview.chromium.org/105383002/diff/20001/content/renderer/media/w...
content/renderer/media/webcontentdecryptionmodule_impl.cc:73:
COMPILE_ASSERT(kStartingSessionId != media::MediaKeys::kInvalidSessionId,
On 2013/12/09 18:45:53, ddorwin wrote:
> maybe > since we're going to be incrementing - we wouldn't want to go through
> it.

Good idea. Done.

https://codereview.chromium.org/105383002/diff/20001/content/renderer/media/w...
content/renderer/media/webcontentdecryptionmodule_impl.cc:118: return
++next_session_id_;
On 2013/12/09 18:45:53, ddorwin wrote:
> This means 2 will be the first session ID. Is there a reason you used prefix
++?

Done.

[dmichael (off chromium), 2013-12-09 21:12:27]

ppapi/ lgtm

[xhwang, 2013-12-09 21:21:53]

lgtm % nit

https://codereview.chromium.org/105383002/diff/60001/content/renderer/media/w...
File content/renderer/media/webcontentdecryptionmodule_impl.cc (right):

https://codereview.chromium.org/105383002/diff/60001/content/renderer/media/w...
content/renderer/media/webcontentdecryptionmodule_impl.cc:34: // Generate a
unique internal session id.
nit: Generate_s_

[jrummell, 2013-12-09 22:25:31]

Updated for nit.

https://codereview.chromium.org/105383002/diff/60001/content/renderer/media/w...
File content/renderer/media/webcontentdecryptionmodule_impl.cc (right):

https://codereview.chromium.org/105383002/diff/60001/content/renderer/media/w...
content/renderer/media/webcontentdecryptionmodule_impl.cc:34: // Generate a
unique internal session id.
On 2013/12/09 21:21:54, xhwang wrote:
> nit: Generate_s_

Done.

[palmer, 2013-12-10 01:20:49]

IPC security rubberstamp LGTM. I looked only at the *messages*.h files.

[commit-bot: I haz the power, 2013-12-10 01:35:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jrummell@chromium.org/105383002/80001

[commit-bot: I haz the power, 2013-12-10 07:26:03]

Change committed as 239698