Bug 1390372: Set top site title to page title if path is non-empty. r=liuche draft
authorMichael Comella <michael.l.comella@gmail.com>
Mon, 14 Aug 2017 18:58:03 -0700
changeset 646254 c090095bf3aa692c60afcb1341d4e151c0b77736
parent 646253 0a3aa337545539437ff2a046216d8178bdabccca
child 726167 0c9e7bc02ff2aefb840272af3692d4a7ce981c69
push id74041
push usermichael.l.comella@gmail.com
push dateTue, 15 Aug 2017 01:58:29 +0000
reviewersliuche
bugs1390372
milestone57.0a1
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) {