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

Introduce Browser Permissions for WebDriver BiDi #431

Merged
merged 1 commit into from
Jan 11, 2024
Merged

Conversation

OrKoN
Copy link
Contributor

@OrKoN OrKoN commented Nov 27, 2023

@miketaylr
Copy link
Member

@OrKoN I see this is currently a draft, happy to review when you think it's ready.

@OrKoN OrKoN marked this pull request as ready for review November 28, 2023 08:13
@OrKoN
Copy link
Contributor Author

OrKoN commented Nov 28, 2023

@miketaylr thanks! I double checked if I have included everything from the previous PR and looks like I fixed the compilation errors so I believe it's now ready for review.

Copy link
Member

@miketaylr miketaylr left a comment

Choose a reason for hiding this comment

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

LGTM, but let's get someone else to review as well (preferably someone more familiar with WebDriver-BiDi). :)

@marcoscaceres
Copy link
Member

Filed WebKit bug...

hey, @gsnedders, would you mind having a look at this too?

@marcoscaceres
Copy link
Member

@OrKoN, apart from tests, what else needs to happen on the WPT side? Do we need to add somethings to testdriver_vendor.js etc?

@OrKoN
Copy link
Contributor Author

OrKoN commented Dec 4, 2023

@marcoscaceres this PR replicates the WebDriver Classic functionality so there is no need to extend testdriver.js currently. For now we only plan to add WPT tests for the extension spec itself (similar to webdriver/tests/classic/permissions/set.py). Eventually we want to expose WebDriver BiDi via testdriver-vendor.js when there is a need to testing event-based features of WebDriver BiDi or if there are new BiDi features that are not available in WebDriver Classic.

Copy link
Member

@marcoscaceres marcoscaceres left a comment

Choose a reason for hiding this comment

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

We reviewed on the WebKit side and we are content for this to land too.

@OrKoN
Copy link
Contributor Author

OrKoN commented Dec 8, 2023

@miketaylr do you know if we need more reviewers? I can start working on WPT tests and Chrome implementation at the end of December. Should we wait until WPT tests are there before landing?

@miketaylr
Copy link
Member

@miketaylr do you know if we need more reviewers? I can start working on WPT tests and Chrome implementation at the end of December. Should we wait until WPT tests are there before landing?

Nope - we're good on the review side of things, especially with WebKit's support. It would be ideal if the WPTs landed before we merged, but if you pinky swear to do them I don't mind merging now.

@OrKoN
Copy link
Contributor Author

OrKoN commented Dec 29, 2023

@miketaylr I have started WPT tests here web-platform-tests/wpt#43802 and started to work on the implementation. One thing I have noticed is that probably we would need to adjust the BiDi part of the spec text. In particular, Let settings be the current settings object. is ambiguous in the context of WebDriver BiDi and we should explicitly specify which browsing context to use. Does it make sense to make those changes in this PR and land it or start a new one?

@miketaylr
Copy link
Member

@OrKoN let's do whatever is simplest for you. I'm happy to merge this as-is, and we can iterate in a new PR, or we can continue here.

@OrKoN
Copy link
Contributor Author

OrKoN commented Jan 2, 2024

@miketaylr I think it will be simpler to land this first so it's easier to review the differences and avoid merge conflicts. Would you mind merging the PR then?

@marcoscaceres marcoscaceres merged commit 8094691 into w3c:main Jan 11, 2024
2 checks passed
@miketaylr
Copy link
Member

Thanks Marcos! Sorry to have missed this notification @OrKoN...

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.

Umbrella / Meta: Browser Permissions for WebDriver BiDi as an extension module
3 participants