Media Engagement: in incognito, properly override regular profile data.

This is fixing a bug where methods returning all scores would
sometimes have two entries when the incognito profile received new MEI
related events. This is due to some unexpected behaviour from content
settings (in `GetSettingsForOneType`).

Bug: 807269
Change-Id: Id6b8be8ddd2dac6e1b1fec1922896d760e78d31f
Reviewed-on: https://chromium-review.googlesource.com/893260Reviewed-by: Tommy Steimel <steimel@chromium.org>
Commit-Queue: Mounir Lamouri (slow) <mlamouri@chromium.org>
Cr-Commit-Position: refs/heads/master@{#532867}

chrome/browser/media/media_engagement_service.cc

@@ -290,6 +290,11 @@ std::vector<MediaEngagementScore> MediaEngagementService::GetAllStoredScores()
                                   content_settings::ResourceIdentifier(),
                                   &content_settings);
 
+  // `GetSettingsForOneType` mixes incognito and non-incognito results in
+  // incognito profiles creating duplicates. The incognito results are first so
+  // we should discard the results following.
+  std::map<GURL, const ContentSettingPatternSource*> filtered_results;
+
   for (const auto& site : content_settings) {
     GURL origin(site.primary_pattern.ToString());
     if (!origin.is_valid()) {
@@ -297,8 +302,21 @@ std::vector<MediaEngagementScore> MediaEngagementService::GetAllStoredScores()
       continue;
     }
 
+    const auto& result = filtered_results.find(origin);
+    if (result != filtered_results.end()) {
+      DCHECK(result->second->incognito && !site.incognito);
+      continue;
+    }
+
+    filtered_results[origin] = &site;
+  }
+
+  for (const auto& it : filtered_results) {
+    const auto& origin = it.first;
+    auto* const site = it.second;
+
     std::unique_ptr<base::Value> clone =
-        std::make_unique<base::Value>(site.setting_value->Clone());
+        std::make_unique<base::Value>(site->setting_value->Clone());
 
     data.push_back(MediaEngagementScore(
         clock_, origin, base::DictionaryValue::From(std::move(clone)),

chrome/browser/media/media_engagement_service_unittest.cc

@@ -209,6 +209,15 @@ class MediaEngagementServiceTest : public ChromeRenderViewHostTestHarness {
 
   void SetSchemaVersion(int version) { service_->SetSchemaVersion(version); }
 
+  std::vector<MediaEngagementScore> GetAllStoredScores(
+      const MediaEngagementService* service) const {
+    return service->GetAllStoredScores();
+  }
+
+  std::vector<MediaEngagementScore> GetAllStoredScores() const {
+    return GetAllStoredScores(service_.get());
+  }
+
  private:
   base::SimpleTestClock test_clock_;
 
@@ -299,6 +308,79 @@ TEST_F(MediaEngagementServiceTest, IncognitoEngagementService) {
   ExpectScores(url4, 0.0, 1, 1, Now());
 }
 
+TEST_F(MediaEngagementServiceTest, IncognitoOverrideRegularProfile) {
+  const GURL kUrl1("https://example.org");
+  const GURL kUrl2("https://example.com");
+
+  SetScores(kUrl1, MediaEngagementScore::GetScoreMinVisits(), 1);
+  SetScores(kUrl2, 1, 0);
+
+  ExpectScores(kUrl1, 0.05, MediaEngagementScore::GetScoreMinVisits(), 1,
+               TimeNotSet());
+  ExpectScores(kUrl2, 0.0, 1, 0, TimeNotSet());
+
+  MediaEngagementService* incognito_service =
+      MediaEngagementService::Get(profile()->GetOffTheRecordProfile());
+  ExpectScores(incognito_service, kUrl1, 0.05,
+               MediaEngagementScore::GetScoreMinVisits(), 1, TimeNotSet());
+  ExpectScores(incognito_service, kUrl2, 0.0, 1, 0, TimeNotSet());
+
+  // Scores should be the same in incognito and regular profile.
+  {
+    std::vector<std::pair<GURL, double>> kExpectedResults = {
+        {kUrl2, 0.0}, {kUrl1, 0.05},
+    };
+
+    const auto& scores = GetAllStoredScores();
+    const auto& incognito_scores = GetAllStoredScores(incognito_service);
+
+    EXPECT_EQ(kExpectedResults.size(), scores.size());
+    EXPECT_EQ(kExpectedResults.size(), incognito_scores.size());
+
+    for (size_t i = 0; i < scores.size(); ++i) {
+      EXPECT_EQ(kExpectedResults[i].first, scores[i].origin());
+      EXPECT_EQ(kExpectedResults[i].second, scores[i].actual_score());
+
+      EXPECT_EQ(kExpectedResults[i].first, incognito_scores[i].origin());
+      EXPECT_EQ(kExpectedResults[i].second, incognito_scores[i].actual_score());
+    }
+  }
+
+  incognito_service->RecordVisit(kUrl1);
+  incognito_service->RecordPlayback(kUrl2);
+
+  // Score shouldn't have changed in regular profile.
+  {
+    std::vector<std::pair<GURL, double>> kExpectedResults = {
+        {kUrl2, 0.0}, {kUrl1, 0.05},
+    };
+
+    const auto& scores = GetAllStoredScores();
+    EXPECT_EQ(kExpectedResults.size(), scores.size());
+
+    for (size_t i = 0; i < scores.size(); ++i) {
+      EXPECT_EQ(kExpectedResults[i].first, scores[i].origin());
+      EXPECT_EQ(kExpectedResults[i].second, scores[i].actual_score());
+    }
+  }
+
+  // Incognito scores should have the same number of entries but have new
+  // values.
+  {
+    std::vector<std::pair<GURL, double>> kExpectedResults = {
+        {kUrl2, 0.0}, {kUrl1, 1.0 / 21.0},
+    };
+
+    const auto& scores = GetAllStoredScores(incognito_service);
+    EXPECT_EQ(kExpectedResults.size(), scores.size());
+
+    for (size_t i = 0; i < scores.size(); ++i) {
+      EXPECT_EQ(kExpectedResults[i].first, scores[i].origin());
+      EXPECT_EQ(kExpectedResults[i].second, scores[i].actual_score());
+    }
+  }
+}
+
 TEST_F(MediaEngagementServiceTest, CleanupOriginsOnHistoryDeletion) {
   GURL origin1("http://www.google.com/");
   GURL origin1a("http://www.google.com/search?q=asdf");