Simplify/Cleanup MediaClient

Simplify/Cleanup MediaClient

This CL does a number of things to cleanup MediaClient's 
implementation and usage. This clears the way for an expanded role
in describing MediaCapabilities. 

Remove CastMediaClient. Merge methods into CastContentRenderClient.
  ContentRenderClient is now the one-stop-shop for embedder 
  customization. I initially  explored giving each embedder its own
  MediaClient, but this ended up splitting media customization
  between ContentRenderClient and MediaClient (some customization is
  too high level to move to MediaClient). The current route is also
  less code. 

Remove all Chrome specific logic from RenderMediaClient. 
  RenderMediaClient is now the plumbing for *all* content embedder
  customizations to be visible in media/. Chrome is just one of many
  embedders and much of the chrome specific code was stale anyway.

Remove KeySystemNameForUMA from MediaClient interface
  Decentralizing this logic was overly complicated. 
  Now centralized to key_systems.cc.

Other little cleanups
  Centralize all media/ Rappor usage in MediaLog.
  Move cast key_systems_cast into chromecast/renderer/media.
  Simplify key_systems_unittest.cc

BUG=695264
Review-Url: https://codereview.chromium.org/2712983004
Cr-Commit-Position: refs/heads/master@{#463025}
Committed: https://chromium.googlesource.com/chromium/src/+/8aeee0b5c65d2ea16d2c52839cf80bcf99273555

[chcunningham, 2017-03-02 19:38:34]

cc: mlamouri@chromium.org

[chcunningham, 2017-03-11 00:38:00]

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

[commit-bot: I haz the power, 2017-03-11 00:38:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-11 00:47:30]

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

[commit-bot: I haz the power, 2017-03-11 00:47:30]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[chcunningham, 2017-03-13 20:44:05]

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

[commit-bot: I haz the power, 2017-03-13 20:44:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chcunningham, 2017-03-13 20:48:58]

The CQ bit was unchecked by chcunningham@chromium.org

[chcunningham, 2017-03-13 20:51:03]

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

[commit-bot: I haz the power, 2017-03-13 20:52:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-13 21:04:48]

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

[commit-bot: I haz the power, 2017-03-13 21:04:49]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[chcunningham, 2017-03-14 01:51:53]

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

[commit-bot: I haz the power, 2017-03-14 01:52:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-14 02:01:32]

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

[commit-bot: I haz the power, 2017-03-14 02:01:33]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[chcunningham, 2017-03-15 21:25:21]

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

[commit-bot: I haz the power, 2017-03-15 21:26:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-15 21:31:26]

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

[commit-bot: I haz the power, 2017-03-15 21:31:27]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[chcunningham, 2017-03-18 00:04:26]

Patchset #1 (id:1) has been deleted

[chcunningham, 2017-03-18 00:04:36]

Patchset #1 (id:20001) has been deleted

[chcunningham, 2017-03-18 00:04:47]

Patchset #1 (id:40001) has been deleted

[chcunningham, 2017-03-18 00:04:58]

Patchset #1 (id:60001) has been deleted

[chcunningham, 2017-03-18 00:05:08]

Patchset #1 (id:80001) has been deleted

[chcunningham, 2017-03-18 00:05:18]

Patchset #1 (id:100001) has been deleted

[chcunningham, 2017-03-18 00:05:26]

Patchset #1 (id:120001) has been deleted

[chcunningham, 2017-03-18 00:05:35]

Patchset #1 (id:140001) has been deleted

[chcunningham, 2017-03-18 00:06:05]

Description was changed from

==========
Detangle Cast and Chrome MediaClient.

BUG=
==========

to

==========
This CL does a number of things to cleanup MediaClient's 
implementation and usage. This clears the way for an expanded role
in describing MediaCapabilities. 

remove IsUpdateNeeded from MediaClient interface
  This was a widevine specific mechanism and is no longer needed
  since we now ship widevine with chrome (vs component install).

remove KeySystemNameForUMA from MediaClient interface
  Decentralizing this logic was overly complicated. 
  Now centralized to key_systems.cc.

new ContentMediaClient in content/public/renderer
  This is similar to the old RenderMediaClient, but without all the
  chrome-specific logic. Used by most embedders that want to 
  customize key systems but not decode capabilities.
  Base class for CastMediaClient

::SetMediaClient now only called in various ContentRenderClients
  We should not want a media client by default - only needed if 
  embedder wants to customize media/

move cast_media_client from chromecast/ common/ -> renderer/
  Should have been there all along

BUG=695264
==========

[chcunningham, 2017-03-18 00:06:14]

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

[commit-bot: I haz the power, 2017-03-18 00:07:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-18 00:12:03]

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

[commit-bot: I haz the power, 2017-03-18 00:12:04]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[chcunningham, 2017-03-18 00:17:22]

chcunningham@chromium.org changed reviewers:
+ xhwang@chromium.org

[chcunningham, 2017-03-18 00:17:23]

Hey Xiaohan, this change incorporates a number of things we've discussed. PTAL

Ignore the bots for now - I think they're unahppy that I've renamed some files
that are modified by dependent patch sets.

https://codereview.chromium.org/2712983004/diff/160001/content/renderer/media...
File content/renderer/media/render_media_client_unittest.cc (left):

https://codereview.chromium.org/2712983004/diff/160001/content/renderer/media...
content/renderer/media/render_media_client_unittest.cc:120:
TEST_F(RenderMediaClientTest, KeySystemNameForUMA) {
This whole file is now deleted. It was only testing methods that are now
removed.

[chcunningham, 2017-03-18 00:18:07]

chcunningham@chromium.org changed reviewers:
+ servolk@chromium.org

[chcunningham, 2017-03-18 00:18:08]

+servolk for cast

[chcunningham, 2017-03-21 22:24:02]

Friendly ping.

[xhwang, 2017-03-21 22:30:42]

oops, this totally got off my radar. will take a look today or tomorrow. sorry
for the delay!

[servolk, 2017-03-22 01:22:29]

servolk@chromium.org changed reviewers:
+ halliwell@chromium.org

[servolk, 2017-03-22 01:22:51]

On 2017/03/21 22:30:42, xhwang_slow wrote:
> oops, this totally got off my radar. will take a look today or tomorrow. sorry
> for the delay!

chromecast/ lgtm
+Luke to ensure that removal of key systems stuff from the media client is ok
from cast_shell point of view.

[chcunningham, 2017-03-22 01:33:48]

Thanks Sergey!

> +Luke to ensure that removal of key systems stuff from the media client is ok
> from cast_shell point of view.

Behavior should be exactly as it is today - the cast key systems are still
described by cast content client. But do double check me ;)

[chcunningham, 2017-03-22 01:34:14]

On 2017/03/22 01:33:48, chcunningham wrote:
> Thanks Sergey!
> 
> > +Luke to ensure that removal of key systems stuff from the media client is
ok
> > from cast_shell point of view.
> 
> Behavior should be exactly as it is today - the cast key systems are still
> described by cast content client. But do double check me ;)

Luke, I'll need your LGTM anyway - for cast owners

[halliwell, 2017-03-22 02:39:39]

On 2017/03/22 01:34:14, chcunningham wrote:
> On 2017/03/22 01:33:48, chcunningham wrote:
> > Thanks Sergey!
> > 
> > > +Luke to ensure that removal of key systems stuff from the media client is
> ok
> > > from cast_shell point of view.
> > 
> > Behavior should be exactly as it is today - the cast key systems are still
> > described by cast content client. But do double check me ;)
> 
> Luke, I'll need your LGTM anyway - for cast owners

lgtm

[xhwang, 2017-03-22 05:58:52]

Thanks for doing this! I really appreciate the simplification brought by this
CL!

I have some high level comments for discussion.

