[Paint Preview] Refactor compositor

A largely mechanical refactoring of the compositor. This has one minor
change where if a file deserializes to an invalid SkPicture it is
expunged from the tree.

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

components/services/paint_preview_compositor/BUILD.gn

@@ -14,6 +14,8 @@ static_library("paint_preview_compositor") {
     "paint_preview_compositor_impl.h",
     "paint_preview_frame.cc",
     "paint_preview_frame.h",
+    "skp_result.cc",
+    "skp_result.h",
   ]
 
   deps = [

components/services/paint_preview_compositor/paint_preview_compositor_impl.cc

@@ -10,8 +10,8 @@
 #include "base/trace_event/trace_event.h"
 #include "components/paint_preview/common/file_stream.h"
 #include "components/paint_preview/common/proto/paint_preview.pb.h"
-#include "components/paint_preview/common/serial_utils.h"
 #include "components/services/paint_preview_compositor/public/mojom/paint_preview_compositor.mojom.h"
+#include "components/services/paint_preview_compositor/skp_result.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "third_party/skia/include/core/SkCanvas.h"
 #include "third_party/skia/include/core/SkImageInfo.h"
@@ -19,6 +19,67 @@
 
 namespace paint_preview {
 
+namespace {
+
+base::flat_map<base::UnguessableToken, SkpResult> DeserializeAllFrames(
+    base::flat_map<base::UnguessableToken, base::File>* file_map) {
+  std::vector<std::pair<base::UnguessableToken, SkpResult>> results;
+  results.reserve(file_map->size());
+
+  for (auto& it : *file_map) {
+    if (!it.second.IsValid())
+      continue;
+
+    SkpResult result;
+    FileRStream rstream(std::move(it.second));
+    SkDeserialProcs procs = MakeDeserialProcs(&result.ctx);
+    result.skp = SkPicture::MakeFromStream(&rstream, &procs);
+    if (!result.skp || result.skp->cullRect().width() == 0 ||
+        result.skp->cullRect().height() == 0) {
+      continue;
+    }
+
+    results.push_back({it.first, std::move(result)});
+  }
+
+  return base::flat_map<base::UnguessableToken, SkpResult>(std::move(results));
+}
+
+base::Optional<PaintPreviewFrame> BuildFrame(
+    const base::UnguessableToken& token,
+    const PaintPreviewFrameProto& frame_proto,
+    const base::flat_map<base::UnguessableToken, SkpResult>& results) {
+  auto it = results.find(token);
+  if (it == results.end())
+    return base::nullopt;
+
+  const SkpResult& result = it->second;
+  PaintPreviewFrame frame;
+  frame.skp = result.skp;
+
+  for (const auto& id_pair : frame_proto.content_id_to_embedding_tokens()) {
+    // It is possible that subframes recorded in this map were not captured
+    // (e.g. renderer crash, closed, etc.). Missing subframes are allowable
+    // since having just the main frame is sufficient to create a preview.
+    auto rect_it = result.ctx.find(id_pair.content_id());
+    if (rect_it == result.ctx.end())
+      continue;
+
+    mojom::SubframeClipRect rect;
+    rect.frame_guid = base::UnguessableToken::Deserialize(
+        id_pair.embedding_token_high(), id_pair.embedding_token_low());
+    rect.clip_rect = rect_it->second;
+
+    if (!results.count(rect.frame_guid))
+      continue;
+
+    frame.subframe_clip_rects.push_back(rect);
+  }
+  return frame;
+}
+
+}  // namespace
+
 PaintPreviewCompositorImpl::PaintPreviewCompositorImpl(
     mojo::PendingReceiver<mojom::PaintPreviewCompositor> receiver,
     base::OnceClosure disconnect_handler) {
@@ -48,12 +109,17 @@ void PaintPreviewCompositorImpl::BeginComposite(
   PaintPreviewProto paint_preview;
   bool ok = paint_preview.ParseFromArray(mapping.memory(), mapping.size());
   if (!ok) {
+    DVLOG(1) << "Failed to parse proto.";
     std::move(callback).Run(
         mojom::PaintPreviewCompositor::Status::kDeserializingFailure,
         std::move(response));
     return;
   }
-  if (!AddFrame(paint_preview.root_frame(), &request->file_map, &response)) {
+  auto frames = DeserializeAllFrames(&request->file_map);
+
+  // Adding the root frame must succeed.
+  if (!AddFrame(paint_preview.root_frame(), frames, &response)) {
+    DVLOG(1) << "Root frame not found.";
     std::move(callback).Run(
         mojom::PaintPreviewCompositor::Status::kCompositingFailure,
         std::move(response));
@@ -62,8 +128,10 @@ void PaintPreviewCompositorImpl::BeginComposite(
   response->root_frame_guid = base::UnguessableToken::Deserialize(
       paint_preview.root_frame().embedding_token_high(),
       paint_preview.root_frame().embedding_token_low());
+
+  // Adding subframes is optional.
   for (const auto& subframe_proto : paint_preview.subframes())
-    AddFrame(subframe_proto, &request->file_map, &response);
+    AddFrame(subframe_proto, frames, &response);
 
   std::move(callback).Run(mojom::PaintPreviewCompositor::Status::kSuccess,
                           std::move(response));
@@ -78,6 +146,7 @@ void PaintPreviewCompositorImpl::BitmapForFrame(
   SkBitmap bitmap;
   auto frame_it = frames_.find(frame_guid);
   if (frame_it == frames_.end()) {
+    DVLOG(1) << "Frame not found for " << frame_guid.ToString();
     std::move(callback).Run(
         mojom::PaintPreviewCompositor::Status::kCompositingFailure, bitmap);
     return;
@@ -100,45 +169,18 @@ void PaintPreviewCompositorImpl::SetRootFrameUrl(const GURL& url) {
   url_ = url;
 }
 
-PaintPreviewFrame PaintPreviewCompositorImpl::DeserializeFrame(
-    const PaintPreviewFrameProto& frame_proto,
-    base::File file_handle) {
-  PaintPreviewFrame frame;
-  FileRStream rstream(std::move(file_handle));
-  DeserializationContext ctx;
-  SkDeserialProcs procs = MakeDeserialProcs(&ctx);
-
-  frame.skp = SkPicture::MakeFromStream(&rstream, &procs);
-
-  for (const auto& id_pair : frame_proto.content_id_to_embedding_tokens()) {
-    // It is possible that subframes recorded in this map were not captured
-    // (e.g. renderer crash, closed, etc.). Missing subframes are allowable
-    // since having just the main frame is sufficient to create a preview.
-    auto rect_it = ctx.find(id_pair.content_id());
-    if (rect_it == ctx.end())
-      continue;
-
-    mojom::SubframeClipRect rect;
-    rect.frame_guid = base::UnguessableToken::Deserialize(
-        id_pair.embedding_token_high(), id_pair.embedding_token_low());
-    rect.clip_rect = rect_it->second;
-    frame.subframe_clip_rects.push_back(rect);
-  }
-  return frame;
-}
-
 bool PaintPreviewCompositorImpl::AddFrame(
     const PaintPreviewFrameProto& frame_proto,
-    FileMap* file_map,
+    const base::flat_map<base::UnguessableToken, SkpResult>& skp_map,
     mojom::PaintPreviewBeginCompositeResponsePtr* response) {
   base::UnguessableToken guid = base::UnguessableToken::Deserialize(
       frame_proto.embedding_token_high(), frame_proto.embedding_token_low());
-  auto file_it = file_map->find(guid);
-  if (file_it == file_map->end() || !file_it->second.IsValid())
+
+  base::Optional<PaintPreviewFrame> maybe_frame =
+      BuildFrame(guid, frame_proto, skp_map);
+  if (!maybe_frame.has_value())
     return false;
-  PaintPreviewFrame frame =
-      DeserializeFrame(frame_proto, std::move(file_it->second));
-  file_map->erase(file_it);
+  const PaintPreviewFrame& frame = maybe_frame.value();
 
   auto frame_data = mojom::FrameData::New();
   SkRect sk_rect = frame.skp->cullRect();
@@ -148,7 +190,7 @@ bool PaintPreviewCompositorImpl::AddFrame(
     frame_data->subframes.push_back(subframe_clip_rect.Clone());
 
   (*response)->frames.insert({guid, std::move(frame_data)});
-  frames_.insert({guid, std::move(frame)});
+  frames_.insert({guid, std::move(maybe_frame.value())});
   return true;
 }
 

components/services/paint_preview_compositor/paint_preview_compositor_impl.h

@@ -10,6 +10,7 @@
 #include "base/callback.h"
 #include "base/containers/flat_map.h"
 #include "components/paint_preview/common/proto/paint_preview.pb.h"
+#include "components/paint_preview/common/serial_utils.h"
 #include "components/services/paint_preview_compositor/paint_preview_frame.h"
 #include "components/services/paint_preview_compositor/public/mojom/paint_preview_compositor.mojom.h"
 #include "mojo/public/cpp/bindings/pending_receiver.h"
@@ -19,6 +20,8 @@
 
 namespace paint_preview {
 
+struct SkpResult;
+
 class PaintPreviewCompositorImpl : public mojom::PaintPreviewCompositor {
  public:
   using FileMap = base::flat_map<base::UnguessableToken, base::File>;
@@ -45,16 +48,12 @@ class PaintPreviewCompositorImpl : public mojom::PaintPreviewCompositor {
   void SetRootFrameUrl(const GURL& url) override;
 
  private:
-  // Deserializes the contents of |file_handle| and associates it with the
-  // metadata in |frame_proto|.
-  PaintPreviewFrame DeserializeFrame(const PaintPreviewFrameProto& frame_proto,
-                                     base::File file_handle);
-
   // Adds |frame_proto| to |frames_| and copies required data into |response|.
   // Consumes the corresponding file in |file_map|. Returns true on success.
-  bool AddFrame(const PaintPreviewFrameProto& frame_proto,
-                FileMap* file_map,
-                mojom::PaintPreviewBeginCompositeResponsePtr* response);
+  bool AddFrame(
+      const PaintPreviewFrameProto& frame_proto,
+      const base::flat_map<base::UnguessableToken, SkpResult>& skp_map,
+      mojom::PaintPreviewBeginCompositeResponsePtr* response);
 
   mojo::Receiver<mojom::PaintPreviewCompositor> receiver_{this};
 

components/services/paint_preview_compositor/paint_preview_compositor_impl_unittest.cc

@@ -215,6 +215,11 @@ TEST(PaintPreviewCompositorTest, TestBeginComposite) {
   // results.
   file_map.erase(kSubframe_0_0_ID);
   expected_data.erase(kSubframe_0_0_ID);
+  // Remove the kSubframe_0_0_ID from the subframe list since it isn't
+  // available.
+  auto& vec = expected_data[kSubframe_0_ID]->subframes;
+  vec.front() = std::move(vec.back());
+  vec.pop_back();
 
   mojom::PaintPreviewBeginCompositeRequestPtr request =
       mojom::PaintPreviewBeginCompositeRequest::New();

components/services/paint_preview_compositor/skp_result.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/services/paint_preview_compositor/skp_result.h"
+
+namespace paint_preview {
+
+SkpResult::SkpResult() = default;
+SkpResult::~SkpResult() = default;
+
+SkpResult::SkpResult(SkpResult&& other) = default;
+SkpResult& SkpResult::operator=(SkpResult&& rhs) = default;
+
+}  // namespace paint_preview

components/services/paint_preview_compositor/skp_result.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_SERVICES_PAINT_PREVIEW_COMPOSITOR_SKP_RESULT_H_
+#define COMPONENTS_SERVICES_PAINT_PREVIEW_COMPOSITOR_SKP_RESULT_H_
+
+#include "components/paint_preview/common/serial_utils.h"
+#include "third_party/skia/include/core/SkPicture.h"
+#include "third_party/skia/include/core/SkRefCnt.h"
+
+namespace paint_preview {
+
+// Struct for containing an SkPicture and its deserialization context.
+struct SkpResult {
+  SkpResult();
+  ~SkpResult();
+
+  SkpResult(const SkpResult& other) = delete;
+  SkpResult& operator=(const SkpResult& rhs) = delete;
+
+  SkpResult(SkpResult&& other);
+  SkpResult& operator=(SkpResult&& rhs);
+
+  sk_sp<SkPicture> skp;
+  DeserializationContext ctx;
+};
+
+}  // namespace paint_preview
+
+#endif  // COMPONENTS_SERVICES_PAINT_PREVIEW_COMPOSITOR_SKP_RESULT_H_