[Paint Preview] Refactor PlayerCompositorDelegate + Tests

This CL refactors the PlayerCompositorDelegate to structure it in a way
that is more amenable to testing. This CL also adds a significant amount
of test coverage for PlayerCompositorDelegate.

PlayerCompositorDelegateAndroid is refactored to be compatible with the
changes to PlayerCompositorDelegate, but it is not tested as it is
relatively well tested from the Java side.

This boosts coverage to > 85%. Further coverage is difficult as the
edges are a result of difficult to simulate failures.

Bug: 1128927
Change-Id: Id7467209ab2858043655ae374e55ac699e4e1778
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2412893Reviewed-by: Mehran Mahmoudi <mahmoudi@chromium.org>
Commit-Queue: Calder Kitagawa <ckitagawa@chromium.org>
Cr-Commit-Position: refs/heads/master@{#807555}

components/paint_preview/player/android/player_compositor_delegate_android.cc

@@ -78,16 +78,17 @@ PlayerCompositorDelegateAndroid::PlayerCompositorDelegateAndroid(
     const JavaParamRef<jstring>& j_url_spec,
     const JavaParamRef<jstring>& j_directory_key,
     const JavaParamRef<jobject>& j_compositor_error_callback)
-    : PlayerCompositorDelegate(
-          paint_preview_service,
-          GURL(base::android::ConvertJavaStringToUTF8(env, j_url_spec)),
-          DirectoryKey{
-              base::android::ConvertJavaStringToUTF8(env, j_directory_key)},
-          base::BindOnce(
-              &base::android::RunIntCallbackAndroid,
-              ScopedJavaGlobalRef<jobject>(j_compositor_error_callback))),
+    : PlayerCompositorDelegate(),
       request_id_(0),
       startup_timestamp_(base::TimeTicks::Now()) {
+  PlayerCompositorDelegate::Initialize(
+      paint_preview_service,
+      GURL(base::android::ConvertJavaStringToUTF8(env, j_url_spec)),
+      DirectoryKey{
+          base::android::ConvertJavaStringToUTF8(env, j_directory_key)},
+      base::BindOnce(
+          &base::android::RunIntCallbackAndroid,
+          ScopedJavaGlobalRef<jobject>(j_compositor_error_callback)));
   java_ref_.Reset(env, j_object);
 }
 
@@ -203,12 +204,11 @@ void PlayerCompositorDelegateAndroid::RequestBitmap(
       "paint_preview", "PlayerCompositorDelegateAndroid::RequestBitmap",
       TRACE_ID_LOCAL(request_id_));
 
-  gfx::Rect clip_rect =
-      gfx::Rect(j_clip_x, j_clip_y, j_clip_width, j_clip_height);
   PlayerCompositorDelegate::RequestBitmap(
       base::android::UnguessableTokenAndroid::FromJavaUnguessableToken(
           env, j_frame_guid),
-      clip_rect, j_scale_factor,
+      gfx::Rect(j_clip_x, j_clip_y, j_clip_width, j_clip_height),
+      j_scale_factor,
       base::BindOnce(&PlayerCompositorDelegateAndroid::OnBitmapCallback,
                      weak_factory_.GetWeakPtr(),
                      ScopedJavaGlobalRef<jobject>(j_bitmap_callback),
@@ -228,32 +228,34 @@ void PlayerCompositorDelegateAndroid::OnBitmapCallback(
       TRACE_ID_LOCAL(request_id), "status", static_cast<int>(status), "bytes",
       sk_bitmap.computeByteSize());
 
-  if (status == mojom::PaintPreviewCompositor::BitmapStatus::kSuccess &&
-      !sk_bitmap.isNull()) {
-    base::ThreadPool::PostTaskAndReplyWithResult(
-        FROM_HERE, {base::TaskPriority::USER_VISIBLE},
-        base::BindOnce(&ConvertToJavaBitmap, sk_bitmap),
-        base::BindOnce(base::BindOnce(
-            [](const ScopedJavaGlobalRef<jobject>& j_bitmap_callback,
-               const ScopedJavaGlobalRef<jobject>& j_error_callback,
-               const ScopedJavaGlobalRef<jobject>& j_bitmap) {
-              if (!j_bitmap) {
-                base::android::RunRunnableAndroid(j_error_callback);
-                return;
-              }
-              base::android::RunObjectCallbackAndroid(j_bitmap_callback,
-                                                      j_bitmap);
-            },
-            j_bitmap_callback, j_error_callback)));
-    if (request_id == 0) {
-      auto delta = base::TimeTicks::Now() - startup_timestamp_;
-      if (delta.InMicroseconds() >= 0) {
-        base::UmaHistogramTimes("Browser.PaintPreview.Player.TimeToFirstBitmap",
-                                delta);
-      }
-    }
-  } else {
+  if (status != mojom::PaintPreviewCompositor::BitmapStatus::kSuccess ||
+      sk_bitmap.isNull()) {
     base::android::RunRunnableAndroid(j_error_callback);
+    return;
+  }
+
+  base::ThreadPool::PostTaskAndReplyWithResult(
+      FROM_HERE, {base::TaskPriority::USER_VISIBLE},
+      base::BindOnce(&ConvertToJavaBitmap, sk_bitmap),
+      base::BindOnce(base::BindOnce(
+          [](const ScopedJavaGlobalRef<jobject>& j_bitmap_callback,
+             const ScopedJavaGlobalRef<jobject>& j_error_callback,
+             const ScopedJavaGlobalRef<jobject>& j_bitmap) {
+            if (!j_bitmap) {
+              base::android::RunRunnableAndroid(j_error_callback);
+              return;
+            }
+            base::android::RunObjectCallbackAndroid(j_bitmap_callback,
+                                                    j_bitmap);
+          },
+          j_bitmap_callback, j_error_callback)));
+
+  if (request_id == 0) {
+    auto delta = base::TimeTicks::Now() - startup_timestamp_;
+    if (delta.InMicroseconds() >= 0) {
+      base::UmaHistogramTimes("Browser.PaintPreview.Player.TimeToFirstBitmap",
+                              delta);
+    }
   }
 }
 
@@ -268,6 +270,7 @@ ScopedJavaLocalRef<jstring> PlayerCompositorDelegateAndroid::OnClick(
       gfx::Rect(static_cast<int>(j_x), static_cast<int>(j_y), 1U, 1U));
   if (res.empty())
     return base::android::ConvertUTF8ToJavaString(env, "");
+
   base::UmaHistogramBoolean("Browser.PaintPreview.Player.LinkClicked", true);
   // TODO(crbug/1061435): Resolve cases where there are multiple links.
   // For now just return the first in the list.

components/paint_preview/player/player_compositor_delegate.cc

@@ -96,27 +96,26 @@ PrepareCompositeRequest(const paint_preview::PaintPreviewProto& proto) {
 
 }  // namespace
 
-PlayerCompositorDelegate::PlayerCompositorDelegate(
-    PaintPreviewBaseService* paint_preview_service,
-    const GURL& expected_url,
-    const DirectoryKey& key,
-    base::OnceCallback<void(int)> compositor_error,
-    bool skip_service_launch)
-    : compositor_error_(std::move(compositor_error)),
-      paint_preview_service_(paint_preview_service),
-      key_(key),
-      compress_on_close_(true),
-      paint_preview_compositor_service_(nullptr,
+PlayerCompositorDelegate::PlayerCompositorDelegate()
+    : paint_preview_compositor_service_(nullptr,
                                         base::OnTaskRunnerDeleter(nullptr)),
       paint_preview_compositor_client_(nullptr,
-                                       base::OnTaskRunnerDeleter(nullptr)) {
-  if (skip_service_launch) {
-    paint_preview_service_->GetCapturedPaintPreviewProto(
-        key, base::nullopt,
-        base::BindOnce(&PlayerCompositorDelegate::OnProtoAvailable,
-                       weak_factory_.GetWeakPtr(), expected_url));
-    return;
+                                       base::OnTaskRunnerDeleter(nullptr)) {}
+
+PlayerCompositorDelegate::~PlayerCompositorDelegate() {
+  if (compress_on_close_) {
+    paint_preview_service_->GetTaskRunner()->PostTask(
+        FROM_HERE,
+        base::BindOnce(base::IgnoreResult(&FileManager::CompressDirectory),
+                       paint_preview_service_->GetFileManager(), key_));
   }
+}
+
+void PlayerCompositorDelegate::Initialize(
+    PaintPreviewBaseService* paint_preview_service,
+    const GURL& expected_url,
+    const DirectoryKey& key,
+    base::OnceCallback<void(int)> compositor_error) {
   TRACE_EVENT_NESTABLE_ASYNC_BEGIN0("paint_preview",
                                     "PlayerCompositorDelegate CreateCompositor",
                                     TRACE_ID_LOCAL(this));
@@ -125,6 +124,35 @@ PlayerCompositorDelegate::PlayerCompositorDelegate(
           &PlayerCompositorDelegate::OnCompositorServiceDisconnected,
           weak_factory_.GetWeakPtr()));
 
+  InitializeInternal(paint_preview_service, expected_url, key,
+                     std::move(compositor_error));
+}
+
+void PlayerCompositorDelegate::InitializeWithFakeServiceForTest(
+    PaintPreviewBaseService* paint_preview_service,
+    const GURL& expected_url,
+    const DirectoryKey& key,
+    base::OnceCallback<void(int)> compositor_error,
+    std::unique_ptr<PaintPreviewCompositorService, base::OnTaskRunnerDeleter>
+        fake_compositor_service) {
+  paint_preview_compositor_service_ = std::move(fake_compositor_service);
+  paint_preview_compositor_service_->SetDisconnectHandler(
+      base::BindOnce(&PlayerCompositorDelegate::OnCompositorServiceDisconnected,
+                     weak_factory_.GetWeakPtr()));
+
+  InitializeInternal(paint_preview_service, expected_url, key,
+                     std::move(compositor_error));
+}
+
+void PlayerCompositorDelegate::InitializeInternal(
+    PaintPreviewBaseService* paint_preview_service,
+    const GURL& expected_url,
+    const DirectoryKey& key,
+    base::OnceCallback<void(int)> compositor_error) {
+  compositor_error_ = std::move(compositor_error);
+  paint_preview_service_ = paint_preview_service;
+  key_ = key;
+
   paint_preview_compositor_client_ =
       paint_preview_compositor_service_->CreateCompositor(
           base::BindOnce(&PlayerCompositorDelegate::OnCompositorClientCreated,
@@ -134,13 +162,33 @@ PlayerCompositorDelegate::PlayerCompositorDelegate(
                      weak_factory_.GetWeakPtr()));
 }
 
-PlayerCompositorDelegate::~PlayerCompositorDelegate() {
-  if (compress_on_close_) {
-    paint_preview_service_->GetTaskRunner()->PostTask(
-        FROM_HERE,
-        base::BindOnce(base::IgnoreResult(&FileManager::CompressDirectory),
-                       paint_preview_service_->GetFileManager(), key_));
+void PlayerCompositorDelegate::RequestBitmap(
+    const base::UnguessableToken& frame_guid,
+    const gfx::Rect& clip_rect,
+    float scale_factor,
+    base::OnceCallback<void(mojom::PaintPreviewCompositor::BitmapStatus,
+                            const SkBitmap&)> callback) {
+  DCHECK(IsInitialized());
+  if (!paint_preview_compositor_client_) {
+    std::move(callback).Run(
+        mojom::PaintPreviewCompositor::BitmapStatus::kMissingFrame, SkBitmap());
+    return;
   }
+
+  paint_preview_compositor_client_->BitmapForSeparatedFrame(
+      frame_guid, clip_rect, scale_factor, std::move(callback));
+}
+
+std::vector<const GURL*> PlayerCompositorDelegate::OnClick(
+    const base::UnguessableToken& frame_guid,
+    const gfx::Rect& rect) {
+  DCHECK(IsInitialized());
+  std::vector<const GURL*> urls;
+  auto it = hit_testers_.find(frame_guid);
+  if (it != hit_testers_.end())
+    it->second->HitTest(rect, &urls);
+
+  return urls;
 }
 
 void PlayerCompositorDelegate::OnCompositorReadyStatusAdapter(
@@ -148,7 +196,7 @@ void PlayerCompositorDelegate::OnCompositorReadyStatusAdapter(
     mojom::PaintPreviewBeginCompositeResponsePtr composite_response) {
   CompositorStatus new_status;
   switch (status) {
-    // falltrhough
+    // fallthrough
     case mojom::PaintPreviewCompositor::BeginCompositeStatus::kSuccess:
     case mojom::PaintPreviewCompositor::BeginCompositeStatus::kPartialSuccess:
       new_status = CompositorStatus::OK;
@@ -191,7 +239,6 @@ void PlayerCompositorDelegate::OnProtoAvailable(
     const GURL& expected_url,
     PaintPreviewBaseService::ProtoReadStatus proto_status,
     std::unique_ptr<PaintPreviewProto> proto) {
-  // TODO(crbug.com/1021590): Handle initialization errors.
   if (proto_status == PaintPreviewBaseService::ProtoReadStatus::kExpired) {
     OnCompositorReady(CompositorStatus::CAPTURE_EXPIRED, nullptr);
     return;
@@ -233,8 +280,6 @@ void PlayerCompositorDelegate::OnProtoAvailable(
     return;
   }
 
-  hit_testers_ = BuildHitTesters(*proto);
-
   if (!paint_preview_compositor_client_) {
     OnCompositorReady(CompositorStatus::COMPOSITOR_CLIENT_DISCONNECT, nullptr);
     return;
@@ -242,9 +287,10 @@ void PlayerCompositorDelegate::OnProtoAvailable(
 
   paint_preview_compositor_client_->SetRootFrameUrl(proto_url);
 
+  proto_ = std::move(proto);
   base::ThreadPool::PostTaskAndReplyWithResult(
       FROM_HERE, {base::MayBlock(), base::TaskPriority::USER_VISIBLE},
-      base::BindOnce(&PrepareCompositeRequest, *proto),
+      base::BindOnce(&PrepareCompositeRequest, *proto_),
       base::BindOnce(&PlayerCompositorDelegate::SendCompositeRequest,
                      weak_factory_.GetWeakPtr()));
 }
@@ -261,6 +307,11 @@ void PlayerCompositorDelegate::SendCompositeRequest(
       std::move(begin_composite_request),
       base::BindOnce(&PlayerCompositorDelegate::OnCompositorReadyStatusAdapter,
                      weak_factory_.GetWeakPtr()));
+
+  // Defer building hit testers so it happens in parallel with preparing the
+  // compositor.
+  hit_testers_ = BuildHitTesters(*proto_);
+  proto_.reset();
 }
 
 void PlayerCompositorDelegate::OnCompositorClientDisconnected() {
@@ -271,30 +322,4 @@ void PlayerCompositorDelegate::OnCompositorClientDisconnected() {
   }
 }
 
-void PlayerCompositorDelegate::RequestBitmap(
-    const base::UnguessableToken& frame_guid,
-    const gfx::Rect& clip_rect,
-    float scale_factor,
-    base::OnceCallback<void(mojom::PaintPreviewCompositor::BitmapStatus,
-                            const SkBitmap&)> callback) {
-  if (!paint_preview_compositor_client_) {
-    std::move(callback).Run(
-        mojom::PaintPreviewCompositor::BitmapStatus::kMissingFrame, SkBitmap());
-    return;
-  }
-
-  paint_preview_compositor_client_->BitmapForSeparatedFrame(
-      frame_guid, clip_rect, scale_factor, std::move(callback));
-}
-
-std::vector<const GURL*> PlayerCompositorDelegate::OnClick(
-    const base::UnguessableToken& frame_guid,
-    const gfx::Rect& rect) {
-  std::vector<const GURL*> urls;
-  auto it = hit_testers_.find(frame_guid);
-  if (it != hit_testers_.end())
-    it->second->HitTest(rect, &urls);
-  return urls;
-}
-
 }  // namespace paint_preview

components/paint_preview/player/player_compositor_delegate.h

@@ -28,20 +28,31 @@ namespace paint_preview {
 
 class DirectoryKey;
 
+// Class to facilitate a player creating and communicating with an instance of
+// PaintPreviewCompositor.
 class PlayerCompositorDelegate {
  public:
-  PlayerCompositorDelegate(PaintPreviewBaseService* paint_preview_service,
-                           const GURL& url,
-                           const DirectoryKey& key,
-                           base::OnceCallback<void(int)> compositor_error,
-                           bool skip_service_launch = false);
+  PlayerCompositorDelegate();
   virtual ~PlayerCompositorDelegate();
 
   PlayerCompositorDelegate(const PlayerCompositorDelegate&) = delete;
   PlayerCompositorDelegate& operator=(const PlayerCompositorDelegate&) = delete;
 
+  // Initializes the compositor.
+  void Initialize(PaintPreviewBaseService* paint_preview_service,
+                  const GURL& url,
+                  const DirectoryKey& key,
+                  base::OnceCallback<void(int)> compositor_error);
+
+  // Returns whether initialization has happened.
+  bool IsInitialized() const { return paint_preview_service_; }
+
+  // Overrides whether to compress the directory when the player is closed. By
+  // default compression will happen.
   void SetCompressOnClose(bool compress) { compress_on_close_ = compress; }
 
+  // Implementations should override this to handle alternative compositor ready
+  // situations.
   virtual void OnCompositorReady(
       CompositorStatus compositor_status,
       mojom::PaintPreviewBeginCompositeResponsePtr composite_response) {}
@@ -59,12 +70,34 @@ class PlayerCompositorDelegate {
   std::vector<const GURL*> OnClick(const base::UnguessableToken& frame_guid,
                                    const gfx::Rect& rect);
 
+  // Test methods:
+
+  // Initializes the compositor without a real service for testing purposes.
+  void InitializeWithFakeServiceForTest(
+      PaintPreviewBaseService* paint_preview_service,
+      const GURL& expected_url,
+      const DirectoryKey& key,
+      base::OnceCallback<void(int)> compositor_error,
+      std::unique_ptr<PaintPreviewCompositorService, base::OnTaskRunnerDeleter>
+          fake_compositor_service);
+
+  PaintPreviewCompositorService* GetCompositorServiceForTest() {
+    return paint_preview_compositor_service_.get();
+  }
+
+  PaintPreviewCompositorClient* GetClientForTest() {
+    return paint_preview_compositor_client_.get();
+  }
+
  protected:
   base::OnceCallback<void(int)> compositor_error_;
-  PaintPreviewBaseService* paint_preview_service_;
-  DirectoryKey key_;
 
  private:
+  void InitializeInternal(PaintPreviewBaseService* paint_preview_service,
+                          const GURL& expected_url,
+                          const DirectoryKey& key,
+                          base::OnceCallback<void(int)> compositor_error);
+
   void OnCompositorReadyStatusAdapter(
       mojom::PaintPreviewCompositor::BeginCompositeStatus status,
       mojom::PaintPreviewBeginCompositeResponsePtr composite_response);
@@ -73,21 +106,26 @@ class PlayerCompositorDelegate {
 
   void OnCompositorClientCreated(const GURL& expected_url,
                                  const DirectoryKey& key);
+
   void OnCompositorClientDisconnected();
 
   void OnProtoAvailable(const GURL& expected_url,
                         PaintPreviewBaseService::ProtoReadStatus proto_status,
                         std::unique_ptr<PaintPreviewProto> proto);
+
   void SendCompositeRequest(
       mojom::PaintPreviewBeginCompositeRequestPtr begin_composite_request);
 
-  bool compress_on_close_;
+  PaintPreviewBaseService* paint_preview_service_{nullptr};
+  DirectoryKey key_;
+  bool compress_on_close_{true};
   std::unique_ptr<PaintPreviewCompositorService, base::OnTaskRunnerDeleter>
       paint_preview_compositor_service_;
   std::unique_ptr<PaintPreviewCompositorClient, base::OnTaskRunnerDeleter>
       paint_preview_compositor_client_;
   base::flat_map<base::UnguessableToken, std::unique_ptr<HitTester>>
       hit_testers_;
+  std::unique_ptr<PaintPreviewProto> proto_;
 
   base::WeakPtrFactory<PlayerCompositorDelegate> weak_factory_{this};
 };