[Extensions] Use a separate IPC message for extension events

[Extensions] Use a separate IPC message for extension events

Currently, extension events are dispatched using the MessageInvoke IPC,
which instructs the the renderer to invoke the JS methods to disptach
the event. This has a few problems:
- It reuses the invoke IPC when it really shouldn't. Looks like there's
  a long-standing TODO to change this.
- It's incompatible if we ever change the way we want to dispatch
  events, as we are with the C++ bindings work.

Migrate event dispatching to a separate IPC message, and remove the
now-unneeded user gesture parameter from the MessageInvoke IPC.

BUG=666004
BUG=653596
Committed: https://crrev.com/ff516ab47c3fe130da26006e0f677ee5a78a9f19
Cr-Commit-Position: refs/heads/master@{#432645}

[Devlin, 2016-11-15 01:21:10]

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

[commit-bot: I haz the power, 2016-11-15 01:22:30]

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-15 03:27:07]

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

[commit-bot: I haz the power, 2016-11-15 03:27:08]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Devlin, 2016-11-15 16:52:24]

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

[commit-bot: I haz the power, 2016-11-15 16:52:44]

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-15 17:44:48]

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

[commit-bot: I haz the power, 2016-11-15 17:44:49]

Dry run: This issue passed the CQ dry run.

[Devlin, 2016-11-16 17:18:54]

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

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

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 18:00:56]

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

[commit-bot: I haz the power, 2016-11-16 18:00:57]

Dry run: This issue passed the CQ dry run.

[Devlin, 2016-11-16 19:16:50]

Description was changed from

==========
Remove event
BUG=
==========

to

==========
[Extensions] Use a separate IPC message for extension events

Currently, extension events are dispatched using the MessageInvoke IPC,
which instructs the the renderer to invoke the JS methods to disptach
the event. This has a few problems:
- It reuses the invoke IPC when it really shouldn't. Looks like there's
  a long-standing TODO to change this.
- It's incompatible if we ever change the way we want to dispatch
  events, as we are with the C++ bindings work.

Migrate event dispatching to a separate IPC message, and remove the
now-unneeded user gesture parameter from the MessageInvoke IPC.

BUG=666004
BUG=653596
==========

[Devlin, 2016-11-16 19:17:29]

rdevlin.cronin@chromium.org changed reviewers:
+ lazyboy@chromium.org

[Devlin, 2016-11-16 19:17:30]

lazyboy@, mind taking a look?

+cc jbroman since it's somewhat related to the bindings work.

[lazyboy, 2016-11-16 20:21:02]

lgtm with nits

https://codereview.chromium.org/2495213007/diff/40001/extensions/browser/even...
File extensions/browser/event_router.cc (left):

https://codereview.chromium.org/2495213007/diff/40001/extensions/browser/even...
extensions/browser/event_router.cc:112: args.Set(3, new
base::FundamentalValue(event_id));
Ah this was passed to JS handler, but never used?

https://codereview.chromium.org/2495213007/diff/40001/extensions/common/exten...
File extensions/common/extension_messages.h (right):

https://codereview.chromium.org/2495213007/diff/40001/extensions/common/exten...
extensions/common/extension_messages.h:455: // Note: The arguments are separate
from the params to avoid having the message
nit: s/The arguments are/|event_args| is

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

