Bug 1390372: Set top site title to page title if path is non-empty. r=liuche
authorMichael Comella <michael.l.comella@gmail.com>
Mon, 14 Aug 2017 18:58:03 -0700
changeset 376442 f273e82ed541b043890803f7a5255854fc14e538
parent 376441 6de0b3854124ae6a9da0bcd288c64c162f22fa08
child 376443 4e94b77bafeeea9ce80928ede0099874ff922d91
push id32384
push userarchaeopteryx@coole-files.de
push dateThu, 24 Aug 2017 11:27:12 +0000
treeherdermozilla-central@8d1350135a04 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersliuche
bugs1390372
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 1390372: Set top site title to page title if path is non-empty. r=liuche MozReview-Commit-ID: CoWqRlBtKhO
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
@@ -21,17 +21,16 @@ import org.mozilla.gecko.activitystream.
 import org.mozilla.gecko.activitystream.homepanel.menu.ActivityStreamContextMenu;
 import org.mozilla.gecko.activitystream.homepanel.model.TopSite;
 import org.mozilla.gecko.home.HomePager;
 import org.mozilla.gecko.icons.IconCallback;
 import org.mozilla.gecko.icons.IconResponse;
 import org.mozilla.gecko.icons.Icons;
 import org.mozilla.gecko.util.DrawableUtil;
 import org.mozilla.gecko.util.StringUtils;
-import org.mozilla.gecko.util.TouchTargetUtil;
 import org.mozilla.gecko.util.URIUtils;
 import org.mozilla.gecko.util.ViewUtil;
 import org.mozilla.gecko.widget.FaviconView;
 
 import java.lang.ref.WeakReference;
 import java.net.URI;
 import java.net.URISyntaxException;
 import java.util.UUID;
@@ -105,34 +104,50 @@ import java.util.concurrent.Future;
         final Drawable pinDrawable;
         if (topSite.isPinned()) {
             pinDrawable = DrawableUtil.tintDrawable(itemView.getContext(), R.drawable.as_pin, Color.WHITE);
         } else {
             pinDrawable = null;
         }
         TextViewCompat.setCompoundDrawablesRelativeWithIntrinsicBounds(title, pinDrawable, null, null, null);
 
-        final URI topSiteURI;
+        setTopSiteTitle(topSite);
+    }
+
+    private void setTopSiteTitle(final TopSite topSite) {
+        URI topSiteURI = null; // not final so we can use in the Exception case.
+        boolean wasException = false;
         try {
             topSiteURI = new URI(topSite.getUrl());
         } catch (final URISyntaxException e) {
-            // If this is not a valid URI, there is not much processing we can do on it.
-            // Also, see comment below regarding setCenteredText.
-            setTopSiteTitle(title, topSite.getUrl());
-            return;
+            wasException = true;
         }
 
-        // 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();
+        // At a high level, the logic is: if the path empty, use "subdomain.domain", otherwise use the
+        // page title. 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). If we ever want better titles, we could create a heuristic to pull the title
+        // from parts of the URL, page title, etc.
+        if (wasException || !URIUtils.isPathEmpty(topSiteURI)) {
+            // See comment below regarding setCenteredText.
+            final String pageTitle = topSite.getTitle();
+            final String updateText = !TextUtils.isEmpty(pageTitle) ? pageTitle : topSite.getUrl();
+            setTopSiteTitleHelper(title, updateText);
+
+        } else {
+            // 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();
+        }
     }
 
-    private static void setTopSiteTitle(final TextView textView, final String title) {
+    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());
     }
 
     @Override
     public void onIconResponse(IconResponse response) {
         faviconView.updateImage(response);
@@ -163,17 +178,17 @@ import java.util.concurrent.Future;
             }
 
             final String updateText;
             if (TextUtils.isEmpty(hostText)) {
                 updateText = "";
             } else {
                 updateText = StringUtils.stripCommonSubdomains(hostText);
             }
-            setTopSiteTitle(titleView, updateText);
+            setTopSiteTitleHelper(titleView, updateText);
         }
 
         /**
          * Returns true if the tag on the given view matches the tag from the constructor. We do this to ensure
          * the View we're making this request for hasn't been re-used by the time this request completes.
          */
         @UiThread
         private boolean isTagSameAsStartTag(final View viewToCheck) {