872806 - Distribution support for add-ons

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

We should look into supporting a distribution/extensions directory, similarly to how we do search engines.

Comment 1

[:Margaret Leibovic]

On the roadmap for 24, but I haven't started working on a patch for this yet. We may want to just track for 25, although distribution code patches are usually isolated enough that they pose a low risk for uplift.

Comment 2

[:Margaret Leibovic]

So, good news, this actually *just works* if you put add-ons in distribution/extenstions/ in the APK. The XPIs or folders just need to be named with the add-on's (valid) ID, e.g. [e-mail].

Unfortunately, this doesn't work with /system distributions. Maybe we need to look into copying over /system distribution folders to /data the same way we do for APK distributions. Or we need to somehow tell XPIProvider to look somewhere else:
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/XPIProvider.jsm#2319

Comment 3

[:Margaret Leibovic]

Attachment: broken WIP for /system distributions

In theory this would work, but it doesn't because the XPIProvider call to get the distribution directory is happening before we set the "distribution.path" pref. Maybe instead of passing this path from Java, we should just implement parallel logic in DirectoryProvider to return the right directory.

Then we could update the Distribution.getPrefs() to also just go through the directory service to get the correct directory, instead of manually doing it ourselves (we probably should have just done this originally anyway):
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#7450

The one issue with not passing the path is that I would need the package name in JS. Do we have a way of getting that?

Comment 4

[:Margaret Leibovic]

Attachment: patch

This patch makes the DirectoryProvider in charge of telling the Gecko side of things where to look for a distribution, which feels nicer than passing around the path and setting a pref with it.

We could file a follow-up to stop storing the path in Java as well, but that pref is currently used in Distribution.getBookmarks() and I didn't want to mess around with the logic there in this patch.

I also updated our components/moz.build file to not pre-process files that don't need it. I know it's maybe a little scope-creepy for this bug, but I felt to lazy to file a separate bug :)

I tested this patch with a /system distribution and an APK distribution with different add-ons, prefs, and bookmarks. It works in both cases, and it correctly uses the APK distribution over the /system one. I should also look into adding an add-on testcase to testDistribution...

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 770347 [details] [diff] [review]
patch

I really like this! Great improvement!

Removing a SharedPreference also feels good.

Comment 6

[:Margaret Leibovic]

Attachment: (Part 2) Remove unnecessary distribution path shared pref

Here's a patch to remove using the shared pref. This also makes the logic in getBookarks() a bit clearer, since before it wasn't quite as obvious that we were giving the APK distribution priority over the system distribution.

Comment 7

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dd8cea3d849c
https://hg.mozilla.org/integration/mozilla-inbound/rev/46160d052cac

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

And backed out for causing all of these test failures: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=46160d052cac

https://hg.mozilla.org/integration/mozilla-inbound/rev/3e1a142987cd

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

And a followup to clean up the backout because apparently deleting the "<<<<<"s, hg adding the resolved file, committing and pushing isn't enough to actually delete those markers...

https://hg.mozilla.org/integration/mozilla-inbound/rev/08021baad915

Comment 10

[:Margaret Leibovic]

Attachment: (Part 1 v2) Update DirectoryProvider to handle returning distribution location in JS

It turns out the changes to the moz.build file were what was giving me problems. This is looking pretty green now:
https://tbpl.mozilla.org/?tree=Try&rev=c61ef021dd84

I also updated this patch to return the data directory distribution path if the system distribution doesn't exist, just so that it's more consistent with the current/desktop behavior.

Comment 11

[:Margaret Leibovic]

Completely green try run, so I'm landing:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b86d3d23fec
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e4f2cd2eec0

That backout didn't actually back out all of the second patch, which is a bit worrisome. I just re-landed the remaining half of the patch, but it's a good lesson to actually look at backout changesets to make sure they're done properly. This would have been caught if we had tests for /system distributions, but I'm not sure how we would write to the ROM of test devices.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0b86d3d23fec
https://hg.mozilla.org/mozilla-central/rev/3e4f2cd2eec0

Comment 13

[:Margaret Leibovic]

Attachment: roll-up patch for aurora

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: distributions in the system directory can't include add-ons, which is an important feature for partnerships
Testing completed (on m-c, etc.): landed on m-c, tested locally with both APK and system directory distributions
Risk to taking this patch (and alternatives if risky): low-risk, this only affects distributions
String or IDL/UUID changes made by this patch: n/a

Comment 14

[bhavana bajaj [:bajaj]]

(In reply to :Margaret Leibovic from comment #13)
> Created attachment 772119 [details] [diff] [review]
> roll-up patch for aurora
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): n/a
> User impact if declined: distributions in the system directory can't include
> add-ons, which is an important feature for partnerships
> Testing completed (on m-c, etc.): landed on m-c, tested locally with both
> APK and system directory distributions
> Risk to taking this patch (and alternatives if risky): low-risk, this only
> affects distributions
> String or IDL/UUID changes made by this patch: n/a

Why do we ned to uplift this and not let it ride the trains instead ? Am i missing some dependencies ?

Comment 15

[:Margaret Leibovic]

My understanding is that this is an important feature for partnerships, and we want it in the product sooner rather than later. I'm setting needinfo? to Erin to see if she can elaborate on why this should be in 24 as opposed to 25.

Comment 16

[:Margaret Leibovic]

Also setting needinfo? to Karen in case she can help explain why this needs to land in 24 (or if she's okay letting it wait until 25).

Comment 17

[Karen Rudnitski [:kar]]

I am pushing for this to be included in 24 in order to support potential distribution opportunities in a way that is much more scalable and manageable for us. If more colour or context is required, please ping me over email.

Comment 18

[bhavana bajaj [:bajaj]]

(In reply to Karen Rudnitski [:kar] from comment #17)
> I am pushing for this to be included in 24 in order to support potential
> distribution opportunities in a way that is much more scalable and
> manageable for us. If more colour or context is required, please ping me
> over email.

Understood ! Thanks for the information Karen. Given this, it should be good to land.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f3d2f88ce830

Comment 20

[:Margaret Leibovic]

I updated the distribution documentation to include this:
https://wiki.mozilla.org/Mobile/Distribution_Files#Add-ons