https://codereview.chromium.org/2495213007/diff/40001/extensions/renderer/dis...
extensions/renderer/dispatcher.cc:1109: InvokeModuleSystemMethod(NULL,
extension_id, module_name, function_name,
nullptr

[Devlin, 2016-11-16 20:52:55]

https://codereview.chromium.org/2495213007/diff/40001/extensions/browser/even...
File extensions/browser/event_router.cc (left):

https://codereview.chromium.org/2495213007/diff/40001/extensions/browser/even...
extensions/browser/event_router.cc:112: args.Set(3, new
base::FundamentalValue(event_id));
On 2016/11/16 20:21:02, lazyboy wrote:
> Ah this was passed to JS handler, but never used?

It was actually abused to pass the event id for the EventAck message in the
response.  So we read it out in the renderer and then dispatched a response with
it.  You can see the code in dispatcher.cc.

It's replaced with the much more apparent "event_id" param. :)

https://codereview.chromium.org/2495213007/diff/40001/extensions/common/exten...
File extensions/common/extension_messages.h (right):

https://codereview.chromium.org/2495213007/diff/40001/extensions/common/exten...
extensions/common/extension_messages.h:455: // Note: The arguments are separate
from the params to avoid having the message
On 2016/11/16 20:21:02, lazyboy wrote:
> nit: s/The arguments are/|event_args| is

Done.

https://codereview.chromium.org/2495213007/diff/40001/extensions/renderer/dis...
File extensions/renderer/dispatcher.cc (left):

https://codereview.chromium.org/2495213007/diff/40001/extensions/renderer/dis...
extensions/renderer/dispatcher.cc:674: args.GetInteger(3, &message_id);
(Note: This is where it used to be used)

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

https://codereview.chromium.org/2495213007/diff/40001/extensions/renderer/dis...
extensions/renderer/dispatcher.cc:1109: InvokeModuleSystemMethod(NULL,
extension_id, module_name, function_name,
On 2016/11/16 20:21:02, lazyboy wrote:
> nullptr

Done.

[Devlin, 2016-11-16 20:52:59]

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

[commit-bot: I haz the power, 2016-11-16 20:54:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Devlin, 2016-11-16 20:54:11]

rdevlin.cronin@chromium.org changed reviewers:
+ tsepez@chromium.org

[Devlin, 2016-11-16 20:54:12]

+Tom for the IPC messages.

[Tom Sepez, 2016-11-16 21:12:19]

messages LGTM

[Devlin, 2016-11-16 21:18:36]

The CQ bit was unchecked by rdevlin.cronin@chromium.org

[Devlin, 2016-11-16 21:18:42]

The CQ bit was checked by rdevlin.cronin@chromium.org

[Devlin, 2016-11-16 21:18:42]

The patchset sent to the CQ was uploaded after l-g-t-m from lazyboy@chromium.org
Link to the patchset: https://codereview.chromium.org/2495213007/#ps60001
(title: "lazyboys")

[commit-bot: I haz the power, 2016-11-16 21:19:46]

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 22:26:03]

Description was changed from

==========
[Extensions] Use a separate IPC message for extension events

Currently, extension events are dispatched using the MessageInvoke IPC,
which instructs the the renderer to invoke the JS methods to disptach
the event. This has a few problems:
- It reuses the invoke IPC when it really shouldn't. Looks like there's
  a long-standing TODO to change this.
- It's incompatible if we ever change the way we want to dispatch
  events, as we are with the C++ bindings work.

Migrate event dispatching to a separate IPC message, and remove the
now-unneeded user gesture parameter from the MessageInvoke IPC.

BUG=666004
BUG=653596
==========

to

==========
[Extensions] Use a separate IPC message for extension events

Currently, extension events are dispatched using the MessageInvoke IPC,
which instructs the the renderer to invoke the JS methods to disptach
the event. This has a few problems:
- It reuses the invoke IPC when it really shouldn't. Looks like there's
  a long-standing TODO to change this.
- It's incompatible if we ever change the way we want to dispatch
  events, as we are with the C++ bindings work.

Migrate event dispatching to a separate IPC message, and remove the
now-unneeded user gesture parameter from the MessageInvoke IPC.

BUG=666004
BUG=653596
==========

[commit-bot: I haz the power, 2016-11-16 22:26:04]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-16 22:34:33]

Description was changed from

==========
[Extensions] Use a separate IPC message for extension events

Currently, extension events are dispatched using the MessageInvoke IPC,
which instructs the the renderer to invoke the JS methods to disptach
the event. This has a few problems:
- It reuses the invoke IPC when it really shouldn't. Looks like there's
  a long-standing TODO to change this.
- It's incompatible if we ever change the way we want to dispatch
  events, as we are with the C++ bindings work.

Migrate event dispatching to a separate IPC message, and remove the
now-unneeded user gesture parameter from the MessageInvoke IPC.

BUG=666004
BUG=653596
==========

to

==========
[Extensions] Use a separate IPC message for extension events

Currently, extension events are dispatched using the MessageInvoke IPC,
which instructs the the renderer to invoke the JS methods to disptach
the event. This has a few problems:
- It reuses the invoke IPC when it really shouldn't. Looks like there's
  a long-standing TODO to change this.
- It's incompatible if we ever change the way we want to dispatch
  events, as we are with the C++ bindings work.

Migrate event dispatching to a separate IPC message, and remove the
now-unneeded user gesture parameter from the MessageInvoke IPC.

BUG=666004
BUG=653596

Committed: https://crrev.com/ff516ab47c3fe130da26006e0f677ee5a78a9f19
Cr-Commit-Position: refs/heads/master@{#432645}
==========

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

Patchset 4 (id:??) landed as
https://crrev.com/ff516ab47c3fe130da26006e0f677ee5a78a9f19
Cr-Commit-Position: refs/heads/master@{#432645}