[Feed] Move ImageLoader url iteration to Java.

This change sets up the ImageLoader to be backed by the
haredImageCache when it becomes available. In the short term, this
change all fixes the order the urls are tried such that we strictly
respect the order provided by the Feed. One downside is that more JNI
calls are made.

Bug: 840578
Change-Id: Ie3309005c995ff9e800add44de6e189752722b78
Reviewed-on: https://chromium-review.googlesource.com/1237333Reviewed-by: Filip Gorski <fgorski@chromium.org>
Reviewed-by: Gang Wu <gangwu@chromium.org>
Commit-Queue: Sky Malice <skym@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593625}

chrome/android/feed/core/java/src/org/chromium/chrome/browser/feed/FeedImageLoader.java

@@ -5,6 +5,7 @@
 package org.chromium.chrome.browser.feed;
 
 import android.content.Context;
+import android.graphics.Bitmap;
 import android.graphics.drawable.BitmapDrawable;
 import android.graphics.drawable.Drawable;
 import android.net.Uri;
@@ -14,13 +15,11 @@ import android.text.TextUtils;
 import com.google.android.libraries.feed.common.functional.Consumer;
 import com.google.android.libraries.feed.host.imageloader.ImageLoaderApi;
 
-import org.chromium.base.Callback;
-import org.chromium.chrome.browser.feed.FeedImageLoaderBridge.ImageResponse;
+import org.chromium.base.ThreadUtils;
 import org.chromium.chrome.browser.profiles.Profile;
 import org.chromium.chrome.browser.suggestions.ThumbnailGradient;
 
