[Autofill Assistant] Immediately mark first-time lite script users.

This fixes an issue where first-time users would not be flagged as such
if they simply closed the tab. Now, this is marked as soon as the UI is
shown.

Bug: b/168281605
Change-Id: Ifcb32d69536ed6f7c8f6acbf68e67287d62c0404
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2414170
Commit-Queue: Clemens Arbesser <arbesser@google.com>
Reviewed-by: Sandro Maggi <sandromaggi@google.com>
Cr-Commit-Position: refs/heads/master@{#807861}

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

@@ -44,11 +44,22 @@ class AutofillAssistantLiteScriptCoordinator {
                 AutofillAssistantPreferencesUtil.isAutofillAssistantFirstTimeLiteScriptUser()
                 ? firstTimeUserScriptPath
                 : returningUserScriptPath;
-        AutofillAssistantLiteService liteService =
-                new AutofillAssistantLiteService(mWebContents, usedScriptPath,
-                        finishedState
-                        -> handleLiteScriptResult(finishedState, onFinishedCallback,
-                                firstTimeUserScriptPath, returningUserScriptPath));
+        AutofillAssistantLiteService liteService = new AutofillAssistantLiteService(
+                mWebContents, usedScriptPath, new AutofillAssistantLiteService.Delegate() {
+                    @Override
+                    public void onFinished(int state) {
+                        handleLiteScriptResult(state, onFinishedCallback, firstTimeUserScriptPath,
+                                returningUserScriptPath);
+                    }
+
+                    @Override
+                    public void onUiShown() {
+                        // The prompt was displayed on screen, hence we mark them as returning user
+                        // from now on.
+                        AutofillAssistantPreferencesUtil
+                                .setAutofillAssistantReturningLiteScriptUser();
+                    }
+                });
         AutofillAssistantServiceInjector.setServiceToInject(liteService);
         Map<String, String> parameters = new HashMap<>();
         parameters.put(PARAMETER_TRIGGER_SCRIPT_USED, usedScriptPath);
@@ -73,9 +84,6 @@ class AutofillAssistantLiteScriptCoordinator {
             case LiteScriptFinishedState.LITE_SCRIPT_PROMPT_FAILED_CONDITION_NO_LONGER_TRUE:
             case LiteScriptFinishedState.LITE_SCRIPT_PROMPT_FAILED_OTHER:
             case LiteScriptFinishedState.LITE_SCRIPT_PROMPT_SUCCEEDED:
-                // The prompt was displayed on screen, hence we mark them as returning user from now
-                // on.
-                AutofillAssistantPreferencesUtil.setAutofillAssistantReturningLiteScriptUser();
                 break;
         }
 

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

@@ -4,7 +4,6 @@
 
 package org.chromium.chrome.browser.autofill_assistant;
 
-import org.chromium.base.Callback;
 import org.chromium.base.annotations.CalledByNative;
 import org.chromium.base.annotations.JNINamespace;
 import org.chromium.base.annotations.NativeMethods;
@@ -17,16 +16,21 @@ import org.chromium.content_public.browser.WebContents;
 @JNINamespace("autofill_assistant")
 public class AutofillAssistantLiteService
         implements AutofillAssistantServiceInjector.NativeServiceProvider {
+    interface Delegate {
+        /** The lite script has finished with {@code state}. */
+        void onFinished(@LiteScriptFinishedState int state);
+        /** The UI was shown for the first time to the user. */
+        void onUiShown();
+    }
     private final WebContents mWebContents;
     private final String mTriggerScriptPath;
-    // Returns a state corresponding to {code LiteScriptFinishedState}.
-    private Callback<Integer> mNotifyFinishedCallback;
+    private Delegate mDelegate;
 
-    AutofillAssistantLiteService(WebContents webContents, String triggerScriptPath,
-            Callback<Integer> notifyFinishedCallback) {
+    AutofillAssistantLiteService(
+            WebContents webContents, String triggerScriptPath, Delegate delegate) {
         mWebContents = webContents;
         mTriggerScriptPath = triggerScriptPath;
-        mNotifyFinishedCallback = notifyFinishedCallback;
+        mDelegate = delegate;
     }
 
     @Override
@@ -39,10 +43,17 @@ public class AutofillAssistantLiteService
 
     @CalledByNative
     private void onFinished(@LiteScriptFinishedState int state) {
-        if (mNotifyFinishedCallback != null) {
-            mNotifyFinishedCallback.onResult(state);
+        if (mDelegate != null) {
+            mDelegate.onFinished(state);
             // Ignore subsequent notifications.
-            mNotifyFinishedCallback = null;
+            mDelegate = null;
+        }
+    }
+
+    @CalledByNative
+    private void onUiShown() {
+        if (mDelegate != null) {
+            mDelegate.onUiShown();
         }
     }
 

chrome/browser/android/autofill_assistant/lite_service_bridge.cc

@@ -14,13 +14,26 @@
 
 namespace autofill_assistant {
 
-void OnFinished(base::android::ScopedJavaGlobalRef<jobject> java_lite_service,
-                Metrics::LiteScriptFinishedState state) {
-  VLOG(1) << "Lite service finished with state " << static_cast<int>(state);
-  JNIEnv* env = base::android::AttachCurrentThread();
-  Java_AutofillAssistantLiteService_onFinished(env, java_lite_service,
-                                               static_cast<int>(state));
-}
+class LiteServiceJavaDelegate : public LiteService::Delegate {
+ public:
+  LiteServiceJavaDelegate(
+      const base::android::JavaParamRef<jobject>& java_lite_service)
+      : java_lite_service_(java_lite_service) {}
+  void OnUiShown() const override {
+    JNIEnv* env = base::android::AttachCurrentThread();
+    Java_AutofillAssistantLiteService_onUiShown(env, java_lite_service_);
+  }
+
+  void OnFinished(Metrics::LiteScriptFinishedState state) const override {
+    VLOG(1) << "Lite service finished with state " << static_cast<int>(state);
+    JNIEnv* env = base::android::AttachCurrentThread();
+    Java_AutofillAssistantLiteService_onFinished(env, java_lite_service_,
+                                                 static_cast<int>(state));
+  }
+
+ private:
+  base::android::ScopedJavaGlobalRef<jobject> java_lite_service_;
+};
 
 // static
 jlong JNI_AutofillAssistantLiteService_CreateLiteService(
@@ -44,8 +57,7 @@ jlong JNI_AutofillAssistantLiteService_CreateLiteService(
                                     /* access_token_fetcher = */ nullptr,
                                     /* auth_enabled = */ false),
       base::android::ConvertJavaStringToUTF8(env, jtrigger_script_path),
-      base::BindOnce(&OnFinished, base::android::ScopedJavaGlobalRef<jobject>(
-                                      java_lite_service))));
+      std::make_unique<LiteServiceJavaDelegate>(java_lite_service)));
 }
 
 }  // namespace autofill_assistant

components/autofill_assistant/browser/lite_service.cc

@@ -15,19 +15,18 @@
 
 namespace autofill_assistant {
 
-LiteService::LiteService(
-    std::unique_ptr<Service> service_impl,
-    const std::string& trigger_script_path,
-    base::OnceCallback<void(Metrics::LiteScriptFinishedState)>
-        notify_finished_callback)
+LiteService::LiteService(std::unique_ptr<Service> service_impl,
+                         const std::string& trigger_script_path,
+                         std::unique_ptr<Delegate> delegate)
     : service_impl_(std::move(service_impl)),
       trigger_script_path_(trigger_script_path),
-      notify_finished_callback_(std::move(notify_finished_callback)) {}
+      delegate_(std::move(delegate)) {}
 
 LiteService::~LiteService() {
-  if (notify_finished_callback_) {
-    std::move(notify_finished_callback_)
-        .Run(Metrics::LiteScriptFinishedState::LITE_SCRIPT_SERVICE_DELETED);
+  if (delegate_) {
+    delegate_->OnFinished(
+        Metrics::LiteScriptFinishedState::LITE_SCRIPT_SERVICE_DELETED);
+    delegate_.reset();
   }
 }
 
@@ -38,12 +37,6 @@ bool LiteService::IsLiteService() const {
 void LiteService::GetScriptsForUrl(const GURL& url,
                                    const TriggerContext& trigger_context,
                                    ResponseCallback callback) {
-  if (!notify_finished_callback_) {
-    NOTREACHED();
-    std::move(callback).Run(true, std::string());
-    return;
-  }
-
   SupportsScriptResponseProto response;
   auto* lite_script = response.add_scripts();
   lite_script->set_path(trigger_script_path_);
@@ -62,11 +55,6 @@ void LiteService::GetActions(const std::string& script_path,
                              const std::string& do_not_use_global_payload,
                              const std::string& do_not_use_script_payload,
                              ResponseCallback callback) {
-  if (!notify_finished_callback_) {
-    NOTREACHED();
-    std::move(callback).Run(true, std::string());
-    return;
-  }
   // Should never happen, but let's guard for this just in case.
   if (script_path != trigger_script_path_) {
     StopWithoutErrorMessage(
@@ -88,11 +76,6 @@ void LiteService::GetActions(const std::string& script_path,
 void LiteService::OnGetActions(ResponseCallback callback,
                                bool result,
                                const std::string& response) {
-  if (!notify_finished_callback_) {
-    NOTREACHED();
-    std::move(callback).Run(true, std::string());
-    return;
-  }
   if (!result) {
     StopWithoutErrorMessage(
         std::move(callback),
@@ -153,7 +136,7 @@ void LiteService::GetNextActions(
     const std::string& previous_script_payload,
     const std::vector<ProcessedActionProto>& processed_actions,
     ResponseCallback callback) {
-  if (!notify_finished_callback_) {
+  if (!delegate_) {
     // The lite script has already terminated. We need to run |callback| with
     // |success|=true and an empty response to ensure a graceful stop of the
     // script (i.e., without error message).
@@ -191,6 +174,7 @@ void LiteService::GetNextActions(
         trigger_script_second_part_->SerializeToString(&serialized_second_part);
         trigger_script_second_part_.reset();
         std::move(callback).Run(true, serialized_second_part);
+        delegate_->OnUiShown();
         return;
     }
   } else {
@@ -233,9 +217,12 @@ void LiteService::GetNextActions(
 void LiteService::StopWithoutErrorMessage(
     ResponseCallback callback,
     Metrics::LiteScriptFinishedState state) {
-  // Run callback BEFORE terminating the controller. See comment in header
-  // for |notify_finished_callback_|.
-  std::move(notify_finished_callback_).Run(state);
+  // Notify delegate BEFORE terminating the controller. See comment in header
+  // for |OnFinished|.
+  if (delegate_) {
+    delegate_->OnFinished(state);
+    delegate_.reset();
+  }
 
   // Stop script.
   ActionsResponseProto response;

components/autofill_assistant/browser/lite_service.h

@@ -24,11 +24,32 @@ namespace autofill_assistant {
 // other information.
 class LiteService : public Service {
  public:
-  explicit LiteService(
-      std::unique_ptr<Service> service_impl,
-      const std::string& trigger_script_path,
-      base::OnceCallback<void(Metrics::LiteScriptFinishedState)>
-          notify_finished_callback);
+  class Delegate {
+   public:
+    virtual ~Delegate() {}
+    // Called the first time the UI is shown to the user.
+    virtual void OnUiShown() const = 0;
+
+    // The lite script has finished with |state|.
+    //
+    // Note that this notification will be run BEFORE the controller shuts down.
+    // This is necessary to transition between old and new bottom sheet
+    // contents.
+    // Case 1: onboarding will be shown. The controller will terminate
+    // gracefully with an explicit stop action executed after the notification
+    // was run.
+    // Case 2: onboarding will not be shown. The controller must terminate
+    // immediately and make way for the main script controller. Since the
+    // notification is run while the old controller is still around, the caller
+    // can hot-swap controllers and smoothly transition bottom sheet contents.
+    // Case 3: the lite script failed (|state| != LITE_SCRIPT_SUCCESS). The
+    // controller will terminate gracefully with an explicit stop action.
+    virtual void OnFinished(Metrics::LiteScriptFinishedState state) const = 0;
+  };
+
+  explicit LiteService(std::unique_ptr<Service> service_impl,
+                       const std::string& trigger_script_path,
+                       std::unique_ptr<Delegate> delegate);
   // If the destructor is called before |GetNextActions|, the script was
   // terminated before finishing (user cancelled, closed the tab, etc.).
   ~LiteService() override;
@@ -80,20 +101,9 @@ class LiteService : public Service {
   std::unique_ptr<Service> service_impl_;
   // The script path to fetch actions from.
   std::string trigger_script_path_;
-  // Notifies the java bridge of the finished state.
-  //
-  // Note that this callback will be run BEFORE the controller shuts down. This
-  // is necessary to transition between old and new bottom sheet contents.
-  // Case 1: onboarding will be shown. The controller will terminate gracefully
-  // with an explicit stop action executed after the callback was run.
-  // Case 2: onboarding will not be shown. The controller must terminate
-  // immediately and make way for the main script controller. Since the callback
-  // is run while the old controller is still around, the caller can hot-swap
-  // controllers and smoothly transition bottom sheet contents.
-  // Case 3: the lite script failed (|state| != LITE_SCRIPT_SUCCESS). The
-  // controller will terminate gracefully with an explicit stop action.
-  base::OnceCallback<void(Metrics::LiteScriptFinishedState)>
-      notify_finished_callback_;
+  // The delegate to notify of changes.
+  std::unique_ptr<Delegate> delegate_;
+
   // The second part of the trigger script, i.e., the actions that should be run
   // after a successful prompt(browse) action in the first part of the script.
   std::unique_ptr<ActionsResponseProto> trigger_script_second_part_;

components/autofill_assistant/browser/lite_service_unittest.cc

@@ -32,14 +32,24 @@ using ::testing::_;
 using ::testing::AtMost;
 using ::testing::Eq;
 
+class MockDelegate : public LiteService::Delegate {
+ public:
+  ~MockDelegate() {}
+  MOCK_CONST_METHOD0(OnUiShown, void());
+  MOCK_CONST_METHOD1(OnFinished, void(Metrics::LiteScriptFinishedState state));
+};
+
 class LiteServiceTest : public testing::Test {
  protected:
   LiteServiceTest() {
     auto service_impl = std::make_unique<MockService>();
     mock_native_service_ = service_impl.get();
-    lite_service_ =
-        std::make_unique<LiteService>(std::move(service_impl), kFakeScriptPath,
-                                      mock_notification_callback_.Get());
+
+    auto delegate = std::make_unique<MockDelegate>();
+    mock_delegate_ = delegate.get();
+
+    lite_service_ = std::make_unique<LiteService>(
+        std::move(service_impl), kFakeScriptPath, std::move(delegate));
     EXPECT_CALL(*mock_native_service_, OnGetScriptsForUrl).Times(0);
     EXPECT_CALL(*mock_native_service_, OnGetNextActions).Times(0);
 
@@ -72,27 +82,27 @@ class LiteServiceTest : public testing::Test {
     std::string serialized_stop;
     stop.SerializeToString(&serialized_stop);
     EXPECT_CALL(mock_response_callback_, Run(true, serialized_stop)).Times(1);
-    EXPECT_CALL(mock_notification_callback_, Run(state));
+    EXPECT_CALL(*mock_delegate_, OnFinished(state));
   }
 
-  base::MockCallback<base::OnceCallback<void(Metrics::LiteScriptFinishedState)>>
-      mock_notification_callback_;
   base::MockCallback<base::OnceCallback<void(bool, const std::string&)>>
       mock_response_callback_;
   MockService* mock_native_service_ = nullptr;
+  MockDelegate* mock_delegate_ = nullptr;
   std::unique_ptr<LiteService> lite_service_;
   ActionsResponseProto get_actions_response_;
 };
 
 TEST_F(LiteServiceTest, RunsNotificationOnDelete) {
-  base::MockCallback<base::OnceCallback<void(Metrics::LiteScriptFinishedState)>>
-      callback;
+  auto delegate = std::make_unique<MockDelegate>();
+  auto* delegate_ptr = delegate.get();
   EXPECT_CALL(
-      callback,
-      Run(Metrics::LiteScriptFinishedState::LITE_SCRIPT_SERVICE_DELETED));
+      *delegate_ptr,
+      OnFinished(
+          Metrics::LiteScriptFinishedState::LITE_SCRIPT_SERVICE_DELETED));
   {
     LiteService lite_service(std::make_unique<MockService>(), kFakeScriptPath,
-                             callback.Get());
+                             std::move(delegate));
   }
 }
 
@@ -159,6 +169,7 @@ TEST_F(LiteServiceTest, StopsOnGetActionsFailed) {
                     Service::ResponseCallback& callback) {
         std::move(callback).Run(false, std::string());
       });
+  EXPECT_CALL(*mock_delegate_, OnUiShown).Times(0);
   lite_service_->GetActions(kFakeScriptPath, GURL(kFakeUrl),
                             TriggerContextImpl(), "", "",
                             mock_response_callback_.Get());
@@ -177,6 +188,7 @@ TEST_F(LiteServiceTest, StopsOnGetActionsParsingError) {
                     Service::ResponseCallback& callback) {
         std::move(callback).Run(true, std::string("invalid proto"));
       });
+  EXPECT_CALL(*mock_delegate_, OnUiShown).Times(0);
   lite_service_->GetActions(kFakeScriptPath, GURL(kFakeUrl),
                             TriggerContextImpl(), "", "",
                             mock_response_callback_.Get());
@@ -188,6 +200,7 @@ TEST_F(LiteServiceTest, StopsOnGetActionsContainsUnsafeActions) {
   get_actions_response_.add_actions()->mutable_prompt();
   ExpectStopWithFinishedState(
       Metrics::LiteScriptFinishedState::LITE_SCRIPT_UNSAFE_ACTIONS);
+  EXPECT_CALL(*mock_delegate_, OnUiShown).Times(0);
   lite_service_->GetActions(kFakeScriptPath, GURL(kFakeUrl),
                             TriggerContextImpl(), "", "",
                             mock_response_callback_.Get());
@@ -253,6 +266,7 @@ TEST_F(LiteServiceTest, GetActionsSplitsActionsResponseAtLastBrowse) {
         processed_actions.back().mutable_prompt_choice()->set_server_payload(
             proto.actions(3).prompt().choices(0).server_payload());
       });
+  EXPECT_CALL(*mock_delegate_, OnUiShown).Times(1);
   lite_service_->GetActions(kFakeScriptPath, GURL(kFakeUrl),
                             TriggerContextImpl(), "", "",
                             mock_response_callback_.Get());
@@ -281,6 +295,7 @@ TEST_F(LiteServiceTest, GetNextActionsFirstPartStopsOnUserNavigateAway) {
 
   ExpectStopWithFinishedState(
       Metrics::LiteScriptFinishedState::LITE_SCRIPT_BROWSE_FAILED_NAVIGATE);
+  EXPECT_CALL(*mock_delegate_, OnUiShown).Times(0);
   lite_service_->GetNextActions(TriggerContextImpl(), "", "", processed_actions,
                                 mock_response_callback_.Get());
 }
@@ -299,6 +314,7 @@ TEST_F(LiteServiceTest, GetNextActionsFirstPartSucceedsOnAutoSelectChoice) {
       "payload");
 
   EXPECT_CALL(mock_response_callback_, Run(true, ""));
+  EXPECT_CALL(*mock_delegate_, OnUiShown).Times(1);
   lite_service_->GetNextActions(TriggerContextImpl(), "", "", processed_actions,
                                 mock_response_callback_.Get());
 }