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

Add a context parameter for browsingContext.create #153

Merged
merged 1 commit into from
Oct 5, 2022
Merged

Add a context parameter for browsingContext.create #153

merged 1 commit into from
Oct 5, 2022

Conversation

foolip
Copy link
Member

@foolip foolip commented Nov 30, 2021 •
edited by pr-preview bot
Loading

This is based on @jgraham's suggestion:
#133 (comment)

Preview | Diff

Preview | Diff

@foolip foolip mentioned this pull request Nov 30, 2021
Merged
foolip
foolip commented Nov 30, 2021
View reviewed changes
index.bs Outdated Show resolved Hide resolved
whimboo
whimboo reviewed Dec 8, 2021
View reviewed changes
index.bs Show resolved Hide resolved
whimboo
whimboo reviewed Mar 22, 2022
View reviewed changes
index.bs Show resolved Hide resolved
juliandescottes
juliandescottes reviewed Apr 13, 2022
View reviewed changes
index.bs Outdated Show resolved Hide resolved
@foolip foolip force-pushed the browsingContext-create-context branch from f9f81f6 to dbaa5b5 Compare May 25, 2022 16:20
@foolip
Copy link
Member Author

foolip commented May 25, 2022

This is ready for review again now.

whimboo
whimboo reviewed May 30, 2022
View reviewed changes
index.bs Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Show resolved Hide resolved
juliandescottes
juliandescottes approved these changes Jun 18, 2022
View reviewed changes
Copy link
Contributor

@juliandescottes juliandescottes left a comment

Choose a reason for hiding this comment

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

Looks good to me, just one nit on top of the suggestions from @whimboo , thanks!

index.bs Outdated Show resolved Hide resolved
sadym-chromium
sadym-chromium approved these changes Aug 29, 2022
View reviewed changes
Copy link
Contributor

@sadym-chromium sadym-chromium left a comment

Choose a reason for hiding this comment

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

Couple of minor comments

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
@whimboo
Copy link
Contributor

whimboo commented Aug 31, 2022

@foolip would you have the time to take a look at this PR again?

@foolip
Copy link
Member Author

foolip commented Sep 14, 2022

I'll come back to this soon.

@whimboo whimboo added enhancement New feature or request module-browsingContext Browsing Context module labels Sep 21, 2022
@foolip foolip force-pushed the browsingContext-create-context branch from c039576 to 7aab2a4 Compare October 5, 2022 10:19
@foolip
Copy link
Member Author

foolip commented Oct 5, 2022

I'll go ahead and merge, or I'm sure to forget about this for another month. If I got something wrong, I'll make it a priority to fix it, just poke me.

@foolip foolip force-pushed the browsingContext-create-context branch from 7aab2a4 to 2ecae3b Compare October 5, 2022 10:24
@foolip foolip force-pushed the browsingContext-create-context branch from 2ecae3b to 54abb51 Compare October 5, 2022 10:28
@foolip foolip merged commit c995112 into master Oct 5, 2022
@foolip foolip deleted the browsingContext-create-context branch October 5, 2022 12:30
github-actions bot added a commit that referenced this pull request Oct 5, 2022
@foolip @github-actions
Add a context parameter for browsingContext.create (#153)
97fcc59 
SHA: c995112
Reason: push, by @foolip

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@whimboo
Copy link
Contributor

whimboo commented Oct 6, 2022

@foolip actually such a change would have required a wpt test addition as well. Would you have the chance to follow-up with that?

@foolip
Copy link
Member Author

foolip commented Oct 6, 2022

@whimboo yes, I've asked @sadym-chromium to add a test when he implements this.

@whimboo
Copy link
Contributor

whimboo commented Oct 6, 2022

Sounds good. So whoever is first with the implementation might do it. We can let you know in case we start to implement at first. Please do the same. Thanks.

@whimboo
Copy link
Contributor

whimboo commented Oct 6, 2022

@sadym-chromium actually yesterday I checked if we already have a bug and found https://bugzilla.mozilla.org/show_bug.cgi?id=1765619. There are already wdspec tests attached. So maybe @juliandescottes could finish the set of patches and we can land it first.

@sadym-chromium
Copy link
Contributor

would be great, thanks!

Actually, I'm not sure if there is a way to test if the parameter actually works, only the signature.

@juliandescottes
Copy link
Contributor

would be great, thanks!

Actually, I'm not sure if there is a way to test if the parameter actually works, only the signature.

Yes we had the same issue. I only added tests for the signature at https://phabricator.services.mozilla.com/D143653 , and I also added some gecko specific tests at https://phabricator.services.mozilla.com/D143654, but you need to assume a specific implementation in order to "test" the feature.

My patches were written before the PR got its last few updates, so I guess they are a bit outdated. But I can update them next week.

whimboo pushed a commit to whimboo/webdriver-bidi that referenced this pull request Feb 3, 2023
@foolip @juliandescottes
@sadym-chromium @whimboo
Add a context parameter for browsingContext.create (w3c#153)
34a6a94 
This is based on @jgraham's suggestion:
w3c#133 (comment)

Co-authored-by: Julian Descottes <jdescottes@mozilla.com>
Co-authored-by: Maksim Sadym <69349599+sadym-chromium@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bwalderman bwalderman bwalderman left review comments

@jgraham jgraham jgraham left review comments

@whimboo whimboo whimboo left review comments

@sadym-chromium sadym-chromium sadym-chromium approved these changes

@juliandescottes juliandescottes juliandescottes approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request module-browsingContext Browsing Context module
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@foolip @whimboo @sadym-chromium @juliandescottes @jgraham @bwalderman