Revamp logic for clearing prefs for variations first run seed.

In additional to simplifying the code to not need to use extra
callback logic, this also makes the pref clearing happen when a
seed is imported initially, rather than later. This may resolve
an issue where the first run seed is not getting applied - if
the cause is that the pref clearing happens before the seed
gets used.

Bug: 1090968
Change-Id: Icd5d6a693ef2c0f1a5fdf2cffec2182a9a6fd5fa
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2246179Reviewed-by: Richard Coles <torne@chromium.org>
Reviewed-by: Nate Fischer <ntfschr@chromium.org>
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Commit-Queue: Alexei Svitkine <asvitkine@chromium.org>
Cr-Commit-Position: refs/heads/master@{#779483}

android_webview/browser/aw_feature_list_creator.cc

@@ -175,7 +175,6 @@ void AwFeatureListCreator::SetUpFieldTrials() {
   client_ = std::make_unique<AwVariationsServiceClient>();
   auto seed_store = std::make_unique<variations::VariationsSeedStore>(
       local_state_.get(), /*initial_seed=*/std::move(seed),
-      /*on_initial_seed_stored=*/base::DoNothing(),
       /*signature_verification_enabled=*/true);
 
   // We set the seed fetch time to when the service downloaded the seed rather

components/variations/service/variations_field_trial_creator_unittest.cc

@@ -487,7 +487,6 @@ TEST_F(FieldTrialCreatorTest, SetupFieldTrials_LoadsCountryOnFirstRun) {
   // the interaction between these two classes is what's being tested.
   auto seed_store = std::make_unique<VariationsSeedStore>(
       &prefs_, std::move(initial_seed),
-      /*on_initial_seed_stored=*/base::DoNothing(),
       /*signature_verification_enabled=*/false);
   VariationsFieldTrialCreator field_trial_creator(
       &prefs_, &variations_service_client, std::move(seed_store),

components/variations/service/variations_service.cc

@@ -267,14 +267,6 @@ std::unique_ptr<SeedResponse> MaybeImportFirstRunSeed(
   return nullptr;
 }
 
-// Called when the VariationsSeedStore first stores a seed.
-void OnInitialSeedStored() {
-#if defined(OS_ANDROID)
-  android::MarkVariationsSeedAsStored();
-  android::ClearJavaFirstRunPrefs();
-#endif
-}
-
 }  // namespace
 
 #if defined(OS_CHROMEOS)
@@ -377,7 +369,6 @@ VariationsService::VariationsService(
                            std::make_unique<VariationsSeedStore>(
                                local_state,
                                MaybeImportFirstRunSeed(local_state),
-                               base::BindOnce(&OnInitialSeedStored),
                                /*signature_verification_enabled=*/true),
                            ui_string_overrider),
       last_request_was_http_retry_(false) {

components/variations/variations_seed_store.cc

@@ -112,15 +112,13 @@ UpdateSeedDateResult GetSeedDateChangeState(
 }  // namespace
 
 VariationsSeedStore::VariationsSeedStore(PrefService* local_state)
-    : VariationsSeedStore(local_state, nullptr, base::DoNothing(), true) {}
+    : VariationsSeedStore(local_state, nullptr, true) {}
 
 VariationsSeedStore::VariationsSeedStore(
     PrefService* local_state,
     std::unique_ptr<SeedResponse> initial_seed,
-    base::OnceCallback<void()> on_initial_seed_stored,
     bool signature_verification_enabled)
     : local_state_(local_state),
-      on_initial_seed_stored_(std::move(on_initial_seed_stored)),
       signature_verification_enabled_(signature_verification_enabled) {
 #if defined(OS_ANDROID)
   if (initial_seed)
@@ -401,11 +399,19 @@ void VariationsSeedStore::ClearPrefs(SeedType seed_type) {
 void VariationsSeedStore::ImportInitialSeed(
     std::unique_ptr<SeedResponse> initial_seed) {
   if (initial_seed->data.empty()) {
+    // Note: This is an expected case on non-first run starts.
     RecordFirstRunSeedImportResult(
         FirstRunSeedImportResult::FAIL_NO_FIRST_RUN_SEED);
     return;
   }
 
+  // Clear the Java-side seed prefs. At this point, the seed has
+  // already been fetched from the Java side, so it's no longer
+  // needed there. This is done regardless if we fail or succeed
+  // below - since if we succeed, we're good to go and if we fail,
+  // we probably don't want to keep around the bad content anyway.
+  android::ClearJavaFirstRunPrefs();
+
   if (initial_seed->date == 0) {
     RecordFirstRunSeedImportResult(
         FirstRunSeedImportResult::FAIL_INVALID_RESPONSE_DATE);
@@ -508,14 +514,12 @@ bool VariationsSeedStore::StoreSeedDataNoDelta(
     return false;
   }
 
-  // This callback is only useful on Chrome for Android.
-  if (!on_initial_seed_stored_.is_null()) {
-    // If currently we do not have any stored pref then we mark seed storing as
-    // successful on the Java side to avoid repeated seed fetches and clear
-    // preferences on the Java side.
-    if (local_state_->GetString(prefs::kVariationsCompressedSeed).empty())
-      std::move(on_initial_seed_stored_).Run();
-  }
+#if defined(OS_ANDROID)
+  // If currently we do not have any stored pref then we mark seed storing as
+  // successful on the Java side to avoid repeated seed fetches.
+  if (local_state_->GetString(prefs::kVariationsCompressedSeed).empty())
+    android::MarkVariationsSeedAsStored();
+#endif
 
   // Update the saved country code only if one was returned from the server.
   if (!country_code.empty())

components/variations/variations_seed_store.h

@@ -34,12 +34,10 @@ class VariationsSeedStore {
   // |initial_seed| may be null. If not null, then it will be stored in this
   // seed store. This is used by Android Chrome to supply the first run seed,
   // and by Android WebView to supply the seed on every run.
-  // |on_initial_seed_stored| will be called the first time a seed is stored.
   // |signature_verification_enabled| can be used in unit tests to disable
   // signature checks on the seed.
   VariationsSeedStore(PrefService* local_state,
                       std::unique_ptr<SeedResponse> initial_seed,
-                      base::OnceCallback<void()> on_initial_seed_stored,
                       bool signature_verification_enabled);
   virtual ~VariationsSeedStore();
 
@@ -205,8 +203,6 @@ class VariationsSeedStore {
   // Cached serial number from the most recently fetched variations seed.
   std::string latest_serial_number_;
 
-  base::OnceCallback<void()> on_initial_seed_stored_;
-
   // Whether to validate signatures on the seed. Always on except in tests.
   bool signature_verification_enabled_ = true;
 

components/variations/variations_seed_store_unittest.cc

@@ -54,7 +54,6 @@ class TestVariationsSeedStore : public VariationsSeedStore {
   explicit TestVariationsSeedStore(PrefService* local_state)
       : VariationsSeedStore(local_state,
                             nullptr,
-                            base::DoNothing(),
                             /*signature_verification_enabled=*/false) {}
   ~TestVariationsSeedStore() override = default;