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 108653)
+++ chrome/browser/search_engines/template_url_service.cc	(working copy)
@@ -457,7 +457,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) {

[Nicolas Zea, 2011/11/04 23:08:06]

indent by one more

[SteveT, 2011/11/07 21:48:56]

Done.

     if ((*i)->prepopulate_id() == prepopulated_default->prepopulate_id())
       return *i;
   }
@@ -526,9 +526,6 @@
   LoadDefaultSearchProviderFromPrefs(&default_from_prefs,
                                      &is_default_search_managed_);
 
-  // TODO(sail): Re-enable the protector on Mac and Linux once various
-  // crashes and UI issues are fixed. See http://crbug.com/102765
-#if defined(OS_WIN)

[Nicolas Zea, 2011/11/04 23:08:06]

Is this supposed to be part of this patch, or one addressing 102765?

[SteveT, 2011/11/07 21:48:56]

Odd. I think this change was pulled in when I merged to tip of tree. According
to 102765 and TOT, these lines are missing, so I'll leave this change alone.

   // Check if the default search provider has been changed and notify
   // Protector instance about it. Don't check if the default search is
   // managed.
@@ -544,7 +541,6 @@
         default_search_provider,
         backup_default_search_provider));
   }
-#endif
 
   // Remove entries that were created because of policy as they may have
   // changed since the database was saved.
@@ -575,12 +571,18 @@
     // 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;
+      GetPrefs()->SetBoolean(prefs::kPendingSyncedDefaultSearchProviderChange,
+                             false);
+    } else if (database_specified_a_default &&
+               NULL == default_search_provider &&
+               !template_urls.empty()) {
       default_search_provider = template_urls[0];
+    }

[Nicolas Zea, 2011/11/04 23:08:06]

What about the else scenario in this case? Is it valid?

[SteveT, 2011/11/07 21:48:56]

I believe it's still valid. Either the database didn't specify a default, which
means we leave it NULL, or it did specify a default AND the default is a valid
pointer, in which case we don't have to find a fallback to replace it.

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

Makes sense, although a comment about this might be useful.

 
     // If the default search provider existed previously, then just
     // set the member variable. Otherwise, we'll set it using the method
@@ -647,6 +649,26 @@
   } 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)) {

[Nicolas Zea, 2011/11/04 23:08:06]

I'm confused about this if statement. Why would pref_name be NULL? And if it is,
why would we want to process it?

Should this be if (pref_name && default_search_prefs...)

[SteveT, 2011/11/07 21:48:56]

I wasn't sure about why pref_name would ever be NULL, but I looked into it and
it looks like it initially set that way for testing - the unit tests would fake
a notification by sending a NOTIF_PREF_CHANGED but with a NULL pointer as the
details. They don't seem to do that anymore and just rely on the
TestingPrefService to send an "actual" pref that sets off the
default_search_prefs_->IsObserved(*pref_name) check.

Jean-Luc, could you comment on this? Can we change it to (pref_name &&
default_search_prefs...) as Nick suggested without adversely affecting any
tests/functionality?

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

If it turns out the current way is necessary, a comment explaining that it's
primarily for testing would be useful. 

+      // 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));
+        if (new_default_search && !is_default_search_managed_) {

[Nicolas Zea, 2011/11/04 23:08:06]

where does is_default_search_managed_ get updated, and can we be sure it happens
before this? It might be better to directly check instead
prefs->IsUserModifiable(pref_name)

[SteveT, 2011/11/07 21:48:56]

is_default_search_managed_ actually gets updated in UpdateDefaultSearch()
(called just below this block). However, it only gets updated by changes to the
managed DSP prefs, and not changes to kSyncedDefaultSearchProviderGUID. Checking
IsUserModifiable(kSyncedDefaultSearchProviderGUID) will always return true since
it is not actually managed.

If there is a race between the DSP becoming managed and the (currently)
unmanaged DSP being changed by sync, this is how it will play out:
 - is_default_search_managed_ is set to true first. In that case, we don't call
SetDefault... below since it is now managed.
 - A new default is synced in first. It gets changed in the SetDefault... call
below. Then the DSP becomes managed... this is handled in a separate run through
Observe, which calls UpdateDefaultSearch below and changes the DSP to the
managed one.

+          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.
+          prefs->SetBoolean(prefs::kPendingSyncedDefaultSearchProviderChange,

[Nicolas Zea, 2011/11/04 23:08:06]

I'm not sure I like the approach of adding an extra flag that we have to persist
just to denote that sync hasn't sent the actual provider yet. Why not just have
a boolean member variable instead, and at startup set it based on whether you
can find the synceddefaultserachproviderguid.

[SteveT, 2011/11/07 21:48:56]

One issue is that we generally don't clear the SyncedDefaultSearchProvider pref,
since it's synced and we need it to ensure that new clients that start syncing
Prefs will see what the synced default is.

However, I do agree with you that having this PendingSyncedDSPChange pref is
kinda lame. I think we can get away with just a member variable. Since
kSyncedDefaultSearchProviderGUID is meant to represent the GUID of the last
known and synced default search provider, we can set the member flag to true if
we start syncing and we notice that the value of
kSyncedDefaultSearchProviderGUID is not equal to the current default's GUID.

I've made this by adding a new pending_synced_default_search_ bool member to
replace the pref, and I've added a unit test "SyncedDefaultArrivesAfterStartup"
to test that this works.

+                            true);
+        }
+      }
+
       // A preference related to default search engine has changed.
       // Update the model if needed.
       UpdateDefaultSearch();
