Refactor session handling for better integration with BiDi #1608

jgraham · 2021-07-14T11:29:31Z

This is a large change, but it's almost entirely refactoring.

The main aim here is to split the session concept so that there's a base "session" which holds the session id and state that's appropriate to both WebDriver classic and BiDi. Then, we define a "HTTP Session" as one that has the HTTP Flag set and use that to compose-in any state that's only used in the classic HTTP-based protocol.

In addition, we rework the way state is passed around the spec so that we are using an explicit session object, rather than making use of a global "current session". This makes things clearer if we allow the possibility of multiple non-HTTP sessions.

There is also a bit of cleanup in the way we pass other variables around, and in the capabilites processing.

Preview | Diff

index.html

whimboo · 2021-07-20T11:10:44Z

index.html


 <li><p>Let <var>body</var> be a JSON <a>Object</a> initialised with:

  <dl>
   <dt>"<code>sessionId</code>"
-   <dd><var>session id</var>
+   <dd><var>session</var>'s <a>session ID</a>.

   <dt>"<code>capabilities</code>"
   <dd><var>capabilities</var>


Maybe this should be similar to session id?

Suggested change

<dd><var>capabilities</var>

<dd><var>session</var>'s <a>capabilities</a>

It's the same <var>capabilities</var> as defined above, so I don't think this change is required.

whimboo · 2021-07-20T11:11:57Z

index.html

- <li><p><a>Set the current top-level browsing context</a>
-   for <var>session</var> with the <a>top-level browsing context</a>
+ <li><p>Set the <a>current top-level browsing context</a>
+   of <var>session</var> to the <a>top-level browsing context</a>


should we add: of the currently selected tab? Otherwise it would be unclear in case the browser starts up with multiple tabs already open.

This part generally doesn't make sense, we can't set the current top level browsing context to the current browsing context, because that doesn't exist. Basically I think the UA is free to pick any borwsing context at this point, but I've tried to add some language suggesting it should pick a reasonable one.

index.html

whimboo · 2021-10-15T05:48:03Z

CC'ing @foolip and @bwalderman for their reviews. It would be great to get this PR landed.

shs96c

I think we'll need to be particularly careful to make sure that it's possible to reformulate this spec on top of Bidi, and I've called out a few places where I'm not sure that would be possible with this PR.

index.html

shs96c · 2022-03-14T10:56:50Z

index.html

-   to their corresponding values.
+   <li><p>If <var>request match</var> is of type <a>error</a>,
+    <a>send an error</a> with <var>request match</var>’s <a>error code</a>
+    and continue.


I guess the intent here is that the returned error is forwarded to the local end? We should also ensure that error's data is also returned.

An error doesn't have data, only a code. But the send an error steps allow the implementation to provide an arbitary message so the intent is really that we give the user some useful information, without wanting to specify exactly what that must be.

[some time later after reading the spec again]

Ah, I see there's an error data concept that is apparently exclusively used with unexpected alert open. Unfortunately it looks like that isn't hooked up to the error handling infrastructure at all corrrectly. I don't really want to further scope-creep this PR by also fixing that, but it could be fixed. Mind filing an issue?

shs96c · 2022-03-14T10:57:48Z

index.html

-  <ol>
-   <li><p>Let <var>session id</var> be the corresponding variable
-    from <var>command parameters</var>.
+    <li><p>If <var>URL variables</var> has an item named session id:


It would be helpful to indicate that "session id" is a variable name.

It's not really a variable name though; it's the name of the field in a struct. In other specs it seems like the fields of the struct are individually defined, so that they can be linked on use. But since the fields of the struct in this case depend on which template variables were populated it seems harder to use that pattern. So I'm not realy sure what the cleanest way to mark this up is.

shs96c · 2022-03-14T10:58:22Z

index.html

-   <a>send an error</a> with <a>error code</a> <a>invalid session id</a>,
-   then jump to step 1 in this overall algorithm.
+     <ol>
+       <li><p>Let <var>session id</var> be <var>URL variables</var>'s session id.


index.html

shs96c · 2022-03-14T14:23:13Z

index.html

-  amongst <a>all associated cookies</a>
-  of the <a>current browsing context</a>’s <a>active document</a>,
-  return <a>success</a> with the <a>serialized cookie</a> as data.
+ <li><p>If the <var>URL variables</var>' name is equal to


Missing s after the apostrophe

jgraham · 2022-03-18T21:20:15Z

I've done a round of fixup here.

One thing that apparently isn't very obvious from the text is how I'm thinking about the session flags. The basic idea was to have a single kind of session object, but let it compose together the HTTP session and BiDi session properties, so that we can have all of the combinations HTTP-only, BiDi-only, HTTP+BiDi, and extend to anything new we want in the future. So the idea was that the flags are used to specify which kind of session a given session is, and knowing that you can tell what properties it must have e.g. if a session is a HTTP session it must have a timeouts property that you can access, but a BiDi session doesn't. The idea wasn't to group together all the boolean properties of a session into a single flags object.

index.html

whimboo · 2023-06-21T07:35:03Z

