Add prefs capability to firefoxOptions. #162

Merged
merged 1 commit into from Sep 6, 2016

Projects

None yet

2 participants

@jgraham
Collaborator
jgraham commented Aug 5, 2016 edited

This takes the form of a dictionary of {pref_name: pref_value}. These
prefs are applied after the default prefs, but before those required
to enable marionette.


This change is Reviewable

@jgraham
Collaborator
jgraham commented Aug 5, 2016
@andreastt
Member

Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, 8 unresolved discussions.


src/main.rs, line 232 [r1] (raw file):

        firefox_options.insert("prefs".into(), Json::Object(prefs));
        capabilities.required.insert("firefoxOptions".into(), Json::Object(firefox_options));

Remove one of the blank lines


src/main.rs, line 241 [r1] (raw file):

        let prefs_set = profile.user_prefs().unwrap();
        println!("{:?}",prefs_set.prefs);

Debug?


src/marionette.rs, line 282 [r1] (raw file):

    pub profile: Option<Profile>,
    pub args: Option<Vec<String>>,
    pub prefs: Vec<(String, Pref)>

Missing ,


src/marionette.rs, line 366 [r1] (raw file):

            let prefs = try!(prefs_data
                             .as_object()
                             .ok_or(WebDriverError::new(ErrorStatus::UnknownError,"Prefs were not an object")));

Space before string


src/marionette.rs, line 428 [r1] (raw file):

            try!(self.start_browser(port, options));
        };

Don’t need two new lines here


src/marionette.rs, line 503 [r1] (raw file):

        prefs.write().map_err(|_| WebDriverError::new(ErrorStatus::UnknownError,
                                                      "Unable to write Firefox profile"))

In case of an error writing we should probably include the reason why. It could be because of no space, write permissions, anything.


src/marionette.rs, line 507 [r1] (raw file):

}

fn pref_from_json(value: &Json) -> WebDriverResult<Pref> {

This seems like something that could live on mozprofile::Preferences::Pref as some kind of FromJson trait?


src/marionette.rs, line 514 [r1] (raw file):

        &Json::Boolean(x) => Ok(Pref::new(x)),
        _ => Err(WebDriverError::new(ErrorStatus::UnknownError,
                                     "Could not convert pref value to string, boolean, or integer"))

s/integer/number/


Comments from Reviewable

@andreastt
Member

@jgraham I think this needs to be rebased.

@jgraham jgraham changed the base branch to master from firefox_options Sep 6, 2016
@andreastt
Member

Reviewed 2 of 4 files at r2, 2 of 5 files at r3.
Review status: 1 of 3 files reviewed at latest revision, 6 unresolved discussions.


README.md, line 83 [r3] (raw file):

        <td>Object
        <td>
        <td>Map of preference name to preference value.

I would suggest to highlight the typing of this dictionary, both in the second and fourth column:

<tr>
    <td><code>prefs</code>
    <td>Object.&lt;string, (string|boolean|number)&gt;
    <td>Map of preference name string from about:config to value,
     which can be one of string, boolean, or number.
</tr>

Or something similar to that.


src/main.rs, line 203 [r3] (raw file):

    fn test_profile() {
        let encoded_profile = example_profile();

Superfluous line


src/marionette.rs, line 514 [r1] (raw file):

Previously, andreastt (Andreas Tolfsen) wrote…

s/integer/number/

I’m wrong about this.

Comments from Reviewable

@andreastt
Member

Reviewed 2 of 5 files at r3.
Review status: all files reviewed at latest revision, 6 unresolved discussions.


Comments from Reviewable

@jgraham jgraham Add prefs capability to firefoxOptions.
This takes the form of a dictionary of {pref_name: pref_value}. These
prefs are applied after the default prefs, but before those required
to enable marionette.
e9f483a
@andreastt
Member

Reviewed 2 of 2 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved.


src/marionette.rs, line 503 [r1] (raw file):

Previously, andreastt (Andreas Tolfsen) wrote…

In case of an error writing we should probably include the reason why. It could be because of no space, write permissions, anything.

Let’s fix this later.

src/marionette.rs, line 507 [r1] (raw file):

Previously, andreastt (Andreas Tolfsen) wrote…

This seems like something that could live on mozprofile::Preferences::Pref as some kind of FromJson trait?

Can also be fixed later.

Comments from Reviewable

@andreastt andreastt merged commit 804def9 into master Sep 6, 2016

3 checks passed

code-review/reviewable 3 files reviewed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment