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

Merge search engines sync data type with Preferences. Sync the default search provider. Add some defensive measures to prevent deletion of the default search engine or unnecessarily uniquifying keywords.

TEST=Ensure that the default search provider syncs when the Preferences sync data type is enabled. Ensure that the normal search engine syncing changes (add, update, delete) all work.
BUG=15548
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=109882

[SteveT, 2011-11-04 21:23:23]

Nick: PTAL at everything except perhaps the SearchEngine integration tests.
Raghu: PTAL at the integration tests/helper changes.
Jean-Luc: PTAL at template_url_service.cc bits that touch the managed default
search provider. Let me know if anything doesn't jive well with the managed DSP.

Thanks, all!

http://codereview.chromium.org/8334030/diff/15003/chrome/browser/prefs/pref_s...
File chrome/browser/prefs/pref_set_observer.cc (right):

http://codereview.chromium.org/8334030/diff/15003/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_set_observer.cc:68:
pref_set->AddPref(prefs::kSyncedDefaultSearchProviderGUID);
I'm not 100% certain that grouping kSyncedDefault... with the rest of these
prefs is the best approach given it's not actually a (potentially) managed pref
like the rest of these. However, it IS a pref related to the default search
provider. Let me know if this seems fine or not.

[SteveT, 2011-11-04 21:26:52]

+jeanluc

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

http://codereview.chromium.org/8334030/diff/15003/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_service.cc (left):

http://codereview.chromium.org/8334030/diff/15003/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:531: #if defined(OS_WIN)
Is this supposed to be part of this patch, or one addressing 102765?

http://codereview.chromium.org/8334030/diff/15003/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_service.cc (right):

http://codereview.chromium.org/8334030/diff/15003/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:460: i !=
template_urls_.end(); ++i) {
indent by one more

http://codereview.chromium.org/8334030/diff/15003/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:585: }
What about the else scenario in this case? Is it valid?

http://codereview.chromium.org/8334030/diff/15003/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:651: if (!pref_name ||
default_search_prefs_->IsObserved(*pref_name)) {
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...)

http://codereview.chromium.org/8334030/diff/15003/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:660: if
(new_default_search && !is_default_search_managed_) {
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)

http://codereview.chromium.org/8334030/diff/15003/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:667:
prefs->SetBoolean(prefs::kPendingSyncedDefaultSearchProviderChange,
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.

http://codereview.chromium.org/8334030/diff/15003/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:729: bool delete_default =
existing_turl == GetDefaultSearchProvider();
prefer paren around expression: (existing... == ...)

http://codereview.chromium.org/8334030/diff/15003/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:732: continue;
should this clear the SyncedDefaultSearchProviderGuid pref?

http://codereview.chromium.org/8334030/diff/15003/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:734: if (delete_default)
how about
if (delete default) {
  if (!is_default_search_managed_)
    default_search_provider_ = NULL;
  else
    ...
  SetDefaultSearchProvider(...)
}
Remove(...)

http://codereview.chromium.org/8334030/diff/15003/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:752:
CheckForSyncedDefaultSearchProvider(guid);
may as well just call this with turl->sync_guid()

http://codereview.chromium.org/8334030/diff/15003/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:862:
CheckForSyncedDefaultSearchProvider(guid);
same here

http://codereview.chromium.org/8334030/diff/15003/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_service_sync_unittest.cc
(right):

http://codereview.chromium.org/8334030/diff/15003/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_sync_unittest.cc:55:
pref_service->SetManagedPref(prefs::kDefaultSearchProviderEnabled,
Are these prefs always set all-at-once like this? I wonder (as a completely
separate idea from this patch) if they should just be combined into one
dictionary pref

http://codereview.chromium.org/8334030/diff/15003/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_sync_unittest.cc:1173:
prefs::kPendingSyncedDefaultSearchProviderChange));
add a case where the synceddefaultsearchproviderguid arrives for a sync guid
that doesn't exist yet?

http://codereview.chromium.org/8334030/diff/15003/chrome/browser/sync/test/in...
File chrome/browser/sync/test/integration/two_client_sessions_sync_test.cc
(right):

