Clean up constructors in pdf_accessibility_shared.h.

- Replace PdfAccessibilityLinkInfo's copy ctor with a move ctor. This
  implicitly deletes the copy ctor. Without a copy ctor, all the usage
  of the struct have to use move semantics correctly and cannot copy
  anymore.
- Mark the PdfAccessibilityLinkInfo ctor that takes a single
  PP_PrivateAccessibilityLinkInfo as explicit.
- Change the PP_PrivateAccessibilityLinkInfo ctor to use an initializer
  list.
- Reorder the ctors so the move ctor and dtor are at the end. Make
  pdf_accessibility_shared.cc match the header.

Then do the same set of changes for PdfAccessibilityImageInfo.

Bug: 981448
Change-Id: Ie3daca03b4226748daed7d39c31444da2a9de47c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1794248
Commit-Queue: Lei Zhang <thestig@chromium.org>
Reviewed-by: Raymes Khoury <raymes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#697137}

components/pdf/renderer/pdf_accessibility_tree_browsertest.cc

@@ -19,6 +19,7 @@
 #include "ui/base/resource/resource_bundle.h"
 
 namespace pdf {
+
 namespace {
 
 const PP_PrivateAccessibilityTextRunInfo kFirstTextRun = {
@@ -41,6 +42,8 @@ const PP_PrivateAccessibilityTextRunInfo kThirdRunMultiLine = {
 const PP_PrivateAccessibilityTextRunInfo kFourthRunMultiLine = {
     6, 12, PP_MakeFloatRectFromXYWH(26.0f, 189.0f, 84.0f, 13.0f)};
 
+const char kChromiumTestUrl[] = "www.cs.chromium.org";
+
 void CompareRect(PP_Rect expected_rect, PP_Rect actual_rect) {
   EXPECT_EQ(expected_rect.point.x, actual_rect.point.x);
   EXPECT_EQ(expected_rect.point.y, actual_rect.point.y);
@@ -217,23 +220,29 @@ TEST_F(PdfAccessibilityTreeTest, TestAccessibilityDisabledDuringPDFLoad) {
 }
 
 TEST_F(PdfAccessibilityTreeTest, TestPdfAccessibilityTreeCreation) {
+  static const char kTestAltText[] = "Alternate text for image";
+
   text_runs_.emplace_back(kFirstTextRun);
   text_runs_.emplace_back(kSecondTextRun);
   chars_.insert(chars_.end(), std::begin(kDummyCharsData),
                 std::end(kDummyCharsData));
 
-  ppapi::PdfAccessibilityLinkInfo link;
-  link.bounds = PP_MakeFloatRectFromXYWH(0.0f, 0.0f, 0.0f, 0.0f);
-  link.url = "www.cs.chromium.org";
-  link.text_run_index = 0;
-  link.text_run_count = 1;
-  links_.push_back(std::move(link));
+  {
+    ppapi::PdfAccessibilityLinkInfo link;
+    link.bounds = PP_MakeFloatRectFromXYWH(0.0f, 0.0f, 0.0f, 0.0f);
+    link.url = kChromiumTestUrl;
+    link.text_run_index = 0;
+    link.text_run_count = 1;
+    links_.push_back(std::move(link));
+  }
 
-  ppapi::PdfAccessibilityImageInfo image;
-  image.bounds = PP_MakeFloatRectFromXYWH(0.0f, 0.0f, 0.0f, 0.0f);
-  image.alt_text = "Alternate text for image";
-  image.text_run_index = 2;
-  images_.push_back(image);
+  {
+    ppapi::PdfAccessibilityImageInfo image;
+    image.bounds = PP_MakeFloatRectFromXYWH(0.0f, 0.0f, 0.0f, 0.0f);
+    image.alt_text = kTestAltText;
+    image.text_run_index = 2;
+    images_.push_back(std::move(image));
+  }
 
   page_info_.text_run_count = text_runs_.size();
   page_info_.char_count = chars_.size();
@@ -281,7 +290,7 @@ TEST_F(PdfAccessibilityTreeTest, TestPdfAccessibilityTreeCreation) {
 
   ui::AXNode* link_node = paragraph_node->children()[0];
   ASSERT_TRUE(link_node);
-  EXPECT_EQ(link.url,
+  EXPECT_EQ(kChromiumTestUrl,
             link_node->GetStringAttribute(ax::mojom::StringAttribute::kUrl));
   EXPECT_EQ(ax::mojom::Role::kLink, link_node->data().role);
   ASSERT_EQ(1u, link_node->children().size());
@@ -299,7 +308,7 @@ TEST_F(PdfAccessibilityTreeTest, TestPdfAccessibilityTreeCreation) {
   ui::AXNode* image_node = paragraph_node->children()[1];
   ASSERT_TRUE(image_node);
   EXPECT_EQ(ax::mojom::Role::kImage, image_node->data().role);
-  EXPECT_EQ(image.alt_text,
+  EXPECT_EQ(kTestAltText,
             image_node->GetStringAttribute(ax::mojom::StringAttribute::kName));
 }
 
@@ -311,12 +320,14 @@ TEST_F(PdfAccessibilityTreeTest, TestPreviousNextOnLine) {
   chars_.insert(chars_.end(), std::begin(kDummyCharsData),
                 std::end(kDummyCharsData));
 
-  ppapi::PdfAccessibilityLinkInfo link;
-  link.bounds = PP_MakeFloatRectFromXYWH(0.0f, 0.0f, 0.0f, 0.0f);
-  link.url = "www.cs.chromium.org";
-  link.text_run_index = 2;
-  link.text_run_count = 2;
-  links_.push_back(std::move(link));
+  {
+    ppapi::PdfAccessibilityLinkInfo link;
+    link.bounds = PP_MakeFloatRectFromXYWH(0.0f, 0.0f, 0.0f, 0.0f);
+    link.url = kChromiumTestUrl;
+    link.text_run_index = 2;
+    link.text_run_count = 2;
+    links_.push_back(std::move(link));
+  }
 
   page_info_.text_run_count = text_runs_.size();
   page_info_.char_count = chars_.size();
@@ -390,7 +401,7 @@ TEST_F(PdfAccessibilityTreeTest, TestPreviousNextOnLine) {
 
   ui::AXNode* link_node = paragraph_node->children()[1];
   ASSERT_TRUE(link_node);
-  EXPECT_EQ(link.url,
+  EXPECT_EQ(kChromiumTestUrl,
             link_node->GetStringAttribute(ax::mojom::StringAttribute::kUrl));
   EXPECT_EQ(ax::mojom::Role::kLink, link_node->data().role);
   ASSERT_EQ(1u, link_node->children().size());
@@ -459,17 +470,23 @@ TEST_F(PdfAccessibilityTreeTest, UnsortedLinkVector) {
   chars_.insert(chars_.end(), std::begin(kDummyCharsData),
                 std::end(kDummyCharsData));
 
-  ppapi::PdfAccessibilityLinkInfo link;
-  link.bounds = PP_MakeFloatRectFromXYWH(0.0f, 0.0f, 0.0f, 0.0f);
-  // Add first link in the vector.
-  link.text_run_index = 2;
-  link.text_run_count = 0;
-  links_.push_back(link);
+  {
+    // Add first link in the vector.
+    ppapi::PdfAccessibilityLinkInfo link;
+    link.bounds = PP_MakeFloatRectFromXYWH(0.0f, 0.0f, 0.0f, 0.0f);
+    link.text_run_index = 2;
+    link.text_run_count = 0;
+    links_.push_back(std::move(link));
+  }
 
-  // Add second link in the vector.
-  link.text_run_index = 0;
-  link.text_run_count = 1;
-  links_.push_back(std::move(link));
+  {
+    // Add second link in the vector.
+    ppapi::PdfAccessibilityLinkInfo link;
+    link.bounds = PP_MakeFloatRectFromXYWH(0.0f, 0.0f, 0.0f, 0.0f);
+    link.text_run_index = 0;
+    link.text_run_count = 1;
+    links_.push_back(std::move(link));
+  }
 
   page_info_.text_run_count = text_runs_.size();
   page_info_.char_count = chars_.size();
@@ -499,11 +516,13 @@ TEST_F(PdfAccessibilityTreeTest, OutOfBoundLink) {
   chars_.insert(chars_.end(), std::begin(kDummyCharsData),
                 std::end(kDummyCharsData));
 
-  ppapi::PdfAccessibilityLinkInfo link;
-  link.bounds = PP_MakeFloatRectFromXYWH(0.0f, 0.0f, 0.0f, 0.0f);
-  link.text_run_index = 3;
-  link.text_run_count = 0;
-  links_.push_back(std::move(link));
+  {
+    ppapi::PdfAccessibilityLinkInfo link;
+    link.bounds = PP_MakeFloatRectFromXYWH(0.0f, 0.0f, 0.0f, 0.0f);
+    link.text_run_index = 3;
+    link.text_run_count = 0;
+    links_.push_back(std::move(link));
+  }
 
   page_info_.text_run_count = text_runs_.size();
   page_info_.char_count = chars_.size();
@@ -533,15 +552,21 @@ TEST_F(PdfAccessibilityTreeTest, UnsortedImageVector) {
   chars_.insert(chars_.end(), std::begin(kDummyCharsData),
                 std::end(kDummyCharsData));
 
-  ppapi::PdfAccessibilityImageInfo image;
-  image.bounds = PP_MakeFloatRectFromXYWH(0.0f, 0.0f, 0.0f, 0.0f);
-  // Add first image to the vector.
-  image.text_run_index = 1;
-  images_.push_back(image);
+  {
+    // Add first image to the vector.
+    ppapi::PdfAccessibilityImageInfo image;
+    image.bounds = PP_MakeFloatRectFromXYWH(0.0f, 0.0f, 0.0f, 0.0f);
+    image.text_run_index = 1;
+    images_.push_back(std::move(image));
+  }
 
-  // Add second image to the vector.
-  image.text_run_index = 0;
-  images_.push_back(image);
+  {
+    // Add second image to the vector.
+    ppapi::PdfAccessibilityImageInfo image;
+    image.bounds = PP_MakeFloatRectFromXYWH(0.0f, 0.0f, 0.0f, 0.0f);
+    image.text_run_index = 0;
+    images_.push_back(std::move(image));
+  }
 
   page_info_.text_run_count = text_runs_.size();
   page_info_.char_count = chars_.size();
@@ -571,10 +596,12 @@ TEST_F(PdfAccessibilityTreeTest, OutOfBoundImage) {
   chars_.insert(chars_.end(), std::begin(kDummyCharsData),
                 std::end(kDummyCharsData));
 
-  ppapi::PdfAccessibilityImageInfo image;
-  image.bounds = PP_MakeFloatRectFromXYWH(0.0f, 0.0f, 0.0f, 0.0f);
-  image.text_run_index = 3;
-  images_.push_back(image);
+  {
+    ppapi::PdfAccessibilityImageInfo image;
+    image.bounds = PP_MakeFloatRectFromXYWH(0.0f, 0.0f, 0.0f, 0.0f);
+    image.text_run_index = 3;
+    images_.push_back(std::move(image));
+  }
 
   page_info_.text_run_count = text_runs_.size();
   page_info_.char_count = chars_.size();
@@ -668,13 +695,15 @@ TEST_F(PdfAccessibilityTreeTest, TestClickActionDataConversion) {
   chars_.insert(chars_.end(), std::begin(kDummyCharsData),
                 std::end(kDummyCharsData));
 
-  ppapi::PdfAccessibilityLinkInfo link;
-  link.url = "www.cs.chromium.org";
-  link.text_run_index = 0;
-  link.text_run_count = 1;
-  link.bounds = {{0, 0}, {10, 10}};
-  link.index_in_page = 0;
-  links_.push_back(std::move(link));
+  {
+    ppapi::PdfAccessibilityLinkInfo link;
+    link.url = kChromiumTestUrl;
+    link.text_run_index = 0;
+    link.text_run_count = 1;
+    link.bounds = {{0, 0}, {10, 10}};
+    link.index_in_page = 0;
+    links_.push_back(std::move(link));
+  }
 
   page_info_.text_run_count = text_runs_.size();
   page_info_.char_count = chars_.size();

components/pdf/renderer/pdf_accessibility_tree_unittest.cc

@@ -4,6 +4,8 @@
 
 #include "components/pdf/renderer/pdf_accessibility_tree.h"
 
+#include <utility>
+
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace pdf {
@@ -65,16 +67,21 @@ TEST(PdfAccessibilityTreeUnitTest, UnsortedLinkVector) {
   std::vector<ppapi::PdfAccessibilityLinkInfo> links;
   std::vector<ppapi::PdfAccessibilityImageInfo> images;
 
-  ppapi::PdfAccessibilityLinkInfo link;
-  // Add first link in the vector.
-  link.text_run_index = 2;
-  link.text_run_count = 0;
-  links.push_back(link);
-
-  // Add second link in the vector.
-  link.text_run_index = 0;
-  link.text_run_count = 1;
-  links.push_back(link);
+  {
+    // Add first link in the vector.
+    ppapi::PdfAccessibilityLinkInfo link;
+    link.text_run_index = 2;
+    link.text_run_count = 0;
+    links.push_back(std::move(link));
+  }
+
+  {
+    // Add second link in the vector.
+    ppapi::PdfAccessibilityLinkInfo link;
+    link.text_run_index = 0;
+    link.text_run_count = 1;
+    links.push_back(std::move(link));
+  }
 
   ASSERT_FALSE(PdfAccessibilityTree::IsDataFromPluginValid(text_runs, chars,
                                                            links, images));
@@ -91,10 +98,12 @@ TEST(PdfAccessibilityTreeUnitTest, OutOfBoundLink) {
   std::vector<ppapi::PdfAccessibilityLinkInfo> links;
   std::vector<ppapi::PdfAccessibilityImageInfo> images;
 
-  ppapi::PdfAccessibilityLinkInfo link;
-  link.text_run_index = 3;
-  link.text_run_count = 0;
-  links.push_back(link);
+  {
+    ppapi::PdfAccessibilityLinkInfo link;
+    link.text_run_index = 3;
+    link.text_run_count = 0;
+    links.push_back(std::move(link));
+  }
 
   ASSERT_FALSE(PdfAccessibilityTree::IsDataFromPluginValid(text_runs, chars,
                                                            links, images));
@@ -111,14 +120,19 @@ TEST(PdfAccessibilityTreeUnitTest, UnsortedImageVector) {
   std::vector<ppapi::PdfAccessibilityLinkInfo> links;
   std::vector<ppapi::PdfAccessibilityImageInfo> images;
 
-  ppapi::PdfAccessibilityImageInfo image;
-  // Add first image to the vector.
-  image.text_run_index = 1;
-  images.push_back(image);
+  {
+    // Add first image to the vector.
+    ppapi::PdfAccessibilityImageInfo image;
+    image.text_run_index = 1;
+    images.push_back(std::move(image));
+  }
 
-  // Add second image to the vector.
-  image.text_run_index = 0;
-  images.push_back(image);
+  {
+    // Add second image to the vector.
+    ppapi::PdfAccessibilityImageInfo image;
+    image.text_run_index = 0;
+    images.push_back(std::move(image));
+  }
 
   ASSERT_FALSE(PdfAccessibilityTree::IsDataFromPluginValid(text_runs, chars,
                                                            links, images));
@@ -135,12 +149,14 @@ TEST(PdfAccessibilityTreeUnitTest, OutOfBoundImage) {
   std::vector<ppapi::PdfAccessibilityLinkInfo> links;
   std::vector<ppapi::PdfAccessibilityImageInfo> images;
 
-  ppapi::PdfAccessibilityImageInfo image;
-  image.text_run_index = 3;
-  images.push_back(image);
+  {
+    ppapi::PdfAccessibilityImageInfo image;
+    image.text_run_index = 3;
+    images.push_back(std::move(image));
+  }
 
   ASSERT_FALSE(PdfAccessibilityTree::IsDataFromPluginValid(text_runs, chars,
                                                            links, images));
 }
 
-}  // namespace pdf
+}  // namespace pdf
\ No newline at end of file

ppapi/shared_impl/pdf_accessibility_shared.cc

@@ -9,31 +9,29 @@ namespace ppapi {
 PdfAccessibilityLinkInfo::PdfAccessibilityLinkInfo() = default;
 
 PdfAccessibilityLinkInfo::PdfAccessibilityLinkInfo(
-    const PdfAccessibilityLinkInfo& other) = default;
-
-PdfAccessibilityLinkInfo::~PdfAccessibilityLinkInfo() = default;
+    const PP_PrivateAccessibilityLinkInfo& link)
+    : url(std::string(link.url, link.url_length)),
+      index_in_page(link.index_in_page),
+      text_run_index(link.text_run_index),
+      text_run_count(link.text_run_count),
+      bounds(link.bounds) {}
 
 PdfAccessibilityLinkInfo::PdfAccessibilityLinkInfo(
-    const PP_PrivateAccessibilityLinkInfo& link) {
-  url = std::string(link.url, link.url_length);
-  index_in_page = link.index_in_page;
-  text_run_index = link.text_run_index;
-  text_run_count = link.text_run_count;
-  bounds = link.bounds;
-}
+    PdfAccessibilityLinkInfo&& other) = default;
+
+PdfAccessibilityLinkInfo::~PdfAccessibilityLinkInfo() = default;
 
 PdfAccessibilityImageInfo::PdfAccessibilityImageInfo() = default;
 
 PdfAccessibilityImageInfo::PdfAccessibilityImageInfo(
-    const PdfAccessibilityImageInfo& other) = default;
-
-PdfAccessibilityImageInfo::~PdfAccessibilityImageInfo() = default;
+    const PP_PrivateAccessibilityImageInfo& image)
+    : alt_text(std::string(image.alt_text, image.alt_text_length)),
+      text_run_index(image.text_run_index),
+      bounds(image.bounds) {}
 
 PdfAccessibilityImageInfo::PdfAccessibilityImageInfo(
-    const PP_PrivateAccessibilityImageInfo& image) {
-  alt_text = std::string(image.alt_text, image.alt_text_length);
-  text_run_index = image.text_run_index;
-  bounds = image.bounds;
-}
+    PdfAccessibilityImageInfo&& other) = default;
+
+PdfAccessibilityImageInfo::~PdfAccessibilityImageInfo() = default;
 
 }  // namespace ppapi

ppapi/shared_impl/pdf_accessibility_shared.h

@@ -16,8 +16,9 @@ namespace ppapi {
 // Needs to stay in sync with PP_PrivateAccessibilityLinkInfo.
 struct PPAPI_SHARED_EXPORT PdfAccessibilityLinkInfo {
   PdfAccessibilityLinkInfo();
-  PdfAccessibilityLinkInfo(const PdfAccessibilityLinkInfo& other);
-  PdfAccessibilityLinkInfo(const PP_PrivateAccessibilityLinkInfo& link);
+  explicit PdfAccessibilityLinkInfo(
+      const PP_PrivateAccessibilityLinkInfo& link);
+  PdfAccessibilityLinkInfo(PdfAccessibilityLinkInfo&& other);
   ~PdfAccessibilityLinkInfo();
 
   std::string url;
@@ -30,8 +31,9 @@ struct PPAPI_SHARED_EXPORT PdfAccessibilityLinkInfo {
 // Needs to stay in sync with PP_PrivateAccessibilityImageInfo.
 struct PPAPI_SHARED_EXPORT PdfAccessibilityImageInfo {
   PdfAccessibilityImageInfo();
-  PdfAccessibilityImageInfo(const PdfAccessibilityImageInfo& other);
-  PdfAccessibilityImageInfo(const PP_PrivateAccessibilityImageInfo& image);
+  explicit PdfAccessibilityImageInfo(
+      const PP_PrivateAccessibilityImageInfo& image);
+  PdfAccessibilityImageInfo(PdfAccessibilityImageInfo&& other);
   ~PdfAccessibilityImageInfo();
 
   std::string alt_text;