Skip to content

feat: Added fenix support to the firefox-android extension runner #1834

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

Merged
merged 3 commits into from
Feb 11, 2020

Conversation

rpl
Copy link
Member

@rpl rpl commented Feb 10, 2020

This pull request applies the following tweaks to make it easier to test a webextensions on a GeckoView-based browsers:

  • Added Fenix and GeckoView to the auto-detected APK names (so that the user doesn't need to explicitly select the APK if Fenix is the only installed one)
  • Exposed the --adb-discovery-timeout parameter (already supported internally but never exposed to as a cli option, it can be used to change the maximum time to wait for the remote debugging server discovery)
  • Added a new --firefox-apk-component cli option (needed to make it easier to use web-ext run with other custom GeckoView-based apps, e.g. a user may add --firefox-apk-component GeckoViewActivity to run an extension on the geckoview_example)
  • Added a new --fennec-mode boolean parameter (which the user can set to "force disable" the "Fennec compatibility mode") (removed as not strictly needed, we may add it in the future if it may actually help the transition from Fennec to Fenix)
  • Only require the READ_EXTERNAL_STORAGE permission for non-Fennec Firefox for Android browsers (required to be able to access the XPI file from the path where web-ext run has uploaded it on the device, the WRITE_EXTERNAL_STORAGE is only required on Fennec because it is executed on a separate Firefox profile created on the sdcard) (geckoview_example doesn't seem to work without the WRITE_EXTERNAL_STORAGE, this PR keeps the requirement unmodified for now, we may re-evaluate later)

The version of the firefox-android extension runner in this pull request still defaults to use the "Fennec Compatibility mode" for APK files without ".fenix" in their name, the --fennec-mode CLI option is meant to allow the user to explicitly set the "Fennec Compatibility mode" if needed.
Once Fenix would completely replace Fennec in the release channel, the "Fennec Compatibility mode" should be changed to default to false.

web-ext run on Fenix - Screencast

Follow ups on the GeckoView / Fenix side:

@coveralls
Copy link

coveralls commented Feb 10, 2020

Coverage Status

Coverage remained the same at 100.0% when pulling 49652a6 on rpl:feature/web-ext-run-fenix into 4f1d8e5 on mozilla:master.

@rpl rpl requested a review from Rob--W February 10, 2020 11:25
Copy link
Member

@Rob--W Rob--W left a comment

Choose a reason for hiding this comment

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

Does this work with generic GeckoView applications?

if (firefoxApk && firefoxApk.includes('.fenix')) {
return false;
}

Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to detect the application version before assuming Fenix? I.e. if <=68.x, assume Fennec, otherwise Fenix.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is it possible to detect the application version before assuming Fenix? I.e. if <=68.x, assume Fennec, otherwise Fenix.

I looked into it a bit, but I haven't found a reasonable way to do that yet, pm list packages doesn't seem to provide version details for the listed packages.

Personally I would prefer to keep this pull request as simple as possible, so that we may even release it as part of the next minor version (which I haven't published yet).

Nevertheless I'm definitely interested on improving the Fenix support and auto-detection in followups.

Copy link
Member

Choose a reason for hiding this comment

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

Fenix is a moving target, whereas we know what Fennec looks like. In the long term, we should be able to assume Fenix by default, unless.

This is not a blocker for this PR, so you can file a follow-up to address this.

@rpl
Copy link
Member Author

rpl commented Feb 10, 2020

Does this work with generic GeckoView applications?

yes, it should work on other GeckoView-based applications, if they provide a way to enable the "Remote Debugging via USB".

if (firefoxApk && firefoxApk.includes('.fenix')) {
return false;
}

Copy link
Member

Choose a reason for hiding this comment

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

Fenix is a moving target, whereas we know what Fennec looks like. In the long term, we should be able to assume Fenix by default, unless.

This is not a blocker for this PR, so you can file a follow-up to address this.

@rpl
Copy link
Member Author

rpl commented Feb 10, 2020

@Rob--W I added some additional changes to allow web-ext run to also work with the geckoview example (in particular I had to add an additional --firefox-apk-component option to allow a user to specify a different Android component to run).

I have also added back the write permission requirement (at least for now), without it the geckoview example doesn't seem to be able to start successfully (to be honest I'm quite surprised about that issue, but I have to dig a bit more into it to find exactly what is going on).

@rpl
Copy link
Member Author

rpl commented Feb 11, 2020

@Rob--W I've just updated this pull request as we agreed over Matrix (in particular by removing the --fennec-mode cli option, and update the test cases accordingly).

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

Successfully merging this pull request may close these issues.

3 participants