Andy McKay Reporter
	Description • 8 years ago

This is a bug to create a module to allow preference flipping for WebExtensions. To be clear, this wouldn't allow an extension to directly flip any arbitrary preference. 

It would be an underlying module with no direct access from an extension. It would then allow us to easily write pref flipping for privacy, accessibility and so on.

Things that we want to think about in this module are:
* what happens when a user un-installs the extension
* what happens when a user disables the extension
* what happens when multiple extensions flip the same pref
* what happens when the user changes the pref

Some of these may have been addressed in prior art in the SDK that we could crib or re-use.

	Bob Silverberg [:bsilverberg] Assignee
	Comment 1 • 8 years ago

As I will be working on the privacy API this quarter, I am taking this too.

	Andy McKay Reporter
	Comment 6 • 8 years ago

Can I confirm that the intent here is that the most recent install that changes the pref is the one that is used. That was my understanding from the meeting we had and bug 1330494 comment 1 seems to say the opposite.

To me it makes sense that if you install a new add-on, it should override the old one. Then when you uninstall the new add-on it reverts back in the stack to the old one.

At least that was my understanding from the meeting.

	Bob Silverberg [:bsilverberg] Assignee
	Comment 7 • 8 years ago

No, my understanding is the opposite, as I said in the comment you reference. Kris was doing most of the talking in the meeting, so let's hear from him. Kris, should the pref that is used be the most recently set one, or the least recently set one?

	Bob Silverberg [:bsilverberg] Assignee
	Comment 10 • 8 years ago

I just pushed some patches for the ExtensionSettingsStore and the ExtensionPreferencesManager. I think all of the basic functionality that these modules need is there, but one thing I haven't done yet is to hook up the `unsetAll` function of the ExtensionPreferencesManager to the uninstall and disable events for WebExtensions. I'm not sure if we want to put that into the ExtensionPreferencesManager itself, or somewhere else that will then call the unsetAll function that exists in the ExtensionPreferencesManager.

	Bob Silverberg [:bsilverberg] Assignee
	Comment 13 • 8 years ago

I just started looking at the Privacy API which needs this functionality (which I should have done first), and I can see that some of what we need for Privacy is likely not covered by the work I've done thus far, so it probably makes sense to hold off reviewing this until I get a chance to take a good look at Privacy and possibly make some changes to these modules.

	Bob Silverberg [:bsilverberg] Assignee
	Comment 14 • 8 years ago

Based on the conversation we had on IRC today, we have decided that:
- The most recently installed extension will have the highest precedence
- Precedence will be based on the install date of the extension, not the date on which the setting change is requested
- Updates should not affect precedence.

	Andrew Swan [:aswan]
	Comment 17 • 8 years ago
mozreview-review

Comment on attachment 8826360 [details]
Bug 1320736 - Part 2: Create ExtensionSettingsStore module,

https://reviewboard.mozilla.org/r/104294/#review108416

In addition to some small line item things below, two big things:
1. I haven't heard that we've got the okay to land async/await code...
2. We've been back and forth on this a lot of times but I thought the consensus was that the behavior would be "newest wins" but you appear to have implemented "oldest wins"?  I didn't do detailed review on the test since if we switch the behavior, a lot of the test will end up being rewritten...

::: toolkit/components/extensions/ExtensionSettingsStore.jsm:7
(Diff revision 5)
> + * This module is used for storing changes to settings that are
> + * requested by extensions, and for finding out what the current value
> + * of a setting should be, based on the precedence chain.

nit: I think this comment could be improved to give a little more detail to a reader who doesn't already know eg what "the precedence chain" is

::: toolkit/components/extensions/ExtensionSettingsStore.jsm:27
(Diff revision 5)
> +XPCOMUtils.defineLazyModuleGetter(this, "JSONFile",
> +                                  "resource://gre/modules/JSONFile.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "OS",
> +                                  "resource://gre/modules/osfile.jsm");
> +
> +const SCHEMA_VERSION = 1;

You don't seem to actually use this anywhere?  If this is meant to let us change the on-disk format, then you should refuse to load from a newer schema version.

::: toolkit/components/extensions/ExtensionSettingsStore.jsm:29
(Diff revision 5)
> +XPCOMUtils.defineLazyModuleGetter(this, "OS",
> +                                  "resource://gre/modules/osfile.jsm");
> +
> +const SCHEMA_VERSION = 1;
> +const JSON_FILE_NAME = "extension-settings.json";
> +const STORE_PATH = OS.Path.join(OS.Constants.Path.profileDir, JSON_FILE_NAME);

I believe this should use `Services.dirsvc.get("ProfD")`

