It's time that we make per-window private browsing builds do something useful. I have a set of patches which let you open up a private browsing window. Not everything works right now, but that's ok, since this will not be part of the default build. This will be part of bug 729865.

Comment on attachment 669002 [details] [diff] [review] Part 1 >--- a/browser/base/content/browser-menubar.inc >+++ b/browser/base/content/browser-menubar.inc >@@ -22,6 +22,12 @@ > accesskey="&newNavigatorCmd.accesskey;" > key="key_newNavigator" > command="cmd_newNavigator"/> >+#ifdef MOZ_PER_WINDOW_PRIVATE_BROWSING >+ <menuitem id="menu_newPrivateWindow" >+ label="&newPrivateWindow.label;" >+ command="Tools:PrivateBrowsing" >+ key="key_privatebrowsing"/> >+#endif Needs an access key.

Comment on attachment 669003 [details] [diff] [review] Part 2 >+#ifdef MOZ_PER_WINDOW_PRIVATE_BROWSING >+ >+// We define a new gPrivateBrowsingUI object, as the per-window PB implementation >+// is completely different to the global PB one. Specifically, the per-window >+// PB implementation does not expose many APIs on the gPrivateBrowsingUI object, >+// and only uses it as a way to initialize and uninitialize private browsing >+// windows. While we could use #ifdefs all around the global PB mode code to >+// make it work in both modes, the amount of duplicated code is small and the >+// code is much more readable this way. Use # rather than // here. >+ init: function PBUI_init() { >+ // Do nothing for normal windows >+ if (!PrivateBrowsingUtils.isWindowPrivate(window)) { >+ return; >+ } >+ >+ // Disable the Clear Recent History... menu item when in PB mode >+ // temporary fix until bug 463607 is fixed >+ document.getElementById("Tools:Sanitize").setAttribute("disabled", "true"); >+ >+ // Adjust the window's title >+ let docElement = document.documentElement; >+ docElement.setAttribute("title", >+ docElement.getAttribute("title_privatebrowsing")); >+ docElement.setAttribute("titlemodifier", >+ docElement.getAttribute("titlemodifier_privatebrowsing")); >+ docElement.setAttribute("privatebrowsingmode", "temporary"); >+ gBrowser.updateTitlebar(); On OS X, this code runs in windows that don't have gBrowser. PBUI_onEnterPrivateBrowsing takes care of this by checking window.location.href == getBrowserURL(). >+ addInitializationCallback: function PBUI_addInitializationCallback(aCallback) { It looks like this won't be needed if you use PrivateBrowsingUtils.isWindowPrivate(window) in utilityOverlay.js.

