chore(bidi): add support for browser.setDownloadBehavior#39311
chore(bidi): add support for browser.setDownloadBehavior#39311hbenl wants to merge 1 commit intomicrosoft:mainfrom
Conversation
This comment has been minimized.
This comment has been minimized.
hbenl
force-pushed
the
setDownloadBehavior
branch
from
1403bb3 to
795cd70
Compare
This comment has been minimized.
This comment has been minimized.
hbenl
force-pushed
the
setDownloadBehavior
branch
from
795cd70 to
9ffca5d
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Note that I added the test "should throw if browser dies" to the expectations file because of an issue with the current implementation of |
This comment has been minimized.
This comment has been minimized.
| return; | ||
|
|
||
| this._browserContext._browser._downloadCreated(originPage, event.navigation, event.url, event.suggestedFilename); | ||
| this._browserContext._browser._downloadCreated(originPage, event.navigation, event.url, event.suggestedFilename, true); |
There was a problem hiding this comment.
Why is Bidi different here? If event.navigation is not unique, maybe we can just combine it with with even.timestamp to produce a UUID or have a mapping in the Bidi client Download->UUID ?
There was a problem hiding this comment.
event.navigation is unique and is still being used to identify the download.
The difference with Bidi is that the browsers use event.suggestedFilename as the filename for writing the download to disk (whereas with other protocols they use the ID that is passed to _downloadCreated() as the filename).
There was a problem hiding this comment.
How does it work if the user has two downloads with the same suggestedFilename, e.g. the user clicks twice on a link with download="foo.zip" attribute? If Firefox already modifies the suggested file name and guarantees its uniqueness, we can probably use it as the uuid or in that case we still need to change the logic of the download to use a different id for canceling download? Is there a plan to change the behavior to save the downloads with a guid file name and returning unmodified suggested file name in Bidi?
There was a problem hiding this comment.
e.g. the user clicks twice on a link with download="foo.zip" attribute
In that case the second download would get foo(1).zip as the suggestedFilename. So suggestedFilename is unique in the download folder at the time when the download is started but if a download gets cancelled and then another one started, they could get the same suggestedFilename.
If Firefox already modifies the suggested file name and guarantees its uniqueness, we can probably use it as the uuid or in that case we still need to change the logic of the download to use a different id for canceling download?
Cancelling downloads hasn't been added to the specification yet (w3c/webdriver-bidi#1031) but I don't expect it to use suggestedFilename to identify a download.
Is there a plan to change the behavior to save the downloads with a guid file name and returning unmodified suggested file name in Bidi?
No.
| constructor(page: Page, downloadsPath: string, uuid: string, url: string, suggestedFilename?: string, useSuggestedFilename?: boolean) { | ||
| const unaccessibleErrorMessage = page.browserContext._options.acceptDownloads === 'deny' ? 'Pass { acceptDownloads: true } when you are creating your browser context.' : undefined; | ||
| this.artifact = new Artifact(page, path.join(downloadsPath, uuid), unaccessibleErrorMessage, () => { | ||
| const downloadPath = path.join(downloadsPath, useSuggestedFilename ? suggestedFilename! : uuid); |
There was a problem hiding this comment.
let's use downloadFilename instead of useSuggestedFilename and avoid the variable with !:
const downloadPath = path.join(downloadsPath, downloadFilename ?? uuid);
hbenl
force-pushed
the
setDownloadBehavior
branch
from
9ffca5d to
25d3ad3
Compare
Test results for "tests 1"2 failed 8 flaky38631 passed, 843 skipped Merge workflow run. |
Test results for "MCP"12 failed 5121 passed, 171 skipped Merge workflow run. |
2 participants
Fixes #39305 and the following tests in Firefox:
library/download.spec.ts:library/downloads-path.spec.ts:library/defaultbrowsercontext-1.spec.ts: