Share store for incognito and regular profile for hints

Bug: 1158343,1151087
Change-Id: Icb9c055daafe808f727bc7cbd372336c37bdccf7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2601637Reviewed-by: Michael Crouse <mcrouse@chromium.org>
Commit-Queue: Sophie Chang <sophiechang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#839135}

chrome/browser/optimization_guide/android/optimization_guide_bridge_unittest.cc

@@ -15,7 +15,6 @@
 #include "chrome/test/base/testing_browser_process.h"
 #include "chrome/test/base/testing_profile.h"
 #include "chrome/test/base/testing_profile_manager.h"
-#include "components/leveldb_proto/public/proto_database_provider.h"
 #include "components/optimization_guide/optimization_guide_prefs.h"
 #include "components/optimization_guide/optimization_guide_service.h"
 #include "components/prefs/pref_service.h"
@@ -38,14 +37,11 @@ class MockOptimizationGuideHintsManager : public OptimizationGuideHintsManager {
   MockOptimizationGuideHintsManager(
       optimization_guide::OptimizationGuideService* optimization_guide_service,
       Profile* profile,
-      base::FilePath file_path,
-      leveldb_proto::ProtoDatabaseProvider* db_provider,
       PrefService* pref_service)
       : OptimizationGuideHintsManager(optimization_guide_service,
                                       profile,
-                                      file_path,
                                       pref_service,
-                                      db_provider,
+                                      /*hint_store=*/nullptr,
                                       /*top_host_provider=*/nullptr,
                                       /*url_loader_factory=*/nullptr) {}
   ~MockOptimizationGuideHintsManager() override = default;
@@ -98,18 +94,14 @@ class OptimizationGuideBridgeTest : public testing::Test {
     optimization_guide_service_ =
         std::make_unique<optimization_guide::OptimizationGuideService>(
             task_environment_.GetMainThreadTaskRunner());
-    db_provider_ = std::make_unique<leveldb_proto::ProtoDatabaseProvider>(
-        temp_dir_.GetPath());
     optimization_guide_hints_manager_ =
         std::make_unique<MockOptimizationGuideHintsManager>(
-            optimization_guide_service_.get(), profile_, temp_dir_.GetPath(),
-            db_provider_.get(), pref_service_.get());
+            optimization_guide_service_.get(), profile_, pref_service_.get());
   }
 
   void TearDown() override {
     optimization_guide_hints_manager_->Shutdown();
     optimization_guide_hints_manager_.reset();
-    db_provider_.reset();
     optimization_guide_service_.reset();
   }
 
@@ -133,7 +125,6 @@ class OptimizationGuideBridgeTest : public testing::Test {
   TestingProfile* profile_;
   std::unique_ptr<optimization_guide::OptimizationGuideService>
       optimization_guide_service_;
-  std::unique_ptr<leveldb_proto::ProtoDatabaseProvider> db_provider_;
   base::ScopedTempDir temp_dir_;
   std::unique_ptr<TestingPrefServiceSimple> pref_service_;
 };

chrome/browser/optimization_guide/optimization_guide_hints_manager.cc

