Skip to content
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

feat(chromium): connect to a browser over cdp #5207

Merged
merged 2 commits into from Feb 10, 2021

Conversation

JoelEinbinder
Copy link
Contributor

No description provided.

...uiOptions,
name: 'chromium',
isChromium: true,
headful: true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why headful?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To ensure we have a default context/page.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we only need persistent for that to happen. Let's not lie to ourselves?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I thought this was in the test. Yes I have no idea why this code is here.

});
async _connectCDP(params: ConnectOptions): Promise<Browser> {
if (this.name() !== 'chromium')
throw new Error('Connecting over CDP is onlly supported for the Chromium BrowserType');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'Connecting over CDP is only supported in Chromium'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I started with this text, but I realized that people reading it are unlikely to have chromium. Instead they have chrome, edge, webview etc. So I was trying to emphasize that they need to use playwright.chromium to connect.

@@ -62,6 +62,11 @@ This methods attaches Playwright to an existing browser instance.
- `logger` <[Logger]> Logger sink for Playwright logging. Optional.
- `timeout` <[float]> Maximum time in milliseconds to wait for the connection to be established. Defaults to
`30000` (30 seconds). Pass `0` to disable timeout.
- `protocol` <"playwright"|"cdp"> To use a Chrome DevTools Protocol endpoint, pass "cdp". Defaults to "playwright".
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd call this cdp: boolean | undefined, so that you only use it if you know what you are doing. Otherwise you have some other legit option in there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am worried that we will want to put some other protocols in here in the future. WebDriver. Firefox cdp. WebInspector protocol for iPhones.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you all think about ChromiumBrowser.connectOverCDP(wsEndpoint, options)? Are we moving away from ChromiumBrowser it is ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have ChromiumBrowserContext but not ChromiumBrowser. I'd rather have a small option than a big method.

docs/src/api/class-browsertype.md Show resolved Hide resolved
const browser = await this._object.cdpConnect(params.wsEndpoint, params, params.timeout);
return {
browser: new BrowserDispatcher(this._scope, browser),
defaultContext: new BrowserContextDispatcher(this._scope, browser._defaultContext!),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How are you sure there is a default context? If I launched with Playwright, there might be none.


it('should connect to an existing cdp session', async ({browserType, testWorkerIndex, browserOptions, createUserDataDir }) => {
const port = 9339 + testWorkerIndex;
const spawnedProcess = spawn(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't you use chromium.launch()? It will take care of killing the browser for you. Otherwise, you leave stray chrome when the test times out.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our chromium.launch specifies --remote-debugging-pipe. You can't launch headful chrome with both pipe and port. In puppeteer we had code that switched to use a websocket if port was specified, but I didn't feel like reviving it for this test.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passing both pipe and port works in headless. With the next roll, it should also work in headful. Let's do it.


### param: BrowserType.connectOverCDP.params
- `params` <[Object]>
- `wsEndpoint` <[string]> A browser websocket endpoint to connect to.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: browser websocket endpoint -> cdp webscoket endpoint?

const browser = await this._object.cdpConnect(params.wsEndpoint, params, params.timeout);
return {
browser: new BrowserDispatcher(this._scope, browser),
defaultContext: browser._defaultContext ? new BrowserContextDispatcher(this._scope, browser._defaultContext!) : undefined,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: no need for exclamation mark.

@@ -391,6 +391,14 @@ BrowserType:
returns:
context: BrowserContext

cdpConnect:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's call this connectOverCDP.


it('should connect to an existing cdp session', async ({browserType, testWorkerIndex, browserOptions, createUserDataDir }) => {
const port = 9339 + testWorkerIndex;
const spawnedProcess = spawn(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passing both pipe and port works in headless. With the next roll, it should also work in headful. Let's do it.

@JoelEinbinder JoelEinbinder force-pushed the connect_cdp branch 3 times, most recently from 6b674a1 to 7161813 Compare February 8, 2021 16:55
src/client/browserType.ts Outdated Show resolved Hide resolved
...uiOptions,
name: 'chromium',
isChromium: true,
headful: true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we only need persistent for that to happen. Let's not lie to ourselves?

@tjenkinson
Copy link
Contributor

👋 this might be a better place for my question in #18052 (comment)

I'm wondering what the difference is to the wsEndpoint in connect(). What protocol does connect() use?

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants