w3c / webdriver-bidi

Bidirectional WebDriver protocol for browser automation
https://w3c.github.io/webdriver-bidi/
336 stars 35 forks source link

Serialize WindowProxy with full browsingContext.info and not just context #580

Open whimboo opened 8 months ago

whimboo commented 8 months ago

When serializing a WindowProxy we currently only add the context id to the WindowProxyProperties. Sadly this misses information if it is a top-level browsing context or a frame, which would be helpful to have in WebDriver clients that also make use of WebDriver classic. Without this information the client doesn't know if it should use a web frame or a web window reference when passing it to script evaluation.

To get around that we could actually add the full browsingContext.Info to the value field. Then the client knows if a parent browsing context exists or not, and can appropriately create the correct WebDriver classic type.

Should we make that change with the cost of having a bit more data to include? We should leave the depth for sure at 0 to only include the current browsing context.

What do you think?

CC @jgraham, @gsnedders, @Lightning00Blade, @OrKoN, @jimevans.

Btw. tests haven't been landed yet but are currently created for the context only field via https://github.com/web-platform-tests/wpt/pull/41601.

OrKoN commented 8 months ago

I think the information on whether the context is top-level or not can be retrieved from the browsing context tree? I am not sure we necessarily need it. Is there an client/use case for this? @Lightning00Blade would we make use of smth like this in Puppeteer?

whimboo commented 8 months ago

Note that this would always require another round trip to retrieve that information, which in case of a remote machine can add an overhead when it has to go through the network.

OrKoN commented 8 months ago

Note that this would always require another round trip to retrieve that information, which in case of a remote machine can add an overhead when it has to go through the network.

Puppeteer keeps a local copy of the browsing context tree so no round-trip is required. The state is updated locally based on the browsing context module events.

whimboo commented 8 months ago

Ok, then it would be good to get some feedback from Selenium folks. CC @shs96c, @jimevans, @AutomatedTester, @titusfortner, @christian-bromann.

jimevans commented 8 months ago

Since Selenium does not keep a cached copy of the browsing context tree, and indeed could not do so because WebDriver classic has no event-based mechanism like that provided by BiDi or CDP, having that additional information would be incredibly useful. The more I work on this spec, the more I feel like round-trip reduction would be to the benefit of all client implementations, and I’ll support almost any initiative that makes such reduction possible. Keeping an up-to-date local copy of the browsing context tree might be the right architectural choice for some client implementations, but it’s the height of arrogance to assume it’s the correct choice for every client implementation.

jgraham commented 8 months ago

As a general rule if we can make it unnecessary for clients to cache state locally that seems like a win. Generally being able to use the browser as a single source of truth avoids a whole class of problems related to state synchronisation and cache invalidation.

css-meeting-bot commented 8 months ago

The Browser Testing and Tools Working Group just discussed Serialize WindowProxy with full browsingContext.info and not just context.

The full IRC log of that discussion <AutomatedTester_> Topic: Serialize WindowProxy with full browsingContext.info and not just context
<AutomatedTester_> github: https://github.com/w3c/webdriver-bidi/issues/580
<AutomatedTester_> whimboo_: This is a follow up to get feedback on the serialising the WindowProxy
<AutomatedTester_> ... and to share back with clients what the type of the window
<sadym> q+
<AutomatedTester_> ... adding this info on the response we can minimize all the round trips back to the client and simplify the interop hopefully
<AutomatedTester_> ack next
<AutomatedTester_> ... in puppeteer they cache the window objects
<AutomatedTester_> ... but we don't expect that in selenium
<AutomatedTester_> MaksimSadym: I wonder if its good enough to send back just the frame or top level
<AutomatedTester_> ... or if we need to pass back a lot more info e.g. parent/children
<whimboo_> q+
<AutomatedTester_> ack next
<jgraham_> q+
<sadym> q+
<AutomatedTester_> whimboo_: if we don't want to have the full browsing context info we could add a flag e.g. if it's a top level it returns without the flag and with the frame it has it
<AutomatedTester_> q?
<AutomatedTester_> ack next
<AutomatedTester_> jgraham_: we can serialise the children. it is a reasonable concern if we are sharing too much info but we can return it with extra info url and ids
<AutomatedTester_> ack next
<jgraham_> s/can serialise/can skip serialising/
<jgraham_> s/extra info/only/
<jgraham_> s/and ids/and parent/
<sadym> We aren't opposed to the suggestion, but I wonder if the justification "X is needed in case of cross-operations between BiDi and Classic to reduce round trips" to extend data / add new commands etc it enough
whimboo commented 5 months ago

To wrap up:

1) We could skip serializing the children and only return the info of the top-level browsing context 2) We could reduce the amount of details for the info and maybe only return the additional parent id if the bc is a frame so it can be determined by the client directly if it is a top-level browsing context (window) or now (frame)

Alternative 2) sounds probably more reasonable then.

We aren't opposed to the suggestion, but I wonder if the justification "X is needed in case of cross-operations between BiDi and Classic to reduce round trips" to extend data / add new commands etc it enough

I would actually leave it up to the Selenium folks and if it's necessary / helpful to them. @jimevans, @AutomatedTester, and @pujagani what do you think?

pujagani commented 5 months ago

I agree with the motivation for this issue and it will be helpful to add this for Selenium. We plan to port all classic commands to use BiDi eventually for Selenium 5 and I can see good use of this information.