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: Support Firefox for Android via --target=firefox-android and related CLI options #868

Merged
merged 42 commits into from Dec 20, 2017

Conversation

Projects
None yet
5 participants
@rpl
Copy link
Member

rpl commented Mar 17, 2017

This PR contains an initial prototype of "web-ext run" support on Firefox for Android and it should fix #737 by providing mostly the same "addon developer experience" and features supported by "web-ext run" on Firefox Desktop (e.g. run Firefox for Android in a temporary Firefox profile, watch the sources and then rebuild and reinstall the addon on changes, show a desktop notification when the build or install is failing with errors).

The current implementation already passes the eslint and flow type checks, and it doesn't break any of the existent tests, and I'm going to create some additional tests for the android support module asap.

In the meantime, you can watch the following demo screencast and/or build web-ext from this branch if you want to use it immediately and provide some preliminary feedback, that will be very much appreciated :-)

https://youtu.be/C_R1C2s3AWQ

Testing a WebExtension on Android is never been so easy!
\o/ Enjoy!

TODO List

  • refactor this branch on top of #962
  • waiting for #962 to be ready to land
  • add new tests to cover the new extension runner and the new code in cmd/run
  • choose a random tcp port to be used for the "remote debugging adb forwarding"
  • ensure that the unit tests are as fast as possible
  • speed up the android extension runner as much as possible (e.g. by running some
    of the setup steps in parallel)

Followups List

  • #985 - ensure that when 'web-ext run' is configured to run the extension on multiple targets (e.g. Firefox Desktop and Firefox Android), the logged messages (and errors) makes clear which of the target the message (or error) is coming from

@rpl rpl force-pushed the rpl:feature/support-run-cmd-on-android branch from 8735b18 to 3849731 Mar 17, 2017

@rpl

This comment has been minimized.

Copy link
Member

rpl commented Mar 17, 2017

The travis build is currently failing because of nsp check detecting a vulnerability in a library included by a dependency of adbkit (the nodejs library used by this PR to interact with adb):

https://travis-ci.org/rpl/web-ext/jobs/212115440#L1234-L1237

I've reported this issue to the upstream in openstf/adbkit#67

@rpl rpl force-pushed the rpl:feature/support-run-cmd-on-android branch from 3849731 to 2515845 Mar 23, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 23, 2017

Coverage Status

Coverage decreased (-7.6%) to 92.423% when pulling 2515845 on rpl:feature/support-run-cmd-on-android into b5b38fc on mozilla:master.

@kumar303

This comment has been minimized.

Copy link
Member

kumar303 commented Mar 23, 2017

This is very cool! I only took a quick look so here are some initial thoughts.

This is the first time we will be introducing a new run target. In theory, one should be able to say "run my add-on in both desktop Firefox and Firefox for Android" and have it install/run in both places simultaneously. This would let you take advantage of the auto-reloader while testing cross-platform functionality. Such a use case would also apply to running an add-on in both Firefox and Chrome. Should we try and work towards this goal in this patch? It could come later too.

To achieve multiple targets like this, you'd probably want to fold your code into the already existing abstraction around running and watching/reloading. That code is a bit hairy though.

The command line option should probably signify "firefox" in some way since Chrome for Android may also support add-ons in the future. So maybe web-ext run --firefox-mobile or --firefox-android ? We may also want to consider handling multiple targets like web-ext run --target firefox firefox-android chrome (i.e. array type option).

@shatur

This comment has been minimized.

Copy link

shatur commented Mar 26, 2017

Hey!
Out of curiosity, I tried to use this really cool feature. But I am unable to load my web-extension on fennec on a real device(tried it on multiple devices). Then I tried it on emulator device and it worked fine. So, @rpl have you tried it on a real device or this issue is on my end.

I am really sorry if this is not the right place to ask this question.

@rpl

This comment has been minimized.

Copy link
Member

rpl commented Mar 26, 2017

@shatur Thanks for trying this PR, I didn't tried it on a real device yet (I was trying to ensure that it will not be destructive first ;-)), did the issue show any visible error (e.g. adb logcat)?

Thanks again for your help.

@rpl

This comment has been minimized.

Copy link
Member

rpl commented Mar 26, 2017

Hey @kumar303, thanks for this first round of comments, I like the idea of preparing the CLI options to be able to turn "web-ext run" into a multi-target command.

Also, my initial intention was to add the android support into the cmd/run.js module, but I had the immediate feeling that it needs a refactoring first and so I though to defer the refactoring a bit by starting with creating a new module (but preventing any duplication of shared feature between the two) and, so that we could take a look at the different workflows/needs of the two targets in this PR and then refactor the two modules accordingly (possibly before landing this feature).

I'll take a look at both asap.

(Another reason why I deferred the refactoring is that we have a couple of pending PRs from contributors that would be affected by a refactoring on the "./cmd/run.js" module, and so I opted to wait a bit, so that I can rebase my change on top of them and start the refactoring from there).

@shatur

This comment has been minimized.

Copy link

shatur commented Mar 29, 2017

Hey @rpl!
Did a little more digging, and I think the problem I am facing is due to my rooted device. I tried this PR on 4 devices, out of which two were rooted devices. This PR works really fine on non-rooted device:-). But on rooted device it gives me this error https://pastebin.mozilla.org/8983546. I will update you if I find something new!

@rpl rpl force-pushed the rpl:feature/support-run-cmd-on-android branch 2 times, most recently from 90ddaba to 77d7795 Jun 12, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 12, 2017

Coverage Status

Coverage decreased (-13.9%) to 86.053% when pulling 77d7795 on rpl:feature/support-run-cmd-on-android into 44d5afb on mozilla:master.

@rpl rpl force-pushed the rpl:feature/support-run-cmd-on-android branch from 77d7795 to 74dc926 Jun 22, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 22, 2017

Coverage Status

Coverage decreased (-14.1%) to 85.878% when pulling 74dc926 on rpl:feature/support-run-cmd-on-android into f78698c on mozilla:master.

@rpl

This comment has been minimized.

Copy link
Member

rpl commented Jun 22, 2017

