in order to support the creation of new containers, we must store them somewhere.

Comment on attachment 8771430 [details] [diff] [review] about_containers_4.patch Review of attachment 8771430 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/contextualidentity/content/aboutContainers.js @@ +20,5 @@ > while (containersList.lastChild) { > containersList.lastChild.remove();; > } > > + let identities = ContextualIdentityService.getIdentities(identities => { ContextualIdentityService.getIdentities(identities => {

Sorry for the spam.

With this patch we can create/remove/update a container. without UI, without any check. Just a WIP.

Comment on attachment 8771450 [details] [diff] [review] db.patch Review of attachment 8771450 [details] [diff] [review]: ----------------------------------------------------------------- I'm sorry to be a pain, but please could you push this to mozreview? A lot of the changes are whitespace only, or involve minor changes to lines that otherwise remain identical, and it's a pain to review them with the diff visuals that splinter provides. I have to manually check that indented code stays the same (or one variable in the line changed names while the rest stayed the same), and it makes it much harder to spot mistakes and more time-consuming to review. Generally, can you provide some background as to why we're trying to switch this to an async relational DB system rather than e.g. prefs or an in-memory store that gets serialized to JSON and back? Both of those are in common use and seem like they would support everything we do here - there's no systematic query-ing of the relational DB at all, even where that might be appropriate - we just use getAll() and then filter in JS. :-\ The async-ness worries me. I would prefer having async initialization that blocks startup at some point, rather than this change. It feels like we would not usually notice any difference on our (fast) systems, but on slower systems or in 'interesting' circumstances with multiple DB accesses or whatever, we might be left with un-filled context menus, or non-matching labels / colors. Already the asynchronicity probably introduces more layout/style flushes. For things like synced tabs, we provide a "we're fetching these" view. Not sure if doing that here would be overkill or if we should at least provide some kind of disabled menuitem that says (fetching containers) or the like. ::: toolkit/components/contextualidentity/ContextualIdentityService.jsm @@ +15,5 @@ > XPCOMUtils.defineLazyGetter(this, "gBrowserBundle", function() { > return Services.strings.createBundle("chrome://browser/locale/browser.properties"); > }); > > +this.CONTAINERSDB_NAME = "settings"; Seems like we should use something clearer and less generic than "settings" as the name for the file/db. @@ +21,5 @@ > +this.CONTAINERSSTORE_NAME = "settings"; > + > +function _ContextualIdentityService() {} > +_ContextualIdentityService.prototype = { > + __proto__: IndexedDBHelper.prototype, Please use Object.create() instead, __proto__ is non-standard. @@ +72,5 @@ > > + upgradeSchema: function upgradeSchema(transaction, db, oldVersion, > + newVersion) { > + if (oldVersion != 0 || newVersion != 1) { > + dump("ContextualIdentityService - ERROR - unknown schema version!!!\n"); Please use Cu.reportError() instead. @@ +86,3 @@ > }, > > + txn: function(txnType, cb) { Do we have to mirror the cryptic naming? Can't we just call this newTransaction() ? @@ +95,5 @@ > }, > > + getIdentities(cb) { > + this.txn("readonly", function(txn, store) { > + store.getAll().onsuccess = function(event) { What happens if two transactions get created using this method? Do they just get queued? Something else? @@ +157,3 @@ > > + if (!identity.alreadyOpened) { > + identity.alreadyOpened = true; Where/how does this get reflected back into the DB and/or other objects? All those transactions are labeled "readonly", is that just not true? Does this even want storing in the DB at all? If not, how does that jive with continually re-getting it? Shouldn't that info be local to the JSM and thrown away on exit? That means this might not even need to be async...

Comment on attachment 8772527 [details] [diff] [review] json.patch Review of attachment 8772527 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/preferences/cookies.js @@ +12,1 @@ > This file only loads when you open the preferences, so it seems OK to just leave this as a lazy getter. If you do add it, please keep the list of imports alphabetically sorted. ::: toolkit/components/contextualidentity/ContextualIdentityService.jsm @@ +129,5 @@ > + this.saveSoon(); > + }, > + > + saveSoon() { > + this._saver.arm(); Should we file a followup bug to make sure a save blocks shutdown, or do you want to take care of this here? @@ +133,5 @@ > + this._saver.arm(); > + }, > + > + save() { > + let bytes = gTextEncoder.encode(JSON.stringify(this._identities)); As written this will write the 'alreadyOpen' variable and then restore it after startup. That seems wrong. @@ +135,5 @@ > + > + save() { > + let bytes = gTextEncoder.encode(JSON.stringify(this._identities)); > + return OS.File.writeAtomic(this._path, bytes, > + { tmpPath: this._path + ".tmp" }) Nit: need a semicolon at the end here. Is eslint not running for these files?

Comment on attachment 8772529 [details] [diff] [review] part 2 Review of attachment 8772529 [details] [diff] [review]: ----------------------------------------------------------------- Ah, this is what I get for reviewing patches one by one... rs=me for using a Set rather than an array - sorry about the mistaken comment about the alreadyOpened thing on the first patch. ::: toolkit/components/contextualidentity/ContextualIdentityService.jsm @@ +74,4 @@ > ], > > _identities: null, > + _openedIdentities: [], Nit: openedIdentities: new Set(), @@ +209,5 @@ > if (!identity || !identity.public) { > return; > } > > + if (this._openedIdentities.indexOf(userContextId) == -1) { this._openedIdentities.has(userContextId) @@ +210,5 @@ > return; > } > > + if (this._openedIdentities.indexOf(userContextId) == -1) { > + this._openedIdentities.push(userContextId); s/push/add/

Comment on attachment 8772791 [details] [diff] [review] part 1 - JSON Review of attachment 8772791 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/contextualidentity/ContextualIdentityService.jsm @@ +11,3 @@ > > +const DEFAULT_TAB_COLOR = "#909090"; > +const SAVE_DELAY_MS = 1500 Missing semicolon

Comment on attachment 8772838 [details] [diff] [review] part 3 - versioning Review of attachment 8772838 [details] [diff] [review]: ----------------------------------------------------------------- What's the lastUserContextId?

Something that I'll use in a completely different patch. Still uploaded here in less than 1 minute.

This is the last patch for this bug. It's easier for me to land this block of code also if create/remove/update are not currently in used anywhere. But they will be in about:containers, eventually.

Comment on attachment 8772906 [details] [diff] [review] part 4 - create/remove/update Review of attachment 8772906 [details] [diff] [review]: ----------------------------------------------------------------- r- because the addition code is busted if you've removed items. This should also have some tests. :-) ::: toolkit/components/contextualidentity/ContextualIdentityService.jsm @@ +160,5 @@ > }, > > + create(name, icon, color) { > + let identity = { > + userContextId: ++this._lastUserContextId, OK, so we're mixing things up a bit here. As written, the IDs are sequential, but the private one can't be removed with remove(), and you're splicing (!) the removed items out. So consider: we start with 5 items: 1: id 1 2: id 2 3: id 3 4: id 4 5: id 5 (and private) and the length is 5. So if you add an item immediately, the id would be 6. Let's not do that, it's not very interesting. Instead, let's say you remove item (3). Now the length is 4 Then you add a new item. What ID will we pick? IOW, I don't think this code works. I also think it needs unit tests. It's easy to land when nothing exercises it, but we should have slightly higher standards for code on m-c. :-) @@ +164,5 @@ > + userContextId: ++this._lastUserContextId, > + public: true, > + icon: icon, > + color: color, > + name: name nit: icon, color, name (when the property and the local variable are the same, you don't need to repeat the key: value in ES2015 or whatever version of JS we're up to) @@ +175,5 @@ > + }, > + > + update(userContextId, name, icon, color) { > + let found = false; > + this._identities.forEach((identity, index) => { let identity = this._identities.find(identity => identity.userContextid == userContextId && identity.public); if (identity) { identity.name = name; identity.color = color; // etc. } return !!identity; @@ +180,5 @@ > + if (identity.userContextId == userContextId && identity.public) { > + this._identities[index].name = name; > + this._identities[index].color = color; > + this._identities[index].icon = icon; > + delete this._identities[index].label; Can we change "label" to be more explicitly a localization ID, maybe "l10nID" or something? That makes more sense of this code.

Comment on attachment 8772838 [details] [diff] [review] part 3 - versioning Review of attachment 8772838 [details] [diff] [review]: ----------------------------------------------------------------- Clearing review because I think it might end up being this code that you also end up updating to make sure new IDs don't conflict. Because IDs shouldn't change (because they're used internally all over the place), we can't really renumber them. Instead, we should have code that determines the first free ID, or we should assume that the array is sorted and use the last item's ID. I would be OK with either of those, but if we do the latter we should ensure that the IDs are sequential when we load the file and replace them with the defaults if not. Really Bad Things would happen if IDs get overwritten or mixed up (things would (not) be private when we expect them (not) to, or be mislabeled, or...)

Final bit of feedback: we will need observer notifications or other types of communication to ensure that if the user updates labeling/colors/icons in about:containers, the relevant bits of UI actually update. If you're not planning to take care of that in this bug, can you ensure we file a followup bug? :-) This is particularly complicated for the 'remove' case. What happens to open browsers with that userContextID? What happens when we ask for labels/colors/icons for a userContextID that has been removed?

> What ID will we pick? 6. This is the reason why I use and store lastUsedContextId. I don't count/iterate to know the 'next' one. I store the lastUserContextID into the JSON object in order to avoid this particular issue. > IOW, I don't think this code works. I'll add some tests.

Right. Bug 1288029 has been filed for this reason. What I think it's the correct approach is: 1. you cannot disable Containers from pref if some of them are opened. 2. you cannot delete a container if it's opened. But this check should not be in the ContextualIdentityService, imo. Maybe some helper function yes, but the logic of remove/update/create should be kept simple.

(In reply to Andrea Marchesini (:baku) from comment #20) > > What ID will we pick? > > 6. This is the reason why I use and store lastUsedContextId. I don't > count/iterate to know the 'next' one. I store the lastUserContextID into the > JSON object in order to avoid this particular issue. Ugh, sorry, I got confused because you use the array length when you initially set lastUserContextId. But you never actually update it except by incrementing it. > > IOW, I don't think this code works. > > I'll add some tests. I think that's still a good idea. :-)

With tests.

Comment on attachment 8773256 [details] [diff] [review] part 4 - create/remove/update Review of attachment 8773256 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/contextualidentity/ContextualIdentityService.jsm @@ +1,5 @@ > /* This Source Code Form is subject to the terms of the Mozilla Public > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > +dump("A\n"); Please remove this. @@ +165,5 @@ > > + create(name, icon, color) { > + let identity = { > + userContextId: ++this._lastUserContextId, > + public: true, icon, color, name Nit: I'd leave them on individual lines, and have a comma after the last one, to make it easy and non-blame-modifying to add more properties. @@ +182,5 @@ > + identity.name = name; > + identity.color = color; > + identity.icon = icon; > + delete this._identities.l10nID; > + delete this._identities.accessKey; These deletes look wrong. You want: delete identity.l10nID delete identity.accessKey right? @@ +192,5 @@ > + > + remove(userContextId) { > + let found = false; > + > + this._identities.forEach((identity, index) => { This can use findIndex, which avoids having a separate bool 'found' and a level of nesting, and looping over the rest of the array once you've found the item you're looking for, ie: let index = this._identities.findIndex(i => i.userContextId == userContextId && identity.public); if (index > -1) { ... } return index > -1; @@ +231,5 @@ > }, > > getIdentities() { > this.ensureDataReady(); > return this._identities.filter(info => info.public); Do we need to make sure the stuff we're handing out here is read-only or a copy, rather than references to the original objects? Because right now, you could call this and assign to identities[2].name and it would modify the internal state of the CIS. ::: toolkit/components/contextualidentity/tests/unit/test_basic.html @@ +46,5 @@ > +}); > + > +// Remove an identity > +add_task(function() { > + equal(cis.getIdentities().length, 5, "Expected 5 containers."); This depends on the previous task, so it should probably be in the same task function. @@ +58,5 @@ > +// Update an identity > +add_task(function() { > + ok(!!cis.getIdentityFromId(2), "Identity 2 exists"); > + > + equal(cis.update(-1, "Container", "Icon", "Color"), false, "Update returns true if everything is OK"); Nit: update assertion message here. ::: toolkit/components/contextualidentity/tests/unit/xpcshell.ini @@ +1,3 @@ > +[DEFAULT] > + > +[test_basic.html] This should be a .js file.

Comment on attachment 8773359 [details] [diff] [review] part 5 - remove containers for real Review of attachment 8773359 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/contextualidentity/content/aboutContainers.js @@ +64,5 @@ > + > + refreshUI(); > +}); > + > +function refreshUI() { We could also just make the UI execute the updates itself instead of re-writing the entire list. Make remove() remove the item, create() just render the new item (for which we already have a helper), and update() just call replaceChild() with a re-rendered new item and a ref to the old one. @@ +110,5 @@ > + "#00a7e0"); > + refreshUI(); > +} > + > +function preferences(userContextId) { Maybe we can just land this without the "testing" code and buttons, and add this functionality in a followup? Either that or keep iterating, I guess. But I'm assuming you're not expecting me to review the testing code... @@ +134,3 @@ > } > > function remove(userContextId) { Shouldn't this be checking that there's no open browsers with this userContextId etc.? @@ +134,4 @@ > } > > function remove(userContextId) { > + ContextualIdentityService.remove(parseInt(userContextId)); Feels like we should make sure the functions here get sanitized arguments, and the click handler gets the usercontextid and makes it sane. :-)

Sorry, that was the wrong patch.

Comment on attachment 8773744 [details] [diff] [review] part 5 - remove containers for real Review of attachment 8773744 [details] [diff] [review]: ----------------------------------------------------------------- rs=me though this will need rebasing based on the feedback on the earlier patch. ::: toolkit/components/contextualidentity/ContextualIdentityService.jsm @@ +198,5 @@ > this._identities.splice(index, 1); > this._openedIdentities.delete(userContextId); > + > + Services.obs.notifyObservers(null, "clear-origin-data", > + JSON.stringify({ userContextId })); What happens with history/formdata etc.? I'm a bit surprised this doesn't use the sanitizer.

Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/acbca9af0a3b part 1 - ContextualIdentityService should use a storage for the containers, r=Gijs https://hg.mozilla.org/integration/mozilla-inbound/rev/939d5064ab91 part 2 - ContextualIdentityService must store telemetry data in a separate array, r=Gijs https://hg.mozilla.org/integration/mozilla-inbound/rev/7ed07b35bad6 part 3 - ContextualIdentityService JSON file should have versions, r=Gijs https://hg.mozilla.org/integration/mozilla-inbound/rev/edad0174cb12 part 4 - ContextualIdentityService create/remove/update, r=Gijs https://hg.mozilla.org/integration/mozilla-inbound/rev/c57fd3af416e part 5 - ContextualIdentityService should dispatch 'clear-origin-data' when a container is deleted, r=Gijs

All backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/820961ff0369 for build bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=32459345&repo=mozilla-inbound#L18150

Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/bf7c4e797f76 part 1 - ContextualIdentityService should use a storage for the containers, r=Gijs https://hg.mozilla.org/integration/mozilla-inbound/rev/93fa501cfd3a part 2 - ContextualIdentityService must store telemetry data in a separate array, r=Gijs https://hg.mozilla.org/integration/mozilla-inbound/rev/c5efa902cbe5 part 3 - ContextualIdentityService JSON file should have versions, r=Gijs https://hg.mozilla.org/integration/mozilla-inbound/rev/c01aeb081e2d part 4 - ContextualIdentityService create/remove/update, r=Gijs https://hg.mozilla.org/integration/mozilla-inbound/rev/165e7b455b8e part 5 - ContextualIdentityService should dispatch 'clear-origin-data' when a container is deleted, r=Gijs

backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=32499351&repo=mozilla-inbound

Backout by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/5ad8ed662107 Backed out changeset 165e7b455b8e https://hg.mozilla.org/integration/mozilla-inbound/rev/50b8cc9b8e57 Backed out changeset c01aeb081e2d https://hg.mozilla.org/integration/mozilla-inbound/rev/37e221f3b0df Backed out changeset c5efa902cbe5 https://hg.mozilla.org/integration/mozilla-inbound/rev/905027321962 Backed out changeset 93fa501cfd3a https://hg.mozilla.org/integration/mozilla-inbound/rev/8b0f3616c00a Backed out changeset bf7c4e797f76 for bc3/bc6 test failures

Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/62eb0641f4be part 1 - ContextualIdentityService should use a storage for the containers, r=Gijs https://hg.mozilla.org/integration/mozilla-inbound/rev/ddc292500b8f part 2 - ContextualIdentityService must store telemetry data in a separate array, r=Gijs https://hg.mozilla.org/integration/mozilla-inbound/rev/57942a8e13b2 part 3 - ContextualIdentityService JSON file should have versions, r=Gijs https://hg.mozilla.org/integration/mozilla-inbound/rev/11a988cd506a part 4 - ContextualIdentityService create/remove/update, r=Gijs https://hg.mozilla.org/integration/mozilla-inbound/rev/f577fea91160 part 5 - ContextualIdentityService should dispatch 'clear-origin-data' when a container is deleted, r=Gijs

https://hg.mozilla.org/mozilla-central/rev/62eb0641f4be https://hg.mozilla.org/mozilla-central/rev/ddc292500b8f https://hg.mozilla.org/mozilla-central/rev/57942a8e13b2 https://hg.mozilla.org/mozilla-central/rev/11a988cd506a https://hg.mozilla.org/mozilla-central/rev/f577fea91160

(In reply to :Gijs Kruitbosch from comment #6) > Generally, can you provide some background as to why we're trying to switch > this to an async relational DB system rather than e.g. prefs or an in-memory > store that gets serialized to JSON and back? Both of those are in common use > and seem like they would support everything we do here - there's no > systematic query-ing of the relational DB at all, even where that might be > appropriate - we just use getAll() and then filter in JS. :-\ There is no reply to this question in any of these comments. Why was this necessary? What did the usage of prefs not allow?

> There is no reply to this question in any of these comments. Why was this > necessary? What did the usage of prefs not allow? We decided to serialize everything into a JSON file. We also use a optimistic approach where the JSON file is loaded at the first execution of ContextualIdentityService.jsm in order to provide a sync API. If the loading is too slow, we lock the UI as we do for other similar components.

Why did the usage of prefs not allow it?

The reason why I prefer a JSON file is because we are currently storing the 4 default containers, each one with an icon name and a color. The user can also create more containers and maybe in the future, customize the icons. I think it's better to have a separate file for all of these instead use/abuse of prefs.

This added significant code overhead when prefs would have been sufficient. You can see the prefs for browser.contentHandlers.types.*.title for an example of how we could have done this pretty easily with prefs, or we could have stored a JSON blob as a pref value like we do for browser.uiCustomization.state.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #40) > This added significant code overhead when prefs would have been sufficient. > You can see the prefs for browser.contentHandlers.types.*.title for an > example of how we could have done this pretty easily with prefs, Number of reasons to not use prefs here, mainly perf/convenience/UX-related: - it's kind of an abuse of prefs to keep values that aren't actually user preference related, such as telemetry identifiers, internal state tracking of which containers are open, etc. - keeping track of potentially localizable content (labels + access keys) inside prefs is... odd. - things like the images used for the containers could potentially be quite large if we end up storing image data in data URIs or similar. Depends how we let people pick those images, but that wasn't clear when this patch was written. - the perf characteristics of prefs (main thread sync IO, writing the entire file every time one part of any Fx/platform pref changes) aren't very nice - the default containers are removable, so they can't be default pref values because then you can't remove them, which means you have to store all the container info in user prefs (which get re-read/written all the time), so it's not very efficient and there's no good way to "reset" them. - "good" prefs are synced with the UI, and about:config lets people toggle prefs. If I change the prefs, that takes effect. In the case of containers, when removing custom containers we want to do things like close all the relevant container tabs. There's no way to enforce that with prefs, meaning users could easily trash their browser by flipping a few things in about:config. That's not an attractive prospect either. > or we could > have stored a JSON blob as a pref value like we do for > browser.uiCustomization.state. That pref is already running up to the "warning" size limitations of prefs on not-a-few profiles. With 20/20 hindsight, I would not have put that in prefs (or found some more condensed storage mechanism). It's not really a "success story" example for putting data in prefs, IMO. Sticking lots of data into prefs slows down both pref reads and writes, which ends up slowing down early startup as well as routine other writing of prefs triggered by e.g. telemetry (yes, it does that...).