557238 - Hide urlbar contents on about:home

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Matt Brubeck (:mbrubeck)]

Fennec currently hides the URI and title on "about:blank".  This patch gives the same treatment to the start page (about:home).  This has the following advantages:

- The start page can now have a real page title, instead of "Enter Search or Address" (an attempt to mimic the urlbar's emptytext, which appears in the wrong color compared to real emptytext).

- "about:home" does not appear in the urlbar the first time the user touches it, potentially confusing some users.

- The patch also replaces several hard-coded "about:blank" special cases into a descriptively-named function.

Comment 1

[Matt Brubeck (:mbrubeck)]

Attachment: patch

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 437054 [details] [diff] [review]
patch

>diff -r 9f05fbde842c chrome/content/aboutHome.xhtml

>-  <title>&urlbar.emptytext;</title>
>+  <title>&homepage.default;</title>

In this case, we really do want the "Enter Search or Address" text here. It's more important to show the hint than to show a real page title.


I wonder if we should put _hideUrlbarText in Util, but rename to isURIEmpty(aURI)

The patch isn't bad otherwise.

Comment 3

[Matt Brubeck (:mbrubeck)]

(In reply to comment #2)
> In this case, we really do want the "Enter Search or Address" text here. It's
> more important to show the hint than to show a real page title.

Mark, the other changes in this patch cause the real hint to always show on about:home regardless of the page title (just as it does on about:blank).  Users won't see "Firefox Start" in the urlbar, but they will see it in the awesomebar if they bookmark the page or if we decide to include "about:" URIs in history.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 437054 [details] [diff] [review]
patch

OK, I see how the <title> is handled now. Pulling in preferences.dtd isn't my favorite thing, but being able to bookmark the page with a good title is nice.

I do think the Util function will serve us better.

Comment 5

[Matt Brubeck (:mbrubeck)]

There's also a regression caused by this patch, something to do with bug 521828 - the <title> doesn't display on the first pageload in some cases.  I'll try to fix that.

Comment 6

[Matt Brubeck (:mbrubeck)]

Attachment: revised patch

Here's a version of the patch that incorporates feedback and fixes the problems mentioned above.  This patch now depends on a separate patch from bug 521828 (attachment 437399 [details] [diff] [review]).

Comment 7

[Matt Brubeck (:mbrubeck)]

Attachment: patch v3

Argh, a few lines got into that patch that weren't supposed to.  Fixed.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 437401 [details] [diff] [review]
patch v3

>diff -r 067718acefeb chrome/content/Util.js

>+  /** Don't display anything in the urlbar for these special URIs. */
>+  isURIEmpty: function isURIEmpty(aURI) {
>+    return (!aURI || aURI == "about:blank" || aURI == "about:home");
>+  },

Let's name this isURLEmpty. We should try to separate the naming of URL and URI a bit more. I think a good convention would be: use "URI" if the variable or function uses (or returns) a nsIURI. Otherwise, use "URL" if the variable or function uses (or returns) a string.

We haven't been good at doing this separation in the past.

>diff -r 067718acefeb chrome/content/browser-ui.js

>   _titleChanged : function(aDocument) {

>-    var caption = aDocument.title;
>-    if (!caption) {
>-      caption = this.getDisplayURI(browser);
>-      if (caption == "about:blank")
>-        caption = "";
>-    }
>+    var uri = this.getDisplayURI(browser);
>+    var caption = aDocument.title || uri;
>+
>+    if (Util.isURIEmpty(uri))
>+      caption = "";

Can we keep the logic the way it was, or change it so getDisplayURI is not called unless we need it to be called.

>   _networkStart: function _networkStart(aRequest) {
>-    let uri = aRequest.QueryInterface(Ci.nsIChannel).URI.spec;
>+    // For chrome URIs especially, we want the urlbar during loading to use the
>+    // "original" URI (about:home), not a rewritten one (jar:file:///...).
>+    let uri = aRequest.QueryInterface(Ci.nsIChannel).originalURI.spec;
>     this._tab.startLoading(uri);

I don't think we need this part

Comment 9

[Matt Brubeck (:mbrubeck)]

Attachment: patch v4

Updated patch:

* Renamed function to isURLEmpty.
* Removed changes to _networkStart.

This still relies on nsIChannel.originalURI.  This is slightly ugly, but I think it's the right thing here - it also fixes a regression from bug 521828 where about:config displays a jar: URL in the urlbar.

(in response to comment #8)
> Can we keep the logic the way it was, or change it so
> getDisplayURI is not called unless we need it to be called.

Sorry, I need to call getDisplayURI every time to decide whether to show the title.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed:
http://hg.mozilla.org/mobile-browser/rev/3ef5eb570bc2

Comment 11

[Aakash Desai [:aakashd]]

verified FIXED On builds:

Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2.4pre) Gecko/20100412 Namoroka/3.6.4pre Fennec/1.1a2pre

and

Mozilla/5.0 (X11; U; Linux armv6l; en-US; rv:1.9.3a4pre) Gecko/20100412 Namoroka/3.7a4pre Fennec/1.1a2pre