[Autofill Assistant] Moved towards ValueReferences where it makes sense.

Also changed IntegerSum from two operands to N operands. Not sure why it
wasn't set up like that in the first place.

Bug: b/145043394
Change-Id: I81bf164b7fd10e76c0741889b479f04061fd5e87
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2130552
Commit-Queue: Clemens Arbesser <arbesser@google.com>
Reviewed-by: Marian Fechete <marianfe@google.com>
Cr-Commit-Position: refs/heads/master@{#755368}

chrome/browser/android/autofill_assistant/generic_ui_controller_android.cc

@@ -340,7 +340,7 @@ bool CreateImplicitInteractionsForView(
           ->mutable_on_value_changed()
           ->set_model_identifier(proto.text_input_view().model_identifier());
       SetTextProto set_text_callback;
-      set_text_callback.set_model_identifier(
+      set_text_callback.mutable_text()->set_model_identifier(
           proto.text_input_view().model_identifier());
       set_text_callback.set_view_identifier(proto.identifier());
       *implicit_set_text_interaction.add_callbacks()->mutable_set_text() =
@@ -364,7 +364,7 @@ bool CreateImplicitInteractionsForView(
           ->mutable_on_value_changed()
           ->set_model_identifier(proto.text_view().model_identifier());
       SetTextProto set_text_callback;
-      set_text_callback.set_model_identifier(
+      set_text_callback.mutable_text()->set_model_identifier(
           proto.text_view().model_identifier());
       set_text_callback.set_view_identifier(proto.identifier());
       *implicit_set_text_interaction.add_callbacks()->mutable_set_text() =

chrome/browser/android/autofill_assistant/generic_ui_interactions_android.cc

@@ -71,24 +71,23 @@ void ShowListPopup(base::WeakPtr<UserModel> user_model,
     return;
   }
 
-  auto item_names = user_model->GetValue(proto.item_names_model_identifier());
+  auto item_names = user_model->GetValue(proto.item_names());
   if (!item_names.has_value()) {
-    DVLOG(2) << "Failed to show list popup: '"
-             << proto.item_names_model_identifier() << "' not found in model.";
+    DVLOG(2) << "Failed to show list popup: '" << proto.item_names()
+             << "' not found in model.";
     return;
   }
   if (item_names->strings().values().size() == 0) {
     DVLOG(2) << "Failed to show list popup: the list of item names in '"
-             << proto.item_names_model_identifier() << "' was empty.";
+             << proto.item_names() << "' was empty.";
     return;
   }
 
   base::Optional<ValueProto> item_types;
-  if (proto.has_item_types_model_identifier()) {
-    item_types = user_model->GetValue(proto.item_types_model_identifier());
+  if (proto.has_item_types()) {
+    item_types = user_model->GetValue(proto.item_types());
     if (!item_types.has_value()) {
-      DVLOG(2) << "Failed to show list popup: '"
-               << proto.item_types_model_identifier()
+      DVLOG(2) << "Failed to show list popup: '" << proto.item_types()
                << "' not found in the model.";
       return;
     }
@@ -177,19 +176,19 @@ void ShowCalendarPopup(base::WeakPtr<UserModel> user_model,
     return;
   }
 
-  auto min_date = user_model->GetValue(proto.min_date_model_identifier());
+  auto min_date = user_model->GetValue(proto.min_date());
   if (!min_date.has_value() || min_date->dates().values().size() != 1) {
     DVLOG(2) << "Failed to show calendar popup: min_date not found or invalid "
                 "in user model at "
-             << proto.min_date_model_identifier();
+             << proto.min_date();
     return;
   }
 
-  auto max_date = user_model->GetValue(proto.max_date_model_identifier());
+  auto max_date = user_model->GetValue(proto.max_date());
   if (!max_date.has_value() || max_date->dates().values().size() != 1) {
     DVLOG(2) << "Failed to show calendar popup: max_date not found or invalid "
                 "in user model at "
-             << proto.max_date_model_identifier();
+             << proto.max_date();
     return;
   }
 
@@ -217,15 +216,15 @@ void SetViewText(
     return;
   }
 
-  auto text = user_model->GetValue(proto.model_identifier());
+  auto text = user_model->GetValue(proto.text());
   if (!text.has_value()) {
     DVLOG(2) << "Failed to set text for " << proto.view_identifier() << ": "
-             << proto.model_identifier() << " not found in model";
+             << proto.text() << " not found in model";
     return;
   }
   if (text->strings().values_size() != 1) {
     DVLOG(2) << "Failed to set text for " << proto.view_identifier()
-             << ": expected " << proto.model_identifier()
+             << ": expected " << proto.text()
              << " to contain single string, but was instead " << *text;
     return;
   }
@@ -259,12 +258,11 @@ void SetViewVisibility(
     return;
   }
 
-  auto visible_value = user_model->GetValue(proto.model_identifier());
+  auto visible_value = user_model->GetValue(proto.visible());
   if (!visible_value.has_value() ||
       visible_value->booleans().values_size() != 1) {
     DVLOG(2) << "Failed to set view visibility for " << proto.view_identifier()
-             << ": " << proto.model_identifier()
-             << " did not contain single boolean";
+             << ": " << proto.visible() << " did not contain single boolean";
     return;
   }
 

chrome/browser/android/autofill_assistant/interaction_handler_android.cc

@@ -76,8 +76,8 @@ CreateInteractionCallbackFromProto(
     base::android::ScopedJavaGlobalRef<jobject> jdelegate) {
   switch (proto.kind_case()) {
     case CallbackProto::kSetValue:
-      if (proto.set_value().model_identifier().empty()) {
-        VLOG(1) << "Error creating SetValue interaction: model_identifier "
+      if (!proto.set_value().has_value()) {
+        VLOG(1) << "Error creating SetValue interaction: value "
                    "not set";
         return base::nullopt;
       }
@@ -91,9 +91,9 @@ CreateInteractionCallbackFromProto(
                               proto.show_info_popup().info_popup(), jcontext));
     }
     case CallbackProto::kShowListPopup:
-      if (proto.show_list_popup().item_names_model_identifier().empty()) {
+      if (!proto.show_list_popup().has_item_names()) {
         VLOG(1) << "Error creating ShowListPopup interaction: "
-                   "items_list_model_identifier not set";
+                   "item_names not set";
         return base::nullopt;
       }
       if (proto.show_list_popup()
@@ -118,9 +118,9 @@ CreateInteractionCallbackFromProto(
                               basic_interactions->GetWeakPtr(),
                               proto.compute_value()));
     case CallbackProto::kSetUserActions:
-      if (proto.set_user_actions().model_identifier().empty()) {
+      if (!proto.set_user_actions().has_user_actions()) {
         VLOG(1) << "Error creating SetUserActions interaction: "
-                   "model_identifier not set";
+                   "user_actions not set";
         return base::nullopt;
       }
       return base::Optional<InteractionHandlerAndroid::InteractionCallback>(
@@ -145,9 +145,9 @@ CreateInteractionCallbackFromProto(
                               proto.show_calendar_popup(), jcontext,
                               jdelegate));
     case CallbackProto::kSetText:
-      if (proto.set_text().model_identifier().empty()) {
+      if (!proto.set_text().has_text()) {
         VLOG(1) << "Error creating SetText interaction: "
-                   "model_identifier not set";
+                   "text not set";
         return base::nullopt;
       }
       if (proto.set_text().view_identifier().empty()) {
@@ -170,9 +170,9 @@ CreateInteractionCallbackFromProto(
                    "user_action_identifier not set";
         return base::nullopt;
       }
-      if (proto.toggle_user_action().enabled_model_identifier().empty()) {
+      if (!proto.toggle_user_action().has_enabled()) {
         VLOG(1) << "Error creating ToggleUserAction interaction: "
-                   "enabled_model_identifier not set";
+                   "enabled not set";
         return base::nullopt;
       }
       return base::Optional<InteractionHandlerAndroid::InteractionCallback>(
@@ -185,9 +185,9 @@ CreateInteractionCallbackFromProto(
                    "view_identifier not set";
         return base::nullopt;
       }
-      if (proto.set_view_visibility().model_identifier().empty()) {
+      if (!proto.set_view_visibility().has_visible()) {
         VLOG(1) << "Error creating SetViewVisibility interaction: "
-                   "model_identifier not set";
+                   "visible not set";
         return base::nullopt;
       }
       return base::Optional<InteractionHandlerAndroid::InteractionCallback>(

components/autofill_assistant/browser/basic_interactions_unittest.cc

@@ -40,24 +40,24 @@ class BasicInteractionsTest : public testing::Test {
 
 TEST_F(BasicInteractionsTest, SetValue) {
   SetModelValueProto proto;
-  *proto.mutable_value() = SimpleValue(std::string("success"));
+  *proto.mutable_value()->mutable_value() = SimpleValue(std::string("success"));
 
   // Model identifier not set.
   EXPECT_FALSE(basic_interactions_.SetValue(proto));
 
   proto.set_model_identifier("output");
   EXPECT_TRUE(basic_interactions_.SetValue(proto));
-  EXPECT_EQ(user_model_.GetValue("output"), proto.value());
+  EXPECT_EQ(user_model_.GetValue("output"), proto.value().value());
 }
 
 TEST_F(BasicInteractionsTest, ComputeValueBooleanAnd) {
   ComputeValueProto proto;
-  proto.mutable_boolean_and()->add_model_identifiers("value_1");
-  proto.mutable_boolean_and()->add_model_identifiers("value_2");
-  proto.mutable_boolean_and()->add_model_identifiers("value_3");
+  proto.mutable_boolean_and()->add_values()->set_model_identifier("value_1");
+  *proto.mutable_boolean_and()->add_values()->mutable_value() =
+      SimpleValue(true);
+  proto.mutable_boolean_and()->add_values()->set_model_identifier("value_3");
 
   user_model_.SetValue("value_1", SimpleValue(true));
-  user_model_.SetValue("value_2", SimpleValue(true));
   user_model_.SetValue("value_3", SimpleValue(false));
 
   // Result model identifier not set.
@@ -79,19 +79,19 @@ TEST_F(BasicInteractionsTest, ComputeValueBooleanAnd) {
   ValueProto multi_bool;
   multi_bool.mutable_booleans()->add_values(true);
   multi_bool.mutable_booleans()->add_values(false);
-  user_model_.SetValue("value_2", multi_bool);
+  user_model_.SetValue("value_3", multi_bool);
   EXPECT_FALSE(basic_interactions_.ComputeValue(proto));
 }
 
 TEST_F(BasicInteractionsTest, ComputeValueBooleanOr) {
   ComputeValueProto proto;
-  proto.mutable_boolean_or()->add_model_identifiers("value_1");
-  proto.mutable_boolean_or()->add_model_identifiers("value_2");
-  proto.mutable_boolean_or()->add_model_identifiers("value_3");
+  proto.mutable_boolean_or()->add_values()->set_model_identifier("value_1");
+  proto.mutable_boolean_or()->add_values()->set_model_identifier("value_2");
+  *proto.mutable_boolean_or()->add_values()->mutable_value() =
+      SimpleValue(false);
 
   user_model_.SetValue("value_1", SimpleValue(false));
   user_model_.SetValue("value_2", SimpleValue(false));
-  user_model_.SetValue("value_3", SimpleValue(false));
 
   // Result model identifier not set.
   EXPECT_FALSE(basic_interactions_.ComputeValue(proto));
@@ -118,7 +118,7 @@ TEST_F(BasicInteractionsTest, ComputeValueBooleanOr) {
 
 TEST_F(BasicInteractionsTest, ComputeValueBooleanNot) {
   ComputeValueProto proto;
-  proto.mutable_boolean_not()->set_model_identifier("value");
+  proto.mutable_boolean_not()->mutable_value()->set_model_identifier("value");
   user_model_.SetValue("value", SimpleValue(false));
 
   // Result model identifier not set.
@@ -145,7 +145,7 @@ TEST_F(BasicInteractionsTest, ComputeValueBooleanNot) {
 
 TEST_F(BasicInteractionsTest, ComputeValueToString) {
   ComputeValueProto proto;
-  proto.mutable_to_string()->set_model_identifier("value");
+  proto.mutable_to_string()->mutable_value()->set_model_identifier("value");
   user_model_.SetValue("value", SimpleValue(1));
 
   // Result model identifier not set.
@@ -209,9 +209,9 @@ TEST_F(BasicInteractionsTest, ComputeValueIntegerSum) {
 
   // Missing fields.
   EXPECT_FALSE(basic_interactions_.ComputeValue(proto));
-  proto.mutable_integer_sum()->set_model_identifier_a("value_a");
+  proto.mutable_integer_sum()->add_values()->set_model_identifier("value_a");
   EXPECT_FALSE(basic_interactions_.ComputeValue(proto));
-  proto.mutable_integer_sum()->set_model_identifier_b("value_b");
+  proto.mutable_integer_sum()->add_values()->set_model_identifier("value_b");
   EXPECT_FALSE(basic_interactions_.ComputeValue(proto));
   proto.set_result_model_identifier("result");
   EXPECT_TRUE(basic_interactions_.ComputeValue(proto));
@@ -231,8 +231,9 @@ TEST_F(BasicInteractionsTest, ComputeValueIntegerSum) {
 
   user_model_.SetValue("value_a", SimpleValue(-1));
   user_model_.SetValue("value_b", SimpleValue(5));
+  *proto.mutable_integer_sum()->add_values()->mutable_value() = SimpleValue(3);
   EXPECT_TRUE(basic_interactions_.ComputeValue(proto));
-  EXPECT_EQ(user_model_.GetValue("result"), SimpleValue(4));
+  EXPECT_EQ(user_model_.GetValue("result"), SimpleValue(7));
 }
 
 TEST_F(BasicInteractionsTest, EndActionWithoutCallbackFails) {
@@ -262,9 +263,11 @@ TEST_F(BasicInteractionsTest, ComputeValueCompare) {
 
   // Fields are missing.
   EXPECT_FALSE(basic_interactions_.ComputeValue(proto));
-  proto.mutable_comparison()->set_model_identifier_a("value_a");
+  proto.mutable_comparison()->mutable_value_a()->set_model_identifier(
+      "value_a");
   EXPECT_FALSE(basic_interactions_.ComputeValue(proto));
-  proto.mutable_comparison()->set_model_identifier_b("value_b");
+  proto.mutable_comparison()->mutable_value_b()->set_model_identifier(
+      "value_b");
   EXPECT_FALSE(basic_interactions_.ComputeValue(proto));
   proto.set_result_model_identifier("result");
   EXPECT_FALSE(basic_interactions_.ComputeValue(proto));
@@ -371,7 +374,7 @@ TEST_F(BasicInteractionsTest, ToggleUserAction) {
   EXPECT_FALSE(basic_interactions_.ToggleUserAction(proto));
   proto.set_user_actions_model_identifier("chips");
   EXPECT_FALSE(basic_interactions_.ToggleUserAction(proto));
-  proto.set_enabled_model_identifier("enabled");
+  proto.mutable_enabled()->set_model_identifier("enabled");
   EXPECT_FALSE(basic_interactions_.ToggleUserAction(proto));
   proto.set_user_action_identifier("done_identifier");
   EXPECT_TRUE(basic_interactions_.ToggleUserAction(proto));

components/autofill_assistant/browser/interactions.proto

@@ -83,7 +83,8 @@ message SetModelValueProto {
   // The model identifier to write to.
   optional string model_identifier = 1;
   // The value to write.
-  optional ValueProto value = 2;
+  optional ValueReferenceProto value = 3;
+  reserved 2;
 }
 
 // Computes a value and stores the result to |result_model_identifier|.
@@ -109,31 +110,33 @@ message ComputeValueProto {
 
 // Performs a logical AND on all specified values.
 message BooleanAndProto {
-  // The model identifiers to logically AND. All identifiers must point to
-  // boolean ValueProtos.
-  repeated string model_identifiers = 1;
+  // The values to logically AND. All values must be single booleans.
+  repeated ValueReferenceProto values = 2;
+  reserved 1;
 }
 
 // Performs a logical OR on all specified values.
 message BooleanOrProto {
-  // The model identifiers to logically OR. All identifiers must point to
-  // boolean ValueProtos.
-  repeated string model_identifiers = 1;
+  // The values to logically OR. All values must be single booleans.
+  repeated ValueReferenceProto values = 2;
+  reserved 1;
 }
 
 // Performs a logical NOT on the specified value.
 message BooleanNotProto {
-  // The model identifier to logically invert. Must point to a boolean value.
-  optional string model_identifier = 1;
+  // The value to logically invert. Must be a single boolean.
+  optional ValueReferenceProto value = 2;
+  reserved 1;
 }
 
 // Creates a string representation of the specified value.
 message ToStringProto {
-  // The model identifier to stringify.
-  optional string model_identifier = 1;
+  // The value to stringify.
+  optional ValueReferenceProto value = 3;
 
   // Optional format options.
   oneof format_options { DateFormatProto date_format = 2; }
+  reserved 1;
 }
 
 // A format string for a date, such as "EEE, MMM d". See
@@ -156,19 +159,21 @@ message ValueComparisonProto {
   }
 
   // The first value to compare.
-  optional string model_identifier_a = 1;
+  optional ValueReferenceProto value_a = 4;
   // The second value to compare.
-  optional string model_identifier_b = 2;
+  optional ValueReferenceProto value_b = 5;
   // The comparison mode.
   optional Mode mode = 3;
+
+  reserved 1, 2;
 }
 
-// Computes the sum of two single integers.
+// Computes the sum of all input integers.
 message IntegerSumProto {
-  // The first model identifier. Must point to an integer value.
-  optional string model_identifier_a = 1;
-  // The second model identifier. Must point to an integer value.
-  optional string model_identifier_b = 2;
+  // The values to sum. All values must be single integers.
+  repeated ValueReferenceProto values = 3;
+
+  reserved 1, 2;
 }
 
 // Displays a standard info popup.
@@ -187,12 +192,12 @@ message ShowListPopupProto {
     DISABLED = 1;
     ENABLED = 2;
   }
-  // The item names to show in the list. Must be a list of strings.
-  optional string item_names_model_identifier = 1;
+  // The item names to show in the list. Must be a StringList.
+  optional ValueReferenceProto item_names = 6;
   // Optional, must be the same length as the list stored at
   // |items_list_model_identifier| if specified. Will default to ENABLED for all
   // items if not specified. Must be a list of int32 |ItemType| if specified.
-  optional string item_types_model_identifier = 2;
+  optional ValueReferenceProto item_types = 7;
   // The indices of the selected items (both input and output). Must be a list
   // of integers.
   optional string selected_item_indices_model_identifier = 3;
@@ -201,14 +206,17 @@ message ShowListPopupProto {
   // Optional output model identifier to store the names of the selected items
   // in a StringList.
   optional string selected_item_names_model_identifier = 5;
+
+  reserved 1, 2;
 }
 
 // Sets the list of available user actions. User actions are either direct
 // actions performed via assistant and/or chips that users can tap.
 message SetUserActionsProto {
-  // The model identifier containing the user actions. Must point to a
-  // UserActionList value.
-  optional string model_identifier = 1;
+  // The user actions to set. Must contain a UserActionList value.
+  optional ValueReferenceProto user_actions = 2;
+
+  reserved 1;
 }
 
 // Enables or disables a single user action. Modifies the value in-place.
@@ -217,9 +225,11 @@ message ToggleUserActionProto {
   optional string user_actions_model_identifier = 1;
   // The identifier of the specific user action to modify.
   optional string user_action_identifier = 2;
-  // Whether the user action should be enabled or disabled. Should point to a
-  // single boolean.
-  optional string enabled_model_identifier = 3;
+  // Whether the user action should be enabled or disabled. Should be a single
+  // boolean.
+  optional ValueReferenceProto enabled = 4;
+
+  reserved 3;
 }
 
 // Ends the generic Ui action (this is usually tied to a chip).
@@ -235,24 +245,25 @@ message EndActionProto {
 // Displays a calendar popup that lets the user select a date.
 message ShowCalendarPopupProto {
   // The date model identifier (both input and output). Must point to a single
-  // DateList. If not set, the calendar will default to the current date.
+  // Date. If not set, the calendar will default to the current date.
   optional string date_model_identifier = 1;
-  // The model identifier for the minimum allowed date. Must point to a single
-  // DateList.
-  optional string min_date_model_identifier = 2;
-  // The model identifier for the minimum allowed date. Must point to a single
-  // DateList.
-  optional string max_date_model_identifier = 3;
+  // The minimum allowed date. Must point to a single Date.
+  optional ValueReferenceProto min_date = 4;
+  // The maximum allowed date. Must point to a single Date.
+  optional ValueReferenceProto max_date = 5;
+
+  reserved 2, 3;
 }
 
 // Modifies the displayed text of a text view or a text input view.
 message SetTextProto {
-  // The model identifier containing the text to set. Must point to a single
-  // string.
-  optional string model_identifier = 1;
+  // The text to set. Must be a single string.
+  optional ValueReferenceProto text = 3;
   // The text view to modify. Must point to a text view or a text input
   // view.
   optional string view_identifier = 2;
+
+  reserved 1;
 }
 
 // Modifies a view's visibiliy.
@@ -260,7 +271,8 @@ message SetViewVisibilityProto {
   // The view to modify.
   optional string view_identifier = 1;
 
-  // The model identifier containing the visibility flag. Must point to a single
-  // bool.
-  optional string model_identifier = 2;
+  // The visibility flag. Must be a single boolean.
+  optional ValueReferenceProto visible = 3;
+
+  reserved 2;
 }

components/autofill_assistant/browser/model.proto

@@ -31,6 +31,13 @@ message ValueProto {
   }
 }
 
+message ValueReferenceProto {
+  oneof kind {
+    ValueProto value = 1;
+    string model_identifier = 2;
+  }
+}
+
 message StringList {
   repeated string values = 1;
 }

components/autofill_assistant/browser/user_model.cc

@@ -42,6 +42,18 @@ base::Optional<ValueProto> UserModel::GetValue(
   return base::nullopt;
 }
 
+base::Optional<ValueProto> UserModel::GetValue(
+    const ValueReferenceProto& reference) const {
+  switch (reference.kind_case()) {
+    case ValueReferenceProto::kValue:
+      return reference.value();
+    case ValueReferenceProto::kModelIdentifier:
+      return GetValue(reference.model_identifier());
+    case ValueReferenceProto::KIND_NOT_SET:
+      return base::nullopt;
+  }
+}
+
 void UserModel::MergeWithProto(const ModelProto& another,
                                bool force_notifications) {
   for (const auto& another_value : another.values()) {

components/autofill_assistant/browser/user_model.h

@@ -49,15 +49,18 @@ class UserModel {
   // Returns the value for |identifier| or nullopt if there is no such value.
   base::Optional<ValueProto> GetValue(const std::string& identifier) const;
 
+  // Returns the value for |reference| or nullopt if there is no such value.
+  base::Optional<ValueProto> GetValue(
+      const ValueReferenceProto& reference) const;
+
   // Returns all specified values in a new std::vector. Returns nullopt if any
-  // of the requested values was not found. |model_identifiers| must be
-  // a std::string iterable.
+  // of the requested values was not found.
   template <class T>
   base::Optional<std::vector<ValueProto>> GetValues(
-      const T& model_identifiers) const {
+      const T& value_references) const {
     std::vector<ValueProto> values;
-    for (const auto& identifier : model_identifiers) {
-      auto value = GetValue(identifier);
+    for (const auto& reference : value_references) {
+      auto value = GetValue(reference);
       if (!value.has_value()) {
         return base::nullopt;
       }

components/autofill_assistant/browser/value_util.cc

@@ -205,6 +205,18 @@ std::ostream& operator<<(std::ostream& out, const ValueProto& value) {
   return out;
 }
 
+std::ostream& operator<<(std::ostream& out,
+                         const ValueReferenceProto& reference) {
+  switch (reference.kind_case()) {
+    case ValueReferenceProto::kValue:
+      return out << reference.value();
+    case ValueReferenceProto::kModelIdentifier:
+      return out << reference.model_identifier();
+    case ValueReferenceProto::KIND_NOT_SET:
+      return out;
+  }
+}
+
 // Intended for debugging.  Writes a string representation of |value| to |out|.
 std::ostream& operator<<(std::ostream& out,
                          const ModelProto::ModelValue& value) {

components/autofill_assistant/browser/value_util.h

@@ -40,6 +40,8 @@ bool operator<(const DateProto& value_a, const DateProto& value_b);
 
 // Intended for debugging.
 std::ostream& operator<<(std::ostream& out, const ValueProto& value);
+std::ostream& operator<<(std::ostream& out,
+                         const ValueReferenceProto& reference);
 std::ostream& operator<<(std::ostream& out,
                          const ModelProto::ModelValue& value);
 std::ostream& operator<<(std::ostream& out, const UserActionProto& value);