Currently I can query to find out details about a contextualIdentity, but I can't tell when they change in a webExtension. In my extension I list all the tabs and show the colour of a container. However if a user goes to "Manage containers" and changes the colour from yellow to orange... the WebExtension won't know without querying again. The standard way to do this is some events like: contextualIdentities.onCreated contextualIdentities.onChanged contextualIdentities.onRemoved The extension can then addListeners if they'd like to find out when they change.

There isn't any implications in adding this is there? This would be useful in other extensions outside the Test Pilot.

No particular issue. BTW, are they needed for Test Pilot?

No as we are in charge of all the creation points, so it's not needed. (if users have managed to add through the about:preferences panel then they have a bug as we disable it).

Assuming I add a test or 5 does the current attached patch look ok?

Comment on attachment 8867511 [details] Bug 1344519 - Add web extension events for containers onUpdated, onCreated and onRemoved https://reviewboard.mozilla.org/r/139050/#review142600 ::: toolkit/components/contextualidentity/ContextualIdentityService.jsm:175 (Diff revision 1) > }, > > create(name, icon, color) { > + let userContextId = ++this._lastUserContextId; > let identity = { > - userContextId: ++this._lastUserContextId, > + userContextId: userContextId, just: userContextId,

Comment on attachment 8867511 [details] Bug 1344519 - Add web extension events for containers onUpdated, onCreated and onRemoved https://reviewboard.mozilla.org/r/139050/#review142602

Yes, it looks correct. A test is needed, and also a proper review from a webextension peer.

As we are likely to launch this on AMO this will cause issues with our addon not being the only crud end points for containers. We will need these events to ensure menus are updated correctly and others will need it to reduce calling the API repeatedly.

So my current thinking is: - Use onChanged like cookies or use onUpdated like tabs? - Should we flatten the contextual identity to be the return value or should we nest it under a "contextualIdentity" key like cookies do - We don't have the full object for removed, should I flatten this differently? Curently: browser.contextualIdentities.onCreated.addListener((e) => {console.log(e);}); // {contextualIdentity: {cookieStoreId: 12, icon: "fingerprint" color: "red", name: "bloop"}} browser.contextualIdentities.onChanged.addListener((e) => {console.log(e);}); // {contextualIdentity: {cookieStoreId: 12, icon: "fingerprint" color: "red", name: "bloop"}} browser.contextualIdentities.onRemoved.addListener((e) => {console.log(e);}); // {cookieStoreId: 12} My rationale to nest in the "contextualIdentity" key is if we needed reason etc. However this feels a little excessive for remove. I could possibly make the event pass the deleted data for the container?

I'm going to make the following changes and hope they are ok: - onUpdated as we have contextualIdentities.update rather than change. - Return the full object to be consistent on onRemoved - Serialise the JSON structure of the current state of the container in ContextualIdentitiesService and unserialize in the extension code rather than query again. Should we care about the user manually flipping the container prefs outside adding our own containers changes? We could file a follow up bug to trigger onUpdated and onRemoved listeners on pref change to the current list of containers.

I made a demo extension as part of testing this: https://github.com/jonathanKingston/extension-debugging/tree/master/container-events Feel free to use this code in the documentation process or update the one Andy had.

Comment on attachment 8867511 [details] Bug 1344519 - Add web extension events for containers onUpdated, onCreated and onRemoved https://reviewboard.mozilla.org/r/139050/#review164622 ::: toolkit/components/contextualidentity/ContextualIdentityService.jsm:195 (Diff revision 2) > name > }; > > this._identities.push(identity); > this.saveSoon(); > + Services.obs.notifyObservers(null, "contextual-identity-created", this.getIdentityObserverOutput(identity)); indentation. ::: toolkit/components/contextualidentity/ContextualIdentityService.jsm:213 (Diff revision 2) > delete identity.l10nID; > delete identity.accessKey; > > - Services.obs.notifyObservers(null, "contextual-identity-updated", userContextId); > this.saveSoon(); > + Services.obs.notifyObservers(null, "contextual-identity-updated", this.getIdentityObserverOutput(identity)); indentation. ::: toolkit/components/extensions/ext-contextualIdentities.js:23 (Diff revision 2) > > return result; > }; > > +const convertIdentityFromObserver = identityJSON => { > + let identity = JSON.parse(identityJSON); This seems fragile. Would be nice if we create the object passing 1 by 1 the parameter we want from the JSON object. Sometimes I wish JS had MOZ_ASSERT()

