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

This reverts commit f672068e.

Reason for revert: Caused failing test https://ci.chromium.org/p/chromium/builders/ci/linux-chromeos-rel/40039

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,rajendrant@chromium.org,mcrouse@chromium.org

Change-Id: Ic8ccef2431cb402a285edd00b29cf78d44b03ab9
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1109068, 1110739
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2325295Reviewed-by: Rakina Zata Amni <rakina@chromium.org>
Commit-Queue: Rakina Zata Amni <rakina@chromium.org>
Cr-Commit-Position: refs/heads/master@{#792528}

chrome/browser/BUILD.gn

@@ -625,8 +625,6 @@ 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,9 +192,6 @@ 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);
@@ -221,9 +218,6 @@ 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);
@@ -262,9 +256,6 @@ 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);
@@ -320,9 +311,6 @@ 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);
@@ -389,9 +377,6 @@ 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);
@@ -457,9 +442,6 @@ 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);
@@ -643,9 +625,6 @@ 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);
@@ -660,15 +639,17 @@ IN_PROC_BROWSER_TEST_F(LiteVideoKeyedServiceBrowserTest,
 
   auto entries =
       ukm_recorder.GetEntriesByName(ukm::builders::LiteVideo::kEntryName);
-  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));
+  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));
+  }
 }
 
 class LiteVideoKeyedServiceCoinflipBrowserTest
@@ -704,9 +685,6 @@ 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);
@@ -715,12 +693,12 @@ IN_PROC_BROWSER_TEST_F(LiteVideoKeyedServiceCoinflipBrowserTest,
 
   auto entries =
       ukm_recorder.GetEntriesByName(ukm::builders::LiteVideo::kEntryName);
-  // 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));
+  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));
+  }
 }

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,7 +13,6 @@
 #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"
@@ -44,6 +43,15 @@ 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
@@ -60,20 +68,26 @@ LiteVideoObserver::LiteVideoObserver(content::WebContents* web_contents)
   lite_video_decider_ = GetLiteVideoDeciderFromWebContents(web_contents);
 }
 
-LiteVideoObserver::~LiteVideoObserver() {
-  FlushUKMMetrics();
+LiteVideoObserver::~LiteVideoObserver() = default;
+
+void LiteVideoObserver::DidStartNavigation(
+    content::NavigationHandle* navigation_handle) {
+  if (navigation_handle->IsInMainFrame())
+    current_mainframe_navigation_id_.reset();
 }
 
 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;
 
@@ -83,20 +97,23 @@ void LiteVideoObserver::DidFinishNavigation(
       lite_video_decider_->CanApplyLiteVideo(navigation_handle,
                                              &blocklist_reason);
 
-  MaybeUpdateCoinflipExperimentState(navigation_handle);
+  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();
+  }
 
+  // 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);
 
-  if (navigation_handle->IsInMainFrame()) {
-    FlushUKMMetrics();
-    nav_metrics_ = lite_video::LiteVideoNavigationMetrics(
-        navigation_handle->GetNavigationId(), decision, blocklist_reason);
-  }
+  RecordUKMMetrics(decision, blocklist_reason);
 
   LOCAL_HISTOGRAM_BOOLEAN("LiteVideo.Navigation.HasHint", hint ? true : false);
 
-  if (decision == lite_video::LiteVideoDecision::kNotAllowed)
+  if (!hint)
     return;
 
   content::RenderFrameHost* render_frame_host =
@@ -132,29 +149,21 @@ lite_video::LiteVideoDecision LiteVideoObserver::MakeLiteVideoDecision(
   return lite_video::LiteVideoDecision::kNotAllowed;
 }
 
-void LiteVideoObserver::FlushUKMMetrics() {
-  if (!nav_metrics_)
+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_) {
     return;
+  }
   ukm::SourceId ukm_source_id = ukm::ConvertToSourceId(
-      nav_metrics_->nav_id(), ukm::SourceIdType::NAVIGATION_ID);
+      *current_mainframe_navigation_id_, ukm::SourceIdType::NAVIGATION_ID);
   ukm::builders::LiteVideo builder(ukm_source_id);
-  builder.SetThrottlingStartDecision(static_cast<int>(nav_metrics_->decision()))
-      .SetBlocklistReason(static_cast<int>(nav_metrics_->blocklist_reason()))
+  builder.SetThrottlingStartDecision(static_cast<int>(decision))
+      .SetBlocklistReason(static_cast<int>(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,7 +8,6 @@
 #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"
@@ -21,6 +20,23 @@ 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
@@ -36,6 +52,8 @@ class LiteVideoObserver
   explicit LiteVideoObserver(content::WebContents* web_contents);
 
   // content::WebContentsObserver.
+  void DidStartNavigation(
+      content::NavigationHandle* navigation_handle) override;
   void DidFinishNavigation(
       content::NavigationHandle* navigation_handle) override;
 
@@ -46,21 +64,18 @@ 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 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 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 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 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_;
+  // 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_;
 
   // Whether the navigations currently being observed should have the LiteVideo
   // optimization heldback due to a coinflip, counterfactual experiment.
@@ -68,6 +83,9 @@ 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();
 };