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

and now with onerror for performance

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).

Needed for updates.

delete old index

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 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"

Any reason not to land this?

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

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.

(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

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

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

The 2nd part missed the merge to aurora.