[Extensions] Introduce bindingUtil.registerEventDispatchHandler() [chromium/src : main]
0 views
Skip to first unread message
Andrea Orru (Gerrit)
Jun 24, 2026, 1:59:53 PM (6 days ago) Jun 24
to Devlin Cronin, Chromium LUCI CQ, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Attention needed from Devlin Cronin
Andrea Orru voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
Attention is currently required from:
- Devlin Cronin
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devlin Cronin (Gerrit)
Jun 24, 2026, 7:52:27 PM (6 days ago) Jun 24
to Andrea Orru, Devlin Cronin, Chromium LUCI CQ, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Attention needed from Andrea Orru
Devlin Cronin added 4 comments![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 4 (Latest):
Devlin Cronin . resolved
Thanks, Andrea! LGTM % nits.
I was originally wondering if there was a way we could just have this on WebRequestEvent itself (in web_request_event.js), but I didn't see a very clean one.
File extensions/renderer/bindings/api_event_handler.cc
Line 138, Patchset 4 (Latest):
v8::Isolate* isolate = v8::Isolate::GetCurrent();Devlin Cronin . unresolved
nit: maybe pass in Isolate to avoid re-fetching it?
Line 316, Patchset 4 (Latest):
DCHECK(on_dispatched_callback.IsEmpty())Devlin Cronin . unresolved
nit: prefer CHECKs in new code
File extensions/renderer/bindings/api_event_handler_unittest.cc
Line 935, Patchset 4 (Latest):
"[\"first\",\"second\"]",Devlin Cronin . unresolved
prefer string literal
Related details
Attention is currently required from:
- Andrea Orru
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devlin Cronin (Gerrit)
Jun 24, 2026, 8:22:44 PM (6 days ago) Jun 24
to Andrea Orru, Devlin Cronin, Chromium LUCI CQ, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Attention needed from Andrea Orru
Devlin Cronin voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
Attention is currently required from:
- Andrea Orru
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrea Orru (Gerrit)
Jun 24, 2026, 8:42:18 PM (6 days ago) Jun 24
to Devlin Cronin, Chromium LUCI CQ, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Attention needed from Andrea Orru
Andrea Orru voted and added 3 comments![]( "Open in Gerrit")
Votes added by Andrea Orru
| Commit-Queue | +2 |
3 comments
File extensions/renderer/bindings/api_event_handler.cc
nit: maybe pass in Isolate to avoid re-fetching it?
Andrea Orru
Done
nit: prefer CHECKs in new code
Andrea Orru
Done
File extensions/renderer/bindings/api_event_handler_unittest.cc
Line 935, Patchset 4:
"[\"first\",\"second\"]",Devlin Cronin . resolved
Andrea Orruprefer string literal
Done
Related details
Attention is currently required from:
- Andrea Orru
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Jun 24, 2026, 9:24:33 PM (6 days ago) Jun 24
to Andrea Orru, Devlin Cronin, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
4 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: extensions/renderer/bindings/api_event_handler_unittest.cc
Insertions: 1, Deletions: 1.
The diff is too large to show. Please review the diff.
```
```
The name of the file: extensions/renderer/bindings/api_event_handler.cc
Insertions: 6, Deletions: 5.
The diff is too large to show. Please review the diff.
```
Change information
Commit message:
[Extensions] Introduce bindingUtil.registerEventDispatchHandler()
This change is part of a series that will eventually refactor out the
sub-event naming hack (e.g. webRequest.onBeforeRequest/sN).
Later CLs in this series will move blocking webRequest dispatch from
once per listener to once per renderer context: the browser will send a
single event per context carrying the union of the matching listeners'
options, and the renderer will then match the request against each of
its own listeners. Each listener callback will receive a copy of the
details stripped to the options it asked for. The renderer will report
the blocking responses to the browser.
This requires the renderer to take over the event's dispatch entirely
and to never invoke listeners through the emitter at all (because the
emitter's plain fan-out runs every listener callback with the same
arguments and cannot do per-listener matching and tailoring).
The existing APIEventHandler hook for influencing dispatch is the
argument massager, which rewrites an event's arguments and hands them
back to the emitter via a curried dispatch function. However, it fits
the case poorly: the curried dispatch function is unused (the renderer
fans out itself), and because FireEventInContext() pushes the event's
EventFilteringInfo onto the emitter expecting dispatch to pop it, a
massager that never calls dispatch leaks one pending filter entry per
event (note that EventFilteringInfo is always empty for webRequest
events).
Add a dedicated primitive instead. RegisterEventDispatchHandler()
registers a function that owns the event's dispatch:
FireEventInContext() hands it the raw arguments and returns, bypassing
the emitter (which, for such events, only transports the listener
registration to the browser) and the event-filter bookkeeping entirely.
Unlike a massager it receives no dispatch function and no filter is
pushed, so there is nothing to leak. Exposed to JS bindings as
`bindingUtil.registerEventDispatchHandler()`.
Design doc: go/no-webrequest-subevent
Bug: 494684626
Change-Id: I502f818ca8d0fb25e9670378ad50105d57bafac5
Reviewed-by: Devlin Cronin <rdevlin...@chromium.org>
Commit-Queue: Andrea Orru <andre...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1652112}
Files:
- M extensions/renderer/bindings/api_binding_js_util.cc
- M extensions/renderer/bindings/api_binding_js_util.h
- M extensions/renderer/bindings/api_event_handler.cc
- M extensions/renderer/bindings/api_event_handler.h
- M extensions/renderer/bindings/api_event_handler_unittest.cc
- M extensions/renderer/bindings/event_emitter.cc
- M extensions/renderer/bindings/event_emitter.h
Change size: M
Delta: 7 files changed, 167 insertions(+), 14 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Devlin Cronin
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages