Plumb listenerIDs correctly for events that clobber chrome.Event.prototype.dispatch.

Make eventArgumentMassagers asynchronous so custom bindings don't need to override chrome.Event.prototype.dispatch.


BUG=121479
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=146038

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=146289

[koz (OOO until 15th September), 2012-07-03 04:29:42]

This change makes dispatchJSON() dispatch events asynchronously, which some APIs
bindings are doing now by installing their own dispatch method.

This fixes two things:

1. ListenerIDs don't get plumbed properly in those cases.
2. The debug mode event validation error reporting doesn't occur in those
asynchronous cases.

[benwells, 2012-07-03 07:38:18]

lgtm.

I don't know enough about this to know if it matters but Event.dispatch is also
overridden by input.ime_custom_bindings.js, so you might want to look at that
and see if it is also a problem.

https://chromiumcodereview.appspot.com/10704073/diff/5001/chrome/renderer/res...
File chrome/renderer/resources/extensions/event.js (right):

https://chromiumcodereview.appspot.com/10704073/diff/5001/chrome/renderer/res...
chrome/renderer/resources/extensions/event.js:253: chromeHidden.Event.dispatch =
function(name, args) {
Why do we have this function as well as dispatchJSON? This only seems to be used
from a unit test.

[not at google - send to devlin, 2012-07-03 07:47:04]

drive-by of DCHECK stuff

https://chromiumcodereview.appspot.com/10704073/diff/5001/chrome/renderer/ext...
File chrome/renderer/extensions/extension_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/10704073/diff/5001/chrome/renderer/ext...
chrome/renderer/extensions/extension_dispatcher.cc:224: error_message =
*v8::String::AsciiValue(args[1]);
Is there a way to get a v8 stack trace in here?

https://chromiumcodereview.appspot.com/10704073/diff/5001/chrome/renderer/ext...
chrome/renderer/extensions/extension_dispatcher.cc:704:
scoped_ptr<NativeHandler>(new DebugNativeHandler()));
drive-by: DCHECK is in base/logging.h, so perhaps call this module "logging"?

https://chromiumcodereview.appspot.com/10704073/diff/5001/chrome/renderer/res...
File chrome/renderer/resources/extensions/event.js (right):

https://chromiumcodereview.appspot.com/10704073/diff/5001/chrome/renderer/res...
chrome/renderer/resources/extensions/event.js:243: DCHECK(false,
result.validationErrors);
Either incorporate the condition into the DCHECK, or expose a NOTREACHED?

[benwells, 2012-07-03 23:34:28]

https://chromiumcodereview.appspot.com/10704073/diff/5001/chrome/renderer/ext...
File chrome/renderer/extensions/extension_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/10704073/diff/5001/chrome/renderer/ext...
chrome/renderer/extensions/extension_dispatcher.cc:221: bool check_value =
args[0]->BooleanValue();
Oh yeah, forgot to ask: Does this handle passing non-booleans into DCHECK? E.g.
if you do DCHECK(result) to make sure you have a non-null result.

[koz (OOO until 15th September), 2012-07-05 04:38:31]

zork, is it okay to delete that validation stuff in
experimental.input.ime_custom_bindings.js? Parts of it are dead now anyway.

https://chromiumcodereview.appspot.com/10704073/diff/5001/chrome/renderer/ext...
File chrome/renderer/extensions/extension_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/10704073/diff/5001/chrome/renderer/ext...
chrome/renderer/extensions/extension_dispatcher.cc:221: bool check_value =
args[0]->BooleanValue();
On 2012/07/03 23:34:29, benwells wrote:
> Oh yeah, forgot to ask: Does this handle passing non-booleans into DCHECK?
E.g.
> if you do DCHECK(result) to make sure you have a non-null result.

This will convert any JS values to bools using the same rules that JS does.

https://chromiumcodereview.appspot.com/10704073/diff/5001/chrome/renderer/ext...
chrome/renderer/extensions/extension_dispatcher.cc:224: error_message =
*v8::String::AsciiValue(args[1]);
On 2012/07/03 07:47:04, kalman wrote:
> Is there a way to get a v8 stack trace in here?

