https://developer.chrome.com/extensions/permissions API is listed on https://wiki.mozilla.org/WebExtensions#List_of_APIs_we_will_likely_support_in_the_future

Tweaking the bug title to make it more findable in searches. The permissions API is intrinsically tied to the optional_permissions manifest property.

Yesterday you mentioned this having 2 steps, where the first one would not require UX. Is that correct? What is the timeline for UX on this? How do we surface required permissions? (https://developer.chrome.com/extensions/permission_warnings) What is the advantage for the user? What is the incentive for the Dev to use optional permissions? What is an example on Chrome that uses optional permissions?

(In reply to Markus Jaritz [:maritz] (UX) from comment #2) > Yesterday you mentioned this having 2 steps, where the first one would not > require UX. Is that correct? After review, I don't think so. There was talk about using some built in API for this, I don't think they'll work. Based on the fact that we don't have UX for this and its a big bug, I'm going to take this off tracking 48 and we'll see when UX has had opportunity to review it. > What is the timeline for UX on this? I think we should do this sooner rather than later. The ability of WebExtensions to explicitly ask for permissions is a good things for users and something we've never had with add-ons before. > How do we surface required permissions? > (https://developer.chrome.com/extensions/permission_warnings) > What is the advantage for the user? > What is the incentive for the Dev to use optional permissions? > What is an example on Chrome that uses optional permissions?

(In reply to Andy McKay [:andym] from comment #3) > (In reply to Markus Jaritz [:maritz] (UX) from comment #2) > > Yesterday you mentioned this having 2 steps, where the first one would not > > require UX. Is that correct? > > After review, I don't think so. There was talk about using some built in API > for this, I don't think they'll work. > > Based on the fact that we don't have UX for this and its a big bug, I'm > going to take this off tracking 48 and we'll see when UX has had opportunity > to review it. We have some UX for permission prompts that are designed for web pages. I think we'd most likely be able to reuse most of that, but we'd need some changes to make it clear that the message is related to an extension rather than a web page. At some point we'll also need a UI to manage and revoke options permissions. I think that's going to have to depend on bug 1212685.

> What is the incentive for the Dev to use optional permissions? I'll try with a simple scenario: Create a simple feed reader which displays a feed list in a popup/action. The user should be able to customize what feed/url to fetch. As far as I know there is no way to request a custom url given by the user as the permission does not allow it.

Work on this bug is happening on GitHub: https://github.com/mozilla/addons/issues/51

Un-assigning since this has morphed from a UX bug to a tracking bug.

Kris, this needs tests of course and I haven't yet touched content scripts but can you take a look and let me know how you want to connect what's here with bug 1333477? I can put in message manager dispatching/handling here or would you rather handle that in 1333477? Note that we also need to update whiteListedHosts in various contexts, as the bugs are worded that would make more sense here but I dont' really care where we do it.

Comment on attachment 8841600 [details] Bug 1197420 Part 1 Schema groundwork for optional permissions https://reviewboard.mozilla.org/r/115752/#review123618 ::: toolkit/components/extensions/schemas/manifest.json:238 (Diff revision 2) > + { "$ref": "OptionalPermission" }, > + { > + "type": "string", > + "enum": [ > + "alarms", > + "geolocation", Hm. Seems like at least geolocation should work as an optional permission...

Comment on attachment 8841601 [details] Bug 1197420 Part 3 Initial browser.permissions api support https://reviewboard.mozilla.org/r/115754/#review123620 Looks good, but I think we need a better solution for the user input handling. ::: browser/modules/ExtensionsUI.jsm:214 (Diff revision 2) > + let strings = this._buildStrings({ > + type: "optional", > + addon: {name}, > + permissions, > + }); > + this.showPermissionsPrompt(browser, strings, icon).then(resolve); Nit: `resolve(this.showPermissionsPrompt(...))` That way we pass along any rejections, too. ::: toolkit/components/extensions/ExtensionPermissions.jsm:24 (Diff revision 2) > +async function lazyInit() { > + if (prefs) { > + return; > + } > + > + let profileDir = Services.dirsvc.get("ProfD", Ci.nsIFile).path; `OS.Constants.Path.profileDir` ::: toolkit/components/extensions/ExtensionPermissions.jsm:28 (Diff revision 2) > + > + let profileDir = Services.dirsvc.get("ProfD", Ci.nsIFile).path; > + prefs = new JSONFile({path: OS.Path.join(profileDir, FILE_NAME)}); > + > + await prefs.load(); > + prefs.ensureDataReady(); Not necessary. Awaiting on `load()` guarantees this. ::: toolkit/components/extensions/ExtensionPermissions.jsm:38 (Diff revision 2) > +} > + > +this.ExtensionPermissions = { > + async get(extension) { > + await lazyInit(); > + return prefs.data[extension.id] || emptyPermissions(); We should probably clone or freeze this, so we don't wind up with changes that don't get saved right away. ::: toolkit/components/extensions/ExtensionPermissions.jsm:53 (Diff revision 2) > + if (!prefs.data[extension.id].permissions.includes(perm)) { > + added.permissions.push(perm); > + prefs.data[extension.id].permissions.push(perm); > + } Please do something like `let perms = prefs.data[extension.id].permissions` outside of the loop. Same for origins and `.remove()` ::: toolkit/components/extensions/ExtensionPermissions.jsm:84 (Diff revision 2) > + > + let removed = emptyPermissions(); > + > + for (let perm of permissions.permissions) { > + let i = prefs.data[extension.id].permissions.indexOf(perm); > + if (i != -1) { `if (i > 0)` please... Checking explicitly against -1 always makes me nervous. ::: toolkit/components/extensions/ext-c-permissions.js:10 (Diff revision 2) > + let winUtils = context.contentWindow.getInterface(Ci.nsIDOMWindowUtils); > + if (!winUtils.isHandlingUserInput) { > + throw new ExtensionError("May only request permissions from a user input handler"); > + } I'd rather we send this information to the parent at the schema level. One of mixedpuppy's patches also requires this information in the parent in several places, so it would simplify a lot of things. ::: toolkit/components/extensions/ext-permissions.js:22 (Diff revision 2) > +const { > + classifyPermission, > + ExtensionError, > +} = ExtensionUtils; > + > +XPCOMUtils.defineLazyPreferenceGetter(this, "PROMPTS", "extensions.webextOptionalPermissionPrompts"); Please no ALL_CAPS for non-constant values. ::: toolkit/components/extensions/ext-permissions.js:39 (Diff revision 2) > + let type = classifyPermission(perm); > + if (type.origin) { > + origins.push(perm); > + } > + } > + optionalOrigins = new MatchPattern(origins); Can we just make this a lazy getter on the Extension instead? ::: toolkit/components/extensions/ext-permissions.js:42 (Diff revision 2) > + let {permissions, origins} = perms; > + permissions = permissions || []; > + origins = origins || []; Please just set `"default": []` for these in the schema. Or `let {permissions = [], origins = []}` if that's not an option for some reason. ::: toolkit/components/extensions/ext-permissions.js:81 (Diff revision 2) > + await ExtensionPermissions.add(context.extension, perms); > + return true; > + }, > + > + async getAll() { > + let perms = context.extension.userPermissions(); Please just make `userPermissions` a getter rather than a method. ::: toolkit/components/extensions/ext-permissions.js:84 (Diff revision 2) > + > + async getAll() { > + let perms = context.extension.userPermissions(); > + return { > + permissions: perms.permissions, > + origins: perms.hosts, Why hosts in one place and origins in another? ::: toolkit/components/extensions/ext-permissions.js:89 (Diff revision 2) > + for (let perm of permissions.permissions) { > + if (!context.extension.hasPermission(perm)) { > + return false; > + } > + } > + > + for (let origin of permissions.origins) { What happens if either of these is null? ::: toolkit/components/extensions/schemas/permissions.json:53 (Diff revision 2) > + "description": "Get a list of all the extension's permissions.", > + "parameters": [ > + { > + "name": "callback", > + "type": "function", > + "optional": true, No `"optional": true`, please.

Comment on attachment 8841601 [details] Bug 1197420 Part 3 Initial browser.permissions api support https://reviewboard.mozilla.org/r/115754/#review123624 ::: toolkit/components/extensions/ExtensionPermissions.jsm:20 (Diff revision 2) > + if (prefs) { > + return; > + } Oh, also, we need to store a promise the first time this is called, and return that if we're not fully initialized yet. Otherwise, when there are concurrent callers, some will wind up with non-fully-initialized pref stores.

Comment on attachment 8848272 [details] Bug 1197420 Part 4 Apply dynamic permission changes https://reviewboard.mozilla.org/r/121204/#review123622 ::: toolkit/components/extensions/Extension.jsm:690 (Diff revision 1) > + this.on("remove-permissions", (ignoreEvent, permissions) => { > + for (let perm of permissions.permissions) { > + this.permissions.delete(perm); > + } > + > + if (permissions.origins.length > 0) { No need for the `if`. ::: toolkit/components/extensions/Extension.jsm:927 (Diff revision 1) > return; > } > > GlobalManager.init(this); > > + return ExtensionPermissions.get(this); We should do this earlier, as part of a `Promies.all`, or at least start the load earlier, and then wait on the promise here. ::: toolkit/components/extensions/ExtensionChild.jsm:786 (Diff revision 1) > hasPermission(permission) { > return this.context.extension.hasPermission(permission); > } > + > + isPermissionRevokable(permission) { > + return ChildAPIManager.OPTIONAL_PERMISSIONS.includes(permission); I think we should check against the manifest's set of optional permissions here, instead. If it's not an optional permission for this particular extension, it'll never be lazily instantiated or revoked. ::: toolkit/components/extensions/ExtensionChild.jsm:790 (Diff revision 1) > + // XXX when does this get cleaned up > + this.context.extension.permissionChangedCallbacks.add(callback); I think we probably want a separate set of these per context, and then the context can do `extension.on("add-permissions", ...)` and such, and remove the listeners on unload. Otherwise we need to store the set of listeners specific to this context, and remove them from the extension instance on unload. ::: toolkit/components/extensions/ExtensionChild.jsm:796 (Diff revision 1) > + let type = Schemas.rootNamespace > + .get("manifest") > + .get("OptionalPermission"); > + let permissions = []; > + for (let choice of type.choices) { > + if (choice.enumeration) { > + permissions = permissions.concat(...choice.enumeration); > + } > + } > + return permissions; No need for this if we check against the extension's own set of optional permissions. They're already validated against this. ::: toolkit/components/extensions/ExtensionContent.jsm:831 (Diff revision 1) > + if (permissions.permissions.length > 0) { > + for (let perm of permissions.permissions) { > + this.permissions.add(perm); > + } > + for (let callback of this.permissionChangedCallbacks) { > + callback(); Please catch errors here. Not catching errors from callbacks always makes me nervous... Same below.

Comment on attachment 8848273 [details] Bug 1197420 Part 5 Tests for optional permissions https://reviewboard.mozilla.org/r/121206/#review123626 ::: toolkit/components/extensions/test/mochitest/chrome.ini:21 (Diff revision 1) > +[test_chrome_ext_downloads_saveAs.html] > +[test_chrome_ext_eventpage_warning.html] > [test_chrome_ext_hybrid_addons.html] > +[test_chrome_ext_idle.html] > +[test_chrome_ext_identity.html] > +skip-if = os == 'android' # unsupported. > +[test_chrome_ext_permissions.html] > +[test_chrome_ext_storage_cleanup.html] > +[test_chrome_ext_trackingprotection.html] > [test_chrome_ext_trustworthy_origin.html] > [test_chrome_ext_webnavigation_resolved_urls.html] > +[test_chrome_ext_webrequest_background_events.html] > +[test_chrome_ext_webrequest_host_permissions.html] Thank you. ::: toolkit/components/extensions/test/mochitest/test_chrome_ext_permissions.html:15 (Diff revision 1) > +<body> > + > +<script type="text/javascript"> > +"use strict"; > + > +add_task(async function test_permissions() { +1 ::: toolkit/components/extensions/test/mochitest/test_chrome_ext_permissions.html:18 (Diff revision 1) > + permissions: ["cookies"], > + origins: ["*://example.com/"], Please also add tests for only "permissions", only "origins", and probably also with neither, for all of the relevant methods. ::: toolkit/components/extensions/test/mochitest/test_chrome_ext_permissions.html:35 (Diff revision 1) > + if (msg == "set-cookie") { > + try { > + browser.cookies.set({ > + url: "http://example.com/", > + name: "COOKIE", > + value: "NOM NOM", Hm... ::: toolkit/components/extensions/test/xpcshell/test_ext_permissions.js:160 (Diff revision 1) > + // Restart, verify permissions are still present > + await AddonTestUtils.promiseRestartManager(); > + await extension.awaitStartup(); Would be good to test initial startup (with the settings store uninitialized) with multiple current extensions.

Comment on attachment 8841601 [details] Bug 1197420 Part 3 Initial browser.permissions api support https://reviewboard.mozilla.org/r/115754/#review123620 > Why hosts in one place and origins in another? `hosts` is never externally visible and that code was written before any of the optional permissions code. In the optional permissions API, `origins` is part of the extension-facing api and so it needs to be this way to be compatible with Chrome (and of course it it more sensible this way). I was wary of changing to much at once but I think I should switch everything over to origins, will do that in a revision.

Comment on attachment 8841600 [details] Bug 1197420 Part 1 Schema groundwork for optional permissions https://reviewboard.mozilla.org/r/115752/#review123618 > Hm. Seems like at least geolocation should work as an optional permission... I just copied the existing list from Chrome: https://developer.chrome.com/extensions/permissions

Comment on attachment 8841601 [details] Bug 1197420 Part 3 Initial browser.permissions api support https://reviewboard.mozilla.org/r/115754/#review123620 > We should probably clone or freeze this, so we don't wind up with changes that don't get saved right away. I'm not sure I follow, are you concerned about the caller writing to the returned object? This is only called from one place (extension startup) where I don't think there's much danger of somebody adding code that changes the object. I can add a guard if you feel strongly though... > I'd rather we send this information to the parent at the schema level. One of mixedpuppy's patches also requires this information in the parent in several places, so it would simplify a lot of things. Do you mind landing this as-is and doing that work in a follow-up? I'm happy to do it and will do it during the 55 cycle. > Please just set `"default": []` for these in the schema. Or `let {permissions = [], origins = []}` if that's not an option for some reason. Whoops these do have `default: []` in the schema, this was just old code > What happens if either of these is null? They have a default value of `[]` so they can't be null

Comment on attachment 8841601 [details] Bug 1197420 Part 3 Initial browser.permissions api support https://reviewboard.mozilla.org/r/115754/#review123624 > Oh, also, we need to store a promise the first time this is called, and return that if we're not fully initialized yet. Otherwise, when there are concurrent callers, some will wind up with non-fully-initialized pref stores. whoops thanks for catching that. it seems very likely to happen at startup to anybody with more than one webextension installed.

Comment on attachment 8848272 [details] Bug 1197420 Part 4 Apply dynamic permission changes https://reviewboard.mozilla.org/r/121204/#review123622 > I think we probably want a separate set of these per context, and then the context can do `extension.on("add-permissions", ...)` and such, and remove the listeners on unload. Otherwise we need to store the set of listeners specific to this context, and remove them from the extension instance on unload. Okay, but the callbacks are going to call `hasPermission()` which consults the Extension. So we need to make sure that the extension is updated before we call any of the callbacks. We can do that by relying on EventEmitter handlers being called in the order they were originally added, but that makes me nervous. Maybe it shouldn't though...

Comment on attachment 8849285 [details] Bug 1197420 Part 2 Extension cleanups for optional permissions https://reviewboard.mozilla.org/r/122098/#review125638

Comment on attachment 8841601 [details] Bug 1197420 Part 3 Initial browser.permissions api support https://reviewboard.mozilla.org/r/115754/#review125640 ::: toolkit/components/extensions/ExtensionPermissions.jsm:24 (Diff revision 3) > + return; > + } > + > + prefs = new JSONFile({path: OS.Path.join(OS.Constants.Path.profileDir, FILE_NAME)}); > + > + await prefs.load(); I think we need to store this load promise, and await on it the next time someone calls `lazyInit()`. Otherwise they may wind up with a not-fully-initialized copy.

Comment on attachment 8841601 [details] Bug 1197420 Part 3 Initial browser.permissions api support https://reviewboard.mozilla.org/r/115754/#review125646

Comment on attachment 8848272 [details] Bug 1197420 Part 4 Apply dynamic permission changes https://reviewboard.mozilla.org/r/121204/#review125648 ::: toolkit/components/extensions/Extension.jsm:912 (Diff revision 3) > - startup() { > + async startup() { > let started = false; > - return this.loadManifest().then(() => { > + > + try { > + let perms; > + [, perms] = await Promise.all([this.loadManifest(), ExtensionPermissions.get(this)]); Nit: `let [, perms] = ...` ::: toolkit/components/extensions/Extension.jsm:1039 (Diff revision 3) > > - return this.permissions.has(perm); > + if (this.permissions.has(perm)) { > + return true; > + } > + > + if (includeOptional && (this.manifest.optional_permissions || []).includes(perm)) { Please just use `"default": []` for `optional_permissions` in the manifest schema, and drop the `|| []`

Comment on attachment 8841601 [details] Bug 1197420 Part 3 Initial browser.permissions api support https://reviewboard.mozilla.org/r/115754/#review123620 > Do you mind landing this as-is and doing that work in a follow-up? I'm happy to do it and will do it during the 55 cycle. Filed bug 1350151 for this

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s 88db4c72a9d8 -d 8528543cc258: rebasing 383932:88db4c72a9d8 "Bug 1197420 Part 1 Schema groundwork for optional permissions r=kmag" rebasing 383933:6bf3abe0db8b "Bug 1197420 Part 2 Extension cleanups for optional permissions r=kmag" merging browser/base/content/test/webextensions/browser_extension_sideloading.js merging toolkit/components/extensions/Extension.jsm merging toolkit/mozapps/extensions/content/extensions.js merging toolkit/mozapps/extensions/internal/XPIProvider.jsm rebasing 383934:46451e501104 "Bug 1197420 Part 3 Initial browser.permissions api support r=kmag" merging browser/locales/en-US/chrome/browser/browser.properties merging toolkit/components/extensions/Extension.jsm merging toolkit/components/extensions/ExtensionUtils.jsm rebasing 383935:5fe8342b1042 "Bug 1197420 Part 4 Apply dynamic permission changes r=kmag" merging toolkit/components/extensions/Extension.jsm merging toolkit/components/extensions/ExtensionChild.jsm merging toolkit/components/extensions/ExtensionCommon.jsm merging toolkit/components/extensions/ExtensionContent.jsm rebasing 383936:4fb68e7d2f3d "Bug 1197420 Part 5 Tests for optional permissions r=kmag" (tip) merging toolkit/components/extensions/test/mochitest/chrome.ini merging toolkit/components/extensions/test/xpcshell/xpcshell.ini warning: conflicts while merging toolkit/components/extensions/test/mochitest/chrome.ini! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)

Pushed by aswan@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6c4c29c0b05a Part 1 Schema groundwork for optional permissions r=kmag https://hg.mozilla.org/integration/autoland/rev/3de2de388ac9 Part 2 Extension cleanups for optional permissions r=kmag https://hg.mozilla.org/integration/autoland/rev/7df6cc66a2eb Part 3 Initial browser.permissions api support r=kmag https://hg.mozilla.org/integration/autoland/rev/cb352ddee812 Part 4 Apply dynamic permission changes r=kmag https://hg.mozilla.org/integration/autoland/rev/5750ae148c78 Part 5 Tests for optional permissions r=kmag

Backed out for failing test_ext_all_apis.html: https://hg.mozilla.org/integration/autoland/rev/32b07901a07087717a53ab3e7fd15e8baca7925e https://hg.mozilla.org/integration/autoland/rev/d16b5157f406d77b592857b5c1d70ab74a3cd535 https://hg.mozilla.org/integration/autoland/rev/7f34c9359eb8beb3c945031b0eefba82c6d13954 https://hg.mozilla.org/integration/autoland/rev/1e0885d9302764b0e8952ddf723d891f35e57b64 https://hg.mozilla.org/integration/autoland/rev/83cea5c428a741b01564bad066c4721139983b9d Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=5750ae148c78047de794a6cb242b1c9a6069c2f6&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=86294637&repo=autoland 12:11:36 INFO - TEST-PASS | browser/components/extensions/test/mochitest/test_ext_all_apis.html | content script APIs 12:11:36 INFO - SpawnTask.js | Leaving test test_enumerate_content_script_apis 12:11:36 INFO - SpawnTask.js | Entering test test_enumerate_background_script_apis 12:11:36 INFO - Extension loaded 12:11:36 INFO - Buffered messages finished 12:11:36 INFO - TEST-UNEXPECTED-FAIL | browser/components/extensions/test/mochitest/test_ext_all_apis.html | background script APIs - Structures begin differing at: 12:11:36 INFO - got[20] = "browser.permissions.contains" 12:11:36 INFO - expected[20] = "browser.runtime.OnInstalledReason" 12:11:36 INFO - 12:11:36 INFO - SimpleTest.isDeeply@SimpleTest/SimpleTest.js:1595:9 12:11:36 INFO - test_enumerate_background_script_apis@browser/components/extensions/test/mochitest/test_ext_all_apis.js:160:3 12:11:36 INFO - onFulfilled@SimpleTest/SpawnTask.js:69:15 12:11:36 INFO - promise callback*next@SimpleTest/SpawnTask.js:104:45 12:11:36 INFO - onFulfilled@SimpleTest/SpawnTask.js:73:7 12:11:36 INFO - promise callback*next@SimpleTest/SpawnTask.js:104:45 12:11:36 INFO - onFulfilled@SimpleTest/SpawnTask.js:73:7 12:11:36 INFO - co/<@SimpleTest/SpawnTask.js:58:5 12:11:36 INFO - co@SimpleTest/SpawnTask.js:54:10 12:11:36 INFO - toPromise@SimpleTest/SpawnTask.js:122:60 12:11:36 INFO - next@SimpleTest/SpawnTask.js:103:19 12:11:36 INFO - onFulfilled@SimpleTest/SpawnTask.js:73:7 12:11:36 INFO - promise callback*next@SimpleTest/SpawnTask.js:104:45 12:11:36 INFO - onFulfilled@SimpleTest/SpawnTask.js:73:7 12:11:36 INFO - co/<@SimpleTest/SpawnTask.js:58:5 12:11:36 INFO - co@SimpleTest/SpawnTask.js:54:10 12:11:36 INFO - add_task/<@SimpleTest/SpawnTask.js:270:9 12:11:36 INFO - setTimeout handler*SimpleTest_setTimeoutShim@SimpleTest/SimpleTest.js:672:12 12:11:36 INFO - add_task@SimpleTest/SpawnTask.js:269:7 12:11:36 INFO - @browser/components/extensions/test/mochitest/test_ext_all_apis.js:127:1

Also failing xpcshell test test_ext_permissions.js: https://treeherder.mozilla.org/logviewer.html#?job_id=86296671&repo=autoland

hg error in cmd: hg identify upstream -r tip:

Pushed by aswan@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3e690bbe8b5a Part 1 Schema groundwork for optional permissions r=kmag https://hg.mozilla.org/integration/autoland/rev/440bab141509 Part 2 Extension cleanups for optional permissions r=kmag https://hg.mozilla.org/integration/autoland/rev/46e135035f10 Part 3 Initial browser.permissions api support r=kmag https://hg.mozilla.org/integration/autoland/rev/925e3a9499ee Part 4 Apply dynamic permission changes r=kmag https://hg.mozilla.org/integration/autoland/rev/8a0125e00903 Part 5 Tests for optional permissions r=kmag

Backed out in https://hg.mozilla.org/integration/autoland/rev/0fa8a7ec4b42 for timing out on Android, https://treeherder.mozilla.org/logviewer.html#?job_id=86373358&repo=autoland and https://treeherder.mozilla.org/logviewer.html#?job_id=86373354&repo=autoland

Pushed by aswan@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b6af7d779efc Part 1 Schema groundwork for optional permissions r=kmag https://hg.mozilla.org/integration/autoland/rev/680dd7916a23 Part 2 Extension cleanups for optional permissions r=kmag https://hg.mozilla.org/integration/autoland/rev/d1628b66e5f8 Part 3 Initial browser.permissions api support r=kmag https://hg.mozilla.org/integration/autoland/rev/f4fbd8e60288 Part 4 Apply dynamic permission changes r=kmag https://hg.mozilla.org/integration/autoland/rev/b56c89bfeb0e Part 5 Tests for optional permissions r=kmag

Backout by archaeopteryx@coole-files.de: https://hg.mozilla.org/integration/autoland/rev/2b54d882c024 Backed out changeset b56c89bfeb0e https://hg.mozilla.org/integration/autoland/rev/94e7f0558069 Backed out changeset f4fbd8e60288 https://hg.mozilla.org/integration/autoland/rev/0b0d2e8ada83 Backed out changeset d1628b66e5f8 https://hg.mozilla.org/integration/autoland/rev/a1667683da70 Backed out changeset 680dd7916a23 https://hg.mozilla.org/integration/autoland/rev/66fe37afe8f1 Backed out changeset b6af7d779efc on request of developer. r=backout

New try run, good on Android: https://treeherder.mozilla.org/#/jobs?repo=try&revision=40f4bed27c01baa24815a37ef1328ff4e135c528

Pushed by aswan@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0a1daa74303c Part 1 Schema groundwork for optional permissions r=kmag https://hg.mozilla.org/integration/autoland/rev/52c2b953c811 Part 2 Extension cleanups for optional permissions r=kmag https://hg.mozilla.org/integration/autoland/rev/a9a6dad3b0b7 Part 3 Initial browser.permissions api support r=kmag https://hg.mozilla.org/integration/autoland/rev/2ba59dc585a3 Part 4 Apply dynamic permission changes r=kmag https://hg.mozilla.org/integration/autoland/rev/4e319a074b49 Part 5 Tests for optional permissions r=kmag

https://hg.mozilla.org/mozilla-central/rev/0a1daa74303c https://hg.mozilla.org/mozilla-central/rev/52c2b953c811 https://hg.mozilla.org/mozilla-central/rev/a9a6dad3b0b7 https://hg.mozilla.org/mozilla-central/rev/2ba59dc585a3 https://hg.mozilla.org/mozilla-central/rev/4e319a074b49

Ive added: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/permissions https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/optional_permissions (I forgot to add compat data for permissions.Permissions, but that's on its way.) I filed bugs for a few issues I've seen: https://bugzilla.mozilla.org/show_bug.cgi?id=1369577 https://bugzilla.mozilla.org/show_bug.cgi?id=1369776 https://bugzilla.mozilla.org/show_bug.cgi?id=1369581 For those issues, I've added compat notes where it seems likely that the current behavior is what we want (https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/permissions/request#Browser_compatibility). Where the behavior seems like a bug that we want to fix (e.g. bug 1369577, and not being able to grant only activeTab) I've not documented the current behavior, in the expectation that the current behavior will change soonish. Let me know if this works for you!

Looks good, thank you. Just a couple of nits: - I don't think we need a browser compatibility note about silently granting some permissions, I believe Chrome does the same - In the main page for the permissions api I think there are a few other reasons one might use optional permissions. One is if an extension needs host permissions but what hosts it will access is not known until runtime (ie in response to a user action or configuration), so instead of asking for <all_urls> the extension can ask at runtime for a specific host or domain.

Thanks for checking Andy. I've made those changes, and also included the point Christiane made in bug 1369776, comment 5.