[Autofill Assistant] Record UKM metrics for trigger scripts from native.

With this change, LiteScriptShownToUser and LiteScriptFinished UKM
metrics will now be recorded with the native coordinator. Unfortunately,
the other two metrics (LiteScriptOnboarding and LiteScriptStarted) are
still in Java for now.

Recording the UKMs in native allows for better unit tests and tighter
control of when to emit those events.

This CL also fixes a bug which surfaced during testing: navigation
events while the web_contents are invisible should be ignored.

This CL does not add any new UKMs. The existing set of UKMs was
approved in crbug.com/1110887 in the privacy section of the design doc.

Note that this is not yet hooked up to prod. The relevant CL is
currently under review, here: http://crrev/c/2524524

Bug: b/172548211
Change-Id: I93cdfc2ac52a33d63247f967063d34a353924833
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2521810
Commit-Queue: Clemens Arbesser <arbesser@google.com>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Marian Fechete <marianfe@google.com>
Cr-Commit-Position: refs/heads/master@{#825397}

components/autofill_assistant/browser/BUILD.gn

@@ -239,10 +239,13 @@ static_library("browser") {
     "//components/password_manager/core/browser/form_parsing:form_parsing",
     "//components/signin/public/identity_manager",
     "//components/strings:components_strings_grit",
+    "//components/ukm/content:content",
     "//components/version_info",
     "//content/public/browser",
     "//google_apis",
     "//net",
+    "//services/metrics/public/cpp:metrics_cpp",
+    "//services/metrics/public/cpp:ukm_builders",
     "//third_party/re2",
   ]
 }
