w3c / webextensions

Charter and administrivia for the WebExtensions Community Group (WECG)
Other
578 stars 50 forks source link

Inconsistency: chrome.downloads.open() return value #528

Closed bershanskiy closed 4 months ago

bershanskiy commented 5 months ago

Update: Firefox and Chromium 123+ now both return Promise<void>, Safari does not support this API, but is willing to match.

Summary

Chromium treats chrome.downloads.open() as a sync operation, while Firefox returns Promise<void>.

Chromium bug

Details

In Chromium, has the following signature (from docs, from source):

void chrome.downloads.open(downloadId: number)

In Firefox, it returns a Promise (from MDN):

Promise<void> browser.downloads.open(downloadId: number)

Safari does not support downloads API, but is willing to use promise.

I believe that the function should return Promise<void> since opening a file is inherently an async operation. Chromium handles it async (code), but the declaration is sync (code). It is sync only if there is an error which is known right away like lack of proper permissions or user activation.

(Updated Feb 15, 2024)

xeenon commented 5 months ago

Safari does not currently support the downloads API. But I agree a promise makes sense here.

bershanskiy commented 4 months ago

@patrickkettner Would you be interested in resolving this difference?

patrickkettner commented 4 months ago

Appreciate the ping. I’ll dig in a bit. I am concerned about it being potentially a breaking change. I’ll see what the team thinks is plausible

On Sat, Feb 10, 2024 at 12:40 PM Anton Bershanskiy @.***> wrote:

@patrickkettner https://github.com/patrickkettner Would you be interested in resolving this difference?

— Reply to this email directly, view it on GitHub https://github.com/w3c/webextensions/issues/528#issuecomment-1937079966, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADRUBW6VOR5BATLEWGOIELYS6WKLAVCNFSM6AAAAABCHCNZYWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMZXGA3TSOJWGY . You are receiving this because you were mentioned.Message ID: @.***>

patrickkettner commented 4 months ago

Thanks for opening https://chromium-review.googlesource.com/c/chromium/src/+/5274756 ! Checking the existing extension usage, the change seems safe. I pinged a reviewer to get the change merged.

bershanskiy commented 4 months ago

The relevant change was merged (commit), Chromium 123+ will return Promise<void>.

bershanskiy commented 4 months ago

Since the inconsistency is resolved, I'm closing this issue.

We might want to change labels: remove "supportive: chrome" and add "implemented: chrome", "implemented: firefox", "implemented: edge".