Merge search engines sync data type with Preferences. Sync the default search provider. Add some ...

chrome/browser/search_engines/template_url_service.cc

Index: chrome/browser/search_engines/template_url_service.cc
===================================================================
--- chrome/browser/search_engines/template_url_service.cc	(revision 108905)
+++ chrome/browser/search_engines/template_url_service.cc	(working copy)
@@ -113,7 +113,8 @@
       time_provider_(&base::Time::Now),
       models_associated_(false),
       processing_syncer_changes_(false),
-      sync_processor_(NULL) {
+      sync_processor_(NULL),
+      pending_synced_default_search_(false) {
   DCHECK(profile_);
   Init(NULL, 0);
 }
@@ -131,7 +132,8 @@
       time_provider_(&base::Time::Now),
       models_associated_(false),
       processing_syncer_changes_(false),
-      sync_processor_(NULL) {
+      sync_processor_(NULL),
+      pending_synced_default_search_(false) {
   Init(initializers, count);
 }
 
@@ -457,7 +459,7 @@
   scoped_ptr<TemplateURL> prepopulated_default(
       TemplateURLPrepopulateData::GetPrepopulatedDefaultSearch(GetPrefs()));
   for (TemplateURLVector::iterator i = template_urls_.begin();
-      i != template_urls_.end(); ) {
+       i != template_urls_.end(); ++i) {
     if ((*i)->prepopulate_id() == prepopulated_default->prepopulate_id())
       return *i;
   }
@@ -577,12 +579,17 @@
     // Note that this saves the default search provider to prefs.
     SetDefaultSearchProviderNoNotify(default_search_provider);
   } else {
-    // If we had a managed default, replace it with the first provider of
-    // the list.
-    if (database_specified_a_default &&
-        NULL == default_search_provider &&
-        !template_urls.empty())
+    // If we had a managed default, replace it with the synced default if
+    // applicable, or the first provider of the list.
+    const TemplateURL* synced_default = GetPendingSyncedDefaultSearchProvder();
+    if (synced_default) {
+      default_search_provider = synced_default;
+      pending_synced_default_search_ = false;
+    } else if (database_specified_a_default &&
+               NULL == default_search_provider &&
+               !template_urls.empty()) {
       default_search_provider = template_urls[0];
+    }
 
     // If the default search provider existed previously, then just
     // set the member variable. Otherwise, we'll set it using the method
@@ -649,6 +656,25 @@
   } else if (type == chrome::NOTIFICATION_PREF_CHANGED) {
     const std::string* pref_name = content::Details<std::string>(details).ptr();
     if (!pref_name || default_search_prefs_->IsObserved(*pref_name)) {
+      // Listen for changes to the default search from Sync. If it is
+      // specifically the synced default search provider GUID that changed, we
+      // have to set it (or wait for it).
+      PrefService* prefs = GetPrefs();
+      if (pref_name && *pref_name == prefs::kSyncedDefaultSearchProviderGUID &&
+          prefs) {
+        const TemplateURL* new_default_search = GetTemplateURLForGUID(
+            prefs->GetString(prefs::kSyncedDefaultSearchProviderGUID));

[jeanluc1, 2011/11/08 19:49:41]

Can GetString return NULL?  What's the behavior of GetTemplateURLForGUID then?

[jeanluc1, 2011/11/08 19:49:41]

Would GetPendingSyncedDefaultSearchProvder work here?

[SteveT, 2011/11/08 21:40:04]

PrefService::GetString can return an empty string (if the pref is not
registered, which should DCHECK first), which, if fed to GetTemplateURLForGUID,
returns NULL. This causes the code below to believe that there is no TURL with
that GUID, and therefore set pending_... to true and wait for the TURL to
arrive, which is what we want.

[SteveT, 2011/11/08 21:40:04]

It would not as it only retrieves the TURL if we are waiting on it. If we are
not waiting on it, but it's actually already synced in, we would miss it
(GetPendingSyncedDefaultSearchProvider would return NULL).

+        if (new_default_search && !is_default_search_managed_) {
+          if (new_default_search != GetDefaultSearchProvider())
+            SetDefaultSearchProvider(new_default_search);
+        } else {
+          // If it's not there, or if default search is currently managed, set a
+          // flag to indicate that we waiting on the search engine entry to come
+          // in through Sync.
+          pending_synced_default_search_ = true;

[jeanluc1, 2011/11/08 19:49:41]

What if we get a second Observe call, this time where we could call
SetDefaultSearchProvider.  Does it set pending_synced_default_search to false?

[SteveT, 2011/11/08 21:40:04]

No it doesn't. We should set pending_synced_default_search ourselves. Done.

+        }
+      }
+
       // A preference related to default search engine has changed.
       // Update the model if needed.
       UpdateDefaultSearch();
@@ -706,17 +732,34 @@
         GetTemplateURLForKeyword(turl->keyword());
 
     if (iter->change_type() == SyncChange::ACTION_DELETE && existing_turl) {
+      bool delete_default = (existing_turl == GetDefaultSearchProvider());
+      if (delete_default && is_default_search_managed_) {
+        // We are not allowed to delete managed default search providers.
+        continue;

[jeanluc1, 2011/11/08 19:49:41]

Can we avoid the continue?  There are so many other things in the loop that I'm
afraid future mainteners won't expect there's a continue somewhere in the loop.

[SteveT, 2011/11/08 21:40:04]

Actually, we can be a bit more defensive and transform that continue into a
NOTREACHED, since we really should not be allowing the system to delete managed
defaults.

+      }
+      if (delete_default)
+        default_search_provider_ = NULL;
+
       Remove(existing_turl);
+
+      if (delete_default)
+        SetDefaultSearchProvider(FindNewDefaultSearchProvider());
     } else if (iter->change_type() == SyncChange::ACTION_ADD &&
                !existing_turl) {
+      std::string guid = turl->sync_guid();
       if (existing_keyword_turl)
         ResolveSyncKeywordConflict(turl.get(), &new_changes);
       // Force the local ID to 0 so we can add it.
       turl->set_id(0);
       Add(turl.release());
+
+      // Possibly set the newly added |turl| as the default search provider.
+      CheckForSyncedDefaultSearchProvider(guid);
     } else if (iter->change_type() == SyncChange::ACTION_UPDATE &&
                existing_turl) {
-      if (existing_keyword_turl)
+      // Possibly resolve a keyword conflict if they have the same keywords but
+      // are not the same entry.
+      if (existing_keyword_turl && existing_keyword_turl != existing_turl)
         ResolveSyncKeywordConflict(turl.get(), &new_changes);
       ResetTemplateURL(existing_turl, turl->short_name(), turl->keyword(),
           turl->url() ? turl->url()->url() : std::string());
@@ -751,6 +794,18 @@
   DCHECK(!sync_processor_);
   sync_processor_ = sync_processor;
 
+  // We just started syncing, so set our wait-for-default flag if we are
+  // expecting a default from Sync.
+  if (GetPrefs()) {
+    std::string default_guid = GetPrefs()->GetString(
+        prefs::kSyncedDefaultSearchProviderGUID);

[jeanluc1, 2011/11/08 19:49:41]

Would GetPendingSyncedDefaultSearchProvder work here?

[SteveT, 2011/11/08 21:40:04]

Unfortunately not. At this point, we can't fully trust the value of
pending_synced_default_search_ yet (in fact, it'll be false every time on
startup/first sync), which makes GetPendingSyncedDefaultSearchProvider return
NULL every time. We just want the default Sync GUID here, and not the actual
synced default search provider (which might not be synced in yet).

+    const TemplateURL* current_default = GetDefaultSearchProvider();
+
+    if (!default_guid.empty() &&
+        (!current_default || current_default->sync_guid() == default_guid))

[Nicolas Zea, 2011/11/08 19:57:27]

Should that last condition be current_default->sync_guid() != default_guid? If
the actual default is already the same as the sync default aren't we fine?

[SteveT, 2011/11/08 21:40:04]

Yes, that was a slip. I'll add a small test case to ensure that if the expected
synced default is already set, nothing gets tripped up. Thanks!

+      pending_synced_default_search_ = true;
+  }
+
   // We do a lot of calls to Add/Remove/ResetTemplateURL here, so ensure we
   // don't step on our own toes.
   AutoReset<bool> processing_changes(&processing_syncer_changes_, true);
@@ -810,6 +865,7 @@
                                        &new_changes);
         local_data_map.erase(old_guid);
       } else {
+        std::string guid = sync_turl->sync_guid();
         // Keyword conflict is possible in this case. Resolve it first before
         // adding the new TemplateURL. Note that we don't remove the local TURL
         // from local_data_map in this case as it may still need to be pushed to
@@ -818,6 +874,9 @@
         // Force the local ID to 0 so we can add it.
         sync_turl->set_id(0);
         Add(sync_turl.release());
+
+        // Possibly set the newly added |turl| as the default search provider.
+        CheckForSyncedDefaultSearchProvider(guid);
       }
     }
   }  // for
