Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add an API to set capabilities #10

Open
2 tasks
equalsraf opened this issue Jan 29, 2018 · 7 comments
Open
2 tasks

Add an API to set capabilities #10

equalsraf opened this issue Jan 29, 2018 · 7 comments

Comments

@equalsraf
Copy link
Contributor

I don't have a very concrete example to propose for this yet. But we need some way to build the NewSessionCmd object that allows setting the capabilities and desired capabilities.

In #9 I took the cheap way out and just used the json! macro to craft the capabilities. But we need something that can be used in a public api like a builder for the capabilities object.

Storage can be implemented via a new type (this uses the code in #9 as an example)

#[derive(Serialize, Default)]
pub struct Capabilities {
    alwaysMatch: collections::BTreeMap<String, JsonValue>,
}

#[derive(Serialize)]
pub struct NewSessionCmd {
    capabilities: Capabilities,
}

impl NewSessionCmd {
    pub fn new() -> Self {
        let mut capabilities: Capabilities = Default::default();
        capabilities.alwaysMatch.insert("goog:chromeOptions".to_string(),
                                                                  json!({"w3c": true}));
        NewSessionCmd {
            capabilities,
        }
    }
}

Maybe something like the following method in NewSessionCmd but have not taken into consideration the firstMatch attribute in the spec. Probably should have a look at it first.

    pub fn capability(&mut self, name: &str, value: Option<JsonValue>) {
        match value {
            Some(value) => self.capabilities.alwaysMatch.insert(name.to_string(), value),
            None => self.capabilities.alwaysMatch.remove(name),
        };
    }

My other doubt here is the type of the supplied value argument. Should we take a serde_json::Value or more generically anything that implements Serialize? Since Value does implement Serialize maybe we should start with just Value.

TODO

  • add api/methods to NewSessionCmd to manipulate the capabilities
  • change DriverSession::create_session API to accept additional parameters?
@fluffysquirrels
Copy link
Owner

We will probably never support the latest options for every browser, so I think having a json::Value somewhere is a good idea. Perhaps JSON inside Capabilities and have a method on it that'll take a string key and JSON value, and a method that'll take some typed Gecko- or ChromeDriverCapabilities and merge with the JSON inside?

I'd be fine with a first implementation that just accepts a JSON blob, with the option to add methods taking typed objects later.

Would you like to work on this issue further, perhaps as part of the ChromeDriver PR?

@equalsraf
Copy link
Contributor Author

I'd be fine with a first implementation that just accepts a JSON blob, with the option to add methods taking typed objects later.

I'll probably do something like this for the chrome tests to work on travis.

@equalsraf
Copy link
Contributor Author

ref for merge implementation serde-rs/json#377 (comment)

@fluffysquirrels
Copy link
Owner

fluffysquirrels commented Feb 1, 2018

I like the sound of something fluent, like your builder idea.

How about:

struct Capabilities {
    ...
}

impl Capabilities {
    // Actually empty collection
    fn empty() -> Capabilities { ... }

    // I really like the sound of your generic Serializable idea if you can get it
    // to compile without too much trouble.
    fn always(&mut self, key: &str, value: Option<serde_json::Value>) -> &mut Self {
         ...
    }
}

impl Default for Capabilities {
    // Default will include e.g. goog:chromeOptions workaround we need for
    // ChromeDriver to behave.
    fn default() -> Capabilties { ... }
}

// Usage example:
let caps =
    Capabilities::default()
        .always("foo", json!{ bar: 12 });
let session = driver.session(caps);

@equalsraf
Copy link
Contributor Author

Should the builder api be in the Capabilities type or the NewSessionCmd type. currently I have them in NewSessionCmd the Capabilities struct is not public and only being used internally.

@equalsraf
Copy link
Contributor Author

equalsraf commented Feb 1, 2018

Not the best naming but still

    /// Extend a capability requirement with a new object.
    ///
    /// Extending a capability that does not exist, or attempting to extend non objects will have
    /// the same effect as calling [always_match()](#method.always_match).
    pub fn extend_always_match(&mut self, name: &str, capability: JsonValue) {
        if let JsonValue::Object(capability) = capability {
            if let Some(&mut JsonValue::Object(ref mut map)) = self.capabilities.alwaysMatch.get_mut(name) {
                map.extend(capability);
                return;
            }
            self.capabilities.alwaysMatch.insert(name.to_string(), JsonValue::Object(capability));
        } else {
            self.capabilities.alwaysMatch.insert(name.to_string(), capability);
        }
    }

For example this adds more options for chrome without discarding our default values.

                let mut session_params: NewSessionCmd = Default::default();
                session_params.extend_always_match("goog:chromeOptions", json!({
                    "args": ["--no-sandbox", "--headless"],
                }));

I've been reading the spec but the semantics of alwaysMatch and firstMatch is not very clear, this description helps though w3c/webdriver#1215 (comment)

@fluffysquirrels
Copy link
Owner

Yes, that link is very helpful for understanding the semantics! Roughly it seems like alwaysMatch is an object of your requirements, firstMatch is an array of objects, which are your preferences in order?

Should the builder api be in the Capabilities type or the NewSessionCmd type.

Thinking about it again, your approach of having it on NewSessionCmd is probably a little neater, with fewer types exposed in the public API.

On naming, I'm happy with any of: extend_always_match, always_match, always.

I doubt many people new to WebDriver would understand any of the names without looking at the documentation or the specs, so I have a slight preference for the shorter ones to make the common case simplest to type and read. The longer ones are a bit more explicit, of course. Bear in mind that if you pick a short version, there could be another method reset_capabilities or reset_always_match to start from scratch and erase the existing values.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants