[LiteVideo] Rework UKM to record on DidFinish and WCO destruction.

This change resolves the UKM recording and ties it just to the
mainframe decision and result. It also flushes when the web contents
observer destructs. A future change will update the event on
rebuffers and consider more events for subframes.

Bug: 1109068
Change-Id: Iacd04920811d96c4eedd3290309a8bad93777b58
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2321520
Auto-Submit: Michael Crouse <mcrouse@chromium.org>
Commit-Queue: rajendrant <rajendrant@chromium.org>
Reviewed-by: rajendrant <rajendrant@chromium.org>
Reviewed-by: Tarun Bansal <tbansal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#792513}

chrome/browser/BUILD.gn

@@ -625,6 +625,8 @@ static_library("browser") {
     "lite_video/lite_video_keyed_service.h",
     "lite_video/lite_video_keyed_service_factory.cc",
     "lite_video/lite_video_keyed_service_factory.h",
+    "lite_video/lite_video_navigation_metrics.cc",
+    "lite_video/lite_video_navigation_metrics.h",
     "lite_video/lite_video_observer.cc",
     "lite_video/lite_video_observer.h",
     "lite_video/lite_video_switches.cc",

chrome/browser/lite_video/lite_video_keyed_service_browsertest.cc

@@ -192,6 +192,9 @@ IN_PROC_BROWSER_TEST_F(LiteVideoKeyedServiceBrowserTest,
   // Navigate metrics get recorded.
   ui_test_utils::NavigateToURL(browser(), GURL("chrome://testserver.com"));
 
+  // Close the tab to flush any UKM metrics.
+  browser()->tab_strip_model()->GetActiveWebContents()->Close();
+
   histogram_tester()->ExpectTotalCount("LiteVideo.Navigation.HasHint", 0);
   auto entries =
       ukm_recorder.GetEntriesByName(ukm::builders::LiteVideo::kEntryName);
@@ -218,6 +221,9 @@ IN_PROC_BROWSER_TEST_F(LiteVideoKeyedServiceBrowserTest,
   // Navigate metrics get recorded.
   ui_test_utils::NavigateToURL(browser(), navigation_url);
 
+  // Close the tab to flush the UKM metrics.
+  browser()->tab_strip_model()->GetActiveWebContents()->Close();
+
   EXPECT_GT(RetryForHistogramUntilCountReached(
                 *histogram_tester(), "LiteVideo.Navigation.HasHint", 1),
             0);
@@ -256,6 +262,9 @@ IN_PROC_BROWSER_TEST_F(LiteVideoKeyedServiceBrowserTest,
   // Navigate metrics get recorded.
   ui_test_utils::NavigateToURL(browser(), navigation_url);
 
+  // Close the tab to flush the UKM metrics.
+  browser()->tab_strip_model()->GetActiveWebContents()->Close();
+
   EXPECT_GT(RetryForHistogramUntilCountReached(
                 *histogram_tester(), "LiteVideo.HintAgent.HasHint", 1),
             0);
@@ -311,6 +320,9 @@ IN_PROC_BROWSER_TEST_F(LiteVideoKeyedServiceBrowserTest,
   NavigateParams params_blocklisted(browser(), url, ui::PAGE_TRANSITION_TYPED);
   ui_test_utils::NavigateToURL(&params_blocklisted);
 
+  // Close the tab to flush the UKM metrics.
+  browser()->tab_strip_model()->GetActiveWebContents()->Close();
+
   EXPECT_GT(RetryForHistogramUntilCountReached(
                 *histogram_tester(), "LiteVideo.Navigation.HasHint", 2),
             0);
@@ -377,6 +389,9 @@ IN_PROC_BROWSER_TEST_F(LiteVideoKeyedServiceBrowserTest,
   NavigateParams params_blocklisted(browser(), url, ui::PAGE_TRANSITION_TYPED);
   ui_test_utils::NavigateToURL(&params_blocklisted);
 
+  // Close the tab to flush the UKM metrics.
+  browser()->tab_strip_model()->GetActiveWebContents()->Close();
+
   EXPECT_GT(RetryForHistogramUntilCountReached(
                 *histogram_tester(), "LiteVideo.Navigation.HasHint", 2),
             0);
@@ -442,6 +457,9 @@ IN_PROC_BROWSER_TEST_F(LiteVideoKeyedServiceBrowserTest,
   // Navigate  again to ensure that it was not blocklisted.
   ui_test_utils::NavigateToURL(&params);
 
+  // Close the tab to flush the UKM metrics.
+  browser()->tab_strip_model()->GetActiveWebContents()->Close();
+
   EXPECT_GT(RetryForHistogramUntilCountReached(
                 *histogram_tester(), "LiteVideo.HintAgent.HasHint", 2),
             0);
@@ -625,6 +643,9 @@ IN_PROC_BROWSER_TEST_F(LiteVideoKeyedServiceBrowserTest,
   // Navigate metrics get recorded.
   ui_test_utils::NavigateToURL(browser(), https_url());
 
+  // Close the tab to flush the UKM metrics.
+  browser()->tab_strip_model()->GetActiveWebContents()->Close();
+
   EXPECT_EQ(RetryForHistogramUntilCountReached(
                 *histogram_tester(), "LiteVideo.Navigation.HasHint", 2),
             2);
@@ -639,17 +660,15 @@ IN_PROC_BROWSER_TEST_F(LiteVideoKeyedServiceBrowserTest,
 
   auto entries =
       ukm_recorder.GetEntriesByName(ukm::builders::LiteVideo::kEntryName);
-  ASSERT_EQ(2u, entries.size());
-  for (auto* entry : entries) {
-    // Both entries should be tied to the mainframe url.
-    ukm_recorder.ExpectEntrySourceHasUrl(entry, https_url());
-    ukm_recorder.ExpectEntryMetric(
-        entry, ukm::builders::LiteVideo::kThrottlingStartDecisionName,
-        static_cast<int>(lite_video::LiteVideoDecision::kNotAllowed));
-    ukm_recorder.ExpectEntryMetric(
-        entry, ukm::builders::LiteVideo::kBlocklistReasonName,
-        static_cast<int>(lite_video::LiteVideoBlocklistReason::kAllowed));
-  }
+  ASSERT_EQ(1u, entries.size());
+  auto* entry = entries[0];
+  ukm_recorder.ExpectEntrySourceHasUrl(entry, https_url());
+  ukm_recorder.ExpectEntryMetric(
+      entry, ukm::builders::LiteVideo::kThrottlingStartDecisionName,
+      static_cast<int>(lite_video::LiteVideoDecision::kNotAllowed));
+  ukm_recorder.ExpectEntryMetric(
+      entry, ukm::builders::LiteVideo::kBlocklistReasonName,
+      static_cast<int>(lite_video::LiteVideoBlocklistReason::kAllowed));
 }
 
 class LiteVideoKeyedServiceCoinflipBrowserTest
@@ -685,6 +704,9 @@ IN_PROC_BROWSER_TEST_F(LiteVideoKeyedServiceCoinflipBrowserTest,
   // Navigate metrics get recorded.
   ui_test_utils::NavigateToURL(browser(), https_url());
 
+  // Close the tab to flush the UKM metrics.
+  browser()->tab_strip_model()->GetActiveWebContents()->Close();
+
   EXPECT_EQ(RetryForHistogramUntilCountReached(
                 *histogram_tester(), "LiteVideo.Navigation.HasHint", 2),
             2);
@@ -693,12 +715,12 @@ IN_PROC_BROWSER_TEST_F(LiteVideoKeyedServiceCoinflipBrowserTest,
 
   auto entries =
       ukm_recorder.GetEntriesByName(ukm::builders::LiteVideo::kEntryName);
-  ASSERT_EQ(2u, entries.size());
-  for (auto* entry : entries) {
-    // Both entries should be tied to the mainframe url.
-    ukm_recorder.ExpectEntrySourceHasUrl(entry, https_url());
-    ukm_recorder.ExpectEntryMetric(
-        entry, ukm::builders::LiteVideo::kThrottlingStartDecisionName,
-        static_cast<int>(lite_video::LiteVideoDecision::kHoldback));
-  }
+  // Only recording the mainframe event.
+  ASSERT_EQ(1u, entries.size());
+  auto* entry = entries[0];
+  // Both entries should be tied to the mainframe url.
+  ukm_recorder.ExpectEntrySourceHasUrl(entry, https_url());
+  ukm_recorder.ExpectEntryMetric(
+      entry, ukm::builders::LiteVideo::kThrottlingStartDecisionName,
+      static_cast<int>(lite_video::LiteVideoDecision::kHoldback));
 }

chrome/browser/lite_video/lite_video_navigation_metrics.cc

+// Copyright 2020 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 "chrome/browser/lite_video/lite_video_navigation_metrics.h"
+
+namespace lite_video {
+
+LiteVideoNavigationMetrics::LiteVideoNavigationMetrics(
+    int64_t nav_id,
+    LiteVideoDecision decision,
+    LiteVideoBlocklistReason blocklist_reason)
+    : nav_id_(nav_id),
+      decision_(decision),
+      blocklist_reason_(blocklist_reason) {}
+
+LiteVideoNavigationMetrics::~LiteVideoNavigationMetrics() = default;
+
+LiteVideoBlocklistReason LiteVideoNavigationMetrics::blocklist_reason() const {
+  return blocklist_reason_;
+}
+
+}  // namespace lite_video

chrome/browser/lite_video/lite_video_navigation_metrics.h

+// Copyright 2020 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.
+
+#ifndef CHROME_BROWSER_LITE_VIDEO_LITE_VIDEO_NAVIGATION_METRICS_H_
+#define CHROME_BROWSER_LITE_VIDEO_LITE_VIDEO_NAVIGATION_METRICS_H_
+
+#include "chrome/browser/lite_video/lite_video_user_blocklist.h"
+
+namespace lite_video {
+
+// The decision if a navigation should attempt to throttle media requests.
+// This should be kept in sync with LiteVideoDecision in enums.xml.
+enum class LiteVideoDecision {
+  kUnknown,
+  // The navigation is allowed by all types of this LiteVideoUserBlocklist.
+  kAllowed,
+  // The navigation is not allowed by all types of this LiteVideoUserBlocklist.
+  kNotAllowed,
+  // The navigation is allowed by all types of this LiteVideoUserBlocklist but
+  // the optimization was heldback for counterfactual experiments.
+  kHoldback,
+
+  // Insert new values before this line.
+  kMaxValue = kHoldback,
+};
+
+class LiteVideoNavigationMetrics {
+ public:
+  LiteVideoNavigationMetrics(int64_t nav_id,
+                             LiteVideoDecision decision,
+                             LiteVideoBlocklistReason blocklist_reason);
+  ~LiteVideoNavigationMetrics();
+
+  int64_t nav_id() const { return nav_id_; }
+  LiteVideoDecision decision() const { return decision_; }
+  LiteVideoBlocklistReason blocklist_reason() const;
+
+ private:
+  int64_t nav_id_;
+  LiteVideoDecision decision_;
+  LiteVideoBlocklistReason blocklist_reason_;
+};
+
+}  // namespace lite_video
+
+#endif  // CHROME_BROWSER_LITE_VIDEO_LITE_VIDEO_NAVIGATION_METRICS_H_

chrome/browser/lite_video/lite_video_observer.cc

@@ -13,6 +13,7 @@
 #include "chrome/browser/lite_video/lite_video_keyed_service.h"
 #include "chrome/browser/lite_video/lite_video_keyed_service_factory.h"
 #include "chrome/browser/lite_video/lite_video_switches.h"
+#include "chrome/browser/lite_video/lite_video_user_blocklist.h"
 #include "chrome/browser/lite_video/lite_video_util.h"
 #include "chrome/browser/profiles/profile.h"
 #include "content/public/browser/navigation_handle.h"
@@ -43,15 +44,6 @@ lite_video::LiteVideoDecider* GetLiteVideoDeciderFromWebContents(
   return nullptr;
 }
 
-// Returns the result of a coinflip.
-bool GetCoinflipExperimentState() {
-  if (!lite_video::features::IsCoinflipExperimentEnabled())
-    return false;
-  if (lite_video::switches::ShouldForceCoinflipHoldback())
-    return true;
-  return base::RandInt(0, 1);
-}
-
 }  // namespace
 
 // static
@@ -68,26 +60,20 @@ LiteVideoObserver::LiteVideoObserver(content::WebContents* web_contents)
   lite_video_decider_ = GetLiteVideoDeciderFromWebContents(web_contents);
 }
 
-LiteVideoObserver::~LiteVideoObserver() = default;
-
-void LiteVideoObserver::DidStartNavigation(
-    content::NavigationHandle* navigation_handle) {
-  if (navigation_handle->IsInMainFrame())
-    current_mainframe_navigation_id_.reset();
+LiteVideoObserver::~LiteVideoObserver() {
+  FlushUKMMetrics();
 }
 
 void LiteVideoObserver::DidFinishNavigation(
     content::NavigationHandle* navigation_handle) {
   DCHECK(navigation_handle);
 
-  if (navigation_handle->IsInMainFrame()) {
-    ineligible_main_frame_ = !navigation_handle->HasCommitted() ||
-                             !navigation_handle->GetURL().SchemeIsHTTPOrHTTPS();
-  }
   if (!navigation_handle->HasCommitted() ||
+      navigation_handle->IsSameDocument() ||
       !navigation_handle->GetURL().SchemeIsHTTPOrHTTPS()) {
     return;
   }
+
   if (!lite_video_decider_)
     return;
 
@@ -97,23 +83,20 @@ void LiteVideoObserver::DidFinishNavigation(
       lite_video_decider_->CanApplyLiteVideo(navigation_handle,
                                              &blocklist_reason);
 
-  if (navigation_handle->IsInMainFrame()) {
-    DCHECK(!current_mainframe_navigation_id_);
-    current_mainframe_navigation_id_ = navigation_handle->GetNavigationId();
-    // The coinflip state should only be updated on a mainframe navigation.
-    is_coinflip_holdback_ = GetCoinflipExperimentState();
-  }
+  MaybeUpdateCoinflipExperimentState(navigation_handle);
 
-  // TODO(crbug/1097792): Record these metrics when the renderer associated with
-  // this navigation ends and the result from the renderer is available.
   lite_video::LiteVideoDecision decision =
       MakeLiteVideoDecision(navigation_handle, hint);
 
-  RecordUKMMetrics(decision, blocklist_reason);
+  if (navigation_handle->IsInMainFrame()) {
+    FlushUKMMetrics();
+    nav_metrics_ = lite_video::LiteVideoNavigationMetrics(
+        navigation_handle->GetNavigationId(), decision, blocklist_reason);
+  }
 
   LOCAL_HISTOGRAM_BOOLEAN("LiteVideo.Navigation.HasHint", hint ? true : false);
 
-  if (!hint)
+  if (decision == lite_video::LiteVideoDecision::kNotAllowed)
     return;
 
   content::RenderFrameHost* render_frame_host =
@@ -149,21 +132,29 @@ lite_video::LiteVideoDecision LiteVideoObserver::MakeLiteVideoDecision(
   return lite_video::LiteVideoDecision::kNotAllowed;
 }
 
-void LiteVideoObserver::RecordUKMMetrics(
-    lite_video::LiteVideoDecision decision,
-    lite_video::LiteVideoBlocklistReason blocklist_reason) {
-  // |current_mainframe_navigation_id_| may be null (e.g., if the mainframe was
-  // non-http/https).
-  DCHECK(current_mainframe_navigation_id_ || ineligible_main_frame_);
-  if (!current_mainframe_navigation_id_) {
+void LiteVideoObserver::FlushUKMMetrics() {
+  if (!nav_metrics_)
     return;
-  }
   ukm::SourceId ukm_source_id = ukm::ConvertToSourceId(
-      *current_mainframe_navigation_id_, ukm::SourceIdType::NAVIGATION_ID);
+      nav_metrics_->nav_id(), ukm::SourceIdType::NAVIGATION_ID);
   ukm::builders::LiteVideo builder(ukm_source_id);
-  builder.SetThrottlingStartDecision(static_cast<int>(decision))
-      .SetBlocklistReason(static_cast<int>(blocklist_reason))
+  builder.SetThrottlingStartDecision(static_cast<int>(nav_metrics_->decision()))
+      .SetBlocklistReason(static_cast<int>(nav_metrics_->blocklist_reason()))
       .Record(ukm::UkmRecorder::Get());
+  nav_metrics_.reset();
+}
+
+// Returns the result of a coinflip.
+void LiteVideoObserver::MaybeUpdateCoinflipExperimentState(
+    content::NavigationHandle* navigation_handle) {
+  if (!navigation_handle->IsInMainFrame())
+    return;
+  if (!lite_video::features::IsCoinflipExperimentEnabled())
+    return;
+
+  is_coinflip_holdback_ = lite_video::switches::ShouldForceCoinflipHoldback()
+                              ? true
+                              : base::RandInt(0, 1);
 }
 
 WEB_CONTENTS_USER_DATA_KEY_IMPL(LiteVideoObserver)

chrome/browser/lite_video/lite_video_observer.h

@@ -8,6 +8,7 @@
 #include "base/macros.h"
 #include "base/optional.h"
 #include "base/timer/timer.h"
+#include "chrome/browser/lite_video/lite_video_navigation_metrics.h"
 #include "chrome/browser/lite_video/lite_video_user_blocklist.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/browser/web_contents_user_data.h"
@@ -20,23 +21,6 @@ class WebContents;
 namespace lite_video {
 class LiteVideoDecider;
 class LiteVideoHint;
-
-// The decision if a navigation should attempt to throttle media requests.
-// This should be kept in sync with LiteVideoDecision in enums.xml.
-enum class LiteVideoDecision {
-  kUnknown,
-  // The navigation is allowed by all types of this LiteVideoUserBlocklist.
-  kAllowed,
-  // The navigation is not allowed by all types of this LiteVideoUserBlocklist.
-  kNotAllowed,
-  // The navigation is allowed by all types of this LiteVideoUserBlocklist but
-  // the optimization was heldback for counterfactual experiments.
-  kHoldback,
-
-  // Insert new values before this line.
-  kMaxValue = kHoldback,
-};
-
 }  // namespace lite_video
 
 class LiteVideoObserver
@@ -52,8 +36,6 @@ class LiteVideoObserver
   explicit LiteVideoObserver(content::WebContents* web_contents);
 
   // content::WebContentsObserver.
-  void DidStartNavigation(
-      content::NavigationHandle* navigation_handle) override;
   void DidFinishNavigation(
       content::NavigationHandle* navigation_handle) override;
 
@@ -64,18 +46,21 @@ class LiteVideoObserver
       base::Optional<lite_video::LiteVideoHint> hint) const;
 
   // Records the metrics for LiteVideos applied to any frames associated with
-  // the current mainframe navigation id. Called once per frame. Also, called
-  // for frames in the same document navigations.
-  void RecordUKMMetrics(lite_video::LiteVideoDecision decision,
-                        lite_video::LiteVideoBlocklistReason blocklist_reason);
+  // the current mainframe navigation id. Called once per mainframe.
+  void FlushUKMMetrics();
+
+  // Updates the coinflip state if the navigation handle is associated with
+  // the mainframe. Should only be called once per new mainframe navigation.
+  void MaybeUpdateCoinflipExperimentState(
+      content::NavigationHandle* navigation_handle);
 
   // The decider capable of making decisions about whether LiteVideos should be
   // applied and the params to use when throttling media requests.
   lite_video::LiteVideoDecider* lite_video_decider_ = nullptr;
 
-  // The current navigation id of the mainframe navigation being observed. Used
-  // for recording tying all UKM metrics to the mainframe navigation source.
-  base::Optional<int64_t> current_mainframe_navigation_id_;
+  // The current metrics about the navigation |this| is observing. Reset
+  // after each time the metrics being held are recorded as a UKM event.
+  base::Optional<lite_video::LiteVideoNavigationMetrics> nav_metrics_;
 
   // Whether the navigations currently being observed should have the LiteVideo
   // optimization heldback due to a coinflip, counterfactual experiment.
@@ -83,9 +68,6 @@ class LiteVideoObserver
   // commits.
   bool is_coinflip_holdback_ = false;
 
-  // True if the main frame was not eligible for LiteVideo.
-  bool ineligible_main_frame_ = false;
-
   WEB_CONTENTS_USER_DATA_KEY_DECL();
 };