Last Comment Bug 722984 - nsBrowserGlue uses global private browsing service to make decisions
: nsBrowserGlue uses global private browsing service to make decisions
Status: RESOLVED FIXED
[mentor=jdm][lang=js]
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 16
Assigned To: Saurabh Anand [:sawrubh]
:
Mentors:
http://mxr.mozilla.org/mozilla-centra...
Depends on: 722840
Blocks: PBnGen
  Show dependency treegraph
 
Reported: 2012-01-31 22:20 PST by Josh Matthews [:jdm] (away until 9/3)
Modified: 2012-06-29 09:26 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP(Patch v1) (3.87 KB, patch)
2012-06-15 16:06 PDT, Saurabh Anand [:sawrubh]
no flags Details | Diff | Splinter Review
Patch v2 (2.63 KB, patch)
2012-06-27 10:01 PDT, Saurabh Anand [:sawrubh]
ehsan: feedback-
Details | Diff | Splinter Review
Patch v3 (2.67 KB, patch)
2012-06-28 10:33 PDT, Saurabh Anand [:sawrubh]
ehsan: review+
Details | Diff | Splinter Review
Patch v4 (2.71 KB, patch)
2012-06-28 14:25 PDT, Saurabh Anand [:sawrubh]
ehsan: checkin+
Details | Diff | Splinter Review

Description Josh Matthews [:jdm] (away until 9/3) 2012-01-31 22:20:51 PST
The service is going away, and it's used in _onQuitRequest to decide whether to show a prompt. We'll probably want to change the logic to check all docshells and possibly show a prompt if there exists a non-PB one.
Comment 1 Josh Matthews [:jdm] (away until 9/3) 2012-01-31 22:21:36 PST
Also, the ContentPermissionPrompt class makes decisions based on the global state. That should also be hooked up to a docshell instead.
Comment 2 Josh Matthews [:jdm] (away until 9/3) 2012-05-25 09:09:38 PDT
The check in _onQuitRequest should probably be replaced by a loop like the one in bug 729867 that checks whether all windows are private ones. The check in ContentPermissionPrompt can be replaced by chromeWin.gPrivateBrowsingUI.privateWindow, as long as the definition of chromeWin is moved up before the check.
Comment 3 Saurabh Anand [:sawrubh] 2012-06-15 16:06:51 PDT
Created attachment 633714 [details] [diff] [review]
WIP(Patch v1)

Please tell me the tests that I need to run for this.
Comment 4 :Ehsan Akhgari 2012-06-18 12:19:15 PDT
Comment on attachment 633714 [details] [diff] [review]
WIP(Patch v1)

Review of attachment 633714 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/nsBrowserGlue.js
@@ +501,5 @@
>  
>      if (!aQuitType)
>        aQuitType = "quit";
> +    
> +    let pbFlagCurrentWindow = getChromeWindowLocal(window).gPrivateBrowsingUI.privateWindow;

There is no |window| variable in a component javascript file.

There's a loop in this function a little bit higher than this location where we iterate over the browser windows.  You can just modify that loop to look at the privateWindow variable of each of them.

Also, note that here we want to show a prompt only if there is a non-private browser window open, so you should maintain a allWindowsPrivate boolean flag instead of otherPBWindowExists.

@@ +523,5 @@
> +      exitingCanceled.data = false;
> +      Services.obs.notifyObservers(exitingCanceled, "private-browsing-last-context-exiting", null);
> +      if (exitingCanceled.data)
> +        return false;
> +    }

Instead of doing all this, you should just return if there is no non-private window, similarly to what the existing code does.

@@ +527,5 @@
> +    }
> +    
> +    if (warnAboutClosingTabs)
> +      // FIXME This has to replaced by an appropriate code to extract gBrowser
> +      return getChromeWindowLocal(window).gBrowser.warnAboutClosingTabs(true);

This shouldn't happen here either.

@@ +1615,4 @@
>                                                     [requestingURI.host], 1);
>  
>        // Don't offer to "always/never share" in PB mode
> +      var chromeWin = getChromeWindow(requestingWindow).wrappedJSObject;

This won't work as you have not moved the definition of requestingWindow...

Also, you shouldn't move them inside the else branch, because then they will be undefined if we take the if branch.  You should move them to before the if/else statement.
Comment 5 :Ehsan Akhgari 2012-06-18 12:21:21 PDT
AFAIK, we don't have tests for the quit prompt case, so you need to test that manually.  For the geolocation part, I'm sure we do have tests somewhere but I can't find any right now.  Maybe dougt knows?
Comment 6 Josh Matthews [:jdm] (away until 9/3) 2012-06-18 12:35:42 PDT
Hum, what's the relation between the code being changed in bug 729867 and here? The results end up looking really similar; is there some way we can end up sharing the checks, Gavin?
Comment 7 :Ehsan Akhgari 2012-06-25 10:39:20 PDT
FWIW bug 722994 comment 19 is relevant here.
Comment 8 Saurabh Anand [:sawrubh] 2012-06-27 10:01:56 PDT
Created attachment 637168 [details] [diff] [review]
Patch v2