@@ -379,6 +382,8 @@ source_set("unit_tests") {
     "//components/password_manager/core/browser:test_support",
     "//components/password_manager/core/common",
     "//components/strings:components_strings_grit",
+    "//components/ukm:test_support",
+    "//components/ukm/content:content",
     "//components/version_info",
     "//content/test:test_support",
     "//services/network:test_support",

components/autofill_assistant/browser/DEPS

@@ -6,6 +6,7 @@ include_rules = [
   "+components/password_manager/content/browser",
   "+components/password_manager/core/browser",
   "+components/password_manager/core/common",
+  "+components/ukm",
   "+components/version_info",
   "+content/public/browser",
   "+content/public/test",
@@ -15,6 +16,7 @@ include_rules = [
   "+crypto",
   "+google_apis",
   "+net",
+  "+services/metrics/public",
   "+services/network/public/cpp",
   "+services/network/public/mojom",
   "+services/network/test",

components/autofill_assistant/browser/metrics.cc

@@ -6,6 +6,8 @@
 
 #include "base/logging.h"
 #include "base/metrics/histogram_functions.h"
+#include "components/ukm/content/source_url_recorder.h"
+#include "services/metrics/public/cpp/ukm_builders.h"
 
 namespace autofill_assistant {
 
@@ -108,4 +110,24 @@ void Metrics::RecordPaymentRequestMandatoryPostalCode(bool required,
                                 mandatory_postal_code);
 }
 
+// static
+void Metrics::RecordLiteScriptFinished(ukm::UkmRecorder* ukm_recorder,
+                                       content::WebContents* web_contents,
+                                       LiteScriptFinishedState event) {
+  ukm::builders::AutofillAssistant_LiteScriptFinished(
+      ukm::GetSourceIdForWebContentsDocument(web_contents))
+      .SetLiteScriptFinished(static_cast<int64_t>(event))
+      .Record(ukm_recorder);
+}
+
+// static
+void Metrics::RecordLiteScriptShownToUser(ukm::UkmRecorder* ukm_recorder,
+                                          content::WebContents* web_contents,
+                                          LiteScriptShownToUser event) {
+  ukm::builders::AutofillAssistant_LiteScriptShownToUser(
+      ukm::GetSourceIdForWebContentsDocument(web_contents))
+      .SetLiteScriptShownToUser(static_cast<int64_t>(event))
+      .Record(ukm_recorder);
+}
+
 }  // namespace autofill_assistant

components/autofill_assistant/browser/metrics.h

@@ -6,10 +6,12 @@
 #define COMPONENTS_AUTOFILL_ASSISTANT_BROWSER_METRICS_H_
 
 #include <ostream>
+#include "content/public/browser/web_contents.h"
+#include "services/metrics/public/cpp/ukm_recorder.h"
 
 namespace autofill_assistant {
 
-// A class to generate Autofill Assistant related histograms.
+// A class to generate Autofill Assistant metrics.
 class Metrics {
  public:
   // The different ways that autofill assistant can stop.
@@ -155,6 +157,7 @@ class Metrics {
   // tools/metrics/ukm/ukm.xml as necessary.
   enum class LiteScriptShownToUser {
     // The number of times a lite script was successfully fetched and started.
+    // Can happen multiple times per run (in case of tab switch).
     LITE_SCRIPT_RUNNING = 0,
     // The number of times a lite script was shown to the user. Can happen
     // multiple times per run.
@@ -311,6 +314,12 @@ class Metrics {
   static void RecordPaymentRequestMandatoryPostalCode(bool required,
                                                       bool initially_right,
                                                       bool success);
+  static void RecordLiteScriptFinished(ukm::UkmRecorder* ukm_recorder,
+                                       content::WebContents* web_contents,
+                                       LiteScriptFinishedState event);
+  static void RecordLiteScriptShownToUser(ukm::UkmRecorder* ukm_recorder,
+                                          content::WebContents* web_contents,
+                                          LiteScriptShownToUser event);
 
   // Intended for debugging: writes string representation of |reason| to |out|.
   friend std::ostream& operator<<(std::ostream& out,

components/autofill_assistant/browser/trigger_scripts/trigger_script_coordinator.cc

@@ -55,14 +55,16 @@ TriggerScriptCoordinator::TriggerScriptCoordinator(
     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)
+    std::unique_ptr<DynamicTriggerConditions> dynamic_trigger_conditions,
+    ukm::UkmRecorder* ukm_recorder)
     : content::WebContentsObserver(client->GetWebContents()),
       client_(client),
       request_sender_(std::move(request_sender)),
       get_trigger_scripts_server_(get_trigger_scripts_server),
       web_controller_(std::move(web_controller)),
       static_trigger_conditions_(std::move(static_trigger_conditions)),
-      dynamic_trigger_conditions_(std::move(dynamic_trigger_conditions)) {}
+      dynamic_trigger_conditions_(std::move(dynamic_trigger_conditions)),
+      ukm_recorder_(ukm_recorder) {}
 
 TriggerScriptCoordinator::~TriggerScriptCoordinator() = default;
 
@@ -108,6 +110,9 @@ void TriggerScriptCoordinator::OnGetTriggerScripts(
     return;
   }
 
+  Metrics::RecordLiteScriptShownToUser(
+      ukm_recorder_, client_->GetWebContents(),
+      Metrics::LiteScriptShownToUser::LITE_SCRIPT_RUNNING);
   StartCheckingTriggerConditions();
 }
 
@@ -116,6 +121,9 @@ void TriggerScriptCoordinator::PerformTriggerScriptAction(
   switch (action) {
     case TriggerScriptProto::NOT_NOW:
       if (visible_trigger_script_ != -1) {
+        Metrics::RecordLiteScriptShownToUser(
+            ukm_recorder_, client_->GetWebContents(),
+            Metrics::LiteScriptShownToUser::LITE_SCRIPT_NOT_NOW);
         trigger_scripts_[visible_trigger_script_]
             ->waiting_for_precondition_no_longer_true(true);
         HideTriggerScript();
@@ -165,10 +173,11 @@ void TriggerScriptCoordinator::RemoveObserver(const Observer* observer) {
 void TriggerScriptCoordinator::DidFinishNavigation(
     content::NavigationHandle* navigation_handle) {
   // Ignore navigation events if any of the following is true:
+  // - not currently checking for preconditions (i.e., not yet started).
   // - not in the main frame.
   // - document does not change (e.g., same page history navigation).
   // - WebContents stays at the existing URL (e.g., downloads).
-  if (!navigation_handle->IsInMainFrame() ||
+  if (!is_checking_trigger_conditions_ || !navigation_handle->IsInMainFrame() ||
       navigation_handle->IsSameDocument() ||
       !navigation_handle->HasCommitted()) {
     return;
@@ -215,6 +224,19 @@ void TriggerScriptCoordinator::OnVisibilityChanged(
   }
 }
 
+void TriggerScriptCoordinator::WebContentsDestroyed() {
+  if (!finished_state_recorded_) {
+    Metrics::RecordLiteScriptFinished(
+        ukm_recorder_, client_->GetWebContents(),
+        visible_trigger_script_ == -1
+            ? Metrics::LiteScriptFinishedState::
+                  LITE_SCRIPT_WEB_CONTENTS_DESTROYED_WHILE_INVISIBLE
+            : Metrics::LiteScriptFinishedState::
+                  LITE_SCRIPT_WEB_CONTENTS_DESTROYED_WHILE_VISIBLE);
+    finished_state_recorded_ = true;
+  }
+}
+
 void TriggerScriptCoordinator::StartCheckingTriggerConditions() {
   is_checking_trigger_conditions_ = true;
   static_trigger_conditions_->Init(
@@ -240,6 +262,9 @@ void TriggerScriptCoordinator::ShowTriggerScript(int index) {
     return;
   }
 
+  Metrics::RecordLiteScriptShownToUser(
+      ukm_recorder_, client_->GetWebContents(),
+      Metrics::LiteScriptShownToUser::LITE_SCRIPT_SHOWN_TO_USER);
   visible_trigger_script_ = index;
   static_trigger_conditions_->set_is_first_time_user(false);
   auto proto = trigger_scripts_[index]->AsProto().user_interface();
@@ -274,6 +299,10 @@ void TriggerScriptCoordinator::OnDynamicTriggerConditionsEvaluated() {
   // Trigger condition for the currently shown trigger script is no longer true.
   if (visible_trigger_script_ != -1 &&
       !evaluated_trigger_conditions[visible_trigger_script_]) {
+    Metrics::RecordLiteScriptShownToUser(
+        ukm_recorder_, client_->GetWebContents(),
+        Metrics::LiteScriptShownToUser::
+            LITE_SCRIPT_HIDE_ON_TRIGGER_CONDITION_NO_LONGER_TRUE);
     HideTriggerScript();
     // Do not return here: a different trigger script may have become eligible
     // at the same time.
@@ -321,6 +350,12 @@ void TriggerScriptCoordinator::OnDynamicTriggerConditionsEvaluated() {
 
 void TriggerScriptCoordinator::NotifyOnTriggerScriptFinished(
     Metrics::LiteScriptFinishedState state) {
+  if (!finished_state_recorded_) {
+    finished_state_recorded_ = true;
+    Metrics::RecordLiteScriptFinished(ukm_recorder_, client_->GetWebContents(),
+                                      state);
+  }
+
   for (Observer& observer : observers_) {
     observer.OnTriggerScriptFinished(state);
   }

components/autofill_assistant/browser/trigger_scripts/trigger_script_coordinator.h

@@ -25,6 +25,7 @@
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_observer.h"
+#include "services/metrics/public/cpp/ukm_recorder.h"
 #include "url/gurl.h"
 
 namespace autofill_assistant {
@@ -54,7 +55,8 @@ class TriggerScriptCoordinator : public content::WebContentsObserver {
       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);
+      std::unique_ptr<DynamicTriggerConditions> dynamic_trigger_conditions,
+      ukm::UkmRecorder* ukm_recorder);
   ~TriggerScriptCoordinator() override;
   TriggerScriptCoordinator(const TriggerScriptCoordinator&) = delete;
   TriggerScriptCoordinator& operator=(const TriggerScriptCoordinator&) = delete;
@@ -80,6 +82,7 @@ class TriggerScriptCoordinator : public content::WebContentsObserver {
   void DidFinishNavigation(
       content::NavigationHandle* navigation_handle) override;
   void OnVisibilityChanged(content::Visibility visibility) override;
+  void WebContentsDestroyed() override;
 
   void StartCheckingTriggerConditions();
   void StopCheckingTriggerConditions();
@@ -143,6 +146,12 @@ class TriggerScriptCoordinator : public content::WebContentsObserver {
   base::TimeDelta periodic_element_check_interval_ =
       base::TimeDelta::FromSeconds(1);
 
+  // The UKM recorder used for metrics.
+  ukm::UkmRecorder* const ukm_recorder_;
+
+  // Flag to ensure that we only get one LiteScriptFinished event per run.
+  bool finished_state_recorded_ = false;
+
   base::WeakPtrFactory<TriggerScriptCoordinator> weak_ptr_factory_{this};
 };
 

components/autofill_assistant/browser/trigger_scripts/trigger_script_coordinator_unittest.cc

@@ -13,6 +13,8 @@
 #include "components/autofill_assistant/browser/trigger_scripts/mock_dynamic_trigger_conditions.h"
 #include "components/autofill_assistant/browser/trigger_scripts/mock_static_trigger_conditions.h"
 #include "components/autofill_assistant/browser/web/mock_web_controller.h"
+#include "components/ukm/content/source_url_recorder.h"
+#include "components/ukm/test_ukm_recorder.h"
 #include "components/version_info/version_info.h"
 #include "content/public/test/navigation_simulator.h"
 #include "content/public/test/test_renderer_host.h"
@@ -53,6 +55,7 @@ class TriggerScriptCoordinatorTest : public content::RenderViewHostTestHarness {
 
   void SetUp() override {
     RenderViewHostTestHarness::SetUp();
+    ukm::InitializeSourceUrlRecorderForWebContents(web_contents());
 
     ON_CALL(mock_client_, GetWebContents).WillByDefault(Return(web_contents()));
 
@@ -74,7 +77,7 @@ class TriggerScriptCoordinatorTest : public content::RenderViewHostTestHarness {
         &mock_client_, std::move(mock_web_controller),
         std::move(mock_request_sender), GURL(kFakeServerUrl),
         std::move(mock_static_trigger_conditions),
-        std::move(mock_dynamic_trigger_conditions));
+        std::move(mock_dynamic_trigger_conditions), &ukm_recorder_);
     coordinator_->AddObserver(&mock_observer_);
 
     SimulateNavigateToUrl(GURL(kFakeDeepLink));
@@ -82,7 +85,7 @@ class TriggerScriptCoordinatorTest : public content::RenderViewHostTestHarness {
 
   void TearDown() override { coordinator_->RemoveObserver(&mock_observer_); }
 
-  void CallOnVisibilityChangedForTest(content::Visibility visibility) {
+  void SimulateWebContentsVisibilityChanged(content::Visibility visibility) {
     coordinator_->OnVisibilityChanged(visibility);
   }
 
@@ -93,7 +96,18 @@ class TriggerScriptCoordinatorTest : public content::RenderViewHostTestHarness {
     content::WebContentsTester::For(web_contents())->TestSetIsLoading(false);
   }
 
+  void AssertRecordedFinishedState(Metrics::LiteScriptFinishedState state) {
+    auto entries =
+        ukm_recorder_.GetEntriesByName("AutofillAssistant.LiteScriptFinished");
+    ASSERT_THAT(entries.size(), Eq(1u));
+    ukm_recorder_.ExpectEntrySourceHasUrl(
+        entries[0], web_contents()->GetLastCommittedURL());
+    EXPECT_EQ(*ukm_recorder_.GetEntryMetric(entries[0], "LiteScriptFinished"),
+              static_cast<int64_t>(state));
+  }
+
  protected:
+  ukm::TestAutoSetUkmRecorder ukm_recorder_;
   NiceMock<MockServiceRequestSender>* mock_request_sender_;
   NiceMock<MockWebController>* mock_web_controller_;
   NiceMock<MockClient> mock_client_;
@@ -124,6 +138,8 @@ TEST_F(TriggerScriptCoordinatorTest, StartFailsForDifferentDomain) {
                                           LITE_SCRIPT_PROMPT_FAILED_NAVIGATE));
   coordinator_->Start(GURL(kFakeDeepLink),
                       std::make_unique<TriggerContextImpl>());
+  AssertRecordedFinishedState(
+      Metrics::LiteScriptFinishedState::LITE_SCRIPT_PROMPT_FAILED_NAVIGATE);
 }
 
 TEST_F(TriggerScriptCoordinatorTest, StartSendsOnlyApprovedFields) {
@@ -164,7 +180,7 @@ TEST_F(TriggerScriptCoordinatorTest, StartSendsOnlyApprovedFields) {
                           /* exp = */ "1,2,4"));
 }
 
-TEST_F(TriggerScriptCoordinatorTest, StopOnNetworkError) {
+TEST_F(TriggerScriptCoordinatorTest, StopOnBackendRequestFailed) {
   EXPECT_CALL(*mock_request_sender_, OnSendRequest(GURL(kFakeServerUrl), _, _))
       .WillOnce(RunOnceCallback<2>(net::HTTP_FORBIDDEN, ""));
   EXPECT_CALL(
@@ -173,6 +189,8 @@ TEST_F(TriggerScriptCoordinatorTest, StopOnNetworkError) {
           Metrics::LiteScriptFinishedState::LITE_SCRIPT_GET_ACTIONS_FAILED));
   coordinator_->Start(GURL(kFakeDeepLink),
                       std::make_unique<TriggerContextImpl>());
+  AssertRecordedFinishedState(
+      Metrics::LiteScriptFinishedState::LITE_SCRIPT_GET_ACTIONS_FAILED);
 }
 
 TEST_F(TriggerScriptCoordinatorTest, StopOnParsingError) {
@@ -183,6 +201,8 @@ TEST_F(TriggerScriptCoordinatorTest, StopOnParsingError) {
                                           LITE_SCRIPT_GET_ACTIONS_PARSE_ERROR));
   coordinator_->Start(GURL(kFakeDeepLink),
                       std::make_unique<TriggerContextImpl>());
+  AssertRecordedFinishedState(
+      Metrics::LiteScriptFinishedState::LITE_SCRIPT_GET_ACTIONS_PARSE_ERROR);
 }
 
 TEST_F(TriggerScriptCoordinatorTest, StartChecksStaticAndDynamicConditions) {
@@ -276,7 +296,7 @@ TEST_F(TriggerScriptCoordinatorTest, PauseAndResumeOnTabVisibilityChange) {
   EXPECT_CALL(*mock_dynamic_trigger_conditions_,
               OnUpdate(mock_web_controller_, _))
       .Times(0);
-  CallOnVisibilityChangedForTest(content::Visibility::HIDDEN);
+  SimulateWebContentsVisibilityChanged(content::Visibility::HIDDEN);
 
   // When a hidden tab becomes visible again, the trigger scripts must be
   // fetched again.
@@ -290,7 +310,7 @@ TEST_F(TriggerScriptCoordinatorTest, PauseAndResumeOnTabVisibilityChange) {
   EXPECT_CALL(*mock_dynamic_trigger_conditions_, GetSelectorMatches)
       .WillOnce(Return(true));
   EXPECT_CALL(mock_observer_, OnTriggerScriptShown).Times(1);
-  CallOnVisibilityChangedForTest(content::Visibility::VISIBLE);
+  SimulateWebContentsVisibilityChanged(content::Visibility::VISIBLE);
 }
 
 TEST_F(TriggerScriptCoordinatorTest, PerformTriggerScriptActionNotNow) {
@@ -359,6 +379,8 @@ TEST_F(TriggerScriptCoordinatorTest, PerformTriggerScriptActionCancelSession) {
       .Times(1);
   EXPECT_CALL(mock_observer_, OnTriggerScriptHidden).Times(1);
   coordinator_->PerformTriggerScriptAction(TriggerScriptProto::CANCEL_SESSION);
+  AssertRecordedFinishedState(Metrics::LiteScriptFinishedState::
+                                  LITE_SCRIPT_PROMPT_FAILED_CANCEL_SESSION);
 }
 
 TEST_F(TriggerScriptCoordinatorTest, PerformTriggerScriptActionCancelForever) {
@@ -387,6 +409,8 @@ TEST_F(TriggerScriptCoordinatorTest, PerformTriggerScriptActionCancelForever) {
       .Times(1);
   EXPECT_CALL(mock_observer_, OnTriggerScriptHidden).Times(1);
   coordinator_->PerformTriggerScriptAction(TriggerScriptProto::CANCEL_FOREVER);
+  AssertRecordedFinishedState(Metrics::LiteScriptFinishedState::
+                                  LITE_SCRIPT_PROMPT_FAILED_CANCEL_FOREVER);
 }
 
 TEST_F(TriggerScriptCoordinatorTest, PerformTriggerScriptActionAccept) {
@@ -415,6 +439,8 @@ TEST_F(TriggerScriptCoordinatorTest, PerformTriggerScriptActionAccept) {
           Metrics::LiteScriptFinishedState::LITE_SCRIPT_PROMPT_SUCCEEDED))
       .Times(1);
   coordinator_->PerformTriggerScriptAction(TriggerScriptProto::ACCEPT);
+  AssertRecordedFinishedState(
+      Metrics::LiteScriptFinishedState::LITE_SCRIPT_PROMPT_SUCCEEDED);
 }
 
 TEST_F(TriggerScriptCoordinatorTest, CancelOnNavigateAway) {
@@ -458,6 +484,50 @@ TEST_F(TriggerScriptCoordinatorTest, CancelOnNavigateAway) {
           Metrics::LiteScriptFinishedState::LITE_SCRIPT_PROMPT_FAILED_NAVIGATE))
       .Times(1);
   SimulateNavigateToUrl(GURL("https://example.different.com/page"));
+  AssertRecordedFinishedState(
+      Metrics::LiteScriptFinishedState::LITE_SCRIPT_PROMPT_FAILED_NAVIGATE);
+}
+
+TEST_F(TriggerScriptCoordinatorTest, IgnoreNavigationEventsWhileNotStarted) {
+  GetTriggerScriptsResponseProto response;
+  *response.add_trigger_scripts()
+       ->mutable_trigger_condition()
+       ->mutable_selector() = ToSelectorProto("#selector");
+  std::string serialized_response;
+  response.SerializeToString(&serialized_response);
+
+  EXPECT_CALL(*mock_request_sender_, OnSendRequest(GURL(kFakeServerUrl), _, _))
+      .WillOnce(RunOnceCallback<2>(net::HTTP_OK, serialized_response));
+  EXPECT_CALL(*mock_static_trigger_conditions_, Init)
+      .WillOnce(RunOnceCallback<3>());
+  EXPECT_CALL(*mock_dynamic_trigger_conditions_,
+              OnUpdate(mock_web_controller_, _))
+      .WillOnce(RunOnceCallback<1>());
+  EXPECT_CALL(*mock_dynamic_trigger_conditions_, GetSelectorMatches)
+      .WillOnce(Return(true));
+  EXPECT_CALL(mock_observer_, OnTriggerScriptShown).Times(1);
+  coordinator_->Start(GURL(kFakeDeepLink),
+                      std::make_unique<TriggerContextImpl>());
+
+  // When a tab becomes invisible, navigation events are disregarded.
+  EXPECT_CALL(mock_observer_, OnTriggerScriptHidden).Times(1);
+  EXPECT_CALL(mock_observer_, OnTriggerScriptFinished).Times(0);
+  EXPECT_CALL(*mock_dynamic_trigger_conditions_,
+              OnUpdate(mock_web_controller_, _))
+      .Times(0);
+  SimulateWebContentsVisibilityChanged(content::Visibility::HIDDEN);
+  SimulateNavigateToUrl(GURL("https://example.different.com"));
+
+  // However, when the tab becomes visible again, resuming the trigger script
+  // will fail if the last committed URL is still on a non-supported domain.
+  EXPECT_CALL(
+      mock_observer_,
+      OnTriggerScriptFinished(
+          Metrics::LiteScriptFinishedState::LITE_SCRIPT_PROMPT_FAILED_NAVIGATE))
+      .Times(1);
+  SimulateWebContentsVisibilityChanged(content::Visibility::VISIBLE);
+  AssertRecordedFinishedState(
+      Metrics::LiteScriptFinishedState::LITE_SCRIPT_PROMPT_FAILED_NAVIGATE);
 }
 
 }  // namespace autofill_assistant