search

w3c / webdriver-bidi

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

Consider removing `ownership` and `sandbox` from `browsingContext.locateNodes` #684

Closed sadym-chromium closed 3 months ago

sadym-chromium commented 3 months ago

browsingContext.locateNodes returns an array of script.NodeRemoteValue, and sharedId can be used instead of handle throughout all the sandboxes.

The reason for that is that in CDP implementation, resolving handle can be done only for the top-level object returned in result. And having that, in order to provide handles for all the nodes, a dedicated CDP call per node in the result should be done.

sadym-chromium commented 3 months ago

@jimevans WDYT? Would it still cover the use cases?

whimboo commented 3 months ago

I think (if I remember correctly) we originally added both because of a request from Google. From our side we do not have a special use-case given as you say the sharedId is stable as long as the process exists.

jimevans commented 3 months ago

@sadym-chromium We need sandbox because we call into the get a realm from a target algorithm. Likewise, we need ownership because we call into the serialize as a remote value algorithm. If we can make appropriate assumptions about the inputs of those values into those algorithms, yes, let’s remove the properties, but I don’t think you can make such assumptions (@jgraham correct me if I’m wrong).

Alternatively, if you want to reconfigure the remote end steps to use different algorithms, I’d be fine removing the properties in that case too. It’s unclear to me what other algorithms should be used in place of those cited above, but I’ll review any PR that makes an attempt to do so.

sadym-chromium commented 3 months ago

I think (if I remember correctly) we originally added both because of a request from Google. From our side we do not have a special use-case given as you say the sharedId is stable as long as the process exists.

@whimboo If we talk about a generic script serialization, we cannot provide a stable sharedId for a generic ecmascript objects, only for DOM. But the browsingContext.locateNodes returns a list of nodes, and we can do that.

sadym-chromium commented 3 months ago

@jimevans so the idea is to serialize with ownership: none, and never provide a handle. If so, we don't need a sandbox anymore. The serialize as a remote value requires realm only to provide handles in

step 4 Let handle id be the handle for an object with realm, ownership type and value.

If we don't have a major concerns here, I will prepare a PR.

sadym-chromium commented 3 months ago

Here we go: https://github.com/w3c/webdriver-bidi/pull/688

sadym-chromium commented 3 months ago

Implemented in https://github.com/w3c/webdriver-bidi/pull/688.