821814 - Settings: upgrade settings DB after settings.json changed

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect, P1)

Comment 1

[Gregor Wagner [:gwagner]]

Attachment: patch

Add missing settings from settings.json if we bump the DB version.

Comment 2

[Gregor Wagner [:gwagner]]

Attachment: patch

and now with onerror for performance

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 692429 [details] [diff] [review]
patch

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

::: dom/settings/SettingsDB.jsm
@@ +13,5 @@
>    if (DEBUG) dump("-*- SettingsDB: " + s + "\n");
>  }
>  
>  this.SETTINGSDB_NAME = "settings";
> +this.SETTINGSDB_VERSION = 2;

You should probably add a big comment to settings.json saying that new settings have to have a db schema bump in order to work.

@@ +74,5 @@
> +                                  settingValue: settings[setting]
> +                                });
> +      req.onerror = function(event) {
> +        if (event.target.error.name == "ConstraintError") {
> +          event.preventDefault();

You should add a comment here saying that you don't want to overwrite existing settings (hence add instead of put).

Comment 4

[Andrew Overholt [:overholt]]

Needed for updates.

Comment 5

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 6

[Gregor Wagner [:gwagner]]

Attachment: patch

delete old index

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 693145 [details] [diff] [review]
patch

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

::: dom/settings/SettingsDB.jsm
@@ +77,5 @@
> +      let cursor = event.target.result;
> +      if (cursor) {
> +        let value = cursor.value;
> +        if (value.settingName in settings) {
> +          value.defaultValue = settings[value.settingName];

You're taking care of defaultValue here, but the old 'settingValue' property might still exist, and if so you should delete/rename it to 'userValue' right?

@@ +80,5 @@
> +        if (value.settingName in settings) {
> +          value.defaultValue = settings[value.settingName];
> +          delete settings[value.settingName];
> +          cursor.update(value);
> +        } else {

If you remove the dump below then this should be 'else if'

@@ +81,5 @@
> +          value.defaultValue = settings[value.settingName];
> +          delete settings[value.settingName];
> +          cursor.update(value);
> +        } else {
> +          dump("Remove setting from SettingsDB: " + value.settingName +"\n");

Nit: Remove dump or put behind DEBUG check.

@@ +82,5 @@
> +          delete settings[value.settingName];
> +          cursor.update(value);
> +        } else {
> +          dump("Remove setting from SettingsDB: " + value.settingName +"\n");
> +          if (value.userValue) {

It could be 'settingValue' too.

@@ +92,5 @@
> +        }
> +        cursor.continue();
> +      } else {
> +        for (let name in settings) {
> +          objectStore.add({ settingName: name, defaultValue: settings[name], userValue: null });

One thing I didn't think of, is null a valid settings value? Would undefined be better?

::: dom/settings/SettingsManager.js
@@ +29,5 @@
>  
>  function SettingsLock(aSettingsManager)
>  {
>    this._open = true;
> +  this._isBusy = false;

Below you use 'isBusy' in several places, but in others you use '_isBusy'!

@@ +71,3 @@
>              checkKeyRequest.onsuccess = function (event) {
>                if (!event.target.result) {
> +                debug("MOZSETTINGS-SET-WARNING: " + key + " is not in the database.\n");

Nit: move this down below (into the else block) so you don't test again, and hide behind 'if (DEBUG)'

@@ +85,5 @@
> +                let obj = {settingName: key, defaultValue: defaultValue, userValue: userValue};
> +                if (DEBUG) debug("store1: " + JSON.stringify(obj));
> +                setReq = store.put(obj);
> +              } else {
> +                //Workaround for cloning issues

We should file a bug if this is still broken.

@@ +99,5 @@
> +                  lock._open = true;
> +                  Services.DOMRequest.fireSuccess(request, 0);
> +                  lock._isBusy = false;
> +                  if (!lock._requests.isEmpty()) {
> +                    lock.process();

I think you want to set lock._open back to false before you process anything else, right? In fact you might want to do lock.process on a setTimeout(0)...

@@ +103,5 @@
> +                    lock.process();
> +                  }
> +                  lock._open = false;
> +                }
> +                cpmm.sendAsyncMessage("Settings:Changed", { key: key, value: userValue });

You definitely want to call this before you start processing more items.

@@ +115,3 @@
>              }
> +            if (last) {
> +              lock._open = true;

Hm, why do you do this here?

@@ +129,3 @@
>  
>              if (event.target.result.length == 0) {
> +              debug("MOZSETTINGS-GET-WARNING: " + info.name + " is not in the database.\n");

Nit: hide behind 'if (DEBUG)'

@@ +138,5 @@
>  
>              for (var i in event.target.result) {
>                let result = event.target.result[i];
>                var name = result.settingName;
> +              dump("VAL: " + result.userValue +", " + result.defaultValue + "\n");

Nit: Hide this or remove.

@@ +139,5 @@
>              for (var i in event.target.result) {
>                let result = event.target.result[i];
>                var name = result.settingName;
> +              dump("VAL: " + result.userValue +", " + result.defaultValue + "\n");
> +              var value = result.userValue !== undefined ? result.userValue : result.defaultValue;

Hm, earlier userValue was set to null, right? I like undefined better, but I think this won't work.

@@ +210,5 @@
>  
>      if (this._settingsManager.hasWritePrivileges) {
>        let req = Services.DOMRequest.createRequest(this._settingsManager._window);
>        if (DEBUG) debug("send: " + JSON.stringify(aSettings));
> +      let settings = aSettings;

This won't actually copy, you should do something else.

Comment 8

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 695034 [details] [diff] [review]
patch

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

Sorry for the delay! Somehow I forgot about this over eggnog.

::: dom/settings/SettingsDB.jsm
@@ +81,5 @@
> +        if (value.settingName in settings) {
> +          if (DEBUG) debug("Upgrade " +settings[value.settingName]);
> +          value.defaultValue = settings[value.settingName];
> +          delete settings[value.settingName];
> +          if (value.settingValue) {

This is wrong for things like "" or [] or null, right? Dunno if that matters but maybe you should do |"settingValue" in value|?

@@ +86,5 @@
> +            value.userValue = value.settingValue;
> +            delete value.settingValue;
> +          }
> +          cursor.update(value);
> +        } else if (value.userValue || value.settingValue) {

Same here?

@@ +87,5 @@
> +            delete value.settingValue;
> +          }
> +          cursor.update(value);
> +        } else if (value.userValue || value.settingValue) {
> +            value.defaultValue = undefined;

Nit: I think the indent on this whole block is 2 spaces too long.

::: dom/settings/SettingsManager.js
@@ +85,5 @@
> +                setReq = store.put(obj);
> +              } else {
> +                //Workaround for cloning issues
> +                let defaultVal = JSON.parse(JSON.stringify(defaultValue));
> +                let userVal = JSON.parse(JSON.stringify(userValue));

Let's not forget to file this bug!

@@ +290,5 @@
>      var lock = new SettingsLock(this);
>      this._locks.enqueue(lock);
>      this._settingsDB.ensureDB(
>        function() { lock.createTransactionAndProcess(); },
> +      function() { dump("Can not open Settings DB. Trying to open an old version?\n"); },

Nit: "Cannot"

Comment 10

[JP Rosevear [:jpr]]

Any reason not to land this?

Comment 11

[Gregor Wagner [:gwagner]]

(In reply to JP Rosevear [:jpr] from comment #10)
> Any reason not to land this?

Will land it today.

Comment 12

[Gregor Wagner [:gwagner]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0210c5931bde

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0210c5931bde

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Did you mean to land

    1.16 -const DEBUG = false;
    1.17 +const DEBUG = true;

?

Comment 15

[Ryan VanderMeulen [:RyanVM]]

This doesn't apply cleanly to aurora/b2g18. Please post a branch-specific patch or get uplift approval for dependencies.

Comment 16

[Gregor Wagner [:gwagner]]

(In reply to :Ms2ger from comment #14)
> Did you mean to land
> 
>     1.16 -const DEBUG = false;
>     1.17 +const DEBUG = true;
> 
> ?

Nope. Thanks for checking!
https://hg.mozilla.org/integration/mozilla-inbound/rev/27ed16bab80f

Comment 17

[Gregor Wagner [:gwagner]]

and on mc
https://hg.mozilla.org/mozilla-central/rev/b9634bb5b16f

Comment 18

[Gregor Wagner [:gwagner]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/737e0654185d

Comment 19

[Gregor Wagner [:gwagner]]

disable debug flag for good:
https://hg.mozilla.org/integration/mozilla-inbound/rev/188622e5ffd5

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/27ed16bab80f

Comment 21

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/188622e5ffd5

Comment 22

[Ed Morley [:emorley]]

The 2nd part missed the merge to aurora.