Hi @shs96c. Would you mind following up on some of the replies from @jgraham? Not sure if you think that those issues are still relevant or can be closed. It would be nice to eventually get this massive PR merged. Thanks.

OrKoN · 2023-10-10T09:45:22Z

it looks like there are no remaining unanswered questions and this PR needs a rebase?

With BiDi we want to support multiple sessions, at most one of which can also enable the HTTP frontend. Therefore we add the concept of multiple sessions, which can have different flags set according to the protocols they support (http, bidi, etc.). This explictly passes the session, parameters and url variables down into each command. In addition the URL variables are handled as a infra struct rather than as a custom thing with implied variables. Instead of having a concept of the "current session" we explicitly pass the session around where required.

OrKoN

LGTM (@sadym-chromium PTAL)

index.html

Co-authored-by: Maksim Sadym <69349599+sadym-chromium@users.noreply.github.com>

Review is outdated now; going to land this to prevent further bitrot, but if there are remaining issues we can handle those in followups.

SHA: 63a397f Reason: push, by jgraham Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

SHA: 63a397f Reason: push, by pull[bot] Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

jgraham requested review from AutomatedTester, shs96c and andreastt as code owners July 14, 2021 11:29

jgraham mentioned this pull request Jul 14, 2021

Add the ability to start a session directly with the bidi connection w3c/webdriver-bidi#99

Merged

whimboo reviewed Jul 20, 2021

View reviewed changes

index.html Show resolved Hide resolved

index.html Show resolved Hide resolved

index.html Show resolved Hide resolved

index.html Show resolved Hide resolved

index.html Show resolved Hide resolved

whimboo reviewed Jul 20, 2021

View reviewed changes

index.html Outdated Show resolved Hide resolved

jgraham force-pushed the http_session branch from dcc017b to 8717fbe Compare September 16, 2021 13:56

whimboo mentioned this pull request Oct 15, 2021

Proposal: upgrade path from WebDriver to bidirectional DevTools Protocol #1331

Closed

shs96c previously requested changes Mar 14, 2022

View reviewed changes

jgraham mentioned this pull request Feb 1, 2023

Broken WebDriver classic references w3c/webdriver-bidi#360

Open

9 tasks

lutien reviewed Jun 5, 2023

View reviewed changes

index.html Show resolved Hide resolved

jgraham requested a review from shs96c June 16, 2023 14:33

jgraham force-pushed the http_session branch 2 times, most recently from 7e9826c to 37d2eb9 Compare February 21, 2024 15:58

jgraham force-pushed the http_session branch from 37d2eb9 to 7ca2626 Compare February 21, 2024 17:49

OrKoN requested a review from sadym-chromium February 22, 2024 08:06

OrKoN approved these changes Feb 22, 2024

View reviewed changes

sadym-chromium reviewed Feb 26, 2024

View reviewed changes

index.html Outdated Show resolved Hide resolved

sadym-chromium approved these changes Feb 26, 2024

View reviewed changes

Update index.html

b70ad7e

Co-authored-by: Maksim Sadym <69349599+sadym-chromium@users.noreply.github.com>

jgraham merged commit 63a397f into master Feb 26, 2024
2 checks passed

jgraham deleted the http_session branch February 26, 2024 16:14

github-actions bot added a commit that referenced this pull request Feb 26, 2024

Refactor session handling for better integration with BiDi (#1608)

c15e91e

SHA: 63a397f Reason: push, by jgraham Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

rakuco mentioned this pull request Feb 28, 2024

Broken references in Generic Sensor API w3c/sensors#486

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor session handling for better integration with BiDi #1608

Refactor session handling for better integration with BiDi #1608

jgraham commented Jul 14, 2021 •

edited by pr-preview bot

Loading

whimboo Jul 20, 2021 •

edited

Loading

jgraham Sep 16, 2021

whimboo Jul 20, 2021

jgraham Sep 16, 2021

whimboo commented Oct 15, 2021

shs96c left a comment

shs96c Mar 14, 2022

jgraham Mar 18, 2022

shs96c Mar 14, 2022

jgraham Mar 18, 2022

shs96c Mar 14, 2022

shs96c Mar 14, 2022

jgraham commented Mar 18, 2022 •

edited

Loading

whimboo commented Jun 21, 2023

OrKoN commented Oct 10, 2023

OrKoN left a comment

	<dd><var>capabilities</var>
	<dd><var>session</var>'s <a>capabilities</a>

Refactor session handling for better integration with BiDi #1608

Refactor session handling for better integration with BiDi #1608

Conversation

jgraham commented Jul 14, 2021 • edited by pr-preview bot Loading

whimboo Jul 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whimboo commented Oct 15, 2021

shs96c left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgraham commented Mar 18, 2022 • edited Loading

whimboo commented Jun 21, 2023

OrKoN commented Oct 10, 2023

OrKoN left a comment

Choose a reason for hiding this comment

jgraham commented Jul 14, 2021 •

edited by pr-preview bot

Loading

whimboo Jul 20, 2021 •

edited

Loading

jgraham commented Mar 18, 2022 •

edited

Loading