https://codereview.chromium.org/2712983004/diff/160001/content/public/rendere...
File content/public/renderer/content_media_client.h (right):

https://codereview.chromium.org/2712983004/diff/160001/content/public/rendere...
content/public/renderer/content_media_client.h:15: // supported Key Systems and
Rapport recording to their ContentRenderClient.
s/Rapport/Rappor

https://codereview.chromium.org/2712983004/diff/160001/content/public/rendere...
content/public/renderer/content_media_client.h:20: class CONTENT_EXPORT
ContentMediaClient : public media::MediaClient {
I have a few thoughts on this. I am not following closely on your overall plan,
and my thoughts are strictly based on the current CL so I might be missing the
big picture.

1. "content/public should contain only interfaces, structs and (rarely) static
functions":
https://www.chromium.org/developers/content-module/content-api

2. About naming. "content" is the multi-process architecture of Chromium, which
covers browser/renderer/gpu/utility/common etc. This MediaClient is strictly
used for media code running in the renderer process, so I still feel
RendererMediaClient or RenderMediaClient (we can debate about Renderer vs Render
for a day) is a better name than ContentMediaClient, especially since it takes a
ContentRENDERERClient...

3.  For IsSupportedVideoConfig(). If ContentMediaClient supports a VideoConfig,
is it guaranteed that this config will be supported on all platforms (e.g. for
all content embedders)? If yes, it seem
CastMediaClient::IsSupportedVideoConfig() should call the base implementation
ContentMediaClient::IsSupportedVideoConfig(). If not, then we probably should
not provide this default implementation in the content layer. Rather, it should
be in something like a ChromeMediaClient. See more discussion below.

4. It seems a bit odd that each ContentRendererClient sets a MediaClient, which
mostly just calls back to the ContentRenderClient to do work. 

When MediaClient was added, there's only one implementation (RenderMediaClient),
which talks to ContentRendererClient for content-embedder-specific behavior. I
don't remember the detailed context around the CastMediaClient. But now looking
at this CL, with the simplification you did (thank you!), it seems we can easily
keep the original model and make things simpler IMHO.

Basically you can keep everything in ContentMediaClient (or
RendererMediaClient), where most calls goes directly to the
content_renderer_client_. The only exception is IsSupportedVideoConfig(), where
we want to provide a default behavior. So you can do something like this:

bool ContentMediaClient::IsSupportedVideoConfig(config) {
  // You can also check |content_renderer_client_| first/only, depending on the
answer to question 3 above.
  if (media::IsSupportedVideoConfig(config))
    return true;
  return content_renderer_client_->IsSupportedVideoConfig(config);
}

The nice thing about this is that all content-embedder-specific behavior is
defined in ContentRendererClient, and we only have one MediaClient
implementation that is a thin glue layer, which seems the right thing to do. In
the current CL, both MediaClient and ContentRendererClient have some
content-embedder-specific code, which is a bit harder to track.

[xhwang, 2017-03-22 06:00:18]

On 2017/03/22 05:58:52, xhwang_slow wrote:
> Thanks for doing this! I really appreciate the simplification brought by this
> CL!
> 
> I have some high level comments for discussion.
> 
>
https://codereview.chromium.org/2712983004/diff/160001/content/public/rendere...
> File content/public/renderer/content_media_client.h (right):
> 
>
https://codereview.chromium.org/2712983004/diff/160001/content/public/rendere...
> content/public/renderer/content_media_client.h:15: // supported Key Systems
and
> Rapport recording to their ContentRenderClient.
> s/Rapport/Rappor
> 
>
https://codereview.chromium.org/2712983004/diff/160001/content/public/rendere...
> content/public/renderer/content_media_client.h:20: class CONTENT_EXPORT
> ContentMediaClient : public media::MediaClient {
> I have a few thoughts on this. I am not following closely on your overall
plan,
> and my thoughts are strictly based on the current CL so I might be missing the
> big picture.
> 
> 1. "content/public should contain only interfaces, structs and (rarely) static
> functions":
> https://www.chromium.org/developers/content-module/content-api
> 
> 2. About naming. "content" is the multi-process architecture of Chromium,
which
> covers browser/renderer/gpu/utility/common etc. This MediaClient is strictly
> used for media code running in the renderer process, so I still feel
> RendererMediaClient or RenderMediaClient (we can debate about Renderer vs
Render
> for a day) is a better name than ContentMediaClient, especially since it takes
a
> ContentRENDERERClient...
> 
> 3.  For IsSupportedVideoConfig(). If ContentMediaClient supports a
VideoConfig,
> is it guaranteed that this config will be supported on all platforms (e.g. for
> all content embedders)? If yes, it seem
> CastMediaClient::IsSupportedVideoConfig() should call the base implementation
> ContentMediaClient::IsSupportedVideoConfig(). If not, then we probably should
> not provide this default implementation in the content layer. Rather, it
should
> be in something like a ChromeMediaClient. See more discussion below.
> 
> 4. It seems a bit odd that each ContentRendererClient sets a MediaClient,
which
> mostly just calls back to the ContentRenderClient to do work. 
> 
> When MediaClient was added, there's only one implementation
(RenderMediaClient),
> which talks to ContentRendererClient for content-embedder-specific behavior. I
> don't remember the detailed context around the CastMediaClient. But now
looking
> at this CL, with the simplification you did (thank you!), it seems we can
easily
> keep the original model and make things simpler IMHO.
> 
> Basically you can keep everything in ContentMediaClient (or
> RendererMediaClient), where most calls goes directly to the
> content_renderer_client_. The only exception is IsSupportedVideoConfig(),
where
> we want to provide a default behavior. So you can do something like this:
> 
> bool ContentMediaClient::IsSupportedVideoConfig(config) {
>   // You can also check |content_renderer_client_| first/only, depending on
the
> answer to question 3 above.
>   if (media::IsSupportedVideoConfig(config))
>     return true;
>   return content_renderer_client_->IsSupportedVideoConfig(config);
> }
> 
> The nice thing about this is that all content-embedder-specific behavior is
> defined in ContentRendererClient, and we only have one MediaClient
> implementation that is a thin glue layer, which seems the right thing to do.
In
> the current CL, both MediaClient and ContentRendererClient have some
> content-embedder-specific code, which is a bit harder to track.

I forgot to mention. If you do (4), we don't need CastMediaClient at all. And we
don't need to set MediaClient in each  ContentRendererClient...

[chcunningham, 2017-03-22 18:25:46]

Thanks Xiaohan!

https://codereview.chromium.org/2712983004/diff/160001/content/public/rendere...
File content/public/renderer/content_media_client.h (right):

https://codereview.chromium.org/2712983004/diff/160001/content/public/rendere...
content/public/renderer/content_media_client.h:20: class CONTENT_EXPORT
ContentMediaClient : public media::MediaClient {
Thanks. I'm agree the direction you propose sounds like a good simplification.
Below I'll  give answers to explain my first attempt and be sure we've
considered everything. And sneak in some follow up questions.

On 2017/03/22 05:58:52, xhwang_slow wrote:
> I have a few thoughts on this. I am not following closely on your overall
plan,
> and my thoughts are strictly based on the current CL so I might be missing the
> big picture.
> 
> 1. "content/public should contain only interfaces, structs and (rarely) static
> functions":
> https://www.chromium.org/developers/content-module/content-api

Ack. I agree I'm not following that guideline. I'm also not the first to break
it ;) - see media_stream_video_sink.cc for an example of an interface with some
default behavior. 

I like that your proposal works around this.

> 
> 2. About naming. "content" is the multi-process architecture of Chromium,
which
> covers browser/renderer/gpu/utility/common etc. This MediaClient is strictly
> used for media code running in the renderer process, so I still feel
> RendererMediaClient or RenderMediaClient (we can debate about Renderer vs
Render
> for a day) is a better name than ContentMediaClient, especially since it takes
a
> ContentRENDERERClient...

Happy to change it back, but for posterity:
I think ContentRenderMediaClient is most correct, but too long. I liked
ContentMediaClient because its impl describes the default state of media
capabilities for content embedders. The fact that those capabilities are in the
"render" process is more of a detail IMHO. 

> 
> 3.  For IsSupportedVideoConfig(). If ContentMediaClient supports a
VideoConfig,
> is it guaranteed that this config will be supported on all platforms (e.g. for
> all content embedders)? If yes, it seem
> CastMediaClient::IsSupportedVideoConfig() should call the base implementation
> ContentMediaClient::IsSupportedVideoConfig(). 

No, embedders are free to change everything about the capabilities. Calling the
base class would be wrong. Embedders should be aware of what specific
capabilities they've added and subtracted, described by their override of
IsSupportedVideoConfig. For capabilities they have not changed I recommend they
should defer to media::IsSupportedVideoConfig(...).

> If not, then we probably should
> not provide this default implementation in the content layer. Rather, it
should
> be in something like a ChromeMediaClient. See more discussion below.

The problem with ChromeMediaClient is that these are not the default
capabilities of "Chrome", they are the default capabilities of "content". Both
chrome and webview embed content without changing these capabilities, so I
attempted to place a base class somewhere that expressed that relationship and
reduced duplication. Your proposal addresses this somewhat - see answer to #4.

> 4. It seems a bit odd that each ContentRendererClient sets a MediaClient,
which
> mostly just calls back to the ContentRenderClient to do work. 
> 
> When MediaClient was added, there's only one implementation
(RenderMediaClient),
> which talks to ContentRendererClient for content-embedder-specific behavior. I
> don't remember the detailed context around the CastMediaClient. But now
looking
> at this CL, with the simplification you did (thank you!), it seems we can
easily
> keep the original model and make things simpler IMHO.
> 
> Basically you can keep everything in ContentMediaClient (or
> RendererMediaClient), where most calls goes directly to the
> content_renderer_client_. The only exception is IsSupportedVideoConfig(),
where
> we want to provide a default behavior. So you can do something like this:
> 
> bool ContentMediaClient::IsSupportedVideoConfig(config) {
>   // You can also check |content_renderer_client_| first/only, depending on
the
> answer to question 3 above.
>   if (media::IsSupportedVideoConfig(config))
>     return true;
>   return content_renderer_client_->IsSupportedVideoConfig(config);
> }
> 
> The nice thing about this is that all content-embedder-specific behavior is
> defined in ContentRendererClient, and we only have one MediaClient
> implementation that is a thin glue layer, which seems the right thing to do.
In
> the current CL, both MediaClient and ContentRendererClient have some
> content-embedder-specific code, which is a bit harder to track.

I follow you. Here was my thinking:

MediaClient could soon be home to more embedder specific logic to describe
decode capabilities for different key system configurations. Since media/ knows
little of the key system decode capabilities (especially their performance
characteristics), I invision EmbedderNameMediaClient doing things like checking
system specs (ram/cpu) against known (researched) requirements for smooth
decoding of particular codec/resolution/framerate. This CL aimed to focus
embedder media clients on this task, leaving the content client glue to a base
class. 

My thinking now: I can still achieve all this with a common RenderMediaClient,
putting everything into ContentRenderClient. Its a lot of nitty gritty detail
for ContentRenderClient, but thats easy to solve with helper classes etc. 

One thing to iron out is how to implement
ContentMediaClient::IsSupportedVideoConfg(). The implementation you proposed
would assume embedders only build on media's default capabilities - not the
case. We can instead have ContentMediaClient defer entirely to
ContentRendererClient and have the default ContentRendererClient simply check
media::IsSupportedVideoConfig(). Sound good?

[chcunningham, 2017-03-22 18:29:04]

On 2017/03/22 18:25:46, chcunningham wrote:
> My thinking now: I can still achieve all this with a common RenderMediaClient,
> putting everything into ContentRenderClient. Its a lot of nitty gritty detail
> for ContentRenderClient, but thats easy to solve with helper classes etc. 

A made a typo above. I meant to write "putting everything into
EmbedderNameContentRenderClient".

The implementation for default ContentRenderClient is discussed separately in
the paragraph immediately after the one where I made the typo.

[xhwang, 2017-03-22 21:54:53]

https://codereview.chromium.org/2712983004/diff/160001/content/public/rendere...
File content/public/renderer/content_media_client.h (right):

https://codereview.chromium.org/2712983004/diff/160001/content/public/rendere...
content/public/renderer/content_media_client.h:20: class CONTENT_EXPORT
ContentMediaClient : public media::MediaClient {
On 2017/03/22 18:25:46, chcunningham wrote:
> Thanks. I'm agree the direction you propose sounds like a good simplification.
> Below I'll  give answers to explain my first attempt and be sure we've
> considered everything. And sneak in some follow up questions.
> 
> On 2017/03/22 05:58:52, xhwang_slow wrote:
> > I have a few thoughts on this. I am not following closely on your overall
> plan,
> > and my thoughts are strictly based on the current CL so I might be missing
the
> > big picture.
> > 
> > 1. "content/public should contain only interfaces, structs and (rarely)
static
> > functions":
> > https://www.chromium.org/developers/content-module/content-api
> 
> Ack. I agree I'm not following that guideline. I'm also not the first to break
> it ;) - see media_stream_video_sink.cc for an example of an interface with
some
> default behavior. 
> 
> I like that your proposal works around this.
> 
> > 
> > 2. About naming. "content" is the multi-process architecture of Chromium,
> which
> > covers browser/renderer/gpu/utility/common etc. This MediaClient is strictly
> > used for media code running in the renderer process, so I still feel
> > RendererMediaClient or RenderMediaClient (we can debate about Renderer vs
> Render
> > for a day) is a better name than ContentMediaClient, especially since it
takes
> a
> > ContentRENDERERClient...
> 
> Happy to change it back, but for posterity:
> I think ContentRenderMediaClient is most correct, but too long. I liked
> ContentMediaClient because its impl describes the default state of media
> capabilities for content embedders. The fact that those capabilities are in
the
> "render" process is more of a detail IMHO.

I always feel "renderer" is part of "content" so when you have "renderer" the
"content" part is a bit redundant. But I'll leave this up to you :)

One way to look at this is that in content/public/renderer folder you have many
classes started with "render" or "rendrere", but only very few with "content":
https://cs.chromium.org/chromium/src/content/public/renderer/?q=content/publi...


> > 3.  For IsSupportedVideoConfig(). If ContentMediaClient supports a
> VideoConfig,
> > is it guaranteed that this config will be supported on all platforms (e.g.
for
> > all content embedders)? If yes, it seem
> > CastMediaClient::IsSupportedVideoConfig() should call the base
implementation
> > ContentMediaClient::IsSupportedVideoConfig(). 
> 
> No, embedders are free to change everything about the capabilities. Calling
the
> base class would be wrong. Embedders should be aware of what specific
> capabilities they've added and subtracted, described by their override of
> IsSupportedVideoConfig. For capabilities they have not changed I recommend
they
> should defer to media::IsSupportedVideoConfig(...).
> > If not, then we probably should
> > not provide this default implementation in the content layer. Rather, it
> should
> > be in something like a ChromeMediaClient. See more discussion below.
> 
> The problem with ChromeMediaClient is that these are not the default
> capabilities of "Chrome", they are the default capabilities of "content". Both
> chrome and webview embed content without changing these capabilities, so I
> attempted to place a base class somewhere that expressed that relationship and
> reduced duplication. Your proposal addresses this somewhat - see answer to #4.

I see. That makes sense. So content/ uses RendererImpl, but content embedders
might use other Renderer implementation.

> > 4. It seems a bit odd that each ContentRendererClient sets a MediaClient,
> which
> > mostly just calls back to the ContentRenderClient to do work. 
> > 
> > When MediaClient was added, there's only one implementation
> (RenderMediaClient),
> > which talks to ContentRendererClient for content-embedder-specific behavior.
I
> > don't remember the detailed context around the CastMediaClient. But now
> looking
> > at this CL, with the simplification you did (thank you!), it seems we can
> easily
> > keep the original model and make things simpler IMHO.
> > 
> > Basically you can keep everything in ContentMediaClient (or
> > RendererMediaClient), where most calls goes directly to the
> > content_renderer_client_. The only exception is IsSupportedVideoConfig(),
> where
> > we want to provide a default behavior. So you can do something like this:
> > 
> > bool ContentMediaClient::IsSupportedVideoConfig(config) {
> >   // You can also check |content_renderer_client_| first/only, depending on
> the
> > answer to question 3 above.
> >   if (media::IsSupportedVideoConfig(config))
> >     return true;
> >   return content_renderer_client_->IsSupportedVideoConfig(config);
> > }
> > 
> > The nice thing about this is that all content-embedder-specific behavior is
> > defined in ContentRendererClient, and we only have one MediaClient
> > implementation that is a thin glue layer, which seems the right thing to do.
> In
> > the current CL, both MediaClient and ContentRendererClient have some
> > content-embedder-specific code, which is a bit harder to track.
> 
> I follow you. Here was my thinking:
> 
> MediaClient could soon be home to more embedder specific logic to describe
> decode capabilities for different key system configurations. Since media/
knows
> little of the key system decode capabilities (especially their performance
> characteristics), I invision EmbedderNameMediaClient doing things like
checking
> system specs (ram/cpu) against known (researched) requirements for smooth
> decoding of particular codec/resolution/framerate. This CL aimed to focus
> embedder media clients on this task, leaving the content client glue to a base
> class. 

Do you have any idea how much stuff we'll add into MediaClient? If a lot,
instead of polluting ContentRendererClient with a lot of media stuff, we could
have a ContentRendererClient::GetMediaClient(), similar to
ContentClient::GetMediaDrmBridgeClient() you just renamed. Then we can put
everything related to media there, including AddSupportedKeySystems().
RecordRapporURL is special since it's used by code outside of media as well, but
having only one exception seems okay.

> My thinking now: I can still achieve all this with a common RenderMediaClient,
> putting everything into ContentRenderClient. Its a lot of nitty gritty detail
> for ContentRenderClient, but thats easy to solve with helper classes etc. 
> 
> One thing to iron out is how to implement
> ContentMediaClient::IsSupportedVideoConfg(). The implementation you proposed
> would assume embedders only build on media's default capabilities - not the
> case. We can instead have ContentMediaClient defer entirely to
> ContentRendererClient and have the default ContentRendererClient simply check
> media::IsSupportedVideoConfig(). Sound good?

SGTM with your correction!

[chcunningham, 2017-03-22 23:22:51]

https://codereview.chromium.org/2712983004/diff/160001/content/public/rendere...
File content/public/renderer/content_media_client.h (right):

https://codereview.chromium.org/2712983004/diff/160001/content/public/rendere...
content/public/renderer/content_media_client.h:20: class CONTENT_EXPORT
ContentMediaClient : public media::MediaClient {
> Do you have any idea how much stuff we'll add into MediaClient? If a lot,
> instead of polluting ContentRendererClient with a lot of media stuff, we could
> have a ContentRendererClient::GetMediaClient(), similar to
> ContentClient::GetMediaDrmBridgeClient() you just renamed. Then we can put
> everything related to media there, including AddSupportedKeySystems().
> RecordRapporURL is special since it's used by code outside of media as well,
but
> having only one exception seems okay.

The logic for key system decode capabilities could end up being a couple of
hundred lines, but I think it will probably end up being largely implemented in
the components/cdm folder. This will allow for re-use between chrome-android and
webview and is already the home for describing android widevine and platform key
systems. For folks like cast, I agree putting the capabilities logic into their
content clients is a bit much, but they can easily defer to a helper class (and
already are).

Alternatively, we can go the MediaClient route (essentially mandating the
interface for a helper class) and move the supported key systems into this class
(instead of deferring to content client). 

I lean slightly toward this second option. I think its a little more structured
and gives embedder media customization its own first class space. Sound good?

[chcunningham, 2017-03-22 23:24:59]

On 2017/03/22 23:22:51, chcunningham wrote:
>
https://codereview.chromium.org/2712983004/diff/160001/content/public/rendere...
> File content/public/renderer/content_media_client.h (right):
> 
>
https://codereview.chromium.org/2712983004/diff/160001/content/public/rendere...
> content/public/renderer/content_media_client.h:20: class CONTENT_EXPORT
> ContentMediaClient : public media::MediaClient {
> > Do you have any idea how much stuff we'll add into MediaClient? If a lot,
> > instead of polluting ContentRendererClient with a lot of media stuff, we
could
> > have a ContentRendererClient::GetMediaClient(), similar to
> > ContentClient::GetMediaDrmBridgeClient() you just renamed. Then we can put
> > everything related to media there, including AddSupportedKeySystems().
> > RecordRapporURL is special since it's used by code outside of media as well,
> but
> > having only one exception seems okay.
> 
> The logic for key system decode capabilities could end up being a couple of
> hundred lines, but I think it will probably end up being largely implemented
in
> the components/cdm folder. This will allow for re-use between chrome-android
and
> webview and is already the home for describing android widevine and platform
key
> systems. For folks like cast, I agree putting the capabilities logic into
their
> content clients is a bit much, but they can easily defer to a helper class
(and
> already are).
> 
> Alternatively, we can go the MediaClient route (essentially mandating the
> interface for a helper class) and move the supported key systems into this
class
> (instead of deferring to content client). 
> 
> I lean slightly toward this second option. I think its a little more
structured
> and gives embedder media customization its own first class space. Sound good?

though I still hate the rappor exception, so feel free to insist on the first
option instead...

[xhwang, 2017-03-23 04:53:25]

On 2017/03/22 23:24:59, chcunningham wrote:
> On 2017/03/22 23:22:51, chcunningham wrote:
> >
>
https://codereview.chromium.org/2712983004/diff/160001/content/public/rendere...
> > File content/public/renderer/content_media_client.h (right):
> > 
> >
>
https://codereview.chromium.org/2712983004/diff/160001/content/public/rendere...
> > content/public/renderer/content_media_client.h:20: class CONTENT_EXPORT
> > ContentMediaClient : public media::MediaClient {
> > > Do you have any idea how much stuff we'll add into MediaClient? If a lot,
> > > instead of polluting ContentRendererClient with a lot of media stuff, we
> could
> > > have a ContentRendererClient::GetMediaClient(), similar to
> > > ContentClient::GetMediaDrmBridgeClient() you just renamed. Then we can put
> > > everything related to media there, including AddSupportedKeySystems().
> > > RecordRapporURL is special since it's used by code outside of media as
well,
> > but
> > > having only one exception seems okay.
> > 
> > The logic for key system decode capabilities could end up being a couple of
> > hundred lines, but I think it will probably end up being largely implemented
> in

How many methods will we add for capabilities? The number of lines for the
implementation is not important. I am mostly concerned about how much stuff we
need to add into ContentRendererClient or the new MediaClient interface.

> > the components/cdm folder. This will allow for re-use between chrome-android
> and
> > webview and is already the home for describing android widevine and platform
> key
> > systems. For folks like cast, I agree putting the capabilities logic into
> their
> > content clients is a bit much, but they can easily defer to a helper class
> (and
> > already are).
> > 
> > Alternatively, we can go the MediaClient route (essentially mandating the
> > interface for a helper class) and move the supported key systems into this
> class
> > (instead of deferring to content client). 
> > 
> > I lean slightly toward this second option. I think its a little more
> structured
> > and gives embedder media customization its own first class space. Sound
good?
> 
> though I still hate the rappor exception, so feel free to insist on the first
> option instead...

I don't really have a strong opinion. Both solution sounds good to me!

[xhwang, 2017-03-23 05:41:14]

For IsKeySystemsUpdateNeeded(), is it causing trouble for now? We don't really
need this for Chrome. But I just realized that some other Chromium-based browser
might depend on it. I have to check with people to see whether we can remove it.
If it's not causing too much trouble for your work, can we keep it for now?

[chcunningham, 2017-03-23 19:14:02]

On 2017/03/23 05:41:14, xhwang_slow wrote:
> For IsKeySystemsUpdateNeeded(), is it causing trouble for now? We don't really
> need this for Chrome. But I just realized that some other Chromium-based
browser
> might depend on it. I have to check with people to see whether we can remove
it.
> If it's not causing too much trouble for your work, can we keep it for now?

It is not so much a problem now - its more just that its very chrome-specific
and none of the content embedders I'm aware of need this. Cast, Chrome (and
Android), and Webview are now all able to describe their key systems when first
asked. 

I also really dislike the way the contract assumes
MediaClient::AddSupportedKeySystems only has one caller. 

Which browser are you worried about? 

I would prefer to solve the problem by getting rid of the cache in
key_systems.cc, and instead having key_systems.cc always call into MediaClient
for key system properties. Down the road, MediaClient will answer questions
about decode capabilities *in the context of EME*. To do so efficiently, it will
need to avoid always making an IPC to the browser process to build these
properties. This allows folks who need to keep updating key systems to do so
under the hood without this weird IsUpdateNeeded API. But I've not made this
change for the time being because it grows the CL and no one seemed to need it
right away.

[chcunningham, 2017-03-30 21:42:41]

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

[commit-bot: I haz the power, 2017-03-30 21:43:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 22:01:28]

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

[commit-bot: I haz the power, 2017-03-30 22:01:29]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[chcunningham, 2017-03-30 22:26:47]

Hold off reviewing the latest - still making some edits.

[chcunningham, 2017-03-31 21:17:37]

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

[commit-bot: I haz the power, 2017-03-31 21:18:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-31 21:30:45]

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

[commit-bot: I haz the power, 2017-03-31 21:30:46]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[chcunningham, 2017-03-31 22:12:54]

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

[commit-bot: I haz the power, 2017-03-31 22:13:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chcunningham, 2017-03-31 22:27:28]

Description was changed from

==========
This CL does a number of things to cleanup MediaClient's 
implementation and usage. This clears the way for an expanded role
in describing MediaCapabilities. 

remove IsUpdateNeeded from MediaClient interface
  This was a widevine specific mechanism and is no longer needed
  since we now ship widevine with chrome (vs component install).

remove KeySystemNameForUMA from MediaClient interface
  Decentralizing this logic was overly complicated. 
  Now centralized to key_systems.cc.

new ContentMediaClient in content/public/renderer
  This is similar to the old RenderMediaClient, but without all the
  chrome-specific logic. Used by most embedders that want to 
  customize key systems but not decode capabilities.
  Base class for CastMediaClient

::SetMediaClient now only called in various ContentRenderClients
  We should not want a media client by default - only needed if 
  embedder wants to customize media/

move cast_media_client from chromecast/ common/ -> renderer/
  Should have been there all along

BUG=695264
==========

to

==========
This CL does a number of things to cleanup MediaClient's 
implementation and usage. This clears the way for an expanded role
in describing MediaCapabilities. 

Remove CastMediaClient. Merge methods into CastContentRenderClient.
  ContentRenderClient is now the one-stop-shop for embedder 
  customization. I initially  explored giving each embedder its own
  MediaClient, but this ended up splitting media customization
  between ContentRenderClient and MediaClient (some customization is
  too high level to move to MediaClient). The current route is also
  less code. 

Remove all Chrome specific logic from RenderMediaClient. 
  RenderMediaClient is now the plumbing for *all* content embedder
  customizations to be visible in media/. Chrome is just one of many
  embedders and much of the chrome specific code was stale anyway.

Remove KeySystemNameForUMA from MediaClient interface
  Decentralizing this logic was overly complicated. 
  Now centralized to key_systems.cc.

Other little cleanups
  Centralize all media/ Rappor usage in MediaLog.
  Move cast key_systems_cast into chromecast/renderer/media.
  Simplify key_systems_unittest.cc

BUG=695264
==========

[chcunningham, 2017-03-31 22:27:57]

Okay - ready for another look :)

[commit-bot: I haz the power, 2017-03-31 23:56:34]

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

[commit-bot: I haz the power, 2017-03-31 23:56:35]

Dry run: This issue passed the CQ dry run.

[xhwang, 2017-04-03 17:47:14]

Looking pretty good! I just have one comment.

https://codereview.chromium.org/2712983004/diff/220001/content/renderer/media...
File content/renderer/media/render_media_client.cc (right):

https://codereview.chromium.org/2712983004/diff/220001/content/renderer/media...
content/renderer/media/render_media_client.cc:34: return
GetContentClient()->renderer()->IsKeySystemsUpdateNeeded();
I am not seeing ChromeContentRendererClient implementing
IsKeySystemsUpdateNeeded() in this CL. What's the plan here?

https://codereview.chromium.org/2712983004/diff/220001/content/renderer/media...
File content/renderer/media/render_media_log.h (right):

https://codereview.chromium.org/2712983004/diff/220001/content/renderer/media...
content/renderer/media/render_media_log.h:42: void RecordRapporWithURL(const
std::string& metric, const GURL& url) override;
Please add a TODO to replace all usage of this one with
RecordRapporWithSecurityOrigin(), and then remove this method.

[chcunningham, 2017-04-03 18:47:39]

Thanks Xiaohan. 

Sergey/Luke, you guys might take another look at cast/ - I deleted your
MediaClient ;). On the whole I'm much happier with the latest iteration, but
CastContentRenderClient did grow some. I think once we do decode *EME*
capabilities it will make sense to have a helper class, but for now its pretty
contained.

https://codereview.chromium.org/2712983004/diff/220001/content/renderer/media...
File content/renderer/media/render_media_client.cc (right):

https://codereview.chromium.org/2712983004/diff/220001/content/renderer/media...
content/renderer/media/render_media_client.cc:34: return
GetContentClient()->renderer()->IsKeySystemsUpdateNeeded();
On 2017/04/03 17:47:13, xhwang_slow wrote:
> I am not seeing ChromeContentRendererClient implementing
> IsKeySystemsUpdateNeeded() in this CL. What's the plan here?

Chrome inherits the behavior from public/renderer/content_renderer_client.cc
(return false).

While some embedders may need the update behavior, my understanding is Chrome is
not needing it. 

Down the road I will refactor this further with a proper observer/notifier
pattern. P3.

https://codereview.chromium.org/2712983004/diff/220001/content/renderer/media...
File content/renderer/media/render_media_log.h (right):

https://codereview.chromium.org/2712983004/diff/220001/content/renderer/media...
content/renderer/media/render_media_log.h:42: void RecordRapporWithURL(const
std::string& metric, const GURL& url) override;
On 2017/04/03 17:47:14, xhwang_slow wrote:
> Please add a TODO to replace all usage of this one with
> RecordRapporWithSecurityOrigin(), and then remove this method.

Implemented the TODO

[xhwang, 2017-04-03 18:55:19]

https://codereview.chromium.org/2712983004/diff/220001/content/renderer/media...
File content/renderer/media/render_media_client.cc (right):

https://codereview.chromium.org/2712983004/diff/220001/content/renderer/media...
content/renderer/media/render_media_client.cc:34: return
GetContentClient()->renderer()->IsKeySystemsUpdateNeeded();
On 2017/04/03 18:47:38, chcunningham wrote:
> On 2017/04/03 17:47:13, xhwang_slow wrote:
> > I am not seeing ChromeContentRendererClient implementing
> > IsKeySystemsUpdateNeeded() in this CL. What's the plan here?
> 
> Chrome inherits the behavior from public/renderer/content_renderer_client.cc
> (return false).
> 
> While some embedders may need the update behavior, my understanding is Chrome
is
> not needing it. 
> 
> Down the road I will refactor this further with a proper observer/notifier
> pattern. P3. 

Sorry I wasn't super clear on this.

chrome/ is actually used by chromium and all chromium-based browsers. What makes
Google Chrome special is the "is_chrome_branded" gn flag. But even though
chromium-based browsers don't use this gn flag, they still use chrome/ and hence
ChromeContentRendererClient. So always returning false will break their CDM
update logic.

[halliwell, 2017-04-03 19:33:26]

On 2017/04/03 18:55:19, xhwang_slow wrote:
>
https://codereview.chromium.org/2712983004/diff/220001/content/renderer/media...
> File content/renderer/media/render_media_client.cc (right):
> 
>
https://codereview.chromium.org/2712983004/diff/220001/content/renderer/media...
> content/renderer/media/render_media_client.cc:34: return
> GetContentClient()->renderer()->IsKeySystemsUpdateNeeded();
> On 2017/04/03 18:47:38, chcunningham wrote:
> > On 2017/04/03 17:47:13, xhwang_slow wrote:
> > > I am not seeing ChromeContentRendererClient implementing
> > > IsKeySystemsUpdateNeeded() in this CL. What's the plan here?
> > 
> > Chrome inherits the behavior from public/renderer/content_renderer_client.cc
> > (return false).
> > 
> > While some embedders may need the update behavior, my understanding is
Chrome
> is
> > not needing it. 
> > 
> > Down the road I will refactor this further with a proper observer/notifier
> > pattern. P3. 
> 
> Sorry I wasn't super clear on this.
> 
> chrome/ is actually used by chromium and all chromium-based browsers. What
makes
> Google Chrome special is the "is_chrome_branded" gn flag. But even though
> chromium-based browsers don't use this gn flag, they still use chrome/ and
hence
> ChromeContentRendererClient. So always returning false will break their CDM
> update logic.

Just taking another look at this.
Btw, chrome/ is _not_ used by other content embedders (at least, not by
chromecast) ;)

[servolk, 2017-04-03 20:14:12]

On 2017/04/03 19:33:26, halliwell wrote:
> On 2017/04/03 18:55:19, xhwang_slow wrote:
> >
>
https://codereview.chromium.org/2712983004/diff/220001/content/renderer/media...
> > File content/renderer/media/render_media_client.cc (right):
> > 
> >
>
https://codereview.chromium.org/2712983004/diff/220001/content/renderer/media...
> > content/renderer/media/render_media_client.cc:34: return
> > GetContentClient()->renderer()->IsKeySystemsUpdateNeeded();
> > On 2017/04/03 18:47:38, chcunningham wrote:
> > > On 2017/04/03 17:47:13, xhwang_slow wrote:
> > > > I am not seeing ChromeContentRendererClient implementing
> > > > IsKeySystemsUpdateNeeded() in this CL. What's the plan here?
> > > 
> > > Chrome inherits the behavior from
public/renderer/content_renderer_client.cc
> > > (return false).
> > > 
> > > While some embedders may need the update behavior, my understanding is
> Chrome
> > is
> > > not needing it. 
> > > 
> > > Down the road I will refactor this further with a proper observer/notifier
> > > pattern. P3. 
> > 
> > Sorry I wasn't super clear on this.
> > 
> > chrome/ is actually used by chromium and all chromium-based browsers. What
> makes
> > Google Chrome special is the "is_chrome_branded" gn flag. But even though
> > chromium-based browsers don't use this gn flag, they still use chrome/ and
> hence
> > ChromeContentRendererClient. So always returning false will break their CDM
> > update logic.
> 
> Just taking another look at this.
> Btw, chrome/ is _not_ used by other content embedders (at least, not by
> chromecast) ;)

This is still lgtm. Doing a/v codec checks through CastContentRendererClient
should be fine, as long as we keep the flexibility of making these decisions are
runtime (since we want to have a single cast_shell binary that runs on multiple
Cast platforms).
Thanks for doing this awesome refactoring, Chris!

[halliwell, 2017-04-03 20:32:47]

On 2017/04/03 20:14:12, servolk wrote:
> On 2017/04/03 19:33:26, halliwell wrote:
> > On 2017/04/03 18:55:19, xhwang_slow wrote:
> > >
> >
>
https://codereview.chromium.org/2712983004/diff/220001/content/renderer/media...
> > > File content/renderer/media/render_media_client.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2712983004/diff/220001/content/renderer/media...
> > > content/renderer/media/render_media_client.cc:34: return
> > > GetContentClient()->renderer()->IsKeySystemsUpdateNeeded();
> > > On 2017/04/03 18:47:38, chcunningham wrote:
> > > > On 2017/04/03 17:47:13, xhwang_slow wrote:
> > > > > I am not seeing ChromeContentRendererClient implementing
> > > > > IsKeySystemsUpdateNeeded() in this CL. What's the plan here?
> > > > 
> > > > Chrome inherits the behavior from
> public/renderer/content_renderer_client.cc
> > > > (return false).
> > > > 
> > > > While some embedders may need the update behavior, my understanding is
> > Chrome
> > > is
> > > > not needing it. 
> > > > 
> > > > Down the road I will refactor this further with a proper
observer/notifier
> > > > pattern. P3. 
> > > 
> > > Sorry I wasn't super clear on this.
> > > 
> > > chrome/ is actually used by chromium and all chromium-based browsers. What
> > makes
> > > Google Chrome special is the "is_chrome_branded" gn flag. But even though
> > > chromium-based browsers don't use this gn flag, they still use chrome/ and
> > hence
> > > ChromeContentRendererClient. So always returning false will break their
CDM
> > > update logic.
> > 
> > Just taking another look at this.
> > Btw, chrome/ is _not_ used by other content embedders (at least, not by
> > chromecast) ;)
> 
> This is still lgtm. Doing a/v codec checks through CastContentRendererClient
> should be fine, as long as we keep the flexibility of making these decisions
are
> runtime (since we want to have a single cast_shell binary that runs on
multiple
> Cast platforms).
> Thanks for doing this awesome refactoring, Chris!

