Skip to content

Commit

Permalink
chore: merge BrowserType and BrowserTypeBase, remove logName (#3837)
Browse files Browse the repository at this point in the history
- We do not need the public BrowserType different from BrowserTypeBase anymore.
- Removing 'logName' parameter from runAbortableTask - it will
be used for metadata instead.
  • Loading branch information
dgozman authored Sep 10, 2020
1 parent bf9c4a3 commit ed3b00e
Show file tree
Hide file tree
Showing 9 changed files with 39 additions and 35 deletions.
10 changes: 5 additions & 5 deletions src/browserServerImpl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
*/

import { LaunchServerOptions } from './client/types';
import { BrowserTypeBase } from './server/browserType';
import { BrowserType } from './server/browserType';
import * as ws from 'ws';
import { Browser } from './server/browser';
import { ChildProcess } from 'child_process';
Expand All @@ -31,9 +31,9 @@ import { SelectorsDispatcher } from './dispatchers/selectorsDispatcher';
import { Selectors } from './server/selectors';

export class BrowserServerLauncherImpl implements BrowserServerLauncher {
private _browserType: BrowserTypeBase;
private _browserType: BrowserType;

constructor(browserType: BrowserTypeBase) {
constructor(browserType: BrowserType) {
this._browserType = browserType;
}

Expand All @@ -50,12 +50,12 @@ export class BrowserServerLauncherImpl implements BrowserServerLauncher {

export class BrowserServerImpl extends EventEmitter implements BrowserServer {
private _server: ws.Server;
private _browserType: BrowserTypeBase;
private _browserType: BrowserType;
private _browser: Browser;
private _wsEndpoint: string;
private _process: ChildProcess;

constructor(browserType: BrowserTypeBase, browser: Browser, port: number = 0) {
constructor(browserType: BrowserType, browser: Browser, port: number = 0) {
super();

this._browserType = browserType;
Expand Down
4 changes: 2 additions & 2 deletions src/dispatchers/browserTypeDispatcher.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,14 @@
* limitations under the License.
*/

import { BrowserTypeBase, BrowserType } from '../server/browserType';
import { BrowserType } from '../server/browserType';
import { BrowserDispatcher } from './browserDispatcher';
import * as channels from '../protocol/channels';
import { Dispatcher, DispatcherScope } from './dispatcher';
import { BrowserContextDispatcher } from './browserContextDispatcher';

export class BrowserTypeDispatcher extends Dispatcher<BrowserType, channels.BrowserTypeInitializer> implements channels.BrowserTypeChannel {
constructor(scope: DispatcherScope, browserType: BrowserTypeBase) {
constructor(scope: DispatcherScope, browserType: BrowserType) {
super(scope, browserType, 'BrowserType', {
executablePath: browserType.executablePath(),
name: browserType.name()
Expand Down
19 changes: 8 additions & 11 deletions src/server/browserType.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,19 +24,12 @@ import { ConnectionTransport, WebSocketTransport } from './transport';
import { BrowserOptions, Browser, BrowserProcess } from './browser';
import { launchProcess, Env, waitForLine, envArrayToObject } from './processLauncher';
import { PipeTransport } from './pipeTransport';
import { Progress, runAbortableTask } from './progress';
import { Progress, ProgressController } from './progress';
import * as types from './types';
import { TimeoutSettings } from '../utils/timeoutSettings';
import { validateHostRequirements } from './validateDependencies';
import { assert, isDebugMode } from '../utils/utils';

export interface BrowserType {
executablePath(): string;
name(): string;
launch(options?: types.LaunchOptions): Promise<Browser>;
launchPersistentContext(userDataDir: string, options?: types.LaunchPersistentOptions): Promise<BrowserContext>;
}

const mkdirAsync = util.promisify(fs.mkdir);
const mkdtempAsync = util.promisify(fs.mkdtemp);
const existsAsync = (path: string): Promise<boolean> => new Promise(resolve => fs.stat(path, err => resolve(!err)));
Expand All @@ -45,7 +38,7 @@ const VIDEOS_FOLDER = path.join(os.tmpdir(), 'playwright_videos-');

type WebSocketNotPipe = { webSocketRegex: RegExp, stream: 'stdout' | 'stderr' };

export abstract class BrowserTypeBase implements BrowserType {
export abstract class BrowserType {
private _name: string;
private _executablePath: string | undefined;
private _webSocketNotPipe: WebSocketNotPipe | null;
Expand Down Expand Up @@ -75,7 +68,9 @@ export abstract class BrowserTypeBase implements BrowserType {
assert(!(options as any).userDataDir, 'userDataDir option is not supported in `browserType.launch`. Use `browserType.launchPersistentContext` instead');
assert(!(options as any).port, 'Cannot specify a port without launching as a server.');
options = validateLaunchOptions(options);
const browser = await runAbortableTask(progress => this._innerLaunch(progress, options, undefined), TimeoutSettings.timeout(options), 'browser').catch(e => { throw this._rewriteStartupError(e); });
const controller = new ProgressController(TimeoutSettings.timeout(options));
controller.setLogName('browser');
const browser = await controller.run(progress => this._innerLaunch(progress, options, undefined).catch(e => { throw this._rewriteStartupError(e); }));
return browser;
}

Expand All @@ -84,7 +79,9 @@ export abstract class BrowserTypeBase implements BrowserType {
options = validateLaunchOptions(options);
const persistent: types.BrowserContextOptions = options;
validateBrowserContextOptions(persistent);
const browser = await runAbortableTask(progress => this._innerLaunch(progress, options, persistent, userDataDir), TimeoutSettings.timeout(options), 'browser').catch(e => { throw this._rewriteStartupError(e); });
const controller = new ProgressController(TimeoutSettings.timeout(options));
controller.setLogName('browser');
const browser = await controller.run(progress => this._innerLaunch(progress, options, persistent, userDataDir).catch(e => { throw this._rewriteStartupError(e); }));
return browser._defaultContext!;
}

Expand Down
4 changes: 2 additions & 2 deletions src/server/chromium/chromium.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,15 +21,15 @@ import { CRBrowser } from './crBrowser';
import { Env } from '../processLauncher';
import { kBrowserCloseMessageId } from './crConnection';
import { rewriteErrorMessage } from '../../utils/stackTrace';
import { BrowserTypeBase } from '../browserType';
import { BrowserType } from '../browserType';
import { ConnectionTransport, ProtocolRequest } from '../transport';
import type { BrowserDescriptor } from '../../utils/browserPaths';
import { CRDevTools } from './crDevTools';
import { BrowserOptions } from '../browser';
import * as types from '../types';
import { isDebugMode, getFromENV } from '../../utils/utils';

export class Chromium extends BrowserTypeBase {
export class Chromium extends BrowserType {
private _devtools: CRDevTools | undefined;
private _debugPort: number | undefined;

Expand Down
8 changes: 5 additions & 3 deletions src/server/chromium/videoRecorder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

import { launchProcess } from '../processLauncher';
import { ChildProcess } from 'child_process';
import { Progress, runAbortableTask } from '../progress';
import { Progress, ProgressController } from '../progress';
import * as types from '../types';
import * as path from 'path';
import * as os from 'os';
Expand All @@ -37,11 +37,13 @@ export class VideoRecorder {
if (!options.outputFile.endsWith('.webm'))
throw new Error('File must have .webm extension');

return await runAbortableTask(async progress => {
const controller = new ProgressController(0);
controller.setLogName('browser');
return await controller.run(async progress => {
const recorder = new VideoRecorder(progress);
await recorder._launch(options);
return recorder;
}, 0, 'browser');
});
}

private constructor(progress: Progress) {
Expand Down
8 changes: 5 additions & 3 deletions src/server/electron/electron.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import * as types from '../types';
import { launchProcess, waitForLine, envArrayToObject } from '../processLauncher';
import { BrowserContext } from '../browserContext';
import type {BrowserWindow} from 'electron';
import { runAbortableTask, ProgressController } from '../progress';
import { ProgressController } from '../progress';
import { EventEmitter } from 'events';
import { helper } from '../helper';
import { BrowserProcess } from '../browser';
Expand Down Expand Up @@ -147,7 +147,9 @@ export class Electron {
handleSIGTERM = true,
handleSIGHUP = true,
} = options;
return runAbortableTask(async progress => {
const controller = new ProgressController(TimeoutSettings.timeout(options));
controller.setLogName('browser');
return controller.run(async progress => {
let app: ElectronApplication | undefined = undefined;
const electronArguments = ['--inspect=0', '--remote-debugging-port=0', '--require', path.join(__dirname, 'electronLoader.js'), ...args];

Expand Down Expand Up @@ -188,6 +190,6 @@ export class Electron {
app = new ElectronApplication(browser, nodeConnection);
await app._init();
return app;
}, TimeoutSettings.timeout(options), 'browser');
});
}
}
4 changes: 2 additions & 2 deletions src/server/firefox/firefox.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,14 @@ import * as fs from 'fs';
import * as path from 'path';
import { FFBrowser } from './ffBrowser';
import { kBrowserCloseMessageId } from './ffConnection';
import { BrowserTypeBase } from '../browserType';
import { BrowserType } from '../browserType';
import { Env } from '../processLauncher';
import { ConnectionTransport } from '../transport';
import { BrowserOptions } from '../browser';
import { BrowserDescriptor } from '../../utils/browserPaths';
import * as types from '../types';

export class Firefox extends BrowserTypeBase {
export class Firefox extends BrowserType {
constructor(packagePath: string, browser: BrowserDescriptor) {
const webSocketRegex = /^Juggler listening on (ws:\/\/.*)$/;
super(packagePath, browser, { webSocketRegex, stream: 'stdout' });
Expand Down
13 changes: 8 additions & 5 deletions src/server/progress.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ export interface Progress {
throwIfAborted(): void;
}

export async function runAbortableTask<T>(task: (progress: Progress) => Promise<T>, timeout: number, logName: LogName = 'api'): Promise<T> {
const controller = new ProgressController(timeout, logName);
export async function runAbortableTask<T>(task: (progress: Progress) => Promise<T>, timeout: number): Promise<T> {
const controller = new ProgressController(timeout);
return controller.run(task);
}

Expand All @@ -47,14 +47,13 @@ export class ProgressController {
// Cleanups to be run only in the case of abort.
private _cleanups: (() => any)[] = [];

private _logName: LogName;
private _logName: LogName = 'api';
private _state: 'before' | 'running' | 'aborted' | 'finished' = 'before';
private _deadline: number;
private _timeout: number;
private _logRecordring: string[] = [];

constructor(timeout: number, logName: LogName = 'api') {
this._logName = logName;
constructor(timeout: number) {
this._timeout = timeout;
this._deadline = timeout ? monotonicTime() + timeout : 0;

Expand All @@ -63,6 +62,10 @@ export class ProgressController {
this._abortedPromise = new Promise(resolve => this._aborted = resolve);
}

setLogName(logName: LogName) {
this._logName = logName;
}

async run<T>(task: (progress: Progress) => Promise<T>): Promise<T> {
assert(this._state === 'before');
this._state = 'running';
Expand Down
4 changes: 2 additions & 2 deletions src/server/webkit/webkit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,13 @@ import { WKBrowser } from '../webkit/wkBrowser';
import { Env } from '../processLauncher';
import * as path from 'path';
import { kBrowserCloseMessageId } from './wkConnection';
import { BrowserTypeBase } from '../browserType';
import { BrowserType } from '../browserType';
import { ConnectionTransport } from '../transport';
import { BrowserOptions } from '../browser';
import { BrowserDescriptor } from '../../utils/browserPaths';
import * as types from '../types';

export class WebKit extends BrowserTypeBase {
export class WebKit extends BrowserType {
constructor(packagePath: string, browser: BrowserDescriptor) {
super(packagePath, browser, null /* use pipe not websocket */);
}
Expand Down

0 comments on commit ed3b00e

Please sign in to comment.