From 4ef9f84ab5aed69c483b3771e3b3c25ed029aa7b Mon Sep 17 00:00:00 2001 From: Pavel Feldman Date: Thu, 19 Dec 2019 10:21:26 -0800 Subject: [PATCH] chore: simplify the launcher routine (#306) --- src/chromium/Browser.ts | 23 ++++++----- src/chromium/Connection.ts | 13 ++---- src/chromium/Launcher.ts | 68 ++++++++++++------------------- src/chromium/features/chromium.ts | 10 ++--- src/firefox/Browser.ts | 20 +++++---- src/firefox/Connection.ts | 12 ++---- src/firefox/Launcher.ts | 28 +++++-------- src/firefox/features/firefox.ts | 10 ++--- src/processLauncher.ts | 11 ++--- src/webkit/Browser.ts | 12 +++--- src/webkit/Connection.ts | 6 +-- src/webkit/Launcher.ts | 24 +++++------ 12 files changed, 97 insertions(+), 140 deletions(-) diff --git a/src/chromium/Browser.ts b/src/chromium/Browser.ts index 3c477fe8e0..b2febed4e7 100644 --- a/src/chromium/Browser.ts +++ b/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; private _defaultContext: BrowserContext; private _contexts = new Map(); _targets = new Map(); readonly chromium: Chromium; static async create( - connection: Connection, - contextIds: string[], - process: childProcess.ChildProcess | null, - closeCallback?: (() => Promise)) { - const browser = new Browser(connection, contextIds, process, closeCallback); + browserWSEndpoint: string, + transport: ConnectionTransport, + process: childProcess.ChildProcess | null) { + const connection = new Connection(transport); + + 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, - closeCallback?: (() => Promise)) { + process: childProcess.ChildProcess | null) { super(); this._connection = connection; this._client = connection.rootSession; this._process = process; - this._closeCallback = closeCallback || (() => Promise.resolve()); - this.chromium = new Chromium(this); + this.chromium = new Chromium(this, browserWSEndpoint); 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(); } diff --git a/src/chromium/Connection.ts b/src/chromium/Connection.ts index 26dd5a8afc..87403c2a7b 100644 --- a/src/chromium/Connection.ts +++ b/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(); readonly rootSession: CDPSession; _closed = false; - constructor(url: string, transport: ConnectionTransport, delay: number | undefined = 0) { + constructor(transport: ConnectionTransport) { super(); - this._url = url; - - this._transport = SlowMoTransport.wrap(transport, delay); + this._transport = transport; 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; diff --git a/src/chromium/Launcher.ts b/src/chromium/Launcher.ts index 9e6bcd8cc2..e3b3a11d06 100644 --- a/src/chromium/Launcher.ts +++ b/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 { ConnectionTransport, WebSocketTransport, PipeTransport } from '../transport'; +import { assert, helper } from '../helper'; +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 => { - debugError(error); - throw error; - }); + return browser.close(); }); - 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 browserWSEndpoint = match[1]; - const transport = await WebSocketTransport.create(browserWSEndpoint); - connection = new Connection(browserWSEndpoint, transport, slowMo); + const match = await waitForLine(launchedProcess, launchedProcess.stderr, /^DevTools listening on (ws:\/\/.*)$/, timeout, timeoutError); + browserWSEndpoint = match[1]; + transport = await WebSocketTransport.create(browserWSEndpoint); } else { - const transport = new PipeTransport(launched.process.stdio[3] as NodeJS.WritableStream, launched.process.stdio[4] as NodeJS.ReadableStream); - connection = new Connection('', transport, slowMo); + transport = new PipeTransport(launchedProcess.stdio[3] as NodeJS.WritableStream, launchedProcess.stdio[4] as NodeJS.ReadableStream); } - 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 { - const { - browserWSEndpoint, - browserURL, - transport, - slowMo = 0, - } = options; + assert(Number(!!options.browserWSEndpoint) + Number(!!options.browserURL) + Number(!!options.transport) === 1, 'Exactly one of browserWSEndpoint, browserURL or transport must be passed to playwright.connect'); - assert(Number(!!browserWSEndpoint) + Number(!!browserURL) + Number(!!transport) === 1, 'Exactly one of browserWSEndpoint, browserURL or transport must be passed to playwright.connect'); - - let connection: Connection = null; - if (transport) { - connection = new Connection('', transport, slowMo); - } else if (browserWSEndpoint) { - const connectionTransport = await WebSocketTransport.create(browserWSEndpoint); - connection = new Connection(browserWSEndpoint, connectionTransport, slowMo); - } else if (browserURL) { - const connectionURL = await getWSEndpoint(browserURL); - const connectionTransport = await WebSocketTransport.create(connectionURL); - connection = new Connection(connectionURL, connectionTransport, slowMo); + let transport: ConnectionTransport | undefined; + let connectionURL: string = ''; + if (options.transport) { + transport = options.transport; + } else if (options.browserWSEndpoint) { + connectionURL = options.browserWSEndpoint; + transport = await WebSocketTransport.create(options.browserWSEndpoint); + } else if (options.browserURL) { + connectionURL = await getWSEndpoint(options.browserURL); + transport = await WebSocketTransport.create(connectionURL); } - - const { browserContextIds } = await connection.rootSession.send('Target.getBrowserContexts'); - return Browser.create(connection, browserContextIds, null, async () => { - connection.rootSession.send('Browser.close').catch(debugError); - }); + return Browser.create(connectionURL, SlowMoTransport.wrap(transport, options.slowMo), null); } _resolveExecutablePath(): { executablePath: string; missingText: string | null; } { diff --git a/src/chromium/features/chromium.ts b/src/chromium/features/chromium.ts index b8df6f9fdb..02e92de442 100644 --- a/src/chromium/features/chromium.ts +++ b/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; } } diff --git a/src/firefox/Browser.ts b/src/firefox/Browser.ts index 2429c5c17f..4a4729aecd 100644 --- a/src/firefox/Browser.ts +++ b/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 _closeCallback: () => Promise; + private _process: ChildProcess; _targets: Map; private _defaultContext: BrowserContext; private _contexts: Map; private _eventListeners: RegisteredListener[]; readonly firefox: Firefox; + readonly _browserWSEndpoint: string; - static async create(connection: Connection, process: import('child_process').ChildProcess | null, closeCallback: () => Promise) { + 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, process: import('child_process').ChildProcess | null, closeCallback: () => Promise) { + constructor(browserWSEndpoint: string, connection: Connection, browserContextIds: Array, process: ChildProcess | null) { super(); this._connection = connection; this._process = process; - this._closeCallback = closeCallback; - this.firefox = new Firefox(this); + this.firefox = new Firefox(browserWSEndpoint); 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 { diff --git a/src/firefox/Connection.ts b/src/firefox/Connection.ts index 46d1e43def..9a609be27a 100644 --- a/src/firefox/Connection.ts +++ b/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; private _transport: ConnectionTransport; private _sessions: Map; _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 { const id = this._rawSend({method, params}); return new Promise((resolve, reject) => { diff --git a/src/firefox/Launcher.ts b/src/firefox/Launcher.ts index b33ec4a997..85640f9986 100644 --- a/src/firefox/Launcher.ts +++ b/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 => { - debugError(error); - throw error; - }); + browser.close(); }); - 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); - const browser = await Browser.create(connection, launched.process, launched.gracefullyClose); + browser = await Browser.create(url, 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; } } @@ -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(connection, null, () => connection.send('Browser.close').catch(debugError)); + return await Browser.create(browserWSEndpoint, SlowMoTransport.wrap(transport, slowMo), null); } executablePath(): string { diff --git a/src/firefox/features/firefox.ts b/src/firefox/features/firefox.ts index 3d5dc8df83..684c78a641 100644 --- a/src/firefox/features/firefox.ts +++ b/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) { - this._connection = browser._connection; + constructor(browserWSEndpoint: string) { + this._browserWSEndpoint = browserWSEndpoint; } wsEndpoint(): string { - return this._connection.url(); + return this._browserWSEndpoint; } } diff --git a/src/processLauncher.ts b/src/processLauncher.ts index bdba530882..336402a504 100644 --- a/src/processLauncher.ts +++ b/src/processLauncher.ts @@ -37,12 +37,7 @@ export type LaunchProcessOptions = { tempDir?: string, }; -export type LaunchProcessResult = { - process: childProcess.ChildProcess; - gracefullyClose: () => Promise; -}; - -export async function launchProcess(options: LaunchProcessOptions, attemptToGracefullyClose: () => Promise): Promise { +export async function launchProcess(options: LaunchProcessOptions, attemptToGracefullyClose: () => Promise): Promise { 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((f, r) => reject = r); + const result = new Promise((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 { helper.removeEventListeners(listeners); diff --git a/src/webkit/Browser.ts b/src/webkit/Browser.ts index 5f32dcc07f..1a080af977 100644 --- a/src/webkit/Browser.ts +++ b/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; private _defaultContext: BrowserContext; private _contexts = new Map(); _targets = new Map(); @@ -37,13 +37,11 @@ export class Browser extends EventEmitter { private _privateEvents = new EventEmitter(); constructor( - connection: Connection, - process: childProcess.ChildProcess | null, - closeCallback?: (() => Promise)) { + transport: ConnectionTransport, + process: childProcess.ChildProcess | null) { super(); - this._connection = connection; + this._connection = new Connection(transport); this._process = process; - this._closeCallback = closeCallback || (() => Promise.resolve()); /** @type {!Map} */ 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 { diff --git a/src/webkit/Connection.ts b/src/webkit/Connection.ts index 2211602462..5c413bfaef 100644 --- a/src/webkit/Connection.ts +++ b/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); } diff --git a/src/webkit/Launcher.ts b/src/webkit/Launcher.ts index 6bb23bd861..7430d65ddd 100644 --- a/src/webkit/Launcher.ts +++ b/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 } from '../transport'; +import { PipeTransport, SlowMoTransport } 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 => { - debugError(error); - throw error; - }); + browser.close(); }); - 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); - connection = new Connection(transport, slowMo); - const browser = new Browser(connection, launched.process, launched.gracefullyClose); + const transport = new PipeTransport(launchedProcess.stdio[3] as NodeJS.WritableStream, launchedProcess.stdio[4] as NodeJS.ReadableStream); + browser = new Browser(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; } }