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.

Sign up for GitHub

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

Improve firefox profile prefs merging: #423

Merged
merged 1 commit into from
Apr 4, 2017
Merged

Improve firefox profile prefs merging: #423

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions src/marionette.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,11 +334,12 @@ impl MarionetteHandler {
.map_err(|_| WebDriverError::new(ErrorStatus::UnknownError,
"Unable to read profile preferences file")));

prefs.insert("marionette.defaultPrefs.port", Pref::new(port as i64));
for &(ref name, ref value) in prefs::DEFAULT.iter() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is obviously the correct behaviour. If you set a custom profile you might reasonably expect to get the prefs you set, except any that are actually required to make marionette function correctly. If you want the default set you can always set those explicitly.

What's the specific problem here that you're trying to solve? (n.b. I hope that in the future we can enable more things that currently require an entire custom profile via more fine grained capabilities e.g. extension installation).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this might be a result of 2bfdc3e.

Many of the preferences in prefs::DEFAULT are actually required for Marionette to function correctly, and some of these we can’t set at runtime. We should probably move those that are absolute requirements over to prefs::REQUIRED, and we should set those we can set at runtime in Marionette. I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1323290 about the latter before Christmas.

As I read this code, the default prefs will only get written to the profile if a custom profile is not provided. The intention of prefs::DEFAULT is to say “we recommend these settings for running Marionette”, and triggering another code path that doesn’t set these if the user provides a profile a bit surprising in my opinion.

I’m also curious what the reason for this change is. I guess you are providing a profile with some preferences that get overridden by geckodriver?

Copy link
Contributor Author

@DrMarcII DrMarcII Jan 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the customers I support, the usual pattern is that they start by using the default settings, and at some point they realize that they need to install an extension or set up some certs that can (currently) most easily be accomplished by providing a custom profile. It is then surprising when a profile created with the sole purpose of doing this causes other behavior changes because DEFAULT prefs are no longer being set. My goal with the change is to make that transition from no profile to providing a profile more seamless by only changing things that the user has explicitly chosen to change when providing the profile.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thinking at the moment is to move those preferences that are absolutely required to meet WebDriver conformance from prefs::DEFAULT to prefs::REQUIRED and to accept this change. This would change the meaning of ‘default preferences’ towards ‘suggested preferences’, as they would not take effect if a custom profile with custom preferences is provided.

if !custom_profile || !prefs.contains_key(name) {
prefs.insert((*name).clone(), (*value).clone());
}
}

if !custom_profile {
prefs.insert_slice(&prefs::DEFAULT[..]);
};
prefs.insert_slice(&extra_prefs[..]);

prefs.insert_slice(&prefs::REQUIRED[..]);
Expand All @@ -347,6 +348,8 @@ impl MarionetteHandler {
prefs.insert("marionette.logging", Pref::new(level.to_string()));
};

prefs.insert("marionette.defaultPrefs.port", Pref::new(port as i64));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this part I agree is correct :)


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