[Paint Preview] Refactor and test paint_preview_utils

paint_preview_utils will be used in an experiment. To this end it should
have test coverage and be refactored/cleaner. This also fixes a crash
that occurred in some situations.

Bug: 1049128
Change-Id: I66b031bd518996a13fdaedd437b80ce8c646bf8c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2038081Reviewed-by: Michael Thiessen <mthiesse@chromium.org>
Reviewed-by: Mehran Mahmoudi <mahmoudi@chromium.org>
Commit-Queue: Calder Kitagawa <ckitagawa@chromium.org>
Cr-Commit-Position: refs/heads/master@{#739065}

components/BUILD.gn

@@ -320,6 +320,7 @@ test("components_unittests") {
       "//components/invalidation/impl",
       "//components/invalidation/impl:java",
       "//components/paint_preview/browser/android:java",
+      "//components/paint_preview/browser/android:unit_tests",
       "//components/paint_preview/player/android:unit_tests",
       "//components/policy/android:policy_java",
       "//components/signin/core/browser",

components/paint_preview/browser/android/BUILD.gn

@@ -26,7 +26,10 @@ android_library("java") {
 }
 
 source_set("android") {
-  sources = [ "paint_preview_utils.cc" ]
+  sources = [
+    "paint_preview_utils.cc",
+    "paint_preview_utils.h",
+  ]
 
   deps = [
     ":jni_headers",
@@ -36,3 +39,24 @@ source_set("android") {
     "//content/public/browser",
   ]
 }
+
+source_set("unit_tests") {
+  testonly = true
+
+  sources = [ "paint_preview_utils_unittest.cc" ]
+
+  deps = [
+    ":android",
+    "//base",
+    "//base/test:test_support",
+    "//components/paint_preview/common:test_utils",
+    "//components/paint_preview/common/mojom",
+    "//content/public/browser",
+    "//content/test:test_support",
+    "//testing/gmock",
+    "//testing/gtest",
+    "//third_party/blink/public:blink_headers",
+    "//third_party/blink/public/common",
+    "//third_party/zlib/google:zip",
+  ]
+}

components/paint_preview/browser/android/paint_preview_utils.cc

@@ -2,102 +2,173 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "components/paint_preview/browser/android/paint_preview_utils.h"
+
 #include <jni.h>
 
+#include <utility>
+
 #include "base/bind.h"
-#include "base/files/file_path.h"
+#include "base/callback.h"
 #include "base/files/file_util.h"
 #include "base/logging.h"
 #include "base/task/post_task.h"
-#include "base/threading/scoped_blocking_call.h"
 #include "base/unguessable_token.h"
 #include "components/paint_preview/browser/android/jni_headers/PaintPreviewUtils_jni.h"
+#include "components/paint_preview/browser/file_manager.h"
+#include "components/paint_preview/browser/paint_preview_client.h"
 #include "components/paint_preview/buildflags/buildflags.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/browser/web_contents_user_data.h"
 
-#if BUILDFLAG(ENABLE_PAINT_PREVIEW)
-#include "components/paint_preview/browser/file_manager.h"
-#include "components/paint_preview/browser/paint_preview_client.h"
-#endif  // BUILDFLAG(ENABLE_PAINT_PREVIEW)
+namespace paint_preview {
 
 namespace {
 
 const char kPaintPreviewTestTag[] = "PaintPreviewTest ";
-
-#if BUILDFLAG(ENABLE_PAINT_PREVIEW)
 const char kPaintPreviewDir[] = "paint_preview";
 const char kCaptureTestDir[] = "capture_test";
+const char kProtoFilename[] = "proto.pb";
 
 struct CaptureMetrics {
   int compressed_size_bytes;
   int capture_time_us;
-  bool success;
 };
 
-void CleanupAndLogResult(const base::FilePath& root_dir,
-                         const CaptureMetrics& metrics) {
-  base::ScopedBlockingCall scoped_blocking_call(FROM_HERE,
-                                                base::BlockingType::MAY_BLOCK);
+void CleanupOnFailure(const base::FilePath& root_dir,
+                      FinishedCallback finished) {
+  VLOG(1) << kPaintPreviewTestTag << "Capture Failed\n";
   base::DeleteFileRecursively(root_dir);
-  VLOG(1) << kPaintPreviewTestTag
-          << "Capture Finished: " << ((metrics.success) ? "Success" : "Failure")
-          << "\n"
+  std::move(finished).Run(base::nullopt);
+}
+
+void CleanupAndLogResult(const base::FilePath& zip_path,
+                         const CaptureMetrics& metrics,
+                         FinishedCallback finished,
+                         bool keep_zip) {
+  VLOG(1) << kPaintPreviewTestTag << "Capture Finished Successfully:\n"
           << "Compressed size " << metrics.compressed_size_bytes << " bytes\n"
           << "Time taken in native " << metrics.capture_time_us << " us";
-}
 
-void MeasureSize(const base::FilePath& root_dir,
-                 const GURL& url,
-                 std::unique_ptr<paint_preview::PaintPreviewProto> proto,
-                 CaptureMetrics metrics) {
-  base::ScopedBlockingCall scoped_blocking_call(FROM_HERE,
-                                                base::BlockingType::MAY_BLOCK);
-  if (!metrics.success) {
-    metrics.success = false;
-    CleanupAndLogResult(root_dir, metrics);
-    return;
-  }
+  if (!keep_zip)
+    base::DeleteFileRecursively(zip_path.DirName());
+  std::move(finished).Run(zip_path);
+}
 
-  paint_preview::FileManager manager(root_dir);
+void CompressAndMeasureSize(const base::FilePath& root_dir,
+                            const GURL& url,
+                            std::unique_ptr<PaintPreviewProto> proto,
+                            CaptureMetrics metrics,
+                            FinishedCallback finished,
+                            bool keep_zip) {
+  FileManager manager(root_dir);
   base::FilePath path;
   bool success = manager.CreateOrGetDirectoryFor(url, &path);
   if (!success) {
-    VLOG(1) << kPaintPreviewTestTag << "Failure: could not create url dir.";
-    metrics.success = false;
-    CleanupAndLogResult(root_dir, metrics);
+    VLOG(1) << kPaintPreviewTestTag << "Failure: could not find url dir.";
+    CleanupOnFailure(root_dir, std::move(finished));
     return;
   }
-  base::File file(path,
+  base::File file(path.AppendASCII(kProtoFilename),
                   base::File::FLAG_CREATE_ALWAYS | base::File::FLAG_WRITE);
   std::string str_proto = proto->SerializeAsString();
   file.WriteAtCurrentPos(str_proto.data(), str_proto.size());
   manager.CompressDirectoryFor(url);
   metrics.compressed_size_bytes = manager.GetSizeOfArtifactsFor(url);
-  CleanupAndLogResult(root_dir, metrics);
-  return;
+
+  CleanupAndLogResult(path.AddExtensionASCII("zip"), metrics,
+                      std::move(finished), keep_zip);
 }
 
 void OnCaptured(base::TimeTicks start_time,
                 const base::FilePath& root_dir,
                 const GURL& url,
+                FinishedCallback finished,
+                bool keep_zip,
                 base::UnguessableToken guid,
-                paint_preview::mojom::PaintPreviewStatus status,
-                std::unique_ptr<paint_preview::PaintPreviewProto> proto) {
+                mojom::PaintPreviewStatus status,
+                std::unique_ptr<PaintPreviewProto> proto) {
   base::TimeDelta time_delta = base::TimeTicks::Now() - start_time;
-  CaptureMetrics result = {
-      0, time_delta.InMicroseconds(),
-      status == paint_preview::mojom::PaintPreviewStatus::kOk};
+
+  if (status != mojom::PaintPreviewStatus::kOk) {
+    base::PostTask(
+        FROM_HERE, {base::ThreadPool(), base::MayBlock()},
+        base::BindOnce(&CleanupOnFailure, root_dir, std::move(finished)));
+    return;
+  }
+
+  CaptureMetrics result = {0, time_delta.InMicroseconds()};
   base::PostTask(
       FROM_HERE, {base::ThreadPool(), base::MayBlock()},
-      base::BindOnce(&MeasureSize, root_dir, url, std::move(proto), result));
+      base::BindOnce(&CompressAndMeasureSize, root_dir, url, std::move(proto),
+                     result, std::move(finished), keep_zip));
+}
+
+base::Optional<base::FilePath> CreateDirectoryForURL(
+    const base::FilePath& root_path,
+    const GURL& url) {
+  base::FilePath url_path;
+  FileManager manager(root_path);
+  if (!manager.CreateOrGetDirectoryFor(url, &url_path)) {
+    VLOG(1) << kPaintPreviewTestTag << "Failure: could not create output dir.";
+    return base::nullopt;
+  }
+  return url_path;
+}
+
+void InitiateCapture(content::WebContents* contents,
+                     FinishedCallback finished,
+                     bool keep_zip,
+                     base::Optional<base::FilePath> url_path) {
+  if (!url_path.has_value()) {
+    std::move(finished).Run(base::nullopt);
+    return;
+  }
+
+  auto* client = PaintPreviewClient::FromWebContents(contents);
+  if (!client) {
+    VLOG(1) << kPaintPreviewTestTag << "Failure: client could not be created.";
+    CleanupOnFailure(url_path->DirName(), std::move(finished));
+    return;
+  }
+
+  PaintPreviewClient::PaintPreviewParams params;
+  params.document_guid = base::UnguessableToken::Create();
+  params.is_main_frame = true;
+  params.root_dir = url_path.value();
+
+  auto start_time = base::TimeTicks::Now();
+  client->CapturePaintPreview(
+      params, contents->GetMainFrame(),
+      base::BindOnce(&OnCaptured, start_time, params.root_dir.DirName(),
+                     contents->GetLastCommittedURL(), std::move(finished),
+                     keep_zip));
 }
-#endif  // BUILDFLAG(ENABLE_PAINT_PREVIEW)
 
 }  // namespace
 
+void Capture(content::WebContents* contents,
+             FinishedCallback finished,
+             bool keep_zip) {
+  PaintPreviewClient::CreateForWebContents(contents);
+
+  base::FilePath root_path = contents->GetBrowserContext()
+                                 ->GetPath()
+                                 .AppendASCII(kPaintPreviewDir)
+                                 .AppendASCII(kCaptureTestDir);
+
+  base::PostTaskAndReplyWithResult(
+      FROM_HERE, {base::ThreadPool(), base::MayBlock()},
+      base::BindOnce(&CreateDirectoryForURL, root_path,
+                     contents->GetLastCommittedURL()),
+      base::BindOnce(&InitiateCapture, base::Unretained(contents),
+                     std::move(finished), keep_zip));
+}
+
+}  // namespace paint_preview
+
 // If the ENABLE_PAINT_PREVIEW buildflags is set this method will trigger a
 // series of actions;
 // 1. Capture a paint preview via the client and measure the time taken.
@@ -110,37 +181,13 @@ static void JNI_PaintPreviewUtils_CapturePaintPreview(
     const base::android::JavaParamRef<jobject>& jweb_contents) {
 #if BUILDFLAG(ENABLE_PAINT_PREVIEW)
   auto* contents = content::WebContents::FromJavaWebContents(jweb_contents);
-  paint_preview::PaintPreviewClient::CreateForWebContents(contents);
-  auto* client = paint_preview::PaintPreviewClient::FromWebContents(contents);
-  if (!client) {
-    VLOG(1) << kPaintPreviewTestTag << "Failure: client could not be created.";
-    return;
-  }
-  paint_preview::PaintPreviewClient::PaintPreviewParams params;
-  params.document_guid = base::UnguessableToken::Create();
-  params.is_main_frame = true;
-  base::FilePath root_path = contents->GetBrowserContext()
-                                 ->GetPath()
-                                 .AppendASCII(kPaintPreviewDir)
-                                 .AppendASCII(kCaptureTestDir);
-  params.root_dir = root_path;
-  GURL url = contents->GetLastCommittedURL();
-  paint_preview::FileManager manager(root_path);
-  bool success = manager.CreateOrGetDirectoryFor(url, &params.root_dir);
-  if (!success) {
-    VLOG(1) << kPaintPreviewTestTag << "Failure: could not create output dir.";
-    return;
-  }
-  auto start_time = base::TimeTicks::Now();
-  client->CapturePaintPreview(
-      params, contents->GetMainFrame(),
-      base::BindOnce(&OnCaptured, start_time, root_path, url));
+  paint_preview::Capture(contents, base::DoNothing(), /* keep_zip= */ false);
 #else
   // In theory this is unreachable as the codepath to reach here is only exposed
   // if the buildflag for ENABLE_PAINT_PREVIEW is set. However, this function
   // will still be compiled as it is called from JNI so this is here as a
   // placeholder.
-  VLOG(1) << kPaintPreviewTestTag
+  VLOG(1) << paint_preview::kPaintPreviewTestTag
           << "Failure: compiled without buildflag ENABLE_PAINT_PREVIEW.";
 #endif  // BUILDFLAG(ENABLE_PAINT_PREVIEW)
 }

components/paint_preview/browser/android/paint_preview_utils.h

+// 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.
+
+#ifndef COMPONENTS_PAINT_PREVIEW_BROWSER_ANDROID_PAINT_PREVIEW_UTILS_H_
+#define COMPONENTS_PAINT_PREVIEW_BROWSER_ANDROID_PAINT_PREVIEW_UTILS_H_
+
+#include "base/callback_forward.h"
+#include "base/files/file_path.h"
+#include "base/optional.h"
+
+namespace content {
+class WebContents;
+}  // namespace content
+
+namespace paint_preview {
+
+using FinishedCallback =
+    base::OnceCallback<void(const base::Optional<base::FilePath>&)>;
+
+// Captures a paint preview of |contents|. On completion returns the path of the
+// zip archive in which the paint preview is stored via |finished| (or an empty
+// path if the capture failed). The zip archive will exist only if |keep_zip| is
+// true.
+void Capture(content::WebContents* contents,
+             FinishedCallback finished,
+             bool keep_zip);
+
+}  // namespace paint_preview
+
+#endif  // COMPONENTS_PAINT_PREVIEW_BROWSER_ANDROID_PAINT_PREVIEW_UTILS_H_

components/paint_preview/browser/android/paint_preview_utils_unittest.cc

+// 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.
+
+#include "components/paint_preview/browser/android/paint_preview_utils.h"
+
+#include <utility>
+
+#include "base/files/file_enumerator.h"
+#include "base/files/file_path.h"
+#include "base/files/file_util.h"
+#include "components/paint_preview/common/mojom/paint_preview_recorder.mojom.h"
+#include "content/public/browser/render_frame_host.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/test/test_renderer_host.h"
+#include "mojo/public/cpp/bindings/associated_receiver.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/blink/public/common/associated_interfaces/associated_interface_provider.h"
+#include "third_party/zlib/google/zip.h"
+
+namespace paint_preview {
+
+namespace {
+
+class MockPaintPreviewRecorder : public mojom::PaintPreviewRecorder {
+ public:
+  MockPaintPreviewRecorder() = default;
+  ~MockPaintPreviewRecorder() override = default;
+
+  MockPaintPreviewRecorder(const MockPaintPreviewRecorder&) = delete;
+  MockPaintPreviewRecorder& operator=(const MockPaintPreviewRecorder&) = delete;
+
+  void CapturePaintPreview(
+      mojom::PaintPreviewCaptureParamsPtr params,
+      mojom::PaintPreviewRecorder::CapturePaintPreviewCallback callback)
+      override {
+    std::move(callback).Run(status_, mojom::PaintPreviewCaptureResponse::New());
+  }
+
+  void SetResponseStatus(mojom::PaintPreviewStatus status) { status_ = status; }
+
+  void BindRequest(mojo::ScopedInterfaceEndpointHandle handle) {
+    binding_.Bind(mojo::PendingAssociatedReceiver<mojom::PaintPreviewRecorder>(
+        std::move(handle)));
+  }
+
+ private:
+  mojom::PaintPreviewStatus status_;
+  mojo::AssociatedReceiver<mojom::PaintPreviewRecorder> binding_{this};
+};
+
+base::Optional<base::FilePath> Unzip(const base::FilePath& zip) {
+  base::FilePath dst_path = zip.RemoveExtension();
+  if (!base::CreateDirectory(dst_path))
+    return base::nullopt;
+  if (!zip::Unzip(zip, dst_path))
+    return base::nullopt;
+  base::DeleteFileRecursively(zip);
+  return dst_path;
+}
+
+}  // namespace
+
+class PaintPreviewUtilsRenderViewHostTest
+    : public content::RenderViewHostTestHarness {
+ public:
+  PaintPreviewUtilsRenderViewHostTest() {}
+
+  PaintPreviewUtilsRenderViewHostTest(
+      const PaintPreviewUtilsRenderViewHostTest&) = delete;
+  PaintPreviewUtilsRenderViewHostTest& operator=(
+      const PaintPreviewUtilsRenderViewHostTest&) = delete;
+
+ protected:
+  void SetUp() override {
+    RenderViewHostTestHarness::SetUp();
+    content::RenderFrameHostTester::For(main_rfh())
+        ->InitializeRenderFrameIfNeeded();
+  }
+
+  void OverrideInterface(MockPaintPreviewRecorder* service) {
+    blink::AssociatedInterfaceProvider* remote_interfaces =
+        web_contents()->GetMainFrame()->GetRemoteAssociatedInterfaces();
+    remote_interfaces->OverrideBinderForTesting(
+        mojom::PaintPreviewRecorder::Name_,
+        base::BindRepeating(&MockPaintPreviewRecorder::BindRequest,
+                            base::Unretained(service)));
+  }
+};
+
+TEST_F(PaintPreviewUtilsRenderViewHostTest, CaptureSingleFrameAndKeep) {
+  auto* contents = content::WebContents::FromRenderFrameHost(main_rfh());
+
+  MockPaintPreviewRecorder service;
+  service.SetResponseStatus(mojom::PaintPreviewStatus::kOk);
+  OverrideInterface(&service);
+
+  base::RunLoop loop;
+  Capture(contents,
+          base::BindOnce(
+              [](base::OnceClosure quit,
+                 const base::Optional<base::FilePath>& maybe_zip_path) {
+                EXPECT_TRUE(maybe_zip_path.has_value());
+
+                const base::FilePath& zip_path = maybe_zip_path.value();
+                EXPECT_EQ(".zip", zip_path.Extension());
+                {
+                  base::ScopedAllowBlockingForTesting scope;
+                  EXPECT_TRUE(base::PathExists(zip_path));
+                  auto unzipped_path = Unzip(zip_path);
+                  EXPECT_TRUE(unzipped_path.has_value());
+
+                  base::FileEnumerator enumerate(unzipped_path.value(), false,
+                                                 base::FileEnumerator::FILES);
+                  size_t count = 0;
+                  bool has_proto = false;
+                  bool has_skp = false;
+                  for (base::FilePath name = enumerate.Next(); !name.empty();
+                       name = enumerate.Next(), ++count) {
+                    if (name.Extension() == ".skp")
+                      has_skp = true;
+                    if (name.BaseName().AsUTF8Unsafe() == "proto.pb")
+                      has_proto = true;
+                  }
+                  EXPECT_EQ(2U, count);
+                  EXPECT_TRUE(has_skp);
+                  EXPECT_TRUE(has_proto);
+                  base::DeleteFileRecursively(zip_path.DirName());
+                }
+                std::move(quit).Run();
+              },
+              loop.QuitClosure()),
+          true);
+  loop.Run();
+}
+
+TEST_F(PaintPreviewUtilsRenderViewHostTest, CaptureSingleFrameAndDelete) {
+  auto* contents = content::WebContents::FromRenderFrameHost(main_rfh());
+
+  MockPaintPreviewRecorder service;
+  service.SetResponseStatus(mojom::PaintPreviewStatus::kOk);
+  OverrideInterface(&service);
+
+  base::RunLoop loop;
+  Capture(contents,
+          base::BindOnce(
+              [](base::OnceClosure quit,
+                 const base::Optional<base::FilePath>& maybe_zip_path) {
+                EXPECT_TRUE(maybe_zip_path.has_value());
+                const base::FilePath& zip_path = maybe_zip_path.value();
+                {
+                  base::ScopedAllowBlockingForTesting scope;
+                  EXPECT_FALSE(base::PathExists(zip_path));
+                  EXPECT_FALSE(base::DirectoryExists(zip_path.DirName()));
+                }
+                std::move(quit).Run();
+              },
+              loop.QuitClosure()),
+          false);
+  loop.Run();
+}
+
+TEST_F(PaintPreviewUtilsRenderViewHostTest, SingleFrameFailure) {
+  auto* contents = content::WebContents::FromRenderFrameHost(main_rfh());
+
+  MockPaintPreviewRecorder service;
+  service.SetResponseStatus(mojom::PaintPreviewStatus::kFailed);
+  OverrideInterface(&service);
+
+  base::RunLoop loop;
+  Capture(contents,
+          base::BindOnce(
+              [](base::OnceClosure quit,
+                 const base::Optional<base::FilePath>& zip_path) {
+                EXPECT_FALSE(zip_path.has_value());
+                std::move(quit).Run();
+              },
+              loop.QuitClosure()),
+          false);
+  loop.Run();
+}
+
+}  // namespace paint_preview