Skip to content

Commit

Permalink
feat(download): adding a new Download._cancel method (#6236)
Browse files Browse the repository at this point in the history
  • Loading branch information
starrify authored Jun 12, 2021
1 parent 2b8ea73 commit 5f6d4a7
Show file tree
Hide file tree
Showing 17 changed files with 133 additions and 6 deletions.
8 changes: 8 additions & 0 deletions docs/src/api/class-download.md
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,14 @@ downloaded content. If [`option: acceptDownloads`] is not set, download events a
not performed and user has no access to the downloaded files.
:::

## async method: Download._cancel

**Chromium-only** Cancels a download.
Will not fail if the download is already finished or canceled.
Upon successful cancellations, `download.failure()` would resolve to `'canceled'`.

Currently **experimental** and may subject to further changes.

## async method: Download.createReadStream
* langs: java, js, csharp
- returns: <[null]|[Readable]>
Expand Down
6 changes: 6 additions & 0 deletions src/client/artifact.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,12 @@ export class Artifact extends ChannelOwner<channels.ArtifactChannel, channels.Ar
});
}

async cancel(): Promise<void> {
return this._wrapApiCall(`${this._apiName}.cancel`, async (channel: channels.ArtifactChannel) => {
return channel.cancel();
});
}

async delete(): Promise<void> {
return this._wrapApiCall(`${this._apiName}.delete`, async (channel: channels.ArtifactChannel) => {
return channel.delete();
Expand Down
4 changes: 4 additions & 0 deletions src/client/download.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,10 @@ export class Download implements api.Download {
return this._artifact.createReadStream();
}

async _cancel(): Promise<void> {
return this._artifact.cancel();
}

async delete(): Promise<void> {
return this._artifact.delete();
}
Expand Down
4 changes: 4 additions & 0 deletions src/dispatchers/artifactDispatcher.ts
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,10 @@ export class ArtifactDispatcher extends Dispatcher<Artifact, channels.ArtifactIn
return { error: error || undefined };
}

async cancel(): Promise<void> {
await this._object.cancel();
}

async delete(): Promise<void> {
await this._object.delete();
this._dispose();
Expand Down
4 changes: 4 additions & 0 deletions src/protocol/channels.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2480,6 +2480,7 @@ export interface ArtifactChannel extends Channel {
saveAsStream(params?: ArtifactSaveAsStreamParams, metadata?: Metadata): Promise<ArtifactSaveAsStreamResult>;
failure(params?: ArtifactFailureParams, metadata?: Metadata): Promise<ArtifactFailureResult>;
stream(params?: ArtifactStreamParams, metadata?: Metadata): Promise<ArtifactStreamResult>;
cancel(params?: ArtifactCancelParams, metadata?: Metadata): Promise<ArtifactCancelResult>;
delete(params?: ArtifactDeleteParams, metadata?: Metadata): Promise<ArtifactDeleteResult>;
}
export type ArtifactPathAfterFinishedParams = {};
Expand Down Expand Up @@ -2509,6 +2510,9 @@ export type ArtifactStreamOptions = {};
export type ArtifactStreamResult = {
stream?: StreamChannel,
};
export type ArtifactCancelParams = {};
export type ArtifactCancelOptions = {};
export type ArtifactCancelResult = void;
export type ArtifactDeleteParams = {};
export type ArtifactDeleteOptions = {};
export type ArtifactDeleteResult = void;
Expand Down
2 changes: 2 additions & 0 deletions src/protocol/protocol.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2043,6 +2043,8 @@ Artifact:
returns:
stream: Stream?

cancel:

delete:


Expand Down
1 change: 1 addition & 0 deletions src/protocol/validator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -960,6 +960,7 @@ export function createScheme(tChannel: (name: string) => Validator): Scheme {
scheme.ArtifactSaveAsStreamParams = tOptional(tObject({}));
scheme.ArtifactFailureParams = tOptional(tObject({}));
scheme.ArtifactStreamParams = tOptional(tObject({}));
scheme.ArtifactCancelParams = tOptional(tObject({}));
scheme.ArtifactDeleteParams = tOptional(tObject({}));
scheme.StreamReadParams = tObject({
size: tOptional(tNumber),
Expand Down
11 changes: 10 additions & 1 deletion src/server/artifact.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,24 +15,28 @@
*/

import fs from 'fs';
import { assert } from '../utils/utils';
import { SdkObject } from './instrumentation';

type SaveCallback = (localPath: string, error?: string) => Promise<void>;
type CancelCallback = () => Promise<void>;

export class Artifact extends SdkObject {
private _localPath: string;
private _unaccessibleErrorMessage: string | undefined;
private _cancelCallback: CancelCallback | undefined;
private _finishedCallback: () => void;
private _finishedPromise: Promise<void>;
private _saveCallbacks: SaveCallback[] = [];
private _finished: boolean = false;
private _deleted = false;
private _failureError: string | null = null;

constructor(parent: SdkObject, localPath: string, unaccessibleErrorMessage?: string) {
constructor(parent: SdkObject, localPath: string, unaccessibleErrorMessage?: string, cancelCallback?: CancelCallback) {
super(parent, 'artifact');
this._localPath = localPath;
this._unaccessibleErrorMessage = unaccessibleErrorMessage;
this._cancelCallback = cancelCallback;
this._finishedCallback = () => {};
this._finishedPromise = new Promise(f => this._finishedCallback = f);
}
Expand Down Expand Up @@ -76,6 +80,11 @@ export class Artifact extends SdkObject {
return this._failureError;
}

async cancel(): Promise<void> {
assert(this._cancelCallback !== undefined);
return this._cancelCallback();
}

async delete(): Promise<void> {
if (this._unaccessibleErrorMessage)
return;
Expand Down
1 change: 1 addition & 0 deletions src/server/browserContext.ts
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,7 @@ export abstract class BrowserContext extends SdkObject {
abstract _doUpdateRequestInterception(): Promise<void>;
abstract _doClose(): Promise<void>;
abstract _onClosePersistent(): Promise<void>;
abstract _doCancelDownload(uuid: string): Promise<void>;

async cookies(urls: string | string[] | undefined = []): Promise<types.NetworkCookie[]> {
if (urls && !Array.isArray(urls))
Expand Down
10 changes: 10 additions & 0 deletions src/server/chromium/crBrowser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -479,6 +479,16 @@ export class CRBrowserContext extends BrowserContext {
}
}

async _doCancelDownload(guid: string) {
// The upstream CDP method is implemented in a way that no explicit error would be given
// regarding the requested `guid`, even if the download is in a state not suitable for
// cancellation (finished, cancelled, etc.) or the guid is invalid at all.
await this._browser._session.send('Browser.cancelDownload', {
guid: guid,
browserContextId: this._browserContextId,
});
}

backgroundPages(): Page[] {
const result: Page[] = [];
for (const backgroundPage of this._browser._backgroundPages.values()) {
Expand Down
4 changes: 3 additions & 1 deletion src/server/download.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,9 @@ export class Download {

constructor(page: Page, downloadsPath: string, uuid: string, url: string, suggestedFilename?: string) {
const unaccessibleErrorMessage = !page._browserContext._options.acceptDownloads ? 'Pass { acceptDownloads: true } when you are creating your browser context.' : undefined;
this.artifact = new Artifact(page, path.join(downloadsPath, uuid), unaccessibleErrorMessage);
this.artifact = new Artifact(page, path.join(downloadsPath, uuid), unaccessibleErrorMessage, () => {
return this._page._browserContext._doCancelDownload(uuid);
});
this._page = page;
this.url = url;
this._suggestedFilename = suggestedFilename;
Expand Down
5 changes: 5 additions & 0 deletions src/server/firefox/ffBrowser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -330,6 +330,11 @@ export class FFBrowserContext extends BrowserContext {
await this._browser._connection.send('Browser.removeBrowserContext', { browserContextId: this._browserContextId });
this._browser._contexts.delete(this._browserContextId);
}

async _doCancelDownload(uuid: string) {
// TODO: Have this implemented
throw new Error('Download cancellation not yet implemented in Firefox');
}
}

function toJugglerProxyOptions(proxy: types.ProxySettings) {
Expand Down
5 changes: 5 additions & 0 deletions src/server/webkit/wkBrowser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -326,4 +326,9 @@ export class WKBrowserContext extends BrowserContext {
await this._browser._browserSession.send('Playwright.deleteContext', { browserContextId: this._browserContextId });
this._browser._contexts.delete(this._browserContextId);
}

async _doCancelDownload(uuid: string) {
// TODO: Have this implemented
throw new Error('Download cancellation not yet implemented in WebKit');
}
}
1 change: 1 addition & 0 deletions tests/config/checkCoverage.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ if (browserName !== 'chromium') {
api.delete('coverage.startCSSCoverage');
api.delete('coverage.stopCSSCoverage');
api.delete('page.pdf');
api.delete('download._cancel');
}

// Some permissions tests are disabled in webkit. See permissions.jest.js
Expand Down
44 changes: 43 additions & 1 deletion tests/download.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,14 @@ it.describe('download event', () => {
res.setHeader('Content-Disposition', 'attachment; filename=file.txt');
res.end(`Hello world`);
});
server.setRoute('/downloadWithDelay', (req, res) => {
res.setHeader('Content-Type', 'application/octet-stream');
res.setHeader('Content-Disposition', 'attachment; filename=file.txt');
// Chromium requires a large enough payload to trigger the download event soon enough
res.write('a'.repeat(4096));
res.write('foo');
res.uncork();
});
});

it('should report downloads with acceptDownloads: false', async ({browser, server}) => {
Expand Down Expand Up @@ -461,6 +469,41 @@ it.describe('download event', () => {
await page.close();
});

it('should be able to cancel pending downloads', async ({browser, server, browserName, browserVersion}) => {
// The exact upstream change is in b449b5c, which still does not appear in the first few 91.* tags until 91.0.4437.0.
it.fixme(browserName === 'chromium' && Number(browserVersion.split('.')[0]) < 91, 'The upstream Browser.cancelDownload command is not available before Chrome 91');
it.fixme(browserName !== 'chromium', 'Download cancellation currently implemented for only Chromium');
const page = await browser.newPage({ acceptDownloads: true });
await page.setContent(`<a href="${server.PREFIX}/downloadWithDelay">download</a>`);
const [ download ] = await Promise.all([
page.waitForEvent('download'),
page.click('a')
]);
await download._cancel();
const failure = await download.failure();
expect(failure).toBe('canceled');
await page.close();
});

it('should not fail explicitly to cancel a download even if that is already finished', async ({browser, server, browserName, browserVersion}) => {
// The exact upstream change is in b449b5c, which still does not appear in the first few 91.* tags until 91.0.4437.0.
it.fixme(browserName === 'chromium' && Number(browserVersion.split('.')[0]) < 91, 'The upstream Browser.cancelDownload command is not available before Chrome 91');
it.fixme(browserName !== 'chromium', 'Download cancellation currently implemented for only Chromium');
const page = await browser.newPage({ acceptDownloads: true });
await page.setContent(`<a href="${server.PREFIX}/download">download</a>`);
const [ download ] = await Promise.all([
page.waitForEvent('download'),
page.click('a')
]);
const path = await download.path();
expect(fs.existsSync(path)).toBeTruthy();
expect(fs.readFileSync(path).toString()).toBe('Hello world');
await download._cancel();
const failure = await download.failure();
expect(failure).toBe(null);
await page.close();
});

it('should report downloads with interception', async ({browser, server}) => {
const page = await browser.newPage({ acceptDownloads: true });
await page.route(/.*/, r => r.continue());
Expand All @@ -474,5 +517,4 @@ it.describe('download event', () => {
expect(fs.readFileSync(path).toString()).toBe('Hello world');
await page.close();
});

});
8 changes: 8 additions & 0 deletions types/types.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9495,6 +9495,14 @@ export interface Dialog {
* performed and user has no access to the downloaded files.
*/
export interface Download {
/**
* **Chromium-only** Cancels a download. Will not fail if the download is already finished or canceled. Upon successful
* cancellations, `download.failure()` would resolve to `'canceled'`.
*
* Currently **experimental** and may subject to further changes.
*/
_cancel(): Promise<void>;

/**
* Returns readable stream for current download or `null` if download failed.
*/
Expand Down
21 changes: 18 additions & 3 deletions utils/doclint/missingDocs.js
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,9 @@ function paramsForMember(member) {
return new Set(member.argsArray.map(a => a.alias));
}

// Including experimental method names (with a leading underscore) that would be otherwise skipped
const allowExperimentalMethods = new Set([ 'Download._cancel' ]);

/**
* @param {string[]} rootNames
*/
Expand Down Expand Up @@ -109,6 +112,20 @@ function listMethods(rootNames, apiFileName) {
return null;
}

/**
* @param {string} className
* @param {string} methodName
*/
function shouldSkipMethodByName(className, methodName) {
if (allowExperimentalMethods.has(`${className}.${methodName}`))
return false;
if (methodName.startsWith('_') || methodName === 'T' || methodName === 'toString')
return true;
if (/** @type {any} */(EventEmitter).prototype.hasOwnProperty(methodName))
return true;
return false;
}

/**
* @param {string} className
* @param {!ts.Type} classType
Expand All @@ -120,9 +137,7 @@ function listMethods(rootNames, apiFileName) {
apiMethods.set(className, methods);
}
for (const [name, member] of /** @type {any[]} */(classType.symbol.members || [])) {
if (name.startsWith('_') || name === 'T' || name === 'toString')
continue;
if (/** @type {any} */(EventEmitter).prototype.hasOwnProperty(name))
if (shouldSkipMethodByName(className, name))
continue;
const memberType = checker.getTypeOfSymbolAtLocation(member, member.valueDeclaration);
const signature = signatureForType(memberType);
Expand Down

0 comments on commit 5f6d4a7

Please sign in to comment.