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.

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: Adds binaryArgs to pass additional flags to Firefox binary #1563

Merged
merged 10 commits into from
Mar 29, 2019
3 changes: 3 additions & 0 deletions src/cmd/run.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ export type CmdRunParams = {|
sourceDir: string,
startUrl?: Array<string>,
target?: Array<string>,
args?: Array<string>,

// Android CLI options.
adbBin?: string,
Expand Down Expand Up @@ -77,6 +78,7 @@ export default async function run(
adbPort,
adbDevice,
firefoxApk,
args,
}: CmdRunParams,
{
buildExtension = defaultBuildExtension,
Expand Down Expand Up @@ -107,6 +109,7 @@ export default async function run(
extensions: [{sourceDir, manifestData}],
keepProfileChanges,
startUrl,
args,
desktopNotifications,
};

Expand Down
1 change: 1 addition & 0 deletions src/extension-runners/base.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ export type ExtensionRunnerParams = {|
profilePath?: string,
keepProfileChanges: boolean,
startUrl: ?string | ?Array<string>,
args?: Array<string>,

// Common injected dependencies.
desktopNotifications: typeof defaultDesktopNotifications,
Expand Down
49 changes: 20 additions & 29 deletions src/extension-runners/firefox-android.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,19 @@ import type {

const log = createLogger(__filename);

const ignoredParams = {
profilePath: '--profile-path',
keepProfileChanges: '--keep-profile-changes',
browserConsole: '--browser-console',
preInstall: '--pre-install',
startUrl: '--start-url',
args: '--args',
};

const getIgnoredParamsWarningsMessage = (optionName) => {
return `The Firefox for Android target does not support ${optionName}`;
};

export type FirefoxAndroidExtensionRunnerParams = {|
...ExtensionRunnerParams,

Expand Down Expand Up @@ -261,35 +274,13 @@ export class FirefoxAndroidExtensionRunner {
}

printIgnoredParamsWarnings() {
if (this.params.profilePath) {
log.warn(
'Firefox for Android target does not support custom profile paths.'
);
}

if (this.params.keepProfileChanges) {
log.warn(
'Firefox for Android target does not support --keep-profile-changes.'
);
}

if (this.params.browserConsole) {
log.warn(
'Firefox for Android target does not support --browser-console option.'
);
}

if (this.params.preInstall) {
log.warn(
'Firefox for Android target does not support --pre-install option.'
);
}

if (this.params.startUrl) {
log.warn(
'Firefox for Android target does not support --start-url option.'
);
}
Object.keys(ignoredParams).forEach((ignoredParam) => {
if (this.params[ignoredParam]) {
log.warn(
getIgnoredParamsWarningsMessage(ignoredParams[ignoredParam])
);
}
});
}

async adbDevicesDiscoveryAndSelect() {
Expand Down
7 changes: 6 additions & 1 deletion src/extension-runners/firefox-desktop.js
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,7 @@ export class FirefoxDesktopExtensionRunner {
startUrl,
firefoxApp,
firefoxClient,
args,
} = this.params;

const binaryArgs = [];
Copy link
Member

Choose a reason for hiding this comment

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

In this version of the PR the arguments are still prepended instead of appended. Restore the binaryArgs variable and use binaryArgs.push(...args); before passing it to firefoxApp.run.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, looks like I misunderstood you. Does it look fine now - 28cb262?

Expand All @@ -231,8 +232,12 @@ export class FirefoxDesktopExtensionRunner {
}
}

if (args) {
binaryArgs.push(...args);
}

this.runningInfo = await firefoxApp.run(this.profile, {
firefoxBinary, binaryArgs,
firefoxBinary, args: binaryArgs,
});

this.runningInfo.firefox.on('close', () => {
Expand Down
7 changes: 3 additions & 4 deletions src/firefox/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ export type FirefoxRunnerParams = {|
'no-remote'?: boolean,
'foreground'?: boolean,
'listen': number,
'binary-args'?: Array<string> | string,
'args'?: Array<string> | string,
Copy link
Member

Choose a reason for hiding this comment

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

You cannot simply rename binary-args to args here, because this is the API to an external library: https://github.com/mozilla-jetpack/node-fx-runner/blob/71e3848b6adac1054829391c826fd88cfb28f621/lib/run.js#L42-L48

Revert all changes in this file (and use binaryArgs instead of args: binaryArgs in the other file).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, so just keep args only for cli? all other code will remain as is?

Copy link
Member

Choose a reason for hiding this comment

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

Yes. The external interface of web-ext run will use args, and binary-args is only used internally, imposed by the contract of the fx-runner interface.

'env'?: {
[key: string]: string
},
Expand Down Expand Up @@ -122,7 +122,6 @@ export type FirefoxRunOptions = {|
fxRunner?: FirefoxRunnerFn,
findRemotePort?: RemotePortFinderFn,
firefoxBinary?: string,
binaryArgs?: Array<string>,
args?: Array<any>,
|};

Expand All @@ -134,7 +133,7 @@ export async function run(
{
fxRunner = defaultFxRunner,
findRemotePort = defaultRemotePortFinder,
firefoxBinary, binaryArgs,
firefoxBinary, args,
}: FirefoxRunOptions = {}
): Promise<FirefoxInfo> {

Expand All @@ -143,9 +142,9 @@ export async function run(
const remotePort = await findRemotePort();

const results = await fxRunner({
args,
// if this is falsey, fxRunner tries to find the default one.
'binary': firefoxBinary,
'binary-args': binaryArgs,
// This ensures a new instance of Firefox is created. It has nothing
// to do with the devtools remote debugger.
'no-remote': true,
Expand Down
6 changes: 6 additions & 0 deletions src/program.js
Original file line number Diff line number Diff line change
Expand Up @@ -503,6 +503,12 @@ Example: $0 --help run.
demandOption: false,
type: 'boolean',
},
'args': {
alias: ['arg'],
describe: 'Additional CLI options passed to the Browser binary',
demandOption: false,
type: 'array',
},
// Firefox for Android CLI options.
'adb-bin': {
describe: 'Specify a custom path to the adb binary',
Expand Down
2 changes: 2 additions & 0 deletions tests/unit/test-cmd/test.run.js
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ describe('run', () => {
firefox: '/path/to/custom/bin/firefox',
pref: {'my.custom.pref': 'value'},
firefoxProfile: '/path/to/custom/profile',
args: ['-headless=false'],
};

await cmd.run(runOptions);
Expand All @@ -130,6 +131,7 @@ describe('run', () => {
firefoxBinary: runnerParams.firefoxBinary,
customPrefs: runnerParams.customPrefs,
firefoxProfile: runnerParams.profilePath,
args: runnerParams.args,
}, expectedRunnerParams);
assert.equal(runnerParams.extensions.length, 1);
assert.equal(runnerParams.extensions[0].sourceDir, cmd.argv.sourceDir);
Expand Down
12 changes: 9 additions & 3 deletions tests/unit/test-extension-runners/test.firefox-android.js
Original file line number Diff line number Diff line change
Expand Up @@ -845,7 +845,7 @@ describe('util/extension-runners/firefox-android', () => {
{
params: {profilePath: '/fake/dir'},
expectedMessage: (
/Android target does not support custom profile paths/
/Android target does not support --profile-path/
),
},
{
Expand All @@ -863,13 +863,19 @@ describe('util/extension-runners/firefox-android', () => {
{
params: {preInstall: true},
expectedMessage: (
/Android target does not support --pre-install option/
/Android target does not support --pre-install/
),
},
{
params: {startUrl: 'http://fake-start-url.org'},
expectedMessage: (
/Android target does not support --start-url option/
/Android target does not support --start-url/
),
},
{
params: {args: ['-headless=false']},
expectedMessage: (
/Android target does not support --args/
),
},
];
Expand Down
20 changes: 14 additions & 6 deletions tests/unit/test-extension-runners/test.firefox-desktop.js
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ describe('util/extension-runners/firefox-desktop', () => {
}));
});

async function testBinaryArgs(extensionRunnerParams, expectedBinaryArgs) {
async function testArgs(extensionRunnerParams, expectedArgs) {
const {params} = prepareExtensionRunnerParams({
params: {
...extensionRunnerParams,
Expand All @@ -172,36 +172,44 @@ describe('util/extension-runners/firefox-desktop', () => {
params.firefoxApp.run,
sinon.match.any,
sinon.match.has(
'binaryArgs',
sinon.match.array.deepEquals(expectedBinaryArgs)
'args',
Copy link
Member

Choose a reason for hiding this comment

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

This one should also be binaryArgs.

sinon.match.array.deepEquals(expectedArgs)
)
);
}

it('passes -jsconsole when --browser-console is specified', async () => {
await testBinaryArgs({
await testArgs({
browserConsole: true,
}, [
'-jsconsole',
]);
});

it('passes single url parameter to Firefox when specified', async () => {
await testBinaryArgs({
await testArgs({
startUrl: 'url1',
}, [
'--url', 'url1',
]);
});

it('passes multiple url parameters to Firefox when specified', async () => {
await testBinaryArgs({
await testArgs({
startUrl: ['url1', 'url2'],
}, [
'--url', 'url1', '--url', 'url2',
]);
});

it('passes args to Firefox', async () => {
await testArgs({
args: ['-headless=true', '-jsconsole'],
}, [
'-headless=true', '-jsconsole',
]);
});

it('passes a custom Firefox profile when specified', async () => {
const {params} = prepareExtensionRunnerParams({
params: {
Expand Down
7 changes: 3 additions & 4 deletions tests/unit/test-firefox/test.firefox.js
Original file line number Diff line number Diff line change
Expand Up @@ -138,12 +138,11 @@ describe('firefox', () => {

it('passes binary args to Firefox', () => {
const fxRunner = createFakeFxRunner();
const binaryArgs = '--safe-mode';
return runFirefox({fxRunner, binaryArgs})
const args = '--safe-mode';
return runFirefox({fxRunner, args})
.then(() => {
sinon.assert.called(fxRunner);
assert.equal(fxRunner.firstCall.args[0]['binary-args'],
binaryArgs);
assert.equal(fxRunner.firstCall.args[0].args, args);
});
});

Expand Down