add support for pinning scripts in remote endpoints

[titusfortner]

This is the feature that was discussed at TPAC. What wasn't mentioned was this implementation where scripts are associated with names. The idea is that intermediary nodes (like Sauce) can provide a list of named scripts available to the user without the user needing to pin them at the beginning of their tests. If scripts were only listed with UUIDs generated by the end node, this would not be possible.

I'm looking forward to hearing back on whether this PR works for how the rest of the group understood the feature request.

There's one spot in here that I'm not sure I've specified correctly, but the intent should be clear, so let me know what needs changing. Thanks!

---

Preview | Diff

[titusfortner]

I've linked my github account to my w3c account, not sure how to revalidate the failing check.

[jgraham]

Is there a corresponding wpt PR adding tests for the new feature?

[titusfortner]

@jgraham https://github.com/web-platform-tests/wpt/pull/19675

[andreastt]

I have two questions:

1. We want to add extension locator strategies to WebDriver, and one way to do that would be to register a chunk of JS with the driver and associate it with a custom locator name. Such an approach would share many characteristics with script pinning, and thus introduce a dependency here.

   My preference would be not to share the same infrastructure for extension locators as for script pinning because I think there are better ways (pre-registration through capabilities for example), but I wanted to make sure to raise the question that we all feel that way, and that extension selectors should not be implemented with pinned scripts?

2. Is there a use case to replace pinned scripts at runtime? If that is not a strong requirement, a simpler implementation would be to provide pinned scripts at session creation.

[jgraham]

I have two questions:

1. We want to add extension locator strategies to WebDriver, and one way to do that would be to register a chunk of JS with the driver and associate it with a custom locator name. Such an approach would share many characteristics with script pinning, and thus introduce a dependency here.
   My preference would be _not to share_ the same infrastructure for extension locators as for script pinning because I think there are better ways (pre-registration through capabilities for example), but I wanted to make sure to raise the question that we all feel that way, and that extension selectors should not be implemented with pinned scripts?

I think they should share the same infrastructure; they are basically the same feature but with the difference that the locator strategies must return either an element or an array of elements. I think that can work well with the approach in this PR.

2. Is there a use case to replace pinned scripts at runtime? If that is not a strong requirement, a simpler implementation would be to provide pinned scripts at session creation.

Only allowing pinned scripts to be uploaded at session creation seems interesting but I wonder if it's flexible enough. At session creation time you might not know that you're running e.g. react app tests which need a particular set of tests. With that in mind I wonder if it should be possible to remove one or all pinned scripts so you could pin some scripts in a fixture for a set of tests and unpin those scripts in the teardown for the tests without requiring the overhead of getting a new session between test modules.

[andreastt]

Only allowing pinned scripts to be uploaded at session creation seems interesting but I wonder if it's flexible enough. At session creation time you might not know that you're running e.g. react app tests which need a particular set of tests. With that in mind I wonder if it should be possible to remove one or all pinned scripts so you could pin some scripts in a fixture for a set of tests and unpin those scripts in the teardown for the tests without requiring the overhead of getting a new session between test modules.

This sounds reasonable to me.

I wonder if this means having an endpoint to clear the pinned scripts might be necessary. The way this proposal works the client would need to hold on to a reference to the pinned script and I wonder if this might be a problem for long-running sessions.

[titusfortner]

@twalpole this was actually your idea that I stole, chime in with your opinions, please.

[titusfortner]

@andreastt

1. We want to add extension locator strategies to WebDriver, and one way to do that would be to register a chunk of JS with the driver and associate it with a custom locator name. Such an approach would share many characteristics with script pinning, and thus introduce a dependency here.

I don't think we need something different for element location, because we already handle the use case of execute/sync endpoint returning an Element ID value.

All the client bindings need to be able to do is to allow the creation of custom locators. Like in Ruby I'd do something like:

# In Selenium bindings
Selenium::WebDriver::SearchContext::CUSTOM_FINDERS = []

def find_element(how, what)
  if FINDERS[how.to_sym]
    by = FINDERS[how.to_sym] 
    bridge.find_element_by by, what.to_s, ref
  elsif CUSTOM_FINDERS[how.to_sym]
    custom_proc = CUSTOM_FINDERS[how.to_sym]
    custom_proc.call(bridge, what)
   end
end

