Update PDFiumEngine::GetSelection() to return an optional value.

`PDFiumEngine::GetSelection()` did not distinguish between when nothing
was selected, and when an empty first page was selected.

Bug: 387387738
Change-Id: Ib5e7748b41183199682968610cef41fbf1c61305
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6218239
Commit-Queue: Ramin Halavati <rhalavati@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1413923}

pdf/accessibility_structs.h

@@ -438,6 +438,11 @@ struct PageCharacterIndex {
   uint32_t char_index = 0;
 };
 
+struct Selection {
+  PageCharacterIndex start;
+  PageCharacterIndex end;
+};
+
 struct AccessibilityActionData {
   AccessibilityActionData();
   AccessibilityActionData(

pdf/pdf_view_web_plugin.cc

@@ -2886,10 +2886,13 @@ void PdfViewWebPlugin::PrepareAndSetAccessibilityViewportInfo() {
       static_cast<int32_t>(engine_->GetCurrentOrientation());
   viewport_info.focus_info = {FocusObjectType::kNone, 0, 0};
 
-  engine_->GetSelection(&viewport_info.selection_start_page_index,
-                        &viewport_info.selection_start_char_index,
-                        &viewport_info.selection_end_page_index,
-                        &viewport_info.selection_end_char_index);
+  std::optional<Selection> selection = engine_->GetSelection();
+  if (selection.has_value()) {
+    viewport_info.selection_start_page_index = selection->start.page_index;
+    viewport_info.selection_start_char_index = selection->start.char_index;
+    viewport_info.selection_end_page_index = selection->end.page_index;
+    viewport_info.selection_end_char_index = selection->end.char_index;
+  }
 
   pdf_accessibility_data_handler_->SetAccessibilityViewportInfo(
       std::move(viewport_info));

pdf/pdfium/accessibility_unittest.cc

@@ -640,33 +640,26 @@ TEST_P(AccessibilityTest, GetAccessibilityTextFieldInfo) {
 }
 
 TEST_P(AccessibilityTest, SelectionActionHandling) {
-  struct Selection {
-    uint32_t start_page_index;
-    uint32_t start_char_index;
-    uint32_t end_page_index;
-    uint32_t end_char_index;
-  };
-
   struct TestCase {
     Selection action;
     Selection expected_result;
   };
 
   static constexpr TestCase kTestCases[] = {
-      {{0, 0, 0, 0}, {0, 0, 0, 0}},
-      {{0, 0, 1, 5}, {0, 0, 1, 5}},
+      {{{0, 0}, {0, 0}}, {{0, 0}, {0, 0}}},
+      {{{0, 0}, {1, 5}}, {{0, 0}, {1, 5}}},
       // Selection action data with invalid char index.
       // GetSelection() should return the previous selection in this case.
-      {{0, 0, 0, 50}, {0, 0, 1, 5}},
+      {{{0, 0}, {0, 50}}, {{0, 0}, {1, 5}}},
       // Selection action data for reverse selection where start selection
       // index is greater than end selection index. GetSelection() should
       // return the sanitized selection value where start selection index
       // is less than end selection index.
-      {{1, 10, 0, 5}, {0, 5, 1, 10}},
-      {{0, 10, 0, 4}, {0, 4, 0, 10}},
+      {{{1, 10}, {0, 5}}, {{0, 5}, {1, 10}}},
+      {{{0, 10}, {0, 4}}, {{0, 4}, {0, 10}}},
       // Selection action data with invalid page index.
       // GetSelection() should return the previous selection in this case.
-      {{0, 10, 2, 4}, {0, 4, 0, 10}},
+      {{{0, 10}, {2, 4}}, {{0, 4}, {0, 10}}},
   };
 
   TestClient client;
@@ -674,43 +667,35 @@ TEST_P(AccessibilityTest, SelectionActionHandling) {
       InitializeEngine(&client, FILE_PATH_LITERAL("hello_world2.pdf"));
   ASSERT_TRUE(engine);
 
+  // `GetSelection()` should return empty when nothing is selected.
+  EXPECT_FALSE(engine->GetSelection().has_value());
+
   for (const auto& test_case : kTestCases) {
     AccessibilityActionData action_data;
     action_data.action = AccessibilityAction::kSetSelection;
     const Selection& sel_action = test_case.action;
-    action_data.selection_start_index.page_index = sel_action.start_page_index;
-    action_data.selection_start_index.char_index = sel_action.start_char_index;
-    action_data.selection_end_index.page_index = sel_action.end_page_index;
-    action_data.selection_end_index.char_index = sel_action.end_char_index;
+    action_data.selection_start_index = sel_action.start;
+    action_data.selection_end_index = sel_action.end;
     action_data.target_rect = {{0, 0}, {0, 0}};
 
     engine->HandleAccessibilityAction(action_data);
-    Selection actual_selection;
-    engine->GetSelection(
-        &actual_selection.start_page_index, &actual_selection.start_char_index,
-        &actual_selection.end_page_index, &actual_selection.end_char_index);
+    std::optional<Selection> actual_selection = engine->GetSelection();
+    ASSERT_TRUE(actual_selection.has_value());
     const Selection& expected_selection = test_case.expected_result;
-    EXPECT_EQ(actual_selection.start_page_index,
-              expected_selection.start_page_index);
-    EXPECT_EQ(actual_selection.start_char_index,
-              expected_selection.start_char_index);
-    EXPECT_EQ(actual_selection.end_page_index,
-              expected_selection.end_page_index);
-    EXPECT_EQ(actual_selection.end_char_index,
-              expected_selection.end_char_index);
+    EXPECT_EQ(actual_selection->start.page_index,
+              expected_selection.start.page_index);
+    EXPECT_EQ(actual_selection->start.char_index,
+              expected_selection.start.char_index);
+    EXPECT_EQ(actual_selection->end.page_index,
+              expected_selection.end.page_index);
+    EXPECT_EQ(actual_selection->end.char_index,
+              expected_selection.end.char_index);
   }
 }
 
 // Tests if PP_PDF_SET_SELECTION updates scroll offsets if the selection is not
 // in the current visible rect.
 TEST_P(AccessibilityTest, SetSelectionAndScroll) {
-  struct Selection {
-    uint32_t start_page_index;
-    uint32_t start_char_index;
-    uint32_t end_page_index;
-    uint32_t end_char_index;
-  };
-
   struct TestCase {
     Selection action;
     Selection expected_result;
@@ -718,8 +703,8 @@ TEST_P(AccessibilityTest, SetSelectionAndScroll) {
   };
 
   static constexpr TestCase kTestCases[] = {
-      {{0, 15, 0, 15}, {0, 15, 0, 15}, {0, 0}},
-      {{1, 15, 1, 15}, {1, 15, 1, 15}, {28, 517}},
+      {{{0, 15}, {0, 15}}, {{0, 15}, {0, 15}}, {0, 0}},
+      {{{1, 15}, {1, 15}}, {{1, 15}, {1, 15}}, {28, 517}},
   };
 
   ScrollEnabledTestClient client;
@@ -733,32 +718,28 @@ TEST_P(AccessibilityTest, SetSelectionAndScroll) {
     AccessibilityActionData action_data;
     action_data.action = AccessibilityAction::kSetSelection;
     const Selection& sel_action = test_case.action;
-    action_data.selection_start_index.page_index = sel_action.start_page_index;
-    action_data.selection_start_index.char_index = sel_action.start_char_index;
-    action_data.selection_end_index.page_index = sel_action.end_page_index;
-    action_data.selection_end_index.char_index = sel_action.end_char_index;
+    action_data.selection_start_index = sel_action.start;
+    action_data.selection_end_index = sel_action.end;
 
     PDFiumPage& page =
-        GetPDFiumPageForTest(*engine, sel_action.start_page_index);
+        GetPDFiumPageForTest(*engine, sel_action.start.page_index);
     gfx::Rect char_bounds =
-        gfx::ToEnclosingRect(page.GetCharBounds(sel_action.start_char_index));
+        gfx::ToEnclosingRect(page.GetCharBounds(sel_action.start.char_index));
     action_data.target_rect = {{char_bounds.x(), char_bounds.y() + 400 * index},
                                char_bounds.size()};
 
     engine->HandleAccessibilityAction(action_data);
-    Selection actual_selection;
-    engine->GetSelection(
-        &actual_selection.start_page_index, &actual_selection.start_char_index,
-        &actual_selection.end_page_index, &actual_selection.end_char_index);
+    std::optional<Selection> actual_selection = engine->GetSelection();
+    ASSERT_TRUE(actual_selection.has_value());
     const Selection& expected_selection = test_case.expected_result;
-    EXPECT_EQ(actual_selection.start_page_index,
-              expected_selection.start_page_index);
-    EXPECT_EQ(actual_selection.start_char_index,
-              expected_selection.start_char_index);
-    EXPECT_EQ(actual_selection.end_page_index,
-              expected_selection.end_page_index);
-    EXPECT_EQ(actual_selection.end_char_index,
-              expected_selection.end_char_index);
+    EXPECT_EQ(actual_selection->start.page_index,
+              expected_selection.start.page_index);
+    EXPECT_EQ(actual_selection->start.char_index,
+              expected_selection.start.char_index);
+    EXPECT_EQ(actual_selection->end.page_index,
+              expected_selection.end.page_index);
+    EXPECT_EQ(actual_selection->end.char_index,
+              expected_selection.end.char_index);
     EXPECT_EQ(test_case.scroll_offset, client.GetScrollRequestDelta());
     index++;
   }

pdf/pdfium/pdfium_engine.cc

@@ -4119,33 +4119,28 @@ void PDFiumEngine::SetSelectionBounds(const gfx::Point& base,
                                    : RangeSelectionDirection::Right;
 }
 
-void PDFiumEngine::GetSelection(uint32_t* selection_start_page_index,
-                                uint32_t* selection_start_char_index,
-                                uint32_t* selection_end_page_index,
-                                uint32_t* selection_end_char_index) {
+std::optional<Selection> PDFiumEngine::GetSelection() const {
   size_t len = selection_.size();
   if (len == 0) {
-    *selection_start_page_index = 0;
-    *selection_start_char_index = 0;
-    *selection_end_page_index = 0;
-    *selection_end_char_index = 0;
-    return;
+    return std::nullopt;
   }
 
-  *selection_start_page_index = selection_[0].page_index();
-  *selection_start_char_index = selection_[0].char_index();
-  *selection_end_page_index = selection_[len - 1].page_index();
+  Selection selection;
+  selection.start.page_index = selection_[0].page_index();
+  selection.start.char_index = selection_[0].char_index();
+  selection.end.page_index = selection_[len - 1].page_index();
 
   // If the selection is all within one page, the end index is the
   // start index plus the char count. But if the selection spans
   // multiple pages, the selection starts at the beginning of the
   // last page in `selection_` and goes to the char count.
   if (len == 1) {
-    *selection_end_char_index =
+    selection.end.char_index =
         selection_[0].char_index() + selection_[0].char_count();
   } else {
-    *selection_end_char_index = selection_[len - 1].char_count();
+    selection.end.char_index = selection_[len - 1].char_count();
   }
+  return selection;
 }
 
 void PDFiumEngine::LoadDocumentAttachmentInfoList() {

pdf/pdfium/pdfium_engine.h

@@ -26,6 +26,7 @@
 #include "base/timer/timer.h"
 #include "base/values.h"
 #include "build/build_config.h"
+#include "pdf/accessibility_structs.h"
 #include "pdf/buildflags.h"
 #include "pdf/document_attachment_info.h"
 #include "pdf/document_layout.h"
@@ -347,10 +348,7 @@ class PDFiumEngine : public DocumentLoader::Client, public IFSDK_PAUSE {
 
   void SetSelectionBounds(const gfx::Point& base, const gfx::Point& extent);
 
-  void GetSelection(uint32_t* selection_start_page_index,
-                    uint32_t* selection_start_char_index,
-                    uint32_t* selection_end_page_index,
-                    uint32_t* selection_end_char_index);
+  std::optional<Selection> GetSelection() const;
 
   // Remove focus from form widgets, consolidating the user input.
   void KillFormFocus();