Closed Bug 1012462 Opened 6 years ago Closed 6 years ago

Support suggested sites in distribution files

Categories

(Firefox for Android :: Awesomescreen, defect)

33 Branch
All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 33
Tracking Status
fennec 33+ ---

People

(Reporter: Margaret, Assigned: lucasr)

References

(Blocks 2 open bugs)

Details

Attachments

(10 files, 6 obsolete files)

2.67 KB, patch
rnewman
: review+
Details | Diff | Splinter Review
1.57 KB, patch
rnewman
: review+
Details | Diff | Splinter Review
2.56 KB, patch
rnewman
: review+
Details | Diff | Splinter Review
3.90 KB, patch
rnewman
: review+
Details | Diff | Splinter Review
2.37 KB, patch
rnewman
: review+
Details | Diff | Splinter Review
2.27 KB, patch
rnewman
: review+
Details | Diff | Splinter Review
6.64 KB, patch
rnewman
: review+
Details | Diff | Splinter Review
19.11 KB, patch
rnewman
: review+
Details | Diff | Splinter Review
1.88 KB, patch
rnewman
: review+
Details | Diff | Splinter Review
22.45 KB, patch
rnewman
: review+
Details | Diff | Splinter Review
I'm not sure how we want to approach this, but maybe we can do something similar to what wesj did for quickshare providers in bug 972208.
tracking-fennec: --- → ?
tracking-fennec: ? → 32+
Assignee: nobody → lucasr.at.mozilla
rnewman has been doing lots of distribution work recently, looping him in.
Only a little input from me at this stage: we can't block on the distribution during startup, so Bug 1014338 applies (see the trivial pattern in Bug 1013684 -- queue up a task to run after the distribution is loaded).

That implies that the suggested sites must be able to change after display, so at least we can show *something*, or we need to be happy for them to be empty until a second or five has passed.

