The global state is going away. This should probably be checking a docshell or something instead, but we may need to create a window-level PB status for this to hook into instead.

http://mxr.mozilla.org/mozilla-central/source/browser/modules/NewTabUtils.jsm is the problem file here, but its various singletons that access the Storage object are used in a number of JS files (http://mxr.mozilla.org/mozilla-central/source/browser/base/content/newtab/newTab.js#23). I propose that we change the gLinks, gPinnedLinks and gBlockedLinks into objects that take a window parameter and store a Storage object that remembers the window for future use. We can get rid of the PrivateBrowsingStorage object and add a third parameter to the getLocalStorageForPrincipal that is true or false depending on the value of window.gPrivateBrowsingUI.privateWindow. So, the main takeaway points: * most of the singletons have to become doubletons (Links, BlockedLinks, PinnedLinks, and Storage) * the removal of PrivateBrowsingStorage should wait until bug 722857 lands * the actual users of g[Pinned|Blocked]Links shouldn't need to be changed, since the objects should just store a reference to a Storage object that is appropriate for the window that initializes them

Saurabh said he's interested in working on this.

Comment on attachment 628203 [details] [diff] [review] Patch v1 Removing feedback? flag from this patch as per conversation on IRC. Saurabh said he's following up with an updated patch soon.