@@ -950,7 +1009,7 @@
 }
 
 void TemplateURLService::Init(const Initializer* initializers,
-                            int num_initializers) {
+                              int num_initializers) {
   // Register for notifications.
   if (profile_) {
     // TODO(sky): bug 1166191. The keywords should be moved into the history
@@ -1486,7 +1545,14 @@
       default_search_provider_ = NULL;
       RemoveNoNotify(old_default);
     }
-    SetDefaultSearchProviderNoNotify(FindNewDefaultSearchProvider());
+
+    // The likely default should be from Sync if we were waiting on Sync.
+    // Otherwise, it should be FindNewDefaultSearchProvider.
+    const TemplateURL* synced_default = GetPendingSyncedDefaultSearchProvder();
+    if (synced_default)
+      pending_synced_default_search_ = false;
+    SetDefaultSearchProviderNoNotify(synced_default ? synced_default :
+        FindNewDefaultSearchProvider());
   }
   NotifyObservers();
 }
@@ -1518,9 +1584,17 @@
     }
   }
 
-  if (!is_default_search_managed_)
+  if (!is_default_search_managed_) {
     SaveDefaultSearchProviderToPrefs(url);
 
+    // If we are syncing, we want to set the synced pref that will notify other
+    // instances to change their default to this new search provider.
+    if (sync_processor_ && !url->sync_guid().empty() && GetPrefs()) {
+      GetPrefs()->SetString(prefs::kSyncedDefaultSearchProviderGUID,
+                            url->sync_guid());
+    }
+  }
+
   if (service_.get())
     service_->SetDefaultSearchProvider(url);
 
