[Paint Preview] Refactor base service to use DirectoryKey

Follow up change to make PaintPreviewBaseService use DirectoryKey for
accessing protos. Also move the access methods to FileManager.


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

components/paint_preview/browser/android/paint_preview_utils.cc

@@ -36,7 +36,6 @@ namespace {
 const char kPaintPreviewTestTag[] = "PaintPreviewTest ";
 const char kPaintPreviewDir[] = "paint_preview";
 const char kCaptureTestDir[] = "capture_test";
-const char kProtoFilename[] = "proto.pb";
 
 struct CaptureMetrics {
   int compressed_size_bytes;
@@ -90,10 +89,11 @@ void CompressAndMeasureSize(const base::FilePath& root_dir,
     CleanupOnFailure(root_dir, std::move(finished));
     return;
   }
-  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());
+  if (!manager.SerializePaintPreviewProto(key, *proto)) {
+    VLOG(1) << kPaintPreviewTestTag << "Failure: could not write proto.";
+    CleanupOnFailure(root_dir, std::move(finished));
+    return;
+  }
   manager.CompressDirectory(key);
   metrics.compressed_size_bytes = manager.GetSizeOfArtifacts(key);
 

components/paint_preview/browser/file_manager.cc

@@ -8,12 +8,14 @@
 #include "base/files/file_util.h"
 #include "base/hash/hash.h"
 #include "base/strings/string_number_conversions.h"
+#include "components/paint_preview/common/file_utils.h"
 #include "third_party/zlib/google/zip.h"
 
 namespace paint_preview {
 
 namespace {
 
+constexpr char kProtoName[] = "proto.pb";
 constexpr char kZipExt[] = ".zip";
 
 }  // namespace
@@ -164,6 +166,22 @@ void FileManager::DeleteAll() {
   base::DeleteFileRecursively(root_directory_);
 }
 
+bool FileManager::SerializePaintPreviewProto(const DirectoryKey& key,
+                                             const PaintPreviewProto& proto) {
+  base::FilePath path;
+  if (!CreateOrGetDirectory(key, &path))
+    return false;
+  return WriteProtoToFile(path.AppendASCII(kProtoName), proto);
+}
+
+std::unique_ptr<PaintPreviewProto> FileManager::DeserializePaintPreviewProto(
+    const DirectoryKey& key) {
+  base::FilePath path;
+  if (!CreateOrGetDirectory(key, &path))
+    return nullptr;
+  return ReadProtoFromFile(path.AppendASCII(kProtoName));
+}
+
 FileManager::StorageType FileManager::GetPathForKey(const DirectoryKey& key,
                                                     base::FilePath* path) {
   base::FilePath directory_path =

components/paint_preview/browser/file_manager.h

@@ -9,6 +9,7 @@
 
 #include "base/files/file_path.h"
 #include "base/time/time.h"
+#include "components/paint_preview/common/proto/paint_preview.pb.h"
 #include "url/gurl.h"
 
 namespace paint_preview {
@@ -63,6 +64,15 @@ class FileManager {
   // Deletes all stored paint previews stored in the |root_directory_|.
   void DeleteAll();
 
+  // Serializes |proto| to the directory for |key|. Returns true on success.
+  bool SerializePaintPreviewProto(const DirectoryKey& key,
+                                  const PaintPreviewProto& proto);
+
+  // Deserializes PaintPreviewProto stored in |key|. Will return nullptr if not
+  // found or the proto cannot be parsed.
+  std::unique_ptr<PaintPreviewProto> DeserializePaintPreviewProto(
+      const DirectoryKey& key);
+
  private:
   enum StorageType {
     kNone = 0,

components/paint_preview/browser/file_manager_unittest.cc

@@ -4,10 +4,14 @@
 
 #include "components/paint_preview/browser/file_manager.h"
 
+#include <memory>
+
 #include "base/files/file.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
+#include "components/paint_preview/common/proto/paint_preview.pb.h"
+#include "components/paint_preview/common/test_utils.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
 
@@ -187,4 +191,28 @@ TEST(FileManagerTest, TestDeleteAll) {
   EXPECT_FALSE(base::PathExists(w3_directory));
 }
 
+TEST(FileManagerTest, HandleProto) {
+  base::ScopedTempDir temp_dir;
+  ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
+  FileManager manager(temp_dir.GetPath());
+  auto key = manager.CreateKey(1U);
+  base::FilePath path;
+  EXPECT_TRUE(manager.CreateOrGetDirectory(key, &path));
+
+  PaintPreviewProto original_proto;
+  auto* root_frame = original_proto.mutable_root_frame();
+  root_frame->set_embedding_token_low(0);
+  root_frame->set_embedding_token_high(0);
+  root_frame->set_is_main_frame(true);
+  root_frame->set_file_path(path.AppendASCII("0.skp").MaybeAsASCII());
+  auto* metadata = original_proto.mutable_metadata();
+  metadata->set_url(GURL("www.chromium.org").spec());
+
+  EXPECT_TRUE(manager.SerializePaintPreviewProto(key, original_proto));
+  EXPECT_TRUE(base::PathExists(path.AppendASCII("proto.pb")));
+  auto read_proto = manager.DeserializePaintPreviewProto(key);
+  EXPECT_NE(read_proto, nullptr);
+  EXPECT_THAT(*read_proto, EqualsProto(original_proto));
+}
+
 }  // namespace paint_preview

components/paint_preview/browser/paint_preview_base_service.cc

@@ -20,7 +20,6 @@
 #include "components/paint_preview/browser/file_manager.h"
 #include "components/paint_preview/browser/paint_preview_client.h"
 #include "components/paint_preview/browser/paint_preview_compositor_service_impl.h"
-#include "components/paint_preview/common/file_utils.h"
 #include "components/paint_preview/common/mojom/paint_preview_recorder.mojom.h"
 #include "content/public/browser/web_contents.h"
 #include "ui/gfx/geometry/rect.h"
@@ -65,18 +64,14 @@ PaintPreviewBaseService::~PaintPreviewBaseService() {
 }
 
 void PaintPreviewBaseService::GetCapturedPaintPreviewProto(
-    const GURL& url,
-    OnReadProtoCallback onReadProtoCallback) {
-  std::move(onReadProtoCallback).Run(nullptr);
-}
-
-void PaintPreviewBaseService::GetCapturedPaintPreviewProtoFromFile(
-    const base::FilePath& file_path,
-    OnReadProtoCallback onReadProtoCallback) {
-  base::ThreadPool::PostTaskAndReplyWithResult(
-      FROM_HERE, {base::MayBlock(), base::TaskPriority::USER_VISIBLE},
-      base::BindOnce(&ReadProtoFromFile, file_path),
-      base::BindOnce(std::move(onReadProtoCallback)));
+    const DirectoryKey& key,
+    OnReadProtoCallback on_read_proto_callback) {
+  base::PostTaskAndReplyWithResult(
+      FROM_HERE,
+      {base::ThreadPool(), base::MayBlock(), base::TaskPriority::USER_VISIBLE},
+      base::BindOnce(&FileManager::DeserializePaintPreviewProto,
+                     base::Unretained(GetFileManager()), key),
+      base::BindOnce(std::move(on_read_proto_callback)));
 }
 
 void PaintPreviewBaseService::CapturePaintPreview(

components/paint_preview/browser/paint_preview_base_service.h

@@ -75,20 +75,13 @@ class PaintPreviewBaseService : public KeyedService {
   // Returns whether the created service is off the record.
   bool IsOffTheRecord() const { return is_off_the_record_; }
 
-  // Acquires the PaintPreviewProto that is associated with |url| and sends it
-  // to |onReadProtoCallback|. Default implementation immediately sends nullptr
-  // to |onReadProtoCallback|. Implementers of this class should override this
-  // function. GetCapturedPaintPreviewProtoFromFile can be used if the proto is
-  // saved on disk.
+  // Acquires the PaintPreviewProto that is associated with |key| and sends it
+  // to |on_read_proto_callback|. The default implementation attempts to invoke
+  // GetFileManager()->DeserializePaintPreviewProto(). Derived classes may
+  // override this function; for example, the proto is cached in memory.
   virtual void GetCapturedPaintPreviewProto(
-      const GURL& url,
-      OnReadProtoCallback onReadProtoCallback);
-
-  // Asynchronously deserializes PaintPreviewProto from |file_path| and sends it
-  // to |onReadProtoCallback|.
-  void GetCapturedPaintPreviewProtoFromFile(
-      const base::FilePath& file_path,
-      OnReadProtoCallback onReadProtoCallback);
+      const DirectoryKey& key,
+      OnReadProtoCallback on_read_proto_callback);
 
   // The following methods both capture a Paint Preview; however, their behavior
   // and intended use is different. The first method is intended for capturing
@@ -98,7 +91,7 @@ class PaintPreviewBaseService : public KeyedService {
   // individual subframes and should be used for only a few use cases.
   //
   // NOTE: |root_dir| in the following methods should be created using
-  // GetFileManager()->CreateOrGetDirectoryFor(<GURL>). However, to provide
+  // GetFileManager()->CreateOrGetDirectoryFor(). However, to provide
   // flexibility in managing the lifetime of created objects and ease cleanup
   // if a capture fails the service implementation is responsible for
   // implementing this management and tracking the directories in existence.

components/paint_preview/common/file_utils.cc

@@ -12,6 +12,15 @@
 
 namespace paint_preview {
 
+bool WriteProtoToFile(const base::FilePath& file_path,
+                      const PaintPreviewProto& proto) {
+  // TODO(crbug.com/1046925): stream to file instead.
+  std::string proto_str;
+  if (!proto.SerializeToString(&proto_str))
+    return false;
+  return base::WriteFile(file_path, proto_str.data(), proto_str.size()) > 0;
+}
+
 std::unique_ptr<PaintPreviewProto> ReadProtoFromFile(
     const base::FilePath& file_path) {
   // TODO(crbug.com/1046925): Use ZeroCopyInputStream instead.

components/paint_preview/common/file_utils.h

@@ -13,6 +13,10 @@ namespace paint_preview {
 
 class PaintPreviewProto;
 
+// Writes |proto| to |file_path|. Returns false on failure.
+bool WriteProtoToFile(const base::FilePath& file_path,
+                      const PaintPreviewProto& proto);
+
 // Reads a PaintPreviewProto from |file_path|. Returns nullptr in case of
 // failure.
 std::unique_ptr<PaintPreviewProto> ReadProtoFromFile(

components/paint_preview/player/android/java/src/org/chromium/components/paintpreview/player/PlayerCompositorDelegateImpl.java

@@ -32,12 +32,12 @@ class PlayerCompositorDelegateImpl implements PlayerCompositorDelegate {
     private CompositorListener mCompositorListener;
     private long mNativePlayerCompositorDelegate;
 
-    PlayerCompositorDelegateImpl(PaintPreviewBaseService service, String url,
+    PlayerCompositorDelegateImpl(PaintPreviewBaseService service, String directoryKey,
             @Nonnull CompositorListener compositorListener) {
         mCompositorListener = compositorListener;
         if (service != null && service.getNativePaintPreviewBaseService() != 0) {
             mNativePlayerCompositorDelegate = PlayerCompositorDelegateImplJni.get().initialize(
-                    this, service.getNativePaintPreviewBaseService(), url);
+                    this, service.getNativePaintPreviewBaseService(), directoryKey);
         }
         // TODO(crbug.com/1021590): Handle initialization errors when
         // mNativePlayerCompositorDelegate == 0.
@@ -103,7 +103,7 @@ class PlayerCompositorDelegateImpl implements PlayerCompositorDelegate {
     @NativeMethods
     interface Natives {
         long initialize(PlayerCompositorDelegateImpl caller, long nativePaintPreviewBaseService,
-                String url);
+                String directoryKey);
         void destroy(long nativePlayerCompositorDelegateAndroid);
         void requestBitmap(long nativePlayerCompositorDelegateAndroid, UnguessableToken frameGuid,
                 Callback<Bitmap> bitmapCallback, Runnable errorCallback, float scaleFactor,

components/paint_preview/player/android/java/src/org/chromium/components/paintpreview/player/PlayerManager.java

@@ -29,9 +29,10 @@ public class PlayerManager {
     private PlayerFrameCoordinator mRootFrameCoordinator;
     private FrameLayout mHostView;
 
-    public PlayerManager(Context context, PaintPreviewBaseService service, String url) {
+    public PlayerManager(Context context, PaintPreviewBaseService service, String directoryKey) {
         mContext = context;
-        mDelegate = new PlayerCompositorDelegateImpl(service, url, this::onCompositorReady);
+        mDelegate =
+                new PlayerCompositorDelegateImpl(service, directoryKey, this::onCompositorReady);
         mHostView = new FrameLayout(mContext);
     }
 

components/paint_preview/player/android/javatests/src/org/chromium/components/paintpreview/player/PaintPreviewPlayerTest.java

@@ -29,7 +29,7 @@ public class PaintPreviewPlayerTest extends DummyUiActivityTestCase {
     // TODO(crbug.com/1049303) Change to test data directory when test Proto and SKP files are
     //  added.
     private static final String TEST_DATA_DIR = Environment.getExternalStorageDirectory().getPath();
-    private static final String TEST_URL = "https://www.google.com";
+    private static final String TEST_DIRECTORY_KEY = "test_key";
 
     @Rule
     public PaintPreviewTestRule mPaintPreviewTestRule = new PaintPreviewTestRule();
@@ -44,7 +44,7 @@ public class PaintPreviewPlayerTest extends DummyUiActivityTestCase {
     public void smokeTest() {
         PostTask.postTask(UiThreadTaskTraits.DEFAULT, () -> {
             TestImplementerService service = new TestImplementerService(TEST_DATA_DIR);
-            mPlayerManager = new PlayerManager(getActivity(), service, TEST_URL);
+            mPlayerManager = new PlayerManager(getActivity(), service, TEST_DIRECTORY_KEY);
         });
         CriteriaHelper.pollUiThread(() -> mPlayerManager != null,
                 "PlayerManager took too long to initialize.", TIMEOUT_MS,

components/paint_preview/player/android/player_compositor_delegate_android.cc

@@ -52,12 +52,12 @@ jlong JNI_PlayerCompositorDelegateImpl_Initialize(
     JNIEnv* env,
     const JavaParamRef<jobject>& j_object,
     jlong paint_preview_service,
-    const JavaParamRef<jstring>& j_string_url) {
+    const JavaParamRef<jstring>& j_directory_key) {
   PlayerCompositorDelegateAndroid* delegate =
       new PlayerCompositorDelegateAndroid(
           env, j_object,
           reinterpret_cast<PaintPreviewBaseService*>(paint_preview_service),
-          j_string_url);
+          j_directory_key);
   return reinterpret_cast<intptr_t>(delegate);
 }
 
@@ -65,10 +65,11 @@ PlayerCompositorDelegateAndroid::PlayerCompositorDelegateAndroid(
     JNIEnv* env,
     const JavaParamRef<jobject>& j_object,
     PaintPreviewBaseService* paint_preview_service,
-    const JavaParamRef<jstring>& j_string_url)
+    const JavaParamRef<jstring>& j_directory_key)
     : PlayerCompositorDelegate(
           paint_preview_service,
-          GURL(base::android::ConvertJavaStringToUTF16(env, j_string_url))) {
+          DirectoryKey{
+              base::android::ConvertJavaStringToUTF8(env, j_directory_key)}) {
   java_ref_.Reset(env, j_object);
 }
 

components/paint_preview/player/player_compositor_delegate.cc

@@ -26,7 +26,6 @@
 #include "mojo/public/cpp/bindings/remote.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "ui/gfx/geometry/rect.h"
-#include "url/gurl.h"
 
 namespace paint_preview {
 namespace {
@@ -100,7 +99,7 @@ PrepareCompositeRequest(const paint_preview::PaintPreviewProto& proto) {
 
 PlayerCompositorDelegate::PlayerCompositorDelegate(
     PaintPreviewBaseService* paint_preview_service,
-    const GURL& url)
+    const DirectoryKey& key)
     : paint_preview_service_(paint_preview_service) {
   paint_preview_compositor_service_ =
       paint_preview_service_->StartCompositorService(base::BindOnce(
@@ -110,8 +109,7 @@ PlayerCompositorDelegate::PlayerCompositorDelegate(
   paint_preview_compositor_client_ =
       paint_preview_compositor_service_->CreateCompositor(
           base::BindOnce(&PlayerCompositorDelegate::OnCompositorClientCreated,
-                         weak_factory_.GetWeakPtr(), url));
-
+                         weak_factory_.GetWeakPtr(), key));
   paint_preview_compositor_client_->SetDisconnectHandler(
       base::BindOnce(&PlayerCompositorDelegate::OnCompositorClientDisconnected,
                      weak_factory_.GetWeakPtr()));
@@ -121,10 +119,10 @@ void PlayerCompositorDelegate::OnCompositorServiceDisconnected() {
   // TODO(crbug.com/1039699): Handle compositor service disconnect event.
 }
 
-void PlayerCompositorDelegate::OnCompositorClientCreated(const GURL& url) {
-  paint_preview_compositor_client_->SetRootFrameUrl(url);
+void PlayerCompositorDelegate::OnCompositorClientCreated(
+    const DirectoryKey& key) {
   paint_preview_service_->GetCapturedPaintPreviewProto(
-      url, base::BindOnce(&PlayerCompositorDelegate::OnProtoAvailable,
+      key, base::BindOnce(&PlayerCompositorDelegate::OnProtoAvailable,
                           weak_factory_.GetWeakPtr()));
 }
 
@@ -134,6 +132,8 @@ void PlayerCompositorDelegate::OnProtoAvailable(
     // TODO(crbug.com/1021590): Handle initialization errors.
     return;
   }
+  paint_preview_compositor_client_->SetRootFrameUrl(
+      GURL(proto->metadata().url()));
 
   base::PostTaskAndReplyWithResult(
       FROM_HERE,

components/paint_preview/player/player_compositor_delegate.h

@@ -21,14 +21,15 @@ class Rect;
 }  // namespace gfx
 
 class SkBitmap;
-class GURL;
 
 namespace paint_preview {
 
+struct DirectoryKey;
+
 class PlayerCompositorDelegate {
  public:
   PlayerCompositorDelegate(PaintPreviewBaseService* paint_preview_service,
-                           const GURL& url);
+                           const DirectoryKey& key);
 
   virtual void OnCompositorReady(
       mojom::PaintPreviewCompositor::Status status,
@@ -52,7 +53,7 @@ class PlayerCompositorDelegate {
  private:
   void OnCompositorServiceDisconnected();
 
-  void OnCompositorClientCreated(const GURL& url);
+  void OnCompositorClientCreated(const DirectoryKey& key);
   void OnCompositorClientDisconnected();
 
   void OnProtoAvailable(std::unique_ptr<PaintPreviewProto> proto);