763468 - Use about:privatebrowsing for new tabs opened in private browsing mode

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

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]]

Attachment: Patch v1

@Tim, Could you suggest some tests that I need to run for this fix.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

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]]

Attachment: 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] (inactive)]

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]]

Attachment: Patch v3

Comment 6

[Saurabh Anand [:sawrubh]]

Fixed the nits and set the r+ flag myself.

Comment 7

[(no longer active)]

Please mark this bug as checkin-needed when you've tested the patch on the try server.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

(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]]

Attachment: Test

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

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]]

Attachment: Test v1

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

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]]

https://tbpl.mozilla.org/?tree=Try&rev=99f5e620a534

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

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] (inactive)]

(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] (inactive)]

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] (UNAVAILABLE)]

(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]]

Attachment: Patch v4

Comment 19

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(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] (UNAVAILABLE)]

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] (inactive)]

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]]

Attachment: Patch v5

Fixed the nits.

Comment 23

[Saurabh Anand [:sawrubh]]

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]]

https://tbpl.mozilla.org/?tree=Try&rev=b5a8c59ecf28

Comment 25

[Tim Taubert [:ttaubert] (inactive)]

Try was green, landed.

https://hg.mozilla.org/integration/fx-team/rev/f0236032c7e1

Comment 26

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/f0236032c7e1

Comment 27

[Girish Sharma [:Optimizer]]

A user can still go to about:newtab, is it required to stop him to go there ?

Comment 28

[(no longer active)]

(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] (inactive)]

(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.