I tested manually for the "closing multiple tabs warning" and it was giving me an alert twice, when I had two open tabs.
@Ehsan, should I move the check |if(allWindowPrivate)| to after |if(!showPrompt)| as I did in bug 722994 ? Or is the check dxr.lanedo.com/mozilla-central/toolkit/content/globalOverlay.js.html?string=globalOverlay.js#34 causing some problems ?
Comment 9 :Ehsan Akhgari 2012-06-27 10:10:06 PDT
Comment on attachment 637168 [details] [diff] [review]
Patch v2

Review of attachment 637168 [details] [diff] [review]:
-----------------------------------------------------------------

I think what I wrote below is why you're getting prompted twice.

::: browser/components/nsBrowserGlue.js
@@ +480,5 @@
>      while (browserEnum.hasMoreElements()) {
>        windowcount++;
>  
>        var browser = browserEnum.getNext();
> +      if (!browser.privateWindow)

This check will always be true, since privateWindow is a property of gPrivateBrowsingUI, not the window object itself.
Comment 10 Saurabh Anand [:sawrubh] 2012-06-28 10:33:51 PDT
Created attachment 637581 [details] [diff] [review]
Patch v3

Fixed the issue pointed out by Ehsan.

This patch is now only showing alert once during PB mode, which is the desired behaviour.
Comment 11 Saurabh Anand [:sawrubh] 2012-06-28 10:38:33 PDT
https://tbpl.mozilla.org/?tree=Try&rev=253990c38c7e
Comment 12 :Ehsan Akhgari 2012-06-28 10:46:28 PDT
Is this ready for review?
Comment 13 Saurabh Anand [:sawrubh] 2012-06-28 12:57:19 PDT
Comment on attachment 637581 [details] [diff] [review]
Patch v3

Asking for review since the tree looks green and healthy and it fulfills the desired behaviour afaict.
Comment 14 :Ehsan Akhgari 2012-06-28 13:28:12 PDT
Comment on attachment 637581 [details] [diff] [review]
Patch v3

Review of attachment 637581 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the below.

::: browser/components/nsBrowserGlue.js
@@ +1586,4 @@
>                                                     [requestingURI.host], 1);
>  
>        // Don't offer to "always/never share" in PB mode
> +      if (!chromeWin.gPrivateBrowsingUI.privateWindow) {

You should also test to make sure gPrivateBrowsingUI exists on the chromeWin object.
Comment 15 Saurabh Anand [:sawrubh] 2012-06-28 14:25:35 PDT
Created attachment 637673 [details] [diff] [review]
Patch v4

Addressed the comments.
Comment 16 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-06-28 14:37:51 PDT
Comment on attachment 637581 [details] [diff] [review]
Patch v3

I think it would be much simpler in general if there was a global private browsing utility function that took a content window and returned whether it's "private" using the docshell tree, rather than jumping through hoops in various places to use gPrivateBrowsingUI.

Something like:
PrivateBrowsingUtils.isWindowPrivate(window)

isWindowPrivate: function (arbitraryWindow) {
  return arbitraryWindow.QueryInterface(Ci.nsIInterfaceRequestor)
     .getInterface(Ci.nsIWebNavigation)
     .QueryInterface(Ci.nsIDocShellTreeItem)
     .rootTreeItem.QueryInterface(Ci.nsILoadContext)
     .usePrivateBrowsing;
}
Comment 17 :Ehsan Akhgari 2012-06-28 14:42:34 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #16)
> Comment on attachment 637581 [details] [diff] [review]
> Patch v3
> 
> I think it would be much simpler in general if there was a global private
> browsing utility function that took a content window and returned whether
> it's "private" using the docshell tree, rather than jumping through hoops in
> various places to use gPrivateBrowsingUI.
> 
> Something like:
> PrivateBrowsingUtils.isWindowPrivate(window)
> 
> isWindowPrivate: function (arbitraryWindow) {
>   return arbitraryWindow.QueryInterface(Ci.nsIInterfaceRequestor)
>      .getInterface(Ci.nsIWebNavigation)
>      .QueryInterface(Ci.nsIDocShellTreeItem)
>      .rootTreeItem.QueryInterface(Ci.nsILoadContext)
>      .usePrivateBrowsing;
> }

Good idea, filed bug 769467 for that.
Comment 19 Ed Morley [:emorley] 2012-06-29 00:46:42 PDT
https://hg.mozilla.org/mozilla-central/rev/51175fc0de28

Note You need to log in before you can comment on or make changes to this bug.