Settings: upgrade settings DB after settings.json changed

RESOLVED FIXED in Firefox 21

Status

()

P1
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: gwagner, Assigned: gwagner)

Tracking

unspecified
B2G C4 (2jan on)
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(blocking-basecamp:+, firefox19 wontfix, firefox20 affected, firefox21 fixed, b2g18 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

6 years ago
blocking-basecamp: --- → ?
(Assignee)

Updated

6 years ago
Assignee: nobody → anygregor
(Assignee)

Comment 1

6 years ago
Created attachment 692418 [details] [diff] [review]
patch

Add missing settings from settings.json if we bump the DB version.
Attachment #692418 - Flags: review?(bent.mozilla)
(Assignee)

Comment 2

6 years ago
Created attachment 692429 [details] [diff] [review]
patch

and now with onerror for performance
Attachment #692418 - Attachment is obsolete: true
Attachment #692418 - Flags: review?(bent.mozilla)
Attachment #692429 - Flags: review?(bent.mozilla)
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).
Attachment #692429 - Flags: review?(bent.mozilla) → review+
Needed for updates.
blocking-basecamp: ? → +
Priority: -- → P1
Target Milestone: --- → B2G C3 (12dec-1jan)
(Assignee)

Comment 5

6 years ago
Created attachment 693119 [details] [diff] [review]
patch
Attachment #692429 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #693119 - Flags: review?(bent.mozilla)
(Assignee)

Comment 6

6 years ago
Created attachment 693145 [details] [diff] [review]
patch

delete old index
Attachment #693119 - Attachment is obsolete: true
Attachment #693119 - Flags: review?(bent.mozilla)
Attachment #693145 - Flags: review?(bent.mozilla)
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.
Attachment #693145 - Flags: review?(bent.mozilla)
(Assignee)

Comment 8

6 years ago
Created attachment 695034 [details] [diff] [review]
patch
Attachment #693145 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #695034 - Flags: review?(bent.mozilla)
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"
Attachment #695034 - Flags: review?(bent.mozilla) → review+
Any reason not to land this?
Target Milestone: B2G C3 (12dec-1jan) → B2G C4 (2jan on)
(Assignee)

Comment 11

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

Will land it today.
https://hg.mozilla.org/mozilla-central/rev/0210c5931bde
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Did you mean to land

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

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

Comment 16

6 years ago
(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
status-firefox19: --- → wontfix
status-firefox20: --- → fixed
The 2nd part missed the merge to aurora.
status-firefox20: fixed → affected
status-firefox21: --- → fixed
(Assignee)

Updated

6 years ago
Duplicate of this bug: 750466
You need to log in before you can comment on or make changes to this bug.