[Autofill Assistant] Remove dep of trigger scripts on client_android.

Bug: b/174305656
Change-Id: I065dc27cee934dd22b9adb4034f2796932a06fbe
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2617940
Commit-Queue: Clemens Arbesser <arbesser@google.com>
Reviewed-by: Marian Fechete <marianfe@google.com>
Reviewed-by: Mathias Carlen <mcarlen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#841556}

chrome/android/features/autofill_assistant/java/src/org/chromium/chrome/browser/autofill_assistant/AutofillAssistantClient.java

@@ -382,12 +382,6 @@ public class AutofillAssistantClient {
         return ChromeAccessibilityUtil.get().isAccessibilityEnabled();
     }
 
-    /** Returns whether the user has seen a trigger script before or not. */
-    @CalledByNative
-    private static boolean isFirstTimeTriggerScriptUser() {
-        return AutofillAssistantPreferencesUtil.isAutofillAssistantFirstTimeLiteScriptUser();
-    }
-
     /** Adds a dynamic action to the given reporter. */
     @CalledByNative
     private void onFetchWebsiteActions(Callback<Boolean> callback, boolean success) {

chrome/android/features/autofill_assistant/java/src/org/chromium/chrome/browser/autofill_assistant/trigger_scripts/AssistantTriggerScriptBridge.java

@@ -128,6 +128,12 @@ public class AssistantTriggerScriptBridge {
         return mContext;
     }
 
+    /** Returns whether the user has seen a trigger script before or not. */
+    @CalledByNative
+    private static boolean isFirstTimeTriggerScriptUser() {
+        return AutofillAssistantPreferencesUtil.isAutofillAssistantFirstTimeLiteScriptUser();
+    }
+
     /**
      * Used by native to update and show the UI. The header should be created and updated using
      * {@code createHeaderAndGetModel} prior to calling this function.

chrome/android/features/autofill_assistant/public/java/src/org/chromium/chrome/browser/autofill_assistant/AutofillAssistantPreferencesUtil.java

@@ -52,7 +52,7 @@ public class AutofillAssistantPreferencesUtil {
     }
 
     /** Returns whether the user has seen a lite script before or not. */
-    static boolean isAutofillAssistantFirstTimeLiteScriptUser() {
+    public static boolean isAutofillAssistantFirstTimeLiteScriptUser() {
         return SharedPreferencesManager.getInstance().readBoolean(
                 ChromePreferenceKeys.AUTOFILL_ASSISTANT_FIRST_TIME_LITE_SCRIPT_USER, true);
     }

chrome/browser/android/autofill_assistant/client_android.cc

@@ -181,7 +181,7 @@ void ClientAndroid::StartTriggerScript(
     const base::android::JavaParamRef<jobjectArray>& jparameter_values,
     jlong jservice_request_sender) {
   trigger_script_bridge_.StartTriggerScript(
-      this, jdelegate,
+      web_contents_, jdelegate,
       GURL(base::android::ConvertJavaStringToUTF8(env, jinitial_url)),
       CreateTriggerContext(env, jexperiment_ids, jparameter_names,
                            jparameter_values),
@@ -531,11 +531,6 @@ bool ClientAndroid::HasHadUI() const {
   return has_had_ui_;
 }
 
-bool ClientAndroid::IsFirstTimeTriggerScriptUser() const {
-  return Java_AutofillAssistantClient_isFirstTimeTriggerScriptUser(
-      AttachCurrentThread());
-}
-
 void ClientAndroid::Shutdown(Metrics::DropOutReason reason) {
   if (!controller_)
     return;

chrome/browser/android/autofill_assistant/client_android.h

@@ -124,7 +124,6 @@ class ClientAndroid : public Client,
   void Shutdown(Metrics::DropOutReason reason) override;
   void RecordDropOut(Metrics::DropOutReason reason) override;
   bool HasHadUI() const override;
-  bool IsFirstTimeTriggerScriptUser() const override;
 
   // Overrides AccessTokenFetcher
   void FetchAccessToken(

chrome/browser/android/autofill_assistant/trigger_script_bridge_android.cc

@@ -10,6 +10,7 @@
 #include "chrome/android/features/autofill_assistant/jni_headers/AssistantTriggerScriptBridge_jni.h"
 #include "chrome/browser/android/autofill_assistant/assistant_header_model.h"
 #include "chrome/browser/android/autofill_assistant/ui_controller_android_utils.h"
+#include "chrome/browser/password_manager/chrome_password_manager_client.h"
 #include "chrome/common/channel_info.h"
 #include "components/autofill_assistant/browser/service/api_key_fetcher.h"
 #include "components/autofill_assistant/browser/service/server_url_fetcher.h"
@@ -20,6 +21,7 @@
 #include "components/autofill_assistant/browser/trigger_scripts/dynamic_trigger_conditions.h"
 #include "components/autofill_assistant/browser/trigger_scripts/static_trigger_conditions.h"
 #include "components/autofill_assistant/browser/web/web_controller.h"
+#include "components/autofill_assistant/browser/website_login_manager_impl.h"
 #include "content/public/browser/web_contents.h"
 #include "services/metrics/public/cpp/ukm_recorder.h"
 
@@ -29,13 +31,21 @@ using base::android::ScopedJavaGlobalRef;
 using base::android::ToJavaArrayOfStrings;
 using base::android::ToJavaIntArray;
 
+namespace {
+bool IsFirstTimeTriggerScriptUser() {
+  return autofill_assistant::
+      Java_AssistantTriggerScriptBridge_isFirstTimeTriggerScriptUser(
+          AttachCurrentThread());
+}
+}  // namespace
+
 namespace autofill_assistant {
 
 TriggerScriptBridgeAndroid::TriggerScriptBridgeAndroid() = default;
 TriggerScriptBridgeAndroid::~TriggerScriptBridgeAndroid() = default;
 
 void TriggerScriptBridgeAndroid::StartTriggerScript(
-    Client* client,
+    content::WebContents* web_contents,
     const JavaParamRef<jobject>& jdelegate,
     const GURL& initial_url,
     std::unique_ptr<TriggerContext> trigger_context,
@@ -59,7 +69,7 @@ void TriggerScriptBridgeAndroid::StartTriggerScript(
             base::Base64UrlDecodePolicy::IGNORE_PADDING, &response)) {
       LOG(ERROR) << "Failed to base64-decode trigger scripts response";
       Metrics::RecordLiteScriptFinished(
-          ukm::UkmRecorder::Get(), client->GetWebContents(),
+          ukm::UkmRecorder::Get(), web_contents,
           Metrics::LiteScriptFinishedState::LITE_SCRIPT_BASE64_DECODING_ERROR);
       return;
     }
@@ -67,7 +77,7 @@ void TriggerScriptBridgeAndroid::StartTriggerScript(
         std::make_unique<ServiceRequestSenderLocalImpl>(response);
   } else {
     service_request_sender = std::make_unique<ServiceRequestSenderImpl>(
-        client->GetWebContents()->GetBrowserContext(),
+        web_contents->GetBrowserContext(),
         /* access_token_fetcher = */ nullptr,
         std::make_unique<NativeURLLoaderFactory>(),
         ApiKeyFetcher().GetAPIKey(chrome::GetChannel()),
@@ -76,8 +86,15 @@ void TriggerScriptBridgeAndroid::StartTriggerScript(
   }
 
   ServerUrlFetcher url_fetcher{ServerUrlFetcher::GetDefaultServerUrl()};
+  if (!website_login_manager_) {
+    website_login_manager_ = std::make_unique<WebsiteLoginManagerImpl>(
+        ChromePasswordManagerClient::FromWebContents(web_contents),
+        web_contents);
+  }
   trigger_script_coordinator_ = std::make_unique<TriggerScriptCoordinator>(
-      client, WebController::CreateForWebContents(client->GetWebContents()),
+      web_contents, website_login_manager_.get(),
+      base::BindRepeating(&IsFirstTimeTriggerScriptUser),
+      WebController::CreateForWebContents(web_contents),
       std::move(service_request_sender),
       url_fetcher.GetTriggerScriptsEndpoint(),
       std::make_unique<StaticTriggerConditions>(),

chrome/browser/android/autofill_assistant/trigger_script_bridge_android.h

@@ -11,7 +11,6 @@
 
 #include "base/android/jni_android.h"
 #include "base/optional.h"
-#include "components/autofill_assistant/browser/client.h"
 #include "components/autofill_assistant/browser/metrics.h"
 #include "components/autofill_assistant/browser/service.pb.h"
 #include "components/autofill_assistant/browser/trigger_context.h"
@@ -31,7 +30,7 @@ class TriggerScriptBridgeAndroid : public TriggerScriptCoordinator::Observer {
 
   // Attempts to start a trigger script on |initial_url|. Will communicate with
   // |jdelegate| to show/hide UI as necessary.
-  void StartTriggerScript(Client* client,
+  void StartTriggerScript(content::WebContents* web_contents,
                           const base::android::JavaParamRef<jobject>& jdelegate,
                           const GURL& initial_url,
                           std::unique_ptr<TriggerContext> trigger_context,
@@ -82,6 +81,10 @@ class TriggerScriptBridgeAndroid : public TriggerScriptCoordinator::Observer {
   void OnTriggerScriptFinished(Metrics::LiteScriptFinishedState state) override;
   void OnVisibilityChanged(bool visible) override;
 
+  // The login manager for fetching login credentials.
+  // TODO(arbesser) move this to the owner of trigger_script_bridge_android.
+  std::unique_ptr<WebsiteLoginManager> website_login_manager_;
+
   // Reference to the Java counterpart to this class.
   base::android::ScopedJavaGlobalRef<jobject> java_object_;
   bool disable_header_animations_for_testing_ = false;

components/autofill_assistant/browser/client.h

@@ -86,9 +86,6 @@ class Client {
   // Whether this client has had an UI.
   virtual bool HasHadUI() const = 0;
 
-  // Returns whether the user has seen a trigger script before or not.
-  virtual bool IsFirstTimeTriggerScriptUser() const = 0;
-
  protected:
   Client() = default;
 };

components/autofill_assistant/browser/trigger_scripts/mock_static_trigger_conditions.h

@@ -15,8 +15,10 @@ class MockStaticTriggerConditions : public StaticTriggerConditions {
   MockStaticTriggerConditions();
   ~MockStaticTriggerConditions() override;
 
-  MOCK_METHOD4(Init,
-               void(Client* client,
+  MOCK_METHOD5(
+      Init,
+      void(WebsiteLoginManager* website_login_manager,
+           base::RepeatingCallback<bool(void)> is_first_time_user_callback,
            const GURL& url,
            TriggerContext* trigger_context,
            base::OnceCallback<void(void)> callback));

components/autofill_assistant/browser/trigger_scripts/static_trigger_conditions.cc

@@ -12,7 +12,9 @@ namespace autofill_assistant {
 StaticTriggerConditions::StaticTriggerConditions() = default;
 StaticTriggerConditions::~StaticTriggerConditions() = default;
 
-void StaticTriggerConditions::Init(Client* client,
+void StaticTriggerConditions::Init(
+    WebsiteLoginManager* website_login_manager,
+    base::RepeatingCallback<bool(void)> is_first_time_user_callback,
     const GURL& url,
     TriggerContext* trigger_context,
     base::OnceCallback<void(void)> callback) {
@@ -21,12 +23,12 @@ void StaticTriggerConditions::Init(Client* client,
   if (callback_) {
     return;
   }
-  is_first_time_user_ = client->IsFirstTimeTriggerScriptUser();
+  is_first_time_user_ = is_first_time_user_callback.Run();
   trigger_context_ = trigger_context;
   has_stored_login_credentials_ = false;
 
   callback_ = std::move(callback);
-  client->GetWebsiteLoginManager()->GetLoginsForUrl(
+  website_login_manager->GetLoginsForUrl(
       url, base::BindOnce(&StaticTriggerConditions::OnGetLogins,
                           weak_ptr_factory_.GetWeakPtr()));
 }

components/autofill_assistant/browser/trigger_scripts/static_trigger_conditions.h

@@ -9,7 +9,6 @@
 
 #include "base/callback_forward.h"
 #include "base/memory/weak_ptr.h"
-#include "components/autofill_assistant/browser/client.h"
 #include "components/autofill_assistant/browser/trigger_context.h"
 #include "components/autofill_assistant/browser/website_login_manager.h"
 #include "url/gurl.h"
@@ -24,10 +23,12 @@ class StaticTriggerConditions {
   StaticTriggerConditions();
   virtual ~StaticTriggerConditions();
 
-  // Initializes the field values according to |url| and the current state of
-  // |client|. Invokes |callback| when done. |client| and |trigger_context| must
-  // outlive this instance.
-  virtual void Init(Client* client,
+  // Initializes the field values using |website_login_manager| and
+  // |is_first_time_user_callback|. Invokes |callback| when done. All parameters
+  // must outlive this call.
+  virtual void Init(
+      WebsiteLoginManager* website_login_manager,
+      base::RepeatingCallback<bool(void)> is_first_time_user_callback,
       const GURL& url,
       TriggerContext* trigger_context,
       base::OnceCallback<void(void)> callback);

components/autofill_assistant/browser/trigger_scripts/static_trigger_conditions_unittest.cc

@@ -4,7 +4,6 @@
 
 #include "components/autofill_assistant/browser/trigger_scripts/static_trigger_conditions.h"
 
-#include "components/autofill_assistant/browser/mock_client.h"
 #include "components/autofill_assistant/browser/mock_website_login_manager.h"
 
 #include "base/test/gmock_callback_support.h"
@@ -29,23 +28,22 @@ class StaticTriggerConditionsTest : public testing::Test {
 
  protected:
   StaticTriggerConditions static_trigger_conditions_;
+  base::MockCallback<base::RepeatingCallback<bool(void)>>
+      mock_is_first_time_user_callback_;
   base::MockCallback<base::OnceCallback<void(void)>> mock_callback_;
-  NiceMock<MockClient> mock_client_;
   NiceMock<MockWebsiteLoginManager> mock_website_login_manager_;
 };
 
 TEST_F(StaticTriggerConditionsTest, Init) {
   TriggerContextImpl trigger_context(/* params = */ {}, /* exp = */ "1,2,4");
-  EXPECT_CALL(mock_client_, IsFirstTimeTriggerScriptUser)
-      .WillOnce(Return(true));
-  EXPECT_CALL(mock_client_, GetWebsiteLoginManager)
-      .WillOnce(Return(&mock_website_login_manager_));
+  EXPECT_CALL(mock_is_first_time_user_callback_, Run).WillOnce(Return(true));
   EXPECT_CALL(mock_website_login_manager_, OnGetLoginsForUrl(GURL(kFakeUrl), _))
       .WillOnce(RunOnceCallback<1>(std::vector<WebsiteLoginManager::Login>{
           WebsiteLoginManager::Login(GURL(kFakeUrl), "fake_username")}));
   EXPECT_CALL(mock_callback_, Run).Times(1);
-  static_trigger_conditions_.Init(&mock_client_, GURL(kFakeUrl),
-                                  &trigger_context, mock_callback_.Get());
+  static_trigger_conditions_.Init(
+      &mock_website_login_manager_, mock_is_first_time_user_callback_.Get(),
+      GURL(kFakeUrl), &trigger_context, mock_callback_.Get());
 
   EXPECT_TRUE(static_trigger_conditions_.is_first_time_user());
   EXPECT_TRUE(static_trigger_conditions_.has_stored_login_credentials());
@@ -65,16 +63,14 @@ TEST_F(StaticTriggerConditionsTest, HasResults) {
   EXPECT_FALSE(static_trigger_conditions_.has_results());
 
   TriggerContextImpl trigger_context(/* params = */ {}, /* exp = */ "1,2,4");
-  EXPECT_CALL(mock_client_, IsFirstTimeTriggerScriptUser)
-      .WillOnce(Return(true));
-  EXPECT_CALL(mock_client_, GetWebsiteLoginManager)
-      .WillOnce(Return(&mock_website_login_manager_));
+  EXPECT_CALL(mock_is_first_time_user_callback_, Run).WillOnce(Return(true));
   EXPECT_CALL(mock_website_login_manager_, OnGetLoginsForUrl(GURL(kFakeUrl), _))
       .WillOnce(RunOnceCallback<1>(std::vector<WebsiteLoginManager::Login>{
           WebsiteLoginManager::Login(GURL(kFakeUrl), "fake_username")}));
   EXPECT_CALL(mock_callback_, Run).Times(1);
-  static_trigger_conditions_.Init(&mock_client_, GURL(kFakeUrl),
-                                  &trigger_context, mock_callback_.Get());
+  static_trigger_conditions_.Init(
+      &mock_website_login_manager_, mock_is_first_time_user_callback_.Get(),
+      GURL(kFakeUrl), &trigger_context, mock_callback_.Get());
   EXPECT_TRUE(static_trigger_conditions_.has_results());
 }
 

components/autofill_assistant/browser/trigger_scripts/trigger_script_coordinator.cc

@@ -42,15 +42,18 @@ std::map<std::string, std::string> ExtractDebugScriptParameters(
 namespace autofill_assistant {
 
 TriggerScriptCoordinator::TriggerScriptCoordinator(
-    Client* client,
+    content::WebContents* web_contents,
+    WebsiteLoginManager* website_login_manager,
+    base::RepeatingCallback<bool(void)> is_first_time_user_callback,
     std::unique_ptr<WebController> web_controller,
     std::unique_ptr<ServiceRequestSender> request_sender,
     const GURL& get_trigger_scripts_server,
     std::unique_ptr<StaticTriggerConditions> static_trigger_conditions,
     std::unique_ptr<DynamicTriggerConditions> dynamic_trigger_conditions,
     ukm::UkmRecorder* ukm_recorder)
-    : content::WebContentsObserver(client->GetWebContents()),
-      client_(client),
+    : content::WebContentsObserver(web_contents),
+      website_login_manager_(website_login_manager),
+      is_first_time_user_callback_(std::move(is_first_time_user_callback)),
       request_sender_(std::move(request_sender)),
       get_trigger_scripts_server_(get_trigger_scripts_server),
       web_controller_(std::move(web_controller)),
@@ -117,7 +120,7 @@ void TriggerScriptCoordinator::OnGetTriggerScripts(
       initial_trigger_condition_evaluations_;
 
   Metrics::RecordLiteScriptShownToUser(
-      ukm_recorder_, client_->GetWebContents(),
+      ukm_recorder_, web_contents(),
       Metrics::LiteScriptShownToUser::LITE_SCRIPT_RUNNING);
   StartCheckingTriggerConditions();
 }
@@ -128,7 +131,7 @@ void TriggerScriptCoordinator::PerformTriggerScriptAction(
     case TriggerScriptProto::NOT_NOW:
       if (visible_trigger_script_ != -1) {
         Metrics::RecordLiteScriptShownToUser(
-            ukm_recorder_, client_->GetWebContents(),
+            ukm_recorder_, web_contents(),
             Metrics::LiteScriptShownToUser::LITE_SCRIPT_NOT_NOW);
         trigger_scripts_[visible_trigger_script_]
             ->waiting_for_precondition_no_longer_true(true);
@@ -169,7 +172,7 @@ void TriggerScriptCoordinator::OnBottomSheetClosedWithSwipe() {
     return;
   }
   Metrics::RecordLiteScriptShownToUser(
-      ukm_recorder_, client_->GetWebContents(),
+      ukm_recorder_, web_contents(),
       Metrics::LiteScriptShownToUser::LITE_SCRIPT_SWIPE_DISMISSED);
   PerformTriggerScriptAction(trigger_scripts_[visible_trigger_script_]
                                  ->AsProto()
@@ -180,8 +183,8 @@ bool TriggerScriptCoordinator::OnBackButtonPressed() {
   if (visible_trigger_script_ == -1) {
     return false;
   }
-  if (client_->GetWebContents()->GetController().CanGoBack()) {
-    client_->GetWebContents()->GetController().GoBack();
+  if (web_contents()->GetController().CanGoBack()) {
+    web_contents()->GetController().GoBack();
   }
   // We need to handle this event, because by default the bottom sheet will
   // close when the back button is pressed.
@@ -295,7 +298,7 @@ void TriggerScriptCoordinator::OnEffectiveVisibilityChanged() {
 void TriggerScriptCoordinator::WebContentsDestroyed() {
   if (!finished_state_recorded_) {
     Metrics::RecordLiteScriptFinished(
-        ukm_recorder_, client_->GetWebContents(),
+        ukm_recorder_, web_contents(),
         visible_trigger_script_ == -1
             ? Metrics::LiteScriptFinishedState::
                   LITE_SCRIPT_WEB_CONTENTS_DESTROYED_WHILE_INVISIBLE
@@ -313,7 +316,8 @@ void TriggerScriptCoordinator::StartCheckingTriggerConditions() {
         trigger_script->AsProto());
   }
   static_trigger_conditions_->Init(
-      client_, deeplink_url_, trigger_context_.get(),
+      website_login_manager_, is_first_time_user_callback_, deeplink_url_,
+      trigger_context_.get(),
       base::BindOnce(&TriggerScriptCoordinator::CheckDynamicTriggerConditions,
                      weak_ptr_factory_.GetWeakPtr()));
 }
@@ -337,7 +341,7 @@ void TriggerScriptCoordinator::ShowTriggerScript(int index) {
   }
 
   Metrics::RecordLiteScriptShownToUser(
-      ukm_recorder_, client_->GetWebContents(),
+      ukm_recorder_, web_contents(),
       Metrics::LiteScriptShownToUser::LITE_SCRIPT_SHOWN_TO_USER);
   visible_trigger_script_ = index;
   auto proto = trigger_scripts_[index]->AsProto().user_interface();
@@ -384,7 +388,7 @@ void TriggerScriptCoordinator::OnDynamicTriggerConditionsEvaluated(
   if (visible_trigger_script_ != -1 &&
       !evaluated_trigger_conditions[visible_trigger_script_]) {
     Metrics::RecordLiteScriptShownToUser(
-        ukm_recorder_, client_->GetWebContents(),
+        ukm_recorder_, web_contents(),
         Metrics::LiteScriptShownToUser::
             LITE_SCRIPT_HIDE_ON_TRIGGER_CONDITION_NO_LONGER_TRUE);
     HideTriggerScript();
@@ -453,8 +457,7 @@ void TriggerScriptCoordinator::NotifyOnTriggerScriptFinished(
     Metrics::LiteScriptFinishedState state) {
   if (!finished_state_recorded_) {
     finished_state_recorded_ = true;
-    Metrics::RecordLiteScriptFinished(ukm_recorder_, client_->GetWebContents(),
-                                      state);
+    Metrics::RecordLiteScriptFinished(ukm_recorder_, web_contents(), state);
   }
 
   for (Observer& observer : observers_) {

components/autofill_assistant/browser/trigger_scripts/trigger_script_coordinator.h

@@ -49,9 +49,11 @@ class TriggerScriptCoordinator : public content::WebContentsObserver {
     virtual void OnVisibilityChanged(bool visible) = 0;
   };
 
-  // |client| and |web_contents| must outlive this instance.
+  // |web_contents| must outlive this instance.
   TriggerScriptCoordinator(
-      Client* client,
+      content::WebContents* web_contents,
+      WebsiteLoginManager* website_login_manager,
+      base::RepeatingCallback<bool(void)> is_first_time_user_callback,
       std::unique_ptr<WebController> web_controller,
       std::unique_ptr<ServiceRequestSender> request_sender,
       const GURL& get_trigger_scripts_server,
@@ -126,9 +128,12 @@ class TriggerScriptCoordinator : public content::WebContentsObserver {
 
   void NotifyOnTriggerScriptFinished(Metrics::LiteScriptFinishedState state);
 
-  // Used to retrieve deps and also to request shutdown and, if applicable,
-  // start of the regular script.
-  Client* client_;
+  // Used to query login information for the current webcontents.
+  WebsiteLoginManager* website_login_manager_;
+
+  // Callback that can be used to query whether a user has seen the trigger
+  // script UI at least once or not.
+  base::RepeatingCallback<bool(void)> is_first_time_user_callback_;
 
   // The original deeplink to request trigger scripts for.
   GURL deeplink_url_;