Consolidate CachedImageFetcher unittests and expand coverage

Did some refactoring to allow for better test support.

TBR=arbesser@google.com

Bug: 1067714
Change-Id: Id18a0e1973c18c1ed9422748d6671cb012ce0f3a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2133174
Commit-Queue: Brandon Wylie <wylieb@chromium.org>
Reviewed-by: Patrick Noland <pnoland@chromium.org>
Cr-Commit-Position: refs/heads/master@{#756400}

chrome/android/features/autofill_assistant/javatests/src/org/chromium/chrome/browser/autofill_assistant/AutofillAssistantUiTestUtil.java

@@ -79,7 +79,7 @@ import jp.tomorrowkey.android.gifplayer.BaseGifImage;
  */
 class AutofillAssistantUiTestUtil {
     /** Image fetcher which synchronously returns a preset image. */
-    static class MockImageFetcher extends ImageFetcher {
+    static class MockImageFetcher extends ImageFetcher.ImageFetcherForTesting {
         private final Bitmap mBitmapToFetch;
         private final BaseGifImage mGifToFetch;
 

chrome/android/junit/src/org/chromium/chrome/browser/image_fetcher/ImageFetcherTest.java

@@ -15,6 +15,7 @@ import android.graphics.Bitmap;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
+import org.mockito.Mock;
 import org.mockito.Mockito;
 import org.mockito.MockitoAnnotations;
 import org.robolectric.annotation.Config;
@@ -37,6 +38,10 @@ public class ImageFetcherTest {
      * Concrete implementation for testing purposes.
      */
     private class ImageFetcherForTest extends ImageFetcher {
+        ImageFetcherForTest(ImageFetcherBridge imageFetcherBridge) {
+            super(imageFetcherBridge);
+        }
+
         @Override
         public void fetchGif(String url, String clientName, Callback<BaseGifImage> callback) {}
 
@@ -56,6 +61,9 @@ public class ImageFetcherTest {
         public void destroy() {}
     }
 
+    @Mock
+    ImageFetcherBridge mBridge;
+
     private final Bitmap mBitmap =
             Bitmap.createBitmap(WIDTH_PX, HEIGHT_PX, Bitmap.Config.ARGB_8888);
 
@@ -64,33 +72,33 @@ public class ImageFetcherTest {
     @Before
     public void setUp() {
         MockitoAnnotations.initMocks(this);
-        mImageFetcher = Mockito.spy(new ImageFetcherForTest());
+        mImageFetcher = Mockito.spy(new ImageFetcherForTest(mBridge));
     }
 
     @Test
     public void testResize() {
-        Bitmap result = ImageFetcher.tryToResizeImage(mBitmap, WIDTH_PX / 2, HEIGHT_PX / 2);
+        Bitmap result = ImageFetcher.resizeImage(mBitmap, WIDTH_PX / 2, HEIGHT_PX / 2);
         assertNotEquals(result, mBitmap);
     }
 
     @Test
     public void testResizeBailsOutIfSizeIsZeroOrLess() {
-        Bitmap result = ImageFetcher.tryToResizeImage(mBitmap, WIDTH_PX - 1, HEIGHT_PX - 1);
+        Bitmap result = ImageFetcher.resizeImage(mBitmap, WIDTH_PX - 1, HEIGHT_PX - 1);
         assertNotEquals(result, mBitmap);
 
-        result = ImageFetcher.tryToResizeImage(mBitmap, 0, HEIGHT_PX);
+        result = ImageFetcher.resizeImage(mBitmap, 0, HEIGHT_PX);
         assertEquals(result, mBitmap);
 
-        result = ImageFetcher.tryToResizeImage(mBitmap, WIDTH_PX, 0);
+        result = ImageFetcher.resizeImage(mBitmap, WIDTH_PX, 0);
         assertEquals(result, mBitmap);
 
-        result = ImageFetcher.tryToResizeImage(mBitmap, 0, 0);
+        result = ImageFetcher.resizeImage(mBitmap, 0, 0);
         assertEquals(result, mBitmap);
 
-        result = ImageFetcher.tryToResizeImage(mBitmap, -1, HEIGHT_PX);
+        result = ImageFetcher.resizeImage(mBitmap, -1, HEIGHT_PX);
         assertEquals(result, mBitmap);
 
-        result = ImageFetcher.tryToResizeImage(mBitmap, WIDTH_PX, -1);
+        result = ImageFetcher.resizeImage(mBitmap, WIDTH_PX, -1);
         assertEquals(result, mBitmap);
     }
 

chrome/android/junit/src/org/chromium/chrome/browser/image_fetcher/InMemoryCachedImageFetcherTest.java

@@ -69,17 +69,17 @@ public class InMemoryCachedImageFetcherTest {
     @Before
     public void setUp() {
         MockitoAnnotations.initMocks(this);
-        ImageFetcherBridge.setupForTesting(mBridge);
+        doReturn(mBridge).when(mCachedImageFetcher).getImageFetcherBridge();
+
         mReferencePool = new DiscardableReferencePool();
         mBitmapCache = new BitmapCache(mReferencePool, DEFAULT_CACHE_SIZE);
         mInMemoryCachedImageFetcher =
                 spy(new InMemoryCachedImageFetcher(mBitmapCache, mCachedImageFetcher));
     }
 
-    public void answerFetch(Bitmap bitmap, CachedImageFetcher cachedImageFetcher,
-            boolean deleteBitmapCacheOnFetch) {
+    public void answerFetch(Bitmap bitmap, boolean deleteBitmapCacheOnFetch) {
         mInMemoryCachedImageFetcher =
-                spy(new InMemoryCachedImageFetcher(mBitmapCache, cachedImageFetcher));
+                spy(new InMemoryCachedImageFetcher(mBitmapCache, mCachedImageFetcher));
         // clang-format off
         doAnswer((InvocationOnMock invocation) -> {
             if (deleteBitmapCacheOnFetch) {
@@ -98,7 +98,7 @@ public class InMemoryCachedImageFetcherTest {
     @Test
     @SmallTest
     public void testFetchImageCachesFirstCall() {
-        answerFetch(mBitmap, mCachedImageFetcher, false);
+        answerFetch(mBitmap, false);
         mInMemoryCachedImageFetcher.fetchImage(
                 URL, UMA_CLIENT_NAME, WIDTH_PX, HEIGHT_PX, mCallback);
         verify(mCallback).onResult(eq(mBitmap));
@@ -119,7 +119,8 @@ public class InMemoryCachedImageFetcherTest {
     @Test
     @SmallTest
     public void testFetchImageReturnsNullWhenFetcherIsNull() {
-        answerFetch(mBitmap, null, false);
+        answerFetch(mBitmap, false);
+        mInMemoryCachedImageFetcher.setImageFetcherForTesting(null);
         doReturn(null)
                 .when(mInMemoryCachedImageFetcher)
                 .tryToGetBitmap(eq(URL), eq(WIDTH_PX), eq(HEIGHT_PX));
@@ -136,7 +137,7 @@ public class InMemoryCachedImageFetcherTest {
     @SmallTest
     public void testFetchImageDoesNotCacheAfterDestroy() {
         try {
-            answerFetch(mBitmap, mCachedImageFetcher, true);
+            answerFetch(mBitmap, true);
 
             // No exception should be thrown here when bitmap cache is null.
             mInMemoryCachedImageFetcher.fetchImage(

chrome/browser/image_fetcher/android/java/src/org/chromium/chrome/browser/image_fetcher/CachedImageFetcher.java

@@ -27,16 +27,55 @@ import jp.tomorrowkey.android.gifplayer.BaseGifImage;
 public class CachedImageFetcher extends ImageFetcher {
     private static final String TAG = "CachedImageFetcher";
 
-    // The native bridge.
-    private ImageFetcherBridge mImageFetcherBridge;
+    static class ImageLoader {
+        /**
+         * Attempt to load an image from disk with the given filepath.
+         *
+         * @param filePath The path to the image on disk (including the filename).
+         * @return The Bitmap that's on disk or null if the there's no file or the decoding failed.
+         */
+        Bitmap tryToLoadImageFromDisk(String filePath) {
+            if (new File(filePath).exists()) {
+                return BitmapFactory.decodeFile(filePath, null);
+            } else {
+                return null;
+            }
+        }
+        /**
+         * Attempt to load a BaseGifImage from disk with the given filepath.
+         *
+         * @param filePath The path to the BaseGifImage on disk (including the filename).
+         * @return The BaseGifImage that's on disk or null if the there's no file or the decoding
+         *         failed.
+         */
+        BaseGifImage tryToLoadGifFromDisk(String filePath) {
+            try {
+                File file = new File(filePath);
+                byte[] fileBytes = new byte[(int) file.length()];
+                FileInputStream fileInputStream = new FileInputStream(filePath);
+
+                int bytesRead = fileInputStream.read(fileBytes);
+                if (bytesRead != fileBytes.length) return null;
+
+                return new BaseGifImage(fileBytes);
+            } catch (IOException e) {
+                Log.w(TAG, "Failed to read: %s", filePath, e);
+                return null;
+            }
+        }
+    }
+
+    private ImageLoader mImageLoader;
 
     /**
      * Creates a CachedImageFetcher with the given bridge.
      *
-     * @param bridge Bridge used to interact with native.
+     * @param imageFetcherBridge Bridge used to interact with native.
+     * @param imageLoader Delegate used to load
      */
-    CachedImageFetcher(ImageFetcherBridge bridge) {
-        mImageFetcherBridge = bridge;
+    CachedImageFetcher(ImageFetcherBridge imageFetcherBridge, ImageLoader imageLoader) {
+        super(imageFetcherBridge);
+        mImageLoader = imageLoader;
     }
 
     @Override
@@ -52,8 +91,8 @@ public class CachedImageFetcher extends ImageFetcher {
         long startTimeMillis = System.currentTimeMillis();
         PostTask.postTask(TaskTraits.USER_VISIBLE, () -> {
             // Try to read the gif from disk, then post back to the ui thread.
-            String filePath = mImageFetcherBridge.getFilePath(url);
-            BaseGifImage cachedGif = tryToLoadGifFromDisk(filePath);
+            String filePath = getImageFetcherBridge().getFilePath(url);
+            BaseGifImage cachedGif = mImageLoader.tryToLoadGifFromDisk(filePath);
             PostTask.postTask(UiThreadTaskTraits.USER_VISIBLE, () -> {
                 continueFetchGifAfterDisk(url, clientName, callback, cachedGif, startTimeMillis);
             });
@@ -66,12 +105,12 @@ public class CachedImageFetcher extends ImageFetcher {
         if (cachedGif != null) {
             callback.onResult(cachedGif);
             reportEvent(clientName, ImageFetcherEvent.JAVA_DISK_CACHE_HIT);
-            mImageFetcherBridge.reportCacheHitTime(clientName, startTimeMillis);
+            getImageFetcherBridge().reportCacheHitTime(clientName, startTimeMillis);
         } else {
-            mImageFetcherBridge.fetchGif(
+            getImageFetcherBridge().fetchGif(
                     getConfig(), url, clientName, (BaseGifImage gifFromNative) -> {
                         callback.onResult(gifFromNative);
-                        mImageFetcherBridge.reportTotalFetchTimeFromNative(
+                        getImageFetcherBridge().reportTotalFetchTimeFromNative(
                                 clientName, startTimeMillis);
                     });
         }
@@ -86,8 +125,8 @@ public class CachedImageFetcher extends ImageFetcher {
         long startTimeMillis = System.currentTimeMillis();
         PostTask.postTask(TaskTraits.USER_VISIBLE, () -> {
             // Try to read the bitmap from disk, then post back to the ui thread.
-            String filePath = mImageFetcherBridge.getFilePath(url);
-            Bitmap bitmap = tryToLoadImageFromDisk(filePath);
+            String filePath = getImageFetcherBridge().getFilePath(url);
+            Bitmap bitmap = mImageLoader.tryToLoadImageFromDisk(filePath);
             PostTask.postTask(UiThreadTaskTraits.USER_VISIBLE, () -> {
                 continueFetchImageAfterDisk(
                         url, clientName, width, height, callback, bitmap, startTimeMillis);
@@ -99,14 +138,16 @@ public class CachedImageFetcher extends ImageFetcher {
     void continueFetchImageAfterDisk(String url, String clientName, int width, int height,
             Callback<Bitmap> callback, Bitmap cachedBitmap, long startTimeMillis) {
         if (cachedBitmap != null) {
+            // In case the image's dimensions on disk don't match the desired dimensions.
+            cachedBitmap = ImageFetcher.resizeImage(cachedBitmap, width, height);
             callback.onResult(cachedBitmap);
             reportEvent(clientName, ImageFetcherEvent.JAVA_DISK_CACHE_HIT);
-            mImageFetcherBridge.reportCacheHitTime(clientName, startTimeMillis);
+            getImageFetcherBridge().reportCacheHitTime(clientName, startTimeMillis);
         } else {
-            mImageFetcherBridge.fetchImage(
+            getImageFetcherBridge().fetchImage(
                     getConfig(), url, clientName, width, height, (Bitmap bitmapFromNative) -> {
                         callback.onResult(bitmapFromNative);
-                        mImageFetcherBridge.reportTotalFetchTimeFromNative(
+                        getImageFetcherBridge().reportTotalFetchTimeFromNative(
                                 clientName, startTimeMillis);
                     });
         }
@@ -119,31 +160,4 @@ public class CachedImageFetcher extends ImageFetcher {
     public @ImageFetcherConfig int getConfig() {
         return ImageFetcherConfig.DISK_CACHE_ONLY;
     }
-
-    /** Wrapper function to decode a file for disk, useful for testing. */
-    @VisibleForTesting
-    Bitmap tryToLoadImageFromDisk(String filePath) {
-        if (new File(filePath).exists()) {
-            return BitmapFactory.decodeFile(filePath, null);
-        } else {
-            return null;
-        }
-    }
-
-    @VisibleForTesting
-    BaseGifImage tryToLoadGifFromDisk(String filePath) {
-        try {
-            File file = new File(filePath);
-            byte[] fileBytes = new byte[(int) file.length()];
-            FileInputStream fileInputStream = new FileInputStream(filePath);
-
-            int bytesRead = fileInputStream.read(fileBytes);
-            if (bytesRead != fileBytes.length) return null;
-
-            return new BaseGifImage(fileBytes);
-        } catch (IOException e) {
-            Log.w(TAG, "Failed to read: %s", filePath, e);
-            return null;
-        }
-    }
 }

chrome/browser/image_fetcher/android/java/src/org/chromium/chrome/browser/image_fetcher/ImageFetcher.java

@@ -27,6 +27,31 @@ public abstract class ImageFetcher {
     public static final String FEED_UMA_CLIENT_NAME = "Feed";
     public static final String NTP_ANIMATED_LOGO_UMA_CLIENT_NAME = "NewTabPageAnimatedLogo";
 
+    /** Base class that can be used for testing. */
+    public abstract static class ImageFetcherForTesting extends ImageFetcher {
+        public ImageFetcherForTesting() {}
+    }
+
+    // Singleton ImageFetcherBridge.
+    private ImageFetcherBridge mImageFetcherBridge;
+
+    /** Copy-constructor to support composite instances of ImageFetcher. */
+    public ImageFetcher(ImageFetcher imageFetcher) {
+        mImageFetcherBridge = imageFetcher.getImageFetcherBridge();
+    }
+
+    /** Base constructor that takes an ImageFetcherBridge. */
+    public ImageFetcher(ImageFetcherBridge imageFetcherBridge) {
+        mImageFetcherBridge = imageFetcherBridge;
+    }
+
+    /** Test constructor */
+    private ImageFetcher() {}
+
+    protected ImageFetcherBridge getImageFetcherBridge() {
+        return mImageFetcherBridge;
+    }
+
     /**
      * Try to resize the given image if the conditions are met.
      *
@@ -37,7 +62,7 @@ public abstract class ImageFetcher {
      * @return The resized image, or the original image if the  conditions aren't met.
      */
     @VisibleForTesting
-    public static Bitmap tryToResizeImage(@Nullable Bitmap bitmap, int width, int height) {
+    public static Bitmap resizeImage(@Nullable Bitmap bitmap, int width, int height) {
         if (bitmap != null && width > 0 && height > 0 && bitmap.getWidth() != width
                 && bitmap.getHeight() != height) {
             /* The resizing rules are the as follows:
@@ -60,7 +85,7 @@ public abstract class ImageFetcher {
      * @param eventId The event to be reported
      */
     public void reportEvent(String clientName, @ImageFetcherEvent int eventId) {
-        ImageFetcherBridge.getInstance().reportEvent(clientName, eventId);
+        mImageFetcherBridge.reportEvent(clientName, eventId);
     }
 
     /**

chrome/browser/image_fetcher/android/java/src/org/chromium/chrome/browser/image_fetcher/ImageFetcherBridge.java

@@ -100,7 +100,7 @@ public class ImageFetcherBridge {
         assert mNativeImageFetcherBridge != 0 : "fetchImage called after destroy";
         ImageFetcherBridgeJni.get().fetchImage(mNativeImageFetcherBridge, ImageFetcherBridge.this,
                 config, url, clientName, (bitmap) -> {
-                    callback.onResult(ImageFetcher.tryToResizeImage(bitmap, width, height));
+                    callback.onResult(ImageFetcher.resizeImage(bitmap, width, height));
                 });
     }
 

chrome/browser/image_fetcher/android/java/src/org/chromium/chrome/browser/image_fetcher/ImageFetcherFactory.java

@@ -62,7 +62,8 @@ public class ImageFetcherFactory {
                 return sNetworkImageFetcher;
             case ImageFetcherConfig.DISK_CACHE_ONLY:
                 if (sCachedImageFetcher == null) {
-                    sCachedImageFetcher = new CachedImageFetcher(imageFetcherBridge);
+                    sCachedImageFetcher = new CachedImageFetcher(
+                            imageFetcherBridge, new CachedImageFetcher.ImageLoader());
                 }
                 return sCachedImageFetcher;
             case ImageFetcherConfig.IN_MEMORY_ONLY:

chrome/browser/image_fetcher/android/java/src/org/chromium/chrome/browser/image_fetcher/InMemoryCachedImageFetcher.java

@@ -6,6 +6,7 @@ package org.chromium.chrome.browser.image_fetcher;
 
 import android.graphics.Bitmap;
 
+import androidx.annotation.NonNull;
 import androidx.annotation.Nullable;
 import androidx.annotation.VisibleForTesting;
 
@@ -34,7 +35,9 @@ public class InMemoryCachedImageFetcher extends ImageFetcher {
      * @param imageFetcher The image fetcher to back this. Must be Cached/NetworkImageFetcher.
      * @param referencePool Pool used to discard references when under memory pressure.
      */
-    InMemoryCachedImageFetcher(ImageFetcher imageFetcher, DiscardableReferencePool referencePool) {
+    InMemoryCachedImageFetcher(
+            @NonNull ImageFetcher imageFetcher, @NonNull DiscardableReferencePool referencePool) {
+        super(imageFetcher);
         init(imageFetcher, new BitmapCache(referencePool, determineCacheSize(DEFAULT_CACHE_SIZE)));
     }
 
@@ -45,8 +48,9 @@ public class InMemoryCachedImageFetcher extends ImageFetcher {
      * @param cacheSize The cache size to use (in bytes), may be smaller depending on the device's
      *         memory.
      */
-    InMemoryCachedImageFetcher(
-            ImageFetcher imageFetcher, DiscardableReferencePool referencePool, int cacheSize) {
+    InMemoryCachedImageFetcher(@NonNull ImageFetcher imageFetcher,
+            @NonNull DiscardableReferencePool referencePool, int cacheSize) {
+        super(imageFetcher);
         init(imageFetcher, new BitmapCache(referencePool, determineCacheSize(cacheSize)));
     }
 
@@ -187,8 +191,14 @@ public class InMemoryCachedImageFetcher extends ImageFetcher {
 
     /** Test constructor. */
     @VisibleForTesting
-    InMemoryCachedImageFetcher(BitmapCache bitmapCache, ImageFetcher imageFetcher) {
+    InMemoryCachedImageFetcher(
+            @NonNull BitmapCache bitmapCache, @NonNull ImageFetcher imageFetcher) {
+        super(imageFetcher);
         mBitmapCache = bitmapCache;
         mImageFetcher = imageFetcher;
     }
+
+    void setImageFetcherForTesting(ImageFetcher imageFetcher) {
+        mImageFetcher = imageFetcher;
+    }
 }

chrome/browser/image_fetcher/android/java/src/org/chromium/chrome/browser/image_fetcher/NetworkImageFetcher.java

@@ -14,16 +14,13 @@ import jp.tomorrowkey.android.gifplayer.BaseGifImage;
  * Image Fetcher implementation that fetches from the network.
  */
 public class NetworkImageFetcher extends ImageFetcher {
-    // The native bridge.
-    private ImageFetcherBridge mImageFetcherBridge;
-
     /**
      * Creates a NetworkImageFetcher.
      *
-     * @param bridge Bridge used to interact with native.
+     * @param imageFetcherBridge Bridge used to interact with native.
      */
-    NetworkImageFetcher(ImageFetcherBridge bridge) {
-        mImageFetcherBridge = bridge;
+    NetworkImageFetcher(ImageFetcherBridge imageFetcherBridge) {
+        super(imageFetcherBridge);
     }
 
     @Override
@@ -33,17 +30,18 @@ public class NetworkImageFetcher extends ImageFetcher {
 
     @Override
     public void fetchGif(String url, String clientName, Callback<BaseGifImage> callback) {
-        mImageFetcherBridge.fetchGif(getConfig(), url, clientName, callback);
+        getImageFetcherBridge().fetchGif(getConfig(), url, clientName, callback);
     }
 
     @Override
     public void fetchImage(
             String url, String clientName, int width, int height, Callback<Bitmap> callback) {
         long startTimeMillis = System.currentTimeMillis();
-        mImageFetcherBridge.fetchImage(
+        getImageFetcherBridge().fetchImage(
                 getConfig(), url, clientName, width, height, (Bitmap bitmapFromNative) -> {
                     callback.onResult(bitmapFromNative);
-                    mImageFetcherBridge.reportTotalFetchTimeFromNative(clientName, startTimeMillis);
+                    getImageFetcherBridge().reportTotalFetchTimeFromNative(
+                            clientName, startTimeMillis);
                 });
     }