[cdp] Use a flat namespace for selenium options

This means that rather than having `se:options = {cdp: uri}` we now just have `se:cdp = uri`. Other capabilities that were stored in `se:options` have also been extracted and placed into a flat namespace. This change makes it easier to extract the information we need (trivially: we remove one lookup in a dictionary), and means that we can now easily merge capabilities with these options set without needing to do any additional work. Previously, we'd need to be aware of `se:options` and do "deep merging", which is easy to overlook.
SeleniumHQ · Feb 23, 2021 · 9ea1953 · 9ea1953
1 parent 33fb89b
commit 9ea1953
Show file tree

Hide file tree

Showing 9 changed files with 18 additions and 66 deletions.
diff --git a/java/client/src/org/openqa/selenium/devtools/DevToolsProvider.java b/java/client/src/org/openqa/selenium/devtools/DevToolsProvider.java
@@ -23,7 +23,6 @@
 import org.openqa.selenium.remote.AugmenterProvider;
 import org.openqa.selenium.remote.ExecuteMethod;
 
-import java.util.Map;
 import java.util.Optional;
 import java.util.function.Predicate;
 
@@ -49,12 +48,11 @@ public HasDevTools getImplementation(Capabilities caps, ExecuteMethod executeMet
   }
 
   private String getCdpUrl(Capabilities caps) {
-    Object options = caps.getCapability("se:options");
-    if (!(options instanceof Map)) {
+    Object cdp = caps.getCapability("se:cdp");
+    if (!(cdp instanceof String)) {
       return null;
     }
 
-    Object cdp = ((Map<?, ?>) options).get("cdp");
-    return cdp == null ? null : String.valueOf(cdp);
+    return (String) cdp;
   }
 }
diff --git a/java/client/src/org/openqa/selenium/devtools/SeleniumCdpConnection.java b/java/client/src/org/openqa/selenium/devtools/SeleniumCdpConnection.java
@@ -26,7 +26,6 @@
 
 import java.net.URI;
 import java.net.URISyntaxException;
