Media Engagement: better handling of history service callbacks.

- When all history is cleared, simply wipe all of MEI data instead of
  handling it per website.
- When history is expired, do nothing. Better handling of this event
  will be implemented as par tof bug 818153

Bug: 815163
Change-Id: I87c5d3e91b4c4e0c3191bb3c4e9c889ac6830790
Reviewed-on: https://chromium-review.googlesource.com/946169
Commit-Queue: Tommy Steimel <steimel@chromium.org>
Reviewed-by: Tommy Steimel <steimel@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#540842}

chrome/browser/media/media_engagement_service.cc

@@ -29,12 +29,24 @@ const char MediaEngagementService::kHistogramScoreAtStartupName[] =
 const char MediaEngagementService::kHistogramURLsDeletedScoreReductionName[] =
     "Media.Engagement.URLsDeletedScoreReduction";
 
+const char MediaEngagementService::kHistogramClearName[] =
+    "Media.Engagement.Clear";
+
 namespace {
 
 // The current schema version of the MEI data. If this value is higher
 // than the stored value, all MEI data will be wiped.
 static const int kSchemaVersion = 4;
 
+// Do not change the values of this enum as it is used for UMA.
+enum class MediaEngagementClearReason {
+  kDataAll = 0,
+  kDataRange = 1,
+  kHistoryAll = 2,
+  kHistoryRange = 3,
+  kCount
+};
+
 bool MediaEngagementFilterAdapter(
     const GURL& predicate,
     const ContentSettingsPattern& primary_pattern,
@@ -66,6 +78,11 @@ void RecordURLsDeletedScoreReduction(double previous_score,
       difference);
 }
 
+void RecordClear(MediaEngagementClearReason reason) {
+  UMA_HISTOGRAM_ENUMERATION(MediaEngagementService::kHistogramClearName, reason,
+                            MediaEngagementClearReason::kCount);
+}
+
 }  // namespace
 
 // static
