[Paint Preview] Remove PaintPreviewTracker scroll offset

- Formalize coordinate space of subframe placeholders to be in their parent's space.

Bug: 1009552
Change-Id: I75c44a648acc18696e019c669d850bc842823af4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2363875Reviewed-by: Calder Kitagawa <ckitagawa@chromium.org>
Commit-Queue: Calder Kitagawa <ckitagawa@chromium.org>
Cr-Commit-Position: refs/heads/master@{#801354}

components/paint_preview/common/paint_preview_tracker.cc

@@ -44,8 +44,7 @@ PaintPreviewTracker::PaintPreviewTracker(
     bool is_main_frame)
     : guid_(guid),
       embedding_token_(embedding_token),
-      is_main_frame_(is_main_frame),
-      scroll_(SkISize::Make(0, 0)) {}
+      is_main_frame_(is_main_frame) {}
 
 PaintPreviewTracker::~PaintPreviewTracker() {
   DCHECK(states_.empty());
@@ -96,8 +95,7 @@ uint32_t PaintPreviewTracker::CreateContentForRemoteFrame(
     const gfx::Rect& rect,
     const base::UnguessableToken& embedding_token) {
   sk_sp<SkPicture> pic = SkPicture::MakePlaceholder(
-      SkRect::MakeXYWH(rect.x() + scroll_.width(), rect.y() + scroll_.height(),
-                       rect.width(), rect.height()));
+      SkRect::MakeXYWH(rect.x(), rect.y(), rect.width(), rect.height()));
   const uint32_t content_id = pic->uniqueID();
   DCHECK(!base::Contains(picture_context_.content_id_to_embedding_token,
                          content_id));
@@ -106,10 +104,6 @@ uint32_t PaintPreviewTracker::CreateContentForRemoteFrame(
   return content_id;
 }
 
-void PaintPreviewTracker::SetScrollForFrame(const SkISize& scroll) {
-  scroll_ = scroll;
-}
-
 void PaintPreviewTracker::AddGlyphs(const SkTextBlob* blob) {
   if (!blob)
     return;
@@ -170,8 +164,8 @@ void PaintPreviewTracker::CustomDataToSkPictureCallback(SkCanvas* canvas,
   DCHECK(it != subframe_pics_.end());
 
   SkRect rect = it->second->cullRect();
-  SkMatrix matrix = SkMatrix::Translate(rect.x(), rect.y());
-  canvas->drawPicture(it->second, &matrix, nullptr);
+  SkMatrix subframe_offset = SkMatrix::Translate(rect.x(), rect.y());
+  canvas->drawPicture(it->second, &subframe_offset, nullptr);
 }
 
 void PaintPreviewTracker::MoveLinks(std::vector<mojom::LinkDataPtr>* out) {

components/paint_preview/common/paint_preview_tracker.h

@@ -62,9 +62,6 @@ class PaintPreviewTracker {
       const gfx::Rect& rect,
       const base::UnguessableToken& embedding_token);
 
-  // Sets the scroll position for the top layer frame.
-  void SetScrollForFrame(const SkISize& scroll);
-
   // Adds the glyphs in |blob| to the glyph usage tracker for the |blob|'s
   // associated typface.
   void AddGlyphs(const SkTextBlob* blob);
@@ -85,7 +82,8 @@ class PaintPreviewTracker {
   // be considered immutable.
 
   // Inserts the OOP subframe placeholder associated with |content_id| into
-  // |canvas|.
+  // |canvas|. The cull rect of the placeholder will encode the position and
+  // size of the the subframe in its parent's coordinate system.
   void CustomDataToSkPictureCallback(SkCanvas* canvas, uint32_t content_id);
 
   // Expose internal maps for use in MakeSerialProcs().
@@ -111,7 +109,6 @@ class PaintPreviewTracker {
   const base::Optional<base::UnguessableToken> embedding_token_;
   const bool is_main_frame_;
 
-  SkISize scroll_;
   SkMatrix matrix_;
   std::vector<SkMatrix> states_;
 

components/paint_preview/common/paint_preview_tracker_unittest.cc

@@ -4,12 +4,14 @@
 
 #include "components/paint_preview/common/paint_preview_tracker.h"
 
+#include "base/containers/flat_map.h"
 #include "base/unguessable_token.h"
 #include "components/paint_preview/common/serial_utils.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/skia/include/core/SkCanvas.h"
 #include "third_party/skia/include/core/SkPicture.h"
 #include "third_party/skia/include/core/SkPictureRecorder.h"
+#include "third_party/skia/include/core/SkRect.h"
 #include "third_party/skia/include/core/SkRefCnt.h"
 #include "third_party/skia/include/core/SkTextBlob.h"
 #include "ui/gfx/geometry/rect.h"
@@ -23,6 +25,36 @@ struct TestContext {
   bool was_called;
 };
 
+// A test canvas for checking that the pictures drawn to it have the cull rect
+// we expect them to.
+class ExpectSubframeCanvas : public SkCanvas {
+ public:
+  void onDrawPicture(const SkPicture* picture,
+                     const SkMatrix*,
+                     const SkPaint*) override {
+    drawn_pictures_.insert({picture->uniqueID(), picture->cullRect()});
+  }
+
+  void ExpectHasPicture(uint32_t expected_picture_id,
+                        const gfx::Rect& expected_bounds) {
+    auto it = drawn_pictures_.find(expected_picture_id);
+    if (it == drawn_pictures_.end()) {
+      ADD_FAILURE() << "Picture ID was not recorded.";
+      return;
+    }
+
+    SkIRect rect = it->second.round();
+    EXPECT_EQ(rect.x(), expected_bounds.x());
+    EXPECT_EQ(rect.y(), expected_bounds.y());
+    EXPECT_EQ(rect.width(), expected_bounds.width());
+    EXPECT_EQ(rect.height(), expected_bounds.height());
+  }
+
+ private:
+  // Map of picture id to expected bounds of pictures drawn into this canvas.
+  base::flat_map<uint32_t, SkRect> drawn_pictures_;
+};
+
 }  // namespace
 
 TEST(PaintPreviewTrackerTest, TestGetters) {
@@ -52,42 +84,10 @@ TEST(PaintPreviewTrackerTest, TestRemoteFramePlaceholderPicture) {
   EXPECT_EQ(context->content_id_to_embedding_token[content_id],
             kEmbeddingTokenChild);
 
-  SkPictureRecorder recorder;
-  SkCanvas* canvas = recorder.beginRecording(100, 100);
-  tracker.CustomDataToSkPictureCallback(canvas, content_id);
-  sk_sp<SkPicture> pic = recorder.finishRecordingAsPicture();
-
-  // TODO(crbug/1009552): find a good way to check that a filler picture was
-  // actually inserted into |pic|. This is difficult without using the
-  // underlying private picture record.
-}
-
-TEST(PaintPreviewTrackerTest, TestRemoteFramePlaceholderPictureWithScroll) {
-  const base::UnguessableToken kDocToken = base::UnguessableToken::Create();
-  const base::UnguessableToken kEmbeddingToken =
-      base::UnguessableToken::Create();
-  PaintPreviewTracker tracker(kDocToken, kEmbeddingToken, true);
-  tracker.SetScrollForFrame(SkISize::Make(10, 20));
-
-  const base::UnguessableToken kEmbeddingTokenChild =
-      base::UnguessableToken::Create();
-  gfx::Rect rect(50, 40, 30, 20);
-  uint32_t content_id =
-      tracker.CreateContentForRemoteFrame(rect, kEmbeddingTokenChild);
-  PictureSerializationContext* context =
-      tracker.GetPictureSerializationContext();
-  EXPECT_TRUE(context->content_id_to_embedding_token.count(content_id));
-  EXPECT_EQ(context->content_id_to_embedding_token[content_id],
-            kEmbeddingTokenChild);
-
-  SkPictureRecorder recorder;
-  SkCanvas* canvas = recorder.beginRecording(100, 100);
-  tracker.CustomDataToSkPictureCallback(canvas, content_id);
-  sk_sp<SkPicture> pic = recorder.finishRecordingAsPicture();
+  ExpectSubframeCanvas canvas;
+  tracker.CustomDataToSkPictureCallback(&canvas, content_id);
 
-  // TODO(crbug/1009552): find a good way to check that a filler picture was
-  // actually inserted into |pic|. This is difficult without using the
-  // underlying private picture record.
+  canvas.ExpectHasPicture(content_id, rect);
 }
 
 TEST(PaintPreviewTrackerTest, TestGlyphRunList) {

components/paint_preview/common/serial_utils.cc

@@ -18,10 +18,12 @@ namespace {
 #pragma pack(push, 1)
 struct SerializedRectData {
   uint32_t content_id;
-  int64_t x;
-  int64_t y;
-  int64_t width;
-  int64_t height;
+
+  // The size of the subframe in the local coordinates when it was drawn.
+  int64_t subframe_width;
+  int64_t subframe_height;
+
+  // The rect of the subframe in its parent frame's root coordinate system.
   int64_t transformed_x;
   int64_t transformed_y;
   int64_t transformed_width;
@@ -39,10 +41,10 @@ sk_sp<SkData> SerializePictureAsRectData(SkPicture* picture, void* ctx) {
   if (it == context->content_id_to_transformed_clip.end())
     return nullptr;
 
+  // This data originates from |PaintPreviewTracker|.
   const SkRect& transformed_cull_rect = it->second;
   SerializedRectData rect_data = {
-      picture->uniqueID(),           picture->cullRect().x(),
-      picture->cullRect().y(),       picture->cullRect().width(),
+      picture->uniqueID(),           picture->cullRect().width(),
       picture->cullRect().height(),  transformed_cull_rect.x(),
       transformed_cull_rect.y(),     transformed_cull_rect.width(),
       transformed_cull_rect.height()};
@@ -71,7 +73,7 @@ sk_sp<SkData> SerializeTypeface(SkTypeface* typeface, void* ctx) {
   return subset_data;
 }
 
-// Deserializies a clip rect for a subframe within the main SkPicture. These
+// Deserializes a clip rect for a subframe within the main SkPicture. These
 // represent subframes and require special decoding as they are custom data
 // rather than a valid SkPicture.
 // Precondition: the version of the SkPicture should be checked prior to
@@ -106,21 +108,19 @@ sk_sp<SkPicture> GetPictureFromDeserialContext(const void* data,
   memcpy(&rect_data, data, sizeof(rect_data));
   auto* context = reinterpret_cast<LoadedFramesDeserialContext*>(ctx);
 
-  auto it = context->subframes.find(rect_data.content_id);
-  if (it == context->subframes.end())
+  auto it = context->find(rect_data.content_id);
+  if (it == context->end())
     return MakeEmptyPicture();
 
   // Scroll and clip the subframe manually since the picture in |ctx| does not
   // encode this information.
-  SkRect subframe_bounds = SkRect::MakeWH(rect_data.width, rect_data.height);
-  subframe_bounds.offset(-context->scroll_offsets.width(),
-                         -context->scroll_offsets.height());
+  SkRect subframe_bounds =
+      SkRect::MakeWH(rect_data.subframe_width, rect_data.subframe_height);
   SkPictureRecorder recorder;
   SkCanvas* canvas = recorder.beginRecording(subframe_bounds);
   canvas->clipRect(subframe_bounds);
   SkMatrix apply_scroll_offsets = SkMatrix::Translate(
-      -context->scroll_offsets.width() - it->second.scroll_offsets.width(),
-      -context->scroll_offsets.height() - it->second.scroll_offsets.height());
+      -it->second.scroll_offsets.width(), -it->second.scroll_offsets.height());
   canvas->drawPicture(it->second.picture, &apply_scroll_offsets, nullptr);
   return recorder.finishRecordingAsPicture();
 }
@@ -147,9 +147,6 @@ FrameAndScrollOffsets::FrameAndScrollOffsets(const FrameAndScrollOffsets&) =
 FrameAndScrollOffsets& FrameAndScrollOffsets::operator=(
     const FrameAndScrollOffsets&) = default;
 
-LoadedFramesDeserialContext::LoadedFramesDeserialContext() = default;
-LoadedFramesDeserialContext::~LoadedFramesDeserialContext() = default;
-
 sk_sp<SkPicture> MakeEmptyPicture() {
   // Effectively a no-op.
   SkPictureRecorder rec;

components/paint_preview/common/serial_utils.h

@@ -69,20 +69,11 @@ struct FrameAndScrollOffsets {
   gfx::Size scroll_offsets;
 };
 
-// Deserialization context for |MakeDeserialProcs| that contains the information
-// needed to embed a subframe within its parent frame.
-struct LoadedFramesDeserialContext {
-  LoadedFramesDeserialContext();
-  ~LoadedFramesDeserialContext();
-
-  // The scroll offsets for the current frame.
-  gfx::Size scroll_offsets;
-
-  // Maps a content ID to a frame's picture. A frame's subframes should be
-  // loaded into this context before |MakeDeserialProcs| is called to ensure
-  // that the resulting |SkPicture| contains all subframes.
-  base::flat_map<uint32_t, FrameAndScrollOffsets> subframes;
-};
+// Maps a content ID to a frame's picture. A frame's subframes should be
+// loaded into this context before |MakeDeserialProcs| is called to ensure
+// that the resulting |SkPicture| contains all subframes.
+using LoadedFramesDeserialContext =
+    base::flat_map<uint32_t, FrameAndScrollOffsets>;
 
 // Creates a no-op SkPicture.
 sk_sp<SkPicture> MakeEmptyPicture();

components/paint_preview/renderer/paint_preview_recorder_impl.cc

@@ -187,7 +187,6 @@ void PaintPreviewRecorderImpl::CapturePaintPreviewInternal(
   auto tracker = std::make_unique<PaintPreviewTracker>(
       params->guid, frame->GetEmbeddingToken(), is_main_frame_);
   auto size = frame->GetScrollOffset();
-  tracker->SetScrollForFrame(SkISize::Make(size.width, size.height));
   response->scroll_offsets = gfx::Size(size.width, size.height);
 
   cc::PaintRecorder recorder;

components/services/paint_preview_compositor/paint_preview_compositor_impl.cc

@@ -337,9 +337,6 @@ sk_sp<SkPicture> PaintPreviewCompositorImpl::DeserializeFrameRecursive(
   // before the current frame is loaded to ensure the order of loading is
   // topologically sorted.
   LoadedFramesDeserialContext deserial_context;
-  deserial_context.scroll_offsets = gfx::Size(
-      frame_proto.has_scroll_offset_x() ? frame_proto.scroll_offset_x() : 0,
-      frame_proto.has_scroll_offset_y() ? frame_proto.scroll_offset_y() : 0);
 
   *subframe_failed = false;
   for (const auto& id_pair : frame_proto.content_id_to_embedding_tokens()) {
@@ -396,7 +393,7 @@ sk_sp<SkPicture> PaintPreviewCompositorImpl::DeserializeFrameRecursive(
                                      subframe_proto_it->has_scroll_offset_y()
                                          ? subframe_proto_it->scroll_offset_y()
                                          : 0);
-    deserial_context.subframes.insert({id_pair.content_id(), frame});
+    deserial_context.insert({id_pair.content_id(), frame});
   }
 
   auto recording_it = recording_map->find(frame_guid);

components/services/paint_preview_compositor/paint_preview_compositor_impl_unittest.cc

@@ -208,10 +208,6 @@ void PopulateFrameProto(
   // observing it.
   PaintPreviewTracker tracker(base::UnguessableToken::Create(), guid,
                               set_is_main_frame);
-  if (set_is_main_frame) {
-    tracker.SetScrollForFrame(
-        SkISize::Make(scroll_offsets.width(), scroll_offsets.height()));
-  }
   mojom::FrameDataPtr expected_frame_data = mojom::FrameData::New();
   expected_frame_data->scroll_extents = scroll_extents;
   expected_frame_data->scroll_offsets = scroll_offsets;