[Media History] Do not save if pref enabled

Do not save data if the SavingBrowserHistoryDisabled
pref/policy is enabled.

BUG=1051239

Change-Id: I81f64b1bb257039e7c6d5081ad94572986b9f674
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2106465Reviewed-by: Tommy Steimel <steimel@chromium.org>
Commit-Queue: Becca Hughes <beccahughes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#751164}

chrome/browser/media/history/media_history_browsertest.cc

@@ -20,6 +20,8 @@
 #include "chrome/browser/ui/browser.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
+#include "components/history/core/common/pref_names.h"
+#include "components/prefs/pref_service.h"
 #include "content/public/browser/media_session.h"
 #include "content/public/test/test_utils.h"
 #include "media/base/media_switches.h"
@@ -34,11 +36,21 @@ namespace {
 constexpr base::TimeDelta kTestClipDuration =
     base::TimeDelta::FromMilliseconds(26771);
 
+enum class TestState {
+  kNormal,
+
+  // Runs the test in incognito mode.
+  kIncognito,
+
+  // Runs the test with the "SavingBrowserHistoryDisabled" policy enabled.
+  kSavingBrowserHistoryDisabled,
+};
+
 }  // namespace
 
 // Runs the test with a param to signify the profile being incognito if true.
 class MediaHistoryBrowserTest : public InProcessBrowserTest,
-                                public testing::WithParamInterface<bool> {
+                                public testing::WithParamInterface<TestState> {
  public:
   MediaHistoryBrowserTest() = default;
   ~MediaHistoryBrowserTest() override = default;
@@ -290,16 +302,28 @@ class MediaHistoryBrowserTest : public InProcessBrowserTest,
   }
 
   Browser* CreateBrowserFromParam() {
-    return GetParam() ? CreateIncognitoBrowser() : browser();
+    if (GetParam() == TestState::kIncognito) {
+      return CreateIncognitoBrowser();
+    } else if (GetParam() == TestState::kSavingBrowserHistoryDisabled) {
+      browser()->profile()->GetPrefs()->SetBoolean(
+          prefs::kSavingBrowserHistoryDisabled, true);
+    }
+
+    return browser();
   }
 
-  bool IsReadOnly() const { return GetParam(); }
+  bool IsReadOnly() const { return GetParam() != TestState::kNormal; }
 
  private:
   base::test::ScopedFeatureList scoped_feature_list_;
 };
 
-INSTANTIATE_TEST_SUITE_P(All, MediaHistoryBrowserTest, testing::Bool());
+INSTANTIATE_TEST_SUITE_P(
+    All,
+    MediaHistoryBrowserTest,
+    testing::Values(TestState::kNormal,
+                    TestState::kIncognito,
+                    TestState::kSavingBrowserHistoryDisabled));
 
 IN_PROC_BROWSER_TEST_P(MediaHistoryBrowserTest,
                        RecordMediaSession_OnNavigate_Incomplete) {

chrome/browser/media/history/media_history_keyed_service.cc

@@ -11,6 +11,8 @@
 #include "chrome/browser/media/history/media_history_keyed_service_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "components/history/core/browser/history_service.h"
+#include "components/history/core/common/pref_names.h"
+#include "components/prefs/pref_service.h"
 #include "content/public/browser/browser_context.h"
 #include "media/base/media_switches.h"
 
@@ -18,13 +20,17 @@ namespace media_history {
 
 // StoreHolder will in most cases hold a local MediaHistoryStore. However, for
 // OTR profiles we hold a pointer to the original profile store. When accessing
-// MediaHistoryStore you should use GetForRead for read operations and
-// GetForWrite for write operations. This can be null if the store is read only.
+// MediaHistoryStore you should use GetForRead for read operations,
+// GetForWrite for write operations and GetForDelete for delete operations.
+// These can be null if the store is read only or we disable storing browsing
+// history.
 class MediaHistoryKeyedService::StoreHolder {
  public:
-  explicit StoreHolder(std::unique_ptr<MediaHistoryStore> local)
-      : local_(std::move(local)) {}
-  explicit StoreHolder(MediaHistoryKeyedService* remote) : remote_(remote) {}
+  explicit StoreHolder(Profile* profile,
+                       std::unique_ptr<MediaHistoryStore> local)
+      : profile_(profile), local_(std::move(local)) {}
+  explicit StoreHolder(Profile* profile, MediaHistoryKeyedService* remote)
+      : profile_(profile), remote_(remote) {}
 
   ~StoreHolder() = default;
   StoreHolder(const StoreHolder& t) = delete;
@@ -37,12 +43,21 @@ class MediaHistoryKeyedService::StoreHolder {
   }
 
   MediaHistoryStore* GetForWrite() {
+    if (profile_->GetPrefs()->GetBoolean(prefs::kSavingBrowserHistoryDisabled))
+      return nullptr;
+    if (local_)
+      return local_.get();
+    return nullptr;
+  }
+
+  MediaHistoryStore* GetForDelete() {
     if (local_)
       return local_.get();
     return nullptr;
   }
 
  private:
+  Profile* profile_;
   std::unique_ptr<MediaHistoryStore> local_;
   MediaHistoryKeyedService* remote_ = nullptr;
 };
@@ -60,14 +75,15 @@ MediaHistoryKeyedService::MediaHistoryKeyedService(Profile* profile)
         MediaHistoryKeyedService::Get(profile->GetOriginalProfile());
     DCHECK(original);
 
-    store_ = std::make_unique<StoreHolder>(original);
+    store_ = std::make_unique<StoreHolder>(profile, original);
   } else {
     auto db_task_runner = base::ThreadPool::CreateUpdateableSequencedTaskRunner(
         {base::MayBlock(), base::TaskPriority::USER_VISIBLE,
          base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN});
 
-    store_ = std::make_unique<StoreHolder>(std::make_unique<MediaHistoryStore>(
-        profile_, std::move(db_task_runner)));
+    store_ = std::make_unique<StoreHolder>(
+        profile_, std::make_unique<MediaHistoryStore>(
+                      profile_, std::move(db_task_runner)));
   }
 }
 
@@ -93,7 +109,7 @@ void MediaHistoryKeyedService::OnURLsDeleted(
     history::HistoryService* history_service,
     const history::DeletionInfo& deletion_info) {
   // The store might not always be writable.
-  auto* store = store_->GetForWrite();
+  auto* store = store_->GetForDelete();
   if (!store)
     return;
 

chrome/browser/media/history/media_history_keyed_service_unittest.cc

@@ -23,7 +23,9 @@
 #include "chrome/test/base/testing_profile.h"
 #include "components/history/core/browser/history_database_params.h"
 #include "components/history/core/browser/history_service.h"
+#include "components/history/core/common/pref_names.h"
 #include "components/history/core/test/test_history_database.h"
+#include "components/prefs/pref_service.h"
 #include "content/public/browser/media_player_watch_time.h"
 #include "content/public/test/test_utils.h"
 #include "services/media_session/public/cpp/media_image.h"
@@ -47,9 +49,18 @@ std::unique_ptr<KeyedService> BuildTestHistoryService(
   return service;
 }
 
+enum class TestState {
+  kNormal,
+
+  // Runs the test with the "SavingBrowserHistoryDisabled" policy enabled.
+  kSavingBrowserHistoryDisabled,
+};
+
 }  // namespace
 
-class MediaHistoryKeyedServiceTest : public ChromeRenderViewHostTestHarness {
+class MediaHistoryKeyedServiceTest
+    : public ChromeRenderViewHostTestHarness,
+      public testing::WithParamInterface<TestState> {
  public:
   void SetUp() override {
     scoped_feature_list_.InitWithFeatures(
@@ -84,6 +95,9 @@ class MediaHistoryKeyedServiceTest : public ChromeRenderViewHostTestHarness {
   }
 
   void TearDown() override {
+    profile()->GetPrefs()->SetBoolean(prefs::kSavingBrowserHistoryDisabled,
+                                      false);
+
     service_->Shutdown();
 
     // Tests that run a history service that uses the mock task runner for
@@ -152,6 +166,14 @@ class MediaHistoryKeyedServiceTest : public ChromeRenderViewHostTestHarness {
     return images;
   }
 
+  void MaybeSetSavingBrowsingHistoryDisabled() {
+    if (GetParam() != TestState::kSavingBrowserHistoryDisabled)
+      return;
+
+    profile()->GetPrefs()->SetBoolean(prefs::kSavingBrowserHistoryDisabled,
+                                      true);
+  }
+
   scoped_refptr<base::TestMockTimeTaskRunner> mock_time_task_runner_;
 
  private:
@@ -162,7 +184,13 @@ class MediaHistoryKeyedServiceTest : public ChromeRenderViewHostTestHarness {
   base::test::ScopedFeatureList scoped_feature_list_;
 };
 
-TEST_F(MediaHistoryKeyedServiceTest, CleanUpDatabaseWhenHistoryIsDeleted) {
+INSTANTIATE_TEST_SUITE_P(
+    All,
+    MediaHistoryKeyedServiceTest,
+    testing::Values(TestState::kNormal,
+                    TestState::kSavingBrowserHistoryDisabled));
+
+TEST_P(MediaHistoryKeyedServiceTest, CleanUpDatabaseWhenHistoryIsDeleted) {
   history::HistoryService* history = HistoryServiceFactory::GetForProfile(
       profile(), ServiceAccessType::IMPLICIT_ACCESS);
   GURL url("http://google.com/test");
@@ -184,6 +212,8 @@ TEST_F(MediaHistoryKeyedServiceTest, CleanUpDatabaseWhenHistoryIsDeleted) {
 
   EXPECT_EQ(2, GetUserDataTableRowCount());
 
+  MaybeSetSavingBrowsingHistoryDisabled();
+
   {
     base::CancelableTaskTracker task_tracker;
 
@@ -201,7 +231,7 @@ TEST_F(MediaHistoryKeyedServiceTest, CleanUpDatabaseWhenHistoryIsDeleted) {
   EXPECT_EQ(0, GetUserDataTableRowCount());
 }
 
-TEST_F(MediaHistoryKeyedServiceTest, CleanUpDatabaseWhenOriginIsDeleted) {
+TEST_P(MediaHistoryKeyedServiceTest, CleanUpDatabaseWhenOriginIsDeleted) {
   history::HistoryService* history = HistoryServiceFactory::GetForProfile(
       profile(), ServiceAccessType::IMPLICIT_ACCESS);
 
@@ -340,6 +370,8 @@ TEST_F(MediaHistoryKeyedServiceTest, CleanUpDatabaseWhenOriginIsDeleted) {
   EXPECT_EQ(all_urls, GetURLsInTable(MediaHistorySessionTable::kTableName));
   EXPECT_EQ(images, GetURLsInTable(MediaHistoryImagesTable::kTableName));
 
+  MaybeSetSavingBrowsingHistoryDisabled();
+
   {
     base::CancelableTaskTracker task_tracker;
 

chrome/browser/media/history/media_history_store_unittest.cc

@@ -26,7 +26,9 @@
 #include "chrome/test/base/testing_profile.h"
 #include "components/history/core/browser/history_database_params.h"
 #include "components/history/core/browser/history_service.h"
+#include "components/history/core/common/pref_names.h"
 #include "components/history/core/test/test_history_database.h"
+#include "components/prefs/pref_service.h"
 #include "content/public/browser/media_player_watch_time.h"
 #include "content/public/test/browser_task_environment.h"
 #include "content/public/test/test_utils.h"
@@ -56,11 +58,22 @@ std::unique_ptr<KeyedService> BuildTestHistoryService(
   return service;
 }
 
+enum class TestState {
+  kNormal,
+
+  // Runs the test in incognito mode.
+  kIncognito,
+
+  // Runs the test with the "SavingBrowserHistoryDisabled" policy enabled.
+  kSavingBrowserHistoryDisabled,
+};
+
 }  // namespace
 
 // Runs the test with a param to signify the profile being incognito if true.
-class MediaHistoryStoreUnitTest : public testing::Test,
-                                  public testing::WithParamInterface<bool> {
+class MediaHistoryStoreUnitTest
+    : public testing::Test,
+      public testing::WithParamInterface<TestState> {
  public:
   MediaHistoryStoreUnitTest() = default;
   void SetUp() override {
@@ -71,6 +84,11 @@ class MediaHistoryStoreUnitTest : public testing::Test,
     g_temp_history_dir = temp_dir_.GetPath();
     profile_ = profile_builder.Build();
 
+    if (GetParam() == TestState::kSavingBrowserHistoryDisabled) {
+      profile_->GetPrefs()->SetBoolean(prefs::kSavingBrowserHistoryDisabled,
+                                       true);
+    }
+
     HistoryServiceFactory::GetInstance()->SetTestingFactory(
         profile_.get(), base::BindRepeating(&BuildTestHistoryService));
 
@@ -162,7 +180,7 @@ class MediaHistoryStoreUnitTest : public testing::Test,
   MediaHistoryKeyedService* service() const {
     // If the param is true then we use the OTR service to simulate being in
     // incognito.
-    if (GetParam())
+    if (GetParam() == TestState::kIncognito)
       return otr_service();
 
     return MediaHistoryKeyedService::Get(profile_.get());
@@ -170,7 +188,7 @@ class MediaHistoryStoreUnitTest : public testing::Test,
 
   MediaHistoryKeyedService* otr_service() const { return otr_service_.get(); }
 
-  bool IsReadOnly() const { return GetParam(); }
+  bool IsReadOnly() const { return GetParam() != TestState::kNormal; }
 
  private:
   base::ScopedTempDir temp_dir_;
@@ -185,7 +203,12 @@ class MediaHistoryStoreUnitTest : public testing::Test,
   std::unique_ptr<TestingProfile> profile_;
 };
 
-INSTANTIATE_TEST_SUITE_P(All, MediaHistoryStoreUnitTest, testing::Bool());
+INSTANTIATE_TEST_SUITE_P(
+    All,
+    MediaHistoryStoreUnitTest,
+    testing::Values(TestState::kNormal,
+                    TestState::kIncognito,
+                    TestState::kSavingBrowserHistoryDisabled));
 
 TEST_P(MediaHistoryStoreUnitTest, CreateDatabaseTables) {
   ASSERT_TRUE(GetDB().DoesTableExist("origin"));
@@ -601,7 +624,10 @@ class MediaHistoryStoreFeedsTest : public MediaHistoryStoreUnitTest {
   base::test::ScopedFeatureList features_;
 };
 
-INSTANTIATE_TEST_SUITE_P(All, MediaHistoryStoreFeedsTest, testing::Bool());
+INSTANTIATE_TEST_SUITE_P(All,
+                         MediaHistoryStoreFeedsTest,
+                         testing::Values(TestState::kNormal,
+                                         TestState::kIncognito));
 
 TEST_P(MediaHistoryStoreFeedsTest, CreateDatabaseTables) {
   ASSERT_TRUE(GetDB().DoesTableExist("mediaFeed"));