Pull chrome stuff out of (and move) extensions::Dispatcher.

Pull chrome stuff out of (and move) extensions::Dispatcher.

This moves Dispatcher into //extensions along with
several of its remaining dependencies.

This also introduces a DispatcherDelegate interface along with implementations for chrome and app_shell.

BUG=359836
TBR=sky for CCRC and ChromeRenderViewTest

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

[Ken Rockot(use gerrit already), 2014-04-29 01:52:39]

Please take a look. Every time I start working on some variant of this CL it
explodes into something ridiculous. I will understand if you refuse to review a
patch of this size.

[not at google - send to devlin, 2014-04-29 16:53:55]

https://codereview.chromium.org/260083006/diff/1/apps/shell/renderer/shell_ex...
File apps/shell/renderer/shell_extensions_renderer_client.h (right):

https://codereview.chromium.org/260083006/diff/1/apps/shell/renderer/shell_ex...
apps/shell/renderer/shell_extensions_renderer_client.h:29:
v8::Handle<v8::Context> v8_context,
use const& for handles to avoid copying them.

there are undoubtedly many, many places where this is wrong (and I probably
wrote several of them).

https://codereview.chromium.org/260083006/diff/1/chrome/renderer/resources/ex...
File chrome/renderer/resources/extensions/app_custom_bindings.js (right):

https://codereview.chromium.org/260083006/diff/1/chrome/renderer/resources/ex...
chrome/renderer/resources/extensions/app_custom_bindings.js:75: //
chrome/renderer/extensions/dispatcher.cc.
comment needs updating

https://codereview.chromium.org/260083006/diff/1/chrome/renderer/resources/ex...
chrome/renderer/resources/extensions/app_custom_bindings.js:76: exports.binding
= app;
nice!

https://codereview.chromium.org/260083006/diff/1/chrome/renderer/resources/ex...
File chrome/renderer/resources/extensions/webstore_custom_bindings.js (right):

https://codereview.chromium.org/260083006/diff/1/chrome/renderer/resources/ex...
chrome/renderer/resources/extensions/webstore_custom_bindings.js:89:
exports.binding = chromeWebstore;
ditto

https://codereview.chromium.org/260083006/diff/1/extensions/renderer/api_acti...
File extensions/renderer/api_activity_logger.h (right):

https://codereview.chromium.org/260083006/diff/1/extensions/renderer/api_acti...
extensions/renderer/api_activity_logger.h:13: 
this file should be a move

https://codereview.chromium.org/260083006/diff/1/extensions/renderer/builtin_...
File extensions/renderer/builtin_native_handlers.h (right):

