Closed Bug 763468 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 16

People

(Reporter: ttaubert, Assigned: sawrubh)

References

Details

(Whiteboard: [fixed-in-fx-team])

Attachments

(2 files, 5 obsolete files)

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.
Attached patch Patch v1 (obsolete) — Splinter Review
@Tim, Could you suggest some tests that I need to run for this fix.
Attachment #633903 - Flags: feedback?(ttaubert)
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)
Attachment #633903 - Flags: feedback?(ttaubert)
Attached patch Patch v2 (obsolete) — Splinter Review
I will soon attach the test also in a separate patch. This patch is only for the changes.
Attachment #633903 - Attachment is obsolete: true
Attachment #634188 - Flags: review?(ttaubert)
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.
Attachment #634188 - Flags: review?(ttaubert) → review+
Attached patch Patch v3 (obsolete) — Splinter Review
Attachment #634188 - Attachment is obsolete: true
Attachment #634832 - Flags: review+
Fixed the nits and set the r+ flag myself.
Please mark this bug as checkin-needed when you've tested the patch on the try server.
(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.
Attached patch Test (obsolete) — Splinter Review
Attachment #635056 - Flags: review?(ttaubert)
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.
Attachment #635056 - Flags: review?(ttaubert) → feedback+
Attached patch Test v1Splinter Review
Attachment #635056 - Attachment is obsolete: true
Attachment #635074 - Flags: review?(ttaubert)
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.
Attachment #635074 - Flags: review?(ttaubert) → review+
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
(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
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.
(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()
Attached patch Patch v4 (obsolete) — Splinter Review
Attachment #634832 - Attachment is obsolete: true
(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 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 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 :)
Attachment #635662 - Flags: review+
Attached patch Patch v5Splinter Review
Fixed the nits.
Attachment #635662 - Attachment is obsolete: true
Comment on attachment 635707 [details] [diff] [review]
Patch v5

Setting the r+ myself since it was only a nit-fix.
Attachment #635707 - Flags: review+
Try was green, landed.

https://hg.mozilla.org/integration/fx-team/rev/f0236032c7e1
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 16
https://hg.mozilla.org/mozilla-central/rev/f0236032c7e1
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Depends on: 767835
Depends on: 767836
A user can still go to about:newtab, is it required to stop him to go there ?
(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 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.
Depends on: 936417
You need to log in before you can comment on or make changes to this bug.