First take at refactoring DriverOptions for .NET

This commit deprecates the AddAdditionalCapability method in the
driver-specific Options classes in favor of two methods. The first,
AddAdditionalOption, adds a capability to the top-level, global
section of a browser's desired capabilities section. The second
method adds a capability to a browser's specific set of options.
Accordingly, these methods are different for each browser's Options
class (AddAdditionalChromeOption for ChromeOptions,
AddAdditionalFirefoxOption for FirefoxOptions,
AddAdditionalInternetExplorerOption for InternetExplorerOptions, etc.).

Also, this commit completes the removal of the DesiredCapabilities
class by removing its visibility from the public API. All use cases
that previously required adding arbitrary capabilities to a
DesiredCapabilities instance should now be manageable by the browser-
specific options classes. Moreover, the ToCapabilities method of the
options classes now returns a read-only ICapabilities object. Users
who find these structures insufficient are encouraged to join the
project IRC or Slack channels to discuss where the deficiencies lie.
Likewise, downstream projects (like Appium) and cloud providers
(like SauceLabs, BrowserStack, etc.) that depend on the .NET language
bindings for functionality should be aware of this change, and should
take immediate steps to update their user-facing code and documentation
to match.

Author: jimevans

dotnet/src/webdriver/Chrome/ChromeOptions.cs

@@ -88,7 +88,6 @@ public class ChromeOptions : DriverOptions
         private List<string> encodedExtensions = new List<string>();
         private List<string> excludedSwitches = new List<string>();
         private List<string> windowTypes = new List<string>();
-        private Dictionary<string, object> additionalCapabilities = new Dictionary<string, object>();
         private Dictionary<string, object> additionalChromeOptions = new Dictionary<string, object>();
         private Dictionary<string, object> userProfilePreferences;
         private Dictionary<string, object> localStatePreferences;
@@ -492,6 +491,27 @@ public void AddWindowTypes(IEnumerable<string> windowTypesToAdd)
             this.windowTypes.AddRange(windowTypesToAdd);
         }
 
+        /// <summary>
+        /// Provides a means to add additional capabilities not yet added as type safe options
+        /// for the Chrome driver.
+        /// </summary>
+        /// <param name="optionName">The name of the capability to add.</param>
+        /// <param name="optionValue">The value of the capability to add.</param>
+        /// <exception cref="ArgumentException">
+        /// thrown when attempting to add a capability for which there is already a type safe option, or
+        /// when <paramref name="optionName"/> is <see langword="null"/> or the empty string.
+        /// </exception>
+        /// <remarks>Calling <see cref="AddAdditionalChromeOption(string, object)"/>
+        /// where <paramref name="optionName"/> has already been added will overwrite the
+        /// existing value with the new value in <paramref name="optionValue"/>.
+        /// Calling this method adds capabilities to the Chrome-specific options object passed to
+        /// chromedriver.exe (property name 'goog:chromeOptions').</remarks>
+        public void AddAdditionalChromeOption(string optionName, object optionValue)
+        {
+            this.ValidateCapabilityName(optionName);
+            this.additionalChromeOptions[optionName] = optionValue;
+        }
+
         /// <summary>
         /// Provides a means to add additional capabilities not yet added as type safe options
         /// for the Chrome driver.
@@ -507,6 +527,7 @@ public void AddWindowTypes(IEnumerable<string> windowTypesToAdd)
         /// existing value with the new value in <paramref name="capabilityValue"/>.
         /// Also, by default, calling this method adds capabilities to the options object passed to
         /// chromedriver.exe.</remarks>