Yes, it turns out. Done.

https://chromiumcodereview.appspot.com/10704073/diff/5001/chrome/renderer/ext...
chrome/renderer/extensions/extension_dispatcher.cc:704:
scoped_ptr<NativeHandler>(new DebugNativeHandler()));
On 2012/07/03 07:47:04, kalman wrote:
> drive-by: DCHECK is in base/logging.h, so perhaps call this module "logging"?

Done.

https://chromiumcodereview.appspot.com/10704073/diff/5001/chrome/renderer/res...
File chrome/renderer/resources/extensions/event.js (right):

https://chromiumcodereview.appspot.com/10704073/diff/5001/chrome/renderer/res...
chrome/renderer/resources/extensions/event.js:243: DCHECK(false,
result.validationErrors);
On 2012/07/03 07:47:04, kalman wrote:
> Either incorporate the condition into the DCHECK, or expose a NOTREACHED?

Done.

https://chromiumcodereview.appspot.com/10704073/diff/5001/chrome/renderer/res...
chrome/renderer/resources/extensions/event.js:253: chromeHidden.Event.dispatch =
function(name, args) {
On 2012/07/03 07:38:18, benwells wrote:
> Why do we have this function as well as dispatchJSON? This only seems to be
used
> from a unit test.

Removed.

[Zachary Kuznia, 2012-07-05 08:30:12]

On 2012/07/05 04:38:31, koz wrote:
> zork, is it okay to delete that validation stuff in
> experimental.input.ime_custom_bindings.js? Parts of it are dead now anyway.

If the function throws an exception, or would otherwise fail, the C++ callback
needs to be invoked.  Otherwise, the ime library can't be informed that the
keystroke wasn't handled.  Additionally, it needs to make sure that it properly
propagates a return value of true or false back to C++, and if no value is
returned, false is sent.

As long as these cases are handled, it's ok to remove the validation.

[koz (OOO until 15th September), 2012-07-06 04:31:29]

On 2012/07/05 08:30:12, Zachary Kuznia wrote:
> On 2012/07/05 04:38:31, koz wrote:
> > zork, is it okay to delete that validation stuff in
> > experimental.input.ime_custom_bindings.js? Parts of it are dead now anyway.
> 
> If the function throws an exception, or would otherwise fail, the C++ callback
> needs to be invoked.  Otherwise, the ime library can't be informed that the
> keystroke wasn't handled.  Additionally, it needs to make sure that it
properly
> propagates a return value of true or false back to C++, and if no value is
> returned, false is sent.
> 
> As long as these cases are handled, it's ok to remove the validation.

Okay, cool. I've added a declarative option "maxListeners" for events to specify
the max number of listeners they support, and I've restructured the code so that
it doesn't duplicate the dispatch logic (which has been made somewhat more
complicated with the arrival of filtered events).

I've removed the validation stuff because if there are validation errors it will
crash the renderer anyway.

[Zachary Kuznia, 2012-07-06 06:05:48]

lgtm

On Fri, Jul 6, 2012 at 1:31 PM, <koz@chromium.org> wrote:

> On 2012/07/05 08:30:12, Zachary Kuznia wrote:
>
>> On 2012/07/05 04:38:31, koz wrote:
>> > zork, is it okay to delete that validation stuff in
>> > experimental.input.ime_custom_**bindings.js? Parts of it are dead now
>> anyway.
>>
>
>  If the function throws an exception, or would otherwise fail, the C++
>> callback
>> needs to be invoked.  Otherwise, the ime library can't be informed that
>> the
>> keystroke wasn't handled.  Additionally, it needs to make sure that it
>>
> properly
>
>> propagates a return value of true or false back to C++, and if no value is
>> returned, false is sent.
>>
>
>  As long as these cases are handled, it's ok to remove the validation.
>>
>
> Okay, cool. I've added a declarative option "maxListeners" for events to
> specify
> the max number of listeners they support, and I've restructured the code
> so that
> it doesn't duplicate the dispatch logic (which has been made somewhat more
> complicated with the arrival of filtered events).
>
> I've removed the validation stuff because if there are validation errors
> it will
> crash the renderer anyway.
>
>
https://chromiumcodereview.**appspot.com/10704073/<https://chromiumcodereview...
>

