[dotnet] Annotate nullability for DriverService and chromium/safari services (#15101)

* [dotnet] Annotate nullability for `DriverService` and some derived types

* Default values to `null` instead of sentinel values.

Author: RenderMichael

dotnet/src/webdriver/Chromium/ChromiumDriverService.cs

@@ -21,6 +21,8 @@
 using System.Globalization;
 using System.Text;
 
+#nullable enable
+
 namespace OpenQA.Selenium.Chromium
 {
     /// <summary>
@@ -30,15 +32,6 @@ public abstract class ChromiumDriverService : DriverService
     {
         private const string DefaultChromeDriverServiceExecutableName = "chromedriver";
 
-        private string logPath = string.Empty;
-        private string urlPathPrefix = string.Empty;
-        private string portServerAddress = string.Empty;
-        private string allowedIPAddresses = string.Empty;
-        private int adbPort = -1;
-        private bool disableBuildCheck;
-        private bool enableVerboseLogging;
-        private bool enableAppendLog;
-
         /// <summary>
         /// Initializes a new instance of the <see cref="ChromiumDriverService"/> class.
         /// </summary>
@@ -51,94 +44,64 @@ protected ChromiumDriverService(string executablePath, string executableFileName
         }
 
         /// <summary>
-        /// Gets or sets the location of the log file written to by the ChromeDriver executable.
+        /// <para>Gets or sets the location of the log file written to by the ChromeDriver executable.</para>
+        /// <para><see langword="null"/> or <see cref="string.Empty"/> signify no log path.</para>
         /// </summary>
-        public string LogPath
-        {
-            get { return this.logPath; }
-            set { this.logPath = value; }
-        }
+        public string? LogPath { get; set; }
 
         /// <summary>
-        /// Gets or sets the base URL path prefix for commands (e.g., "wd/url").
+        /// <para>Gets or sets the base URL path prefix for commands (e.g., "wd/url").</para>
+        /// <para><see langword="null"/> or <see cref="string.Empty"/> signify no prefix.</para>
         /// </summary>
-        public string UrlPathPrefix
-        {
-            get { return this.urlPathPrefix; }
-            set { this.urlPathPrefix = value; }
-        }
+        public string? UrlPathPrefix { get; set; }
 
         /// <summary>
-        /// Gets or sets the address of a server to contact for reserving a port.
+        /// <para>Gets or sets the address of a server to contact for reserving a port.</para>
+        /// <para><see langword="null"/> or <see cref="string.Empty"/> signify no port server.</para>
         /// </summary>
-        public string PortServerAddress
-        {
-            get { return this.portServerAddress; }
-            set { this.portServerAddress = value; }
-        }
+        public string? PortServerAddress { get; set; }
 
         /// <summary>
-        /// Gets or sets the port on which the Android Debug Bridge is listening for commands.
+        /// <para>Gets or sets the port on which the Android Debug Bridge is listening for commands.</para>
+        /// <para>A value less than or equal to 0, or <see langword="null"/>, indicates no Android Debug Bridge specified.</para>
         /// </summary>
-        public int AndroidDebugBridgePort
-        {
-            get { return this.adbPort; }
-            set { this.adbPort = value; }
-        }
+        public int? AndroidDebugBridgePort { get; set; }
 
         /// <summary>
         /// Gets or sets a value indicating whether to skip version compatibility check
         /// between the driver and the browser.
         /// Defaults to <see langword="false"/>.
         /// </summary>
-        public bool DisableBuildCheck
-        {
-            get { return this.disableBuildCheck; }
-            set { this.disableBuildCheck = value; }
-        }
+        public bool DisableBuildCheck { get; set; }
 
         /// <summary>
         /// Gets or sets a value indicating whether to enable verbose logging for the ChromeDriver executable.
         /// Defaults to <see langword="false"/>.
         /// </summary>
-        public bool EnableVerboseLogging
-        {
-            get { return this.enableVerboseLogging; }
-            set { this.enableVerboseLogging = value; }
-        }
+        public bool EnableVerboseLogging { get; set; }
 
         /// <summary>
         /// Gets or sets a value indicating whether to enable appending to an existing ChromeDriver log file.
         /// Defaults to <see langword="false"/>.
         /// </summary>
-        public bool EnableAppendLog
-        {
-            get { return this.enableAppendLog; }
-            set { this.enableAppendLog = value; }
-        }
+        public bool EnableAppendLog { get; set; }
 
         /// <summary>
-        /// Gets or sets the comma-delimited list of IP addresses that are approved to
-        /// connect to this instance of the Chrome driver. Defaults to an empty string,
-        /// which means only the local loopback address can connect.
+        /// <para>Gets or sets the comma-delimited list of IP addresses that are approved to connect to this instance of the Chrome driver.</para>
+        /// <para>A value of <see langword="null"/> or <see cref="string.Empty"/> means only the local loopback address can connect.</para>
         /// </summary>
         [Obsolete($"Use {nameof(AllowedIPAddresses)}")]
-        public string WhitelistedIPAddresses
+        public string? WhitelistedIPAddresses
         {
-            get { return this.allowedIPAddresses; }
-            set { this.allowedIPAddresses = value; }
+            get => this.AllowedIPAddresses;
+            set => this.AllowedIPAddresses = value;
         }
 
         /// <summary>
-        /// Gets or sets the comma-delimited list of IP addresses that are approved to
-        /// connect to this instance of the Chrome driver. Defaults to an empty string,
-        /// which means only the local loopback address can connect.
+        /// <para>Gets or sets the comma-delimited list of IP addresses that are approved to connect to this instance of the Chrome driver.</para>
+        /// <para>A value of <see langword="null"/> or <see cref="string.Empty"/> means only the local loopback address can connect.</para>
         /// </summary>
-        public string AllowedIPAddresses
-        {
-            get => this.allowedIPAddresses;
-            set => this.allowedIPAddresses = value;
-        }
+        public string? AllowedIPAddresses { get; set; }
 
         /// <summary>
         /// Gets the command-line arguments for the driver service.
@@ -148,49 +111,49 @@ protected override string CommandLineArguments
             get
             {
                 StringBuilder argsBuilder = new StringBuilder(base.CommandLineArguments);
-                if (this.adbPort > 0)
+                if (this.AndroidDebugBridgePort is int adb && adb > 0)
                 {
-                    argsBuilder.AppendFormat(CultureInfo.InvariantCulture, " --adb-port={0}", this.adbPort);
+                    argsBuilder.AppendFormat(CultureInfo.InvariantCulture, " --adb-port={0}", adb);
                 }
 
                 if (this.SuppressInitialDiagnosticInformation)
                 {
                     argsBuilder.Append(" --silent");
                 }
 
-                if (this.disableBuildCheck)
+                if (this.DisableBuildCheck)
                 {
                     argsBuilder.Append(" --disable-build-check");
                 }
 
-                if (this.enableVerboseLogging)
+                if (this.EnableVerboseLogging)
                 {
                     argsBuilder.Append(" --verbose");
                 }
 
-                if (this.enableAppendLog)
+                if (this.EnableAppendLog)
                 {
                     argsBuilder.Append(" --append-log");
                 }
 
-                if (!string.IsNullOrEmpty(this.logPath))
+                if (!string.IsNullOrEmpty(this.LogPath))
                 {
-                    argsBuilder.AppendFormat(CultureInfo.InvariantCulture, " --log-path=\"{0}\"", this.logPath);
+                    argsBuilder.AppendFormat(CultureInfo.InvariantCulture, " --log-path=\"{0}\"", this.LogPath);
                 }
 
-                if (!string.IsNullOrEmpty(this.urlPathPrefix))
+                if (!string.IsNullOrEmpty(this.UrlPathPrefix))
                 {
-                    argsBuilder.AppendFormat(CultureInfo.InvariantCulture, " --url-base={0}", this.urlPathPrefix);
+                    argsBuilder.AppendFormat(CultureInfo.InvariantCulture, " --url-base={0}", this.UrlPathPrefix);
                 }
 
-                if (!string.IsNullOrEmpty(this.portServerAddress))
+                if (!string.IsNullOrEmpty(this.PortServerAddress))
                 {
-                    argsBuilder.AppendFormat(CultureInfo.InvariantCulture, " --port-server={0}", this.portServerAddress);
+                    argsBuilder.AppendFormat(CultureInfo.InvariantCulture, " --port-server={0}", this.PortServerAddress);
                 }
 
-                if (!string.IsNullOrEmpty(this.allowedIPAddresses))
+                if (!string.IsNullOrEmpty(this.AllowedIPAddresses))
                 {
-                    argsBuilder.Append(string.Format(CultureInfo.InvariantCulture, " -allowed-ips={0}", this.allowedIPAddresses));
+                    argsBuilder.Append(string.Format(CultureInfo.InvariantCulture, " -allowed-ips={0}", this.AllowedIPAddresses));
                 }
 
                 return argsBuilder.ToString();

dotnet/src/webdriver/DriverService.cs

@@ -20,28 +20,24 @@
 using OpenQA.Selenium.Remote;
 using System;
 using System.Diagnostics;
+using System.Diagnostics.CodeAnalysis;
 using System.Globalization;
 using System.IO;
 using System.Net;
 using System.Net.Http;
 using System.Threading.Tasks;
 
+#nullable enable
+
 namespace OpenQA.Selenium
 {
     /// <summary>
     /// Exposes the service provided by a native WebDriver server executable.
     /// </summary>
     public abstract class DriverService : ICommandServer
     {
-        private string driverServicePath;
-        private string driverServiceExecutableName;
-        private string driverServiceHostName = "localhost";
-        private int driverServicePort;
-        private bool silent;
-        private bool hideCommandPromptWindow;
         private bool isDisposed;
-        private Process driverServiceProcess;
-        private TimeSpan initializationTimeout = TimeSpan.FromSeconds(20);
+        private Process? driverServiceProcess;
 
         /// <summary>
         /// Initializes a new instance of the <see cref="DriverService"/> class.
@@ -55,29 +51,33 @@ public abstract class DriverService : ICommandServer
         /// <exception cref="DriverServiceNotFoundException">
         /// If the specified driver service executable does not exist in the specified directory.
         /// </exception>
-        protected DriverService(string servicePath, int port, string driverServiceExecutableName)
+        protected DriverService(string? servicePath, int port, string? driverServiceExecutableName)
         {
-            this.driverServicePath = servicePath;
-            this.driverServiceExecutableName = driverServiceExecutableName;
-            this.driverServicePort = port;
+            this.DriverServicePath = servicePath;
+            this.DriverServiceExecutableName = driverServiceExecutableName;
+            this.Port = port;
         }
 
         /// <summary>
         /// Occurs when the driver process is starting.
         /// </summary>
-        public event EventHandler<DriverProcessStartingEventArgs> DriverProcessStarting;
+        public event EventHandler<DriverProcessStartingEventArgs>? DriverProcessStarting;
 
         /// <summary>
         /// Occurs when the driver process has completely started.
         /// </summary>
-        public event EventHandler<DriverProcessStartedEventArgs> DriverProcessStarted;
+        public event EventHandler<DriverProcessStartedEventArgs>? DriverProcessStarted;
 
         /// <summary>
         /// Gets the Uri of the service.
         /// </summary>
         public Uri ServiceUrl
         {
-            get { return new Uri(string.Format(CultureInfo.InvariantCulture, "http://{0}:{1}", this.driverServiceHostName, this.driverServicePort)); }
+            get
+            {
+                string url = string.Format(CultureInfo.InvariantCulture, "http://{0}:{1}", this.HostName, this.Port);
+                return new Uri(url);
+            }
         }
 
         /// <summary>
@@ -88,48 +88,30 @@ public Uri ServiceUrl
         /// (non-local) machines. This property can be used as a workaround so
         /// that an IP address (like "127.0.0.1" or "::1") can be used instead.
         /// </remarks>
-        public string HostName
-        {
-            get { return this.driverServiceHostName; }
-            set { this.driverServiceHostName = value; }
-        }
+        public string HostName { get; set; } = "localhost";
 
         /// <summary>
         /// Gets or sets the port of the service.
         /// </summary>
-        public int Port
-        {
-            get { return this.driverServicePort; }
-            set { this.driverServicePort = value; }
-        }
+        public int Port { get; set; }
 
         /// <summary>
         /// Gets or sets a value indicating whether the initial diagnostic information is suppressed
         /// when starting the driver server executable. Defaults to <see langword="false"/>, meaning
         /// diagnostic information should be shown by the driver server executable.
         /// </summary>
-        public bool SuppressInitialDiagnosticInformation
-        {
-            get { return this.silent; }
-            set { this.silent = value; }
-        }
+        public bool SuppressInitialDiagnosticInformation { get; set; }
 
         /// <summary>
         /// Gets a value indicating whether the service is running.
         /// </summary>
-        public bool IsRunning
-        {
-            get { return this.driverServiceProcess != null && !this.driverServiceProcess.HasExited; }
-        }
+        [MemberNotNullWhen(true, nameof(driverServiceProcess))]
+        public bool IsRunning => this.driverServiceProcess != null && !this.driverServiceProcess.HasExited;
 
         /// <summary>
         /// Gets or sets a value indicating whether the command prompt window of the service should be hidden.
         /// </summary>
-        public bool HideCommandPromptWindow
-        {
-            get { return this.hideCommandPromptWindow; }
-            set { this.hideCommandPromptWindow = value; }
-        }
+        public bool HideCommandPromptWindow { get; set; }
 
         /// <summary>
         /// Gets the process ID of the running driver service executable. Returns 0 if the process is not running.
@@ -159,54 +141,33 @@ public int ProcessId
         /// <summary>
         /// Gets or sets a value indicating the time to wait for an initial connection before timing out.
         /// </summary>
-        public TimeSpan InitializationTimeout
-        {
-            get { return this.initializationTimeout; }
-            set { this.initializationTimeout = value; }
-        }
+        public TimeSpan InitializationTimeout { get; set; } = TimeSpan.FromSeconds(20);
 
         /// <summary>
         /// Gets or sets the executable file name of the driver service.
         /// </summary>
-        public string DriverServiceExecutableName
-        {
-            get { return this.driverServiceExecutableName; }
-            set { this.driverServiceExecutableName = value; }
-        }
+        public string? DriverServiceExecutableName { get; set; }
 
         /// <summary>
         /// Gets or sets the path of the driver service.
         /// </summary>
-        public string DriverServicePath
-        {
-            get { return this.driverServicePath; }
-            set { this.driverServicePath = value; }
-        }
+        public string? DriverServicePath { get; set; }
 
         /// <summary>
         /// Gets the command-line arguments for the driver service.
         /// </summary>
-        protected virtual string CommandLineArguments
-        {
-            get { return string.Format(CultureInfo.InvariantCulture, "--port={0}", this.driverServicePort); }
-        }
+        protected virtual string CommandLineArguments => string.Format(CultureInfo.InvariantCulture, "--port={0}", this.Port);
 
         /// <summary>
         /// Gets a value indicating the time to wait for the service to terminate before forcing it to terminate.
         /// </summary>
-        protected virtual TimeSpan TerminationTimeout
-        {
-            get { return TimeSpan.FromSeconds(10); }
-        }
+        protected virtual TimeSpan TerminationTimeout => TimeSpan.FromSeconds(10);
 
         /// <summary>
         /// Gets a value indicating whether the service has a shutdown API that can be called to terminate
         /// it gracefully before forcing a termination.
         /// </summary>
-        protected virtual bool HasShutdown
-        {
-            get { return true; }
-        }
+        protected virtual bool HasShutdown => true;
 
         /// <summary>
         /// Gets a value indicating whether the service is responding to HTTP requests.
@@ -231,7 +192,7 @@ protected virtual bool IsInitialized
                             // that the HTTP status returned is a 200 status, and that the resposne has the correct
                             // Content-Type header. A more sophisticated check would parse the JSON response and
                             // validate its values. At the moment we do not do this more sophisticated check.
-                            isInitialized = response.StatusCode == HttpStatusCode.OK && response.Content.Headers.ContentType.MediaType.StartsWith("application/json", StringComparison.OrdinalIgnoreCase);
+                            isInitialized = response.StatusCode == HttpStatusCode.OK && response.Content.Headers.ContentType is { MediaType: string mediaType } && mediaType.StartsWith("application/json", StringComparison.OrdinalIgnoreCase);
                         }
                     }
                 }
@@ -256,6 +217,7 @@ public void Dispose()
         /// <summary>
         /// Starts the DriverService if it is not already running.
         /// </summary>
+        [MemberNotNull(nameof(driverServiceProcess))]
         public void Start()
         {
             if (this.driverServiceProcess != null)
@@ -265,9 +227,14 @@ public void Start()
 
             this.driverServiceProcess = new Process();
 
-            if (this.driverServicePath != null)
+            if (this.DriverServicePath != null)
             {
-                this.driverServiceProcess.StartInfo.FileName = Path.Combine(this.driverServicePath, this.driverServiceExecutableName);
+                if (this.DriverServiceExecutableName is null)
+                {
+                    throw new InvalidOperationException("If the driver service path is specified, the driver service executable name must be as well");
+                }
+
+                this.driverServiceProcess.StartInfo.FileName = Path.Combine(this.DriverServicePath, this.DriverServiceExecutableName);
             }
             else
             {
@@ -276,7 +243,7 @@ public void Start()
 
             this.driverServiceProcess.StartInfo.Arguments = this.CommandLineArguments;
             this.driverServiceProcess.StartInfo.UseShellExecute = false;
-            this.driverServiceProcess.StartInfo.CreateNoWindow = this.hideCommandPromptWindow;
+            this.driverServiceProcess.StartInfo.CreateNoWindow = this.HideCommandPromptWindow;
 
             DriverProcessStartingEventArgs eventArgs = new DriverProcessStartingEventArgs(this.driverServiceProcess.StartInfo);
             this.OnDriverProcessStarting(eventArgs);
@@ -288,8 +255,7 @@ public void Start()
 
             if (!serviceAvailable)
             {
-                string msg = "Cannot start the driver service on " + this.ServiceUrl;
-                throw new WebDriverException(msg);
+                throw new WebDriverException($"Cannot start the driver service on {this.ServiceUrl}");
             }
         }
 
@@ -327,10 +293,7 @@ protected void OnDriverProcessStarting(DriverProcessStartingEventArgs eventArgs)
                 throw new ArgumentNullException(nameof(eventArgs), "eventArgs must not be null");
             }
 
-            if (this.DriverProcessStarting != null)
-            {
-                this.DriverProcessStarting(this, eventArgs);
-            }
+            this.DriverProcessStarting?.Invoke(this, eventArgs);
         }
 
         /// <summary>
@@ -344,10 +307,7 @@ protected void OnDriverProcessStarted(DriverProcessStartedEventArgs eventArgs)
                 throw new ArgumentNullException(nameof(eventArgs), "eventArgs must not be null");
             }
 
-            if (this.DriverProcessStarted != null)
-            {
-                this.DriverProcessStarted(this, eventArgs);
-            }
+            this.DriverProcessStarted?.Invoke(this, eventArgs);
         }
 
         /// <summary>

dotnet/src/webdriver/Safari/SafariDriverService.cs

@@ -25,6 +25,8 @@
 using System.Text;
 using System.Threading.Tasks;
 
+#nullable enable
+
 namespace OpenQA.Selenium.Safari
 {
     /// <summary>
@@ -37,10 +39,10 @@ public sealed class SafariDriverService : DriverService
         /// <summary>
         /// Initializes a new instance of the <see cref="SafariDriverService"/> class.
         /// </summary>
-        /// <param name="executablePath">The full path to the SafariDriver executable.</param>
+        /// <param name="executablePath">The directory of the SafariDriver executable.</param>
         /// <param name="executableFileName">The file name of the SafariDriver executable.</param>
         /// <param name="port">The port on which the SafariDriver executable should listen.</param>
-        private SafariDriverService(string executablePath, string executableFileName, int port)
+        private SafariDriverService(string? executablePath, string? executableFileName, int port)
             : base(executablePath, port, executableFileName)
         {
         }
@@ -73,7 +75,7 @@ protected override TimeSpan TerminationTimeout
             // because the executable does not have a clean shutdown command,
             // which means we have to kill the process. Using a short timeout
             // gets us to the termination point much faster.
-            get { return TimeSpan.FromMilliseconds(100); }
+            get => TimeSpan.FromMilliseconds(100);
         }
 
         /// <summary>
@@ -84,7 +86,7 @@ protected override bool HasShutdown
         {
             // The Safari driver executable does not have a clean shutdown command,
             // which means we have to kill the process.
-            get { return false; }
+            get => false;
         }
 
         /// <summary>
@@ -107,7 +109,7 @@ public static SafariDriverService CreateDefaultService(string driverPath)
             if (File.Exists(driverPath))
             {
                 fileName = Path.GetFileName(driverPath);
-                driverPath = Path.GetDirectoryName(driverPath);
+                driverPath = Path.GetDirectoryName(driverPath)!;
             }
             else
             {