https://codereview.chromium.org/260083006/diff/1/extensions/renderer/builtin_...
extensions/renderer/builtin_native_handlers.h:15: void
RegisterBuiltinNativeHandlers(Dispatcher* dispatcher,
I think that "builtin" is the wrong term here. "miscellaneous" would be better
since really there's no reason why these handlers were in dispatcher.cc other
than people being lazy and not wanting to create new files for different things.

https://codereview.chromium.org/260083006/diff/1/extensions/renderer/dispatch...
File extensions/renderer/dispatcher.cc (left):

https://codereview.chromium.org/260083006/diff/1/extensions/renderer/dispatch...
extensions/renderer/dispatcher.cc:478: PopulateLazyBindingsMap();
yay!

https://codereview.chromium.org/260083006/diff/1/extensions/renderer/dispatch...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/260083006/diff/1/extensions/renderer/dispatch...
extensions/renderer/dispatcher.cc:186: user_script_slave_.reset(new
UserScriptSlave(&extensions_));
why do you need to keep this in appshell?

https://codereview.chromium.org/260083006/diff/1/extensions/renderer/dispatch...
extensions/renderer/dispatcher.cc:187: request_sender_.reset(new
RequestSender(this));
i guess you explicitly want to support message passing in appshell?

https://codereview.chromium.org/260083006/diff/1/extensions/renderer/dispatch...
File extensions/renderer/dispatcher.h (right):

https://codereview.chromium.org/260083006/diff/1/extensions/renderer/dispatch...
extensions/renderer/dispatcher.h:61: // renderer extension related state.
wow this comment is out of date

https://codereview.chromium.org/260083006/diff/1/extensions/renderer/extensio...
File extensions/renderer/extensions_renderer_client.h (right):

https://codereview.chromium.org/260083006/diff/1/extensions/renderer/extensio...
extensions/renderer/extensions_renderer_client.h:41: // Initializes origin
permissions for a newly created extension context.
this model seems reasonable but a bit unnatural from an OO perspective. having
Dispatcher call out to a global delegate object is hard to reason about. I can
mostly see how all of this fits together but I need to go back and forth between
files.

better would be to make the Delegate relationship explicit. define a
Dispatcher::Delegate with these methods and pass into the Dispatcher on
construction. that shouldn't be too hard.

better still, though a much larger change of course, would be to split up the
role of Dispatcher into module loading and IPC (and then whatever else it needs,
but it's mostly those) then properly layer/compose them.
- IPC handling: split up the roles properly, not just appshell vs chrome but
make the messaging stuff handle its own IPC things.
- Module loading: define a layer on top of ModuleSystem to register all of that
stuff.

I'd like to see the Delegate thing implemented in this change, if possible.
worry about nicer factoring later but I'd really like to see it evolve nicely...
while I can see that this change furthers the goal of appshell, it's a
transformation that doesn't benefit code health IMO*, some positives, but also
some negatives with the seemingly-arbitrary delegate system.


[*] apart from those cleanups you've done along the way

[Ken Rockot(use gerrit already), 2014-04-29 17:06:38]

Nice - no new patch set yet, just responding to some comments.

https://codereview.chromium.org/260083006/diff/1/extensions/renderer/dispatch...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/260083006/diff/1/extensions/renderer/dispatch...
extensions/renderer/dispatcher.cc:186: user_script_slave_.reset(new
UserScriptSlave(&extensions_));
On 2014/04/29 16:53:55, kalman wrote:
> why do you need to keep this in appshell?

We don't, but it's also not really a concept exclusive to chrome. It seemed like
less work (for app_shell) to actually just move it over so I saw it as win-win.

https://codereview.chromium.org/260083006/diff/1/extensions/renderer/dispatch...
extensions/renderer/dispatcher.cc:187: request_sender_.reset(new
RequestSender(this));
On 2014/04/29 16:53:55, kalman wrote:
> i guess you explicitly want to support message passing in appshell?

Indeed. At least I think it will be useful to other consumers of the extensions
system, if not immediately to app_shell.

https://codereview.chromium.org/260083006/diff/1/extensions/renderer/extensio...
File extensions/renderer/extensions_renderer_client.h (right):

https://codereview.chromium.org/260083006/diff/1/extensions/renderer/extensio...
extensions/renderer/extensions_renderer_client.h:41: // Initializes origin
permissions for a newly created extension context.
On 2014/04/29 16:53:55, kalman wrote:
> this model seems reasonable but a bit unnatural from an OO perspective. having
> Dispatcher call out to a global delegate object is hard to reason about. I can
> mostly see how all of this fits together but I need to go back and forth
between
> files.
> 
> better would be to make the Delegate relationship explicit. define a
> Dispatcher::Delegate with these methods and pass into the Dispatcher on
> construction. that shouldn't be too hard.
> 
> better still, though a much larger change of course, would be to split up the
> role of Dispatcher into module loading and IPC (and then whatever else it
needs,
> but it's mostly those) then properly layer/compose them.
> - IPC handling: split up the roles properly, not just appshell vs chrome but
> make the messaging stuff handle its own IPC things.
> - Module loading: define a layer on top of ModuleSystem to register all of
that
> stuff.
> 
> I'd like to see the Delegate thing implemented in this change, if possible.
> worry about nicer factoring later but I'd really like to see it evolve
nicely...
> while I can see that this change furthers the goal of appshell, it's a
> transformation that doesn't benefit code health IMO*, some positives, but also
> some negatives with the seemingly-arbitrary delegate system.
> 
> 
> [*] apart from those cleanups you've done along the way

Yes it's pretty unnatural. One of the many CLs along the way introduced a
Dispatcher::Delegate. I don't remember why I scrapped it, but thinking about it
now there's no reason not to do it here. It's straightforward and relatively
clean.

The larger refactor I also agree with and have put some thought into that. I
don't think it's appropriate for me to take on right now, as much as I'd like
to. Maybe in a few weeks though.

[Ken Rockot(use gerrit already), 2014-04-29 22:38:03]

Delegate added; I also decided to just move those "builtin" native handlers to
their own source modules.

https://codereview.chromium.org/260083006/diff/1/apps/shell/renderer/shell_ex...
File apps/shell/renderer/shell_extensions_renderer_client.h (right):

https://codereview.chromium.org/260083006/diff/1/apps/shell/renderer/shell_ex...
apps/shell/renderer/shell_extensions_renderer_client.h:29:
v8::Handle<v8::Context> v8_context,
On 2014/04/29 16:53:55, kalman wrote:
> use const& for handles to avoid copying them.
> 
> there are undoubtedly many, many places where this is wrong (and I probably
> wrote several of them).

Done.

[not at google - send to devlin, 2014-04-29 23:29:05]

lgtm mostly small comments left.

https://codereview.chromium.org/260083006/diff/20001/apps/shell/renderer/shel...
File apps/shell/renderer/shell_extensions_renderer_client.cc (right):

https://codereview.chromium.org/260083006/diff/20001/apps/shell/renderer/shel...
apps/shell/renderer/shell_extensions_renderer_client.cc:6: #include
"extensions/renderer/script_context.h"
not used

https://codereview.chromium.org/260083006/diff/20001/chrome/renderer/chrome_c...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/260083006/diff/20001/chrome/renderer/chrome_c...
chrome/renderer/chrome_content_renderer_client.cc:1331: return
g_current_client->extension_dispatcher_delegate_
this is pretty weird to look at. hopefully when the IPC handling is cleaned up
this can be its own class rather than lumping in with the delegate.

https://codereview.chromium.org/260083006/diff/20001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/app_custom_bindings.js (right):

https://codereview.chromium.org/260083006/diff/20001/chrome/renderer/resource...
chrome/renderer/resources/extensions/app_custom_bindings.js:75: //
chrome/renderer/extensions/dispatcher.cc.
you can delete this comment

https://codereview.chromium.org/260083006/diff/20001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/webstore_custom_bindings.js (right):

https://codereview.chromium.org/260083006/diff/20001/chrome/renderer/resource...
chrome/renderer/resources/extensions/webstore_custom_bindings.js:88: //
chrome/renderer/extensions/dispatcher.cc.
you can delete this comment.

https://codereview.chromium.org/260083006/diff/20001/chrome/test/base/chrome_...
File chrome/test/base/chrome_render_view_test.cc (right):

https://codereview.chromium.org/260083006/diff/20001/chrome/test/base/chrome_...
chrome/test/base/chrome_render_view_test.cc:59: new extensions::Dispatcher(new
ChromeExtensionsDispatcherDelegate());
leak?

https://codereview.chromium.org/260083006/diff/20001/extensions/renderer/api_...
File extensions/renderer/api_activity_logger.h (right):

https://codereview.chromium.org/260083006/diff/20001/extensions/renderer/api_...
extensions/renderer/api_activity_logger.h:13: 
still not detected as a move?

https://codereview.chromium.org/260083006/diff/20001/extensions/renderer/disp...
File extensions/renderer/dispatcher.h (right):

https://codereview.chromium.org/260083006/diff/20001/extensions/renderer/disp...
extensions/renderer/dispatcher.h:1: // Copyright (c) 2012 The Chromium Authors.
All rights reserved.
why is there a "default_dispatcher_delegate.h" file before this one?

https://codereview.chromium.org/260083006/diff/20001/extensions/renderer/disp...
File extensions/renderer/dispatcher_delegate.h (right):

https://codereview.chromium.org/260083006/diff/20001/extensions/renderer/disp...
extensions/renderer/dispatcher_delegate.h:40: Feature::Context context_type);
I'd rather the "default implementation" be an interface and the only method
that's implemented for src/extensions -- CreateScriptContext -- be implemented
in some lightweight implementation of tat method.

the default implementation of this could have an =0.

https://codereview.chromium.org/260083006/diff/20001/extensions/renderer/exte...
File extensions/renderer/extensions_renderer_client.h (right):

https://codereview.chromium.org/260083006/diff/20001/extensions/renderer/exte...
extensions/renderer/extensions_renderer_client.h:21: class ScriptContext;
does this file need to change at all?

https://codereview.chromium.org/260083006/diff/20001/extensions/renderer/proc...
File extensions/renderer/process_info_native_handler.cc (right):

https://codereview.chromium.org/260083006/diff/20001/extensions/renderer/proc...
extensions/renderer/process_info_native_handler.cc:2: // Use of this source code
is governed by a BSD-style license that can be
aside: we might want to consider a separate directory for native handlers at
some point

[Ken Rockot(use gerrit already), 2014-04-30 00:01:22]

https://codereview.chromium.org/260083006/diff/20001/apps/shell/renderer/shel...
File apps/shell/renderer/shell_extensions_renderer_client.cc (right):

https://codereview.chromium.org/260083006/diff/20001/apps/shell/renderer/shel...
apps/shell/renderer/shell_extensions_renderer_client.cc:6: #include
"extensions/renderer/script_context.h"
On 2014/04/29 23:29:06, kalman wrote:
> not used

Done.

https://codereview.chromium.org/260083006/diff/20001/chrome/renderer/chrome_c...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/260083006/diff/20001/chrome/renderer/chrome_c...
chrome/renderer/chrome_content_renderer_client.cc:1331: return
g_current_client->extension_dispatcher_delegate_
On 2014/04/29 23:29:06, kalman wrote:
> this is pretty weird to look at. hopefully when the IPC handling is cleaned up
> this can be its own class rather than lumping in with the delegate.

Yes, I have half a patch somewhere that creates a WebRequestUsageObserver as its
own separate RenderProcessObserver, and CCRC creates its own one of those. I'll
get it uploaded some day soon

https://codereview.chromium.org/260083006/diff/20001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/app_custom_bindings.js (right):

https://codereview.chromium.org/260083006/diff/20001/chrome/renderer/resource...
chrome/renderer/resources/extensions/app_custom_bindings.js:75: //
chrome/renderer/extensions/dispatcher.cc.
On 2014/04/29 23:29:06, kalman wrote:
> you can delete this comment

Done.

https://codereview.chromium.org/260083006/diff/20001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/webstore_custom_bindings.js (right):

https://codereview.chromium.org/260083006/diff/20001/chrome/renderer/resource...
chrome/renderer/resources/extensions/webstore_custom_bindings.js:88: //
chrome/renderer/extensions/dispatcher.cc.
On 2014/04/29 23:29:06, kalman wrote:
> you can delete this comment.

Done.

https://codereview.chromium.org/260083006/diff/20001/chrome/test/base/chrome_...
File chrome/test/base/chrome_render_view_test.cc (right):

https://codereview.chromium.org/260083006/diff/20001/chrome/test/base/chrome_...
chrome/test/base/chrome_render_view_test.cc:59: new extensions::Dispatcher(new
ChromeExtensionsDispatcherDelegate());
On 2014/04/29 23:29:06, kalman wrote:
> leak?

Aye, I thought everything was just leaking here so I let it go. Actually I guess
RenderViewTest cleans up and ultimately takes Dispatcher with it.

So... fixed.

https://codereview.chromium.org/260083006/diff/20001/extensions/renderer/api_...
File extensions/renderer/api_activity_logger.h (right):

https://codereview.chromium.org/260083006/diff/20001/extensions/renderer/api_...
extensions/renderer/api_activity_logger.h:13: 
On 2014/04/29 23:29:06, kalman wrote:
> still not detected as a move?

Yeah, I can't find a similarity level that counts this as a move but doesn't
also introduce several false positive moves. Not sure if that's any more or less
desirable than one false negative.

https://codereview.chromium.org/260083006/diff/20001/extensions/renderer/disp...
File extensions/renderer/dispatcher.h (right):

https://codereview.chromium.org/260083006/diff/20001/extensions/renderer/disp...
extensions/renderer/dispatcher.h:1: // Copyright (c) 2012 The Chromium Authors.
All rights reserved.
On 2014/04/29 23:29:06, kalman wrote:
> why is there a "default_dispatcher_delegate.h" file before this one?

I was going to write that and decided to just make DispatcherDelegate the
default implementation. Now it has purpose because DefaultDispatcherDelegate
exists.

https://codereview.chromium.org/260083006/diff/20001/extensions/renderer/disp...
File extensions/renderer/dispatcher_delegate.h (right):

https://codereview.chromium.org/260083006/diff/20001/extensions/renderer/disp...
extensions/renderer/dispatcher_delegate.h:40: Feature::Context context_type);
On 2014/04/29 23:29:06, kalman wrote:
> I'd rather the "default implementation" be an interface and the only method
> that's implemented for src/extensions -- CreateScriptContext -- be implemented
> in some lightweight implementation of tat method.
> 
> the default implementation of this could have an =0.

Done.

https://codereview.chromium.org/260083006/diff/20001/extensions/renderer/exte...
File extensions/renderer/extensions_renderer_client.h (right):

https://codereview.chromium.org/260083006/diff/20001/extensions/renderer/exte...
extensions/renderer/extensions_renderer_client.h:21: class ScriptContext;
On 2014/04/29 23:29:06, kalman wrote:
> does this file need to change at all?

Ah, not any more. Fixed.

https://codereview.chromium.org/260083006/diff/20001/extensions/renderer/proc...
File extensions/renderer/process_info_native_handler.cc (right):

https://codereview.chromium.org/260083006/diff/20001/extensions/renderer/proc...
extensions/renderer/process_info_native_handler.cc:2: // Use of this source code
is governed by a BSD-style license that can be
On 2014/04/29 23:29:06, kalman wrote:
> aside: we might want to consider a separate directory for native handlers at
> some point

Good idea.

[Ken Rockot(use gerrit already), 2014-04-30 00:06:42]

+james for apps/shell, +yoz FYI

James, you may as well defer review until your recent CLs land and I can rebase
onto them, because I'm moving RegisterNativeHandlers and PopulateSourceMap from
ERC to the new DispatcherDelegate. So really just FYI for now.

[Ken Rockot(use gerrit already), 2014-04-30 13:37:24]

On 2014/04/30 00:06:42, Ken Rockot wrote:
> +james for apps/shell, +yoz FYI
> 
> James, you may as well defer review until your recent CLs land and I can
rebase
> onto them, because I'm moving RegisterNativeHandlers and PopulateSourceMap
from
> ERC to the new DispatcherDelegate. So really just FYI for now.

OK, rebased. I think everything is in its right place.

[James Cook, 2014-04-30 17:32:00]

An epic win of a CL!  apps/shell LGTM with nits but please see comments in the
rest.

https://codereview.chromium.org/260083006/diff/100001/apps/shell/app_shell.gyp
File apps/shell/app_shell.gyp (right):

https://codereview.chromium.org/260083006/diff/100001/apps/shell/app_shell.gy...
apps/shell/app_shell.gyp:146: 'renderer/shell_dispatcher_delegate.cc',
Please change the CL description to say you introduced a delegate instead of a
ChromeExtensionsDispatcher.

https://codereview.chromium.org/260083006/diff/100001/apps/shell/renderer/she...
File apps/shell/renderer/shell_content_renderer_client.cc (right):

https://codereview.chromium.org/260083006/diff/100001/apps/shell/renderer/she...
apps/shell/renderer/shell_content_renderer_client.cc:16: #include
"extensions/renderer/default_dispatcher_delegate.h"
Do you need this header?

https://codereview.chromium.org/260083006/diff/100001/apps/shell/renderer/she...
File apps/shell/renderer/shell_dispatcher_delegate.h (right):

https://codereview.chromium.org/260083006/diff/100001/apps/shell/renderer/she...
apps/shell/renderer/shell_dispatcher_delegate.h:13: class
ShellDispatcherDelegate : public extensions::DefaultDispatcherDelegate {
Brief class comment please.

https://codereview.chromium.org/260083006/diff/100001/chrome/common/extension...
File
chrome/common/extensions/manifest_handlers/externally_connectable_unittest.cc
(right):

https://codereview.chromium.org/260083006/diff/100001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/externally_connectable_unittest.cc:1:
// Copyright 2013 The Chromium Authors. All rights reserved.
Maybe this file should move to //extensions? It looks like we already have DEPS
exceptions for extension_manifest_test.h and a couple places we allow
feature_channel.h so it could happen with a small DEPS tweak.

https://codereview.chromium.org/260083006/diff/100001/chrome/renderer/extensi...
File chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.h (right):

https://codereview.chromium.org/260083006/diff/100001/chrome/renderer/extensi...
chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.h:17: bool
IsAdblockWithWebRequestInstalled() const { return webrequest_adblock_; }
It's so sad we have this code, but at least it's now isolated in Chrome.

I pinged that bug a month or two ago and they said they still wanted this
tracking.

https://codereview.chromium.org/260083006/diff/100001/chrome/test/base/chrome...
File chrome/test/base/chrome_render_view_test.cc (right):

https://codereview.chromium.org/260083006/diff/100001/chrome/test/base/chrome...
chrome/test/base/chrome_render_view_test.cc:101:
extension_dispatcher_delegate_.get()));
This code makes me think we want the Dispatcher to own its DispatcherDelegate.
In particular I'm not sure where ChromeContentRendererClient gets torn down.
Undoubtedly that happens before ChromeRenderViewTest is destroyed, but it still
feels funny.  What do you think?

https://codereview.chromium.org/260083006/diff/100001/extensions/DEPS
File extensions/DEPS (right):

https://codereview.chromium.org/260083006/diff/100001/extensions/DEPS#newcode21
extensions/DEPS:21: "!grit/renderer_resources.h",
Maybe comment why you need this one.

https://codereview.chromium.org/260083006/diff/100001/extensions/extensions.gyp
File extensions/extensions.gyp (right):

https://codereview.chromium.org/260083006/diff/100001/extensions/extensions.g...
extensions/extensions.gyp:505:
'../chrome/chrome_resources.gyp:chrome_resources',
Which piece of chrome_resources do we need? Maybe comment?

https://codereview.chromium.org/260083006/diff/100001/extensions/renderer/api...
File extensions/renderer/api_activity_logger.h (right):

https://codereview.chromium.org/260083006/diff/100001/extensions/renderer/api...
extensions/renderer/api_activity_logger.h:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
I know kalman already looked at this, but are you sure you want to move this
file?  In the browser process I tried to keep activity logging as a chrome-level
concept (see //extensions/browser/api_activity_monitor.h).

It looks like maybe this could stay in Chrome and the native handler
registration moved to the Chrome delegate.

https://codereview.chromium.org/260083006/diff/100001/extensions/renderer/dis...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/260083006/diff/100001/extensions/renderer/dis...
extensions/renderer/dispatcher.cc:160: class ChromeNativeHandler : public
ObjectBackedNativeHandler {
Maybe brief comment that this is for the "chrome." root of the API tree, not
Chrome code that has to live in //extensions.

https://codereview.chromium.org/260083006/diff/100001/extensions/renderer/dis...
extensions/renderer/dispatcher.cc:449:
IPC_MESSAGE_HANDLER(ExtensionMsg_ActivateExtension, OnActivateExtension)
Hooray for alphabetizing!

https://codereview.chromium.org/260083006/diff/100001/extensions/renderer/dis...
File extensions/renderer/dispatcher_delegate.h (right):

https://codereview.chromium.org/260083006/diff/100001/extensions/renderer/dis...
extensions/renderer/dispatcher_delegate.h:48: ScriptContext* context) {};
no ; after {}

https://codereview.chromium.org/260083006/diff/100001/extensions/renderer/dis...
extensions/renderer/dispatcher_delegate.h:51: virtual void
PopulateSourceMap(ResourceBundleSourceMap* source_map) {};
ditto

https://codereview.chromium.org/260083006/diff/100001/extensions/renderer/dis...
extensions/renderer/dispatcher_delegate.h:57: bool is_within_platform_app) {};
ditto

https://codereview.chromium.org/260083006/diff/100001/extensions/renderer/tes...
File extensions/renderer/test_features_native_handler.cc (right):

https://codereview.chromium.org/260083006/diff/100001/extensions/renderer/tes...
extensions/renderer/test_features_native_handler.cc:26:
source.LoadJSON(IDR_CHROME_EXTENSION_API_FEATURES);
nit: comment about how/when this can go away

[Ken Rockot(use gerrit already), 2014-04-30 19:21:58]

https://codereview.chromium.org/260083006/diff/100001/apps/shell/app_shell.gyp
File apps/shell/app_shell.gyp (right):

https://codereview.chromium.org/260083006/diff/100001/apps/shell/app_shell.gy...
apps/shell/app_shell.gyp:146: 'renderer/shell_dispatcher_delegate.cc',
On 2014/04/30 17:32:01, James Cook wrote:
> Please change the CL description to say you introduced a delegate instead of a
> ChromeExtensionsDispatcher.

Done.

https://codereview.chromium.org/260083006/diff/100001/apps/shell/renderer/she...
File apps/shell/renderer/shell_content_renderer_client.cc (right):

https://codereview.chromium.org/260083006/diff/100001/apps/shell/renderer/she...
apps/shell/renderer/shell_content_renderer_client.cc:16: #include
"extensions/renderer/default_dispatcher_delegate.h"
On 2014/04/30 17:32:01, James Cook wrote:
> Do you need this header?

