feat: install and run an extension with custom Firefox preferences

[saintsebastian]

Current commit without repeated option and with preferences assignment commented. Since there are so many changes for this one, I'd like to know if it is the correct approach so far.

Fixes #88

[saintsebastian]

For some reason even though I've worked from a branch some of my commits are added here as well.

[kumar303]

Hi @saintsebastian ! Thanks for the patch. @rpl could you take a look?

[rpl]

Hi @saintsebastian,
the direction seems the right one to me, I didn't commented all the changes but I've added below more detailed comments about:

• the import/export syntaxes related flow types annotations
• some ideas about the names for the new properties and how to pass them around
• some notes about the yargs option handling

Let me know if I forgot to comment something important, or if some of the comments are not clear or informative enough.

[rpl]

@saintsebastian I was wondering if this showReadyMessage is actually unrelated to this pull request, am I right?

[saintsebastian]

@rpl You are completely correct, for some reason three previous commits from separate branches got stuck here as well. As fas as I understand you can cherry pick a pull request. What's the best way to approach this? Should I continue committing to the same branch?

[rpl]

Hehe, no worries, managing git branches can be tricky sometimes and git commands/workflows are not always very intuitive :-)
(and after a bit of practice everything looks simpler than it really is ;-))

Sometimes it is simpler to rename the old branch (git branch -m OLDNAME NEWNAME), switch to a new pristine branch, named like the one pushed on github (git checkout master && git checkout -b OLDNAME), and start again (and maybe cherry-picking what is still useful from the previous branch)

But in my opinion, there is a lot of value also into learning how to rewrite the existent history in the branch that we are working on (so that we feel confortable that we can manage every situation that we can find ourselves while we are working on a git branch), and so I'd like to give you more information of the topic :-D

There are many ways to get rid of commits from a branch in git, my favorite one is the interactive rebase, e.g. we can ask git to let us rebase interactively the last 4 commits using the command:

$ git rebase -i HEAD~4

and it will open our configured editor (the one in the EDITOR environment variable or the standard one of our system) with the
list of the commits that we are going to rebase:

pick 062d62a fix: Don't log info about the built extension when signing
pick 10f8b7b fix: the code consistency for issue 181
pick 7edc47f fix: another code inconsistency for issue 181
pick 77626b8 fix: issue 88 first commit

In the opened editor, if you remove the first three lines, save and exit, git will remove the correspondent commits from the branch you are working on.

The following are a couple of additional resources on the topic that you may find interesting/useful:

• http://learngitbranching.js.org/ has a nice interactive tutorial on both "reverting single commit" and "interactive rebasing", with nice diagrams to help you visualizing what is happening in the git repo
• https://www.atlassian.com/git/tutorials/rewriting-history/git-rebase has a short description and a diagram
• https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History has more detailed informations on how the git interactive rebase works

[saintsebastian]

Thanks! That was surprisingly easy to do! I'll definitely read on that to avoid mishaps

[rpl]

This makes me notice that we should definitely add to our CONTRIBUTING.md files more informations about the flowtypes syntaxes that we use, because it is not so easy to spot them at a first glance:

