Made the deletion of the default search provider defensive in TemplateURLService.

Made the deletion of the default search provider defensive in TemplateURLService.

Instead of allowing the Sync code to delete the DSP, we instead undelete the DSP. Here we prefer inconsistency to a far worse experience where the DSP is swapped due to a bug.

Tests are updated to reflect this change in functionality.

BUG=132446
TEST=


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

[SteveT, 2012-07-09 20:12:35]

Hey Nick,

Here is the defensive fix we had discussed in one SE sync fixup conversation.
Let me know what you think. I still want to run through some manual tests before
committing this.


Steve

[Nicolas Zea, 2012-07-10 17:07:47]

LGTM

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

http://codereview.chromium.org/10694096/diff/5001/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:944: // unexpected swapped
to something else. The user can safely delete the
unexpected -> unexpectedly

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

http://codereview.chromium.org/10694096/diff/5001/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_sync_unittest.cc:1678:
undelete.sync_data().GetSpecifics().search_engine().keyword());
verify that the sync guid stayed the same?

[SteveT, 2012-07-10 17:58:35]

Will commit once you've responded to the one question inline.

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

http://codereview.chromium.org/10694096/diff/5001/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:944: // unexpected swapped
to something else. The user can safely delete the
On 2012/07/10 17:07:47, nzea wrote:
> unexpected -> unexpectedly

Done.

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

http://codereview.chromium.org/10694096/diff/5001/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_sync_unittest.cc:1678:
undelete.sync_data().GetSpecifics().search_engine().keyword());
On 2012/07/10 17:07:47, nzea wrote:
> verify that the sync guid stayed the same?

This was done above in 1673. Did you mean something else?

[Nicolas Zea, 2012-07-10 19:10:12]

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

http://codereview.chromium.org/10694096/diff/5001/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_sync_unittest.cc:1678:
undelete.sync_data().GetSpecifics().search_engine().keyword());
On 2012/07/10 17:58:35, SteveT wrote:
> On 2012/07/10 17:07:47, nzea wrote:
> > verify that the sync guid stayed the same?
> 
> This was done above in 1673. Did you mean something else?
> 

Ah, I just realized you're doing a lookup based on the guid in the first place
(line 1675). Gotcha, disregard.

[commit-bot: I haz the power, 2012-07-10 19:57:13]

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

[commit-bot: I haz the power, 2012-07-10 21:50:20]

Try job failure for 10694096-15001 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-07-10 21:52:35]

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

[commit-bot: I haz the power, 2012-07-10 23:17:59]

Change committed as 145984

[Peter Kasting, 2012-07-11 00:36:14]

Hey, can we add some histogram or other form of data collection to see how much
this is actually hit?

[SteveT, 2012-07-11 13:11:28]

We recently added (just before this patch) a new Histogram that counts calls to
FIrstPotentialDefaultEngine. (http://codereview.chromium.org/10701103/)

This includes one entry point where FIrstPotentialDefaultEngine is called during
a sync cycle (ie - ProcessSyncChanges or MergeDataAndStartSyncing).

If you don't think that's well correlated enough, we could add something to
track calls to this specific bit of code that performs the undelete.