The global state is going away. If possible, docshell queries should be used instead, or we may need to add a window-level PB status.

We should just be able to duplicate the code from _getBrowserWindowForContentWindow (http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#6426) and check gPrivateBrowsingUI.privateWindow on the result instead of the existing global state check.

Comment on attachment 633915 [details] [diff] [review] Patch v1 Review of attachment 633915 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the below comment. Please submit a patch with this comment addressed for check-in. ::: toolkit/content/globalOverlay.js @@ +23,5 @@ > + .QueryInterface(Ci.nsIDocShellTreeItem) > + .rootTreeItem > + .QueryInterface(Ci.nsIInterfaceRequestor) > + .getInterface(Ci.nsIDOMWindow) > + .wrappedJSObject; Please fix the spacing here, like this: ... = window.QueryInterface(... .getInterface(... ...

Comment on attachment 634178 [details] [diff] [review] Patch v2 Thanks a lot for your patch! https://hg.mozilla.org/integration/mozilla-inbound/rev/fe2fdd5d3aae

Comment on attachment 634178 [details] [diff] [review] Patch v2 Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/19bfe36cace8 - varying numbers of mochitest-chrome failures, starting with a timeout in test_close_download_manager.xul, a la https://tbpl.mozilla.org/php/getParsedLog.php?id=12778051&tree=Mozilla-Inbound.

Sorry Philor, for my patch causing troubles. I'll look into the reasons as soon as possible.

I asked Ed Morley on IRC and he said that most probably this regression is not caused by me. But I'm still looking.

Have you submitted this patch to the try server?

Comment on attachment 634178 [details] [diff] [review] Patch v2 This patch is completely bogus. gPrivateBrowsingUI only exists in browser windows, not in any other window using globalOverlay.js. I just saw this in the pushlog and was going to back it out. Happy to see this already happened thanks to the test suite catching this.

Dão, would other windows have parent browser windows we could check instead?

They might have an opener window, but 1) none of them cares whether the opener window was in private browsing mode, so I don't see how it would be a useful check, and 2) the opener window would be gone in this case, since inPrivateBrowsing only comes into play here if windowCount == 1. I'm not sure why this code takes private browsing into account, but it might be the case that it should just stop doing this given the move to private windows instead of a global private browsing mode.

globalOverlay.js is used by more than just Firefox. Some consumers may not even have the concept of a "browser" window.

(In reply to Dão Gottwald [:dao] from comment #12) > They might have an opener window, but 1) none of them cares whether the > opener window was in private browsing mode, so I don't see how it would be a > useful check, and 2) the opener window would be gone in this case, since > inPrivateBrowsing only comes into play here if windowCount == 1. > > I'm not sure why this code takes private browsing into account, but it might > be the case that it should just stop doing this given the move to private > windows instead of a global private browsing mode. Bug 502307 would still be a concern, right? I think we should just check for the existence of gPrivateBrowsingUI as well.

(In reply to Ehsan Akhgari [:ehsan] from comment #14) > (In reply to Dão Gottwald [:dao] from comment #12) > > They might have an opener window, but 1) none of them cares whether the > > opener window was in private browsing mode, so I don't see how it would be a > > useful check, and 2) the opener window would be gone in this case, since > > inPrivateBrowsing only comes into play here if windowCount == 1. > > > > I'm not sure why this code takes private browsing into account, but it might > > be the case that it should just stop doing this given the move to private > > windows instead of a global private browsing mode. > > Bug 502307 would still be a concern, right? Could you elaborate? That bug claims that "Firefox should warn you when closing multiple tabs regardless of whether or not the private browsing feature is in use", so my naive thinking would be that the code responsible for this shouldn't care about private browsing.

Hmm, after re-reading bug 502307, I'm pretty confused. I don't remember why the original patch there was needed, and my second one didn't stick. So perhaps someone needs to take this whole code out and then test whether bug 502307 comment 0 still applies. :-) Saurabh, do you want to do that?

I want to do anything to get PBnGen landed ;) I will get down to doing this soon.

@Ehsan, can you elaborate what you want me to test. Do you want me test comment 0 of bug 502307 after importing the patch in that bug, or without patch queues applied. I tested in my nightly without having applied any patches, neither this one nor the patch submitted in Bug 502307 and that Comment still seems to apply. I'm not clear how to proceed with this bug. Can someone please clarify it to me.

yes, I meant testing bug 502307 comment 0. Thanks for doing that! I did a bit more digging around, and I think I know more now. This dialog should be shown using this code: <http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js#529> which is being skipped by the check on line 499. This patch <http://hg.mozilla.org/mozilla-central/rev/89bf34c42f88> was landed to mitigate that problem. Now, I think a proper fix would be the following: 1. Revert http://hg.mozilla.org/mozilla-central/rev/89bf34c42f88. 2. Move this block <http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js#495> down to the |if (!showPrompt)| check. 3. The code block in step 2 is using the global PB mode, but that is bug 722984, which has a patch which needs a few more comments addressed. That patch should be rebased on top of the patch in this one. If you test a build with steps 1 and 2 above, bug 502307 comment 0 should not happen any longer.

AFAIK, there is no need for the check in http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js#495 since we must show the warning irrespective of the fact whether the browser is in PB mode or not. Please correct me if I am wrong. @Ehsan, why do you want me to move the block to before the |if (!showPrompt)| check, because then again if the user is in PB mode then the browser will not warn, even though |showPrompt| might have been true.

I think http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js#462 contradicts what Bug 502307 wants.

For that rebasing, do I need to make the same changes that are being made in bug 722984 in this patch also, because then the same changes will be in two patches and while merging the patches might collide (aka bitrotten). Or will this rebasing be done while landing this patch into the tree ? I made the changes in Step 1 and 2 and bug 502307 comment 0 is not happening anymore. But still I don't have my doubts from my previous comments.

(In reply to Saurabh Anand [:sawrubh] from comment #20) > AFAIK, there is no need for the check in > http://mxr.mozilla.org/mozilla-central/source/browser/components/ > nsBrowserGlue.js#495 since we must show the warning irrespective of the fact > whether the browser is in PB mode or not. Please correct me if I am wrong. By default, the quit prompt offers the user the option to save their session. This is not very meaningful in PB, since we explicitly will not save the users's session in PB mode. This is why this block of code was added in bug 468565. > @Ehsan, why do you want me to move the block to before the |if > (!showPrompt)| check, because then again if the user is in PB mode then the > browser will not warn, even though |showPrompt| might have been true. Because that function also has the task of warning the user if they're closing multiple tabs when they have the warn_on_close_multiple_tabs pref set, and the fact that we bail out early of that function is the root cause of bug 502307, I think. (In reply to Saurabh Anand [:sawrubh] from comment #22) > For that rebasing, do I need to make the same changes that are being made in > bug 722984 in this patch also, because then the same changes will be in two > patches and while merging the patches might collide (aka bitrotten). Or will > this rebasing be done while landing this patch into the tree ? No, you can leave that part to bug 722984 to make the review easier. It will make rebasing on that bug a little bit harder, but the patch there shouldn't be too big. > I made the changes in Step 1 and 2 and bug 502307 comment 0 is not happening > anymore. But still I don't have my doubts from my previous comments. I think that is the desired fix for bug 502307 then! :-)

This patch includes : a)Reverts the revisions as pointed by Ehsan. b)Moves the block of code to after the if-else loop and before the |if(!showPrompt)| check. This does not deliberately remove the current code to access private browsing service and make it access the window's PB flag since that is planned to be done in Bug 722984.

I'll push to try after I get a feedback+ from Ehsan, and I promise Philor, won't ask for review until get a green tree this time ;)

Comment on attachment 637148 [details] [diff] [review] Patch v3 Review of attachment 637148 [details] [diff] [review]: ----------------------------------------------------------------- Please mark for checkin when you get green on try.

Comment on attachment 637148 [details] [diff] [review] Patch v3 The tree looks pretty green to me. If you feel it's ok, then please checkin this patch.