Refactoring constructor that accepts required capabilities too. But it should be reworked later to fit W3C standard

Author: barancev

java/client/src/org/openqa/selenium/firefox/FirefoxDriver.java

@@ -25,7 +25,6 @@
 import static org.openqa.selenium.remote.CapabilityType.VERSION;
 
 import com.google.common.base.Preconditions;
-import com.google.common.base.Predicate;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Maps;
 import com.google.common.collect.Sets;
@@ -62,8 +61,7 @@
 import java.util.concurrent.TimeUnit;
 
 /**
- * An implementation of the {#link WebDriver} interface that drives Firefox. This works through a
- * firefox extension, which gets installed automatically if necessary.
+ * An implementation of the {#link WebDriver} interface that drives Firefox.
  */
 public class FirefoxDriver extends RemoteWebDriver implements Killable {
 
@@ -120,7 +118,7 @@ public static final class SystemProperty {
 
   // TODO: make it public as soon as it's fully implemented
   FirefoxDriver(FirefoxOptions options) {
-    super(toExecutor(options), options.toDesiredCapabilities(), null);
+    this(toExecutor(options), options.toDesiredCapabilities(), options.toRequiredCapabilities());
     //binary = options.getBinary();
   }
 
@@ -182,14 +180,21 @@ private static FirefoxOptions getFirefoxOptions(Capabilities capabilities) {
   }
 
   public FirefoxDriver(Capabilities desiredCapabilities, Capabilities requiredCapabilities) {
-    this(getBinary(desiredCapabilities), null, desiredCapabilities, requiredCapabilities);
+    this(getFirefoxOptions(desiredCapabilities).addDesiredCapabilities(desiredCapabilities)
+             .addRequiredCapabilities(requiredCapabilities));
   }
 
   public FirefoxDriver(FirefoxBinary binary, FirefoxProfile profile, Capabilities capabilities) {
     this(getFirefoxOptions(capabilities).setBinary(binary).setProfile(profile)
              .addDesiredCapabilities(capabilities));
   }
 
+  public FirefoxDriver(FirefoxBinary binary, FirefoxProfile profile,
+                       Capabilities desiredCapabilities, Capabilities requiredCapabilities) {
+    this(getFirefoxOptions(desiredCapabilities).setBinary(binary).setProfile(profile)
+             .addDesiredCapabilities(desiredCapabilities).addRequiredCapabilities(requiredCapabilities));
+  }
+
   private static FirefoxProfile prepareProfile(FirefoxProfile profile,
                                                Capabilities desiredCapabilities,
                                                Capabilities requiredCapabilities) {
@@ -297,14 +302,6 @@ private static FirefoxBinary getBinary(Capabilities capabilities) {
     return new FirefoxBinary();
   }
 
-  public FirefoxDriver(FirefoxBinary binary, FirefoxProfile profile,
-      Capabilities desiredCapabilities, Capabilities requiredCapabilities) {
-    this(createCommandExecutor(desiredCapabilities, requiredCapabilities, binary, profile),
-         addProfileTo(desiredCapabilities, prepareProfile(profile, desiredCapabilities, requiredCapabilities)),
-         requiredCapabilities);
-    this.binary = binary;
-  }
-
   public FirefoxDriver(GeckoDriverService driverService) {
     this(new DriverCommandExecutor(driverService), null, null);
   }
@@ -325,23 +322,6 @@ private FirefoxDriver(CommandExecutor executor, Capabilities desiredCapabilities
           dropCapabilities(requiredCapabilities));
   }
 
-  private static CommandExecutor createCommandExecutor(Capabilities desiredCapabilities,
-                                                       Capabilities requiredCapabilities,
-                                                       FirefoxBinary binary,
-                                                       FirefoxProfile profile) {
-    if (isLegacy(desiredCapabilities)) {
-      if (profile == null) {
-        profile = extractProfile(desiredCapabilities, requiredCapabilities);
-      }
-      return new LazyCommandExecutor(binary, profile);
-    }
-    GeckoDriverService.Builder builder = new GeckoDriverService.Builder().usingPort(0);
-    if (binary != null) {
-      builder.usingFirefoxBinary(binary);
-    }
-    return new DriverCommandExecutor(builder.build());
-  }
-
   @Override
   public void setFileDetector(FileDetector detector) {
     throw new WebDriverException(
@@ -470,11 +450,8 @@ private static Capabilities dropCapabilities(Capabilities capabilities) {
 
     if (isLegacy(capabilities)) {
       final Set<String> toRemove = Sets.newHashSet(BINARY, PROFILE);
-      caps = new DesiredCapabilities(Maps.filterKeys(capabilities.asMap(), new Predicate<String>() {
-        public boolean apply(String key) {
-          return !toRemove.contains(key);
-        }
-      }));
+      caps = new DesiredCapabilities(
+          Maps.filterKeys(capabilities.asMap(), key -> !toRemove.contains(key)));
     } else {
       caps = new DesiredCapabilities(capabilities);
     }

java/client/src/org/openqa/selenium/firefox/FirefoxOptions.java

@@ -29,7 +29,6 @@
 
 import org.openqa.selenium.Capabilities;
 import org.openqa.selenium.WebDriverException;
-import org.openqa.selenium.firefox.internal.ProfilesIni;
 import org.openqa.selenium.remote.DesiredCapabilities;
 
 import java.io.IOException;
@@ -68,6 +67,7 @@ public class FirefoxOptions {
   private Level logLevel = null;
   private Boolean legacy;
   private DesiredCapabilities desiredCapabilities = new DesiredCapabilities();
+  private DesiredCapabilities requiredCapabilities = new DesiredCapabilities();
 
   /** INTERNAL ONLY: DO NOT USE */
   static FirefoxOptions fromJsonMap(Map<String, Object> map) throws IOException {
@@ -172,12 +172,21 @@ public FirefoxOptions setProfile(FirefoxProfile profile) {
   }
 
   public FirefoxProfile getProfile() {
-    final FirefoxProfile toReturn = Optional.ofNullable(profile).orElse(
-        Optional.ofNullable(extractProfile()).orElse(new FirefoxProfile()));
+    FirefoxProfile toReturn = profile;
+    if (toReturn == null) {
+      toReturn = extractProfile(desiredCapabilities);
+    }
+    if (toReturn == null) {
+      toReturn = extractProfile(requiredCapabilities);
+    }
+    if (toReturn == null) {
+      toReturn = new FirefoxProfile();
+    }
 
-    booleanPrefs.entrySet().forEach(pref -> toReturn.setPreference(pref.getKey(), pref.getValue()));
-    intPrefs.entrySet().forEach(pref -> toReturn.setPreference(pref.getKey(), pref.getValue()));
-    stringPrefs.entrySet().forEach(pref -> toReturn.setPreference(pref.getKey(), pref.getValue()));
+    FirefoxProfile prefHolder = toReturn;
+    booleanPrefs.entrySet().forEach(pref -> prefHolder.setPreference(pref.getKey(), pref.getValue()));
+    intPrefs.entrySet().forEach(pref -> prefHolder.setPreference(pref.getKey(), pref.getValue()));
+    stringPrefs.entrySet().forEach(pref -> prefHolder.setPreference(pref.getKey(), pref.getValue()));
 
     return toReturn;
   }
@@ -223,7 +232,7 @@ public FirefoxOptions setLogLevel(Level logLevel) {
   public FirefoxOptions addDesiredCapabilities(Capabilities desiredCapabilities) {
     this.desiredCapabilities.merge(desiredCapabilities);
 
-    FirefoxProfile suggestedProfile = extractProfile();
+    FirefoxProfile suggestedProfile = extractProfile(desiredCapabilities);
     if (suggestedProfile !=  null) {
       if (!booleanPrefs.isEmpty() || !intPrefs.isEmpty() || !stringPrefs.isEmpty()) {
         throw new IllegalStateException(
@@ -241,11 +250,32 @@ public FirefoxOptions addDesiredCapabilities(Capabilities desiredCapabilities) {
     return this;
   }
 
-  private FirefoxProfile extractProfile() {
-    if (desiredCapabilities == null) {
+  public FirefoxOptions addRequiredCapabilities(Capabilities requiredCapabilities) {
+    this.requiredCapabilities.merge(requiredCapabilities);
+
+    FirefoxProfile suggestedProfile = extractProfile(desiredCapabilities);
+    if (suggestedProfile !=  null) {
+      if (!booleanPrefs.isEmpty() || !intPrefs.isEmpty() || !stringPrefs.isEmpty()) {
+        throw new IllegalStateException(
+            "Unable to determine if preferences set on this option " +
+            "are the same as the profile in the capabilities");
+      }
+      if (profile != null && !suggestedProfile.equals(profile)) {
+        throw new IllegalStateException(
+            "Profile has been set on both the capabilities and these options, but they're " +
+            "different. Unable to determine which one you want to use.");
+      }
+      profile = suggestedProfile;
+    }
+
+    return this;
+  }
+
+  private FirefoxProfile extractProfile(Capabilities capabilities) {
+    if (capabilities == null) {
       return null;
     }
-    Object raw = desiredCapabilities.getCapability(PROFILE);
+    Object raw = capabilities.getCapability(PROFILE);
     if (raw == null) {
       return null;
 
@@ -304,6 +334,10 @@ public Capabilities toDesiredCapabilities() {
     return capabilities;
   }
 
+  public Capabilities toRequiredCapabilities() {
+    return requiredCapabilities;
+  }
+
   public DesiredCapabilities addTo(DesiredCapabilities capabilities) {
     capabilities.merge(toDesiredCapabilities());
     return capabilities;