the export type statements (and also the import type statements) are related to the Flow types annotations (https://flowtype.org/docs/syntax.html#importing-and-exporting-types).

By annotating the pref property with a {} type we are actually saying to flow that the value can be an object with any kind of property in it.

We should try to put a more specific type annotation here, e.g. by reusing the type that is already defined in the src/firefox/preferences module, e.g. something like:

 // Import objects that are only used as Flow types.
 import type FirefoxProfile from 'firefox-profile';
+import type {FirefoxPreferences} from '../firefox/preferences';
 import type {OnSourceChangeFn} from '../watcher';
 import type Watchpack from 'watchpack';
 import type {
@@ -149,7 +150,7 @@ export type CmdRunParams = {
   firefoxProfile: string,
   preInstall: boolean,
   noReload: boolean,
-  pref: {},
+  pref: FirefoxPreferences,
 };
 
 export type CmdRunOptions = {

The FirefoxPreferences type tells to flow that the preferences should be a map type which has keys of type strings and values that are of type strings, booleans or numbers (and so it will prevents to nest objects as preferences values, which is good given that that is not supported by the Firefox preferences values).

[rpl]

I'd also like a more descriptive name for it, e.g. how about customPreferences or customPrefs?

[saintsebastian]

The name suggesting is great, moreover pref is widely used name in the codebase already. The thing I want to ask concerns the fact that customPrefs is an optional parameter, is that potentially troublesome?

[rpl]

that's right, in flowtype there is a special syntax to make an object property optional:

• https://flowtype.org/docs/objects.html#optional-properties

[rpl]

While propagating the custom properties through the web-ext internal, it would be better to keep the same property same if possible (e.g. customPrefs as proposed in one of the above comments), because it will make it easier for another developer to follow how it flows through the internals.

[rpl]

how about using the options object to pass the custom properties in the createProfile, configureProfile and configureProfile and functions?
(so that we can do something like createProfile(profile, {customPrefs}) etc.)

[saintsebastian]

So this is the last step in configureProfile, which I haven't properly tested yet.

 Object.keys(customPrefs).forEach((custom) => {
  profile.setPreference(custom, customPrefs[custom]);
});

Is using a defined flow type and yargs parsing of values suffice for control over what properties user assigns? At the moment command happily takes any property name (and number of them)

[rpl]

it is hard to be completely sure that a custom property will be completely valid/recognized by Firefox (e.g. if you open the "about:config" url in a Firefox tab you can briefly take a look at the one defined in your Firefox profile), what we can check is probably that:

• preference name:
  • it should have at least one "."
  • it should always start with a lower case letter
    (actually in the preferences names used in Firefox everything on the left of the first "." is always composed only by lowercase letters)
  • it should contain only valid characters (e.g. lowercase and uppercase letters, numbers, and a couple of special characters like . , -, _ and *)
• preference value:
  • it can only be integers, booleans or strings (everything that is not a boolean or an integer)

Most of the above validations on the preference names seems to be doable with regular expressions (https://developer.mozilla.org/en/docs/Web/JavaScript/Guide/Regular_Expressions) that are still readable from us human beings :-)

(we can write some really amazing/crazy things with regex syntaxes but, at least in my own opinion, if it is possible, it is better to use an amount of them that make the final expression still reasonably readable)

On the bright side, if someone really needs this feature, he usually knows pretty well what he is doing, and so the above validations should be more then enough (maybe some of them are even too much).

About the "where it would be better to validate the custom preferences", in general I think that is always better to validate the external inputs only on the "border" of our software systems (which means as near as possible to where the external input has been received) so that we are not going to check them everywhere.

In this case a good position to validate the custom preferences is probably inside the execute method of the Program class :
https://github.com/mozilla/web-ext/blob/master/src/program.js#L83

(it could be probably nicer if we define and export the helper function that will validate these customPrefs object in the src/firefox/preferences.js module and then import and call it from the execute method in the src/program.js module)

[rpl]

I'm not sure that yargs supports a type object, we should probably use type: 'string' here, and split strings that are in the form "mypref.dotted.name=value" into the key and value parts.

Related to the value part, we should detect if the value part is a number (we can use the same trick that yargs uses and described here http://yargs.js.org/docs/#parsing-tricks-numbers) or a boolean (e.g. if the value is equal to the string "true" or "false" convert the value into the correspondent boolean value), and if it is not any of the above types we will assume that it is a string.

About the repeated option: when an option is passed more then ones on the command line, yargs will provide you the array of all the collected values.

[saintsebastian]

After reading doc for some time I understand that it would look like "pref.dotted.name=value" and yargs will automatically turn this into pref = {dotted: {name:value}}, hence repeated option like "pref.name=value pref.anotherName=value" will just be a nested object handled my yargs. That's really cool!
However I'm still not sure about when the type detection is happening. Numbers should be detected automatically as the link you've shared suggests, as I understand coercing has those capabilities and we don't need to know in advance which properties are used http://yargs.js.org/docs/#methods-coercekey-fn. I imagine it looks something like this

yargs.coerce('pref', function (obj) {
 for (var in obj) {
   if (!isNaN(Number(obj.var))) {
     obj.var == Number(obj.var);
   } else if (obj.var === 'true' || obj.var === 'false) {
      obj.var == Boolean(obj.var);
   }
 }
})

Let me know if that is what you had in mind

[rpl]

I took a brief look at this alternative --pref.key.subkey=value syntax, but I'm unsure about it, in particular if it makes this feature more easily implementable and/or if it makes this command line option more or less readable for the user.

The usage on the command line would make the command to looks like:

$ web-ext run --pref.browser.search.region=IT \           
              --pref.general.useragent.locale=it-IT

while I was thinking about something like the following:

$ web-ext run --pref "browser.search.region=IT" \           
              --pref "general.useragent.locale=it-IT"

@kumar303 what do you think about the above two command line syntaxes for this --pref option?

[rpl]

But besides the command line syntax, using the coerce method seems like a good idea to me (and it is also in a better position than what I suggested in one of my comment above, it is as near as possible to where the external input enters our system), the code in src/program.js will become something like:

'pref': {
  describe: 'Launch firefox with custom preferences...',
  demand: false,
  requiresArg: true,
  type: 'string',
  coerce: coerceCLICustomPreference,
}

And in the helper function we can incapsulate all the details related to validate and parse the command line option, e.g. if we opt for the syntax that I was suggesting above:

• if value received does not represent a valid preference, we can throw new UsageError("Invalid Preference..."); and web-ext will not run the command and it will show the usage help and the error message raised from our helper

• we can split key and value and return an object, e.g.

  let [key, value] = value.split("=");

  // ... detect and convert integer and boolean values, 
  // and fallback to a string value 

  // The following is a weird but useful ES6 syntax for object
  // property names which are computed:
  return {[`${key}`]: value};

For the coercion of the value, what I'm thinking of is not too far from the above (there are some typos in the snippet that you pasted above ;-) but no worries, we will get into the details once we are reviewing the actual commit, and typos are pretty common when sketching example code in a comment), e.g. to detect and convert an integer, I was thinking of something like:

if ( value === `${parseInt(value)}`) {
  value = parseInt(value, 10);
}

Because we want to be sure that we only convert the value to a number if it is an integer (and parseInt("123.123", 10) returns 123 and ignore the rest, and parseInt will try to detect the radix if it is not specified explicitly, you can find more details about the parseInt behavior on https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/parseInt).

[rpl]

we should also add requiresArg: true here
(http://yargs.js.org/docs/#requiresArg).

[saintsebastian]

I am not entirely happy with everything. Here are my concerns

• insufficient testing, currently the feature is only tested within configureProfile testing suit, please let me know if I can and should improve that
• Every time I tried to add customPrefs?: FirefoxPreferences to CopyProfileOptions the entire type would break and Flow would refer to following as a source. For this reason the typing is inconsistent (ConfigureProfileOptions and CreateProfileParams already include it) and I would live to try to fix it

export type FirefoxPreferences = {
  [key: string]: bool | string | number,
};

Looking forward for any input and suggestions!

[coveralls]

Coverage decreased (-1.7%) to 98.257% when pulling 28f17b6 on saintsebastian:fix-88 into 9e5f7af on mozilla:master.

[rpl]

Hi @saintsebastian
This PR is near to its goal, There are some changes still required (and I'll go more deeply through the tests in the next review round), and it would be better to:

• rebase these two commits before making any further changes, the build is failing on travis because changelog-lint is linting commit messages that are not coming from your branch (https://travis-ci.org/mozilla/web-ext/jobs/183917577#L3759), I tried it locally and it looks like your branch can be rebased on the updated master without any change, using the following simple sequence of git commands: git fetch origin && git rebase origin/master)

• I'd like to discuss the issue with flow type checking of the CopyProfileOptions and evaluate how to proceed with Kumar before trying to move that parameter into the CopyProfileOptions type

Part of the code is not currently covered by tests, you can check which parts on coveralls:

• https://coveralls.io/builds/9276835

and you can also generate the coverage report locally by running COVERAGE=y npm run test

[rpl]

No need to assign this.customPrefs inside the if statement, we can remove the if that currently surrounds this.customPrefs = customPrefs and it should do mostly the same (it's true that the customPrefs property will be always be created, but it will be assigned to undefined or null)

[saintsebastian]

Originally I've put the if statement there because flow doesn't like undefined assignment, now i've added default customPrefs = {} value instead.

[rpl]

oh thanks! it is better if we order the properties by name, it is unrelated with the goal of this PR but, given that we are here, do you mind if we also move the configureThisProfile before copyFromUserProfile so that they are alphabetically ordered?

[saintsebastian]

Sure! I sorted them in function definition as well for consistency, will apply the same order in tests.

[rpl]

I like the idea of logging the custom preferences that are going to be set, but 'EMPTY' will not be very helpful to the users that are not going to use the new command line option.

Also, take into account that customPrefs has an empty object as its default, and so if it is not explicitly set to null/undefined, customPrefs will always be defined (and so we should also check if Object.keys(customPrefs) returns an empty array)

How about logging a message only if customPrefs is defined by putting log.info into an if statement?
It would also better to:

• include a useful message to the user (e.g. something like Setting custom Firefox preferences: ${...})
• it would be probably better to make the JSON.stringify call to generate a more readable output (the third parameter of JSON.stringify can do that for you ;-))
• print the log.info message before setting the custom preferences

[rpl]

I managed to move the customPrefs into the CopyProfileOptions flowtype definition, by applying the following changes:

diff --git a/src/cmd/run.js b/src/cmd/run.js
index 3aab9d1..fe2a681 100644
--- a/src/cmd/run.js
+++ b/src/cmd/run.js
@@ -301,7 +301,7 @@ export class ExtensionRunner {
     return new Promise((resolve) => {
       if (profilePath) {
         log.debug(`Copying Firefox profile from ${profilePath}`);
-        resolve(firefoxApp.copyProfile(profilePath, customPrefs));
+        resolve(firefoxApp.copyProfile(profilePath, {customPrefs}));
       } else {
         log.debug('Creating new Firefox profile');
         resolve(firefoxApp.createProfile({customPrefs}));
diff --git a/src/firefox/index.js b/src/firefox/index.js
index 4e497cb..1b9327c 100644
--- a/src/firefox/index.js
+++ b/src/firefox/index.js
@@ -259,6 +259,7 @@ export type CopyProfileOptions = {
   app?: PreferencesAppName,
   copyFromUserProfile?: Function,
   configureThisProfile?: ConfigureProfileFn,
+  customPrefs?: FirefoxPreferences,
 };

 /*
@@ -275,11 +276,11 @@ export type CopyProfileOptions = {
  */
 export async function copyProfile(
   profileDirectory: string,
-  customPrefs?: FirefoxPreferences,
   {
     app,
     copyFromUserProfile = defaultUserProfileCopier,
     configureThisProfile = configureProfile,
+    customPrefs = {},
   }: CopyProfileOptions = {},
 ): Promise<FirefoxProfile> {

I must say that in this particular case, the flowtype error wasn't very helpful, because it pointed out this lines and the FirefoxPreferences type definition, but not the actual caller function in "src/cmd/run.js", which was the actual source of the flow check error once the property is moved into the options object but the caller has not been fixed yet.

I looked a bit more deeply into the reasons of the "not very helpful flow error messages", and it seems to be connected to the fact that both the flowtypes are objects type, but CopyProfileOptions is a flowtype which specify particular keys and value type while, the FirefoxPreferences is a type with arbitrary key names, which can theoretically include the key defined in the FirefoxPreference type, but their values can only be of type boolean, string or number... that is very unfortunate :-( , and something to keep in mind for the future.

@kumar303 I'm even wondering if it would be better to pass the customPrefs object as a separate parameter, given how bad the errors is reported due to these two flow type definitions.

[saintsebastian]

It makes perfect sense now. The flow error really confused me, even though that was a simple bug.

[saintsebastian]

Thanks for resolving this!

[rpl]

This import should be merged with the above (as in import {WebExtError, UsageError} from '../errors':)
(and keep the two empty lines between the section of "the imports" and the rest of the sources in the module, it is a conventions mostly used in python, web-ext currently follows this convention and so we should keep it so that the coding style is consistent).

[rpl]

I'm wondering if it would be better to:

• keep the command line option to --pref and use customPrefs only internally
• remove the single character alias 'c', given that it shouldn't be a command line option so commonly used to justify the alias

[rpl]

We should test this on a preference that is not already in the preferences that are always set automatically (e.g. this one is already include because it is used by web-ext run to implement the live reload feature)

[rpl]

By looking at the code coverage reports, it seems that this function is currently never executed by the current defined tests:

https://coveralls.io/builds/9276835/source?filename=src%2Ffirefox%2Fpreferences.js#L124

it is a perfect candidate for being unit tested (the return value is based only of its parameters), let's create a set of test cases for it with the most common inputs and expected outputs.

[rpl]

The return type flowtype annotation is missing from this function.

[rpl]

it looks like this should be customPrefs = {} like in configureProfile.

[coveralls]

Coverage decreased (-0.2%) to 99.842% when pulling 6243455 on saintsebastian:fix-88 into 9e5f7af on mozilla:master.

[saintsebastian]

The biggest thing left to is tests for changes I've made to program.execute() (I am a bit lost on what's the best place and approach to do it, probably program.main()) and clean up in some tests I've missed. Let me know if you find anything else

[coveralls]

Coverage decreased (-0.2%) to 99.842% when pulling 56127b2 on saintsebastian:fix-88 into 88a8a04 on mozilla:master.

[rpl]

Hi @saintsebastian,
good job!
I've added some comments related to:

• minor changes related to the coding style
• some change needed to pass the eslint step
• a proposed small rewrite of the coerceCLICustomPreference, which passes the flowtype checks correctly

[rpl]

this should be reformatted into:

import {
  getPrefs, coerceCLICustomPreference,
} from '../../../src/firefox/preferences';

[rpl]

merge this line with the line above (no need to split it into two lines if the line is not longer than the eslint limit, which is set to 80 columns)

[rpl]

This tests currently fails because there should be a single array parameter instead of two parameters, also it should be formatted as:

      let prefs = coerceCLICustomPreference([
        'valid.preference=true', 'valid.preference2=false',
      ]);

[rpl]

It would be better to use a more specific chai assertion for this, e.g.:

assert.isObject(prefs);

(You can get the list, and related docs, of the available chai assertions here: http://chaijs.com/api/assert/)

[rpl]

This should also use assert.isObject as the above.

[rpl]

no need to test the default empty ({}) customPrefs here

[rpl]

is this change needed?

[rpl]

this is a debugging leftover ;-)

[rpl]

To make flow happy about the type checking of this function, it needs a small rewrite:

export function coerceCLICustomPreference(
  cliPrefs: string | Array<string>
): FirefoxPreferences {
  let customPrefs = {};

  if (!Array.isArray(cliPrefs)) {
    cliPrefs = [cliPrefs];
  }

  for (let pref of cliPrefs) {
    let [key, value] = pref.split('=');
    // Check the property key.
    if (/[^\w{@}.-]/.test(key)) {
      throw new UsageError(`Invalid custom preference name: ${key}`);
    }

    if (value === `${parseInt(value)}`) {
      value = parseInt(value, 10);
    } else if (value === 'true' || value === 'false') {
      value = (value === 'true');
    }

    customPrefs[`${key}`] = value;
  }

  return customPrefs;
}

The function esplicitly return a FirefoxPreferences type, and flow is able using its type inference capability that the for...of loop produce an object and its properties have the expected value types (boolean, number or string), which is the definition of the type FirefoxPreferences.

[rpl]

The JSON.stringify "pretty print" mode is usually achieved with: JSON.stringify(customPrefs, null, 2)

Also, how about reformatting the above two lines into:

const customPrefsStr = JSON.stringify(...);
log.info(`Setting custom Firefox preferences: ${customPrefsStr}`);

[saintsebastian]

Sorry for that mess leftover in tests from a time when customPrefs were not an optional parameter.

[coveralls]

Coverage remained the same at 100.0% when pulling 5346a6a on saintsebastian:fix-88 into 88a8a04 on mozilla:master.

[rpl]

Hi @saintsebastian,
in this round of review comments there are mostly small nits/typos! 🎉

The only real change required is the one related to "skip from the custom preferences, the preferences that are needed to make 'web-ext run' to work correctly".

Great job, this wasn't a simple one.

[rpl]

nit, we can probably remove this comment, the function name is already pretty descriptive.

[rpl]

nit, also this comment is probably redundant with the error message below (which makes pretty clear the goal of the tested regular expression), we can probably remove it and leave an empty line.

[rpl]

nit, this line can probably use a comment (something like, "Rename the simplified command line option name pref to the option name used internally.").

[rpl]

Given that customPrefs is defaulted to {}, I think that we can remove customPrefs from here and line 234.

[rpl]

it would be probably nice to also check that one of the default preferences has been set in the prefFile when there are customPrefs.

[rpl]

nit, convert => converts (and maybe rephrased to make it clear that it is an "array of --pref cli option values")

[rpl]

nit, convert => converts

[rpl]

how about "convert a single --pref cli option value from string to object"?
(and if we rephrase this, we should also rephase the other test case descriptions in the coerceCLICustomPreference group)

[rpl]

nit, typo: prefernces => preferences

how about rephrase it into: "converts --prefs cli options into an object of type FirefoxPreferences" ?

[rpl]

This test description still contains a typo ;-)

[rpl]

we should also add an assert on the prop2 preference.

[rpl]

I'm wondering if it would be reasonable to skip (and maybe print a warning message) the custom properties that are used internally to make "web-ext run" able to work correctly, in particular the following:

  // Allow remote connections to the debugger.
  'devtools.debugger.remote-enabled': true,
  // Disable the prompt for allowing connections.
  'devtools.debugger.prompt-connection': false,
  // Allow unsigned add-ons.
 'xpinstall.signatures.required': false,

[coveralls]

Coverage remained the same at 100.0% when pulling 9ad8304 on saintsebastian:fix-88 into df188f2 on mozilla:master.

[rpl]

@saintsebastian here is a small number of comments related to some final touches.

[rpl]

This can be defined using const, I would also rename it to make it immediately clear its role (e.g. something around "non overridable pref names" or "blacklisted custom pref names") and move out of this function, in the first part of this module (as an exported module const, which would also be helpful for the related tests).

Also, this array should be formatted like in:

const varName = [
  ..., // max 80 cols
  ..., // go to the new line if the line above is already too long 
  ..., // and add a "dangle comma" at the end (which prevents syntax errors if/when 
  ..., // a new element is appended).  
];

[rpl]

we can use continue; after the log.warn above so that we don't need an else block.

[rpl]

log.warn is probably better here.
I would also use a shorter message, something like '${key}' preference is used internally and it cannot be customized..

[rpl]

nit, add an empty line between line 96 and this comment, and add a space between // and Command (and a period, ., at the end of the comment).

[rpl]

👍 perfect!

[rpl]

how about exporting the array of the non overridable preferences from the preferences module, so that we can test that none of that array of preferences can be overridden?

[rpl]

👍

[rpl]

This test description still contains a typo ;-)

[coveralls]

Coverage remained the same at 100.0% when pulling 2e2dec0 on saintsebastian:fix-88 into df188f2 on mozilla:master.

[rpl]

@saintsebastian we are down to three more changes now!

[rpl]

We should change this assertion so that it use the specific chai assertion and it prints an helpful message if it fails:

assert.isUndefined(prefs[pref], `${pref} should be undefined`);

[rpl]

we should use let here instead of var:

for (let pref of ...) {

}

[rpl]

assert.equal(prefs['valid.preference'], 455)

[coveralls]

Coverage remained the same at 100.0% when pulling e6754ed on saintsebastian:fix-88 into df188f2 on mozilla:master.

[rpl]

@saintsebastian 👍 Great job!

r+

@kumar303 This should be ready to go!

[kumar303]

Thanks Elvina! I'm excited for this feature.