Add favicon and publisher name to snippet cards

chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java

Index: chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java b/chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java
index 5a6bcef7a1d21fa39737df1b42d9a8a69ae7d939..e13d6c94361f49e2a14d709d4ff7b57fb8ee2acc 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java
@@ -12,18 +12,22 @@ import android.graphics.drawable.Drawable;
 import android.graphics.drawable.TransitionDrawable;
 import android.media.ThumbnailUtils;
 import android.os.AsyncTask;
+import android.text.TextUtils;
 import android.text.format.DateUtils;
 import android.view.LayoutInflater;
 import android.view.View;
+import android.view.View.MeasureSpec;
 import android.view.ViewGroup;
 import android.widget.ImageView;
 import android.widget.TextView;
 
+import org.chromium.base.ApiCompatibilityUtils;
 import org.chromium.base.Log;
 import org.chromium.base.StreamUtil;
 import org.chromium.base.metrics.RecordHistogram;
 import org.chromium.base.metrics.RecordUserAction;
 import org.chromium.chrome.R;
+import org.chromium.chrome.browser.favicon.FaviconHelper.FaviconImageCallback;
 import org.chromium.chrome.browser.ntp.NewTabPageUma;
 import org.chromium.chrome.browser.ntp.NewTabPageView.NewTabPageManager;
 import org.chromium.chrome.browser.ntp.cards.NewTabPageListItem;
@@ -31,6 +35,7 @@ import org.chromium.chrome.browser.ntp.cards.NewTabPageViewHolder;
 
 import java.io.IOException;
 import java.io.InputStream;
+import java.net.MalformedURLException;
 import java.net.URL;
 
 /**
@@ -38,6 +43,7 @@ import java.net.URL;
  */
 public class SnippetArticleViewHolder extends NewTabPageViewHolder implements View.OnClickListener {
     private static final String TAG = "NtpSnippets";
+    private static final String PUBLISHER_FORMAT_STRING = "%s - %s";
     private static final int FADE_IN_ANIMATION_TIME_MS = 300;
 
     private final NewTabPageManager mNewTabPageManager;
@@ -102,14 +108,16 @@ public class SnippetArticleViewHolder extends NewTabPageViewHolder implements Vi
         SnippetArticle item = (SnippetArticle) article;
 
         mHeadlineTextView.setText(item.mTitle);
-        mPublisherTextView.setText(
+        mPublisherTextView.setText(String.format(PUBLISHER_FORMAT_STRING, item.mPublisher,
                 DateUtils.getRelativeTimeSpanString(item.mTimestamp, System.currentTimeMillis(),
-                        DateUtils.MINUTE_IN_MILLIS));
+                        DateUtils.MINUTE_IN_MILLIS, DateUtils.FORMAT_ABBREV_RELATIVE)));
+
         mArticleSnippetTextView.setText(item.mPreviewText);
         mUrl = item.mUrl;
         mPosition = item.mPosition;
 
         updateThumbnail(item);
+        upadateFavicon(item);
     }
 
     private void updateThumbnail(final SnippetArticle snippet) {
@@ -177,6 +185,78 @@ public class SnippetArticleViewHolder extends NewTabPageViewHolder implements Vi
         transitionDrawable.startTransition(FADE_IN_ANIMATION_TIME_MS);
     }
 
+    private void upadateFavicon(final SnippetArticle snippet) {

[Bernhard Bauer, 2016/04/27 13:22:05]

Nit: updateFavicon :)

Also, the final is unnecessary.

[May, 2016/04/27 16:45:08]

Done.

