Retroactively addressed sky@'s comments from CL http://codereview.chromium.org/7566036/

Retroactively addressed sky@'s comments from CL http://codereview.chromium.org/7566036/. Added a unit test along side TemplateURLService's SyncableService changes.

BUG=15548
TEST=Ensure new test (TemplateURLServiceSyncTest.MergeTwiceWithSameSyncData) passes.

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

[SteveT, 2011-09-01 14:21:54]

Hey Scott -

As promised, I've addressed all of your previous comments from my last
TemplateURLService-related sync patch. I've copied over your comments, inline,
so you don't have to look at the previous patch.

Nick - Please have a look at the new unit test. Now that we copy over all values
from sync into the local search engine entry, we should also pick up the
last_modified timestamp from sync... which means that the next time we START to
sync (if the entry did not change), we should not modify it on merge.

PTAL. Thanks!


Steve

http://codereview.chromium.org/7826003/diff/1005/chrome/browser/search_engine...
File chrome/browser/search_engines/template_url_service.cc (left):

http://codereview.chromium.org/7826003/diff/1005/chrome/browser/search_engine...
chrome/browser/search_engines/template_url_service.cc:500:
GetSearchProvidersUsingKeywordResult(*result,
sky 2011/08/17 23:22:29
This may end up modifying the set of TemplateURLs (at least the prepopulate
ones). You'll need to deal with that in some way. In fact it's worth a test case
to make sure migration gets handled correctly.

stevet: Got it. I won't address this here, but I'll make sure we have a test
case that covers it when we get to integration testing.

http://codereview.chromium.org/7826003/diff/1005/chrome/browser/search_engine...
chrome/browser/search_engines/template_url_service.cc:691: return error;
sky 2011/08/17 23:22:29
Are you sure we want to return here and not continue on? Say you have two
machines, A and B. A is connected, B is offline. If you delete the same TURL on
both machines, then B becomes connected, might you hit this branch? Maybe
MergeData happens first so this isn't an issue?

stevet: Fixed. We'll return the last error we see from this loop.

http://codereview.chromium.org/7826003/diff/1005/chrome/browser/search_engine...
chrome/browser/search_engines/template_url_service.cc:701: SyncError
TemplateURLService::MergeDataAndStartSyncing(
sky 2011/08/17 23:22:29
Is it guaranteed these methods are only invoked after loading has completed?
Could you add a DCHECK to that effect?

stevet: DCHECK added.

http://codereview.chromium.org/7826003/diff/1005/chrome/browser/search_engine...
chrome/browser/search_engines/template_url_service.cc:739: sync_turl->url() ?
sync_turl->url()->url() : std::string());
sky 2011/08/17 23:22:29
I believe you'll also want safe_for_autoreplace copied over too, otherwise any
synced TURL will end up with safe_for_autoreplace false.

stevet: We now copy every cloud value over except the local ID (which we don't
sync anymore). This should preserve |safe_for_autoreplace|.

http://codereview.chromium.org/7826003/diff/1005/chrome/browser/search_engine...
chrome/browser/search_engines/template_url_service.cc:877: specifics);
sky 2011/08/17 23:22:29
Is there any protection to make sure we don't end up with duplicate ids? In fact
do we even need to sync the id?

stevet: Removed |id| in patch http://codereview.chromium.org/7669052/ (and
|logo_id| as well)

http://codereview.chromium.org/7826003/diff/1005/chrome/browser/search_engine...
chrome/browser/search_engines/template_url_service.cc:1679: string16
new_keyword;
sky 2011/08/17 23:22:29
nit: pull into if as you don't use it outside the if.

stevet: Done.

http://codereview.chromium.org/7826003/diff/1005/chrome/browser/search_engine...
File chrome/browser/search_engines/template_url_service.h (left):

http://codereview.chromium.org/7826003/diff/1005/chrome/browser/search_engine...
chrome/browser/search_engines/template_url_service.h:134: // The caller should
not try to delete the returned pointer; the data store
sky 2011/08/17 23:22:29
nit: Returns the TemplateURL with the specified guid, or NULL if not found.

stevet: Done.

http://codereview.chromium.org/7826003/diff/1005/chrome/browser/search_engine...
chrome/browser/search_engines/template_url_service.h:475: // Given a TemplateURL
from Sync, resolves any keyword conflicts by checking
sky 2011/08/17 23:22:29
nit: add (cloud) after Sync.

stevet: Done.

http://codereview.chromium.org/7826003/diff/1005/chrome/browser/search_engine...
chrome/browser/search_engines/template_url_service.h:484: SyncChangeList&
change_list);
sky 2011/08/17 23:22:29
Because this may modify change_list, it should be a pointer.

stevet: Done.

http://codereview.chromium.org/7826003/diff/1005/chrome/browser/search_engine...
chrome/browser/search_engines/template_url_service.h:500: SyncChangeList&
change_list);
sky 2011/08/17 23:22:29
change_list should be a pointer here too.

stevet: Done.

[sky, 2011-09-01 17:12:27]

LGTM thanks!

[SteveT, 2011-09-02 20:58:08]

Ping - Nick, could you take a look? No rush, but it'd be nice to get this in
before the M15 cut as it polishes off a few bugs in search engine sync.

[Nicolas Zea, 2011-09-03 18:35:14]

Lgtn

[Nicolas Zea, 2011-09-03 18:35:31]

Err, lgtm

[commit-bot: I haz the power, 2011-09-03 23:06:05]

Change committed as 99578