Not any more. Removed.

https://codereview.chromium.org/260083006/diff/100001/apps/shell/renderer/she...
File apps/shell/renderer/shell_dispatcher_delegate.h (right):

https://codereview.chromium.org/260083006/diff/100001/apps/shell/renderer/she...
apps/shell/renderer/shell_dispatcher_delegate.h:13: class
ShellDispatcherDelegate : public extensions::DefaultDispatcherDelegate {
On 2014/04/30 17:32:01, James Cook wrote:
> Brief class comment please.

Done.

https://codereview.chromium.org/260083006/diff/100001/chrome/common/extension...
File
chrome/common/extensions/manifest_handlers/externally_connectable_unittest.cc
(right):

https://codereview.chromium.org/260083006/diff/100001/chrome/common/extension...
chrome/common/extensions/manifest_handlers/externally_connectable_unittest.cc:1:
// Copyright 2013 The Chromium Authors. All rights reserved.
On 2014/04/30 17:32:01, James Cook wrote:
> Maybe this file should move to //extensions? It looks like we already have
DEPS
> exceptions for extension_manifest_test.h and a couple places we allow
> feature_channel.h so it could happen with a small DEPS tweak.

Done. I bumped feature_channel.h up to an exception for any test module.

https://codereview.chromium.org/260083006/diff/100001/chrome/test/base/chrome...
File chrome/test/base/chrome_render_view_test.cc (right):

https://codereview.chromium.org/260083006/diff/100001/chrome/test/base/chrome...
chrome/test/base/chrome_render_view_test.cc:101:
extension_dispatcher_delegate_.get()));
On 2014/04/30 17:32:01, James Cook wrote:
> This code makes me think we want the Dispatcher to own its DispatcherDelegate.
> In particular I'm not sure where ChromeContentRendererClient gets torn down.
> Undoubtedly that happens before ChromeRenderViewTest is destroyed, but it
still
> feels funny.  What do you think?

