Ensure that MergeDataAndStartSyncing considers already synced entries when setting the synced DSP.

chrome/browser/search_engines/template_url_service.cc

 // Copyright (c) 2012 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/compiler_specific.h"
 #include "base/environment.h"
@@ skipping 738 matching lines @@
     // Note that this saves the default search provider to prefs.
     if (!default_search_provider ||
         (!default_search_provider->IsExtensionKeyword() &&
          default_search_provider->SupportsReplacement())) {
       bool success = SetDefaultSearchProviderNoNotify(default_search_provider);
       DCHECK(success);
     }
   } else {
     // If we had a managed default, replace it with the synced default if
     // applicable, or the first provider of the list.
     TemplateURL* synced_default = GetPendingSyncedDefaultSearchProvider();

[Nicolas Zea, 2012/08/15 21:46:28]

Doesn't this logic handle the case this patch addresses? Or was there something
else involved? (see comment below)

[SteveT, 2012/08/15 21:56:57]

This doesn't seem to do the trick for the case where you sign out and in during
the same Chrome session (since OnWebDataServiceRequestDone only runs on
startup).

[Nicolas Zea, 2012/08/15 22:26:03]

Huh, I thought it ran on all webdb requests, but ok.

[SteveT, 2012/08/16 19:50:53]

Ah sorry, you are right - OnWebDataServiceRequestDone runs per webdb request,
such as Adding, Updating, or Removing keywords from the webdb.

However, it doesn't trigger when you sign back in, so the out-of-sync DSP isn't
resolved by this bit of code here.

     if (synced_default) {
       default_search_provider = synced_default;
       pending_synced_default_search_ = false;
     } else if (database_specified_a_default &&
                default_search_provider == NULL) {
       UMA_HISTOGRAM_ENUMERATION(kFirstPotentialEngineHistogramName,
           FIRST_POTENTIAL_CALLSITE_ON_LOAD, FIRST_POTENTIAL_CALLSITE_MAX);
       default_search_provider = FirstPotentialDefaultEngine(template_urls);
     }
 
@@ skipping 314 matching lines @@
 
   // 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);
     const TemplateURL* current_default = GetDefaultSearchProvider();
 
     if (!default_guid.empty() &&
         (!current_default || current_default->sync_guid() != default_guid))
       pending_synced_default_search_ = true;

[Nicolas Zea, 2012/08/15 21:46:28]

we should be hitting this logic right?

[SteveT, 2012/08/15 21:56:57]

We should be, but the real problem is that there is no code below to actually do
the assignment of the new DSP. We accounted for cases where the DSP comes in as
a newly synced TURL, but not the case where it was already synced.

   }
 
   // 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);
 
   // We've started syncing, so set our origin member to the base Sync value.
   // As we move through Sync Code, we may set this to increasingly specific
   // origins so we can tell what exactly caused a DSP change.
   AutoReset<DefaultSearchChangeOrigin> change_origin(&dsp_change_origin_,
@@ skipping 43 matching lines @@
           NotifyObservers();
       } else if (sync_turl->last_modified() < local_turl->last_modified()) {
         // Otherwise, we know we have newer data, so update Sync with our
         // data fields.
         new_changes.push_back(
             syncer::SyncChange(FROM_HERE,
                                syncer::SyncChange::ACTION_UPDATE,
                                local_data_map[local_turl->sync_guid()]));
       }
       local_data_map.erase(iter->first);
+
+      // Attempt to reset the default search provider incase it was changed

[Nicolas Zea, 2012/08/15 21:46:28]

incase -> in case

[SteveT, 2012/08/15 21:56:57]

Done.

+      // since the last time search engines was synced.
+      SetDefaultSearchProviderIfNewlySynced(local_turl->sync_guid());

[Nicolas Zea, 2012/08/15 22:26:03]

Would moving this to the end of MergeData and calling GetPendingSyncedDefault
instead make more sense? Seems like it's not a per-url check you need, but just
a "did we have a pending, does it exist now, if so set it" flow, much like in
the OnWebDBRequestDone

[SteveT, 2012/08/16 19:50:53]

That makes a lot of sense for MergeDataAndStartSyncing, and I've made the
changes to reflect this.

     } else {
       // The search engine from the cloud has not been synced locally. Merge it
       // into our local model. This will handle any conflicts with local (and
       // already-synced) TemplateURLs. It will prefer to keep entries from Sync
       // over not-yet-synced TemplateURLs.
       MergeInSyncTemplateURL(sync_turl.get(), sync_data_map, &new_changes,
                              &local_data_map);
     }
   }
 
@@ skipping 1305 matching lines @@
     // TODO(mpcomplete): If we allow editing extension keywords, then those
     // should be persisted to disk and synced.
     if (template_url->sync_guid().empty() &&
         !template_url->IsExtensionKeyword()) {
       template_url->data_.sync_guid = base::GenerateGUID();
       if (service_.get())
         service_->UpdateKeyword(template_url->data());
     }
   }
 }