feat: Support Firefox for Android via --target=firefox-android and related CLI options #868

rpl · 2017-03-17T13:43:44Z

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 chore: Refactoring out extension runners from the 'cmd/run' module #962
waiting for chore: Refactoring out extension runners from the 'cmd/run' module #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

Prefix logged messages and usage errors when "web-ext run" is managing multiple extension runners #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 · 2017-03-17T15:36:07Z

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

coveralls · 2017-03-23T05:20:13Z

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

kumar303 · 2017-03-23T09:47:32Z

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).

tushar-saini · 2017-03-26T18:54:24Z

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 · 2017-03-26T19:36:23Z

@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 · 2017-03-26T19:48:13Z

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).

tushar-saini · 2017-03-29T16:35:25Z

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!

coveralls · 2017-06-12T15:27:09Z

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

coveralls · 2017-06-22T18:28:42Z

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

rpl · 2017-06-22T18:39:36Z

@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 · 2017-06-23T18:53:38Z

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

coveralls · 2017-06-23T19:49:42Z

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

coveralls · 2017-06-23T20:02:07Z

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

coveralls · 2017-06-29T17:22:47Z

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

coveralls · 2017-06-29T17:55:54Z

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

coveralls · 2017-06-29T18:45:11Z

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

coveralls · 2017-06-29T21:29:52Z

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

coveralls · 2017-10-23T19:31:47Z

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

…helper module.

coveralls · 2017-11-06T12:34:21Z

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

coveralls · 2017-11-06T13:09:15Z

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

kumar303

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.

kumar303 · 2017-12-01T22:32:45Z

src/cmd/run.js

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


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.

kumar303 · 2017-12-01T22:35:45Z

src/extension-runners/firefox-android.js

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


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

kumar303 · 2017-12-01T22:38:53Z

src/extension-runners/firefox-android.js

+
+    // 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.


These detailed comments are helpful, thanks.

kumar303 · 2017-12-01T22:41:19Z

src/extension-runners/firefox-android.js

+      await adbUtils.clearArtifactsDir(selectedAdbDevice);
+    }
+
+    // Call all the registered clenaup callbacks.


typo: cleanup

kumar303 · 2017-12-01T22:46:56Z

src/extension-runners/firefox-android.js

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


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.

kumar303 · 2017-12-01T23:46:36Z

src/util/adb.js

