chore: simplify the launcher routine (#306)

src/chromium/Browser.ts

@@ -29,38 +29,39 @@ import { FrameManager } from './FrameManager';
 import * as network from '../network';
 import { Permissions } from './features/permissions';
 import { Overrides } from './features/overrides';
+import { ConnectionTransport } from '../transport';
 
 export class Browser extends EventEmitter {
   private _process: childProcess.ChildProcess;
   _connection: Connection;
   _client: CDPSession;
-  private _closeCallback: () => Promise<void>;
   private _defaultContext: BrowserContext;
   private _contexts = new Map<string, BrowserContext>();
   _targets = new Map<string, Target>();
   readonly chromium: Chromium;
 
   static async create(
-    connection: Connection,
+    browserWSEndpoint: string,
-    contextIds: string[],
+    transport: ConnectionTransport,
-    process: childProcess.ChildProcess | null,
+    process: childProcess.ChildProcess | null) {
-    closeCallback?: (() => Promise<void>)) {
+    const connection = new Connection(transport);
-    const browser = new Browser(connection, contextIds, process, closeCallback);
+
+    const { browserContextIds } = await connection.rootSession.send('Target.getBrowserContexts');
+    const browser = new Browser(browserWSEndpoint, connection, browserContextIds, process);
     await connection.rootSession.send('Target.setDiscoverTargets', { discover: true });
     return browser;
   }
 
   constructor(
+    browserWSEndpoint: string,
     connection: Connection,
     contextIds: string[],
-    process: childProcess.ChildProcess | null,
+    process: childProcess.ChildProcess | null) {
-    closeCallback?: (() => Promise<void>)) {
     super();
     this._connection = connection;
     this._client = connection.rootSession;
     this._process = process;
-    this._closeCallback = closeCallback || (() => Promise.resolve());
+    this.chromium = new Chromium(this, browserWSEndpoint);
-    this.chromium = new Chromium(this);
 
     this._defaultContext = this._createBrowserContext(null, {});
     for (const contextId of contextIds)
@@ -228,7 +229,7 @@ export class Browser extends EventEmitter {
   }
 
   async close() {
-    await this._closeCallback.call(null);
+    await this._connection.rootSession.send('Browser.close');
     this.disconnect();
   }
 

src/chromium/Connection.ts

@@ -17,7 +17,7 @@
 
 import * as debug from 'debug';
 import { EventEmitter } from 'events';
-import { ConnectionTransport, SlowMoTransport } from '../transport';
+import { ConnectionTransport } from '../transport';
 import { assert } from '../helper';
 import { Protocol } from './protocol';
 
@@ -28,18 +28,15 @@ export const ConnectionEvents = {
 };
 
 export class Connection extends EventEmitter {
-  private _url: string;
   private _lastId = 0;
   private _transport: ConnectionTransport;
   private _sessions = new Map<string, CDPSession>();
   readonly rootSession: CDPSession;
   _closed = false;
 
-  constructor(url: string, transport: ConnectionTransport, delay: number | undefined = 0) {
+  constructor(transport: ConnectionTransport) {
     super();
-    this._url = url;
+    this._transport = transport;
-
-    this._transport = SlowMoTransport.wrap(transport, delay);
     this._transport.onmessage = this._onMessage.bind(this);
     this._transport.onclose = this._onClose.bind(this);
     this.rootSession = new CDPSession(this, 'browser', '');
@@ -54,10 +51,6 @@ export class Connection extends EventEmitter {
     return this._sessions.get(sessionId) || null;
   }
 
-  url(): string {
-    return this._url;
-  }
-
   _rawSend(sessionId: string, message: any): number {
     const id = ++this._lastId;
     message.id = id;

src/chromium/Launcher.ts

@@ -23,10 +23,9 @@ import * as path from 'path';
 import * as URL from 'url';
 import { Browser } from './Browser';
 import { BrowserFetcher, BrowserFetcherOptions } from '../browserFetcher';
-import { Connection } from './Connection';
 import { TimeoutError } from '../errors';
-import { assert, debugError, helper } from '../helper';
+import { assert, helper } from '../helper';
-import { ConnectionTransport, WebSocketTransport, PipeTransport } from '../transport';
+import { ConnectionTransport, WebSocketTransport, PipeTransport, SlowMoTransport } from '../transport';
 import * as util from 'util';
 import { launchProcess, waitForLine } from '../processLauncher';
 
@@ -112,7 +111,7 @@ export class Launcher {
 
     const usePipe = chromeArguments.includes('--remote-debugging-pipe');
 
-    const launched = await launchProcess({
+    const launchedProcess = await launchProcess({
       executablePath: chromeExecutable,
       args: chromeArguments,
       env,
@@ -123,31 +122,29 @@ export class Launcher {
       pipe: usePipe,
       tempDir: temporaryUserDataDir
     }, () => {
-      if (temporaryUserDataDir || !connection)
+      if (temporaryUserDataDir || !browser)
         return Promise.reject();
-      return connection.rootSession.send('Browser.close').catch(error => {
+      return browser.close();
-        debugError(error);
-        throw error;
-      });
     });
 
-    let connection: Connection | null = null;
+    let browser: Browser | undefined;
     try {
+      let transport: ConnectionTransport | null = null;
+      let browserWSEndpoint: string = '';
       if (!usePipe) {
         const timeoutError = new TimeoutError(`Timed out after ${timeout} ms while trying to connect to Chrome! The only Chrome revision guaranteed to work is r${this._preferredRevision}`);
-        const match = await waitForLine(launched.process, launched.process.stderr, /^DevTools listening on (ws:\/\/.*)$/, timeout, timeoutError);
+        const match = await waitForLine(launchedProcess, launchedProcess.stderr, /^DevTools listening on (ws:\/\/.*)$/, timeout, timeoutError);
-        const browserWSEndpoint = match[1];
+        browserWSEndpoint = match[1];
-        const transport = await WebSocketTransport.create(browserWSEndpoint);
+        transport = await WebSocketTransport.create(browserWSEndpoint);
-        connection = new Connection(browserWSEndpoint, transport, slowMo);
       } else {
-        const transport = new PipeTransport(launched.process.stdio[3] as NodeJS.WritableStream, launched.process.stdio[4] as NodeJS.ReadableStream);
+        transport = new PipeTransport(launchedProcess.stdio[3] as NodeJS.WritableStream, launchedProcess.stdio[4] as NodeJS.ReadableStream);
-        connection = new Connection('', transport, slowMo);
       }
-      const browser = await Browser.create(connection, [], launched.process, launched.gracefullyClose);
+      browser = await Browser.create(browserWSEndpoint, SlowMoTransport.wrap(transport, slowMo), launchedProcess);
       await browser._waitForTarget(t => t.type() === 'page');
       return browser;
     } catch (e) {
-      await launched.gracefullyClose();
+      if (browser)
+        await browser.close();
       throw e;
     }
   }
@@ -185,31 +182,20 @@ export class Launcher {
       browserWSEndpoint?: string;
       browserURL?: string;
       transport?: ConnectionTransport; })): Promise<Browser> {
-    const {
+    assert(Number(!!options.browserWSEndpoint) + Number(!!options.browserURL) + Number(!!options.transport) === 1, 'Exactly one of browserWSEndpoint, browserURL or transport must be passed to playwright.connect');
-      browserWSEndpoint,
-      browserURL,
-      transport,
-      slowMo = 0,
-    } = options;
 
-    assert(Number(!!browserWSEndpoint) + Number(!!browserURL) + Number(!!transport) === 1, 'Exactly one of browserWSEndpoint, browserURL or transport must be passed to playwright.connect');
+    let transport: ConnectionTransport | undefined;
-
+    let connectionURL: string = '';
-    let connection: Connection = null;
+    if (options.transport) {
-    if (transport) {
+      transport = options.transport;
-      connection = new Connection('', transport, slowMo);
+    } else if (options.browserWSEndpoint) {
-    } else if (browserWSEndpoint) {
+      connectionURL = options.browserWSEndpoint;
-      const connectionTransport = await WebSocketTransport.create(browserWSEndpoint);
+      transport = await WebSocketTransport.create(options.browserWSEndpoint);
-      connection = new Connection(browserWSEndpoint, connectionTransport, slowMo);
+    } else if (options.browserURL) {
-    } else if (browserURL) {
+      connectionURL = await getWSEndpoint(options.browserURL);
-      const connectionURL = await getWSEndpoint(browserURL);
+      transport = await WebSocketTransport.create(connectionURL);
-      const connectionTransport = await WebSocketTransport.create(connectionURL);
-      connection = new Connection(connectionURL, connectionTransport, slowMo);
     }
-
+    return Browser.create(connectionURL, SlowMoTransport.wrap(transport, options.slowMo), null);
-    const { browserContextIds } = await connection.rootSession.send('Target.getBrowserContexts');
-    return Browser.create(connection, browserContextIds, null, async () => {
-      connection.rootSession.send('Browser.close').catch(debugError);
-    });
   }
 
   _resolveExecutablePath(): { executablePath: string; missingText: string | null; } {

src/chromium/features/chromium.ts

@@ -18,7 +18,7 @@ import { EventEmitter } from 'events';
 import { assert } from '../../helper';
 import { Browser } from '../Browser';
 import { BrowserContext } from '../../browserContext';
-import { CDPSession, Connection } from '../Connection';
+import { CDPSession } from '../Connection';
 import { Page } from '../../page';
 import { readProtocolStream } from '../protocolHelper';
 import { Target } from '../Target';
@@ -26,16 +26,16 @@ import { Worker } from './workers';
 import { FrameManager } from '../FrameManager';
 
 export class Chromium extends EventEmitter {
-  private _connection: Connection;
   private _client: CDPSession;
   private _recording = false;
   private _path = '';
   private _tracingClient: CDPSession | undefined;
   private _browser: Browser;
+  private _browserWSEndpoint: string;
 
-  constructor(browser: Browser) {
+  constructor(browser: Browser, browserWSEndpoint: string) {
     super();
-    this._connection = browser._connection;
+    this._browserWSEndpoint = browserWSEndpoint;
     this._client = browser._client;
     this._browser = browser;
   }
@@ -101,6 +101,6 @@ export class Chromium extends EventEmitter {
   }
 
   wsEndpoint(): string {
-    return this._connection.url();
+    return this._browserWSEndpoint;
   }
 }

src/firefox/Browser.ts

@@ -16,6 +16,7 @@
  */
 
 import { EventEmitter } from 'events';
+import { ChildProcess } from 'child_process';
 import { helper, RegisteredListener, assert } from '../helper';
 import { Connection, ConnectionEvents, JugglerSessionEvents } from './Connection';
 import { Events } from './events';
@@ -26,30 +27,31 @@ import { FrameManager } from './FrameManager';
 import { Firefox } from './features/firefox';
 import * as network from '../network';
 import { BrowserContext, BrowserContextOptions } from '../browserContext';
+import { ConnectionTransport } from '../transport';
 
 export class Browser extends EventEmitter {
   _connection: Connection;
-  private _process: import('child_process').ChildProcess;
+  private _process: ChildProcess;
-  private _closeCallback: () => Promise<void>;
   _targets: Map<string, Target>;
   private _defaultContext: BrowserContext;
   private _contexts: Map<string, BrowserContext>;
   private _eventListeners: RegisteredListener[];
   readonly firefox: Firefox;
+  readonly _browserWSEndpoint: string;
 
-  static async create(connection: Connection, process: import('child_process').ChildProcess | null, closeCallback: () => Promise<void>) {
+  static async create(browserWSEndpoint: string, transport: ConnectionTransport, process: ChildProcess | null) {
+    const connection = new Connection(transport);
     const {browserContextIds} = await connection.send('Target.getBrowserContexts');
-    const browser = new Browser(connection, browserContextIds, process, closeCallback);
+    const browser = new Browser(browserWSEndpoint, connection, browserContextIds, process);
     await connection.send('Target.enable');
     return browser;
   }
 
-  constructor(connection: Connection, browserContextIds: Array<string>, process: import('child_process').ChildProcess | null, closeCallback: () => Promise<void>) {
+  constructor(browserWSEndpoint: string, connection: Connection, browserContextIds: Array<string>, process: ChildProcess | null) {
     super();
     this._connection = connection;
     this._process = process;
-    this._closeCallback = closeCallback;
+    this.firefox = new Firefox(browserWSEndpoint);
-    this.firefox = new Firefox(this);
 
     this._targets = new Map();
 
@@ -93,7 +95,7 @@ export class Browser extends EventEmitter {
     return this._defaultContext;
   }
 
-  process(): import('child_process').ChildProcess | null {
+  process(): ChildProcess | null {
     return this._process;
   }
 
@@ -151,7 +153,7 @@ export class Browser extends EventEmitter {
 
   async close() {
     helper.removeEventListeners(this._eventListeners);
-    await this._closeCallback();
+    await this._connection.send('Browser.close');
   }
 
   _createBrowserContext(browserContextId: string | null, options: BrowserContextOptions): BrowserContext {

src/firefox/Connection.ts

@@ -18,7 +18,7 @@
 import {assert} from '../helper';
 import {EventEmitter} from 'events';
 import * as debug from 'debug';
-import { ConnectionTransport, SlowMoTransport } from '../transport';
+import { ConnectionTransport } from '../transport';
 import { Protocol } from './protocol';
 const debugProtocol = debug('playwright:protocol');
 
@@ -27,20 +27,18 @@ export const ConnectionEvents = {
 };
 
 export class Connection extends EventEmitter {
-  private _url: string;
   private _lastId: number;
   private _callbacks: Map<number, {resolve: Function, reject: Function, error: Error, method: string}>;
   private _transport: ConnectionTransport;
   private _sessions: Map<string, JugglerSession>;
   _closed: boolean;
 
-  constructor(url: string, transport: ConnectionTransport, delay: number | undefined = 0) {
+  constructor(transport: ConnectionTransport) {
     super();
-    this._url = url;
+    this._transport = transport;
     this._lastId = 0;
     this._callbacks = new Map();
 
-    this._transport = SlowMoTransport.wrap(transport, delay);
     this._transport.onmessage = this._onMessage.bind(this);
     this._transport.onclose = this._onClose.bind(this);
     this._sessions = new Map();
@@ -55,10 +53,6 @@ export class Connection extends EventEmitter {
     return this._sessions.get(sessionId) || null;
   }
 
-  url(): string {
-    return this._url;
-  }
-
   send(method: string, params: object | undefined = {}): Promise<any> {
     const id = this._rawSend({method, params});
     return new Promise((resolve, reject) => {

src/firefox/Launcher.ts

@@ -17,14 +17,13 @@
 
 import * as os from 'os';
 import * as path from 'path';
-import { Connection } from './Connection';
 import { Browser } from './Browser';
 import { BrowserFetcher, BrowserFetcherOptions } from '../browserFetcher';
 import * as fs from 'fs';
 import * as util from 'util';
-import { debugError, assert } from '../helper';
+import { assert } from '../helper';
 import { TimeoutError } from '../errors';
-import { WebSocketTransport } from '../transport';
+import { WebSocketTransport, SlowMoTransport } from '../transport';
 import { launchProcess, waitForLine } from '../processLauncher';
 
 const mkdtempAsync = util.promisify(fs.mkdtemp);
@@ -97,7 +96,7 @@ export class Launcher {
         throw new Error(missingText);
       firefoxExecutable = executablePath;
     }
-    const launched = await launchProcess({
+    const launchedProcess = await launchProcess({
       executablePath: firefoxExecutable,
       args: firefoxArguments,
       env: os.platform() === 'linux' ? {
@@ -112,26 +111,23 @@ export class Launcher {
       pipe: false,
       tempDir: temporaryProfileDir
     }, () => {
-      if (temporaryProfileDir || !connection)
+      if (temporaryProfileDir || !browser)
         return Promise.reject();
-      return connection.send('Browser.close').catch(error => {
+      browser.close();
-        debugError(error);
-        throw error;
-      });
     });
 
-    let connection: Connection | null = null;
+    let browser: Browser | undefined;
     try {
       const timeoutError = new TimeoutError(`Timed out after ${timeout} ms while trying to connect to Firefox!`);
-      const match = await waitForLine(launched.process, launched.process.stdout, /^Juggler listening on (ws:\/\/.*)$/, timeout, timeoutError);
+      const match = await waitForLine(launchedProcess, launchedProcess.stdout, /^Juggler listening on (ws:\/\/.*)$/, timeout, timeoutError);
       const url = match[1];
       const transport = await WebSocketTransport.create(url);
-      connection = new Connection(url, transport, slowMo);
+      browser = await Browser.create(url, SlowMoTransport.wrap(transport, slowMo), launchedProcess);
-      const browser = await Browser.create(connection, launched.process, launched.gracefullyClose);
       await browser._waitForTarget(t => t.type() === 'page');
       return browser;
     } catch (e) {
-      await launched.gracefullyClose;
+      if (browser)
+        await browser.close();
       throw e;
     }
   }
@@ -141,10 +137,8 @@ export class Launcher {
       browserWSEndpoint,
       slowMo = 0,
     } = options;
-    let connection = null;
     const transport = await WebSocketTransport.create(browserWSEndpoint);
-    connection = new Connection(browserWSEndpoint, transport, slowMo);
+    return await Browser.create(browserWSEndpoint, SlowMoTransport.wrap(transport, slowMo), null);
-    return await Browser.create(connection, null, () => connection.send('Browser.close').catch(debugError));
   }
 
   executablePath(): string {

src/firefox/features/firefox.ts

@@ -1,17 +1,15 @@
 // Copyright (c) Microsoft Corporation.
 // Licensed under the MIT license.
 
-import { Browser } from '../Browser';
-import { Connection } from '../Connection';
 
 export class Firefox {
-  private _connection: Connection;
+  private _browserWSEndpoint: string;
 
-  constructor(browser: Browser) {
+  constructor(browserWSEndpoint: string) {
-    this._connection = browser._connection;
+    this._browserWSEndpoint = browserWSEndpoint;
   }
 
   wsEndpoint(): string {
-    return this._connection.url();
+    return this._browserWSEndpoint;
   }
 }

src/processLauncher.ts

@@ -37,12 +37,7 @@ export type LaunchProcessOptions = {
   tempDir?: string,
 };
 
-export type LaunchProcessResult = {
+export async function launchProcess(options: LaunchProcessOptions, attemptToGracefullyClose: () => Promise<any>): Promise<childProcess.ChildProcess> {
-  process: childProcess.ChildProcess;
-  gracefullyClose: () => Promise<void>;
-};
-
-export async function launchProcess(options: LaunchProcessOptions, attemptToGracefullyClose: () => Promise<any>): Promise<LaunchProcessResult> {
   let stdio: ('ignore' | 'pipe')[] = ['pipe', 'pipe', 'pipe'];
   if (options.pipe) {
     if (options.dumpio)
@@ -65,7 +60,7 @@ export async function launchProcess(options: LaunchProcessOptions, attemptToGrac
 
   if (!spawnedProcess.pid) {
     let reject: (e: Error) => void;
-    const result = new Promise<LaunchProcessResult>((f, r) => reject = r);
+    const result = new Promise<childProcess.ChildProcess>((f, r) => reject = r);
     spawnedProcess.once('error', error => {
       reject(new Error('Failed to launch browser: ' + error));
     });
@@ -99,7 +94,7 @@ export async function launchProcess(options: LaunchProcessOptions, attemptToGrac
     listeners.push(helper.addEventListener(process, 'SIGTERM', gracefullyClose));
   if (options.handleSIGHUP)
     listeners.push(helper.addEventListener(process, 'SIGHUP', gracefullyClose));
-  return { process: spawnedProcess, gracefullyClose };
+  return spawnedProcess;
 
   async function gracefullyClose(): Promise<void> {
     helper.removeEventListeners(listeners);

src/webkit/Browser.ts

@@ -25,11 +25,11 @@ import { Target } from './Target';
 import { Protocol } from './protocol';
 import { Events } from '../events';
 import { BrowserContext, BrowserContextOptions } from '../browserContext';
+import { ConnectionTransport } from '../transport';
 
 export class Browser extends EventEmitter {
   private readonly _process: childProcess.ChildProcess;
   readonly _connection: Connection;
-  private _closeCallback: () => Promise<void>;
   private _defaultContext: BrowserContext;
   private _contexts = new Map<string, BrowserContext>();
   _targets = new Map<string, Target>();
@@ -37,13 +37,11 @@ export class Browser extends EventEmitter {
   private _privateEvents = new EventEmitter();
 
   constructor(
-    connection: Connection,
+    transport: ConnectionTransport,
-    process: childProcess.ChildProcess | null,
+    process: childProcess.ChildProcess | null) {
-    closeCallback?: (() => Promise<void>)) {
     super();
-    this._connection = connection;
+    this._connection = new Connection(transport);
     this._process = process;
-    this._closeCallback = closeCallback || (() => Promise.resolve());
 
     /** @type {!Map<string, !Target>} */
     this._targets = new Map();
@@ -181,7 +179,7 @@ export class Browser extends EventEmitter {
 
   async close() {
     helper.removeEventListeners(this._eventListeners);
-    await this._closeCallback.call(null);
+    await this._connection.send('Browser.close');
   }
 
   _createBrowserContext(browserContextId: string | undefined, options: BrowserContextOptions): BrowserContext {

src/webkit/Connection.ts

@@ -18,7 +18,7 @@
 import { assert } from '../helper';
 import * as debug from 'debug';
 import { EventEmitter } from 'events';
-import { ConnectionTransport, SlowMoTransport } from '../transport';
+import { ConnectionTransport } from '../transport';
 import { Protocol } from './protocol';
 
 const debugProtocol = debug('playwright:protocol');
@@ -38,9 +38,9 @@ export class Connection extends EventEmitter {
 
   _closed = false;
 
-  constructor(transport: ConnectionTransport, delay: number | undefined = 0) {
+  constructor(transport: ConnectionTransport) {
     super();
-    this._transport = SlowMoTransport.wrap(transport, delay);
+    this._transport = transport;
     this._transport.onmessage = this._dispatchMessage.bind(this);
     this._transport.onclose = this._onClose.bind(this);
   }

src/webkit/Launcher.ts

@@ -15,11 +15,10 @@
  * limitations under the License.
  */
 
-import { debugError, assert } from '../helper';
+import { assert } from '../helper';
 import { Browser } from './Browser';
 import { BrowserFetcher, BrowserFetcherOptions } from '../browserFetcher';
-import { Connection } from './Connection';
+import { PipeTransport, SlowMoTransport } from '../transport';
-import { PipeTransport } from '../transport';
 import { execSync } from 'child_process';
 import * as path from 'path';
 import * as util from 'util';
@@ -78,7 +77,7 @@ export class Launcher {
     if (process.platform === 'darwin' && options.headless !== false)
       webkitArguments.push('--headless');
 
-    const launched = await launchProcess({
+    const launchedProcess = await launchProcess({
       executablePath: webkitExecutable,
       args: webkitArguments,
       env,
@@ -89,23 +88,20 @@ export class Launcher {
       pipe: true,
       tempDir: null
     }, () => {
-      if (!connection)
+      if (!browser)
         return Promise.reject();
-      return connection.send('Browser.close').catch(error => {
+      browser.close();
-        debugError(error);
-        throw error;
-      });
     });
 
-    let connection: Connection | null = null;
+    let browser: Browser | undefined;
     try {
-      const transport = new PipeTransport(launched.process.stdio[3] as NodeJS.WritableStream, launched.process.stdio[4] as NodeJS.ReadableStream);
+      const transport = new PipeTransport(launchedProcess.stdio[3] as NodeJS.WritableStream, launchedProcess.stdio[4] as NodeJS.ReadableStream);
-      connection = new Connection(transport, slowMo);
+      browser = new Browser(SlowMoTransport.wrap(transport, slowMo), launchedProcess);
-      const browser = new Browser(connection, launched.process, launched.gracefullyClose);
       await browser._waitForTarget(t => t._type === 'page');
       return browser;
     } catch (e) {
-      await launched.gracefullyClose();
+      if (browser)
+        await browser.close();
       throw e;
     }
   }