Make ExtensionMsg_MessageInvoke run a module system function rather than a

Replace all occurrences of CallChromeHiddenMethod (and friends) with
CallModuleMethod.

This also makes ExtensionMsg_MessageInvoke run a module system function rather
than a chromeHidden function, meaning we can now lazily load the extension
event handling code (i.e. miscellaneous_bindings).

BUG=55316
TBR=cdn@chromium.org, koz@chromium.org

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

[not at google - send to devlin, 2013-05-31 00:02:18]

koz - everything
cdn - extension_messages.h (this is the last one, promise)


This is based off https://codereview.chromium.org/16226004/ which has been lg'd
(just waiting for security lg).

And the reason it's a bit haphazard is because it's extracted from
https://codereview.chromium.org/15841013/ - apologies if this is a bit too
broad, if you find things that are then push back.

[not at google - send to devlin, 2013-05-31 00:05:53]

Sorry the CL description wasn't broad enough - it actually gets rid of
CallChromeHiddenMethod entirely, part of which involved changing that method
invoke thing.

and yeah, note in dispatcher.cc not eagerly requiring miscellaneous_bindings
anymore.

[koz (OOO until 15th September), 2013-05-31 04:28:43]

lgtm

Cool change!

https://codereview.chromium.org/15855010/diff/10001/chrome/renderer/extension...
File chrome/renderer/extensions/chrome_v8_context_set.h (right):

https://codereview.chromium.org/15855010/diff/10001/chrome/renderer/extension...
chrome/renderer/extensions/chrome_v8_context_set.h:63: // Re-entrantly runs
|callback| with each ChromeV8Context that belongs to
The word "re-entrantly" makes me think that the callback is being called back
into while it is being called, but in reality it's just getting invoked one time
on each ChromeV8Context. Could you rephrase?

https://codereview.chromium.org/15855010/diff/10001/chrome/renderer/extension...
File chrome/renderer/extensions/extension_helper.cc (right):

https://codereview.chromium.org/15855010/diff/10001/chrome/renderer/extension...
chrome/renderer/extensions/extension_helper.cc:261:
dispatcher_->v8_context_set().ForEach(
It feels a little weird to have to pass Dispatcher::CallModuleMethod like this.
Consider refactoring to look like

dispatcher_->v8_context_set()->CallModuleMethodOnAllContexts(...);

Or even

dispatcher_->CallModuleMethodOnAllContexts(...);

This would clean up a bunch of other ForEach() call sites, too. Up to you,
though.

https://codereview.chromium.org/15855010/diff/10001/chrome/renderer/extension...
File chrome/renderer/extensions/miscellaneous_bindings.cc (right):

https://codereview.chromium.org/15855010/diff/10001/chrome/renderer/extension...
chrome/renderer/extensions/miscellaneous_bindings.cc:261: v8::Handle<v8::Value>
retval = (*it)->module_system()->CallModuleMethod(
No more need for v8::TryCatch?

https://codereview.chromium.org/15855010/diff/10001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/event.js (right):

https://codereview.chromium.org/15855010/diff/10001/chrome/renderer/resources...
chrome/renderer/resources/extensions/event.js:448:
unloadEvent.addListener(function() {
Very nice :-)

[Cris Neckar, 2013-05-31 18:30:31]

IPC changes LGTM

[Matt Perry, 2013-05-31 20:22:22]

This is really awesome.

[not at google - send to devlin, 2013-05-31 22:47:07]

https://codereview.chromium.org/15855010/diff/10001/chrome/renderer/extension...
File chrome/renderer/extensions/chrome_v8_context_set.h (right):

https://codereview.chromium.org/15855010/diff/10001/chrome/renderer/extension...
chrome/renderer/extensions/chrome_v8_context_set.h:63: // Re-entrantly runs
|callback| with each ChromeV8Context that belongs to
On 2013/05/31 04:28:44, koz wrote:
> The word "re-entrantly" makes me think that the callback is being called back
> into while it is being called, but in reality it's just getting invoked one
time
> on each ChromeV8Context. Could you rephrase?

Done.

https://codereview.chromium.org/15855010/diff/10001/chrome/renderer/extension...
File chrome/renderer/extensions/extension_helper.cc (right):

https://codereview.chromium.org/15855010/diff/10001/chrome/renderer/extension...
chrome/renderer/extensions/extension_helper.cc:261:
dispatcher_->v8_context_set().ForEach(
On 2013/05/31 04:28:44, koz wrote:
> It feels a little weird to have to pass Dispatcher::CallModuleMethod like
this.
> Consider refactoring to look like
> 
> dispatcher_->v8_context_set()->CallModuleMethodOnAllContexts(...);
> 
> Or even
> 
> dispatcher_->CallModuleMethodOnAllContexts(...);
> 
> This would clean up a bunch of other ForEach() call sites, too. Up to you,
> though.

I had decided to do it this way because there were the 3 different forms of
CallModuleMethod and it seems an arbitrary choice which of the forms to lift
onto ChromeV8Context - certainly I wouldn't lift a ListValue version onto it,
which is what all of these take.

However I agree it's weird. I'll factor this a bit differently and share the
concept at a higher level.

https://codereview.chromium.org/15855010/diff/10001/chrome/renderer/extension...
File chrome/renderer/extensions/miscellaneous_bindings.cc (right):

https://codereview.chromium.org/15855010/diff/10001/chrome/renderer/extension...
chrome/renderer/extensions/miscellaneous_bindings.cc:261: v8::Handle<v8::Value>
retval = (*it)->module_system()->CallModuleMethod(
On 2013/05/31 04:28:44, koz wrote:
> No more need for v8::TryCatch?

Yeah CallModuleMethod handles it. I figure it's simpler than forcing callers to
deal with it; I've been making it a crash error if that happens (when JS wants
to call into user code they should make sure to catch and deal with errors).

https://codereview.chromium.org/15855010/diff/10001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/event.js (right):

https://codereview.chromium.org/15855010/diff/10001/chrome/renderer/resources...
chrome/renderer/resources/extensions/event.js:448:
unloadEvent.addListener(function() {
On 2013/05/31 04:28:44, koz wrote:
> Very nice :-)

:)

[commit-bot: I haz the power, 2013-05-31 23:03:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/15855010/23001

[commit-bot: I haz the power, 2013-05-31 23:14:44]

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

[commit-bot: I haz the power, 2013-05-31 23:16:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/15855010/23001

[commit-bot: I haz the power, 2013-06-01 04:55:25]

The commit queue went berserk retrying too often for a
seemingly flaky test on builder android_dbg:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2013-06-01 05:44:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/15855010/23001

[commit-bot: I haz the power, 2013-06-01 11:00:18]

The commit queue went berserk retrying too often for a
seemingly flaky test on builder android_dbg:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2013-06-02 18:44:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/15855010/23001

[commit-bot: I haz the power, 2013-06-02 23:40:51]

The commit queue went berserk retrying too often for a
seemingly flaky test on builder android_dbg:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2013-06-03 14:24:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/15855010/61001

[commit-bot: I haz the power, 2013-06-03 14:39:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/15855010/64003

[commit-bot: I haz the power, 2013-06-03 15:15:36]

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

[commit-bot: I haz the power, 2013-06-03 15:42:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/15855010/62008

[commit-bot: I haz the power, 2013-06-04 23:45:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/15855010/74034

[not at google - send to devlin, 2013-06-05 05:01:28]

Committed patchset #10 manually as r204157 (presubmit successful).