Distribution support for add-ons

RESOLVED FIXED in Firefox 24

Status

()

Firefox for Android
General
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Margaret, Assigned: Margaret)

Tracking

Trunk
Firefox 25
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox24 fixed, firefox25 fixed, fennec25+)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

5 years ago
We should look into supporting a distribution/extensions directory, similarly to how we do search engines.
(Assignee)

Updated

4 years ago
Assignee: nobody → margaret.leibovic
(Assignee)

Comment 1

4 years ago
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.
tracking-fennec: --- → ?
tracking-fennec: ? → 25+
(Assignee)

Comment 2

4 years ago
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. copyprofile@mozilla.org.xpi.

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
(Assignee)

Comment 3

4 years ago
Created attachment 769910 [details] [diff] [review]
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?
(Assignee)

Comment 4

4 years ago
Created attachment 770347 [details] [diff] [review]
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...
Attachment #769910 - Attachment is obsolete: true
Attachment #770347 - Flags: review?(mark.finkle)
Comment on attachment 770347 [details] [diff] [review]
patch

I really like this! Great improvement!

Removing a SharedPreference also feels good.
Attachment #770347 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 6

4 years ago
Created attachment 770388 [details] [diff] [review]
(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.
Attachment #770388 - Flags: review?(mark.finkle)
Attachment #770388 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 7

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd8cea3d849c
https://hg.mozilla.org/integration/mozilla-inbound/rev/46160d052cac
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
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
(Assignee)

Comment 10

4 years ago
Created attachment 771111 [details] [diff] [review]
(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.
Attachment #771111 - Flags: review?(mark.finkle)
(Assignee)

Updated

4 years ago
Attachment #770347 - Attachment is obsolete: true
Attachment #771111 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 11

4 years ago
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.
https://hg.mozilla.org/mozilla-central/rev/0b86d3d23fec
https://hg.mozilla.org/mozilla-central/rev/3e4f2cd2eec0
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
(Assignee)

Comment 13

4 years ago
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
Attachment #772119 - Flags: approval-mozilla-aurora?
(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 ?
(Assignee)

Comment 15

4 years ago
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.
Flags: needinfo?(elancaster)
(Assignee)

Comment 16

4 years ago
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).
Flags: needinfo?(krudnitski)
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.
Flags: needinfo?(krudnitski)
(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.

Updated

4 years ago
Attachment #772119 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/f3d2f88ce830
status-firefox24: --- → fixed
status-firefox25: --- → fixed
(Assignee)

Comment 20

4 years ago
I updated the distribution documentation to include this:
https://wiki.mozilla.org/Mobile/Distribution_Files#Add-ons
Flags: needinfo?(elancaster)
You need to log in before you can comment on or make changes to this bug.