+
+    this.artifactsDirMap.delete(deviceId);
+
+    await this.runShellCommand(deviceId, [


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

kumar303 · 2017-12-01T23:47:29Z

src/util/adb.js

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


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

kumar303 · 2017-12-01T23:48:56Z

src/util/adb.js

+  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


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

kumar303 · 2017-12-02T00:01:14Z

tests/unit/test-util/test.adb.js

+package:org.some.other.software
+`;
+
+const fakeSocketFilePrefix = (


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

kumar303 · 2017-12-02T00:04:08Z

tests/unit/test-util/test.adb.js

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


this indent looks off

…sh zip file

coveralls · 2017-12-14T23:30:31Z

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

coveralls · 2017-12-15T16:46:30Z

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

kumar303

r+wc

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

kumar303 · 2017-12-15T21:37:27Z

tests/unit/test-extension-runners/test.firefox-android.js

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


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

kumar303 · 2017-12-15T21:38:06Z

tests/unit/test-extension-runners/test.firefox-android.js

+         await runnerInstance.run();
+
+         await runnerInstance.reloadAllExtensions()
+           .then((results) => {


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

kumar303 · 2017-12-15T21:41:25Z

tests/unit/test-util/test.adb.js

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


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 · 2017-12-18T16:59:29Z

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

kumar303

Let's do this!

…un-cmd-on-android

coveralls · 2017-12-20T14:38:33Z

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

rpl added the priority: enhancement label Mar 17, 2017

muffinresearch added the state: pull request ready label Mar 17, 2017

rpl force-pushed the feature/support-run-cmd-on-android branch from 8735b18 to 3849731 Compare March 17, 2017 15:18

rpl force-pushed the feature/support-run-cmd-on-android branch from 3849731 to 2515845 Compare March 23, 2017 05:15

rpl mentioned this pull request May 31, 2017

chore: Refactoring out extension runners from the 'cmd/run' module #962

Merged

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

rpl force-pushed the feature/support-run-cmd-on-android branch from 77d7795 to 74dc926 Compare June 22, 2017 18:25

rpl mentioned this pull request Jun 23, 2017

Prefix logged messages and usage errors when "web-ext run" is managing multiple extension runners #985

Open

rpl force-pushed the feature/support-run-cmd-on-android branch from 56624e3 to 31550fe Compare June 23, 2017 19:46

rpl force-pushed the feature/support-run-cmd-on-android branch from 31550fe to 9c13336 Compare June 23, 2017 19:56

rpl force-pushed the feature/support-run-cmd-on-android branch 2 times, most recently from 42fb0ce to 2254994 Compare June 28, 2017 17:38

chore: fixes test failures and flow/eslint errors.

59403a8

rpl force-pushed the feature/support-run-cmd-on-android branch from 98863b6 to 59403a8 Compare October 23, 2017 19:26

kumar303 mentioned this pull request Oct 24, 2017

Allow connecting to already running Firefox when using web-ext run #1100

Open

rpl mentioned this pull request Nov 3, 2017

Are there any concrete plans to start interoperating or incorporating web-ext with Google Chrome, Opera, and other platforms? #1130

Open

rpl added 2 commits November 6, 2017 13:10

chore: created a new util/adb helper module

c008add

chore: refactor firefox-android extension runner to use the ADBUtils …

8d3072e

…helper module.

chore: Minor tweaks based on review comments

d9ac7c4

rpl requested a review from kumar303 November 6, 2017 13:03

kumar303 suggested changes Dec 2, 2017

View reviewed changes

rpl added 5 commits December 14, 2017 16:38

Merge branch 'master' into feature/support-run-cmd-on-android

bc5ca58

chore: apply changes based on last round of review comments

1b68f91

chore: update adbkit dependency to 2.11.0

0d6348a

chore: fix withTempDir usage in android extension runner build and pu…

15a0164

…sh zip file

Merge branch 'master' into feature/support-run-cmd-on-android

a498101

chore: more tweaks based on last round of review comments

d3cc22c

rpl requested a review from kumar303 December 15, 2017 17:27

kumar303 approved these changes Dec 15, 2017

View reviewed changes

chore: minor tweaks based on the final round of review comments

f8f5ccc

kumar303 approved these changes Dec 19, 2017

View reviewed changes

Merge remote-tracking branch 'upstream/master' into feature/support-r…

e322205

…un-cmd-on-android

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

corbindavenport mentioned this pull request Jan 5, 2018

Improve support for Firefox on Android corbindavenport/noplugin#21

Open

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


		this.artifactsDirMap.delete(deviceId);

		await this.runShellCommand(deviceId, [

feat: Support Firefox for Android via --target=firefox-android and related CLI options #868

feat: Support Firefox for Android via --target=firefox-android and related CLI options #868

Conversation

rpl commented Mar 17, 2017 • edited

TODO List

Followups List

rpl commented Mar 17, 2017

coveralls commented Mar 23, 2017

kumar303 commented Mar 23, 2017

tushar-saini commented Mar 26, 2017

rpl commented Mar 26, 2017

rpl commented Mar 26, 2017

tushar-saini commented Mar 29, 2017

coveralls commented Jun 12, 2017

coveralls commented Jun 22, 2017

rpl commented Jun 22, 2017

coveralls commented Jun 23, 2017

coveralls commented Jun 23, 2017

coveralls commented Jun 23, 2017

coveralls commented Jun 29, 2017

coveralls commented Jun 29, 2017

coveralls commented Jun 29, 2017

coveralls commented Jun 29, 2017

coveralls commented Oct 23, 2017

coveralls commented Nov 6, 2017

coveralls commented Nov 6, 2017

kumar303 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 14, 2017

coveralls commented Dec 15, 2017

kumar303 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 18, 2017

kumar303 left a comment

Choose a reason for hiding this comment

coveralls commented Dec 20, 2017

rpl commented Mar 17, 2017 •

edited