Componentize EncryptedMediaMessageFilter and rename it CdmMessageFilter.

Componentize EncryptedMediaMessageFilter and rename it CdmMessageFilter.
    
The motivation is to reuse the Widevine keysystem registration logic in Android Webview.

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

[ycheo (away), 2014-04-25 12:35:24]

PTAL.

@ddorwin, qinmin, xhwang for all.

@tsepez for ipc.

@jam for /chrome, /content.

[jam, 2014-04-25 17:17:56]

https://codereview.chromium.org/253593002/diff/1/chrome/renderer/media/DEPS
File chrome/renderer/media/DEPS (right):

https://codereview.chromium.org/253593002/diff/1/chrome/renderer/media/DEPS#n...
chrome/renderer/media/DEPS:2: "+content/common/media",  # For media IPC
messages.
chrome can never include files not in content/public

also, IPCs don't span module boundaries (i.e. chrome can't send an IPC that
content dispatches).

why is the ipc being sent from chrome now if you're changing the handling to
content? i.e. why not send it from content?

[ycheo (away), 2014-04-28 16:09:42]

https://codereview.chromium.org/253593002/diff/1/chrome/renderer/media/DEPS
File chrome/renderer/media/DEPS (right):

https://codereview.chromium.org/253593002/diff/1/chrome/renderer/media/DEPS#n...
chrome/renderer/media/DEPS:2: "+content/common/media",  # For media IPC
messages.
On 2014/04/25 17:17:56, jam wrote:
> chrome can never include files not in content/public
> 
> also, IPCs don't span module boundaries (i.e. chrome can't send an IPC that
> content dispatches).
Oh, I didn't know this.

> 
> why is the ipc being sent from chrome now if you're changing the handling to
> content? i.e. why not send it from content?
Per your advice, I moved the whole function AddAndroidWidevine() from chrome to
content.

[Tom Sepez, 2014-04-28 18:21:52]

Messages in and of themselves are LGTM from a security perspective, but be sure
to addresss John's concerns. You can address the nits as you see fit.

https://codereview.chromium.org/253593002/diff/30001/android_webview/renderer...
File android_webview/renderer/aw_key_systems.cc (right):

https://codereview.chromium.org/253593002/diff/30001/android_webview/renderer...
android_webview/renderer/aw_key_systems.cc:7: using content::KeySystemInfo;
nit: the identifier KeySystemInfo now appears but once in this file, it now
takes more characters and is less clear by "using" it.

https://codereview.chromium.org/253593002/diff/30001/chrome/renderer/media/ch...
File chrome/renderer/media/chrome_key_systems.cc (right):

https://codereview.chromium.org/253593002/diff/30001/chrome/renderer/media/ch...
chrome/renderer/media/chrome_key_systems.cc:198: #endif
nit: // defined ... while you're at it, since you're messing with these ifdefs.

https://codereview.chromium.org/253593002/diff/30001/content/public/renderer/...
File content/public/renderer/key_system_info.h (right):

https://codereview.chromium.org/253593002/diff/30001/content/public/renderer/...
content/public/renderer/key_system_info.h:41: #if defined(ENABLE_PEPPER_CDMS)
nit: maybe just pass this as NULL in the !ENABLE_PEPPER_CDMS to avoid  the ifdef
in the middle of the method declaration.

https://codereview.chromium.org/253593002/diff/30001/content/renderer/media/c...
File content/renderer/media/crypto/key_systems.cc (right):

https://codereview.chromium.org/253593002/diff/30001/content/renderer/media/c...
content/renderer/media/crypto/key_systems.cc:82: static const char
kKeySystemSuffixHrNonCompositing[] = ".hrnoncompositing";
nit: kinda wish this constant wasn't buried so deep in file.

[ddorwin, 2014-04-28 18:47:44]

The Android IPC to get key systems supported by the platform should probably be
in content so it can be reused by various content clients, including chrome/.

However, the specific key systems should be in the content client/ like they are
on desktop. I believe this means that the KeySystems code must IPC instead of
calling ContentRendererClient directly. The browser side of the IPC will then
ask ContentBrowserClient for supported key systems.

https://codereview.chromium.org/253593002/diff/30001/content/public/renderer/...
File content/public/renderer/key_system_info.h (right):

