[Extensions Bindings] Add a registerEventArgumentMassager

[Extensions Bindings] Add a registerEventArgumentMassager

Some custom hooks need to modify ("massage") the arguments originally
passed from the browser. This can be used to hide implementation
details, introduce an unserializable concept (like a file handler), or
add extra parameters (like a callback).

Add support for this by extending the APIBindingBridge with a
RegisterEventArgumentMassager method (exposed as
registerEventArgumentMassager in the bindings), and extend
APIEventHandler to support these.

BUG=653596
Review-Url: https://codereview.chromium.org/2727583004
Cr-Commit-Position: refs/heads/master@{#454735}
Committed: https://chromium.googlesource.com/chromium/src/+/f9cdb557485c0186a1f4520c966628736465b84c

[Devlin, 2017-03-01 21:50:47]

Description was changed from

==========
argument massagers
BUG=
==========

to

==========
[Extensions Bindings] Add a registerEventArgumentMassager

Some custom hooks need to modify ("massage") the arguments originally
passed from the browser. This can be used to hide implementation
details, introduce an unserializable concept (like a file handler), or
add extra parameters (like a callback).

Add support for this by extending the APIBindingBridge with a
RegisterEventArgumentMassager method (exposed as
registerEventArgumentMassager in the bindings), and extend
APIEventHandler to support these.

BUG=653596
==========

[Devlin, 2017-03-01 21:55:45]

Patchset #2 (id:20001) has been deleted

[Devlin, 2017-03-01 21:55:49]

Patchset #1 (id:1) has been deleted

[Devlin, 2017-03-01 21:59:30]

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

[jbroman, 2017-03-01 22:33:35]

https://codereview.chromium.org/2727583004/diff/40001/extensions/renderer/api...
File extensions/renderer/api_event_handler.cc (right):

https://codereview.chromium.org/2727583004/diff/40001/extensions/renderer/api...
extensions/renderer/api_event_handler.cc:198: data->massagers[event_name] =
nit: could use this if you prefer (slightly shorter):

data->massagers[event_name].Reset(context->GetIsolate(), massager);

https://codereview.chromium.org/2727583004/diff/40001/extensions/renderer/api...
File extensions/renderer/api_event_handler_unittest.cc (right):

https://codereview.chromium.org/2727583004/diff/40001/extensions/renderer/api...
extensions/renderer/api_event_handler_unittest.cc:725: auto argument_massager =
[](const v8::FunctionCallbackInfo<v8::Value>& info) {
This works, but elsewhere you usually use a function implemented in JS. Any
reason to deviate from that? (Using JS seems nicer because it's how it would
actually be used, so we test something closer to the real path.)

https://codereview.chromium.org/2727583004/diff/40001/extensions/renderer/api...
extensions/renderer/api_event_handler_unittest.cc:743:
RunFunctionOnGlobal(dispatch, context, arraysize(function_args),
Since you mention that the dispatch function can be called asynchronously, we
probably ought to have at least one test for that.

https://codereview.chromium.org/2727583004/diff/40001/extensions/renderer/api...
extensions/renderer/api_event_handler_unittest.cc:749:
massager_template->GetFunction(context).ToLocalChecked();
nit: Otherwise, might as well use v8::Function::New here as well, not that it
really matters given this is a unit test.

[Devlin, 2017-03-02 17:19:27]

https://codereview.chromium.org/2727583004/diff/40001/extensions/renderer/api...
File extensions/renderer/api_event_handler.cc (right):

https://codereview.chromium.org/2727583004/diff/40001/extensions/renderer/api...
extensions/renderer/api_event_handler.cc:198: data->massagers[event_name] =
On 2017/03/01 22:33:35, jbroman wrote:
> nit: could use this if you prefer (slightly shorter):
> 
> data->massagers[event_name].Reset(context->GetIsolate(), massager);

Sure, done.

I sometimes wonder which is faster under the hood, though - the former could in
theory be optimized by the compiler to construct the new global in-place in the
map with the proper value, the shorter Reset() version means that we first have
to construct an empty global, then call reset on it to initialize it to the
proper state.  It seems, to me, like that would be slower.

Of course, all of this is basically setting a pointer, so... not that important.
 But interesting to think about. :)

https://codereview.chromium.org/2727583004/diff/40001/extensions/renderer/api...
File extensions/renderer/api_event_handler_unittest.cc (right):

https://codereview.chromium.org/2727583004/diff/40001/extensions/renderer/api...
extensions/renderer/api_event_handler_unittest.cc:725: auto argument_massager =
[](const v8::FunctionCallbackInfo<v8::Value>& info) {
On 2017/03/01 22:33:35, jbroman wrote:
> This works, but elsewhere you usually use a function implemented in JS. Any
> reason to deviate from that? (Using JS seems nicer because it's how it would
> actually be used, so we test something closer to the real path.)

Translated to JS.

https://codereview.chromium.org/2727583004/diff/40001/extensions/renderer/api...
extensions/renderer/api_event_handler_unittest.cc:743:
RunFunctionOnGlobal(dispatch, context, arraysize(function_args),
On 2017/03/01 22:33:35, jbroman wrote:
> Since you mention that the dispatch function can be called asynchronously, we
> probably ought to have at least one test for that.

Added a second unit test for this.

https://codereview.chromium.org/2727583004/diff/40001/extensions/renderer/api...
extensions/renderer/api_event_handler_unittest.cc:749:
massager_template->GetFunction(context).ToLocalChecked();
On 2017/03/01 22:33:35, jbroman wrote:
> nit: Otherwise, might as well use v8::Function::New here as well, not that it
> really matters given this is a unit test.

Ack, but moot. :)

[jbroman, 2017-03-02 21:12:41]

lgtm

https://codereview.chromium.org/2727583004/diff/40001/extensions/renderer/api...
File extensions/renderer/api_event_handler.cc (right):

https://codereview.chromium.org/2727583004/diff/40001/extensions/renderer/api...
extensions/renderer/api_event_handler.cc:198: data->massagers[event_name] =
On 2017/03/02 at 17:19:26, Devlin wrote:
> On 2017/03/01 22:33:35, jbroman wrote:
> > nit: could use this if you prefer (slightly shorter):
> > 
> > data->massagers[event_name].Reset(context->GetIsolate(), massager);
> 
> Sure, done.
> 
> I sometimes wonder which is faster under the hood, though - the former could
in theory be optimized by the compiler to construct the new global in-place in
the map with the proper value, the shorter Reset() version means that we first
have to construct an empty global, then call reset on it to initialize it to the
proper state.  It seems, to me, like that would be slower.
> 
> Of course, all of this is basically setting a pointer, so... not that
important.  But interesting to think about. :)

From a language perspective, the compiler is required to call the assignment
operator here (and the STL called the default constructor earlier). After
inlining, Reset() actually has slightly less code (e.g. the move approach
requires writing the destructor for Persistent Base, etc.). That said, I'd
expect a compiler to generate nearly identical code (which calls the function to
make a global handle and then sets a pointer) either way, so I think you should
feel free to choose whichever looks better. :)

[lazyboy, 2017-03-03 21:52:19]

lgtm

https://codereview.chromium.org/2727583004/diff/60001/extensions/renderer/api...
File extensions/renderer/api_event_handler_unittest.cc (right):

https://codereview.chromium.org/2727583004/diff/60001/extensions/renderer/api...
extensions/renderer/api_event_handler_unittest.cc:810: // Dispatch the event.
Can we also add a test case where we don't (re)dispatch the event at all? (i.e.
end the test after line 808). To see we handle that case gracefully.

[Devlin, 2017-03-03 22:20:09]

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

[commit-bot: I haz the power, 2017-03-03 22:20:58]

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, 2017-03-03 22:30:27]

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

[commit-bot: I haz the power, 2017-03-03 22:30:28]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Devlin, 2017-03-03 23:41:02]

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

[commit-bot: I haz the power, 2017-03-03 23:41:14]

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, 2017-03-04 00:33:27]

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

[commit-bot: I haz the power, 2017-03-04 00:33:28]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-03-04 00:59:13]

https://codereview.chromium.org/2727583004/diff/60001/extensions/renderer/api...
File extensions/renderer/api_event_handler_unittest.cc (right):

https://codereview.chromium.org/2727583004/diff/60001/extensions/renderer/api...
extensions/renderer/api_event_handler_unittest.cc:810: // Dispatch the event.
On 2017/03/03 21:52:18, lazyboy wrote:
> Can we also add a test case where we don't (re)dispatch the event at all?
(i.e.
> end the test after line 808). To see we handle that case gracefully.

Sure, done.

[Devlin, 2017-03-04 00:59:18]

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

[Devlin, 2017-03-04 00:59:19]

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

[commit-bot: I haz the power, 2017-03-04 00:59:29]

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, 2017-03-04 01:03:09]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1488589158529920,
"parent_rev": "ae57f580d05f5d8165294613c0774d5cfc925b50", "commit_rev":
"f9cdb557485c0186a1f4520c966628736465b84c"}

[commit-bot: I haz the power, 2017-03-04 01:04:54]

Description was changed from

==========
[Extensions Bindings] Add a registerEventArgumentMassager

Some custom hooks need to modify ("massage") the arguments originally
passed from the browser. This can be used to hide implementation
details, introduce an unserializable concept (like a file handler), or
add extra parameters (like a callback).

Add support for this by extending the APIBindingBridge with a
RegisterEventArgumentMassager method (exposed as
registerEventArgumentMassager in the bindings), and extend
APIEventHandler to support these.

BUG=653596
==========

to

==========
[Extensions Bindings] Add a registerEventArgumentMassager

Some custom hooks need to modify ("massage") the arguments originally
passed from the browser. This can be used to hide implementation
details, introduce an unserializable concept (like a file handler), or
add extra parameters (like a callback).

Add support for this by extending the APIBindingBridge with a
RegisterEventArgumentMassager method (exposed as
registerEventArgumentMassager in the bindings), and extend
APIEventHandler to support these.

BUG=653596

Review-Url: https://codereview.chromium.org/2727583004
Cr-Commit-Position: refs/heads/master@{#454735}
Committed:
https://chromium.googlesource.com/chromium/src/+/f9cdb557485c0186a1f4520c9666...
==========

[commit-bot: I haz the power, 2017-03-04 01:04:55]

Committed patchset #4 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/f9cdb557485c0186a1f4520c9666...