-import java.util.ArrayList;
-import java.util.HashMap;
+import java.util.Iterator;
 import java.util.List;
 
 /**
@@ -66,106 +65,87 @@ public class FeedImageLoader implements ImageLoaderApi {
     public void loadDrawable(
             List<String> urls, int widthPx, int heightPx, Consumer<Drawable> consumer) {
         assert mFeedImageLoaderBridge != null;
-        List<String> assetUrls = new ArrayList<>();
-        List<String> networkUrls = new ArrayList<>();
+        loadDrawableWithIter(urls.iterator(), consumer);
+    }
 
-        // Maps position in the networkUrls list to overlay image direction.
-        HashMap<Integer, Integer> overlayImages = new HashMap<>();
+    /** Cleans up FeedImageLoaderBridge. */
+    public void destroy() {
+        assert mFeedImageLoaderBridge != null;
+        mFeedImageLoaderBridge.destroy();
+        mFeedImageLoaderBridge = null;
+    }
+
+    /**
+     * Tries to load the next value in urlsIter, and recursively calls itself on failure to
+     * continue processing. Being recursive allows resuming after an async call across the bridge.
+     *
+     * @param urlsIter The stateful iterator of all urls to load. Each call removes one value.
+     * @param consumer The callback to be given the first successful image.
+     */
+    private void loadDrawableWithIter(Iterator<String> urlsIter, Consumer<Drawable> consumer) {
+        assert mFeedImageLoaderBridge != null;
+        if (!urlsIter.hasNext()) {
+            // Post to ensure callback is not run synchronously.
+            ThreadUtils.postOnUiThread(() -> consumer.accept(null));
+            return;
+        }
 
-        // Since loading APK resource("asset://"") only can be done in Java side, we filter out
-        // asset urls, and pass the other URLs to C++ side. This will change the order of |urls|,
-        // because we will process asset:// URLs after network URLs, but once
-        // https://crbug.com/840578 resolved, we can process URLs ordering same as |urls|.
-        for (String url : urls) {
+        String url = urlsIter.next();
         if (url.startsWith(ASSET_PREFIX)) {
-                assetUrls.add(url);
+            Drawable drawable = getAssetDrawable(url);
+            if (drawable == null) {
+                loadDrawableWithIter(urlsIter, consumer);
+            } else {
+                // Post to ensure callback is not run synchronously.
+                ThreadUtils.postOnUiThread(() -> consumer.accept(drawable));
+            }
         } else if (url.startsWith(OVERLAY_IMAGE_PREFIX)) {
             Uri uri = Uri.parse(url);
-
+            int direction = overlayDirection(uri);
             String sourceUrl = uri.getQueryParameter(OVERLAY_IMAGE_URL_PARAM);
-                if (!TextUtils.isEmpty(sourceUrl)) {
-                    networkUrls.add(sourceUrl);
-                    addOverlayDirectionToMap(overlayImages, networkUrls.size() - 1, uri);
+            assert !TextUtils.isEmpty(sourceUrl) : "Overlay image source URL empty";
+            mFeedImageLoaderBridge.fetchImage(sourceUrl, (Bitmap bitmap) -> {
+                if (bitmap == null) {
+                    loadDrawableWithIter(urlsIter, consumer);
                 } else {
-                    assert false : "Overlay image source URL empty";
+                    consumer.accept(ThumbnailGradient.createDrawableWithGradientIfNeeded(
+                            bitmap, direction, mActivityContext.getResources()));
                 }
+            });
         } else {
-                // Assume this is a regular web image.
-                networkUrls.add(url);
-            }
-        }
-
-        if (networkUrls.size() == 0) {
-            Drawable drawable = getAssetDrawable(assetUrls);
-            consumer.accept(drawable);
-            return;
-        }
-
-        mFeedImageLoaderBridge.fetchImage(networkUrls, new Callback<ImageResponse>() {
-            @Override
-            public void onResult(ImageResponse response) {
-                if (response.bitmap != null) {
-                    Drawable drawable;
-                    if (overlayImages.containsKey(response.imagePositionInList)) {
-                        drawable = ThumbnailGradient.createDrawableWithGradientIfNeeded(
-                                response.bitmap, overlayImages.get(response.imagePositionInList),
-                                mActivityContext.getResources());
+            mFeedImageLoaderBridge.fetchImage(url, (Bitmap bitmap) -> {
+                if (bitmap == null) {
+                    loadDrawableWithIter(urlsIter, consumer);
                 } else {
-                        drawable = new BitmapDrawable(
-                                mActivityContext.getResources(), response.bitmap);
-                    }
-
-                    consumer.accept(drawable);
-                    return;
-                }
-
-                // Since no image was available for downloading over the network, attempt to load a
-                // drawable locally.
-                Drawable drawable = getAssetDrawable(assetUrls);
-                consumer.accept(drawable);
+                    consumer.accept(new BitmapDrawable(mActivityContext.getResources(), bitmap));
                 }
             });
         }
-
-    /** Cleans up FeedImageLoaderBridge. */
-    public void destroy() {
-        assert mFeedImageLoaderBridge != null;
-
-        mFeedImageLoaderBridge.destroy();
-        mFeedImageLoaderBridge = null;
     }
 
-    private Drawable getAssetDrawable(List<String> assetUrls) {
-        for (String url : assetUrls) {
+    /**
+     * @param url The fully qualified name of the resource.
+     * @return The resource as a Drawable on success, null otherwise.
+     */
+    private Drawable getAssetDrawable(String url) {
         String resourceName = url.substring(ASSET_PREFIX.length());
-            int resourceId = mActivityContext.getResources().getIdentifier(
+        int id = mActivityContext.getResources().getIdentifier(
                 resourceName, DRAWABLE_RESOURCE_TYPE, mActivityContext.getPackageName());
-            if (resourceId != 0) {
-                Drawable drawable = AppCompatResources.getDrawable(mActivityContext, resourceId);
-                if (drawable != null) {
-                    return drawable;
-                }
-            }
-        }
-        return null;
+        return id == 0 ? null : AppCompatResources.getDrawable(mActivityContext, id);
     }
 
     /**
-     * Determine where the thumbnail is located in the card using the "direction" param and add it
-     * to the provided HashMap.
-     * @param overlayImageMap The HashMap used to store the overlay direction.
-     * @param key The key for the overlay image.
+     * Returns where the thumbnail is located in the card using the "direction" query param.
      * @param overlayImageUri The URI for the overlay image.
+     * @return The direction in which the thumbnail is located relative to the card.
      */
-    private void addOverlayDirectionToMap(
-            HashMap<Integer, Integer> overlayImageMap, int key, Uri overlayImageUri) {
+    private int overlayDirection(Uri overlayImageUri) {
         String direction = overlayImageUri.getQueryParameter(OVERLAY_IMAGE_DIRECTION_PARAM);
-        if (TextUtils.equals(direction, OVERLAY_IMAGE_DIRECTION_START)) {
-            overlayImageMap.put(key, ThumbnailGradient.ThumbnailLocation.START);
-        } else if (TextUtils.equals(direction, OVERLAY_IMAGE_DIRECTION_END)) {
-            overlayImageMap.put(key, ThumbnailGradient.ThumbnailLocation.END);
-        } else {
-            assert false : "Overlay image direction must be either start or end";
-        }
+        assert TextUtils.equals(direction, OVERLAY_IMAGE_DIRECTION_START)
+                || TextUtils.equals(direction, OVERLAY_IMAGE_DIRECTION_END)
+            : "Overlay image direction must be either start or end";
+        return TextUtils.equals(direction, OVERLAY_IMAGE_DIRECTION_START)
+                ? ThumbnailGradient.ThumbnailLocation.START
+                : ThumbnailGradient.ThumbnailLocation.END;
     }
 }

chrome/android/feed/core/java/src/org/chromium/chrome/browser/feed/FeedImageLoaderBridge.java

@@ -7,12 +7,9 @@ package org.chromium.chrome.browser.feed;
 import android.graphics.Bitmap;
 
 import org.chromium.base.Callback;
-import org.chromium.base.annotations.CalledByNative;
 import org.chromium.base.annotations.JNINamespace;
 import org.chromium.chrome.browser.profiles.Profile;
 
-import java.util.List;
-
 /**
  * Provides access to native implementations of image loader.
  */
@@ -46,32 +43,14 @@ public class FeedImageLoaderBridge {
      * Fetches images for feed. A {@code null} Bitmap is returned if no image is available. The
      * callback is never called synchronously.
      */
-    public void fetchImage(List<String> urls, Callback<ImageResponse> callback) {
+    public void fetchImage(String url, Callback<Bitmap> callback) {
         assert mNativeFeedImageLoaderBridge != 0;
-
-        String[] urlsArray = urls.toArray(new String[urls.size()]);
-
-        nativeFetchImage(mNativeFeedImageLoaderBridge, urlsArray, callback);
-    }
-
-    @CalledByNative
-    public static ImageResponse createImageResponse(int imagePositionInList, Bitmap bitmap) {
-        return new ImageResponse(imagePositionInList, bitmap);
-    }
-
-    static class ImageResponse {
-        public int imagePositionInList;
-        public Bitmap bitmap;
-
-        public ImageResponse(int imagePositionInList, Bitmap bitmap) {
-            this.imagePositionInList = imagePositionInList;
-            this.bitmap = bitmap;
-        }
+        nativeFetchImage(mNativeFeedImageLoaderBridge, url, callback);
     }
 
     // Native methods
     private native long nativeInit(Profile profile);
     private native void nativeDestroy(long nativeFeedImageLoaderBridge);
     private native void nativeFetchImage(
-            long nativeFeedImageLoaderBridge, String[] urls, Callback<ImageResponse> callback);
+            long nativeFeedImageLoaderBridge, String url, Callback<Bitmap> callback);
 }

chrome/browser/android/feed/feed_image_loader_bridge.cc

@@ -7,12 +7,11 @@
 #include <jni.h>
 
 #include <string>
-#include <vector>
+#include <utility>
 
 #include "base/android/callback_android.h"
-#include "base/android/jni_array.h"
+#include "base/android/jni_string.h"
 #include "base/bind.h"
-#include "base/threading/thread_task_runner_handle.h"
 #include "chrome/browser/android/feed/feed_host_service_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_android.h"
@@ -22,7 +21,6 @@
 #include "ui/gfx/android/java_bitmap.h"
 #include "ui/gfx/image/image.h"
 
-using base::android::JavaIntArrayToIntVector;
 using base::android::JavaParamRef;
 using base::android::JavaRef;
 using base::android::ScopedJavaGlobalRef;
@@ -57,31 +55,25 @@ void FeedImageLoaderBridge::Destroy(JNIEnv* env,
 
 void FeedImageLoaderBridge::FetchImage(JNIEnv* j_env,
                                        const JavaRef<jobject>& j_this,
-                                       const JavaRef<jobjectArray>& j_urls,
+                                       const JavaRef<jstring>& j_url,
                                        const JavaRef<jobject>& j_callback) {
-  std::vector<std::string> urls;
-  base::android::AppendJavaStringArrayToStringVector(j_env, j_urls.obj(),
-                                                     &urls);
-
   ScopedJavaGlobalRef<jobject> callback(j_callback);
+  std::string url = base::android::ConvertJavaStringToUTF8(j_url);
   feed_image_manager_->FetchImage(
-      urls, base::BindOnce(&FeedImageLoaderBridge::OnImageFetched,
+      {std::move(url)},
+      base::BindOnce(&FeedImageLoaderBridge::OnImageFetched,
                      weak_ptr_factory_.GetWeakPtr(), callback));
 }
 
 void FeedImageLoaderBridge::OnImageFetched(
     ScopedJavaGlobalRef<jobject> callback,
     const gfx::Image& image,
-    size_t image_position) {
-  JNIEnv* env = base::android::AttachCurrentThread();
+    size_t ignored) {
   ScopedJavaLocalRef<jobject> j_bitmap;
   if (!image.IsEmpty()) {
     j_bitmap = gfx::ConvertToJavaBitmap(image.ToSkBitmap());
   }
-
-  RunObjectCallbackAndroid(callback,
-                           Java_FeedImageLoaderBridge_createImageResponse(
-                               env, image_position, j_bitmap));
+  RunObjectCallbackAndroid(callback, j_bitmap);
 }
 
 }  // namespace feed

chrome/browser/android/feed/feed_image_loader_bridge.h

@@ -14,9 +14,7 @@ namespace feed {
 class FeedImageManager;
 
 // Native counterpart of FeedImageLoaderBridge.java. Holds non-owning pointers
-// to native implementation, to which operations are delegated. Results are
-// passed back by a single argument callback so
-// base::android::RunObjectCallbackAndroid() can be used. This bridge is
+// to native implementation, to which operations are delegated. This bridge is
 // instantiated, owned, and destroyed from Java.
 class FeedImageLoaderBridge {
  public:
@@ -27,13 +25,13 @@ class FeedImageLoaderBridge {
 
   void FetchImage(JNIEnv* j_env,
                   const base::android::JavaRef<jobject>& j_this,
-                  const base::android::JavaRef<jobjectArray>& j_urls,
+                  const base::android::JavaRef<jstring>& j_url,
                   const base::android::JavaRef<jobject>& j_callback);
 
  private:
   void OnImageFetched(base::android::ScopedJavaGlobalRef<jobject> callback,
                       const gfx::Image& image,
-                      size_t image_position);
+                      size_t ignored);
 
   FeedImageManager* feed_image_manager_;