From 73ea4ce524f3af193957e6285217168568c619a3 Mon Sep 17 00:00:00 2001 From: Max Schmitt Date: Fri, 13 Sep 2024 16:41:13 +0200 Subject: [PATCH] review feedback --- .../playwright-core/src/server/android/android.ts | 3 ++- .../playwright-core/src/server/bidi/bidiBrowser.ts | 4 ++-- packages/playwright-core/src/server/browser.ts | 4 ++-- .../playwright-core/src/server/browserContext.ts | 8 ++++---- packages/playwright-core/src/server/browserType.ts | 4 ++-- .../playwright-core/src/server/chromium/chromium.ts | 3 +-- .../playwright-core/src/server/chromium/crPage.ts | 4 ++-- .../playwright-core/src/server/electron/electron.ts | 3 ++- packages/playwright-core/src/server/fetch.ts | 2 +- .../playwright-core/src/server/firefox/ffBrowser.ts | 6 +++--- .../src/server/socksClientCertificatesInterceptor.ts | 12 ++++++------ packages/playwright-core/src/server/types.ts | 2 +- .../playwright-core/src/server/webkit/wkBrowser.ts | 4 ++-- 13 files changed, 30 insertions(+), 29 deletions(-) diff --git a/packages/playwright-core/src/server/android/android.ts b/packages/playwright-core/src/server/android/android.ts index 0b4cb331b0..1af083916c 100644 --- a/packages/playwright-core/src/server/android/android.ts +++ b/packages/playwright-core/src/server/android/android.ts @@ -29,6 +29,7 @@ import { validateBrowserContextOptions } from '../browserContext'; import { ProgressController } from '../progress'; import { CRBrowser } from '../chromium/crBrowser'; import { helper } from '../helper'; +import type * as types from '../types'; import { PipeTransport } from '../../protocol/transport'; import { RecentLogsCollector } from '../../utils/debugLogger'; import { gracefullyCloseSet } from '../../utils/processLauncher'; @@ -309,7 +310,7 @@ export class AndroidDevice extends SdkObject { return await this._connectToBrowser(socketName); } - private async _connectToBrowser(socketName: string, options: channels.BrowserNewContextParams = {}): Promise { + private async _connectToBrowser(socketName: string, options: types.BrowserContextOptions = {}): Promise { const socket = await this._waitForLocalAbstract(socketName); const androidBrowser = new AndroidBrowser(this, socket); await androidBrowser._init(); diff --git a/packages/playwright-core/src/server/bidi/bidiBrowser.ts b/packages/playwright-core/src/server/bidi/bidiBrowser.ts index 0c658a82b4..cc98e2ff3a 100644 --- a/packages/playwright-core/src/server/bidi/bidiBrowser.ts +++ b/packages/playwright-core/src/server/bidi/bidiBrowser.ts @@ -111,7 +111,7 @@ export class BidiBrowser extends Browser { this._didClose(); } - async doCreateNewContext(options: channels.BrowserNewContextParams): Promise { + async doCreateNewContext(options: types.BrowserContextOptions): Promise { const { userContext } = await this._browserSession.send('browser.createUserContext', {}); const context = new BidiBrowserContext(this, userContext, options); await context._initialize(); @@ -190,7 +190,7 @@ export class BidiBrowser extends Browser { export class BidiBrowserContext extends BrowserContext { declare readonly _browser: BidiBrowser; - constructor(browser: BidiBrowser, browserContextId: string | undefined, options: channels.BrowserNewContextParams) { + constructor(browser: BidiBrowser, browserContextId: string | undefined, options: types.BrowserContextOptions) { super(browser, options, browserContextId); this._authenticateProxyViaHeader(); } diff --git a/packages/playwright-core/src/server/browser.ts b/packages/playwright-core/src/server/browser.ts index faa1a16d3b..04a23e7eac 100644 --- a/packages/playwright-core/src/server/browser.ts +++ b/packages/playwright-core/src/server/browser.ts @@ -41,7 +41,7 @@ export type BrowserOptions = { downloadsPath: string, tracesDir: string, headful?: boolean, - persistent?: channels.BrowserNewContextParams, // Undefined means no persistent context. + persistent?: types.BrowserContextOptions, // Undefined means no persistent context. browserProcess: BrowserProcess, customExecutablePath?: string; proxy?: ProxySettings, @@ -85,7 +85,7 @@ export abstract class Browser extends SdkObject { const clientCertificatesProxy = await createClientCertificatesProxyIfNeeded(options, this.options); if (clientCertificatesProxy) { options.proxyOverride = await clientCertificatesProxy.listen(); - options.ignoreHTTPSErrorsOverride = true; + options.internalIgnoreHTTPSErrors = true; } let context; try { diff --git a/packages/playwright-core/src/server/browserContext.ts b/packages/playwright-core/src/server/browserContext.ts index 75593f9f22..e1166f3e97 100644 --- a/packages/playwright-core/src/server/browserContext.ts +++ b/packages/playwright-core/src/server/browserContext.ts @@ -93,7 +93,7 @@ export abstract class BrowserContext extends SdkObject { readonly clock: Clock; _clientCertificatesProxy: ClientCertificatesProxy | undefined; - constructor(browser: Browser, options: channels.BrowserNewContextParams, browserContextId: string | undefined) { + constructor(browser: Browser, options: types.BrowserContextOptions, browserContextId: string | undefined) { super(browser, 'browser-context'); this.attribution.context = this; this._browser = browser; @@ -659,7 +659,7 @@ export function assertBrowserContextIsNotOwned(context: BrowserContext) { } } -export async function createClientCertificatesProxyIfNeeded(options: channels.BrowserNewContextOptions, browserOptions?: BrowserOptions) { +export async function createClientCertificatesProxyIfNeeded(options: types.BrowserContextOptions, browserOptions?: BrowserOptions) { if (!options.clientCertificates?.length) return; if ((options.proxy?.server && options.proxy?.server !== 'per-context') || (browserOptions?.proxy?.server && browserOptions?.proxy?.server !== 'http://per-context')) @@ -668,7 +668,7 @@ export async function createClientCertificatesProxyIfNeeded(options: channels.Br return new ClientCertificatesProxy(options); } -export function validateBrowserContextOptions(options: channels.BrowserNewContextParams, browserOptions: BrowserOptions) { +export function validateBrowserContextOptions(options: types.BrowserContextOptions, browserOptions: BrowserOptions) { if (options.noDefaultViewport && options.deviceScaleFactor !== undefined) throw new Error(`"deviceScaleFactor" option is not supported with null "viewport"`); if (options.noDefaultViewport && !!options.isMobile) @@ -717,7 +717,7 @@ export function verifyGeolocation(geolocation?: types.Geolocation) { throw new Error(`geolocation.accuracy: precondition 0 <= ACCURACY failed.`); } -export function verifyClientCertificates(clientCertificates?: channels.BrowserNewContextParams['clientCertificates']) { +export function verifyClientCertificates(clientCertificates?: types.BrowserContextOptions['clientCertificates']) { if (!clientCertificates) return; for (const cert of clientCertificates) { diff --git a/packages/playwright-core/src/server/browserType.ts b/packages/playwright-core/src/server/browserType.ts index 8e7c9cbd4d..b651273758 100644 --- a/packages/playwright-core/src/server/browserType.ts +++ b/packages/playwright-core/src/server/browserType.ts @@ -111,7 +111,7 @@ export abstract class BrowserType extends SdkObject { return browser._defaultContext!; } - async _innerLaunchWithRetries(progress: Progress, options: types.LaunchOptions, persistent: channels.BrowserNewContextParams | undefined, protocolLogger: types.ProtocolLogger, userDataDir?: string): Promise { + async _innerLaunchWithRetries(progress: Progress, options: types.LaunchOptions, persistent: types.BrowserContextOptions | undefined, protocolLogger: types.ProtocolLogger, userDataDir?: string): Promise { try { return await this._innerLaunch(progress, options, persistent, protocolLogger, userDataDir); } catch (error) { @@ -125,7 +125,7 @@ export abstract class BrowserType extends SdkObject { } } - async _innerLaunch(progress: Progress, options: types.LaunchOptions, persistent: channels.BrowserNewContextParams | undefined, protocolLogger: types.ProtocolLogger, maybeUserDataDir?: string): Promise { + async _innerLaunch(progress: Progress, options: types.LaunchOptions, persistent: types.BrowserContextOptions | undefined, protocolLogger: types.ProtocolLogger, maybeUserDataDir?: string): Promise { options.proxy = options.proxy ? normalizeProxySettings(options.proxy) : undefined; const browserLogsCollector = new RecentLogsCollector(); const { browserProcess, userDataDir, artifactsDir, transport } = await this._launchProcess(progress, options, !!persistent, browserLogsCollector, maybeUserDataDir); diff --git a/packages/playwright-core/src/server/chromium/chromium.ts b/packages/playwright-core/src/server/chromium/chromium.ts index 625ee75fc7..30bd2871b8 100644 --- a/packages/playwright-core/src/server/chromium/chromium.ts +++ b/packages/playwright-core/src/server/chromium/chromium.ts @@ -31,7 +31,6 @@ import { CRDevTools } from './crDevTools'; import type { BrowserOptions, BrowserProcess } from '../browser'; import { Browser } from '../browser'; import type * as types from '../types'; -import type * as channels from '@protocol/channels'; import type { HTTPRequestParams } from '../../utils/network'; import { fetchData } from '../../utils/network'; import { getUserAgent } from '../../utils/userAgent'; @@ -98,7 +97,7 @@ export class Chromium extends BrowserType { await cleanedUp; }; const browserProcess: BrowserProcess = { close: doClose, kill: doClose }; - const persistent: channels.BrowserNewContextParams = { noDefaultViewport: true }; + const persistent: types.BrowserContextOptions = { noDefaultViewport: true }; const browserOptions: BrowserOptions = { slowMo: options.slowMo, name: 'chromium', diff --git a/packages/playwright-core/src/server/chromium/crPage.ts b/packages/playwright-core/src/server/chromium/crPage.ts index 82c54c9d0c..5a7fb5e4af 100644 --- a/packages/playwright-core/src/server/chromium/crPage.ts +++ b/packages/playwright-core/src/server/chromium/crPage.ts @@ -543,7 +543,7 @@ class FrameSession { const options = this._crPage._browserContext._options; if (options.bypassCSP) promises.push(this._client.send('Page.setBypassCSP', { enabled: true })); - if (options.ignoreHTTPSErrors || options.ignoreHTTPSErrorsOverride) + if (options.ignoreHTTPSErrors || options.internalIgnoreHTTPSErrors) promises.push(this._client.send('Security.setIgnoreCertificateErrors', { ignore: true })); if (this._isMainFrame()) promises.push(this._updateViewport()); @@ -1213,7 +1213,7 @@ async function emulateTimezone(session: CRSession, timezoneId: string) { const contextDelegateSymbol = Symbol('delegate'); // Chromium reference: https://source.chromium.org/chromium/chromium/src/+/main:components/embedder_support/user_agent_utils.cc;l=434;drc=70a6711e08e9f9e0d8e4c48e9ba5cab62eb010c2 -function calculateUserAgentMetadata(options: channels.BrowserNewContextParams) { +function calculateUserAgentMetadata(options: types.BrowserContextOptions) { const ua = options.userAgent; if (!ua) return undefined; diff --git a/packages/playwright-core/src/server/electron/electron.ts b/packages/playwright-core/src/server/electron/electron.ts index b8f361b48a..1606c407d5 100644 --- a/packages/playwright-core/src/server/electron/electron.ts +++ b/packages/playwright-core/src/server/electron/electron.ts @@ -36,6 +36,7 @@ import type { BrowserWindow } from 'electron'; import type { Progress } from '../progress'; import { ProgressController } from '../progress'; import { helper } from '../helper'; +import type * as types from '../types'; import { eventsHelper } from '../../utils/eventsHelper'; import type { BrowserOptions, BrowserProcess } from '../browser'; import type { Playwright } from '../playwright'; @@ -265,7 +266,7 @@ export class Electron extends SdkObject { close: gracefullyClose, kill }; - const contextOptions: channels.BrowserNewContextParams = { + const contextOptions: types.BrowserContextOptions = { ...options, noDefaultViewport: true, }; diff --git a/packages/playwright-core/src/server/fetch.ts b/packages/playwright-core/src/server/fetch.ts index 7a6102a50d..5d00dc05a6 100644 --- a/packages/playwright-core/src/server/fetch.ts +++ b/packages/playwright-core/src/server/fetch.ts @@ -50,7 +50,7 @@ type FetchRequestOptions = { timeoutSettings: TimeoutSettings; ignoreHTTPSErrors?: boolean; baseURL?: string; - clientCertificates?: channels.BrowserNewContextOptions['clientCertificates']; + clientCertificates?: types.BrowserContextOptions['clientCertificates']; }; type HeadersObject = Readonly<{ [name: string]: string }>; diff --git a/packages/playwright-core/src/server/firefox/ffBrowser.ts b/packages/playwright-core/src/server/firefox/ffBrowser.ts index eed3d6beb7..b26a2850ee 100644 --- a/packages/playwright-core/src/server/firefox/ffBrowser.ts +++ b/packages/playwright-core/src/server/firefox/ffBrowser.ts @@ -89,7 +89,7 @@ export class FFBrowser extends Browser { return !this._connection._closed; } - async doCreateNewContext(options: channels.BrowserNewContextParams): Promise { + async doCreateNewContext(options: types.BrowserContextOptions): Promise { if (options.isMobile) throw new Error('options.isMobile is not supported in Firefox'); const { browserContextId } = await this.session.send('Browser.createBrowserContext', { removeOnDetach: true }); @@ -173,7 +173,7 @@ export class FFBrowser extends Browser { export class FFBrowserContext extends BrowserContext { declare readonly _browser: FFBrowser; - constructor(browser: FFBrowser, browserContextId: string | undefined, options: channels.BrowserNewContextParams) { + constructor(browser: FFBrowser, browserContextId: string | undefined, options: types.BrowserContextOptions) { super(browser, options, browserContextId); } @@ -206,7 +206,7 @@ export class FFBrowserContext extends BrowserContext { promises.push(this._browser.session.send('Browser.setUserAgentOverride', { browserContextId, userAgent: this._options.userAgent })); if (this._options.bypassCSP) promises.push(this._browser.session.send('Browser.setBypassCSP', { browserContextId, bypassCSP: true })); - if (this._options.ignoreHTTPSErrors || this._options.ignoreHTTPSErrorsOverride) + if (this._options.ignoreHTTPSErrors || this._options.internalIgnoreHTTPSErrors) promises.push(this._browser.session.send('Browser.setIgnoreHTTPSErrors', { browserContextId, ignoreHTTPSErrors: true })); if (this._options.javaScriptEnabled === false) promises.push(this._browser.session.send('Browser.setJavaScriptDisabled', { browserContextId, javaScriptDisabled: true })); diff --git a/packages/playwright-core/src/server/socksClientCertificatesInterceptor.ts b/packages/playwright-core/src/server/socksClientCertificatesInterceptor.ts index 3f279262b9..6d4b334dba 100644 --- a/packages/playwright-core/src/server/socksClientCertificatesInterceptor.ts +++ b/packages/playwright-core/src/server/socksClientCertificatesInterceptor.ts @@ -23,7 +23,7 @@ import { createSocket, createTLSSocket } from '../utils/happy-eyeballs'; import { escapeHTML, generateSelfSignedCertificate, ManualPromise, rewriteErrorMessage } from '../utils'; import type { SocksSocketClosedPayload, SocksSocketDataPayload, SocksSocketRequestedPayload } from '../common/socksProxy'; import { SocksProxy } from '../common/socksProxy'; -import type * as channels from '@protocol/channels'; +import type * as types from './types'; import { debugLogger } from '../utils/debugLogger'; let dummyServerTlsOptions: tls.TlsOptions | undefined = undefined; @@ -235,7 +235,7 @@ export class ClientCertificatesProxy { alpnCache: ALPNCache; constructor( - contextOptions: Pick + contextOptions: Pick ) { this.alpnCache = new ALPNCache(); this.ignoreHTTPSErrors = contextOptions.ignoreHTTPSErrors; @@ -261,9 +261,9 @@ export class ClientCertificatesProxy { loadDummyServerCertsIfNeeded(); } - _initSecureContexts(clientCertificates: channels.BrowserNewContextOptions['clientCertificates']) { + _initSecureContexts(clientCertificates: types.BrowserContextOptions['clientCertificates']) { // Step 1. Group certificates by origin. - const origin2certs = new Map(); + const origin2certs = new Map(); for (const cert of clientCertificates || []) { const origin = normalizeOrigin(cert.origin); const certs = origin2certs.get(origin) || []; @@ -301,7 +301,7 @@ function normalizeOrigin(origin: string): string { } function convertClientCertificatesToTLSOptions( - clientCertificates: channels.BrowserNewContextOptions['clientCertificates'] + clientCertificates: types.BrowserContextOptions['clientCertificates'] ): Pick | undefined { if (!clientCertificates || !clientCertificates.length) return; @@ -322,7 +322,7 @@ function convertClientCertificatesToTLSOptions( } export function getMatchingTLSOptionsForOrigin( - clientCertificates: channels.BrowserNewContextOptions['clientCertificates'], + clientCertificates: types.BrowserContextOptions['clientCertificates'], origin: string ): Pick | undefined { const matchingCerts = clientCertificates?.filter(c => diff --git a/packages/playwright-core/src/server/types.ts b/packages/playwright-core/src/server/types.ts index 8daadc7e8a..b58ea5af83 100644 --- a/packages/playwright-core/src/server/types.ts +++ b/packages/playwright-core/src/server/types.ts @@ -157,7 +157,7 @@ export type LaunchOptions = channels.BrowserTypeLaunchOptions & { export type BrowserContextOptions = channels.BrowserNewContextOptions & { proxyOverride?: ProxySettings; - ignoreHTTPSErrorsOverride?: boolean; + internalIgnoreHTTPSErrors?: boolean; }; export type ProtocolLogger = (direction: 'send' | 'receive', message: object) => void; diff --git a/packages/playwright-core/src/server/webkit/wkBrowser.ts b/packages/playwright-core/src/server/webkit/wkBrowser.ts index f0c337266a..c9bda10ddd 100644 --- a/packages/playwright-core/src/server/webkit/wkBrowser.ts +++ b/packages/playwright-core/src/server/webkit/wkBrowser.ts @@ -207,7 +207,7 @@ export class WKBrowser extends Browser { export class WKBrowserContext extends BrowserContext { declare readonly _browser: WKBrowser; - constructor(browser: WKBrowser, browserContextId: string | undefined, options: channels.BrowserNewContextParams) { + constructor(browser: WKBrowser, browserContextId: string | undefined, options: types.BrowserContextOptions) { super(browser, options, browserContextId); this._validateEmulatedViewport(options.viewport); this._authenticateProxyViaHeader(); @@ -222,7 +222,7 @@ export class WKBrowserContext extends BrowserContext { downloadPath: this._browser.options.downloadsPath, browserContextId })); - if (this._options.ignoreHTTPSErrors || this._options.ignoreHTTPSErrorsOverride) + if (this._options.ignoreHTTPSErrors || this._options.internalIgnoreHTTPSErrors) promises.push(this._browser._browserSession.send('Playwright.setIgnoreCertificateErrors', { browserContextId, ignore: true })); if (this._options.locale) promises.push(this._browser._browserSession.send('Playwright.setLanguages', { browserContextId, languages: [this._options.locale] }));