Introduce CSVPassword::TryParse()

CSVPassword already has the method Parse(PasswordForm*), which parses
the associated CSV row. It returns the success/failure of the parsing
and stores the corresponding PasswordForm into the location passed as
the only argument. If the argument is null, Parse() only checks that
the CSV row is a correct serialization of a credential and then does
not create the PasswordForm. This is often used in the code.

This CL introduces the TryParse() method, which is equivalent to
calling (the old) Parse(nullptr). Its purpose is to improve readability
of the code: with Parse(nullptr) the reader might wonder what the type
and meaning of the "nullptr" is, whereas with TryParse() it is clear
that this is some kind of check that parsing works.

Parse() now DCHECKS that the passed PasswordForm* is not null.

Bug: 934326
Change-Id: I92578ac1f87798535a7e8ba11bd7b464dd724629
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1932776
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org>
Cr-Commit-Position: refs/heads/master@{#719222}

components/password_manager/core/browser/import/csv_password.cc

@@ -36,6 +36,23 @@ CSVPassword::CSVPassword(const ColumnMap& map, base::StringPiece csv_row)
 CSVPassword::~CSVPassword() = default;
 
 CSVPassword::Status CSVPassword::Parse(PasswordForm* form) const {
+  DCHECK(form) << "Null target PasswordForm. Use TryParse() if the resulting "
+                  "PasswordForm is not needed.";
+  return ParseImpl(form);
+}
+
+CSVPassword::Status CSVPassword::TryParse() const {
+  return ParseImpl(nullptr);
+}
+
+PasswordForm CSVPassword::ParseValid() const {
+  PasswordForm result;
+  Status status = ParseImpl(&result);
+  DCHECK_EQ(Status::kOK, status);
+  return result;
+}
+
+CSVPassword::Status CSVPassword::ParseImpl(PasswordForm* form) const {
   // |map_| must be an (1) injective and (2) surjective (3) partial map. (3) is
   // enforced by its type, (2) is checked later in the code and (1) follows from
   // (2) and the following size() check.
@@ -92,11 +109,4 @@ CSVPassword::Status CSVPassword::Parse(PasswordForm* form) const {
   return Status::kOK;
 }
 
-PasswordForm CSVPassword::ParseValid() const {
-  PasswordForm result;
-  Status status = Parse(&result);
-  DCHECK_EQ(Status::kOK, status);
-  return result;
-}
-
 }  // namespace password_manager

components/password_manager/core/browser/import/csv_password.h

@@ -34,16 +34,22 @@ class CSVPassword {
   CSVPassword& operator=(CSVPassword&&) = delete;
   ~CSVPassword();
 
-  // Returns whether the associated CSV row can be parsed successfully.
-  // If returning success and |form| is not null, it also stores the parsed
-  // result in |*form|. It does not return base::Optional<PasswordForm> for
-  // efficiency reasons in cases when the parsed form is not needed.
+  // Returns whether the associated CSV row can be parsed successfully. If
+  // returning success, it also stores the parsed result in |*form|.
   Status Parse(autofill::PasswordForm* form) const;
+  // TryParse() returns the same value as Parse(). However, TryParse() does not
+  // attempt to create and store the corresponding PasswordForm anywhere.
+  // Therefore TryParse() is faster than Parse() and a better choice for only
+  // checking a correctness of a CSV serialization of a credential.
+  Status TryParse() const;
   // Convenience wrapper around Parse() for cases known to be correctly
   // parseable.
   autofill::PasswordForm ParseValid() const;
 
  private:
+  // ParseImpl is the common base of Parse() and TryParse().
+  Status ParseImpl(autofill::PasswordForm* form) const;
+
   // The members |map_| and |row_| are only modified in constructor or
   // operator=().
 

components/password_manager/core/browser/import/csv_password_iterator.cc

@@ -83,8 +83,7 @@ void CSVPasswordIterator::SeekToNextValidRow() {
       // Skip over empty lines, and
       (csv_row_.empty() && !csv_rest_.empty()) ||
       // lines which are not correctly encoded passwords.
-      (!csv_row_.empty() &&
-       password_->Parse(nullptr) != CSVPassword::Status::kOK));
+      (!csv_row_.empty() && password_->TryParse() != CSVPassword::Status::kOK));
 }
 
 base::StringPiece ConsumeCSVLine(base::StringPiece* input) {

components/password_manager/core/browser/import/csv_password_iterator_unittest.cc

@@ -84,10 +84,10 @@ TEST(CSVPasswordIteratorTest, MostRowsCorrect) {
 
   CSVPasswordIterator check = iter;
   for (size_t i = 0; i < base::size(kExpectedUsernames); ++i) {
-    EXPECT_EQ(CSVPassword::Status::kOK, (check++)->Parse(nullptr))
+    EXPECT_EQ(CSVPassword::Status::kOK, (check++)->TryParse())
         << "on line " << i;
   }
-  EXPECT_NE(CSVPassword::Status::kOK, check->Parse(nullptr));
+  EXPECT_NE(CSVPassword::Status::kOK, check->TryParse());
 
   for (const base::StringPiece& expected_username : kExpectedUsernames) {
     PasswordForm result = (iter++)->ParseValid();
@@ -120,7 +120,7 @@ TEST(CSVPasswordIteratorTest, LastRowCorrect) {
   EXPECT_EQ("http://no-failure.example.com/", pf.signon_realm);
 
   // After iterating over all lines, there is no more data to parse.
-  EXPECT_NE(CSVPassword::Status::kOK, iter->Parse(nullptr));
+  EXPECT_NE(CSVPassword::Status::kOK, iter->TryParse());
 }
 
 TEST(CSVPasswordIteratorTest, NoRowCorrect) {

components/password_manager/core/browser/import/csv_password_unittest.cc

@@ -99,7 +99,7 @@ TEST_P(CSVPasswordTestSuccess, Parse) {
   const TestCase& test_case = GetParam();
   SCOPED_TRACE(test_case.name);
   const CSVPassword csv_pwd(test_case.map, test_case.csv);
-  EXPECT_EQ(Status::kOK, csv_pwd.Parse(nullptr));
+  EXPECT_EQ(Status::kOK, csv_pwd.TryParse());
 
   const PasswordForm result = csv_pwd.ParseValid();
 
@@ -250,7 +250,7 @@ TEST_P(CSVPasswordTestFailure, Parse) {
   const TestCase& test_case = GetParam();
   SCOPED_TRACE(test_case.name);
   EXPECT_EQ(test_case.status,
-            CSVPassword(test_case.map, test_case.csv).Parse(nullptr));
+            CSVPassword(test_case.map, test_case.csv).TryParse());
 }
 
 INSTANTIATE_TEST_SUITE_P(