Closed Bug 1312802 Opened 8 years ago Closed 7 years ago

Implement chrome.privacy API

Categories

(WebExtensions :: Compatibility, defect, P2)

defect

Tracking

(firefox54 fixed)

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed
webextensions +

People

(Reporter: groovecoder, Assigned: bsilverberg)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [privacy] triaged)

Attachments

(1 file)

Privacy Badger is adding support for WebRTC protection:

https://github.com/EFForg/privacybadgerchrome/pull/969

Their code uses chrome.privacy.network, which is not implemented in Firefox. So, their WebRTC protection will only work in Chrome unless/until we implement chrome.privacy.network, or find an alternative implementation.
Adding privacy to whiteboard so this will show up(?) at http://arewewebextensionsyet.com/#privacy.
Whiteboard: [privacy]
Also related bug 1309926 comment 1, where uBlock also needs to tweak some prefs.

Not quite sure how well what exists in the Chrome privacy API maps to our settings, so we'll probably need to run through those and come up with our own mappings.
Blocks: 1309926
Priority: -- → P2
Whiteboard: [privacy] → [privacy] triaged
Depends on: 1320736
webextensions: --- → +
Assignee: nobody → bob.silverberg
Component: WebExtensions: Untriaged → WebExtensions: Compatibility
Comment on attachment 8836859 [details]
Bug 1312802 - Implement chrome.privacy API,

Note that I just added some code that automatically removes settings when an extension is shutdown, but I fear it won't ignore updates, as we've discussed in the past, so maybe something different needs to be done for that.
Comment on attachment 8836859 [details]
Bug 1312802 - Implement chrome.privacy API,

I'm going to assume you haven't started looking at this yet. I added the last setting that uBlock Origin needs, `browser.privacy.network.webRTCIPHandlingPolicy` and I've just rolled those changes into the patch that I had pushed before.

The logic for the setting/prefs is not finalized, but that shouldn't prevent you from reviewing the code.
Comment on attachment 8836859 [details]
Bug 1312802 - Implement chrome.privacy API,

https://reviewboard.mozilla.org/r/112160/#review114404

Overall this is looking great, but enough little things that I'd like to take another look before landing.

::: toolkit/components/extensions/ext-privacy.js:5
(Diff revision 6)
> +/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */
> +/* vim: set sts=2 sw=2 et tw=80: */
> +"use strict";
> +
> +var {classes: Cc, interfaces: Ci, utils: Cu} = Components;

const

