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

This is a reland of f672068e

The tests timeout with closing the tab right after the navigation, so
wait to close the tab until after the histograms are recorded.


Original change's description:
> [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}

TBR=tbansal@chromium.org

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

chrome/browser/BUILD.gn

@@ -623,6 +623,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);
@@ -637,19 +655,20 @@ IN_PROC_BROWSER_TEST_F(LiteVideoKeyedServiceBrowserTest,
       "LiteVideo.CanApplyLiteVideo.UserBlocklist.SubFrame",
       lite_video::LiteVideoBlocklistReason::kAllowed, 1);
 
+  // Close the tab to flush the UKM metrics.
+  browser()->tab_strip_model()->GetActiveWebContents()->Close();
+
   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
@@ -691,14 +710,17 @@ IN_PROC_BROWSER_TEST_F(LiteVideoKeyedServiceCoinflipBrowserTest,
   histogram_tester()->ExpectBucketCount("LiteVideo.Navigation.HasHint", true,
                                         2);
 
+  // Close the tab to flush the UKM metrics.
+  browser()->tab_strip_model()->GetActiveWebContents()->Close();
+
   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();
 };