Comment on attachment 8867511 [details] Bug 1344519 - Add web extension events for containers onUpdated, onCreated and onRemoved https://reviewboard.mozilla.org/r/139050/#review165860 a few little nits but overall looks great, thanks! ::: toolkit/components/contextualidentity/ContextualIdentityService.jsm:195 (Diff revision 3) > name > }; > > this._identities.push(identity); > this.saveSoon(); > + Services.obs.notifyObservers(null, "contextual-identity-created", nit: can you pass all the details as the subject of the notification instead of as a json encoded string? ::: toolkit/components/extensions/ext-contextualIdentities.js:144 (Diff revision 3) > return Promise.resolve(convertedIdentity); > }, > + > + onCreated: new EventManager(context, "contextualIdentities.onCreated", fire => { > + let observer = (subject, topic, identityJSON) => { > + if (containersEnabled) { I'm confused by this check. If it is possible to create/change/remove containers even when using them is disabled by the pref, won't extensions want to be able to track those changes? ::: toolkit/components/extensions/test/xpcshell/test_ext_contextual_identities.js:116 (Diff revision 3) > + > + browser.test.notifyPass("contextualIdentities_events"); > + } > + > + let extension = ExtensionTestUtils.loadExtension({ > + background: `(${backgroundScript})()`, nit, this can just be `background: backgroundScript,` (or rename the function to be `background` and the entire line can be `background,`)

Hey thanks for the review! I have some questions. > I'm confused by this check. If it is possible to create/change/remove containers even when using them is disabled by the pref, won't extensions want to be able to track those changes? The check is there such that containers could be enabled by the pref or some other means and the containers addon could be notified of the additions? Maybe this isn't important at all if my other patch to land that container addons can always enable containers? > nit: can you pass all the details as the subject of the notification instead of as a json encoded string? Do you mean userContextId or something? The issue here is the delete happens before the containers code gets called so I can't query for the container there. Or are you speaking of just passing a raw object as the subject, is that possible even? > nit, this can just be `background: backgroundScript,` (or rename the function to be `background` and the entire line can be `background,`) Ah yeah cool :) thanks.

(In reply to Jonathan Kingston [:jkt] from comment #17) > > I'm confused by this check. If it is possible to create/change/remove containers even when using them is disabled by the pref, won't extensions want to be able to track those changes? > > The check is there such that containers could be enabled by the pref or some > other means and the containers addon could be notified of the additions? > Maybe this isn't important at all if my other patch to land that container > addons can always enable containers? Sorry, I didn't follow the first sentence. I agree that if we force containers to be on then this is irrelevant though (I've been waiting on that other patch for others to weigh in on the high level concept before going over the code in detail) > > nit: can you pass all the details as the subject of the notification instead of as a json encoded string? > > Do you mean userContextId or something? The issue here is the delete happens > before the containers code gets called so I can't query for the container > there. Or are you speaking of just passing a raw object as the subject, is > that possible even? Yeah, I meant passing the raw object as the subject. Its possible, the observer service is all xpcom stuff but its doable with wrappedJSObject. Here's an example: http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/toolkit/mozapps/extensions/AddonManager.jsm#2060-2061 http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/browser/modules/ExtensionsUI.jsm#221

> Sorry, I didn't follow the first sentence. My hunch was that we could have containers extensions listen for when containers get added rather than having a containersEnabled event they would listen for. I think we can safely assume that if containers get enabled with extensions that want to use them we can ignore this check. > (I've been waiting on that other patch for others to weigh in on the high level concept before going over the code in detail) No worries I have been put on something else this week, I'm currently thinking of making the ContainersService observe the pref and remove/enable containers there and clear all tabs. It's a refactor I haven't had time to do at the moment. > Its possible, the observer service is all xpcom stuff but its doable with wrappedJSObject. Ah ok, I think I have done this before in cpp methods actually, thanks.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/59765ae0aee3 Add web extension events for containers onUpdated, onCreated and onRemoved r=aswan,baku