::: toolkit/components/extensions/ext-privacy.js:12
(Diff revision 6)
> +XPCOMUtils.defineLazyModuleGetter(this, "Preferences",
> +                                  "resource://gre/modules/Preferences.jsm");
> +
> +Cu.import("resource://gre/modules/ExtensionPreferencesManager.jsm");
> +Cu.import("resource://gre/modules/ExtensionUtils.jsm");
> +var {

const

::: toolkit/components/extensions/ext-privacy.js:23
(Diff revision 6)
> +    throw new ExtensionError(
> +      `Firefox does not support the ${scope} settings scope.`);
> +  }
> +}
> +
> +async function getSetting(extension, settingId, details, callback) {

can you use name isntead of settingId like you do in the preferences manager?  (and throughout the rest of the file)

::: toolkit/components/extensions/ext-privacy.js:24
(Diff revision 6)
> +      `Firefox does not support the ${scope} settings scope.`);
> +  }
> +}
> +
> +async function getSetting(extension, settingId, details, callback) {
> +  let returnObj = {

why create the temp variable?  just return this direclty

::: toolkit/components/extensions/ext-privacy.js:56
(Diff revision 6)
> +    let prefs = {};
> +    for (let pref of this.prefNames) {
> +      if (pref === "network.http.speculative-parallel-limit") {
> +        if (value) {
> +          prefs[pref] = ExtensionPreferencesManager.getDefaultValue(pref);
> +        } else {
> +          prefs[pref] = 0;
> +        }
> +      } else if (pref === "network.dns.disablePrefetch") {
> +        prefs[pref] = !value;
> +      } else {
> +        prefs[pref] = value;
> +      }
> +    }
> +    return prefs;

This whole thing would be more compact and readable as somethign like:
```
return {
  "network.http.speculative-parallel-limit": value ? undefined : 0,
  "network.dns.disablePrefetch": !value,
  "network.predictor.enabled": value,
  "network.prefetch-next": value,
};
```

::: toolkit/components/extensions/ext-privacy.js:60
(Diff revision 6)
> +  setCallback(value) {
> +    let prefs = {};
> +    for (let pref of this.prefNames) {
> +      if (pref === "network.http.speculative-parallel-limit") {
> +        if (value) {
> +          prefs[pref] = ExtensionPreferencesManager.getDefaultValue(pref);

I haven't looked at your most recent changes to the preferences manager, but my last comment suggested that you could just let the pref manager handle this by returning undefined here

::: toolkit/components/extensions/ext-privacy.js:120
(Diff revision 6)
> +    return {[this.prefNames[0]]: value};
> +  },
> +});
> +
> +/* eslint-disable mozilla/balanced-listeners */
> +extensions.on("shutdown", async (type, extension) => {

Instead of listening globally for extension shutdowns, please just use `context.callOnClose()` when an extension first sets any privacy setting.

::: toolkit/components/extensions/ext-privacy.js:133
(Diff revision 6)
> +          get: async function(details) {
> +            return await getSetting(
> +              extension, "network.networkPredictionEnabled", details, () => {
> +                return Preferences.get("network.predictor.enabled") &&
> +                  Preferences.get("network.prefetch-next") &&
> +                  Preferences.get("network.http.speculative-parallel-limit") > 0 &&
> +                  !Preferences.get("network.dns.disablePrefetch");
> +              });
> +          },
> +          set: async function(details) {
> +            return await setSetting(
> +              extension, "network.networkPredictionEnabled", details);
> +          },
> +          clear: async function(details) {
> +            return await clearSetting(
> +              extension, "network.networkPredictionEnabled", details);
> +          },

There's a bunch of repeated boilerplate here, how about creating a function that constructs one of these objects (ie, the thing that has get(), set(), and clear()).  While you're doing that, please just inline the code for setSetting() and clearSetting().

::: toolkit/components/extensions/schemas/privacy.json:22
(Diff revision 6)
> +      }
> +    ]
> +  },
> +  {
> +    "namespace": "privacy",
> +    "permissions": ["privacy"]

Seems like this ought to have a permission string for permission prompts.

::: toolkit/components/extensions/schemas/privacy.json:26
(Diff revision 6)
> +    "namespace": "privacy",
> +    "permissions": ["privacy"]
> +  },
> +  {
> +    "namespace": "privacy.network",
> +    "description": "Use the <code>chrome.privacy</code> API to control usage of the features in the browser that can affect a user's privacy.",

chrome -> browser

::: toolkit/components/extensions/schemas/types.json:133
(Diff revision 6)
> +            ]
> +          }
> +        ],
> +        "events": [
> +          {
> +            "name": "onChange",

should this be marked as unsupported?
also, is there a bug to implement it?

::: toolkit/components/extensions/test/xpcshell/test_ext_privacy.js:38
(Diff revision 6)
> +      if (msg === "uninstall") {
> +        await browser.management.uninstallSelf();

this doesn't appear to be used?

::: toolkit/components/extensions/test/xpcshell/test_ext_privacy.js:45
(Diff revision 6)
> +      } else {
> +        let data = args[0];
> +        // The second argument is the end of the api name,
> +        // e.g., "network.networkPredictionEnabled".
> +        let apiObj = args[1].split(".").reduce((o, i) => o[i], browser.privacy);
> +        let npeData;

what does npe mean?

::: toolkit/components/extensions/test/xpcshell/test_ext_privacy.js:70
(Diff revision 6)
> +  // Set prefs to our initial values.
> +  for (let setting in SETTINGS) {
> +    for (let pref in SETTINGS[setting]) {
> +      Preferences.set(pref, SETTINGS[setting][pref]);
> +    }
> +  }
> +
> +  do_register_cleanup(() => {
> +    // Reset the prefs.
> +    for (let setting in SETTINGS) {
> +      for (let pref in SETTINGS[setting]) {
> +        Preferences.reset(pref);
> +      }
> +    }
> +  });

you can use SpecialPowers.pushPrefEnv() for this

::: toolkit/components/extensions/test/xpcshell/test_ext_privacy.js:109
(Diff revision 6)
> +
> +  await promiseStartupManager();
> +
> +  for (let extension of testExtensions) {
> +    await extension.startup();
> +    await extension.awaitMessage("ready");

i don't think this is necessary?

::: toolkit/components/extensions/test/xpcshell/test_ext_privacy.js:225
(Diff revision 6)
> +  }
> +
> +  await promiseShutdownManager();
> +});
> +
> +add_task(async function test_privacy_webRTCIPHandlingPolicy() {

why isn't this just another case in the test above?
Attachment #8836859 - Flags: review?(aswan)
Blocks: 1340156
Comment on attachment 8836859 [details]
Bug 1312802 - Implement chrome.privacy API,

https://reviewboard.mozilla.org/r/112160/#review114404

> This whole thing would be more compact and readable as somethign like:
> ```
> return {
>   "network.http.speculative-parallel-limit": value ? undefined : 0,
>   "network.dns.disablePrefetch": !value,
>   "network.predictor.enabled": value,
>   "network.prefetch-next": value,
> };
> ```

Nice!

> Instead of listening globally for extension shutdowns, please just use `context.callOnClose()` when an extension first sets any privacy setting.

Ok, I have made this change. Will this work in all cases, or will we still have an issue with updates vs. uninstalls/disables? Also, does the callOnClose only get triggered when an extension is uninstalled or disabled, or is it also triggered when the browser is shutdown? If the latter, then that also will be an issue, as we don't want this to happen then.

> There's a bunch of repeated boilerplate here, how about creating a function that constructs one of these objects (ie, the thing that has get(), set(), and clear()).  While you're doing that, please just inline the code for setSetting() and clearSetting().

Great idea! That really cleaned up the code a lot. :)

> Seems like this ought to have a permission string for permission prompts.

Yep. I have proposed a string and asked Scott for feedback.

> should this be marked as unsupported?
> also, is there a bug to implement it?

https://bugzilla.mozilla.org/show_bug.cgi?id=1340156

> what does npe mean?

It was "networkPredictionEnabled" originally, but then I added a second setting to the test, so I've changed it to a more generic name.

> you can use SpecialPowers.pushPrefEnv() for this

Cannot use SpecialPowers in xpcshell tests. :(

> why isn't this just another case in the test above?

There are a few reasons it ended up this way.

This setting does not simply toggle between `true` and `false` - it takes different values which end up with different combinations of preferences. The other two settings are handled pretty much identically in the test, so it made sense to put them in a loop. I'd rather not add special cases into the loop for this setting.

A lot of what's being tested in the first test case is the logic for precedence. I.e., making sure the correct extension has control of the setting in different situations. I actually wrote it first for `network.networkPredictionEnabled` and when I added `websites.hyperlinkAuditingEnabled` later I realized I might as well make it into a loop to get a bit more coverage. I.e., making sure that no weird issues occur with more than one setting. When I added `network.webRTCIPHandlingPolicy` even later I felt like I didn't need to test that logic yet again, as it's well tested by the first test case, so my tests for `network.webRTCIPHandlingPolicy` simply make sure the pref flipping works correctly for the setting. When we add more settings in the future I think we can just add more test cases like this one.

So for now I'm happy to keep them separate, but if you feel strongly about me rewriting the first test case to also deal with this setting I can do so.
Comment on attachment 8836859 [details]
Bug 1312802 - Implement chrome.privacy API,

https://reviewboard.mozilla.org/r/112160/#review114404

> Ok, I have made this change. Will this work in all cases, or will we still have an issue with updates vs. uninstalls/disables? Also, does the callOnClose only get triggered when an extension is uninstalled or disabled, or is it also triggered when the browser is shutdown? If the latter, then that also will be an issue, as we don't want this to happen then.

The callback will be invoked in the same conditions either way.  I think it will get called at browser shutdown but you should be able to look at extension.shutdownReason and skip the cleanup it if is APP_SHUTDOWN.
Comment on attachment 8836859 [details]
Bug 1312802 - Implement chrome.privacy API,

https://reviewboard.mozilla.org/r/112160/#review114702

Nice.  It would be good to have somebody more familiar with the actual preferences that are getting changed review this as well.  I think the way the preferences manager is now structured, it is straightforward to look at just ext-privacy.js to see how an extension-visible setting is mapped to underlying preferences without having to worry about the mechanics underneath.

::: toolkit/components/extensions/ext-privacy.js:28
(Diff revision 7)
> +  }
> +}
> +
> +function getAPI(extension, context, name, callback) {
> +  return {
> +    get: async function(details) {

This can just be `async get(details) {`
Similarly for the other functions below

::: toolkit/components/extensions/ext-privacy.js:39
(Diff revision 7)
> +        value: await callback(),
> +      };
> +    },
> +    set: async function(details) {
> +      checkScope(details.scope);
> +      if (!extensionContexts.has(extension)) {

The external Set seems like overkill here, how about just creating a boolean inside getAPI and consulting it here?
Attachment #8836859 - Flags: review?(aswan) → review+
Comment on attachment 8836859 [details]
Bug 1312802 - Implement chrome.privacy API,

https://reviewboard.mozilla.org/r/112160/#review114404

> The callback will be invoked in the same conditions either way.  I think it will get called at browser shutdown but you should be able to look at extension.shutdownReason and skip the cleanup it if is APP_SHUTDOWN.

I looked at all the reasons at [1] and it looks like we only want to do the cleanup for ADDON_DISABLE and ADDON_UNINSTALL, so I've coded it that way which should also address the update issue (assuming that updates have an  ADDON_UPGRADE reason).

[1] http://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#217
Comment on attachment 8836859 [details]
Bug 1312802 - Implement chrome.privacy API,

https://reviewboard.mozilla.org/r/112160/#review114702

> The external Set seems like overkill here, how about just creating a boolean inside getAPI and consulting it here?

Ok, so getAPI gets called for each extension? I'm still learning how all of this stuff actually fits together. That definitely makes it simpler.
Comment on attachment 8836859 [details]
Bug 1312802 - Implement chrome.privacy API,

Requesting review from Tim Huang for the logic for `networkPredictionEnabled`, which I added based on our email conversation.
Attachment #8836859 - Flags: review?(tihuang)
Comment on attachment 8836859 [details]
Bug 1312802 - Implement chrome.privacy API,

Requesting review from Byron Campen for the logic behind the `webRTCIPHandlingPolicy` setting. I sent you an email with a few questions, but made a first attempt at the logic in this patch. Feel free to contact me to discuss it further.
Attachment #8836859 - Flags: review?(docfaraday)
Comment on attachment 8836859 [details]
Bug 1312802 - Implement chrome.privacy API,

https://reviewboard.mozilla.org/r/112160/#review114404

> I looked at all the reasons at [1] and it looks like we only want to do the cleanup for ADDON_DISABLE and ADDON_UNINSTALL, so I've coded it that way which should also address the update issue (assuming that updates have an  ADDON_UPGRADE reason).
> 
> [1] http://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#217

Hm, shutdown with a reason of ADDON_UPGRADE means the old version is being disabled to make way for the new one, but if there's an error during the installation or startup of the new extension, we're left in a funny state with the extension not installed but its old settings still applied...  I thought you handled this by checking installDate down in the settings manager?
Comment on attachment 8836859 [details]
Bug 1312802 - Implement chrome.privacy API,

https://reviewboard.mozilla.org/r/112160/#review114702

> Ok, so getAPI gets called for each extension? I'm still learning how all of this stuff actually fits together. That definitely makes it simpler.

Yep.  Note that extension and context are parameters to that function :-)
Comment on attachment 8836859 [details]
Bug 1312802 - Implement chrome.privacy API,

https://reviewboard.mozilla.org/r/112160/#review114404

> Hm, shutdown with a reason of ADDON_UPGRADE means the old version is being disabled to make way for the new one, but if there's an error during the installation or startup of the new extension, we're left in a funny state with the extension not installed but its old settings still applied...  I thought you handled this by checking installDate down in the settings manager?

I don't think the check for installDate has anything to do with this. This is about automatically removing settings when an extension is disabled or uninstalled. The installDate thing is used to figure out which extension should have control when the list of extensions requesting changes to a setting is changed, but that seems a seperate issue from making sure that an extension's settings are removed from the list when it is uninstalled or disabled.

If an aborted ADDON_UPGRADE could end up causing us to have settings in place for an extension that is no longer installed, how would you suggest addressing that? Note that we cannot rely on ADDON_INSTALL reviving settings, as often these settings will be set via a user action, not automatically when an extension is installed.
Comment on attachment 8836859 [details]
Bug 1312802 - Implement chrome.privacy API,

https://reviewboard.mozilla.org/r/112160/#review115000

::: toolkit/components/extensions/ext-privacy.js:105
(Diff revision 8)
> +      case "disable_non_proxied_udp":
> +        prefs["media.peerconnection.ice.relay_only"] = true;
> +        break;

This might be relay_only, or it might be proxy_only. You'd have to ask the google guys what precisely they mean by "disable_non_proxied_udp" (I suspect they really mean "disable_non_relayed_udp", but it is hard to say.)

Also, you might want to ask them if this setting also forces use of the default route, or disables host candidates (for TCP). All of these settings are independently useable in our code, but Chrome has tangled them together some.
Attachment #8836859 - Flags: review?(docfaraday)
Comment on attachment 8836859 [details]
Bug 1312802 - Implement chrome.privacy API,

https://reviewboard.mozilla.org/r/112160/#review114404

> I don't think the check for installDate has anything to do with this. This is about automatically removing settings when an extension is disabled or uninstalled. The installDate thing is used to figure out which extension should have control when the list of extensions requesting changes to a setting is changed, but that seems a seperate issue from making sure that an extension's settings are removed from the list when it is uninstalled or disabled.
> 
> If an aborted ADDON_UPGRADE could end up causing us to have settings in place for an extension that is no longer installed, how would you suggest addressing that? Note that we cannot rely on ADDON_INSTALL reviving settings, as often these settings will be set via a user action, not automatically when an extension is installed.

I'm not sure, is the idea really that these settings are persistent?  If an extension changes a setting and then is updated to a new version that never refers to that setting again, is the old setting supposed to stick around?
What about the disable/enable case?  If an extension has changed a setting and then is disabled, and then is re-enabled, should its previous setting be revived?
Comment on attachment 8836859 [details]
Bug 1312802 - Implement chrome.privacy API,

https://reviewboard.mozilla.org/r/112160/#review114404

> I'm not sure, is the idea really that these settings are persistent?  If an extension changes a setting and then is updated to a new version that never refers to that setting again, is the old setting supposed to stick around?
> What about the disable/enable case?  If an extension has changed a setting and then is disabled, and then is re-enabled, should its previous setting be revived?

Yes, I do think these settings are meant to be persistent. That's the whole point of the settings manager and the precedence chain, isn't it? 

I would assume that if an extension changes a setting and then is updated to a new version that never refers to that setting again, the settings manager would still retain the setting. How are we to know that the setting should be removed? Conversely, if that wasn't the case, and we wiped all settings on an update, then the user would end up with a new version of the add-on that behaves differently from the old version, unless they were to re-set the settings via the add-on's UI. The former seems like far less of an evil than the latter.

Regarding disabling/re-enabling, I believe we discussed this before, and agreed that a disable would be treated just like an uninstall, so in that case the settings would be wiped. I realize that this is inconsistent with what's being proposed for upgrade, but again seems like the least evil path. How does this work for other types of settings that add-ons can set (like storage.local and/or localStorage)? Are those settings wiped when an add-on is disabled, or only when it is fully uninstalled?
Comment on attachment 8836859 [details]
Bug 1312802 - Implement chrome.privacy API,

https://reviewboard.mozilla.org/r/112160/#review114404

> Yes, I do think these settings are meant to be persistent. That's the whole point of the settings manager and the precedence chain, isn't it? 
> 
> I would assume that if an extension changes a setting and then is updated to a new version that never refers to that setting again, the settings manager would still retain the setting. How are we to know that the setting should be removed? Conversely, if that wasn't the case, and we wiped all settings on an update, then the user would end up with a new version of the add-on that behaves differently from the old version, unless they were to re-set the settings via the add-on's UI. The former seems like far less of an evil than the latter.
> 
> Regarding disabling/re-enabling, I believe we discussed this before, and agreed that a disable would be treated just like an uninstall, so in that case the settings would be wiped. I realize that this is inconsistent with what's being proposed for upgrade, but again seems like the least evil path. How does this work for other types of settings that add-ons can set (like storage.local and/or localStorage)? Are those settings wiped when an add-on is disabled, or only when it is fully uninstalled?

storage.local and localStorage are wiped on uninstall but not on disable
Comment on attachment 8836859 [details]
Bug 1312802 - Implement chrome.privacy API,

https://reviewboard.mozilla.org/r/112160/#review114404

> storage.local and localStorage are wiped on uninstall but not on disable

Ok, well maybe we should do the same with this then; only wipe on true uninstall. At least that would be somewhat consistent.
Comment on attachment 8836859 [details]
Bug 1312802 - Implement chrome.privacy API,

https://reviewboard.mozilla.org/r/112160/#review114404

> Ok, well maybe we should do the same with this then; only wipe on true uninstall. At least that would be somewhat consistent.

To be clear, the ideal thing would be to remember but stop applying the settings across disable/enable and across upgrades, right?  You would need support all the way down to the settings manager but it seems doable.  Can you file a followup bug for that?
Comment on attachment 8836859 [details]
Bug 1312802 - Implement chrome.privacy API,

LGTM
Attachment #8836859 - Flags: review?(tihuang) → review+
(In reply to Andrew Swan [:aswan] from comment #30)
> Comment on attachment 8836859 [details]
> Bug 1312802 - Implement chrome.privacy API,
> 
> https://reviewboard.mozilla.org/r/112160/#review114404
> 
> > Ok, well maybe we should do the same with this then; only wipe on true uninstall. At least that would be somewhat consistent.
> 
> To be clear, the ideal thing would be to remember but stop applying the
> settings across disable/enable and across upgrades, right?  You would need
> support all the way down to the settings manager but it seems doable.  Can
> you file a followup bug for that?

I'm not sure I agree about upgrade, unless you just mean between the uninstall and re-install that happens as part of an upgrade. Is that what you meant?
Comment on attachment 8836859 [details]
Bug 1312802 - Implement chrome.privacy API,

https://reviewboard.mozilla.org/r/112160/#review115000

> This might be relay_only, or it might be proxy_only. You'd have to ask the google guys what precisely they mean by "disable_non_proxied_udp" (I suspect they really mean "disable_non_relayed_udp", but it is hard to say.)
> 
> Also, you might want to ask them if this setting also forces use of the default route, or disables host candidates (for TCP). All of these settings are independently useable in our code, but Chrome has tangled them together some.

Thanks Byron. I have sent an email out to the authors of the IETF draft (who are also Google employees) in the hope they can either answer the questions or direct me to someone who can. Based on my reading of the IETF draft and the comments from you, I am assuming they do in fact mean `proxy_only` for now, so will implement this that way. If a get a different response from them I can change it later.
Blocks: 1341277
Comment on attachment 8836859 [details]
Bug 1312802 - Implement chrome.privacy API,

https://reviewboard.mozilla.org/r/112160/#review114404

> To be clear, the ideal thing would be to remember but stop applying the settings across disable/enable and across upgrades, right?  You would need support all the way down to the settings manager but it seems doable.  Can you file a followup bug for that?

I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1341277 as a follow-up. Can you take a look and see if it captures what you think the new behaviour should be?
Try looks good, requesting check in.
Keywords: checkin-needed
Autoland couldn't rebase this for landing.
Keywords: checkin-needed
Sorry about that. Should be good now.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/5627b59436db
Implement chrome.privacy API, r=aswan
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5627b59436db
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
docs: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/privacy

I documented the browser settings stuff in the `privacy` API itself, but perhaps it would be better in a separate `types` API? Also, the browser settings stuff is a bit intricate, so I hope I've got the details right.
Flags: needinfo?(bob.silverberg)
Thanks Will. Everything you wrote seems accurate, but yeah, it's kind of a weird, complicated API. I'm not sure if there is a better way to document it.

One thing that isn't discussed, that perhaps should be, is how the interaction between different extensions works. For example, if extension A is installed first, and changes the setting for privacy.network.webRTCIPHandlingPolicy, then extension B, which is installed later, can still request a change to privacy.network.webRTCIPHandlingPolicy, and that request will be stored, but it won't take effect until extension A releases control of that setting either by being disabled, uninstalled, or the extension itself releasing the setting by calling clear().

Does the above example make sense? I do think this is something we need to document, but I'm not sure how. If you'd like to chat more about exactly how it all works I'd be happy to do so, and maybe you can find a way to word it that's more understandable than my ramblings.
Flags: needinfo?(bob.silverberg) → needinfo?(wbamberg)
I stand corrected (by andym). I had the precedence order backwards. A newer extension can override an older extension, not vice versa.
Bob, thanks for helping to explain browser settings precedence logic :).

* I've tried to explain it here: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/privacy/BrowserSetting/set

* Also, as we discussed, I removed stuff about private browsing windows. This makes the explanation simpler (although it does have the undesirable side-effect of giving us have APIs which take mandatory arguments which must be empty objects :/.

* Also, I wondered whether disabling/reenabling an add-on alters its precedence.  It doesn't seem to (so if I install A, then B, then disable/re-enable A, then B still has precedence).
Flags: needinfo?(wbamberg) → needinfo?(bob.silverberg)
A minor documentation point is that we'll try and use the same underlying library for every pref change in Firefox. As such the rules of when an add-on changes what, might expand beyond just privacy for example we do this with the newtab override (I believe).
Yes, I wondered about that. Then would it be better to move BrowserSetting into its own "types" API, like:

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/types/BrowserSetting 

I was on the fence about this.
(In reply to Will Bamberg [:wbamberg] from comment #45)
> Bob, thanks for helping to explain browser settings precedence logic :).
> 
> * I've tried to explain it here:
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/privacy/
> BrowserSetting/set

This looks good Will. Great job! One thing that's missing/incorrect: In scenario #1 (where the pref is locked), we still store and queue the setting. You didn't mention that, whereas you do mention it for scenario #4.
> 
> * Also, as we discussed, I removed stuff about private browsing windows.
> This makes the explanation simpler (although it does have the undesirable
> side-effect of giving us have APIs which take mandatory arguments which must
> be empty objects :/.
> 
> * Also, I wondered whether disabling/reenabling an add-on alters its
> precedence.  It doesn't seem to (so if I install A, then B, then
> disable/re-enable A, then B still has precedence).

I think I answered this in IRC, but yes, you are correct that disable/re-enable has no effect on precedence. It's based on install date.
Flags: needinfo?(bob.silverberg)
(In reply to Will Bamberg [:wbamberg] from comment #47)
> Yes, I wondered about that. Then would it be better to move BrowserSetting
> into its own "types" API, like:
> 
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/types/
> BrowserSetting 
> 
> I was on the fence about this.

Yes, that seems like a better idea. Aren't we already using this for new tab and/or home page overrides?
I've moved this under `types`: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/types/BrowserSetting and made the change you asked for in comment 48. Marking as dev-doc-complete, but as always just let me know if you see anything else here.
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.