Add service registration for Mojo services defined under chrome/

Add service registration for Mojo services defined under chrome/
to chrome_web_contents_observer.
Add RegisterMojoServices virtual method to
extension_web_contents_observer.

BUG=

Committed: https://crrev.com/166e5b457da60fa11992b2e7efe3444f3875a465
Cr-Commit-Position: refs/heads/master@{#323836}

[Kevin M, 2015-03-27 19:22:20]

kmarshall@chromium.org changed reviewers:
+ mfoltz@chromium.org, sammc@chromium.org

[Kevin M, 2015-03-27 19:22:21]

Hi Sam,

FYI, I'm adding this change so that we can register a Mojo service under
chrome/... without creating a dependency cycle from extensions/ to chrome/ in
the process.

[mark a. foltz, 2015-03-27 20:24:11]

https://codereview.chromium.org/1035263002/diff/1/chrome/browser/extensions/c...
File chrome/browser/extensions/chrome_extension_web_contents_observer.cc
(right):

https://codereview.chromium.org/1035263002/diff/1/chrome/browser/extensions/c...
chrome/browser/extensions/chrome_extension_web_contents_observer.cc:96:
content::RenderFrameHost* render_frame_host,
What is the relationship between |render_frame_host| and the |web_contents| that
this observer is instantiated with?

https://codereview.chromium.org/1035263002/diff/1/chrome/browser/extensions/c...
File chrome/browser/extensions/chrome_extension_web_contents_observer.h (right):

https://codereview.chromium.org/1035263002/diff/1/chrome/browser/extensions/c...
chrome/browser/extensions/chrome_extension_web_contents_observer.h:37: void
RegisterMojoServices(content::RenderFrameHost* render_frame_host,
Usually observers don't expose methods to callers that side effect the thing
being observed...

Or maybe this should be OnRegisterMojoServices?

https://codereview.chromium.org/1035263002/diff/1/extensions/browser/extensio...
File extensions/browser/extension_web_contents_observer.cc (right):

https://codereview.chromium.org/1035263002/diff/1/extensions/browser/extensio...
extensions/browser/extension_web_contents_observer.cc:24: const Extension*
GetExtensionForRenderFrame(
Is there no other way to map the render_frame_host to the extension?  Should the
creator of the observer be able to get a ptr to the Extension being observed?

[Kevin M, 2015-03-27 22:42:37]

https://codereview.chromium.org/1035263002/diff/1/chrome/browser/extensions/c...
File chrome/browser/extensions/chrome_extension_web_contents_observer.cc
(right):

https://codereview.chromium.org/1035263002/diff/1/chrome/browser/extensions/c...
chrome/browser/extensions/chrome_extension_web_contents_observer.cc:96:
content::RenderFrameHost* render_frame_host,
On 2015/03/27 20:24:11, mark a. foltz wrote:
> What is the relationship between |render_frame_host| and the |web_contents|
that
> this observer is instantiated with?

render_frame_host is created as a result of |web_contents| constructing a
RenderView. The RenderFrameCreated observer method is invoked after
render_frame_host is created.

https://codereview.chromium.org/1035263002/diff/1/chrome/browser/extensions/c...
File chrome/browser/extensions/chrome_extension_web_contents_observer.h (right):

https://codereview.chromium.org/1035263002/diff/1/chrome/browser/extensions/c...
chrome/browser/extensions/chrome_extension_web_contents_observer.h:37: void
RegisterMojoServices(content::RenderFrameHost* render_frame_host,
On 2015/03/27 20:24:11, mark a. foltz wrote:
> Usually observers don't expose methods to callers that side effect the thing
> being observed...
> 
> Or maybe this should be OnRegisterMojoServices?

Moved this out of the WebContentsObserver overrides block, since this is
actually just overriding ExtensionWebContentsObserver's method.

This does indeed change the object being observed, but it's no different than
preexisting functionality. sammc@ might be able to comment on that decision.

I believe it should stay as RegisterMojoServices because the method itself is
what's doing the Mojo registration; it's not fired after the fact.

https://codereview.chromium.org/1035263002/diff/1/extensions/browser/extensio...
File extensions/browser/extension_web_contents_observer.cc (right):

https://codereview.chromium.org/1035263002/diff/1/extensions/browser/extensio...
extensions/browser/extension_web_contents_observer.cc:24: const Extension*
GetExtensionForRenderFrame(
On 2015/03/27 20:24:11, mark a. foltz wrote:
> Is there no other way to map the render_frame_host to the extension?  Should
the
> creator of the observer be able to get a ptr to the Extension being observed?

I don't think so... the creator of the observer doesn't have the ability to push
additional parameters through to the constructor (the constructor is triggered
indirectly, through a templatized factory method, which only takes a single
WebContents parameter).

The creator could potentially pull the observer back out by accessing it via its
user data key, and then set its extension, but adding an additional dependency
on that userdata key could clutter the code structure.

FWIW, the contents of this method was moved verbatim from
service_registration.cc:
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...

[Kevin M, 2015-03-31 19:19:45]

Friendly ping.

[Sam McNally, 2015-03-31 23:38:51]

https://codereview.chromium.org/1035263002/diff/1/chrome/browser/extensions/c...
File chrome/browser/extensions/chrome_extension_web_contents_observer.h (right):

https://codereview.chromium.org/1035263002/diff/1/chrome/browser/extensions/c...
chrome/browser/extensions/chrome_extension_web_contents_observer.h:37: void
RegisterMojoServices(content::RenderFrameHost* render_frame_host,
On 2015/03/27 22:42:37, Kevin M wrote:
> On 2015/03/27 20:24:11, mark a. foltz wrote:
> > Usually observers don't expose methods to callers that side effect the thing
> > being observed...
> > 
> > Or maybe this should be OnRegisterMojoServices?
> 
> Moved this out of the WebContentsObserver overrides block, since this is
> actually just overriding ExtensionWebContentsObserver's method.
> 
> This does indeed change the object being observed, but it's no different than
> preexisting functionality. sammc@ might be able to comment on that decision.
> 
> I believe it should stay as RegisterMojoServices because the method itself is
> what's doing the Mojo registration; it's not fired after the fact.

This might be better off as a method in ExtensionsBrowserClient, to be called
from service_registration.cc.

https://codereview.chromium.org/1035263002/diff/1/extensions/browser/extensio...
File extensions/browser/extension_web_contents_observer.cc (right):

https://codereview.chromium.org/1035263002/diff/1/extensions/browser/extensio...
extensions/browser/extension_web_contents_observer.cc:24: const Extension*
GetExtensionForRenderFrame(
On 2015/03/27 22:42:37, Kevin M wrote:
> On 2015/03/27 20:24:11, mark a. foltz wrote:
> > Is there no other way to map the render_frame_host to the extension?  Should
> the
> > creator of the observer be able to get a ptr to the Extension being
observed?
> 
> I don't think so... the creator of the observer doesn't have the ability to
push
> additional parameters through to the constructor (the constructor is triggered
> indirectly, through a templatized factory method, which only takes a single
> WebContents parameter).
> 
> The creator could potentially pull the observer back out by accessing it via
its
> user data key, and then set its extension, but adding an additional dependency
> on that userdata key could clutter the code structure.
> 
> FWIW, the contents of this method was moved verbatim from
> service_registration.cc:
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...

I don't think there's a way to do this as a simple lookup. Unless this has
changed recently, the extensions system doesn't really work well with
RenderFrames.

[mark a. foltz, 2015-04-01 00:10:56]

Looks good, once the final code organization is decided I'll look again.

https://codereview.chromium.org/1035263002/diff/20001/extensions/browser/exte...
File extensions/browser/extension_web_contents_observer.cc (right):

https://codereview.chromium.org/1035263002/diff/20001/extensions/browser/exte...
extensions/browser/extension_web_contents_observer.cc:121:
GetExtensionForRenderFrame(render_frame_host));
Why not pass |extension| here?

[Kevin M, 2015-04-01 23:23:51]

https://codereview.chromium.org/1035263002/diff/1/chrome/browser/extensions/c...
File chrome/browser/extensions/chrome_extension_web_contents_observer.h (right):

https://codereview.chromium.org/1035263002/diff/1/chrome/browser/extensions/c...
chrome/browser/extensions/chrome_extension_web_contents_observer.h:37: void
RegisterMojoServices(content::RenderFrameHost* render_frame_host,
On 2015/03/31 23:38:51, Sam McNally wrote:
> On 2015/03/27 22:42:37, Kevin M wrote:
> > On 2015/03/27 20:24:11, mark a. foltz wrote:
> > > Usually observers don't expose methods to callers that side effect the
thing
> > > being observed...
> > > 
> > > Or maybe this should be OnRegisterMojoServices?
> > 
> > Moved this out of the WebContentsObserver overrides block, since this is
> > actually just overriding ExtensionWebContentsObserver's method.
> > 
> > This does indeed change the object being observed, but it's no different
than
> > preexisting functionality. sammc@ might be able to comment on that decision.
> > 
> > I believe it should stay as RegisterMojoServices because the method itself
is
> > what's doing the Mojo registration; it's not fired after the fact.
> 
> This might be better off as a method in ExtensionsBrowserClient, to be called
> from service_registration.cc.

Done, thanks for the suggestion.

https://codereview.chromium.org/1035263002/diff/20001/extensions/browser/exte...
File extensions/browser/extension_web_contents_observer.cc (right):

https://codereview.chromium.org/1035263002/diff/20001/extensions/browser/exte...
extensions/browser/extension_web_contents_observer.cc:121:
GetExtensionForRenderFrame(render_frame_host));
On 2015/04/01 00:10:56, mark a. foltz wrote:
> Why not pass |extension| here?

Done.

[Sam McNally, 2015-04-02 04:20:26]

lgtm

[mark a. foltz, 2015-04-02 20:02:16]

lgtm

[Kevin M, 2015-04-03 17:38:09]

The CQ bit was checked by kmarshall@chromium.org

[Kevin M, 2015-04-03 17:38:09]

The patchset sent to the CQ was uploaded after l-g-t-m from sammc@chromium.org,
mfoltz@chromium.org
Link to the patchset: https://codereview.chromium.org/1035263002/#ps100001
(title: "Sync with master.")

[commit-bot: I haz the power, 2015-04-03 17:38:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1035263002/100001

[commit-bot: I haz the power, 2015-04-03 17:49:59]

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

[commit-bot: I haz the power, 2015-04-03 17:50:00]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Kevin Marshall, 2015-04-03 17:55:01]

marshallk@google.com changed reviewers:
+ marshallk@google.com, rockot@chromium.org,

[Kevin Marshall, 2015-04-03 17:55:02]

+rockot

[Ken Rockot(use gerrit already), 2015-04-03 22:00:35]

rockot@chromium.org changed reviewers:
+ rockot@chromium.org

[Ken Rockot(use gerrit already), 2015-04-03 22:00:36]

lgtm

[Kevin M, 2015-04-03 22:23:02]

The CQ bit was checked by kmarshall@chromium.org

[commit-bot: I haz the power, 2015-04-03 22:24:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1035263002/100001

[commit-bot: I haz the power, 2015-04-03 22:29:55]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-04-03 22:32:34]

Patchset 6 (id:??) landed as
https://crrev.com/166e5b457da60fa11992b2e7efe3444f3875a465
Cr-Commit-Position: refs/heads/master@{#323836}