+        [Obsolete("Use the temporary AddAdditionalOption method or the AddAdditionalChromeOption method for adding additional options")]
         public override void AddAdditionalCapability(string capabilityName, object capabilityValue)
         {
             // Add the capability to the chromeOptions object by default. This is to handle
@@ -530,35 +551,16 @@ public override void AddAdditionalCapability(string capabilityName, object capab
         /// <remarks>Calling <see cref="AddAdditionalCapability(string, object, bool)"/>
         /// where <paramref name="capabilityName"/> has already been added will overwrite the
         /// existing value with the new value in <paramref name="capabilityValue"/></remarks>
+        [Obsolete("Use the temporary AddAdditionalOption method or the AddAdditionalChromeOption method for adding additional options")]
         public void AddAdditionalCapability(string capabilityName, object capabilityValue, bool isGlobalCapability)
         {
-            if (this.IsKnownCapabilityName(capabilityName))
-            {
-                string typeSafeOptionName = this.GetTypeSafeOptionName(capabilityName);
-                string message = string.Format(CultureInfo.InvariantCulture, "There is already an option for the {0} capability. Please use the {1} instead.", capabilityName, typeSafeOptionName);
-
-                // TODO: Remove this if block when chromedriver bug 2371 is fixed
-                // (https://bugs.chromium.org/p/chromedriver/issues/detail?id=2371)
-                if (capabilityName == ForceAlwaysMatchCapabilityName)
-                {
-                    message = string.Format(CultureInfo.InvariantCulture, "The {0} capability is internal to the driver, and not intended to be set from users' code. Do not attempt to set this capability.", capabilityName);
-                }
-
-                throw new ArgumentException(message, "capabilityName");
-            }
-
-            if (string.IsNullOrEmpty(capabilityName))
-            {
-                throw new ArgumentException("Capability name may not be null an empty string.", "capabilityName");
-            }
-
             if (isGlobalCapability)
             {
-                this.additionalCapabilities[capabilityName] = capabilityValue;
+                this.AddAdditionalOption(capabilityName, capabilityValue);
             }
             else
             {
-                this.additionalChromeOptions[capabilityName] = capabilityValue;
+                this.AddAdditionalChromeOption(capabilityName, capabilityValue);
             }
         }
 
@@ -572,7 +574,7 @@ public override ICapabilities ToCapabilities()
         {
             Dictionary<string, object> chromeOptions = this.BuildChromeOptionsDictionary();
 
-            DesiredCapabilities capabilities = this.GenerateDesiredCapabilities(false);
+            IWritableCapabilities capabilities = this.GenerateDesiredCapabilities(false);
             capabilities.SetCapability(ChromeOptions.Capability, chromeOptions);
 
             Dictionary<string, object> loggingPreferences = this.GenerateLoggingPreferencesDictionary();
@@ -581,11 +583,6 @@ public override ICapabilities ToCapabilities()
                 capabilities.SetCapability(CapabilityType.LoggingPreferences, loggingPreferences);
             }
 
-            foreach (KeyValuePair<string, object> pair in this.additionalCapabilities)
-            {
-                capabilities.SetCapability(pair.Key, pair.Value);
-            }
-
             return capabilities.AsReadOnly();
         }
 

dotnet/src/webdriver/DriverOptions.cs

@@ -21,6 +21,7 @@
 using OpenQA.Selenium.Remote;
 using System;
 using System.Collections.Generic;
+using System.Globalization;
 
 namespace OpenQA.Selenium
 {
@@ -180,6 +181,26 @@ public Proxy Proxy
             set { this.proxy = value; }
         }
 
+        /// <summary>
+        /// Provides a means to add additional capabilities not yet added as type safe options
+        /// for the specific browser driver.
+        /// </summary>
+        /// <param name="optionName">The name of the capability to add.</param>
+        /// <param name="optionValue">The value of the capability to add.</param>
+        /// <exception cref="ArgumentException">
+        /// thrown when attempting to add a capability for which there is already a type safe option, or
+        /// when <paramref name="optionName"/> is <see langword="null"/> or the empty string.
+        /// </exception>
+        /// <remarks>Calling <see cref="AddAdditionalOption(string, object)"/>
+        /// where <paramref name="optionName"/> has already been added will overwrite the
+        /// existing value with the new value in <paramref name="optionValue"/>.
+        /// </remarks>
+        public virtual void AddAdditionalOption(string optionName, object optionValue)
+        {
+            this.ValidateCapabilityName(optionName);
+            this.additionalCapabilities[optionName] = optionValue;
+        }
+
         /// <summary>
         /// Provides a means to add additional capabilities not yet added as type safe options
         /// for the specific browser driver.
@@ -194,6 +215,7 @@ public Proxy Proxy
         /// where <paramref name="capabilityName"/> has already been added will overwrite the
         /// existing value with the new value in <paramref name="capabilityValue"/>.
         /// </remarks>
+        [Obsolete("Use the temporary AddAdditionalOption method or the browser-specific method for adding additional options")]
         public abstract void AddAdditionalCapability(string capabilityName, object capabilityValue);
 
         /// <summary>
@@ -288,6 +310,30 @@ internal Dictionary<string, object> ToDictionary()
             return desired.CapabilitiesDictionary;
         }
 
+        /// <summary>
+        /// Validates the name of the capability to verify it is not a capability
+        /// for which a type-safe property or method already exists.
+        /// </summary>
+        /// <param name="capabilityName">The name of the capability to validate.</param>
+        /// <exception cref="ArgumentException">
+        /// thrown when attempting to add a capability for which there is already a type safe option, or
+        /// when <paramref name="capabilityName"/> is <see langword="null"/> or the empty string.
+        /// </exception>
+        protected void ValidateCapabilityName(string capabilityName)
+        {
+            if (string.IsNullOrEmpty(capabilityName))
+            {
+                throw new ArgumentException("Capability name may not be null an empty string.", "capabilityName");
+            }
+
+            if (this.IsKnownCapabilityName(capabilityName))
+            {
+                string typeSafeOptionName = this.GetTypeSafeOptionName(capabilityName);
+                string message = string.Format(CultureInfo.InvariantCulture, "There is already an option for the {0} capability. Please use the {1} instead.", capabilityName, typeSafeOptionName);
+                throw new ArgumentException(message, "capabilityName");
+            }
+        }
+
         /// <summary>
         /// Adds a known capability to the list of known capabilities and associates it
         /// with the type-safe property name of the options class to be used instead.
