KEMBAR78
Define the browsingContext.close command by juliandescottes · Pull Request #172 · w3c/webdriver-bidi · GitHub
Skip to content

Define the browsingContext.close command #172

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
Merged
merged 1 commit into from
Feb 25, 2022
Merged

Define the browsingContext.close command #172

merged 1 commit into from
Feb 25, 2022

Conversation

@juliandescottes
Copy link
Contributor

@juliandescottes juliandescottes commented Feb 4, 2022 •
edited by pr-preview bot
Loading

Fixes #170.

This adds the definition for browsingContext.close. I copied and modified existing browsingContext commands and tried to match https://w3c.github.io/webdriver/#close-window.

Opening the PR to get a first round of feedback, thanks in advance!
cc @jgraham @whimboo

Preview | Diff

@juliandescottes
Copy link
Contributor Author

Also for now I left out the question about what to do when closing the last window.
I would first like to clean up the "straightforward" part of the PR from obvious mistakes before diving into this question.

jgraham
jgraham requested changes Feb 4, 2022
View reviewed changes
@juliandescottes juliandescottes force-pushed the command-browsingContext-close branch from e79e322 to fa75c27 Compare February 5, 2022 07:22
@juliandescottes
Copy link
Contributor Author

Thanks for getting to the review so quickly @jgraham!

whimboo
whimboo previously requested changes Feb 7, 2022
View reviewed changes
Copy link
Contributor

@whimboo whimboo left a comment

Choose a reason for hiding this comment

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

I had a quick look and in general it looks fine. Still two comments inline which would require an update.

@juliandescottes juliandescottes force-pushed the command-browsingContext-close branch from fa75c27 to 54f9233 Compare February 7, 2022 13:25
@whimboo
Copy link
Contributor

whimboo commented Feb 7, 2022

Also for now I left out the question about what to do when closing the last window. I would first like to clean up the "straightforward" part of the PR from obvious mistakes before diving into this question.

Thanks for the updates @juliandescottes. So I assume this is left to do then.

Btw. this fixes issue #170.

@juliandescottes
Copy link
Contributor Author

Also for now I left out the question about what to do when closing the last window. I would first like to clean up the "straightforward" part of the PR from obvious mistakes before diving into this question.

Thanks for the updates @juliandescottes. So I assume this is left to do then.

Btw. this fixes issue #170.

Yes, I'll bring it up during our meeting and will update here afterwards. Also added the reference to #170 in the description, thanks for the reminder!

@juliandescottes juliandescottes mentioned this pull request Feb 8, 2022
Closed
@juliandescottes juliandescottes force-pushed the command-browsingContext-close branch from 54f9233 to 7e73690 Compare February 23, 2022 17:43
@juliandescottes
Copy link
Contributor Author

FWIW, Issue(w3c/webdriver-bidi#170) seems to generate a valid link

@juliandescottes
Copy link
Contributor Author

@foolip can you re-review since you reviewed the latest version before the update :) Thanks!

@foolip
Copy link
Member

foolip commented Feb 23, 2022

@jgraham do you want to review again?

@jgraham jgraham dismissed whimboo’s stale review February 25, 2022 20:31

Comments are addressed

@jgraham jgraham merged commit 44dedf7 into w3c:master Feb 25, 2022
@Lightning00Blade Lightning00Blade mentioned this pull request Jan 8, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jgraham jgraham jgraham approved these changes

@foolip foolip foolip approved these changes

@whimboo whimboo Awaiting requested review from whimboo

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Specify closing a browsing context

4 participants

@juliandescottes @whimboo @foolip @jgraham