lgtm, this is a great cleanup!

[chcunningham, 2017-04-04 23:34:03]

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

[commit-bot: I haz the power, 2017-04-04 23:35:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chcunningham, 2017-04-04 23:35:33]

The CQ bit was unchecked by chcunningham@chromium.org

[chcunningham, 2017-04-04 23:42:55]

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

[commit-bot: I haz the power, 2017-04-04 23:43:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chcunningham, 2017-04-04 23:44:30]

Thanks Luke/Sergey. Xiaohan, comments addressed.

https://codereview.chromium.org/2712983004/diff/220001/content/renderer/media...
File content/renderer/media/render_media_client.cc (right):

https://codereview.chromium.org/2712983004/diff/220001/content/renderer/media...
content/renderer/media/render_media_client.cc:34: return
GetContentClient()->renderer()->IsKeySystemsUpdateNeeded();
On 2017/04/03 18:55:18, xhwang_slow wrote:
> On 2017/04/03 18:47:38, chcunningham wrote:
> > On 2017/04/03 17:47:13, xhwang_slow wrote:
> > > I am not seeing ChromeContentRendererClient implementing
> > > IsKeySystemsUpdateNeeded() in this CL. What's the plan here?
> > 
> > Chrome inherits the behavior from public/renderer/content_renderer_client.cc
> > (return false).
> > 
> > While some embedders may need the update behavior, my understanding is
Chrome
> is
> > not needing it. 
> > 
> > Down the road I will refactor this further with a proper observer/notifier
> > pattern. P3. 
> 
> Sorry I wasn't super clear on this.
> 
> chrome/ is actually used by chromium and all chromium-based browsers. What
makes
> Google Chrome special is the "is_chrome_branded" gn flag. But even though
> chromium-based browsers don't use this gn flag, they still use chrome/ and
hence
> ChromeContentRendererClient. So always returning false will break their CDM
> update logic.

