[Screenshots] Add a test for retry logic in ScreenshotCoordinator.

Bug: 1093377
Change-Id: I4df1bf7bdfe6efe0cbf09b20b9815e7f9eebfeea
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2364655
Commit-Queue: Travis Skare <skare@chromium.org>
Reviewed-by: Matthew Jones <mdjones@chromium.org>
Reviewed-by: Tommy Nyquist <nyquist@chromium.org>
Reviewed-by: Jeffrey Cohen <jeffreycohen@chromium.org>
Reviewed-by: Alex Chau <alexchau@chromium.org>
Cr-Commit-Position: refs/heads/master@{#808478}

chrome/android/BUILD.gn

@@ -779,6 +779,8 @@ junit_binary("chrome_junit_tests") {
     "//chrome/android/features/keyboard_accessory:internal_java",
     "//chrome/android/features/start_surface/internal:java",
     "//chrome/android/features/tab_ui:java",
+    "//chrome/android/modules/image_editor/provider:java",
+    "//chrome/android/modules/image_editor/public:java",
     "//chrome/android/webapk/libs/client:client_java",
     "//chrome/android/webapk/libs/common:common_java",
     "//chrome/android/webapk/libs/common:splash_java",
@@ -793,6 +795,7 @@ junit_binary("chrome_junit_tests") {
     "//chrome/browser/flags:flags_junit_tests",
     "//chrome/browser/flags:java",
     "//chrome/browser/fullscreen/android:java",
+    "//chrome/browser/image_editor/public:java",
     "//chrome/browser/image_fetcher:java",
     "//chrome/browser/optimization_guide/android:junit_tests",
     "//chrome/browser/performance_hints/android:java",

chrome/android/java/src/org/chromium/chrome/browser/app/ChromeActivity.java

@@ -1347,7 +1347,7 @@ public abstract class ChromeActivity<C extends ChromeActivityComponent>
         }
 
         if (ChromeFeatureList.isEnabled(ChromeFeatureList.CHROME_SHARE_SCREENSHOT)) {
-            ImageEditorModuleProvider.maybeInstallModuleDeferred();
+            ImageEditorModuleProvider.get().maybeInstallModuleDeferred();
         }
 
         super.finishNativeInitialization();

chrome/android/java/src/org/chromium/chrome/browser/screenshot/OWNERS

-file://chrome/browser/sharing/OWNERS
+file://components/send_tab_to_self/OWNERS
 
 # COMPONENT: UI>Browser>Sharing
 # OS: Android

chrome/android/modules/image_editor/provider/BUILD.gn

@@ -10,5 +10,6 @@ android_library("java") {
   deps = [
     "//chrome/android/modules/image_editor/public:java",
     "//components/module_installer/android:module_installer_java",
+    "//third_party/android_deps:androidx_annotation_annotation_java",
   ]
 }

chrome/android/modules/image_editor/provider/java/src/org/chromium/chrome/modules/image_editor/ImageEditorModuleProvider.java

@@ -4,16 +4,39 @@
 
 package org.chromium.chrome.modules.image_editor;
 
+import androidx.annotation.VisibleForTesting;
+
 import org.chromium.components.module_installer.engine.InstallListener;
 
 /**
  * Installs and loads the module.
  */
 public class ImageEditorModuleProvider {
+    private static final Object INSTANCE_LOCK = new Object();
+    private static ImageEditorModuleProvider sInstance;
+
+    /** @return The singleton instance of {@link ImageEditorModuleProvider}. */
+    public static ImageEditorModuleProvider get() {
+        synchronized (INSTANCE_LOCK) {
+            if (sInstance == null) {
+                sInstance = new ImageEditorModuleProvider();
+            }
+        }
+        return sInstance;
+    }
+
+    /** Sets the singleton instance of {@link ImageEditorModuleProvider} for testing. */
+    @VisibleForTesting
+    public static void setInstanceForTesting(ImageEditorModuleProvider provider) {
+        synchronized (INSTANCE_LOCK) {
+            sInstance = provider;
+        }
+    }
+
     /**
      * Returns true if the module is installed.
      */
-    public static boolean isModuleInstalled() {
+    public boolean isModuleInstalled() {
         return ImageEditorModule.isInstalled();
     }
 
@@ -21,7 +44,7 @@ public class ImageEditorModuleProvider {
      * Requests deferred installation of the module, i.e. when on unmetered network connection and
      * device is charging.
      */
-    public static void maybeInstallModuleDeferred() {
+    public void maybeInstallModuleDeferred() {
         if (isModuleInstalled()) {
             return;
         }
@@ -34,7 +57,7 @@ public class ImageEditorModuleProvider {
      *
      * @param listener Called when the install has finished.
      */
-    public static void maybeInstallModule(InstallListener listener) {
+    public void maybeInstallModule(InstallListener listener) {
         if (isModuleInstalled()) {
             listener.onComplete(false);
             return;
@@ -51,7 +74,7 @@ public class ImageEditorModuleProvider {
      *
      * TODO(crbug/1024586): Return fallback editor?
      */
-    public static ImageEditorProvider getImageEditorProvider() {
+    public ImageEditorProvider getImageEditorProvider() {
         return ImageEditorModule.getImpl();
     }
 }

chrome/browser/share/DEPS

@@ -13,6 +13,7 @@ include_rules = [
   "+chrome/android/java/src/org/chromium/chrome/browser/modules/ModuleInstallUi.java",
   "+chrome/android/java/src/org/chromium/chrome/browser/multiwindow/MultiWindowUtils.java",
   "+chrome/android/java/src/org/chromium/chrome/browser/notifications",
+  "+chrome/android/java/src/org/chromium/chrome/browser/screenshot/EditorScreenshotSource.java",
   "+chrome/android/java/src/org/chromium/chrome/browser/screenshot/EditorScreenshotTask.java",
   "+chrome/android/java/src/org/chromium/chrome/browser/share",
   "+chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java",

chrome/browser/share/android/java/src/org/chromium/chrome/browser/share/screenshot/ScreenshotCoordinator.java

@@ -7,9 +7,12 @@ package org.chromium.chrome.browser.share.screenshot;
 import android.app.Activity;
 import android.graphics.Bitmap;
 
+import androidx.annotation.VisibleForTesting;
+
 import org.chromium.chrome.R;
 import org.chromium.chrome.browser.image_editor.ImageEditorDialogCoordinator;
 import org.chromium.chrome.browser.modules.ModuleInstallUi;
+import org.chromium.chrome.browser.screenshot.EditorScreenshotSource;
 import org.chromium.chrome.browser.screenshot.EditorScreenshotTask;
 import org.chromium.chrome.browser.share.share_sheet.ChromeOptionShareCallback;
 import org.chromium.chrome.browser.tab.Tab;
@@ -21,23 +24,54 @@ import org.chromium.components.browser_ui.bottomsheet.BottomSheetController;
  */
 public class ScreenshotCoordinator {
     // Maximum number of attempts to install the DFM per session.
-    private static final int MAX_INSTALL_ATTEMPTS = 5;
+    @VisibleForTesting
+    static final int MAX_INSTALL_ATTEMPTS = 5;
     private static int sInstallAttempts;
 
     private final Activity mActivity;
     private final Tab mTab;
+    private final ScreenshotShareSheetDialog mDialog;
     private final ChromeOptionShareCallback mChromeOptionShareCallback;
     private final BottomSheetController mBottomSheetController;
 
-    private EditorScreenshotTask mScreenshotTask;
+    private EditorScreenshotSource mScreenshotSource;
     private Bitmap mScreenshot;
 
+    /**
+     * Constructs a new ScreenshotCoordinator which may launch the editor, or a fallback.
+     *
+     * @param activity The parent activity.
+     * @param tab The Tab which contains the content to share.
+     * @param screenshotSource The Source interface to use to take a screenshot.
+     * @param chromeOptionShareCallback An interface to share sheet APIs.
+     */
+    public ScreenshotCoordinator(Activity activity, Tab tab,
+            ChromeOptionShareCallback chromeOptionShareCallback,
+            BottomSheetController sheetController) {
+        this(activity, tab, new EditorScreenshotTask(activity, sheetController),
+                new ScreenshotShareSheetDialog(), chromeOptionShareCallback, sheetController);
+    }
+
+    /**
+     * Constructor for testing and inner construction.
+     *
+     * @param activity The parent activity.
+     * @param tab The Tab which contains the content to share.
+     * @param screenshotSource The Source interface to use to take a screenshot.
+     * @param dialog The Share Sheet dialog to use as fallback.
+     * @param chromeOptionShareCallback An interface to share sheet APIs.
+     * @param imageEditorModuleProvider An interface to install and/or instantiate the image editor.
+     */
+    @VisibleForTesting
     public ScreenshotCoordinator(Activity activity, Tab tab,
+            EditorScreenshotSource screenshotSource, ScreenshotShareSheetDialog dialog,
             ChromeOptionShareCallback chromeOptionShareCallback,
             BottomSheetController sheetController) {
         mActivity = activity;
         mTab = tab;
+        mDialog = dialog;
         mChromeOptionShareCallback = chromeOptionShareCallback;
+        mScreenshotSource = screenshotSource;
         mBottomSheetController = sheetController;
     }
 
@@ -45,9 +79,8 @@ public class ScreenshotCoordinator {
      * Takes a screenshot of the current tab and attempts to launch the screenshot image editor.
      */
     public void captureScreenshot() {
-        mScreenshotTask = new EditorScreenshotTask(mActivity, mBottomSheetController);
-        mScreenshotTask.capture(() -> {
-            mScreenshot = mScreenshotTask.getScreenshot();
+        mScreenshotSource.capture(() -> {
+            mScreenshot = mScreenshotSource.getScreenshot();
             if (mScreenshot == null) {
                 // TODO(crbug/1024586): Show error message
             } else {
@@ -62,7 +95,7 @@ public class ScreenshotCoordinator {
      * directly opens the screenshot sharesheet instead.
      */
     private void handleScreenshot() {
-        if (ImageEditorModuleProvider.isModuleInstalled()) {
+        if (ImageEditorModuleProvider.get().isModuleInstalled()) {
             launchEditor();
         } else if (sInstallAttempts < MAX_INSTALL_ATTEMPTS) {
             sInstallAttempts++;
@@ -76,7 +109,8 @@ public class ScreenshotCoordinator {
      * Launches the screenshot image editor.
      */
     private void launchEditor() {
-        ImageEditorDialogCoordinator editor = ImageEditorModuleProvider.getImageEditorProvider()
+        ImageEditorDialogCoordinator editor = ImageEditorModuleProvider.get()
+                                                      .getImageEditorProvider()
                                                       .getImageEditorDialogCoordinator();
         editor.launchEditor(mActivity, mScreenshot, mTab, mChromeOptionShareCallback);
         mScreenshot = null;
@@ -87,7 +121,7 @@ public class ScreenshotCoordinator {
      */
     private void launchSharesheet() {
         ScreenshotShareSheetDialogCoordinator shareSheet =
-                new ScreenshotShareSheetDialogCoordinator(mActivity, mScreenshot, mTab,
+                new ScreenshotShareSheetDialogCoordinator(mActivity, mDialog, mScreenshot, mTab,
                         mChromeOptionShareCallback, this::retryInstallEditor);
         shareSheet.showShareSheet();
         mScreenshot = null;
@@ -123,7 +157,7 @@ public class ScreenshotCoordinator {
                 });
         ui.showInstallStartUi();
 
-        ImageEditorModuleProvider.maybeInstallModule((success) -> {
+        ImageEditorModuleProvider.get().maybeInstallModule((success) -> {
             if (success) {
                 ui.showInstallSuccessUi();
                 if (onSuccessRunnable != null) {

chrome/browser/share/android/java/src/org/chromium/chrome/browser/share/screenshot/ScreenshotShareSheetDialogCoordinator.java

@@ -8,6 +8,8 @@ import android.app.Activity;
 import android.app.FragmentManager;
 import android.graphics.Bitmap;
 
+import androidx.annotation.VisibleForTesting;
+
 import org.chromium.base.Callback;
 import org.chromium.chrome.browser.share.share_sheet.ChromeOptionShareCallback;
 import org.chromium.chrome.browser.tab.Tab;
@@ -22,25 +24,27 @@ public class ScreenshotShareSheetDialogCoordinator {
     /**
      * Constructs a new Screenshot Dialog.
      *
-     * @param context The context to use for user permissions.
-     * @param screenshot The screenshot to be shared.
-     * @param tab The shared tab.
-     * @param chromeOptionShareCallback the callback to trigger on share.
-     * @param installCallback the callback to trigger on install.
+     * @param activity The parent activity.
+     * @param dialog The Share Sheet dialog to use as fallback.
+     * @param screenshot The Bitmap of the screenshot to share.
+     * @param tab The Tab which contains the content to share.
+     * @param shareCallback Callback called when falling back to the share sheet.
+     * @param installCallback Callback called when the image editor is installed and run.
      */
-    public ScreenshotShareSheetDialogCoordinator(Activity activity, Bitmap screenshot, Tab tab,
-            ChromeOptionShareCallback chromeOptionShareCallback,
-            Callback<Runnable> installCallback) {
+    public ScreenshotShareSheetDialogCoordinator(Activity activity,
+            ScreenshotShareSheetDialog dialog, Bitmap screenshot, Tab tab,
+            ChromeOptionShareCallback shareCallback, Callback<Runnable> installCallback) {
         mFragmentManager = activity.getFragmentManager();
+        mDialog = dialog;
         mScreenshot = screenshot;
-        mDialog = new ScreenshotShareSheetDialog();
-        mDialog.init(mScreenshot, tab, chromeOptionShareCallback, installCallback);
+        mDialog.init(mScreenshot, tab, shareCallback, installCallback);
     }
 
     /**
      * Show the main share sheet dialog.
      */
-    protected void showShareSheet() {
+    @VisibleForTesting(otherwise = VisibleForTesting.PROTECTED)
+    public void showShareSheet() {
         mDialog.show(mFragmentManager, null);
     }
 }

chrome/browser/share/android/javatests/src/org/chromium/chrome/browser/share/screenshot/ScreenshotCoordinatorTest.java

+// Copyright 2020 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.share.screenshot;
+
+import static org.mockito.AdditionalAnswers.answerVoid;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.doAnswer;
+import static org.mockito.Mockito.doNothing;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import android.app.Activity;
+import android.app.FragmentManager;
+import android.graphics.Bitmap;
+
+import androidx.annotation.Nullable;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.MockitoAnnotations;
+
+import org.chromium.base.test.BaseRobolectricTestRunner;
+import org.chromium.chrome.browser.flags.ChromeFeatureList;
+import org.chromium.chrome.browser.image_editor.ImageEditorDialogCoordinator;
+import org.chromium.chrome.browser.screenshot.EditorScreenshotSource;
+import org.chromium.chrome.browser.share.share_sheet.ChromeOptionShareCallback;
+import org.chromium.chrome.browser.tab.Tab;
+import org.chromium.chrome.modules.image_editor.ImageEditorModuleProvider;
+import org.chromium.chrome.modules.image_editor.ImageEditorProvider;
+import org.chromium.chrome.test.util.browser.Features;
+import org.chromium.components.browser_ui.bottomsheet.BottomSheetController;
+import org.chromium.components.module_installer.engine.InstallListener;
+
+// clang-format off
+/**
+ * Tests for {@link ScreenshotCoordinator}.
+ */
+@RunWith(BaseRobolectricTestRunner.class)
+@Features.EnableFeatures({ChromeFeatureList.CHROME_SHARE_SCREENSHOT,
+        ChromeFeatureList.CHROME_SHARING_HUB})
+public class ScreenshotCoordinatorTest {
+    // clang-format on
+    @Mock
+    private Activity mActivity;
+
+    @Mock
+    private FragmentManager mFragmentManagerMock;
+
+    @Mock
+    private ChromeOptionShareCallback mChromeOptionShareCallback;
+
+    // FakeEditorScreenshotTask abstracts taking a screenshot; it always succeeds and
+    // returns mBimap from the test class.
+    private final class FakeEditorScreenshotTask implements EditorScreenshotSource {
+        public FakeEditorScreenshotTask() {}
+
+        @Override
+        public void capture(@Nullable Runnable callback) {
+            callback.run();
+        }
+
+        @Override
+        public boolean isReady() {
+            return true;
+        }
+
+        @Override
+        public Bitmap getScreenshot() {
+            return mBitmap;
+        }
+    }
+
+    @Mock
+    private ImageEditorDialogCoordinator mImageEditorDialogCoordinatorMock;
+
+    @Mock
+    private ImageEditorModuleProvider mImageEditorModuleProviderMock;
+
+    @Mock
+    private ScreenshotShareSheetDialog mScreenshotShareSheetDialogMock;
+
+    @Mock
+    private BottomSheetController mBottomSheetControllerMock;
+
+    @Mock
+    private Tab mTab;
+
+    // FakeImageEditorProviderImpl returns a mock, so we may stub out launchEditor().
+    private class FakeImageEditorProviderImpl implements ImageEditorProvider {
+        @Override
+        public ImageEditorDialogCoordinator getImageEditorDialogCoordinator() {
+            return mImageEditorDialogCoordinatorMock;
+        }
+    }
+    private FakeImageEditorProviderImpl mFakeImageEditorProvider;
+
+    // Bitmap used for successful screenshot capture requests.
+    private Bitmap mBitmap;
+
+    // Object under test.
+    private ScreenshotCoordinator mScreenshotCoordinator;
+
+    @Before
+    public void setUp() {
+        MockitoAnnotations.initMocks(this);
+
+        when(mActivity.getFragmentManager()).thenReturn(mFragmentManagerMock);
+
+        mFakeImageEditorProvider = new FakeImageEditorProviderImpl();
+        when(mImageEditorModuleProviderMock.getImageEditorProvider())
+                .thenReturn(mFakeImageEditorProvider);
+        doNothing()
+                .when(mImageEditorDialogCoordinatorMock)
+                .launchEditor(mActivity, mBitmap, mTab, mChromeOptionShareCallback);
+
+        mBitmap = Bitmap.createBitmap(800, 600, Bitmap.Config.ARGB_8888);
+
+        ImageEditorModuleProvider.setInstanceForTesting(mImageEditorModuleProviderMock);
+
+        // Instantiate the object under test.
+        mScreenshotCoordinator = new ScreenshotCoordinator(mActivity, mTab,
+                new FakeEditorScreenshotTask(), mScreenshotShareSheetDialogMock,
+                mChromeOptionShareCallback, mBottomSheetControllerMock);
+    }
+
+    @Test
+    public void editorLaunchesWhenModuleInstalled() {
+        when(mImageEditorModuleProviderMock.isModuleInstalled()).thenReturn(true);
+        // Simulate a successful install.
+        doAnswer(answerVoid((InstallListener listener) -> listener.onComplete(/*success=*/true)))
+                .when(mImageEditorModuleProviderMock)
+                .maybeInstallModule(any());
+
+        // The first request for the feature will ask for module install, which succeeds,
+        // and the callback will launch the editor.
+        mScreenshotCoordinator.captureScreenshot();
+
+        // Ensure the editor launches.
+        verify(mImageEditorDialogCoordinatorMock)
+                .launchEditor(mActivity, mBitmap, mTab, mChromeOptionShareCallback);
+    }
+
+    @Test
+    public void retryInstallSuccess() {
+        // First request does not have the module available, and install fails.
+        when(mImageEditorModuleProviderMock.isModuleInstalled()).thenReturn(false);
+        doAnswer(answerVoid((InstallListener listener) -> listener.onComplete(/*success=*/false)))
+                .when(mImageEditorModuleProviderMock)
+                .maybeInstallModule(any());
+
+        doNothing().when(mScreenshotShareSheetDialogMock).show(any(FragmentManager.class), any());
+
+        mScreenshotCoordinator.captureScreenshot();
+
+        // Failed install loads the share sheet.
+        verify(mScreenshotShareSheetDialogMock).show(any(FragmentManager.class), any());
+        // The editor is not launched.
+        verify(mImageEditorDialogCoordinatorMock, never()).launchEditor(any(), any(), any(), any());
+
+        // A second install is attempted and succeeds.
+        when(mImageEditorModuleProviderMock.isModuleInstalled()).thenReturn(true);
+        doAnswer(answerVoid((InstallListener listener) -> listener.onComplete(/*success=*/true)))
+                .when(mImageEditorModuleProviderMock)
+                .maybeInstallModule(any());
+
+        mScreenshotCoordinator.captureScreenshot();
+        // The editor should launch without requiring a discrete user action.
+        verify(mImageEditorDialogCoordinatorMock)
+                .launchEditor(mActivity, mBitmap, mTab, mChromeOptionShareCallback);
+    }
+
+    @Test
+    public void maxInstallTriesExceeded() {
+        // Attempt several installs which persistently fail, up to the limit.
+        when(mImageEditorModuleProviderMock.isModuleInstalled()).thenReturn(false);
+        doAnswer(answerVoid((InstallListener listener) -> listener.onComplete(/*success=*/false)))
+                .when(mImageEditorModuleProviderMock)
+                .maybeInstallModule(any());
+        for (int attempt = 0; attempt < ScreenshotCoordinator.MAX_INSTALL_ATTEMPTS; attempt++) {
+            mScreenshotCoordinator.captureScreenshot();
+        }
+
+        // Ensure we tried to install N times.
+        verify(mImageEditorModuleProviderMock, times(ScreenshotCoordinator.MAX_INSTALL_ATTEMPTS))
+                .maybeInstallModule(any());
+        // Failed install loads the share sheet.
+        verify(mScreenshotShareSheetDialogMock, times(ScreenshotCoordinator.MAX_INSTALL_ATTEMPTS))
+                .show(any(FragmentManager.class), any());
+        // Ensure the editor was never loaded.
+        verify(mImageEditorDialogCoordinatorMock, never()).launchEditor(any(), any(), any(), any());
+
+        // Subsequent attempts will not invoke installation.
+        mScreenshotCoordinator.captureScreenshot();
+        // maybeInstallModule will not be called again; count will remain the same, at the limit.
+        verify(mImageEditorModuleProviderMock, times(ScreenshotCoordinator.MAX_INSTALL_ATTEMPTS))
+                .maybeInstallModule(any());
+        // The share sheet will be loaded directly in all cases plus the failure case.
+        verify(mScreenshotShareSheetDialogMock,
+                times(ScreenshotCoordinator.MAX_INSTALL_ATTEMPTS + 1))
+                .show(any(FragmentManager.class), any());
+        // The editor should not attempt to be loaded.
+        verify(mImageEditorDialogCoordinatorMock, never()).launchEditor(any(), any(), any(), any());
+    }
+}

chrome/browser/share/android/test_java_sources.gni

@@ -16,6 +16,7 @@ share_test_java_sources = [
 
 share_junit_test_java_sources = [
   "//chrome/browser/share/android/javatests/src/org/chromium/chrome/browser/share/link_to_text/LinkToTextCoordinatorTest.java",
+  "//chrome/browser/share/android/javatests/src/org/chromium/chrome/browser/share/screenshot/ScreenshotCoordinatorTest.java",
   "//chrome/browser/share/android/javatests/src/org/chromium/chrome/browser/share/screenshot/ScreenshotShareSheetMediatorUnitTest.java",
   "//chrome/browser/share/android/javatests/src/org/chromium/chrome/browser/share/send_tab_to_self/NotificationSharedPrefManagerTest.java",
   "//chrome/browser/share/android/javatests/src/org/chromium/chrome/browser/share/send_tab_to_self/SendTabToSelfAndroidBridgeTest.java",