From aae5fca23714be9ea9e9531402768e58a95052a9 Mon Sep 17 00:00:00 2001 From: Pavel Feldman Date: Tue, 11 Feb 2020 12:06:58 -0800 Subject: [PATCH] feat(api): make browser.newPage own the created context (#930) --- docs/api.md | 11 ++--------- src/browser.ts | 14 +++++--------- src/browserContext.ts | 5 +++++ src/chromium/crBrowser.ts | 9 ++------- src/firefox/ffBrowser.ts | 9 ++------- src/page.ts | 3 +++ src/webkit/wkBrowser.ts | 9 ++------- test/browser.spec.js | 17 +++++++++-------- test/chromium/tracing.spec.js | 2 +- test/web.spec.js | 2 +- 10 files changed, 32 insertions(+), 49 deletions(-) diff --git a/docs/api.md b/docs/api.md index 6432f8bd8d0f2..ec48bc5e3d83f 100644 --- a/docs/api.md +++ b/docs/api.md @@ -153,7 +153,6 @@ See [ChromiumBrowser], [FirefoxBrowser] and [WebKitBrowser] for browser-specific - [browser.isConnected()](#browserisconnected) - [browser.newContext(options)](#browsernewcontextoptions) - [browser.newPage([options])](#browsernewpageoptions) -- [browser.pages()](#browserpages) #### event: 'disconnected' @@ -233,12 +232,9 @@ Creates a new browser context. It won't share cookies/cache with other browser c - `permissions` <[Object]> A map from origin keys to permissions values. See [browserContext.setPermissions](#browsercontextsetpermissionsorigin-permissions) for more details. - returns: <[Promise]<[Page]>> -Creates a new page in a new browser context. +Creates a new page in a new browser context. Closing this page will close the context as well. -#### browser.pages() -- returns: <[Promise]<[Array]<[Page]>>> Promise which resolves to an array of all open pages. - -An array of all the pages inside all the browser contexts. +This is a convenience API that should only be used for the single-page scenarios and short snippets. Production code and testing frameworks should explicitly create [browser.newContext](#browsernewcontextoptions) followed by the [browserContext.newPage](#browsercontextnewpage) to control their exact life times. ### class: BrowserContext @@ -3667,7 +3663,6 @@ await browser.stopTracing(); - [browser.isConnected()](#browserisconnected) - [browser.newContext(options)](#browsernewcontextoptions) - [browser.newPage([options])](#browsernewpageoptions) -- [browser.pages()](#browserpages) #### event: 'targetchanged' @@ -3834,7 +3829,6 @@ Firefox browser instance does not expose Firefox-specific features. - [browser.isConnected()](#browserisconnected) - [browser.newContext(options)](#browsernewcontextoptions) - [browser.newPage([options])](#browsernewpageoptions) -- [browser.pages()](#browserpages) ### class: WebKitBrowser @@ -3850,7 +3844,6 @@ WebKit browser instance does not expose WebKit-specific features. - [browser.isConnected()](#browserisconnected) - [browser.newContext(options)](#browsernewcontextoptions) - [browser.newPage([options])](#browsernewpageoptions) -- [browser.pages()](#browserpages) ### Working with selectors diff --git a/src/browser.ts b/src/browser.ts index 3b6bbdf641279..b5cca2533176b 100644 --- a/src/browser.ts +++ b/src/browser.ts @@ -21,7 +21,6 @@ import { Page } from './page'; export interface Browser extends platform.EventEmitterType { newContext(options?: BrowserContextOptions): Promise; contexts(): BrowserContext[]; - pages(): Promise; newPage(options?: BrowserContextOptions): Promise; isConnected(): boolean; close(): Promise; @@ -32,14 +31,11 @@ export type ConnectOptions = { wsEndpoint: string }; -export async function collectPages(browser: Browser): Promise { - const result: Promise[] = []; - for (const browserContext of browser.contexts()) - result.push(browserContext.pages()); - const pages: Page[] = []; - for (const group of await Promise.all(result)) - pages.push(...group); - return pages; +export async function createPageInNewContext(browser: Browser, options?: BrowserContextOptions): Promise { + const context = await browser.newContext(options); + const page = await context.newPage(); + page._ownedContext = context; + return page; } export type LaunchType = 'local' | 'server' | 'persistent'; diff --git a/src/browserContext.ts b/src/browserContext.ts index 6d8f56c53505d..ca56d75520d2b 100644 --- a/src/browserContext.ts +++ b/src/browserContext.ts @@ -82,6 +82,11 @@ export class BrowserContext extends platform.EventEmitter { } async newPage(): Promise { + const pages = this._delegate.existingPages(); + for (const page of pages) { + if (page._ownedContext) + throw new Error('Please use browser.newContext() for multi-page scripts that share the context.'); + } return this._delegate.newPage(); } diff --git a/src/chromium/crBrowser.ts b/src/chromium/crBrowser.ts index 377fec31bb548..755b85525cd3c 100644 --- a/src/chromium/crBrowser.ts +++ b/src/chromium/crBrowser.ts @@ -24,7 +24,7 @@ import { Page, Worker } from '../page'; import { CRTarget } from './crTarget'; import { Protocol } from './protocol'; import { CRPage } from './crPage'; -import { Browser, collectPages } from '../browser'; +import { Browser, createPageInNewContext } from '../browser'; import * as network from '../network'; import * as types from '../types'; import * as platform from '../platform'; @@ -168,13 +168,8 @@ export class CRBrowser extends platform.EventEmitter implements Browser { return Array.from(this._contexts.values()); } - async pages(): Promise { - return collectPages(this); - } - async newPage(options?: BrowserContextOptions): Promise { - const context = await this.newContext(options); - return context.newPage(); + return createPageInNewContext(this, options); } async _targetCreated(event: Protocol.Target.targetCreatedPayload) { diff --git a/src/firefox/ffBrowser.ts b/src/firefox/ffBrowser.ts index 27625b29ac4a0..82cea3e284b01 100644 --- a/src/firefox/ffBrowser.ts +++ b/src/firefox/ffBrowser.ts @@ -15,7 +15,7 @@ * limitations under the License. */ -import { Browser, collectPages } from '../browser'; +import { Browser, createPageInNewContext } from '../browser'; import { BrowserContext, BrowserContextOptions } from '../browserContext'; import { Events } from '../events'; import { assert, helper, RegisteredListener } from '../helper'; @@ -84,13 +84,8 @@ export class FFBrowser extends platform.EventEmitter implements Browser { return Array.from(this._contexts.values()); } - async pages(): Promise { - return collectPages(this); - } - async newPage(options?: BrowserContextOptions): Promise { - const context = await this.newContext(options); - return context.newPage(); + return createPageInNewContext(this, options); } async _waitForTarget(predicate: (target: Target) => boolean, options: { timeout?: number; } = {}): Promise { diff --git a/src/page.ts b/src/page.ts index d1a4fc6db394a..b3c22d29cc698 100644 --- a/src/page.ts +++ b/src/page.ts @@ -114,6 +114,7 @@ export class Page extends platform.EventEmitter { readonly pdf: ((options?: types.PDFOptions) => Promise) | undefined; readonly coverage: Coverage | undefined; readonly _requestHandlers: { url: types.URLMatch, handler: (request: network.Request) => void }[] = []; + _ownedContext: BrowserContext | undefined; constructor(delegate: PageDelegate, browserContext: BrowserContext) { super(); @@ -472,6 +473,8 @@ export class Page extends platform.EventEmitter { await this._delegate.closePage(runBeforeUnload); if (!runBeforeUnload) await this._closedPromise; + if (this._ownedContext) + await this._ownedContext.close(); } isClosed(): boolean { diff --git a/src/webkit/wkBrowser.ts b/src/webkit/wkBrowser.ts index 2018f1bd53e1d..c17ec2faf048e 100644 --- a/src/webkit/wkBrowser.ts +++ b/src/webkit/wkBrowser.ts @@ -15,7 +15,7 @@ * limitations under the License. */ -import { Browser, collectPages } from '../browser'; +import { Browser, createPageInNewContext } from '../browser'; import { BrowserContext, BrowserContextOptions } from '../browserContext'; import { assert, helper, RegisteredListener } from '../helper'; import * as network from '../network'; @@ -89,13 +89,8 @@ export class WKBrowser extends platform.EventEmitter implements Browser { return Array.from(this._contexts.values()); } - async pages(): Promise { - return collectPages(this); - } - async newPage(options?: BrowserContextOptions): Promise { - const context = await this.newContext(options); - return context.newPage(); + return createPageInNewContext(this, options); } async _waitForFirstPageTarget(timeout: number): Promise { diff --git a/test/browser.spec.js b/test/browser.spec.js index 4232dce3a85bd..c79494a3434bb 100644 --- a/test/browser.spec.js +++ b/test/browser.spec.js @@ -24,24 +24,25 @@ module.exports.describe = function({testRunner, expect, playwright, CHROMIUM, WE const {it, fit, xit, dit} = testRunner; const {beforeAll, beforeEach, afterAll, afterEach} = testRunner; - describe('Browser', function() { + describe('Browser.newPage', function() { it('should create new page', async function({browser}) { - expect((await browser.pages()).length).toBe(0); const page1 = await browser.newPage(); - expect((await browser.pages()).length).toBe(1); expect(browser.contexts().length).toBe(1); const page2 = await browser.newPage(); - expect((await browser.pages()).length).toBe(2); expect(browser.contexts().length).toBe(2); - await page1.context().close(); - expect((await browser.pages()).length).toBe(1); + await page1.close(); expect(browser.contexts().length).toBe(1); - await page2.context().close(); - expect((await browser.pages()).length).toBe(0); + await page2.close(); expect(browser.contexts().length).toBe(0); }); + it('should throw upon second create new page', async function({browser}) { + const page = await browser.newPage(); + let error; + await page.context().newPage().catch(e => error = e); + expect(error.message).toContain('Please use browser.newContext()'); + }); }); }; diff --git a/test/chromium/tracing.spec.js b/test/chromium/tracing.spec.js index fc9c16309e8bc..d242af429be14 100644 --- a/test/chromium/tracing.spec.js +++ b/test/chromium/tracing.spec.js @@ -58,7 +58,7 @@ module.exports.describe = function({testRunner, expect, defaultBrowserOptions, p const newPage = await browser.newPage(); let error = null; await browser.startTracing(newPage, {path: outputFile}).catch(e => error = e); - await newPage.context().close(); + await newPage.close(); expect(error).toBeTruthy(); await browser.stopTracing(); }); diff --git a/test/web.spec.js b/test/web.spec.js index 8e84de2d5e1be..d18d2b3cb79e1 100644 --- a/test/web.spec.js +++ b/test/web.spec.js @@ -46,7 +46,7 @@ module.exports.describe = function({testRunner, expect, defaultBrowserOptions, p afterEach(async state => { await state.page.evaluate(() => teardown()); - await state.page.context().close(); + await state.page.close(); state.page = null; });