[Autofill Assistant] Moved value utility methods into dedicated file.

Also extended test coverage for the affected methods. Added << overload
for ModelValue (intended only for debugging).

This is a refactoring only.

Bug: b/145043394
Change-Id: I5c6a055e6b1050ac79345e51d5472b78a67ca745
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2041614
Commit-Queue: Clemens Arbesser <arbesser@google.com>
Reviewed-by: Sandro Maggi <sandromaggi@google.com>
Cr-Commit-Position: refs/heads/master@{#738961}

components/autofill_assistant/browser/BUILD.gn

@@ -147,6 +147,8 @@ jumbo_static_library("browser") {
     "user_data_util.h",
     "user_model.cc",
     "user_model.h",
+    "value_util.cc",
+    "value_util.h",
     "viewport_mode.h",
     "web/element_finder.cc",
     "web/element_finder.h",
@@ -244,6 +246,7 @@ source_set("unit_tests") {
     "trigger_context_unittest.cc",
     "user_data_util_unittest.cc",
     "user_model_unittest.cc",
+    "value_util_unittest.cc",
   ]
 
   deps = [

components/autofill_assistant/browser/user_model.cc

@@ -8,107 +8,6 @@
 
 namespace autofill_assistant {
 
-// Compares two 'repeated' fields and returns true if every element matches.
-template <typename T>
-bool RepeatedFieldEquals(const T& values_a, const T& values_b) {
-  if (values_a.size() != values_b.size()) {
-    return false;
-  }
-  for (int i = 0; i < values_a.size(); i++) {
-    if (values_a[i] != values_b[i]) {
-      return false;
-    }
-  }
-  return true;
-}
-
-// '==' operator specialization for RepeatedPtrField.
-template <typename T>
-bool operator==(const google::protobuf::RepeatedPtrField<T>& values_a,
-                const google::protobuf::RepeatedPtrField<T>& values_b) {
-  return RepeatedFieldEquals(values_a, values_b);
-}
-
-// '==' operator specialization for RepeatedField.
-template <typename T>
-bool operator==(const google::protobuf::RepeatedField<T>& values_a,
-                const google::protobuf::RepeatedField<T>& values_b) {
-  return RepeatedFieldEquals(values_a, values_b);
-}
-
-// Compares two |ValueProto| instances and returns true if they exactly match.
-bool operator==(const ValueProto& value_a, const ValueProto& value_b) {
-  if (value_a.kind_case() != value_b.kind_case()) {
-    return false;
-  }
-  switch (value_a.kind_case()) {
-    case ValueProto::kStrings:
-      return value_a.strings().values() == value_b.strings().values();
-      break;
-    case ValueProto::kBooleans:
-      return value_a.booleans().values() == value_b.booleans().values();
-      break;
-    case ValueProto::kInts:
-      return value_a.ints().values() == value_b.ints().values();
-      break;
-    case ValueProto::KIND_NOT_SET:
-      return true;
-  }
-  return true;
-}
-
-// Comapres two |ModelValue| instances and returns true if they exactly match.
-bool operator==(const ModelProto::ModelValue& value_a,
-                const ModelProto::ModelValue& value_b) {
-  return value_a.identifier() == value_b.identifier() &&
-         value_a.value() == value_b.value();
-}
-
-// Intended for debugging. Writes a string representation of |values| to |out|.
-template <typename T>
-std::ostream& WriteRepeatedField(std::ostream& out, const T& values) {
-  std::string separator = "";
-  out << "[";
-  for (const auto& value : values) {
-    out << separator << value;
-    separator = ", ";
-  }
-  out << "]";
-  return out;
-}
-
-// Intended for debugging. '<<' operator specialization for RepeatedPtrField.
-template <typename T>
-std::ostream& operator<<(std::ostream& out,
-                         const google::protobuf::RepeatedPtrField<T>& values) {
-  return WriteRepeatedField(out, values);
-}
-
-// Intended for debugging. '<<' operator specialization for RepeatedField.
-template <typename T>
-std::ostream& operator<<(std::ostream& out,
-                         const google::protobuf::RepeatedField<T>& values) {
-  return WriteRepeatedField(out, values);
-}
-
-// Intended for debugging.  Writes a string representation of |value| to |out|.
-std::ostream& operator<<(std::ostream& out, const ValueProto& value) {
-  switch (value.kind_case()) {
-    case ValueProto::kStrings:
-      out << value.strings().values();
-      break;
-    case ValueProto::kBooleans:
-      out << value.booleans().values();
-      break;
-    case ValueProto::kInts:
-      out << value.ints().values();
-      break;
-    case ValueProto::KIND_NOT_SET:
-      break;
-  }
-  return out;
-}
-
 UserModel::Observer::Observer() = default;
 UserModel::Observer::~Observer() = default;
 

components/autofill_assistant/browser/user_model.h

@@ -12,7 +12,8 @@
 #include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
 #include "base/optional.h"
-#include "components/autofill_assistant/browser/service.pb.h"
+#include "components/autofill_assistant/browser/model.pb.h"
+#include "components/autofill_assistant/browser/value_util.h"
 
 namespace autofill_assistant {
 
@@ -69,17 +70,6 @@ class UserModel {
   DISALLOW_COPY_AND_ASSIGN(UserModel);
 };
 
-// Custom comparison operator for |ValueProto|, because we can't use
-// |MessageDifferencer| for protobuf lite and can't rely on serialization.
-bool operator==(const ValueProto& value_a, const ValueProto& value_b);
-
-// Custom comparison operator for |ModelValue|.
-bool operator==(const ModelProto::ModelValue& value_a,
-                const ModelProto::ModelValue& value_b);
-
-// Intended for debugging.
-std::ostream& operator<<(std::ostream& out, const ValueProto& value);
-
 }  //  namespace autofill_assistant
 
 #endif  //  COMPONENTS_AUTOFILL_ASSISTANT_BROWSER_USER_MODEL_H_

components/autofill_assistant/browser/user_model_unittest.cc

@@ -93,82 +93,41 @@ TEST_F(UserModelTest, InsertNewValues) {
                                                 Pair("value_c", value_c)));
 }
 
-TEST_F(UserModelTest, OverwriteExistingValue) {
+TEST_F(UserModelTest, OverwriteWithExistingValueFiresNoChangeEvent) {
   ValueProto value = CreateStringValue();
   EXPECT_CALL(mock_observer_, OnValueChanged("identifier", value)).Times(1);
   model_.SetValue("identifier", value);
 
-  value.mutable_strings()->add_values("new string");
-  EXPECT_CALL(mock_observer_, OnValueChanged("identifier", value)).Times(1);
-  model_.SetValue("identifier", value);
+  ValueProto same_value = CreateStringValue();
+  model_.SetValue("identifier", same_value);
 
   EXPECT_THAT(GetValues(), UnorderedElementsAre(Pair("identifier", value)));
 }
 
-TEST_F(UserModelTest, DifferentTypesComparison) {
-  ValueProto value_a = CreateStringValue();
-  ValueProto value_b = CreateIntValue();
+TEST_F(UserModelTest, OverwriteWithDifferentValueFiresChangeEvent) {
+  ValueProto value = CreateStringValue();
+  EXPECT_CALL(mock_observer_, OnValueChanged("identifier", _)).Times(2);
+  model_.SetValue("identifier", value);
 
-  EXPECT_CALL(mock_observer_, OnValueChanged("identifier", value_a)).Times(1);
-  EXPECT_CALL(mock_observer_, OnValueChanged("identifier", value_b)).Times(1);
-  model_.SetValue("identifier", value_a);
-  model_.SetValue("identifier", value_b);
+  ValueProto another_value = CreateStringValue();
+  another_value.mutable_strings()->add_values("tomato");
+  model_.SetValue("identifier", another_value);
 
-  EXPECT_THAT(GetValues(), UnorderedElementsAre(Pair("identifier", value_b)));
+  EXPECT_THAT(GetValues(),
+              UnorderedElementsAre(Pair("identifier", another_value)));
 }
 
-TEST_F(UserModelTest, StringComparison) {
+TEST_F(UserModelTest, ForceNotificationAlwaysFiresChangeEvent) {
+  testing::InSequence seq;
   ValueProto value_a = CreateStringValue();
-  ValueProto value_b = value_a;
-
-  EXPECT_CALL(mock_observer_, OnValueChanged("identifier", value_a)).Times(1);
-  model_.SetValue("identifier", value_a);
-  model_.SetValue("identifier", value_b);
-
-  value_a.mutable_strings()->add_values("potato");
-  value_b.mutable_strings()->add_values("tomato");
-  EXPECT_CALL(mock_observer_, OnValueChanged("identifier", value_a)).Times(1);
-  EXPECT_CALL(mock_observer_, OnValueChanged("identifier", value_b)).Times(1);
-  model_.SetValue("identifier", value_a);
-  model_.SetValue("identifier", value_b);
-
-  EXPECT_THAT(GetValues(), UnorderedElementsAre(Pair("identifier", value_b)));
-}
-
-TEST_F(UserModelTest, IntComparison) {
-  ValueProto value_a = CreateIntValue();
-  ValueProto value_b = value_a;
-
-  EXPECT_CALL(mock_observer_, OnValueChanged("identifier", value_a)).Times(1);
-  model_.SetValue("identifier", value_a);
-  model_.SetValue("identifier", value_b);
-
-  value_a.mutable_ints()->add_values(1);
-  value_b.mutable_ints()->add_values(0);
-  EXPECT_CALL(mock_observer_, OnValueChanged("identifier", value_a)).Times(1);
-  EXPECT_CALL(mock_observer_, OnValueChanged("identifier", value_b)).Times(1);
-  model_.SetValue("identifier", value_a);
-  model_.SetValue("identifier", value_b);
-
-  EXPECT_THAT(GetValues(), UnorderedElementsAre(Pair("identifier", value_b)));
-}
-
-TEST_F(UserModelTest, BoolComparison) {
-  ValueProto value_a = CreateBoolValue();
-  ValueProto value_b = value_a;
-
-  EXPECT_CALL(mock_observer_, OnValueChanged("identifier", value_a)).Times(1);
-  model_.SetValue("identifier", value_a);
-  model_.SetValue("identifier", value_b);
+  EXPECT_CALL(mock_observer_, OnValueChanged("a", value_a)).Times(1);
+  model_.SetValue("a", value_a);
 
-  value_a.mutable_booleans()->add_values(true);
-  value_b.mutable_booleans()->add_values(false);
-  EXPECT_CALL(mock_observer_, OnValueChanged("identifier", value_a)).Times(1);
-  EXPECT_CALL(mock_observer_, OnValueChanged("identifier", value_b)).Times(1);
-  model_.SetValue("identifier", value_a);
-  model_.SetValue("identifier", value_b);
+  EXPECT_CALL(mock_observer_, OnValueChanged("a", value_a)).Times(0);
+  model_.SetValue("a", value_a);
 
-  EXPECT_THAT(GetValues(), UnorderedElementsAre(Pair("identifier", value_b)));
+  EXPECT_CALL(mock_observer_, OnValueChanged("a", value_a)).Times(1);
+  model_.SetValue("a", value_a, /* force_notification = */ true);
 }
 
 TEST_F(UserModelTest, MergeWithProto) {
@@ -260,17 +219,4 @@ TEST_F(UserModelTest, UpdateProto) {
                 Property(&ModelProto::ModelValue::value, Eq(value_c)))));
 }
 
-TEST_F(UserModelTest, ForceNotification) {
-  testing::InSequence seq;
-  ValueProto value_a = CreateStringValue();
-  EXPECT_CALL(mock_observer_, OnValueChanged("a", value_a)).Times(1);
-  model_.SetValue("a", value_a);
-
-  EXPECT_CALL(mock_observer_, OnValueChanged("a", value_a)).Times(0);
-  model_.SetValue("a", value_a);
-
-  EXPECT_CALL(mock_observer_, OnValueChanged("a", value_a)).Times(1);
-  model_.SetValue("a", value_a, /* force_notification = */ true);
-}
-
 }  // namespace autofill_assistant

components/autofill_assistant/browser/value_util.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/autofill_assistant/browser/value_util.h"
+
+namespace autofill_assistant {
+
+// Compares two 'repeated' fields and returns true if every element matches.
+template <typename T>
+bool RepeatedFieldEquals(const T& values_a, const T& values_b) {
+  if (values_a.size() != values_b.size()) {
+    return false;
+  }
+  for (int i = 0; i < values_a.size(); i++) {
+    if (values_a[i] != values_b[i]) {
+      return false;
+    }
+  }
+  return true;
+}
+
+// '==' operator specialization for RepeatedPtrField.
+template <typename T>
+bool operator==(const google::protobuf::RepeatedPtrField<T>& values_a,
+                const google::protobuf::RepeatedPtrField<T>& values_b) {
+  return RepeatedFieldEquals(values_a, values_b);
+}
+
+// '==' operator specialization for RepeatedField.
+template <typename T>
+bool operator==(const google::protobuf::RepeatedField<T>& values_a,
+                const google::protobuf::RepeatedField<T>& values_b) {
+  return RepeatedFieldEquals(values_a, values_b);
+}
+
+// Compares two |ValueProto| instances and returns true if they exactly match.
+bool operator==(const ValueProto& value_a, const ValueProto& value_b) {
+  if (value_a.kind_case() != value_b.kind_case()) {
+    return false;
+  }
+  switch (value_a.kind_case()) {
+    case ValueProto::kStrings:
+      return value_a.strings().values() == value_b.strings().values();
+      break;
+    case ValueProto::kBooleans:
+      return value_a.booleans().values() == value_b.booleans().values();
+      break;
+    case ValueProto::kInts:
+      return value_a.ints().values() == value_b.ints().values();
+      break;
+    case ValueProto::KIND_NOT_SET:
+      return true;
+  }
+  return true;
+}
+
+// Comapres two |ModelValue| instances and returns true if they exactly match.
+bool operator==(const ModelProto::ModelValue& value_a,
+                const ModelProto::ModelValue& value_b) {
+  return value_a.identifier() == value_b.identifier() &&
+         value_a.value() == value_b.value();
+}
+
+// Intended for debugging. Writes a string representation of |values| to |out|.
+template <typename T>
+std::ostream& WriteRepeatedField(std::ostream& out, const T& values) {
+  std::string separator = "";
+  out << "[";
+  for (const auto& value : values) {
+    out << separator << value;
+    separator = ", ";
+  }
+  out << "]";
+  return out;
+}
+
+// Intended for debugging. '<<' operator specialization for RepeatedPtrField.
+template <typename T>
+std::ostream& operator<<(std::ostream& out,
+                         const google::protobuf::RepeatedPtrField<T>& values) {
+  return WriteRepeatedField(out, values);
+}
+
+// Intended for debugging. '<<' operator specialization for RepeatedField.
+template <typename T>
+std::ostream& operator<<(std::ostream& out,
+                         const google::protobuf::RepeatedField<T>& values) {
+  return WriteRepeatedField(out, values);
+}
+
+// Intended for debugging.  Writes a string representation of |value| to |out|.
+std::ostream& operator<<(std::ostream& out, const ValueProto& value) {
+  switch (value.kind_case()) {
+    case ValueProto::kStrings:
+      out << value.strings().values();
+      break;
+    case ValueProto::kBooleans:
+      out << value.booleans().values();
+      break;
+    case ValueProto::kInts:
+      out << value.ints().values();
+      break;
+    case ValueProto::KIND_NOT_SET:
+      break;
+  }
+  return out;
+}
+
+// Intended for debugging.  Writes a string representation of |value| to |out|.
+std::ostream& operator<<(std::ostream& out,
+                         const ModelProto::ModelValue& value) {
+  out << value.identifier() << ": " << value.value();
+  return out;
+}
+
+}  // namespace autofill_assistant

components/autofill_assistant/browser/value_util.h

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef COMPONENTS_AUTOFILL_ASSISTANT_BROWSER_VALUE_UTIL_H_
+#define COMPONENTS_AUTOFILL_ASSISTANT_BROWSER_VALUE_UTIL_H_
+
+#include <ostream>
+#include "components/autofill_assistant/browser/model.pb.h"
+
+namespace autofill_assistant {
+
+// Custom comparison operator for |ValueProto|, because we can't use
+// |MessageDifferencer| for protobuf lite and can't rely on serialization.
+bool operator==(const ValueProto& value_a, const ValueProto& value_b);
+
+// Custom comparison operator for |ModelValue|.
+bool operator==(const ModelProto::ModelValue& value_a,
+                const ModelProto::ModelValue& value_b);
+
+// Intended for debugging.
+std::ostream& operator<<(std::ostream& out, const ValueProto& value);
+std::ostream& operator<<(std::ostream& out,
+                         const ModelProto::ModelValue& value);
+
+}  //  namespace autofill_assistant
+
+#endif  //  COMPONENTS_AUTOFILL_ASSISTANT_BROWSER_VALUE_UTIL_H_

components/autofill_assistant/browser/value_util_unittest.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/autofill_assistant/browser/value_util.h"
+#include "testing/gmock/include/gmock/gmock.h"
+
+namespace autofill_assistant {
+
+namespace value_util {
+
+class ValueUtilTest : public testing::Test {
+ public:
+  ValueUtilTest() = default;
+  ~ValueUtilTest() override {}
+
+  ValueProto CreateStringValue() const {
+    ValueProto value;
+    value.mutable_strings()->add_values("Aurea prima");
+    value.mutable_strings()->add_values("sata est,");
+    value.mutable_strings()->add_values("aetas quae");
+    value.mutable_strings()->add_values("vindice nullo");
+    value.mutable_strings()->add_values("ü万𠜎");
+    return value;
+  }
+
+  ValueProto CreateIntValue() const {
+    ValueProto value;
+    value.mutable_ints()->add_values(1);
+    value.mutable_ints()->add_values(123);
+    value.mutable_ints()->add_values(5);
+    value.mutable_ints()->add_values(-132);
+    return value;
+  }
+
+  ValueProto CreateBoolValue() const {
+    ValueProto value;
+    value.mutable_booleans()->add_values(true);
+    value.mutable_booleans()->add_values(false);
+    value.mutable_booleans()->add_values(true);
+    value.mutable_booleans()->add_values(true);
+    return value;
+  }
+};
+
+TEST_F(ValueUtilTest, DifferentTypesComparison) {
+  ValueProto value_a;
+  ValueProto value_b = CreateStringValue();
+  ValueProto value_c = CreateIntValue();
+  ValueProto value_d = CreateBoolValue();
+
+  EXPECT_FALSE(value_a == value_b);
+  EXPECT_FALSE(value_a == value_c);
+  EXPECT_FALSE(value_a == value_d);
+  EXPECT_FALSE(value_b == value_c);
+  EXPECT_FALSE(value_b == value_d);
+  EXPECT_FALSE(value_c == value_d);
+
+  EXPECT_TRUE(value_a == value_a);
+  EXPECT_TRUE(value_b == value_b);
+  EXPECT_TRUE(value_c == value_c);
+  EXPECT_TRUE(value_d == value_d);
+}
+
+TEST_F(ValueUtilTest, EmptyValueComparison) {
+  ValueProto value_a;
+  ValueProto value_b;
+  EXPECT_TRUE(value_a == value_b);
+
+  value_a.mutable_strings()->add_values("potato");
+  EXPECT_FALSE(value_a == value_b);
+
+  value_a.mutable_strings()->clear_values();
+  EXPECT_FALSE(value_a == value_b);
+
+  value_a.clear_kind();
+  EXPECT_TRUE(value_a == value_b);
+}
+
+TEST_F(ValueUtilTest, StringComparison) {
+  ValueProto value_a = CreateStringValue();
+  ValueProto value_b = value_a;
+  EXPECT_TRUE(value_a == value_b);
+
+  value_a.mutable_strings()->add_values("potato");
+  EXPECT_FALSE(value_a == value_b);
+
+  value_b.mutable_strings()->add_values("tomato");
+  EXPECT_FALSE(value_a == value_b);
+
+  value_a.mutable_strings()->set_values(value_a.strings().values_size() - 1,
+                                        "tomato");
+  EXPECT_TRUE(value_a == value_b);
+}
+
+TEST_F(ValueUtilTest, IntComparison) {
+  ValueProto value_a = CreateIntValue();
+  ValueProto value_b = value_a;
+  EXPECT_TRUE(value_a == value_b);
+
+  value_a.mutable_ints()->add_values(1);
+  value_b.mutable_ints()->add_values(0);
+  EXPECT_FALSE(value_a == value_b);
+
+  value_a.mutable_ints()->set_values(value_a.ints().values_size() - 1, 0);
+  EXPECT_TRUE(value_a == value_b);
+}
+
+TEST_F(ValueUtilTest, BoolComparison) {
+  ValueProto value_a = CreateBoolValue();
+  ValueProto value_b = value_a;
+  EXPECT_TRUE(value_a == value_b);
+
+  value_a.mutable_booleans()->add_values(true);
+  value_b.mutable_booleans()->add_values(false);
+  EXPECT_FALSE(value_a == value_b);
+
+  value_a.mutable_booleans()->set_values(value_a.booleans().values_size() - 1,
+                                         false);
+  EXPECT_TRUE(value_a == value_b);
+}
+
+}  // namespace value_util
+}  // namespace autofill_assistant