Remove ChromeV8Extension & most of ChromeV8Context

Remove ChromeV8Extension & most of ChromeV8Context

This replaces all ChromeV8Extension usage with ObjectBackedNativeHandler
and in the process changes most ChromeV8Context references into
ScriptContext references.

Also, since it would be ugly and confusing otherwise,
ChromeV8ContextSet is now ScriptContextSet and is moved
to //extensions/renderer.

BUG=359836
TBR=sky@chromium.org for //c/test and ChromeContentRendererCilent
TBR=bbudge@chromium.org for //c/r/pepper

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

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

Hey Ben, could you please take a look at this?

[not at google - send to devlin, 2014-04-17 15:11:10]

awesome, lgtm. I left a smattering of comments about more cleanup we could do
later.

+devlin who has an OKR about cleanup
+koz -- worried that this will complicate your work on pepper

https://codereview.chromium.org/240603003/diff/20001/chrome/renderer/extensio...
File chrome/renderer/extensions/app_bindings.cc (right):

https://codereview.chromium.org/240603003/diff/20001/chrome/renderer/extensio...
chrome/renderer/extensions/app_bindings.cc:62:
ChromeV8ExtensionHandler(context),
ChromeV8ExtensionHandle is pretty cool actually. It wraps classes to make them
able to send and receive IPCs in a more sugary way. Several other classes (like
RequestSender) go through the Dispatcher which is silly, they should just listen
to the IPCs themselves.

It's also not cool because it doesn't add much value, really, and violates the
C++ multi-inheritance style guide besides.

So -- just putting this on your roadmap. We should delete
ChromeV8ExtensionHander and if it's actually valuable enough to maintain some
sort of sugar around handling IPCs then implement it using composition not
inheritance.

https://codereview.chromium.org/240603003/diff/20001/chrome/renderer/extensio...
File chrome/renderer/extensions/dispatcher.cc (right):