@@ -348,8 +394,8 @@ protected Dictionary<string, object> GenerateLoggingPreferencesDictionary()
         /// Generates the current options as a capabilities object for further processing.
         /// </summary>
         /// <param name="isSpecificationCompliant">A value indicating whether to generate capabilities compliant with the W3C WebDriver Specification.</param>
-        /// <returns>A <see cref="DesiredCapabilities"/> object representing the current options for further processing.</returns>
-        protected DesiredCapabilities GenerateDesiredCapabilities(bool isSpecificationCompliant)
+        /// <returns>A <see cref="IWritableCapabilities"/> object representing the current options for further processing.</returns>
+        protected IWritableCapabilities GenerateDesiredCapabilities(bool isSpecificationCompliant)
         {
             DesiredCapabilities capabilities = new DesiredCapabilities();
             if (!string.IsNullOrEmpty(this.browserName))
@@ -428,6 +474,11 @@ protected DesiredCapabilities GenerateDesiredCapabilities(bool isSpecificationCo
                 }
             }
 
+            foreach (KeyValuePair<string, object> pair in this.additionalCapabilities)
+            {
+                capabilities.SetCapability(pair.Key, pair.Value);
+            }
+
             return capabilities;
         }
     }

dotnet/src/webdriver/Edge/EdgeOptions.cs

@@ -23,32 +23,6 @@
 
 namespace OpenQA.Selenium.Edge
 {
-    /// <summary>
-    /// Specifies the behavior of waiting for page loads in the Edge driver.
-    /// </summary>
-    public enum EdgePageLoadStrategy
-    {
-        /// <summary>
-        /// Indicates the behavior is not set.
-        /// </summary>
-        Default,
-
-        /// <summary>
-        /// Waits for pages to load and ready state to be 'complete'.
-        /// </summary>
-        Normal,
-
-        /// <summary>
-        /// Waits for pages to load and for ready state to be 'interactive' or 'complete'.
-        /// </summary>
-        Eager,
-
-        /// <summary>
-        /// Does not wait for pages to load, returning immediately.
-        /// </summary>
-        None
-    }
-
     /// <summary>
     /// Class to manage options specific to <see cref="EdgeDriver"/>
     /// </summary>
@@ -76,15 +50,16 @@ public class EdgeOptions : DriverOptions
         private const string ExtensionPathsCapability = "ms:extensionPaths";
         private const string StartPageCapability = "ms:startPage";
 
-        private EdgePageLoadStrategy pageLoadStrategy = EdgePageLoadStrategy.Default;
-        private Dictionary<string, object> additionalCapabilities = new Dictionary<string, object>();
         private bool useInPrivateBrowsing;
         private string startPage;
         private List<string> extensionPaths = new List<string>();
 
         public EdgeOptions() : base()
         {
             this.BrowserName = BrowserNameValue;
+            this.AddKnownCapabilityName(UseInPrivateBrowsingCapability, "UseInPrivateBrowsing property");
+            this.AddKnownCapabilityName(StartPageCapability, "StartPage property");
+            this.AddKnownCapabilityName(ExtensionPathsCapability, "AddExtensionPaths method");
         }
 
         /// <summary>
@@ -156,14 +131,10 @@ public void AddExtensionPaths(IEnumerable<string> extensionPathsToAdd)
         /// </exception>
         /// <remarks>Calling <see cref="AddAdditionalCapability"/> where <paramref name="capabilityName"/>
         /// has already been added will overwrite the existing value with the new value in <paramref name="capabilityValue"/></remarks>
+        [Obsolete("Use the temporary AddAdditionalOption method for adding additional options")]
         public override void AddAdditionalCapability(string capabilityName, object capabilityValue)
         {
-            if (string.IsNullOrEmpty(capabilityName))
-            {
-                throw new ArgumentException("Capability name may not be null an empty string.", "capabilityName");
-            }
-
-            this.additionalCapabilities[capabilityName] = capabilityValue;
+            this.AddAdditionalOption(capabilityName, capabilityValue);
         }
 
         /// <summary>
@@ -174,7 +145,7 @@ public override void AddAdditionalCapability(string capabilityName, object capab
         /// <returns>The DesiredCapabilities for Edge with these options.</returns>
         public override ICapabilities ToCapabilities()
         {
-            DesiredCapabilities capabilities = this.GenerateDesiredCapabilities(false);
+            IWritableCapabilities capabilities = this.GenerateDesiredCapabilities(true);
 
             if (this.useInPrivateBrowsing)
             {
@@ -191,13 +162,7 @@ public override ICapabilities ToCapabilities()
                 capabilities.SetCapability(ExtensionPathsCapability, this.extensionPaths);
             }
 
-            foreach (KeyValuePair<string, object> pair in this.additionalCapabilities)
-            {
-                capabilities.SetCapability(pair.Key, pair.Value);
-            }
-
-            // Should return capabilities.AsReadOnly(), and will in a future release.
-            return capabilities;
+            return capabilities.AsReadOnly();
         }
     }
 }