Reland "[Autofill Assistant] Bind the script tracker with the controller."

This is a reland of 5bf6f1b1

Original change's description:
> [Autofill Assistant] Bind the script tracker with the controller.
> 
> With this change, the controller tells the script tracker to run checks
> when necessary, calls the UI controller to choose a script to run and
> run the script that was chosen. The implementation of the UI controller
> for choosing the script is not included into this change.
> 
> Integration is preliminary, just to get something to get started until
> we have a usable UI to try it out. The UI part, implementing script
> selection, is missing from this change.
> 
> Change-Id: I867bec0e554dfdd87fc2f95e7c9088391621c5c9
> Bug: 806868
> Reviewed-on: https://chromium-review.googlesource.com/1194034
> Commit-Queue: Stephane Zermatten <szermatt@chromium.org>
> Reviewed-by: Ganggui Tang <gogerald@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#589135}

Bug: 806868
Change-Id: I40b66adca74162a1a41a47f1e065d2bf3aa225ed
Reviewed-on: https://chromium-review.googlesource.com/1209709
Commit-Queue: Stephane Zermatten <szermatt@chromium.org>
Reviewed-by: Ganggui Tang <gogerald@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589923}

components/autofill_assistant/browser/BUILD.gn

@@ -63,6 +63,7 @@ jumbo_static_library("browser") {
 source_set("unit_tests") {
   testonly = true
   sources = [
+    "controller_unittest.cc",
     "mock_run_once_callback.h",
     "mock_service.cc",
     "mock_service.h",
@@ -79,6 +80,7 @@ source_set("unit_tests") {
     ":proto",
     "//base",
     "//base/test:test_support",
+    "//content/test:test_support",
     "//testing/gmock",
     "//testing/gtest",
   ]

components/autofill_assistant/browser/controller.cc

@@ -17,7 +17,10 @@ namespace autofill_assistant {
 void Controller::CreateAndStartForWebContents(
     content::WebContents* web_contents,
     std::unique_ptr<Client> client) {
-  new Controller(web_contents, std::move(client));
+  new Controller(web_contents, std::move(client),
+                 WebController::CreateForWebContents(web_contents),
+                 std::make_unique<Service>(client->GetApiKey(),
+                                           web_contents->GetBrowserContext()));
 }
 
 Service* Controller::GetService() {
@@ -37,54 +40,100 @@ ClientMemory* Controller::GetClientMemory() {
 }
 
 Controller::Controller(content::WebContents* web_contents,
-                       std::unique_ptr<Client> client)
+                       std::unique_ptr<Client> client,
+                       std::unique_ptr<WebController> web_controller,
+                       std::unique_ptr<Service> service)
     : content::WebContentsObserver(web_contents),
       client_(std::move(client)),
-      web_controller_(WebController::CreateForWebContents(web_contents)),
-      service_(std::make_unique<Service>(client_->GetApiKey(),
-                                         web_contents->GetBrowserContext())),
+      web_controller_(std::move(web_controller)),
+      service_(std::move(service)),
       script_tracker_(std::make_unique<ScriptTracker>(this, this)) {
   GetUiController()->SetUiDelegate(this);
   GetUiController()->ShowOverlay();
   if (!web_contents->IsLoading()) {
-    GetScripts();
+    GetOrCheckScripts(web_contents->GetLastCommittedURL());
   }
 }
 
 Controller::~Controller() {}
 
-void Controller::GetScripts() {
-  service_->GetScriptsForUrl(
-      web_contents()->GetLastCommittedURL(),
-      base::BindOnce(&Controller::OnGetScripts, base::Unretained(this)));
+void Controller::GetOrCheckScripts(const GURL& url) {
+  if (script_tracker_->running())
+    return;
+
+  if (script_domain_ != url.host()) {
+    script_domain_ = url.host();
+    service_->GetScriptsForUrl(
+        url,
+        base::BindOnce(&Controller::OnGetScripts, base::Unretained(this), url));
+  } else {
+    script_tracker_->CheckScripts();
+  }
 }
 
-void Controller::OnGetScripts(bool result, const std::string& response) {
+void Controller::OnGetScripts(const GURL& url,
+                              bool result,
+                              const std::string& response) {
+  // If the domain of the current URL changed since the request was sent, the
+  // response is not relevant anymore and can be safely discarded.
+  if (url.host() != script_domain_)
+    return;
+
   if (!result) {
-    LOG(ERROR) << "Failed to get assistant scripts for URL "
-               << web_contents()->GetLastCommittedURL().spec();
+    LOG(ERROR) << "Failed to get assistant scripts for URL " << url.spec();
     // TODO(crbug.com/806868): Terminate Autofill Assistant.
     return;
   }
+
   std::vector<std::unique_ptr<Script>> scripts;
   bool parse_result = ProtocolUtils::ParseScripts(response, &scripts);
   DCHECK(parse_result);
   script_tracker_->SetAndCheckScripts(std::move(scripts));
 }
 
+void Controller::OnScriptExecuted(const std::string& script_path,
+                                  bool success) {
+  GetUiController()->HideOverlay();
+  if (!success) {
+    LOG(ERROR) << "Failed to execute script " << script_path;
+    // TODO(crbug.com/806868): Handle script execution failure.
+  }
+
+  GetOrCheckScripts(web_contents()->GetLastCommittedURL());
+}
+
 void Controller::OnClickOverlay() {
   GetUiController()->HideOverlay();
   // TODO(crbug.com/806868): Stop executing scripts.
 }
 
 void Controller::OnScriptSelected(const std::string& script_path) {
-  // TODO(crbug.com/806868): Forward to the execution engine.
+  DCHECK(!script_path.empty());
+
+  GetUiController()->ShowOverlay();
+  script_tracker_->ExecuteScript(
+      script_path, base::BindOnce(&Controller::OnScriptExecuted,
+                                  // script_tracker_ is owned by Controller.
+                                  base::Unretained(this), script_path));
 }
 
 void Controller::OnDestroy() {
   delete this;
 }
 
+void Controller::DidGetUserInteraction(const blink::WebInputEvent::Type type) {
+  switch (type) {
+    case blink::WebInputEvent::kGestureLongTap:
+    case blink::WebInputEvent::kGestureTap:
+      if (!script_tracker_->running())
+        script_tracker_->CheckScripts();
+      break;
+
+    default:
+      break;
+  }
+}
+
 void Controller::OnRunnableScriptsChanged(
     const std::vector<ScriptHandle>& runnable_scripts) {
   // Script selection is disabled when a script is already running. We will
@@ -99,7 +148,7 @@ void Controller::DidFinishLoad(content::RenderFrameHost* render_frame_host,
                                const GURL& validated_url) {
   // TODO(crbug.com/806868): Find a better time to get and update assistant
   // scripts.
-  GetScripts();
+  GetOrCheckScripts(validated_url);
 }
 
 void Controller::WebContentsDestroyed() {

components/autofill_assistant/browser/controller.h

@@ -8,6 +8,7 @@
 #include <memory>
 #include <string>
 
+#include "base/macros.h"
 #include "components/autofill_assistant/browser/client.h"
 #include "components/autofill_assistant/browser/client_memory.h"
 #include "components/autofill_assistant/browser/script.h"
@@ -24,6 +25,8 @@ class WebContents;
 }  // namespace content
 
 namespace autofill_assistant {
+class ControllerTest;
+
 // Autofill assistant controller controls autofill assistant action detection,
 // display, execution and so on. The instance of this object self deletes when
 // the web contents is being destroyed.
@@ -42,12 +45,18 @@ class Controller : public ScriptExecutorDelegate,
   ClientMemory* GetClientMemory() override;
 
  private:
+  friend ControllerTest;
+
   Controller(content::WebContents* web_contents,
-             std::unique_ptr<Client> client);
+             std::unique_ptr<Client> client,
+             std::unique_ptr<WebController> web_controller,
+             std::unique_ptr<Service> service);
   ~Controller() override;
 
-  void GetScripts();
-  void OnGetScripts(bool result, const std::string& response);
+  void GetOrCheckScripts(const GURL& url);
+  void OnGetScripts(const GURL& url, bool result, const std::string& response);
+  void OnScriptChosen(const std::string& script_path);
+  void OnScriptExecuted(const std::string& script_path, bool success);
 
   // Overrides content::UiDelegate:
   void OnClickOverlay() override;
@@ -59,6 +68,7 @@ class Controller : public ScriptExecutorDelegate,
       const std::vector<ScriptHandle>& runnable_scripts) override;
 
   // Overrides content::WebContentsObserver:
+  void DidGetUserInteraction(const blink::WebInputEvent::Type type) override;
   void DidFinishLoad(content::RenderFrameHost* render_frame_host,
                      const GURL& validated_url) override;
   void WebContentsDestroyed() override;
@@ -67,6 +77,9 @@ class Controller : public ScriptExecutorDelegate,
   std::unique_ptr<WebController> web_controller_;
   std::unique_ptr<Service> service_;
   std::unique_ptr<ScriptTracker> script_tracker_;
+
+  // Domain of the last URL the controller requested scripts from.
+  std::string script_domain_;
   ClientMemory memory_;
 
   DISALLOW_COPY_AND_ASSIGN(Controller);

components/autofill_assistant/browser/controller_unittest.cc

+// Copyright 2018 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/controller.h"
+
+#include <memory>
+#include <utility>
+
+#include "base/test/mock_callback.h"
+#include "components/autofill_assistant/browser/mock_run_once_callback.h"
+#include "components/autofill_assistant/browser/mock_service.h"
+#include "components/autofill_assistant/browser/mock_ui_controller.h"
+#include "components/autofill_assistant/browser/mock_web_controller.h"
+#include "components/autofill_assistant/browser/service.h"
+#include "content/public/test/test_renderer_host.h"
+#include "content/public/test/web_contents_tester.h"
+#include "testing/gmock/include/gmock/gmock.h"
+
+namespace autofill_assistant {
+
+using ::testing::_;
+using ::testing::ElementsAre;
+using ::testing::Eq;
+using ::testing::NiceMock;
+using ::testing::SizeIs;
+using ::testing::StrEq;
+
+namespace {
+
+class FakeClient : public Client {
+ public:
+  FakeClient(std::unique_ptr<UiController> ui_controller)
+      : ui_controller_(std::move(ui_controller)) {}
+
+  // Implements Client
+  std::string GetApiKey() override { return ""; }
+
+  UiController* GetUiController() override { return ui_controller_.get(); }
+
+ private:
+  std::unique_ptr<UiController> ui_controller_;
+};
+
+}  // namespace
+
+class ControllerTest : public content::RenderViewHostTestHarness {
+ public:
+  ControllerTest() {}
+  ~ControllerTest() override {}
+
+  void SetUp() override {
+    content::RenderViewHostTestHarness::SetUp();
+
+    auto ui_controller = std::make_unique<NiceMock<MockUiController>>();
+    mock_ui_controller_ = ui_controller.get();
+    auto web_controller = std::make_unique<NiceMock<MockWebController>>();
+    mock_web_controller_ = web_controller.get();
+    auto service = std::make_unique<NiceMock<MockService>>();
+    mock_service_ = service.get();
+    controller_ = new Controller(
+        web_contents(), std::make_unique<FakeClient>(std::move(ui_controller)),
+        std::move(web_controller), std::move(service));
+
+    // Fetching scripts succeeds for all URLs, but return nothing.
+    ON_CALL(*mock_service_, OnGetScriptsForUrl(_, _))
+        .WillByDefault(RunOnceCallback<1>(true, ""));
+
+    // Scripts run, but have no actions.
+    ON_CALL(*mock_service_, OnGetActions(_, _))
+        .WillByDefault(RunOnceCallback<1>(true, ""));
+
+    // Element "exists" exists.
+    ON_CALL(*mock_web_controller_, OnElementExists(ElementsAre("exists"), _))
+        .WillByDefault(RunOnceCallback<1>(true));
+
+    tester_ = content::WebContentsTester::For(web_contents());
+  }
+
+  void TearDown() override {
+    controller_->OnDestroy();  // deletes the controller and mocks
+    content::RenderViewHostTestHarness::TearDown();
+  }
+
+ protected:
+  static void AddRunnableScript(SupportsScriptResponseProto* response,
+                                const std::string& name,
+                                const std::string& path) {
+    SupportedScriptProto* script = response->add_scripts();
+    script->set_path(path);
+    script->mutable_presentation()->set_name(name);
+    script->mutable_presentation()
+        ->mutable_precondition()
+        ->add_elements_exist()
+        ->add_selectors("exists");
+  }
+
+  // Updates the current url of the controller and forces a refresh, without
+  // bothering with actually rendering any page content.
+  void SimulateNavigateToUrl(const GURL& url) {
+    tester_->SetLastCommittedURL(url);
+    controller_->DidFinishLoad(nullptr, url);
+  }
+
+  // Sets up the next call to the service for scripts to return |response|.
+  void SetNextScriptResponse(const SupportsScriptResponseProto& response) {
+    std::string response_str;
+    response.SerializeToString(&response_str);
+
+    EXPECT_CALL(*mock_service_, OnGetScriptsForUrl(_, _))
+        .WillOnce(RunOnceCallback<1>(true, response_str));
+  }
+
+  UiDelegate* GetUiDelegate() { return controller_; }
+
+  MockService* mock_service_;
+  MockWebController* mock_web_controller_;
+  MockUiController* mock_ui_controller_;
+  content::WebContentsTester* tester_;
+  Controller* controller_;
+};
+
+TEST_F(ControllerTest, FetchAndRunScripts) {
+  // Going to the URL triggers a whole flow:
+  // 1. loading scripts
+  SupportsScriptResponseProto script_response;
+  AddRunnableScript(&script_response, "script1 name", "script1");
+  AddRunnableScript(&script_response, "script2 name", "script2");
+  SetNextScriptResponse(script_response);
+
+  // 2. checking the scripts
+  // 3. offering the choices: script1 and script2
+  // 4. script1 is chosen
+  EXPECT_CALL(*mock_ui_controller_, UpdateScripts(SizeIs(2)))
+      .WillOnce([this](const std::vector<ScriptHandle>& scripts) {
+        EXPECT_EQ("script1", scripts[0].path);
+        EXPECT_EQ("script2", scripts[1].path);
+        GetUiDelegate()->OnScriptSelected("script1");
+      });
+
+  // 5. script1 run successfully (no actions).
+  EXPECT_CALL(*mock_service_, OnGetActions(StrEq("script1"), _))
+      .WillOnce(RunOnceCallback<1>(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.
+
+  // Start the flow.
+  SimulateNavigateToUrl(GURL("http://a.example.com/path"));
+}
+
+TEST_F(ControllerTest, RefreshScriptWhenDomainChanges) {
+  EXPECT_CALL(*mock_service_,
+              OnGetScriptsForUrl(Eq(GURL("http://a.example.com/path1")), _))
+      .WillOnce(RunOnceCallback<1>(true, ""));
+  EXPECT_CALL(*mock_service_,
+              OnGetScriptsForUrl(Eq(GURL("http://b.example.com/path1")), _))
+      .WillOnce(RunOnceCallback<1>(true, ""));
+
+  SimulateNavigateToUrl(GURL("http://a.example.com/path1"));
+  SimulateNavigateToUrl(GURL("http://a.example.com/path2"));
+  SimulateNavigateToUrl(GURL("http://b.example.com/path1"));
+  SimulateNavigateToUrl(GURL("http://b.example.com/path2"));
+}
+
+}  // namespace autofill_assistant

components/autofill_assistant/browser/script_tracker.h

@@ -18,7 +18,6 @@
 
 namespace autofill_assistant {
 class ScriptExecutorDelegate;
-class ScriptTrackerTest;
 
 // The script tracker keeps track of which scripts are available, which are
 // running, which have run, which are runnable whose preconditions are met.
@@ -66,8 +65,6 @@ class ScriptTracker {
   bool running() const { return executor_ != nullptr; }
 
  private:
-  friend ScriptTrackerTest;
-
   void OnScriptRun(base::OnceCallback<void(bool)> original_callback,
                    bool success);
   void UpdateRunnableScriptsIfNecessary();

components/autofill_assistant/browser/ui_controller.h

@@ -5,9 +5,11 @@
 #ifndef COMPONENTS_AUTOFILL_ASSISTANT_BROWSER_UI_CONTROLLER_H_
 #define COMPONENTS_AUTOFILL_ASSISTANT_BROWSER_UI_CONTROLLER_H_
 
+#include "components/autofill_assistant/browser/script.h"
 #include "components/autofill_assistant/browser/ui_delegate.h"
 
 #include <string>
+#include <vector>
 
 #include "base/callback_forward.h"
 

components/autofill_assistant/browser/ui_delegate.h

@@ -20,7 +20,7 @@ class UiDelegate {
   // detached from the associated activity.
   virtual void OnDestroy() = 0;
 
-  // Called when a script has been selected.
+  // Called when a script was selected for execution.
   virtual void OnScriptSelected(const std::string& script_path) = 0;
 
  protected: