Closed Bug 1205335 Opened 6 years ago Closed 6 years ago

Allow homepage to be set by distributions

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(firefox44 fixed)

RESOLVED FIXED
Firefox 44
Tracking Status
firefox44 --- fixed

People

(Reporter: mhaigh, Assigned: mhaigh)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 2 obsolete files)

In bug 1195721 we created a homepage for Fennec.  This bug is to track the work needed to allow distros to be able to set that homepage.
I believe that to get this to work we would just need distributions to be able to set shared preferences (currently they can only set gecko preferences). I think we could just add another section to preferences.json for this, and parse that out in Distribution.java.

One thing to consider is that the distribution handling logic will run after the startup logic, so we won't have this home page the first time the browser is launched. Maybe we could do something to load the home page in the background while the first run experience is visible, but I worry that could be a bad experience. I would be fine with not showing the home page on first run, but we would need to make sure distributions are okay with that (or are aware of it).
Rather than just add support for a new homepage, I think it's wise that we add support for android preferences from distro files.

I've added support from within the preferences.json file, an example file would look like this:

  {
    "Global": {
      "id": "sample",
      "version": 1.0,
      "about": "Sample Distribution"
    },
    "Preferences": {
      "privacy.donottrackheader.enabled": true
    },
    "LocalizablePreferences": {
      "browser.search.defaultenginename": "Bugzilla@Mozilla"
    },
    "AndroidPreferences": {
      "homepage": "http://www.mozilla.com"
    }
Attachment #8664953 - Flags: feedback?(rnewman)
Attachment #8664953 - Flags: feedback?(margaret.leibovic)
Not a bad idea. Should "AndroidPreferences" also support locales?
(In reply to Mark Finkle (:mfinkle) from comment #3)
> Not a bad idea. Should "AndroidPreferences" also support locales?

I'm not sure it's worth adding that complexity right now, unless we have a strong reason for it. I believe the reason we have localized preferences for gecko prefs is to support that part gecko pref system (and that we were copying the desktop distribution implementation).

I say we should wait to add multi-locale pref support until we hear a request for that.
Comment on attachment 8664953 [details] [diff] [review]
Bug 1205335 - Allow homepage to be set by distributions

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

Looking good! As I mentioned in our 1x1, I also think that we should add a testcase to testDistribution.

::: mobile/android/base/distribution/Distribution.java
@@ +404,5 @@
> +            Telemetry.addToHistogram(HISTOGRAM_CODE_CATEGORY, CODE_CATEGORY_MALFORMED_DISTRIBUTION);
> +            return null;
> +        } catch (JSONException e) {
> +            Log.e(LOGTAG, "Error parsing preferences.json", e);
> +            Telemetry.addToHistogram(HISTOGRAM_CODE_CATEGORY, CODE_CATEGORY_MALFORMED_DISTRIBUTION);

You'll need to add this to Histograms.json, and make sure you get review from a data steward (bsmedberg, vladan, or ally).

::: mobile/android/base/preferences/AndroidDistributionPreferencesImport.java
@@ +29,5 @@
> +        if (androidPreferences == null) {
> +            return;
> +        }
> +
> +        JSONObject preferences = androidPreferences.preferences;

What's the reasoning for creating a new DistributionAndroidPreferences data type, as opposed to just returning a JSONObject directly?
Attachment #8664953 - Flags: feedback?(margaret.leibovic) → feedback+
> ::: mobile/android/base/distribution/Distribution.java
> @@ +404,5 @@
> > +            Telemetry.addToHistogram(HISTOGRAM_CODE_CATEGORY, CODE_CATEGORY_MALFORMED_DISTRIBUTION);
> > +            return null;
> > +        } catch (JSONException e) {
> > +            Log.e(LOGTAG, "Error parsing preferences.json", e);
> > +            Telemetry.addToHistogram(HISTOGRAM_CODE_CATEGORY, CODE_CATEGORY_MALFORMED_DISTRIBUTION);
> 
> You'll need to add this to Histograms.json, and make sure you get review
> from a data steward (bsmedberg, vladan, or ally).

We already have telemetry using these values elsewhere in the same file.  I assume no data steward is needed in this case.

> 
> ::: mobile/android/base/preferences/AndroidDistributionPreferencesImport.java
> @@ +29,5 @@
> > +        if (androidPreferences == null) {
> > +            return;
> > +        }
> > +
> > +        JSONObject preferences = androidPreferences.preferences;
> 
> What's the reasoning for creating a new DistributionAndroidPreferences data
> type, as opposed to just returning a JSONObject directly?

Initially I thought more work was going to be need by this object but it all happened elsewhere.
Bug 1205335 - Allow homepage to be set by distributions' r?mfinkle
Attachment #8665850 - Flags: review?(mark.finkle)
Comment on attachment 8665850 [details]
MozReview Request: Bug 1205335 - Allow homepage to be set by distributions; r?mfinkle

Bug 1205335 - Allow homepage to be set by distributions; r?mfinkle
Attachment #8665850 - Attachment description: MozReview Request: Bug 1205335 - Allow homepage to be set by distributions' r?mfinkle → MozReview Request: Bug 1205335 - Allow homepage to be set by distributions; r?mfinkle
Comment on attachment 8664953 [details] [diff] [review]
Bug 1205335 - Allow homepage to be set by distributions

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

Changeset comment is totally wrong :)

I agree with Margaret's comments, too.

::: mobile/android/base/distribution/Distribution.java
@@ +164,5 @@
>              instance = new Distribution(context);
>          }
>          return instance;
>      }
> +    public static class DistributionAndroidPreferences {

Newlines before and after.

@@ +165,5 @@
>          }
>          return instance;
>      }
> +    public static class DistributionAndroidPreferences {
> +        public JSONObject preferences;

final.

@@ +381,5 @@
>              Telemetry.addToHistogram(HISTOGRAM_CODE_CATEGORY, CODE_CATEGORY_MALFORMED_DISTRIBUTION);
>              return null;
>          }
>      }
> +    public DistributionAndroidPreferences getAndroidPreferences() {

Newline before.

@@ +382,5 @@
>              return null;
>          }
>      }
> +    public DistributionAndroidPreferences getAndroidPreferences() {
> +        File descFile = getDistributionFile("preferences.json");

final.

@@ +392,5 @@
> +        try {
> +            JSONObject all = new JSONObject(FileUtils.getFileContents(descFile));
> +
> +            if (!all.has("AndroidPreferences")) {
> +                Log.e(LOGTAG, "Distribution preferences.json has no AndroidPreferences entry!");

This isn't an error; existing distributions won't have this.

::: mobile/android/base/preferences/AndroidDistributionPreferencesImport.java
@@ +17,5 @@
> +import java.util.Iterator;
> +
> +public class AndroidDistributionPreferencesImport {
> +
> +    public static final String LOGTAG = AndroidDistributionPreferencesImport.class.getSimpleName();

This is too long to be an Android log tag. I'd suggest "DistroSharedPrefsImport".

@@ +37,5 @@
> +
> +        Iterator<?> keys = preferences.keys();
> +        final SharedPreferences.Editor sharedPreferences = GeckoSharedPrefs.forProfile(context).edit();
> +        while (keys.hasNext()) {
> +            String key = (String) keys.next();

I think we ought to whitelist keys.

@@ +47,5 @@
> +                continue;
> +            }
> +
> +            if (value instanceof String) {
> +                sharedPreferences.putString(GeckoPreferences.NON_PREF_PREFIX + key, (String) value);

This tree looks much like GeckoSharedPrefs.putEntry. Reuse that?
Attachment #8664953 - Flags: feedback?(rnewman) → feedback+
Status: NEW → ASSIGNED
Margaret is a better reviewer than I for this.
Attachment #8665850 - Flags: review?(mark.finkle) → review?(margaret.leibovic)
Comment on attachment 8665850 [details]
MozReview Request: Bug 1205335 - Allow homepage to be set by distributions; r?mfinkle

https://reviewboard.mozilla.org/r/20367/#review18755

Looks great.

::: mobile/android/base/distribution/Distribution.java:394
(Diff revision 2)
> +                Log.e(LOGTAG, "Distribution preferences.json has no AndroidPreferences entry!");

I don't think this needs to be an error level log, since it's perfectly acceptable for a distribution to not specify android prefs.

::: mobile/android/tests/browser/robocop/testDistribution.java:363
(Diff revision 2)
> +                    mAsserter.is(sharedPreferences.getFloat(GeckoPreferences.NON_PREF_PREFIX + name, 0), 3f, "check " + prefTestFloat);

Nice testcase!
Attachment #8665850 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 8665850 [details]
MozReview Request: Bug 1205335 - Allow homepage to be set by distributions; r?mfinkle

https://reviewboard.mozilla.org/r/20367/#review18781

::: mobile/android/base/preferences/AndroidDistributionPreferencesImport.java:46
(Diff revision 2)
> +            if (value instanceof String) {

I really want us to whitelist prefs.  This is just asking for trouble otherwise.

::: mobile/android/base/preferences/AndroidDistributionPreferencesImport.java:53
(Diff revision 2)
> +                sharedPreferences.putLong(GeckoPreferences.NON_PREF_PREFIX + key, (long) value);

You're doing something tricky here, right?  You're using that putLong((int) value) works for getInt and getLong?  Worth commenting, since it's assymetric from your test, and a thing that confused me before.

::: mobile/android/tests/browser/robocop/testDistribution.java:353
(Diff revision 2)
> +            for (String name : prefNames) {

I don't see the advantage of the loop over 5 statements, but as you wish.

::: mobile/android/tests/browser/robocop/testDistribution.java:361
(Diff revision 2)
> +                    mAsserter.is(sharedPreferences.getLong(GeckoPreferences.NON_PREF_PREFIX + name, 0), 2l, "check " + prefTestLong);

Make this a real long, out of int range.

::: mobile/android/tests/browser/robocop/testDistribution.java:363
(Diff revision 2)
> +                    mAsserter.is(sharedPreferences.getFloat(GeckoPreferences.NON_PREF_PREFIX + name, 0), 3f, "check " + prefTestFloat);

And make this an actual float, not a cast integer.
Attachment #8665850 - Flags: review+
Blocks: homepage
No longer depends on: homepage
Here's the latest work for this.  I've not implemented a whitelist, but all else is done.
Assignee: mhaigh → nobody
Status: ASSIGNED → NEW
(In reply to Martyn Haigh (:mhaigh) from comment #19)
> Created attachment 8671922 [details] [diff] [review]
> Allow android prefs to be set from distro file
> 
> Here's the latest work for this.  I've not implemented a whitelist, but all
> else is done.

I don't think we need a whitelist. Mozilla builds these distribution directories for partners, so we can review what's going into them.

This looks fine to me... let's just land it.
(In reply to :Margaret Leibovic from comment #20)
> (In reply to Martyn Haigh (:mhaigh) from comment #19)
> > Created attachment 8671922 [details] [diff] [review]
> > Allow android prefs to be set from distro file
> > 
> > Here's the latest work for this.  I've not implemented a whitelist, but all
> > else is done.
> 
> I don't think we need a whitelist. Mozilla builds these distribution
> directories for partners, so we can review what's going into them.
> 
> This looks fine to me... let's just land it.

We have consumers, like toonetown, that use distributions that Mozilla does not participate in.  And we're building a platform here.  rnewman and I both think a whitelist is important.  If you must, file follow-up -- but the amount of work is small and the potential for unintended consequences high.
(In reply to Nick Alexander :nalexander from comment #21)
> (In reply to :Margaret Leibovic from comment #20)

> > I don't think we need a whitelist. Mozilla builds these distribution
> > directories for partners, so we can review what's going into them.
> > 
> > This looks fine to me... let's just land it.
> 
> We have consumers, like toonetown, that use distributions that Mozilla does
> not participate in.  And we're building a platform here.  rnewman and I both
> think a whitelist is important.  If you must, file follow-up -- but the
> amount of work is small and the potential for unintended consequences high.

I'm not for or against a whirelist, but it does make me wonder:
* Firefox has done Gecko-prefs in distributions for years and not used a whitelist. Do we know if this was an issue?
* A whitelist means needing to be very proactive to add things or remove things from the list. We bascially become a blocker for any new distribution. It also becomes a go-faster issue.
* Are 3rd parties like toonetown a concern? I'm not sure. How is this different than 3rd parties using GeckoView? Would we limit usage there?

Food for thought.
(In reply to Nick Alexander :nalexander from comment #21)
> (In reply to :Margaret Leibovic from comment #20)
> > (In reply to Martyn Haigh (:mhaigh) from comment #19)
> > > Created attachment 8671922 [details] [diff] [review]
> > > Allow android prefs to be set from distro file
> > > 
> > > Here's the latest work for this.  I've not implemented a whitelist, but all
> > > else is done.
> > 
> > I don't think we need a whitelist. Mozilla builds these distribution
> > directories for partners, so we can review what's going into them.
> > 
> > This looks fine to me... let's just land it.
> 
> We have consumers, like toonetown, that use distributions that Mozilla does
> not participate in.  And we're building a platform here.  rnewman and I both
> think a whitelist is important.  If you must, file follow-up -- but the
> amount of work is small and the potential for unintended consequences high.

The amount of work to implement a whitelist is small, but then we have to maintain it. If someone is creating a distribution that Mozilla doesn't support, I'd argue it's their job to make sure they don't break things.

And like mfinkle said in the last comment, this is how the gecko prefs side of this has worked forever (including desktop). I'd rather wait until we hear this is actually a problem before investing in fixing it.
(In reply to :Margaret Leibovic from comment #23)
> (In reply to Nick Alexander :nalexander from comment #21)
> > (In reply to :Margaret Leibovic from comment #20)
> > > (In reply to Martyn Haigh (:mhaigh) from comment #19)
> > > > Created attachment 8671922 [details] [diff] [review]
> > > > Allow android prefs to be set from distro file
> > > > 
> > > > Here's the latest work for this.  I've not implemented a whitelist, but all
> > > > else is done.
> > > 
> > > I don't think we need a whitelist. Mozilla builds these distribution
> > > directories for partners, so we can review what's going into them.
> > > 
> > > This looks fine to me... let's just land it.
> > 
> > We have consumers, like toonetown, that use distributions that Mozilla does
> > not participate in.  And we're building a platform here.  rnewman and I both
> > think a whitelist is important.  If you must, file follow-up -- but the
> > amount of work is small and the potential for unintended consequences high.
> 
> The amount of work to implement a whitelist is small, but then we have to
> maintain it. If someone is creating a distribution that Mozilla doesn't
> support, I'd argue it's their job to make sure they don't break things.
> 
> And like mfinkle said in the last comment, this is how the gecko prefs side
> of this has worked forever (including desktop). I'd rather wait until we
> hear this is actually a problem before investing in fixing it.

Fair 'nough.  Ship it!
Assignee: nobody → martyn.haigh+bugzilla
Status: NEW → ASSIGNED
toonetown: will this patch let you drop that SharedPrefs patch of yours?  If yes, I'd be quite happy; if not, I'd like to know the shortcomings...
Flags: needinfo?(nathan)
Attachment #8664953 - Attachment is obsolete: true
Attachment #8665850 - Attachment is obsolete: true
(In reply to Nick Alexander :nalexander from comment #21)
> (In reply to :Margaret Leibovic from comment #20)
> > (In reply to Martyn Haigh (:mhaigh) from comment #19)
> > > Created attachment 8671922 [details] [diff] [review]
> > > Allow android prefs to be set from distro file
> > > 
> > > Here's the latest work for this.  I've not implemented a whitelist, but all
> > > else is done.
> > 
> > I don't think we need a whitelist. Mozilla builds these distribution
> > directories for partners, so we can review what's going into them.
> > 
> > This looks fine to me... let's just land it.
> 
> We have consumers, like toonetown, that use distributions that Mozilla does
> not participate in.  And we're building a platform here.  rnewman and I both
> think a whitelist is important.  If you must, file follow-up -- but the
> amount of work is small and the potential for unintended consequences high.

What is meant by "whitelist"?  It might be something that I could live without...

(BTW - I'll try and look at the patch a little later today and see how it would fit my needs...it appears like it very well might be just what I would want)
(In reply to Nathan Toone [:toonetown] from comment #26)
> (In reply to Nick Alexander :nalexander from comment #21)
> > (In reply to :Margaret Leibovic from comment #20)
> > > (In reply to Martyn Haigh (:mhaigh) from comment #19)
> > > > Created attachment 8671922 [details] [diff] [review]
> > > > Allow android prefs to be set from distro file
> > > > 
> > > > Here's the latest work for this.  I've not implemented a whitelist, but all
> > > > else is done.
> > > 
> > > I don't think we need a whitelist. Mozilla builds these distribution
> > > directories for partners, so we can review what's going into them.
> > > 
> > > This looks fine to me... let's just land it.
> > 
> > We have consumers, like toonetown, that use distributions that Mozilla does
> > not participate in.  And we're building a platform here.  rnewman and I both
> > think a whitelist is important.  If you must, file follow-up -- but the
> > amount of work is small and the potential for unintended consequences high.
> 
> What is meant by "whitelist"?  It might be something that I could live
> without...

A whitelist would specify a small set of prefs that you could set.  As it is now, you can set any pref you care to.

> (BTW - I'll try and look at the patch a little later today and see how it
> would fit my needs...it appears like it very well might be just what I would
> want)

Thanks.
https://hg.mozilla.org/mozilla-central/rev/c043c708384c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
This actually looks very promising to me for what I'm needing to use it for.  I'm going to try and rework my distribution to use this instead of my big fat ugly java patch. :)
Flags: needinfo?(nathan)
Is it possible to set a preference in the distribution *without* the "android.not_a_preference." prefix?  If not, what is the reasoning behind not allowing that?
(In reply to Nathan Toone [:toonetown] from comment #32)
> Is it possible to set a preference in the distribution *without* the
> "android.not_a_preference." prefix?  If not, what is the reasoning behind
> not allowing that?

Yes, it is possible.

This "not_a_preference" business is a convention we developed to distinguish preferences that are not stored as Gecko preferences (originally all prefs were stored in Gecko, none in SharedPreferences). Some archaeology revealed this was added here:
http://hg.mozilla.org/mozilla-central/rev/81cf04b16eb1

I'm not sure this convention makes sense anymore, but here we are.
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.