https://codereview.chromium.org/253593002/diff/30001/content/public/renderer/...
content/public/renderer/key_system_info.h:38: static KeySystemInfo Build(const
char* key_system,
This violates the Content API design:
http://www.chromium.org/developers/content-module/content-api

https://codereview.chromium.org/253593002/diff/30001/content/renderer/media/c...
File content/renderer/media/crypto/key_systems.cc (right):

https://codereview.chromium.org/253593002/diff/30001/content/renderer/media/c...
content/renderer/media/crypto/key_systems.cc:217:
GetContentClient()->renderer()->AddKeySystems(&key_systems_info);
I think we should not make (or compile support for) this call on OS_ANDROID.
Instead, we should make the IPC to get the answers from the browser, which may
then ask chrome/.

https://codereview.chromium.org/253593002/diff/30001/content/renderer/media/c...
content/renderer/media/crypto/key_systems.cc:222:
AddAndroidWidevine(&key_systems_info);
Like desktop, this should be in chrome/, but on the browser side.

[ddorwin, 2014-04-28 19:02:54]

Really, the IPC is in the correct location (chrome/) already. This keeps as much
code as possible in the renderer. However, that means other content clients
can't reuse the IPC mechanism.

Ideally, we'd leave the code as is but call back into some common code to do the
IPC. I don't think there's a way to do that, though. Are there other places
where AW and Chrome need to share code?

[boliu, 2014-04-28 22:33:42]

On 2014/04/28 19:02:54, ddorwin wrote:
> Are there other places where AW and Chrome need to share code?

src/components?

I've factored out things as small as a single function (auto_login_parser) into
components to share between chrome/ and android_webview/. I don't want to wade
into this review, but if there's common code that should live above content,
then components is probably where it should go.

[ycheo (away), 2014-04-28 23:30:59]

On 2014/04/28 22:33:42, boliu wrote:
> On 2014/04/28 19:02:54, ddorwin wrote:
> > Are there other places where AW and Chrome need to share code?
> 
> src/components?
> 
> I've factored out things as small as a single function (auto_login_parser)
into
> components to share between chrome/ and android_webview/. I don't want to wade
> into this review, but if there's common code that should live above content,
> then components is probably where it should go.

@boliu,
Thanks, Bo, for the information.

@ddorwin,
Then, could you check if the following desgin is right?
1. Move IPC (EncryptedMediaMessageFilter) into component.
2. Should each embedder (chrome and android_webview) register the filter by
themselves or is it OK to register it in content only for Android like the
following?
https://codereview.chromium.org/253593002/diff/30001/content/browser/renderer...
3. Move AddAndroidWidevine() into component also.  Each embedder will call this.

[ddorwin, 2014-04-29 00:26:24]

On 2014/04/28 23:30:59, Yuncheol Heo wrote:
> On 2014/04/28 22:33:42, boliu wrote:
> > On 2014/04/28 19:02:54, ddorwin wrote:
> > > Are there other places where AW and Chrome need to share code?
> > 
> > src/components?
> > 
> > I've factored out things as small as a single function (auto_login_parser)
> into
> > components to share between chrome/ and android_webview/. I don't want to
wade
> > into this review, but if there's common code that should live above content,
> > then components is probably where it should go.
> 
> @boliu,
> Thanks, Bo, for the information.
> 
> @ddorwin,
> Then, could you check if the following desgin is right?
> 1. Move IPC (EncryptedMediaMessageFilter) into component.

Probably src/components/media/...

xhwang, scherkus: WDYT?

> 2. Should each embedder (chrome and android_webview) register the filter by
> themselves or is it OK to register it in content only for Android like the
> following?
>
https://codereview.chromium.org/253593002/diff/30001/content/browser/renderer...

I think it's fine for all IPC-using platforms (aka Android for now) to register
this, but it seems weird for content/ to provide an IPC target only used by
components/. Maybe the embedders should call a function in components/ to add
support for the component.

> 3. Move AddAndroidWidevine() into component also.  Each embedder will call
this.

If we move things, it seems we should probably move most of
chrome_key_systems.cc. That is, the "how to register CDM X if you want to" part.
This would also include the generic logic, such as checking for the presence of
a Pepper CDM.

Then, chrome_key_systems.cc, etc. would only exist to call the methods for the
key systems it wants to support.

[xhwang, 2014-04-29 00:43:27]

On 2014/04/29 00:26:24, ddorwin wrote:
> On 2014/04/28 23:30:59, Yuncheol Heo wrote:
> > On 2014/04/28 22:33:42, boliu wrote:
> > > On 2014/04/28 19:02:54, ddorwin wrote:
> > > > Are there other places where AW and Chrome need to share code?
> > > 
> > > src/components?
> > > 
> > > I've factored out things as small as a single function (auto_login_parser)
> > into
> > > components to share between chrome/ and android_webview/. I don't want to
> wade
> > > into this review, but if there's common code that should live above
content,
> > > then components is probably where it should go.
> > 
> > @boliu,
> > Thanks, Bo, for the information.
> > 
> > @ddorwin,
> > Then, could you check if the following desgin is right?
> > 1. Move IPC (EncryptedMediaMessageFilter) into component.
> 
> Probably src/components/media/...
> 
> xhwang, scherkus: WDYT?

I don't know much about "components". But from this README it seems a good fit.

https://code.google.com/p/chromium/codesearch#chromium/src/components/README&l=2

> 
> > 2. Should each embedder (chrome and android_webview) register the filter by
> > themselves or is it OK to register it in content only for Android like the
> > following?
> >
>
https://codereview.chromium.org/253593002/diff/30001/content/browser/renderer...
> 
> I think it's fine for all IPC-using platforms (aka Android for now) to
register
> this, but it seems weird for content/ to provide an IPC target only used by
> components/. Maybe the embedders should call a function in components/ to add
> support for the component.
> 
> > 3. Move AddAndroidWidevine() into component also.  Each embedder will call
> this.
> 
> If we move things, it seems we should probably move most of
> chrome_key_systems.cc. That is, the "how to register CDM X if you want to"
part.
> This would also include the generic logic, such as checking for the presence
of
> a Pepper CDM.

chrome_content_renderer_client.cc already includes a lot of components stuff:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/ch...

So this sgtm.
 
> Then, chrome_key_systems.cc, etc. would only exist to call the methods for the
> key systems it wants to support.

[ycheo (away), 2014-04-29 00:49:52]

https://codereview.chromium.org/253593002/diff/30001/content/public/renderer/...
File content/public/renderer/key_system_info.h (right):

https://codereview.chromium.org/253593002/diff/30001/content/public/renderer/...
content/public/renderer/key_system_info.h:38: static KeySystemInfo Build(const
char* key_system,
On 2014/04/28 18:47:45, ddorwin wrote:
> This violates the Content API design:
> http://www.chromium.org/developers/content-module/content-api

The content api design allows to have constructors.
Is it OK to convert this into the constructor?

[ddorwin, 2014-04-29 00:53:11]

https://codereview.chromium.org/253593002/diff/30001/content/public/renderer/...
File content/public/renderer/key_system_info.h (right):

https://codereview.chromium.org/253593002/diff/30001/content/public/renderer/...
content/public/renderer/key_system_info.h:38: static KeySystemInfo Build(const
char* key_system,
On 2014/04/29 00:49:52, Yuncheol Heo wrote:
> On 2014/04/28 18:47:45, ddorwin wrote:
> > This violates the Content API design:
> > http://www.chromium.org/developers/content-module/content-api
> 
> The content api design allows to have constructors.
> Is it OK to convert this into the constructor?

If you were to put it in a constructor, then we can probably make some of the
values const, but then it's unclear why they aren't private and in a class,
which is not allowed.

xhwang was considering some changes here, so discuss with him.

Before making a decision here, let's see what happens when much of the code is
moved to components/.

[ycheo (away), 2014-04-30 09:05:58]

Hi reviewers,

The componetized version is ready.
PTAL.

[ycheo (away), 2014-05-01 21:52:56]

On 2014/04/30 09:05:58, Yuncheol Heo wrote:
> Hi reviewers,
> 
> The componetized version is ready.
> PTAL.

@ddorwin, xhwang,
Gentle reminder.

[xhwang, 2014-05-01 22:41:21]

Generally looking pretty good. Just a couple of comments.

https://codereview.chromium.org/253593002/diff/90001/chrome/renderer/media/ch...
File chrome/renderer/media/chrome_key_systems.cc (right):

https://codereview.chromium.org/253593002/diff/90001/chrome/renderer/media/ch...
chrome/renderer/media/chrome_key_systems.cc:154: static bool
IsWidevineCdmAvailable(SupportedCodecs* supported_codecs) {
The function name doesn't reflect what the function is doing. The function also
get supported codecs, which is more than "wv cdm available".

https://codereview.chromium.org/253593002/diff/90001/components/encrypted_med...
File components/encrypted_media/renderer/widevine_key_systems.cc (right):

https://codereview.chromium.org/253593002/diff/90001/components/encrypted_med...
components/encrypted_media/renderer/widevine_key_systems.cc:23: namespace
encrypted_media {
ddorwin: shall we use "encrypted_media" or "eme" for the namespace, folder name
and file name prefixes?

[ycheo (away), 2014-05-01 23:27:31]

https://codereview.chromium.org/253593002/diff/90001/chrome/renderer/media/ch...
File chrome/renderer/media/chrome_key_systems.cc (right):

https://codereview.chromium.org/253593002/diff/90001/chrome/renderer/media/ch...
chrome/renderer/media/chrome_key_systems.cc:154: static bool
IsWidevineCdmAvailable(SupportedCodecs* supported_codecs) {
On 2014/05/01 22:41:22, xhwang wrote:
> The function name doesn't reflect what the function is doing. The function
also
> get supported codecs, which is more than "wv cdm available".

Yes, the naming is difficult.
I have 3 options and could you choose one or recommend another one?
1. CheckWidevineAvailabilityAndGetSupportedCodecs().
2. GetSupportedCodecsIfWidevineIsAvailable().
3. Split the function as two (IsWidevineCdmAvailable(), GetSupportedCodecs())
and expose additional_param_(names|values) as parameters.

[xhwang, 2014-05-02 00:22:30]

https://codereview.chromium.org/253593002/diff/90001/chrome/renderer/media/ch...
File chrome/renderer/media/chrome_key_systems.cc (right):

https://codereview.chromium.org/253593002/diff/90001/chrome/renderer/media/ch...
chrome/renderer/media/chrome_key_systems.cc:154: static bool
IsWidevineCdmAvailable(SupportedCodecs* supported_codecs) {
On 2014/05/01 23:27:32, Yuncheol Heo wrote:
> On 2014/05/01 22:41:22, xhwang wrote:
> > The function name doesn't reflect what the function is doing. The function
> also
> > get supported codecs, which is more than "wv cdm available".
> 
> Yes, the naming is difficult.
> I have 3 options and could you choose one or recommend another one?
> 1. CheckWidevineAvailabilityAndGetSupportedCodecs().
> 2. GetSupportedCodecsIfWidevineIsAvailable().
> 3. Split the function as two (IsWidevineCdmAvailable(), GetSupportedCodecs())
> and expose additional_param_(names|values) as parameters.

This is only used before calling AddPepperBasedWidevine(). Can we always call
AddPepperBasedWidevine() and put the content of this function in
AddPepperBasedWidevine()? That can also help us hide SupportedCodecs (which is
an impl detail) in AddChromeKeySystems().

[ycheo (away), 2014-05-02 00:30:25]

https://codereview.chromium.org/253593002/diff/90001/chrome/renderer/media/ch...
File chrome/renderer/media/chrome_key_systems.cc (right):

https://codereview.chromium.org/253593002/diff/90001/chrome/renderer/media/ch...
chrome/renderer/media/chrome_key_systems.cc:154: static bool
IsWidevineCdmAvailable(SupportedCodecs* supported_codecs) {
On 2014/05/02 00:22:31, xhwang wrote:
> On 2014/05/01 23:27:32, Yuncheol Heo wrote:
> > On 2014/05/01 22:41:22, xhwang wrote:
> > > The function name doesn't reflect what the function is doing. The function
> > also
> > > get supported codecs, which is more than "wv cdm available".
> > 
> > Yes, the naming is difficult.
> > I have 3 options and could you choose one or recommend another one?
> > 1. CheckWidevineAvailabilityAndGetSupportedCodecs().
> > 2. GetSupportedCodecsIfWidevineIsAvailable().
> > 3. Split the function as two (IsWidevineCdmAvailable(),
GetSupportedCodecs())
> > and expose additional_param_(names|values) as parameters.
> 
> This is only used before calling AddPepperBasedWidevine(). Can we always call
> AddPepperBasedWidevine() and put the content of this function in
> AddPepperBasedWidevine()? That can also help us hide SupportedCodecs (which is
> an impl detail) in AddChromeKeySystems().

That means I need to componentize PluginInfoMessageFilter also, right?

[xhwang, 2014-05-02 00:48:25]

On 2014/05/02 00:30:25, Yuncheol Heo wrote:
>
https://codereview.chromium.org/253593002/diff/90001/chrome/renderer/media/ch...
> File chrome/renderer/media/chrome_key_systems.cc (right):
> 
>
https://codereview.chromium.org/253593002/diff/90001/chrome/renderer/media/ch...
> chrome/renderer/media/chrome_key_systems.cc:154: static bool
> IsWidevineCdmAvailable(SupportedCodecs* supported_codecs) {
> On 2014/05/02 00:22:31, xhwang wrote:
> > On 2014/05/01 23:27:32, Yuncheol Heo wrote:
> > > On 2014/05/01 22:41:22, xhwang wrote:
> > > > The function name doesn't reflect what the function is doing. The
function
> > > also
> > > > get supported codecs, which is more than "wv cdm available".
> > > 
> > > Yes, the naming is difficult.
> > > I have 3 options and could you choose one or recommend another one?
> > > 1. CheckWidevineAvailabilityAndGetSupportedCodecs().
> > > 2. GetSupportedCodecsIfWidevineIsAvailable().
> > > 3. Split the function as two (IsWidevineCdmAvailable(),
> GetSupportedCodecs())
> > > and expose additional_param_(names|values) as parameters.
> > 
> > This is only used before calling AddPepperBasedWidevine(). Can we always
call
> > AddPepperBasedWidevine() and put the content of this function in
> > AddPepperBasedWidevine()? That can also help us hide SupportedCodecs (which
is
> > an impl detail) in AddChromeKeySystems().
> 
> That means I need to componentize PluginInfoMessageFilter also, right?

I think the problem comes from the fact that AddPepperBasedWidevine() is in
components but IsPepperCdmRegistered() is still in Chrome. Since
AddPepperBasedWidevine() is not used by anything other than chrome, shall we
actually de-componentize AddPepperBasedWidevine()?

ddorwin: WDYT?

[ddorwin, 2014-05-02 01:12:57]

https://codereview.chromium.org/253593002/diff/90001/chrome/renderer/media/ch...
File chrome/renderer/media/chrome_key_systems.cc (right):

https://codereview.chromium.org/253593002/diff/90001/chrome/renderer/media/ch...
chrome/renderer/media/chrome_key_systems.cc:140: if (codecs[i] ==
kCdmSupportedCodecVorbis)
These constants are currently defined in widevine_cdm_common.h. This function
implies that it is generic, but it is not. If we want it to be, we need to move
them. (I don't see an advantage at the moment..

https://codereview.chromium.org/253593002/diff/90001/chrome/renderer/media/ch...
chrome/renderer/media/chrome_key_systems.cc:154: static bool
IsWidevineCdmAvailable(SupportedCodecs* supported_codecs) {
On 2014/05/02 00:30:26, Yuncheol Heo wrote:
> On 2014/05/02 00:22:31, xhwang wrote:
> > On 2014/05/01 23:27:32, Yuncheol Heo wrote:
> > > On 2014/05/01 22:41:22, xhwang wrote:
> > > > The function name doesn't reflect what the function is doing. The
function
> > > also
> > > > get supported codecs, which is more than "wv cdm available".
> > > 
> > > Yes, the naming is difficult.
> > > I have 3 options and could you choose one or recommend another one?
> > > 1. CheckWidevineAvailabilityAndGetSupportedCodecs().
> > > 2. GetSupportedCodecsIfWidevineIsAvailable().
> > > 3. Split the function as two (IsWidevineCdmAvailable(),
> GetSupportedCodecs())
> > > and expose additional_param_(names|values) as parameters.
> > 
> > This is only used before calling AddPepperBasedWidevine(). Can we always
call
> > AddPepperBasedWidevine() and put the content of this function in
> > AddPepperBasedWidevine()? That can also help us hide SupportedCodecs (which
is
> > an impl detail) in AddChromeKeySystems().
> 
> That means I need to componentize PluginInfoMessageFilter also, right?

On 2014/05/02 00:48:25, xhwang wrote:
> 
> I think the problem comes from the fact that AddPepperBasedWidevine() is in
> components but IsPepperCdmRegistered() is still in Chrome. Since
> AddPepperBasedWidevine() is not used by anything other than chrome, shall we
> actually de-componentize AddPepperBasedWidevine()?
> 
> ddorwin: WDYT?

Since we have all the details in components, I think it's fine to keep them
there. However, we do need to fix this function.

How about this?
// Returns true if the Widevine is supported and the codecs successfully
obtained.
bool GetPepperBasedWidevineCodecs(codecs);

Or better yet, just wrap 184-189 in a local AddPepperBasedWidevine() function
that inlines this function. That seems to make the mose sense.

I guess that returns to what we previously had with AddPepperBasedWidevine()

https://codereview.chromium.org/253593002/diff/90001/components/encrypted_med...
File components/encrypted_media.gypi (right):

https://codereview.chromium.org/253593002/diff/90001/components/encrypted_med...
components/encrypted_media.gypi:1: # Copyright 2014 The Chromium Authors. All
rights reserved.
"encrypted_media" seems like a very specific name. I would have expected it to
be "media", though I see many other components are specific.

Is this really handling "encrypted media" or is it just "cdm"?

https://codereview.chromium.org/253593002/diff/90001/components/encrypted_med...
File components/encrypted_media/renderer/widevine_key_systems.cc (right):

https://codereview.chromium.org/253593002/diff/90001/components/encrypted_med...
components/encrypted_media/renderer/widevine_key_systems.cc:28: WIDEVINE_HR,
Remove. Never used.

https://codereview.chromium.org/253593002/diff/90001/components/encrypted_med...
components/encrypted_media/renderer/widevine_key_systems.cc:52: case
WIDEVINE_HR:
Remove See above.

https://codereview.chromium.org/253593002/diff/90001/components/encrypted_med...
components/encrypted_media/renderer/widevine_key_systems.cc:77: void
AddPepperBasedWidevine(std::vector<KeySystemInfo>* concrete_key_systems,
Output parameter should be last.

https://codereview.chromium.org/253593002/diff/90001/components/encrypted_med...
components/encrypted_media/renderer/widevine_key_systems.cc:78: bool
is_widevine_hr_supported,
Unnecessary. See above.

[xhwang, 2014-05-02 01:18:43]

https://codereview.chromium.org/253593002/diff/90001/components/encrypted_med...
File components/encrypted_media.gypi (right):

https://codereview.chromium.org/253593002/diff/90001/components/encrypted_med...
components/encrypted_media.gypi:1: # Copyright 2014 The Chromium Authors. All
rights reserved.
On 2014/05/02 01:12:58, ddorwin wrote:
> "encrypted_media" seems like a very specific name. I would have expected it to
> be "media", though I see many other components are specific.
> 
> Is this really handling "encrypted media" or is it just "cdm"?

cdm sgtm. we already have src/media/cdm folder.

[ycheo (away), 2014-05-02 01:39:33]

https://codereview.chromium.org/253593002/diff/90001/components/encrypted_med...
File components/encrypted_media.gypi (right):

https://codereview.chromium.org/253593002/diff/90001/components/encrypted_med...
components/encrypted_media.gypi:1: # Copyright 2014 The Chromium Authors. All
rights reserved.
On 2014/05/02 01:18:44, xhwang wrote:
> On 2014/05/02 01:12:58, ddorwin wrote:
> > "encrypted_media" seems like a very specific name. I would have expected it
to
> > be "media", though I see many other components are specific.
> > 
> > Is this really handling "encrypted media" or is it just "cdm"?
> 
> cdm sgtm. we already have src/media/cdm folder.

Thanks for suggesting good component name.
The only concern is that the namespace is conflicted with
content_decryption_module.h in ppapi:
https://code.google.com/p/chromium/codesearch#chromium/src/media/cdm/ppapi/ap...

Do you think it's OK to use the same namespace or any other suggestion on the
namespace?

[ycheo (away), 2014-05-02 10:05:46]

I changed that 'cdm' component only has the android things.
So, I wonder I need to change the name as 'cdm_android' or something.

https://codereview.chromium.org/253593002/diff/90001/chrome/renderer/media/ch...
File chrome/renderer/media/chrome_key_systems.cc (right):

https://codereview.chromium.org/253593002/diff/90001/chrome/renderer/media/ch...
chrome/renderer/media/chrome_key_systems.cc:154: static bool
IsWidevineCdmAvailable(SupportedCodecs* supported_codecs) {
On 2014/05/02 01:12:58, ddorwin wrote:
> On 2014/05/02 00:30:26, Yuncheol Heo wrote:
> > On 2014/05/02 00:22:31, xhwang wrote:
> > > On 2014/05/01 23:27:32, Yuncheol Heo wrote:
> > > > On 2014/05/01 22:41:22, xhwang wrote:
> > > > > The function name doesn't reflect what the function is doing. The
> function
> > > > also
> > > > > get supported codecs, which is more than "wv cdm available".
> > > > 
> > > > Yes, the naming is difficult.
> > > > I have 3 options and could you choose one or recommend another one?
> > > > 1. CheckWidevineAvailabilityAndGetSupportedCodecs().
> > > > 2. GetSupportedCodecsIfWidevineIsAvailable().
> > > > 3. Split the function as two (IsWidevineCdmAvailable(),
> > GetSupportedCodecs())
> > > > and expose additional_param_(names|values) as parameters.
> > > 
> > > This is only used before calling AddPepperBasedWidevine(). Can we always
> call
> > > AddPepperBasedWidevine() and put the content of this function in
> > > AddPepperBasedWidevine()? That can also help us hide SupportedCodecs
(which
> is
> > > an impl detail) in AddChromeKeySystems().
> > 
> > That means I need to componentize PluginInfoMessageFilter also, right?
> 
> On 2014/05/02 00:48:25, xhwang wrote:
> > 
> > I think the problem comes from the fact that AddPepperBasedWidevine() is in
> > components but IsPepperCdmRegistered() is still in Chrome. Since
> > AddPepperBasedWidevine() is not used by anything other than chrome, shall we
> > actually de-componentize AddPepperBasedWidevine()?
> > 
> > ddorwin: WDYT?
> 
> Since we have all the details in components, I think it's fine to keep them
> there. However, we do need to fix this function.
> 
> How about this?
> // Returns true if the Widevine is supported and the codecs successfully
> obtained.
> bool GetPepperBasedWidevineCodecs(codecs);
> 
> Or better yet, just wrap 184-189 in a local AddPepperBasedWidevine() function
> that inlines this function. That seems to make the mose sense.
> 
> I guess that returns to what we previously had with AddPepperBasedWidevine()

I applied the second one.

https://codereview.chromium.org/253593002/diff/90001/components/encrypted_med...
File components/encrypted_media/renderer/widevine_key_systems.cc (right):

https://codereview.chromium.org/253593002/diff/90001/components/encrypted_med...
components/encrypted_media/renderer/widevine_key_systems.cc:23: namespace
encrypted_media {
On 2014/05/01 22:41:22, xhwang wrote:
> ddorwin: shall we use "encrypted_media" or "eme" for the namespace, folder
name
> and file name prefixes?

Changed to 'cdm' per ddorwin's advice.

https://codereview.chromium.org/253593002/diff/90001/components/encrypted_med...
components/encrypted_media/renderer/widevine_key_systems.cc:28: WIDEVINE_HR,
On 2014/05/02 01:12:58, ddorwin wrote:
> Remove. Never used.

Done.

https://codereview.chromium.org/253593002/diff/90001/components/encrypted_med...
components/encrypted_media/renderer/widevine_key_systems.cc:52: case
WIDEVINE_HR:
On 2014/05/02 01:12:58, ddorwin wrote:
> Remove See above.

Done.

https://codereview.chromium.org/253593002/diff/90001/components/encrypted_med...
components/encrypted_media/renderer/widevine_key_systems.cc:77: void
AddPepperBasedWidevine(std::vector<KeySystemInfo>* concrete_key_systems,
On 2014/05/02 01:12:58, ddorwin wrote:
> Output parameter should be last.

The function itself was removed.

https://codereview.chromium.org/253593002/diff/90001/components/encrypted_med...
components/encrypted_media/renderer/widevine_key_systems.cc:78: bool
is_widevine_hr_supported,
On 2014/05/02 01:12:58, ddorwin wrote:
> Unnecessary. See above.

Done.

[xhwang, 2014-05-02 17:44:19]

This looks good to me. Just a few nit comments.

Also, next time, please keep rebase changes in a different PS :)

https://codereview.chromium.org/253593002/diff/110001/components/cdm/browser/...
File components/cdm/browser/DEPS (right):

https://codereview.chromium.org/253593002/diff/110001/components/cdm/browser/...
components/cdm/browser/DEPS:3: "+media/base",
media/base/android, which is what we need right now?

https://codereview.chromium.org/253593002/diff/110001/components/cdm/browser/...
File components/cdm/browser/encrypted_media_message_filter_android.h (right):

https://codereview.chromium.org/253593002/diff/110001/components/cdm/browser/...
components/cdm/browser/encrypted_media_message_filter_android.h:1: // Copyright
2013 The Chromium Authors. All rights reserved.
how about also rename file names from "encrypted_media*" to "cdm*"?

[ddorwin, 2014-05-02 17:58:55]

Thanks.

https://codereview.chromium.org/253593002/diff/110001/chrome/renderer/media/c...
File chrome/renderer/media/chrome_key_systems.cc (right):

https://codereview.chromium.org/253593002/diff/110001/chrome/renderer/media/c...
chrome/renderer/media/chrome_key_systems.cc:135: void GetSupportedCodecs(
Improvement: We should rename this to GetSupportedCodecsForPepperBasedCdm() or
something like that so it's clear what it does.

https://codereview.chromium.org/253593002/diff/110001/chrome/renderer/media/c...
chrome/renderer/media/chrome_key_systems.cc:203: #if defined(ENABLE_PEPPER_CDMS)
I wonder if we should move the ifdef logic to the individual functions. Then
this function would just be:

  AddExternalClearKey(key_systems_info);

#if defined(WIDEVINE_CDM_AVAILABLE)
  AddWidevine(key_systems_info);
#endif

https://codereview.chromium.org/253593002/diff/110001/components/cdm/renderer...
File components/cdm/renderer/widevine_key_systems.cc (right):

https://codereview.chromium.org/253593002/diff/110001/components/cdm/renderer...
components/cdm/renderer/widevine_key_systems.cc:22: #if
defined(WIDEVINE_CDM_AVAILABLE)
This can be outside the namespace (at line 17).

https://codereview.chromium.org/253593002/diff/110001/components/cdm/renderer...
components/cdm/renderer/widevine_key_systems.cc:35: static void
AddWidevineWithCodecs(
This function (and the one above) is now duplicated. This is likely to lead to
bugs where we forget to update Android and developer pain when this Android-only
code gets broken by desktop changes (or vice versa).

I think this should be the home for as much of the Widevine registration logic
on all platforms. (AddPepperBasedWidevine() is probably fine where it is since
it assumes other parts of chrome/ code.)

[ycheo (away), 2014-05-05 10:15:08]

Please take another look at the CL.

https://codereview.chromium.org/253593002/diff/110001/chrome/renderer/media/c...
File chrome/renderer/media/chrome_key_systems.cc (right):

https://codereview.chromium.org/253593002/diff/110001/chrome/renderer/media/c...
chrome/renderer/media/chrome_key_systems.cc:135: void GetSupportedCodecs(
On 2014/05/02 17:58:55, ddorwin wrote:
> Improvement: We should rename this to GetSupportedCodecsForPepperBasedCdm() or
> something like that so it's clear what it does.

Done.
Renamed it to GetSupportedCodecsForPepperCdm()

https://codereview.chromium.org/253593002/diff/110001/chrome/renderer/media/c...
chrome/renderer/media/chrome_key_systems.cc:203: #if defined(ENABLE_PEPPER_CDMS)
On 2014/05/02 17:58:55, ddorwin wrote:
> I wonder if we should move the ifdef logic to the individual functions. Then
> this function would just be:
> 
>   AddExternalClearKey(key_systems_info);
> 
> #if defined(WIDEVINE_CDM_AVAILABLE)
>   AddWidevine(key_systems_info);
> #endif

Done.

https://codereview.chromium.org/253593002/diff/110001/components/cdm/browser/...
File components/cdm/browser/DEPS (right):

https://codereview.chromium.org/253593002/diff/110001/components/cdm/browser/...
components/cdm/browser/DEPS:3: "+media/base",
On 2014/05/02 17:44:20, xhwang wrote:
> media/base/android, which is what we need right now?

Done.

https://codereview.chromium.org/253593002/diff/110001/components/cdm/browser/...
File components/cdm/browser/encrypted_media_message_filter_android.h (right):

https://codereview.chromium.org/253593002/diff/110001/components/cdm/browser/...
components/cdm/browser/encrypted_media_message_filter_android.h:1: // Copyright
2013 The Chromium Authors. All rights reserved.
On 2014/05/02 17:44:20, xhwang wrote:
> how about also rename file names from "encrypted_media*" to "cdm*"?

Done.

https://codereview.chromium.org/253593002/diff/110001/components/cdm/renderer...
File components/cdm/renderer/widevine_key_systems.cc (right):

https://codereview.chromium.org/253593002/diff/110001/components/cdm/renderer...
components/cdm/renderer/widevine_key_systems.cc:22: #if
defined(WIDEVINE_CDM_AVAILABLE)
On 2014/05/02 17:58:55, ddorwin wrote:
> This can be outside the namespace (at line 17).

Done.

https://codereview.chromium.org/253593002/diff/110001/components/cdm/renderer...
components/cdm/renderer/widevine_key_systems.cc:35: static void
AddWidevineWithCodecs(
On 2014/05/02 17:58:55, ddorwin wrote:
> This function (and the one above) is now duplicated. This is likely to lead to
> bugs where we forget to update Android and developer pain when this
Android-only
> code gets broken by desktop changes (or vice versa).
> 
> I think this should be the home for as much of the Widevine registration logic
> on all platforms. (AddPepperBasedWidevine() is probably fine where it is since
> it assumes other parts of chrome/ code.)

Done.

[ddorwin, 2014-05-05 17:20:24]

Thanks. Changes for my comments LG. I'll let the others complete the review.

https://codereview.chromium.org/253593002/diff/130001/chrome/renderer/media/c...
File chrome/renderer/media/chrome_key_systems.cc (right):

https://codereview.chromium.org/253593002/diff/130001/chrome/renderer/media/c...
chrome/renderer/media/chrome_key_systems.cc:149: additional_param_values,
&codecs);
nit: last param on a separate line for consistency

https://codereview.chromium.org/253593002/diff/130001/components/cdm/renderer...
File components/cdm/renderer/widevine_key_systems.cc (right):

https://codereview.chromium.org/253593002/diff/130001/components/cdm/renderer...
components/cdm/renderer/widevine_key_systems.cc:16: 
nit: The ifdef can even be here.

[xhwang, 2014-05-05 17:47:52]

lgtm, thanks!

[ycheo (away), 2014-05-06 07:16:21]

PTAL.

@jochen, Please review changes in:
  compontents/*

@jam, Please review changes in:
  chrome/*

@boliu, Please review changes in:
  android_webview/*

https://codereview.chromium.org/253593002/diff/130001/chrome/renderer/media/c...
File chrome/renderer/media/chrome_key_systems.cc (right):

https://codereview.chromium.org/253593002/diff/130001/chrome/renderer/media/c...
chrome/renderer/media/chrome_key_systems.cc:149: additional_param_values,
&codecs);
On 2014/05/05 17:20:25, ddorwin wrote:
> nit: last param on a separate line for consistency

Done.

https://codereview.chromium.org/253593002/diff/130001/components/cdm/renderer...
File components/cdm/renderer/widevine_key_systems.cc (right):

https://codereview.chromium.org/253593002/diff/130001/components/cdm/renderer...
components/cdm/renderer/widevine_key_systems.cc:16: 
On 2014/05/05 17:20:25, ddorwin wrote:
> nit: The ifdef can even be here.

Done.

[jochen (gone - plz use gerrit), 2014-05-06 09:48:04]

lgtm with comments

https://codereview.chromium.org/253593002/diff/170001/components/cdm/browser/...
File components/cdm/browser/DEPS (right):

https://codereview.chromium.org/253593002/diff/170001/components/cdm/browser/...
components/cdm/browser/DEPS:3: "+media/base/android",
you should list that as dependency in the gyp file as well

[boliu, 2014-05-06 21:49:59]

android_webview lgtm

[ycheo (away), 2014-05-06 23:24:14]

https://codereview.chromium.org/253593002/diff/170001/components/cdm/browser/...
File components/cdm/browser/DEPS (right):

https://codereview.chromium.org/253593002/diff/170001/components/cdm/browser/...
components/cdm/browser/DEPS:3: "+media/base/android",
On 2014/05/06 09:48:04, jochen wrote:
> you should list that as dependency in the gyp file as well

Done.
Thanks for pointing this out.

[ycheo (away), 2014-05-07 12:54:47]

@jam,
Could you review the changes in chrome/* again?

[jam, 2014-05-08 07:25:01]

chrome lgtm

[ycheo (away), 2014-05-08 10:36:11]

The CQ bit was checked by ycheo@chromium.org

[commit-bot: I haz the power, 2014-05-08 10:40:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ycheo@chromium.org/253593002/310001

[commit-bot: I haz the power, 2014-05-09 14:54:46]

Change committed as 269297