It would be very nice to focus the location bar when the private browsing mode is entered and about:privatebrowsing is shown. Furthermore, do we want to clear the location bar contents ("about:privatebrowsing") as well?

>Furthermore, do we want to clear the location bar contents >("about:privatebrowsing") as well? yeah, users are a lot more comfortable entering information into a blank field than replacing something that they do not understand.

(In reply to comment #0) > Furthermore, do we want to clear the location bar contents > ("about:privatebrowsing") as well? FYI: Part of the patch in bug 455553 makes this quite easy and extensible (I'd be interested in also not displaying "about:sessionrestore" in the address bar).

(In reply to comment #2) > FYI: Part of the patch in bug 455553 makes this quite easy and extensible (I'd > be interested in also not displaying "about:sessionrestore" in the address > bar). Thanks for mentioning this! I'll give that code a shot and if it works, I'll just sneak in about:sessionrestore there as well. :-)

Focus the location bar, and don't display the URL for about:privatebrowsing and about:sessionrestore.

This would be great to have, I usually find myself wanting to paste or type a URL immediately after entering private browsing.

Comment on attachment 355556 [details] [diff] [review] Patch (v1) >diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js >+ var isLoadingPB = uriToLoad == "about:privatebrowsing"; Declare this right before where it's used, rather than at the beginning of the function? Could also just pass it directly to setTimeout and avoid the temporary... > // Replace "about:blank" with an empty string > // only if there's no opener (bug 370555). > if (uri.spec == "about:blank") > value = content.opener ? "about:blank" : ""; >+ else if (gInitialPages.indexOf(aURI.spec) != -1) >+ value = ""; > else > value = losslessDecodeURI(uri); I suppose this is relying on about:sessionstore and about:privatebrowsing not being URI_SAFE_FOR_UNTRUSTED_CONTENT to avoid bug 370555... might be worth a comment? And shouldn't you be testing "uri" rather than aURI? >- if ((location == "about:blank" && !content.opener) || >+ if ((gInitialPages.indexOf(location) != -1 && !content.opener) || > location == "") { // Second condition is for new tabs, otherwise > // reload function is enabled until tab is refreshed. > this.reloadCommand.setAttribute("disabled", "true"); > } else { > this.reloadCommand.removeAttribute("disabled"); > } > // wallpaper patch to prevent an unnecessary blank tab (bug 343895) > var blankTabToRemove = null; > if (gBrowser.tabContainer.childNodes.length == 1 && > !gPrefService.getBoolPref("browser.tabs.autoHide") && > gBrowser.selectedBrowser.sessionHistory.count < 2 && >- gBrowser.selectedBrowser.currentURI.spec == "about:blank" && >+ gInitialPages.indexOf(gBrowser.selectedBrowser.currentURI.spec) != -1 && > !gBrowser.selectedBrowser.contentDocument.body.hasChildNodes() && > !gBrowser.selectedTab.hasAttribute("busy")) > blankTabToRemove = gBrowser.selectedTab; It's not immediately clear to me that these two changes are desired. If we're going to land this on the branch I think it would be best to omit them and perhaps file followups.

(In reply to comment #6) > (From update of attachment 355556 [details] [diff] [review]) > >diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js > > >+ var isLoadingPB = uriToLoad == "about:privatebrowsing"; > > Declare this right before where it's used, rather than at the beginning of the > function? Could also just pass it directly to setTimeout and avoid the > temporary... Done. > > // Replace "about:blank" with an empty string > > // only if there's no opener (bug 370555). > > if (uri.spec == "about:blank") > > value = content.opener ? "about:blank" : ""; > >+ else if (gInitialPages.indexOf(aURI.spec) != -1) > >+ value = ""; > > else > > value = losslessDecodeURI(uri); > > I suppose this is relying on about:sessionstore and about:privatebrowsing not > being URI_SAFE_FOR_UNTRUSTED_CONTENT to avoid bug 370555... might be worth a > comment? And shouldn't you be testing "uri" rather than aURI? Yes, I added the comment, and changed to |uri|. There is one more case which this patch didn't handle, which was the case where about:privatebrowsing is not loaded when a new browser window is created, which may be what we do in bug 473843 or bug 476463. The new patch handles that case as well. > >- if ((location == "about:blank" && !content.opener) || > >+ if ((gInitialPages.indexOf(location) != -1 && !content.opener) || > > location == "") { // Second condition is for new tabs, otherwise > > // reload function is enabled until tab is refreshed. > > this.reloadCommand.setAttribute("disabled", "true"); > > } else { > > this.reloadCommand.removeAttribute("disabled"); > > } > > > // wallpaper patch to prevent an unnecessary blank tab (bug 343895) > > var blankTabToRemove = null; > > if (gBrowser.tabContainer.childNodes.length == 1 && > > !gPrefService.getBoolPref("browser.tabs.autoHide") && > > gBrowser.selectedBrowser.sessionHistory.count < 2 && > >- gBrowser.selectedBrowser.currentURI.spec == "about:blank" && > >+ gInitialPages.indexOf(gBrowser.selectedBrowser.currentURI.spec) != -1 && > > !gBrowser.selectedBrowser.contentDocument.body.hasChildNodes() && > > !gBrowser.selectedTab.hasAttribute("busy")) > > blankTabToRemove = gBrowser.selectedTab; > > It's not immediately clear to me that these two changes are desired. If we're > going to land this on the branch I think it would be best to omit them and > perhaps file followups. The first change is necessary, otherwise the reload button for about:sessionrestore and about:privatebrowsing would be enabled, which is weird with an empty location bar, and not really useful. The second change however seems unnecessary, and it won't even have any effect, because |!gBrowser.selectedBrowser.contentDocument.body.hasChildNodes()| will be false anyway, so I've removed it.

Gavin: ping?

(In reply to comment #7) > The first change is necessary, otherwise the reload button for > about:sessionrestore and about:privatebrowsing would be enabled, which is weird > with an empty location bar, and not really useful. OK, but there are a lot of "weird and not really useful" issues that we don't fix for the sake of it in mostly unrelated bugs :) I'm not sure URLBarSetURI focusing the location bar is appropriate - can you put that next to the relevant URLBarSetURI caller instead? Even better would be to put it near the code that triggers the about:privatebrowsing load... The comment for URI_SAFE_FOR_UNTRUSTED_CONTENT should probably be next to the declaration rather than next to one of the references (or next to both, I guess).

(In reply to comment #9) > OK, but there are a lot of "weird and not really useful" issues that we don't > fix for the sake of it in mostly unrelated bugs :) Fair enough, I removed that change from the patch. > I'm not sure URLBarSetURI focusing the location bar is appropriate - can you > put that next to the relevant URLBarSetURI caller instead? Even better would be > to put it near the code that triggers the about:privatebrowsing load... I moved it to gPrivateBrowsingUI.onEnterPrivateBrowsing, right after the SSTabRestoring event is triggered. > The comment for URI_SAFE_FOR_UNTRUSTED_CONTENT should probably be next to the > declaration rather than next to one of the references (or next to both, I > guess). Hmm, the comment was present next to the declaration as well, I'm not sure what you mean. Also, the new patch comes with an automated test.

Hmm, actually, can't we just keep the content.opener behavior for all gInitialPages URIs? The about:sessionrestore and about:privatebrowsing won't have content openers, right? I'm a bit confused about the SSTabRestoring thing. Why would it be fired when _entering_ private browsing? Is it triggered by the "dummy session" from _onBeforePrivateBrowsingModeChange?

(In reply to comment #11) > Hmm, actually, can't we just keep the content.opener behavior for all > gInitialPages URIs? The about:sessionrestore and about:privatebrowsing won't > have content openers, right? That's right. I updated the patch according to this point. > I'm a bit confused about the SSTabRestoring thing. Why would it be fired when > _entering_ private browsing? Is it triggered by the "dummy session" from > _onBeforePrivateBrowsingModeChange? Yes. Currently the session handling mechanism is the same both when entering and leaving the private browsing mode. The only difference is the session which would get restored (a single about:privatebrowsing tab for the former, and the previous session for the latter).

Sorry for the many review iterations, but I keep thinking of something new each time I look at this patch! Given that about:privatebrowsing is privileged, can't we put the url bar focusing code there in an onload handler?

Also, the comments about URI_SAFE_FOR_UNTRUSTED_CONTENT are no longer needed, since you're using the same opener logic that already existed for about:blank.

(In reply to comment #13) > Sorry for the many review iterations, but I keep thinking of something new each > time I look at this patch! No problem, I appreciate the ideas. :-) > Given that about:privatebrowsing is privileged, can't we put the url bar > focusing code there in an onload handler? Actually, we can! I don't know why I hadn't thought about that myself, but that's much more elegant. Here is a new patch which does that, and also removes the unneeded comments.

Comment on attachment 364780 [details] [diff] [review] Patch (v4) >diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js > // Replace "about:blank" with an empty string > // only if there's no opener (bug 370555). >+ if (gInitialPages.indexOf(uri.spec) != -1) >+ value = content.opener ? uri.spec : ""; Adjust the comment to mention "initial page URIs" rather than "about:blank"? >diff --git a/browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml b/browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml >+ window.__defineGetter__("mainWindow", function() { >+ delete this.mainWindow; >+ return this.mainWindow = window.QueryInterface(Ci.nsIInterfaceRequestor) >+ .getInterface(Ci.nsIWebNavigation) >+ .QueryInterface(Ci.nsIDocShellTreeItem) >+ .rootTreeItem >+ .QueryInterface(Ci.nsIInterfaceRequestor) >+ .getInterface(Ci.nsIDOMWindow); >+ }); Seems odd to mix "window.__defineGetter__" with "delete this.mainWindow" - can you use either "this" or "window" consistently? >+ // Focus the location bar >+ mainWindow.focusElement(mainWindow.gURLBar); gURLBar might be null (if the user customized it away), and focusElement doesn't appear to deal with that. We could probably make it return early in that case, but the least invasive change would be to add a null check here. >diff --git a/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_urlbarfocus.js b/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_urlbarfocus.js >+function test() { >+ function createProgressListener(location, fun) { Looks like the test is no longer using this function... >+ // ensure that the URL bar is no longer focused after leaving the private browsing mode >+ isnot(document.commandDispatcher.focusedElement, gURLBar.inputField, >+ "URL Bar should no longer be focused after leaving the private browsing mode"); What guarantees this behavior? Tests like these worry me a bit, because of their reliance on load events and timeouts. It's hard to be sure that the pages you load will only cause one load event to be fired, and to be sure that the timeout is sufficient to make the test pass consistently on unit test VMs. We should pay close attention to this test in the days after it's landed to ensure we're not adding more random failures... r=me with those addressed.

(In reply to comment #16) > >+ // ensure that the URL bar is no longer focused after leaving the private browsing mode > >+ isnot(document.commandDispatcher.focusedElement, gURLBar.inputField, > >+ "URL Bar should no longer be focused after leaving the private browsing mode"); > > What guarantees this behavior? This code: <http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/nsSessionStore.js#2138>

Verified fixed on trunk with builds on OS X and Windows: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090304 Minefield/3.2a1pre

Comment on attachment 364780 [details] [diff] [review] Patch (v4) a191=beltzner

Verified fixed on 1.9.1 with builds on OS X and Windows: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090404 Shiretoko/3.5b4pre ID:20090404035045 Ehsan, what I have noticed while verifying this patch, is that the content inside the location bar flickers. For me it goes through following states: URL => "Search Bookmarks and History" > "about:privatebrowsing" > Empty. Can we somehow improve this behavior?

(In reply to comment #22) > Ehsan, what I have noticed while verifying this patch, is that the content > inside the location bar flickers. For me it goes through following states: URL > => "Search Bookmarks and History" > "about:privatebrowsing" > Empty. Can we > somehow improve this behavior? That's definitely something we should look into improving. I'd appreciate if you can file a bug on this so that we can investigate it there.

Filed as bug 486858.