[PDF Ink Signatures] Transform PdfInkModule invalidation areas

The invalidation area provided by PdfInkModule to its client is supposed
to be in screen coordinates.  This is done correctly for in-progress
strokes, but completed/erased strokes have been providing areas whose
values are in canonical coordinates.

This issue becomes noticeable when drawing of completed strokes happens
automatically because they are part of the in-memory PDF document, and
no longer explicitly drawn by PdfInkModule.

Add a new helper to convert a finished stroke's envelope into the screen
coordinates that are needed by PdfInkModule's client.

Bug: 375445386
Change-Id: Ie0276db5225b4b9b0e1b7c79e0792d1437105d04
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5960211
Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Alan Screen <awscreen@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1375995}

pdf/pdf_ink_conversions.cc

@@ -7,6 +7,7 @@
 #include "base/time/time.h"
 #include "third_party/ink/src/ink/brush/brush.h"
 #include "third_party/ink/src/ink/color/color.h"
+#include "third_party/ink/src/ink/geometry/point.h"
 #include "third_party/ink/src/ink/strokes/input/stroke_input.h"
 #include "ui/gfx/geometry/point_f.h"
 
@@ -17,7 +18,7 @@ ink::StrokeInput CreateInkStrokeInput(ink::StrokeInput::ToolType tool_type,
                                       base::TimeDelta elapsed_time) {
   return {
       .tool_type = tool_type,
-      .position = {position.x(), position.y()},
+      .position = InkPointFromGfxPoint(position),
       .elapsed_time = ink::Duration32::Seconds(
           static_cast<float>(elapsed_time.InSecondsF())),
   };
@@ -29,4 +30,8 @@ SkColor GetSkColorFromInkBrush(const ink::Brush& brush) {
   return SkColorSetARGB(rgba.a, rgba.r, rgba.g, rgba.b);
 }
 
+ink::Point InkPointFromGfxPoint(const gfx::PointF& point) {
+  return ink::Point(point.x(), point.y());
+}
+
 }  // namespace chrome_pdf

pdf/pdf_ink_conversions.h

@@ -6,6 +6,7 @@
 #define PDF_PDF_INK_CONVERSIONS_H_
 
 #include "base/time/time.h"
+#include "third_party/ink/src/ink/geometry/point.h"
 #include "third_party/ink/src/ink/strokes/input/stroke_input.h"
 #include "third_party/skia/include/core/SkColor.h"
 
@@ -25,6 +26,8 @@ ink::StrokeInput CreateInkStrokeInput(ink::StrokeInput::ToolType tool_type,
 
 SkColor GetSkColorFromInkBrush(const ink::Brush& brush);
 
+ink::Point InkPointFromGfxPoint(const gfx::PointF& point);
+
 }  // namespace chrome_pdf
 
 #endif  // PDF_PDF_INK_CONVERSIONS_H_

pdf/pdf_ink_module.cc

@@ -50,7 +50,6 @@
 #include "third_party/skia/include/core/SkColor.h"
 #include "ui/gfx/geometry/point_f.h"
 #include "ui/gfx/geometry/rect.h"
-#include "ui/gfx/geometry/rect_conversions.h"
 #include "ui/gfx/geometry/rect_f.h"
 #include "ui/gfx/geometry/skia_conversions.h"
 
@@ -92,11 +91,6 @@ ink::Rect GetEraserRect(const gfx::PointF& center, int distance_to_center) {
       {center.x() + distance_to_center, center.y() + distance_to_center});
 }
 