(In reply to Dão Gottwald [:dao] from comment #5) > >+ addInitializationCallback: function PBUI_addInitializationCallback(aCallback) { > > It looks like this won't be needed if you use > PrivateBrowsingUtils.isWindowPrivate(window) in utilityOverlay.js. That code also reads the autoStarted property, and in the global PB case, that depends on gPrivateBrowsingUI to be initialized. And I just landed some patches in other bugs which replace the privateBrowsingEnabled usage with PrivateBrowsingUtils. I'd rather keep the initialization callback logic around until we can remove the global PB mode code, at which point the code in utilityOverlay.js can be simplified and we can remove the initialization callback stuff from gPrivateBrowsingUI as well.

With the access key added.

(In reply to Ehsan Akhgari [:ehsan] from comment #6) > (In reply to Dão Gottwald [:dao] from comment #5) > > >+ addInitializationCallback: function PBUI_addInitializationCallback(aCallback) { > > > > It looks like this won't be needed if you use > > PrivateBrowsingUtils.isWindowPrivate(window) in utilityOverlay.js. > > That code also reads the autoStarted property, and in the global PB case, > that depends on gPrivateBrowsingUI to be initialized. And I just landed > some patches in other bugs which replace the privateBrowsingEnabled usage > with PrivateBrowsingUtils. It doesn't seem to make much sense to keep autoStarted on the gPrivateBrowsingUI object, as the former is window-independent. You could of course keep it for backwards compatibility, but utilityOverlay.js should just get it from PrivateBrowsingUtils.

Comment on attachment 669156 [details] [diff] [review] Part 1 >--- a/browser/base/content/browser-appmenu.inc >+++ b/browser/base/content/browser-appmenu.inc >@@ -27,6 +27,13 @@ > label="&newNavigatorCmd.label;" > command="cmd_newNavigator" > key="key_newNavigator"/> >+#ifdef MOZ_PER_WINDOW_PRIVATE_BROWSING >+ <menuitem id="appmenu_newPrivateWindow" >+ label="&newPrivateWindow.label;" >+ accesskey="&newPrivateWindow.accesskey;" Items in this menu don't get access keys. (We have a test to verify this.) >+<!ENTITY newPrivateWindow.label "New Private Browsing Window"> Can you get ui-review for this? "New Private Window" might be preferable.

(In reply to Dão Gottwald [:dao] from comment #9) > (In reply to Ehsan Akhgari [:ehsan] from comment #6) > > (In reply to Dão Gottwald [:dao] from comment #5) > > > >+ addInitializationCallback: function PBUI_addInitializationCallback(aCallback) { > > > > > > It looks like this won't be needed if you use > > > PrivateBrowsingUtils.isWindowPrivate(window) in utilityOverlay.js. > > > > That code also reads the autoStarted property, and in the global PB case, > > that depends on gPrivateBrowsingUI to be initialized. And I just landed > > some patches in other bugs which replace the privateBrowsingEnabled usage > > with PrivateBrowsingUtils. > > It doesn't seem to make much sense to keep autoStarted on the > gPrivateBrowsingUI object, as the former is window-independent. You could of > course keep it for backwards compatibility, but utilityOverlay.js should > just get it from PrivateBrowsingUtils. Agreed. As I said, I will make this change in the future.

(In reply to Dão Gottwald [:dao] from comment #10) > Comment on attachment 669156 [details] [diff] [review] > Part 1 > > >--- a/browser/base/content/browser-appmenu.inc > >+++ b/browser/base/content/browser-appmenu.inc > >@@ -27,6 +27,13 @@ > > label="&newNavigatorCmd.label;" > > command="cmd_newNavigator" > > key="key_newNavigator"/> > >+#ifdef MOZ_PER_WINDOW_PRIVATE_BROWSING > >+ <menuitem id="appmenu_newPrivateWindow" > >+ label="&newPrivateWindow.label;" > >+ accesskey="&newPrivateWindow.accesskey;" > > Items in this menu don't get access keys. (We have a test to verify this.) Oh ok, didn't know that! > >+<!ENTITY newPrivateWindow.label "New Private Browsing Window"> > > Can you get ui-review for this? "New Private Window" might be preferable. I will, and I'll submit a new version when the ui-review is complete.

Comment on attachment 669156 [details] [diff] [review] Part 1 Not sure who needs to ui-r this. Please note that this UI will be disabled for now, and we can iterate on it later on and fix things before enabling it by default.

(In reply to Ehsan Akhgari [:ehsan] from comment #11) > > It doesn't seem to make much sense to keep autoStarted on the > > gPrivateBrowsingUI object, as the former is window-independent. You could of > > course keep it for backwards compatibility, but utilityOverlay.js should > > just get it from PrivateBrowsingUtils. > > Agreed. As I said, I will make this change in the future. You said you'd make it dependent on the complete removal of global PB mode code. I don't see the connection. Why add this code now and drag it around till then? (In reply to Ehsan Akhgari [:ehsan] from comment #13) > Comment on attachment 669156 [details] [diff] [review] > Part 1 > > Not sure who needs to ui-r this. Please note that this UI will be disabled > for now, and we can iterate on it later on and fix things before enabling it > by default. Each string change generates work for a large group of people. Please be deliberate here.

(In reply to comment #14) > (In reply to Ehsan Akhgari [:ehsan] from comment #11) > > > It doesn't seem to make much sense to keep autoStarted on the > > > gPrivateBrowsingUI object, as the former is window-independent. You could of > > > course keep it for backwards compatibility, but utilityOverlay.js should > > > just get it from PrivateBrowsingUtils. > > > > Agreed. As I said, I will make this change in the future. > > You said you'd make it dependent on the complete removal of global PB mode > code. I don't see the connection. Why add this code now and drag it around till > then? No, sorry. I meant making it dependent on MOZ_PER_WINDOW_PRIVATE_BROWSING. Our plan is to flip that flag when we're ready to ship per-window PB, and remove all of the code hidden behind it some time after when we're sure that per-window PB is indeed ship quality. Sorry for the confusion, we're on the same page here. :-) > (In reply to Ehsan Akhgari [:ehsan] from comment #13) > > Comment on attachment 669156 [details] [diff] [review] > > Part 1 > > > > Not sure who needs to ui-r this. Please note that this UI will be disabled > > for now, and we can iterate on it later on and fix things before enabling it > > by default. > > Each string change generates work for a large group of people. Please be > deliberate here. Agreed.

Comment on attachment 669157 [details] [diff] [review] Part 2 >+ this._inited = true; >+ >+ this._initCallbacks.forEach(function (callback) callback.apply()); >+ this._initCallbacks = []; >+ }, >+ >+ get autoStarted() { >+ return false; // auto-started PB not supported for now >+ }, >+ >+ get initialized() { >+ return this._inited; >+ }, >+ >+ addInitializationCallback: function PBUI_addInitializationCallback(aCallback) { >+ if (this._inited) >+ return; >+ >+ this._initCallbacks.push(aCallback); >+ } So this code is being removed in bug 799780. Not sure why we needed a separate bug for this, but whatever...

Hey - UI-r+ed, but with a preference for "New Private Window" rather than "New Private Browsing Window."

Cool, thanks. Switched the string to New Private Window.

Comment on attachment 670028 [details] [diff] [review] Part 1 >+<!ENTITY newPrivateWindow.label "New Private Window"> >+<!ENTITY newPrivateWindow.accesskey "B"> needs a different access key

Switched the accesskey to W, and switched Work Offline's accesskey to k.

Comment on attachment 670033 [details] [diff] [review] Part 1 > <menuitem id="goOfflineMenuitem" > label="&goOfflineCmd.label;" >- accesskey="&goOfflineCmd.accesskey;" >+ accesskey="&goOfflineCmd2.accesskey;" >-<!ENTITY goOfflineCmd.accesskey "w"> >+<!ENTITY goOfflineCmd2.accesskey "k"> Keep goOfflineCmd.accesskey as the entity name since this change is en-US-specific.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a22d34ae87dc https://hg.mozilla.org/integration/mozilla-inbound/rev/cdb3a2b88e2d https://hg.mozilla.org/integration/mozilla-inbound/rev/7d629baabca7

https://hg.mozilla.org/mozilla-central/rev/a22d34ae87dc https://hg.mozilla.org/mozilla-central/rev/cdb3a2b88e2d https://hg.mozilla.org/mozilla-central/rev/7d629baabca7