Add AddAdditionalCapability(string capabilityName, object capabilityValue, bool isGlobalCapability) overload to DriverOptions

[ohadschn]

The current state is inconsistent:

1. ChromeOptions, FirefoxOptions, and InternetExplorerOptions each have their own AddAdditionalCapability(string, object, bool) overload (unrelated in terms of polymorphism).
2. SafariOptions and EdgeOptions don't a AddAdditionalCapability(string, object, bool) overload at all.

This means that you can't share code and have to have undesirable casts like the following:

static void AddGlobalCapability(this DriverOptions options, string name, object capabilityValue)
{
    switch (options)
    {
        case ChromeOptions chromeOptions:
            chromeOptions.AddAdditionalCapability(name, capabilityValue, true);
            break;
        case FirefoxOptions firefoxOptions:
            firefoxOptions.AddAdditionalCapability(name, capabilityValue, true);
            break;
        case InternetExplorerOptions internetExplorerOptions:
            internetExplorerOptions.AddAdditionalCapability(name, capabilityValue, true);
            break;
        default:
            options.AddAdditionalCapability(name, capabilityValue);
            break;
    }
}

Note how the exact same call has to be written multiple times due to said lack of polymorphism.

This API discrepancy also demonstrates inconsistency between the semantics of AddAdditionalCapability(string capabilityName, object capabilityValue). For Chrome, FF, and IE it adds browser-specific capabilities (by defaulting to isGlobalCapability = false), whereas for Safari and Edge it adds global capabilities (the equivalent of defaulting to isGlobalCapability = true had there been a similar overload accepting the latter).

[MattKeenum]

I would like to second this. A simple abstract method on the parent DriverOptions class that overloads AddAdditionalCapability with a boolean variable (exactly how all of the children classes are already doing) would allow us to avoid @ohadschn 's example.

[jimevans]

Alright, here's the problem. The name AddAdditionalCapability is about as perfect a method name as one could craft for what it's doing. However, I absolutely do not want, and will not be, adding an abstract three-argument overload. That introduces an API contract on implementing classes that makes no sense whatsoever. In order to preserve the name of the method in the long run, and to appropriately separate the concerns for the generic factory case, the solution will be as follows:

1. AddAdditionalCapability will be marked with the Obsolete attribute, and will be considered deprecated for the 4.0 release.
2. The base DriverOptions class will gain a method called AddAdditionalOption, which will add a non-type-safe capability value to the top-level of the JSON blob created for transmission across the wire (equivalent to the three-argument overload with isGlobalCapability = true).
3. The browser-specific subclasses will gain appropriately named methods to replace the AddAdditionalCapability method. These will be named AddAdditional<browser name>Option for the appropriate options class (e.g., AddAdditionalChromeOption, AddAdditionalFirefoxOption, etc.). Given that the raw property name of the capability often includes the name "option" (cf. goog:chromeOptions, moz:firefoxOptions, se:ieOptions, etc.), this seems like a reasonable naming scheme for such methods.
4. At some point after the initial 4.0 release, the base class's AddAdditionalOption method will be marked deprecated, and renamed back to AddAdditionalCapability.

People are free to disagree with this approach, but silently changing the semantics of the existing method (i.e., eliminating the three-argument overload behavior) seems hostile, and there is no compelling use case for having a method that adds browser-specific capabilities, if no browser-specific capabilities exist. Please be mindful of how your phrase your disagreements in this issue report; I would rather not have to shut off all discussion by locking the issue.

Downstream projects such as Appium will need to be aware of these changes, and adjust their subclasses accordingly.

[ohadschn]

So basically starting version 4.0 the code I presented above could simply be written as options.AddAdditionalOption(name, capabilityValue) and starting say version 5.0 it could be written as options.AddAdditionalCapability(name, capabilityValue) (where in both cases the capability would be global)?

[jimevans]

@ohadschn That’s the idea.

[jimevans]

With the refactor of the DriverOptions class in 5bd9f79, this issue is resolved, with a plan going forward to return the method to its original name in a future revision. As such, I'm going to close this issue. The change should be available when the first 4.0 alpha versions are released (though there's no set schedule for that, so inquiries as to availability will be tersely rejected).