[Media Engagement] Cleanup Media.Engagement.SessionFinished UKM event

The Media.Engagement.SessionFinished UKM event has
lots of fields from when we were experimenting. We
should remove these prior to our storage cleanup.

BUG=998685

Change-Id: I87bbd5221d97161e042345e00b778096c3729523
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1775195Reviewed-by: Steven Holte <holte@chromium.org>
Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Commit-Queue: Becca Hughes <beccahughes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#713295}

chrome/browser/media/media_engagement_contents_observer_unittest.cc

@@ -302,13 +302,8 @@ class MediaEngagementContentsObserverTest
                       int playbacks_total,
                       int visits_total,
                       int score,
-                      int playbacks_delta,
-                      bool high_score,
                       int audible_players_delta,
-                      int audible_players_total,
-                      int significant_players_delta,
-                      int significant_players_total,
-                      int seconds_since_playback) {
+                      int significant_players_delta) {
     using Entry = ukm::builders::Media_Engagement_SessionFinished;
 
     auto ukm_entries = test_ukm_recorder_.GetEntriesByName(Entry::kEntryName);
@@ -322,25 +317,12 @@ class MediaEngagementContentsObserverTest
                                 ukm_entry, Entry::kVisits_TotalName));
     EXPECT_EQ(score, *test_ukm_recorder_.GetEntryMetric(
                          ukm_entry, Entry::kEngagement_ScoreName));
-    EXPECT_EQ(playbacks_delta, *test_ukm_recorder_.GetEntryMetric(
-                                   ukm_entry, Entry::kPlaybacks_DeltaName));
-    EXPECT_EQ(high_score, !!*test_ukm_recorder_.GetEntryMetric(
-                              ukm_entry, Entry::kEngagement_IsHighName));
     EXPECT_EQ(audible_players_delta,
               *test_ukm_recorder_.GetEntryMetric(
                   ukm_entry, Entry::kPlayer_Audible_DeltaName));
-    EXPECT_EQ(audible_players_total,
-              *test_ukm_recorder_.GetEntryMetric(
-                  ukm_entry, Entry::kPlayer_Audible_TotalName));
     EXPECT_EQ(significant_players_delta,
               *test_ukm_recorder_.GetEntryMetric(
                   ukm_entry, Entry::kPlayer_Significant_DeltaName));
