-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Add WebDriver BiDi data for command/event parameters #26993
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
Conversation
Tip: Review these changes grouped by change (recommended for most PRs), or grouped by feature (for large PRs). |
0374a19
to
28b2fd3
Compare
e5400cf
to
a75400a
Compare
071aab0
to
ea6f748
Compare
This pull request has merge conflicts that must be resolved before it can be merged. |
5b46134
to
e749028
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed the changes, and noted especially the following cases:
- (More important) Noticable version changes or status changes.
- (Less urgent) In some cases, parameters are partially supported, but there is no note explaining the limitation (should answer: what works, what doesn't work). (This can be recorded in the spreadsheet using a "Note" in the version cell.)
@whimboo @sadym-chromium Let me know if you have any questions. 🙏
webdriver/bidi/browsingContext.json
Outdated
"version_added": "101", | ||
"partial_implementation": true, | ||
"notes": "Basic support (see [bug 1730641](https://bugzil.la/1730641))" | ||
"version_added": "101" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@whimboo Before, Firefox data for browsingContext.navigate() indicated partial support from 101, now it states full support (including all parameters) in that version. Correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the meta bug shows we have had a lot of follow-up fixes for this command and we are not done yet with all of them. I don't think that we should list each and every improvement in the note to this command. The initial landing contained support for all the parameters. Do you think otherwise?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be helpful for developers to mention the most relevant limitations (which doesn't imply marking the feature as partial).
For example:
Does not handle navigations that start a download (bug 1763126), navigations to javascript URLs (bug 1763131), or navigations where the initial request never comes back or times out (bug 1763135).
webdriver/bidi/webExtension.json
Outdated
"version_added": false | ||
"version_added": "135", | ||
"partial_implementation": true, | ||
"notes": "Basic support" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sadym-chromium Would be good to add details about Chrome's partial implementation of the webExtension.install()
command, ideally linking to a bug.
webdriver/bidi/webExtension.json
Outdated
"chrome": { | ||
"version_added": "135", | ||
"partial_implementation": true, | ||
"notes": "Basic support" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sadym-chromium Would be good to add details about Chrome's partial implementation of the extensionData
parameter of the webExtension.install
command, ideally linking to a bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't plan to implement other types of extensionData
, so no tracking bug there.
"chrome": { | ||
"version_added": "135", | ||
"partial_implementation": true, | ||
"notes": "Basic support" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sadym-chromium Would be good to add details about Chrome's partial implementation of the extension
parameter of the webExtension.uninstall
command, ideally linking to a bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I marked it as partial
, as unlike our e2e tests, the WPT tests are not passing. It needs som investigation for extended status.
"firefox": { | ||
"version_added": "122", | ||
"partial_implementation": true, | ||
"notes": "Basic support" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@whimboo Would be good to add details about Firefox's partial implementation of the response
parameter of the network.authRequired
event, ideally linking to a bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@juliandescottes could you please have a look?
67f4e65
to
fd0ca91
Compare
1cb0e38
to
b6d2489
Compare
@caugner I assume this PR still needs another update to fetch the latest changes from the spreadsheet. |
d29c8b9
to
dd0bb70
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @whimboo @sadym-chromium @juliandescottes for your swift input! 🙌
The new parameter support data is looking good now, so let's land this as is, and follow up on the remaining open review threads later. 🎉
(This PR was generated by the
update-webdriver-bidi-data
workflow.)