[NTP Snippets] 📷 Add placeholders and start caching thumbnails

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

Index: chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/ThumbnailFetchingTask.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/ThumbnailFetchingTask.java b/chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/ThumbnailFetchingTask.java
new file mode 100644
index 0000000000000000000000000000000000000000..cf310d15499b6c09117ce12e9c335e603cbbc36a
--- /dev/null
+++ b/chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/ThumbnailFetchingTask.java
@@ -0,0 +1,99 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.chrome.browser.ntp.snippets;
+
+import android.content.res.Resources;
+import android.graphics.Bitmap;
+import android.graphics.BitmapFactory;
+import android.graphics.drawable.BitmapDrawable;
+import android.graphics.drawable.Drawable;
+import android.graphics.drawable.TransitionDrawable;
+import android.media.ThumbnailUtils;
+import android.os.AsyncTask;
+import android.widget.ImageView;
+
+import org.chromium.base.Log;
+import org.chromium.base.StreamUtil;
+import org.chromium.chrome.R;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.net.MalformedURLException;
+import java.net.URL;
+
+/**
+ *  Async task to download the thumbnail from a URL and set it on a provided view.
+ *
+ * TODO(maybelle): This task to retrieve the thumbnail from the web is temporary while
+ * we are prototyping this feature for clank. For the real production feature, we

[Theresa, 2016/04/11 23:42:27]

Avoid code names in public CLs.

[dgn, 2016/04/12 11:17:15]

Done.

+ * will likely have to download/decode/store the thumbnail on the native side.
+ * See https://crbug.com/594122
+ */
+class ThumbnailFetchingTask extends AsyncTask<Void, Void, Bitmap> {
+    private static final String TAG = "NtpSnippets";
+    private static final int FADE_IN_ANIMATION_TIME_MS = 300;
+
+    private final SnippetArticle mSnippetArticle;
+    private final ImageView mThumbnailView;
+
+    /**
+     * @param snippetArticle the article for which we want to download the thumbnail

[Theresa, 2016/04/11 23:42:27]

Nit: use proper sentence capitalization/punctuation on comments. "The article...
thumbnail."

Also capitalize "The" below.

[dgn, 2016/04/12 11:17:15]

Done.

+     * @param thumbnailView the view where the downloaded thumbnail will be set.
+     */
+    ThumbnailFetchingTask(SnippetArticle snippetArticle, ImageView thumbnailView) {
+        mSnippetArticle = snippetArticle;
+        mThumbnailView = thumbnailView;
+
+        if (snippetArticle.getThumbnailBitmap() == null) {
+            mThumbnailView.setImageResource(R.drawable.ic_snippet_thumbnail_placeholder);

[Theresa, 2016/04/11 23:42:27]

Vector drawables are an L+ feature. Did you test this pre-L? I think this should
just work pre-L (except for the lint warning... not sure how to work around that
- maybe Stack Overflow has some ideas) since Chrome extends AppCompatActivity
and we recently rolled the AppCompat support library to a version that includes
vector support, but it would be good to test.

If it doesn't work, here's a resource on AppCompat support for vector drawables:
https://chris.banes.me/2016/02/25/appcompat-vector/

[dgn, 2016/04/12 11:17:14]

Yes I looked into that, apparently there is nothing special to do. We had to
change some build files when updating the support library to 23.2 (which ports
the VectorDrawable support and starts using them internally), but new they
worked just fine on my KitKat device.

+        } else {

[Bernhard Bauer, 2016/04/12 08:34:34]

Instead of doing this in the constructor (and the check for an empty URL below),
could we skip creating the task completely in this case?

[dgn, 2016/04/12 11:17:14]

Done.

+            mThumbnailView.setImageBitmap(this.mSnippetArticle.getThumbnailBitmap());

[Bernhard Bauer, 2016/04/12 08:34:34]

The |this| isn't necessary anymore now that this is a top-level class.

[dgn, 2016/04/12 11:17:15]

Done.

+            Log.d(TAG, "Thumbnail already fetched, skipping download.");
+            cancel(true);
+        }
+    }
+
+    @Override
+    protected Bitmap doInBackground(Void... params) {
+        assert this.mSnippetArticle.getThumbnailBitmap() == null;
+
+        if (this.mSnippetArticle.mThumbnailUrl.isEmpty()) {
+            Log.e(TAG, "Could not get image thumbnail due to empty URL");
+            return null;
+        }
+
+        InputStream is = null;
+        try {
+            is = (InputStream) new URL(this.mSnippetArticle.mThumbnailUrl).getContent();

[Theresa, 2016/04/11 23:42:27]

Can you use openStream() instead to avoid the cast?

[dgn, 2016/04/12 11:17:14]

Done.

+            Bitmap downloadedBitmap = BitmapFactory.decodeStream(is);
+            Resources res = mThumbnailView.getResources();
+            int targetSize = (int) res.getDimension(R.dimen.snippets_thumbnail_size);

[Theresa, 2016/04/11 23:42:27]

Use res.getDimensionPixelSize() - this will convert to an int for you.

[dgn, 2016/04/12 11:17:14]

Thanks! Done.

+            return ThumbnailUtils.extractThumbnail(downloadedBitmap, targetSize, targetSize,
+                    ThumbnailUtils.OPTIONS_RECYCLE_INPUT);
+        } catch (MalformedURLException e) {
+            Log.e(TAG, "Could not get image thumbnail due to malformed URL", e);
+        } catch (IOException e) {
+            Log.e(TAG, "Could not get image thumbnail", e);
+        } finally {
+            StreamUtil.closeQuietly(is);
+        }
+        return null;
+    }
+
+    @Override
+    protected void onPostExecute(Bitmap thumbnail) {
+        if (thumbnail == null) return; // Nothing to do, we keep the placeholder.
+
+        // Store the bitmap to skip the download task next time we display this snippet.
+        this.mSnippetArticle.setThumbnailBitmap(thumbnail);

[Bernhard Bauer, 2016/04/12 08:34:34]

Hm... Ideally I think I'd like this to be a bit more decoupled from the model
and the view. For example, we could make it so that this task doesn't know about
the classes that use it (mSnippetArticle and mThumbnailView), so it only does
the fetching, then its clients add the callbacks that determine what happens
when we get a bitmap back. See also my comment in line 51: That would also be
something that the client of this class would do.

[dgn, 2016/04/12 11:17:14]

Done. After that there was pretty much nothing but the actual download in this
class so I just moved it to the ViewHolder.

+
+        // Cross-fade between the placeholder and the thumbnail

[Theresa, 2016/04/11 23:42:27]

nit: add a period to the end of this sentence.

[dgn, 2016/04/12 11:17:15]

Done.

+        Drawable[] layers = {mThumbnailView.getDrawable(),
+                new BitmapDrawable(mThumbnailView.getResources(), thumbnail)};
+        TransitionDrawable transitionDrawable = new TransitionDrawable(layers);
+        mThumbnailView.setImageDrawable(transitionDrawable);
+        transitionDrawable.startTransition(FADE_IN_ANIMATION_TIME_MS);
+    }
+}