Allow for the setting of application level preferences from a distribution

RESOLVED FIXED in Firefox 55

Status

()

Firefox for Android
Distributions
RESOLVED FIXED
a year ago
7 months ago

People

(Reporter: mkaply, Assigned: mkaply)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 affected, firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

a year ago
Currently the distribution mechanism can only be used to set profile level preferences.

We should allow app level preferences to be set as well (for instance, session restore).
(Assignee)

Updated

a year ago
Blocks: 1314038
Blocks: 1293713
Comment hidden (mozreview-request)

Comment 2

7 months ago
mozreview-review
Comment on attachment 8850272 [details]
Bug 1295675 - Allow app level preferences.

https://reviewboard.mozilla.org/r/122908/#review125314

Nice!

Please update our wiki page:
https://wiki.mozilla.org/Mobile/Distribution_Files
Attachment #8850272 - Flags: review?(s.kaspari) → review+

Updated

7 months ago
Keywords: checkin-needed

Comment 3

7 months ago
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/565c6c66205d
Allow app level preferences. r=sebastian
Keywords: checkin-needed

Comment 4

7 months ago
sorry had to back this out for android-checkstyle failure like https://treeherder.mozilla.org/logviewer.html#?job_id=86095946&repo=autoland&lineNumber=3545

https://hg.mozilla.org/integration/autoland/rev/78c1934f6a77
Flags: needinfo?(cnevinchen)

Comment 5

7 months ago
Created attachment 8850857 [details]
patch.txt

Sorry I've added the hook to check for style before I push for review so this won't happen aggain.
Flags: needinfo?(cnevinchen) → needinfo?(ihsiao)

Comment 6

7 months ago
Please reopen the reviewboard, get review+, and then ask for checkin.
Flags: needinfo?(ihsiao)
Comment hidden (mozreview-request)

Comment 8

7 months ago
Comment on attachment 8850272 [details]
Bug 1295675 - Allow app level preferences.

Hi Sebastian.
I've added two spaces for check style.
Please help review it.. thank you! (per comment 6)


Best regards,
Nevin
Flags: needinfo?(s.kaspari)

Comment 9

7 months ago
Comment on attachment 8850857 [details]
patch.txt

I've resent the review-request so this patch is not needed.

Comment 10

7 months ago
mozreview-review
Comment on attachment 8850272 [details]
Bug 1295675 - Allow app level preferences.

https://reviewboard.mozilla.org/r/122908/#review125834
Attachment #8850857 - Attachment is obsolete: true
Flags: needinfo?(s.kaspari)

Updated

7 months ago
Keywords: checkin-needed

Comment 11

7 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f62c008a7b9f
Allow app level preferences. r=sebastian
Keywords: checkin-needed
I had to back this out for testdistribution failures like https://treeherder.mozilla.org/logviewer.html#?job_id=86303408&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/f9acfdca68a4
Flags: needinfo?(mozilla)
(Assignee)

Comment 13

7 months ago
mozreview-review
Comment on attachment 8850272 [details]
Bug 1295675 - Allow app level preferences.

https://reviewboard.mozilla.org/r/122908/#review126480

::: mobile/android/base/java/org/mozilla/gecko/preferences/DistroSharedPrefsImport.java:34
(Diff revision 2)
> +        if (appPref.length() != 0) {
> +            applyPreferences(appPref, GeckoSharedPrefs.forApp(context).edit());
> +        }
>  
> -        final JSONObject preferences = distribution.getAndroidPreferences();
> -        if (preferences.length() == 0) {
> +        final JSONObject profilePref = distribution.getPreferences(Distribution.PREF_KEY_PROFILE_PREFERENCES);
> +        if (appPref.length() != 0) {

This should be profilePref
Attachment #8850272 - Flags: review-
(Assignee)

Comment 14

7 months ago
Test failure was caused by a typo in the patch.

I've added the issue to the review request.
Flags: needinfo?(mozilla)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 18

7 months ago
This was backed out, so you'll need to submit a complete patch again.

Comment 19

7 months ago
Sorry I don't understand.
Do you mean I should combine the two patch in this bug and submit a complete patch again?
Thank you!
Flags: needinfo?(mozilla)
(Assignee)

Comment 20

7 months ago
Yes, to make it easier for someone else to check in the code, it's better to have one complete patch.
Flags: needinfo?(mozilla)
Comment hidden (mozreview-request)

Updated

7 months ago
Attachment #8851936 - Attachment is obsolete: true
Attachment #8851936 - Flags: review?(mozilla)

Comment 22

7 months ago
How about it now? Thanks!
Flags: needinfo?(mozilla)
(Assignee)

Comment 23

7 months ago
Awesome, thanks. I'll review and check in.
Flags: needinfo?(mozilla)
(Assignee)

Comment 24

7 months ago
mozreview-review
Comment on attachment 8850272 [details]
Bug 1295675 - Allow app level preferences.

https://reviewboard.mozilla.org/r/122908/#review127312
Attachment #8850272 - Flags: review?(mozilla) → review+
(Assignee)

Comment 25

7 months ago
Nevin:

Can you reopen the review request?

https://reviewboard.mozilla.org/r/122906/

Then we can resubmit to autoland.

I've already done successful try runs.

Comment 26

7 months ago
Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/9c6514e9b04b
Allow app level preferences. r=mkaply,sebastian

Comment 27

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9c6514e9b04b
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.