@@ -1679,7 +1753,8 @@
 
   const TemplateURL* existing_turl =
       GetTemplateURLForKeyword(sync_turl->keyword());
-  if (!existing_turl)
+  // If there is no conflict, or it's just conflicting with itself, return.
+  if (!existing_turl || existing_turl->sync_guid() == sync_turl->sync_guid())
     return false;
 
   if (existing_turl->last_modified() > sync_turl->last_modified() ||
@@ -1742,3 +1817,31 @@
     change_list->push_back(SyncChange(SyncChange::ACTION_UPDATE, sync_data));
   }
 }
+
+void TemplateURLService::CheckForSyncedDefaultSearchProvider(
+    const std::string& guid) {
+  // If we're not syncing or if default search is managed by policy, ignore.
+  if (!sync_processor_ || is_default_search_managed_)
+    return;
+
+  PrefService* prefs = GetPrefs();
+  if (prefs && pending_synced_default_search_ &&
+      prefs->GetString(prefs::kSyncedDefaultSearchProviderGUID) == guid) {
+    // Make sure this actually exists. We should not be calling this unless we
+    // really just added this TemplateURL.
+    const TemplateURL* turl_from_sync = GetTemplateURLForGUID(guid);
+    DCHECK(turl_from_sync);
+    SetDefaultSearchProvider(turl_from_sync);
+    pending_synced_default_search_ = false;
+  }
+}
+
+const TemplateURL* TemplateURLService::GetPendingSyncedDefaultSearchProvder() {

[jeanluc1, 2011/11/08 19:49:41]

Provder -> Provider

[SteveT, 2011/11/08 21:40:04]

Done.

+  PrefService* prefs = GetPrefs();
+  if (!prefs || !pending_synced_default_search_)
+    return NULL;
+
+  // Could be NULL if no such thing exists.
+  return GetTemplateURLForGUID(
+      prefs->GetString(prefs::kSyncedDefaultSearchProviderGUID));
+}