Extension event binding: APIEventHandler works differently than event.js |
|||
Issue descriptionThe gn flag "is_chrome_branded" will cause the extension event dispatching being hooked to either APIEventHandler[1] or event.js[2]. Chrome/browser_tests with "is_chrome_branded=true" will use event.js. So it can wire the return value of the handler function (event.js#398). Chrome/browser_tests with "is_chrome_branded=false" will use APIEventHandler. So it ignores the return value of the handler function (api_event_handler.cc#91). The extensions' event handlers rarely have return values, but IME extension is an exception. Please refer to input.ime_custom_bindings.js#26. The IME extension API's browser tests (chrome/browser/chromeos/input_method/input_method_engine_browsertests.cc) have NOT covered the cases of returning "true", so the tests haven't failed yet (thus buried the issue). To be able to add more tests for IME extension API, APIEventHandler should NOT ignore the return value of the handler function. [1]: https://cs.chromium.org/chromium/src/extensions/renderer/bindings/api_event_handler.cc [2]: https://cs.chromium.org/chromium/src/extensions/renderer/resources/event.js
,
Jul 25
Sorry, this might be a false alarm as I found some code here: https://cs.chromium.org/chromium/src/extensions/renderer/bindings/event_emitter.cc?l=147&gsn=DispatchSync Will investigate why it isn't working.
,
Jul 25
Ah! Interesting. It looks like the IME custom bindings are the only caller that actually relies on the return result of the dispatch method passed into the argument massager, and there wasn't any test that exercised this behavior. I'll write up a CL to fix this for native bindings. Thanks for the report!
,
Aug 6
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d614235c00db75b9ab192974fcbd16755a557471 commit d614235c00db75b9ab192974fcbd16755a557471 Author: Devlin Cronin <rdevlin.cronin@chromium.org> Date: Mon Aug 06 01:16:07 2018 [Extensions Binding] Return the result of argument massager event dispatch Event argument massagers are passed a function to dispatch the event. We need to ensure the arguments are returned to the caller, similar to how calling dispatch() on the event itself would be treated. This is necessary for the input IME custom bindings. Wire this up, and add a unit test for the same. Bug: 867310 Change-Id: I48841d60cb4940a15cd439ce6c4ea156eeeeaab6 Reviewed-on: https://chromium-review.googlesource.com/1150410 Commit-Queue: Devlin <rdevlin.cronin@chromium.org> Reviewed-by: Jeremy Roman <jbroman@chromium.org> Cr-Commit-Position: refs/heads/master@{#580790} [modify] https://crrev.com/d614235c00db75b9ab192974fcbd16755a557471/extensions/renderer/bindings/api_event_handler.cc [modify] https://crrev.com/d614235c00db75b9ab192974fcbd16755a557471/extensions/renderer/bindings/api_event_handler_unittest.cc [modify] https://crrev.com/d614235c00db75b9ab192974fcbd16755a557471/extensions/renderer/bindings/event_emitter.cc [modify] https://crrev.com/d614235c00db75b9ab192974fcbd16755a557471/extensions/renderer/bindings/event_emitter.h
,
Aug 31
|
|||
►
Sign in to add a comment |
|||
Comment 1 by shuchen@chromium.org
, Jul 25