-
Notifications
You must be signed in to change notification settings - Fork 3.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: move more of video capture into browser automations #23587
Changes from all commits
297e41b
173de9f
e493600
beb3f2d
4957a83
c255548
b01e8c0
ce15192
8bd49f3
8e8f62a
501895a
5e74399
d25b9e0
9b8cfbe
a7f6d4b
4deada7
e1b7791
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,7 +20,7 @@ import { BrowserCriClient } from './browser-cri-client' | |
import type { LaunchedBrowser } from '@packages/launcher/lib/browsers' | ||
import type { CriClient } from './cri-client' | ||
import type { Automation } from '../automation' | ||
import type { BrowserLaunchOpts, BrowserNewTabOpts } from '@packages/types' | ||
import type { BrowserLaunchOpts, BrowserNewTabOpts, WriteVideoFrame } from '@packages/types' | ||
|
||
const debug = debugModule('cypress:server:browsers:chrome') | ||
|
||
|
@@ -249,22 +249,10 @@ const _disableRestorePagesPrompt = function (userDir) { | |
.catch(() => { }) | ||
} | ||
|
||
const _maybeRecordVideo = async function (client, options, browserMajorVersion) { | ||
if (!options.onScreencastFrame) { | ||
debug('options.onScreencastFrame is false') | ||
async function _recordVideo (cdpAutomation: CdpAutomation, writeVideoFrame: WriteVideoFrame, browserMajorVersion: number) { | ||
const opts = browserMajorVersion >= CHROME_VERSION_WITH_FPS_INCREASE ? screencastOpts() : screencastOpts(1) | ||
|
||
return client | ||
} | ||
|
||
debug('starting screencast') | ||
client.on('Page.screencastFrame', (meta) => { | ||
options.onScreencastFrame(meta) | ||
client.send('Page.screencastFrameAck', { sessionId: meta.sessionId }) | ||
}) | ||
|
||
await client.send('Page.startScreencast', browserMajorVersion >= CHROME_VERSION_WITH_FPS_INCREASE ? screencastOpts() : screencastOpts(1)) | ||
|
||
return client | ||
await cdpAutomation.startVideoRecording(writeVideoFrame, opts) | ||
flotwig marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
// a utility function that navigates to the given URL | ||
|
@@ -434,7 +422,9 @@ const _handlePausedRequests = async (client) => { | |
const _setAutomation = async (client: CriClient, automation: Automation, resetBrowserTargets: (shouldKeepTabOpen: boolean) => Promise<void>, options: BrowserLaunchOpts) => { | ||
const cdpAutomation = await CdpAutomation.create(client.send, client.on, resetBrowserTargets, automation, !!options.experimentalSessionAndOrigin) | ||
|
||
return automation.use(cdpAutomation) | ||
automation.use(cdpAutomation) | ||
|
||
return cdpAutomation | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (for comparison, this is a good use of async/await, because we want to call a method on cdpAutomation after the first promise resolves) |
||
} | ||
|
||
export = { | ||
|
@@ -448,7 +438,7 @@ export = { | |
|
||
_removeRootExtension, | ||
|
||
_maybeRecordVideo, | ||
_recordVideo, | ||
|
||
_navigateUsingCRI, | ||
|
||
|
@@ -468,7 +458,7 @@ export = { | |
return browserCriClient | ||
}, | ||
|
||
async _writeExtension (browser: Browser, options) { | ||
async _writeExtension (browser: Browser, options: BrowserLaunchOpts) { | ||
if (browser.isHeadless) { | ||
debug('chrome is running headlessly, not installing extension') | ||
|
||
|
@@ -565,7 +555,7 @@ export = { | |
await this.attachListeners(browser, options.url, pageCriClient, automation, options) | ||
}, | ||
|
||
async connectToExisting (browser: Browser, options: BrowserLaunchOpts, automation) { | ||
async connectToExisting (browser: Browser, options: BrowserLaunchOpts, automation: Automation) { | ||
const port = await protocol.getRemoteDebuggingPort() | ||
|
||
debug('connecting to existing chrome instance with url and debugging port', { url: options.url, port }) | ||
|
@@ -580,17 +570,17 @@ export = { | |
await this._setAutomation(pageCriClient, automation, browserCriClient.resetBrowserTargets, options) | ||
}, | ||
|
||
async attachListeners (browser: Browser, url: string, pageCriClient, automation: Automation, options: BrowserLaunchOpts & { onInitializeNewBrowserTab?: () => void }) { | ||
async attachListeners (browser: Browser, url: string, pageCriClient: CriClient, automation: Automation, options: BrowserLaunchOpts | BrowserNewTabOpts) { | ||
if (!browserCriClient) throw new Error('Missing browserCriClient in attachListeners') | ||
|
||
await this._setAutomation(pageCriClient, automation, browserCriClient.resetBrowserTargets, options) | ||
const cdpAutomation = await this._setAutomation(pageCriClient, automation, browserCriClient.resetBrowserTargets, options) | ||
|
||
await pageCriClient.send('Page.enable') | ||
|
||
await options.onInitializeNewBrowserTab?.() | ||
await options['onInitializeNewBrowserTab']?.() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why this change? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It bypasses type checks on Open to suggestions on a cleaner way to do this... |
||
|
||
await Promise.all([ | ||
this._maybeRecordVideo(pageCriClient, options, browser.majorVersion), | ||
options.writeVideoFrame && this._recordVideo(cdpAutomation, options.writeVideoFrame, browser.majorVersion), | ||
this._handleDownloads(pageCriClient, options.downloadsFolder, automation), | ||
]) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,9 +8,13 @@ import { CdpAutomation, screencastOpts, CdpCommand, CdpEvent } from './cdp_autom | |
import * as savedState from '../saved_state' | ||
import utils from './utils' | ||
import * as errors from '../errors' | ||
import type { BrowserInstance } from './types' | ||
import type { Browser, BrowserInstance } from './types' | ||
import type { BrowserWindow, WebContents } from 'electron' | ||
import type { Automation } from '../automation' | ||
import type { BrowserLaunchOpts, Preferences } from '@packages/types' | ||
|
||
// TODO: unmix these two types | ||
type ElectronOpts = Windows.WindowOptions & BrowserLaunchOpts | ||
|
||
const debug = Debug('cypress:server:browsers:electron') | ||
const debugVerbose = Debug('cypress-verbose:server:browsers:electron') | ||
|
@@ -68,7 +72,7 @@ const _getAutomation = async function (win, options, parent) { | |
// after upgrading to Electron 8, CDP screenshots can hang if a screencast is not also running | ||
// workaround: start and stop screencasts between screenshots | ||
// @see https://github.com/cypress-io/cypress/pull/6555#issuecomment-596747134 | ||
if (!options.onScreencastFrame) { | ||
if (!options.writeVideoFrame) { | ||
await sendCommand('Page.startScreencast', screencastOpts()) | ||
const ret = await fn(message, data) | ||
|
||
|
@@ -105,37 +109,18 @@ function _installExtensions (win: BrowserWindow, extensionPaths: string[], optio | |
})) | ||
} | ||
|
||
const _maybeRecordVideo = async function (webContents, options) { | ||
const { onScreencastFrame } = options | ||
|
||
debug('maybe recording video %o', { onScreencastFrame }) | ||
|
||
if (!onScreencastFrame) { | ||
return | ||
} | ||
|
||
webContents.debugger.on('message', (event, method, params) => { | ||
if (method === 'Page.screencastFrame') { | ||
onScreencastFrame(params) | ||
webContents.debugger.sendCommand('Page.screencastFrameAck', { sessionId: params.sessionId }) | ||
} | ||
}) | ||
|
||
await webContents.debugger.sendCommand('Page.startScreencast', screencastOpts()) | ||
} | ||
|
||
export = { | ||
_defaultOptions (projectRoot, state, options, automation) { | ||
_defaultOptions (projectRoot: string | undefined, state: Preferences, options: BrowserLaunchOpts, automation: Automation): ElectronOpts { | ||
const _this = this | ||
|
||
const defaults = { | ||
x: state.browserX, | ||
y: state.browserY, | ||
const defaults: Windows.WindowOptions = { | ||
x: state.browserX || undefined, | ||
y: state.browserY || undefined, | ||
width: state.browserWidth || 1280, | ||
height: state.browserHeight || 720, | ||
devTools: state.isBrowserDevToolsOpen, | ||
minWidth: 100, | ||
minHeight: 100, | ||
devTools: state.isBrowserDevToolsOpen || undefined, | ||
contextMenu: true, | ||
partition: this._getPartition(options), | ||
trackState: { | ||
|
@@ -148,8 +133,21 @@ export = { | |
webPreferences: { | ||
sandbox: true, | ||
}, | ||
show: !options.browser.isHeadless, | ||
// prevents a tiny 1px padding around the window | ||
// causing screenshots/videos to be off by 1px | ||
resizable: !options.browser.isHeadless, | ||
onCrashed () { | ||
const err = errors.get('RENDERER_CRASHED') | ||
|
||
errors.log(err) | ||
|
||
if (!options.onError) throw new Error('Missing onError in onCrashed') | ||
|
||
options.onError(err) | ||
}, | ||
onFocus () { | ||
if (options.show) { | ||
if (!options.browser.isHeadless) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. options.show seems clearer to me? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Side note - the data flow and variable naming in this launcher is off the rails. It's due for a major refactor... |
||
return menu.set({ withInternalDevTools: true }) | ||
} | ||
}, | ||
|
@@ -176,18 +174,12 @@ export = { | |
}, | ||
} | ||
|
||
if (options.browser.isHeadless) { | ||
// prevents a tiny 1px padding around the window | ||
// causing screenshots/videos to be off by 1px | ||
options.resizable = false | ||
} | ||
|
||
return _.defaultsDeep({}, options, defaults) | ||
}, | ||
|
||
_getAutomation, | ||
|
||
async _render (url: string, automation: Automation, preferences, options: { projectRoot: string, isTextTerminal: boolean }) { | ||
async _render (url: string, automation: Automation, preferences, options: { projectRoot?: string, isTextTerminal: boolean }) { | ||
const win = Windows.create(options.projectRoot, preferences) | ||
|
||
if (preferences.browser.isHeadless) { | ||
|
@@ -212,21 +204,25 @@ export = { | |
|
||
const [parentX, parentY] = parent.getPosition() | ||
|
||
options = this._defaultOptions(projectRoot, state, options, automation) | ||
const electronOptions = this._defaultOptions(projectRoot, state, options, automation) | ||
|
||
_.extend(options, { | ||
_.extend(electronOptions, { | ||
x: parentX + 100, | ||
y: parentY + 100, | ||
trackState: false, | ||
// in run mode, force new windows to automatically open with show: false | ||
// this prevents window.open inside of javascript client code to cause a new BrowserWindow instance to open | ||
// https://github.com/cypress-io/cypress/issues/123 | ||
show: !options.isTextTerminal, | ||
}) | ||
|
||
const win = Windows.create(projectRoot, options) | ||
const win = Windows.create(projectRoot, electronOptions) | ||
|
||
// needed by electron since we prevented default and are creating | ||
// our own BrowserWindow (https://electron.atom.io/docs/api/web-contents/#event-new-window) | ||
e.newGuest = win | ||
|
||
return this._launch(win, url, automation, options) | ||
return this._launch(win, url, automation, electronOptions) | ||
}, | ||
|
||
async _launch (win: BrowserWindow, url: string, automation: Automation, options) { | ||
|
@@ -278,7 +274,7 @@ export = { | |
|
||
automation.use(cdpAutomation) | ||
await Promise.all([ | ||
_maybeRecordVideo(win.webContents, options), | ||
options.writeVideoFrame && cdpAutomation.startVideoRecording(options.writeVideoFrame), | ||
this._handleDownloads(win, options.downloadsFolder, automation), | ||
]) | ||
|
||
|
@@ -459,30 +455,26 @@ export = { | |
throw new Error('Attempting to connect to existing browser for Cypress in Cypress which is not yet implemented for electron') | ||
}, | ||
|
||
async open (browser, url, options, automation) { | ||
const { projectRoot, isTextTerminal } = options | ||
|
||
async open (browser: Browser, url: string, options: BrowserLaunchOpts, automation: Automation) { | ||
debug('open %o', { browser, url }) | ||
|
||
const State = await savedState.create(projectRoot, isTextTerminal) | ||
const State = await savedState.create(options.projectRoot, options.isTextTerminal) | ||
const state = await State.get() | ||
|
||
debug('received saved state %o', state) | ||
|
||
// get our electron default options | ||
// TODO: this is bad, don't mutate the options object | ||
options = this._defaultOptions(projectRoot, state, options, automation) | ||
|
||
// get the GUI window defaults now | ||
options = Windows.defaults(options) | ||
const electronOptions: ElectronOpts = Windows.defaults( | ||
this._defaultOptions(options.projectRoot, state, options, automation), | ||
) | ||
|
||
debug('browser window options %o', _.omitBy(options, _.isFunction)) | ||
debug('browser window options %o', _.omitBy(electronOptions, _.isFunction)) | ||
|
||
const defaultLaunchOptions = utils.getDefaultLaunchOptions({ | ||
preferences: options, | ||
preferences: electronOptions, | ||
}) | ||
|
||
const launchOptions = await utils.executeBeforeBrowserLaunch(browser, defaultLaunchOptions, options) | ||
const launchOptions = await utils.executeBeforeBrowserLaunch(browser, defaultLaunchOptions, electronOptions) | ||
|
||
const { preferences } = launchOptions | ||
|
||
|
@@ -493,7 +485,7 @@ export = { | |
isTextTerminal: options.isTextTerminal, | ||
}) | ||
|
||
await _installExtensions(win, launchOptions.extensions, options) | ||
await _installExtensions(win, launchOptions.extensions, electronOptions) | ||
|
||
// cause the webview to receive focus so that | ||
// native browser focus + blur events fire correctly | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
async function() { await Promise() }
is mostly equivalent tofunction() { return Promise() }
. You only need to await things if you want flow control to return to this function after a promise completes (eg, to return a different value than the inner promise resolves to).Same with the outer
startVideoRecording
as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you mean, but I prefer it this way, since I don't want to "return" any value. A synchronous function wouldn't
return
on this line either. Absent of a coding style guide, there's a mix of both ways in the codebase.