fix(electron): return a ChromiumBrowserContext for electron #4913
Conversation
src/dispatchers/browserDispatcher.ts
Outdated
| @@ -45,21 +45,21 @@ export class BrowserDispatcher extends Dispatcher<Browser, channels.BrowserIniti | |||
| } | |||
|
|
|||
| async crNewBrowserCDPSession(): Promise<channels.BrowserCrNewBrowserCDPSessionResult> { | |||
| if (this._object._options.name !== 'chromium') | |||
| if (this._object._options.name !== 'chromium' && this._object._options.name !== 'electron') | |||
There was a problem hiding this comment.
maybe add a helper function like isChromiumBasedBrowser() since its duplicated logic which is "hard" to maintain.
There was a problem hiding this comment.
Yeah, something like isChromiumBased(). I wonder what happens with android? Can we do newCDPSession there? We probably should.
There was a problem hiding this comment.
Browsers now register themselves with isChromium. I changed the protocol so that isChromium is sent rather than browserName. And android now reports a ChromiumBrowserContext.
JoelEinbinder
force-pushed
the
electron_chromium_browsercontext
branch
from
cc4993c to
2113dea
Compare
JoelEinbinder
force-pushed
the
electron_chromium_browsercontext
branch
from
2113dea to
e61f97c
Compare
| @@ -507,7 +507,7 @@ BrowserContext: | |||
| type: interface | |||
|
|
|||
| initializer: | |||
| browserName: string | |||
| isChromium: boolean | |||
There was a problem hiding this comment.
Why don't we send both? I find browserName pretty useful.
There was a problem hiding this comment.
browserName in the client was only used to distinguish chromium and non-chromium based browsers. Removing it ensures that nobody does browserName === 'chromum' when they mean isChromium.
If will need it back, it will probably be for some feature that requires larger protocol changes. So let's not have it until we need it.
There was a problem hiding this comment.
Are you sure about what other language clients do? I am somewhat hesitant breaking them without a good reason.
There was a problem hiding this comment.
I thought none of them distinguish browsers. I'll check to confirm. I imagine if they are using browserName, they will want to want to fix the same bug and consider electron to be chromium.
There was a problem hiding this comment.
python reads the browser name, but should switch to isChromium after this patch. https://github.com/microsoft/playwright-python/blob/30c5b52cc460f3cce8ba1910682c9d74e0f6f5bf/playwright/_impl/_object_factory.py#L57
There was a problem hiding this comment.
Java, Go, and Crystal do not appear to distinguish between browsers by name.
Co-authored-by: Max Schmitt <[email protected]>
fixes #4905