Skip to content

Allow doubles for ProgressEvent's loaded and total #394

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 1 commit into from
Mar 11, 2025

Conversation

domenic
Copy link
Member

@domenic domenic commented Jan 21, 2025

This allows specifications to use ProgressEvents while not necessarily giving away the exact number of bytes, for example by using numbers between 0 and 1. See discussion in webmachinelearning/writing-assistance-apis#15 for the concrete use case.

(See WHATWG Working Mode: Changes for more details.)


I wanted to get this PR up to judge implementer interest. It would be a decent bit cleaner if we all agreed this was reasonable and merged the change now, instead of having to float a patch in the relevant specs and thus causing all the databases to have two definitions of ProgressEvent.


Preview | Diff

@annevk
Copy link
Member

annevk commented Jan 21, 2025

I'll double check, in principle it seems fine. @smaug----?

domenic added a commit to webmachinelearning/writing-assistance-apis that referenced this pull request Jan 22, 2025
This allows specifications to use ProgressEvents while not necessarily giving away the exact number of bytes, while maintaining a granularity greater than just 1 part in 100. See discussion in webmachinelearning/writing-assistance-apis#15 for the concrete use case.
@domenic domenic force-pushed the double-progressevent branch from 48e8e0d to 5b4e883 Compare January 22, 2025 05:03
@smaug----
Copy link

smaug---- commented Jan 22, 2025

Should be fine.
Technically not backwards compatible, but I'd be a bit surprised if someone relied on the number -> long long conversion when calling the constructor.

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.

If you could ping me once Chromium has successfully shipped this I'd appreciate this and I'll be sure to align WebKit at that point.

@domenic
Copy link
Member Author

domenic commented Jan 23, 2025

Awesome. I'm hoping to delegate the implementation and tests to someone else so it might be a week or so, but once we have tests up I'll update this thread with a link to them and open other-browser bugs. And I'll ping again once we have it shipped.

domenic added a commit to webmachinelearning/writing-assistance-apis that referenced this pull request Jan 24, 2025
domenic added a commit to webmachinelearning/writing-assistance-apis that referenced this pull request Jan 24, 2025
domenic added a commit to webmachinelearning/writing-assistance-apis that referenced this pull request Jan 24, 2025
@smaug----
Copy link

Bug with a patch https://bugzilla.mozilla.org/show_bug.cgi?id=1943649 (webidl codegen for events is nice in this case)
Waiting for the tests before landing ;)

@domenic
Copy link
Member Author

domenic commented Mar 4, 2025

Should we merge this now, since all the boxes are checked? Or would we prefer to wait to see this land in Chromium to prove web-compatibility?

(I think that this one is so low-risk that there's no need to wait, personally. Unlike whatwg/webidl#1465 where waiting would be more prudent.)

@annevk
Copy link
Member

annevk commented Mar 4, 2025

I think it's okay to merge this around the same time the tests are merged.

@smaug----
Copy link

FWIW, I landed the patch to Nightly, waiting for the tests now ;)

@domenic
Copy link
Member Author

domenic commented Mar 7, 2025

We got approval to ship in Chromium now, so the tests should land very soon!

@domenic domenic merged commit 4a6401c into main Mar 11, 2025
2 checks passed
@domenic domenic deleted the double-progressevent branch March 11, 2025 01:01
@domenic
Copy link
Member Author

domenic commented Mar 11, 2025

The tests have now merged, so I went ahead and merged this spec PR. Thanks all!

@annevk
Copy link
Member

annevk commented Mar 19, 2025

@smaug---- I think your IDL-only change might be insufficient, unless bindings work rather differently in Gecko. At least I had to change some C++ to get tests to pass, which also made me realize that there's a couple more throwing cases now, but hopefully none that people run into in practice.

@smaug----
Copy link

As I explained on Matrix, in Gecko one can create the full C++ implementation out of a "normal" kind of Event interface..

sebsebmc added a commit to sebsebmc/servo that referenced this pull request Apr 13, 2025
github-merge-queue bot pushed a commit to servo/servo that referenced this pull request Apr 13, 2025
See: whatwg/xhr#394

Testing: WPT tests exist for this

Signed-off-by: Sebastian C <[email protected]>
ddbeck added a commit to ddbeck/web-features that referenced this pull request Jul 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants