[PDF Ink Signatures] Use custom cursor for drawing tools

Use GenerateToolCursor() in PdfInkModule to draw custom cursors that
match the color/size of the selected drawing tool.

Bug: 342602620
Change-Id: Ib05f8ca9ebf8575b9d57fde37b80e2df9c87af93
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5778151
Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com>
Commit-Queue: Lei Zhang <thestig@chromium.org>
Reviewed-by: Andy Phan <andyphan@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1341401}

pdf/pdf_ink_module.cc

@@ -36,9 +36,11 @@
 #include "pdf/input_utils.h"
 #include "pdf/pdf_features.h"
 #include "pdf/pdf_ink_brush.h"
+#include "pdf/pdf_ink_cursor.h"
 #include "pdf/pdf_ink_transform.h"
 #include "third_party/blink/public/common/input/web_input_event.h"
 #include "third_party/blink/public/common/input/web_mouse_event.h"
+#include "third_party/skia/include/core/SkBitmap.h"
 #include "third_party/skia/include/core/SkColor.h"
 #include "ui/gfx/geometry/point_f.h"
 #include "ui/gfx/geometry/rect.h"
@@ -557,6 +559,7 @@ void PdfInkModule::HandleSetAnnotationBrushMessage(
   if (brush_type_string == "eraser") {
     auto& eraser_state = current_tool_state_.emplace<EraserState>();
     eraser_state.eraser_size = size;
+    MaybeSetCursor();
     return;
   }
 
@@ -583,11 +586,13 @@ void PdfInkModule::HandleSetAnnotationBrushMessage(
   current_tool_state_.emplace<DrawingStrokeState>();
   drawing_stroke_state().brush =
       std::make_unique<PdfInkBrush>(brush_type.value(), params);
+  MaybeSetCursor();
 }
 
 void PdfInkModule::HandleSetAnnotationModeMessage(
     const base::Value::Dict& message) {
   enabled_ = message.FindBool("enable").value();
+  MaybeSetCursor();
 }
 
 std::vector<std::unique_ptr<InkInProgressStroke>>
@@ -763,6 +768,30 @@ void PdfInkModule::ApplyUndoRedoDiscards(
   }
 }
 
+void PdfInkModule::MaybeSetCursor() {
+  if (!enabled()) {
+    // Do nothing when disabled. The code outside of PdfInkModule will select a
+    // normal mouse cursor and switch to that.
+    return;
+  }
+
+  SkColor color;
+  float brush_size;
+  if (is_drawing_stroke()) {
+    const auto& ink_brush = drawing_stroke_state().brush->GetInkBrush();
+    color = ink_brush.GetColor();
+    brush_size = ink_brush.GetSize();
+  } else {
+    CHECK(is_erasing_stroke());
+    constexpr SkColor kEraserColor = SK_ColorWHITE;
+    color = kEraserColor;
+    brush_size = erasing_stroke_state().eraser_size;
+  }
+
+  client_->UpdateInkCursorImage(
+      GenerateToolCursor(color, CursorDiameterFromBrushSize(brush_size)));
+}
+
 PdfInkModule::DrawingStrokeState::DrawingStrokeState() = default;
 
 PdfInkModule::DrawingStrokeState::~DrawingStrokeState() = default;

pdf/pdf_ink_module.h

@@ -23,6 +23,7 @@
 #include "pdf/page_orientation.h"
 #include "pdf/pdf_ink_undo_redo_model.h"
 #include "third_party/abseil-cpp/absl/types/variant.h"
+#include "third_party/skia/include/core/SkBitmap.h"
 #include "ui/gfx/geometry/point_f.h"
 #include "ui/gfx/geometry/rect.h"
 
@@ -78,9 +79,6 @@ class PdfInkModule {
     // Gets current zoom factor.
     virtual float GetZoom() const = 0;
 
-    // Notifies the client that a stroke has finished drawing or erasing.
-    virtual void StrokeFinished() {}
-
     // Notifies the client to invalidate the `rect`.  Coordinates are
     // screen-based, based on the same viewport origin that was used to specify
     // the `blink::WebMouseEvent` positions during stroking.
@@ -89,6 +87,12 @@ class PdfInkModule {
     // Returns whether the page at `index` is visible or not.
     virtual bool IsPageVisible(int index) = 0;
 
+    // Notifies the client that a stroke has finished drawing or erasing.
+    virtual void StrokeFinished() {}
+
+    // Asks the client to change the cursor to `bitmap`.
+    virtual void UpdateInkCursorImage(SkBitmap bitmap) {}
+
     // Returns the 0-based page index for the given `point` if it is on a
     // visible page, or -1 if `point` is not on a visible page.
     virtual int VisiblePageIndexFromPoint(const gfx::PointF& point) = 0;
@@ -276,6 +280,8 @@ class PdfInkModule {
   void ApplyUndoRedoDiscards(
       const PdfInkUndoRedoModel::DiscardedDrawCommands& discards);
 
+  void MaybeSetCursor();
+
   const raw_ref<Client> client_;
 
   bool enabled_ = false;

pdf/pdf_ink_module_unittest.cc

@@ -33,8 +33,10 @@
 #include "ui/gfx/geometry/size.h"
 #include "ui/gfx/geometry/vector2d_f.h"
 
+using testing::_;
 using testing::ElementsAre;
 using testing::ElementsAreArray;
+using testing::InSequence;
 using testing::Pair;
 
 namespace chrome_pdf {
@@ -109,6 +111,8 @@ class FakeClient : public PdfInkModule::Client {
 
   void StrokeFinished() override { ++stroke_finished_count_; }
 
+  MOCK_METHOD(void, UpdateInkCursorImage, (SkBitmap bitmap), (override));
+
   int VisiblePageIndexFromPoint(const gfx::PointF& point) override {
     for (size_t i = 0; i < page_layouts_.size(); ++i) {
       if (IsPageVisible(i) && page_layouts_[i].Contains(point)) {
@@ -188,7 +192,7 @@ TEST_F(PdfInkModuleTest, UnknownMessage) {
 // Verify that a set eraser message sets the annotation brush to an eraser.
 TEST_F(PdfInkModuleTest, HandleSetAnnotationBrushMessageEraser) {
   EnableAnnotationMode();
-  EXPECT_EQ(true, ink_module().enabled());
+  EXPECT_TRUE(ink_module().enabled());
 
   base::Value::Dict message = CreateSetAnnotationBrushMessageForTesting(
       "eraser", /*size=*/2.5, nullptr);
@@ -204,7 +208,7 @@ TEST_F(PdfInkModuleTest, HandleSetAnnotationBrushMessageEraser) {
 // given params.
 TEST_F(PdfInkModuleTest, HandleSetAnnotationBrushMessagePen) {
   EnableAnnotationMode();
-  EXPECT_EQ(true, ink_module().enabled());
+  EXPECT_TRUE(ink_module().enabled());
 
   TestAnnotationBrushMessageParams message_params{/*color_r=*/10,
                                                   /*color_g=*/255,
@@ -226,7 +230,7 @@ TEST_F(PdfInkModuleTest, HandleSetAnnotationBrushMessagePen) {
 // highlighter, with the given params.
 TEST_F(PdfInkModuleTest, HandleSetAnnotationBrushMessageHighlighter) {
   EnableAnnotationMode();
-  EXPECT_EQ(true, ink_module().enabled());
+  EXPECT_TRUE(ink_module().enabled());
 
   TestAnnotationBrushMessageParams message_params{/*color_r=*/240,
                                                   /*color_g=*/133,
@@ -248,7 +252,7 @@ TEST_F(PdfInkModuleTest, HandleSetAnnotationBrushMessageHighlighter) {
 // brush.
 TEST_F(PdfInkModuleTest, HandleSetAnnotationBrushMessageColorZero) {
   EnableAnnotationMode();
-  EXPECT_EQ(true, ink_module().enabled());
+  EXPECT_TRUE(ink_module().enabled());
 
   TestAnnotationBrushMessageParams message_params{/*color_r=*/0, /*color_g=*/0,
                                                   /*color_b=*/0};
@@ -283,6 +287,58 @@ TEST_F(PdfInkModuleTest, HandleSetAnnotationModeMessage) {
   EXPECT_FALSE(ink_module().enabled());
 }
 
+TEST_F(PdfInkModuleTest, MaybeSetCursorWhenTogglingAnnotationMode) {
+  EXPECT_FALSE(ink_module().enabled());
+
+  EXPECT_CALL(client(), UpdateInkCursorImage(_))
+      .WillOnce(
+          [this](SkBitmap bitmap) { EXPECT_TRUE(ink_module().enabled()); });
+
+  base::Value::Dict message =
+      CreateSetAnnotationModeMessageForTesting(/*enable=*/true);
+  EXPECT_TRUE(ink_module().OnMessage(message));
+  EXPECT_TRUE(ink_module().enabled());
+
+  message.Set("enable", false);
+  EXPECT_TRUE(ink_module().OnMessage(message));
+  EXPECT_FALSE(ink_module().enabled());
+}
+
+TEST_F(PdfInkModuleTest, MaybeSetCursorWhenChangingBrushes) {
+  {
+    InSequence seq;
+    EXPECT_CALL(client(), UpdateInkCursorImage(_))
+        .WillOnce([](SkBitmap bitmap) {
+          EXPECT_EQ(6, bitmap.width());
+          EXPECT_EQ(6, bitmap.height());
+        });
+    EXPECT_CALL(client(), UpdateInkCursorImage(_))
+        .WillOnce([](SkBitmap bitmap) {
+          EXPECT_EQ(20, bitmap.width());
+          EXPECT_EQ(20, bitmap.height());
+        });
+    EXPECT_CALL(client(), UpdateInkCursorImage(_))
+        .WillOnce([](SkBitmap bitmap) {
+          EXPECT_EQ(10, bitmap.width());
+          EXPECT_EQ(10, bitmap.height());
+        });
+  }
+
+  EnableAnnotationMode();
+  EXPECT_TRUE(ink_module().enabled());
+
+  TestAnnotationBrushMessageParams message_params{/*color_r=*/0,
+                                                  /*color_g=*/255,
+                                                  /*color_b=*/0};
+  base::Value::Dict message = CreateSetAnnotationBrushMessageForTesting(
+      "pen", /*size=*/16.0, &message_params);
+  EXPECT_TRUE(ink_module().OnMessage(message));
+
+  message = CreateSetAnnotationBrushMessageForTesting("eraser", /*size=*/8.0,
+                                                      nullptr);
+  EXPECT_TRUE(ink_module().OnMessage(message));
+}
+
 class PdfInkModuleStrokeTest : public PdfInkModuleTest {
  protected:
   // Mouse locations used for `RunStrokeCheckTest()`.

pdf/pdf_view_web_plugin.cc

@@ -916,6 +916,14 @@ void PdfViewWebPlugin::NavigateToDestination(int page,
 }
 
 void PdfViewWebPlugin::UpdateCursor(ui::mojom::CursorType new_cursor_type) {
+#if BUILDFLAG(ENABLE_PDF_INK2)
+  if (ink_module_ && ink_module_->enabled()) {
+    // Block normal mouse cursor updates, so the cursor set by PdfInkModule
+    // while it is enabled does not get overwritten.
+    return;
+  }
+#endif
+
   cursor_ = new_cursor_type;
 }
 
@@ -2036,6 +2044,11 @@ void PdfViewWebPlugin::StrokeFinished() {
   client_->PostMessage(std::move(message));
 }
 
+void PdfViewWebPlugin::UpdateInkCursorImage(SkBitmap bitmap) {
+  gfx::Point hotspot(bitmap.width() / 2, bitmap.height() / 2);
+  cursor_ = ui::Cursor::NewCustom(std::move(bitmap), std::move(hotspot));
+}
+
 int PdfViewWebPlugin::VisiblePageIndexFromPoint(const gfx::PointF& point) {
   for (int i = 0; i < engine_->GetNumberOfPages(); ++i) {
     if (!IsPageVisible(i)) {

pdf/pdf_view_web_plugin.h

@@ -403,13 +403,14 @@ class PdfViewWebPlugin final : public PDFiumEngineClient,
                           int32_t page_object_index) override;
 
 #if BUILDFLAG(ENABLE_PDF_INK2)
-  // PdfInkModule:
+  // PdfInkModule::Client:
   PageOrientation GetOrientation() const override;
   gfx::Rect GetPageContentsRect(int index) override;
   gfx::Vector2dF GetViewportOriginOffset() override;
   float GetZoom() const override;
   bool IsPageVisible(int index) override;
   void StrokeFinished() override;
+  void UpdateInkCursorImage(SkBitmap bitmap) override;
   int VisiblePageIndexFromPoint(const gfx::PointF& point) override;
 #endif
 

pdf/pdf_view_web_plugin_unittest.cc

@@ -19,7 +19,6 @@
 #include "base/memory/weak_ptr.h"
 #include "base/run_loop.h"
 #include "base/task/single_thread_task_runner.h"
-#include "base/test/bind.h"
 #include "base/test/scoped_feature_list.h"
 #include "base/test/values_test_util.h"
 #include "base/time/time.h"
@@ -40,6 +39,7 @@
 #include "pdf/pdf_accessibility_image_fetcher.h"
 #include "pdf/pdf_features.h"
 #include "pdf/test/mock_web_associated_url_loader.h"
+#include "pdf/test/pdf_ink_test_helpers.h"
 #include "pdf/test/test_helpers.h"
 #include "pdf/test/test_pdfium_engine.h"
 #include "printing/metafile_skia.h"
@@ -94,7 +94,6 @@ using ::testing::ElementsAre;
 using ::testing::ElementsAreArray;
 using ::testing::Eq;
 using ::testing::InSequence;
-using ::testing::Invoke;
 using ::testing::IsEmpty;
 using ::testing::IsFalse;
 using ::testing::IsTrue;
@@ -2442,7 +2441,47 @@ TEST_F(PdfViewWebPluginPrintPreviewTest,
 }
 
 #if BUILDFLAG(ENABLE_PDF_INK2)
-using PdfViewWebPluginInkTest = PdfViewWebPluginTest;
+class PdfViewWebPluginInkTest : public PdfViewWebPluginTest {
+ private:
+  base::test::ScopedFeatureList feature_list_{features::kPdfInk2};
+};
+
+TEST_F(PdfViewWebPluginInkTest, UpdateCursor) {
+  UpdatePluginGeometryWithoutWaiting(2.0f, {0, 0, 20, 20});
+
+  ON_CALL(*engine_ptr_, HandleInputEvent)
+      .WillByDefault([this](const blink::WebInputEvent& event) -> bool {
+        plugin_->UpdateCursor(ui::mojom::CursorType::kPointer);
+        return false;
+      });
+
+  blink::WebMouseEvent mouse_event;
+  mouse_event.SetType(blink::WebInputEvent::Type::kMouseMove);
+  mouse_event.SetPositionInWidget(10.0f, 20.0f);
+
+  ui::Cursor cursor;
+  EXPECT_EQ(ui::mojom::CursorType::kNull, cursor.type());
+  EXPECT_EQ(blink::WebInputEventResult::kNotHandled,
+            plugin_->HandleInputEvent(
+                blink::WebCoalescedInputEvent(mouse_event, ui::LatencyInfo()),
+                &cursor));
+  EXPECT_EQ(ui::mojom::CursorType::kPointer, cursor.type());
+
+  plugin_->OnMessage(CreateSetAnnotationModeMessageForTesting(/*enable=*/true));
+  EXPECT_EQ(blink::WebInputEventResult::kNotHandled,
+            plugin_->HandleInputEvent(
+                blink::WebCoalescedInputEvent(mouse_event, ui::LatencyInfo()),
+                &cursor));
+  EXPECT_EQ(ui::mojom::CursorType::kCustom, cursor.type());
+
+  plugin_->OnMessage(
+      CreateSetAnnotationModeMessageForTesting(/*enable=*/false));
+  EXPECT_EQ(blink::WebInputEventResult::kNotHandled,
+            plugin_->HandleInputEvent(
+                blink::WebCoalescedInputEvent(mouse_event, ui::LatencyInfo()),
+                &cursor));
+  EXPECT_EQ(ui::mojom::CursorType::kPointer, cursor.type());
+}
 
 TEST_F(PdfViewWebPluginInkTest, VisiblePageIndexFromPoint) {
   ON_CALL(*engine_ptr_, GetPageContentsRect)