Implements the script status match in the script preconditions.

Refactored the available_script filtering: we now express it as a precondition
on the ScriptStatus for the current script. I made this precondition a default
precondition only when there is no existing ScriptStatusMatch explicitly set,
so it is now possible to run a script twice.

Change-Id: I2ceb87f62b18dfdb455c9fff2f883000da918848
Bug: 806868
Reviewed-on: https://chromium-review.googlesource.com/1224553
Commit-Queue: Ernest Galbrun <galbrun@chromium.org>
Reviewed-by: Rouslan Solomakhin <rouslan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#594649}

components/autofill_assistant/browser/controller_unittest.cc

@@ -157,13 +157,7 @@ TEST_F(ControllerTest, FetchAndRunScripts) {
   EXPECT_CALL(*mock_service_, OnGetActions(StrEq("script1"), _, _))
       .WillOnce(RunOnceCallback<2>(true, ""));
 
-  // 6. offering the choice: script2
-  EXPECT_CALL(*mock_ui_controller_, UpdateScripts(SizeIs(1)))
-      .WillOnce([](const std::vector<ScriptHandle>& scripts) {
-        EXPECT_EQ("script2", scripts[0].path);
-      });
-  // 7. As nothing is selected from the 2nd UpdateScripts call, the flow
-  // terminates.
+  // 6. As nothing is selected the flow terminates.
 
   // Start the flow.
   SimulateNavigateToUrl(GURL("http://a.example.com/path"));
@@ -201,7 +195,6 @@ TEST_F(ControllerTest, Autostart) {
 
   EXPECT_CALL(*mock_service_, OnGetActions(StrEq("runnable"), _, _))
       .WillOnce(RunOnceCallback<2>(true, ""));
-  EXPECT_CALL(*mock_ui_controller_, UpdateScripts(SizeIs(0)));
 
   SimulateNavigateToUrl(GURL("http://a.example.com/path"));
 }

components/autofill_assistant/browser/script_precondition.cc

@@ -13,12 +13,11 @@
 #include "url/gurl.h"
 
 namespace autofill_assistant {
-
 // Static
 std::unique_ptr<ScriptPrecondition> ScriptPrecondition::FromProto(
-    const ScriptPreconditionProto& proto) {
+    const ScriptPreconditionProto& script_precondition_proto) {
   std::vector<std::vector<std::string>> elements_exist;
-  for (const auto& element : proto.elements_exist()) {
+  for (const auto& element : script_precondition_proto.elements_exist()) {
     std::vector<std::string> selectors;
     for (const auto& selector : element.selectors()) {
       selectors.emplace_back(selector);
@@ -27,12 +26,12 @@ std::unique_ptr<ScriptPrecondition> ScriptPrecondition::FromProto(
   }
 
   std::set<std::string> domain_match;
-  for (const auto& domain : proto.domain()) {
+  for (const auto& domain : script_precondition_proto.domain()) {
     domain_match.emplace(domain);
   }
 
   std::vector<std::unique_ptr<re2::RE2>> path_pattern;
-  for (const auto& pattern : proto.path_pattern()) {
+  for (const auto& pattern : script_precondition_proto.path_pattern()) {
     auto re = std::make_unique<re2::RE2>(pattern);
     if (re->error_code() != re2::RE2::NoError) {
       LOG(ERROR) << "Invalid regexp in script precondition '" << pattern;
@@ -42,20 +41,26 @@ std::unique_ptr<ScriptPrecondition> ScriptPrecondition::FromProto(
   }
 
   std::vector<ScriptParameterMatchProto> parameter_match;
-  for (const auto& match : proto.script_parameter_match()) {
+  for (const auto& match : script_precondition_proto.script_parameter_match()) {
     parameter_match.emplace_back(match);
   }
 
   std::vector<FormValueMatchProto> form_value_match;
-  for (const auto& match : proto.form_value_match()) {
+  for (const auto& match : script_precondition_proto.form_value_match()) {
     form_value_match.emplace_back(match);
   }
 
-  // TODO(crbug.com/806868): Detect unknown or unsupported conditions and reject
-  // them.
+  std::vector<ScriptStatusMatchProto> status_matches;
+  for (const auto& status_match :
+       script_precondition_proto.script_status_match()) {
+    status_matches.push_back(status_match);
+  }
+
+  // TODO(crbug.com/806868): Detect unknown or unsupported conditions and
+  // reject them.
   return std::make_unique<ScriptPrecondition>(
       elements_exist, domain_match, std::move(path_pattern), parameter_match,
-      form_value_match);
+      form_value_match, status_matches);
 }
 
 ScriptPrecondition::~ScriptPrecondition() {}
@@ -63,9 +68,12 @@ ScriptPrecondition::~ScriptPrecondition() {}
 void ScriptPrecondition::Check(
     WebController* web_controller,
     const std::map<std::string, std::string>& parameters,
+    const std::map<std::string, ScriptStatusProto>& executed_scripts,
     base::OnceCallback<void(bool)> callback) {
   const GURL& url = web_controller->GetUrl();
-  if (!MatchDomain(url) || !MatchPath(url) || !MatchParameters(parameters)) {
+
+  if (!MatchDomain(url) || !MatchPath(url) || !MatchParameters(parameters) ||
+      !MatchScriptStatus(executed_scripts)) {
     std::move(callback).Run(false);
     return;
   }
@@ -106,7 +114,8 @@ ScriptPrecondition::ScriptPrecondition(
     const std::set<std::string>& domain_match,
     std::vector<std::unique_ptr<re2::RE2>> path_pattern,
     const std::vector<ScriptParameterMatchProto>& parameter_match,
-    const std::vector<FormValueMatchProto>& form_value_match)
+    const std::vector<FormValueMatchProto>& form_value_match,
+    const std::vector<ScriptStatusMatchProto>& status_match)
     : elements_exist_(elements_exist),
       pending_preconditions_check_count_(0),
       all_preconditions_check_satisfied_(false),
@@ -114,6 +123,7 @@ ScriptPrecondition::ScriptPrecondition(
       path_pattern_(std::move(path_pattern)),
       parameter_match_(parameter_match),
       form_value_match_(form_value_match),
+      status_match_(status_match),
       weak_ptr_factory_(this) {}
 
 void ScriptPrecondition::OnCheckElementExists(bool result) {
@@ -168,6 +178,26 @@ bool ScriptPrecondition::MatchParameters(
   return true;
 }
 
+bool ScriptPrecondition::MatchScriptStatus(
+    const std::map<std::string, ScriptStatusProto>& executed_scripts) const {
+  for (const auto status_match : status_match_) {
+    auto status = SCRIPT_STATUS_NOT_RUN;
+    auto iter = executed_scripts.find(status_match.script());
+    if (iter != executed_scripts.end()) {
+      status = iter->second;
+    }
+    bool has_same_status = status_match.status() == status;
+    switch (status_match.comparator()) {
+      case ScriptStatusMatchProto::DIFFERENT:
+        return !has_same_status;
+      case ScriptStatusMatchProto::EQUAL:
+      default:
+        return has_same_status;
+    }
+  }
+  return true;
+}
+
 void ScriptPrecondition::OnGetFieldValue(const std::string& value) {
   DCHECK_LT(0u, pending_preconditions_check_count_);
   pending_preconditions_check_count_--;

components/autofill_assistant/browser/script_precondition.h

@@ -29,20 +29,22 @@ class ScriptPrecondition {
   // Builds a precondition from its proto representation. Returns nullptr if the
   // preconditions are invalid.
   static std::unique_ptr<ScriptPrecondition> FromProto(
-      const ScriptPreconditionProto& proto);
+      const ScriptPreconditionProto& script_precondition_proto);
 
   ScriptPrecondition(
       const std::vector<std::vector<std::string>>& elements_exist,
       const std::set<std::string>& domain_match,
       std::vector<std::unique_ptr<re2::RE2>> path_pattern,
       const std::vector<ScriptParameterMatchProto>& parameter_match,
-      const std::vector<FormValueMatchProto>& form_value_match);
+      const std::vector<FormValueMatchProto>& form_value_match,
+      const std::vector<ScriptStatusMatchProto>& status_match);
   ~ScriptPrecondition();
 
   // Check whether the conditions satisfied and return the result through
   // |callback|.
   void Check(WebController* web_controller,
              const std::map<std::string, std::string>& parameters,
+             const std::map<std::string, ScriptStatusProto>& executed_scripts,
              base::OnceCallback<void(bool)> callback);
 
  private:
@@ -51,6 +53,9 @@ class ScriptPrecondition {
   bool MatchPath(const GURL& url) const;
   bool MatchParameters(
       const std::map<std::string, std::string>& parameters) const;
+  bool MatchScriptStatus(
+      const std::map<std::string, ScriptStatusProto>& executed_scripts) const;
+
   void OnGetFieldValue(const std::string& value);
 
   // Return if all checks have been completed and we have not returned anything
@@ -74,6 +79,9 @@ class ScriptPrecondition {
   // Conditions on form fields value.
   std::vector<FormValueMatchProto> form_value_match_;
 
+  // Conditions regarding the execution status of passed scripts.
+  std::vector<ScriptStatusMatchProto> status_match_;
+
   base::WeakPtrFactory<ScriptPrecondition> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(ScriptPrecondition);

components/autofill_assistant/browser/script_precondition_unittest.cc

@@ -82,13 +82,15 @@ class ScriptPreconditionTest : public testing::Test {
       return false;
 
     DirectCallback callback;
-    precondition->Check(&mock_web_controller_, parameters_, callback.Get());
+    precondition->Check(&mock_web_controller_, parameters_, executed_scripts_,
+                        callback.Get());
     return callback.GetResultOrDie();
   }
 
   GURL url_;
   MockWebController mock_web_controller_;
   std::map<std::string, std::string> parameters_;
+  std::map<std::string, ScriptStatusProto> executed_scripts_;
 };
 
 TEST_F(ScriptPreconditionTest, NoConditions) {
@@ -143,6 +145,54 @@ TEST_F(ScriptPreconditionTest, BadPathPattern) {
   EXPECT_EQ(nullptr, ScriptPrecondition::FromProto(proto));
 }
 
+TEST_F(ScriptPreconditionTest, WrongScriptStatusEqualComparator) {
+  ScriptPreconditionProto proto;
+
+  ScriptStatusMatchProto* script_status_match = proto.add_script_status_match();
+  script_status_match->set_script("previous_script_success");
+  script_status_match->set_comparator(ScriptStatusMatchProto::EQUAL);
+  script_status_match->set_status(SCRIPT_STATUS_NOT_RUN);
+  executed_scripts_["previous_script_success"] = SCRIPT_STATUS_SUCCESS;
+
+  EXPECT_FALSE(Check(proto));
+}
+
+TEST_F(ScriptPreconditionTest, WrongScriptStatusDifferentComparator) {
+  ScriptPreconditionProto proto;
+
+  ScriptStatusMatchProto* script_status_match = proto.add_script_status_match();
+  script_status_match->set_script("previous_script_success");
+  script_status_match->set_comparator(ScriptStatusMatchProto::DIFFERENT);
+  script_status_match->set_status(SCRIPT_STATUS_NOT_RUN);
+  executed_scripts_["previous_script_success"] = SCRIPT_STATUS_SUCCESS;
+
+  EXPECT_TRUE(Check(proto));
+}
+
+TEST_F(ScriptPreconditionTest, WrongScriptStatusComparatorNotSet) {
+  ScriptPreconditionProto proto;
+
+  ScriptStatusMatchProto* script_status_match = proto.add_script_status_match();
+  script_status_match->set_script("previous_script_success");
+  script_status_match->set_comparator(ScriptStatusMatchProto::EQUAL);
+  script_status_match->set_status(SCRIPT_STATUS_NOT_RUN);
+  executed_scripts_["previous_script_success"] = SCRIPT_STATUS_SUCCESS;
+
+  EXPECT_FALSE(Check(proto));
+}
+
+TEST_F(ScriptPreconditionTest, WrongScriptStatus) {
+  ScriptPreconditionProto proto;
+
+  ScriptStatusMatchProto* script_status_match = proto.add_script_status_match();
+  script_status_match->set_script("previous_script_success");
+  script_status_match->set_comparator(ScriptStatusMatchProto::EQUAL);
+  script_status_match->set_status(SCRIPT_STATUS_NOT_RUN);
+  executed_scripts_["previous_script_success"] = SCRIPT_STATUS_SUCCESS;
+
+  EXPECT_FALSE(Check(proto));
+}
+
 TEST_F(ScriptPreconditionTest, ParameterMustExist) {
   ScriptPreconditionProto proto;
   ScriptParameterMatchProto* match = proto.add_script_parameter_match();

components/autofill_assistant/browser/script_tracker.cc

@@ -40,26 +40,18 @@ void ScriptTracker::CheckScripts() {
   DCHECK_EQ(pending_precondition_check_count_, 0);
   DCHECK(pending_runnable_scripts_.empty());
 
-  std::vector<Script*> scripts_to_check;
-  for (const auto& entry : available_scripts_) {
-    Script* script = entry.first;
-    if (executed_scripts_.find(script->handle.path) ==
-            executed_scripts_.end() &&
-        script->precondition) {
-      scripts_to_check.emplace_back(script);
-    }
-  }
-
   // pending_precondition_check_count_ lets OnPreconditionCheck know when to
   // stop. It must be set before the callback can possibly be run.
-  pending_precondition_check_count_ = scripts_to_check.size();
+  pending_precondition_check_count_ = available_scripts_.size();
   if (pending_precondition_check_count_ == 0) {
     // Possibly report an empty set of runnable scripts.
     UpdateRunnableScriptsIfNecessary();
   } else {
-    for (Script* script : scripts_to_check) {
+    for (const auto& entry : available_scripts_) {
+      Script* script = entry.first;
       script->precondition->Check(
           delegate_->GetWebController(), delegate_->GetParameters(),
+          executed_scripts_,
           base::BindOnce(&ScriptTracker::OnPreconditionCheck,
                          weak_ptr_factory_.GetWeakPtr(), script));
     }
@@ -73,17 +65,19 @@ void ScriptTracker::ExecuteScript(const std::string& script_path,
     return;
   }
 
-  DCHECK(executed_scripts_.find(script_path) == executed_scripts_.end());
-  executed_scripts_.insert(script_path);
+  executed_scripts_[script_path] = SCRIPT_STATUS_RUNNING;
   executor_ = std::make_unique<ScriptExecutor>(script_path, delegate_);
   executor_->Run(base::BindOnce(&ScriptTracker::OnScriptRun,
-                                weak_ptr_factory_.GetWeakPtr(),
+                                weak_ptr_factory_.GetWeakPtr(), script_path,
                                 std::move(callback)));
 }
 
 void ScriptTracker::OnScriptRun(
+    const std::string& script_path,
     base::OnceCallback<void(bool)> original_callback,
     bool success) {
+  executed_scripts_[script_path] =
+      success ? SCRIPT_STATUS_SUCCESS : SCRIPT_STATUS_FAILURE;
   executor_.reset();
   std::move(original_callback).Run(success);
 }

components/autofill_assistant/browser/script_tracker.h

@@ -15,6 +15,7 @@
 #include "base/memory/weak_ptr.h"
 #include "components/autofill_assistant/browser/script.h"
 #include "components/autofill_assistant/browser/script_executor.h"
+#include "components/autofill_assistant/browser/service.pb.h"
 
 namespace autofill_assistant {
 class ScriptExecutorDelegate;
@@ -65,7 +66,8 @@ class ScriptTracker {
   bool running() const { return executor_ != nullptr; }
 
  private:
-  void OnScriptRun(base::OnceCallback<void(bool)> original_callback,
+  void OnScriptRun(const std::string& script_path,
+                   base::OnceCallback<void(bool)> original_callback,
                    bool success);
   void UpdateRunnableScriptsIfNecessary();
 
@@ -89,10 +91,8 @@ class ScriptTracker {
   // any pending check.
   std::map<Script*, std::unique_ptr<Script>> available_scripts_;
 
-  // Set of scripts that have been executed. They'll be excluded from runnable.
-  // TODO(crbug.com/806868): Track script execution status and forward
-  // that information to the script precondition.
-  std::set<std::string> executed_scripts_;
+  // List of scripts that have been executed and their corresponding statuses.
+  std::map<std::string, ScriptStatusProto> executed_scripts_;
 
   // Number of precondition checks run for CheckScripts that are still
   // pending.

components/autofill_assistant/browser/script_tracker_unittest.cc

@@ -86,6 +86,13 @@ class ScriptTrackerTest : public testing::Test,
         ->mutable_precondition()
         ->add_elements_exist()
         ->add_selectors(selector);
+    ScriptStatusMatchProto dont_run_twice_precondition;
+    dont_run_twice_precondition.set_script(path);
+    dont_run_twice_precondition.set_comparator(ScriptStatusMatchProto::EQUAL);
+    dont_run_twice_precondition.set_status(SCRIPT_STATUS_NOT_RUN);
+    *script->mutable_presentation()
+         ->mutable_precondition()
+         ->add_script_status_match() = dont_run_twice_precondition;
     return script;
   }
 

components/autofill_assistant/browser/service.proto

@@ -60,6 +60,41 @@ message SupportedScriptProto {
   optional PresentationProto presentation = 2;
 }
 
+enum ScriptStatusProto {
+  // Never explicitly set. Reading this value means the enum field is either
+  // not set or set to a value not listed here.
+  UNKNOWN_SCRIPT_STATUS = 0;
+  // The script finished successfully.
+  SCRIPT_STATUS_SUCCESS = 1;
+  // The script failed.
+  SCRIPT_STATUS_FAILURE = 2;
+  // The user cancelled the script.
+  SCRIPT_STATUS_CANCELLED = 3;
+  // The script is currently running.
+  SCRIPT_STATUS_RUNNING = 4;
+  // The script was not run.
+  SCRIPT_STATUS_NOT_RUN = 5;
+}
+
+// Condition on the status of a previous script run.
+message ScriptStatusMatchProto {
+  enum Comparator {
+    UNSPECIFIED = 0;
+    EQUAL = 1;
+    DIFFERENT = 2;
+  }
+
+  // Required. Path of the script whose status should be checked.
+  optional string script = 1;
+
+  // Required. The status the script should have for the condition to hold.
+  optional ScriptStatusProto status = 2;
+
+  // Optional. The comparison performed when checking the status. It will be
+  // interpreted as EQUAL if not set.
+  optional Comparator comparator = 3;
+}
+
 message ScriptPreconditionProto {
   // Combined with AND: the elements referenced here must be present.
   repeated ElementReferenceProto elements_exist = 3;
@@ -69,7 +104,7 @@ message ScriptPreconditionProto {
   repeated string domain = 6;
   // Combined with AND: all matches must be true for precondition to hold.
   repeated ScriptParameterMatchProto script_parameter_match = 7;
-  // Combined with AND: all matches must be true for precondition to hold.
+  repeated ScriptStatusMatchProto script_status_match = 8;
   repeated FormValueMatchProto form_value_match = 9;
 }