-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Queue a task to resolve createImageBitmap #11327
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
fixes whatwg#5329 fixes whatwg#10611 We went the "create our own task-queue" way, which seems to be the easiest here. Now, it's expected that early errors reject synchronously, while everything rejecting or resolving "in parallel" is ran from a task. This matches Firefox's behavior. WebKit only matches the synchronous rejections of any source and the async settlement of Blob. Chrome only matches the synchronous rejections of any source and the async settlement of Blob in a Window context and makes everything async in a Worker. We also fixed a missing </p> nearby, and ran The Great Rewrapper on the affected paragraph.
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 seems good to me, with a nit about the task source.
@annevk would you be willing to express implementer interest from WebKit in accordance with your comments on #10611 (comment) ?
@mfreed7 do you know who might own this code in Chromium?
source
Outdated
|
||
<dd> | ||
<p>This <span>task source</span> is used solely by <span | ||
data-x="dom-createImageBitmap">createImageBitmap</span>.</p> | ||
</dd> |
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.
If we want to use a new task source for this, defining it inline like https://html.spec.whatwg.org/#canvas-blob-serialisation-task-source might be better.
I couldn't find any evidence of what task source Firefox uses at https://searchfox.org/mozilla-central/source/dom/canvas/ImageBitmap.cpp ... maybe @smaug---- can help us understand the code?
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.
Oh , I missed this exists and only had the recent rendering task source example in mind.
Note that I went on creating a new task source instead of reusing the DOM manipulation one because here we can also be called from a worker context, and I thought the DOM manipulation task source would be inappropriate there.
Also, if we do define it as being its own task source, I'm not sure it's observable whether it actually ends in a "default" task source with many other calls or not. They'd still follow their own call order which is what we want I think, and if other task sources go in between that's also acceptable.
Following the suggestion on the PR.
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 removal of "in parallel" seems okay given that there's a custom task source, which in practice gives the same amount of freedoms (while not having the bug of settling a promise outside of a task).
Following the comments from Anne, this moves the dfn of imagebitmap before we use it in its own paragraph. This also renames the variable 'p' to 'promise'.
We originally did touch that section and thus did some fixup while here. While that change got moved somewhere else, we keep the fixups and make them even better.
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.
Nice!
I'm still a bit unclear on the implementer interest status here. We know Gecko is interested because they implement this, but @Kaiido put down WebKit as interested and I'm not sure we've gotten a clear affirmative statement in that regard. |
Anne did, not me. We still need to hear from Chrome though, at least to be sure they don't oppose. |
I think it's very unlikely Chromium will oppose, but yeah, let's give @mfreed7 a few days in case he wants to take a closer look. If not, I'll feel comfortable speaking for Chromium in saying we're OK with it. |
Hmm, maybe @shaseley might have an opinion here? I would not be the owner of this code, so I'd defer to other experts. |
No objections from me on the scheduling side. My only concern is potentially breaking existing sites, but given Gecko implements it this way, sites are already having to handle this. And I think being consistent makes sense, so aligning on Gecko's behavior LGTM. |
Tests for whatwg/html#11327 See whatwg/html#10611 where it was resolved that Firefox behavior (always resolving the promise in a task) was the better solution. These tests currently pass in Firefox and fail in Chrome and Safari.
fixes #5329
fixes #10611
We went the "create our own task-queue" way, which seems to be the easiest here.
Now, it's expected that early errors reject synchronously, while everything rejecting or resolving "in parallel" is ran from a task. This matches Firefox's behavior.
Chrome and WebKit only match the synchronous rejections of any source and the async settlement of Blob.
(See WHATWG Working Mode: Changes for more details.)
/imagebitmap-and-animations.html ( diff )
/webappapis.html ( diff )