@@ -253,9 +253,8 @@ bool ShouldIgnoreNewlyRegisteredOptimizationType(
 OptimizationGuideHintsManager::OptimizationGuideHintsManager(
     optimization_guide::OptimizationGuideService* optimization_guide_service,
     Profile* profile,
-    const base::FilePath& profile_path,
     PrefService* pref_service,
-    leveldb_proto::ProtoDatabaseProvider* database_provider,
+    optimization_guide::OptimizationGuideStore* hint_store,
     optimization_guide::TopHostProvider* top_host_provider,
     scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory)
     : optimization_guide_service_(optimization_guide_service),
@@ -264,13 +263,7 @@ OptimizationGuideHintsManager::OptimizationGuideHintsManager(
       profile_(profile),
       pref_service_(pref_service),
       hint_cache_(std::make_unique<optimization_guide::HintCache>(
-          optimization_guide::features::ShouldPersistHintsToDisk()
-              ? std::make_unique<optimization_guide::OptimizationGuideStore>(
-                    database_provider,
-                    profile_path.AddExtensionASCII(
-                        optimization_guide::kOptimizationGuideHintStore),
-                    background_task_runner_)
-              : nullptr,
+          hint_store,
           optimization_guide::features::MaxHostKeyedHintCacheSize())),
       page_navigation_hints_fetchers_(
           optimization_guide::features::MaxConcurrentPageNavigationFetches()),
@@ -346,7 +339,9 @@ void OptimizationGuideHintsManager::OnHintsComponentAvailable(
   }
 
   std::unique_ptr<optimization_guide::StoreUpdateData> update_data =
-      hint_cache_->MaybeCreateUpdateDataForComponentHints(info.version);
+      profile_->IsOffTheRecord()
+          ? nullptr
+          : hint_cache_->MaybeCreateUpdateDataForComponentHints(info.version);
 
   // Processes the hints from the newly available component on a background
   // thread, providing a StoreUpdateData for component update from the hint
@@ -399,7 +394,8 @@ OptimizationGuideHintsManager::ProcessHintsComponent(
   // aren't hints sent down via the component, but we need to figure out
   // threading since these hints are now stored in memory prior to being
   // persisted.
-  if (update_data) {
+  // Don't store hints in the store if it's off the record.
+  if (update_data && !profile_->IsOffTheRecord()) {
     bool did_process_hints = hint_cache_->ProcessAndCacheHints(
         config->mutable_hints(), update_data.get());
     optimization_guide::RecordProcessHintsComponentResult(
@@ -504,7 +500,9 @@ void OptimizationGuideHintsManager::OnHintCacheInitialized() {
       optimization_guide::switches::ParseComponentConfigFromCommandLine();
   if (manual_config) {
     std::unique_ptr<optimization_guide::StoreUpdateData> update_data =
-        hint_cache_->MaybeCreateUpdateDataForComponentHints(
+        profile_->IsOffTheRecord()
+            ? nullptr
+            : hint_cache_->MaybeCreateUpdateDataForComponentHints(
                   base::Version(kManualConfigComponentVersion));
     hint_cache_->ProcessAndCacheHints(manual_config->mutable_hints(),
                                       update_data.get());
@@ -922,8 +920,10 @@ void OptimizationGuideHintsManager::RegisterOptimizationTypes(
     base::Optional<double> value = previously_registered_opt_types->FindBoolKey(
         optimization_guide::proto::OptimizationType_Name(optimization_type));
     if (!value) {
-      if (!ShouldIgnoreNewlyRegisteredOptimizationType(optimization_type))
+      if (!profile_->IsOffTheRecord() &&
+          !ShouldIgnoreNewlyRegisteredOptimizationType(optimization_type)) {
         should_clear_hints_for_new_type_ = true;
+      }
       previously_registered_opt_types->SetBoolKey(
           optimization_guide::proto::OptimizationType_Name(optimization_type),
           true);
@@ -1259,13 +1259,13 @@ void OptimizationGuideHintsManager::OnNavigationStartOrRedirect(
     base::OnceClosure callback) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
+  LoadHintForNavigation(navigation_handle, std::move(callback));
+
   if (optimization_guide::switches::
           DisableFetchingHintsAtNavigationStartForTesting()) {
     return;
   }
 
-  LoadHintForNavigation(navigation_handle, std::move(callback));
-
   MaybeFetchHintsForNavigation(navigation_handle);
 }
 
@@ -1357,6 +1357,11 @@ void OptimizationGuideHintsManager::OnNavigationFinish(
   }
 }
 
+optimization_guide::OptimizationGuideStore*
+OptimizationGuideHintsManager::hint_store() {
+  return hint_cache_->hint_store();
+}
+
 bool OptimizationGuideHintsManager::HasAllInformationForDecisionAvailable(
     const GURL& navigation_url,
     optimization_guide::proto::OptimizationType optimization_type) {

chrome/browser/optimization_guide/optimization_guide_hints_manager.h

@@ -31,18 +31,10 @@
 #include "net/nqe/effective_connection_type.h"
 #include "services/network/public/cpp/network_quality_tracker.h"
 
-namespace base {
-class FilePath;
-}  // namespace base
-
 namespace content {
 class NavigationHandle;
 }  // namespace content
 
-namespace leveldb_proto {
-class ProtoDatabaseProvider;
-}  // namespace leveldb_proto
-
 namespace network {
 class SharedURLLoaderFactory;
 }  // namespace network
@@ -53,6 +45,7 @@ class HintsFetcherFactory;
 class OptimizationFilter;
 class OptimizationMetadata;
 class OptimizationGuideService;
+class OptimizationGuideStore;
 enum class OptimizationTargetDecision;
 enum class OptimizationTypeDecision;
 class StoreUpdateData;
@@ -71,9 +64,8 @@ class OptimizationGuideHintsManager
   OptimizationGuideHintsManager(
       optimization_guide::OptimizationGuideService* optimization_guide_service,
       Profile* profile,
-      const base::FilePath& profile_path,
       PrefService* pref_service,
-      leveldb_proto::ProtoDatabaseProvider* database_provider,
+      optimization_guide::OptimizationGuideStore* hint_store,
       optimization_guide::TopHostProvider* top_host_provider,
       scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory);
 
@@ -164,6 +156,9 @@ class OptimizationGuideHintsManager
   // |navigation_redirect_chain| has finished.
   void OnNavigationFinish(const std::vector<GURL>& navigation_redirect_chain);
 
+  // Returns the persistent store for |this|.
+  optimization_guide::OptimizationGuideStore* hint_store();
+
   // Add hints to the cache with the provided metadata. For testing only.
   void AddHintForTesting(
       const GURL& url,

chrome/browser/optimization_guide/optimization_guide_hints_manager_unittest.cc

@@ -30,6 +30,7 @@
 #include "components/optimization_guide/optimization_guide_features.h"
 #include "components/optimization_guide/optimization_guide_prefs.h"
 #include "components/optimization_guide/optimization_guide_service.h"
+#include "components/optimization_guide/optimization_guide_store.h"
 #include "components/optimization_guide/optimization_guide_switches.h"
 #include "components/optimization_guide/proto_database_provider_test_base.h"
 #include "components/optimization_guide/top_host_provider.h"
@@ -302,9 +303,12 @@ class OptimizationGuideHintsManagerTest
         base::MakeRefCounted<network::WeakWrapperSharedURLLoaderFactory>(
             &test_url_loader_factory_);
 
+    hint_store_ = std::make_unique<optimization_guide::OptimizationGuideStore>(
+        db_provider_.get(), temp_dir(),
+        task_environment_.GetMainThreadTaskRunner());
     hints_manager_ = std::make_unique<OptimizationGuideHintsManager>(
-        optimization_guide_service_.get(), &testing_profile_, temp_dir(),
-        pref_service_.get(), db_provider_.get(), top_host_provider,
+        optimization_guide_service_.get(), &testing_profile_,
+        pref_service_.get(), hint_store_.get(), top_host_provider,
         url_loader_factory_);
     hints_manager_->SetClockForTesting(task_environment_.GetMockClock());
 
@@ -319,6 +323,7 @@ class OptimizationGuideHintsManagerTest
   void ResetHintsManager() {
     hints_manager_->Shutdown();
     hints_manager_.reset();
+    hint_store_.reset();
     RunUntilIdle();
   }
 
@@ -442,6 +447,7 @@ class OptimizationGuideHintsManagerTest
   base::test::ScopedFeatureList scoped_feature_list_;
   TestingProfile testing_profile_;
   std::unique_ptr<content::TestWebContentsFactory> web_contents_factory_;
+  std::unique_ptr<optimization_guide::OptimizationGuideStore> hint_store_;
   std::unique_ptr<OptimizationGuideHintsManager> hints_manager_;
   std::unique_ptr<TestOptimizationGuideService> optimization_guide_service_;
   std::unique_ptr<TestingPrefServiceSimple> pref_service_;

chrome/browser/optimization_guide/optimization_guide_keyed_service.cc

@@ -126,6 +126,7 @@ void OptimizationGuideKeyedService::Initialize() {
   // For incognito profiles, we act in "read-only" mode of the original
   // profile's store and do not fetch any new hints or models.
   scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory;
+  optimization_guide::OptimizationGuideStore* hint_store;
   optimization_guide::OptimizationGuideStore*
       prediction_model_and_features_store;
   if (profile->IsOffTheRecord()) {
@@ -133,6 +134,7 @@ void OptimizationGuideKeyedService::Initialize() {
         OptimizationGuideKeyedServiceFactory::GetForProfile(
             profile->GetOriginalProfile());
     DCHECK(original_ogks);
+    hint_store = original_ogks->GetHintsManager()->hint_store();
     prediction_model_and_features_store =
         original_ogks->GetPredictionManager()->model_and_features_store();
   } else {
@@ -147,6 +149,18 @@ void OptimizationGuideKeyedService::Initialize() {
     ChromeMetricsServiceAccessor::RegisterSyntheticFieldTrial(
         "SyntheticOptimizationGuideRemoteFetching",
         optimization_guide_fetching_enabled ? "Enabled" : "Disabled");
+
+    hint_store_ =
+        optimization_guide::features::ShouldPersistHintsToDisk()
+            ? std::make_unique<optimization_guide::OptimizationGuideStore>(
+                  proto_db_provider,
+                  profile_path.AddExtensionASCII(
+                      optimization_guide::kOptimizationGuideHintStore),
+                  base::ThreadPool::CreateSequencedTaskRunner(
+                      {base::MayBlock(), base::TaskPriority::BEST_EFFORT}))
+            : nullptr;
+    hint_store = hint_store_.get();
+
     prediction_model_and_features_store_ =
         std::make_unique<optimization_guide::OptimizationGuideStore>(
             proto_db_provider,
@@ -160,8 +174,8 @@ void OptimizationGuideKeyedService::Initialize() {
   }
 
   hints_manager_ = std::make_unique<OptimizationGuideHintsManager>(
-      g_browser_process->optimization_guide_service(), profile, profile_path,
-      profile->GetPrefs(), proto_db_provider, top_host_provider_.get(),
+      g_browser_process->optimization_guide_service(), profile,
+      profile->GetPrefs(), hint_store, top_host_provider_.get(),
       url_loader_factory);
   prediction_manager_ = std::make_unique<optimization_guide::PredictionManager>(
       prediction_model_and_features_store, top_host_provider_.get(),

chrome/browser/optimization_guide/optimization_guide_keyed_service.h

@@ -148,17 +148,20 @@ class OptimizationGuideKeyedService
 
   content::BrowserContext* browser_context_;
 
+  // The store of hints.
+  std::unique_ptr<optimization_guide::OptimizationGuideStore> hint_store_;
+
   // Manages the storing, loading, and fetching of hints.
   std::unique_ptr<OptimizationGuideHintsManager> hints_manager_;
 
-  // Manages the storing, loading, and evaluating of optimization target
-  // prediction models.
-  std::unique_ptr<optimization_guide::PredictionManager> prediction_manager_;
-
   // The store of optimization target prediction models and features.
   std::unique_ptr<optimization_guide::OptimizationGuideStore>
       prediction_model_and_features_store_;
 
+  // Manages the storing, loading, and evaluating of optimization target
+  // prediction models.
+  std::unique_ptr<optimization_guide::PredictionManager> prediction_manager_;
+
   // The top host provider to use for fetching information for the user's top
   // hosts. Will be null if the user has not consented to this type of browser
   // behavior.

chrome/browser/optimization_guide/optimization_guide_keyed_service_browsertest.cc

@@ -671,41 +671,68 @@ IN_PROC_BROWSER_TEST_F(
 #endif
 }
 
-// TODO(crbug/1158343): Re-enable when flake is fixed.
 IN_PROC_BROWSER_TEST_F(
     OptimizationGuideKeyedServiceDataSaverUserWithInfobarShownTest,
-    DISABLED_IncognitoCanStillReadFromComponentHints) {
-  // Instantiate off the record Optimization Guide Service.
-  OptimizationGuideKeyedServiceFactory::GetForProfile(
-      browser()->profile()->GetPrimaryOTRProfile())
-      ->RegisterOptimizationTypes({optimization_guide::proto::NOSCRIPT});
-
+    IncognitoCanStillReadFromComponentHints) {
   // Wait until initialization logic finishes running and component pushed to
   // both incognito and regular browsers.
   PushHintsComponentAndWaitForCompletion();
-  base::RunLoop().RunUntilIdle();
 
   // Set up incognito browser and incognito OptimizationGuideKeyedService
   // consumer.
   Browser* otr_browser = CreateIncognitoBrowser(browser()->profile());
-  auto otr_consumer =
-      std::make_unique<OptimizationGuideConsumerWebContentsObserver>(
-          otr_browser->tab_strip_model()->GetActiveWebContents());
-  std::unique_ptr<base::RunLoop> run_loop = std::make_unique<base::RunLoop>();
-  otr_consumer->set_callback(base::BindOnce(
-      [](base::RunLoop* run_loop,
-         optimization_guide::OptimizationGuideDecision decision,
-         const optimization_guide::OptimizationMetadata& metadata) {
-        // Should still get decision in incognito.
-        EXPECT_EQ(decision,
-                  optimization_guide::OptimizationGuideDecision::kTrue);
-        run_loop->Quit();
-      },
-      run_loop.get()));
 
-  // Navigate to a URL that has a hint from a component.
+  // Instantiate off the record Optimization Guide Service.
+  OptimizationGuideKeyedService* otr_ogks =
+      OptimizationGuideKeyedServiceFactory::GetForProfile(
+          browser()->profile()->GetPrimaryOTRProfile());
+  otr_ogks->RegisterOptimizationTypes({optimization_guide::proto::NOSCRIPT});
+
+  // Navigate to a URL that has a hint from a component and wait for that hint
+  // to have loaded.
+  base::HistogramTester histogram_tester;
   ui_test_utils::NavigateToURL(otr_browser, url_with_hints());
-  run_loop->Run();
+  RetryForHistogramUntilCountReached(histogram_tester,
+                                     "OptimizationGuide.LoadedHint.Result", 1);
+
+  EXPECT_EQ(
+      optimization_guide::OptimizationGuideDecision::kTrue,
+      otr_ogks->CanApplyOptimization(
+          url_with_hints(), optimization_guide::proto::NOSCRIPT, nullptr));
+}
+
+IN_PROC_BROWSER_TEST_F(
+    OptimizationGuideKeyedServiceDataSaverUserWithInfobarShownTest,
+    IncognitoStillProcessesBloomFilter) {
+  PushHintsComponentAndWaitForCompletion();
+
+  CreateIncognitoBrowser(browser()->profile());
+
+  // Instantiate off the record Optimization Guide Service.
+  OptimizationGuideKeyedService* otr_ogks =
+      OptimizationGuideKeyedServiceFactory::GetForProfile(
+          browser()->profile()->GetPrimaryOTRProfile());
+
+  base::HistogramTester histogram_tester;
+
+  // Register an optimization type with an optimization filter.
+  otr_ogks->RegisterOptimizationTypes(
+      {optimization_guide::proto::FAST_HOST_HINTS});
+  // Wait until filter is loaded. This histogram will record twice: once when
+  // the config is found and once when the filter is created.
+  RetryForHistogramUntilCountReached(
+      histogram_tester,
+      "OptimizationGuide.OptimizationFilterStatus.FastHostHints", 2);
+
+  EXPECT_EQ(optimization_guide::OptimizationGuideDecision::kFalse,
+            otr_ogks->CanApplyOptimization(
+                GURL("https://blockedhost.com/whatever"),
+                optimization_guide::proto::FAST_HOST_HINTS, nullptr));
+  histogram_tester.ExpectUniqueSample(
+      "OptimizationGuide.ApplyDecision.FastHostHints",
+      static_cast<int>(optimization_guide::OptimizationTypeDecision::
+                           kNotAllowedByOptimizationFilter),
+      1);
 }
 
 class OptimizationGuideKeyedServiceCommandLineOverridesTest

components/optimization_guide/hint_cache.cc

@@ -15,10 +15,9 @@
 
 namespace optimization_guide {
 
-HintCache::HintCache(
-    std::unique_ptr<OptimizationGuideStore> optimization_guide_store,
+HintCache::HintCache(OptimizationGuideStore* optimization_guide_store,
                      int max_memory_cache_host_keyed_hints)
-    : optimization_guide_store_(std::move(optimization_guide_store)),
+    : optimization_guide_store_(optimization_guide_store),
       host_keyed_cache_(max_memory_cache_host_keyed_hints),
       url_keyed_hint_cache_(features::MaxURLKeyedHintCacheSize()),
       clock_(base::DefaultClock::GetInstance()) {}

components/optimization_guide/hint_cache.h

@@ -37,8 +37,7 @@ class HintCache {
   // Construct the HintCache with an optional backing store and max host-keyed
   // cache size. If a backing store is not provided, all hints will only be
   // stored in-memory.
-  explicit HintCache(
-      std::unique_ptr<OptimizationGuideStore> optimization_guide_store,
+  explicit HintCache(OptimizationGuideStore* optimization_guide_store,
                      int max_host_keyed_memory_cache_size);
   ~HintCache();
 
@@ -138,6 +137,11 @@ class HintCache {
   // Returns whether the persistent hint store owned by this is available.
   bool IsHintStoreAvailable() const;
 
+  // Returns the persistent store for |this|.
+  optimization_guide::OptimizationGuideStore* hint_store() {
+    return optimization_guide_store_;
+  }
+
   // Override |clock_| for testing.
   void SetClockForTesting(const base::Clock* clock);
 
@@ -165,8 +169,9 @@ class HintCache {
       const OptimizationGuideStore::EntryKey& store_hint_entry_key,
       std::unique_ptr<MemoryHint> hint);
 
-  // The backing store used with this hint cache. Set during construction.
-  const std::unique_ptr<OptimizationGuideStore> optimization_guide_store_;
+  // The backing store used with this hint cache. Set during construction. Not
+  // owned. Guaranteed to outlive |this|.
+  OptimizationGuideStore* optimization_guide_store_;
 
   // The cache of host-keyed hints loaded from the store. Maps store
   // EntryKey to Hint proto. This serves two purposes:

components/optimization_guide/hint_cache_unittest.cc

@@ -52,11 +52,12 @@ class HintCacheTest : public ProtoDatabaseProviderTestBase,
                                     bool purge_existing_data = false) {
     auto database_path = temp_dir_.GetPath();
     auto database_task_runner = task_environment_.GetMainThreadTaskRunner();
-    hint_cache_ = std::make_unique<HintCache>(
+    optimization_guide_store_ =
         IsBackedByPersistentStore()
             ? std::make_unique<OptimizationGuideStore>(
                   db_provider_.get(), database_path, database_task_runner)
-            : nullptr,
+            : nullptr;
+    hint_cache_ = std::make_unique<HintCache>(optimization_guide_store_.get(),
                                               memory_cache_size);
     is_store_initialized_ = false;
     hint_cache_->Initialize(purge_existing_data,
@@ -70,6 +71,7 @@ class HintCacheTest : public ProtoDatabaseProviderTestBase,
 
   void DestroyHintCache() {
     hint_cache_.reset();
+    optimization_guide_store_.reset();
     loaded_hint_ = nullptr;
     is_store_initialized_ = false;
     are_component_hints_updated_ = false;
@@ -165,6 +167,7 @@ class HintCacheTest : public ProtoDatabaseProviderTestBase,
   base::test::TaskEnvironment task_environment_{
       base::test::TaskEnvironment::TimeSource::MOCK_TIME};
 
+  std::unique_ptr<OptimizationGuideStore> optimization_guide_store_;
   std::unique_ptr<HintCache> hint_cache_;
   const proto::Hint* loaded_hint_;