-gfx::Rect InkRectToEnclosingGfxRect(const ink::Rect& rect) {
-  return gfx::ToEnclosingRect(
-      gfx::RectF(rect.XMin(), rect.YMin(), rect.Width(), rect.Height()));
-}
-
 SkRect GetDrawPageClipRect(const gfx::Rect& content_rect,
                            const gfx::Vector2dF& origin_offset) {
   gfx::RectF clip_rect(content_rect);
@@ -582,14 +576,14 @@ bool PdfInkModule::EraseHelper(const gfx::PointF& position, int page_index) {
     CHECK(undo_redo_success);
   }
 
-  const std::optional<ink::Rect>& invalidate_rect =
-      invalidate_envelope.AsRect();
-  if (!invalidate_rect.has_value()) {
+  if (invalidate_envelope.IsEmpty()) {
     return false;
   }
 
-  // If `invalidate_rect` has a value, then something got erased.
-  client_->Invalidate(InkRectToEnclosingGfxRect(invalidate_rect.value()));
+  // If `invalidate_envelope` isn't empty, then something got erased.
+  client_->Invalidate(CanonicalInkEnvelopeToInvalidationScreenRect(
+      invalidate_envelope, client_->GetOrientation(),
+      client_->GetPageContentsRect(page_index), client_->GetZoom()));
   return true;
 }
 
@@ -837,10 +831,9 @@ void PdfInkModule::ApplyUndoRedoCommandsHelper(std::set<InkStrokeId> ids,
       ids.erase(id);
     }
 
-    const std::optional<ink::Rect>& invalidate_rect =
-        invalidate_envelope.AsRect();
-    CHECK(invalidate_rect.has_value());
-    client_->Invalidate(InkRectToEnclosingGfxRect(invalidate_rect.value()));
+    client_->Invalidate(CanonicalInkEnvelopeToInvalidationScreenRect(
+        invalidate_envelope, client_->GetOrientation(),
+        client_->GetPageContentsRect(page_index), client_->GetZoom()));
     client_->UpdateThumbnail(page_index);
 
     if (ids.empty()) {

pdf/pdf_ink_module_unittest.cc

@@ -1240,7 +1240,7 @@ TEST_F(PdfInkModuleUndoRedoTest, UndoRedoInvalidationsBasic) {
   // `ink::ModeledShape::Bounds()` looks like it is smaller than what would be
   // expected given the inputs.
   const gfx::Rect kInvalidationAreaUndoRedo(gfx::Point(8.0f, 13.0f),
-                                            gfx::Size(24.0f, 6.0f));
+                                            gfx::Size(25.0f, 7.0f));
   EXPECT_THAT(
       client().invalidations(),
       ElementsAre(kInvalidationAreaMouseDown, kInvalidationAreaMouseMove,
@@ -1277,11 +1277,8 @@ TEST_F(PdfInkModuleUndoRedoTest, UndoRedoInvalidationsScaledRotated90) {
   // entire stroke makes sense.  What is being returned by
   // `ink::ModeledShape::Bounds()` looks like it is smaller than what would be
   // expected given the inputs.
-  // TODO(crbug.com/375445386): Invalidation area for undo/redo should be
-  // similar to location & combined size of the invalidation areas that happened
-  // while stroking.
-  const gfx::Rect kInvalidationAreaUndoRedo(gfx::Point(6.0f, 43.0f),
-                                            gfx::Size(4.0f, 13.0f));
+  const gfx::Rect kInvalidationAreaUndoRedo(gfx::Point(7.0f, 12.0f),
+                                            gfx::Size(27.0f, 9.0f));
   EXPECT_THAT(
       client().invalidations(),
       ElementsAre(kInvalidationAreaMouseDown, kInvalidationAreaMouseMove,

pdf/pdf_ink_transform.cc

@@ -4,12 +4,62 @@
 
 #include "pdf/pdf_ink_transform.h"
 
+#include <algorithm>
+#include <optional>
+
 #include "base/check_op.h"
 #include "base/notreached.h"
+#include "third_party/ink/src/ink/geometry/envelope.h"
+#include "third_party/ink/src/ink/geometry/rect.h"
 #include "ui/gfx/geometry/rect.h"
+#include "ui/gfx/geometry/rect_conversions.h"
+#include "ui/gfx/geometry/rect_f.h"
+#include "ui/gfx/geometry/vector2d_f.h"
 
 namespace chrome_pdf {
 
+namespace {
+
+// Performs what is mostly an inverse operation of
+// `EventPositionToCanonicalPosition()`, to convert from canonical coordinates
+// to screen coordinates.  This is not a strict inverse because it
+// intentionally does not adjust for `page_content_rect` offset from origin.
+// This is due to its usage for invalidation calculations, where clients account
+// for that offset separately.
+gfx::PointF CanonicalPositionToScreenPosition(
+    const gfx::PointF& canonical_position,
+    PageOrientation orientation,
+    const gfx::Rect& page_content_rect,
+    float scale_factor) {
+  CHECK_GT(scale_factor, 0.0f);
+  CHECK(!page_content_rect.IsEmpty());
+  gfx::PointF screen_position = canonical_position;
+  screen_position.Scale(scale_factor);
+  switch (orientation) {
+    case PageOrientation::kOriginal:
+      // No further modification needed.
+      break;
+    case PageOrientation::kClockwise90:
+      screen_position.SetPoint(
+          page_content_rect.width() - screen_position.y() - 1,
+          screen_position.x());
+      break;
+    case PageOrientation::kClockwise180:
+      screen_position.SetPoint(
+          page_content_rect.width() - screen_position.x() - 1,
+          page_content_rect.height() - screen_position.y() - 1);
+      break;
+    case PageOrientation::kClockwise270:
+      screen_position.SetPoint(
+          screen_position.y(),
+          page_content_rect.height() - screen_position.x() - 1);
+      break;
+  }
+  return screen_position;
+}
+
+}  // namespace
+
 gfx::PointF EventPositionToCanonicalPosition(const gfx::PointF& event_position,
                                              PageOrientation orientation,
                                              const gfx::Rect& page_content_rect,
@@ -71,4 +121,28 @@ ink::AffineTransform GetInkRenderTransform(
   NOTREACHED();
 }
 
+gfx::Rect CanonicalInkEnvelopeToInvalidationScreenRect(
+    const ink::Envelope& envelope,
+    PageOrientation orientation,
+    const gfx::Rect& page_content_rect,
+    float scale_factor) {
+  const std::optional<ink::Rect>& ink_rect = envelope.AsRect();
+  CHECK(ink_rect.has_value());
+
+  gfx::PointF p1 = CanonicalPositionToScreenPosition(
+      gfx::PointF(ink_rect->XMin(), ink_rect->YMin()), orientation,
+      page_content_rect, scale_factor);
+  gfx::PointF p2 = CanonicalPositionToScreenPosition(
+      gfx::PointF(ink_rect->XMax(), ink_rect->YMax()), orientation,
+      page_content_rect, scale_factor);
+
+  // Width and height get +1 since both of the points are to be included in the
+  // area; otherwise it would be an open rectangle on two edges.
+  float x = std::min(p1.x(), p2.x());
+  float y = std::min(p1.y(), p2.y());
+  float w = std::max(p1.x(), p2.x()) - x + 1;
+  float h = std::max(p1.y(), p2.y()) - y + 1;
+  return gfx::ToEnclosingRect(gfx::RectF(x, y, w, h));
+}
+
 }  // namespace chrome_pdf

pdf/pdf_ink_transform.h

@@ -9,14 +9,18 @@
 #include "pdf/page_orientation.h"
 #include "third_party/ink/src/ink/geometry/affine_transform.h"
 #include "ui/gfx/geometry/point_f.h"
+#include "ui/gfx/geometry/rect.h"
 
 static_assert(BUILDFLAG(ENABLE_PDF_INK2), "ENABLE_PDF_INK2 not set to true");
 
 namespace gfx {
-class Rect;
 class Vector2dF;
 }  // namespace gfx
 
+namespace ink {
+class Envelope;
+}  // namespace ink
+
 namespace chrome_pdf {
 
 // Converts a screen-based event input position into a page-based CSS pixels
@@ -41,7 +45,7 @@ gfx::PointF EventPositionToCanonicalPosition(const gfx::PointF& event_position,
                                              const gfx::Rect& page_content_rect,
                                              float scale_factor);
 
-// Generate the affine transformation for rendering a page's strokes to the
+// Generates the affine transformation for rendering a page's strokes to the
 // screen, based on the page and its position within the viewport.  Parameters
 // are the same as for `EventPositionToCanonicalPosition()`, with the addition
 // of:
@@ -95,6 +99,17 @@ ink::AffineTransform GetInkRenderTransform(
     const gfx::Rect& page_content_rect,
     float scale_factor);
 
+// Converts `ink::Envelope` to screen coordinates as needed for invalidation.
+// Uses the same `orientation`, `page_content_rect`, and `scale_factor`
+// parameters as used in `EventPositionToCanonicalPosition()`.  This function
+// uses them in reverse, to convert canonical coordinates back to screen
+// coordinates.  The caller must provide a non-empty `envelope`.
+gfx::Rect CanonicalInkEnvelopeToInvalidationScreenRect(
+    const ink::Envelope& envelope,
+    PageOrientation orientation,
+    const gfx::Rect& page_content_rect,
+    float scale_factor);
+
 }  // namespace chrome_pdf
 
 #endif  // PDF_PDF_INK_TRANSFORM_H_

pdf/pdf_ink_transform_unittest.cc

@@ -7,10 +7,12 @@
 #include <array>
 
 #include "pdf/page_orientation.h"
+#include "pdf/pdf_ink_conversions.h"
 #include "pdf/test/pdf_ink_test_helpers.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/ink/src/ink/geometry/affine_transform.h"
+#include "third_party/ink/src/ink/geometry/envelope.h"
 #include "ui/gfx/geometry/point_f.h"
 #include "ui/gfx/geometry/rect.h"
 #include "ui/gfx/geometry/size.h"
@@ -281,4 +283,135 @@ TEST(PdfInkTransformTest, RenderTransformOffsetZoomScrolledClockwise90) {
               InkAffineTransformEq(0.0f, -2.0f, 137.0f, 2.0f, 0.0f, -4.0f));
 }
 
+TEST(PdfInkTransformTest,
+     CanonicalInkEnvelopeToInvalidationScreenRectIdentity) {
+  // Representation of page contents in screen coordinates without scale or
+  // rotation.
+  constexpr gfx::Rect kPageContentRect(kPageSizePortrait);
+
+  {
+    // Envelope that covers the entire page contents, in canonical coordinates.
+    ink::Envelope envelope(InkPointFromGfxPoint(kCanonicalPositionTopLeft));
+    envelope.Add(InkPointFromGfxPoint(kCanonicalPositionBottomRight));
+
+    // Invalidation rectangle for `envelope` should result in the same value as
+    // the entire page contents.
+    gfx::Rect screen_rect = CanonicalInkEnvelopeToInvalidationScreenRect(
+        envelope, PageOrientation::kOriginal, kPageContentRect, kScaleFactor1x);
+    EXPECT_EQ(screen_rect, kPageContentRect);
+  }
+
+  {
+    // Envelope that covers a portion of page contents, in canonical
+    // coordinates.
+    ink::Envelope envelope(InkPointFromGfxPoint({20.0f, 35.0f}));
+    envelope.Add(InkPointFromGfxPoint({40.0f, 45.0f}));
+
+    // Invalidation rectangle for `envelope` should result in save value as
+    // input.
+    gfx::Rect screen_rect = CanonicalInkEnvelopeToInvalidationScreenRect(
+        envelope, PageOrientation::kOriginal, kPageContentRect, kScaleFactor1x);
+    EXPECT_EQ(screen_rect, gfx::Rect(20.0f, 35.0f, 21.0f, 11.0f));
+  }
+}
+
+TEST(PdfInkTransformTest,
+     CanonicalInkEnvelopeToInvalidationScreenRectScaledAndRotated90) {
+  // Scaled and rotated representation of page contents in screen coordinates.
+  constexpr gfx::Rect kPageContentRect({0, 0}, kPageSizeLandscape2x);
+
+  {
+    // Envelope that covers the entire page contents, in canonical coordinates.
+    ink::Envelope envelope(InkPointFromGfxPoint(kCanonicalPositionTopLeft));
+    envelope.Add(InkPointFromGfxPoint(kCanonicalPositionBottomRight +
+                                      kCanonicalPositionHalf));
+
+    // Invalidation rectangle for `envelope` should be scaled and rotated,
+    // resulting in the same value as the entire page contents.
+    gfx::Rect screen_rect = CanonicalInkEnvelopeToInvalidationScreenRect(
+        envelope, PageOrientation::kClockwise90, kPageContentRect,
+        kScaleFactor2x);
+    EXPECT_EQ(screen_rect, kPageContentRect);
+  }
+
+  {
+    // Envelope that covers a portion of page contents, in canonical
+    // coordinates.
+    ink::Envelope envelope(InkPointFromGfxPoint({20.0f, 35.0f}));
+    envelope.Add(InkPointFromGfxPoint({40.0f, 45.0f}));
+
+    // Invalidation rectangle for `envelope` should be scaled and rotated.
+    gfx::Rect screen_rect = CanonicalInkEnvelopeToInvalidationScreenRect(
+        envelope, PageOrientation::kClockwise90, kPageContentRect,
+        kScaleFactor2x);
+    EXPECT_EQ(screen_rect, gfx::Rect(29.0f, 40.0f, 21.0f, 41.0f));
+  }
+}
+
+TEST(PdfInkTransformTest,
+     CanonicalInkEnvelopeToInvalidationScreenRectScaledAndRotated180) {
+  // Scaled and rotated representation of page contents in screen coordinates.
+  constexpr gfx::Rect kPageContentRect({0, 0}, kPageSizePortrait2x);
+
+  {
+    // Envelope that covers the entire page contents, in canonical coordinates.
+    ink::Envelope envelope(InkPointFromGfxPoint(kCanonicalPositionTopLeft));
+    envelope.Add(InkPointFromGfxPoint(kCanonicalPositionBottomRight +
+                                      kCanonicalPositionHalf));
+
+    // Invalidation rectangle for `envelope` should be scaled and rotated,
+    // resulting in the same value as the entire page contents.
+    gfx::Rect screen_rect = CanonicalInkEnvelopeToInvalidationScreenRect(
+        envelope, PageOrientation::kClockwise180, kPageContentRect,
+        kScaleFactor2x);
+    EXPECT_EQ(screen_rect, kPageContentRect);
+  }
+
+  {
+    // Envelope that covers a portion of page contents, in canonical
+    // coordinates.
+    ink::Envelope envelope(InkPointFromGfxPoint({20.0f, 35.0f}));
+    envelope.Add(InkPointFromGfxPoint({40.0f, 45.0f}));
+
+    // Invalidation rectangle for `envelope` should be scaled and rotated.
+    gfx::Rect screen_rect = CanonicalInkEnvelopeToInvalidationScreenRect(
+        envelope, PageOrientation::kClockwise180, kPageContentRect,
+        kScaleFactor2x);
+    EXPECT_EQ(screen_rect, gfx::Rect(19.0f, 29.0f, 41.0f, 21.0f));
+  }
+}
+
+TEST(PdfInkTransformTest,
+     CanonicalInkEnvelopeToInvalidationScreenRectScaledAndRotated270) {
+  // Scaled and rotated representation of page contents in screen coordinates.
+  constexpr gfx::Rect kPageContentRect({0, 0}, kPageSizeLandscape2x);
+
+  {
+    // Envelope that covers the entire page contents, in canonical coordinates.
+    ink::Envelope envelope(InkPointFromGfxPoint(kCanonicalPositionTopLeft));
+    envelope.Add(InkPointFromGfxPoint(kCanonicalPositionBottomRight +
+                                      kCanonicalPositionHalf));
+
+    // Invalidation rectangle for `envelope` should be scaled and rotated,
+    // resulting in the same value as the entire page contents.
+    gfx::Rect screen_rect = CanonicalInkEnvelopeToInvalidationScreenRect(
+        envelope, PageOrientation::kClockwise270, kPageContentRect,
+        kScaleFactor2x);
+    EXPECT_EQ(screen_rect, kPageContentRect);
+  }
+
+  {
+    // Envelope that covers a portion of page contents, in canonical
+    // coordinates.
+    ink::Envelope envelope(InkPointFromGfxPoint({20.0f, 35.0f}));
+    envelope.Add(InkPointFromGfxPoint({40.0f, 45.0f}));
+
+    // Invalidation rectangle for `envelope` should be scaled and rotated.
+    gfx::Rect screen_rect = CanonicalInkEnvelopeToInvalidationScreenRect(
+        envelope, PageOrientation::kClockwise270, kPageContentRect,
+        kScaleFactor2x);
+    EXPECT_EQ(screen_rect, gfx::Rect(70.0f, 19.0f, 21.0f, 41.0f));
+  }
+}
+
 }  // namespace chrome_pdf