+        // The favicon size should match the textview height
+        int widthSpec = MeasureSpec.makeMeasureSpec(0, MeasureSpec.UNSPECIFIED);
+        int heightSpec = MeasureSpec.makeMeasureSpec(0, MeasureSpec.UNSPECIFIED);
+        mPublisherTextView.measure(widthSpec, heightSpec);
+        fetchFavicon(snippet, mPublisherTextView.getHeight());
+    }
+
+    // Fetch the favicon using the article's url first. If that fails, try to fetch the
+    // favicon for the hostname
+    private void fetchFavicon(final SnippetArticle snippet, final int faviconSizePx) {
+        mNewTabPageManager.getLocalFaviconImageForURL(
+                snippet.mUrl, faviconSizePx, new FaviconImageCallback() {
+                    @Override
+                    public void onFaviconAvailable(Bitmap image, String iconUrl) {
+                        if (image == null) {
+                            fetchFaviconForHostname(snippet.mUrl, faviconSizePx);
+                        } else {
+                            Drawable siteFavicon = new BitmapDrawable(
+                                    mPublisherTextView.getContext().getResources(), image);
+                            siteFavicon.setBounds(0, 0, faviconSizePx, faviconSizePx);
+                            ApiCompatibilityUtils.setCompoundDrawablesRelative(
+                                    mPublisherTextView, siteFavicon, null, null, null);
+                            mPublisherTextView.setVisibility(View.VISIBLE);
+                        }
+                    }
+                });
+    }
+
+    private void fetchFaviconForHostname(String urlStr, final int sizePx) {
+        URL url = null;

[Bernhard Bauer, 2016/04/27 13:22:05]

I think it's more common to use java.net.URI when you don't actually want to
load the URL from the network (at least not via Java in this case).

[May, 2016/04/27 16:45:08]

Done.

+        try {
+            url = new URL(urlStr);
+        } catch (MalformedURLException e) {
+            drawDefaultFavicon(sizePx);
+            return;
+        }
+
+        String origin = null;

[Bernhard Bauer, 2016/04/27 13:22:05]

I wouldn't call this origin if it's just protocol and host (technically, an
origin includes the port as well). Also, could we extract this into a method
that returns the URL to use for the favicon? That might help with dealing with
some of the error cases here.

[May, 2016/04/27 16:45:08]

Not sure I understand the second part. We first try to get the cached favicon of
the exact article URL. If that fails, we do this thing where we get the origin
(okay, minus the port) and try to get the favicon for that. Not sure what
extracting that into a method helps with?

[Bernhard Bauer, 2016/04/28 14:19:20]

What I meant was extracting the part that removes the path from the URL into a
method, because that has multiple ways to fail (so you could just return null,
and have the caller handle that in a single place).

But really, some of the error checking here is unnecessary: In line 225, |uri|
can't be null (if parsing the URI failed, we'd have returned), and because of
that, the origin-ish will also not be empty in line 227.

[May, 2016/04/28 18:01:52]

True, I removed the checks and now there's just one possible error. Hooray? Re:
name for that origin-ish string, couldn't decide on a name for the string so
just did it in place. 

+        if (url != null) origin = String.format("%s://%s", url.getProtocol(), url.getHost());
+
+        if (!TextUtils.isEmpty(origin)) {
+            mNewTabPageManager.getLocalFaviconImageForURL(
+                    origin, sizePx, new FaviconImageCallback() {
+                        @Override
+                        public void onFaviconAvailable(Bitmap image, String iconUrl) {
+                            if (image == null) {
+                                drawDefaultFavicon(sizePx);
+                            } else {
+                                Drawable domainFavicon = new BitmapDrawable(

[Bernhard Bauer, 2016/04/27 13:22:05]

I think you could extract this into a shared method. Maybe one that takes a
Drawable and sets it on the |mPublisherTextView|, and one that takes a Bitmap
and then calls the former method with a BitmapDrawable? Then you could also
reuse the former method for drawDefaultFavicon().

[May, 2016/04/27 16:45:07]

Done.

+                                        mPublisherTextView.getContext().getResources(), image);
+                                domainFavicon.setBounds(0, 0, sizePx, sizePx);
+                                ApiCompatibilityUtils.setCompoundDrawablesRelative(
+                                        mPublisherTextView, domainFavicon, null, null, null);
+                                mPublisherTextView.setVisibility(View.VISIBLE);
+                            }
+                        }
+                    });
+        } else {
+            drawDefaultFavicon(sizePx);
+        }
+    }
+
+    private void drawDefaultFavicon(int faviconSizePx) {
+        Drawable newIcon = ApiCompatibilityUtils.getDrawable(
+                mPublisherTextView.getContext().getResources(), R.drawable.default_favicon);
+        newIcon.setBounds(0, 0, faviconSizePx, faviconSizePx);
+        ApiCompatibilityUtils.setCompoundDrawablesRelative(
+                mPublisherTextView, newIcon, null, null, null);
+        mPublisherTextView.setVisibility(View.VISIBLE);
+    }
+
     @Override
     public boolean isDismissable() {
         return true;