Address comments on DumpAccessibilityTestHelper

Address comments left on http://crrev.com/c/1669947 post-land.

Bug: 974397
Change-Id: I2f50f9c676d89ed1c20b06913d6607cd0dc41122
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1750151Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Nektarios Paisios <nektar@chromium.org>
Commit-Queue: Kevin Babbitt <kbabbitt@microsoft.com>
Cr-Commit-Position: refs/heads/master@{#686602}

chrome/browser/pdf/pdf_extension_test.cc

@@ -2337,16 +2337,16 @@ class PDFExtensionAccessibilityTreeDumpTest
     // the expectation file contains a skip marker.
     // This is used to skip certain tests on certain platforms.
     content::DumpAccessibilityTestHelper test_helper(formatter.get());
-    base::Optional<base::FilePath> expected_file_path =
+    base::FilePath expected_file_path =
         test_helper.GetExpectationFilePath(test_file_path);
-    if (!expected_file_path) {
+    if (expected_file_path.empty()) {
       LOG(INFO) << "No expectation file present, ignoring test on this "
                    "platform.";
       return;
     }
 
     base::Optional<std::vector<std::string>> expected_lines =
-        test_helper.LoadExpectationFile(*expected_file_path);
+        test_helper.LoadExpectationFile(expected_file_path);
     if (!expected_lines) {
       LOG(INFO) << "Skipping this test on this platform.";
       return;
@@ -2378,7 +2378,7 @@ class PDFExtensionAccessibilityTreeDumpTest
 
     // Validate the dump against the expectation file.
     EXPECT_TRUE(test_helper.ValidateAgainstExpectation(
-        test_file_path, *expected_file_path, actual_lines, *expected_lines));
+        test_file_path, expected_file_path, actual_lines, *expected_lines));
   }
 
   void AddDefaultFilters(std::vector<PropertyFilter>* property_filters) {

content/browser/accessibility/dump_accessibility_browsertest_base.cc

@@ -225,16 +225,15 @@ void DumpAccessibilityTestBase::RunTestForPlatform(
   // We have to check for this in advance in order to avoid waiting on a
   // WAIT-FOR directive in the source file that's looking for something not
   // supported on the current platform.
-  base::Optional<base::FilePath> expected_file =
-      test_helper.GetExpectationFilePath(file_path);
-  if (!expected_file) {
+  base::FilePath expected_file = test_helper.GetExpectationFilePath(file_path);
+  if (expected_file.empty()) {
     LOG(INFO) << "No expectation file present, ignoring test on this "
                  "platform.";
     return;
   }
 
   base::Optional<std::vector<std::string>> expected_lines =
-      test_helper.LoadExpectationFile(*expected_file);
+      test_helper.LoadExpectationFile(expected_file);
   if (!expected_lines) {
     LOG(INFO) << "Skipping this test on this platform.";
     return;
@@ -377,7 +376,7 @@ void DumpAccessibilityTestBase::RunTestForPlatform(
 
   // Validate against the expectation file.
   bool matches_expectation = test_helper.ValidateAgainstExpectation(
-      file_path, *expected_file, actual_lines, *expected_lines);
+      file_path, expected_file, actual_lines, *expected_lines);
   EXPECT_TRUE(matches_expectation);
   if (!matches_expectation)
     OnDiffFailed();

content/public/test/dump_accessibility_test_helper.cc

@@ -26,8 +26,7 @@ DumpAccessibilityTestHelper::DumpAccessibilityTestHelper(
     AccessibilityTreeFormatter* formatter)
     : formatter_(formatter) {}
 
-base::Optional<base::FilePath>
-DumpAccessibilityTestHelper::GetExpectationFilePath(
+base::FilePath DumpAccessibilityTestHelper::GetExpectationFilePath(
     const base::FilePath& test_file_path) {
   base::ScopedAllowBlockingForTesting allow_blocking;
   base::FilePath expected_file_path;
@@ -56,7 +55,7 @@ DumpAccessibilityTestHelper::GetExpectationFilePath(
             << " (it can be empty) and then run this test "
             << "with the switch: --"
             << switches::kGenerateAccessibilityTestExpectations;
-  return base::nullopt;
+  return base::FilePath();
 }
 
 base::Optional<std::vector<std::string>>

content/public/test/dump_accessibility_test_helper.h

@@ -18,17 +18,17 @@ class AccessibilityTreeFormatter;
 // A helper class for writing accessibility tree dump tests.
 class DumpAccessibilityTestHelper {
  public:
-  DumpAccessibilityTestHelper(AccessibilityTreeFormatter* formatter);
+  explicit DumpAccessibilityTestHelper(AccessibilityTreeFormatter* formatter);
   ~DumpAccessibilityTestHelper() = default;
 
   // Returns a path to an expectation file for the current platform. If no
   // suitable expectation file can be found, logs an error message and returns
-  // nullopt.
-  base::Optional<base::FilePath> GetExpectationFilePath(
-      const base::FilePath& test_file_path);
+  // an empty path.
+  base::FilePath GetExpectationFilePath(const base::FilePath& test_file_path);
 
-  // Loads the given expectation file. Returns nullopt if the file contains a
-  // skip marker.
+  // Loads the given expectation file and returns the contents. An expectation
+  // file may be empty, in which case an empty vector is returned.
+  // Returns nullopt if the file contains a skip marker.
   static base::Optional<std::vector<std::string>> LoadExpectationFile(
       const base::FilePath& expected_file);