Last Comment Bug 763468 - Use about:privatebrowsing for new tabs opened in private browsing mode
: Use about:privatebrowsing for new tabs opened in private browsing mode
Status: RESOLVED FIXED
[fixed-in-fx-team]
:
Product: Firefox
Classification: Client Software
Component: Tabbed Browser (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 16
Assigned To: Saurabh Anand [:sawrubh]
:
Mentors:
Depends on: 767835 767836 936417
Blocks: 722990 762938
  Show dependency treegraph
 
Reported: 2012-06-11 06:41 PDT by Tim Taubert [:ttaubert]
Modified: 2013-11-08 04:02 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (1.24 KB, patch)
2012-06-17 09:12 PDT, Saurabh Anand [:sawrubh]
no flags Details | Diff | Review
Patch v2 (1.30 KB, patch)
2012-06-18 15:02 PDT, Saurabh Anand [:sawrubh]
ttaubert: review+
Details | Diff | Review
Patch v3 (1.30 KB, patch)
2012-06-20 03:25 PDT, Saurabh Anand [:sawrubh]
saurabhanandiit: review+
Details | Diff | Review
Test (3.20 KB, patch)
2012-06-20 13:58 PDT, Saurabh Anand [:sawrubh]
ttaubert: feedback+
Details | Diff | Review
Test v1 (3.67 KB, patch)
2012-06-20 14:44 PDT, Saurabh Anand [:sawrubh]
ttaubert: review+
Details | Diff | Review
Patch v4 (2.45 KB, patch)
2012-06-22 01:15 PDT, Saurabh Anand [:sawrubh]
ttaubert: review+
Details | Diff | Review
Patch v5 (2.32 KB, patch)
2012-06-22 05:38 PDT, Saurabh Anand [:sawrubh]
saurabhanandiit: review+
Details | Diff | Review

Description Tim Taubert [:ttaubert] 2012-06-11 06:41:05 PDT
As per bug 762938 comment #13 we want to show about:privatebrowsing for new tabs opened while in private browsing mode.

Saurabh said he wants to work on it, assigning to him.
Comment 1 Saurabh Anand [:sawrubh] 2012-06-17 09:12:55 PDT
Created attachment 633903 [details] [diff] [review]
Patch v1

@Tim, Could you suggest some tests that I need to run for this fix.
Comment 2 Tim Taubert [:ttaubert] 2012-06-17 09:28:48 PDT
Comment on attachment 633903 [details] [diff] [review]
Patch v1

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

Thanks for your patch, Saurabh! That's the right place for this but we need to add an observer that calls update() when entering or leaving private browsing mode so that the value is updated. There are no existing tests that would cover this behavior, we would need to add a new one.

::: browser/base/content/utilityOverlay.js
@@ +16,5 @@
> +                          .QueryInterface(Ci.nsIDocShellTreeItem)
> +                          .rootTreeItem
> +                          .QueryInterface(Ci.nsIInterfaceRequestor)
> +                          .getInterface(Ci.nsIDOMWindow)
> +                          .wrappedJSObject;

No need to do this. utilityOverlay.js runs in the chrome window.

@@ +18,5 @@
> +                          .QueryInterface(Ci.nsIInterfaceRequestor)
> +                          .getInterface(Ci.nsIDOMWindow)
> +                          .wrappedJSObject;
> +    if (chromeWin !=null) {
> +      if (chromeWin.gPrivateBrowsingUI.privateWindow)

if ("gPrivateBrowsingUI" in window && gPrivateBrowsingUI.privateWindow)
Comment 3 Saurabh Anand [:sawrubh] 2012-06-18 15:02:08 PDT
Created attachment 634188 [details] [diff] [review]
Patch v2

I will soon attach the test also in a separate patch. This patch is only for the changes.
Comment 4 Tim Taubert [:ttaubert] 2012-06-19 02:25:41 PDT
Comment on attachment 634188 [details] [diff] [review]
Patch v2

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

Great work, thanks! Looking forward to the mochitest!

(PS: You don't need to ask for review again when you fixed the nits. You can just upload the updated patch and set the r+ flag yourself.)

::: browser/base/content/utilityOverlay.js
@@ +9,4 @@
>  
>  XPCOMUtils.defineLazyGetter(this, "BROWSER_NEW_TAB_URL", function () {
>    const PREF = "browser.newtab.url";
> +  const TOPIC = "private-browsing-transition-complete";

Tiny nit: please add a new line here to give it some space to breathe.

@@ +22,4 @@
>    }
>  
>    Services.prefs.addObserver(PREF, update, false);
> +  Services.obs.addObserver(update,TOPIC,false);

Another tiny nit: please add a new line here and spaces after the commas.
Comment 5 Saurabh Anand [:sawrubh] 2012-06-20 03:25:51 PDT
Created attachment 634832 [details] [diff] [review]
Patch v3
Comment 6 Saurabh Anand [:sawrubh] 2012-06-20 03:27:39 PDT
Fixed the nits and set the r+ flag myself.
Comment 7 :Ehsan Akhgari (busy, don't ask for review please) 2012-06-20 09:25:25 PDT
Please mark this bug as checkin-needed when you've tested the patch on the try server.
Comment 8 Tim Taubert [:ttaubert] 2012-06-20 09:29:48 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #7)
> Please mark this bug as checkin-needed when you've tested the patch on the
> try server.

Still waiting for a test. I know he's already working on it.
Comment 9 Saurabh Anand [:sawrubh] 2012-06-20 13:58:40 PDT
Created attachment 635056 [details] [diff] [review]
Test
Comment 10 Tim Taubert [:ttaubert] 2012-06-20 14:11:54 PDT
Comment on attachment 635056 [details] [diff] [review]
Test

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

Looks really good to me. Just some minor additions/fixes before I'll r+ it :)

::: browser/base/content/test/Makefile.in
@@ +145,4 @@
>                   browser_bug623893.js \
>                   browser_bug624734.js \
>                   browser_bug647886.js \
> +                 browser_bug763468.js \

Please insert the test at the right position in the Makefile. The tests are ordered by bug number.

::: browser/base/content/test/browser_bug763468.js
@@ +5,5 @@
> +// This test makes sure that opening a new tab in private browsing mode opens about:privatebrowsing
> +
> +// initialization
> +const pb = Cc["@mozilla.org/privatebrowsing;1"].
> +         getService(Ci.nsIPrivateBrowsingService);

Nit: Please indent this a bit more to match the "Cc".

@@ +32,5 @@
> +        // Check the new tab opened while in private browsing mode
> +        is(gBrowser.selectedBrowser.currentURI.spec, "about:privatebrowsing", "URL of NewTab should be about:privatebrowsing in PB mode");
> +
> +        // exit private browsing mode
> +        togglePrivateBrowsing(function () {

Can you please open another tab here and check that it's now 'newTabUrl' again. Just to make sure returning from pb mode restores everything as it should.
Comment 11 Saurabh Anand [:sawrubh] 2012-06-20 14:44:59 PDT
Created attachment 635074 [details] [diff] [review]
Test v1
Comment 12 Tim Taubert [:ttaubert] 2012-06-20 14:50:09 PDT
Comment on attachment 635074 [details] [diff] [review]
Test v1

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

Great, thanks! Now let's push it to try and land it afterwards.
Comment 13 Saurabh Anand [:sawrubh] 2012-06-20 15:10:30 PDT
https://tbpl.mozilla.org/?tree=Try&rev=99f5e620a534
Comment 14 Tim Taubert [:ttaubert] 2012-06-21 01:38:41 PDT
Looks like we need to fix another test as well:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_tabMatchesInAwesomebar.js | 'about:privatebrowsing' not found in autocomplete.

And only on Mac there is:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bug565667.js | an unexpected uncaught JS exception reported through window.onerror - TypeError: gBrowser is null at chrome://browser/content/browser.js:12548
Comment 15 Tim Taubert [:ttaubert] 2012-06-21 01:49:52 PDT
(In reply to Tim Taubert [:ttaubert] from comment #14)
> And only on Mac there is:
> 
> TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/browser/browser/base/content/test/
> browser_bug565667.js | an unexpected uncaught JS exception reported through
> window.onerror - TypeError: gBrowser is null at
> chrome://browser/content/browser.js:12548

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#7148
Comment 16 Tim Taubert [:ttaubert] 2012-06-21 02:05:54 PDT
A simple fix might be to check if (gBrowser) but I'm not sure why it should be null when running the test for bug 565667.
Comment 17 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-06-21 17:40:42 PDT
(In reply to Tim Taubert [:ttaubert] from comment #14)
> Looks like we need to fix another test as well:
> 
> TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/browser/browser/base/content/test/
> browser_tabMatchesInAwesomebar.js | 'about:privatebrowsing' not found in
> autocomplete.

Pages to be registered for the switch-to-tab functionality are first filtered by isBlankPageURL(), which in turn uses BROWSER_NEW_TAB_URL. So ideally, you want to change line 250 of that test (where it checks if the URL is about:blank) to use browserWin.isBlankPageURL()
Comment 18 Saurabh Anand [:sawrubh] 2012-06-22 01:15:26 PDT
Created attachment 635662 [details] [diff] [review]
Patch v4
Comment 19 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-06-22 02:35:56 PDT
(In reply to Tim Taubert [:ttaubert] from comment #16)
> A simple fix might be to check if (gBrowser) but I'm not sure why it should
> be null when running the test for bug 565667.

Because on OSX, browser.js is loaded into non-browser windows, for which gBrowser is null. And that test opens a non-browser window, then attempts to open a tab from it (which is fine, the code handles that), but that involves (eventually) accessing that getter... and them boom.
Comment 20 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-06-22 02:37:56 PDT
Comment on attachment 635662 [details] [diff] [review]
Patch v4

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

::: browser/base/content/browser.js
@@ +7145,5 @@
>     * and the setter should only be used in tests.
>     */
>    get privateWindow() {
> +    if (gBrowser)
> +      return gBrowser.docShell.QueryInterface(Ci.nsILoadContext)

Can shorten this and make it a bit easier to read:

if (!gBrowser)
  return false;
<do other stuff>
Comment 21 Tim Taubert [:ttaubert] 2012-06-22 05:23:13 PDT
Comment on attachment 635662 [details] [diff] [review]
Patch v4

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

Looks very good and tests are passing locally. Please push it to try a last time - better safe than sorry - and address the last nit Blair and I mentioned. Thanks!

::: browser/base/content/browser.js
@@ +7145,4 @@
>     * and the setter should only be used in tests.
>     */
>    get privateWindow() {
> +    if (gBrowser)

You could also do this like:

> get privateWindow() {
>   return gBrowser && gBrowser.docShell.QueryInterface(Ci.nsILoadContext);
> }

I let you decide :)
Comment 22 Saurabh Anand [:sawrubh] 2012-06-22 05:38:58 PDT
Created attachment 635707 [details] [diff] [review]
Patch v5

Fixed the nits.
Comment 23 Saurabh Anand [:sawrubh] 2012-06-22 05:40:51 PDT
Comment on attachment 635707 [details] [diff] [review]
Patch v5

Setting the r+ myself since it was only a nit-fix.
Comment 24 Saurabh Anand [:sawrubh] 2012-06-22 05:41:50 PDT
https://tbpl.mozilla.org/?tree=Try&rev=b5a8c59ecf28
Comment 25 Tim Taubert [:ttaubert] 2012-06-23 01:41:21 PDT
Try was green, landed.

https://hg.mozilla.org/integration/fx-team/rev/f0236032c7e1
Comment 26 Tim Taubert [:ttaubert] 2012-06-23 11:19:00 PDT
https://hg.mozilla.org/mozilla-central/rev/f0236032c7e1
Comment 27 Girish Sharma [:Optimizer] 2012-06-28 08:22:18 PDT
A user can still go to about:newtab, is it required to stop him to go there ?
Comment 28 :Ehsan Akhgari (busy, don't ask for review please) 2012-06-28 08:56:47 PDT
(In reply to Girish Sharma [:Optimizer] from comment #27)
> A user can still go to about:newtab, is it required to stop him to go there ?

No.
Comment 29 Tim Taubert [:ttaubert] 2012-06-28 09:00:08 PDT
(In reply to Girish Sharma [:Optimizer] from comment #27)
> A user can still go to about:newtab, is it required to stop him to go there ?

No, in bug 722990 we implemented that no new content can be added to about:newtab while in private browsing mode.

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