Simplify/Cleanup MediaClient

content/public/renderer/content_media_client.h

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CONTENT_PUBLIC_RENDERER_CONTENT_MEDIA_CLIENT_
+#define CONTENT_PUBLIC_RENDERER_CONTENT_MEDIA_CLIENT_
+
+#include "base/threading/thread_checker.h"
+#include "content/public/renderer/content_renderer_client.h"
+#include "media/base/media_client.h"
+
+namespace content {
+
+// Provides a default MediaClient for content embedders simply want to defer
+// supported Key Systems and Rapport recording to their ContentRenderClient.

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

s/Rapport/Rappor

+//
+// IsSupportedVideoConfig() will assess support for the Media Platform in its
+// default state. Embedders who customize the Media Platform (e.g. Chromecast)
+// should override this method to reflect their changes.
+class CONTENT_EXPORT ContentMediaClient : public media::MediaClient {

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

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.

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

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:
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.
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. 
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(...).
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 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?

[xhwang, 2017/03/22 21:54:52]

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...
I see. That makes sense. So content/ uses RendererImpl, but content embedders
might use other Renderer implementation.
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.
SGTM with your correction!

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

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?

+ public:
+  explicit ContentMediaClient(
+      content::ContentRendererClient* content_renderer_client);
+  ~ContentMediaClient() override;
+
+  // MediaClient implementation.
+  void AddSupportedKeySystems(
+      std::vector<std::unique_ptr<media::KeySystemProperties>>*
+          key_systems_properties) override;
+  void RecordRapporURL(const std::string& metric, const GURL& url) override;
+  bool IsSupportedVideoConfig(const media::VideoConfig& config) override;
+
+ protected:
+  friend class ContentMediaClientTest;
+
+  // Makes sure all methods are called from the same thread.
+  base::ThreadChecker thread_checker_;
+
+  content::ContentRendererClient* content_renderer_client_;
+
+  DISALLOW_COPY_AND_ASSIGN(ContentMediaClient);
+};
+
+}  // namespace content
+
+#endif  // CONTENT_PUBLIC_RENDERER_CONTENT_MEDIA_CLIENT_