-
Notifications
You must be signed in to change notification settings - Fork 337
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
Add WebDriver BiDi network request logging #1540
base: main
Are you sure you want to change the base?
Conversation
fff0efb
to
7884a67
Compare
7884a67
to
4fa3194
Compare
This commit can be reverted once whatwg/fetch#1540 lands
This commit can be reverted once whatwg/fetch#1540 lands
This commit can be reverted once whatwg/fetch#1540 lands
This commit can be reverted once whatwg/fetch#1540 lands
This commit can be reverted once whatwg/fetch#1540 lands
4eb1c56
to
968dea4
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.
Thanks for tackling this. Looks reasonable overall, but I found some issues.
968dea4
to
5df348c
Compare
* Add WebDriver BIDI needed for whatwg/fetch#1540 (comment) * Update refs/biblio.json Co-authored-by: François Daoust <fd@tidoust.net> --------- Co-authored-by: François Daoust <fd@tidoust.net>
5df348c
to
f010d62
Compare
f010d62
to
e5bc282
Compare
34edf7e
to
2998bd1
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.
@jgraham apologies for the delay. Please resolve existing comment threads and maybe ping me if you haven't seen an update in a while.
I was wondering if there was a high-level description available somewhere so I can verify the various calls happen at the right time. Also happy to trust you, but it might help to have some additional review?
2747ae7
to
d0c7b8a
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.
Question about the overview still stands. Looks good otherwise.
<p>A <a for=/>request</a> has an associated <dfn export for=request>navigation id</dfn> | ||
(null or a string). Unless stated otherwise, it is null. | ||
|
||
<p class=note>This is for exclusive use by HTML's navigate algorithm. [[!HTML]] |
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.
What do you think about calling these fields "WebDriver navigation id" and "WebDriver id"? If we ever make them more generic we can rename them and the way WebDriver consumes them can just be as ID, but at least that would discourage others from using them for purposes they're not intended for without additional review.
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 don't really have a strong opinion so I changed them.
fetch.bs
Outdated
<a for=request>body</a>. | ||
<a for=request>body</a> and <a for=request>id</a>. | ||
|
||
<li><p>Set <var>newRequest</var>'s <a for=request>id</a> to a new unique string. |
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.
Is this a UUID in practice? Do we just want to settle on that?
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.
It doesn't seem to be at the moment. I'd happily require a UUID here, but I feel like we don't quite have consensus on UUIDs everywhere yet.
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.
@annevk would you mind taking another look at this PR? Thanks!
d0c7b8a
to
a808110
Compare
@annevk gently ping for review. Thanks. |
w3c/webdriver-bidi#204 has a brief overview in the description. I don't know if you wanted more detail than that. I think we know at this point there are some problems e.g. w3c/webdriver-bidi#722 which covers what happens with auth requests (technically specified, but probably not in a way that's easy to use). |
Thanks, that helps a lot! I left some questions in the authentication issue. I think we should probably try to solve for the model we want first. At least it doesn't make much sense to me to land something we don't feel confident about. |
(See WHATWG Working Mode: Changes for more details.)
Preview | Diff