::: toolkit/components/extensions/ExtensionSettingsStore.jsm:39
(Diff revision 5)
> + * objects are available.
> + *
> + * Currently only adds a prefs object, but will be used in future for other
> + * object types.
> + */
> +function dataPostProcessor(data) {

Just inline this in `getStore()` please

::: toolkit/components/extensions/ExtensionSettingsStore.jsm:60
(Diff revision 5)
> +  }
> +  return _store;
> +}
> +
> +// Return the top item for a key, or the initialValue if no top item exists.
> +function getTopItem(store, type, key) {

why take `store` as a parameter instead of just referencing `_store` directly?

::: toolkit/components/extensions/ExtensionSettingsStore.jsm:61
(Diff revision 5)
> +  return _store;
> +}
> +
> +// Return the top item for a key, or the initialValue if no top item exists.
> +function getTopItem(store, type, key) {
> +  let keyInfo = store.data[type][key];

are you certain that `store.data[type]` will always exist when you get here?

::: toolkit/components/extensions/ExtensionSettingsStore.jsm:63
(Diff revision 5)
> +
> +// Return the top item for a key, or the initialValue if no top item exists.
> +function getTopItem(store, type, key) {
> +  let keyInfo = store.data[type][key];
> +
> +  if (keyInfo) {

you can just do an early return `if (!keyInfo)` to avoid an extra level of indentation here

::: toolkit/components/extensions/ExtensionSettingsStore.jsm:77
(Diff revision 5)
> +this.ExtensionSettingsStore = {
> +  /**
> +   * Adds a setting to the store, possibly returning the current top precedent
> +   * setting.
> +   *
> +   * @param {object} extension The extension for which a setting is being added.

This should actually be Extension, not just an object right?

::: toolkit/components/extensions/ExtensionSettingsStore.jsm:97
(Diff revision 5)
> +  async addSetting(extension, type, key, value, initialValueCallback) {
> +    if (typeof initialValueCallback != "function") {
> +      // TODO: This format (or using an Error object) seems to be required in
> +      // order for Assert.rejects to work. Either a string, or an actual Error
> +      // object work, but using {message: "text"} doesn't seem to work.
> +      return Promise.reject("initialValueCallback must be a function.");

Since this is an async function, you can just throw here

::: toolkit/components/extensions/ExtensionSettingsStore.jsm:104
(Diff revision 5)
> +
> +    let id = extension.id;
> +    // We need to know the install date of the extension for precedence handling.
> +    let addon = await AddonManager.getAddonByID(id);
> +    let installDate = addon.installDate;
> +    // dump(`\n----- extension: ${JSON.stringify(addon.installDate, null, 4)}`);

remove this before landing

::: toolkit/components/extensions/ExtensionSettingsStore.jsm:109
(Diff revision 5)
> +    // dump(`\n----- extension: ${JSON.stringify(addon.installDate, null, 4)}`);
> +
> +    let store = await getStore();
> +    store.ensureDataReady();
> +
> +    if (!store.data[type][key]) {

This looks like you need to check for the presence of `store.data[type]` first

::: toolkit/components/extensions/ExtensionSettingsStore.jsm:113
(Diff revision 5)
> +
> +    if (!store.data[type][key]) {
> +      // The setting for this key does not exist. Set the initial value.
> +      let initialValuePromise = initialValueCallback(key);
> +      if (!initialValuePromise.then) {
> +        return Promise.reject("initialValueCallback must return a promise.");

I don't think you need to test this here, just await it directly and await will do any necessary checking and throw if something of the wrong type is returned

::: toolkit/components/extensions/ExtensionSettingsStore.jsm:123
(Diff revision 5)
> +        precedenceList: [],
> +      };
> +    }
> +    let keyInfo = store.data[type][key];
> +    // Check for this item in the precedenceList.
> +    let foundItem = keyInfo.precedenceList.find(item => item.id == id);

You can avoid searching the list twice by just using `findIndex()` here

::: toolkit/components/extensions/ExtensionSettingsStore.jsm:164
(Diff revision 5)
> +    let store = await getStore();
> +    store.ensureDataReady();
> +
> +    let keyInfo = store.data[type][key];
> +
> +    if (keyInfo) {

early return `if (!keyInfo)` to avoid nesting

::: toolkit/components/extensions/ExtensionSettingsStore.jsm:168
(Diff revision 5)
> +
> +    if (keyInfo) {
> +      let id = extension.id;
> +      let foundItem = keyInfo.precedenceList.find(item => item.id == id);
> +
> +      if (foundItem) {

same early return comment

::: toolkit/components/extensions/ExtensionSettingsStore.jsm:169
(Diff revision 5)
> +        let removingTopItem = keyInfo.precedenceList.indexOf(foundItem) == 0;
> +        keyInfo.precedenceList = keyInfo.precedenceList.filter(item => item.id != id);

this code is making 3 passes over the list.  efficiency here isn't a huge concern but if you just use `.findIndex()` above and `splice()` here, I think it would also be more readable.

::: toolkit/components/extensions/ExtensionSettingsStore.jsm:195
(Diff revision 5)
> +    let store = await getStore();
> +    store.ensureDataReady();
> +
> +    let keysObj = store.data[type];
> +    let items = [];
> +    for (let key in keysObj) {

I think you need to test that keyObj is actually an object

::: toolkit/components/extensions/test/xpcshell/test_ext_extensionSettingsStore.js:37
(Diff revision 5)
> +function initialValue(key) {
> +  return `key:${key}`;
> +}
> +
> +function initialValueCallback(key) {
> +  let initial = initialValue(key);
> +  return Promise.resolve(initial);
> +}

If you make the change suggested above to just `await` the callback, this doesn't even need to be a Promise, it can just be `initialValueCallback = key => \`key:${key}\`;`

::: toolkit/components/extensions/test/xpcshell/test_ext_extensionSettingsStore.js:67
(Diff revision 5)
> +  await promiseStartupManager();
> +
> +  for (let extension of testExtensions) {
> +    await extension.startup();
> +  }
> +  let extensions = testExtensions.map(extension => extension.extension._extension);

This warrants a comment.  The values in `testExtensions` are test framework extension wrappers but you're trying to get down to actual instances of `Extension`

	Bob Silverberg [:bsilverberg] Assignee
	Comment 18 • 8 years ago

My apologies, Andrew. This is not ready for review. I had removed the r? flag, but it got re-added. I think most of your comments will apply to the next version, so it wasn't really a waste of your time to do this preliminary review, but that would explain some of the issues. Thanks for taking the time to look at the patch.

	Bob Silverberg [:bsilverberg] Assignee
	Comment 29 • 8 years ago
mozreview-review-reply

Comment on attachment 8826360 [details]
Bug 1320736 - Part 2: Create ExtensionSettingsStore module,

https://reviewboard.mozilla.org/r/104294/#review108416

1. I asked Kris about the async/await thing in non-test code in IRC and he said "go for it". I'm not sure if we need to seek someone else's okay as well.
2. Yeah, as mentioned in my bug comment this wasn't actually ready for review yet, which explains that issue.

> You don't seem to actually use this anywhere?  If this is meant to let us change the on-disk format, then you should refuse to load from a newer schema version.

This was something I brought over from another consumer of JSONFile.jsm, but I've decided to just remove it as I don't think we'll ever need it. This schema is very simple and single-purpose and I don't see it changing in the future.

> Just inline this in `getStore()` please

I changed things around so we don't even need a postProcessor anymore. It didn't really make sense.

> why take `store` as a parameter instead of just referencing `_store` directly?

I fixed this, but replaced it with something different that you make take issue with. ;)  I'm now calling `getStore` in order to get the store, and I am doing that because there is now logic in `getStore` to deal with the situation in which the `type` has not already been added to the store. Calling `getStore` here means I don't need to check for an existing `type`.

> are you certain that `store.data[type]` will always exist when you get here?

I addressed this issue by ensuring that `store.data[type]` will always exist for any call to store.

> I think you need to test that keyObj is actually an object

Because of other changes I made, `keysObj` will always be an object.

> If you make the change suggested above to just `await` the callback, this doesn't even need to be a Promise, it can just be `initialValueCallback = key => \`key:${key}\`;`

I'm not sure I understand this comment. I created the `initialValue` function in order to use it in multiple places (including in an assert). I did remove the Promise stuff though, if that addresses the issue.

	Andrew Swan [:aswan]
	Comment 30 • 8 years ago
mozreview-review

Comment on attachment 8830913 [details]
Bug 1320736 - Part 1: Set installDate in  installAddonFromLocation,

https://reviewboard.mozilla.org/r/107580/#review111314

	Andrew Swan [:aswan]
	Comment 31 • 8 years ago
mozreview-review

Comment on attachment 8826360 [details]
Bug 1320736 - Part 2: Create ExtensionSettingsStore module,

https://reviewboard.mozilla.org/r/104294/#review111316

a bunch of comments but they're all small...

::: toolkit/components/extensions/ExtensionSettingsStore.jsm:51
(Diff revision 9)
> +XPCOMUtils.defineLazyModuleGetter(this, "OS",
> +                                  "resource://gre/modules/osfile.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "Services",
> +                                  "resource://gre/modules/Services.jsm");

since you use these both right below this, you can just Cu.import() them

::: toolkit/components/extensions/ExtensionSettingsStore.jsm:82
(Diff revision 9)
> +
> +// Return an object with properties for key and value, or null
> +// if no setting has been stored for that key.
> +async function getTopItem(type, key) {
> +  let store = await getStore(type);
> +  store.ensureDataReady();

it looks like every caller to `getStore()` immediately calls this, why not just move it into `getStore()`?

::: toolkit/components/extensions/ExtensionSettingsStore.jsm:104
(Diff revision 9)
> +   * setting.
> +   *
> +   * @param {Extension} extension The extension for which a setting is being added.
> +   * @param {string} type The type of setting to be stored. For example `pref`.
> +   * @param {string} key A string that uniquely identifies the setting, for
> +   * example, a preference name.

can you indent this please, same below

::: toolkit/components/extensions/ExtensionSettingsStore.jsm:122
(Diff revision 9)
> +    // We need to know the install date of the extension for precedence handling.
> +    let addon = await AddonManager.getAddonByID(id);
> +    let installDate = addon.installDate;

this isn't needed if this extension is already in the precedence list -- move it into the block where it is used below

::: toolkit/components/extensions/ExtensionSettingsStore.jsm:131
(Diff revision 9)
> +      let initialValuePromise = initialValueCallback(key);
> +      let initialValue = await initialValuePromise;

merge these two lines

::: toolkit/components/extensions/ExtensionSettingsStore.jsm:166
(Diff revision 9)
> +  /**
> +   * Removes a setting from the store, returning the current top precedent
> +   * setting.
> +   *
> +   * @param {Extension} extension The extension for which a setting is being removed.
> +   * @param {string} type The type of setting to be stored. For example `pref`.

stored -> removed

::: toolkit/components/extensions/ExtensionSettingsStore.jsm:180
(Diff revision 9)
> +    let store = await getStore(type);
> +    store.ensureDataReady();
> +
> +    let keyInfo = store.data[type][key];
> +    if (!keyInfo) {
> +      return null;

This should never happen, so throw an Error here.
Same a few lines below if an entry is not found in the precendenceList

::: toolkit/components/extensions/ExtensionSettingsStore.jsm:191
(Diff revision 9)
> +    if (foundIndex == -1) {
> +      return null;
> +    }
> +
> +    keyInfo.precedenceList.splice(foundIndex, 1);
> +    store.saveSoon();

If the list now has length 0, this key should be removed from the store.

::: toolkit/components/extensions/test/xpcshell/test_ext_extensionSettingsStore.js:41
(Diff revision 9)
> +function initialValueCallback(key) {
> +  return initialValue(key);
> +}

i don't understand the purpose of this function

::: toolkit/components/extensions/test/xpcshell/test_ext_extensionSettingsStore.js:96
(Diff revision 9)
> +  // Add a setting for the oldest extension.
> +  for (let key of KEY_LIST) {
> +    let extensionIndex = 0;
> +    let itemToAdd = ITEMS[key][extensionIndex];
> +    let item = await ExtensionSettingsStore.addSetting(
> +      extensions[extensionIndex], "pref", itemToAdd.key, itemToAdd.value, initialValueCallback);

can you add a check that the initial value function is not called here (and same below)

::: toolkit/components/extensions/test/xpcshell/test_ext_extensionSettingsStore.js:103
(Diff revision 9)
> +    item = await ExtensionSettingsStore.getSetting("pref", key);
> +    deepEqual(
> +      item,
> +      ITEMS[key][1],
> +      "getSetting returns correct item with more than one item in the list.");
> +    let levelOfControl = await ExtensionSettingsStore.getLevelOfControl(extensions[extensionIndex], "pref", key);

also call this before adding the setting, should be the same result

::: toolkit/components/extensions/test/xpcshell/test_ext_extensionSettingsStore.js:122
(Diff revision 9)
> +    item = await ExtensionSettingsStore.getSetting("pref", key);
> +    deepEqual(
> +      item,
> +      ITEMS[key][2],
> +      "getSetting returns correct item with more than one item in the list.");
> +    let levelOfControl = await ExtensionSettingsStore.getLevelOfControl(extensions[extensionIndex], "pref", key);

add a test that calls this before adding the value, it should return controllable

	Andrew Swan [:aswan]
	Comment 32 • 8 years ago
mozreview-review

Comment on attachment 8827448 [details]
Bug 1320736 - Part 3: Create ExtensionPreferencesManager module,

https://reviewboard.mozilla.org/r/105134/#review111332

I didn't get all the way through the test and want to keep reading, but need to step away for a bit, and want to get the feedback so far published...

::: toolkit/components/extensions/ExtensionPreferencesManager.jsm:25
(Diff revision 9)
> +XPCOMUtils.defineLazyModuleGetter(this, "ExtensionSettingsStore",
> +                                  "resource://gre/modules/ExtensionSettingsStore.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "Preferences",
> +                                  "resource://gre/modules/Preferences.jsm");
> +
> +function defualtInitialValueCallback(prefName) {

that's not the spelling i'm accustomed to but you've used it consistently throughout the file...  is that hows its spelled in canada?  :-)

::: toolkit/components/extensions/ExtensionPreferencesManager.jsm:26
(Diff revision 9)
> +                                  "resource://gre/modules/ExtensionSettingsStore.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "Preferences",
> +                                  "resource://gre/modules/Preferences.jsm");
> +
> +function defualtInitialValueCallback(prefName) {
> +  return Promise.resolve(Preferences.get(prefName));

the `Promise.resolve` is unnecessary here

::: toolkit/components/extensions/ExtensionPreferencesManager.jsm:60
(Diff revision 9)
> +    if (!canSetValueIntoPref(value)) {
> +      throw new Error(`Cannot set value with type ${typeof value} into a preference.`);
> +    }

the prefs code will throw if somebody tries to use an illegal value, why have a redundant check here?

::: toolkit/components/extensions/ExtensionPreferencesManager.jsm:64
(Diff revision 9)
> +  async setPref(extension, prefName, value, initialValueCallback = defualtInitialValueCallback) {
> +    if (!canSetValueIntoPref(value)) {
> +      throw new Error(`Cannot set value with type ${typeof value} into a preference.`);
> +    }
> +    let item = await ExtensionSettingsStore.addSetting(
> +      extension, "pref", prefName, value, initialValueCallback);

pull `"pref"` out into a string constant defined at the top the file

::: toolkit/components/extensions/ExtensionPreferencesManager.jsm:67
(Diff revision 9)
> +    }
> +    let item = await ExtensionSettingsStore.addSetting(
> +      extension, "pref", prefName, value, initialValueCallback);
> +    if (item) {
> +      Preferences.set(prefName, item.value);
> +      return Promise.resolve(true);

no need for `Promise.resolve` here or right below

::: toolkit/components/extensions/ExtensionPreferencesManager.jsm:82
(Diff revision 9)
> +   */
> +  async unsetPref(extension, prefName) {
> +    let item = await ExtensionSettingsStore.removeSetting(
> +      extension, "pref", prefName);
> +    if (item) {
> +      if (canSetValueIntoPref(item.value)) {

I don't get this, under what conditions do you get something back for which this returns false?

::: toolkit/components/extensions/test/xpcshell/test_ext_extensionPreferencesManager.js:32
(Diff revision 9)
> +function initialValueCallback(key) {
> +  let initial = initialValue(key);
> +  return Promise.resolve(initial);
> +}

`Promise.resolve` is unnecessary, making this whole function unnecessary

	Bob Silverberg [:bsilverberg] Assignee
	Comment 36 • 8 years ago
mozreview-review-reply

Comment on attachment 8826360 [details]
Bug 1320736 - Part 2: Create ExtensionSettingsStore module,

https://reviewboard.mozilla.org/r/104294/#review111316

> This should never happen, so throw an Error here.
> Same a few lines below if an entry is not found in the precendenceList

Good suggestion. I also added tests for these two error conditions.

> also call this before adding the setting, should be the same result

Good suggestion. It actually shouldn't have the same result, it should return "controllable_by_this_extension" before adding the setting, but maybe that's what you meant.

	Bob Silverberg [:bsilverberg] Assignee
	Comment 40 • 8 years ago
mozreview-review-reply

Comment on attachment 8827448 [details]
Bug 1320736 - Part 3: Create ExtensionPreferencesManager module,

https://reviewboard.mozilla.org/r/105134/#review111332

> that's not the spelling i'm accustomed to but you've used it consistently throughout the file...  is that hows its spelled in canada?  :-)

Yeah, that's a Canadian thing called copy-and-paste-eh

> the prefs code will throw if somebody tries to use an illegal value, why have a redundant check here?

The value passed into this method may not end up getting set as a pref right away. For example if an older extension is setting a pref. It will just get added to the precedence chain, but not actually set as a pref. Later, when things change in the precedence chain (e.g., a newer extension is uninstalled), the value could be set into a pref, and I want to avoid storing a value for later use that is not valid for a pref. Does that make sense?

> I don't get this, under what conditions do you get something back for which this returns false?

This can happen if the value that is stored in `initialValue` in the SettingsStore is not a valid preference value. That value is handed back to this method via `item.value` so we need to make sure that it is neither null nor an invalid type for a preference. We cannot validate it in SettingsStore (not in a generic way, anyway), because SettingStore doesn't care about what type of data we're storing. I could change that, as it would know we are storing a "pref", but I'd rather not put logic into SettingsStore for specific types if I can avoid it.

	Andrew Swan [:aswan]
	Comment 41 • 8 years ago
mozreview-review

Comment on attachment 8826360 [details]
Bug 1320736 - Part 2: Create ExtensionSettingsStore module,

https://reviewboard.mozilla.org/r/104294/#review112206

::: toolkit/components/extensions/ExtensionSettingsStore.jsm:73
(Diff revision 11)
> +  // Ensure a property exists for the given type.
> +  if (!_store.data[type]) {
> +    _store.data[type] = {};
> +  }
> +
> +  _store.ensureDataReady();

shouldn't this come before the checking of `type` above?

	Andrew Swan [:aswan]
	Comment 42 • 8 years ago
mozreview-review-reply

Comment on attachment 8827448 [details]
Bug 1320736 - Part 3: Create ExtensionPreferencesManager module,

https://reviewboard.mozilla.org/r/105134/#review111332

> The value passed into this method may not end up getting set as a pref right away. For example if an older extension is setting a pref. It will just get added to the precedence chain, but not actually set as a pref. Later, when things change in the precedence chain (e.g., a newer extension is uninstalled), the value could be set into a pref, and I want to avoid storing a value for later use that is not valid for a pref. Does that make sense?

It does, but prefs can store non-primitive values...  I think the interface to this module would need to change if/when somebody wants to use it for a complex value though so perhaps we just go with this for now.
If you want to think about it ahead of time though, you would need to add a type argument.  See eg http://searchfox.org/mozilla-central/rev/672c83ed65da286b68be1d02799c35fdd14d0134/modules/libpref/nsIPrefBranch.idl#180

> This can happen if the value that is stored in `initialValue` in the SettingsStore is not a valid preference value. That value is handed back to this method via `item.value` so we need to make sure that it is neither null nor an invalid type for a preference. We cannot validate it in SettingsStore (not in a generic way, anyway), because SettingStore doesn't care about what type of data we're storing. I could change that, as it would know we are storing a "pref", but I'd rather not put logic into SettingsStore for specific types if I can avoid it.

If you really want to validate this, how about wrapping the intial value callback in `setPref()` and throwing when the initial value is first accessed if it is invalid?

	Andrew Swan [:aswan]
	Comment 43 • 8 years ago
mozreview-review

Comment on attachment 8827448 [details]
Bug 1320736 - Part 3: Create ExtensionPreferencesManager module,

https://reviewboard.mozilla.org/r/105134/#review112254

I'm still uneasy about the validation code basically duplicating Preferences code but the alternatives don't seem that much better...

::: toolkit/components/extensions/ExtensionPreferencesManager.jsm:27
(Diff revision 11)
> +XPCOMUtils.defineLazyModuleGetter(this, "Preferences",
> +                                  "resource://gre/modules/Preferences.jsm");
> +
> +const PREF_TYPE = "pref";
> +
> +async function defaultInitialValueCallback(prefName) {

async isn't needed here

::: toolkit/components/extensions/ExtensionPreferencesManager.jsm:50
(Diff revision 11)
> +  /**
> +   * Indicates that a preference should be set.
> +   *
> +   * @param {Extension} extension The extension for which a preference is being added.
> +   * @param {string} prefName The name of the preference to set.
> +   * @param {string} value The value to be stored in the preference.

This isn't limited to strings (though it is limited to string|number|boolean right now)

::: toolkit/components/extensions/ExtensionPreferencesManager.jsm:52
(Diff revision 11)
> +   *
> +   * @param {Extension} extension The extension for which a preference is being added.
> +   * @param {string} prefName The name of the preference to set.
> +   * @param {string} value The value to be stored in the preference.
> +   * @param {function} initialValueCallback An async function to be called to
> +   * determine the initial value for the preference. This must return a promise

please indent

::: toolkit/components/extensions/ExtensionPreferencesManager.jsm:58
(Diff revision 11)
> +   * which resolves to the initial value of the preference, and will be passed
> +   * the prefName argument. It defaults to defaultInitialValueCallback, which
> +   * simply calls Preferences.get() using the prefName.
> +   *
> +   * @returns {Promise} Rejects if the value is not valid for a preference.
> +   * Resolves to true if the preference was changed and to false if the

please indent

::: toolkit/components/extensions/ExtensionPreferencesManager.jsm:75
(Diff revision 11)
> +    }
> +    return false;
> +  },
> +
> +  /**
> +   * Indicates that a preference is being unset.

nit: I think this would be more clearly stated as something like "Indicate that this extension no longer wants to set the given preference"

::: toolkit/components/extensions/ExtensionPreferencesManager.jsm:114
(Diff revision 11)
> +   *
> +   * @param {Extension} extension The extension for which levelOfControl is being
> +   *                              requested.
> +   * @param {string} prefName The name of the preference to check.
> +   *
> +   * @returns {string} The level of control of the extension over the preference.

I don't know if we have an established standard for jsdoc of async functions, this actually returns a Promise that resolves to a string.
That's how you documented setPref(), it should be consistent here.

::: toolkit/components/extensions/test/xpcshell/test_ext_extensionPreferencesManager.js:14
(Diff revision 11)
> +XPCOMUtils.defineLazyModuleGetter(this, "ExtensionSettingsStore",
> +                                  "resource://gre/modules/ExtensionSettingsStore.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "Preferences",
> +                                  "resource://gre/modules/Preferences.jsm");
> +
> +const TEST_PREF = "extensions.webextensions.base-content-security-policy";

how did you choose this?
if you want something that is already set, i think it would be better to make something up and set it before executing any tests (with a cleanup function to remove it when the test is finished)

::: toolkit/components/extensions/test/xpcshell/test_ext_extensionPreferencesManager.js:28
(Diff revision 11)
> +// Allow for unsigned addons.
> +AddonTestUtils.overrideCertDB();
> +
> +createAppInfo("xpcshell@tests.mozilla.org", "XPCShell", "1", "42");
> +
> +async function initialValue(key) {

no async needed

::: toolkit/components/extensions/test/xpcshell/test_ext_extensionPreferencesManager.js:128
(Diff revision 11)
> +  }
> +  let setPrefs = await ExtensionSettingsStore.getAllForExtension(extensions[0], "pref");
> +  deepEqual(setPrefs, PREFS_TO_SET, "Expected prefs were set for extension.");
> +  await ExtensionPreferencesManager.unsetAll(extensions[0]);
> +  for (let pref of PREFS_TO_SET) {
> +    equal(Preferences.get(pref), undefined, "unsetAll unset the pref.");

I think you can use `ok(!Preferences.has(pref), "...")` here

	Bob Silverberg [:bsilverberg] Assignee
	Comment 47 • 8 years ago
mozreview-review-reply

Comment on attachment 8827448 [details]
Bug 1320736 - Part 3: Create ExtensionPreferencesManager module,

https://reviewboard.mozilla.org/r/105134/#review111332

> It does, but prefs can store non-primitive values...  I think the interface to this module would need to change if/when somebody wants to use it for a complex value though so perhaps we just go with this for now.
> If you want to think about it ahead of time though, you would need to add a type argument.  See eg http://searchfox.org/mozilla-central/rev/672c83ed65da286b68be1d02799c35fdd14d0134/modules/libpref/nsIPrefBranch.idl#180

Interesting. I was going based on the `set` method in Preferences.jsm [1] which can only be used to set a String, Number or Boolean value. I think due to the high priority of this task, and the fact that for now we only actually need to use it for booleans, I'll just leave it as is for now.

[1] http://searchfox.org/mozilla-central/source/toolkit/modules/Preferences.jsm#111

> If you really want to validate this, how about wrapping the intial value callback in `setPref()` and throwing when the initial value is first accessed if it is invalid?

I'm not sure I follow. Are you saying that, because the developer is saying that this value is the initial value for the pref, I could just try to set it into the pref using `set` and see if that throws? If so, then yes, I think that makes sense, although it seems a bit weird to be setting a pref to something that it should already be. I'm not sure what you mean by the second part though: "and throwing when the initial value is first accessed if it is invalid". If I do the first, Preferences.jsm should just throw for me and I won't need to throw myself.

Actually, looking at it more closely, in ExtensionPreferenceManager, we don't even know if the `initialValueCallback` is going to be executed at all. It will only be executed by the ExtensionSettingsStore if we haven't already stored a setting for this pref, so calling setPref with its value would actually be incorrect.

Maybe I'm just misunderstanding your suggestion. Could you be more explicit about what you think I can do differently?

	Andrew Swan [:aswan]
	Comment 49 • 8 years ago
mozreview-review-reply

Comment on attachment 8827448 [details]
Bug 1320736 - Part 3: Create ExtensionPreferencesManager module,

https://reviewboard.mozilla.org/r/105134/#review111332

> I'm not sure I follow. Are you saying that, because the developer is saying that this value is the initial value for the pref, I could just try to set it into the pref using `set` and see if that throws? If so, then yes, I think that makes sense, although it seems a bit weird to be setting a pref to something that it should already be. I'm not sure what you mean by the second part though: "and throwing when the initial value is first accessed if it is invalid". If I do the first, Preferences.jsm should just throw for me and I won't need to throw myself.
> 
> Actually, looking at it more closely, in ExtensionPreferenceManager, we don't even know if the `initialValueCallback` is going to be executed at all. It will only be executed by the ExtensionSettingsStore if we haven't already stored a setting for this pref, so calling setPref with its value would actually be incorrect.
> 
> Maybe I'm just misunderstanding your suggestion. Could you be more explicit about what you think I can do differently?

I was trying to suggest that up in setPref(), instead of passing `initialValueCallback` straight through to `ExtensionSettingsStore.add`, you could create a new function that calls `initialValueCallback`, and then throws if `canSetValueIntoPref` is false on the thing that comes back.  That way, the exception gets raised closer to the source of the error.
Thinking about this more though, when do you ever anticipate somebody using the preferences manager and not using the default initial value callback?

	Bob Silverberg [:bsilverberg] Assignee
	Comment 50 • 8 years ago
mozreview-review-reply

Comment on attachment 8827448 [details]
Bug 1320736 - Part 3: Create ExtensionPreferencesManager module,

https://reviewboard.mozilla.org/r/105134/#review111332

> I was trying to suggest that up in setPref(), instead of passing `initialValueCallback` straight through to `ExtensionSettingsStore.add`, you could create a new function that calls `initialValueCallback`, and then throws if `canSetValueIntoPref` is false on the thing that comes back.  That way, the exception gets raised closer to the source of the error.
> Thinking about this more though, when do you ever anticipate somebody using the preferences manager and not using the default initial value callback?

Ok, I see what you mean now. As for your follow-up question, that's a good point. I cannot actually think of a case where one would need to override the default initial value callback in setPref. I guess I provided the ability for maximum flexibility, but maybe it doesn't make sense. Do you think it would be better to just remove `initialValueCallback` as an argument to `setPref`, in which case I could remove the value check as well?

	Andrew Swan [:aswan]
	Comment 51 • 8 years ago
mozreview-review-reply

Comment on attachment 8827448 [details]
Bug 1320736 - Part 3: Create ExtensionPreferencesManager module,

https://reviewboard.mozilla.org/r/105134/#review111332

> Ok, I see what you mean now. As for your follow-up question, that's a good point. I cannot actually think of a case where one would need to override the default initial value callback in setPref. I guess I provided the ability for maximum flexibility, but maybe it doesn't make sense. Do you think it would be better to just remove `initialValueCallback` as an argument to `setPref`, in which case I could remove the value check as well?

If we don't have a use for it then yes, lets remove it.  When we have a need for it, we can have this discussion again :-)

	Bob Silverberg [:bsilverberg] Assignee
	Comment 52 • 8 years ago
mozreview-review-reply

Comment on attachment 8827448 [details]
Bug 1320736 - Part 3: Create ExtensionPreferencesManager module,

https://reviewboard.mozilla.org/r/105134/#review111332

> If we don't have a use for it then yes, lets remove it.  When we have a need for it, we can have this discussion again :-)

Sounds good, thanks Andrew. This would have been ready to land, but now I have to deal with that group thing that Kris and I were discussing on IRC.

	Bob Silverberg [:bsilverberg] Assignee
	Comment 56 • 8 years ago

Comment on attachment 8827448 [details]
Bug 1320736 - Part 3: Create ExtensionPreferencesManager module,

There have been some pretty major changes to the ExtensionPreferencesManager, so this needs another review. Sorry about that. 

There were a few minor changes to Part 2, but I don't think that needs to be re-reviewed.

You may want to take a look at the patch for bug #1312802 along with this as they are, unfortunately, pretty tightly coupled. I tried to avoid that, but couldn't figure out a way to do this. I went through many iterations on this, so hopefully it's pretty decent by now. It's still missing some stuff, but I wanted to get a review on what I've done this far to make sure I am on the right track.

	Andrew Swan [:aswan]
	Comment 57 • 8 years ago
mozreview-review

Comment on attachment 8827448 [details]
Bug 1320736 - Part 3: Create ExtensionPreferencesManager module,

https://reviewboard.mozilla.org/r/105134/#review113908

::: toolkit/components/extensions/ExtensionPreferencesManager.jsm:37
(Diff revision 14)
> +                                  "resource://gre/modules/ExtensionSettingsStore.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "Preferences",
> +                                  "resource://gre/modules/Preferences.jsm");
> +
> +XPCOMUtils.defineLazyGetter(this, "defaultPreferences", function() {
> +  Cu.import("resource://gre/modules/Preferences.jsm");

this shouldn't be needed?

::: toolkit/components/extensions/ExtensionPreferencesManager.jsm:70
(Diff revision 14)
> + *
> + * @param {Object} values
> + *        An object with one property per preference, which holds the initial
> + *        value of that preference.
> + */
> +function restoreInitialValues(values) {

since you just use this function in one place, can you inline it below so the reader doesn't have to jump around to follow the flow

::: toolkit/components/extensions/ExtensionPreferencesManager.jsm:87
(Diff revision 14)
> +   * Adds a setting to the settingsMap. This is how an API tells the
> +   * preferences manager what its setting object is. The preferences
> +   * manager needs to know this when settings need to be removed
> +   * automatically.
> +   *
> +   * @param {string} settingId The unique id of the setting.

nit: i think this parameter would be more clearly described as "name" throughout.

::: toolkit/components/extensions/ExtensionPreferencesManager.jsm:107
(Diff revision 14)
> +  /**
> +   * Gets the current value of the setting, based on the setting's prefs.
> +   *
> +   * @param {string} settingId The unique id of the setting.
> +   *
> +   * @returns {Promise}
> +   *          Resolves to the value of the setting.
> +   */
> +  async getSetting(settingId) {
> +    return await settingsMap.get(settingId).getCallback();
> +  },

This seems pointless, its just a way for a user of this module to provide a function and then ask this module to call that function.  That seems more naturally handled entirely within the calling code, which would keep this module simpler.

::: toolkit/components/extensions/ExtensionPreferencesManager.jsm:120
(Diff revision 14)
> +  async getSetting(settingId) {
> +    return await settingsMap.get(settingId).getCallback();
> +  },
> +
> +  /**
> +   * Indicates that a group of preferences should be set.

nit: I think this would be clearer if described as something like "Indicates that an extension would like to change the value of a previously defined setting".

::: toolkit/components/extensions/ExtensionPreferencesManager.jsm:123
(Diff revision 14)
> +
> +  /**
> +   * Indicates that a group of preferences should be set.
> +   *
> +   * @param {Extension} extension
> +   *        The extension for which a preference is being added.

related to the above, preference->setting and added->set

::: toolkit/components/extensions/ExtensionPreferencesManager.jsm:139
(Diff revision 14)
> +  async setSetting(extension, settingId, value) {
> +    let setting = settingsMap.get(settingId);
> +    let item = await ExtensionSettingsStore.addSetting(
> +      extension, STORE_TYPE, settingId, value, initialValueCallback.bind(setting));
> +    if (item) {
> +      await setting.setCallback(item.value);

The division of responsibilites between this module and the caller here is confusing.  I thought this module was supposed to take care of all direct interaction with Preferences.jsm, which would mean this callback would just return a bunch of pref name/values that this code would actually apply.

	Andrew Swan [:aswan]
	Comment 58 • 8 years ago
mozreview-review-reply

Comment on attachment 8827448 [details]
Bug 1320736 - Part 3: Create ExtensionPreferencesManager module,

https://reviewboard.mozilla.org/r/105134/#review113908

Bah, I hit publish earlier than I meant to.  I think we would benefit from another iteration on what the interface between this module and users of it looks like.

	Bob Silverberg [:bsilverberg] Assignee
	Comment 62 • 8 years ago
mozreview-review-reply

Comment on attachment 8827448 [details]
Bug 1320736 - Part 3: Create ExtensionPreferencesManager module,

https://reviewboard.mozilla.org/r/105134/#review113908

Thanks for the review, Andrew. I addressed all of your issues. Your suggestions with respect to `getCallback` and `setCallback` were very good. I am much happier with the code now. This is ready for another review now, and I have also updated the patch for the privacy API to match the changes made here.

	Andrew Swan [:aswan]
	Comment 63 • 8 years ago
mozreview-review

Comment on attachment 8827448 [details]
Bug 1320736 - Part 3: Create ExtensionPreferencesManager module,

https://reviewboard.mozilla.org/r/105134/#review114322

::: toolkit/components/extensions/ExtensionPreferencesManager.jsm:14
(Diff revisions 14 - 15)
>   * prefNames:   An array of strings, each of which is a preference on
>   *              which the setting depends.

nit: I think this could be simplified further by omitting this field and just remembering the list of preferences that you get back when you call the setCallback.  Then the caller doesn't have to provide an object, just the function that maps a setting value to a collection of preference name/value pairs.

::: toolkit/components/extensions/ExtensionPreferencesManager.jsm:140
(Diff revisions 14 - 15)
> +        for (let pref in prefs) {
> +          Preferences.set(pref, prefs[pref]);
> +        }

I think you could combine this loop with the one from above, that is choose the thing to iterate over based on whether item.initialValue is present or not.
You currently don't have the reset-if-value-is-undefined logic when you're not resetting to the initial value, but I think you could keep it and that would also simplify some of the uses (ie, a setCallback could just return undefined instead of having to look up the default value and return it).  If you're going to do that, make sure to add the undefined handling to setSetting() as well!

	Andrew Swan [:aswan]
	Comment 64 • 8 years ago
mozreview-review

Comment on attachment 8827448 [details]
Bug 1320736 - Part 3: Create ExtensionPreferencesManager module,

https://reviewboard.mozilla.org/r/105134/#review114330

Looking good.  Thinking about it some more, I think that taking prefNames out of the thing that gets passed to `addSetting()` would be feasible, but perhaps too much work to justify doing it.  I do think that unifying the "apply a bunch of preferences" logic would be good but then I think we're all set.

::: toolkit/components/extensions/test/xpcshell/test_ext_extensionPreferencesManager.js:64
(Diff revision 15)
> +const {
> +  createAppInfo,
> +  promiseShutdownManager,
> +  promiseStartupManager,
> +} = AddonTestUtils;
> +
> +AddonTestUtils.init(this);

can you move this above the declaration of the test data structures

::: toolkit/components/extensions/test/xpcshell/test_ext_extensionPreferencesManager.js:73
(Diff revision 15)
> +} = AddonTestUtils;
> +
> +AddonTestUtils.init(this);
> +
> +// Allow for unsigned addons.
> +AddonTestUtils.overrideCertDB();

i don't think you need this since you're using temporary installation

	Bob Silverberg [:bsilverberg] Assignee
	Comment 68 • 8 years ago
mozreview-review-reply

Comment on attachment 8827448 [details]
Bug 1320736 - Part 3: Create ExtensionPreferencesManager module,

https://reviewboard.mozilla.org/r/105134/#review114322

> nit: I think this could be simplified further by omitting this field and just remembering the list of preferences that you get back when you call the setCallback.  Then the caller doesn't have to provide an object, just the function that maps a setting value to a collection of preference name/value pairs.

I agree with your withdrawal of this issue below, so I am dropping this one.

> I think you could combine this loop with the one from above, that is choose the thing to iterate over based on whether item.initialValue is present or not.
> You currently don't have the reset-if-value-is-undefined logic when you're not resetting to the initial value, but I think you could keep it and that would also simplify some of the uses (ie, a setCallback could just return undefined instead of having to look up the default value and return it).  If you're going to do that, make sure to add the undefined handling to setSetting() as well!

This actually allowed me to extract that logic into a separate function to reduce the code in both `setSetting` and `unsetSetting`. Thanks!

	Bob Silverberg [:bsilverberg] Assignee
	Comment 69 • 8 years ago

Try looks good. Requesting check in.

	Pulsebot
	Comment 70 • 8 years ago

Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a3afa2da7c50
Part 1: Set installDate in  installAddonFromLocation, r=aswan
https://hg.mozilla.org/integration/autoland/rev/1c6b622f2eb7
Part 2: Create ExtensionSettingsStore module, r=aswan
https://hg.mozilla.org/integration/autoland/rev/c95e2918d304
Part 3: Create ExtensionPreferencesManager module, r=aswan

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 71 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/a3afa2da7c50
https://hg.mozilla.org/mozilla-central/rev/1c6b622f2eb7
https://hg.mozilla.org/mozilla-central/rev/c95e2918d304