@@ -149,6 +166,11 @@ void MediaEngagementService::SetSchemaVersion(int version) {
 void MediaEngagementService::ClearDataBetweenTime(
     const base::Time& delete_begin,
     const base::Time& delete_end) {
+  if (delete_begin == base::Time() && delete_end == base::Time::Max())
+    RecordClear(MediaEngagementClearReason::kDataAll);
+  else
+    RecordClear(MediaEngagementClearReason::kDataRange);
+
   HostContentSettingsMapFactory::GetForProfile(profile_)
       ->ClearSettingsForOneTypeWithPredicate(
           CONTENT_SETTINGS_TYPE_MEDIA_ENGAGEMENT, base::Time(),
@@ -177,6 +199,19 @@ void MediaEngagementService::OnURLsDeleted(
     bool expired,
     const history::URLRows& deleted_rows,
     const std::set<GURL>& favicon_urls) {
+  if (all_history) {
+    RecordClear(MediaEngagementClearReason::kHistoryAll);
+
+    HostContentSettingsMapFactory::GetForProfile(profile_)
+        ->ClearSettingsForOneType(CONTENT_SETTINGS_TYPE_MEDIA_ENGAGEMENT);
+    return;
+  }
+
+  // TODO(818153): history expiration currently has no effect on MEI but entries
+  // that no longer appear in history should be removed from the database.
+  if (expired)
+    return;
+
   std::map<GURL, int> origins;
   for (const history::URLRow& row : deleted_rows) {
     GURL origin = row.url().GetOrigin();
@@ -186,6 +221,9 @@ void MediaEngagementService::OnURLsDeleted(
     origins[origin]++;
   }
 
+  if (!origins.empty())
+    RecordClear(MediaEngagementClearReason::kHistoryRange);
+
   for (auto const& kv : origins) {
     // Remove the number of visits consistent with the number
     // of URLs from the same origin we are removing.

chrome/browser/media/media_engagement_service.h

@@ -102,6 +102,10 @@ class MediaEngagementService : public KeyedService,
   // is cleared.
   static const char kHistogramURLsDeletedScoreReductionName[];
 
+  // The name of the histogram that records the reason why the engagement was
+  // cleared, either partially or fully.
+  static const char kHistogramClearName[];
+
  private:
   friend class MediaEngagementBrowserTest;
   friend class MediaEngagementContentsObserverTest;

chrome/browser/media/media_engagement_service_unittest.cc

@@ -114,6 +114,8 @@ class MediaEngagementServiceTest : public ChromeRenderViewHostTestHarness {
     service_ = base::WrapUnique(StartNewMediaEngagementService());
   }
 
+  MediaEngagementService* service() const { return service_.get(); }
+
   MediaEngagementService* StartNewMediaEngagementService() {
     MediaEngagementService* service =
         new MediaEngagementService(profile(), &test_clock_);
@@ -459,6 +461,11 @@ TEST_F(MediaEngagementServiceTest, CleanupOriginsOnHistoryDeletion) {
         MediaEngagementService::kHistogramURLsDeletedScoreReductionName, 5, 2);
     histogram_tester.ExpectBucketCount(
         MediaEngagementService::kHistogramURLsDeletedScoreReductionName, 4, 1);
+
+    histogram_tester.ExpectTotalCount(
+        MediaEngagementService::kHistogramClearName, 1);
+    histogram_tester.ExpectBucketCount(
+        MediaEngagementService::kHistogramClearName, 3, 1);
   }
 
   {
@@ -488,6 +495,11 @@ TEST_F(MediaEngagementServiceTest, CleanupOriginsOnHistoryDeletion) {
         MediaEngagementService::kHistogramURLsDeletedScoreReductionName, 1);
     histogram_tester.ExpectBucketCount(
         MediaEngagementService::kHistogramURLsDeletedScoreReductionName, 5, 1);
+
+    histogram_tester.ExpectTotalCount(
+        MediaEngagementService::kHistogramClearName, 1);
+    histogram_tester.ExpectBucketCount(
+        MediaEngagementService::kHistogramClearName, 3, 1);
   }
 
   {
@@ -519,12 +531,148 @@ TEST_F(MediaEngagementServiceTest, CleanupOriginsOnHistoryDeletion) {
         MediaEngagementService::kHistogramURLsDeletedScoreReductionName, 1);
     histogram_tester.ExpectBucketCount(
         MediaEngagementService::kHistogramURLsDeletedScoreReductionName, 0, 1);
+
+    histogram_tester.ExpectTotalCount(
+        MediaEngagementService::kHistogramClearName, 1);
+    histogram_tester.ExpectBucketCount(
+        MediaEngagementService::kHistogramClearName, 3, 1);
+  }
+}
+
+TEST_F(MediaEngagementServiceTest, CleanUpDatabaseWhenHistoryIsDeleted) {
+  GURL origin1("http://www.google.com/");
+  GURL origin1a("http://www.google.com/search?q=asdf");
+  GURL origin1b("http://www.google.com/maps/search?q=asdf");
+  GURL origin2("https://drive.google.com/");
+  GURL origin3("http://deleted.com/");
+  GURL origin3a("http://deleted.com/test");
+  GURL origin4("http://notdeleted.com");
+
+  // origin1 will have a score that is high enough to not return zero
+  // and we will ensure it has the same score. origin2 will have a score
+  // that is zero and will remain zero. origin3 will have a score
+  // and will be cleared. origin4 will have a normal score.
+  SetScores(origin1, MediaEngagementScore::GetScoreMinVisits() + 2, 14);
+  SetScores(origin2, 2, 1);
+  SetScores(origin3, 2, 1);
+  SetScores(origin4, MediaEngagementScore::GetScoreMinVisits(), 10);
+
+  base::Time today = GetReferenceTime();
+  base::Time yesterday_afternoon = GetReferenceTime() -
+                                   base::TimeDelta::FromDays(1) +
+                                   base::TimeDelta::FromHours(4);
+  base::Time yesterday_week = GetReferenceTime() - base::TimeDelta::FromDays(8);
+  SetNow(today);
+
+  history::HistoryService* history = HistoryServiceFactory::GetForProfile(
+      profile(), ServiceAccessType::IMPLICIT_ACCESS);
+
+  history->AddPage(origin1, yesterday_afternoon, history::SOURCE_BROWSED);
+  history->AddPage(origin1a, yesterday_afternoon, history::SOURCE_BROWSED);
+  history->AddPage(origin1b, yesterday_week, history::SOURCE_BROWSED);
+  history->AddPage(origin2, yesterday_afternoon, history::SOURCE_BROWSED);
+  history->AddPage(origin3, yesterday_week, history::SOURCE_BROWSED);
+  history->AddPage(origin3a, yesterday_afternoon, history::SOURCE_BROWSED);
+
+  // Check that the scores are valid at the beginning.
+  ExpectScores(origin1, 7.0 / 11.0,
+               MediaEngagementScore::GetScoreMinVisits() + 2, 14, TimeNotSet());
+  EXPECT_EQ(14.0 / 22.0, GetActualScore(origin1));
+  ExpectScores(origin2, 0.05, 2, 1, TimeNotSet());
+  EXPECT_EQ(1 / 20.0, GetActualScore(origin2));
+  ExpectScores(origin3, 0.05, 2, 1, TimeNotSet());
+  EXPECT_EQ(1 / 20.0, GetActualScore(origin3));
+  ExpectScores(origin4, 0.5, MediaEngagementScore::GetScoreMinVisits(), 10,
+               TimeNotSet());
+  EXPECT_EQ(0.5, GetActualScore(origin4));
+
+  {
+    base::HistogramTester histogram_tester;
+
+    base::RunLoop run_loop;
+    base::CancelableTaskTracker task_tracker;
+    // Clear all history.
+    history->ExpireHistoryBetween(std::set<GURL>(), base::Time(), base::Time(),
+                                  run_loop.QuitClosure(), &task_tracker);
+    run_loop.Run();
+
+    // origin1 should have a score that is not zero and is the same as the old
+    // score (sometimes it may not match exactly due to rounding). origin2
+    // should have a score that is zero but it's visits and playbacks should
+    // have decreased. origin3 should have had a decrease in the number of
+    // visits. origin4 should have the old score.
+    ExpectScores(origin1, 0.0, 0, 0, TimeNotSet());
+    EXPECT_EQ(0, GetActualScore(origin1));
+    ExpectScores(origin2, 0.0, 0, 0, TimeNotSet());
+    EXPECT_EQ(0, GetActualScore(origin2));
+    ExpectScores(origin3, 0.0, 0, 0, TimeNotSet());
+    ExpectScores(origin4, 0.0, 0, 0, TimeNotSet());
+
+    histogram_tester.ExpectTotalCount(
+        MediaEngagementService::kHistogramURLsDeletedScoreReductionName, 0);
+
+    histogram_tester.ExpectTotalCount(
+        MediaEngagementService::kHistogramClearName, 1);
+    histogram_tester.ExpectBucketCount(
+        MediaEngagementService::kHistogramClearName, 2, 1);
+  }
+}
+
+TEST_F(MediaEngagementServiceTest, HistoryExpirationIsNoOp) {
+  GURL origin1("http://www.google.com/");
+  GURL origin1a("http://www.google.com/search?q=asdf");
+  GURL origin1b("http://www.google.com/maps/search?q=asdf");
+  GURL origin2("https://drive.google.com/");
+  GURL origin3("http://deleted.com/");
+  GURL origin3a("http://deleted.com/test");
+  GURL origin4("http://notdeleted.com");
+
+  SetScores(origin1, MediaEngagementScore::GetScoreMinVisits() + 2, 14);
+  SetScores(origin2, 2, 1);
+  SetScores(origin3, 2, 1);
+  SetScores(origin4, MediaEngagementScore::GetScoreMinVisits(), 10);
+
+  ExpectScores(origin1, 7.0 / 11.0,
+               MediaEngagementScore::GetScoreMinVisits() + 2, 14, TimeNotSet());
+  EXPECT_EQ(14.0 / 22.0, GetActualScore(origin1));
+  ExpectScores(origin2, 0.05, 2, 1, TimeNotSet());
+  EXPECT_EQ(1 / 20.0, GetActualScore(origin2));
+  ExpectScores(origin3, 0.05, 2, 1, TimeNotSet());
+  EXPECT_EQ(1 / 20.0, GetActualScore(origin3));
+  ExpectScores(origin4, 0.5, MediaEngagementScore::GetScoreMinVisits(), 10,
+               TimeNotSet());
+  EXPECT_EQ(0.5, GetActualScore(origin4));
+
+  {
+    base::HistogramTester histogram_tester;
+
+    service()->OnURLsDeleted(nullptr, false, true, history::URLRows(),
+                             std::set<GURL>());
+
+    // Same as above, nothing should have changed.
+    ExpectScores(origin1, 7.0 / 11.0,
+                 MediaEngagementScore::GetScoreMinVisits() + 2, 14,
+                 TimeNotSet());
+    EXPECT_EQ(14.0 / 22.0, GetActualScore(origin1));
+    ExpectScores(origin2, 0.05, 2, 1, TimeNotSet());
+    EXPECT_EQ(1 / 20.0, GetActualScore(origin2));
+    ExpectScores(origin3, 0.05, 2, 1, TimeNotSet());
+    EXPECT_EQ(1 / 20.0, GetActualScore(origin3));
+    ExpectScores(origin4, 0.5, MediaEngagementScore::GetScoreMinVisits(), 10,
+                 TimeNotSet());
+    EXPECT_EQ(0.5, GetActualScore(origin4));
+
+    histogram_tester.ExpectTotalCount(
+        MediaEngagementService::kHistogramURLsDeletedScoreReductionName, 0);
+    histogram_tester.ExpectTotalCount(
+        MediaEngagementService::kHistogramClearName, 0);
   }
 }
 
 TEST_F(MediaEngagementServiceTest,
        CleanupDataOnSiteDataCleanup_OutsideBoundary) {
   GURL origin("https://www.google.com");
+  base::HistogramTester histogram_tester;
 
   base::Time today = GetReferenceTime();
   SetNow(today);
@@ -535,12 +683,18 @@ TEST_F(MediaEngagementServiceTest,
   ClearDataBetweenTime(today - base::TimeDelta::FromDays(2),
                        today - base::TimeDelta::FromDays(1));
   ExpectScores(origin, 0.05, 1, 1, today);
+
+  histogram_tester.ExpectTotalCount(MediaEngagementService::kHistogramClearName,
+                                    1);
+  histogram_tester.ExpectBucketCount(
+      MediaEngagementService::kHistogramClearName, 1, 1);
 }
 
 TEST_F(MediaEngagementServiceTest,
        CleanupDataOnSiteDataCleanup_WithinBoundary) {
   GURL origin1("https://www.google.com");
   GURL origin2("https://www.google.co.uk");
+  base::HistogramTester histogram_tester;
 
   base::Time today = GetReferenceTime();
   base::Time yesterday = today - base::TimeDelta::FromDays(1);
@@ -555,10 +709,16 @@ TEST_F(MediaEngagementServiceTest,
   ClearDataBetweenTime(two_days_ago, yesterday);
   ExpectScores(origin1, 0, 0, 0, TimeNotSet());
   ExpectScores(origin2, 0, 0, 0, TimeNotSet());
+
+  histogram_tester.ExpectTotalCount(MediaEngagementService::kHistogramClearName,
+                                    1);
+  histogram_tester.ExpectBucketCount(
+      MediaEngagementService::kHistogramClearName, 1, 1);
 }
 
 TEST_F(MediaEngagementServiceTest, CleanupDataOnSiteDataCleanup_NoTimeSet) {
   GURL origin("https://www.google.com");
+  base::HistogramTester histogram_tester;
 
   base::Time today = GetReferenceTime();
 
@@ -568,6 +728,11 @@ TEST_F(MediaEngagementServiceTest, CleanupDataOnSiteDataCleanup_NoTimeSet) {
   ClearDataBetweenTime(today - base::TimeDelta::FromDays(2),
                        today - base::TimeDelta::FromDays(1));
   ExpectScores(origin, 0.0, 1, 0, TimeNotSet());
+
+  histogram_tester.ExpectTotalCount(MediaEngagementService::kHistogramClearName,
+                                    1);
+  histogram_tester.ExpectBucketCount(
+      MediaEngagementService::kHistogramClearName, 1, 1);
 }
 
 TEST_F(MediaEngagementServiceTest, LogScoresOnStartupToHistogram) {
@@ -599,6 +764,31 @@ TEST_F(MediaEngagementServiceTest, LogScoresOnStartupToHistogram) {
       MediaEngagementService::kHistogramScoreAtStartupName, 83, 1);
 }
 
+TEST_F(MediaEngagementServiceTest, CleanupDataOnSiteDataCleanup_All) {
+  GURL origin1("https://www.google.com");
+  GURL origin2("https://www.google.co.uk");
+  base::HistogramTester histogram_tester;
+
+  base::Time today = GetReferenceTime();
+  base::Time yesterday = today - base::TimeDelta::FromDays(1);
+  base::Time two_days_ago = today - base::TimeDelta::FromDays(2);
+  SetNow(today);
+
+  SetScores(origin1, 1, 1);
+  SetScores(origin2, 1, 1);
+  SetLastMediaPlaybackTime(origin1, yesterday);
+  SetLastMediaPlaybackTime(origin2, two_days_ago);
+
+  ClearDataBetweenTime(base::Time(), base::Time::Max());
+  ExpectScores(origin1, 0, 0, 0, TimeNotSet());
+  ExpectScores(origin2, 0, 0, 0, TimeNotSet());
+
+  histogram_tester.ExpectTotalCount(MediaEngagementService::kHistogramClearName,
+                                    1);
+  histogram_tester.ExpectBucketCount(
+      MediaEngagementService::kHistogramClearName, 0, 1);
+}
+
 TEST_F(MediaEngagementServiceTest, HasHighEngagement) {
   GURL url1("https://www.google.com");
   GURL url2("https://www.google.co.uk");

tools/metrics/histograms/enums.xml

@@ -28536,6 +28536,13 @@ Called by update_use_counter_css.py.-->
   <int value="21" label="Gesture requirement overridden by play method."/>
 </enum>
 
+<enum name="MediaEngagementClearReason">
+  <int value="0" label="Data (all)"/>
+  <int value="1" label="Data (range)"/>
+  <int value="2" label="History (all)"/>
+  <int value="3" label="History (range)"/>
+</enum>
+
 <enum name="MediaEngagementSessionEvent">
   <int value="0" label="Created"/>
   <int value="1" label="Significant Playback"/>

tools/metrics/histograms/histograms.xml

@@ -35372,6 +35372,15 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
   </summary>
 </histogram>
 
+<histogram name="Media.Engagement.Clear" enum="MediaEngagementClearReason">
+  <owner>mlamouri@chromium.org</owner>
+  <owner>media-dev@chromium.org</owner>
+  <summary>
+    Records the reason why the Media Engagement data was cleared. Partial
+    changes and full wipeout will both be recorded as one event.
+  </summary>
+</histogram>
+
 <histogram name="Media.Engagement.PreloadedList.CheckResult"
     enum="PreloadedListCheckResult">
   <owner>beccahughes@chromium.org</owner>