w3c / encrypted-media

Encrypted Media Extensions
https://w3c.github.io/encrypted-media/
Other
180 stars 80 forks source link

Clarify explicitly when promises are to be resolved #461

Closed jrummell-chromium closed 2 months ago

jrummell-chromium commented 4 years ago

Currently the spec notes "Since promise handlers are queued as microtasks, these will be executed ahead of any events queued by the preceding steps" in several places. For example, in generateRequest:

  1. Queue a task to run the following steps:
    1. If any of the preceding steps failed, reject promise with a new DOMException whose name is the appropriate error name.
    2. Set the sessionId attribute to session id.
    3. Set this object's callable value to true.
    4. Run the Queue a "message" Event algorithm on the session, providing message type and message.
    5. Resolve promise.

NOTE: Since promise handlers are queued as microtasks, these will be executed ahead of any events queued by the preceding steps.

To make the spec clearer, it would be better to indicate exactly when the promise is resolved, compared to other steps. The example above would switch 4 and 5 to become:

  1. Resolve promise.
  2. Run the Queue a "message" Event algorithm on the session, providing message type and message.

And then the note is not needed, as it is now clear that the promise should be resolved before the "message" event is generated (and does not depend on promise handlers being implemented as microtasks). This also appears to be the suggestion in #19 (comment) although it was switched back when addressing #14.

Resolving the promise before firing an event is used in other specs (e.g. Presentation API).

There is a potential problem with this in close(). Currently the attribute closed promise is resolved before the close() method promise is resolved. This will break the WPT tests for playback-temporary-events as it checks that the attribute promise is resolved before the close() method promise is resolved.

joeyparrish commented 3 months ago

NOTE: Since promise handlers are queued as microtasks, these will be executed ahead of any events queued by the preceding steps.

To make the spec clearer, it would be better to indicate exactly when the promise is resolved, compared to other steps. The example above would switch 4 and 5 to become:

  1. Resolve promise.
  2. Run the Queue a "message" Event algorithm on the session, providing message type and message.

And then the note is not needed, as it is now clear that the promise should be resolved before the "message" event is generated (and does not depend on promise handlers being implemented as microtasks).

I don't think this matters so much. It's arbitrary, IMO. That promises are microtasks is not something we depend on, so much as a fact of the web environment. (I recall this being part of a low-level spec for the web, but I haven't been able to find a reference to back this up. I could be wrong.)

The introduction of Promises into this spec happened at a time when few of us were deeply familiar with Promises and their underlying details, though, so I can understand why there has been some confusion.

It's easy to swap those steps, and it shouldn't hurt anything.

Resolving the promise before firing an event is used in other specs (e.g. Presentation API).

I'm all for consistency.

There is a potential problem with this in close(). Currently the attribute closed promise is resolved before the close() method promise is resolved. This will break the WPT tests for playback-temporary-events as it checks that the attribute promise is resolved before the close() method promise is resolved.

We probably could and should fix wpt not to care which order those two promises are resolved in, so long as they are in fact both resolved. They should be equivalent promises for all intents and purposes.


So I'll flip the order of those two steps, and I'll work on a PR for WPT to make the test more flexible.

joeyparrish commented 3 months ago

EME spec PR to reorder steps: https://github.com/w3c/encrypted-media/pull/538

WPT PR to make the test more flexible WRT closed: https://github.com/web-platform-tests/wpt/pull/46268 (Edit: now merged)