Conversation
juliandescottes
left a comment
There was a problem hiding this comment.
This looks great! I'll let Henrik do the final technical review, just one nitpick on my side.
whimboo
left a comment
There was a problem hiding this comment.
That's a great start with the session module @dipikabh! Very well written with all the details and nice examples. I've some questions and suggestions still, which you can find inline. Please let one of us know when something is unclear, and we can help to clarify.
files/en-us/web/webdriver/reference/bidi/modules/session/index.md
Outdated
Show resolved
Hide resolved
files/en-us/web/webdriver/reference/bidi/modules/session/new/index.md
Outdated
Show resolved
Hide resolved
| - `webSocketUrl` {{optional_inline}} | ||
| - : A string that contains the WebSocket URL for the session. | ||
|
|
||
| The browser may also return vendor-specific capabilities prefixed with a browser identifier (for example, `moz:buildID` for Firefox). |
There was a problem hiding this comment.
I wonder if we can have a separate page for those vendor specific capabilities.
There was a problem hiding this comment.
There's a precedent with firefoxOptions for vendor-specific capabilities. However, that's client-sent.
We don't have dedicated pages for browser-sent capabilities. So we might not cover them in that detail. But if important, I can include an example or a note about Firefox-specific metadata included in return value.
There was a problem hiding this comment.
Is it that we don't want to cover vendor specific capabilities or what's the reason why we cannot cover them in such a detail? Maybe we could have a separate section on the firefoxOptions page that handles return-only capabilities?
There was a problem hiding this comment.
As I understand it, this metadata, moz:buildID, comes under Extensible in the spec - it's an implementation detail that can vary by browser. That said, we do already document firefoxOptions.
It might open the door for other browsers to add content for their return capability parameters.
I'm curious - how many such Firefox-specific fields are there in the return value, and what is the expected level of detail?
Is it something your team can help put together as a first draft and I can edit?
separate section on the firefoxOptions page that handles return-only capabilities
I think if we document return-only capabilities, they would be better as new, separate pages.
There was a problem hiding this comment.
Currently we have the following Mozilla specific vendor capabilities:
Some of them are only returned and others are used for processing capabilities as well (means, they will be fed with the new session command).
There was a problem hiding this comment.
Perhaps I can queue up this task for a future date
files/en-us/web/webdriver/reference/bidi/modules/session/new/index.md
Outdated
Show resolved
Hide resolved
whimboo
left a comment
There was a problem hiding this comment.
I think that looks good for the initial landing of the session module. Great additions to have the placeholders for all modules present as well now. Thanks a lot!
|
Thanks a lot @whimboo for the tech review and approval! I've addressed/answered your comments and questions. There is just one more minor update since your last review. I also have two open questions (here and here) before I invite the content team for an edit review. |
|
This pull request has merge conflicts that must be resolved before it can be merged. |
f88503e to
4c1ac73
Compare
|
Rebased this branch to include the changes in #43314, which moves Capabilities and Errors out of |
|
Hi Chris, I'd appreciate if you can provide me an editorial review for this when you get a chance. Thanks! |
There was a problem hiding this comment.
Hey @dipikabh, here's my editorial review!
I think this is largely useful and well-structured. My comments are fairly picky, and some are definitely a result of my inexperience with WebDriver BiDi. Still worth asking through, IMO.
I was approaching these articles as a beginner (which I am) while trying to put myself in the mindset of someone with previous BiDi experience, and thinking about what they might need.
files/en-us/web/webdriver/reference/bidi/modules/emulation/index.md
Outdated
Show resolved
Hide resolved
files/en-us/web/webdriver/reference/bidi/modules/webextension/index.md
Outdated
Show resolved
Hide resolved
files/en-us/web/webdriver/reference/bidi/modules/session/new/index.md
Outdated
Show resolved
Hide resolved
files/en-us/web/webdriver/reference/bidi/modules/session/new/index.md
Outdated
Show resolved
Hide resolved
files/en-us/web/webdriver/reference/bidi/modules/session/status/index.md
Show resolved
Hide resolved
|
Thanks for the thorough review, Chris! Lots of food for thought. |
There was a problem hiding this comment.
OK, I'm pretty happy with this now, @dipikabh, so I'm approving from an editorial standpoint.
You've done a great job on this first set of documentation for WebDriver BiDi. It's obviously an unknown for us on the MDN team, and there will likely be things you'll want to improve on later after you've had some wider feedback. But for now, I think this is more than good enough to publish.
Good work.
|
Thanks so much, @chrisdavidmills! Your feedback has helped me think through the phrasing and details to look out for. It'll definitely help as I work through more BiDi pages.
Agree, that's always the case |
Description
This PR adds the following pages:
sessionlisting pagesession.newsession.statussession.endIt also updates:
webdriver.yaml)UPDATE: As part of addressing review feedback, this PR now also adds stub pages for all modules.
Additional details
Since BiDi builds on capabilities and errors defined in Classic, we can share a lot of those pages between the two.
So it might make sense to move them to common location under
Web/WebDriver/Referenceand out of the folders underWeb/WebDriver/Reference/Classicwhere they currently sit.Web/WebDriver/Referenceso that both Classic and BiDi commands can link to them.I'll make these changes in a separate PR. ✅
UPDATE: Done in #43314
@whimboo pointed out that the top landing page https://developer.mozilla.org/en-US/docs/Web/WebDriver is also in need of an update since the addition of our BiDi section - I'll update this page along with the above structural changes.
Spec links
session.new: https://w3c.github.io/webdriver-bidi/#command-session-newsession.status: https://w3c.github.io/webdriver-bidi/#command-session-statussession.end: https://w3c.github.io/webdriver-bidi/#command-session-endRelated issue
Doc issue: mdn/mdn#339