Move media::mojom::RemoterFactory to content, add ContentBrowserClient API.

Move media::mojom::RemoterFactory to content, add ContentBrowserClient API.

This reverts commit face478e9879692bae410d450acceaddf6106532 ("Disable
spammy logging in InterfaceRegistry affecting too many people."), and
moves the implementation of media::mojom::RemoterFactory to content. From
there, embedders (such as Chrome) may optionally override a new
ContentBrowserClient::CreateMediaRemoter() method to provide the browser-
side media remoting implementation.

BUG=664595
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/54dca6cd9ef8e64e8600ed9cb2994e3e8352e514
Cr-Commit-Position: refs/heads/master@{#432839}

[miu, 2016-11-16 02:53:11]

Description was changed from

==========
Add default media::mojom::RemoterFactory, to be overridden by embedders.

This reverts commit face478e9879692bae410d450acceaddf6106532 ("Disable
spammy logging in InterfaceRegistry affecting too many people."), adds
a "no-op" default implementation of media::mojom::RemoterFactory to
content::RenderFrameHostImpl, and adjusts the Chrome content browser
manifest overlay JSON to reflect that Chrome is the only embedder that
overrides the default and provides a full implementation.

BUG=664595
==========

to

==========
Add default media::mojom::RemoterFactory, to be overridden by embedders.

This reverts commit face478e9879692bae410d450acceaddf6106532 ("Disable
spammy logging in InterfaceRegistry affecting too many people."), adds
a "no-op" default implementation of media::mojom::RemoterFactory to
content::RenderFrameHostImpl, and adjusts the Chrome content browser
manifest overlay JSON to reflect that Chrome is the only embedder that
overrides the default and provides a full implementation.

BUG=664595
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[miu, 2016-11-16 02:54:52]

miu@chromium.org changed reviewers:
+ ben@chromium.org, sky@chromium.org

[miu, 2016-11-16 02:54:52]

ben: PTAL.

sky: OWNERS stamp for render_frame_host_impl.cc.

[miu, 2016-11-16 02:55:23]

[+cc rockot@]

[Ben Goodger (Google), 2016-11-16 03:58:50]

https://codereview.chromium.org/2498203004/diff/20001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_manifest_overlay.json (right):

https://codereview.chromium.org/2498203004/diff/20001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_manifest_overlay.json:55:
"media::mojom::RemoterFactory",
If you're exposing this interface in //content this should be in
//content/public/app/mojo/content_browser_manifest.json not here, otherwise it
won't be exposed to other content embedders.

You would have discovered this if you had a browsertest in content.

[sky, 2016-11-16 03:58:54]

sky@chromium.org changed reviewers:
- sky@chromium.org

[sky, 2016-11-16 03:58:54]

I'm not an owner of content. I'm removing myself. If you need me for something
else, add me back.

[miu, 2016-11-16 18:49:21]

miu@chromium.org changed reviewers:
+ nick@chromium.org

[miu, 2016-11-16 18:49:21]

nick: PTAL for OWNERS on render_frame_host_impl.cc.

[Ben Goodger (Google), 2016-11-16 18:56:14]

https://codereview.chromium.org/2498203004/diff/20001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_manifest_overlay.json (right):

https://codereview.chromium.org/2498203004/diff/20001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_manifest_overlay.json:55:
"media::mojom::RemoterFactory",
Scratch this. Looking at content/public/app/mojo/content_browser_manifest.json,
it looks like this interface is already provided there so this is unnecessary.

https://codereview.chromium.org/2498203004/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2498203004/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:2257: class
NullRemoterFactory final : public media::mojom::RemoterFactory {
Since the renderer requires this always, rather than replacing having the
embedder replace the factory (which seems a little scary, if the call out to
RenderRenderFrameMojoInterfaces ever moves around), why not just add a
special-purpose method to the browser client to bind this request & have the
canonical RemoteFactory factory live here?

[miu, 2016-11-16 22:30:05]

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

[miu, 2016-11-16 22:30:06]

ben: PS3 and PS4 address your comments. PTAL.

nick: Looks like I need a slightly broader content OWNERS review now. :)

xhwang: Need OWNERS stamp for adding media/media_features.h to
chrome/browser/DEPS.

https://codereview.chromium.org/2498203004/diff/20001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_manifest_overlay.json (right):

https://codereview.chromium.org/2498203004/diff/20001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_manifest_overlay.json:55:
"media::mojom::RemoterFactory",
On 2016/11/16 03:58:50, Ben Goodger (Google) wrote:
> If you're exposing this interface in //content this should be in
> //content/public/app/mojo/content_browser_manifest.json not here, otherwise it
> won't be exposed to other content embedders.

Okay. There's already one in the content manifest, so we're good there. (I
wasn't sure if I needed one for the Chrome embedder as well; but I see from your
follow-up comment this should be removed.)

> You would have discovered this if you had a browsertest in content.

You could run pretty much *any* test in content_browsertests or layout_tests or
content_shell to expose this issue (before I disabled the logging yesterday).
FWICT, the log spamming started with your change:
https://codereview.chromium.org/2457493004 It only recently got back to me when
my manager asked me why I was spamming his test logs...

https://codereview.chromium.org/2498203004/diff/20001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_manifest_overlay.json:55:
"media::mojom::RemoterFactory",
On 2016/11/16 18:56:13, Ben Goodger (Google) wrote:
> Scratch this. Looking at
content/public/app/mojo/content_browser_manifest.json,
> it looks like this interface is already provided there so this is unnecessary.

Done.

https://codereview.chromium.org/2498203004/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2498203004/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:2257: class
NullRemoterFactory final : public media::mojom::RemoterFactory {
On 2016/11/16 18:56:13, Ben Goodger (Google) wrote:
> Since the renderer requires this always, rather than replacing having the
> embedder replace the factory (which seems a little scary, if the call out to
> RenderRenderFrameMojoInterfaces ever moves around), why not just add a
> special-purpose method to the browser client to bind this request & have the
> canonical RemoteFactory factory live here?

Done. Prior to this, I had assumed it was normal/SOP for the service-side
binding to be optional--based on whether support was present in the
embedder--and so it would be fine if some embedders did not register with the
InterfaceRegistry. AFAIK, this was never made clear in any Mojo-in-Chromium
documentation.

[xhwang, 2016-11-16 22:35:45]

DEPS on media/media_features.h lgtm

[ncarter (slow), 2016-11-16 22:40:59]

content/ lgtm. I think this is a good solution.

https://codereview.chromium.org/2498203004/diff/60001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2498203004/diff/60001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:3248: #if
BUILDFLAG(ENABLE_MEDIA_REMOTING) && defined(ENABLE_MEDIA_ROUTER)
Consider having this function's declaration be parameterized only on:

#if BUILDFLAG(ENABLE_MEDIA_REMOTING)

and only use defined(ENABLE_MEDIA_ROUTER) in the .cc file, in the body of the
function.

I think that'll make it a little easier to understand what happens in the
!ENABLE_MEDIA_ROUTER case -- it would also give you an #else spot where you
could add a comment explaining what happens when you drop it on the floor.

[miu, 2016-11-16 22:54:36]

Description was changed from

==========
Add default media::mojom::RemoterFactory, to be overridden by embedders.

This reverts commit face478e9879692bae410d450acceaddf6106532 ("Disable
spammy logging in InterfaceRegistry affecting too many people."), adds
a "no-op" default implementation of media::mojom::RemoterFactory to
content::RenderFrameHostImpl, and adjusts the Chrome content browser
manifest overlay JSON to reflect that Chrome is the only embedder that
overrides the default and provides a full implementation.

BUG=664595
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Move media::mojom::RemoterFactory to content, add ContentBrowserClient API.

This reverts commit face478e9879692bae410d450acceaddf6106532 ("Disable
spammy logging in InterfaceRegistry affecting too many people."), and
moves the implementation of media::mojom::RemoterFactory to content. From
there, embedders (such as Chrome) may optionally override a new
ContentBrowserClient::CreateMediaRemoter() method to provide the browser-
side media remoting implementation.

BUG=664595
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[miu, 2016-11-16 22:55:03]

ben: LGTY now?

(Updated change description to reflect new approach.)

https://codereview.chromium.org/2498203004/diff/60001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2498203004/diff/60001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:3248: #if
BUILDFLAG(ENABLE_MEDIA_REMOTING) && defined(ENABLE_MEDIA_ROUTER)
On 2016/11/16 22:40:59, ncarter wrote:
> Consider having this function's declaration be parameterized only on:
> 
> #if BUILDFLAG(ENABLE_MEDIA_REMOTING)
> 
> and only use defined(ENABLE_MEDIA_ROUTER) in the .cc file, in the body of the
> function.
> 
> I think that'll make it a little easier to understand what happens in the
> !ENABLE_MEDIA_ROUTER case -- it would also give you an #else spot where you
> could add a comment explaining what happens when you drop it on the floor.

Done.

[miu, 2016-11-16 22:55:25]

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

[commit-bot: I haz the power, 2016-11-16 22:56:41]

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, 2016-11-16 23:05:05]

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

[commit-bot: I haz the power, 2016-11-16 23:05:06]

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...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Ben Goodger (Google), 2016-11-16 23:46:59]

lgtm

[miu, 2016-11-17 09:19:17]

The CQ bit was checked by miu@chromium.org

[miu, 2016-11-17 09:19:18]

The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org,
xhwang@chromium.org, ben@chromium.org
Link to the patchset: https://codereview.chromium.org/2498203004/#ps120001
(title: "REBASE")

[commit-bot: I haz the power, 2016-11-17 09:19:48]

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, 2016-11-17 11:00:08]

Description was changed from

==========
Move media::mojom::RemoterFactory to content, add ContentBrowserClient API.

This reverts commit face478e9879692bae410d450acceaddf6106532 ("Disable
spammy logging in InterfaceRegistry affecting too many people."), and
moves the implementation of media::mojom::RemoterFactory to content. From
there, embedders (such as Chrome) may optionally override a new
ContentBrowserClient::CreateMediaRemoter() method to provide the browser-
side media remoting implementation.

BUG=664595
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Move media::mojom::RemoterFactory to content, add ContentBrowserClient API.

This reverts commit face478e9879692bae410d450acceaddf6106532 ("Disable
spammy logging in InterfaceRegistry affecting too many people."), and
moves the implementation of media::mojom::RemoterFactory to content. From
there, embedders (such as Chrome) may optionally override a new
ContentBrowserClient::CreateMediaRemoter() method to provide the browser-
side media remoting implementation.

BUG=664595
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-11-17 11:00:10]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-11-17 11:02:35]

Description was changed from

==========
Move media::mojom::RemoterFactory to content, add ContentBrowserClient API.

This reverts commit face478e9879692bae410d450acceaddf6106532 ("Disable
spammy logging in InterfaceRegistry affecting too many people."), and
moves the implementation of media::mojom::RemoterFactory to content. From
there, embedders (such as Chrome) may optionally override a new
ContentBrowserClient::CreateMediaRemoter() method to provide the browser-
side media remoting implementation.

BUG=664595
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Move media::mojom::RemoterFactory to content, add ContentBrowserClient API.

This reverts commit face478e9879692bae410d450acceaddf6106532 ("Disable
spammy logging in InterfaceRegistry affecting too many people."), and
moves the implementation of media::mojom::RemoterFactory to content. From
there, embedders (such as Chrome) may optionally override a new
ContentBrowserClient::CreateMediaRemoter() method to provide the browser-
side media remoting implementation.

BUG=664595
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/54dca6cd9ef8e64e8600ed9cb2994e3e8352e514
Cr-Commit-Position: refs/heads/master@{#432839}
==========

[commit-bot: I haz the power, 2016-11-17 11:02:37]

Patchset 7 (id:??) landed as
https://crrev.com/54dca6cd9ef8e64e8600ed9cb2994e3e8352e514
Cr-Commit-Position: refs/heads/master@{#432839}