Backed out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=119795653&repo=autoland https://hg.mozilla.org/integration/autoland/rev/a03f5a66ca83a85e8c8429afc371752f2ef7784c

Comment on attachment 8867511 [details] Bug 1344519 - Add web extension events for containers onUpdated, onCreated and onRemoved https://reviewboard.mozilla.org/r/139050/#review168740 ::: toolkit/components/contextualidentity/tests/unit/test_basic.js:10 (Diff revision 5) > const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components; > > Cu.import("resource://gre/modules/ContextualIdentityService.jsm"); > +Cu.import("resource://gre/modules/osfile.jsm"); > > -const TEST_STORE_FILE_NAME = "test-containers.json"; > +const TEST_STORE_FILE_PATH = OS.Path.join(OS.Constants.Path.profileDir, "test-containers.json"); I'm not really sure why the following error wasn't an issue before: https://treeherder.mozilla.org/logviewer.html#?job_id=119795653&repo=autoland&lineNumber=2692 Essentially this relates to using a relative path for the test file: http://searchfox.org/mozilla-central/source/toolkit/components/contextualidentity/ContextualIdentityService.jsm#269 :KWierso has there been any changes in how xpcshell read files perhaps? Maybe the file isolation sandbox work. I'm also not really sure why this wouldn't fail the test. The other error is somewhat understandable in that the lazy reading of the strings didn't initialise before and now we explicitly read these before sending the events. Either way is this ready for merging?

See: https://bugzilla.mozilla.org/show_bug.cgi?id=1344519#c27 Thanks

It's possible the sandbox level 3 stuff broke this. I'd say it's worth trying again.

Autoland can't push this until all pending issues in MozReview are marked as resolved.

Sorry again!

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/31db4b302143 Add web extension events for containers onUpdated, onCreated and onRemoved r=aswan,baku

I had to back this out for android xpcshell failures like https://treeherder.mozilla.org/logviewer.html#?job_id=120837712&repo=autoland

Backout by kwierso@gmail.com: https://hg.mozilla.org/integration/autoland/rev/18e395b49530 Backed out changeset 31db4b302143 for android xpcshell test_basic.js failures a=backout CLOSED TREE

TL;DR I think the risk is super low to check this in again. I added the strings which made the tests fail :KWierso, am I in need of review again. Both of my reviewers are away it seems. Essentially we should have more tests covering Android and we expanded the test such that it broke Android, but t would have already been broken somewhat without these strings. Either way web extensions are unlikely to be running on Android at the moment as they don't have any UX, I also haven't had time to work on testing physically an extension on Android.

Requesting check-in here as no changes were made besides the modification of strings.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/3323bbf5cea8 Add web extension events for containers onUpdated, onCreated and onRemoved r=aswan,baku

I've added some docs page: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/contextualIdentities/onCreated https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/contextualIdentities/onRemoved https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/contextualIdentities/onUpdated Please let me know if this covers it.

This looks great, sorry for the delay in reviewing. Thanks!

I tryed reproducing the issue using the example extension and the https://addons.mozilla.org/en-us/firefox/addon/multi-account-containers/ webextension from amo but was unable to reproduce it. What am I suppose to be looking out for? Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.

I installed every event: https://addons.mozilla.org/en-US/firefox/addon/every-event/ Clicked on the browser action button to get to configuration and turned logging off for all events EXCEPT contextualIdentity events, then enabled notifications. Then with a browser console open, I went to about:preferences and created a new container, updated it and then deleted it. I got these messages: Event fired: browser.contextualIdentities.onCreated Arguments: Object { contextualIdentity: Object } Event fired: browser.contextualIdentities.onUpdated Arguments: Object { contextualIdentity: Object } Event fired: browser.contextualIdentities.onRemoved Arguments: Object { contextualIdentity: Object } I would call this verified fixed, but that flow might work for you.

I can reproduce this issue on Firefox 57.0a1 (20170813183258) under Wind 10 64-bit. This issue is verified as fixed on Firefox 57.0b11 (20171023180840) under Wind 10 64-bit and Mac OS X 10.13.