Fixed!

[commit-bot: I haz the power, 2017-04-04 23:58:32]

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

[commit-bot: I haz the power, 2017-04-04 23:58:34]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[chcunningham, 2017-04-05 00:26:08]

Xiaohan,

Bot failures are due to undefined kWidevineKeySystem on certain builds. I can
wrap everything in ifdefs, or alternatively just change the include from the
generated .h to be widevine_cdm_common.h. The second option requires updating
chrome/renderer/DEPS to add third_party/widevine. Which option should I take? I
note that key_systems.cc already includes widevine_cdm_common.h, so implicitly
chrome is already depending on widevine... not sure when to use the generated
widevine header.

[xhwang, 2017-04-05 00:39:39]

On 2017/04/05 00:26:08, chcunningham wrote:
> Xiaohan,
> 
> Bot failures are due to undefined kWidevineKeySystem on certain builds. I can
> wrap everything in ifdefs, or alternatively just change the include from the
> generated .h to be widevine_cdm_common.h. The second option requires updating
> chrome/renderer/DEPS to add third_party/widevine. Which option should I take?
I
> note that key_systems.cc already includes widevine_cdm_common.h, so implicitly
> chrome is already depending on widevine... not sure when to use the generated
> widevine header.

In principle, content layer should not know about widevine. Each content
embedders (e.g. chrome/) know what key systems they support (including
widevine). But the support of Widevine in chrome/ is optional and should always
be guarded by #ifdefs.