http://codereview.chromium.org/8334030/diff/15003/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/two_client_sessions_sync_test.cc:166:
ASSERT_EQ(10, GetClient(1)->GetLastSessionSnapshot()->
bleh. Can we go ahead and add a helper method in sync_test.h called
NumberOfDefaultItems(), and just have that+3 here (and similarly below)?

http://codereview.chromium.org/8334030/diff/15003/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/8334030/diff/15003/chrome/common/pref_names.cc...
chrome/common/pref_names.cc:261: // (and are not synced).
Mention that this is ignored (and not affected by) the case of the default
search provider being managed.

[SteveT, 2011-11-07 21:48:55]

Thanks for the initial look. I've addressed *most* of your concerns. PTAL and
let me know what you think.

Still waiting on some responses from Jean-Luc.

http://codereview.chromium.org/8334030/diff/15003/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_service.cc (left):

http://codereview.chromium.org/8334030/diff/15003/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:531: #if defined(OS_WIN)
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.

http://codereview.chromium.org/8334030/diff/15003/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_service.cc (right):

http://codereview.chromium.org/8334030/diff/15003/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:460: i !=
template_urls_.end(); ++i) {
On 2011/11/04 23:08:06, nzea wrote:
> indent by one more

Done.

http://codereview.chromium.org/8334030/diff/15003/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:585: }
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.

http://codereview.chromium.org/8334030/diff/15003/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:651: if (!pref_name ||
default_search_prefs_->IsObserved(*pref_name)) {
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?

http://codereview.chromium.org/8334030/diff/15003/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:660: if
(new_default_search && !is_default_search_managed_) {
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.

http://codereview.chromium.org/8334030/diff/15003/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:667:
prefs->SetBoolean(prefs::kPendingSyncedDefaultSearchProviderChange,
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.

http://codereview.chromium.org/8334030/diff/15003/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:729: bool delete_default =
existing_turl == GetDefaultSearchProvider();
On 2011/11/04 23:08:06, nzea wrote:
> prefer paren around expression: (existing... == ...)

Done.

http://codereview.chromium.org/8334030/diff/15003/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:732: continue;
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.

http://codereview.chromium.org/8334030/diff/15003/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:734: if (delete_default)
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.

http://codereview.chromium.org/8334030/diff/15003/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:752:
CheckForSyncedDefaultSearchProvider(guid);
We can't really do this since ownership of turl is given up when we call Add()
above.

http://codereview.chromium.org/8334030/diff/15003/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:862:
CheckForSyncedDefaultSearchProvider(guid);
As above.

http://codereview.chromium.org/8334030/diff/15003/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_service_sync_unittest.cc
(right):

http://codereview.chromium.org/8334030/diff/15003/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_sync_unittest.cc:55:
pref_service->SetManagedPref(prefs::kDefaultSearchProviderEnabled,
I believe the policy system allows an admin to set any combination of these
managed prefs at once, but generally they get set as a group at a time.
Jean-Luc: Any thoughts on this? I was following the format of your unit test for
managed DSPs. Should we be testing changes at a lower granularity both here and
in your unit tests?

http://codereview.chromium.org/8334030/diff/15003/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_sync_unittest.cc:1173:
prefs::kPendingSyncedDefaultSearchProviderChange));
Ah yes, of course :)

Added SyncedDefaultGUIDArrivesFirst to account for that case.

http://codereview.chromium.org/8334030/diff/15003/chrome/browser/sync/test/in...
File chrome/browser/sync/test/integration/two_client_sessions_sync_test.cc
(right):

http://codereview.chromium.org/8334030/diff/15003/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/two_client_sessions_sync_test.cc:166:
ASSERT_EQ(10, GetClient(1)->GetLastSessionSnapshot()->
All done, even though we unfortunately still have a hard-coded constant of 7
default synced items.

http://codereview.chromium.org/8334030/diff/15003/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/8334030/diff/15003/chrome/common/pref_names.cc...
chrome/common/pref_names.cc:261: // (and are not synced).
On 2011/11/04 23:08:06, nzea wrote:
> Mention that this is ignored (and not affected by) the case of the default
> search provider being managed.

Done.

[Raghu Simha, 2011-11-07 22:18:12]

Thanks for making these changes. Integration test changes LGTM pending green
bots.

http://codereview.chromium.org/8334030/diff/20003/chrome/browser/sync/test/in...
File chrome/browser/sync/test/integration/search_engines_helper.cc (right):

http://codereview.chromium.org/8334030/diff/20003/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/search_engines_helper.cc:150: LOG(INFO) <<
"A had default with URL: " << default_a->url()->url()
Is url()->url() a typo or do you need to go that deep to get the actual URL
string?

http://codereview.chromium.org/8334030/diff/20003/chrome/browser/sync/test/in...
File chrome/browser/sync/test/integration/search_engines_helper.h (right):

http://codereview.chromium.org/8334030/diff/20003/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/search_engines_helper.h:64: void
DeleteSearchEngine(int profile, const string16 keyword);
Optional: Since you've added DeleteSearchEngineBySeed, do you want to call this
DeleteSearchEngineByKeyword?

http://codereview.chromium.org/8334030/diff/20003/chrome/browser/sync/test/in...
File chrome/browser/sync/test/integration/sync_test.h (right):

http://codereview.chromium.org/8334030/diff/20003/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/sync_test.h:184: int NumberOfDefaultItems()
const;
nit: Perhaps name this NumberOfDefaultSyncItems() to make its purpose clear?

[SteveT, 2011-11-08 00:17:27]

Thanks for the look, Raghu. Made changes as you suggested and made some
adjustments to the flaky Sessions tests that I missed. Re-running tests.

http://codereview.chromium.org/8334030/diff/20003/chrome/browser/sync/test/in...
File chrome/browser/sync/test/integration/search_engines_helper.cc (right):

http://codereview.chromium.org/8334030/diff/20003/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/search_engines_helper.cc:150: LOG(INFO) <<
"A had default with URL: " << default_a->url()->url()
Yeah, I want to print the URL itself. This output is just for diagnostics.

http://codereview.chromium.org/8334030/diff/20003/chrome/browser/sync/test/in...
File chrome/browser/sync/test/integration/search_engines_helper.h (right):

http://codereview.chromium.org/8334030/diff/20003/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/search_engines_helper.h:64: void
DeleteSearchEngine(int profile, const string16 keyword);
Sure, that's definitely more clear. Done here, in search_engines_helper.cc, and
in two_client_search_engines_sync_test.cc where this is used.

http://codereview.chromium.org/8334030/diff/20003/chrome/browser/sync/test/in...
File chrome/browser/sync/test/integration/sync_test.h (right):

http://codereview.chromium.org/8334030/diff/20003/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/sync_test.h:184: int NumberOfDefaultItems()
const;
Sure. Done here and in sync_test.cc. Had to reformat the lines using this in
two_client_sessions_sync_test.cc. Let me know if you're happy with that change.

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

http://codereview.chromium.org/8334030/diff/21007/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_service.cc (right):

http://codereview.chromium.org/8334030/diff/21007/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:666:
prefs->GetString(prefs::kSyncedDefaultSearchProviderGUID));
Can GetString return NULL?  What's the behavior of GetTemplateURLForGUID then?

http://codereview.chromium.org/8334030/diff/21007/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:666:
prefs->GetString(prefs::kSyncedDefaultSearchProviderGUID));
Would GetPendingSyncedDefaultSearchProvder work here?

