Closed
Bug 763468
Opened 12 years ago
Closed 12 years ago
Use about:privatebrowsing for new tabs opened in private browsing mode
Categories
(Firefox :: Tabbed Browser, defect)
Firefox
Tabbed Browser
Tracking
()
RESOLVED
FIXED
Firefox 16
People
(Reporter: ttaubert, Assigned: sawrubh)
References
Details
(Whiteboard: [fixed-in-fx-team])
Attachments
(2 files, 5 obsolete files)
3.67 KB,
patch
|
ttaubert
:
review+
|
Details | Diff | Splinter Review |
2.32 KB,
patch
|
sawrubh
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
@Tim, Could you suggest some tests that I need to run for this fix.
Attachment #633903 -
Flags: feedback?(ttaubert)
Reporter | ||
Comment 2•12 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•12 years ago
|
||
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•12 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•12 years ago
|
||
Attachment #634188 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Attachment #634832 -
Flags: review+
Assignee | ||
Comment 6•12 years ago
|
||
Fixed the nits and set the r+ flag myself.
Comment 7•12 years ago
|
||
Please mark this bug as checkin-needed when you've tested the patch on the try server.
Reporter | ||
Comment 8•12 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•12 years ago
|
||
Attachment #635056 -
Flags: review?(ttaubert)
Reporter | ||
Comment 10•12 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•12 years ago
|
||
Attachment #635056 -
Attachment is obsolete: true
Attachment #635074 -
Flags: review?(ttaubert)
Reporter | ||
Comment 12•12 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•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=99f5e620a534
Reporter | ||
Comment 14•12 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•12 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•12 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.
Comment 17•12 years ago
|
||
(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•12 years ago
|
||
Attachment #634832 -
Attachment is obsolete: true
Comment 19•12 years ago
|
||
(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•12 years ago
|
||
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•12 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•12 years ago
|
||
Fixed the nits.
Attachment #635662 -
Attachment is obsolete: true
Assignee | ||
Comment 23•12 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•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=b5a8c59ecf28
Reporter | ||
Comment 25•12 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•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f0236032c7e1
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 27•12 years ago
|
||
A user can still go to about:newtab, is it required to stop him to go there ?
Comment 28•12 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.
Reporter | ||
Comment 29•12 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.
You need to log in
before you can comment on or make changes to this bug.
Description
•