In your case, since you are already in chrome/, using the generated widevine
header sgtm.

[chcunningham, 2017-04-05 21:33:04]

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

[commit-bot: I haz the power, 2017-04-05 21:34:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-05 21:43:51]

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

[commit-bot: I haz the power, 2017-04-05 21:43:52]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[chcunningham, 2017-04-05 22:23:05]

> In your case, since you are already in chrome/, using the generated widevine
> header sgtm.

Done.

[chcunningham, 2017-04-05 22:24:54]

chcunningham@chromium.org changed reviewers:
+ jochen@chromium.org

[chcunningham, 2017-04-05 22:24:55]

+jochen@chromium.org for content

[chcunningham, 2017-04-05 23:33:57]

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

[commit-bot: I haz the power, 2017-04-05 23:34:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-06 01:57:36]

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

[commit-bot: I haz the power, 2017-04-06 01:57:37]

Dry run: This issue passed the CQ dry run.

[xhwang, 2017-04-06 04:52:11]

I <3 the final solution! LGTM

[jochen (gone - plz use gerrit), 2017-04-06 13:00:05]

can you please format the CL description to repeat the CL title in the first
line? the CQ will only use the CL description for the commit, not the CL title
:-/

https://codereview.chromium.org/2712983004/diff/320001/chrome/renderer/media/...
File chrome/renderer/media/chrome_key_systems_provider.h (right):

