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

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Runtime, defect)

Description

[Jan Keromnes [:janx]]

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.

Comment 1

[Jan Keromnes [:janx]]

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

Comment 2

[Jan Keromnes [:janx]]

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.

Comment 3

[Jan Keromnes [:janx]]

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?

Comment 4

[Dão Gottwald [: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).

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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.

Comment 6

[Jan Keromnes [:janx]]

Attachment: Sync app.update.{channel,url} settings with preferences. r=21

Comment 7

[Jan Keromnes [:janx]]

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.

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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

Comment 9

[Jan Keromnes [:janx]]

Attachment: Sync app.update.{channel,url} settings with preferences. r=21

Comment 10

[Jan Keromnes [:janx]]

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 11

[Jan Keromnes [:janx]]

Comment on attachment 8410143 [details] [diff] [review]
Sync app.update.{channel,url} settings with preferences. r=21

21:r+

Comment 12

[Jan Keromnes [:janx]]

Try push just in case https://tbpl.mozilla.org/?tree=Try&rev=8690e4e02eb9

Comment 13

[Jan Keromnes [:janx]]

Apart from the few unrelated-looking fails, the try push seems green to me.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/1ff975b6a290

Comment 15

[[:fabrice] Fabrice Desré]

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

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1ff975b6a290

Comment 17

[Jan Keromnes [:janx]]

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?

Comment 18

[Jan Keromnes [:janx]]

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.

Comment 19

[Jan Keromnes [:janx]]

Attachment: Sync app.update.{channel,url} settings with preferences. r=21

Comment 20

[Jan Keromnes [:janx]]

Attachment: Sync app.update.{channel,url} settings with preferences. r=21

Comment 21

[Jan Keromnes [:janx]]

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.

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/b2g-inbound/rev/1d7b9a01b8cf

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1d7b9a01b8cf