-
-
Notifications
You must be signed in to change notification settings - Fork 8k
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
[SafariDriver] [Java] Support custom extensions for Safari / SafariOptions API #87
Conversation
For this particular change, with respect to novice Selenium and Safari users, I'd like to point out that to use the feature optimally, you'd always want to place a copy of the Safari extensions (the installers) to add in a temp location to then load with the code presented here, or store the extensions in a read-only directory. Because Safari will (as I observe) always delete the extension installer file (name.safariextz) after the extension is installed, a lame move by Apple, since in some cases, you haven't archived a backup of the extension and you've just lost it on install. Or does the code changes here transparently handle that scenario so you don't have to worry about it? |
@@ -88,6 +88,12 @@ public Response send(Command command) throws InterruptedException { | |||
|
|||
return responses.poll(3, TimeUnit.MINUTES); | |||
} | |||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing blank line between methods.
Thanks for picking this up. Have you signed the CLA? |
@jleyba Yes I've signed the CLA. With the latest commits, none of the previous public APIs were changed (except for renames of two classes). I have, however, added new methods (see 3d3124d) for defining the options. What remains it to get the SafariDriver to recognize the new options on SafariOptions, preferably remaining backwards-compatible. Should I mark the previous capabilities as deprecated, and if yes, how? |
@jleyba I've rebased the commits into logical chunks, and updated their commit messages as well as the initial post of this PR. Could you review it? For the record, this is the result of the tests: http://pastebin.com/VMELWhMA (there are several unrelated errors, but these already existed before my PR, and they are not related to the changes I made). |
I haven't forgotten about this PR. Will try to finish up my next round of comments today. |
@@ -46,6 +49,7 @@ | |||
* <p><strong>Warning:</strong> Since Safari uses a single profile for the | |||
* current user, enabling this capability will permanently erase any existing | |||
* session data. | |||
* @deprecated use {@link SafariOptions#setUseCleanSession(boolean)} instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add the @Deprecation annotation too
* {@link #addExtensions(java.util.List)} and | ||
* {@link #setDriverExtension(File)}. | ||
* | ||
* @param skipExtensionInstallation If true, the installation of extensions is skipped. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: document boolean values as "Whether..." statements: Whether to skip extension installation.
Looks good - just a few nits |
@jleyba Thanks for the review. I've revised the code. The first commit deals with comments and The functionality has not changed, my tests are still running fine. I'll squash my last two commits after you've reviewed it. |
return itsOkToQuitMultipleTimes; | ||
if (DriverCommand.QUIT.equals(command.getName())) { | ||
if (server.isRunning() && connection != null) { | ||
connection.close(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can revert these changes. I just pushed a change that should make this unnecessary
https://code.google.com/p/selenium/source/detail?r=d84a49c4e2a3
@jleyba Can you review my latest two commits please? If they look good to you, I'll remove the commit 2ee4817 and squash the last commits to finalize the PR. |
Hi everybody. |
@badavid Please answer the following questions:
|
I have openned a new ticket. |
I have import all java class from the zip but i can't find other librairy to compile correctly ( org.json.jboss etc ..) |
Everything LGTM. |
LGTM ? What does it means ? |
Looks Good To Me ;) |
@jleyba Thanks for the approval. I've squashed the commits. It's identical to the previous state, except for the removal of commit 2ee4817 (as a response to #87 (diff)). |
@Rob--W Can you squash everything down into a single commit? |
@jleyba Done. |
[SafariDriver] Tests for custom Safari extensions The SafariOptions API is modelled after the ChromeOptions API, to make the learning curve easier. The APIs are compared at http://stackoverflow.com/a/17117849/938089 The SafariDriverExtension class has been renamed to SafariExtensions to emphasize the new role of the class. It's not just responsible for the installation of the SafariDriver, but also for the installation of custom Safari extensions. Moved several options from SafariDriver to SafariOptions: - Hard-coded "port" property in SafariDriver + SafariDriverServer was - Processing of "data dir" / "clean session" / "skip extension install" options has been moved from SafariDriver to SafariOptions. The corresponding capabilities are still supported, though deprecated. - Previously, setting the "skip extension install" property implied that the user has installed a custom safari driver in the browser. Setting the option would skip the replacement of Extensions.plist, causing all existing extensions to stay. With the advent of custom extensions, "skip extension install" became ambiguous: Does the user want to use a custom driver, or skip installation of all extensions, in order to use his existing extensions? The "skip extension install" feature has been replaced with two methods, "setSkipExtensionInstallation" and "setDriverExtension". The NO_INSTALL_EXTENSION_CAPABILITY capability maps to "setSkipExtensionInstallation" for backwards-compatibility. - The old capabilities have been deprecated, but they're still recognized. When the new SafariOptions.CAPABILITY capability is present, the old capabilities will be ignored. - For backwards-compatibility with old Selenium remote servers, the old capabilities are still set by the toCapabilities() method.
Merged in 2a453fa |
This is no longer working with selenium 3. Is there another way to do it ? |
I am unable to get the addExtensions method for SafariOptions class object, could any one guide me on this |
Solves https://code.google.com/p/selenium/issues/detail?id=4852
I've created a new class (
SafariOptions
) which allows one to add one or more custom extensions. To make it easier for developers to use it, the method name and signature is similar toChromeOptions
'saddExtensions(File...)
.This PR does not only bring extensions. It also separated the options from the SafariDriver. The old capabilities are still supported, but have been deprecated.
All new APIs are well-documented, so regenerating the javadocs after acceptance makes sense.
Example of installing extension
Tests
Three tests have been added:
I've also patched the implementation of the
DriverCommand.QUIT
method, by closing the Web Socket connection in Java instead of sending QUIT to the Safari extension (the Safari extension silently ignores the command).Before this fix, invoking
safariDriver.quit()
would always cause an IOException to be thrown: "An existing connection was forcibly closed by the remote host".Another problem was that an existing test class did not inherit from SafariTestBase. This caused several tests to behave badly, which has also been fixed.
Ideas for future improvements
My current objective was to automate the tests of a Safari extension. Others might actually want to preserve the existing set of installed extensions, instead of adding new ones. In order to achieve this, use a third-party library (such as https://code.google.com/p/plist/) to parse
Extensions.plist
, and add the WebDriver Safari extension by appending values to Extensions.plist instead of overwriting it.Signed SFC
I've signed the SFC Individual Contributor License Agreement.