[Media Engagement] Remove visits with media tag

This was never used and therefore we can remove
it. If there is old data stored then this will
also remove it.

BUG=998687

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

chrome/browser/media/media_engagement_score.cc

@@ -21,8 +21,6 @@ const char MediaEngagementScore::kHasHighScoreKey[] = "hasHighScore";
 const char MediaEngagementScore::kAudiblePlaybacksKey[] = "audiblePlaybacks";
 const char MediaEngagementScore::kSignificantPlaybacksKey[] =
     "significantPlaybacks";
-const char MediaEngagementScore::kVisitsWithMediaTagKey[] =
-    "visitsWithMediaTag";
 const char MediaEngagementScore::kHighScoreChanges[] = "highScoreChanges";
 const char MediaEngagementScore::kSignificantMediaPlaybacksKey[] =
     "mediaElementPlaybacks";
@@ -122,8 +120,6 @@ MediaEngagementScore::MediaEngagementScore(
                       &audible_playbacks_);
   GetIntegerFromScore(score_dict_.get(), kSignificantPlaybacksKey,
                       &significant_playbacks_);
-  GetIntegerFromScore(score_dict_.get(), kVisitsWithMediaTagKey,
-                      &visits_with_media_tag_);
   GetIntegerFromScore(score_dict_.get(), kHighScoreChanges,
                       &high_score_changes_);
   GetIntegerFromScore(score_dict_.get(), kSignificantMediaPlaybacksKey,
@@ -208,7 +204,6 @@ bool MediaEngagementScore::UpdateScoreDict() {
   bool is_high = false;
   int stored_audible_playbacks = 0;
   int stored_significant_playbacks = 0;
-  int stored_visits_with_media_tag = 0;
   int high_score_changes = 0;
   int stored_media_element_playbacks = 0;
   int stored_audio_context_playbacks = 0;
@@ -239,8 +234,6 @@ bool MediaEngagementScore::UpdateScoreDict() {
                       &stored_audible_playbacks);
   GetIntegerFromScore(score_dict_.get(), kSignificantPlaybacksKey,
                       &stored_significant_playbacks);
-  GetIntegerFromScore(score_dict_.get(), kVisitsWithMediaTagKey,
-                      &stored_visits_with_media_tag);
   GetIntegerFromScore(score_dict_.get(), kHighScoreChanges,
                       &high_score_changes);
   GetIntegerFromScore(score_dict_.get(), kSignificantMediaPlaybacksKey,
@@ -253,7 +246,6 @@ bool MediaEngagementScore::UpdateScoreDict() {
                  is_high_ != is_high ||
                  stored_audible_playbacks != audible_playbacks() ||
                  stored_significant_playbacks != significant_playbacks() ||
-                 stored_visits_with_media_tag != visits_with_media_tag() ||
                  stored_media_element_playbacks != media_element_playbacks() ||
                  stored_audio_context_playbacks != audio_context_playbacks() ||
                  stored_last_media_playback_internal !=
@@ -276,13 +268,16 @@ bool MediaEngagementScore::UpdateScoreDict() {
   score_dict_->SetBoolean(kHasHighScoreKey, is_high_);
   score_dict_->SetInteger(kAudiblePlaybacksKey, audible_playbacks_);
   score_dict_->SetInteger(kSignificantPlaybacksKey, significant_playbacks_);
-  score_dict_->SetInteger(kVisitsWithMediaTagKey, visits_with_media_tag_);
   score_dict_->SetInteger(kHighScoreChanges, high_score_changes_);
   score_dict_->SetInteger(kSignificantMediaPlaybacksKey,
                           media_element_playbacks_);
   score_dict_->SetInteger(kSignificantAudioContextPlaybacksKey,
                           audio_context_playbacks_);
 
+  // visitsWithMediaTag was deprecated in https://crbug.com/998687 and should
+  // be removed if we see it in |score_dict_|.
+  score_dict_->RemoveKey("visitsWithMediaTag");
+
   return true;
 }
 

chrome/browser/media/media_engagement_score.h

@@ -26,20 +26,18 @@ class MediaEngagementScore final {
   // store whether the score is considered high. kAudiblePlaybacksKey will
   // store the number of audible playbacks on an origin.
   // kSignificantPlaybacksKey will store the number of significant playbacks
-  // on an origin. kVisitsWithMediaTagKey will store the number of visits to
-  // an origin where at least one page had a media tag.
-  // kSignificantMediaPlaybacksKey will store significant playbacks that
-  // originated from media elements and kSignificantAudioContextPlaybacksKey
-  // will store significant playbacks originated from audio contexts. The
-  // sum of the two may be higher than kMediaPlaybacksKey as a visit may have
-  // both an audio context playback and a media element playback.
+  // on an origin. kSignificantMediaPlaybacksKey will store significant
+  // playbacks that originated from media elements and
+  // kSignificantAudioContextPlaybacksKey will store significant playbacks
+  // originated from audio contexts. The sum of the two may be higher than
+  // kMediaPlaybacksKey as a visit may have both an audio context playback
+  // and a media element playback.
   static const char kVisitsKey[];
   static const char kMediaPlaybacksKey[];
   static const char kLastMediaPlaybackTimeKey[];
   static const char kHasHighScoreKey[];
   static const char kAudiblePlaybacksKey[];
   static const char kSignificantPlaybacksKey[];
-  static const char kVisitsWithMediaTagKey[];
   static const char kHighScoreChanges[];
   static const char kSignificantMediaPlaybacksKey[];
   static const char kSignificantAudioContextPlaybacksKey[];
@@ -106,12 +104,6 @@ class MediaEngagementScore final {
     set_significant_playbacks(significant_playbacks_ + amount);
   }
 
-  // Get/increment the number of visits where at least one page had a media tag.
-  int visits_with_media_tag() const { return visits_with_media_tag_; }
-  void IncrementVisitsWithMediaTag() {
-    set_visits_with_media_tag(visits_with_media_tag_ + 1);
-  }
-
   // Get/increment the number of significant playbacks from media elements this
   // origin had.
   int media_element_playbacks() const { return media_element_playbacks_; }
@@ -152,9 +144,6 @@ class MediaEngagementScore final {
   void set_significant_playbacks(int playbacks) {
     significant_playbacks_ = playbacks;
   }
-  void set_visits_with_media_tag(int visits) {
-    visits_with_media_tag_ = visits;
-  }
   void set_media_element_playbacks(int playbacks) {
     media_element_playbacks_ = playbacks;
   }
@@ -196,9 +185,6 @@ class MediaEngagementScore final {
   // The number of significant media playbacks this origin had.
   int significant_playbacks_ = 0;
 
-  // The number of visits with a media tag this origin had.
-  int visits_with_media_tag_ = 0;
-
   // The last time media was played back on this origin.
   base::Time last_media_playback_time_;
 

chrome/browser/media/media_engagement_score_unittest.cc

@@ -68,7 +68,6 @@ class MediaEngagementScoreTest : public ChromeRenderViewHostTestHarness {
                    bool has_high_score,
                    int audible_playbacks,
                    int significant_playbacks,
-                   int visits_with_media_tag,
                    int high_score_changes,
                    int media_element_playbacks,
                    int audio_context_playbacks) {
@@ -79,7 +78,6 @@ class MediaEngagementScoreTest : public ChromeRenderViewHostTestHarness {
     EXPECT_EQ(has_high_score, score->high_score());
     EXPECT_EQ(audible_playbacks, score->audible_playbacks());
     EXPECT_EQ(significant_playbacks, score->significant_playbacks());
-    EXPECT_EQ(visits_with_media_tag, score->visits_with_media_tag());
     EXPECT_EQ(high_score_changes, score->high_score_changes());
     EXPECT_EQ(media_element_playbacks, score->media_element_playbacks());
     EXPECT_EQ(audio_context_playbacks, score->audio_context_playbacks());
@@ -92,7 +90,6 @@ class MediaEngagementScoreTest : public ChromeRenderViewHostTestHarness {
     score->IncrementMediaPlaybacks();
     score->IncrementAudiblePlaybacks(1);
     score->IncrementSignificantPlaybacks(1);
-    score->IncrementVisitsWithMediaTag();
     score->IncrementMediaElementPlaybacks();
     score->IncrementAudioContextPlaybacks();
   }
@@ -105,7 +102,6 @@ class MediaEngagementScoreTest : public ChromeRenderViewHostTestHarness {
       bool has_high_score,
       int audible_playbacks,
       int significant_playbacks,
-      int visits_with_media_tag,
       int high_score_changes,
       int media_element_playbacks,
       int audio_context_playbacks,
@@ -115,9 +111,8 @@ class MediaEngagementScoreTest : public ChromeRenderViewHostTestHarness {
                                  std::move(score_dict), nullptr /* settings */);
     VerifyScore(initial_score, expected_visits, expected_media_playbacks,
                 expected_last_media_playback_time, has_high_score,
-                audible_playbacks, significant_playbacks, visits_with_media_tag,
-                high_score_changes, media_element_playbacks,
-                audio_context_playbacks);
+                audible_playbacks, significant_playbacks, high_score_changes,
+                media_element_playbacks, audio_context_playbacks);
 
     // Updating the score dict should return false, as the score shouldn't
     // have changed at this point.
@@ -199,7 +194,7 @@ TEST_F(MediaEngagementScoreTest, MojoSerialization) {
 TEST_F(MediaEngagementScoreTest, EmptyDictionary) {
   std::unique_ptr<base::DictionaryValue> dict(new base::DictionaryValue());
   TestScoreInitializesAndUpdates(std::move(dict), 0, 0, base::Time(), false, 0,
-                                 0, 0, 0, 0, 0, true);
+                                 0, 0, 0, 0, true);
 }
 
 // Test that scores are read / written correctly from / to partially empty
@@ -209,7 +204,7 @@ TEST_F(MediaEngagementScoreTest, PartiallyEmptyDictionary) {
   dict->SetInteger(MediaEngagementScore::kVisitsKey, 2);
 
   TestScoreInitializesAndUpdates(std::move(dict), 2, 0, base::Time(), false, 0,
-                                 0, 0, 0, 0, 0, true);
+                                 0, 0, 0, 0, true);
 }
 
 // Test that scores are read / written correctly from / to populated score
@@ -223,14 +218,13 @@ TEST_F(MediaEngagementScoreTest, PopulatedDictionary) {
   dict->SetBoolean(MediaEngagementScore::kHasHighScoreKey, true);
   dict->SetInteger(MediaEngagementScore::kAudiblePlaybacksKey, 2);
   dict->SetInteger(MediaEngagementScore::kSignificantPlaybacksKey, 4);
-  dict->SetInteger(MediaEngagementScore::kVisitsWithMediaTagKey, 6);
   dict->SetInteger(MediaEngagementScore::kHighScoreChanges, 3);
   dict->SetInteger(MediaEngagementScore::kSignificantMediaPlaybacksKey, 1);
   dict->SetInteger(MediaEngagementScore::kSignificantAudioContextPlaybacksKey,
                    2);
 
   TestScoreInitializesAndUpdates(std::move(dict), 20, 12, test_clock.Now(),
-                                 true, 2, 4, 6, 3, 1, 2, true);
+                                 true, 2, 4, 3, 1, 2, true);
 }
 
 // Test getting and commiting the score works correctly with different
@@ -246,7 +240,7 @@ TEST_F(MediaEngagementScoreTest, ContentSettingsMultiOrigin) {
 
   // Verify the score is originally zero, try incrementing and storing
   // the score.
-  VerifyScore(score, 0, 0, base::Time(), false, 0, 0, 0, 0, 0, 0);
+  VerifyScore(score, 0, 0, base::Time(), false, 0, 0, 0, 0, 0);
   score->IncrementVisits();
   UpdateScore(score);
   score->Commit();
@@ -262,11 +256,9 @@ TEST_F(MediaEngagementScoreTest, ContentSettingsMultiOrigin) {
       new MediaEngagementScore(&test_clock, same_origin, settings_map);
   MediaEngagementScore* different_origin_score =
       new MediaEngagementScore(&test_clock, different_origin, settings_map);
-  VerifyScore(new_score, 2, 1, test_clock.Now(), false, 1, 1, 1, 0, 1, 1);
-  VerifyScore(same_origin_score, 2, 1, test_clock.Now(), false, 1, 1, 1, 0, 1,
-              1);
-  VerifyScore(different_origin_score, 0, 0, base::Time(), false, 0, 0, 0, 0, 0,
-              0);
+  VerifyScore(new_score, 2, 1, test_clock.Now(), false, 1, 1, 0, 1, 1);
+  VerifyScore(same_origin_score, 2, 1, test_clock.Now(), false, 1, 1, 0, 1, 1);
+  VerifyScore(different_origin_score, 0, 0, base::Time(), false, 0, 0, 0, 0, 0);
 
   delete score;
   delete new_score;
@@ -282,7 +274,6 @@ TEST_F(MediaEngagementScoreTest, ContentSettings) {
   int example_media_playbacks = 5;
   int example_audible_playbacks = 3;
   int example_significant_playbacks = 5;
-  int example_visits_with_media_tags = 20;
   int example_high_score_changes = 1;
   int example_media_element_playbacks = 1;
   int example_audio_context_playbacks = 3;
@@ -301,8 +292,6 @@ TEST_F(MediaEngagementScoreTest, ContentSettings) {
                          example_audible_playbacks);
   score_dict->SetInteger(MediaEngagementScore::kSignificantPlaybacksKey,
                          example_significant_playbacks);
-  score_dict->SetInteger(MediaEngagementScore::kVisitsWithMediaTagKey,
-                         example_visits_with_media_tags);
   score_dict->SetInteger(MediaEngagementScore::kHighScoreChanges,
                          example_high_score_changes);
   score_dict->SetInteger(MediaEngagementScore::kSignificantMediaPlaybacksKey,
@@ -323,8 +312,6 @@ TEST_F(MediaEngagementScoreTest, ContentSettings) {
   EXPECT_FALSE(score->high_score());
   EXPECT_EQ(score->audible_playbacks(), example_audible_playbacks);
   EXPECT_EQ(score->significant_playbacks(), example_significant_playbacks);
-  EXPECT_EQ(score->visits_with_media_tag(), example_visits_with_media_tags);
-  EXPECT_EQ(score->visits_with_media_tag(), example_visits_with_media_tags);
   EXPECT_EQ(score->high_score_changes(), example_high_score_changes);
   EXPECT_EQ(score->media_element_playbacks(), example_media_element_playbacks);
   EXPECT_EQ(score->audio_context_playbacks(), example_audio_context_playbacks);
@@ -341,7 +328,6 @@ TEST_F(MediaEngagementScoreTest, ContentSettings) {
   bool stored_has_high_score;
   int stored_audible_playbacks;
   int stored_significant_playbacks;
-  int stored_visits_with_media_tag;
   int stored_high_score_changes;
   int stored_media_element_playbacks;
   int stored_audio_context_playbacks;
@@ -360,8 +346,6 @@ TEST_F(MediaEngagementScoreTest, ContentSettings) {
                      &stored_audible_playbacks);
   values->GetInteger(MediaEngagementScore::kSignificantPlaybacksKey,
                      &stored_significant_playbacks);
-  values->GetInteger(MediaEngagementScore::kVisitsWithMediaTagKey,
-                     &stored_visits_with_media_tag);
   values->GetInteger(MediaEngagementScore::kHighScoreChanges,
                      &stored_high_score_changes);
   values->GetInteger(MediaEngagementScore::kSignificantMediaPlaybacksKey,
@@ -374,7 +358,6 @@ TEST_F(MediaEngagementScoreTest, ContentSettings) {
             test_clock.Now().ToInternalValue());
   EXPECT_EQ(stored_audible_playbacks, example_audible_playbacks + 1);
   EXPECT_EQ(stored_significant_playbacks, example_significant_playbacks + 1);
-  EXPECT_EQ(stored_visits_with_media_tag, example_visits_with_media_tags + 1);
   EXPECT_TRUE(stored_has_high_score);
   EXPECT_EQ(stored_high_score_changes, example_high_score_changes + 1);
   EXPECT_EQ(stored_media_element_playbacks,
@@ -419,7 +402,7 @@ TEST_F(MediaEngagementScoreTest, HighScoreLegacy_High) {
   {
     std::unique_ptr<MediaEngagementScore> score(
         new MediaEngagementScore(&test_clock, origin, settings_map));
-    VerifyScore(score.get(), 20, 6, base::Time(), true, 0, 0, 0, 1, 6, 0);
+    VerifyScore(score.get(), 20, 6, base::Time(), true, 0, 0, 1, 6, 0);
   }
 }
 
@@ -442,7 +425,7 @@ TEST_F(MediaEngagementScoreTest, HighScoreLegacy_Low) {
   {
     std::unique_ptr<MediaEngagementScore> score(
         new MediaEngagementScore(&test_clock, origin, settings_map));
-    VerifyScore(score.get(), 20, 4, base::Time(), false, 0, 0, 0, 0, 4, 0);
+    VerifyScore(score.get(), 20, 4, base::Time(), false, 0, 0, 0, 4, 0);
   }
 }
 
@@ -735,12 +718,39 @@ TEST_F(MediaEngagementScoreTest, PopulatedDictionary_HTTPSOnly) {
   dict->SetBoolean(MediaEngagementScore::kHasHighScoreKey, true);
   dict->SetInteger(MediaEngagementScore::kAudiblePlaybacksKey, 2);
   dict->SetInteger(MediaEngagementScore::kSignificantPlaybacksKey, 4);
-  dict->SetInteger(MediaEngagementScore::kVisitsWithMediaTagKey, 6);
   dict->SetInteger(MediaEngagementScore::kHighScoreChanges, 3);
   dict->SetInteger(MediaEngagementScore::kSignificantMediaPlaybacksKey, 1);
   dict->SetInteger(MediaEngagementScore::kSignificantAudioContextPlaybacksKey,
                    2);
 
   TestScoreInitializesAndUpdates(std::move(dict), 0, 0, base::Time(), false, 0,
-                                 0, 0, 0, 0, 0, false);
+                                 0, 0, 0, 0, false);
+}
+
+TEST_F(MediaEngagementScoreTest, DoNotStoreDeprecatedFields) {
+  constexpr char kVisitsWithMediaTag[] = "visitsWithMediaTag";
+
+  // Store data with deprecated fields in content settings.
+  url::Origin origin = url::Origin::Create(GURL("https://www.google.com"));
+  HostContentSettingsMap* settings_map =
+      HostContentSettingsMapFactory::GetForProfile(profile());
+  std::unique_ptr<base::DictionaryValue> score_dict =
+      std::make_unique<base::DictionaryValue>();
+  score_dict->SetInteger(kVisitsWithMediaTag, 10);
+  settings_map->SetWebsiteSettingDefaultScope(
+      origin.GetURL(), GURL(), CONTENT_SETTINGS_TYPE_MEDIA_ENGAGEMENT,
+      content_settings::ResourceIdentifier(), std::move(score_dict));
+
+  // Run the data through media engagement score.
+  auto score =
+      std::make_unique<MediaEngagementScore>(&test_clock, origin, settings_map);
+  UpdateScore(score.get());
+  score->Commit();
+
+  // Check the deprecated fields have been dropped.
+  std::unique_ptr<base::DictionaryValue> values =
+      base::DictionaryValue::From(settings_map->GetWebsiteSetting(
+          origin.GetURL(), GURL(), CONTENT_SETTINGS_TYPE_MEDIA_ENGAGEMENT,
+          content_settings::ResourceIdentifier(), nullptr));
+  EXPECT_FALSE(values->HasKey(kVisitsWithMediaTag));
 }