http://codereview.chromium.org/8334030/diff/21007/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:674:
pending_synced_default_search_ = true;
What if we get a second Observe call, this time where we could call
SetDefaultSearchProvider.  Does it set pending_synced_default_search to false?

http://codereview.chromium.org/8334030/diff/21007/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:738: continue;
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.

http://codereview.chromium.org/8334030/diff/21007/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:801:
prefs::kSyncedDefaultSearchProviderGUID);
Would GetPendingSyncedDefaultSearchProvder work here?

http://codereview.chromium.org/8334030/diff/21007/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:1839: const TemplateURL*
TemplateURLService::GetPendingSyncedDefaultSearchProvder() {
Provder -> Provider

http://codereview.chromium.org/8334030/diff/21007/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_service.h (right):

http://codereview.chromium.org/8334030/diff/21007/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.h:499: void
CheckForSyncedDefaultSearchProvider(const std::string& guid);
Seems like the "sets it as the default" is the more important action, so a name
like SetDefaultSearchProviderIfNewlySynced (or something like that) is more
descriptive.

[Nicolas Zea, 2011-11-08 19:57:26]

http://codereview.chromium.org/8334030/diff/15003/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_service.cc (right):

http://codereview.chromium.org/8334030/diff/15003/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:585: }
On 2011/11/07 21:48:56, SteveT wrote:
> 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.

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

http://codereview.chromium.org/8334030/diff/15003/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:651: if (!pref_name ||
default_search_prefs_->IsObserved(*pref_name)) {
On 2011/11/07 21:48:56, SteveT wrote:
> 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?

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

http://codereview.chromium.org/8334030/diff/21007/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_service.cc (right):

http://codereview.chromium.org/8334030/diff/21007/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:805: (!current_default ||
current_default->sync_guid() == default_guid))
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?

http://codereview.chromium.org/8334030/diff/21007/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_service.h (right):

