Closed Bug 995206 Opened 6 years ago Closed 6 years ago

Sync app.update.{channel,url} settings with preferences.

Categories

(Firefox OS Graveyard :: Runtime, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
1.4 S6 (25apr)

People

(Reporter: janx, Assigned: janx)

References

Details

Attachments

(1 file, 4 obsolete files)

To expose the app.update.{channel,url} preferences in Firefox OS' Developer menu, we need to create settings that are set to and update these preferences.
Component: Preferences → Gaia::Settings
Product: Firefox → Firefox OS
Version: Trunk → unspecified
Comment on attachment 8405406 [details] [diff] [review]
Sync app.update.{channel,url} settings with preferences.

Vivien, could you please have a look? Please complain if you feel this needs tests.
Attachment #8405406 - Flags: review?(21)
Dão, this is a gecko change in the b2g/ folder, so I thought it wouldn't make sense to file it under Gaia::*. Could you please explain why you moved it there, so that in the future I'll know better where to file bugs?
Flags: needinfo?(dao)
You'll need to ask someone familiar with the b2g/ structure. All I know is b2g patches belong to the "Firefox OS" product, not "Firefox" (the desktop browser).
Component: Gaia::Settings → General
Flags: needinfo?(dao)
Component: General → Runtime
Comment on attachment 8405406 [details] [diff] [review]
Sync app.update.{channel,url} settings with preferences.

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

::: b2g/chrome/content/settings.js
@@ +564,5 @@
> +    window.navigator.mozSettings.createLock().set({
> +      'deviceinfo.update_channel': value
> +    });
> +  });
> +})();

Let's add a new flag to the simple mapping method at the end of the file. The flag would let you tell that the pref changes takes over settings changes.
Attachment #8405406 - Flags: review?(21)
Attachment #8405406 - Attachment is obsolete: true
Comment on attachment 8408342 [details] [diff] [review]
Sync app.update.{channel,url} settings with preferences. r=21

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

Thanks for the review!

Here is a patch using a new `resetToPref` flag with the simple mapping mechanism.
Attachment #8408342 - Flags: review?(21)
Comment on attachment 8408342 [details] [diff] [review]
Sync app.update.{channel,url} settings with preferences. r=21

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

::: b2g/chrome/content/settings.js
@@ +722,3 @@
>    }
>  
> +  let setPref;

nit: s/setPref/setPrefFunction
Attachment #8408342 - Flags: review?(21) → review+
Attachment #8408342 - Attachment is obsolete: true
Cheers for the review!

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #8)
> ::: b2g/chrome/content/settings.js
> @@ +722,3 @@
> >    }
> >  
> > +  let setPref;
> 
> nit: s/setPref/setPrefFunction

Added a comment instead to make sure people understand it's a function.
Comment on attachment 8410143 [details] [diff] [review]
Sync app.update.{channel,url} settings with preferences. r=21

21:r+
Attachment #8410143 - Flags: review+
Apart from the few unrelated-looking fails, the try push seems green to me.
Keywords: checkin-needed
I backed out since this patch is causing:

E/GeckoConsole(21949): [JavaScript Error: "TypeError: setPref is not a function" {file: "chrome://b2g/content/settings.js" line: 741}]
E/GeckoConsole(21949): [JavaScript Error: "TypeError: setPref is not a function" {file: "chrome://b2g/content/settings.js" line: 741}]
E/GeckoConsole(21949): [JavaScript Error: "TypeError: setPref is not a function" {file: "chrome://b2g/content/settings.js" line: 741}]
E/GeckoConsole(21949): [JavaScript Error: "TypeError: setPref is not a function" {file: "chrome://b2g/content/settings.js" line: 741}]
E/GeckoConsole(21949): [JavaScript Error: "TypeError: setPref is not a function" {file: "chrome://b2g/content/settings.js" line: 741}]
E/GeckoConsole(21949): [JavaScript Error: "TypeError: setPref is not a function" {file: "chrome://b2g/content/settings.js" line: 741}]
E/GeckoConsole(21949): [JavaScript Error: "TypeError: setPref is not a function" {file: "chrome://b2g/content/settings.js" line: 741}]

during startup.

https://hg.mozilla.org/integration/b2g-inbound/rev/ece426b71e9f
https://hg.mozilla.org/mozilla-central/rev/1ff975b6a290
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.4 S6 (25apr)
Wes, it looks like you cut b2g-inbound just after my patch, leaving Fabrice's backout of my patch behind.

I'll investigate the `setPref` issue, but maybe you'll want the patch backed out on mozilla-central, either by merging b2g-inbound again or by backing it out separately?
Status: RESOLVED → REOPENED
Flags: needinfo?(kwierso)
Resolution: FIXED → ---
Removing the needinfo since you rolled b2g-inbound into https://hg.mozilla.org/mozilla-central/ again. I was able to fix my issue, but I'm waiting for http://git.mozilla.org/releases/gecko.git to update to mozilla-central so that I can rebase on top of the backout.
Flags: needinfo?(kwierso)
Attachment #8410143 - Attachment is obsolete: true
Comment on attachment 8411054 [details] [diff] [review]
Sync app.update.{channel,url} settings with preferences. r=21

Thank you Fabrice for catching the logcat errors! The problem was due to:

> let defaultValue = setting.defaultValue || setting;

falling back to `setting` when `setting.defaultValue == false`, whereas the intention was to fall back to `setting` only when `setting.defaultValue === undefined`. Changed this line to:

> let defaultValue = setting.defaultValue;
> if (defaultValue === undefined) {
>   defaultValue = setting;
> }

Keeping Vivien's r+ since this is only a minor change to the patch.
Attachment #8411054 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1d7b9a01b8cf
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Depends on: 1000252
You need to log in before you can comment on or make changes to this bug.