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

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

Add a regression test to verify this.

BUG=142872
TEST=Ensure that bug 142872 is no longer reproducible.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=152156

[SteveT, 2012-08-15 21:17:07]

PTAL.

This resolves the DSP syncing issue I mentioned after my rewrite.

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

couple clarification questions

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

http://codereview.chromium.org/10831338/diff/5001/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:759: TemplateURL*
synced_default = GetPendingSyncedDefaultSearchProvider();
Doesn't this logic handle the case this patch addresses? Or was there something
else involved? (see comment below)

http://codereview.chromium.org/10831338/diff/5001/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:1094:
pending_synced_default_search_ = true;
we should be hitting this logic right?

http://codereview.chromium.org/10831338/diff/5001/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:1159: // Attempt to reset
the default search provider incase it was changed
incase -> in case

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

Responses inline.

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

http://codereview.chromium.org/10831338/diff/5001/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:759: TemplateURL*
synced_default = GetPendingSyncedDefaultSearchProvider();
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).

http://codereview.chromium.org/10831338/diff/5001/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:1094:
pending_synced_default_search_ = true;
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.

http://codereview.chromium.org/10831338/diff/5001/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:1159: // Attempt to reset
the default search provider incase it was changed
On 2012/08/15 21:46:28, nzea wrote:
> incase -> in case

Done.

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

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

http://codereview.chromium.org/10831338/diff/5001/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:759: TemplateURL*
synced_default = GetPendingSyncedDefaultSearchProvider();
On 2012/08/15 21:56:57, SteveT wrote:
> 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).

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

http://codereview.chromium.org/10831338/diff/5001/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:1161:
SetDefaultSearchProviderIfNewlySynced(local_turl->sync_guid());
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]

Back to you. Thanks!

https://chromiumcodereview.appspot.com/10831338/diff/5001/chrome/browser/sear...
File chrome/browser/search_engines/template_url_service.cc (right):

https://chromiumcodereview.appspot.com/10831338/diff/5001/chrome/browser/sear...
chrome/browser/search_engines/template_url_service.cc:759: TemplateURL*
synced_default = GetPendingSyncedDefaultSearchProvider();
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.

https://chromiumcodereview.appspot.com/10831338/diff/5001/chrome/browser/sear...
chrome/browser/search_engines/template_url_service.cc:1161:
SetDefaultSearchProviderIfNewlySynced(local_turl->sync_guid());
That makes a lot of sense for MergeDataAndStartSyncing, and I've made the
changes to reflect this.

[Nicolas Zea, 2012-08-16 20:03:03]

LGTM

https://chromiumcodereview.appspot.com/10831338/diff/6003/chrome/browser/sear...
File chrome/browser/search_engines/template_url_service_sync_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10831338/diff/6003/chrome/browser/sear...
chrome/browser/search_engines/template_url_service_sync_unittest.cc:1601:
TemplateURL* expected_default = model()->GetDefaultSearchProvider();
nit: this isn't the expected, but the current right? expected is key2.

[SteveT, 2012-08-16 20:24:00]

Cool. Thanks for the review. Will commit after another pass of manual tests.

https://chromiumcodereview.appspot.com/10831338/diff/6003/chrome/browser/sear...
File chrome/browser/search_engines/template_url_service_sync_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10831338/diff/6003/chrome/browser/sear...
chrome/browser/search_engines/template_url_service_sync_unittest.cc:1601:
TemplateURL* expected_default = model()->GetDefaultSearchProvider();
Right, renamed this variable.

[commit-bot: I haz the power, 2012-08-17 15:58:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevet@chromium.org/10831338/7003

[commit-bot: I haz the power, 2012-08-17 22:25:57]

Change committed as 152156