https://codereview.chromium.org/2712983004/diff/320001/chrome/renderer/media/...
chrome/renderer/media/chrome_key_systems_provider.h:16: namespace chrome {
it's not really common to use namespace chrome unless there's a naming clash
with e.g. content or so

https://codereview.chromium.org/2712983004/diff/320001/chrome/renderer/media/...
chrome/renderer/media/chrome_key_systems_provider.h:18: class
TestKeySystemsProviderDelegate {
Is that a testing delegate? In that case, I'd expect it to be in a different
file

https://codereview.chromium.org/2712983004/diff/320001/chrome/renderer/media/...
chrome/renderer/media/chrome_key_systems_provider.h:21: virtual void
AddTestKeySystems(
please add comments what this is supposed to do

https://codereview.chromium.org/2712983004/diff/320001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2712983004/diff/320001/content/renderer/rende...
content/renderer/render_frame_impl.cc:2804: new
RenderMediaLog(url::Origin(frame_->getSecurityOrigin()).GetURL());
what happens if the security origin is opaque?

[jochen (gone - plz use gerrit), 2017-04-06 13:01:12]

jochen@chromium.org changed reviewers:
+ mlamouri@chromium.org

[jochen (gone - plz use gerrit), 2017-04-06 13:01:13]

+mlamouri fyi

btw, have you considered splitting up the CL into the separate parts you call
out in the CL description?

[chcunningham, 2017-04-06 19:31:21]

Description was changed from

==========
This CL does a number of things to cleanup MediaClient's 
implementation and usage. This clears the way for an expanded role
in describing MediaCapabilities. 

Remove CastMediaClient. Merge methods into CastContentRenderClient.
  ContentRenderClient is now the one-stop-shop for embedder 
  customization. I initially  explored giving each embedder its own
  MediaClient, but this ended up splitting media customization
  between ContentRenderClient and MediaClient (some customization is
  too high level to move to MediaClient). The current route is also
  less code. 

Remove all Chrome specific logic from RenderMediaClient. 
  RenderMediaClient is now the plumbing for *all* content embedder
  customizations to be visible in media/. Chrome is just one of many
  embedders and much of the chrome specific code was stale anyway.

Remove KeySystemNameForUMA from MediaClient interface
  Decentralizing this logic was overly complicated. 
  Now centralized to key_systems.cc.

Other little cleanups
  Centralize all media/ Rappor usage in MediaLog.
  Move cast key_systems_cast into chromecast/renderer/media.
  Simplify key_systems_unittest.cc

BUG=695264
==========

to

==========
Simplify/Cleanup MediaClient

This CL does a number of things to cleanup MediaClient's 
implementation and usage. This clears the way for an expanded role
in describing MediaCapabilities. 

Remove CastMediaClient. Merge methods into CastContentRenderClient.
  ContentRenderClient is now the one-stop-shop for embedder 
  customization. I initially  explored giving each embedder its own
  MediaClient, but this ended up splitting media customization
  between ContentRenderClient and MediaClient (some customization is
  too high level to move to MediaClient). The current route is also
  less code. 

Remove all Chrome specific logic from RenderMediaClient. 
  RenderMediaClient is now the plumbing for *all* content embedder
  customizations to be visible in media/. Chrome is just one of many
  embedders and much of the chrome specific code was stale anyway.

Remove KeySystemNameForUMA from MediaClient interface
  Decentralizing this logic was overly complicated. 
  Now centralized to key_systems.cc.

Other little cleanups
  Centralize all media/ Rappor usage in MediaLog.
  Move cast key_systems_cast into chromecast/renderer/media.
  Simplify key_systems_unittest.cc

BUG=695264
==========

[chcunningham, 2017-04-06 20:48:34]

> can you please format the CL description to repeat the CL title in the first
> line?

Done


> btw, have you considered splitting up the CL into the separate parts you call
> out in the CL description?

I actually split out some big pieces in ealier patch sets (now deleted). I agree
it could be split further, but I also think its pretty manageable as-is. The
bulk of the change is already enthusiastically LGTM'd.

https://codereview.chromium.org/2712983004/diff/320001/chrome/renderer/media/...
File chrome/renderer/media/chrome_key_systems_provider.h (right):

https://codereview.chromium.org/2712983004/diff/320001/chrome/renderer/media/...
chrome/renderer/media/chrome_key_systems_provider.h:16: namespace chrome {
On 2017/04/06 13:00:05, jochen wrote:
> it's not really common to use namespace chrome unless there's a naming clash
> with e.g. content or so

I noticed that in a number of files - whats the history there? 

I've seen a quite a few now have the chrome namespace (perhaps due to conflicts)
- are you wanting me to remove the namespace here?

https://codereview.chromium.org/2712983004/diff/320001/chrome/renderer/media/...
chrome/renderer/media/chrome_key_systems_provider.h:18: class
TestKeySystemsProviderDelegate {
On 2017/04/06 13:00:05, jochen wrote:
> Is that a testing delegate? In that case, I'd expect it to be in a different
> file

I've replaced this interface with a simple callback. The callback is bound only
in the unit test file.

https://codereview.chromium.org/2712983004/diff/320001/chrome/renderer/media/...
chrome/renderer/media/chrome_key_systems_provider.h:21: virtual void
AddTestKeySystems(
On 2017/04/06 13:00:05, jochen wrote:
> please add comments what this is supposed to do

N/A given new changes. I do have some comments above the test_provider_ member
to help readers.

https://codereview.chromium.org/2712983004/diff/320001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2712983004/diff/320001/content/renderer/rende...
content/renderer/render_frame_impl.cc:2804: new
RenderMediaLog(url::Origin(frame_->getSecurityOrigin()).GetURL());
On 2017/04/06 13:00:05, jochen wrote:
> what happens if the security origin is opaque?

xhwang - do you know? I took a quick look at the rappor code and didn't see any
special handling. 

My code doesn't change this behavior, but I agree its worth finding out more.

[chcunningham, 2017-04-06 20:58:13]

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

[commit-bot: I haz the power, 2017-04-06 20:59:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xhwang, 2017-04-06 21:05:15]

https://codereview.chromium.org/2712983004/diff/320001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2712983004/diff/320001/content/renderer/rende...
content/renderer/render_frame_impl.cc:2804: new
RenderMediaLog(url::Origin(frame_->getSecurityOrigin()).GetURL());
On 2017/04/06 20:48:34, chcunningham wrote:
> On 2017/04/06 13:00:05, jochen wrote:
> > what happens if the security origin is opaque?
> 
> xhwang - do you know? I took a quick look at the rappor code and didn't see
any
> special handling. 
> 
> My code doesn't change this behavior, but I agree its worth finding out more. 

jochen: What exactly do you mean by "opaque origin"? Is that the same as "unique
origin"? I feel this should be handled by Rappor backend, instead of  being
handled everywhere when rappor is reported.

[commit-bot: I haz the power, 2017-04-06 23:51:12]

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

[commit-bot: I haz the power, 2017-04-06 23:51:14]

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

[jochen (gone - plz use gerrit), 2017-04-07 07:05:25]

lgtm assuming the answer is yes

https://codereview.chromium.org/2712983004/diff/320001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2712983004/diff/320001/content/renderer/rende...
content/renderer/render_frame_impl.cc:2804: new
RenderMediaLog(url::Origin(frame_->getSecurityOrigin()).GetURL());
On 2017/04/06 at 21:05:15, xhwang_slow wrote:
> On 2017/04/06 20:48:34, chcunningham wrote:
> > On 2017/04/06 13:00:05, jochen wrote:
> > > what happens if the security origin is opaque?
> > 
> > xhwang - do you know? I took a quick look at the rappor code and didn't see
any
> > special handling. 
> > 
> > My code doesn't change this behavior, but I agree its worth finding out
more. 
> 
> jochen: What exactly do you mean by "opaque origin"? Is that the same as
"unique origin"? I feel this should be handled by Rappor backend, instead of 
being handled everywhere when rappor is reported.

yeah, unique origins are called opaque in the spec. hurray for consistency.

so you're saying RenderMediaLog() can deal with an empty GURL() object as
parameter then?

[mlamouri (slow - plz ping), 2017-04-07 15:07:49]

lgtm

[chcunningham, 2017-04-07 18:47:06]

Thanks everyone!

> so you're saying RenderMediaLog() can deal with an empty GURL() object as
> parameter then?

Yes. RenderMediaLog's handling of GURL is purely to pass it through to
ContentRendererClient, which ultimately just feeds it to Rappor.

[chcunningham, 2017-04-07 20:52:21]

The CQ bit was checked by chcunningham@chromium.org

[chcunningham, 2017-04-07 20:52:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
servolk@chromium.org, halliwell@chromium.org, xhwang@chromium.org,
jochen@chromium.org, mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/2712983004/#ps360001
(title: "Fix test leak")

[commit-bot: I haz the power, 2017-04-07 20:53:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-07 22:41:45]

CQ is committing da patch.

Bot data: {"patchset_id": 360001, "attempt_start_ts": 1491598341909860,
"parent_rev": "1b22d05a5146584837929c70b04f87402156e649", "commit_rev":
"8aeee0b5c65d2ea16d2c52839cf80bcf99273555"}

[commit-bot: I haz the power, 2017-04-07 22:42:39]

Description was changed from

==========
Simplify/Cleanup MediaClient

This CL does a number of things to cleanup MediaClient's 
implementation and usage. This clears the way for an expanded role
in describing MediaCapabilities. 

Remove CastMediaClient. Merge methods into CastContentRenderClient.
  ContentRenderClient is now the one-stop-shop for embedder 
  customization. I initially  explored giving each embedder its own
  MediaClient, but this ended up splitting media customization
  between ContentRenderClient and MediaClient (some customization is
  too high level to move to MediaClient). The current route is also
  less code. 

Remove all Chrome specific logic from RenderMediaClient. 
  RenderMediaClient is now the plumbing for *all* content embedder
  customizations to be visible in media/. Chrome is just one of many
  embedders and much of the chrome specific code was stale anyway.

Remove KeySystemNameForUMA from MediaClient interface
  Decentralizing this logic was overly complicated. 
  Now centralized to key_systems.cc.

Other little cleanups
  Centralize all media/ Rappor usage in MediaLog.
  Move cast key_systems_cast into chromecast/renderer/media.
  Simplify key_systems_unittest.cc

BUG=695264
==========

to

==========
Simplify/Cleanup MediaClient

This CL does a number of things to cleanup MediaClient's 
implementation and usage. This clears the way for an expanded role
in describing MediaCapabilities. 

Remove CastMediaClient. Merge methods into CastContentRenderClient.
  ContentRenderClient is now the one-stop-shop for embedder 
  customization. I initially  explored giving each embedder its own
  MediaClient, but this ended up splitting media customization
  between ContentRenderClient and MediaClient (some customization is
  too high level to move to MediaClient). The current route is also
  less code. 

Remove all Chrome specific logic from RenderMediaClient. 
  RenderMediaClient is now the plumbing for *all* content embedder
  customizations to be visible in media/. Chrome is just one of many
  embedders and much of the chrome specific code was stale anyway.

Remove KeySystemNameForUMA from MediaClient interface
  Decentralizing this logic was overly complicated. 
  Now centralized to key_systems.cc.

Other little cleanups
  Centralize all media/ Rappor usage in MediaLog.
  Move cast key_systems_cast into chromecast/renderer/media.
  Simplify key_systems_unittest.cc

BUG=695264

Review-Url: https://codereview.chromium.org/2712983004
Cr-Commit-Position: refs/heads/master@{#463025}
Committed:
https://chromium.googlesource.com/chromium/src/+/8aeee0b5c65d2ea16d2c52839cf8...
==========

[commit-bot: I haz the power, 2017-04-07 22:42:42]

Committed patchset #11 (id:360001) as
https://chromium.googlesource.com/chromium/src/+/8aeee0b5c65d2ea16d2c52839cf8...