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

Define the browsingContext.close command #172

Merged
merged 1 commit into from
Feb 25, 2022

Conversation

juliandescottes
Copy link
Contributor

@juliandescottes juliandescottes commented Feb 4, 2022

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.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@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
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.

index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
@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 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
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.

Specify closing a browsing context
4 participants