feat(api): make browser.newPage own the created context (#930)

microsoft · Feb 11, 2020 · aae5fca · aae5fca
1 parent ad9d6cc
commit aae5fca
Show file tree

Hide file tree

Showing 10 changed files with 32 additions and 49 deletions.
diff --git 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)
 <!-- GEN:stop -->
 
 #### 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)
 <!-- GEN:stop -->
 
 #### 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)
 <!-- GEN:stop -->
 
 ### 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)
 <!-- GEN:stop -->
 
 ### Working with selectors

diff --git 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<BrowserContext>;
   contexts(): BrowserContext[];
-  pages(): Promise<Page[]>;
   newPage(options?: BrowserContextOptions): Promise<Page>;
   isConnected(): boolean;
   close(): Promise<void>;
@@ -32,14 +31,11 @@ export type ConnectOptions = {
   wsEndpoint: string
 };
 
-export async function collectPages(browser: Browser): Promise<Page[]> {
-  const result: Promise<Page[]>[] = [];
-  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<Page> {
+  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
@@ -82,6 +82,11 @@ export class BrowserContext extends platform.EventEmitter {
   }
 
   async newPage(): Promise<Page> {
+    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
@@ -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<Page[]> {
-    return collectPages(this);
-  }
-
   async newPage(options?: BrowserContextOptions): Promise<Page> {
-    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
@@ -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<Page[]> {
-    return collectPages(this);
-  }
-
   async newPage(options?: BrowserContextOptions): Promise<Page> {
-    const context = await this.newContext(options);
-    return context.newPage();
+    return createPageInNewContext(this, options);
   }
 
   async _waitForTarget(predicate: (target: Target) => boolean, options: { timeout?: number; } = {}): Promise<Target> {

diff --git a/src/page.ts b/src/page.ts
@@ -114,6 +114,7 @@ export class Page extends platform.EventEmitter {
   readonly pdf: ((options?: types.PDFOptions) => Promise<platform.BufferType>) | 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
@@ -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<Page[]> {
-    return collectPages(this);
-  }
-
   async newPage(options?: BrowserContextOptions): Promise<Page> {
-    const context = await this.newContext(options);
-    return context.newPage();
+    return createPageInNewContext(this, options);
   }
 
   async _waitForFirstPageTarget(timeout: number): Promise<void> {

diff --git 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
@@ -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
@@ -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;
     });