chore: use internal BrowserOptions to unify browsers (#2230)

Author: dgozman

src/browser.ts

@@ -21,6 +21,17 @@ import { Download } from './download';
 import type { BrowserServer } from './server/browserServer';
 import { Events } from './events';
 import { InnerLogger, Log } from './logger';
+import * as types from './types';
+
+export type BrowserOptions = {
+  logger: InnerLogger,
+  downloadsPath: string,
+  headful?: boolean,
+  persistent?: boolean,
+  slowMo?: number,
+  viewport?: types.Size | null,
+  ownedServer?: BrowserServer,
+};
 
 export interface Browser extends EventEmitter {
   newContext(options?: BrowserContextOptions): Promise<BrowserContext>;
@@ -31,14 +42,12 @@ export interface Browser extends EventEmitter {
 }
 
 export abstract class BrowserBase extends EventEmitter implements Browser, InnerLogger {
-  _downloadsPath: string = '';
+  readonly _options: BrowserOptions;
   private _downloads = new Map<string, Download>();
-  _ownedServer: BrowserServer | null = null;
-  readonly _logger: InnerLogger;
 
-  constructor(logger: InnerLogger) {
+  constructor(options: BrowserOptions) {
     super();
-    this._logger = logger;
+    this._options = options;
   }
 
   abstract newContext(options?: BrowserContextOptions): Promise<BrowserContext>;
@@ -54,7 +63,7 @@ export abstract class BrowserBase extends EventEmitter implements Browser, Inner
   }
 
   _downloadCreated(page: Page, uuid: string, url: string, suggestedFilename?: string) {
-    const download = new Download(page, this._downloadsPath, uuid, url, suggestedFilename);
+    const download = new Download(page, this._options.downloadsPath, uuid, url, suggestedFilename);
     this._downloads.set(uuid, download);
   }
 
@@ -74,8 +83,8 @@ export abstract class BrowserBase extends EventEmitter implements Browser, Inner
   }
 
   async close() {
-    if (this._ownedServer) {
-      await this._ownedServer.close();
+    if (this._options.ownedServer) {
+      await this._options.ownedServer.close();
     } else {
       await Promise.all(this.contexts().map(context => context.close()));
       this._disconnect();
@@ -85,11 +94,11 @@ export abstract class BrowserBase extends EventEmitter implements Browser, Inner
   }
 
   _isLogEnabled(log: Log): boolean {
-    return this._logger._isLogEnabled(log);
+    return this._options.logger._isLogEnabled(log);
   }
 
   _log(log: Log, message: string | Error, ...args: any[]) {
-    return this._logger._log(log, message, ...args);
+    return this._options.logger._log(log, message, ...args);
   }
 }
 

src/chromium/crBrowser.ts

@@ -15,7 +15,7 @@
  * limitations under the License.
  */
 
-import { BrowserBase } from '../browser';
+import { BrowserBase, BrowserOptions } from '../browser';
 import { assertBrowserContextIsNotOwned, BrowserContext, BrowserContextBase, BrowserContextOptions, validateBrowserContextOptions, verifyGeolocation } from '../browserContext';
 import { Events as CommonEvents } from '../events';
 import { assert, helper } from '../helper';
@@ -29,7 +29,7 @@ import { readProtocolStream } from './crProtocolHelper';
 import { Events } from './events';
 import { Protocol } from './protocol';
 import { CRExecutionContext } from './crExecutionContext';
-import { InnerLogger, logError } from '../logger';
+import { logError } from '../logger';
 
 export class CRBrowser extends BrowserBase {
   readonly _connection: CRConnection;
@@ -44,13 +44,12 @@ export class CRBrowser extends BrowserBase {
   private _tracingRecording = false;
   private _tracingPath: string | null = '';
   private _tracingClient: CRSession | undefined;
-  readonly _isHeadful: boolean;
 
-  static async connect(transport: ConnectionTransport, isPersistent: boolean, logger: InnerLogger, options: { slowMo?: number, headless?: boolean, viewport?: types.Size | null } = {}): Promise<CRBrowser> {
-    const connection = new CRConnection(SlowMoTransport.wrap(transport, options.slowMo), logger);
-    const browser = new CRBrowser(connection, logger, { persistent: isPersistent, headful: !options.headless, viewport: options.viewport });
+  static async connect(transport: ConnectionTransport, options: BrowserOptions): Promise<CRBrowser> {
+    const connection = new CRConnection(SlowMoTransport.wrap(transport, options.slowMo), options.logger);
+    const browser = new CRBrowser(connection, options);
     const session = connection.rootSession;
-    if (!isPersistent) {
+    if (!options.persistent) {
       await session.send('Target.setAutoAttach', { autoAttach: true, waitForDebuggerOnStart: true, flatten: true });
       return browser;
     }
@@ -83,14 +82,13 @@ export class CRBrowser extends BrowserBase {
     return browser;
   }
 
-  constructor(connection: CRConnection, logger: InnerLogger, options: { headful?: boolean, persistent?: boolean, viewport?: types.Size | null } = {}) {
-    super(logger);
+  constructor(connection: CRConnection, options: BrowserOptions) {
+    super(options);
     this._connection = connection;
     this._session = this._connection.rootSession;
 
     if (options.persistent)
       this._defaultContext = new CRBrowserContext(this, null, validateBrowserContextOptions({ viewport: options.viewport }));
-    this._isHeadful = !!options.headful;
     this._connection.on(ConnectionEvents.Disconnected, () => {
       for (const context of this._contexts.values())
         context._browserClosed();
@@ -150,7 +148,7 @@ export class CRBrowser extends BrowserBase {
 
     if (targetInfo.type === 'page') {
       const opener = targetInfo.openerId ? this._crPages.get(targetInfo.openerId) || null : null;
-      const crPage = new CRPage(session, targetInfo.targetId, context, opener, this._isHeadful);
+      const crPage = new CRPage(session, targetInfo.targetId, context, opener, !!this._options.headful);
       this._crPages.set(targetInfo.targetId, crPage);
       crPage.pageOrError().then(() => {
         context!.emit(CommonEvents.BrowserContext.Page, crPage._page);
@@ -289,7 +287,7 @@ export class CRBrowserContext extends BrowserContextBase {
       this._browser._session.send('Browser.setDownloadBehavior', {
         behavior: this._options.acceptDownloads ? 'allowAndName' : 'deny',
         browserContextId: this._browserContextId || undefined,
-        downloadPath: this._browser._downloadsPath
+        downloadPath: this._browser._options.downloadsPath
       })
     ];
     if (this._options.permissions)

src/firefox/ffBrowser.ts

@@ -15,7 +15,7 @@
  * limitations under the License.
  */
 
-import { BrowserBase } from '../browser';
+import { BrowserBase, BrowserOptions } from '../browser';
 import { assertBrowserContextIsNotOwned, BrowserContext, BrowserContextBase, BrowserContextOptions, validateBrowserContextOptions, verifyGeolocation } from '../browserContext';
 import { Events } from '../events';
 import { assert, helper, RegisteredListener } from '../helper';
@@ -27,7 +27,6 @@ import { ConnectionEvents, FFConnection } from './ffConnection';
 import { headersArray } from './ffNetworkManager';
 import { FFPage } from './ffPage';
 import { Protocol } from './protocol';
-import { InnerLogger } from '../logger';
 
 export class FFBrowser extends BrowserBase {
   _connection: FFConnection;
@@ -36,19 +35,19 @@ export class FFBrowser extends BrowserBase {
   readonly _contexts: Map<string, FFBrowserContext>;
   private _eventListeners: RegisteredListener[];
 
-  static async connect(transport: ConnectionTransport, logger: InnerLogger, attachToDefaultContext: boolean, slowMo?: number): Promise<FFBrowser> {
-    const connection = new FFConnection(SlowMoTransport.wrap(transport, slowMo), logger);
-    const browser = new FFBrowser(connection, logger, attachToDefaultContext);
-    await connection.send('Browser.enable', { attachToDefaultContext });
+  static async connect(transport: ConnectionTransport, options: BrowserOptions): Promise<FFBrowser> {
+    const connection = new FFConnection(SlowMoTransport.wrap(transport, options.slowMo), options.logger);
+    const browser = new FFBrowser(connection, options);
+    await connection.send('Browser.enable', { attachToDefaultContext: !!options.persistent });
     return browser;
   }
 
-  constructor(connection: FFConnection, logger: InnerLogger, isPersistent: boolean) {
-    super(logger);
+  constructor(connection: FFConnection, options: BrowserOptions) {
+    super(options);
     this._connection = connection;
     this._ffPages = new Map();
 
-    if (isPersistent)
+    if (options.persistent)
       this._defaultContext = new FFBrowserContext(this, null, validateBrowserContextOptions({}));
     this._contexts = new Map();
     this._connection.on(ConnectionEvents.Disconnected, () => {
@@ -159,7 +158,7 @@ export class FFBrowserContext extends BrowserContextBase {
         browserContextId,
         downloadOptions: {
           behavior: this._options.acceptDownloads ? 'saveToDisk' : 'cancel',
-          downloadsDir: this._browser._downloadsPath,
+          downloadsDir: this._browser._options.downloadsPath,
         },
       }),
     ];

src/server/browserType.ts

@@ -36,6 +36,11 @@ type LaunchOptionsBase = BrowserArgOptions & {
   env?: {[key: string]: string|number|boolean}
 };
 
+export function processBrowserArgOptions(options: LaunchOptionsBase): { devtools: boolean, headless: boolean } {
+  const { devtools = false, headless = !devtools } = options;
+  return { devtools, headless };
+}
+
 export type ConnectOptions = {
   wsEndpoint: string,
   slowMo?: number,

src/server/chromium.ts

@@ -25,7 +25,7 @@ import * as ws from 'ws';
 import { launchProcess } from './processLauncher';
 import { kBrowserCloseMessageId } from '../chromium/crConnection';
 import { PipeTransport } from './pipeTransport';
-import { LaunchOptions, BrowserArgOptions, ConnectOptions, LaunchServerOptions, AbstractBrowserType } from './browserType';
+import { LaunchOptions, BrowserArgOptions, ConnectOptions, LaunchServerOptions, AbstractBrowserType, processBrowserArgOptions } from './browserType';
 import { LaunchType } from '../browser';
 import { BrowserServer, WebSocketWrapper } from './browserServer';
 import { Events } from '../events';
@@ -48,10 +48,13 @@ export class Chromium extends AbstractBrowserType<CRBrowser> {
     return await browserServer._initializeOrClose(deadline, async () => {
       if ((options as any).__testHookBeforeCreateBrowser)
         await (options as any).__testHookBeforeCreateBrowser();
-      const browser = await CRBrowser.connect(transport!, false, logger, options);
-      browser._ownedServer = browserServer;
-      browser._downloadsPath = downloadsPath;
-      return browser;
+      return await CRBrowser.connect(transport!, {
+        slowMo: options.slowMo,
+        headful: !processBrowserArgOptions(options).headless,
+        logger,
+        downloadsPath,
+        ownedServer: browserServer
+      });
     });
   }
 
@@ -62,12 +65,18 @@ export class Chromium extends AbstractBrowserType<CRBrowser> {
   async launchPersistentContext(userDataDir: string, options: LaunchOptions = {}): Promise<BrowserContext> {
     const { timeout = 30000 } = options;
     const deadline = TimeoutSettings.computeDeadline(timeout);
-    const { transport, browserServer, logger } = await this._launchServer(options, 'persistent', userDataDir);
+    const { transport, browserServer, downloadsPath, logger } = await this._launchServer(options, 'persistent', userDataDir);
     return await browserServer._initializeOrClose(deadline, async () => {
       if ((options as any).__testHookBeforeCreateBrowser)
         await (options as any).__testHookBeforeCreateBrowser();
-      const browser = await CRBrowser.connect(transport!, true, logger, options);
-      browser._ownedServer = browserServer;
+      const browser = await CRBrowser.connect(transport!, {
+        slowMo: options.slowMo,
+        persistent: true,
+        logger,
+        downloadsPath,
+        headful: !processBrowserArgOptions(options).headless,
+        ownedServer: browserServer
+      });
       const context = browser._defaultContext!;
       if (!options.ignoreDefaultArgs || Array.isArray(options.ignoreDefaultArgs))
         await context._loadDefaultContext();
@@ -109,6 +118,11 @@ export class Chromium extends AbstractBrowserType<CRBrowser> {
     const chromeExecutable = executablePath || this.executablePath();
     if (!chromeExecutable)
       throw new Error(`No executable path is specified. Pass "executablePath" option directly.`);
+
+    // Note: it is important to define these variables before launchProcess, so that we don't get
+    // "Cannot access 'browserServer' before initialization" if something went wrong.
+    let transport: PipeTransport | undefined = undefined;
+    let browserServer: BrowserServer | undefined = undefined;
     const { launchedProcess, gracefullyClose, downloadsPath } = await launchProcess({
       executablePath: chromeExecutable,
       args: chromeArguments,
@@ -134,8 +148,6 @@ export class Chromium extends AbstractBrowserType<CRBrowser> {
       },
     });
 
-    let transport: PipeTransport | undefined = undefined;
-    let browserServer: BrowserServer | undefined = undefined;
     const stdio = launchedProcess.stdio as unknown as [NodeJS.ReadableStream, NodeJS.WritableStream, NodeJS.WritableStream, NodeJS.WritableStream, NodeJS.ReadableStream];
     transport = new PipeTransport(stdio[3], stdio[4], logger);
     browserServer = new BrowserServer(launchedProcess, gracefullyClose, launchType === 'server' ? wrapTransportWithWebSocket(transport, logger, port) : null);
@@ -146,16 +158,13 @@ export class Chromium extends AbstractBrowserType<CRBrowser> {
     return await WebSocketTransport.connect(options.wsEndpoint, async transport => {
       if ((options as any).__testHookBeforeCreateBrowser)
         await (options as any).__testHookBeforeCreateBrowser();
-      return CRBrowser.connect(transport, false, new RootLogger(options.logger), options);
+      return CRBrowser.connect(transport, { slowMo: options.slowMo, logger: new RootLogger(options.logger), downloadsPath: '' });
     });
   }
 
   private _defaultArgs(options: BrowserArgOptions = {}, launchType: LaunchType, userDataDir: string): string[] {
-    const {
-      devtools = false,
-      headless = !devtools,
-      args = [],
-    } = options;
+    const { devtools, headless } = processBrowserArgOptions(options);
+    const { args = [] } = options;
     const userDataDirArg = args.find(arg => arg.startsWith('--user-data-dir'));
     if (userDataDirArg)
       throw new Error('Pass userDataDir parameter instead of specifying --user-data-dir argument');

src/server/electron.ts

@@ -204,8 +204,7 @@ export class Electron  {
       return transport;
     });
     const browserServer = new BrowserServer(launchedProcess, gracefullyClose, null);
-    const browser = await CRBrowser.connect(chromeTransport, true, logger, { ...options, viewport: null });
-    browser._ownedServer = browserServer;
+    const browser = await CRBrowser.connect(chromeTransport, { headful: true, logger, persistent: true, viewport: null, ownedServer: browserServer, downloadsPath: '' });
     app = new ElectronApplication(logger, browser, nodeConnection);
     await app._init();
     return app;