-import java.util.Map;
 import java.util.Optional;
 
 public class SeleniumCdpConnection extends Connection {
@@ -58,13 +57,8 @@ public static Optional<Connection> create(HttpClient.Factory clientFactory, Capa
   }
 
   public static Optional<URI> getCdpUri(Capabilities capabilities) {
-    Object options = capabilities.getCapability("se:options");
+    Object cdp = capabilities.getCapability("se:cdp");
 
-    if (!(options instanceof Map)) {
-      return Optional.empty();
-    }
-
-    Object cdp = ((Map<?, ?>) options).get("cdp");
     if (!(cdp instanceof String)) {
       return Optional.empty();
     }

diff --git a/java/client/test/org/openqa/selenium/remote/RemoteWebDriverBuilderTest.java b/java/client/test/org/openqa/selenium/remote/RemoteWebDriverBuilderTest.java
@@ -183,11 +183,11 @@ public void shouldAllowCapabilitiesToBeSetGlobally() {
 
     RemoteWebDriver.builder()
       .oneOf(new FirefoxOptions())
-      .setCapability("se:options", "cheese")
+      .setCapability("se:option", "cheese")
       .address("http://localhost:34576")
       .connectingWith(config -> req -> {
         listCapabilities(req).stream()
-          .map(caps -> "cheese".equals(caps.getCapability("se:options")))
+          .map(caps -> "cheese".equals(caps.getCapability("se:option")))
           .reduce(Boolean::logicalAnd)
           .ifPresent(seen::set);
         return CANNED_SESSION_RESPONSE;

diff --git a/java/server/src/org/openqa/selenium/grid/docker/DockerSessionFactory.java b/java/server/src/org/openqa/selenium/grid/docker/DockerSessionFactory.java
@@ -289,7 +289,7 @@ private Map<String, String> getVideoContainerEnvVars(Capabilities sessionRequest
 
   private TimeZone getTimeZone(Capabilities sessionRequestCapabilities) {
     Optional<Object> timeZone =
-      ofNullable(getCapability(sessionRequestCapabilities, "timeZone"));
+      ofNullable(sessionRequestCapabilities.getCapability("se:timeZone"));
     if (timeZone.isPresent()) {
       String tz =  timeZone.get().toString();
       if (Arrays.asList(TimeZone.getAvailableIDs()).contains(tz)) {
@@ -301,7 +301,7 @@ private TimeZone getTimeZone(Capabilities sessionRequestCapabilities) {
 
   private Dimension getScreenResolution(Capabilities sessionRequestCapabilities) {
     Optional<Object> screenResolution =
-      ofNullable(getCapability(sessionRequestCapabilities, "screenResolution"));
+      ofNullable(sessionRequestCapabilities.getCapability("se:screenResolution"));
     if (!screenResolution.isPresent()) {
       return null;
     }
@@ -325,20 +325,10 @@ private Dimension getScreenResolution(Capabilities sessionRequestCapabilities) {
 
   private boolean recordVideoForSession(Capabilities sessionRequestCapabilities) {
     Optional<Object> recordVideo =
-      ofNullable(getCapability(sessionRequestCapabilities, "recordVideo"));
+      ofNullable(sessionRequestCapabilities.getCapability("se:recordVideo"));
     return recordVideo.isPresent() && Boolean.parseBoolean(recordVideo.get().toString());
   }
 
-  private Object getCapability(Capabilities sessionRequestCapabilities, String capabilityName) {
-    Object rawSeleniumOptions = sessionRequestCapabilities.getCapability("se:options");
-    if (rawSeleniumOptions instanceof Map) {
-      @SuppressWarnings("unchecked")
-      Map<String, Object> seleniumOptions = (Map<String, Object>) rawSeleniumOptions;
-      return seleniumOptions.get(capabilityName);
-    }
-    return null;
-  }
-
   private void saveSessionCapabilities(Capabilities sessionRequestCapabilities, String path) {
     String capsToJson = new Json().toJson(sessionRequestCapabilities);
     try {

diff --git a/java/server/src/org/openqa/selenium/grid/node/config/DriverServiceSessionFactory.java b/java/server/src/org/openqa/selenium/grid/node/config/DriverServiceSessionFactory.java
@@ -17,7 +17,6 @@
 
 package org.openqa.selenium.grid.node.config;
 
-import com.google.common.collect.ImmutableMap;
 import org.openqa.selenium.Capabilities;
 import org.openqa.selenium.ImmutableCapabilities;
 import org.openqa.selenium.PersistentCapabilities;
@@ -42,8 +41,6 @@
 import org.openqa.selenium.remote.tracing.Status;
 import org.openqa.selenium.remote.tracing.Tracer;
 
-import java.io.IOException;
-import java.io.UncheckedIOException;
 import java.net.URI;
 import java.net.URL;
 import java.time.Instant;
@@ -173,14 +170,6 @@ public void stop() {
   }
 
   private Capabilities addCdpCapability(Capabilities caps, URI uri) {
-    Object raw = caps.getCapability("se:options");
-    if (!(raw instanceof Map)) {
-      return new PersistentCapabilities(caps).setCapability("se:options", ImmutableMap.of("cdp", uri));
-    }
-
-    //noinspection unchecked
-    Map<String, Object> current = new HashMap<>((Map<String, Object>) raw);
-    current.put("cdp", uri);
-    return new PersistentCapabilities(caps).setCapability("se:options", ImmutableMap.copyOf(current));
+    return new PersistentCapabilities(caps).setCapability("se:cdp", uri);
   }
 }
diff --git a/java/server/src/org/openqa/selenium/grid/node/k8s/OneShotNode.java b/java/server/src/org/openqa/selenium/grid/node/k8s/OneShotNode.java
@@ -63,11 +63,9 @@
 import java.net.URISyntaxException;
 import java.time.Duration;
 import java.time.Instant;
-import java.util.HashMap;
 import java.util.Map;
 import java.util.Optional;
 import java.util.ServiceLoader;
-import java.util.TreeMap;
 import java.util.UUID;
 import java.util.logging.Logger;
 import java.util.stream.StreamSupport;
@@ -243,16 +241,8 @@ private Field findClientField(Class<?> clazz) {
   private Capabilities rewriteCapabilities(RemoteWebDriver driver) {
     // Rewrite the se:options if necessary to add cdp url
     if (driverInfo.isSupportingCdp()) {
-      Object rawSeleniumOptions = driver.getCapabilities().getCapability("se:options");
-      if (rawSeleniumOptions == null || rawSeleniumOptions instanceof Map) {
-        @SuppressWarnings("unchecked") Map<String, Object> original = (Map<String, Object>) rawSeleniumOptions;
-        Map<String, Object> updated = new TreeMap<>(original == null ? new HashMap<>() : original);
-
-        String cdpPath = String.format("/session/%s/se/cdp", driver.getSessionId());
-        updated.put("cdp", rewrite(cdpPath));
-
-        return new PersistentCapabilities(driver.getCapabilities()).setCapability("se:options", updated);
-      }
+      String cdpPath = String.format("/session/%s/se/cdp", driver.getSessionId());
+      return new PersistentCapabilities(driver.getCapabilities()).setCapability("se:cdp", rewrite(cdpPath));
     }
 
     return ImmutableCapabilities.copyOf(driver.getCapabilities());

diff --git a/java/server/src/org/openqa/selenium/grid/node/local/LocalNode.java b/java/server/src/org/openqa/selenium/grid/node/local/LocalNode.java
@@ -81,7 +81,6 @@
 import java.util.Map;
 import java.util.Optional;
 import java.util.Set;
-import java.util.TreeMap;
 import java.util.UUID;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.atomic.AtomicInteger;
@@ -447,16 +446,8 @@ private Session createExternalSession(ActiveSession other, URI externalUri, bool
 
     // Rewrite the se:options if necessary to send the cdp url back
     if (isSupportingCdp) {
-      Object rawSeleniumOptions = other.getCapabilities().getCapability("se:options");
-      if (rawSeleniumOptions instanceof Map) {
-        @SuppressWarnings("unchecked") Map<String, Object> original = (Map<String, Object>) rawSeleniumOptions;
-        Map<String, Object> updated = new TreeMap<>(original);
-
-        String cdpPath = String.format("/session/%s/se/cdp", other.getId());
-        updated.put("cdp", rewrite(cdpPath));
-
-        toUse = new PersistentCapabilities(toUse).setCapability("se:options", updated);
-      }
+      String cdpPath = String.format("/session/%s/se/cdp", other.getId());
+      toUse = new PersistentCapabilities(toUse).setCapability("se:cdp", rewrite(cdpPath));
     }
 
     return new Session(other.getId(), externalUri, other.getStereotype(), toUse, Instant.now());

diff --git a/javascript/node/selenium-webdriver/lib/webdriver.js b/javascript/node/selenium-webdriver/lib/webdriver.js
@@ -1159,12 +1159,12 @@ class WebDriver {
    */
   async createCDPConnection(target) {
     const caps = await this.getCapabilities()
-    const seOptions = caps['map_'].get('se:options') || new Map()
+    const seCdp = caps['map_'].get('se:cdp')
     const vendorInfo =
       caps['map_'].get(this.VENDOR_COMMAND_PREFIX + ':chromeOptions') ||
       caps['map_'].get('moz:debuggerAddress') ||
       new Map()
-    const debuggerUrl = seOptions['cdp'] || vendorInfo['debuggerAddress'] || vendorInfo
+    const debuggerUrl = seCdp || vendorInfo['debuggerAddress'] || vendorInfo
     this._wsUrl = await this.getWsUrl(debuggerUrl, target)
 
     return new Promise((resolve, reject) => {

diff --git a/py/selenium/webdriver/remote/log.py b/py/selenium/webdriver/remote/log.py
@@ -165,8 +165,8 @@ async def _get_bidi_connection(self):
         global cdp
         import_cdp()
         ws_url = None
-        if self.driver.caps.get("se:options"):
-            version, ws_url = self.driver.caps.get("se:options").get("cdp")
+        if self.driver.caps.get("se:cdp"):
+            version, ws_url = self.driver.caps.get("se:cdp")
         else:
             version, ws_url = self._get_cdp_details()