http://codereview.chromium.org/8334030/diff/21007/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.h:586: // TemplateURL entry
it refers to.
As I understand it, this is not only for when we're waiting for the default
search provider to come in, but also when we're waiting for the default to
become unmanaged so we can set the sync default as the actual default right? 

Elaborate in comment?

[SteveT, 2011-11-08 21:40:03]

Nick/Jean-Luc. I've responded to all your comments - back to you guys for
another look. Thanks!


Steve

http://codereview.chromium.org/8334030/diff/21007/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_service.cc (right):

http://codereview.chromium.org/8334030/diff/21007/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:666:
prefs->GetString(prefs::kSyncedDefaultSearchProviderGUID));
On 2011/11/08 19:49:41, jeanluc1 wrote:
> Can GetString return NULL?  What's the behavior of GetTemplateURLForGUID then?

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.

http://codereview.chromium.org/8334030/diff/21007/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:666:
prefs->GetString(prefs::kSyncedDefaultSearchProviderGUID));
On 2011/11/08 19:49:41, jeanluc1 wrote:
> Would GetPendingSyncedDefaultSearchProvder work here?

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).

http://codereview.chromium.org/8334030/diff/21007/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:674:
pending_synced_default_search_ = true;
On 2011/11/08 19:49:41, jeanluc1 wrote:
> What if we get a second Observe call, this time where we could call
> SetDefaultSearchProvider.  Does it set pending_synced_default_search to false?

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

http://codereview.chromium.org/8334030/diff/21007/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:738: continue;
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.

http://codereview.chromium.org/8334030/diff/21007/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:801:
prefs::kSyncedDefaultSearchProviderGUID);
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).

http://codereview.chromium.org/8334030/diff/21007/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:805: (!current_default ||
current_default->sync_guid() == default_guid))
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!

http://codereview.chromium.org/8334030/diff/21007/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:1839: const TemplateURL*
TemplateURLService::GetPendingSyncedDefaultSearchProvder() {
On 2011/11/08 19:49:41, jeanluc1 wrote:
> Provder -> Provider

Done.

http://codereview.chromium.org/8334030/diff/21007/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_service.h (right):

http://codereview.chromium.org/8334030/diff/21007/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.h:499: void
CheckForSyncedDefaultSearchProvider(const std::string& guid);
Agreed. Done.

http://codereview.chromium.org/8334030/diff/21007/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.h:586: // TemplateURL entry
it refers to.
Yeah, that's correct. Beefed up the comment.

[Nicolas Zea, 2011-11-08 21:47:34]

LGTM, thanks!

[Raghu Simha, 2011-11-09 00:22:53]

Looks like you'll need to make two more changes to code that was landed
yesterday and got merged into this patch when you rebased.

http://codereview.chromium.org/8334030/diff/26001/chrome/browser/ui/webui/syn...
File chrome/browser/ui/webui/sync_setup_handler.cc (right):

http://codereview.chromium.org/8334030/diff/26001/chrome/browser/ui/webui/syn...
chrome/browser/ui/webui/sync_setup_handler.cc:207:
pref_service->GetBoolean(prefs::kSyncSearchEngines)) ||
Delete this line.

http://codereview.chromium.org/8334030/diff/26001/chrome/browser/ui/webui/syn...
chrome/browser/ui/webui/sync_setup_handler.cc:580:
types.find(syncable::SEARCH_ENGINES) != types.end());
Delete this line.

[SteveT, 2011-11-09 16:33:18]

Cool, thanks for pointing this out, Raghu! I've re-run associated tests and
things look good in this latest patch.

Just waiting on Jean-Luc's OK.

http://codereview.chromium.org/8334030/diff/26001/chrome/browser/ui/webui/syn...
File chrome/browser/ui/webui/sync_setup_handler.cc (right):

http://codereview.chromium.org/8334030/diff/26001/chrome/browser/ui/webui/syn...
chrome/browser/ui/webui/sync_setup_handler.cc:207:
pref_service->GetBoolean(prefs::kSyncSearchEngines)) ||
On 2011/11/09 00:22:53, rsimha wrote:
> Delete this line.

Done.

http://codereview.chromium.org/8334030/diff/26001/chrome/browser/ui/webui/syn...
chrome/browser/ui/webui/sync_setup_handler.cc:580:
types.find(syncable::SEARCH_ENGINES) != types.end());
On 2011/11/09 00:22:53, rsimha wrote:
> Delete this line.

Done.

[SteveT, 2011-11-10 14:45:52]

Ping - Hey Jean-Luc, could you take another look? Thanks.

[jeanluc1, 2011-11-12 22:58:33]

lgtm