-    EXPECT_EQ(significant_players_total,
-              *test_ukm_recorder_.GetEntryMetric(
-                  ukm_entry, Entry::kPlayer_Significant_TotalName));
-    EXPECT_EQ(seconds_since_playback,
-              *test_ukm_recorder_.GetEntryMetric(
-                  ukm_entry, Entry::kPlaybacks_SecondsSinceLastName));
   }
 
   void ExpectUkmIgnoredEntries(const url::Origin& origin,
@@ -1015,7 +997,7 @@ TEST_F(MediaEngagementContentsObserverTest, RecordUkmMetricsOnDestroy) {
 
   SimulateDestroy();
   ExpectScores(origin, 21.0 / 25.0, 25, 21, 5, 2, 1, 0);
-  ExpectUkmEntry(origin, 21, 25, 84, 1, true, 2, 5, 1, 2, 0);
+  ExpectUkmEntry(origin, 21, 25, 84, 2, 1);
 }
 
 TEST_F(MediaEngagementContentsObserverTest,
@@ -1033,7 +1015,7 @@ TEST_F(MediaEngagementContentsObserverTest,
 
   // AudioContext playbacks should count as a significant playback.
   ExpectScores(origin, 21.0 / 25.0, 25, 21, 2, 1, 0, 1);
-  ExpectUkmEntry(origin, 21, 25, 84, 0, true, 0, 2, 0, 1, 0);
+  ExpectUkmEntry(origin, 21, 25, 84, 0, 0);
 }
 
 TEST_F(MediaEngagementContentsObserverTest,
@@ -1046,7 +1028,7 @@ TEST_F(MediaEngagementContentsObserverTest,
 
   SimulateDestroy();
   ExpectScores(origin, 20.0 / 25.0, 25, 20, 2, 1, 0, 0);
-  ExpectUkmEntry(origin, 20, 25, 80, 0, true, 0, 2, 0, 1, 0);
+  ExpectUkmEntry(origin, 20, 25, 80, 0, 0);
 }
 
 TEST_F(MediaEngagementContentsObserverTest, RecordUkmMetricsOnNavigate) {
@@ -1063,7 +1045,7 @@ TEST_F(MediaEngagementContentsObserverTest, RecordUkmMetricsOnNavigate) {
 
   Navigate(GURL("https://www.example.org"));
   ExpectScores(origin, 21.0 / 25.0, 25, 21, 5, 2, 1, 0);
-  ExpectUkmEntry(origin, 21, 25, 84, 1, true, 2, 5, 1, 2, 0);
+  ExpectUkmEntry(origin, 21, 25, 84, 2, 1);
 }
 
 TEST_F(MediaEngagementContentsObserverTest,
@@ -1082,7 +1064,7 @@ TEST_F(MediaEngagementContentsObserverTest,
 
   // AudioContext playbacks should count as a media playback.
   ExpectScores(origin, 21.0 / 25.0, 25, 21, 2, 1, 0, 1);
-  ExpectUkmEntry(origin, 21, 25, 84, 0, true, 0, 2, 0, 1, 0);
+  ExpectUkmEntry(origin, 21, 25, 84, 0, 0);
 }
 
 TEST_F(MediaEngagementContentsObserverTest,
@@ -1095,7 +1077,7 @@ TEST_F(MediaEngagementContentsObserverTest,
 
   Navigate(GURL("https://www.example.org"));
   ExpectScores(origin, 6 / 28.0, 28, 6, 2, 1, 0, 0);
-  ExpectUkmEntry(origin, 6, 28, 21, 0, false, 0, 2, 0, 1, 0);
+  ExpectUkmEntry(origin, 6, 28, 21, 0, 0);
 }
 
 TEST_F(MediaEngagementContentsObserverTest,
@@ -1119,7 +1101,7 @@ TEST_F(MediaEngagementContentsObserverTest,
   SimulateDestroy();
   ExpectScores(origin, 21.0 / 25.0, 25, 21, 5, 3, 1, 0);
   ExpectLastPlaybackTime(origin, first);
-  ExpectUkmEntry(origin, 21, 25, 84, 1, true, 2, 5, 2, 3, 900);
+  ExpectUkmEntry(origin, 21, 25, 84, 2, 2);
 }
 
 TEST_F(MediaEngagementContentsObserverTest, DoNotCreateSessionOnInternalUrl) {

chrome/browser/media/media_engagement_session.cc

@@ -4,7 +4,6 @@
 
 #include "chrome/browser/media/media_engagement_session.h"
 
-#include "chrome/browser/media/media_engagement_preloaded_list.h"
 #include "chrome/browser/media/media_engagement_score.h"
 #include "chrome/browser/media/media_engagement_service.h"
 #include "media/base/media_switches.h"
@@ -118,30 +117,14 @@ void MediaEngagementSession::RecordUkmMetrics() {
   if (!ukm_recorder)
     return;
 
-  bool is_preloaded = false;
-  if (base::FeatureList::IsEnabled(media::kPreloadMediaEngagementData)) {
-    is_preloaded =
-        MediaEngagementPreloadedList::GetInstance()->CheckOriginIsPresent(
-            origin_);
-  }
-
   MediaEngagementScore score = service_->CreateEngagementScore(origin_);
   ukm::builders::Media_Engagement_SessionFinished(ukm_source_id_)
-      .SetPlaybacks_AudioContextTotal(score.audio_context_playbacks())
-      .SetPlaybacks_MediaElementTotal(score.media_element_playbacks())
       .SetPlaybacks_Total(score.media_playbacks())
       .SetVisits_Total(score.visits())
       .SetEngagement_Score(round(score.actual_score() * 100))
-      .SetPlaybacks_Delta(significant_media_element_playback_recorded_)
       .SetEngagement_IsHigh(score.high_score())
-      .SetEngagement_IsHigh_Changed(high_score_changed_)
-      .SetEngagement_IsHigh_Changes(score.high_score_changes())
-      .SetEngagement_IsPreloaded(is_preloaded)
       .SetPlayer_Audible_Delta(audible_players_total_)
-      .SetPlayer_Audible_Total(score.audible_playbacks())
       .SetPlayer_Significant_Delta(significant_players_total_)
-      .SetPlayer_Significant_Total(score.significant_playbacks())
-      .SetPlaybacks_SecondsSinceLast(time_since_playback_for_ukm_.InSeconds())
       .Record(ukm_recorder);
 }
 
@@ -165,8 +148,6 @@ void MediaEngagementSession::CommitPendingData() {
     score.IncrementVisits();
 
   if (WasSignificantPlaybackRecorded() && HasPendingPlaybackToCommit()) {
-    const base::Time old_time = score.last_media_playback_time();
-
     score.IncrementMediaPlaybacks();
 
     if (pending_data_to_commit_.audio_context_playback)
@@ -177,18 +158,6 @@ void MediaEngagementSession::CommitPendingData() {
 
     // Use the stored significant playback time.
     score.set_last_media_playback_time(first_significant_playback_time_);
-
-    // This code should be reached once and |time_since_playback_for_ukm_| can't
-    // be set.
-    DCHECK(time_since_playback_for_ukm_.is_zero());
-
-    if (!old_time.is_null()) {
-      // Calculate the time since the last playback and the first significant
-      // playback this visit. If there is no last playback time then we will
-      // record 0.
-      time_since_playback_for_ukm_ =
-          score.last_media_playback_time() - old_time;
-    }
   }
 
   if (pending_data_to_commit_.players) {

chrome/browser/media/media_engagement_session.h

@@ -113,9 +113,6 @@ class MediaEngagementSession : public base::RefCounted<MediaEngagementSession> {
   // The time the first significant playback occurred.
   base::Time first_significant_playback_time_;
 
-  // The time between significant playbacks to be recorded to UKM.
-  base::TimeDelta time_since_playback_for_ukm_;
-
   // Whether the session was restored.
   RestoreType restore_status_ = RestoreType::kNotRestored;
 

chrome/browser/media/media_engagement_session_unittest.cc

@@ -96,11 +96,6 @@ class MediaEngagementSessionTest : public testing::Test {
     session->RecordUkmMetrics();
   }
 
-  static base::TimeDelta GetTimeSincePlaybackForSession(
-      MediaEngagementSession* session) {
-    return session->time_since_playback_for_ukm_;
-  }
-
   MediaEngagementSessionTest()
       : origin_(url::Origin::Create(GURL("https://example.com"))) {}
 
@@ -682,34 +677,18 @@ TEST_F(MediaEngagementSessionTest, RecordUkmMetrics) {
 
     auto* ukm_entry = ukm_entries[0];
     test_ukm_recorder().ExpectEntrySourceHasUrl(ukm_entry, origin().GetURL());
-    EXPECT_EQ(0, *test_ukm_recorder().GetEntryMetric(
-                     ukm_entry, Entry::kPlaybacks_AudioContextTotalName));
-    EXPECT_EQ(1, *test_ukm_recorder().GetEntryMetric(
-                     ukm_entry, Entry::kPlaybacks_MediaElementTotalName));
     EXPECT_EQ(1, *test_ukm_recorder().GetEntryMetric(
                      ukm_entry, Entry::kPlaybacks_TotalName));
     EXPECT_EQ(1, *test_ukm_recorder().GetEntryMetric(ukm_entry,
                                                      Entry::kVisits_TotalName));
     EXPECT_EQ(5, *test_ukm_recorder().GetEntryMetric(
                      ukm_entry, Entry::kEngagement_ScoreName));
-    EXPECT_EQ(1, *test_ukm_recorder().GetEntryMetric(
-                     ukm_entry, Entry::kPlaybacks_DeltaName));
     EXPECT_EQ(0, *test_ukm_recorder().GetEntryMetric(
                      ukm_entry, Entry::kEngagement_IsHighName));
     EXPECT_EQ(0, *test_ukm_recorder().GetEntryMetric(
                      ukm_entry, Entry::kPlayer_Audible_DeltaName));
-    EXPECT_EQ(0, *test_ukm_recorder().GetEntryMetric(
-                     ukm_entry, Entry::kPlayer_Audible_TotalName));
     EXPECT_EQ(0, *test_ukm_recorder().GetEntryMetric(
                      ukm_entry, Entry::kPlayer_Significant_DeltaName));
-    EXPECT_EQ(0, *test_ukm_recorder().GetEntryMetric(
-                     ukm_entry, Entry::kPlayer_Significant_TotalName));
-    EXPECT_EQ(0, *test_ukm_recorder().GetEntryMetric(
-                     ukm_entry, Entry::kPlaybacks_SecondsSinceLastName));
-    EXPECT_EQ(0, *test_ukm_recorder().GetEntryMetric(
-                     ukm_entry, Entry::kEngagement_IsHigh_ChangesName));
-    EXPECT_EQ(0, *test_ukm_recorder().GetEntryMetric(
-                     ukm_entry, Entry::kEngagement_IsHigh_ChangedName));
   }
 
   session->RecordSignificantAudioContextPlayback();
@@ -723,96 +702,19 @@ TEST_F(MediaEngagementSessionTest, RecordUkmMetrics) {
 
     auto* ukm_entry = ukm_entries[1];
     test_ukm_recorder().ExpectEntrySourceHasUrl(ukm_entry, origin().GetURL());
-    EXPECT_EQ(1, *test_ukm_recorder().GetEntryMetric(
-                     ukm_entry, Entry::kPlaybacks_AudioContextTotalName));
-    EXPECT_EQ(1, *test_ukm_recorder().GetEntryMetric(
-                     ukm_entry, Entry::kPlaybacks_MediaElementTotalName));
+
     EXPECT_EQ(2, *test_ukm_recorder().GetEntryMetric(
                      ukm_entry, Entry::kPlaybacks_TotalName));
     EXPECT_EQ(1, *test_ukm_recorder().GetEntryMetric(ukm_entry,
                                                      Entry::kVisits_TotalName));
     EXPECT_EQ(10, *test_ukm_recorder().GetEntryMetric(
                       ukm_entry, Entry::kEngagement_ScoreName));
-    EXPECT_EQ(1, *test_ukm_recorder().GetEntryMetric(
-                     ukm_entry, Entry::kPlaybacks_DeltaName));
     EXPECT_EQ(0, *test_ukm_recorder().GetEntryMetric(
                      ukm_entry, Entry::kEngagement_IsHighName));
     EXPECT_EQ(0, *test_ukm_recorder().GetEntryMetric(
                      ukm_entry, Entry::kPlayer_Audible_DeltaName));
-    EXPECT_EQ(0, *test_ukm_recorder().GetEntryMetric(
-                     ukm_entry, Entry::kPlayer_Audible_TotalName));
     EXPECT_EQ(0, *test_ukm_recorder().GetEntryMetric(
                      ukm_entry, Entry::kPlayer_Significant_DeltaName));
-    EXPECT_EQ(0, *test_ukm_recorder().GetEntryMetric(
-                     ukm_entry, Entry::kPlayer_Significant_TotalName));
-    EXPECT_EQ(0, *test_ukm_recorder().GetEntryMetric(
-                     ukm_entry, Entry::kPlaybacks_SecondsSinceLastName));
-    EXPECT_EQ(0, *test_ukm_recorder().GetEntryMetric(
-                     ukm_entry, Entry::kEngagement_IsHigh_ChangesName));
-    EXPECT_EQ(0, *test_ukm_recorder().GetEntryMetric(
-                     ukm_entry, Entry::kEngagement_IsHigh_ChangedName));
-  }
-}
-
-TEST_F(MediaEngagementSessionTest, RecordUkmMetrics_Changed_NowHigh) {
-  const std::string url_string = origin().GetURL().spec();
-  using Entry = ukm::builders::Media_Engagement_SessionFinished;
-
-  // Set the visits and playbacks to just below the threshold so the next
-  // significant playback will result in the playback being high.
-  SetVisitsAndPlaybacks(19, 5);
-  EXPECT_FALSE(ScoreIsHigh());
-
-  scoped_refptr<MediaEngagementSession> session = new MediaEngagementSession(
-      service(), origin(), MediaEngagementSession::RestoreType::kNotRestored,
-      ukm_source_id());
-
-  session->RecordSignificantMediaElementPlayback();
-  CommitPendingDataForSession(session.get());
-
-  EXPECT_EQ(0u, test_ukm_recorder().GetEntriesByName(Entry::kEntryName).size());
-
-  RecordUkmMetricsForSession(session.get());
-
-  {
-    auto ukm_entries = test_ukm_recorder().GetEntriesByName(Entry::kEntryName);
-    EXPECT_EQ(1u, ukm_entries.size());
-
-    auto* ukm_entry = ukm_entries[0];
-    test_ukm_recorder().ExpectEntrySourceHasUrl(ukm_entry, origin().GetURL());
-    EXPECT_EQ(1u, *test_ukm_recorder().GetEntryMetric(
-                      ukm_entry, Entry::kEngagement_IsHigh_ChangedName));
-  }
-}
-
-TEST_F(MediaEngagementSessionTest, RecordUkmMetrics_Changed_WasHigh) {
-  const std::string url_string = origin().GetURL().spec();
-  using Entry = ukm::builders::Media_Engagement_SessionFinished;
-
-  // Set the visits and playbacks to just above the lower threshold and the is
-  // high bit to true so the next visit will cross the threshold.
-  SetVisitsAndPlaybacks(20, 20);
-  SetVisitsAndPlaybacks(20, 4);
-  EXPECT_TRUE(ScoreIsHigh());
-
-  scoped_refptr<MediaEngagementSession> session = new MediaEngagementSession(
-      service(), origin(), MediaEngagementSession::RestoreType::kNotRestored,
-      ukm_source_id());
-
-  CommitPendingDataForSession(session.get());
-
-  EXPECT_EQ(0u, test_ukm_recorder().GetEntriesByName(Entry::kEntryName).size());
-
-  RecordUkmMetricsForSession(session.get());
-
-  {
-    auto ukm_entries = test_ukm_recorder().GetEntriesByName(Entry::kEntryName);
-    EXPECT_EQ(1u, ukm_entries.size());
-
-    auto* ukm_entry = ukm_entries[0];
-    test_ukm_recorder().ExpectEntrySourceHasUrl(ukm_entry, origin().GetURL());
-    EXPECT_EQ(1u, *test_ukm_recorder().GetEntryMetric(
-                      ukm_entry, Entry::kEngagement_IsHigh_ChangedName));
   }
 }
 
@@ -960,42 +862,6 @@ TEST_F(MediaEngagementSessionTest, DestructorCommitDataIfNeeded) {
   }
 }
 
-// Tests that the TimeSinceLastPlayback is set to zero if there is no previous
-// record.
-TEST_F(MediaEngagementSessionTest, TimeSinceLastPlayback_NoPreviousRecord) {
-  scoped_refptr<MediaEngagementSession> session = new MediaEngagementSession(
-      service(), origin(), MediaEngagementSession::RestoreType::kNotRestored,
-      ukm_source_id());
-
-  EXPECT_TRUE(GetTimeSincePlaybackForSession(session.get()).is_zero());
-
-  // Advance in time and play.
-  test_clock()->Advance(base::TimeDelta::FromSeconds(42));
-  session->RecordSignificantMediaElementPlayback();
-
-  EXPECT_TRUE(GetTimeSincePlaybackForSession(session.get()).is_zero());
-}
-
-// Tests that the TimeSinceLastPlayback is set to the delta when there is a
-// previous record.
-TEST_F(MediaEngagementSessionTest, TimeSinceLastPlayback_PreviousRecord) {
-  scoped_refptr<MediaEngagementSession> session = new MediaEngagementSession(
-      service(), origin(), MediaEngagementSession::RestoreType::kNotRestored,
-      ukm_source_id());
-
-  EXPECT_TRUE(GetTimeSincePlaybackForSession(session.get()).is_zero());
-
-  // Advance in time and play.
-  test_clock()->Advance(base::TimeDelta::FromSeconds(42));
-  RecordPlayback(origin());
-
-  test_clock()->Advance(base::TimeDelta::FromSeconds(42));
-  session->RecordSignificantMediaElementPlayback();
-  CommitPendingDataForSession(session.get());
-
-  EXPECT_EQ(42, GetTimeSincePlaybackForSession(session.get()).InSeconds());
-}
-
 TEST_F(MediaEngagementSessionTest, RestoredSession_SimpleVisitNotRecorded) {
   scoped_refptr<MediaEngagementSession> session = new MediaEngagementSession(
       service(), origin(), MediaEngagementSession::RestoreType::kRestored,

tools/metrics/ukm/ukm.xml

@@ -3945,16 +3945,25 @@ be describing additional metrics about the same event.
     </summary>
   </metric>
   <metric name="Engagement.IsHigh.Changed">
+    <obsolete>
+      Deprecated 8/19 as part of https://crbug.com/998685
+    </obsolete>
     <summary>
       Whether the IsHigh bit changed during the current session.
     </summary>
   </metric>
   <metric name="Engagement.IsHigh.Changes">
+    <obsolete>
+      Deprecated 8/19 as part of https://crbug.com/998685
+    </obsolete>
     <summary>
       Counts of IsHigh changing (from 0 to 1 or 1 to 0).
     </summary>
   </metric>
   <metric name="Engagement.IsPreloaded">
+    <obsolete>
+      Deprecated 8/19 as part of https://crbug.com/998685
+    </obsolete>
     <summary>
       Whether the origin was preloaded (from the chrome://component) as a high
       engagement origin.
@@ -3969,12 +3978,18 @@ be describing additional metrics about the same event.
     </summary>
   </metric>
   <metric name="Playbacks.AudioContextTotal">
+    <obsolete>
+      Deprecated 8/19 as part of https://crbug.com/998685
+    </obsolete>
     <summary>
       The total number of significant media playbacks on this origin that came
       from WebAudio / AudioContext.
     </summary>
   </metric>
   <metric name="Playbacks.Delta">
+    <obsolete>
+      Deprecated 8/19 as part of https://crbug.com/998685
+    </obsolete>
     <summary>
       The number of significant media playbacks on this origin during this
       session (a visit to an origin on the same tab). A playback is determined
@@ -3983,12 +3998,18 @@ be describing additional metrics about the same event.
     </summary>
   </metric>
   <metric name="Playbacks.MediaElementTotal">
+    <obsolete>
+      Deprecated 8/19 as part of https://crbug.com/998685
+    </obsolete>
     <summary>
       The total number of significant media playbacks on this origin that came
       from media elements.
     </summary>
   </metric>
   <metric name="Playbacks.SecondsSinceLast">
+    <obsolete>
+      Deprecated 8/19 as part of https://crbug.com/998685
+    </obsolete>
     <summary>
       The number of seconds between significant media playback on the last visit
       and significant media playback on the current visit. If there was no
@@ -4007,6 +4028,9 @@ be describing additional metrics about the same event.
     </summary>
   </metric>
   <metric name="Player.Audible.Total">
+    <obsolete>
+      Deprecated 8/19 as part of https://crbug.com/998685
+    </obsolete>
     <summary>
       The delta from above but instead of a single visit, the total of all
       deltas for all visits on this origin.
@@ -4020,6 +4044,9 @@ be describing additional metrics about the same event.
     </summary>
   </metric>
   <metric name="Player.Significant.Total">
+    <obsolete>
+      Deprecated 8/19 as part of https://crbug.com/998685
+    </obsolete>
     <summary>
       The delta from above but instead of a single visit, the total of all
       deltas for all visits on this origin.