-
Notifications
You must be signed in to change notification settings - Fork 74
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Add WebDriver #1592
Add WebDriver #1592
Conversation
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.
Couple of comments
- I'd rename this simply webdriver.yml, drop the "2" since it's not used anywhere else
- Looks like it's missing the VirtualSensor APIs (
webdriver.commands.CreateVirtualSensor
,webdriver.commands.UpdateVirtualSensorReading
)
CC @ddbeck - review the reviewer
LGTM - want @ddbeck to review the reviewer
Thanks for the review @petele!
✅
Thanks, added! |
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.
This is failing npm run test
because of the compute_from
line. Accepting the suggestion should fix that.
hm, worked locally w/ |
Agreed, it's caught me up a couple of times. I usually will run |
flipping back to draft while we work out whether "later additions" are separate features or not per @jgraham's comment on chat:
|
Just a fly-by comment about the summary of this PR:
BiDi is an extension to the WebDriver specification, but it's not single vendor (assuming that means implemented only in one engine/browser here). It's in implementation in Firefox and Chromium at the moment. See https://wpt.fyi/results/webdriver/tests/bidi?label=experimental&label=master&aligned, Firefox, Chrome and Edge all pass a significant number of BiDi webplatform tests. |
oh that's great, thanks for the context @juliandescottes! i'll check that the underlying data here is correct, and will fold it in. |
@ddbeck a couple of notes with this one: Baseline status: Navigator.webdriver is baseline high, but everything under it is all over the place, so I'm leaving as Group vs merge vs neither: Going w/ neither at this point. BiDi is a whole different paradigm, so it's pretty hard to consider it the same "feature" as WebDriver Classic. We could group, but don't need to block on that now - maybe we have a |
add @ddbeck's changes to description Co-authored-by: Daniel D. Beck <daniel@ddbeck.com>
fwiw, I would put these in a separate feature. These are defined in a separate spec (Sensors) https://w3c.github.io/sensors/#automation. The same is happening for Compute Pressure, coming to BCD with mdn/browser-compat-data#24211 for which I would have created an own web-feature ID, too. Basically these things are "WebDriver extensions". |
ack, per chat:
@Elchi3 lmk if the webdriver sensors feature looks like what you had in mind |
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.
Structure looks good to me. Made a suggestion for the description.
r+
Description fixes from @Elchi3 Co-authored-by: Florian Scholz <fs@florianscholz.com>
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.
Everything here looks fine, but I don't think the sensors stuff is ready to ship with the general webdriver stuff. See my line comment for a suggestion. Very close to done though!
Resources
Name
No real issues here. There's a "2" in there but developers refer to it as WebDriver everywhere I looked.
Description
Took a bit from both spec and MDN.
Group
Interesting one since it's not really a "web API" in the typical sense. No group for now.BiDi is already added as
feature/webdriver-bidi.yml
.Perhaps we need a WebDriver group, or should merge? Doesn't need to block landing the core of this though.
Splits/Merges
There's BiDi, but it is a "late addition",
single vendor[1] and seems to be referred to separately. So not merging at this time.