Make SafariOptions extend MutableCapabilities

This means that the `MutableCapabilities` need some smarts
to handle the common pattern of attempting to do something
like:

```
DesiredCapabilities caps = new DesiredCapabilities();
caps.setCapability(SafariOptions.CAPABILITY, new SafariOptiosn());
```

Author: shs96c

java/client/src/org/openqa/selenium/MutableCapabilities.java

@@ -21,14 +21,23 @@
 import java.io.Serializable;
 import java.util.Collections;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.Map;
+import java.util.Set;
 
 public class MutableCapabilities implements Capabilities, Serializable {
 
   private static final long serialVersionUID = -112816287184979465L;
+  private static final Set<String> OPTION_KEYS;
+  static {
+    HashSet<String> keys = new HashSet<>();
+    keys.add("safari.options");
+    OPTION_KEYS = Collections.unmodifiableSet(keys);
+  }
 
   private final Map<String, Object> caps = new HashMap<>();
 
+
   public MutableCapabilities() {
     // no-arg constructor
   }
@@ -106,6 +115,15 @@ public void setCapability(String capabilityName, Platform value) {
   }
 
   public void setCapability(String key, Object value) {
+    // We have to special-case some keys and values because of the popular idiom of calling
+    // something like "capabilities.setCapability(SafariOptions.CAPABILITY, new SafariOptions());
+    // and this is no longer needed as options are capabilities. There will be a large amount of
+    // legacy code that will always try and follow this pattern, however.
+    if (OPTION_KEYS.contains(key) && value instanceof Capabilities) {
+      merge((Capabilities) value);
+      return;
+    }
+
     caps.put(key, value);
   }
 

java/client/src/org/openqa/selenium/safari/SafariDriver.java

@@ -46,7 +46,9 @@ public SafariDriver() {
    * @see SafariOptions#fromCapabilities(org.openqa.selenium.Capabilities)
    *
    * @param desiredCapabilities capabilities requested of the driver
+   * @deprecated Use {@link SafariDriver(SafariOptions)} instead.
    */
+  @Deprecated
   public SafariDriver(Capabilities desiredCapabilities) {
     this(SafariOptions.fromCapabilities(desiredCapabilities));
   }
@@ -57,7 +59,7 @@ public SafariDriver(Capabilities desiredCapabilities) {
    * @param safariOptions safari specific options / capabilities for the driver
    */
   public SafariDriver(SafariOptions safariOptions) {
-    super(getExecutor(safariOptions), safariOptions.toCapabilities());
+    super(getExecutor(safariOptions), safariOptions);
   }
 
   private static CommandExecutor getExecutor(SafariOptions options) {

java/client/src/org/openqa/selenium/safari/SafariOptions.java

@@ -21,11 +21,15 @@
 import com.google.gson.JsonObject;
 
 import org.openqa.selenium.Capabilities;
+import org.openqa.selenium.MutableCapabilities;
+import org.openqa.selenium.Platform;
 import org.openqa.selenium.WebDriverException;
+import org.openqa.selenium.remote.CapabilityType;
 import org.openqa.selenium.remote.DesiredCapabilities;
 
 import java.io.IOException;
 import java.util.Map;
+import java.util.TreeMap;
 
 /**
  * Class to manage options specific to {@link SafariDriver}.
@@ -39,13 +43,12 @@
  * SafariDriver driver = new SafariDriver(options);
  *
  * // For use with RemoteWebDriver:
- * DesiredCapabilities capabilities = DesiredCapabilities.safari();
- * capabilities.setCapability(SafariOptions.CAPABILITY, options);
  * RemoteWebDriver driver = new RemoteWebDriver(
- *     new URL("http://localhost:4444/wd/hub"), capabilities);
+ *     new URL("http://localhost:4444/wd/hub"),
+ *     options);
  * </code></pre>
  */
-public class SafariOptions {
+public class SafariOptions extends MutableCapabilities {
 
   /**
    * Key used to store SafariOptions in a {@link DesiredCapabilities} object.
@@ -60,20 +63,16 @@ private Option() {}  // Utility class.
     private static final String PORT = "port";
   }
 
-  /**
-   * @see #setPort(int)
-   */
-  private int port = 0;
+  private Map<String, Object> options = new TreeMap<>();
 
-  /**
-   * @see #setUseCleanSession(boolean)
-   */
-  private boolean useCleanSession = false;
+  public SafariOptions() {
+    setCapability(Option.PORT, 0);
+    setCapability(Option.CLEAN_SESSION, false);
+    setCapability(Option.TECHNOLOGY_PREVIEW, false);
 
-  /**
-   * @see #setUseTechnologyPreview(boolean)
-   */
-  private boolean useTechnologyPreview = false;
+    setCapability(CapabilityType.BROWSER_NAME, "safari");
+    setCapability(CapabilityType.PLATFORM, Platform.MAC);
+  }
 
   /**
    * Construct a {@link SafariOptions} instance from given capabilites.
@@ -109,7 +108,7 @@ public static SafariOptions fromCapabilities(Capabilities capabilities)
    *    or 0 if the server should select a free port.
    */
   SafariOptions setPort(int port) {
-    this.port = port;
+    options.put(Option.PORT, port);
     return this;
   }
 
@@ -124,7 +123,7 @@ SafariOptions setPort(int port) {
    * @param useCleanSession If true, the SafariDriver will erase all existing session data.
    */
   public SafariOptions setUseCleanSession(boolean useCleanSession) {
-    this.useCleanSession = useCleanSession;
+    options.put(Option.CLEAN_SESSION, useCleanSession);
     return this;
   }
 
@@ -136,7 +135,7 @@ public SafariOptions setUseCleanSession(boolean useCleanSession) {
    *     otherwise will use the release version of Safari.
    */
   public SafariOptions setUseTechnologyPreview(boolean useTechnologyPreview) {
-    this.useTechnologyPreview = useTechnologyPreview;
+    options.put(Option.TECHNOLOGY_PREVIEW, useTechnologyPreview);
     return this;
   }
 
@@ -148,45 +147,27 @@ public SafariOptions setUseTechnologyPreview(boolean useTechnologyPreview) {
    * @see #setPort(int)
    */
   public int getPort() {
-    return port;
+    return (int) options.getOrDefault(Option.PORT, 0);
   }
 
   /**
    * @return Whether the SafariDriver should erase all session data before launching Safari.
    * @see #setUseCleanSession(boolean)
    */
   public boolean getUseCleanSession() {
-    return useCleanSession;
+    return (boolean) options.getOrDefault(Option.CLEAN_SESSION, false);
   }
 
   public boolean getUseTechnologyPreview() {
-    return useTechnologyPreview;
+    return (boolean) options.getOrDefault(Option.TECHNOLOGY_PREVIEW, false);
   }
 
   // (De)serialization of the options
 
-  /**
-   * Converts this instance to its JSON representation.
-   *
-   * @return The JSON representation of the options.
-   * @throws IOException If an error occurred while reading the Safari extension files.
-   */
-  public JsonObject toJson() throws IOException {
-    JsonObject options = new JsonObject();
-    options.addProperty(Option.PORT, port);
-    options.addProperty(Option.CLEAN_SESSION, useCleanSession);
-    options.addProperty(Option.TECHNOLOGY_PREVIEW, useTechnologyPreview);
-    return options;
-  }
-
   /**
    * Parse a Map and reconstruct the {@link SafariOptions}.
-   * A temporary directory is created to hold all Safari extension files.
    *
-   * @param options A Map derived from the output of {@link #toJson()}.
    * @return A {@link SafariOptions} instance associated with these extensions.
-   * @throws IOException If an error occurred while writing the safari extensions to a
-   *    temporary directory.
    */
   private static SafariOptions fromJsonMap(Map<?, ?> options) throws IOException {
     SafariOptions safariOptions = new SafariOptions();
@@ -215,26 +196,10 @@ private static SafariOptions fromJsonMap(Map<?, ?> options) throws IOException {
    * reflected in the returned capabilities.
    *
    * @return DesiredCapabilities for Safari with these extensions.
+   * @deprecated {@code SafariOptions} are already {@link MutableCapabilities}.
    */
-  DesiredCapabilities toCapabilities() {
-    DesiredCapabilities capabilities = DesiredCapabilities.safari();
-    capabilities.setCapability(CAPABILITY, this);
-    return capabilities;
-  }
-
-  @Override
-  public boolean equals(Object other) {
-    if (!(other instanceof SafariOptions)) {
-      return false;
-    }
-    SafariOptions that = (SafariOptions) other;
-    return this.port == that.port
-        && this.useCleanSession == that.useCleanSession
-        && this.useTechnologyPreview == that.useTechnologyPreview;
-  }
-
-  @Override
-  public int hashCode() {
-    return Objects.hashCode(this.port, this.useCleanSession, this.useTechnologyPreview);
+  @Deprecated
+  MutableCapabilities toCapabilities() {
+    return this;
   }
 }

java/client/test/org/openqa/selenium/safari/SafariDriverTests.java

@@ -32,6 +32,7 @@
     AlertsTest.class,
     CleanSessionTest.class,
     CrossDomainTest.class,
+    SafariOptionsTest.class,
     TechnologyPreviewTest.class,
 })
 public class SafariDriverTests {

java/client/test/org/openqa/selenium/safari/SafariOptionsTest.java

@@ -0,0 +1,36 @@
+// Licensed to the Software Freedom Conservancy (SFC) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The SFC licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.openqa.selenium.safari;
+
+import static org.junit.Assert.assertEquals;
+
+import org.junit.Test;
+import org.openqa.selenium.remote.DesiredCapabilities;
+
+public class SafariOptionsTest {
+
+  @Test
+  public void commonUsagePatternWorks() {
+    SafariOptions options = new SafariOptions().setPort(99);
+    DesiredCapabilities caps = new DesiredCapabilities();
+    caps.setCapability(SafariOptions.CAPABILITY, options);
+
+    assertEquals(options.asMap(), caps.asMap());
+  }
+
+}