Keyboard shortcuts should do one thing only, not behave contextually like this. We may need to explicitly not pass a parent window if the desired privacy of the new window and the privacy of the parent don't match.

I'm not quite sure what we should do here. It sucks for the shortcut to change what it does based on the context, but I don't know what the alternative would be. Should we disable the Cmd+N shortcut in private windows? Wouldn't that cause people to wonder why the shortcut doesn't work? Should we just open a non-private window? That seems like a bad idea too. Should we not change anything? I'm interested in UX feedback on this!

I think opening a non-private window is totally reasonable, especially since that's the behaviour of at least Chrome.

I think could be used Ctrl + Shift + P to open a new non-private window.

Alright, let me think a bit more about this to see if I can convince myself.

This is particularly bad because it affects the menu items (ie. File > New Window). If you don't have a public window open, you cannot open a public window.

(In reply to comment #5) > This is particularly bad because it affects the menu items (ie. File > New > Window). If you don't have a public window open, you cannot open a public > window. That is a great point, and a firm argument in favor of comment 2.

OK, comment 5 is really convincing. Plus, private windows will be pretty clear at least initially since they show about:privatebrowsing, and even more so with the upcoming theme changes.

This part implements the necessary API.

Comment on attachment 690281 [details] [diff] [review] Part 2 >- <command id="cmd_newNavigator" oncommand="OpenBrowserWindow()"/> >+ <command id="cmd_newNavigator" oncommand="OpenBrowserWindow({nonPrivate: true})"/> Please make this {private: false} like in bug 817337.

Do we still want to support the OpenBrowserWindow behavior of inheriting the current window's privacy status?

May take on comment 12 is that we probably shouldn't keep that behavior and change the logic to this: #ifdef MOZ_PER_WINDOW_PRIVATE_BROWSING if (typeof options == "object" && options.private) { #else if (gPrivateBrowsingUI.privateBrowsingEnabled) { #endif [..] } else { extraFeatures = ",non-private"; }

No, I think we still want to keep that behavior. That should be what happens by default, and the newNavigator command is an exception to that rule...

What does "by default" mean here? When exactly is that behavior wanted when calling OpenBrowserWindow? OpenBrowserWindow is just the JS equivalent to the XUL command.

(In reply to comment #15) > What does "by default" mean here? When exactly is that behavior wanted when > calling OpenBrowserWindow? OpenBrowserWindow is just the JS equivalent to the > XUL command. By "by default", I meant without explicitly specifying a private property on the options argument. Looking at the callers, there is a call site here <http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#1911> which I think is doing the right thing (inheriting the status of the parent window). There is also a large number of call sites in our tests which have been depending on OpenBrowserWindow inheriting the status of the parent window. And that is also what the platform side of things assumes by default (so that is the behavior that you will get if you use openDialog("chrome://browser/content/") for example.) Of course I can go through all of those callers and audit/fix them all, but this seems rather excessive for the purposes of this bug, and unless you feel very strongly about that, I'd prefer to defer that work to another bug.

(In reply to Ehsan Akhgari [:ehsan] from comment #16) > (In reply to comment #15) > > What does "by default" mean here? When exactly is that behavior wanted when > > calling OpenBrowserWindow? OpenBrowserWindow is just the JS equivalent to the > > XUL command. > > By "by default", I meant without explicitly specifying a private property on > the options argument. I knew that. I was asking for the use case. > Looking at the callers, there is a call site here > <http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser. > js#1911> which I think is doing the right thing (inheriting the status of > the parent window). We want the home page there, not about:privatebrowsing, so as far as I can tell it's not doing the right thing. > There is also a large number of call sites in our tests > which have been depending on OpenBrowserWindow inheriting the status of the > parent window. I expected that it wouldn't make a difference for most of our tests. Are the tests you're talking about private browsing tests? Can you bulk-replace OpenBrowserWindow() with OpenBrowserWindow({private: PrivateBrowsingUtils.isWindowPrivate(window)}) there? > And that is also what the platform side of things assumes by > default (so that is the behavior that you will get if you use > openDialog("chrome://browser/content/") for example.) Right, I'm not talking about openDialog, which is a completely different beast. I'm talking about OpenBrowserWindow as the JS counterpart of cmd_newNavigator, which aligns very well with what this bug is about.

(In reply to comment #17) > (In reply to Ehsan Akhgari [:ehsan] from comment #16) > > (In reply to comment #15) > > > What does "by default" mean here? When exactly is that behavior wanted when > > > calling OpenBrowserWindow? OpenBrowserWindow is just the JS equivalent to the > > > XUL command. > > > > By "by default", I meant without explicitly specifying a private property on > > the options argument. > > I knew that. I was asking for the use case. The home page toolbar button is an example of the use case. > > Looking at the callers, there is a call site here > > <http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser. > > js#1911> which I think is doing the right thing (inheriting the status of > > the parent window). > > We want the home page there, not about:privatebrowsing, so as far as I can tell > it's not doing the right thing. Hmm, shift+clicking on the home button in a private window correctly opens about:home in a new private window. > > There is also a large number of call sites in our tests > > which have been depending on OpenBrowserWindow inheriting the status of the > > parent window. > > I expected that it wouldn't make a difference for most of our tests. Are the > tests you're talking about private browsing tests? Can you bulk-replace > OpenBrowserWindow() with OpenBrowserWindow({private: > PrivateBrowsingUtils.isWindowPrivate(window)}) there? I don't think that I can just bulk-replace that... At the very least those call sites should be audited to make sure that we're not changing what the tests expect. Also, wouldn't such bulk-replacement make it pointless to accept the private property to not be set at all? > > And that is also what the platform side of things assumes by > > default (so that is the behavior that you will get if you use > > openDialog("chrome://browser/content/") for example.) > > Right, I'm not talking about openDialog, which is a completely different beast. > I'm talking about OpenBrowserWindow as the JS counterpart of cmd_newNavigator, > which aligns very well with what this bug is about. I can definitely go ahead and audit all of the callers and fix them up to pass an explicit private argument to OpenBrowserWindow, if you want me to. I just want to avoid doing that in this bug if you don't feel strongly about this. :-) Please let me know... Thanks!

Comment on attachment 690280 [details] [diff] [review] Part 1 The docs in the IDL need to be ... more extensive. In particular, the interaction of the two flags needs documenting. r=me with that.

(In reply to Ehsan Akhgari [:ehsan] from comment #18) > (In reply to comment #17) > > (In reply to Ehsan Akhgari [:ehsan] from comment #16) > > > By "by default", I meant without explicitly specifying a private property on > > > the options argument. > > > > I knew that. I was asking for the use case. > > The home page toolbar button is an example of the use case. Is it? The keyboard short cut this bug was filed for opens the home page in a new window; it seems that shift+clicking the home button is just a different way of doing the same thing. > > We want the home page there, not about:privatebrowsing, so as far as I can tell > > it's not doing the right thing. > > Hmm, shift+clicking on the home button in a private window correctly opens > about:home in a new private window. I see, OpenBrowserWindow implements different behavior depending on whether the privacy status is specified or inherited. That's kind of confusing and would be simplified by what I proposed in comment 13. > > I expected that it wouldn't make a difference for most of our tests. Are the > > tests you're talking about private browsing tests? Can you bulk-replace > > OpenBrowserWindow() with OpenBrowserWindow({private: > > PrivateBrowsingUtils.isWindowPrivate(window)}) there? > > I don't think that I can just bulk-replace that... At the very least those > call sites should be audited to make sure that we're not changing what the > tests expect. OpenBrowserWindow() with inheritance should have exactly the same effect as OpenBrowserWindow({private: PrivateBrowsingUtils.isWindowPrivate(window)}), right? > Also, wouldn't such bulk-replacement make it pointless to accept the private > property to not be set at all? No, I actually don't care about these tests, I was merely proposing a way to get them out of the way. What I care about is our and add-ons' production code calling OpenBrowserWindow. > I can definitely go ahead and audit all of the callers and fix them up to > pass an explicit private argument to OpenBrowserWindow, if you want me to. > I just want to avoid doing that in this bug if you don't feel strongly about > this. :-) Please let me know... Since adjusting the API would affect every single line of browser code you're touching in part 2 (i.e. it's no incremental change) and this bug doesn't seem critical to per-window PB being tested in nightlies, I'd like to get the API settled here.

(In reply to comment #20) > (In reply to Ehsan Akhgari [:ehsan] from comment #18) > > (In reply to comment #17) > > > (In reply to Ehsan Akhgari [:ehsan] from comment #16) > > > > By "by default", I meant without explicitly specifying a private property on > > > > the options argument. > > > > > > I knew that. I was asking for the use case. > > > > The home page toolbar button is an example of the use case. > > Is it? The keyboard short cut this bug was filed for opens the home page in a > new window; it seems that shift+clicking the home button is just a different > way of doing the same thing. Oh, hmm. I never thought of shift+clicking the home button that way, to be honest... But I see your point. > > > We want the home page there, not about:privatebrowsing, so as far as I can tell > > > it's not doing the right thing. > > > > Hmm, shift+clicking on the home button in a private window correctly opens > > about:home in a new private window. > > I see, OpenBrowserWindow implements different behavior depending on whether the > privacy status is specified or inherited. That's kind of confusing and would be > simplified by what I proposed in comment 13. OK. > > > I expected that it wouldn't make a difference for most of our tests. Are the > > > tests you're talking about private browsing tests? Can you bulk-replace > > > OpenBrowserWindow() with OpenBrowserWindow({private: > > > PrivateBrowsingUtils.isWindowPrivate(window)}) there? > > > > I don't think that I can just bulk-replace that... At the very least those > > call sites should be audited to make sure that we're not changing what the > > tests expect. > > OpenBrowserWindow() with inheritance should have exactly the same effect as > OpenBrowserWindow({private: PrivateBrowsingUtils.isWindowPrivate(window)}), > right? Yes, you're right. > > Also, wouldn't such bulk-replacement make it pointless to accept the private > > property to not be set at all? > > No, I actually don't care about these tests, I was merely proposing a way to > get them out of the way. What I care about is our and add-ons' production code > calling OpenBrowserWindow. Right. > > I can definitely go ahead and audit all of the callers and fix them up to > > pass an explicit private argument to OpenBrowserWindow, if you want me to. > > I just want to avoid doing that in this bug if you don't feel strongly about > > this. :-) Please let me know... > > Since adjusting the API would affect every single line of browser code you're > touching in part 2 (i.e. it's no incremental change) and this bug doesn't seem > critical to per-window PB being tested in nightlies, I'd like to get the API > settled here. OK, I'll do that here then.

Comment on attachment 690280 [details] [diff] [review] Part 1 https://hg.mozilla.org/integration/mozilla-inbound/rev/988ef008a1d7

I checked all callers; none were broken, but I made a few explicit for clarity.

Whoops, forgot to qref.

Argh. Patch queues are hard.

Comment on attachment 692059 [details] [diff] [review] Simplify the semantics of OpenBrowserWindow: new windows are public unless explicitly passed a 'private' option or global private browsing is in effect. >@@ -3487,27 +3487,26 @@ function OpenBrowserWindow(options) > > var charsetArg = new String(); > var handler = Components.classes["@mozilla.org/browser/clh;1"] > .getService(Components.interfaces.nsIBrowserHandler); > var defaultArgs = handler.defaultArgs; > var wintype = document.documentElement.getAttribute('windowtype'); > > var extraFeatures = ""; >- var forcePrivate = false; > #ifdef MOZ_PER_WINDOW_PRIVATE_BROWSING >- forcePrivate = typeof options == "object" && "private" in options && options.private; >+ if (typeof options == "object" && "private" in options && options.private) { > #else remove ' && "private" in options' > <commandset id="mainCommandSet"> >- <command id="cmd_newNavigator" oncommand="OpenBrowserWindow()"/> >+ <command id="cmd_newNavigator" oncommand="OpenBrowserWindow({private: false})"/> > case "window": >- OpenBrowserWindow(); >+ OpenBrowserWindow({private: false}); > break; >- <menuitem label="&newNavigatorCmd.label;" oncommand="OpenBrowserWindowFromDockMenu();" /> >+ <menuitem label="&newNavigatorCmd.label;" oncommand="OpenBrowserWindowFromDockMenu({private: false});" /> I don't see much value in these changes and would rather not add this extra layer for hg blame. >+ waitForFocus(function() { >+ nonPrivateWin = privateWin.OpenBrowserWindow({private: false}); >+ ok(!PrivateBrowsingUtils.isWindowPrivate(nonPrivateWin), "privateWin.OpenBrowserWindow({private: false}) should open a normal window"); >+ nonPrivateWin.close(); >+ privateWin.close(); >+ finish(); >+ }, privateWin); I don't see the point of waiting for focus here. You probably want this: whenDelayedStartupFinished(privateWin, function () {...});

(In reply to Josh Matthews [:jdm] from comment #28) > https://hg.mozilla.org/integration/mozilla-inbound/rev/7878092679cf Please can you backout and reland with a corrected commit message :-)

(Happy for you to do so in the same push & use DONTBUILD)

(And filed bug 821553 to fix the hook to ensure the bug number has to be in the first line of the commit message)

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/0a05fb0f9add remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/032490690d65 Relanded with better commit message.

Thank you :-)

Verified (by the window title) fixed with 2013-01-04-03-08-23-mozilla-central-firefox-20.0a1.en-US.linux-x86_64