Skip to content

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

Merged
merged 4 commits into from
May 26, 2025

Conversation

Kaiido
Copy link
Member

@Kaiido Kaiido commented May 22, 2025

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 )

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.
Copy link
Member

@domenic domenic left a 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>
Copy link
Member

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?

Copy link
Member Author

@Kaiido Kaiido May 22, 2025

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.
Copy link
Member

@annevk annevk left a 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).

Kaiido added 2 commits May 22, 2025 17:16
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.
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@domenic
Copy link
Member

domenic commented May 23, 2025

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.

@Kaiido
Copy link
Member Author

Kaiido commented May 23, 2025

but @Kaiido put down WebKit as interested

Anne did, not me. We still need to hear from Chrome though, at least to be sure they don't oppose.

@domenic
Copy link
Member

domenic commented May 23, 2025

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.

@mfreed7
Copy link
Contributor

mfreed7 commented May 23, 2025

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.

@shaseley
Copy link

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.

@domenic domenic merged commit e288a0e into whatwg:main May 26, 2025
2 checks passed
domenic pushed a commit to web-platform-tests/wpt that referenced this pull request Jun 4, 2025
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

createImageBitmap's Promise resolving createImageBitmap doesn't queue a task to resolve its promises
5 participants