[Autofill Assistant] Server Payload is saved between requests.

The Server Payload should not only be passed for GetNextActions, but
also for GetActions.

Now it is saved inside ClientMemory as ScriptExecutor has too short
lifetime (only 1 request/response roundtrip).

Bug: 806868
Change-Id: Id18ab90bfc2c4ea69bd07caab6373c6d0dbf139f
Reviewed-on: https://chromium-review.googlesource.com/c/1298013
Commit-Queue: Lukasz Suder <lsuder@google.com>
Reviewed-by: Stephane Zermatten <szermatt@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602373}

components/autofill_assistant/browser/mock_service.h

@@ -34,6 +34,7 @@ class MockService : public Service {
   void GetActions(const std::string& script_path,
                   const GURL& ignored_url,
                   const std::map<std::string, std::string>& parameters,
+                  const std::string& server_payload,
                   ResponseCallback callback) override {
     OnGetActions(script_path, parameters, callback);
   }

components/autofill_assistant/browser/protocol_utils.cc

@@ -105,7 +105,8 @@ bool ProtocolUtils::ParseScripts(
 std::string ProtocolUtils::CreateInitialScriptActionsRequest(
     const std::string& script_path,
     const GURL& url,
-    const std::map<std::string, std::string>& parameters) {
+    const std::map<std::string, std::string>& parameters,
+    const std::string& server_payload) {
   ScriptActionRequestProto request_proto;
   InitialScriptActionsRequestProto* initial_request_proto =
       request_proto.mutable_initial_request();
@@ -119,6 +120,10 @@ std::string ProtocolUtils::CreateInitialScriptActionsRequest(
   request_proto.mutable_client_context()->mutable_chrome()->set_chrome_version(
       version_info::GetProductNameAndVersionForUserAgent());
 
+  if (!server_payload.empty()) {
+    request_proto.set_server_payload(server_payload);
+  }
+
   std::string serialized_initial_request_proto;
   bool success =
       request_proto.SerializeToString(&serialized_initial_request_proto);

components/autofill_assistant/browser/protocol_utils.h

@@ -43,7 +43,8 @@ class ProtocolUtils {
   static std::string CreateInitialScriptActionsRequest(
       const std::string& script_path,
       const GURL& url,
-      const std::map<std::string, std::string>& parameters);
+      const std::map<std::string, std::string>& parameters,
+      const std::string& server_payload);
 
   // Create request to get next sequence of actions for a script.
   static std::string CreateNextScriptActionsRequest(

components/autofill_assistant/browser/protocol_utils_unittest.cc

@@ -75,7 +75,8 @@ TEST(ProtocolUtilsTest, CreateInitialScriptActionsRequest) {
   ScriptActionRequestProto request;
   EXPECT_TRUE(
       request.ParseFromString(ProtocolUtils::CreateInitialScriptActionsRequest(
-          "script_path", GURL("http://example.com/"), parameters)));
+          "script_path", GURL("http://example.com/"), parameters,
+          "server_payload")));
 
   EXPECT_THAT(request.client_context().chrome().chrome_version(),
               Not(IsEmpty()));
@@ -88,6 +89,7 @@ TEST(ProtocolUtilsTest, CreateInitialScriptActionsRequest) {
   EXPECT_EQ("b", initial.script_parameters(0).value());
   EXPECT_EQ("c", initial.script_parameters(1).name());
   EXPECT_EQ("d", initial.script_parameters(1).value());
+  EXPECT_EQ("server_payload", request.server_payload());
 }
 
 TEST(ProtocolUtilsTest, CreateGetScriptsRequest) {

components/autofill_assistant/browser/script_executor.cc

@@ -14,6 +14,7 @@
 #include "base/time/time.h"
 #include "components/autofill/core/browser/credit_card.h"
 #include "components/autofill_assistant/browser/batch_element_checker.h"
+#include "components/autofill_assistant/browser/client_memory.h"
 #include "components/autofill_assistant/browser/protocol_utils.h"
 #include "components/autofill_assistant/browser/service.h"
 #include "components/autofill_assistant/browser/ui_controller.h"
@@ -29,8 +30,12 @@ constexpr base::TimeDelta kWaitForSelectorDeadline =
 }  // namespace
 
 ScriptExecutor::ScriptExecutor(const std::string& script_path,
+                               const std::string& server_payload,
+                               ScriptExecutor::Listener* listener,
                                ScriptExecutorDelegate* delegate)
     : script_path_(script_path),
+      last_server_payload_(server_payload),
+      listener_(listener),
       delegate_(delegate),
       at_end_(CONTINUE),
       should_stop_script_(false),
@@ -46,7 +51,7 @@ void ScriptExecutor::Run(RunScriptCallback callback) {
 
   delegate_->GetService()->GetActions(
       script_path_, delegate_->GetWebController()->GetUrl(),
-      delegate_->GetParameters(),
+      delegate_->GetParameters(), last_server_payload_,
       base::BindOnce(&ScriptExecutor::OnGetActions,
                      weak_ptr_factory_.GetWeakPtr()));
 }
@@ -201,6 +206,9 @@ void ScriptExecutor::OnGetActions(bool result, const std::string& response) {
 
   bool parse_result =
       ProtocolUtils::ParseActions(response, &last_server_payload_, &actions_);
+  if (listener_) {
+    listener_->OnServerPayloadChanged(last_server_payload_);
+  }
   if (!parse_result) {
     RunCallback(false);
     return;

components/autofill_assistant/browser/script_executor.h

@@ -22,8 +22,22 @@ namespace autofill_assistant {
 // Class to execute an assistant script.
 class ScriptExecutor : public ActionDelegate {
  public:
-  // |delegate| should outlive this object and should not be nullptr.
+  // Listens to events on ScriptExecutor.
+  // TODO(b/806868): Make server_payload a part of callback instead of the
+  // listener.
+  class Listener {
+   public:
+    virtual ~Listener() = default;
+
+    // Called when new server_payload is available.
+    virtual void OnServerPayloadChanged(const std::string& server_payload) = 0;
+  };
+
+  // |delegate| and |listener| should outlive this object and should not be
+  // nullptr.
   ScriptExecutor(const std::string& script_path,
+                 const std::string& server_payload,
+                 ScriptExecutor::Listener* listener,
                  ScriptExecutorDelegate* delegate);
   ~ScriptExecutor() override;
 
@@ -105,12 +119,13 @@ class ScriptExecutor : public ActionDelegate {
   void OnProcessedAction(std::unique_ptr<ProcessedActionProto> action);
 
   std::string script_path_;
+  std::string last_server_payload_;
+  ScriptExecutor::Listener* const listener_;
   ScriptExecutorDelegate* delegate_;
   RunScriptCallback callback_;
 
   std::vector<std::unique_ptr<Action>> actions_;
   std::vector<ProcessedActionProto> processed_actions_;
-  std::string last_server_payload_;
   AtEnd at_end_;
   bool should_stop_script_;
   bool should_clean_contextual_ui_on_finish_;

components/autofill_assistant/browser/script_executor_unittest.cc

@@ -32,10 +32,12 @@ using ::testing::StrEq;
 using ::testing::StrictMock;
 using ::testing::_;
 
-class ScriptExecutorTest : public testing::Test, public ScriptExecutorDelegate {
+class ScriptExecutorTest : public testing::Test,
+                           public ScriptExecutorDelegate,
+                           public ScriptExecutor::Listener {
  public:
   void SetUp() override {
-    executor_ = std::make_unique<ScriptExecutor>("script path", this);
+    executor_ = std::make_unique<ScriptExecutor>("script path", "", this, this);
     url_ = GURL("http://example.com/");
 
     // In this test, "tell" actions always succeed and "click" actions always
@@ -68,6 +70,8 @@ class ScriptExecutorTest : public testing::Test, public ScriptExecutorDelegate {
     return nullptr;
   }
 
+  void OnServerPayloadChanged(const std::string& server_payload) override {}
+
   content::WebContents* GetWebContents() override { return nullptr; }
 
   std::string Serialize(const google::protobuf::MessageLite& message) {

components/autofill_assistant/browser/script_tracker.cc

@@ -86,7 +86,8 @@ void ScriptTracker::ExecuteScript(const std::string& script_path,
   }
 
   executed_scripts_[script_path] = SCRIPT_STATUS_RUNNING;
-  executor_ = std::make_unique<ScriptExecutor>(script_path, delegate_);
+  executor_ = std::make_unique<ScriptExecutor>(
+      script_path, last_server_payload_, this, delegate_);
   ScriptExecutor::RunScriptCallback run_script_callback = base::BindOnce(
       &ScriptTracker::OnScriptRun, weak_ptr_factory_.GetWeakPtr(), script_path,
       std::move(callback));
@@ -190,4 +191,8 @@ void ScriptTracker::ClearAvailableScripts() {
   TerminatePendingChecks();
 }
 
+void ScriptTracker::OnServerPayloadChanged(const std::string& server_payload) {
+  last_server_payload_ = server_payload;
+}
+
 }  // namespace autofill_assistant

components/autofill_assistant/browser/script_tracker.h

@@ -26,7 +26,7 @@ class ScriptExecutorDelegate;
 //
 // User of this class is responsible for retrieving and passing scripts to the
 // tracker and letting the tracker know about changes to the DOM.
-class ScriptTracker {
+class ScriptTracker : public ScriptExecutor::Listener {
  public:
   // Listens to changes on the ScriptTracker state.
   class Listener {
@@ -44,7 +44,7 @@ class ScriptTracker {
   ScriptTracker(ScriptExecutorDelegate* delegate,
                 ScriptTracker::Listener* listener);
 
-  ~ScriptTracker();
+  ~ScriptTracker() override;
 
   // Updates the set of available |scripts|. This interrupts any pending checks,
   // but don't start a new one.'
@@ -84,6 +84,9 @@ class ScriptTracker {
   void UpdateRunnableScriptsIfNecessary();
   void OnCheckDone();
 
+  // Overrides ScriptExecutor::Listener.
+  void OnServerPayloadChanged(const std::string& server_payload) override;
+
   // Cleans up any state use by pending checks. Stops running pending checks.
   void TerminatePendingChecks();
 
@@ -129,6 +132,8 @@ class ScriptTracker {
   // |pending_run_script_callback_| is not nullptr.
   ScriptExecutor::RunScriptCallback pending_run_script_callback_;
 
+  std::string last_server_payload_;
+
   base::WeakPtrFactory<ScriptTracker> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(ScriptTracker);

components/autofill_assistant/browser/service.cc

@@ -96,12 +96,13 @@ void Service::GetScriptsForUrl(
 void Service::GetActions(const std::string& script_path,
                          const GURL& url,
                          const std::map<std::string, std::string>& parameters,
+                         const std::string& server_payload,
                          ResponseCallback callback) {
   DCHECK(!script_path.empty());
 
   SendRequest(AddLoader(script_action_server_url_,
                         ProtocolUtils::CreateInitialScriptActionsRequest(
-                            script_path, url, parameters),
+                            script_path, url, parameters, server_payload),
                         std::move(callback)));
 }
 

components/autofill_assistant/browser/service.h

@@ -51,6 +51,7 @@ class Service {
   virtual void GetActions(const std::string& script_path,
                           const GURL& url,
                           const std::map<std::string, std::string>& parameters,
+                          const std::string& server_payload,
                           ResponseCallback callback);
 
   // Get next sequence of actions according to server payload in previous