@@ -704,17 +726,35 @@
         GetTemplateURLForKeyword(turl->keyword());
 
     if (iter->change_type() == SyncChange::ACTION_DELETE && existing_turl) {
+      bool delete_default = existing_turl == GetDefaultSearchProvider();

[Nicolas Zea, 2011/11/04 23:08:06]

prefer paren around expression: (existing... == ...)

[SteveT, 2011/11/07 21:48:56]

Done.

+      if (delete_default && is_default_search_managed_) {
+        // We are not allowed to delete managed default search providers.
+        continue;

[Nicolas Zea, 2011/11/04 23:08:06]

should this clear the SyncedDefaultSearchProviderGuid pref?

[SteveT, 2011/11/07 21:48:56]

Hm, I don't think so. We have to keep SyncedDefaultSearchProviderGUID's value
around while the DSP is managed so we know which search engine to look for as
soon as the DSP becomes unmanaged.

+      }
+      if (delete_default)

[Nicolas Zea, 2011/11/04 23:08:06]

how about
if (delete default) {
  if (!is_default_search_managed_)
    default_search_provider_ = NULL;
  else
    ...
  SetDefaultSearchProvider(...)
}
Remove(...)

[SteveT, 2011/11/07 21:48:56]

So what we're trying to do here is to allow deletion of the current default by
changing the default to something else before deleting the TURL. In your
suggested snippet, it's possible that
SetDefaultSearchProvider(FindNewDefault...) can reset default_search_provider_
to be the TURL we call remove on.

+        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);

[Nicolas Zea, 2011/11/04 23:08:06]

may as well just call this with turl->sync_guid()

[SteveT, 2011/11/07 21:48:56]

We can't really do this since ownership of turl is given up when we call Add()
above.

     } 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());
@@ -808,6 +848,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
@@ -816,6 +857,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);

[Nicolas Zea, 2011/11/04 23:08:06]

same here

[SteveT, 2011/11/07 21:48:56]

As above.

       }
     }
   }  // for
@@ -948,7 +992,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
@@ -1484,7 +1528,15 @@
       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)
+      GetPrefs()->SetBoolean(prefs::kPendingSyncedDefaultSearchProviderChange,
+                             false);
+    SetDefaultSearchProviderNoNotify(synced_default ? synced_default :
+        FindNewDefaultSearchProvider());
   }
   NotifyObservers();
 }
@@ -1516,9 +1568,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);
 
@@ -1677,7 +1737,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() ||
@@ -1740,3 +1801,34 @@
     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 &&
+      prefs->GetBoolean(prefs::kPendingSyncedDefaultSearchProviderChange) &&
+      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);
+    prefs->SetBoolean(prefs::kPendingSyncedDefaultSearchProviderChange, false);
+  }
+}
+
+const TemplateURL* TemplateURLService::GetPendingSyncedDefaultSearchProvder() {
+  PrefService* prefs = GetPrefs();
+  if (!prefs ||
+      !prefs->GetBoolean(prefs::kPendingSyncedDefaultSearchProviderChange)) {
+    return NULL;
+  }
+
+  // Could be NULL if no such thing exists.
+  return GetTemplateURLForGUID(
+      prefs->GetString(prefs::kSyncedDefaultSearchProviderGUID));
+}