Closed
Bug 722994
Opened 13 years ago
Closed 12 years ago
globalOverlay.js uses global Private Browsing state to control display of a prompt
Categories
(Toolkit :: General, defect)
Toolkit
General
Tracking
()
RESOLVED
FIXED
mozilla16
People
(Reporter: jdm, Assigned: sawrubh)
References
()
Details
(Whiteboard: [mentor=jdm][lang=js])
Attachments
(1 file, 2 obsolete files)
2.14 KB,
patch
|
ehsan.akhgari
:
review+
ehsan.akhgari
:
checkin+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•13 years ago
|
||
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.
Reporter | ||
Updated•13 years ago
|
Whiteboard: [mentor=jdm][lang=js]
Assignee | ||
Comment 2•12 years ago
|
||
Assignee: nobody → saurabhanandiit
Status: NEW → ASSIGNED
Attachment #633915 -
Flags: feedback?(ehsan)
Comment 3•12 years ago
|
||
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(...
...
Attachment #633915 -
Flags: feedback?(ehsan) → review+
Assignee | ||
Comment 4•12 years ago
|
||
Attachment #633915 -
Attachment is obsolete: true
Attachment #634178 -
Flags: checkin?(ehsan)
Updated•12 years ago
|
Target Milestone: --- → mozilla16
Comment 5•12 years ago
|
||
Comment on attachment 634178 [details] [diff] [review]
Patch v2
Thanks a lot for your patch!
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe2fdd5d3aae
Attachment #634178 -
Flags: checkin?(ehsan) → checkin+
Comment 6•12 years ago
|
||
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.
Attachment #634178 -
Flags: checkin+ → checkin-
Assignee | ||
Comment 7•12 years ago
|
||
Sorry Philor, for my patch causing troubles. I'll look into the reasons as soon as possible.
Assignee | ||
Comment 8•12 years ago
|
||
I asked Ed Morley on IRC and he said that most probably this regression is not caused by me. But I'm still looking.
Comment 9•12 years ago
|
||
Have you submitted this patch to the try server?
Comment 10•12 years ago
|
||
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.
Attachment #634178 -
Flags: review-
Reporter | ||
Comment 11•12 years ago
|
||
Dão, would other windows have parent browser windows we could check instead?
Comment 12•12 years ago
|
||
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.
Comment 13•12 years ago
|
||
globalOverlay.js is used by more than just Firefox. Some consumers may not even have the concept of a "browser" window.
Comment 14•12 years ago
|
||
(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.
Comment 15•12 years ago
|
||
(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.
Comment 16•12 years ago
|
||
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?
Assignee | ||
Comment 17•12 years ago
|
||
I want to do anything to get PBnGen landed ;) I will get down to doing this soon.
Assignee | ||
Comment 18•12 years ago
|
||
@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.
Comment 19•12 years ago
|
||
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.
Assignee | ||
Comment 20•12 years ago
|
||
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.
Assignee | ||
Comment 21•12 years ago
|
||
I think http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js#462 contradicts what Bug 502307 wants.
Assignee | ||
Comment 22•12 years ago
|
||
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.
Comment 23•12 years ago
|
||
(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! :-)
Assignee | ||
Comment 24•12 years ago
|
||
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.
Attachment #634178 -
Attachment is obsolete: true
Attachment #637148 -
Flags: feedback?(ehsan)
Assignee | ||
Comment 25•12 years ago
|
||
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 26•12 years ago
|
||
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.
Attachment #637148 -
Flags: feedback?(ehsan) → review+
Assignee | ||
Comment 27•12 years ago
|
||
Assignee | ||
Comment 28•12 years ago
|
||
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.
Attachment #637148 -
Flags: checkin?(ehsan)
Comment 29•12 years ago
|
||
Updated•12 years ago
|
Attachment #637148 -
Flags: checkin?(ehsan) → checkin+
Comment 30•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•