Yes I agree, but I want to hold off on this until the adblock garbage moves out
of ChromeExtensionsDispatcherDelegate (soon, I promise). Until then, CCRC wants
to own its delegate directly and only share it with Dispatcher. Sad but
temporary.

https://codereview.chromium.org/260083006/diff/100001/extensions/DEPS
File extensions/DEPS (right):

https://codereview.chromium.org/260083006/diff/100001/extensions/DEPS#newcode21
extensions/DEPS:21: "!grit/renderer_resources.h",
On 2014/04/30 17:32:01, James Cook wrote:
> Maybe comment why you need this one.

Done.

https://codereview.chromium.org/260083006/diff/100001/extensions/renderer/api...
File extensions/renderer/api_activity_logger.h (right):

https://codereview.chromium.org/260083006/diff/100001/extensions/renderer/api...
extensions/renderer/api_activity_logger.h:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
On 2014/04/30 17:32:01, James Cook wrote:
> I know kalman already looked at this, but are you sure you want to move this
> file?  In the browser process I tried to keep activity logging as a
chrome-level
> concept (see //extensions/browser/api_activity_monitor.h).
> 
> It looks like maybe this could stay in Chrome and the native handler
> registration moved to the Chrome delegate.

Yes, at least for now. The core API bindings JS code depends on it. I think it's
OK if this lives in the renderer and sends host messages that the browser just
ignores in app_shell.

Maybe we can refactor the bindings code to not have this as a hard dependency,
but that'll be another CL.

https://codereview.chromium.org/260083006/diff/100001/extensions/renderer/dis...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/260083006/diff/100001/extensions/renderer/dis...
extensions/renderer/dispatcher.cc:160: class ChromeNativeHandler : public
ObjectBackedNativeHandler {
On 2014/04/30 17:32:01, James Cook wrote:
> Maybe brief comment that this is for the "chrome." root of the API tree, not
> Chrome code that has to live in //extensions.

Done.

https://codereview.chromium.org/260083006/diff/100001/extensions/renderer/tes...
File extensions/renderer/test_features_native_handler.cc (right):

https://codereview.chromium.org/260083006/diff/100001/extensions/renderer/tes...
extensions/renderer/test_features_native_handler.cc:26:
source.LoadJSON(IDR_CHROME_EXTENSION_API_FEATURES);
On 2014/04/30 17:32:01, James Cook wrote:
> nit: comment about how/when this can go away

Done.

[Ken Rockot(use gerrit already), 2014-04-30 19:28:19]

The CQ bit was checked by [email protected]

[commit-bot: I haz the power, 2014-04-30 19:29:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/260083006/140001

[commit-bot: I haz the power, 2014-04-30 19:34:37]

The CQ bit was unchecked by [email protected]

[commit-bot: I haz the power, 2014-04-30 19:34:38]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium

[Ken Rockot(use gerrit already), 2014-04-30 19:35:36]

The CQ bit was checked by [email protected]

[commit-bot: I haz the power, 2014-04-30 19:36:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/260083006/140001

[commit-bot: I haz the power, 2014-04-30 19:42:42]

The CQ bit was unchecked by [email protected]

[commit-bot: I haz the power, 2014-04-30 19:42:42]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium

[Ken Rockot(use gerrit already), 2014-04-30 19:56:44]

The CQ bit was checked by [email protected]

[commit-bot: I haz the power, 2014-04-30 19:58:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/260083006/140001

[commit-bot: I haz the power, 2014-04-30 20:17:51]

The CQ bit was unchecked by [email protected]

[commit-bot: I haz the power, 2014-04-30 20:17:51]

Try jobs failed on following builders:
  linux_chromium_rel on tryserver.chromium

[Ken Rockot(use gerrit already), 2014-04-30 20:25:11]

The CQ bit was checked by [email protected]

[commit-bot: I haz the power, 2014-04-30 20:26:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/260083006/160001

[Ken Rockot(use gerrit already), 2014-04-30 20:40:38]

The CQ bit was unchecked by [email protected]

[commit-bot: I haz the power, 2014-04-30 20:41:23]

The CQ bit was unchecked by [email protected]

[commit-bot: I haz the power, 2014-04-30 20:41:24]

Try jobs failed on following builders:
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
  linux_chromium_rel on tryserver.chromium

[Ken Rockot(use gerrit already), 2014-04-30 20:41:56]

The CQ bit was checked by [email protected]

[commit-bot: I haz the power, 2014-04-30 20:43:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/260083006/160002

[commit-bot: I haz the power, 2014-04-30 21:17:15]

The CQ bit was unchecked by [email protected]

[commit-bot: I haz the power, 2014-04-30 21:17:15]

Try jobs failed on following builders:
  android_clang_dbg on tryserver.chromium

[James Cook, 2014-05-01 05:07:58]

On 2014/04/30 21:17:15, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   android_clang_dbg on tryserver.chromium

I just put my DOMActivityLogger patch in the CQ, which will almost certainly
conflict with this one.   Feel free to uncheck the CQ box on my patch if you
want to land first. https://codereview.chromium.org/268553002/

[Ken Rockot(use gerrit already), 2014-05-01 15:17:23]

The CQ bit was checked by [email protected]

[commit-bot: I haz the power, 2014-05-01 15:18:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/260083006/190001

[commit-bot: I haz the power, 2014-05-01 17:29:16]

Change committed as 267564