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.

Sign up for GitHub

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

Jump to bottom

feat: BiDi implementation of Puppeteer.connect for Firefox #11451

Merged
merged 14 commits into from Dec 1, 2023
Merged

feat: BiDi implementation of Puppeteer.connect for Firefox #11451

merged 14 commits into from Dec 1, 2023

Conversation

sadym-chromium
Copy link
Collaborator

@sadym-chromium sadym-chromium commented Nov 27, 2023 •
edited by OrKoN

Add support of re-connecting to pure BiDi browser (Firefox).

  1. Split connection logic between common, BiDi and CDP.
  2. When connecting via BiDi protocol, first try to connect via BiDi, and if the endpoint does not support BiDi, fall back to BiDi over CDP.
  3. Adjust BiDi connection to fail gracefully if endpoint supports CDP.
  4. Add unbind to BiDi connection to be able to keep WS transport open.

@sadym-chromium sadym-chromium marked this pull request as draft November 27, 2023 15:52
@OrKoN OrKoN changed the title feat: support Puppeteer.connect with FireFox feat: support Puppeteer.connect with Firefox Nov 27, 2023
@sadym-chromium
Copy link
Collaborator Author

Converting to draft, as there seems to be significant regression and lots of tests are failing now.

@sadym-chromium sadym-chromium force-pushed the sadym/ff-connect branch 3 times, most recently from 329556a to 39594ce Compare November 28, 2023 15:00
@sadym-chromium
Copy link
Collaborator Author

Tests are still timing out, but the concerns are addressed.

@sadym-chromium sadym-chromium force-pushed the sadym/ff-connect branch 2 times, most recently from e2de9fb to a2898ea Compare November 29, 2023 05:28
@sadym-chromium sadym-chromium force-pushed the sadym/ff-connect branch 3 times, most recently from 98eacc5 to c1ac50e Compare November 30, 2023 14:39
@conventional-commit-lint-gcf Conventional Commit Lint GCF
Copy link

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

@sadym-chromium sadym-chromium marked this pull request as ready for review November 30, 2023 14:47
@sadym-chromium
Copy link
Collaborator Author

@OrKoN PTAL

OrKoN
OrKoN reviewed Nov 30, 2023
View reviewed changes
test/TestExpectations.json Outdated Show resolved Hide resolved
@OrKoN
Copy link
Collaborator

OrKoN commented Nov 30, 2023

Please include in the title that it is about WebDriver BiDi

@sadym-chromium sadym-chromium changed the title feat: support Puppeteer.connect with Firefox feat: BiDi Firefox implementation of Puppeteer.connect Nov 30, 2023
OrKoN
OrKoN reviewed Nov 30, 2023
View reviewed changes
test/TestExpectations.json Outdated Show resolved Hide resolved
@sadym-chromium sadym-chromium enabled auto-merge (squash) November 30, 2023 21:31
Maksim Sadym and others added 14 commits December 1, 2023 09:21
@OrKoN
Re-apply all the changes from sadym/ff-connect
3b8d72c 
* d88f2b67df8 fix: handle close
* 028213092b6 Revert "Close CDP connection when closing BiDi over CDP"
* 85ed086f6de Close CDP connection when closing BiDi over CDP
* d2650c54329 Rename
* c16b85c3eb5 Proper clean up BiDi connection
* 7c3eeef8e19 reformat
* cce2da7a976 comment
* 0d8c3222004 `DEFAULT_VIEWPORT`
* eb2323148b0 Inline `_getCdpConnection`
* c5c465d5900 comment
* e8f91a584e1 Rename `wsTransport` to `connectionTransport`
* 72aff3ed6bf comment
* e3e92ae4246 Unbind BiDi connection
* 9415e646047 comment
* ca6ce356990 format
* d8052d913a5 Use proper close call
* 55c0bc12339 proper close
* adf968bd1be Refactor getting WS connection
* be7fe15bd59 Reuse WS connection
* 7671c2e4aab Split BiDi and CDP logic
* bccf15cbe76 feat: implement `Puppeteer.connect` for FireFox
@OrKoN
Remove duplicate
4522317
@OrKoN
Update expectations
34c3096
@OrKoN
Fix closeCallback
4d2fd63
@OrKoN
Fix should be able to reconnect test
72bc1e9
@OrKoN
Don't send a command if the connection is closed
9143986
@OrKoN
Enable FF reconnection test
42e1899
@OrKoN
Revert "Fix should be able to reconnect test"
5b0e3b8 
This reverts commit a0444c2.
@OrKoN
Revert "Enable FF reconnection test"
0868e73 
This reverts commit d65f316.
@OrKoN
Update test expectations
2a848f3
@OrKoN
format
59b5b7d
@OrKoN
Update error message
93b5116
@sadym-chromium @OrKoN
Update test/TestExpectations.json
3ced52a 
Co-authored-by: Alex Rudenko <OrKoN@users.noreply.github.com>
@sadym-chromium @OrKoN
Update test/TestExpectations.json
e41775c 
Co-authored-by: Alex Rudenko <OrKoN@users.noreply.github.com>
@OrKoN OrKoN changed the title feat: BiDi Firefox implementation of Puppeteer.connect feat: BiDi implementation of Puppeteer.connect for Firefox Dec 1, 2023
@sadym-chromium sadym-chromium enabled auto-merge (squash) December 1, 2023 08:26
@OrKoN OrKoN disabled auto-merge December 1, 2023 08:26
@OrKoN OrKoN enabled auto-merge (squash) December 1, 2023 08:26
@OrKoN OrKoN merged commit be081ba into main Dec 1, 2023
52 checks passed
@OrKoN OrKoN deleted the sadym/ff-connect branch December 1, 2023 08:30
@release-please release-please bot mentioned this pull request Dec 1, 2023
Merged
@Zapeth
Copy link

Zapeth commented Dec 22, 2023

Are there any settings required in Firefox for this to work, apart from fission.webContentIsolationStrategy=0?

I get Firefox is not supported in BiDi over CDP mode error when trying to connect() with protocol: 'webDriverBiDi' (I also tried setting fission.bfcacheInParent=false, but that only gets rid of the CDP warnings in Firefox output)

However, launch() with {product: 'firefox', protocol: 'webDriverBiDi', executablePath: '/usr/bin/firefox'} works

@Lightning00Blade Lightning00Blade mentioned this pull request Jan 26, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OrKoN OrKoN OrKoN approved these changes

@Lightning00Blade Lightning00Blade Awaiting requested review from Lightning00Blade

Assignees
No one assigned
Labels
automerge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@sadym-chromium @OrKoN @Zapeth @Lightning00Blade