@shatur my apologies for not being able to come back to this sooner (it turned out that it would have been better to refactor the "cmd/run" module before introducing these changes, and so, now that we have merged the refactoring PR in #962, I started to look actively into this again).

I suspect that the issue could be something related to something that is installed only on the rooted devices and that can prevent it from working correctly (I did most of the testing while working on this on a rooted emulator and it didn't have any issue, and so I think that the reason is not that the devices are "rooted" but something that is installed only on the "rooted" devices used to test it).

Another possible issue can be related to the specific Android versions (the emulator I used was an Android 4.3 AOSP), I'll try to put together a bit more variegated testing environment before landing it.

also, thank you a lot for testing it on some non-rooted devices, that was definitely an important scenario, that I didn't have tested yet this branch on.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 23, 2017

Coverage Status

Coverage decreased (-12.4%) to 87.595% when pulling 56624e3 on rpl:feature/support-run-cmd-on-android into f78698c on mozilla:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 23, 2017

Coverage Status

Coverage decreased (-12.2%) to 87.786% when pulling 31550fe on rpl:feature/support-run-cmd-on-android into f78698c on mozilla:master.

@rpl rpl force-pushed the rpl:feature/support-run-cmd-on-android branch from 31550fe to 9c13336 Jun 23, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 23, 2017

Coverage Status

Coverage decreased (-11.8%) to 88.168% when pulling 9c13336 on rpl:feature/support-run-cmd-on-android into f78698c on mozilla:master.

@rpl rpl force-pushed the rpl:feature/support-run-cmd-on-android branch 2 times, most recently from 42fb0ce to 2254994 Jun 27, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 29, 2017

Coverage Status

Coverage decreased (-4.6%) to 95.407% when pulling 39c1e03 on rpl:feature/support-run-cmd-on-android into b0b6227 on mozilla:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 29, 2017

Coverage Status

Coverage decreased (-4.05%) to 95.954% when pulling e0d9ee5 on rpl:feature/support-run-cmd-on-android into b0b6227 on mozilla:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 29, 2017

Coverage Status

Coverage decreased (-4.05%) to 95.954% when pulling 184803d on rpl:feature/support-run-cmd-on-android into b0b6227 on mozilla:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 29, 2017

Coverage Status

Coverage decreased (-3.7%) to 96.329% when pulling 3dc92b6 on rpl:feature/support-run-cmd-on-android into b0b6227 on mozilla:master.

@rpl rpl force-pushed the rpl:feature/support-run-cmd-on-android branch from 98863b6 to 59403a8 Oct 23, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 23, 2017

Coverage Status

Coverage decreased (-0.9%) to 99.093% when pulling 59403a8 on rpl:feature/support-run-cmd-on-android into 134b795 on mozilla:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 6, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 8d3072e on rpl:feature/support-run-cmd-on-android into 134b795 on mozilla:master.

@rpl rpl requested a review from kumar303 Nov 6, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 6, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling d9ac7c4 on rpl:feature/support-run-cmd-on-android into 134b795 on mozilla:master.

@kumar303
Copy link
Member

kumar303 left a comment

This patch is working great for me. We should ship it asap! Sorry for the delay in re-reviewing this.

My main concern is that building to the artifacts dir might be disruptive for some people. I offered a solution.

My other comments are mostly about cleaning up the console.

ignoreFiles,
asNeeded: false,
// TODO: choose a different artifactsDir for additional safety?
overwriteDest: true,

This comment has been minimized.

@kumar303

kumar303 Dec 2, 2017

Member

When testing it, I see this in the console:

Destination exists, overwriting: /Users/kumar/dev/webextensions-examples/window-manipulator/web-ext-artifacts/window_manipulator-1.0.zip

Shouldn't this be using a temp dir? You could do it like:

{
  artifactsDir: tmpDir.path(),
  ...
}

That's how the sign command does it.


/**
* This module provide an ExtensionRunner subclass that manage an extension executed
* in a Firefox for Desktop instance.

This comment has been minimized.

@kumar303

kumar303 Dec 2, 2017

Member

It should say Firefox for Android, right? I think maybe this comment is a duplicate anyway? I see the correct comment up above.


// Create profile prefs (with enabled remote RDP server), prepare the
// artifacts and temporary directory on the selected device, and
// push the profile preferences to the remote profile dir.

This comment has been minimized.

@kumar303

kumar303 Dec 2, 2017

Member

These detailed comments are helpful, thanks.

await adbUtils.clearArtifactsDir(selectedAdbDevice);
}

// Call all the registered clenaup callbacks.

This comment has been minimized.

@kumar303

kumar303 Dec 2, 2017

Member

typo: cleanup

devices = await adbUtils.discoverDevices();

if (devices.length === 0) {
throw new UsageError('No Android device found through ADB.');

This comment has been minimized.

@kumar303

kumar303 Dec 2, 2017

Member

Would it be helpful to provide some hints? How about this:

No Android device found through ADB. Make sure the device is connected and USB debugging is enabled.


this.artifactsDirMap.delete(deviceId);

await this.runShellCommand(deviceId, [

This comment has been minimized.

@kumar303

kumar303 Dec 2, 2017

Member

It would be nice to precede this with a debug message saying that we are about to delete ${artifactsDir}

await adbClient.push(deviceId, localPath, devicePath)
.then(function(transfer) {
return new Promise((resolve) => {
// TODO(rpl): optionally show progress in the console

This comment has been minimized.

@kumar303

kumar303 Dec 2, 2017

Member

I think it's so fast over USB that we don't need a progress indicator

async setupForward(deviceId: string, remote: string, local: string) {
const {adbClient} = this;

// TODO(rpl): we should use adb.listForwards and reuse the existent one if any (especially

This comment has been minimized.

@kumar303

kumar303 Dec 2, 2017

Member

typo: ...and reuse the existing one if any...

package:org.some.other.software
`;

const fakeSocketFilePrefix = (

This comment has been minimized.

@kumar303

kumar303 Dec 2, 2017

Member

Could you add a comment about this? Did you copy it from a real file?

new Buffer(`
android.permission.READ_EXTERNAL_STORAGE: granted=true
android.permission.WRITE_EXTERNAL_STORAGE: granted=true
`)

This comment has been minimized.

@kumar303

kumar303 Dec 2, 2017

Member

this indent looks off

@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 14, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling a498101 on rpl:feature/support-run-cmd-on-android into f4ff99a on mozilla:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 15, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling d3cc22c on rpl:feature/support-run-cmd-on-android into f4ff99a on mozilla:master.

@rpl rpl requested a review from kumar303 Dec 15, 2017

@kumar303
Copy link
Member

kumar303 left a comment

r+wc

Looking good! I just had a few test cleanup requests.


await runnerInstance.reloadExtensionBySourceDir(
'/non-existent/source-dir'
).then((results) => {

This comment has been minimized.

@kumar303

kumar303 Dec 15, 2017

Member

Since you're using await, this call to then() could be removed.

await runnerInstance.run();

await runnerInstance.reloadAllExtensions()
.then((results) => {

This comment has been minimized.

@kumar303

kumar303 Dec 15, 2017

Member

It looks like you can remove this call to then() as well

`;

// Enable chai-as-promised plugin.
chai.use(chaiAsPromised);

This comment has been minimized.

@kumar303

kumar303 Dec 15, 2017

Member

Is this making a global change to the test suite? If so, I think it would be better in tests/unit/runner.js -- would that work?

@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 18, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling f8f5ccc on rpl:feature/support-run-cmd-on-android into f4ff99a on mozilla:master.

@kumar303
Copy link
Member

kumar303 left a comment

Let's do this!

future-hover-board

@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 20, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling e322205 on rpl:feature/support-run-cmd-on-android into 284d4c6 on mozilla:master.

@kumar303 kumar303 merged commit 1b22d60 into mozilla:master Dec 20, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details

@kumar303 kumar303 changed the title feat: Added Firefox for Android support and --android and --android-device CLI options feat: Support Firefox for Android via --target=firefox-android and related CLI options Jan 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment