Closed Bug 1251042 Opened 8 years ago Closed 8 years ago

Enable mochitests for extensions to run on Android

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set
normal

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Iteration:
48.2 - Apr 4
Tracking Status
firefox48 --- fixed

People

(Reporter: aswan, Assigned: mattw)

References

Details

(Whiteboard: triaged)

Attachments

(1 file)

Tests for chrome.downloads.* are currently disabled on Android.
This bug is to track figuring out what we can support on Android and re-enabling tests as appropriate.
Flags: blocking-webextensions?
Whiteboard: triaged
This isn't specific to downloads. All WebExtension tests are currently disabled on Android, because none of them work.
Flags: blocking-webextensions? → blocking-webextensions-
Summary: implement chrome.downloads.* on Android → Run extensions chrome mochitests on Android
Assignee: nobody → mwein
Iteration: --- → 48.2 - Apr 4
This change fixes an issue that was preventing all mochitests from running successfully on Android.  After this fix, some still fail for various reasons, such as depending on APIs that are only implementing for the browser.  I disabled the failing tests and created a follow up bug to track fixing them - https://bugzilla.mozilla.org/show_bug.cgi?id=1258975.
Comment on attachment 8733821 [details]
MozReview Request: Bug 1251042: Enable mochitests for extensions to run on Android. r=nalexander

https://reviewboard.mozilla.org/r/41981/#review38521

lgtm, assuming that you're actually green.  This commit isn't the last (failing) try build, so I assume this is actually green...

::: mobile/android/chrome/content/content.js:109
(Diff revision 1)
>  
>  addMessageListener("RemoteLogins:fillForm", function(message) {
>    LoginManagerContent.receiveMessage(message, content);
>  });
> +
> +ExtensionContent.init(this);

When reading this, I was concerned about the load/performance impact of this.  I convinced myself that right now, `init` does very little and won't impact perf.  However, there's no guarantee over time.

Consider commenting here or in `init` about this, if you feel it's likely this has perf/load time implications.

::: mobile/android/installer/package-manifest.in:313
(Diff revision 1)
>  @BINPATH@/components/storage-mozStorage.js
>  @BINPATH@/components/crypto-SDR.js
>  @BINPATH@/components/NetworkGeolocationProvider.manifest
>  @BINPATH@/components/NetworkGeolocationProvider.js
>  @BINPATH@/components/extensions.manifest
> +@BINPATH@/components/utils.manifest

Reading this, I evaluated the likely APK size increase.  Seems tiny -- just a small single JS file -- can you confirm?

::: toolkit/components/extensions/test/mochitest/mochitest.ini:2
(Diff revision 1)
>  [DEFAULT]
> -skip-if = os == 'android' || buildapp == 'mulet'
> +skip-if = buildapp == 'mulet' || asan

This `asan` seem orthogonal to the rest of the patch.  Explain?
Attachment #8733821 - Flags: review?(nalexander) → review+
https://reviewboard.mozilla.org/r/41981/#review38521

> When reading this, I was concerned about the load/performance impact of this.  I convinced myself that right now, `init` does very little and won't impact perf.  However, there's no guarantee over time.
> 
> Consider commenting here or in `init` about this, if you feel it's likely this has perf/load time implications.

We also call this for every frame loader on desktop and b2g, so it's already designed to do as little as possible until some functionality is necessary. If anything, it should become more efficient in the future, rather than less.

> This `asan` seem orthogonal to the rest of the patch.  Explain?

This looks like a merge botch. We only re-enabled the ASAN tests a few days ago.
Summary: Run extensions chrome mochitests on Android → Enable mochitests for extensions to run on Android
I had to disable a few more tests, but the try build is now green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba252d458d0f.
Comment on attachment 8733821 [details]
MozReview Request: Bug 1251042: Enable mochitests for extensions to run on Android. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41981/diff/1-2/
Attachment #8733821 - Attachment description: MozReview Request: Bug 1251042 Make mochitests work on Android. r?nalexander f?kmag → MozReview Request: Bug 1251042 Make mochitests work on Android. r=nalexander
Comment on attachment 8733821 [details]
MozReview Request: Bug 1251042: Enable mochitests for extensions to run on Android. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41981/diff/2-3/
Matt, there are also chrome mochitests in toolkit/components/extensions (see test/mochitest/chrome.ini) that should be enabled (or attempted) as part of this bug.
Oh thanks, I enabled them and they all pass except for one.  The one that fails uses `browser.tabs` which I think is currently only implemented in the browser.  I created Bug 1258975 for tracking the tests that need to be fixed for various reasons and added this one to the list.
Needs rebasing on top of fx-team tip.
Keywords: checkin-needed
Comment on attachment 8733821 [details]
MozReview Request: Bug 1251042: Enable mochitests for extensions to run on Android. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41981/diff/3-4/
Attachment #8733821 - Attachment description: MozReview Request: Bug 1251042 Make mochitests work on Android. r=nalexander → MozReview Request: Bug 1251042: Enable mochitests for extensions to run on Android. r=nalexander try: -b o -p android-x86,android-api-15 -u mochitest
Comment on attachment 8733821 [details]
MozReview Request: Bug 1251042: Enable mochitests for extensions to run on Android. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41981/diff/4-5/
Attachment #8733821 - Attachment description: MozReview Request: Bug 1251042: Enable mochitests for extensions to run on Android. r=nalexander try: -b o -p android-x86,android-api-15 -u mochitest → MozReview Request: Bug 1251042: Enable mochitests for extensions to run on Android. r=nalexander
https://hg.mozilla.org/mozilla-central/rev/e1f49f739dfd
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Matthew, could you explain to me why what landed (not what was reviewed) changed toolkit/components/passwordmgr?
Flags: needinfo?(mwein)
I initially thought this change was affecting those tests, but I don't believe it is now.  So, I think those changes specific to passwordmng are a mistake. The changes consist of adding a few comments and disabling a test on android.

I can create a follow up to remove those changes if you think that's okay.  Otherwise, we could back it out and then I'll remove those changes.
Flags: needinfo?(mwein)
*passwordmgr
(In reply to Matthew Wein [:mattw] from comment #25)
> Otherwise, we could back it out and then I'll remove those changes.

OK, I just backed out the password manager changes
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.