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

chrome/browser/search_engines/template_url_service.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/search_engines/template_url_service.h"
 
 #include "base/auto_reset.h"
 #include "base/command_line.h"
 #include "base/environment.h"
 #include "base/i18n/case_conversion.h"
@@ skipping 439 matching lines @@
 
   // We're not loaded, rely on the default search provider stored in prefs.
   return initial_default_search_provider_.get();
 }
 
 const TemplateURL* TemplateURLService::FindNewDefaultSearchProvider() {
   // See if the prepoluated default still exists.
   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;
   }
   // If not, use the first of the templates.
   return template_urls_.empty() ? NULL : template_urls_[0];
 }
 
 void TemplateURLService::AddObserver(TemplateURLServiceObserver* observer) {
   model_observers_.AddObserver(observer);
 }
@@ skipping 48 matching lines @@
                                        &default_search_provider,
                                        &new_resource_keyword_version);
 
   bool database_specified_a_default = NULL != default_search_provider;
 
   // Check if default search provider is now managed.
   scoped_ptr<TemplateURL> default_from_prefs;
   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.
   const TemplateURL* backup_default_search_provider = NULL;
   if (!is_default_search_managed_ &&
       DidDefaultSearchProviderChange(
           *result,
           template_urls,
           &backup_default_search_provider)) {
     // Protector will delete itself when it's needed no longer.
     protector::Protector* protector = new protector::Protector(profile());
     protector->ShowChange(protector::CreateDefaultSearchProviderChange(
         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.
   RemoveProvidersCreatedByPolicy(&template_urls,
                                  &default_search_provider,
                                  default_from_prefs.get());
 
   if (is_default_search_managed_) {
     SetTemplateURLs(template_urls);
 
@@ skipping 10 matching lines @@
       if (managed_default) {
         managed_default->set_created_by_policy(true);
         managed_default->set_id(0);
         AddNoNotify(managed_default);
       }
       default_search_provider = managed_default;
     }
     // 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
     // to ensure that it is saved properly after its id is set.
     if (default_search_provider && default_search_provider->id() != 0) {
       default_search_provider_ = default_search_provider;
       default_search_provider = NULL;
     }
     SetTemplateURLs(template_urls);
 
@@ skipping 45 matching lines @@
     content::Details<history::URLVisitedDetails> visit_details(details);
     if (!loaded())
       visits_to_add_.push_back(*visit_details.ptr());
     else
       UpdateKeywordSearchTermsForURL(*visit_details.ptr());
   } else if (type == chrome::NOTIFICATION_GOOGLE_URL_UPDATED) {
     if (loaded_)
       GoogleBaseURLChanged();
   } 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();
     }
   } else {
     NOTREACHED();
   }
 }
 
 SyncDataList TemplateURLService::GetAllSyncData(
@@ skipping 37 matching lines @@
     if (!turl.get()) {
       NOTREACHED() << "Failed to read search engine.";
       continue;
     }
 
     const TemplateURL* existing_turl = GetTemplateURLForGUID(turl->sync_guid());
     const TemplateURL* existing_keyword_turl =
         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());
     } else {
       // Something really unexpected happened. Either we received an
       // ACTION_INVALID, or Sync is in a crazy state:
       //  . Trying to DELETE or UPDATE a non-existent search engine.
       //  . Trying to ADD a search engine that already exists.
       NOTREACHED() << "Unexpected sync change state.";
       error = SyncError(FROM_HERE, "ProcessSyncChanges failed on ChangeType " +
@@ skipping 73 matching lines @@
       if (dupe_turl) {
         // Merge duplicates and remove the processed local TURL from the map.
         TemplateURL* modifiable_dupe_turl =
             const_cast<TemplateURL*>(dupe_turl);
         std::string old_guid = dupe_turl->sync_guid();
         MergeSyncAndLocalURLDuplicates(sync_turl.release(),
                                        modifiable_dupe_turl,
                                        &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
         // the cloud.
         ResolveSyncKeywordConflict(sync_turl.get(), &new_changes);
         // 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
 
   // The remaining SyncData in local_data_map should be everything that needs to
   // be pushed as ADDs to sync.
   for (SyncDataMap::const_iterator iter = local_data_map.begin();
       iter != local_data_map.end(); ++iter) {
     new_changes.push_back(SyncChange(SyncChange::ACTION_ADD, iter->second));
   }
@@ skipping 112 matching lines @@
                                                      const GURL& url,
                                                      const string16& term) {
   HistoryService* history = profile_  ?
       profile_->GetHistoryService(Profile::EXPLICIT_ACCESS) : NULL;
   if (!history)
     return;
   history->SetKeywordSearchTermsForURL(url, t_url->id(), term);
 }
 
 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
     // db, which will mean we no longer need this notification and the history
     // backend can handle automatically adding the search terms as the user
     // navigates.
     registrar_.Add(this, chrome::NOTIFICATION_HISTORY_URL_VISITED,
                    content::Source<Profile>(profile_->GetOriginalProfile()));
     PrefService* prefs = GetPrefs();
     default_search_prefs_.reset(
@@ skipping 515 matching lines @@
     DCHECK(is_default_search_managed_ && !new_is_default_managed);
     is_default_search_managed_ = new_is_default_managed;
     // If we had a default, delete the previous default if created by policy
     // and set a likely default.
     if (NULL != default_search_provider_ &&
         default_search_provider_->created_by_policy()) {
       const TemplateURL* old_default = default_search_provider_;
       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();
 }
 
 void TemplateURLService::SetDefaultSearchProviderNoNotify(
     const TemplateURL* url) {
   DCHECK(!url || find(template_urls_.begin(), template_urls_.end(), url) !=
          template_urls_.end());
   default_search_provider_ = url;
 
@@ skipping 11 matching lines @@
 #if defined(OS_WIN) && defined(GOOGLE_CHROME_BUILD)
       // Needs to be evaluated. See http://crbug.com/62328.
       base::ThreadRestrictions::ScopedAllowIO allow_io;
       RLZTracker::RecordProductEvent(rlz_lib::CHROME,
                                      rlz_lib::CHROME_OMNIBOX,
                                      rlz_lib::SET_TO_GOOGLE);
 #endif
     }
   }
 
-  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);
 
   // Inform sync the change to the show_in_default_list flag.
   if (url)
     ProcessTemplateURLChange(url, SyncChange::ACTION_UPDATE);
 }
 
 void TemplateURLService::AddNoNotify(TemplateURL* template_url) {
   DCHECK(template_url);
@@ skipping 138 matching lines @@
 }
 
 bool TemplateURLService::ResolveSyncKeywordConflict(
     TemplateURL* sync_turl,
     SyncChangeList* change_list) {
   DCHECK(sync_turl);
   DCHECK(change_list);
 
   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() ||
       existing_turl->created_by_policy()) {
     string16 new_keyword = UniquifyKeyword(*sync_turl);
     DCHECK(!GetTemplateURLForKeyword(new_keyword));
     sync_turl->set_keyword(new_keyword);
     // If we update the cloud TURL, we need to push an update back to sync
     // informing it that something has changed.
     SyncData sync_data = CreateSyncDataFromTemplateURL(*sync_turl);
@@ skipping 42 matching lines @@
   } else {
     // Change the local TURL's GUID to the server's GUID and push an update to
     // Sync. This ensures that the rest of local_url's fields are sync'd up to
     // the server, and the next time local_url is synced, it is recognized by
     // having the same GUID.
     ResetTemplateURLGUID(local_turl, sync_turl->sync_guid());
     SyncData sync_data = CreateSyncDataFromTemplateURL(*local_turl);
     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));
+}