The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 16

Status

()

Firefox
Tabbed Browser
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: ttaubert, Assigned: sawrubh)

Tracking

Trunk
Firefox 16
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 633903 [details] [diff] [review]
Patch v1

@Tim, Could you suggest some tests that I need to run for this fix.
Attachment #633903 - Flags: feedback?(ttaubert)
(Reporter)

Comment 2

5 years ago
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)
(Assignee)

Comment 3

5 years ago
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.
Attachment #633903 - Attachment is obsolete: true
Attachment #634188 - Flags: review?(ttaubert)
(Reporter)

Comment 4

5 years ago
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+
(Assignee)

Comment 5

5 years ago
Created attachment 634832 [details] [diff] [review]
Patch v3
Attachment #634188 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #634832 - Flags: review+
(Assignee)

Comment 6

5 years ago
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.
(Reporter)

Comment 8

5 years ago
(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.
(Assignee)

Comment 9

5 years ago
Created attachment 635056 [details] [diff] [review]
Test
Attachment #635056 - Flags: review?(ttaubert)
(Reporter)

Comment 10

5 years ago
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+
(Assignee)

Comment 11

5 years ago
Created attachment 635074 [details] [diff] [review]
Test v1
Attachment #635056 - Attachment is obsolete: true
Attachment #635074 - Flags: review?(ttaubert)
(Reporter)

Comment 12

5 years ago
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+
(Assignee)

Comment 13

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=99f5e620a534
(Reporter)

Comment 14

5 years ago
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
(Reporter)

Comment 15

5 years ago
(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
(Reporter)

Comment 16

5 years ago
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()
(Assignee)

Comment 18

5 years ago
Created attachment 635662 [details] [diff] [review]
Patch v4
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>
(Reporter)

Comment 21

5 years ago
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+
(Assignee)

Comment 22

5 years ago
Created attachment 635707 [details] [diff] [review]
Patch v5

Fixed the nits.
Attachment #635662 - Attachment is obsolete: true
(Assignee)

Comment 23

5 years ago
Comment on attachment 635707 [details] [diff] [review]
Patch v5

Setting the r+ myself since it was only a nit-fix.
Attachment #635707 - Flags: review+
(Assignee)

Comment 24

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=b5a8c59ecf28
(Reporter)

Comment 25

5 years ago
Try was green, landed.

https://hg.mozilla.org/integration/fx-team/rev/f0236032c7e1
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 16
(Reporter)

Comment 26

5 years ago
https://hg.mozilla.org/mozilla-central/rev/f0236032c7e1
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Depends on: 767835
(Reporter)

Updated

5 years ago
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.
(Reporter)

Comment 29

5 years ago
(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.

Updated

3 years ago
Depends on: 936417
You need to log in before you can comment on or make changes to this bug.