Fixing minor code and code comment issues.

This CL makes following changes
- Uses default keyword for dtors in
ActionHandlingFakePepperPluginInstance, FakeRendererPpapiHost and
PdfAccessibilityTreeTest classes.
- Removes ConvertAXScrollToPdfScrollAlignment function from member
variable of PdfAxActionTarget to a function in anonymous namespace.
- Rephrased and moved comments in pdf_accessibility_tree_browsertest.cc
and accessibility_unittest.cc files.

Bug: 769940
Change-Id: If7515d09e271d867af72a2f74e10e43b10fff236
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1753370
Commit-Queue: Virender Singh <virens@microsoft.com>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Kevin Babbitt <kbabbitt@microsoft.com>
Cr-Commit-Position: refs/heads/master@{#687314}

components/pdf/renderer/pdf_accessibility_tree_browsertest.cc

@@ -27,12 +27,15 @@ void CompareRect(PP_Rect expected_rect, PP_Rect actual_rect) {
   EXPECT_EQ(expected_rect.size.width, actual_rect.size.width);
 }
 
+// This class overrides content::FakePepperPluginInstance to record received
+// action data when tests make an accessibility action call.
 class ActionHandlingFakePepperPluginInstance
     : public content::FakePepperPluginInstance {
  public:
   ActionHandlingFakePepperPluginInstance() = default;
-  ~ActionHandlingFakePepperPluginInstance() override {}
+  ~ActionHandlingFakePepperPluginInstance() override = default;
 
+  // content::FakePepperPluginInstance:
   void HandleAccessibilityAction(
       const PP_PdfAccessibilityActionData& action_data) override {
     received_action_data_ = action_data;
@@ -48,14 +51,14 @@ class ActionHandlingFakePepperPluginInstance
 
 class FakeRendererPpapiHost : public content::RendererPpapiHost {
  public:
-  FakeRendererPpapiHost(content::RenderFrame* render_frame)
+  explicit FakeRendererPpapiHost(content::RenderFrame* render_frame)
       : FakeRendererPpapiHost(render_frame, nullptr) {}
   FakeRendererPpapiHost(
       content::RenderFrame* render_frame,
       ActionHandlingFakePepperPluginInstance* fake_pepper_plugin_instance)
       : render_frame_(render_frame),
         fake_pepper_plugin_instance_(fake_pepper_plugin_instance) {}
-  ~FakeRendererPpapiHost() override {}
+  ~FakeRendererPpapiHost() override = default;
 
   ppapi::host::PpapiHost* GetPpapiHost() override { return nullptr; }
   bool IsValidInstance(PP_Instance instance) override { return true; }
@@ -116,7 +119,7 @@ class FakeRendererPpapiHost : public content::RendererPpapiHost {
 class PdfAccessibilityTreeTest : public content::RenderViewTest {
  public:
   PdfAccessibilityTreeTest() {}
-  ~PdfAccessibilityTreeTest() override {}
+  ~PdfAccessibilityTreeTest() override = default;
 
   void SetUp() override {
     content::RenderViewTest::SetUp();
@@ -171,7 +174,7 @@ TEST_F(PdfAccessibilityTreeTest, TestEmptyPDFPage) {
 TEST_F(PdfAccessibilityTreeTest, TestAccessibilityDisabledDuringPDFLoad) {
   content::RenderFrame* render_frame = view_->GetMainRenderFrame();
   render_frame->SetAccessibilityModeForTest(ui::AXMode::kWebContents);
-  ASSERT_TRUE(render_frame->GetRenderAccessibility() != nullptr);
+  ASSERT_TRUE(render_frame->GetRenderAccessibility());
 
   FakeRendererPpapiHost host(view_->GetMainRenderFrame());
   PP_Instance instance = 0;
@@ -188,37 +191,18 @@ TEST_F(PdfAccessibilityTreeTest, TestAccessibilityDisabledDuringPDFLoad) {
                                                   chars_);
 }
 
-class FakePdfAccessibilityTree : public pdf::PdfAccessibilityTree {
- public:
-  FakePdfAccessibilityTree(content::RendererPpapiHost* host,
-                           PP_Instance instance)
-      : PdfAccessibilityTree(host, instance) {}
-  ~FakePdfAccessibilityTree() override {}
-
-  void HandleAction(const PP_PdfAccessibilityActionData& action_data) {
-    received_action_data = action_data;
-  }
-
-  PP_PdfAccessibilityActionData GetReceivedActionData() {
-    return received_action_data;
-  }
-
- private:
-  PP_PdfAccessibilityActionData received_action_data;
-};
-
 TEST_F(PdfAccessibilityTreeTest, TestActionDataConversion) {
   // This test verifies the AXActionData conversion to
   // PP_AccessibilityActionData.
   content::RenderFrame* render_frame = view_->GetMainRenderFrame();
   render_frame->SetAccessibilityModeForTest(ui::AXMode::kWebContents);
-  ASSERT_TRUE(render_frame->GetRenderAccessibility() != nullptr);
+  ASSERT_TRUE(render_frame->GetRenderAccessibility());
 
   ActionHandlingFakePepperPluginInstance fake_pepper_instance;
   FakeRendererPpapiHost host(view_->GetMainRenderFrame(),
                              &fake_pepper_instance);
   PP_Instance instance = 0;
-  FakePdfAccessibilityTree pdf_accessibility_tree(&host, instance);
+  PdfAccessibilityTree pdf_accessibility_tree(&host, instance);
 
   pdf_accessibility_tree.SetAccessibilityViewportInfo(viewport_info_);
   pdf_accessibility_tree.SetAccessibilityDocInfo(doc_info_);
@@ -228,7 +212,7 @@ TEST_F(PdfAccessibilityTreeTest, TestActionDataConversion) {
   ui::AXNode* root_node = pdf_accessibility_tree.GetRoot();
   std::unique_ptr<ui::AXActionTarget> pdf_action_target =
       pdf_accessibility_tree.CreateActionTarget(*root_node);
-  ASSERT_TRUE(pdf_action_target != nullptr);
+  ASSERT_TRUE(pdf_action_target);
   EXPECT_EQ(ui::AXActionTarget::Type::kPdf, pdf_action_target->GetType());
   EXPECT_TRUE(pdf_action_target->ScrollToMakeVisibleWithSubFocus(
       gfx::Rect(0, 0, 50, 50), ax::mojom::ScrollAlignment::kScrollAlignmentLeft,
@@ -274,13 +258,13 @@ TEST_F(PdfAccessibilityTreeTest, TestActionDataConversion) {
 TEST_F(PdfAccessibilityTreeTest, TestEmptyPdfAxActions) {
   content::RenderFrame* render_frame = view_->GetMainRenderFrame();
   render_frame->SetAccessibilityModeForTest(ui::AXMode::kWebContents);
-  ASSERT_TRUE(render_frame->GetRenderAccessibility() != nullptr);
+  ASSERT_TRUE(render_frame->GetRenderAccessibility());
 
   ActionHandlingFakePepperPluginInstance fake_pepper_instance;
   FakeRendererPpapiHost host(view_->GetMainRenderFrame(),
                              &fake_pepper_instance);
   PP_Instance instance = 0;
-  FakePdfAccessibilityTree pdf_accessibility_tree(&host, instance);
+  PdfAccessibilityTree pdf_accessibility_tree(&host, instance);
 
   pdf_accessibility_tree.SetAccessibilityViewportInfo(viewport_info_);
   pdf_accessibility_tree.SetAccessibilityDocInfo(doc_info_);

components/pdf/renderer/pdf_ax_action_target.cc

@@ -4,8 +4,36 @@
 
 #include "components/pdf/renderer/pdf_ax_action_target.h"
 
+#include "components/pdf/renderer/pdf_accessibility_tree.h"
+
 namespace pdf {
 
+namespace {
+
+PP_PdfAccessibilityScrollAlignment ConvertAXScrollToPdfScrollAlignment(
+    ax::mojom::ScrollAlignment scroll_alignment) {
+  switch (scroll_alignment) {
+    case ax::mojom::ScrollAlignment::kScrollAlignmentCenter:
+      return PP_PdfAccessibilityScrollAlignment::PP_PDF_SCROLL_ALIGNMENT_CENTER;
+    case ax::mojom::ScrollAlignment::kScrollAlignmentTop:
+      return PP_PdfAccessibilityScrollAlignment::PP_PDF_SCROLL_ALIGNMENT_TOP;
+    case ax::mojom::ScrollAlignment::kScrollAlignmentBottom:
+      return PP_PdfAccessibilityScrollAlignment::PP_PDF_SCROLL_ALIGNMENT_BOTTOM;
+    case ax::mojom::ScrollAlignment::kScrollAlignmentLeft:
+      return PP_PdfAccessibilityScrollAlignment::PP_PDF_SCROLL_ALIGNMENT_LEFT;
+    case ax::mojom::ScrollAlignment::kScrollAlignmentRight:
+      return PP_PdfAccessibilityScrollAlignment::PP_PDF_SCROLL_ALIGNMENT_RIGHT;
+    case ax::mojom::ScrollAlignment::kScrollAlignmentClosestEdge:
+      return PP_PdfAccessibilityScrollAlignment::
+          PP_PDF_SCROLL_ALIGNMENT_CLOSEST_EDGE;
+    case ax::mojom::ScrollAlignment::kNone:
+    default:
+      return PP_PdfAccessibilityScrollAlignment::PP_PDF_SCROLL_NONE;
+  }
+}
+
+}  // namespace
+
 PdfAXActionTarget::PdfAXActionTarget(const ui::AXNode& plugin_node,
                                      pdf::PdfAccessibilityTree* pdf_tree_source)
     : target_plugin_node_(plugin_node),
@@ -112,27 +140,4 @@ bool PdfAXActionTarget::ScrollToGlobalPoint(const gfx::Point& point) const {
   return false;
 }
 
-PP_PdfAccessibilityScrollAlignment
-PdfAXActionTarget::ConvertAXScrollToPdfScrollAlignment(
-    ax::mojom::ScrollAlignment scroll_alignment) const {
-  switch (scroll_alignment) {
-    case ax::mojom::ScrollAlignment::kScrollAlignmentCenter:
-      return PP_PdfAccessibilityScrollAlignment::PP_PDF_SCROLL_ALIGNMENT_CENTER;
-    case ax::mojom::ScrollAlignment::kScrollAlignmentTop:
-      return PP_PdfAccessibilityScrollAlignment::PP_PDF_SCROLL_ALIGNMENT_TOP;
-    case ax::mojom::ScrollAlignment::kScrollAlignmentBottom:
-      return PP_PdfAccessibilityScrollAlignment::PP_PDF_SCROLL_ALIGNMENT_BOTTOM;
-    case ax::mojom::ScrollAlignment::kScrollAlignmentLeft:
-      return PP_PdfAccessibilityScrollAlignment::PP_PDF_SCROLL_ALIGNMENT_LEFT;
-    case ax::mojom::ScrollAlignment::kScrollAlignmentRight:
-      return PP_PdfAccessibilityScrollAlignment::PP_PDF_SCROLL_ALIGNMENT_RIGHT;
-    case ax::mojom::ScrollAlignment::kScrollAlignmentClosestEdge:
-      return PP_PdfAccessibilityScrollAlignment::
-          PP_PDF_SCROLL_ALIGNMENT_CLOSEST_EDGE;
-    case ax::mojom::ScrollAlignment::kNone:
-    default:
-      return PP_PdfAccessibilityScrollAlignment::PP_PDF_SCROLL_NONE;
-  }
-}
-
 }  // namespace pdf

components/pdf/renderer/pdf_ax_action_target.h

@@ -5,14 +5,16 @@
 #ifndef COMPONENTS_PDF_RENDERER_PDF_AX_ACTION_TARGET_H_
 #define COMPONENTS_PDF_RENDERER_PDF_AX_ACTION_TARGET_H_
 
-#include "components/pdf/renderer/pdf_accessibility_tree.h"
-#include "content/public/renderer/plugin_ax_tree_source.h"
-#include "ppapi/c/private/ppp_pdf.h"
 #include "ui/accessibility/ax_action_target.h"
-#include "ui/accessibility/ax_node.h"
+
+namespace ui {
+class AXNode;
+}
 
 namespace pdf {
 
+class PdfAccessibilityTree;
+
 // Abstracts an AXNode, representing a PDF node, for dispatching
 // accessibility actions.
 class PdfAXActionTarget : public ui::AXActionTarget {
@@ -51,9 +53,6 @@ class PdfAXActionTarget : public ui::AXActionTarget {
   bool ScrollToGlobalPoint(const gfx::Point& point) const override;
 
  private:
-  PP_PdfAccessibilityScrollAlignment ConvertAXScrollToPdfScrollAlignment(
-      ax::mojom::ScrollAlignment scroll_alignment) const;
-
   const ui::AXNode& target_plugin_node_;
   pdf::PdfAccessibilityTree* pdf_accessibility_tree_source_;
 };

pdf/pdfium/accessibility_unittest.cc

@@ -158,20 +158,21 @@ TEST_F(AccessibilityTest, GetUnderlyingTextRangeForRect) {
   EXPECT_EQ(char_count, 5u);
 }
 
-// This class is required to just override the ScrollBy
-// functionality for testing in a specific way. It will
-// keep the TestClient class clean for extension by others.
+// This class overrides TestClient to record points received when a scroll
+// call is made by tests.
 class ScrollEnabledTestClient : public TestClient {
  public:
   ScrollEnabledTestClient() = default;
   ~ScrollEnabledTestClient() override = default;
 
-  void ScrollBy(const pp::Point& point) override { recieved_point_ = point; }
+  // Records the point received in a ScrollBy action request from tests.
+  void ScrollBy(const pp::Point& point) override { received_point_ = point; }
 
-  const pp::Point& GetScrollRequestPoints() { return recieved_point_; }
+  // Returns the point received in a ScrollBy action for validation in tests.
+  const pp::Point& GetScrollRequestPoints() const { return received_point_; }
 
  private:
-  pp::Point recieved_point_;
+  pp::Point received_point_;
 };
 
 TEST_F(AccessibilityTest, TestScrollIntoViewActionHandling) {
@@ -191,8 +192,8 @@ TEST_F(AccessibilityTest, TestScrollIntoViewActionHandling) {
   // Simulate a zoom update in the PDFiumEngine.
   engine->ZoomUpdated(1.5);
   engine->HandleAccessibilityAction(action_data);
-  PP_Point expected_point = {180, 0};
-  EXPECT_EQ(expected_point, client.GetScrollRequestPoints());
+  constexpr PP_Point kExpectedPoint = {180, 0};
+  EXPECT_EQ(kExpectedPoint, client.GetScrollRequestPoints());
 }
 
 }  // namespace chrome_pdf