fixes for SeleniumHQ#9856 based on code review

titusfortner · Sep 23, 2021 · df133e9 · df133e9
1 parent d706012
commit df133e9
Show file tree

Hide file tree

Showing 13 changed files with 48 additions and 61 deletions.
diff --git a/java/src/org/openqa/selenium/chromium/AddHasCasting.java b/java/src/org/openqa/selenium/chromium/AddHasCasting.java
@@ -25,7 +25,7 @@
 import org.openqa.selenium.remote.CommandInfo;
 import org.openqa.selenium.remote.ExecuteMethod;
 
-import java.util.ArrayList;
+import java.util.List;
 import java.util.Map;
 import java.util.function.Predicate;
 
@@ -51,8 +51,8 @@ public Class<HasCasting> getDescribedInterface() {
   @Override
   public HasCasting getImplementation(Capabilities capabilities, ExecuteMethod executeMethod) {
     return new HasCasting() {
-      @Override public ArrayList<Map<String, String>> getCastSinks() {
-        return (ArrayList<Map<String, String>>) executeMethod.execute(GET_CAST_SINKS, null);
+      @Override public List<Map<String, String>> getCastSinks() {
+        return (List<Map<String, String>>) executeMethod.execute(GET_CAST_SINKS, null);
       }
 
       @Override public void selectCastSink(String deviceName) {

diff --git a/java/src/org/openqa/selenium/chromium/AddHasCdp.java b/java/src/org/openqa/selenium/chromium/AddHasCdp.java
@@ -25,25 +25,21 @@
 import org.openqa.selenium.remote.CommandInfo;
 import org.openqa.selenium.remote.ExecuteMethod;
 
-import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.Map;
 import java.util.function.Predicate;
 
-import static org.openqa.selenium.remote.BrowserType.CHROME;
-import static org.openqa.selenium.remote.BrowserType.EDGE;
+import static org.openqa.selenium.chromium.ChromiumDriver.KNOWN_CHROMIUM_BROWSERS;
 
 public abstract class AddHasCdp implements AugmenterProvider<HasCdp>, AdditionalHttpCommands {
 
-    public static final String EXECUTE_CDP = "executeCdpCommand";
+  public static final String EXECUTE_CDP = "executeCdpCommand";
 
   @Override
   public abstract Map<String, CommandInfo> getAdditionalCommands();
 
   @Override
   public Predicate<Capabilities> isApplicable() {
-    String[] validBrowsers = new String[] { EDGE, CHROME, "msedge" };
-    return caps -> new ArrayList<>(Arrays.asList(validBrowsers)).contains(caps.getBrowserName());
+    return caps -> KNOWN_CHROMIUM_BROWSERS.contains(caps.getBrowserName());
   }
 
   @Override

diff --git a/java/src/org/openqa/selenium/chromium/AddHasLaunchApp.java b/java/src/org/openqa/selenium/chromium/AddHasLaunchApp.java
@@ -27,18 +27,15 @@
 import org.openqa.selenium.remote.ExecuteMethod;
 import org.openqa.selenium.remote.http.HttpMethod;
 
-import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.Map;
 import java.util.function.Predicate;
 
-import static org.openqa.selenium.remote.BrowserType.CHROME;
-import static org.openqa.selenium.remote.BrowserType.EDGE;
+import static org.openqa.selenium.chromium.ChromiumDriver.KNOWN_CHROMIUM_BROWSERS;
 
 @AutoService({AdditionalHttpCommands.class, AugmenterProvider.class})
 public class AddHasLaunchApp implements AugmenterProvider<HasLaunchApp>, AdditionalHttpCommands {
 
-    public static final String LAUNCH_APP = "launchApp";
+  public static final String LAUNCH_APP = "launchApp";
 
   private static final Map<String, CommandInfo> COMMANDS = ImmutableMap.of(
     LAUNCH_APP, new CommandInfo("/session/:sessionId/chromium/launch_app", HttpMethod.POST));
@@ -50,8 +47,7 @@ public Map<String, CommandInfo> getAdditionalCommands() {
 
   @Override
   public Predicate<Capabilities> isApplicable() {
-    String[] validBrowsers = new String[] { EDGE, CHROME, "msedge" };
-    return caps -> new ArrayList<>(Arrays.asList(validBrowsers)).contains(caps.getBrowserName());
+    return caps -> KNOWN_CHROMIUM_BROWSERS.contains(caps.getBrowserName());
   }
 
   @Override

diff --git a/java/src/org/openqa/selenium/chromium/AddHasNetworkConditions.java b/java/src/org/openqa/selenium/chromium/AddHasNetworkConditions.java
@@ -28,13 +28,10 @@
 import org.openqa.selenium.remote.http.HttpMethod;
 
 import java.time.Duration;
-import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.Map;
 import java.util.function.Predicate;
 
-import static org.openqa.selenium.remote.BrowserType.CHROME;
-import static org.openqa.selenium.remote.BrowserType.EDGE;
+import static org.openqa.selenium.chromium.ChromiumDriver.KNOWN_CHROMIUM_BROWSERS;
 
 @AutoService({AdditionalHttpCommands.class, AugmenterProvider.class})
 public class AddHasNetworkConditions implements AugmenterProvider<HasNetworkConditions>, AdditionalHttpCommands {
@@ -56,8 +53,7 @@ public Map<String, CommandInfo> getAdditionalCommands() {
 
   @Override
   public Predicate<Capabilities> isApplicable() {
-    String[] validBrowsers = new String[] { EDGE, CHROME, "msedge" };
-    return caps -> new ArrayList<>(Arrays.asList(validBrowsers)).contains(caps.getBrowserName());
+    return caps -> KNOWN_CHROMIUM_BROWSERS.contains(caps.getBrowserName());
   }
 
   @Override
@@ -74,8 +70,8 @@ public ChromiumNetworkConditions getNetworkConditions() {
         ChromiumNetworkConditions networkConditions = new ChromiumNetworkConditions();
         networkConditions.setOffline((Boolean) result.getOrDefault(ChromiumNetworkConditions.OFFLINE, false));
         networkConditions.setLatency(Duration.ofMillis((Long) result.getOrDefault(ChromiumNetworkConditions.LATENCY, 0)));
-        networkConditions.setDownloadThroughput((Number) result.getOrDefault(ChromiumNetworkConditions.DOWNLOAD_THROUGHPUT, -1));
-        networkConditions.setDownloadThroughput((Number) result.getOrDefault(ChromiumNetworkConditions.UPLOAD_THROUGHPUT, -1));
+        networkConditions.setDownloadThroughput((int) result.getOrDefault(ChromiumNetworkConditions.DOWNLOAD_THROUGHPUT, -1));
+        networkConditions.setDownloadThroughput((int) result.getOrDefault(ChromiumNetworkConditions.UPLOAD_THROUGHPUT, -1));
         return networkConditions;
       }
 

diff --git a/java/src/org/openqa/selenium/chromium/AddHasPermissions.java b/java/src/org/openqa/selenium/chromium/AddHasPermissions.java
@@ -27,18 +27,15 @@
 import org.openqa.selenium.remote.ExecuteMethod;
 import org.openqa.selenium.remote.http.HttpMethod;
 
-import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.Map;
 import java.util.function.Predicate;
 
-import static org.openqa.selenium.remote.BrowserType.CHROME;
-import static org.openqa.selenium.remote.BrowserType.EDGE;
+import static org.openqa.selenium.chromium.ChromiumDriver.KNOWN_CHROMIUM_BROWSERS;
 
 @AutoService({AdditionalHttpCommands.class, AugmenterProvider.class})
 public class AddHasPermissions implements AugmenterProvider<HasPermissions>, AdditionalHttpCommands {
 
-    public static final String SET_PERMISSION = "setPermission";
+  public static final String SET_PERMISSION = "setPermission";
 
   private static final Map<String, CommandInfo> COMMANDS = ImmutableMap.of(
     SET_PERMISSION, new CommandInfo("/session/:sessionId/permissions", HttpMethod.POST));
@@ -50,8 +47,7 @@ public Map<String, CommandInfo> getAdditionalCommands() {
 
   @Override
   public Predicate<Capabilities> isApplicable() {
-    String[] validBrowsers = new String[] { EDGE, CHROME, "msedge" };
-    return caps -> new ArrayList<>(Arrays.asList(validBrowsers)).contains(caps.getBrowserName());
+    return caps -> KNOWN_CHROMIUM_BROWSERS.contains(caps.getBrowserName());
   }
 
   @Override

diff --git a/java/src/org/openqa/selenium/chromium/ChromiumDriver.java b/java/src/org/openqa/selenium/chromium/ChromiumDriver.java
@@ -17,6 +17,7 @@
 
 package org.openqa.selenium.chromium;
 
+import com.google.common.collect.ImmutableList;
 import org.openqa.selenium.BuildInfo;
 import org.openqa.selenium.Capabilities;
 import org.openqa.selenium.Credentials;
@@ -54,31 +55,35 @@
 import org.openqa.selenium.remote.mobile.RemoteNetworkConnection;
 
 import java.net.URI;
-import java.util.ArrayList;
+import java.util.List;
 import java.util.Map;
 import java.util.Optional;
 import java.util.function.Predicate;
 import java.util.function.Supplier;
 import java.util.logging.Logger;
 
+import static org.openqa.selenium.remote.BrowserType.CHROME;
+import static org.openqa.selenium.remote.BrowserType.EDGE;
+
 /**
  * A {@link WebDriver} implementation that controls a Chromium browser running on the local machine.
  * It is used as the base class for Chromium-based browser drivers (Chrome, Edgium).
  */
 public class ChromiumDriver extends RemoteWebDriver implements
   HasAuthentication,
+  HasCasting,
+  HasCdp,
   HasDevTools,
+  HasLaunchApp,
   HasLogEvents,
+  HasNetworkConditions,
+  HasPermissions,
   HasTouchScreen,
   LocationContext,
   NetworkConnection,
-  WebStorage,
-  HasNetworkConditions,
-  HasCasting,
-  HasCdp,
-  HasPermissions,
-  HasLaunchApp {
+  WebStorage {
 
+  public static final List KNOWN_CHROMIUM_BROWSERS = ImmutableList.of(EDGE, CHROME, "msedge");
   private static final Logger LOG = Logger.getLogger(ChromiumDriver.class.getName());
 
   private final Capabilities capabilities;
@@ -217,7 +222,7 @@ public DevTools getDevTools() {
   }
 
   @Override
-  public ArrayList<Map<String, String>> getCastSinks() {
+  public List<Map<String, String>> getCastSinks() {
     return casting.getCastSinks();
   }
 

diff --git a/java/src/org/openqa/selenium/chromium/ChromiumNetworkConditions.java b/java/src/org/openqa/selenium/chromium/ChromiumNetworkConditions.java
@@ -24,10 +24,10 @@
  *
  */
 public class ChromiumNetworkConditions {
-  private Boolean offline = false;
+  private boolean offline = false;
   private Duration latency = Duration.ZERO;
-  private Number downloadThroughput = -1;
-  private Number uploadThroughput = -1;
+  private int downloadThroughput = -1;
+  private int uploadThroughput = -1;
 
   public static final String OFFLINE = "offline";
   public static final String LATENCY = "latency";
@@ -38,7 +38,7 @@ public class ChromiumNetworkConditions {
    *
    * @return whether network is simulated to be offline.
    */
-  public Boolean getOffline() {
+  public boolean getOffline() {
     return offline;
   }
 
@@ -74,7 +74,7 @@ public void setLatency(Duration latency) {
    *
    * @return the current download throughput in kb/second.
    */
-  public Number getDownloadThroughput() {
+  public int getDownloadThroughput() {
     return downloadThroughput;
   }
 
@@ -83,7 +83,7 @@ public Number getDownloadThroughput() {
    *
    * @param downloadThroughput thoughput in kb/second
    */
-  public void setDownloadThroughput(Number downloadThroughput) {
+  public void setDownloadThroughput(int downloadThroughput) {
     this.downloadThroughput = downloadThroughput;
   }
 
@@ -92,7 +92,7 @@ public void setDownloadThroughput(Number downloadThroughput) {
    *
    * @return the current upload throughput in kb/second.
    */
-  public Number getUploadThroughput() {
+  public int getUploadThroughput() {
     return uploadThroughput;
   }
 
@@ -101,7 +101,7 @@ public Number getUploadThroughput() {
    *
    * @param uploadThroughput thoughput in kb/second
    */
-  public void setUploadThroughput(Number uploadThroughput) {
+  public void setUploadThroughput(int uploadThroughput) {
     this.uploadThroughput = uploadThroughput;
   }
 }
diff --git a/java/src/org/openqa/selenium/chromium/HasCasting.java b/java/src/org/openqa/selenium/chromium/HasCasting.java
@@ -19,7 +19,7 @@
 
 import org.openqa.selenium.Beta;
 
-import java.util.ArrayList;
+import java.util.List;
 import java.util.Map;
 
 /**
@@ -34,7 +34,7 @@ public interface HasCasting {
    * @return array of ID / Name pairs of available cast sink targets
    *
    */
-  ArrayList<Map<String, String>> getCastSinks();
+  List<Map<String, String>> getCastSinks();
 
   /**
    * Selects a cast sink (Cast device) as the recipient of media router intents (connect or play).

diff --git a/java/src/org/openqa/selenium/safari/AddHasPermissions.java b/java/src/org/openqa/selenium/safari/AddHasPermissions.java
@@ -29,8 +29,6 @@
 import java.util.Map;
 import java.util.function.Predicate;
 
-import static java.util.Collections.singletonMap;
-
 @AutoService({AdditionalHttpCommands.class, AugmenterProvider.class})
 public class AddHasPermissions implements AugmenterProvider<HasPermissions>, AdditionalHttpCommands {
 
@@ -60,7 +58,7 @@ public Class<HasPermissions> getDescribedInterface() {
   public HasPermissions getImplementation(Capabilities capabilities, ExecuteMethod executeMethod) {
     return new HasPermissions() {
       @Override
-      public void setPermissions(String permission, Boolean value) {
+      public void setPermissions(String permission, boolean value) {
         executeMethod.execute(SET_PERMISSIONS, ImmutableMap.of("permissions", ImmutableMap.of(permission, value)));
       }
 

diff --git a/java/src/org/openqa/selenium/safari/HasPermissions.java b/java/src/org/openqa/selenium/safari/HasPermissions.java
@@ -34,7 +34,7 @@ public interface HasPermissions {
    * @param permission the name of the item to set permission on.
    * @param value whether the permission has been granted.
    */
-  void setPermissions(String permission, Boolean value);
+  void setPermissions(String permission, boolean value);
 
   /**
    *

diff --git a/java/src/org/openqa/selenium/safari/SafariDriver.java b/java/src/org/openqa/selenium/safari/SafariDriver.java
@@ -89,7 +89,7 @@ public SafariDriver(SafariDriverService safariServer, SafariOptions safariOption
   }
 
   @Override
-  public void setPermissions(String permission, Boolean value) {
+  public void setPermissions(String permission, boolean value) {
     this.permissions.setPermissions(permission, value);
   }
 

diff --git a/java/test/org/openqa/selenium/chrome/ChromeDriverFunctionalTest.java b/java/test/org/openqa/selenium/chrome/ChromeDriverFunctionalTest.java
@@ -34,7 +34,7 @@
 import java.net.MalformedURLException;
 import java.net.URL;
 import java.time.Duration;
-import java.util.ArrayList;
+import java.util.List;
 import java.util.Map;
 
 import static org.assertj.core.api.Assertions.assertThat;
@@ -119,7 +119,7 @@ public void shouldCast() throws InterruptedException {
     // Does not get list the first time it is called
     driver.getCastSinks();
     Thread.sleep(1500);
-    ArrayList<Map<String, String>> castSinks = driver.getCastSinks();
+    List<Map<String, String>> castSinks = driver.getCastSinks();
 
     // Can not call these commands if there are no sinks available
     if (castSinks.size() > 0) {
@@ -139,7 +139,7 @@ public void shouldAllowRemoteWebDriverToAugmentHasCasting() throws InterruptedEx
       // Does not get list the first time it is called
       ((HasCasting) augmentedDriver).getCastSinks();
       Thread.sleep(1000);
-      ArrayList<Map<String, String>> castSinks = ((HasCasting) augmentedDriver).getCastSinks();
+      List<Map<String, String>> castSinks = ((HasCasting) augmentedDriver).getCastSinks();
 
       // Can not call these commands if there are no sinks available
       if (castSinks.size() > 0) {

diff --git a/java/test/org/openqa/selenium/edge/EdgeDriverFunctionalTest.java b/java/test/org/openqa/selenium/edge/EdgeDriverFunctionalTest.java
@@ -31,7 +31,7 @@
 import java.net.MalformedURLException;
 import java.net.URL;
 import java.time.Duration;
-import java.util.ArrayList;
+import java.util.List;
 import java.util.Map;
 
 import static org.assertj.core.api.Assertions.assertThat;
@@ -74,7 +74,7 @@ public void shouldCast() throws InterruptedException {
       // Does not get list the first time it is called
       driver.getCastSinks();
       Thread.sleep(1500);
-      ArrayList<Map<String, String>> castSinks = driver.getCastSinks();
+      List<Map<String, String>> castSinks = driver.getCastSinks();
 
       // Can not call these commands if there are no sinks available
       if (castSinks.size() > 0) {
@@ -97,7 +97,7 @@ public void shouldAllowRemoteWebDriverToAugmentHasCasting() throws InterruptedEx
       // Does not get list the first time it is called
       ((HasCasting) augmentedDriver).getCastSinks();
       Thread.sleep(1000);
-      ArrayList<Map<String, String>> castSinks = ((HasCasting) augmentedDriver).getCastSinks();
+      List<Map<String, String>> castSinks = ((HasCasting) augmentedDriver).getCastSinks();
 
       // Can not call these commands if there are no sinks available
       if (castSinks.size() > 0) {