636035 - Refactor handling of defaults (i.e. preferences) for profiles in mozprofile

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Given bug 633189 and others we need a more clean way in specifying preferences which should be set before the first start, without having to modify Mozmill in various ways. If possible we should even extent that to other files, so we can pre-populate profiles with available test-files.

A feature like that would also block bug 551103.

Comment 1

[Jeff Hammel]

It would be "easy" to set preferences from the command line:

mozmill --pref 'foo=bar' ...

You could also take prefs e.g. from a .ini file, since they're just key, value pairs 

However, for both of these, there is a hard thing: namely, how do you tell the type that the preference is supposed to be? (string, int, bool...).  I'd prefer to determine this automatically if possible.

In any case, this shouldn't be a hard thing to implement once the spec is agreed on

Comment 2

[cmtalbert]

Ideally, we determine it from the input.  But because there are not guarantees, for instance, we *do* have prefs that are "1" (the string) instead of 1 (the int), this simply must be specified by the user.  Sucky yes, but there you go.  I prefer having an option to specify them on the command line (let's do what mochitest/reftest do here) and let's also have a file for setting a bunch of complicated prefs.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Jeff, I can remember a bug you have filed for default settings to get specified in an ini file. Sounds like it would depend on it.

Comment 4

[Jeff Hammel]

So I propose the following three prong attack:
- read prefs from the command line --pref 'foo=bar' (or pref=foo=bar)
- read prefs from (one or more) .ini files --preferences myprefs.ini
- and (already done) you can still specify them via the API

In the first two cases, you parse the strings as follows
- integers and true/false are converted to what they should be
- all else are strings
- ...but in the case where you have e.g. '1' or "1", the quotes are removed and these are just strings
- ... and if we really needed the literal string '1', we could do "'1'" and strip the other layer of quotes

That should cover all cases. Any objections?

Comment 5

[cmtalbert]

(In reply to comment #4)
> So I propose the following three prong attack:
> - read prefs from the command line --pref 'foo=bar' (or pref=foo=bar)
> - read prefs from (one or more) .ini files --preferences myprefs.ini
> - and (already done) you can still specify them via the API
> 
> In the first two cases, you parse the strings as follows
> - integers and true/false are converted to what they should be
> - all else are strings
> - ...but in the case where you have e.g. '1' or "1", the quotes are removed
> and these are just strings
> - ... and if we really needed the literal string '1', we could do "'1'" and
> strip the other layer of quotes
> 
> That should cover all cases. Any objections?

Sounds great.

Comment 6

[Jeff Hammel]

(In reply to comment #4)
> So I propose the following three prong attack:
> - read prefs from the command line --pref 'foo=bar' (or pref=foo=bar)
> - read prefs from (one or more) .ini files --preferences myprefs.ini
> - and (already done) you can still specify them via the API

Also, conceivably you could use a prefs.js file

Comment 7

[Jeff Hammel]

The other thing that should be considered is, for an .ini file, what format should it be in?

ConfigParser, the stdlib python .ini parser, uses a format like:

[DEFAULT]
browser.migration.version = 5

[MyHomepage]
browser.startup.homepage = http://planet.mozilla.org/

[OtherHomepage]
browser.startup.homepage = http://k0s.org/

Reading this, the DEFAULT options are added to each section:

>>> parser.options('MyHomepage')
['browser.migration.version', 'browser.startup.homepage']

So what sort of command line to use here?

Proposal:

`--preferences prefs.ini` reads the [DEFAULT] section from prefs.ini and changes them into preferences

`--preferences prefs.ini:MyHomepage` reads the [MyHomepage] section from prefs.ini which includes the preferences from [DEFAULT]

You can chain these: --preferences prefs.ini:MyHomepage --preferences prefs.ini:OtherSection --preferences moreprefs.ini

Also, we could do .json too (should be pretty easy to tell JSON from .ini, I don't think we need a separate command line flag)

I don't know if we want/need to take on all of these for this bug. I'll implement the command line stuff, maybe the .ini stuff if its not too contraversial, maybe the JSON stuff, and we can ticket follow-ups

Comment 8

[Jeff Hammel]

Note that currently, from the command line, we use a ':' for a pref separator:

https://github.com/mozautomation/mozmill/blob/master/mozprofile/mozprofile/cli.py#L78

Do we want to continue with this?  No response == 'yes'

Comment 9

[Jeff Hammel]

Attachment: adds preferences handling to mozmill

Comment 10

[Heather Arthur [:harth]]

Comment on attachment 539620 [details] [diff] [review]
adds preferences handling to mozmill

Review of attachment 539620 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozprofile/mozprofile/cli.py
@@ +49,5 @@
> +__all__ = ['CLIMixin', 'MozProfileCLI', 'cli']
> +
> +
> +class CLIMixin(object):
> +

Why do we need this object? Why not just put everything in the CLI?

Comment 11

[Jeff Hammel]

(In reply to comment #10)
> Comment on attachment 539620 [details] [diff] [review] [review]
> adds preferences handling to mozmill
> 
> Review of attachment 539620 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: mozprofile/mozprofile/cli.py
> @@ +49,5 @@
> > +__all__ = ['CLIMixin', 'MozProfileCLI', 'cli']
> > +
> > +
> > +class CLIMixin(object):
> > +
> 
> Why do we need this object? Why not just put everything in the CLI?

The `mozprofile` command line currently also offers the --print-addon-id. While this is appropriate to mozprofile, its probably not functionality that mozmill or mozrunner should have, IMHO.  `mozprofile` should be about profiles, as the behaviour of the command lines is much different, `mozprofile`'s CLI delegates much of the control to the cli() function, as it is much different from mozrunner and mozmill, which (mostly) invoke e.g. firefox and does stuff (e.g. runs tests).  I don't really think mozmill should be a swiss army knife for doing everything.  All relevant options for the operation of mozmill should be bubbled up to mozrunner -> mozmill, but for things that mozmill doesn't need to do, e.g. print out addon-ids, these should live in a more appropriate script.

Alternatively, we can take out the --print-addon-id

Comment 12

[Jeff Hammel]

> Alternatively, we can take out the --print-addon-id
Or make a command line program, `print-addon-id`, that just does this, since it is unrelated to other functionality.

Comment 13

[Heather Arthur [:harth]]

As discussed over Fiesta Del Mar, we should remove --print-addon-ids unless it's being used for something. It appears to have just been a debugging helper for a short time.

Comment 14

[Jeff Hammel]

Attachment: changes made

remove print-addon and unbitrot

Comment 15

[Heather Arthur [:harth]]

Comment on attachment 539946 [details] [diff] [review]
changes made

Looks good to me if you've tested.

Comment 16

[Jeff Hammel]

pushed to master: https://github.com/mozautomation/mozmill/commit/b55d57b22744cf1fc8d33f8f2fc06dfde09e4560

I have pretty good tests for the preferences, less so for addon-manifests and addons, but I've done these by hand and they seem to work for the cases I've tried.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+2]]

So what about prefs which absolutely have to be set by the framework itself like disabling the modal quit warning at shutdown? IMO we should define at least the minimum prefs which are necessary to set in any case to get Mozmill working properly.

Comment 18

[Jeff Hammel]

There are a few things here.  Most importantly, this fix does not change the default preferences in e.g. FirefoxProfile: https://github.com/mozautomation/mozmill/blob/master/mozprofile/mozprofile/profile.py#L172 . These are still there in unaltered form.

I believe that these *should* be refactored.  As you said, we care about the minimum prefs required to get *Mozmill* working properly.  Strictly speaking, mozprofile does not know about mozmill.  While they are in the same repository, this is an arbitrary convenience. There is a one-way dependency of Mozmill on mozprofile but not vice-versa and....we should not have implicit dependencies.  Mozprofile is already used in other projects, so its important that we don't put mozmill-specifics in it that are undesirable for other code.

IMHO, FirefoxProfile and ThunderbirdProfile should (at least as they stand) probably just go away and the desired preferences for running be provided by FirefoxRunner and ThunderbirdRunner. Additional Mozmill-specific preferences could be sent down the chain by Mozmill itself.

However, there is (also IMHO) no particular reason to make any of these changes until we need them.  While I think its good to note this for future work, as stated in the first paragraph of this comment, this change does not affect any of the preferences in profile.py currently, it just allows you to add new preferences via --pref, a JSON file, or an .ini file or override existing preferences.  Additional work should be ticketed as needed.

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Thanks Jeff. That sounds fine. I have filed bug 665662.