Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(31)

Issue 274323002: Revert of Some refactorings to facilitate a larger change to TemplateURLService. (Closed)

Created:
6 years, 7 months ago by Alpha Left Google
Modified:
6 years, 7 months ago
CC:
chromium-reviews
Visibility:
Public.

Description

Revert of Some refactorings to facilitate a larger change to TemplateURLService. (https://codereview.chromium.org/270533007/) Reason for revert: Use after free detected with this test: EditSearchEngineControllerTest.EditTemplateURL Failure bot: http://build.chromium.org/p/chromium.memory/builders/Mac%20ASan%2064%20Tests%20%281%29/builds/759/steps/unit_tests/logs/stdio Original issue's description: > Some refactorings to facilitate a larger change to TemplateURLService. > > BUG=365762 > R=pkasting@chromium.org > > Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269310 TBR=pkasting@chromium.org,erikwright@chromium.org NOTREECHECKS=true NOTRY=true BUG=365762

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+69 lines, -57 lines) Patch
M chrome/browser/search_engines/default_search_manager.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/search_engines/default_search_pref_migration.cc View 1 chunk +46 lines, -35 lines 0 comments Download
M chrome/browser/search_engines/template_url_service.h View 5 chunks +8 lines, -7 lines 0 comments Download
M chrome/browser/search_engines/template_url_service.cc View 8 chunks +13 lines, -13 lines 0 comments Download
M chrome/browser/search_engines/template_url_service_sync_unittest.cc View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 4 (0 generated)
Alpha Left Google
Created Revert of Some refactorings to facilitate a larger change to TemplateURLService.
6 years, 7 months ago (2014-05-10 20:00:48 UTC) #1
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hclam@chromium.org/274323002/1
6 years, 7 months ago (2014-05-10 20:01:59 UTC) #2
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 7 months ago (2014-05-10 20:06:18 UTC) #3
commit-bot: I haz the power
6 years, 7 months ago (2014-05-10 20:06:18 UTC) #4
Failed to apply patch for
chrome/browser/search_engines/default_search_pref_migration.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/search_engines/default_search_pref_migration.cc
  Hunk #1 FAILED at 6.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/search_engines/default_search_pref_migration.cc.rej

Patch:       chrome/browser/search_engines/default_search_pref_migration.cc
Index: chrome/browser/search_engines/default_search_pref_migration.cc
diff --git a/chrome/browser/search_engines/default_search_pref_migration.cc
b/chrome/browser/search_engines/default_search_pref_migration.cc
index
835254a3c21a1376561e1d1149a363108fb2e63f..7516bfc9c2ad1a2c779ff171f103a157ed27090d
100644
--- a/chrome/browser/search_engines/default_search_pref_migration.cc
+++ b/chrome/browser/search_engines/default_search_pref_migration.cc
@@ -6,57 +6,68 @@
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
-#include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/metrics/histogram.h"
 #include "base/prefs/pref_service.h"
 #include "chrome/browser/search_engines/default_search_manager.h"
 #include "chrome/browser/search_engines/template_url.h"
+#include "chrome/browser/search_engines/template_url_prepopulate_data.h"
 #include "chrome/browser/search_engines/template_url_service.h"
 
 namespace {
 
-// Loads the user-selected DSE (if there is one, and it's not masked by policy
-// or an extension) from legacy preferences.
-scoped_ptr<TemplateURLData> LoadDefaultSearchProviderFromPrefs(
-    PrefService* pref_service) {
+void MigrateDefaultSearchPref(PrefService* pref_service) {
+  DefaultSearchManager default_search_manager(
+      pref_service, DefaultSearchManager::ObserverCallback());
+
+  if (default_search_manager.GetDefaultSearchEngineSource() ==
+      DefaultSearchManager::FROM_USER) {
+    return;
+  }
+
   scoped_ptr<TemplateURLData> legacy_dse_from_prefs;
   bool legacy_is_managed = false;
-  TemplateURLService::LoadDefaultSearchProviderFromPrefs(
-      pref_service, &legacy_dse_from_prefs, &legacy_is_managed);
-  return legacy_is_managed ?
-      scoped_ptr<TemplateURLData>() : legacy_dse_from_prefs.Pass();
-}
+  bool has_legacy_dse_from_prefs =
+      TemplateURLService::LoadDefaultSearchProviderFromPrefs(
+          pref_service, &legacy_dse_from_prefs, &legacy_is_managed);
 
-void MigrateDefaultSearchPref(PrefService* pref_service) {
-  DCHECK(pref_service);
-
-  scoped_ptr<TemplateURLData> legacy_dse_from_prefs =
-      LoadDefaultSearchProviderFromPrefs(pref_service);
-  if (!legacy_dse_from_prefs)
+  if (!has_legacy_dse_from_prefs) {
+    // The DSE is undefined. Nothing to migrate.
     return;
-
-  DefaultSearchManager default_search_manager(
-      pref_service, DefaultSearchManager::ObserverCallback());
-  DefaultSearchManager::Source modern_source;
-  TemplateURLData* modern_value =
-      default_search_manager.GetDefaultSearchEngine(&modern_source);
-  if (modern_source == DefaultSearchManager::FROM_FALLBACK) {
-    // |modern_value| is the prepopulated default. If it matches the legacy DSE
-    // we assume it is not a user-selected value.
-    if (!modern_value ||
-        legacy_dse_from_prefs->prepopulate_id != modern_value->prepopulate_id)
{
-      // This looks like a user-selected value, so let's migrate it.
-      // TODO(erikwright): Remove this migration logic when this stat
approaches
-      // zero.
-      UMA_HISTOGRAM_BOOLEAN("Search.MigratedPrefToDictionaryValue", true);
-      default_search_manager.SetUserSelectedDefaultSearchEngine(
-          *legacy_dse_from_prefs);
-    }
+  }
+  if (!legacy_dse_from_prefs) {
+    // The DSE is defined as NULL. This can only really be done via policy.
+    // Policy-defined values will be automatically projected into the new
+    // format. Even if the user did somehow set this manually we do not have a
+    // way to migrate it.
+    return;
+  }
+  if (legacy_is_managed) {
+    // The DSE is policy-managed, not user-selected. It will automatically be
+    // projected into the new location.
+    return;
   }
 
+  // If the pre-populated DSE matches the DSE from prefs we assume it is not a
+  // user-selected value.
+  scoped_ptr<TemplateURLData> prepopulated_dse(
+      TemplateURLPrepopulateData::GetPrepopulatedDefaultSearch(pref_service));
+  if (prepopulated_dse &&
+      legacy_dse_from_prefs->prepopulate_id ==
+          prepopulated_dse->prepopulate_id) {
+    return;
+  }
+
+  UMA_HISTOGRAM_BOOLEAN("Search.MigratedPrefToDictionaryValue", true);
+
+  // This looks like a user-selected value, so let's migrate it. Subsequent
+  // changes to this value will be automatically stored in the correct
location.
+  default_search_manager.SetUserSelectedDefaultSearchEngine(
+      *legacy_dse_from_prefs);
+
   // TODO(erikwright): Clear the legacy value when the modern value is the
-  // authority.
+  // authority. Don't forget to do this even if we don't migrate (because we
+  // migrated prior to implementing the clear.
 }
 
 void OnPrefsInitialized(PrefService* pref_service,

Powered by Google App Engine
This is Rietveld 408576698