Closed
Bug 1322856
Opened 8 years ago
Closed 7 years ago
Expose ContextualIdentities (aka containers) to WebExtensions
Categories
(WebExtensions :: General, defect)
WebExtensions
General
Tracking
(firefox53 fixed)
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: baku, Assigned: baku)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
(Keywords: dev-doc-complete, Whiteboard: [userContextId])
Attachments
(2 files, 1 obsolete file)
22.77 KB,
patch
|
Details | Diff | Splinter Review | |
4.83 KB,
patch
|
kmag
:
review+
|
Details | Diff | Splinter Review |
I didn't find a way to put this interface behind pref but probably, because a special permission is required, this should be enough.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → amarchesini
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8817827 -
Flags: review?(kmaglione+bmo)
Assignee | ||
Updated•8 years ago
|
Blocks: ContextualIdentity
Updated•8 years ago
|
Whiteboard: [userContextId]
Comment 2•8 years ago
|
||
Comment on attachment 8817827 [details] [diff] [review] container.patch Review of attachment 8817827 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, aside from some minor issues. Thanks! ::: toolkit/components/extensions/ext-contextualIdentities.js @@ +31,5 @@ > + > + getAll(details) { > + let identities = []; > + ContextualIdentityService.getIdentities().forEach(identity => { > + if ("name" in details && details.name && No need for the `"name" in details` check. We only emit warnings for undefined properties if they're accessed in a non-boolean context. And optional properties are always normalized to exist, in any case. @@ +60,5 @@ > + if (!identity) { > + return Promise.resolve(null); > + } > + > + if ("name" in details) { The binding code automatically defines omitted optional properties as `null`, so please check `details.name !== null` and so forth. ::: toolkit/components/extensions/ext-cookies.js @@ +79,5 @@ > result.expirationDate = cookie.expiry; > } > > if (cookie.originAttributes.userContextId) { > + result.storeId = global.getCookieStoreIdForContainer(cookie.originAttributes.userContextId); Nit: No need for the `global.` Just add the function to the list of globals in .eslintrc ::: toolkit/components/extensions/schemas/contextual_identities.json @@ +10,5 @@ > + "$extend": "Permission", > + "choices": [{ > + "type": "string", > + "enum": [ > + "contextualIdentities" We should probably filter out this permission, and warn, if contextual identities aren't enabled, in `ExtensionData.readManifest` in Extensions.jsm. @@ +46,5 @@ > + "name": "cookieStoreId", > + "description": "The ID of the contextual identity cookie store. " > + }, > + { > + "type": "function", Callback APIs are only used for compatibility with Chrome. Since this doesn't exist in Chrome, please just use `"async": true`, and omit the callback argument. Same for the other methods. @@ +65,5 @@ > + "parameters": [ > + { > + "type": "object", > + "name": "details", > + "description": "Information to filter the contextual identities being retrieved.", For consistency with other APIs, if this function is going to accept a filter, it should probably be called "query", rather than "getAll". ::: toolkit/components/extensions/test/mochitest/chrome.ini @@ +27,5 @@ > [test_ext_cookies_expiry.html] > [test_ext_cookies_permissions_bad.html] > [test_ext_cookies_permissions_good.html] > [test_ext_cookies_containers.html] > +[test_ext_contextual_identities.html] Is there any reason not to make this a plain mochitest? We currently don't run chrome mochitests out-of-process, so we lose some test coverage. ::: toolkit/components/extensions/test/mochitest/test_ext_contextual_identities.html @@ +63,5 @@ > + ci = await browser.contextualIdentities.update(ci.cookieStoreId, {name: "barfoo", color: "blue", icon: "icon icon"}); > + browser.test.assertTrue(!!ci, "We have an identity"); > + browser.test.assertEq("barfoo", ci.name, "identity.name is correct"); > + browser.test.assertEq("blue", ci.color, "identity.color is correct"); > + browser.test.assertEq("icon icon", ci.icon, "identity.icon is correct"); We should test that update() with only a subset of properties doesn't change the ones that were omitted.
Attachment #8817827 -
Flags: review?(kmaglione+bmo) → review+
Assignee | ||
Comment 4•8 years ago
|
||
Attachment #8817827 -
Attachment is obsolete: true
Assignee | ||
Comment 5•8 years ago
|
||
Attachment #8819216 -
Flags: review?(kmaglione+bmo)
Updated•8 years ago
|
Attachment #8819216 -
Flags: review?(kmaglione+bmo) → review+
Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/5a145b21bd64 Expose ContextualIdentities (aka containers) to WebExtensions - part 1, r=kmaglione https://hg.mozilla.org/integration/mozilla-inbound/rev/5db9fcefa874 Expose ContextualIdentities (aka containers) to WebExtensions - part 2, r=kmaglione
Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/48994775ce8f Eslint failures in Extension.jsm fixed - part 3, r=me
Comment 8•8 years ago
|
||
Backed out for failing own test test_ext_contextual_identities.js: https://hg.mozilla.org/integration/mozilla-inbound/rev/a199014fd2b5fe100e10b9b4557558f6341c16cb https://hg.mozilla.org/integration/mozilla-inbound/rev/dcb0721c0c50771d89daeefdddf23329d6e35e50 https://hg.mozilla.org/integration/mozilla-inbound/rev/90b91d69b5760c4bb454ae6d6ad4e04ae3782f20 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=5db9fcefa874c25c01983bc09b733d43f8db278f Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=40711440&repo=mozilla-inbound [task 2016-12-16T22:44:47.401619Z] 22:44:47 INFO - TEST-PASS | toolkit/components/extensions/test/xpcshell/test_ext_contextual_identities.js | test_contextualIdentities_without_permissions - [test_contextualIdentities_without_permissions : 166] test result correct - "contextualIdentities_permission" == "contextualIdentities_permission" [task 2016-12-16T22:44:47.403295Z] 22:44:47 INFO - (xpcshell/head.js) | test run_next_test 1 pending (2) [task 2016-12-16T22:44:47.404663Z] 22:44:47 INFO - (xpcshell/head.js) | test test_contextualIdentities_without_permissions finished (2) [task 2016-12-16T22:44:47.406297Z] 22:44:47 INFO - toolkit/components/extensions/test/xpcshell/test_ext_contextual_identities.js | Starting test_contextualIdentity_no_containers [task 2016-12-16T22:44:47.407791Z] 22:44:47 INFO - (xpcshell/head.js) | test test_contextualIdentity_no_containers pending (2) [task 2016-12-16T22:44:47.409174Z] 22:44:47 INFO - "Extension loaded" [task 2016-12-16T22:44:47.410722Z] 22:44:47 INFO - (xpcshell/head.js) | test run_next_test 1 finished (2) [task 2016-12-16T22:44:47.412351Z] 22:44:47 INFO - PROCESS | 9351 | ++DOCSHELL 0x7f76d76c2800 == 3 [pid = 9351] [id = {9ed1c7ec-065e-4a63-a9d1-346d7edad87f}] [task 2016-12-16T22:44:47.414125Z] 22:44:47 INFO - PROCESS | 9351 | ++DOMWINDOW == 8 (0x7f76d76cb000) [pid = 9351] [serial = 8] [outer = (nil)] [task 2016-12-16T22:44:47.421388Z] 22:44:47 INFO - PROCESS | 9351 | ++DOMWINDOW == 9 (0x7f76db6ef800) [pid = 9351] [serial = 9] [outer = 0x7f76d76cb000] [task 2016-12-16T22:44:47.423124Z] 22:44:47 INFO - PROCESS | 9351 | ++DOMWINDOW == 10 (0x7f76dd98f000) [pid = 9351] [serial = 10] [outer = 0x7f76d76cb000] [task 2016-12-16T22:44:47.424808Z] 22:44:47 INFO - PROCESS | 9351 | ++DOMWINDOW == 11 (0x7f76d3c05800) [pid = 9351] [serial = 11] [outer = 0x7f76d76cb000] [task 2016-12-16T22:44:47.429807Z] 22:44:47 INFO - PROCESS | 9351 | ++DOCSHELL 0x7f76d3c0b000 == 4 [pid = 9351] [id = {482a3d50-d183-4f2f-b510-3a4f76edcdab}] [task 2016-12-16T22:44:47.431516Z] 22:44:47 INFO - PROCESS | 9351 | ++DOMWINDOW == 12 (0x7f76d3c0c000) [pid = 9351] [serial = 12] [outer = (nil)] [task 2016-12-16T22:44:47.433123Z] 22:44:47 INFO - PROCESS | 9351 | ++DOMWINDOW == 13 (0x7f76d3c12000) [pid = 9351] [serial = 13] [outer = 0x7f76d3c0c000] [task 2016-12-16T22:44:47.434900Z] 22:44:47 INFO - PROCESS | 9351 | JavaScript strict warning: chrome://global/content/bindings/browser.xml, line 409: ReferenceError: reference to undefined property "localName" [task 2016-12-16T22:44:47.436513Z] 22:44:47 INFO - PROCESS | 9351 | ++DOMWINDOW == 14 (0x7f76d3c16800) [pid = 9351] [serial = 14] [outer = 0x7f76d3c0c000] [task 2016-12-16T22:44:47.438175Z] 22:44:47 INFO - "CONSOLE_MESSAGE: (warn) [JavaScript Warning: "ReferenceError: reference to undefined property "localName"" {file: "chrome://global/content/bindings/browser.xml" line: 409}]" [task 2016-12-16T22:44:47.439885Z] 22:44:47 INFO - PROCESS | 9351 | [9351] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80070057: file /home/worker/workspace/build/src/dom/base/nsContentUtils.cpp, line 6175 [task 2016-12-16T22:44:47.441740Z] 22:44:47 WARNING - TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/xpcshell/test_ext_contextual_identities.js | test_contextualIdentity_no_containers - [test_contextualIdentity_no_containers : 92] contextualIdentities API is not available when the containers are disabled - false == true [task 2016-12-16T22:44:47.443407Z] 22:44:47 INFO - resource://testing-common/ExtensionXPCShellUtils.jsm:attachListeners/<:92 [task 2016-12-16T22:44:47.444847Z] 22:44:47 INFO - resource://gre/modules/ExtensionUtils.jsm:runSafeSyncWithoutClone:71 [task 2016-12-16T22:44:47.446381Z] 22:44:47 INFO - resource://gre/modules/ExtensionUtils.jsm:emit/promises<:384 [task 2016-12-16T22:44:47.448654Z] 22:44:47 INFO - resource://gre/modules/ExtensionUtils.jsm:emit:383 [task 2016-12-16T22:44:47.450571Z] 22:44:47 INFO - resource://gre/modules/Extension.jsm:receiveMessage:695 [task 2016-12-16T22:44:47.452496Z] 22:44:47 INFO - /home/worker/workspace/build/tests/xpcshell/head.js:_do_main:210 [task 2016-12-16T22:44:47.454426Z] 22:44:47 INFO - /home/worker/workspace/build/tests/xpcshell/head.js:_execute_test:545 [task 2016-12-16T22:44:47.459931Z] 22:44:47 INFO - -e:null:1 Android shows more failures: https://treeherder.mozilla.org/logviewer.html#?job_id=40710593&repo=mozilla-inbound TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/xpcshell/test_ext_contextual_identities.js | test_contextualIdentity_with_permissions - [test_contextualIdentity_with_permissions : 221] A promise chain failed to handle a rejection: An unexpected error occurred - rejection date: Fri Dec 16 2016 22:20:31 GMT+0000 (GMT) - stack: null - false == true TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/xpcshell/test_ext_contextual_identities.js | test_contextualIdentity_with_permissions - [test_contextualIdentity_with_permissions : 73] Extension left running at test shutdown - "running" == "unloaded"
Flags: needinfo?(amarchesini)
Comment 9•8 years ago
|
||
The Android failures persisted with part 3 applied.
Comment 10•7 years ago
|
||
Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/077c30e250d6 Expose ContextualIdentities (aka containers) to WebExtensions - part 1, r=kmaglione https://hg.mozilla.org/integration/mozilla-inbound/rev/6f7b9683818a Expose ContextualIdentities (aka containers) to WebExtensions - part 2, r=kmaglione
Comment 11•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/077c30e250d6 https://hg.mozilla.org/mozilla-central/rev/6f7b9683818a
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Updated•7 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(amarchesini)
Comment 12•7 years ago
|
||
I've written some docs for this API: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/contextualIdentities Please let me know if this looks OK.
Flags: needinfo?(amarchesini)
Assignee | ||
Comment 13•7 years ago
|
||
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/contextualIdentities/remove the title should be: ContextualIdentities.remove() looks good to me.
Flags: needinfo?(amarchesini)
Comment 14•7 years ago
|
||
There should likely be a warning that the API works in Android however it doesn't have any UX related to it.
Comment 15•7 years ago
|
||
Thanks, I've made those two updates.
Keywords: dev-doc-needed → dev-doc-complete
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•