-
Notifications
You must be signed in to change notification settings - Fork 344
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
feat: install and run an extension with custom Firefox preferences #658
Changes from all commits
0bcc629
594ceb8
56127b2
4f452af
5346a6a
9ad8304
2e2dec0
e6754ed
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,12 @@ | ||
/* @flow */ | ||
import {WebExtError} from '../errors'; | ||
import {WebExtError, UsageError} from '../errors'; | ||
import {createLogger} from '../util/logger'; | ||
|
||
const log = createLogger(__filename); | ||
export const nonOverridablePreferences = [ | ||
'devtools.debugger.remote-enabled', 'devtools.debugger.prompt-connection', | ||
'xpinstall.signatures.required', | ||
]; | ||
|
||
// Flow Types | ||
|
||
|
@@ -117,3 +123,35 @@ export function getPrefs( | |
...appPrefs, | ||
}; | ||
} | ||
|
||
export function coerceCLICustomPreference( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To make flow happy about the type checking of this function, it needs a small rewrite:
The function esplicitly return a |
||
cliPrefs: string | Array<string> | ||
): FirefoxPreferences { | ||
let customPrefs = {}; | ||
|
||
if (!Array.isArray(cliPrefs)) { | ||
cliPrefs = [cliPrefs]; | ||
} | ||
|
||
for (let pref of cliPrefs) { | ||
let [key, value] = pref.split('='); | ||
|
||
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'); | ||
} | ||
|
||
if (nonOverridablePreferences.includes(key)) { | ||
log.warn(`'${key}' preference cannot be customized.`); | ||
continue; | ||
} | ||
customPrefs[`${key}`] = value; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
|
||
} | ||
|
||
return customPrefs; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,7 @@ import yargs from 'yargs'; | |
import defaultCommands from './cmd'; | ||
import {UsageError} from './errors'; | ||
import {createLogger, consoleStream as defaultLogStream} from './util/logger'; | ||
import {coerceCLICustomPreference} from './firefox/preferences'; | ||
|
||
const log = createLogger(__filename); | ||
const envPrefix = 'WEB_EXT'; | ||
|
@@ -94,6 +95,9 @@ export class Program { | |
const argv = this.yargs.argv; | ||
const cmd = argv._[0]; | ||
|
||
// Command line option (pref) renamed for internal use (customPref). | ||
argv.customPrefs = argv.pref; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit, this line can probably use a comment (something like, "Rename the simplified command line option name |
||
|
||
let runCommand = this.commands[cmd]; | ||
|
||
if (argv.verbose) { | ||
|
@@ -286,6 +290,14 @@ Example: $0 --help run. | |
demand: false, | ||
type: 'boolean', | ||
}, | ||
'pref': { | ||
describe: 'Launch firefox with custom preferences. Lightweight ' + | ||
'alternative to creating custom profile.', | ||
demand: false, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should also add |
||
requiresArg: true, | ||
type: 'string', | ||
coerce: coerceCLICustomPreference, | ||
}, | ||
}) | ||
.command('lint', 'Validate the web extension source', commands.lint, { | ||
'output': { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -235,7 +235,7 @@ describe('firefox', () => { | |
sinon.spy((profile) => Promise.resolve(profile)); | ||
|
||
return firefox.copyProfile(baseProfile.path(), | ||
{configureThisProfile, app}) | ||
{app, configureThisProfile}) | ||
.then((profile) => { | ||
assert.equal(configureThisProfile.called, true); | ||
assert.equal(configureThisProfile.firstCall.args[0], profile); | ||
|
@@ -345,6 +345,22 @@ describe('firefox', () => { | |
} | ||
)); | ||
|
||
it('writes custom preferences', () => withTempProfile( | ||
(profile) => { | ||
const customPrefs = {'extensions.checkCompatibility.nightly': true}; | ||
return firefox.configureProfile(profile, {customPrefs}) | ||
.then((profile) => fs.readFile(path.join(profile.path(), 'user.js'))) | ||
.then((prefFile) => { | ||
// Check for custom pref set by configureProfile(). | ||
assert.include(prefFile.toString(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it would be probably nice to also check that one of the default preferences has been set in the prefFile when there are customPrefs. |
||
'"extensions.checkCompatibility.nightly", true'); | ||
// Check that one of the default preferences is set as well | ||
assert.include(prefFile.toString(), | ||
'"devtools.debugger.remote-enabled", true'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 perfect! |
||
}); | ||
} | ||
)); | ||
|
||
}); | ||
|
||
describe('installExtension', () => { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,8 +2,10 @@ | |
import {describe, it} from 'mocha'; | ||
import {assert} from 'chai'; | ||
|
||
import {WebExtError} from '../../../src/errors'; | ||
import {getPrefs} from '../../../src/firefox/preferences'; | ||
import {WebExtError, UsageError} from '../../../src/errors'; | ||
import { | ||
getPrefs, coerceCLICustomPreference, nonOverridablePreferences, | ||
} from '../../../src/firefox/preferences'; | ||
|
||
|
||
describe('firefox/preferences', () => { | ||
|
@@ -34,4 +36,54 @@ describe('firefox/preferences', () => { | |
|
||
}); | ||
|
||
describe('coerceCLICustomPreference', () => { | ||
|
||
it('converts a single --pref cli option from string to object', () => { | ||
const prefs = coerceCLICustomPreference('valid.preference=true'); | ||
assert.isObject(prefs); | ||
assert.equal(prefs['valid.preference'], true); | ||
}); | ||
|
||
it('converts array of --pref cli option values into object', () => { | ||
const prefs = coerceCLICustomPreference([ | ||
'valid.preference=true', 'valid.preference2=false', | ||
]); | ||
assert.isObject(prefs); | ||
assert.equal(prefs['valid.preference'], true); | ||
assert.equal(prefs['valid.preference2'], false); | ||
}); | ||
|
||
it('converts boolean values', () => { | ||
const prefs = coerceCLICustomPreference('valid.preference=true'); | ||
assert.equal(prefs['valid.preference'], true); | ||
}); | ||
|
||
it('converts number values', () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should also test that strings that contains floats converted into strings (e.g. |
||
const prefs = coerceCLICustomPreference('valid.preference=455'); | ||
assert.equal(prefs['valid.preference'], 455); | ||
}); | ||
|
||
it('converts float values', () => { | ||
const prefs = coerceCLICustomPreference('valid.preference=4.55'); | ||
assert.equal(prefs['valid.preference'], '4.55'); | ||
}); | ||
|
||
it('does not allow certain default preferences to be customized', () => { | ||
const nonChangeablePrefs = nonOverridablePreferences.map((prop) => { | ||
return prop += '=true'; | ||
}); | ||
const prefs = coerceCLICustomPreference(nonChangeablePrefs); | ||
for (let pref of nonChangeablePrefs) { | ||
assert.isUndefined(prefs[pref], `${pref} should be undefined`); | ||
} | ||
}); | ||
|
||
it('throws an error for invalid preferences', () => { | ||
assert.throws(() => coerceCLICustomPreference('*&%£=true'), | ||
UsageError, | ||
'UsageError: Invalid custom preference name: *&%£'); | ||
}); | ||
|
||
}); | ||
|
||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -327,6 +327,21 @@ describe('program.main', () => { | |
}); | ||
}); | ||
|
||
it('converts custom preferences into an object', () => { | ||
const fakeCommands = fake(commands, { | ||
run: () => Promise.resolve(), | ||
}); | ||
return execProgram( | ||
['run', '--pref', 'prop=true', '--pref', 'prop2=value2'], | ||
{commands: fakeCommands}) | ||
.then(() => { | ||
const {customPrefs} = fakeCommands.run.firstCall.args[0]; | ||
assert.isObject(customPrefs); | ||
assert.equal(customPrefs.prop, true); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should also add an assert on the prop2 preference. |
||
assert.equal(customPrefs.prop2, 'value2'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
}); | ||
}); | ||
|
||
}); | ||
|
||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
beforecopyFromUserProfile
so that they are alphabetically ordered?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure! I sorted them in function definition as well for consistency, will apply the same order in tests.