[benwells, 2012-07-06 06:25:56]

still lgtm, with nit.

https://chromiumcodereview.appspot.com/10704073/diff/7012/chrome/renderer/res...
File chrome/renderer/resources/extensions/input.ime_custom_bindings.js (right):

https://chromiumcodereview.appspot.com/10704073/diff/7012/chrome/renderer/res...
chrome/renderer/resources/extensions/input.ime_custom_bindings.js:17: result =
callback.apply(null, args);
Nit: use chrome.Event.prototype.dispathToListener, or don't clobber
dispatchToListener, as you see fit.

[koz (OOO until 15th September), 2012-07-06 07:31:20]

Looks like tts also overrides dispatch, so I fixed that, too.

https://chromiumcodereview.appspot.com/10704073/diff/7012/chrome/renderer/res...
File chrome/renderer/resources/extensions/input.ime_custom_bindings.js (right):

https://chromiumcodereview.appspot.com/10704073/diff/7012/chrome/renderer/res...
chrome/renderer/resources/extensions/input.ime_custom_bindings.js:17: result =
callback.apply(null, args);
On 2012/07/06 06:25:56, benwells wrote:
> Nit: use chrome.Event.prototype.dispathToListener, or don't clobber
> dispatchToListener, as you see fit.

Done.

[Matt Perry, 2012-07-09 20:36:47]

lgtm

https://chromiumcodereview.appspot.com/10704073/diff/15001/chrome/renderer/ex...
File chrome/renderer/extensions/extension_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/10704073/diff/15001/chrome/renderer/ex...
chrome/renderer/extensions/extension_dispatcher.cc:225: std::stringstream
error_message;
Streams are forbidden by the style guide. Use StringPrintf and/or += .

https://chromiumcodereview.appspot.com/10704073/diff/15001/chrome/renderer/re...
File chrome/renderer/resources/extensions/event.js (right):

https://chromiumcodereview.appspot.com/10704073/diff/15001/chrome/renderer/re...
chrome/renderer/resources/extensions/event.js:212: // callback is a
function(args, dispatch). args are the args we recieve from
receive*

[koz (OOO until 15th September), 2012-07-11 00:00:38]

https://chromiumcodereview.appspot.com/10704073/diff/15001/chrome/renderer/ex...
File chrome/renderer/extensions/extension_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/10704073/diff/15001/chrome/renderer/ex...
chrome/renderer/extensions/extension_dispatcher.cc:225: std::stringstream
error_message;
On 2012/07/09 20:36:47, Matt Perry wrote:
> Streams are forbidden by the style guide. Use StringPrintf and/or += .

Done.

https://chromiumcodereview.appspot.com/10704073/diff/15001/chrome/renderer/re...
File chrome/renderer/resources/extensions/event.js (right):

https://chromiumcodereview.appspot.com/10704073/diff/15001/chrome/renderer/re...
chrome/renderer/resources/extensions/event.js:212: // callback is a
function(args, dispatch). args are the args we recieve from
On 2012/07/09 20:36:47, Matt Perry wrote:
> receive*

Done.

[commit-bot: I haz the power, 2012-07-11 00:00:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/10704073/8004

[commit-bot: I haz the power, 2012-07-11 00:21:20]

Try job failure for 10704073-8004 (retry) on mac_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-07-11 00:52:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/10704073/3006

[commit-bot: I haz the power, 2012-07-11 02:42:00]

Change committed as 146038

[commit-bot: I haz the power, 2012-07-12 03:40:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/10704073/1028

[commit-bot: I haz the power, 2012-07-12 05:27:55]

Change committed as 146289