When the Google base URL changes and causes keyword updates, save those to the database.

When the Google base URL changes and causes keyword updates, save those to the database and send them to sync.  Note that even if the database had the wrong keyword, a TemplateURL would get reset to the correct keyword on next load (in the model, not in the database), so this is mostly useful for keeping state as consistent as possible rather than preventing visible local problems.

One tricky bit here is that we don't want these sorts of changes to change the keywords of existing URLs in other sync clients, lest we give an entry the wrong keyword for the local environment. I could have dealt with this by never sending such updates to sync at all, but given that remote clients need to deal with these sorts of issues anyway for other reasons (e.g. a user adding a new TemplateURL that uses Google base URLs), it seemed less error-prone to use the standard UpdateNoNotify() functionality and then ensure remote clients handle things correctly. This will also ensure that if we e.g. change the TemplateURL's last_modified date when updating the base URL, that change will get propagated to sync, which is (once again) good for keeping the server and all clients seeing the most consistent state possible.

BUG=128249
TEST=none
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=140608

[SteveT, 2012-06-05 15:11:42]

Looking good. Small things inline.

http://codereview.chromium.org/10458077/diff/24001/chrome/browser/search_engi...
File chrome/browser/search_engines/template_url_service.cc (right):

http://codereview.chromium.org/10458077/diff/24001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_service.cc:1823:
KeywordToTemplateMap::const_iterator i =
nit: I always found it confusing and bad that this original code used the name
|i| twice in this block. Mind changing one  or both to better names?

http://codereview.chromium.org/10458077/diff/24001/chrome/browser/search_engi...
File chrome/browser/search_engines/template_url_service.h (left):

http://codereview.chromium.org/10458077/diff/24001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_service.h:267: // . PREF_CHANGED:
checks whether the default search engine has changed.
nit: Why did you remove this comment? Do we not document what we do in the
overriding class?

http://codereview.chromium.org/10458077/diff/24001/chrome/browser/search_engi...
File chrome/browser/search_engines/template_url_service_sync_unittest.cc
(right):

http://codereview.chromium.org/10458077/diff/24001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_service_sync_unittest.cc:1630:
PassProcessor(), CreateAndPassSyncErrorFactory());
Hm, are we missing something here that changes the base URL again and verifies
that we send out a SyncChange?

Or does the second bit in SyncBaseURLs below handle that case?

[Peter Kasting, 2012-06-05 20:49:03]

https://chromiumcodereview.appspot.com/10458077/diff/24001/chrome/browser/sea...
File chrome/browser/search_engines/template_url_service.cc (right):

https://chromiumcodereview.appspot.com/10458077/diff/24001/chrome/browser/sea...
chrome/browser/search_engines/template_url_service.cc:1823:
KeywordToTemplateMap::const_iterator i =
On 2012/06/05 15:11:42, SteveT wrote:
> nit: I always found it confusing and bad that this original code used the name
> |i| twice in this block. Mind changing one  or both to better names?

Done.

https://chromiumcodereview.appspot.com/10458077/diff/24001/chrome/browser/sea...
File chrome/browser/search_engines/template_url_service.h (left):

https://chromiumcodereview.appspot.com/10458077/diff/24001/chrome/browser/sea...
chrome/browser/search_engines/template_url_service.h:267: // . PREF_CHANGED:
checks whether the default search engine has changed.
On 2012/06/05 15:11:42, SteveT wrote:
> nit: Why did you remove this comment? Do we not document what we do in the
> overriding class?

The comment was already inaccurate and incomplete, and I figured if people want
to know what we observe, they can look at the function definition, where the
behavior is either obvious or commented already.

https://chromiumcodereview.appspot.com/10458077/diff/24001/chrome/browser/sea...
File chrome/browser/search_engines/template_url_service_sync_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10458077/diff/24001/chrome/browser/sea...
chrome/browser/search_engines/template_url_service_sync_unittest.cc:1630:
PassProcessor(), CreateAndPassSyncErrorFactory());
On 2012/06/05 15:11:42, SteveT wrote:
> Hm, are we missing something here that changes the base URL again and verifies
> that we send out a SyncChange?
> 
> Or does the second bit in SyncBaseURLs below handle that case?

Yeah, this fragment was a leftover from before I combined all the tests into
one.  Removed.

[SteveT, 2012-06-05 20:57:12]

lgtm