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.

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

[Javascript] Add support for installing addon by remote path in Firefox #9494

Conversation

nickgaya
Copy link
Contributor

@nickgaya nickgaya commented May 19, 2021

Description

Add a remotePath parameter to the firefox Driver.installAddon() method to allow specifying a path to the addon on the WebDriver server.

Motivation and Context

Geckodriver allows two ways of specifying an addon to install at runtime:

  • The addon contents may be specified with the addon parameter
  • The path to the addon may be specified with the path parameter

Currently the Javascript code reads the specified path to a string and sends it using the addon parameter. This requires the addon to be in a "packed" .xpi or .zip file format.

This change adds the capability to send the path to the remote server instead of the addon data. In this case, the path may be either a .xpi/.zip file or an unpacked directory.

See #8357

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@CLAassistant
Copy link

CLAassistant commented May 19, 2021

CLA assistant check
All committers have signed the CLA.

@nickgaya nickgaya changed the title Add support for installing addon by remote path in javascript api for Firefox [Javascript] Add support for installing addon by remote path in Firefox May 20, 2021
@AutomatedTester
Copy link
Member

@nickgaya Sorry for taking so long to get to this PR. What does remotePath mean here? Do you just mean a path?

You mention that it uses the path on the server, how does it get there?

@nickgaya
Copy link
Contributor Author

@AutomatedTester When using Selenium Grid, the WebDriver library is invoked on one machine (the "local" environment) and sends commands to a browser which may be running on a different machine (the "remote" environment).

The remotePath parameter is a boolean which indicates whether the path parameter refers to a local path, or a path on the remote machine:

  • If remotePath is false (the default), the client reads the path locally and sends the contents to the server as binary data. This only supports extensions that have been packed into a .zip/.xpi file.

  • If remotePath is true, the client sends the path to the remote server. This enables us to specify an unpacked extension directory (see the new test case).

Copy link
Member

@titusfortner titusfortner 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 this work, but I'm implementing the other languages per #8357 the way the JS bindings are now. There is not a compelling reason not to use toString('base64') on everything and always using addon parameter. The Mozilla commit indicated this was a more of a replacement than an alternative, and having an extra parameter to toggle between these is an unnecessary/confusing complication/maintenance burden, imo.

@nickgaya
Copy link
Contributor Author

Hi @titusfortner, I'm not sure exactly what you mean.

I'm implementing the other languages per #8357 the way the JS bindings are now

#8357 describes a use-case which is not currently supported by the JS bindings, as far as I know.

There is not a compelling reason not to use toString('base64')

This PR allows the user to install "unpacked" addon directories which cannot be converted to a base64-encoded string. I believe this is a fairly common use-case when developing/testing an addon.

See this project for an example. In this example, I used the low-level command API as a workaround, but it would be preferable to be able to do this with the installAddon() method.

@titusfortner
Copy link
Member

titusfortner commented Dec 27, 2021

which cannot be converted to a base64-encoded string

Ah, but it can be, I've tested it. You can zip the directory, encode it and send it along with addon.

@nickgaya
Copy link
Contributor Author

nickgaya commented Dec 27, 2021

@titusfortner You are correct that you can install a packed extension .zip or .xpi file with the current API.

This PR addresses feature request #8357, "As a Firefox extension developer, I would like to install a temporary extension from a folder".

Since it's already supported by geckodriver I don't see a reason for Selenium not to support this.

Ultimately I think it's best if we can make both forms of the install addon command available to developers via the webdriver API. If the remotePath parameter is confusing, perhaps we could change that so it's more understandable. One option would be to accept either a path or a byte string for the first argument, and use either path or addon parameters based on which was provided.

@titusfortner
Copy link
Member

titusfortner commented Dec 27, 2021

Look, I see exactly what you are doing and why and it does indeed allow you to set a profile directory as temporary and use it when the directory exists on the same machine as the driver.

You've even created a new extension, which is great, and I'd like to put it in our //common directory for use by all the languages. Can you make a PR to add an extensions directory and put packed file and unpacked directory here? https://github.com/SeleniumHQ/selenium/tree/trunk/common

I'm not a JS dev, so I can't speak to the code, but I've been working with this specific feature in all the other languages, and this solution has a few things that aren't ideal.

  1. remotePath is a confusing name for most users. People's first instinct is to set it as true if they are executing on a remote machine, whereas 99% of the time people who need to toggle remotePath will do so when they are running locally (with this implementation).
  2. In Selenium project we use the File Detector for this, specifically so people don't have to toggle this kind behavior.
  3. The final implementation should work for someone who has a directory on the local machine and wants to test it on a remote machine
  4. The final implementation should generally fit what is being done in other languages (but we/I can update the other languages if we find a better approach in JS).

So maybe the method can check if the path provided is a directory and zip it for the user. It will work both locally and remotely without the need for a toggle or a File Detector.

@nickgaya
Copy link
Contributor Author

Okay, if there's a plan to address #8357 in a different way that's standardized across languages, then this PR is probably not needed.

The unpacked extension in this PR is just the unzipped version of the existing webextension.xpi in /javascript/node/selenium-webdriver/lib/test/data/firefox; feel free to use that when writing tests for other language bindings.

@nickgaya nickgaya closed this Dec 27, 2021
@nickgaya nickgaya deleted the javascript-firefox-install-addon-remote-path branch December 27, 2021 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants