Ensure that TemplateURLs removed by prepopulate search engine merging is also removed from Sync.

Ensure that TemplateURLs removed by prepopulate search engine merging is also removed from Sync.

We do this by keeping a list of pre-sync deleted GUIDs, which is populated whenever a TemplateURL entry is removed from the DB during startup merging. We then use this list to ignore the appropriate entries from Sync, and to inform Sync that they should be deleted.

BUG=128210
TEST=unit_test TemplateURLServiceSyncTest.PreSyncDeletes


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

[SteveT, 2012-05-15 20:23:23]

Peter: PTAL. This is just the DELETE processing that we had discussed, and only
for the prepopulate delete-dupes-and-merge bits.

Likely at least one more patch to handle UPDATEs before we can close off 128210.

Nick: FYI, the pre-sync deletes stuff we had discussed.

[Peter Kasting, 2012-05-15 21:28:10]

https://chromiumcodereview.appspot.com/10384188/diff/4001/chrome/browser/sear...
File chrome/browser/search_engines/search_provider_install_data.cc (right):

https://chromiumcodereview.appspot.com/10384188/diff/4001/chrome/browser/sear...
chrome/browser/search_engines/search_provider_install_data.cc:246:
GetSearchProvidersUsingKeywordResult(*result,
Nit: This has so many arguments, feel free to condense onto fewer lines

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

https://chromiumcodereview.appspot.com/10384188/diff/4001/chrome/browser/sear...
chrome/browser/search_engines/template_url_service.cc:2231:
pre_sync_deletes_.insert(guid);
Why not just do this directly in the test?  It has access to do so anyway.

https://chromiumcodereview.appspot.com/10384188/diff/4001/chrome/browser/sear...
File chrome/browser/search_engines/template_url_service.h (right):

https://chromiumcodereview.appspot.com/10384188/diff/4001/chrome/browser/sear...
chrome/browser/search_engines/template_url_service.h:640: std::set<std::string>
pre_sync_deletes_;
Is there any way to keep this as a local that's passed in to the Merge function,
as opposed to a member?  Keeping it as state on the class instance itself feels
slightly wrong to me for some reason.

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

https://chromiumcodereview.appspot.com/10384188/diff/4001/chrome/browser/sear...
chrome/browser/search_engines/template_url_service_sync_unittest.cc:1539: }
Can we also explicitly check that a pre-sync delete of a GUID that sync doesn't
know about is not turned into an ACTION_DELETE, and that the model's list of
deletes is cleared after the Merge() call (if you end up having to keep that
list as a member -- this is unnecessary if it's passed in as an argument)?

[SteveT, 2012-05-16 13:26:43]

PTAL.

http://codereview.chromium.org/10384188/diff/4001/chrome/browser/search_engin...
File chrome/browser/search_engines/search_provider_install_data.cc (right):

http://codereview.chromium.org/10384188/diff/4001/chrome/browser/search_engin...
chrome/browser/search_engines/search_provider_install_data.cc:246:
GetSearchProvidersUsingKeywordResult(*result,
On 2012/05/15 21:28:10, Peter Kasting wrote:
> Nit: This has so many arguments, feel free to condense onto fewer lines

Done.

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

http://codereview.chromium.org/10384188/diff/4001/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:2231:
pre_sync_deletes_.insert(guid);
True. Removed this method.

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

http://codereview.chromium.org/10384188/diff/4001/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.h:640: std::set<std::string>
pre_sync_deletes_;
That's a bit difficult. The reason is because the method where this set is used,
MergeDataAndStartSyncing, is 1) a overridden virtual from some sync interface
that we can't change, and 2) is actually called by the Sync internals and not by
us. So that makes it rather hard to get that local from
OnWebDataServiceRequestDone over to MergeDataAndStartSyncing.

I see why you're concerned, since this set is very much so a one-off thing that
is only used (most of the time) around startup. Happy to make this change if you
can come up with some way to get the data into MergeDataAndStartSyncing without
adding new members.

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

http://codereview.chromium.org/10384188/diff/4001/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_sync_unittest.cc:1539: }
I believe that is done with the "aaa" case. See lines 1523 and 1538.

I've added the EXPECT for the empty list at the end. Good call.

[Peter Kasting, 2012-05-16 18:18:50]

LGTM.  I don't know of an obvious way to fix the MergeDataAndStartSyncing()
issue without major surgery to the sync system.  It might be nice for sync to
have some sort of generalized way to call "this service's MergeData function"
and pass along some bundle of locally-important data, but that certainly
shouldn't go in this patch, and may not even make any sense at all (because I
just invented that idea a second ago).

[commit-bot: I haz the power, 2012-05-16 18:28:45]

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

[commit-bot: I haz the power, 2012-05-16 20:07:07]

Change committed as 137490