https://codereview.chromium.org/240603003/diff/20001/chrome/renderer/extensio...
chrome/renderer/extensions/dispatcher.cc:596:
MessagingBindings::DispatchOnConnect(script_context_set_.GetAll(),
MessagingBindings is another class that should be listening to IPCs directly.

https://codereview.chromium.org/240603003/diff/20001/chrome/renderer/extensio...
File chrome/renderer/extensions/dispatcher.h (right):

https://codereview.chromium.org/240603003/diff/20001/chrome/renderer/extensio...
chrome/renderer/extensions/dispatcher.h:73: const ScriptContextSet&
script_context_set() const {
looks like there are a number of places that only need a Dispatcher in order to
access this. it would be nice to change those to take a ScriptContextSet
instead.

https://codereview.chromium.org/240603003/diff/20001/chrome/renderer/extensio...
File chrome/renderer/extensions/extension_helper.cc (right):

https://codereview.chromium.org/240603003/diff/20001/chrome/renderer/extensio...
chrome/renderer/extensions/extension_helper.cc:384: "onAppWindowClosed");
this method looks unnecessary. it doesn't even need to be an IPC, the browser
could directly call MessageInvoke:

https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/....

https://codereview.chromium.org/240603003/diff/20001/chrome/renderer/extensio...
File chrome/renderer/extensions/file_browser_private_custom_bindings.cc (right):

https://codereview.chromium.org/240603003/diff/20001/chrome/renderer/extensio...
chrome/renderer/extensions/file_browser_private_custom_bindings.cc:21: :
ObjectBackedNativeHandler(context) {
random place for this comment, but it would be even better if the
ObjectBackedNativeHandler functionality were exposed via composition not
inheritance.

not now, maybe not ever :)

https://codereview.chromium.org/240603003/diff/20001/chrome/renderer/extensio...
File chrome/renderer/extensions/messaging_bindings.cc (right):

https://codereview.chromium.org/240603003/diff/20001/chrome/renderer/extensio...
chrome/renderer/extensions/messaging_bindings.cc:102:
dispatcher_->ClearPortData(port_id);
why does Dispatcher know anything about ports..? weird.

[Ken Rockot(use gerrit already), 2014-04-17 16:17:15]

Alright sweet, I'll file+CC some bugs regarding some of the other cleanup
comments.

https://codereview.chromium.org/240603003/diff/20001/chrome/renderer/extensio...
File chrome/renderer/extensions/app_bindings.cc (right):

https://codereview.chromium.org/240603003/diff/20001/chrome/renderer/extensio...
chrome/renderer/extensions/app_bindings.cc:62:
ChromeV8ExtensionHandler(context),
On 2014/04/17 15:11:10, kalman wrote:
> ChromeV8ExtensionHandle is pretty cool actually. It wraps classes to make them
> able to send and receive IPCs in a more sugary way. Several other classes
(like
> RequestSender) go through the Dispatcher which is silly, they should just
listen
> to the IPCs themselves.
> 
> It's also not cool because it doesn't add much value, really, and violates the
> C++ multi-inheritance style guide besides.
> 
> So -- just putting this on your roadmap. We should delete
> ChromeV8ExtensionHander and if it's actually valuable enough to maintain some
> sort of sugar around handling IPCs then implement it using composition not
> inheritance.

Great, I think we're on the same page here.

https://codereview.chromium.org/240603003/diff/20001/chrome/renderer/extensio...
File chrome/renderer/extensions/file_browser_private_custom_bindings.cc (right):

https://codereview.chromium.org/240603003/diff/20001/chrome/renderer/extensio...
chrome/renderer/extensions/file_browser_private_custom_bindings.cc:21: :
ObjectBackedNativeHandler(context) {
On 2014/04/17 15:11:10, kalman wrote:
> random place for this comment, but it would be even better if the
> ObjectBackedNativeHandler functionality were exposed via composition not
> inheritance.
> 
> not now, maybe not ever :)

Thinking about it. We shall see!

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

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2014-04-17 16:17:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rockot@chromium.org/240603003/20001

[commit-bot: I haz the power, 2014-04-17 16:44:17]

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

[commit-bot: I haz the power, 2014-04-17 16:44:17]

Retried try job too often on ios_rel_device for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_rel_de...

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

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2014-04-17 17:17:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rockot@chromium.org/240603003/20001

[commit-bot: I haz the power, 2014-04-17 19:18:01]

Change committed as 264605

[koz (OOO until 15th September), 2014-04-22 00:13:38]

https://codereview.chromium.org/240603003/diff/20001/chrome/renderer/extensio...
File chrome/renderer/extensions/app_bindings.cc (right):

https://codereview.chromium.org/240603003/diff/20001/chrome/renderer/extensio...
chrome/renderer/extensions/app_bindings.cc:62:
ChromeV8ExtensionHandler(context),
On 2014/04/17 16:17:15, Ken Rockot wrote:
> On 2014/04/17 15:11:10, kalman wrote:
> > ChromeV8ExtensionHandle is pretty cool actually. It wraps classes to make
them
> > able to send and receive IPCs in a more sugary way. Several other classes
> (like
> > RequestSender) go through the Dispatcher which is silly, they should just
> listen
> > to the IPCs themselves.
> > 
> > It's also not cool because it doesn't add much value, really, and violates
the
> > C++ multi-inheritance style guide besides.
> > 
> > So -- just putting this on your roadmap. We should delete
> > ChromeV8ExtensionHander and if it's actually valuable enough to maintain
some
> > sort of sugar around handling IPCs then implement it using composition not
> > inheritance.
> 
> Great, I think we're on the same page here.

Totally. De-centralising communication with the browser will also help us get
ready for Mojo, where we'd have multiple connections to the browser.

https://codereview.chromium.org/240603003/diff/20001/chrome/renderer/extensio...
File chrome/renderer/extensions/extension_helper.cc (right):

https://codereview.chromium.org/240603003/diff/20001/chrome/renderer/extensio...
chrome/renderer/extensions/extension_helper.cc:384: "onAppWindowClosed");
On 2014/04/17 15:11:10, kalman wrote:
> this method looks unnecessary. it doesn't even need to be an IPC, the browser
> could directly call MessageInvoke:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/....

I think it's good to move away from calling JS methods from the browser, because
it makes it harder to enable (e.g.) Pepper plugins to talk to the same IPC
surface if the browser assumes JS is running on the remote end.

[not at google - send to devlin, 2014-04-22 00:15:37]

https://codereview.chromium.org/240603003/diff/20001/chrome/renderer/extensio...
File chrome/renderer/extensions/extension_helper.cc (right):

https://codereview.chromium.org/240603003/diff/20001/chrome/renderer/extensio...
chrome/renderer/extensions/extension_helper.cc:384: "onAppWindowClosed");
On 2014/04/22 00:13:38, koz wrote:
> On 2014/04/17 15:11:10, kalman wrote:
> > this method looks unnecessary. it doesn't even need to be an IPC, the
browser
> > could directly call MessageInvoke:
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/....
> 
> I think it's good to move away from calling JS methods from the browser,
because
> it makes it harder to enable (e.g.) Pepper plugins to talk to the same IPC
> surface if the browser assumes JS is running on the remote end.

That makes sense.