[LongScreenshots] Refactored Entry logic and added error logic.

Broke LongScreenshotsEntry into entry logic and bitmap generation logic
for a cleaner separation. Also added logic to propagate errors.

Bug: 1153969
Change-Id: Ie81a7ab8fc8c368f11127d219b37101d62ee61f9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2622055Reviewed-by: Kyle Milka <kmilka@chromium.org>
Reviewed-by: Calder Kitagawa <ckitagawa@chromium.org>
Reviewed-by: Mehran Mahmoudi <mahmoudi@chromium.org>
Commit-Queue: Tanya Gupta <tgupta@chromium.org>
Cr-Commit-Position: refs/heads/master@{#843608}

chrome/browser/share/android/java/src/org/chromium/chrome/browser/share/long_screenshots/BitmapGenerator.java

+// Copyright 2021 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.long_screenshots;
+
+import android.content.Context;
+import android.graphics.Bitmap;
+import android.graphics.Rect;
+
+import androidx.annotation.NonNull;
+
+import org.chromium.chrome.browser.tab.Tab;
+import org.chromium.components.paint_preview.common.proto.PaintPreview.PaintPreviewProto;
+import org.chromium.components.paintpreview.player.CompositorStatus;
+import org.chromium.url.GURL;
+
+/**
+ * Class responsible for processing the initial request, calling {@link LongScreenshotsTab} service
+ * to capture the webpage, then using {@link LongScreenshotsCompositor} to composite the bitmap.
+ * Callers of this class should supply a GeneratorCallback to receive the final bitmap or any errors
+ * along the way.
+ */
+public class BitmapGenerator implements LongScreenshotsTabService.CaptureProcessor {
+    private Context mContext;
+    private Rect mRect;
+    // Response with a pointer to the skia image
+    private PaintPreviewProto mProtoResponse;
+
+    // Compositor delegate responsible for compositing the skia
+    private LongScreenshotsCompositor mCompositor;
+    private LongScreenshotsTabService mLongScreenshotsTabService;
+    private Tab mTab;
+
+    private static final String DIR_NAME = "long_screenshots_dir";
+
+    protected GeneratorCallBack mGeneratorCallback;
+
+    /**
+     * Users of the {@link LongScreenshotsEntry} class have to implement and pass this interface in
+     * the constructor.
+     */
+    public interface GeneratorCallBack {
+        /**
+         * Called when the compositor cannot be successfully initialized.
+         */
+        void onCompositorError(@CompositorStatus int status);
+
+        /**
+         * Called when the bitmap has been successfully generated.
+         */
+        void onBitmapGenerated(Bitmap bitmap);
+
+        /**
+         * Called when the capture failed.
+         */
+        void onCaptureError(@Status int status);
+    }
+
+    /**
+     * @param context An instance of current Android {@link Context}.
+     * @param tab The current tab being screen-shotted.
+     * @param rect The area of the webpage to capture
+     * @param callback Callback to receive updates from the generation.
+     */
+    public BitmapGenerator(
+            Context context, Tab tab, @NonNull Rect rect, GeneratorCallBack callback) {
+        mContext = context;
+        mTab = tab;
+        mGeneratorCallback = callback;
+        mRect = rect;
+
+        mLongScreenshotsTabService = LongScreenshotsTabServiceFactory.getServiceInstance();
+        mLongScreenshotsTabService.setCaptureProcessor(this);
+    }
+
+    /**
+     * Starts the capture of the screenshot.
+     */
+    public void captureScreenshot() {
+        mLongScreenshotsTabService.captureTab(mTab, mRect);
+    }
+
+    /**
+     * Called from native after the tab has been captured.
+     *
+     * @param response
+     * @param status
+     */
+    @Override
+    public void processCapturedTab(PaintPreviewProto response, @Status int status) {
+        if (status != Status.OK) {
+            mGeneratorCallback.onCaptureError(status);
+        } else {
+            mCompositor = new LongScreenshotsCompositor(new GURL(response.getMetadata().getUrl()),
+                    mLongScreenshotsTabService, DIR_NAME, response, mRect,
+                    mGeneratorCallback::onBitmapGenerated, mGeneratorCallback::onCompositorError);
+        }
+    }
+
+    /**
+     * Called after the bitmap has been composited and can be shown to the user.
+     *
+     * @param result Bitmap to display in the dialog.
+     */
+    private void onBitmapResult(Bitmap result) {
+        mGeneratorCallback.onBitmapGenerated(result);
+    }
+
+    /**
+     * Destroy and clean up any memory.
+     */
+    public void destroy() {
+        if (mCompositor != null) {
+            mCompositor.destroy();
+            mCompositor = null;
+        }
+    }
+}

chrome/browser/share/android/java/src/org/chromium/chrome/browser/share/long_screenshots/EntryManager.java

@@ -5,26 +5,23 @@
 package org.chromium.chrome.browser.share.long_screenshots;
 
 import android.content.Context;
-import android.graphics.Bitmap;
 
-import org.chromium.base.Callback;
 import org.chromium.chrome.browser.tab.Tab;
 import org.chromium.content_public.browser.RenderCoordinates;
 import org.chromium.ui.display.DisplayAndroid;
 
-import java.util.ArrayList;
-import java.util.List;
+import java.util.LinkedList;
 
 /**
  * Entry manager responsible for managing all the of the {@LongScreenshotEntry}. This should be used
- * to generate and retreive the needed bitmaps. The first bitmap can be generated by calling
- * {@link generateInitialBitmapBlocking}.
+ * to generate and retrieve the needed bitmaps. The first bitmap can be generated by calling
+ * {@link generateInitialBitmap}.
  */
 public class EntryManager {
     private Context mContext;
     private Tab mTab;
 
-    private List<LongScreenshotsEntry> mEntries;
+    private LinkedList<LongScreenshotsEntry> mEntries;
     private int mDisplayHeightPx;
 
     /**
@@ -34,39 +31,24 @@ public class EntryManager {
     public EntryManager(Context context, Tab tab) {
         mContext = context;
         mTab = tab;
-        mEntries = new ArrayList<LongScreenshotsEntry>();
+        mEntries = new LinkedList<LongScreenshotsEntry>();
         calculateClipHeight();
     }
 
     /**
-     * Generates the first bitmap of the page that is the height of the phone display. Calls the
-     * provided callback with the generated bitmap.
-     *
-     * @param onGenerated Callback to pass the generated bitmap.
+     * Generates the first bitmap of the page that is the height of the phone display. Callers of
+     * this function should add a listener to the returned entry to get that status of the
+     * generation and retrieve the bitmap.
      */
-    public void generateInitialBitmap(Callback<Bitmap> onGenerated) {
-        LongScreenshotsEntry entry =
-                new LongScreenshotsEntry(mContext, mTab, new LongScreenshotsEntry.Listener() {
-                    @Override
-                    public void onCompositorError(int status) {
-                        // TODO(tgupta): Auto-generated method stub
-                    }
-
-                    @Override
-                    public void onCaptureError() {
-                        // TODO(tgupta): Auto-generated method stub
-                    }
-
-                    @Override
-                    public void onBitmapGenerated(LongScreenshotsEntry entry) {
-                        mEntries.add(entry);
-                        onGenerated.onResult(entry.getBitmap());
-                    }
-                });
+    public LongScreenshotsEntry generateInitialEntry() {
         RenderCoordinates coords = RenderCoordinates.fromWebContents(mTab.getWebContents());
         int startY = coords.getScrollYPixInt() / coords.getPageScaleFactorInt();
-        entry.setClipBounds(startY, mDisplayHeightPx);
-        entry.captureScreenshot();
+
+        LongScreenshotsEntry entry =
+                new LongScreenshotsEntry(mContext, mTab, startY, mDisplayHeightPx);
+        entry.generateBitmap();
+        mEntries.add(entry);
+        return entry;
     }
 
     /**

chrome/browser/share/android/java/src/org/chromium/chrome/browser/share/long_screenshots/LongScreenshotsCompositor.java

@@ -28,6 +28,7 @@ public class LongScreenshotsCompositor {
     private PlayerCompositorDelegate mDelegate;
     private Callback<Bitmap> mBitmapCallback;
     private Rect mRect;
+    private Callback<Integer> mErrorCallback;
 
     private static PlayerCompositorDelegate.Factory sCompositorDelegateFactory =
             new CompositorDelegateFactory();
@@ -41,26 +42,25 @@ public class LongScreenshotsCompositor {
      * @param rect The area of the captured webpage that should be composited.
      * @param response The proto with the address of the captured bitmap.
      * @param bitmapCallback Callback to process the composited bitmap.
+     * @param errorCallback Callback to process any errors.
      */
     public LongScreenshotsCompositor(GURL url,
             NativePaintPreviewServiceProvider nativePaintPreviewServiceProvider,
             String directoryKey, PaintPreviewProto response, Rect rect,
-            Callback<Bitmap> bitmapCallback) {
+            Callback<Bitmap> bitmapCallback, Callback<Integer> errorCallback) {
         mBitmapCallback = bitmapCallback;
         mRect = rect;
+        mErrorCallback = errorCallback;
 
-        // TODO(tgupta): Look into warmupCompositor
-        mDelegate = getCompositorDelegateFactory().createForProto(nativePaintPreviewServiceProvider,
-                response, url, directoryKey, true, this::onCompositorReady,
-                this::onCompositorError);
+        mDelegate = new PlayerCompositorDelegateImpl(nativePaintPreviewServiceProvider, response,
+                url, directoryKey, true, this::onCompositorReady, errorCallback);
     }
 
     /**
      * Called when the compositor cannot be successfully initialized.
      */
     private void onCompositorError(@CompositorStatus int status) {
-        // do nothing for now
-        // TODO(tgupta): Figure out what to display to the user at this point.
+        mErrorCallback.onResult(status);
     }
 
     /**
@@ -72,7 +72,7 @@ public class LongScreenshotsCompositor {
     protected void onCompositorReady(UnguessableToken rootFrameGuid, UnguessableToken[] frameGuids,
             int[] frameContentSize, int[] scrollOffsets, int[] subFramesCount,
             UnguessableToken[] subFrameGuids, int[] subFrameClipRects) {
-        // TODO(tgupta): Keep track of the returned id.
+        // TODO(tgupta): Pass the request id back to the Entry for tracking.
         mDelegate.requestBitmap(mRect, 1, mBitmapCallback, this::onError);
     }
 
@@ -80,8 +80,7 @@ public class LongScreenshotsCompositor {
      * Called when there was an error compositing the bitmap.
      */
     public void onError() {
-        // do nothing for now
-        // TODO(tgupta): Figure out what to display to the user at this point.
+        mErrorCallback.onResult(CompositorStatus.REQUEST_BITMAP_FAILURE);
     }
 
     public void destroy() {

chrome/browser/share/android/java/src/org/chromium/chrome/browser/share/long_screenshots/LongScreenshotsCoordinator.java

@@ -5,9 +5,7 @@
 package org.chromium.chrome.browser.share.long_screenshots;
 
 import android.app.Activity;
-import android.graphics.Bitmap;
 
-import org.chromium.base.Callback;
 import org.chromium.chrome.browser.paint_preview.PaintPreviewCompositorUtils;
 import org.chromium.chrome.browser.share.screenshot.ScreenshotCoordinator;
 import org.chromium.chrome.browser.share.share_sheet.ChromeOptionShareCallback;
@@ -49,10 +47,11 @@ public class LongScreenshotsCoordinator extends ScreenshotCoordinator {
     public void captureScreenshot() {
         EntryManager entryManager = new EntryManager(mActivity, mTab);
 
-        entryManager.generateInitialBitmap(new Callback<Bitmap>() {
+        LongScreenshotsEntry entry = entryManager.generateInitialEntry();
+        entry.setListener(new LongScreenshotsEntry.EntryListener() {
             @Override
-            public void onResult(Bitmap result) {
-                mScreenshot = result;
+            public void onResult(int status) {
+                mScreenshot = entry.getBitmap();
                 launchSharesheet();
             }
         });

chrome/browser/share/android/java_sources.gni

@@ -9,6 +9,7 @@ share_java_sources = [
   "//chrome/browser/share/android/java/src/org/chromium/chrome/browser/share/clipboard/ClipboardImageFileProvider.java",
   "//chrome/browser/share/android/java/src/org/chromium/chrome/browser/share/link_to_text/LinkToTextCoordinator.java",
   "//chrome/browser/share/android/java/src/org/chromium/chrome/browser/share/link_to_text/LinkToTextMetricsBridge.java",
+  "//chrome/browser/share/android/java/src/org/chromium/chrome/browser/share/long_screenshots/BitmapGenerator.java",
   "//chrome/browser/share/android/java/src/org/chromium/chrome/browser/share/long_screenshots/EntryManager.java",
   "//chrome/browser/share/android/java/src/org/chromium/chrome/browser/share/long_screenshots/LongScreenshotsCompositor.java",
   "//chrome/browser/share/android/java/src/org/chromium/chrome/browser/share/long_screenshots/LongScreenshotsCoordinator.java",

chrome/browser/share/android/javatests/src/org/chromium/chrome/browser/share/long_screenshots/BitmapGeneratorTest.java

+// Copyright 2021 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.long_screenshots;
+
+import android.graphics.Bitmap;
+import android.graphics.Rect;
+
+import androidx.test.filters.MediumTest;
+
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+import org.junit.runner.RunWith;
+
+import org.chromium.base.test.util.CommandLineFlags;
+import org.chromium.base.test.util.Criteria;
+import org.chromium.base.test.util.CriteriaHelper;
+import org.chromium.base.test.util.Feature;
+import org.chromium.base.test.util.Matchers;
+import org.chromium.chrome.browser.flags.ChromeSwitches;
+import org.chromium.chrome.browser.paint_preview.PaintPreviewCompositorUtils;
+import org.chromium.chrome.browser.tab.Tab;
+import org.chromium.chrome.test.ChromeJUnit4ClassRunner;
+import org.chromium.chrome.test.ChromeTabbedActivityTestRule;
+import org.chromium.components.paintpreview.player.CompositorStatus;
+import org.chromium.content_public.browser.LoadUrlParams;
+import org.chromium.content_public.browser.test.util.TestThreadUtils;
+import org.chromium.net.test.EmbeddedTestServer;
+
+/** Tests for the LongScreenshotsEntryTest. */
+@RunWith(ChromeJUnit4ClassRunner.class)
+@CommandLineFlags.Add({ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE})
+public class BitmapGeneratorTest {
+    @Rule
+    public final ChromeTabbedActivityTestRule mActivityTestRule =
+            new ChromeTabbedActivityTestRule();
+
+    @Rule
+    public TemporaryFolder mTemporaryFolder = new TemporaryFolder();
+
+    private Tab mTab;
+    private BitmapGenerator mGenerator;
+    private TestListener mTestListener;
+    private Bitmap mGeneratedBitmap;
+
+    class TestListener implements BitmapGenerator.GeneratorCallBack {
+        boolean mOnBitmapGeneratedCalled;
+
+        @Override
+        public void onCompositorError(@CompositorStatus int status) {}
+
+        @Override
+        public void onBitmapGenerated(Bitmap bitmap) {
+            mOnBitmapGeneratedCalled = true;
+            mGeneratedBitmap = bitmap;
+        }
+
+        @Override
+        public void onCaptureError(@Status int status) {}
+
+        boolean getOnBitmapGeneratedCalled() {
+            return mOnBitmapGeneratedCalled;
+        }
+    }
+
+    @Before
+    public void setUp() throws Exception {
+        mActivityTestRule.startMainActivityOnBlankPage();
+        mTab = mActivityTestRule.getActivity().getActivityTab();
+        mTestListener = new TestListener();
+
+        TestThreadUtils.runOnUiThreadBlocking(() -> {
+            mGenerator = new BitmapGenerator(
+                    mActivityTestRule.getActivity(), mTab, new Rect(0, 0, 100, 100), mTestListener);
+            PaintPreviewCompositorUtils.warmupCompositor();
+        });
+    }
+
+    @After
+    public void tearDown() throws Exception {
+        TestThreadUtils.runOnUiThreadBlocking(() -> { mGenerator.destroy(); });
+    }
+
+    /**
+     * Verifies that a Tab's contents are captured.
+     */
+    @Test
+    @MediumTest
+    @Feature({"LongScreenshots"})
+    public void testCaptured() throws Exception {
+        EmbeddedTestServer testServer = mActivityTestRule.getTestServer();
+        final String url = testServer.getURL("/chrome/test/data/android/about.html");
+
+        TestThreadUtils.runOnUiThreadBlocking(() -> {
+            mTab.loadUrl(new LoadUrlParams(url));
+            mGenerator.captureScreenshot();
+        });
+
+        CriteriaHelper.pollUiThread(() -> {
+            Criteria.checkThat("Callback was not called",
+                    mTestListener.getOnBitmapGeneratedCalled(), Matchers.is(true));
+        }, 10000L, 50L);
+
+        Assert.assertNotNull(mGeneratedBitmap);
+    }
+}

chrome/browser/share/android/javatests/src/org/chromium/chrome/browser/share/long_screenshots/LongScreenshotsCompositorTest.java

@@ -131,9 +131,16 @@ public class LongScreenshotsCompositorTest {
             }
         };
 
+        Callback<Integer> onErrorCallback = new Callback<Integer>() {
+            @Override
+            public void onResult(Integer result) {
+                Assert.fail("Error should not be thrown");
+            }
+        };
+
         LongScreenshotsCompositor compositor = new LongScreenshotsCompositor(mTestGurl,
                 mNativePaintPreviewServiceProvider, "test_directory_key",
-                PaintPreviewProto.getDefaultInstance(), mRect, onBitmapResult);
+                PaintPreviewProto.getDefaultInstance(), mRect, onBitmapResult, onErrorCallback);
 
         // Mimic the service calling onCompositorReady
         compositor.onCompositorReady(null, null, null, null, null, null, null);

chrome/browser/share/android/javatests/src/org/chromium/chrome/browser/share/long_screenshots/LongScreenshotsEntryTest.java

@@ -4,102 +4,7 @@
 
 package org.chromium.chrome.browser.share.long_screenshots;
 
-import androidx.test.filters.MediumTest;
-
-import org.junit.After;
-import org.junit.Assert;
-import org.junit.Before;
-import org.junit.Rule;
-import org.junit.Test;
-import org.junit.rules.TemporaryFolder;
-import org.junit.runner.RunWith;
-
-import org.chromium.base.test.util.CommandLineFlags;
-import org.chromium.base.test.util.Criteria;
-import org.chromium.base.test.util.CriteriaHelper;
-import org.chromium.base.test.util.Feature;
-import org.chromium.base.test.util.Matchers;
-import org.chromium.chrome.browser.flags.ChromeSwitches;
-import org.chromium.chrome.browser.paint_preview.PaintPreviewCompositorUtils;
-import org.chromium.chrome.browser.tab.Tab;
-import org.chromium.chrome.test.ChromeJUnit4ClassRunner;
-import org.chromium.chrome.test.ChromeTabbedActivityTestRule;
-import org.chromium.content_public.browser.LoadUrlParams;
-import org.chromium.content_public.browser.test.util.TestThreadUtils;
-import org.chromium.net.test.EmbeddedTestServer;
-
 /** Tests for the LongScreenshotsEntryTest. */
-@RunWith(ChromeJUnit4ClassRunner.class)
-@CommandLineFlags.Add({ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE})
 public class LongScreenshotsEntryTest {
-    @Rule
-    public final ChromeTabbedActivityTestRule mActivityTestRule =
-            new ChromeTabbedActivityTestRule();
-
-    @Rule
-    public TemporaryFolder mTemporaryFolder = new TemporaryFolder();
-
-    private Tab mTab;
-    private LongScreenshotsEntry mEntry;
-    private TestListener mTestListener;
-
-    class TestListener implements LongScreenshotsEntry.Listener {
-        boolean mOnBitmapGeneratedCalled;
-
-        @Override
-        public void onCompositorError(int status) {}
-
-        @Override
-        public void onBitmapGenerated(LongScreenshotsEntry entry) {
-            mOnBitmapGeneratedCalled = true;
-        }
-
-        @Override
-        public void onCaptureError() {}
-
-        boolean getOnBitmapGeneratedCalled() {
-            return mOnBitmapGeneratedCalled;
-        }
-    }
-
-    @Before
-    public void setUp() throws Exception {
-        mActivityTestRule.startMainActivityOnBlankPage();
-        mTab = mActivityTestRule.getActivity().getActivityTab();
-        mTestListener = new TestListener();
-
-        TestThreadUtils.runOnUiThreadBlocking(() -> {
-            mEntry = new LongScreenshotsEntry(mActivityTestRule.getActivity(), mTab, mTestListener);
-            PaintPreviewCompositorUtils.warmupCompositor();
-        });
-    }
-
-    @After
-    public void tearDown() throws Exception {
-        TestThreadUtils.runOnUiThreadBlocking(() -> { mEntry.destroy(); });
-    }
-
-    /**
-     * Verifies that a Tab's contents are captured.
-     */
-    @Test
-    @MediumTest
-    @Feature({"LongScreenshots"})
-    public void testCaptured() throws Exception {
-        EmbeddedTestServer testServer = mActivityTestRule.getTestServer();
-        final String url = testServer.getURL("/chrome/test/data/android/about.html");
-
-        TestThreadUtils.runOnUiThreadBlocking(() -> {
-            mTab.loadUrl(new LoadUrlParams(url));
-            mEntry.setClipBounds(0, 100);
-            mEntry.captureScreenshot();
-        });
-
-        CriteriaHelper.pollUiThread(() -> {
-            Criteria.checkThat("Callback was not called",
-                    mTestListener.getOnBitmapGeneratedCalled(), Matchers.is(true));
-        }, 10000L, 50L);
-
-        Assert.assertNotNull(mEntry.getBitmap());
-    }
+    // TODO(tgupta): Write tests for LongScreenshotsEntry.
 }

chrome/browser/share/android/test_java_sources.gni

@@ -5,7 +5,7 @@
 # TODO(crbug.com/1022172): This should be a separate build target when circular dependencies are removed.
 share_test_java_sources = [
   "//chrome/browser/share/android/javatests/src/org/chromium/chrome/browser/share/clipboard/ClipboardImageFileProviderTest.java",
-  "//chrome/browser/share/android/javatests/src/org/chromium/chrome/browser/share/long_screenshots/LongScreenshotsEntryTest.java",
+  "//chrome/browser/share/android/javatests/src/org/chromium/chrome/browser/share/long_screenshots/BitmapGeneratorTest.java",
   "//chrome/browser/share/android/javatests/src/org/chromium/chrome/browser/share/long_screenshots/LongScreenshotsTabServiceTest.java",
   "//chrome/browser/share/android/javatests/src/org/chromium/chrome/browser/share/screenshot/ScreenshotShareSheetSaveDelegateTest.java",
   "//chrome/browser/share/android/javatests/src/org/chromium/chrome/browser/share/screenshot/ScreenshotShareSheetViewTest.java",

components/paint_preview/player/compositor_status.h

@@ -28,6 +28,8 @@ enum class CompositorStatus : int {
   TIMED_OUT,
   STOPPED_DUE_TO_MEMORY_PRESSURE,
   SKIPPED_DUE_TO_MEMORY_PRESSURE,
+  // Used by long screenshots code only when call to requestBitmap fails.
+  REQUEST_BITMAP_FAILURE,
   COUNT,
 };
 

tools/metrics/histograms/enums.xml

@@ -73211,6 +73211,7 @@ would be helpful to identify which type is being sent.
   <int value="12" label="Timed Out"/>
   <int value="13" label="Stopped Due To Memory Pressure"/>
   <int value="14" label="Skipped Due To Memory Pressure"/>
+  <int value="15" label="Failure When Requesting Bitmap"/>
 </enum>
 
 <enum name="TabbedPaintPreviewExitCause">