# In framework code (or user's test code)
def self.register_react_locator!
  driver.pin_script(:react_location, react_locate_script)
  Selenium::WebDriver::CUSTOM_FINDERS[:react] = Proc.new |context, args| { 
  context.execute_script(:react_location, *args)}
end

# In user's test code
MyTestFramework.register_react_locator!
driver.find_element(react_locate: 'MyComponent')

2. Is there a use case to replace pinned scripts at runtime? If that is not a strong requirement, a simpler implementation would be to provide pinned scripts at session creation.

Are you suggesting that we should only pin scripts at session creation? I'd like more flexibility than that.

[andreastt]

I don't think we need something different for element location, because we already handle the use case of execute/sync endpoint returning an Element ID value.

In my original comment I was actually suggesting we might need something different for custom locator strategies.

It is true you can work around this in the local end by calling a pinned script, but it would be more flexible and less dependent on a particular client implementation if the element retrieval commands would accept custom strategies in the shape of {"strategy": "react"}, where the react strategy is pre-defined in some way.

In any case, I’m glad we’ve established there’s no dependency here.

Are you suggesting that we should only pin scripts at session creation? I'd like more flexibility than that.

Some concrete use cases for script pinning at runtime were mentioned in https://github.com/w3c/webdriver/pull/1445#issuecomment-541732678 and I said in https://github.com/w3c/webdriver/pull/1445#issuecomment-541749149 that I was happy with the reasoning for that.

[titusfortner]

In my original comment I was actually suggesting we might need something different for custom locator strategies.

it would be more flexible and less dependent on a particular client implementation if the element retrieval commands would accept custom strategies

Yes, I agree the bindings need this. I think @shs96c said he had ideas related to this, but I don't know what he was considering. I like the lambda / closure approach I outlined above (which I think does what you are suggesting @andreastt), but I'm not sure how "Ruby" of an approach that is relative to what other languages can do.

[titusfortner]

So to summarize the various things that are still outstanding in the comments above.

I'm going to:

1. ~~Change the names to not be part of the endpoints so that we can use colons~~ 4a79a5dbe0ebc079915320005cf3c4b769fb2650
2. ~~Change the data structure to use Map instead of list & table~~ accf942c9ac3e9479645835b119e1d41c71f14fc

Outstanding questions/decisions:

1. Do we want list / delete / delete all endpoints (my vote is no)
2. Do references to "script" need to be changed to "pinned script" for clarity? (I don't think so, but let me know)

Are there other outstanding issues to be addressed from the above that I'm glossing over?

[twalpole]

@titusfortner After a quick perusal of the comments here, for my use case I have no need for getting a list of the names, or for deleting scripts. Being able to overwrite an existing script would probably be useful though. I definitely wouldn't want a limit on the script size since the whole point of the feature is to replace large script transfers with much smaller calls. I'll try and read everything in more depth later today and may have some more useful comments.

[andreastt]

@titusfortner

I think there is agreement on the general approach, but there are a number of outstanding issues that prevents this PR from landing.

Do you have time to move this forward?

I would like to wrap up all outstanding PRs before I proceed with https://github.com/w3c/webdriver/issues/1462.

[titusfortner]

I'm... not certain I will have time this week. Maybe the weekend or early next week?

[christian-bromann]

This can be eventually replaced by the Bootstrap Scripts idea that was suggested for the BiDi proposal. This would solve the same use case of pinning scripts to a certain execution context (e.g. on a frame before page load) to attach functionality to the script context.

[shs96c]

I'm not sure that's true. A bootstrap script is run in every new instance of a context. The pinnable scripts are a shorthand for sending a single script to be executed right now across the wire: that is, the pinned scripts aren't available in every context, and so don't interfere with things like loading times or the performance of the browser.

The two could probably play nicely together: either choosing to update a bootstrap script using a pinned script, or calling something that needs to be on every page that's injected using a bootstrap script via a pinned script.

[whimboo]

Recently we landed the spec prose for Preload scripts in WebDriver BiDi. As such I'm fairly sure that we no longer want to add this feature to WebDriver classic. If we close as wontfix we should also close https://github.com/web-platform-tests/wpt/pull/19675.

@jgraham do you have any objections?

[jgraham]

I agree that the main feature here is covered by preload scripts. The additional feature request at TPAC was the ability to use these as part of a locator strategy, but in theory middleware could already make that work (by intercepting the locator strategy and converting it into script execution).

[christian-bromann]

This is awesome, thanks folks!