fix: Enable extensions to load on android build variants

[MikeM711]

This PR fixes #1891 to add support for extensions loaded on android build variants.

Tested using:

web-ext run -t firefox-android --android-device=<device ID here> --firefox-apk=org.mozilla.reference.browser.debug --firefox-apk-component=org.mozilla.reference.browser.BrowserActivity

web-ext run -t firefox-android --android-device=<device ID here> --firefox-apk=org.mozilla.fenix.performancetest --firefox-apk-component=org.mozilla.fenix.HomeActivity

[coveralls]

Coverage remained the same at 100.0% when pulling b61d311 on MikeM711:pr/load-extensions-on-android-build-variants into a58280d on mozilla:master.

[Rob--W]

Thanks. Once this PR is merged, do you consider it to fully resolve #1891, or would you like to also explore whitelists? (If so, it's probably better to file a new issue that summarizes the discussion so far).

[Rob--W]

Prepend a comment:

// adb expands the following to: `${apk}/${apk}.${apkComponent}`

[MikeM711]

Just to be sure, would you like this comment before the let component; line?

[Rob--W]

It kind of applies to every line indeed. Let's modify the apkComponent variable if needed and add the comment before component, like this:

if (!apkComponent) {
  apkComponent = ".App";
} else if (!apkComponent.includes(".")) {
  apkComponent = '.' + apkComponent;
}
// if `apkComponent` starts with a '.', then adb will expand
// the following to: `${apk}/${apk}.${apkComponent}`
const component = `${apk}/${apkComponent}`;

[MikeM711]

That looks really good - define the component after the apkComponent check, I like it

[MikeM711]

Sure! We can take a look at whitelists.

If I'm understanding correctly, would whitelisting be similar to writing:

web-ext run -t firefox-android --android-device=<device ID here>

and receiving a list of packages to choose from? And using that idea to show a list of --firefox-apk-component options?

[Rob--W]

Sure! We can take a look at whitelists.

If I'm understanding correctly, would whitelisting be similar to writing:

web-ext run -t firefox-android --android-device=<device ID here>

and receiving a list of packages to choose from? And using that idea to show a list of --firefox-apk-component options?

No, .App should still be used by default (unless that is somehow not providing the default activity for testing). web-ext run should ideally not require parameters if it can find reasonable defaults on its own. In #1891 there was some discussion on performancetest and how it doesn't work on debug builds; the hardcoded list could help with resolving that. But that's something to discuss in a separate issue, not this PR. Otherwise the discussion is going to be scattered over three issues (this issue, #1981 and the new issue to file), which makes it difficult to follow.

[Rob--W]

Hi @MikeM711 , could you address the review feedback from before?

We plan to release a new update on Monday or Tuesday, and if you are able to resolve those comments by then, then this feature can be included in the next update.

[MikeM711]

Hi Rob, apologies for the wait.
I will be modifying that apkComponent, testing that out, and adding the extra comments after work (5:00PM EST), definitely expect an update today.

[Rob--W]

Thanks!

[MikeM711]

No problem! Thanks @Rob--W and also @jonalmeida for the help.

Rob - is exploring whitelists in a separate issue still something you would like to do? I'd be happy to lend a hand if you still think there is some merit to the idea.

[Rob--W]

Rob - is exploring whitelists in a separate issue still something you would like to do? I'd be happy to lend a hand if you still think there is some merit to the idea.

I'm positive towards a patch to support debug builds without requiring a fully qualified value in --apk-component. We already have some hardcoded app IDs in

web-ext/src/util/adb.js

Lines 120 to 124 in e604112

	line.startsWith('org.mozilla.fennec') ||
	line.startsWith('org.mozilla.fenix') ||
	line.startsWith('org.mozilla.geckoview') ||
	line.startsWith('org.mozilla.firefox') ||
	line.startsWith('org.mozilla.reference.browser')

, and I think that the patch would probably refactor the code by moving that hardcoded list elsewhere, and then be reused to improve the app detection and generate a good candidate for debug build detection.

[MikeM711]

Great, I'll be filing a new issue for that under "feature request".

Before I begin a new issue, I think it would be best to make sure it starts off on the right foot. Just making sure, we would like the expected behavior to go from something like this

--firefox-apk=org.mozilla.fenix.performancetest --firefox-apk-component=org.mozilla.fenix.HomeActivity

to this?

--firefox-apk=org.mozilla.fenix.performancetest --firefox-apk-component=HomeActivity

Just making sure I'm understanding the vocab properly.

[Rob--W]

@MikeM711 Yes, your understanding is correct.