Bug 1393579: Show subdomain.domain for pages without titles in top sites. r=liuche
authorMichael Comella <michael.l.comella@gmail.com>
Tue, 12 Sep 2017 13:58:51 -0700
changeset 430260 72a9bca22654ea28ba17a50d9dd9d078e14fa309
parent 430259 0eb129729d90b4c03b0a88e34398578a97f59828
child 430261 c9b73b821bd5393ffc64a1cc81085095a696ecb0
push id7761
push userjlund@mozilla.com
push dateFri, 15 Sep 2017 00:19:52 +0000
treeherdermozilla-beta@c38455951db4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersliuche
bugs1393579
milestone57.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1393579: Show subdomain.domain for pages without titles in top sites. r=liuche MozReview-Commit-ID: 9SugVwZDbD7
mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/topsites/TopSitesCard.java
--- a/mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/topsites/TopSitesCard.java
+++ b/mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/topsites/TopSitesCard.java
@@ -119,53 +119,57 @@ import java.util.concurrent.Future;
         }
         TextViewCompat.setCompoundDrawablesRelativeWithIntrinsicBounds(title, pinDrawable, null, null, null);
 
         setTopSiteTitle(topSite);
     }
 
     private void setTopSiteTitle(final TopSite topSite) {
         URI topSiteURI = null; // not final so we can use in the Exception case.
-        boolean wasException = false;
+        boolean isInvalidURI = false;
         try {
             topSiteURI = new URI(topSite.getUrl());
         } catch (final URISyntaxException e) {
-            wasException = true;
+            isInvalidURI = true;
         }
 
         final boolean isSiteSuggestedFromDistribution = BrowserDB.from(itemView.getContext()).getSuggestedSites()
                 .containsSiteAndSiteIsFromDistribution(topSite.getUrl());
 
         // Some already installed distributions are unlikely to be updated (OTA, system) and their suggested
         // site titles were written for the old top sites, where we had more room to display titles: we want
         // to provide them with more lines. However, it's complex to distinguish a distribution intended for
         // the old top sites and the new one so for code simplicity, we allow all distributions more lines for titles.
         title.setMaxLines(isSiteSuggestedFromDistribution ? 2 : 1);
 
-        // At a high level, the logic is: if the path non-empty or the site is suggested by a distribution, use the page
-        // title, otherwise use "subdomain.domain". From a UX perspective, people refer to domains by their name ("it's
-        // on wikipedia") and it's a clean look. However, if a url has a path, it will not fit on the screen with the
-        // domain so we need an alternative: the page title is an easy win (though not always perfect, e.g. when SEO
-        // keywords are added). We use page titles with distributions because that's what those distributions expect to
-        // be shown. If we ever want better top site titles, we could create a heuristic to pull the title from parts
-        // of the URL, page title, etc.
-        if (wasException ||
-                isSiteSuggestedFromDistribution ||
-                !URIUtils.isPathEmpty(topSiteURI)) {
-            // See comment below regarding setCenteredText.
-            final String pageTitle = topSite.getTitle();
+        // We use page titles with distributions because that's what the creators of those distributions expect to
+        // be shown. Also, we need a valid URI for our preferred case so we stop here if we don't have one.
+        final String pageTitle = topSite.getTitle();
+        if (isInvalidURI || isSiteSuggestedFromDistribution) {
             final String updateText = !TextUtils.isEmpty(pageTitle) ? pageTitle : topSite.getUrl();
-            setTopSiteTitleHelper(title, updateText);
+            setTopSiteTitleHelper(title, updateText); // See comment below regarding setCenteredText.
 
-        } else {
+        // This is our preferred case: we display "subdomain.domain". People refer to sites by their domain ("it's
+        // on wikipedia!") and it's a clean look so we display the domain if we can.
+        //
+        // If the path is non-empty, we'd normally go to the case below (see that comment). However, if there's no
+        // title, we'd prefer to fallback on "subdomain.domain" rather than a url, which is really ugly.
+        } else if (URIUtils.isPathEmpty(topSiteURI) ||
+                (!URIUtils.isPathEmpty(topSiteURI) && TextUtils.isEmpty(pageTitle))) {
             // Our AsyncTask calls setCenteredText(), which needs to have all drawable's in place to correctly
             // layout the text, so we need to wait with requesting the title until we've set our pin icon.
             final UpdateCardTitleAsyncTask titleAsyncTask = new UpdateCardTitleAsyncTask(itemView.getContext(),
                     topSiteURI, title);
             titleAsyncTask.execute();
+
+        // We have a site with a path that has a non-empty title. It'd be impossible to distinguish multiple sites
+        // with "subdomain.domain" so if there's a path, we have to use something else: "domain/path" would overflow
+        // before it's useful so we use the page title.
+        } else {
+            setTopSiteTitleHelper(title, pageTitle); // See comment above regarding setCenteredText.
         }
     }
 
     private static void setTopSiteTitleHelper(final TextView textView, final String title) {
         // We use consistent padding all around the title, and the top padding is never modified,
         // so we can pass that in as the default padding:
         ViewUtil.setCenteredText(textView, title, textView.getPaddingTop());
     }