Further changes we'll need to make: * the storage observers should be unnecessary - private windows have different storage instances than public ones, and there shouldn't be any reason to reset the caches once we leave PB mode for the same reason * the telemetry for the pref in the patch is incorrect - we need to check the actual value of the pref (eg. http://mxr.mozilla.org/mozilla-central/source/browser/modules/NewTabUtils.jsm#219)

Hey Saurabh, thank you for diving so deep into the code! I'm actually not sure that we want to keep the current behavior for the New Tab Page and that we really need a separate PrivateBrowsingStorage. IMHO we shouldn't allow any modification to the newtab page at all when the user is in private browsing mode. This way they still can access all their shortcuts but not drag any strange links on it. Another option would be to handle it kind of like Chrome does. We could have a specialized newtab page (e.g. about:privatebrowsing) for private browsing mode that shows some useful information to the user. We could also modify about:newtab itself to reflect the browser's state change? Anyway we didn't really spec out the behavior in PB mode, yet. Adding some people involved in the newtab page feature to find out what kind of behavior we want before we continue spending time in the wrong direction. (Also, bug 748530 comment #7 seems relevant.)

(In reply to Tim Taubert [:ttaubert] from comment #7) > Another option would be to handle it kind of like Chrome does. We could have > a specialized newtab page (e.g. about:privatebrowsing) for private browsing > mode that shows some useful information to the user. This would solve confusion for users affected by bug 762438.

The goal of this bug is not to design a new interaction for new tab pages in PB mode. The goal here is to make NewTabUtils.jsm and friends stop accessing the global PB mode flag. Once we get all of the foundation pieces, we can discuss the UI details of per-window PB mode, but we're not quite there yet, so let's keep this bug in the scope it was filed for! :-)

(In reply to Ehsan Akhgari [:ehsan] from comment #9) > The goal of this bug is not to design a new interaction for new tab pages in > PB mode. The goal here is to make NewTabUtils.jsm and friends stop > accessing the global PB mode flag. Yes, you're right. I thought about this too when writing the comment. The thing is though that the patch converts singletons to doubletons and touches a lot of stuff that isn't necessary and would have to be reverted if we can decide about a simpler solution for new tab pages in PB mode. I could probably open another bug, blocking this one.

The question is, do we have a UI decision on how the new tab should work in PB mode *right now*? If we don't, then I don't want to stall this bug waiting for that to happen. Arguably this is a decision which should have been made when the new tab window was being implemented. Unfortunately it didn't happen, and I wasn't looped in either...

(In reply to Ehsan Akhgari [:ehsan] from comment #11) > The question is, do we have a UI decision on how the new tab should work in > PB mode *right now*? If we don't, then I don't want to stall this bug > waiting for that to happen. I want to find out if this involves a longer discussion or if UX agrees about a certain behavior. I don't want to block per-window private browsing any longer. Will ping all the people involved to get some quick feedback. > Arguably this is a decision which should have been made when the new tab > window was being implemented. Unfortunately it didn't happen, and I wasn't > looped in either... Right :|

With bug 763468 implemented users can still open "about:newtab" by simply typing it into the address bar. I'm not sure how we should behave in this probably very rare situation. Should we just make "about:newtab" immutable? Should we redirect to "about:privatebrowsing"?

(In reply to Tim Taubert [:ttaubert] from comment #13) > With bug 763468 implemented users can still open "about:newtab" by simply > typing it into the address bar. I'm not sure how we should behave in this > probably very rare situation. Should we just make "about:newtab" immutable? > Should we redirect to "about:privatebrowsing"? I don't think we should do anything if the user types in the URL manually. We should just show them the new tab page.

(In reply to Ehsan Akhgari [:ehsan] from comment #14) > (In reply to Tim Taubert [:ttaubert] from comment #13) > > With bug 763468 implemented users can still open "about:newtab" by simply > > typing it into the address bar. I'm not sure how we should behave in this > > probably very rare situation. Should we just make "about:newtab" immutable? > > Should we redirect to "about:privatebrowsing"? > > I don't think we should do anything if the user types in the URL manually. > We should just show them the new tab page. And allow them to modify its data persistently in PB mode? Or behave like we do now and forget all the modifications after returning from PB?

(In reply to Tim Taubert [:ttaubert] from comment #15) > (In reply to Ehsan Akhgari [:ehsan] from comment #14) > > (In reply to Tim Taubert [:ttaubert] from comment #13) > > > With bug 763468 implemented users can still open "about:newtab" by simply > > > typing it into the address bar. I'm not sure how we should behave in this > > > probably very rare situation. Should we just make "about:newtab" immutable? > > > Should we redirect to "about:privatebrowsing"? > > > > I don't think we should do anything if the user types in the URL manually. > > We should just show them the new tab page. > > And allow them to modify its data persistently in PB mode? Or behave like we > do now and forget all the modifications after returning from PB? Ideally, disallowing pages to be added to the list (instead of allowing that at first and then forgetting about it), and allow all other modifications, as they won't have any privacy implications.

(In reply to Ehsan Akhgari [:ehsan] from comment #16) > Ideally, disallowing pages to be added to the list (instead of allowing that > at first and then forgetting about it), and allow all other modifications, > as they won't have any privacy implications. Sounds like a good solution to me. So we'd just need to remove the PrivateBrowsingStorage and the associated observers from the JSM because we don't need them anymore. If about:newtab detects it has been loaded into a private-browsing docShell then its cells should just ignore any attempts to drop links on it. Saurabh, is that still something you want to work on? I can definitely give some more guidance if needed.

As Josh pointed, I have also removed the pin and delete icon(basically frozen the links) in case of PB mode. I have also removed the Storage singleton observer.

Comment on attachment 634235 [details] [diff] [review] Patch v3 Review of attachment 634235 [details] [diff] [review]: ----------------------------------------------------------------- We're almost there. Looks pretty good to me. Please do also remove the lazy service getter definition for "gPrivateBrowsing" at the top of the JSM. ::: browser/base/content/newtab/cells.js @@ +99,5 @@ > */ > handleEvent: function Cell_handleEvent(aEvent) { > + // We are basically going to avoid doing anything when the user is in PB mode > + // or when the draggedSite is external. > + // gDrag.draggedSite is true-ish for internal dragged page. Let's say: "We're not responding to external drag/drop events when our parent window is in private browsing mode." No need to mention gDrag.draggedSite. @@ +100,5 @@ > handleEvent: function Cell_handleEvent(aEvent) { > + // We are basically going to avoid doing anything when the user is in PB mode > + // or when the draggedSite is external. > + // gDrag.draggedSite is true-ish for internal dragged page. > + if (inPrivateBrowsingMode && !(!!gDrag.draggedSite)) if (inPrivateBrowsingMode && !gDrag.draggedSite) ::: browser/base/content/newtab/grid.js @@ +128,5 @@ > for (let i = 0; i < length; i++) { > if (links[i]) > this.createSite(links[i], cells[i]); > + if (inPrivateBrowsingMode) > + cells[i].site.node.setAttribute("frozen", "true"); Please revert this change. We allow modifications to the grid, not additions. ::: browser/base/content/newtab/newTab.js @@ +28,5 @@ > + .rootTreeItem > + .QueryInterface(Ci.nsIInterfaceRequestor) > + .getInterface(Ci.nsIDOMWindow) > + .wrappedJSObject > + .gPrivateBrowsingUI.privateWindow; We should also check if ("gPrivateBrowsingUI" in domWindow), just to make sure. ::: browser/modules/NewTabUtils.jsm @@ +59,1 @@ > get currentStorage() { We can remove this getter and its usage. Just s/currentStorage/domStorage/ everywhere.

Did you run the tests in browser/base/content/test/newtab/? There should be some in there that should fail :) Please make sure the tests are running.

Comment on attachment 634806 [details] [diff] [review] Patch v4 Review of attachment 634806 [details] [diff] [review]: ----------------------------------------------------------------- Thank you, Saurabh. This looks really good to me. Can you please fix the two nits I mentioned below, upload the updated patch and mark it as checkin-needed? ::: browser/base/content/newtab/newTab.js @@ +32,5 @@ > + > +let inPrivateBrowsingMode = false; > + > +if ("gPrivateBrowsingUI" in chromeWin) > + inPrivateBrowsingMode = chromeWin.gPrivateBrowsingUI.privateWindow; Nit: There's a lot of trailing white space at this line, please remove it. ::: browser/modules/NewTabUtils.jsm @@ +19,1 @@ > XPCOMUtils.defineLazyModuleGetter(this, "Dict", "resource://gre/modules/Dict.jsm"); I forgot about this, please remove it as well. The PrivateBrowsingStorage was the only consumer of it.

Comment on attachment 634817 [details] [diff] [review] Patch v5 As per conversation on IRC, waiting for try push before checking in.

For breaking news(pun intended), check out : https://tbpl.mozilla.org/?tree=Try&rev=81ad018dc43a

Proudly present a lush green tree : https://tbpl.mozilla.org/?tree=Try&rev=81ad018dc43a. Enjoy landing ;)