(The bug for fixing delayed loading of quickshare providers is Bug 1014242, which has a patch, but I don't know a good way to test it.)
Depends on: 1014338
(In reply to Richard Newman [:rnewman] from comment #2)
> Only a little input from me at this stage: we can't block on the
> distribution during startup, so Bug 1014338 applies (see the trivial pattern
> in Bug 1013684 -- queue up a task to run after the distribution is loaded).
> 
> That implies that the suggested sites must be able to change after display,
> so at least we can show *something*, or we need to be happy for them to be
> empty until a second or five has passed.
> 
> (The bug for fixing delayed loading of quickshare providers is Bug 1014242,
> which has a patch, but I don't know a good way to test it.)

Ok, I can probably just force suggested sites to refresh once the distribution files are ready.
Before I start working on this, I'd like to get a better understanding about what we want here. Some questions:

1. What is the expected behaviour for suggested sites coming from distribution files?
 - Do they take precedence over the built-in/default site suggestions?
 - Are they are always displayed first in the list or replace the default list completely?
 - Something else?
2. Is there any requirement around locales? Are we going to support distribution files that are locale-aware somehow?
3. My initial impression is that a server-side solution (a la snippets) would be more appropriate in terms of fetching a list of suggested sites dynamically instead of distribution files. But maybe I'm missing the big picture here.
tracking-fennec: 32+ → 33+
Flags: needinfo?(deb)
Posting my initial thoughts here to get some early feedback. This is more nuanced than it looks.

We ship default suggested sites as raw Android resources. These resources are l10n material. This means that switching to a different locale implies fetching a different default list of suggested sites.

Right now, the suggested sites code simply fetches the list from the raw resource and caches the list in memory for future top sites queries. Simple stuff.

The distribution file here would be an optional second source of suggested sites. The distribution zip will have to provide both the list of suggested sites and their respective images for each screen density (mdpi, hdpi, xhdpi, etc).

With that said, we mainly need to figure out two things here:

1. How do we load and integrate the suggested sites from the distribution with the default list?
2. How (format, directory structure, etc) should distributions ship suggested sites?

For 1, I can see two possible paths.

The first option is to always load both default and distribution suggested sites separately to generate the final list in memory. This means we'd load a raw resource and a distribution file on every app startup (the result is memcached for subsequent queries). This code path only runs if we need sites suggestions in the top sites grid btw. The nice thing about this approach is simplicity. For instance, handling locale changes would be a lot simpler. The disadvantage is the possible performance hit from always loading suggested sites from two sources on startup.

The second option is loading the distribution file only once when it gets deployed, prepend it to the default list of suggested sites, and write the result to a separate file in the profile directory. From then on, we'd load the suggested sites from this separate file. The advantage of this approach is performance. The drawback is the added complexity around the 'invalidation' of this separate file i.e. we'd need to regenerate it every time we change the default list in a new release and when the user picks a new locale.

For 2, the distribution zip could follow a structure similar to searchplugins. I assume we'll want the distribution's suggested sites to be localizable. So, we'd have something like:

/suggestedsites/common/suggestedsites.json
/suggestedsites/locale/pt-BR/suggestedsites.json
/suggestedsites/locale/fi/suggestedsites.json
/suggestedsites/res/mdpi/somesite.png
/suggestedsites/res/hdpi/somesite.png
/suggestedsites/res/xhdpi/somesite.png
/suggestedsites/res/mdpi/anothersite.png
/suggestedsites/res/hdpi/anothersite.png
/suggestedsites/res/xhdpi/anothersite.png
...

I'm still figuring out how we're going to define the URL for these images in suggestedsites.json. These image URLs obviously can't have hardcoded paths. My current plan is to allow distributions to use a special type of URL like 'suggestedsite://distribution/somefile.png' that Picasso would know how handle (using the Distribution API under the hood to find the images with the right density and so on). This will require some new APIs in upstream Picasso which I'm already working on (still waiting for feedback from the maintainers). The suggestedsites.json file would have things like:

{ title: "Distribution site",
  url: "http://distributionsite.org",
  bgcolor: '#ffcc00',
  imageUrl: 'suggestedsite://distribution/somefile' }

Thoughts?
(In reply to Lucas Rocha (:lucasr) from comment #5)

> The disadvantage is the possible performance hit from always loading
> suggested sites from two sources on startup.

Worth measuring. Loading the resource should be cheap; loading the distro file maybe not.

> The second option is loading the distribution file only once when it gets
> deployed, prepend it to the default list of suggested sites, and write the
> result to a separate file in the profile directory.

That's the approach that quickshare takes, fwiw.

> Thoughts?

That scheme makes sense to me. I wonder if it would be enough to just use a relative URL for the image -- "mysite.png"?
(In reply to Richard Newman [:rnewman] from comment #6)
> (In reply to Lucas Rocha (:lucasr) from comment #5)
> 
> > The disadvantage is the possible performance hit from always loading
> > suggested sites from two sources on startup.
> 
> Worth measuring. Loading the resource should be cheap; loading the distro
> file maybe not.

True. I'd like to go with this approach if the performance hit is negligible. 
 
> > The second option is loading the distribution file only once when it gets
> > deployed, prepend it to the default list of suggested sites, and write the
> > result to a separate file in the profile directory.
> 
> That's the approach that quickshare takes, fwiw.

Yeah, the main difference here is that we have to care about locale changes and updates to the default list. 

> > Thoughts?
> 
> That scheme makes sense to me. I wonder if it would be enough to just use a
> relative URL for the image -- "mysite.png"?

Good point, I'll try this.
Blocks: suggested-sites-v2
No longer blocks: suggested-sites-v1
As far as I'm aware, there is a team working on a backend service for this, which should also be able to hook into when it's ready.  I'll dig in and see if there's a weekly meeting one or more of us can start to call into to find out about progress, specs, etc.
Flags: needinfo?(deb)
To clarify for the bug record: downloadable tiles like this won't be shipping in a short enough term to eliminate the need for this bug.
Thought I'd write down for posterity a couple of restrictions that Lucas hinted at in Comment 7.

Downloaded distribution processing (33+, Bug 1013024) is likely to occur shortly after suggested sites has been displayed, and while the view is still visible.

Packaged distribution processing might well *directly overlap* with building that view -- it's async but we already have the file on disk.

That implies three things:

* Oh boy does this need to be thread-safe.

* The defaults need to be changeable at runtime. Partners won't be pleased if users don't see the distributed content until they relaunch the browser (which on a tablet might be days later). We have this problem with quickshare (Bug 1021176), search engines during locale transitions (Bug 1018240), and add-on init (Bug 923581), but this is way more visible.

* Consequently, we need to be considerate of transitions -- either the grid should only change when you switch activities or panels, or we need to animate somehow. We definitely shouldn't do a top sites-style flicker of doom.

Let me know if there's anything I can do to help out, Lucas.
Status: NEW → ASSIGNED
Hardware: ARM → All
Version: Firefox 30 → Firefox 33
Attachment #8452779 - Flags: review?(rnewman)
Comment on attachment 8452781 [details] [diff] [review]
Part 2: Add prefix argument to generateSites() in TestSuggestedSites (r=rnewman)

Prep work for the new test bits for distribution support.
Attachment #8452781 - Flags: review?(rnewman)
Comment on attachment 8452782 [details] [diff] [review]
Part 3: Add APIs to generate Site instance to/from JSON (r=rnewman)

Will be used to load and save site instances to/from JSON.
Attachment #8452782 - Flags: review?(rnewman)
Attachment #8452783 - Flags: review?(rnewman)
Comment on attachment 8452784 [details] [diff] [review]
Part 5: Add distribution support in SuggestedSites (r=rnewman)

I went with the second option described in comment #5 (merge and save result to a file inside the profile directory).

TODO (patches coming soon):
- Image loading support for suggested sites defined in distribution files
- Handle locale changes i.e. regenerate suggestedsites.json accordingly.
Attachment #8452784 - Flags: review?(rnewman)
Comment on attachment 8452785 [details] [diff] [review]
Part 6: Enable distribution support in SuggestedSites (r=rnewman)

Use the new feature in BrowserApp.
Attachment #8452785 - Flags: review?(rnewman)
Thanks for bringing up these points.

(In reply to Richard Newman [:rnewman] from comment #10)
> Thought I'd write down for posterity a couple of restrictions that Lucas
> hinted at in Comment 7.
> 
> Downloaded distribution processing (33+, Bug 1013024) is likely to occur
> shortly after suggested sites has been displayed, and while the view is
> still visible.
> 
> Packaged distribution processing might well *directly overlap* with building
> that view -- it's async but we already have the file on disk.
> 
> That implies three things:
> 
> * Oh boy does this need to be thread-safe.

I tried to keep that in mind in my patches. I might have missed something. Let me know.
 
> * The defaults need to be changeable at runtime. Partners won't be pleased
> if users don't see the distributed content until they relaunch the browser
> (which on a tablet might be days later). We have this problem with
> quickshare (Bug 1021176), search engines during locale transitions (Bug
> 1018240), and add-on init (Bug 923581), but this is way more visible.

Yep, patch 5 implements runtime updates. As I mentioned, I will implement locale switching in a separate patch in this bug. Should be simple to implement.

> * Consequently, we need to be considerate of transitions -- either the grid
> should only change when you switch activities or panels, or we need to
> animate somehow. We definitely shouldn't do a top sites-style flicker of
> doom.

My patch doesn't try to do anything fancy UI-wise. It simply forces top sites to refresh once it loads the distribution file. So, it will potentially cause some flickering. Only on first run but still.

Don't we have plans to add some sort of first-run experience that would mitigate these issues a bit?
(In reply to Lucas Rocha (:lucasr) from comment #21)

Thanks for getting these into my queue before your PTO!


> Yep, patch 5 implements runtime updates. As I mentioned, I will implement
> locale switching in a separate patch in this bug. Should be simple to
> implement.

Fingers crossed! :D


> My patch doesn't try to do anything fancy UI-wise. It simply forces top
> sites to refresh once it loads the distribution file. So, it will
> potentially cause some flickering. Only on first run but still.

OK. I'd like to consider whether we can mitigate this somehow (not necessarily in this bug). Here are some ideas:

* Chain initial displaying of the grid off the distribution task, just as we do for adding bookmarks etc. If there's no distro (99% of users), this will be essentially instantaneous -- the callback will be queued up right away on the background thread, and the suggested sites will appear very shortly thereafter. If there's a distro to download, we'll show nothing for a second or two. No flicker.

* Add a transition. More UI work, but at least we'll show stuff immediately.

* FRE (below).

Because this is front-and-center, and I anticipate partners will want to use this more than they want bookmarks, I'd like us to think about what we can get done for 33.

(Ideas from the peanut gallery most welcome!)



> Don't we have plans to add some sort of first-run experience that would
> mitigate these issues a bit?

We do -- and they'd mitigate all of them completely, I think! -- but probably not in 33 unless we want to hack in some kind of awful splash screen. Not many weeks left, and I think UX is over-committed.
Attachment #8452779 - Flags: review?(rnewman) → review+
Attachment #8452781 - Flags: review?(rnewman) → review+
Comment on attachment 8452782 [details] [diff] [review]
Part 3: Add APIs to generate Site instance to/from JSON (r=rnewman)

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

r+ with any necessary attention paid to null/NULL handling.

::: mobile/android/base/db/SuggestedSites.java
@@ +103,5 @@
> +
> +        public JSONObject toJSON() throws JSONException {
> +            final JSONObject json = new JSONObject();
> +
> +            json.put(JSON_KEY_URL, url);

Are any of these allowed to be null/not present?
Attachment #8452782 - Flags: review?(rnewman) → review+
Comment on attachment 8452783 [details] [diff] [review]
Part 4: Factor out code to create list of suggested sites (r=rnewman)

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

r+ with nits.

::: mobile/android/base/db/SuggestedSites.java
@@ +134,5 @@
>              sites = new LinkedHashMap<String, Site>(jsonSites.length());
>  
>              final int count = jsonSites.length();
>              for (int i = 0; i < count; i++) {
> +                final Site site = new Site((JSONObject) jsonSites.get(i));

Use

  http://developer.android.com/reference/org/json/JSONArray.html#getJSONObject%28int%29

@@ +135,5 @@
>  
>              final int count = jsonSites.length();
>              for (int i = 0; i < count; i++) {
> +                final Site site = new Site((JSONObject) jsonSites.get(i));
> +                sites.put(site.url, site);

There's nothing stopping site.url from being null.
Attachment #8452783 - Flags: review?(rnewman) → review+
Comment on attachment 8452784 [details] [diff] [review]
Part 5: Add distribution support in SuggestedSites (r=rnewman)

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

Looking good!

::: mobile/android/base/db/SuggestedSites.java
@@ +70,5 @@
>  
>      // SharedPreference key for suggested sites that should be hidden.
>      public static final String PREF_SUGGESTED_SITES_HIDDEN = "suggestedSites.hidden";
>  
> +    // File in profile dir with the list of suggested sites

Nit: trailing period.

@@ +131,2 @@
>      private Map<String, Site> cachedSites;
>      private Locale cachedLocale;

What's the threading model here?

@@ +146,5 @@
> +        this.distribution = distribution;
> +        this.file = file;
> +    }
> +
> +    private List<Locale> getPossibleLocales() {

I'd name this "getAcceptableLocales", and comment that this returns the current locale and its fallback (en_US) in order.

@@ +188,5 @@
> +        if (sites == null || sites.isEmpty()) {
> +            return;
> +        }
> +
> +        FileOutputStream fos = null;

You're writing characters, so consider using OutputStreamWriter so that you can specify charset correctly.

@@ +196,5 @@
> +            for (Site site : sites.values()) {
> +                jsonSites.put(site.toJSON());
> +            }
> +
> +            fos = new FileOutputStream(file);

You can lift this outside the `try`, make the close non-conditional, and handily distinguish between pre-write and intra-write exceptions:

  final FileOutputStream fos;
  try {
    fos = new FileOutputStream(file);
  } catch (Exception e) {
    Log.w(LOGTAG, "Couldn't open suggested sites file for writing.", e);
    return;
  }

  try {
    ...
  } finally {
    fos.close();
  }

@@ +286,5 @@
> +                                                  locale.toString(), file.toString());
> +
> +                final File f = distribution.getDistributionFile(path);
> +                if (f == null) {
> +                    Log.i(LOGTAG, "No suggested sites for locale: " + locale);

Log.d

@@ +290,5 @@
> +                    Log.i(LOGTAG, "No suggested sites for locale: " + locale);
> +                    continue;
> +                }
> +
> +                return loadFromStream(new FileInputStream(f));

Eliminate loadFromStream entirely with this handy one-liner, and get charsets right:

    final String body = new Scanner(f, "UTF-8").useDelimiter("\\A").next();
    return loadSites(body);

You could easily make that a loadSites(File) overload.

@@ +293,5 @@
> +
> +                return loadFromStream(new FileInputStream(f));
> +            } catch (Exception e) {
> +                Log.e(LOGTAG, "Failed to open suggested sites in distribution.", e);
> +                return null;

I'd be inclined to put this inside the loop. You don't want one malformed file to prevent access to the fallback localization.

@@ +304,3 @@
>      private Map<String, Site> loadFromFile() {
> +        try {
> +            return loadFromStream(new FileInputStream(file));

Same point re loadFromStream.

::: mobile/android/tests/browser/junit3/src/tests/TestSuggestedSites.java
@@ +44,3 @@
>  import org.mozilla.gecko.preferences.GeckoPreferences;
>  
>  public class TestSuggestedSites extends BrowserTestCase {

Could you also add some sanity checks to the Robocop test in base/tests/testDistribution.java?
Attachment #8452784 - Flags: review?(rnewman) → feedback+
Comment on attachment 8452785 [details] [diff] [review]
Part 6: Enable distribution support in SuggestedSites (r=rnewman)

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

::: mobile/android/base/BrowserApp.java
@@ +476,5 @@
>          final Context appContext = getApplicationContext();
>  
>          // Init suggested sites engine in BrowserDB.
> +        final SuggestedSites suggestedSites =
> +                new SuggestedSites(appContext, Distribution.getInstance(appContext));

This is very likely to be too early in our startup lifecycle to be instantiating the first Distribution instance.

(Testing will be needed to confirm this.)

During first run we receive an intent from the Play Store specifying which distribution to use, and we need to ensure that our receiver has got that message before the Distribution is created.

This might mean that we need to interleave control here, complicating part 5 -- e.g., creating SuggestedSites here, a little later kicking off a Distribution runnable, and if distribution.exists(), handing it to SuggestedSites with an apology and a polite request to do the right thing.
Lucas: to test this with a real live distribution:

* Uninstall (not just Clear Data).
* Install your APK.
* adb shell setprop log.tag.GeckoDistribution VERBOSE
* adb shell setprop log.tag.GeckoReferrerReceiver VERBOSE
* Use adb shell to send an intent:

https://wiki.mozilla.org/Mobile/Distribution_Files#Testing_distribution_download

Make sure you send this to the right package -- org.mozilla.fennec_lucasr, most likely.

* Launch the app, see if your suggested sites init has a distribution available when it runs (distribution.exists() in your runnable). You might find that this early access breaks distro processing altogether, or everything might work.

The test distro has a bookmarked named "Cheese".
(In reply to Richard Newman [:rnewman] from comment #25)
> Comment on attachment 8452784 [details] [diff] [review]
> Part 5: Add distribution support in SuggestedSites (r=rnewman)
> 
> Review of attachment 8452784 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looking good!
> 
> ::: mobile/android/base/db/SuggestedSites.java
> @@ +70,5 @@
> >  
> >      // SharedPreference key for suggested sites that should be hidden.
> >      public static final String PREF_SUGGESTED_SITES_HIDDEN = "suggestedSites.hidden";
> >  
> > +    // File in profile dir with the list of suggested sites
> 
> Nit: trailing period.

Done.

> @@ +131,2 @@
> >      private Map<String, Site> cachedSites;
> >      private Locale cachedLocale;
> 
> What's the threading model here?

We went simple here: all public methods are synchronized. All access to cachedSites has to be synchronized on 'this' too. This doesn't need to be a high-throughput API as suggested sites don't change very often and parallel operations are rare.

> @@ +146,5 @@
> > +        this.distribution = distribution;
> > +        this.file = file;
> > +    }
> > +
> > +    private List<Locale> getPossibleLocales() {
> 
> I'd name this "getAcceptableLocales", and comment that this returns the
> current locale and its fallback (en_US) in order.

Done.

> @@ +188,5 @@
> > +        if (sites == null || sites.isEmpty()) {
> > +            return;
> > +        }
> > +
> > +        FileOutputStream fos = null;
> 
> You're writing characters, so consider using OutputStreamWriter so that you
> can specify charset correctly.

Good point, done.

> @@ +196,5 @@
> > +            for (Site site : sites.values()) {
> > +                jsonSites.put(site.toJSON());
> > +            }
> > +
> > +            fos = new FileOutputStream(file);
> 
> You can lift this outside the `try`, make the close non-conditional, and
> handily distinguish between pre-write and intra-write exceptions:
> 
>   final FileOutputStream fos;
>   try {
>     fos = new FileOutputStream(file);
>   } catch (Exception e) {
>     Log.w(LOGTAG, "Couldn't open suggested sites file for writing.", e);
>     return;
>   }
> 
>   try {
>     ...
>   } finally {
>     fos.close();
>   }


 
> @@ +286,5 @@
> > +                                                  locale.toString(), file.toString());
> > +
> > +                final File f = distribution.getDistributionFile(path);
> > +                if (f == null) {
> > +                    Log.i(LOGTAG, "No suggested sites for locale: " + locale);
> 
> Log.d

Done.
 
> @@ +290,5 @@
> > +                    Log.i(LOGTAG, "No suggested sites for locale: " + locale);
> > +                    continue;
> > +                }
> > +
> > +                return loadFromStream(new FileInputStream(f));
> 
> Eliminate loadFromStream entirely with this handy one-liner, and get
> charsets right:
> 
>     final String body = new Scanner(f, "UTF-8").useDelimiter("\\A").next();
>     return loadSites(body);
> 
> You could easily make that a loadSites(File) overload.

Neat, done.
 
> @@ +293,5 @@
> > +
> > +                return loadFromStream(new FileInputStream(f));
> > +            } catch (Exception e) {
> > +                Log.e(LOGTAG, "Failed to open suggested sites in distribution.", e);
> > +                return null;
> 
> I'd be inclined to put this inside the loop. You don't want one malformed
> file to prevent access to the fallback localization.

Good point. I changed it to keep trying other locales even if an exception is thrown.  

> @@ +304,3 @@
> >      private Map<String, Site> loadFromFile() {
> > +        try {
> > +            return loadFromStream(new FileInputStream(file));
> 
> Same point re loadFromStream.

Done.

> ::: mobile/android/tests/browser/junit3/src/tests/TestSuggestedSites.java
> @@ +44,3 @@
> >  import org.mozilla.gecko.preferences.GeckoPreferences;
> >  
> >  public class TestSuggestedSites extends BrowserTestCase {
> 
> Could you also add some sanity checks to the Robocop test in
> base/tests/testDistribution.java?

Will do.
(In reply to Richard Newman [:rnewman] from comment #24)
> Comment on attachment 8452783 [details] [diff] [review]
> Part 4: Factor out code to create list of suggested sites (r=rnewman)
> 
> Review of attachment 8452783 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with nits.
> 
> ::: mobile/android/base/db/SuggestedSites.java
> @@ +134,5 @@
> >              sites = new LinkedHashMap<String, Site>(jsonSites.length());
> >  
> >              final int count = jsonSites.length();
> >              for (int i = 0; i < count; i++) {
> > +                final Site site = new Site((JSONObject) jsonSites.get(i));
> 
> Use
> 
>  
> http://developer.android.com/reference/org/json/JSONArray.
> html#getJSONObject%28int%29

Done.

> @@ +135,5 @@
> >  
> >              final int count = jsonSites.length();
> >              for (int i = 0; i < count; i++) {
> > +                final Site site = new Site((JSONObject) jsonSites.get(i));
> > +                sites.put(site.url, site);
> 
> There's nothing stopping site.url from being null.

Good point. Added a check in Site's constructor that throws if any of the properties are empty.
(In reply to Richard Newman [:rnewman] from comment #23)
> Comment on attachment 8452782 [details] [diff] [review]
> Part 3: Add APIs to generate Site instance to/from JSON (r=rnewman)
> 
> Review of attachment 8452782 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with any necessary attention paid to null/NULL handling.
> 
> ::: mobile/android/base/db/SuggestedSites.java
> @@ +103,5 @@
> > +
> > +        public JSONObject toJSON() throws JSONException {
> > +            final JSONObject json = new JSONObject();
> > +
> > +            json.put(JSON_KEY_URL, url);
> 
> Are any of these allowed to be null/not present?

Nope. I added an explicit check for empty values in all constructors.
Attachment #8452784 - Attachment is obsolete: true
Attachment #8452785 - Attachment is obsolete: true
Attachment #8452785 - Flags: review?(rnewman)
Comment on attachment 8454447 [details] [diff] [review]
Part 5: Factor out getLanguageTag() into LocaleUtils (r=rnewman)

Because I want the file paths to use gecko-style tags i.e. en-US instead of en_US.
Attachment #8454447 - Flags: review?(rnewman)
Comment on attachment 8454448 [details] [diff] [review]
Part 6: Add distribution support in SuggestedSites (r=rnewman)

Covers the review comments and more.
Attachment #8454448 - Flags: review?(rnewman)
Attachment #8454451 - Flags: review?(rnewman)
Comment on attachment 8454452 [details] [diff] [review]
Part 9: Support locale switching in SuggestedSites (r=rnewman)

Very similar to how we handle locale changes in about:home.
Attachment #8454452 - Flags: review?(rnewman)
Comment on attachment 8454450 [details] [diff] [review]
Part 7: Enable distribution support in SuggestedSites (r=rnewman)

Tested this with the instructions in comment #27 with local hardcoded distro values. Seems to be working fine.
Attachment #8454450 - Flags: review?(rnewman)
Comment on attachment 8454481 [details] [diff] [review]
Part 6: Add distribution support in SuggestedSites (r=rnewman)

A few extra tweaks and thread-safety fixes.
Attachment #8454481 - Flags: review?(rnewman)
Attachment #8454448 - Attachment is obsolete: true
Attachment #8454448 - Flags: review?(rnewman)
Comment on attachment 8454447 [details] [diff] [review]
Part 5: Factor out getLanguageTag() into LocaleUtils (r=rnewman)

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

Amusingly, this already exists in Sync's Utils.java, because the Sync codebase doesn't have access to Fennec's LocaleManager implementation.

But is there a reason not to just call BrowserLocaleManager.getLanguageTag? I don't see much benefit in having a separate class to hold locale utilities (at least, not yet) -- BLM *is* a locale-handling utility class -- and we'd have to pay the cost of another class in the APK and another source file.
Comment on attachment 8454481 [details] [diff] [review]
Part 6: Add distribution support in SuggestedSites (r=rnewman)

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

Looks good!

::: mobile/android/base/db/SuggestedSites.java
@@ +164,5 @@
> +
> +    /**
> +     * Return the current locale and its fallback (en_US) in order.
> +     */
> +    private List<Locale> getAcceptableLocales() {

This should be static.

@@ +177,5 @@
> +
> +        return locales;
> +    }
> +
> +    private Map<String, Site> loadSites(File f) throws IOException {

static

@@ +184,3 @@
>      }
>  
>      private Map<String, Site> loadSites(String jsonString) {

static

@@ +206,5 @@
>  
>          return sites;
>      }
>  
> +    private void saveSites(Map<String, Site> sites) {

ThreadUtils.assertNotOnUiThread(), comment that access to this should be synchronized on `file`.

@@ +264,5 @@
> +                // Update cached list of sites.
> +                setCachedSites(sites);
> +
> +                // Save the result to disk.
> +                synchronized(file) {

Nit: space before (

@@ +275,5 @@
> +            }
> +        });
> +    }
> +
> +    private Map<String, Site> loadFromDistribution() {

Comment that this should be guarded by a call to distribution.exists(), and only called in a distribution ready callback.

You might consider taking the distribution as an argument, making this static, and instead noting that the provided distribution should be ready and should exist.

@@ +302,5 @@
> +    }
> +
> +    private Map<String, Site> loadFromProfile() {
> +        try {
> +            synchronized(file) {

Space.

::: mobile/android/tests/browser/junit3/src/tests/TestSuggestedSites.java
@@ +433,5 @@
> +        // yet. This will happen asynchronously once the distribution
> +        // is installed.
> +        Cursor c = suggestedSites.get(DEFAULT_LIMIT);
> +        assertEquals(DEFAULT_COUNT, c.getCount());
> +        c.close();

I have a gentle preference for closing cursors with `finally` blocks, even in tests, for two reasons:

* It discourages others from directly copying incorrect patterns from test code.
* If the assertion fails and a test continues, a confusing error about an unclosed cursor being finalized will be spammed to the log, and perhaps be detected as the failure cause.

@@ +475,5 @@
> +        }
> +
> +        c.close();
> +        tempFile.delete();
> +        sitesFile.delete();

These should certainly happen in a `finally` block, or in a setUp/tearDown pair in the test file.
Attachment #8454481 - Flags: review?(rnewman) → review+
Comment on attachment 8454450 [details] [diff] [review]
Part 7: Enable distribution support in SuggestedSites (r=rnewman)

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

::: mobile/android/base/BrowserApp.java
@@ +546,5 @@
>              "Settings:Show",
>              "Telemetry:Gather",
>              "Updater:Launch");
>  
>          Distribution.init(this);

It would be really neat to have this static call return the Distribution instance, which saves you making another synchronized call to getInstance immediately below. Make it so! :D
Attachment #8454450 - Flags: review?(rnewman) → review+
Attachment #8454451 - Flags: review?(rnewman) → review+
Comment on attachment 8454452 [details] [diff] [review]
Part 9: Support locale switching in SuggestedSites (r=rnewman)

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

If it works, I'm happy :)
Attachment #8454452 - Flags: review?(rnewman) → review+
Comment on attachment 8455278 [details] [diff] [review]
Part 10: Ensure URL images always fit within the view bounds (r=rnewman)

We're being a bit flexible in the density lookup and might end up using images with higher resolution. This is just to ensure the images loaded from URLs will always fit within the view bounds.
Attachment #8455278 - Flags: review?(rnewman)
Comment on attachment 8455280 [details] [diff] [review]
Part 11: Support image loading for distribution files (r=rnewman)

I've proposed a new API in Picasso to add support for custom image loading. But I haven't got feedback from the maintainers yet (see [1]). For now, the only way to do it is by using a custom Downloader in Picasso. Definitely not ideal but it's a good enough compromise until we get proper APIs from upstream.

[1] https://github.com/square/picasso/pull/512
Attachment #8455280 - Flags: review?(rnewman)
Blocks: 1038127
(In reply to Richard Newman [:rnewman] from comment #43)
> Comment on attachment 8454481 [details] [diff] [review]
> Part 6: Add distribution support in SuggestedSites (r=rnewman)
> 
> Review of attachment 8454481 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good!
> 
> ::: mobile/android/base/db/SuggestedSites.java
> @@ +164,5 @@
> > +
> > +    /**
> > +     * Return the current locale and its fallback (en_US) in order.
> > +     */
> > +    private List<Locale> getAcceptableLocales() {
> 
> This should be static.

Done.

> @@ +177,5 @@
> > +
> > +        return locales;
> > +    }
> > +
> > +    private Map<String, Site> loadSites(File f) throws IOException {
> 
> static

Done.

> @@ +184,3 @@
> >      }
> >  
> >      private Map<String, Site> loadSites(String jsonString) {
> 
> static

Done.

> @@ +206,5 @@
> >  
> >          return sites;
> >      }
> >  
> > +    private void saveSites(Map<String, Site> sites) {
> 
> ThreadUtils.assertNotOnUiThread(), comment that access to this should be
> synchronized on `file`.

Good idea, done.

> @@ +264,5 @@
> > +                // Update cached list of sites.
> > +                setCachedSites(sites);
> > +
> > +                // Save the result to disk.
> > +                synchronized(file) {
> 
> Nit: space before (

Fixed.

> @@ +275,5 @@
> > +            }
> > +        });
> > +    }
> > +
> > +    private Map<String, Site> loadFromDistribution() {
> 
> Comment that this should be guarded by a call to distribution.exists(), and
> only called in a distribution ready callback.
> 
> You might consider taking the distribution as an argument, making this
> static, and instead noting that the provided distribution should be ready
> and should exist.

Sounds good to me, done.

> @@ +302,5 @@
> > +    }
> > +
> > +    private Map<String, Site> loadFromProfile() {
> > +        try {
> > +            synchronized(file) {
> 
> Space.

Fixed.

> ::: mobile/android/tests/browser/junit3/src/tests/TestSuggestedSites.java
> @@ +433,5 @@
> > +        // yet. This will happen asynchronously once the distribution
> > +        // is installed.
> > +        Cursor c = suggestedSites.get(DEFAULT_LIMIT);
> > +        assertEquals(DEFAULT_COUNT, c.getCount());
> > +        c.close();
> 
> I have a gentle preference for closing cursors with `finally` blocks, even
> in tests, for two reasons:
> 
> * It discourages others from directly copying incorrect patterns from test
> code.
> * If the assertion fails and a test continues, a confusing error about an
> unclosed cursor being finalized will be spammed to the log, and perhaps be
> detected as the failure cause.

Filed mentored bug 1038127. Only updated the tests added in this bug.

> @@ +475,5 @@
> > +        }
> > +
> > +        c.close();
> > +        tempFile.delete();
> > +        sitesFile.delete();
> 
> These should certainly happen in a `finally` block, or in a setUp/tearDown
> pair in the test file.

Done.
(In reply to Richard Newman [:rnewman] from comment #44)
> Comment on attachment 8454450 [details] [diff] [review]
> Part 7: Enable distribution support in SuggestedSites (r=rnewman)
> 
> Review of attachment 8454450 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/BrowserApp.java
> @@ +546,5 @@
> >              "Settings:Show",
> >              "Telemetry:Gather",
> >              "Updater:Launch");
> >  
> >          Distribution.init(this);
> 
> It would be really neat to have this static call return the Distribution
> instance, which saves you making another synchronized call to getInstance
> immediately below. Make it so! :D

Done.
(In reply to Richard Newman [:rnewman] from comment #42)
> Comment on attachment 8454447 [details] [diff] [review]
> Part 5: Factor out getLanguageTag() into LocaleUtils (r=rnewman)
> 
> Review of attachment 8454447 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Amusingly, this already exists in Sync's Utils.java, because the Sync
> codebase doesn't have access to Fennec's LocaleManager implementation.
> 
> But is there a reason not to just call BrowserLocaleManager.getLanguageTag?
> I don't see much benefit in having a separate class to hold locale utilities
> (at least, not yet) -- BLM *is* a locale-handling utility class -- and we'd
> have to pay the cost of another class in the APK and another source file.

Calling something on a 'BrowserLocaleManager' just felt a bit heavy-handed to me for a simple utility method. I don't feel strongly about it though. Patch removed.
Attachment #8454447 - Attachment is obsolete: true
Attachment #8454447 - Flags: review?(rnewman)
Comment on attachment 8455397 [details] [diff] [review]
Part 11: Support image loading for distribution files (r=rnewman)

Forgot to replace all instances of Picasso.with() with the new ImageLoader.with() API. We don't want to have multiple image memcaches around.
Attachment #8455397 - Flags: review?(rnewman)
Attachment #8455280 - Attachment is obsolete: true
Attachment #8455280 - Flags: review?(rnewman)
Comment on attachment 8455278 [details] [diff] [review]
Part 10: Ensure URL images always fit within the view bounds (r=rnewman)

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

::: mobile/android/base/home/TopSitesGridItemView.java
@@ +233,5 @@
>       * @param imageUrl URL of the image to show.
>       * @param bgColor background color to use in the view.
>       */
>      public void displayThumbnail(String imageUrl, int bgColor) {
> +        mThumbnailView.setScaleType(SCALE_TYPE_URL);

For incorrectly sized thumbnails this'll leave us with letterboxing. Perhaps we should use CENTER_CROP instead?

Also, this is displayThumbnail, and we have SCALE_TYPE_THUMBNAIL, which is already CENTER_CROP. Just use that here?
(In reply to Richard Newman [:rnewman] from comment #55)
> Comment on attachment 8455278 [details] [diff] [review]
> Part 10: Ensure URL images always fit within the view bounds (r=rnewman)
> 
> Review of attachment 8455278 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/home/TopSitesGridItemView.java
> @@ +233,5 @@
> >       * @param imageUrl URL of the image to show.
> >       * @param bgColor background color to use in the view.
> >       */
> >      public void displayThumbnail(String imageUrl, int bgColor) {
> > +        mThumbnailView.setScaleType(SCALE_TYPE_URL);
> 
> For incorrectly sized thumbnails this'll leave us with letterboxing. Perhaps
> we should use CENTER_CROP instead?
> 
> Also, this is displayThumbnail, and we have SCALE_TYPE_THUMBNAIL, which is
> already CENTER_CROP. Just use that here?

CENTER_CROP will unconditionally scale up images to fill the whole area. For suggested images, we just want to ensure that they're kept within view bounds without necessarily scaling them up. Maybe the method names are causing a bit of confusion here: displayThumbnail(url, color) is only used for images downloaded from a URL (suggested sites and metadata images). Maybe I should rename this to something like displayRemoteImage() or something.
Comment on attachment 8455397 [details] [diff] [review]
Part 11: Support image loading for distribution files (r=rnewman)

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

Looking good!

Splinter will put my comments in the wrong order. Apologies :)

::: mobile/android/base/home/ImageLoader.java
@@ +1,1 @@
> +package org.mozilla.gecko.home;

License.

@@ +28,5 @@
> +    // tries to find images with higher density (2.0 and 1.5). If no image is found,
> +    // try a lower density (0.5). See loadDistributionImage().
> +    private static final float[] densityFactors = new float[] { 1.0f, 2.0f, 1.5f, 0.5f };
> +
> +    private static Picasso instance;

This is the second concern I have with this patch -- do we *always* want to use the same image loader? Do we want to put thumbnails, suggested site images, etc. etc. into the same loader and cache?

This is not a negative review comment, just somewhere I don't think I have enough context to offer a good opinion. Consider this an opportunity to enlighten me!

@@ +34,5 @@
> +    public static synchronized Picasso with(Context context) {
> +        if (instance == null) {
> +            Picasso.Builder builder = new Picasso.Builder(context);
> +
> +            final Distribution distribution = Distribution.getInstance(context);

I have only two concerns with this patch.

The first is this line. We share a single ImageLoader across the whole app. That means we don't have control over when this line is called. If BrowserApp > Top Sites > ... > this line 'wins' the race versus the first-run ReferrerReceiver setting the referrer in Distribution, then we'll break distro loading from referrers.

There are three ways we can address this.

1. Be very careful about when we instantiate ImageLoader, exactly the same way we're careful about when we instantiate Distribution today (and earlier in this patch series).
2. Introduce some coordination between ReferrerReceiver, a timer, and Distribution, such that it's *always* safe to call Distribution.getInstance -- it waits a sane amount of time for a referrer to arrive.
3. Allow for ImageLoader to accept a Distribution instance later.

(3) would be relatively straightforward: exactly when we decide to load suggested sites from the distribution (which you tested earlier, re point (1)), grab the static ImageLoader and hand it the Distribution, rather than doing this in the constructor.

If there's no distribution (!distribution.exists()), this handily avoids us even holding on to a reference to the instance.

Thoughts?

@@ +74,5 @@
> +        }
> +
> +        @Override
> +        public Response load(Uri uri, boolean localCacheOnly) throws IOException {
> +            final String scheme = uri.getScheme();

Will we ever get a null URI here?

@@ +95,5 @@
> +         *   gecko.distribution://<basepath>/<imagename>
> +         *
> +         * Which will look for the following file in the distribution:
> +         *
> +         *   <distribution-root-dir>/<basepath>/<device-density>/<imagename>.png

I feel like this method ought to be cut up a little more to be more testable -- a pure function from URI + density => file path -- but I realize that involving density later in the process makes this difficult. Sigh.

@@ +113,5 @@
> +                filename = ssp.substring(slashIndex + 1);
> +                basePath = ssp.substring(0, slashIndex);
> +            }
> +
> +            Set<String> triedDensities = new HashSet<String>();

Strongly tempted to use an EnumSet here. We have a fixed, known list of four densities; anything more than a couple of bits for that is wasteful, and we'd get type-safe densities as a side benefit.

@@ +127,5 @@
> +                Log.d(LOGTAG, "Trying to load image from distribution " + path);
> +
> +                f = distribution.getDistributionFile(path);
> +                if (f != null) {
> +                    break;

return new Response(new FileInputStream(f), true);

then you can just throw immediately after the loop. And you can kill the early declaration of File, just declaring it `final` each time on line 129.

::: mobile/android/tests/browser/junit3/src/tests/TestImageDownloader.java
@@ +89,5 @@
> +            return null;
> +        }
> +
> +        public List<String> getAccessedFiles() {
> +            return Collections.unmodifiableList(accessedFiles);

Nice.
The "testcontent" distribution should now include suggested sites. Perhaps obviously, you'll need these patches applied to test!
(In reply to Richard Newman [:rnewman] from comment #58)
> @@ +28,5 @@
> > +    // tries to find images with higher density (2.0 and 1.5). If no image is found,
> > +    // try a lower density (0.5). See loadDistributionImage().
> > +    private static final float[] densityFactors = new float[] { 1.0f, 2.0f, 1.5f, 0.5f };
> > +
> > +    private static Picasso instance;
> 
> This is the second concern I have with this patch -- do we *always* want to
> use the same image loader? Do we want to put thumbnails, suggested site
> images, etc. etc. into the same loader and cache?
> 
> This is not a negative review comment, just somewhere I don't think I have
> enough context to offer a good opinion. Consider this an opportunity to
> enlighten me!

My take on this is that we should use a single memcache for all images *but* we should account for the types of image in the cache when expiring elements i.e. a straight LRU memcache for all images is not good enough. For example, you might want to keep suggested site images around if possible as they're more likely to be fetched multiple times than, say, empty state images on hub panels.

Right now, we only use Picasso in hub panels (built-in and from add-ons) and I haven't seen any (perceived) perf issues caused by inefficient memcache. Things will get a bit more interesting if/when we start using Picasso to load things like site thumbnails and favicons though (partly bug 997774). For now, I think we're fine.

In any case, having a single memcache for all these images in the app would probably make it easier to implement global image expiration policies in a single place — which is probably a good idea. In practice, we'd implement our own custom Cache[1] implementation for Picasso.

[1] http://dxr.mozilla.org/mozilla-central/source/mobile/android/thirdparty/com/squareup/picasso/Cache.java
(In reply to Richard Newman [:rnewman] from comment #58)
> ::: mobile/android/base/home/ImageLoader.java
> @@ +1,1 @@
> > +package org.mozilla.gecko.home;
> 
> License.

Added.
 
> @@ +34,5 @@
> > +    public static synchronized Picasso with(Context context) {
> > +        if (instance == null) {
> > +            Picasso.Builder builder = new Picasso.Builder(context);
> > +
> > +            final Distribution distribution = Distribution.getInstance(context);
> 
> The first is this line. We share a single ImageLoader across the whole app.
> That means we don't have control over when this line is called. If
> BrowserApp > Top Sites > ... > this line 'wins' the race versus the
> first-run ReferrerReceiver setting the referrer in Distribution, then we'll
> break distro loading from referrers.

Could you describe how exactly this could break? By 'win', do you mean a getInstance() call before the Referrer is set? Not sure I follow.
 
> There are three ways we can address this.
> 
> 1. Be very careful about when we instantiate ImageLoader, exactly the same
> way we're careful about when we instantiate Distribution today (and earlier
> in this patch series).
> 2. Introduce some coordination between ReferrerReceiver, a timer, and
> Distribution, such that it's *always* safe to call Distribution.getInstance
> -- it waits a sane amount of time for a referrer to arrive.
> 3. Allow for ImageLoader to accept a Distribution instance later.
> 
> (3) would be relatively straightforward: exactly when we decide to load
> suggested sites from the distribution (which you tested earlier, re point
> (1)), grab the static ImageLoader and hand it the Distribution, rather than
> doing this in the constructor.
> 
> If there's no distribution (!distribution.exists()), this handily avoids us
> even holding on to a reference to the instance.
> 
> Thoughts?

Let me try 3. 

> @@ +74,5 @@
> > +        }
> > +
> > +        @Override
> > +        public Response load(Uri uri, boolean localCacheOnly) throws IOException {
> > +            final String scheme = uri.getScheme();
> 
> Will we ever get a null URI here?

Nope, Picasso doesn't allow requests with null URIs.  

> @@ +95,5 @@
> > +         *   gecko.distribution://<basepath>/<imagename>
> > +         *
> > +         * Which will look for the following file in the distribution:
> > +         *
> > +         *   <distribution-root-dir>/<basepath>/<device-density>/<imagename>.png
> 
> I feel like this method ought to be cut up a little more to be more testable
> -- a pure function from URI + density => file path -- but I realize that
> involving density later in the process makes this difficult. Sigh.
> 
> @@ +113,5 @@
> > +                filename = ssp.substring(slashIndex + 1);
> > +                basePath = ssp.substring(0, slashIndex);
> > +            }
> > +
> > +            Set<String> triedDensities = new HashSet<String>();
> 
> Strongly tempted to use an EnumSet here. We have a fixed, known list of four
> densities; anything more than a couple of bits for that is wasteful, and
> we'd get type-safe densities as a side benefit.

Started with that then changed direction. Let try that.

> @@ +127,5 @@
> > +                Log.d(LOGTAG, "Trying to load image from distribution " + path);
> > +
> > +                f = distribution.getDistributionFile(path);
> > +                if (f != null) {
> > +                    break;
> 
> return new Response(new FileInputStream(f), true);
> 
> then you can just throw immediately after the loop. And you can kill the
> early declaration of File, just declaring it `final` each time on line 129.

Nice, done.
Attachment #8455397 - Attachment is obsolete: true
Attachment #8455397 - Flags: review?(rnewman)
Comment on attachment 8456156 [details] [diff] [review]
Part 11: Support image loading for distribution files (r=rnewman)

Update with the suggested changes except the one about moving the Distribution.getInstance() call somewhere else. I need to understand the Distribution life-cycle a bit better before changing the patch.
Attachment #8456156 - Flags: review?(rnewman)
(In reply to Lucas Rocha (:lucasr) from comment #61)

> Could you describe how exactly this could break? By 'win', do you mean a
> getInstance() call before the Referrer is set? Not sure I follow.

Yes, exactly. On first run, if installed via a referrer, Android will deliver an intent to us at some point after startup (and experimentation suggests that it's really really soon after startup). Our receiver sets a field (`referrer`) on Distribution. That field acts like a dead-drop. 

If the first Distribution is instantiated after that field is set, everything works: we see the referrer, download the distribution, process it, then call the post-distro runnables.

If, however, we instantiate a Distribution really early -- at the start of Activity.onCreate, say -- we might well cause the singleton Distribution instance to look at that field before the receiver is done processing the intent.

(We can even see this with the code as it stands now, if you break the APK: if mozglue and libxul are b0rked, startup short-circuits way too quickly, and the distro sometimes won't be processed.)

We're reliant on a very reliable timing relationship, rather than relying on some kind of explicit (and expensive) coordination -- the referrer intent currently arrives hundreds of milliseconds before the first use of a distribution, and we never need to block to wait for it. The downside of that is we need to exercise some care in changes that might affect that timing by bringing the first use forward.
Attachment #8455278 - Flags: review?(rnewman) → review+
Comment on attachment 8456156 [details] [diff] [review]
Part 11: Support image loading for distribution files (r=rnewman)

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

Hah! I forgot that I already added code to mitigate early init.

The Distribution instance doesn't look at the referrer until doInit is called. Simply instantiating a Distribution is safe. So the only race here would be if the ImageLoader tried to access the distribution file before the referrer receiver ran, which seems unlikely to me.

I'll test and land all of these patches later today.

::: mobile/android/base/home/ImageLoader.java
@@ +141,5 @@
> +
> +                final String path = getPathForDensity(basePath, density, filename);
> +                Log.d(LOGTAG, "Trying to load image from distribution " + path);
> +
> +                final File f = distribution.getDistributionFile(path);

We should be aware that this can trigger distribution init, should it happen soon enough. It shouldn't, because we init elsewhere.
Attachment #8456156 - Flags: review?(rnewman) → review+
Friendly reminder for adding a section over at https://wiki.mozilla.org/Mobile/Distribution_Files
Keywords: dev-doc-needed
Needed a little rebasing to handle the removal of LocaleUtils and some conflicts.

Try:

https://tbpl.mozilla.org/?tree=Try&rev=16f3f18a9e37

Will test locally with real distribution in a few hours.
Flags: needinfo?(rnewman)
\o/
Flags: needinfo?(rnewman)
Target Milestone: --- → Firefox 33
(In reply to Richard Newman [:rnewman] from comment #70)
> Documented:
> 
> https://wiki.mozilla.org/Mobile/Distribution_Files#Suggested_sites

Added some extra bits about the suggestedsites.json format and the gecko.distribution:// URLs.
Depends on: 1055826
You need to log in before you can comment on or make changes to this bug.