From 58fc6b40033ea204e8be57256b1315744175d55c Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Mon, 17 Aug 2020 16:19:21 -0700 Subject: [PATCH] chore: align some server-side methods with rpc calls (#3504) - Never write to console on the server side - we use stdout for communication. This includes logPolitely and deprecate. - Pass undefined instead of null in some BrowserContext methods. - Use explicit _setFileChooserIntercepted instead of on/off magic. --- src/browserContext.ts | 14 +++++----- src/chromium/crBrowser.ts | 8 +++--- src/firefox/ffBrowser.ts | 12 ++++----- src/helper.ts | 16 ------------ src/install/browserFetcher.ts | 10 +++++++- src/install/installer.ts | 8 +++--- src/page.ts | 18 +++---------- src/rpc/client/browserContext.ts | 3 +++ src/rpc/client/clientHelper.ts | 30 ++++++++++++++++++++++ src/rpc/server/browserContextDispatcher.ts | 4 +-- src/rpc/server/pageDispatcher.ts | 11 +++----- src/server/chromium.ts | 3 +-- src/server/electron.ts | 6 ++--- src/webkit/wkBrowser.ts | 8 +++--- 14 files changed, 78 insertions(+), 73 deletions(-) create mode 100644 src/rpc/client/clientHelper.ts diff --git a/src/browserContext.ts b/src/browserContext.ts index 577ebdaede..84e516101b 100644 --- a/src/browserContext.ts +++ b/src/browserContext.ts @@ -15,7 +15,7 @@ * limitations under the License. */ -import { isUnderTest, helper, deprecate} from './helper'; +import { helper } from './helper'; import * as network from './network'; import { Page, PageBinding } from './page'; import { TimeoutSettings } from './timeoutSettings'; @@ -38,10 +38,10 @@ export interface BrowserContext extends EventEmitter { clearCookies(): Promise; grantPermissions(permissions: string[], options?: { origin?: string }): Promise; clearPermissions(): Promise; - setGeolocation(geolocation: types.Geolocation | null): Promise; + setGeolocation(geolocation?: types.Geolocation): Promise; setExtraHTTPHeaders(headers: types.Headers): Promise; setOffline(offline: boolean): Promise; - setHTTPCredentials(httpCredentials: types.Credentials | null): Promise; + setHTTPCredentials(httpCredentials?: types.Credentials): Promise; addInitScript(script: Function | string | { path?: string, content?: string }, arg?: any): Promise; exposeBinding(name: string, playwrightBinding: frames.FunctionWithSource): Promise; route(url: types.URLMatch, handler: network.RouteHandler): Promise; @@ -101,8 +101,8 @@ export abstract class BrowserContextBase extends EventEmitter implements Browser abstract clearCookies(): Promise; abstract _doGrantPermissions(origin: string, permissions: string[]): Promise; abstract _doClearPermissions(): Promise; - abstract setGeolocation(geolocation: types.Geolocation | null): Promise; - abstract _doSetHTTPCredentials(httpCredentials: types.Credentials | null): Promise; + abstract setGeolocation(geolocation?: types.Geolocation): Promise; + abstract _doSetHTTPCredentials(httpCredentials?: types.Credentials): Promise; abstract setExtraHTTPHeaders(headers: types.Headers): Promise; abstract setOffline(offline: boolean): Promise; abstract _doAddInitScript(expression: string): Promise; @@ -117,9 +117,7 @@ export abstract class BrowserContextBase extends EventEmitter implements Browser return await this._doCookies(urls as string[]); } - setHTTPCredentials(httpCredentials: types.Credentials | null): Promise { - if (!isUnderTest()) - deprecate(`context.setHTTPCredentials`, `warning: method |context.setHTTPCredentials()| is deprecated. Instead of changing credentials, create another browser context with new credentials.`); + setHTTPCredentials(httpCredentials?: types.Credentials): Promise { return this._doSetHTTPCredentials(httpCredentials); } diff --git a/src/chromium/crBrowser.ts b/src/chromium/crBrowser.ts index 7e7178a6dc..a5b6f83070 100644 --- a/src/chromium/crBrowser.ts +++ b/src/chromium/crBrowser.ts @@ -380,10 +380,10 @@ export class CRBrowserContext extends BrowserContextBase { await this._browser._session.send('Browser.resetPermissions', { browserContextId: this._browserContextId || undefined }); } - async setGeolocation(geolocation: types.Geolocation | null): Promise { + async setGeolocation(geolocation?: types.Geolocation): Promise { if (geolocation) geolocation = verifyGeolocation(geolocation); - this._options.geolocation = geolocation || undefined; + this._options.geolocation = geolocation; for (const page of this.pages()) await (page._delegate as CRPage).updateGeolocation(); } @@ -400,8 +400,8 @@ export class CRBrowserContext extends BrowserContextBase { await (page._delegate as CRPage).updateOffline(); } - async _doSetHTTPCredentials(httpCredentials: types.Credentials | null): Promise { - this._options.httpCredentials = httpCredentials || undefined; + async _doSetHTTPCredentials(httpCredentials?: types.Credentials): Promise { + this._options.httpCredentials = httpCredentials; for (const page of this.pages()) await (page._delegate as CRPage).updateHttpCredentials(); } diff --git a/src/firefox/ffBrowser.ts b/src/firefox/ffBrowser.ts index ceb5e34d43..7b094ab88f 100644 --- a/src/firefox/ffBrowser.ts +++ b/src/firefox/ffBrowser.ts @@ -288,11 +288,11 @@ export class FFBrowserContext extends BrowserContextBase { await this._browser._connection.send('Browser.resetPermissions', { browserContextId: this._browserContextId || undefined }); } - async setGeolocation(geolocation: types.Geolocation | null): Promise { + async setGeolocation(geolocation?: types.Geolocation): Promise { if (geolocation) geolocation = verifyGeolocation(geolocation); - this._options.geolocation = geolocation || undefined; - await this._browser._connection.send('Browser.setGeolocationOverride', { browserContextId: this._browserContextId || undefined, geolocation }); + this._options.geolocation = geolocation; + await this._browser._connection.send('Browser.setGeolocationOverride', { browserContextId: this._browserContextId || undefined, geolocation: geolocation || null }); } async setExtraHTTPHeaders(headers: types.Headers): Promise { @@ -308,9 +308,9 @@ export class FFBrowserContext extends BrowserContextBase { await this._browser._connection.send('Browser.setOnlineOverride', { browserContextId: this._browserContextId || undefined, override: offline ? 'offline' : 'online' }); } - async _doSetHTTPCredentials(httpCredentials: types.Credentials | null): Promise { - this._options.httpCredentials = httpCredentials || undefined; - await this._browser._connection.send('Browser.setHTTPCredentials', { browserContextId: this._browserContextId || undefined, credentials: httpCredentials }); + async _doSetHTTPCredentials(httpCredentials?: types.Credentials): Promise { + this._options.httpCredentials = httpCredentials; + await this._browser._connection.send('Browser.setHTTPCredentials', { browserContextId: this._browserContextId || undefined, credentials: httpCredentials || null }); } async _doAddInitScript(source: string) { diff --git a/src/helper.ts b/src/helper.ts index 9f5a169912..c624d5bc06 100644 --- a/src/helper.ts +++ b/src/helper.ts @@ -38,14 +38,6 @@ export type Listener = (...args: any[]) => void; const isInDebugMode = !!getFromENV('PWDEBUG'); -const deprecatedHits = new Set(); -export function deprecate(methodName: string, message: string) { - if (deprecatedHits.has(methodName)) - return; - deprecatedHits.add(methodName); - console.warn(message); -} - class Helper { static evaluationString(fun: Function | string, ...args: any[]): string { @@ -361,14 +353,6 @@ export function getFromENV(name: string) { return value; } -export function logPolitely(toBeLogged: string) { - const logLevel = process.env.npm_config_loglevel; - const logLevelDisplay = ['silent', 'error', 'warn'].indexOf(logLevel || '') > -1; - - if (!logLevelDisplay) - console.log(toBeLogged); // eslint-disable-line no-console -} - const escapeGlobChars = new Set(['/', '$', '^', '+', '.', '(', ')', '=', '!', '|']); export const helper = Helper; diff --git a/src/install/browserFetcher.ts b/src/install/browserFetcher.ts index 7b9753e1a0..4c5f6d7e41 100644 --- a/src/install/browserFetcher.ts +++ b/src/install/browserFetcher.ts @@ -23,7 +23,7 @@ import * as ProgressBar from 'progress'; import { getProxyForUrl } from 'proxy-from-env'; import * as URL from 'url'; import * as util from 'util'; -import { assert, logPolitely, getFromENV } from '../helper'; +import { assert, getFromENV } from '../helper'; import * as browserPaths from './browserPaths'; import { BrowserName, BrowserPlatform, BrowserDescriptor } from './browserPaths'; @@ -250,3 +250,11 @@ function httpRequest(url: string, method: string, response: (r: any) => void) { request.end(); return request; } + +export function logPolitely(toBeLogged: string) { + const logLevel = process.env.npm_config_loglevel; + const logLevelDisplay = ['silent', 'error', 'warn'].indexOf(logLevel || '') > -1; + + if (!logLevelDisplay) + console.log(toBeLogged); // eslint-disable-line no-console +} diff --git a/src/install/installer.ts b/src/install/installer.ts index df5d43cb6a..c200b31e7f 100644 --- a/src/install/installer.ts +++ b/src/install/installer.ts @@ -15,7 +15,7 @@ */ import * as crypto from 'crypto'; -import { getFromENV, logPolitely } from '../helper'; +import { getFromENV } from '../helper'; import * as fs from 'fs'; import * as path from 'path'; import * as util from 'util'; @@ -36,7 +36,7 @@ export async function installBrowsersWithProgressBar(packagePath: string) { const linksDir = path.join(browsersPath, '.links'); if (getFromENV('PLAYWRIGHT_SKIP_BROWSER_DOWNLOAD')) { - logPolitely('Skipping browsers download because `PLAYWRIGHT_SKIP_BROWSER_DOWNLOAD` env variable is set'); + browserFetcher.logPolitely('Skipping browsers download because `PLAYWRIGHT_SKIP_BROWSER_DOWNLOAD` env variable is set'); return false; } await fsMkdirAsync(linksDir, { recursive: true }); @@ -65,7 +65,7 @@ async function validateCache(packagePath: string, browsersPath: string, linksDir } } catch (e) { if (linkTarget) - logPolitely('Failed to process descriptor at ' + linkTarget); + browserFetcher.logPolitely('Failed to process descriptor at ' + linkTarget); await fsUnlinkAsync(linkPath).catch(e => {}); } } @@ -77,7 +77,7 @@ async function validateCache(packagePath: string, browsersPath: string, linksDir for (const browserPath of usedBrowserPaths) directories.delete(browserPath); for (const directory of directories) { - logPolitely('Removing unused browser at ' + directory); + browserFetcher.logPolitely('Removing unused browser at ' + directory); await removeFolderAsync(directory).catch(e => {}); } diff --git a/src/page.ts b/src/page.ts index 4e8c7d7baf..ba98b81ac0 100644 --- a/src/page.ts +++ b/src/page.ts @@ -17,7 +17,7 @@ import * as dom from './dom'; import * as frames from './frames'; -import { assert, helper, Listener, debugLogger } from './helper'; +import { assert, helper, debugLogger } from './helper'; import * as input from './input'; import * as js from './javascript'; import * as network from './network'; @@ -386,20 +386,8 @@ export class Page extends EventEmitter { } } - on(event: string | symbol, listener: Listener): this { - if (event === Events.Page.FileChooser) { - if (!this.listenerCount(event)) - this._delegate.setFileChooserIntercepted(true); - } - super.on(event, listener); - return this; - } - - removeListener(event: string | symbol, listener: Listener): this { - super.removeListener(event, listener); - if (event === Events.Page.FileChooser && !this.listenerCount(event)) - this._delegate.setFileChooserIntercepted(false); - return this; + async _setFileChooserIntercepted(enabled: boolean): Promise { + await this._delegate.setFileChooserIntercepted(enabled); } } diff --git a/src/rpc/client/browserContext.ts b/src/rpc/client/browserContext.ts index e74cfc2437..24cd5ae0e3 100644 --- a/src/rpc/client/browserContext.ts +++ b/src/rpc/client/browserContext.ts @@ -21,6 +21,7 @@ import * as network from './network'; import { BrowserContextChannel, BrowserContextInitializer } from '../channels'; import { ChannelOwner } from './channelOwner'; import { helper } from '../../helper'; +import { isUnderTest, deprecate } from './clientHelper'; import { Browser } from './browser'; import { Events } from './events'; import { TimeoutSettings } from '../../timeoutSettings'; @@ -157,6 +158,8 @@ export class BrowserContext extends ChannelOwner { + if (!isUnderTest()) + deprecate(`context.setHTTPCredentials`, `warning: method |context.setHTTPCredentials()| is deprecated. Instead of changing credentials, create another browser context with new credentials.`); return this._wrapApiCall('browserContext.setHTTPCredentials', async () => { await this._channel.setHTTPCredentials({ httpCredentials: httpCredentials || undefined }); }); diff --git a/src/rpc/client/clientHelper.ts b/src/rpc/client/clientHelper.ts new file mode 100644 index 0000000000..53809acd01 --- /dev/null +++ b/src/rpc/client/clientHelper.ts @@ -0,0 +1,30 @@ +/** + * Copyright 2017 Google Inc. All rights reserved. + * Modifications copyright (c) Microsoft Corporation. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { isUnderTest as commonIsUnderTest } from '../../helper'; + +const deprecatedHits = new Set(); +export function deprecate(methodName: string, message: string) { + if (deprecatedHits.has(methodName)) + return; + deprecatedHits.add(methodName); + console.warn(message); +} + +export function isUnderTest() { + return commonIsUnderTest(); +} diff --git a/src/rpc/server/browserContextDispatcher.ts b/src/rpc/server/browserContextDispatcher.ts index 851c15bc77..1130214408 100644 --- a/src/rpc/server/browserContextDispatcher.ts +++ b/src/rpc/server/browserContextDispatcher.ts @@ -92,7 +92,7 @@ export class BrowserContextDispatcher extends Dispatcher { - await this._context.setGeolocation(params.geolocation || null); + await this._context.setGeolocation(params.geolocation); } async setExtraHTTPHeaders(params: { headers: types.HeadersArray }): Promise { @@ -104,7 +104,7 @@ export class BrowserContextDispatcher extends Dispatcher { - await this._context.setHTTPCredentials(params.httpCredentials || null); + await this._context.setHTTPCredentials(params.httpCredentials); } async addInitScript(params: { source: string }): Promise { diff --git a/src/rpc/server/pageDispatcher.ts b/src/rpc/server/pageDispatcher.ts index bd43139fb4..15c68d2f35 100644 --- a/src/rpc/server/pageDispatcher.ts +++ b/src/rpc/server/pageDispatcher.ts @@ -36,7 +36,6 @@ import { CRCoverage } from '../../chromium/crCoverage'; export class PageDispatcher extends Dispatcher implements PageChannel { private _page: Page; - private _onFileChooser: (fileChooser: FileChooser) => void; constructor(scope: DispatcherScope, page: Page) { // TODO: theoretically, there could be more than one frame already. @@ -53,11 +52,10 @@ export class PageDispatcher extends Dispatcher implements page.on(Events.Page.DOMContentLoaded, () => this._dispatchEvent('domcontentloaded')); page.on(Events.Page.Dialog, dialog => this._dispatchEvent('dialog', { dialog: new DialogDispatcher(this._scope, dialog) })); page.on(Events.Page.Download, dialog => this._dispatchEvent('download', { download: new DownloadDispatcher(this._scope, dialog) })); - // We add this listener lazily, to avoid intercepting file chooser when noone listens. - this._onFileChooser = fileChooser => this._dispatchEvent('fileChooser', { + this._page.on(Events.Page.FileChooser, (fileChooser: FileChooser) => this._dispatchEvent('fileChooser', { element: new ElementHandleDispatcher(this._scope, fileChooser.element()), isMultiple: fileChooser.isMultiple() - }); + })); page.on(Events.Page.FrameAttached, frame => this._onFrameAttached(frame)); page.on(Events.Page.FrameDetached, frame => this._onFrameDetached(frame)); page.on(Events.Page.Load, () => this._dispatchEvent('load')); @@ -143,10 +141,7 @@ export class PageDispatcher extends Dispatcher implements } async setFileChooserInterceptedNoReply(params: { intercepted: boolean }) { - if (params.intercepted) - this._page.on(Events.Page.FileChooser, this._onFileChooser); - else - this._page.removeListener(Events.Page.FileChooser, this._onFileChooser); + await this._page._setFileChooserIntercepted(params.intercepted); } async keyboardDown(params: { key: string }): Promise { diff --git a/src/server/chromium.ts b/src/server/chromium.ts index 164b5544c0..ac93c7ff2e 100644 --- a/src/server/chromium.ts +++ b/src/server/chromium.ts @@ -17,7 +17,7 @@ import * as path from 'path'; import * as os from 'os'; -import { getFromENV, logPolitely, helper } from '../helper'; +import { getFromENV, helper } from '../helper'; import { CRBrowser } from '../chromium/crBrowser'; import { Env } from './processLauncher'; import { kBrowserCloseMessageId } from '../chromium/crConnection'; @@ -39,7 +39,6 @@ export class Chromium extends BrowserTypeBase { if (debugPort !== undefined) { if (Number.isNaN(debugPort)) throw new Error(`PLAYWRIGHT_CHROMIUM_DEBUG_PORT must be a number, but is set to "${debugPortStr}"`); - logPolitely(`NOTE: Chromium will be launched in debug mode on port ${debugPort}`); } super(packagePath, browser, debugPort ? { webSocketRegex: /^DevTools listening on (ws:\/\/.*)$/, stream: 'stderr' } : null); diff --git a/src/server/electron.ts b/src/server/electron.ts index bd7e4d7f89..efa4b4d33c 100644 --- a/src/server/electron.ts +++ b/src/server/electron.ts @@ -106,7 +106,7 @@ export class ElectronApplication extends EventEmitter { return page; } - return await this.waitForEvent(ElectronEvents.ElectronApplication.Window, (page: ElectronPage) => page._browserWindowId === windowId); + return await this._waitForEvent(ElectronEvents.ElectronApplication.Window, (page: ElectronPage) => page._browserWindowId === windowId); } context(): BrowserContext { @@ -114,13 +114,13 @@ export class ElectronApplication extends EventEmitter { } async close() { - const closed = this.waitForEvent(ElectronEvents.ElectronApplication.Close); + const closed = this._waitForEvent(ElectronEvents.ElectronApplication.Close); await this._nodeElectronHandle!.evaluate(({ app }) => app.quit()); this._nodeConnection.close(); await closed; } - async waitForEvent(event: string, predicate?: Function): Promise { + private async _waitForEvent(event: string, predicate?: Function): Promise { const progressController = new ProgressController(this._timeoutSettings.timeout({})); if (event !== ElectronEvents.ElectronApplication.Close) this._browserContext._closePromise.then(error => progressController.abort(error)); diff --git a/src/webkit/wkBrowser.ts b/src/webkit/wkBrowser.ts index 2a7a77e2ec..ee66206bfe 100644 --- a/src/webkit/wkBrowser.ts +++ b/src/webkit/wkBrowser.ts @@ -286,10 +286,10 @@ export class WKBrowserContext extends BrowserContextBase { await Promise.all(this.pages().map(page => (page._delegate as WKPage)._clearPermissions())); } - async setGeolocation(geolocation: types.Geolocation | null): Promise { + async setGeolocation(geolocation?: types.Geolocation): Promise { if (geolocation) geolocation = verifyGeolocation(geolocation); - this._options.geolocation = geolocation || undefined; + this._options.geolocation = geolocation; const payload: any = geolocation ? { ...geolocation, timestamp: Date.now() } : undefined; await this._browser._browserSession.send('Playwright.setGeolocationOverride', { browserContextId: this._browserContextId, geolocation: payload }); } @@ -306,8 +306,8 @@ export class WKBrowserContext extends BrowserContextBase { await (page._delegate as WKPage).updateOffline(); } - async _doSetHTTPCredentials(httpCredentials: types.Credentials | null): Promise { - this._options.httpCredentials = httpCredentials || undefined; + async _doSetHTTPCredentials(httpCredentials?: types.Credentials): Promise { + this._options.httpCredentials = httpCredentials; for (const page of this.pages()) await (page._delegate as WKPage).updateHttpCredentials(); }