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 WebDriver BiDi network request logging #1540

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add WebDriver BiDi network request logging #1540

wants to merge 1 commit into from

Conversation

jgraham
Copy link
Member

@jgraham jgraham commented Nov 18, 2022 •
edited by pr-preview bot

Preview | Diff

@jgraham jgraham mentioned this pull request Nov 18, 2022
Merged
jgraham added a commit to w3c/webdriver-bidi that referenced this pull request Mar 8, 2023
@jgraham
Remove link to term that will later be defined in fetch
599474d 
This commit can be reverted once whatwg/fetch#1540 lands
jgraham added a commit to w3c/webdriver-bidi that referenced this pull request Mar 8, 2023
@jgraham
Remove link to term that will later be defined in fetch
e09c908 
This commit can be reverted once whatwg/fetch#1540 lands
jgraham added a commit to w3c/webdriver-bidi that referenced this pull request Mar 29, 2023
@jgraham
Remove link to term that will later be defined in fetch
05062e2 
This commit can be reverted once whatwg/fetch#1540 lands
jgraham added a commit to w3c/webdriver-bidi that referenced this pull request Mar 29, 2023
@jgraham
Remove link to term that will later be defined in fetch
057aa4c 
This commit can be reverted once whatwg/fetch#1540 lands
jgraham added a commit to w3c/webdriver-bidi that referenced this pull request Mar 30, 2023
@jgraham
Remove link to term that will later be defined in fetch
e97c0af 
This commit can be reverted once whatwg/fetch#1540 lands
@jgraham jgraham closed this May 15, 2023
@jgraham jgraham reopened this May 15, 2023
@jgraham jgraham force-pushed the webdriver_request_logging branch 2 times, most recently from 4eb1c56 to 968dea4 Compare May 19, 2023 08:37
@jgraham jgraham marked this pull request as ready for review May 19, 2023 08:40
annevk
annevk reviewed May 22, 2023
View reviewed changes
Copy link
Member

@annevk annevk left a 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.

fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
@jgraham jgraham force-pushed the webdriver_request_logging branch from 968dea4 to 5df348c Compare May 22, 2023 12:53
@dontcallmedom dontcallmedom mentioned this pull request May 22, 2023
Merged
dontcallmedom added a commit to tobie/specref that referenced this pull request May 22, 2023
@dontcallmedom @tidoust
Add WebDriver BIDI (#746)
c6b1ebe 
* 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>
@jgraham jgraham requested a review from annevk July 21, 2023 09:18
@jgraham jgraham closed this Jul 27, 2023
@jgraham jgraham reopened this Jul 27, 2023
@jgraham jgraham force-pushed the webdriver_request_logging branch 2 times, most recently from 34edf7e to 2998bd1 Compare July 27, 2023 15:38
annevk
annevk reviewed Sep 28, 2023
View reviewed changes
Copy link
Member

@annevk annevk left a 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?

fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
@jgraham jgraham force-pushed the webdriver_request_logging branch 3 times, most recently from 2747ae7 to d0c7b8a Compare January 4, 2024 12:35
@jgraham jgraham requested a review from annevk January 5, 2024 12:25
annevk
annevk reviewed Feb 1, 2024
View reviewed changes
Copy link
Member

@annevk annevk left a 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.

fetch.bs
<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]]
Copy link
Member

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.

Copy link
Member Author

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.
Copy link
Member

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?

Copy link
Member Author

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.

Copy link

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!

@jgraham jgraham requested a review from annevk April 9, 2024 11:43
@whimboo
Copy link

whimboo commented May 23, 2024

@annevk gently ping for review. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@whimboo whimboo whimboo left review comments

@dontcallmedom dontcallmedom dontcallmedom left review comments

@annevk annevk Awaiting requested review from annevk

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jgraham @whimboo @dontcallmedom @annevk