834681 - Add support for basic distribution modifications

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

We'd like to be able to ship versions of Firefox that have vendor modifications, just like desktop Firefox. Modifications include:

* Default bookmarks
  * Title, URL and Favicon
  * Maybe add support for pre-pinned bookmarks
  * Someday we need to add support for a pre-thumbnail, but for now that is a separate bug
* Preferences
  * Simple user-pref changes
  * Session based "default" prefs will also need to be supported
* Light-weight theme

Other requirements:
* We need to make sure that newer updates to Firefox, such as from the Play Store, do not blow away the distribution data.
  * We could copy the distribution data outside of the APK on firstrun. That should save it from updates.
* We do not have a means for vendors to update the distribution data (like add new bookmarks) separately from a Firefox update. If the distribution data needs to change, a new Firefox install would be needed.

Future features:
* Support adding search engines
* Support adding add-ons

Comment 1

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

Some info on the desktop support: https://wiki.mozilla.org/Distribution_INI_File

Some of this info shows the tight coupling to the Firefox desktop UI (notably the bookmarks sections). Other sections show ways to support multiple locales.

Comment 2

[:Margaret Leibovic]

Attachment: WIP

This patch reads from a /distribution/prefs.json file to set prefs (including the required distribution ones).

I feel like we should try to clean this up and land it, then worry about bookmarks in a separate patch.

Some issues left to consider:
-This just assumes the Java part will run before the Gecko part. Based on my logging, multiple seconds are passing between these two things, so it might be a fair assumption, but to be safe we could send some event to let Gecko know we're ready.
-Is this the best place to be calling Distribution.init() in BrowserApp?
-I just left the existing distribution stuff alone, but maybe we want to change that around to avoid confusion?

As a note, using JSON made this code so much nicer that desktop's distribution.js, so I think that's a win.

Comment 3

[:Margaret Leibovic]

Attachment: test prefs.json

This is what I was using for testing. I basically just converted the desktop distributuion.ini file format to JSON.

Do we like this format, or is there room for improvement? I feel like keeping it similar to desktop is easier than making it different.

Comment 4

[:Margaret Leibovic]

Cc'ing Brian in case he wants to look at the patch, too.

Comment 5

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

Comment on attachment 707897 [details] [diff] [review]
WIP


>diff --git a/mobile/android/base/BrowserApp.java b/mobile/android/base/BrowserApp.java

>         registerEventListener("Telemetry:Gather");
>+
>+        Distribution.init(this);

This seems like a good place for now. 

>diff --git a/mobile/android/base/Distribution.java b/mobile/android/base/Distribution.java

