-
Notifications
You must be signed in to change notification settings - Fork 319
Making functional events simpler #1199
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
Conversation
docs/index.bs
Outdated
Specifications *may* define an algorithm |callbackSteps| where the corresponding <a>functional event</a> can be created and fired with specification specific objects. The algorithm is passed <var ignore>globalObject</var> (a {{ServiceWorkerGlobalScope}} object) at which it *may* fire its <a>functional events</a>. This algorithm is called on a <a>task</a> <a lt="queue a task">queued</a> by <a>Handle Functional Event</a> algorithm. | ||
|
||
Note: See an <a href="https://notifications.spec.whatwg.org/#activating-a-notification">example</a> hook defined in <a biblio data-biblio-type="informative" lt="notifications">Notifications API</a>. | ||
To request a <a>functional event</a> dispatch to the [=service worker registration/active worker=] of a [=/service worker registration=], specifications *may* invoke <a>fire a functional event</a>. |
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.
Nit: SW uses some PascalCase with intervening space for algorithm names. HTML, Fetch, and many others use lower case letters. Streams use PascalCase.. I always wanted to discuss if we better change our naming practice for algorithms' name?
docs/index.bs
Outdated
@@ -3105,35 +3101,57 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe | |||
</section> | |||
|
|||
<section algorithm> | |||
<h3 id="handle-functional-event-algorithm"><dfn export>Handle Functional Event</dfn></h3> | |||
<h3 id="fire-functional-event-algorithm"><dfn export>Fire a Functional Event</dfn></h3> |
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.
Nit: Fire Functional Event for consistency, but opened to any suggestion for naming practice.
docs/index.bs
Outdated
:: |registration|, a [=/service worker registration=] | ||
:: |callbackSteps|, an algorithm | ||
:: |initializationSteps|, optional steps to initialise |event|, constructed from |eventConstructor| |
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.
It seems we should explicitly provide |createdEvent| as an argument to |initializationSteps| so the callers will set up the properties to it when needed. Just as you did with |dispatchedEvent| to |postDispatchSteps|.
docs/index.bs
Outdated
: propertyName | ||
:: value | ||
: anotherPropertyName | ||
:: anotherValue |
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.
It seems callers should give us an |initializationSteps| where they set up the properties themselves (rather than providing the properties to Fire Functional Event as an argument)?
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.
I did it this way to match invocations of https://dom.spec.whatwg.org/#concept-event-fire, but I'm not against making it sub-steps.
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.
I thought the |initializationSteps| would be a place where the properties can be initialized. But checking out the DOM spec link that you shared, I'm fine with that too.
@jakearchibald, I'm happy with the changes this PR's addressing. When we get to the final snapshot, backporting to V1 seems nice. |
docs/index.bs
Outdated
|
||
: Input | ||
:: |event|, an {{ExtendableEvent}} object | ||
:: |eventName|, a DOMString |
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.
I think it needs to be just a string for internal algorithms.
ab1a929
to
dae02eb
Compare
@jungkees I've rebased this and also updated the v1 spec. I think it's best to stick with property initialisation rather than a complete set of steps for now, just to be similar to "fire an event". Happy to change it later if that turns out to be bad. |
docs/index.bs
Outdated
Specifications *may* define an algorithm |callbackSteps| where the corresponding <a>functional event</a> can be created and fired with specification specific objects. The algorithm is passed <var ignore>globalObject</var> (a {{ServiceWorkerGlobalScope}} object) at which it *may* fire its <a>functional events</a>. This algorithm is called on a <a>task</a> <a lt="queue a task">queued</a> by <a>Handle Functional Event</a> algorithm. | ||
|
||
Note: See an <a href="https://notifications.spec.whatwg.org/#activating-a-notification">example</a> hook defined in <a biblio data-biblio-type="informative" lt="notifications">Notifications API</a>. | ||
To request a [=functional event=] dispatch to the [=service worker registration/active worker=] of a [=/service worker registration=], specifications *may* invoke [=Fire Functional Event=]. |
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.
This isn't an issue this PR brought. Would it be better to change the conformance requirement language to should or even must? I think there's a chance that some prospective specs might want to define their own steps intentionally. So, should seems to be a good requirement for this?
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.
I went for 'should', as there may be reasons why another spec needs to do it manually
LGTM with a comment on the conformance requirement language. It'd be great if you could make PRs to the call sites :). |
This is a fix for #1196.
I took inspiration from fire an event.
This should clear up the confusion around realms and it make it easier for other specs.
@jungkees if you're happy with this, I'll port this to V1 and submit PRs to the specs that fire functional events.
Preview | Diff