3.36 KB, patch
|Details | Diff | Splinter Review|
11.20 KB, patch
|Details | Diff | Splinter Review|
12.71 KB, patch
|Details | Diff | Splinter Review|
We should look into supporting a distribution/extensions directory, similarly to how we do search engines.
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: --- → ?
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. email@example.com. 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
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?
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...
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+
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+
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
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)
Attachment #770347 - Attachment is obsolete: true
Attachment #771111 - Flags: review?(mark.finkle) → review+
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.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
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 ?
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.
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).
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.
(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.
Attachment #772119 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
status-firefox24: --- → fixed
status-firefox25: --- → fixed
I updated the distribution documentation to include this: https://wiki.mozilla.org/Mobile/Distribution_Files#Add-ons
You need to log in before you can comment on or make changes to this bug.