>+            public void run() {
>+
>+                // Bail if we've already initialized the distribution.

nit: Remove the leading blank line

>+                SharedPreferences settings = activity.getPreferences(Activity.MODE_PRIVATE);
>+                String keyName = activity.getPackageName() + ".distribution_initialized";
>+                if (settings.getBoolean(keyName, false))
>+                    return;

I wonder if we should consider using a version number here? Maybe overkill for now. We should check with Kev and the partner repack team to see if they ever use the version data to overwrite the distribution files.

>+    private static void copyFiles(Activity activity) throws IOException {
>+        Log.i(LOGTAG, "Searching for distribution files");

nit: Remove any extraneous Logs

>+        while (zipEntries.hasMoreElements()) {
>+
>+            ZipEntry fileEntry = zipEntries.nextElement();

nit: Remove the leading blank line

>+            Log.i(LOGTAG, "Copying distribution file - " + name);

Same

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>   init: function dc_init() {
>+    dump("Distribution.init");
>+

Same about remove the extraneous logs

>   observe: function dc_observe(aSubject, aTopic, aData) {

>+    // This event is only used for campagin tracking

campaign

>+    let aboutString = Cc["@mozilla.org/supports-string;1"].
>+                      createInstance(Ci.nsISupportsString);

No wrap needed

>+        switch (typeof value) {
>+        case "boolean":

nit: indent the case blocks

>+    let localizedString = Cc["@mozilla.org/pref-localizedstring;1"].
>+                          createInstance(Ci.nsIPrefLocalizedString);

No wrap needed

My OCD is making me want to name the file "preferences.json"

I agree that we could land this and followup with bookmark support in a separate patch or bug.

Comment 6

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

Comment on attachment 707898 [details]
test prefs.json

This format looks good to me.

Comment 7

[:Margaret Leibovic]

Attachment: patch

Here's a cleaned up patch. I also updated my test JSON file to make sure the localized prefs stuff works.

Comment 8

[:Margaret Leibovic]

Attachment: test preferences.json

Comment 9

[Brian Nicholson (:bnicholson)]

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

Review of attachment 708264 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/Distribution.java
@@ +77,5 @@
> +                dir.mkdirs();
> +
> +            InputStream fileStream = zip.getInputStream(fileEntry);
> +            OutputStream outStream = new FileOutputStream(outFile);
> +            while (fileStream.available() > 0) {

According to http://docs.oracle.com/javase/6/docs/api/java/io/InputStream.html#available%28%29, InputStream.available() returns the number of bytes that can be read without blocking. Since you just want to read until the end of the stream, I think the usual way to do this is something like "while ((read = fileStream.read()) != -1)".

Comment 10

[:Margaret Leibovic]

Attachment: patch v2

I updated the patch to address bnicholson's concern. Also flagged him in case he wants to take over reviewing :)

Comment 11

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

Comment on attachment 708320 [details] [diff] [review]
patch v2

LGTM

Comment 12

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/493805ba1c85

Comment 13

[Brian Nicholson (:bnicholson)]

Comment on attachment 708320 [details] [diff] [review]
patch v2

Review of attachment 708320 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! My main concern is the possible race the first time the distribution is unpacked. Since it happens asynchronously on the Gecko background thread, isn't it possible that Gecko could load before the distribution files have been unzipped? If so, maybe we should figure out a way to delay the Gecko initialization until the distribution initialization has taken place. Or perhaps the distribution stuff can just go on the main thread since it's a one-shot occurrence?

::: mobile/android/base/Distribution.java
@@ +20,5 @@
> +import java.io.IOException;
> +import java.io.InputStream;
> +import java.io.OutputStream;
> +
> +import java.lang.Exception;

Nit: Do you need this import?

@@ +26,5 @@
> +import java.util.zip.ZipEntry;
> +import java.util.zip.ZipFile;
> +
> +public final class Distribution {
> +    private static final String LOGTAG = "Distribution";

Nit: Should we follow the convention of using the Gecko prefix (i.e., "GeckoDistribution") to make logs more grep-able?

Comment 14

[Brian Nicholson (:bnicholson)]

Oops, looks like I took to long to submit my review.

Comment 15

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

(In reply to Brian Nicholson (:bnicholson) from comment #13)

> Looks good! My main concern is the possible race the first time the
> distribution is unpacked. Since it happens asynchronously on the Gecko
> background thread, isn't it possible that Gecko could load before the
> distribution files have been unzipped? If so, maybe we should figure out a
> way to delay the Gecko initialization until the distribution initialization
> has taken place. Or perhaps the distribution stuff can just go on the main
> thread since it's a one-shot occurrence?

We could send an event to Gecko, like the "Distribution:Set" event (rename the current one to "Campaign:Set"). Those events are queued and will definitely be received by Gecko, it won't matter if Gecko beats the file copy or not.

New bug?

Comment 16

[:Margaret Leibovic]

Mid-aired with mfinkle, but my comment was going to be similar...

(In reply to Brian Nicholson (:bnicholson) from comment #13)

> Looks good! My main concern is the possible race the first time the
> distribution is unpacked. Since it happens asynchronously on the Gecko
> background thread, isn't it possible that Gecko could load before the
> distribution files have been unzipped? If so, maybe we should figure out a
> way to delay the Gecko initialization until the distribution initialization
> has taken place. Or perhaps the distribution stuff can just go on the main
> thread since it's a one-shot occurrence?

Yeah, we are relying on the fact that Gecko is slow to load. I can file a follow-up bug to address this, but I think we should do something like sending an event to Gecko.

Doing I/O on the main thread sounds like a bad idea. Even if it's stuff we want to get done soon, there's also a lot of other important things that would block.

> > +import java.lang.Exception;
> 
> Nit: Do you need this import?

Whoops.

> Nit: Should we follow the convention of using the Gecko prefix (i.e.,
> "GeckoDistribution") to make logs more grep-able?

Sounds good to me. I can land these nits in a follow-up.

Comment 17

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #15)
> (In reply to Brian Nicholson (:bnicholson) from comment #13)
> 
> > Looks good! My main concern is the possible race the first time the
> > distribution is unpacked. Since it happens asynchronously on the Gecko
> > background thread, isn't it possible that Gecko could load before the
> > distribution files have been unzipped? If so, maybe we should figure out a
> > way to delay the Gecko initialization until the distribution initialization
> > has taken place. Or perhaps the distribution stuff can just go on the main
> > thread since it's a one-shot occurrence?
> 
> We could send an event to Gecko, like the "Distribution:Set" event (rename
> the current one to "Campaign:Set"). Those events are queued and will
> definitely be received by Gecko, it won't matter if Gecko beats the file
> copy or not.
> 
> New bug?

I filed bug 836838.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Either this bug or bug 836517 was causing Android test hangs. Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/eefb9fb2af78

Comment 19

[:Margaret Leibovic]

Try run with patch for bug 836838 was clean:
https://tbpl.mozilla.org/?tree=Try&rev=5165a08fb5fb

https://hg.mozilla.org/integration/mozilla-inbound/rev/7f543b37a359

Comment 20

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7f543b37a359

Comment 21

[:Margaret Leibovic]

Comment on attachment 708320 [details] [diff] [review]
patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: no distribution support (we want to uplift this so that partners can use it sooner rather than later)
Testing completed (on m-c, etc.): landed on m-c 2/3, already on aurora
Risk to taking this patch (and alternatives if risky): low-risk, this code is pretty self-contained
String or UUID changes made by this patch: n/a

Comment 22

[Lukas Blakk [:lsblakk] use ?needinfo]

Margaret - will partners be using this on Beta or is this just to bump the release of this support up to FF20's release in 5 weeks?

Comment 23

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

(In reply to Lukas Blakk [:lsblakk] from comment #22)
> Margaret - will partners be using this on Beta or is this just to bump the
> release of this support up to FF20's release in 5 weeks?

neither. this is to get the fix baking on beta now, in advance of possible build on release next week (see bug#845467).

Comment 24

[:Margaret Leibovic]

https://hg.mozilla.org/releases/mozilla-beta/rev/6584c0014b1c

Comment 25

[:Margaret Leibovic]

https://hg.mozilla.org/releases/mozilla-release/rev/9f828a03024e

Comment 26

[Aaron Train [:aaronmt]]

Kevin, can you verify this one?

Comment 27

[Kevin Brosnan [Ex-Mozilla]]

This has been verified by a partner, Mozilla China and Taiwan as well as myself.