-
Notifications
You must be signed in to change notification settings - Fork 3.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DOM: Observable EventTarget integration 1/N #43455
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wpt-pr-bot
approved these changes
Nov 30, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The review process for this patch is being conducted in the Chromium project.
chromium-wpt-export-bot
force-pushed
the
chromium-export-cl-5073394
branch
from
November 30, 2023 17:01
4ebcda9
to
3f55340
Compare
chromium-wpt-export-bot
force-pushed
the
chromium-export-cl-5073394
branch
from
November 30, 2023 18:33
3f55340
to
cac47fa
Compare
chromium-wpt-export-bot
force-pushed
the
chromium-export-cl-5073394
branch
8 times, most recently
from
December 7, 2023 16:24
7cccec4
to
830dd8b
Compare
This CL implements "limited" and "leaky" EventTarget integration with the Observable API. See below for what "limited" and "leaky" mean. Concretely, this involves introducing the `on()` method to the EventTarget interface, so that all EventTargets can return Observables that listen for events. This is the part that really makes Observables a "better addEventListener()". This is the first instance of a natively-constructed Observable, as opposed to a JS-constructed Observable. This means the subscription callback passed to the Observable constructor is not just a JS callback function with user-defined code, but instead is a C++ delegate class, called `SubscribeDelegate` which has its first concrete implementation provided by EventTarget (in event_target.cc). The concrete implementation of this interface that this CL introduces, adds an event listener to the given EventTarget, upon subscription. The events are forwarded to the Subscriber's `next()` method. This is what unlocks more ergonomic event handling with the composable Observable primitive and all of its (coming) operators. 1. The EventTarget integration is considered "limited" because we do not support any of the `AddEventListenerOptions` yet, as of this CL. A subsequent CL will add support for a more restricted version of the `AddEventListenerOptions`, called `ObservableEventListenerOptions`, which does not include a `once` option, or an `AbortSignal`, since Observable operators and subscription is responsible for managing those aspects. Concretely, an `ObservableEventListenerOptions` will resolve to an `AddEventListenerOptionsResolved` accordingly. See: - WICG/observable#66 - WICG/observable#67 - WICG/observable#65 2. The EventTarget integration is considered "leaky" as of this CL, because there is currently no way to remove an event listener added by an EventTarget-vended Observable. This will come in a subsequent CL, which will pass the test that is currently failing in this CL. See WICG/observable#75 for discussion about tying the subscription termination to removing an event listener. From a technical perspective, this is pretty easy — it involves adding an abort algorithm to `Subscriber#signal` (which has already been wired up properly by now!) that removes the given per-Subscription `ObservableEventListener` NativeEventListener from the associated EventTarget. That implementation has already been sketched out in https://crrev.com/c/4262153 and the design doc. It will included in a follow-up CL, to reduce the complexity of this one. For WPTs: Co-authored-by: [email protected] [email protected] Bug: 1485981 Change-Id: Iafeddb0894b8eed2be1d95c181fc44d7650c0d47 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5073394 Reviewed-by: Mason Freed <[email protected]> Commit-Queue: Dominic Farolino <[email protected]> Cr-Commit-Position: refs/heads/main@{#1237501}
chromium-wpt-export-bot
force-pushed
the
chromium-export-cl-5073394
branch
from
December 14, 2023 14:34
830dd8b
to
e25da28
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This CL implements "limited" and "leaky" EventTarget integration with
the Observable API. See below for what "limited" and "leaky" mean.
Concretely, this involves introducing the
on()
method to theEventTarget interface, so that all EventTargets can return Observables
that listen for events. This is the part that really makes Observables a
"better addEventListener()".
This is the first instance of a natively-constructed Observable, as
opposed to a JS-constructed Observable. This means the subscription
callback passed to the Observable constructor is not just a JS callback
function with user-defined code, but instead is a C++ delegate class,
called
SubscribeDelegate
which has its first concrete implementationprovided by EventTarget (in event_target.cc). The concrete
implementation of this interface that this CL introduces, adds an event
listener to the given EventTarget, upon subscription. The events are
forwarded to the Subscriber's
next()
method. This is what unlocksmore ergonomic event handling with the composable Observable primitive
and all of its (coming) operators.
support any of the
AddEventListenerOptions
yet, as of this CL. Asubsequent CL will add support for a more restricted version of the
AddEventListenerOptions
, calledObservableEventListenerOptions
,which does not include a
once
option, or anAbortSignal
, sinceObservable operators and subscription is responsible for managing those
aspects. Concretely, an
ObservableEventListenerOptions
willresolve to an
AddEventListenerOptionsResolved
accordingly. See:AddEventListenerOptions
? WICG/observable#66ObservableEventListenerOptions
WICG/observable#67because there is currently no way to remove an event listener added by
an EventTarget-vended Observable. This will come in a subsequent CL,
which will pass the test that is currently failing in this CL. See
How to removeEventListener? WICG/observable#75 for discussion about
tying the subscription termination to removing an event listener.
From a technical perspective, this is pretty easy — it involves adding
an abort algorithm to
Subscriber#signal
(which has already been wiredup properly by now!) that removes the given per-Subscription
ObservableEventListener
NativeEventListener from the associatedEventTarget. That implementation has already been sketched out in
https://crrev.com/c/4262153 and the design doc. It will included in a
follow-up CL, to reduce the complexity of this one.
For WPTs:
Co-authored-by: [email protected]
R=[email protected]
Bug: 1485981
Change-Id: Iafeddb0894b8eed2be1d95c181fc44d7650c0d47
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5073394
Reviewed-by: Mason Freed <[email protected]>
Commit-Queue: Dominic Farolino <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1237501}