Ensure post-sync TemplateURL of prepopulated engines use built-in version.

This patch ensures that whenever a sync operation in TemplateURLService adds or updates a TemplateURL corresponding to a prepopulated engine, the latest built-in version of the prepopulated engine data is used.

We also bump the the version number of prepopulated_engines.json to fix currently broken databases.

BUG=171350
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=180627

[beaudoin, 2013-01-30 22:11:29]

Hey Peter. This implements the fix we discussed on issue 171350, if you could
just make sure the approach is sane. Thanks!

[Peter Kasting, 2013-01-30 22:32:33]

No tests.  Can we write a test?

https://codereview.chromium.org/12084076/diff/4001/chrome/browser/search_engi...
File chrome/browser/search_engines/template_url_prepopulate_data.cc (right):

https://codereview.chromium.org/12084076/diff/4001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_prepopulate_data.cc:1217: if (engine
== NULL)
Nit: Or:

return engine ? MakePrepopulatedTemplateURLFromPrepopulateEngine(profile,
*engine) : NULL;

https://codereview.chromium.org/12084076/diff/4001/chrome/browser/search_engi...
File chrome/browser/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/12084076/diff/4001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_service.cc:1815: if
(prepopulated_turl.get() != NULL) {
Nit: You can remove ".get() != NULL" at this point, I believe

https://codereview.chromium.org/12084076/diff/4001/chrome/browser/search_engi...
File chrome/browser/search_engines/template_url_service.h (right):

https://codereview.chromium.org/12084076/diff/4001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_service.h:451: // prepopulated URL.
TemplateURL not coming from a prepopulated URL are not
Nit: TemplateURLs

https://codereview.chromium.org/12084076/diff/4001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_service.h:453: void
UpdatePrepopulatedTemplateURL(TemplateURL* existing_turl);
Nit: Seems like the function could be named better -- it implies that
|existing_turl| is a prepopulated TemplateURL.

There's also no commentary as to why we do this or what happens if we don't.

[beaudoin, 2013-02-01 21:12:38]

Modified my approach because of the multiple-engines-share-the-same-id problem
(see bug description). I'm not 100% satisfied with this since of the potential
sync-across-country problem (which already exists in the current merge process)
and because I'm new-ing/delete-ing 3 or 4 engines in that ScopedVector everytime
an engine is added or updated in the template_url_service. It shouldn't happen
too often though.

I'm writing tests right now, but wanted to send that out for sanity check.

https://codereview.chromium.org/12084076/diff/4001/chrome/browser/search_engi...
File chrome/browser/search_engines/template_url_prepopulate_data.cc (right):

https://codereview.chromium.org/12084076/diff/4001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_prepopulate_data.cc:1217: if (engine
== NULL)
On 2013/01/30 22:32:33, Peter Kasting wrote:
> Nit: Or:
> 
> return engine ? MakePrepopulatedTemplateURLFromPrepopulateEngine(profile,
> *engine) : NULL;

Function gone.

https://codereview.chromium.org/12084076/diff/4001/chrome/browser/search_engi...
File chrome/browser/search_engines/template_url_service.h (right):

https://codereview.chromium.org/12084076/diff/4001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_service.h:451: // prepopulated URL.
TemplateURL not coming from a prepopulated URL are not
On 2013/01/30 22:32:33, Peter Kasting wrote:
> Nit: TemplateURLs

Done.

https://codereview.chromium.org/12084076/diff/4001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_service.h:453: void
UpdatePrepopulatedTemplateURL(TemplateURL* existing_turl);
On 2013/01/30 22:32:33, Peter Kasting wrote:
> Nit: Seems like the function could be named better -- it implies that
> |existing_turl| is a prepopulated TemplateURL.
> 
> There's also no commentary as to why we do this or what happens if we don't.

Done.

[Peter Kasting, 2013-02-01 22:47:06]

I agree with you that the efficiency seems a bit low.  I think the core of this
is that really, we don't actually need to do this on every Add/Update call.  For
example, when the user manually modifies one of these engines, it will already
comply with the restrictions we're enforcing here.  Similarly when we rev the
prepopulate data or (assuming the restrictions are enforced on the data we write
to the database) when we read data out of the DB at startup.

So I think we should enumerate the places that could make engines violate these
checks, and restrict our mucking to there.  In my mind, that means probably just
sync.  (We should also think through the "engines specified by policy" cases and
make sure all those do the right thing).

https://codereview.chromium.org/12084076/diff/7002/chrome/browser/search_engi...
File chrome/browser/search_engines/util.cc (right):

https://codereview.chromium.org/12084076/diff/7002/chrome/browser/search_engi...
chrome/browser/search_engines/util.cc:140:
DCHECK(original_turl->prepopulate_id() == prepopulated_url->prepopulate_id);
Nit: DCHECK_EQ

[beaudoin, 2013-02-02 00:02:28]

Moved the prepopulated engine merge to sync-only. Also bumped the version number
of prepopulated_engines.json since already-corrupted webdatabases will not be
upgraded otherwise.

Still working on a good test...

[beaudoin, 2013-02-02 04:03:36]

Here's the version with the tests, PTAL.

[beaudoin, 2013-02-04 17:24:04]

+stevet for comments on unit tests.

[SteveT, 2013-02-04 19:00:18]

Looking good. A few things inline. Thanks!

https://codereview.chromium.org/12084076/diff/25001/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url_service_sync_unittest.cc
(right):

https://codereview.chromium.org/12084076/diff/25001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service_sync_unittest.cc:229: // the
|url| and |guid|. The caller owns the returned TemplateURL*.
Do you want to mention something about what you do to the timestamps?

https://codereview.chromium.org/12084076/diff/25001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service_sync_unittest.cc:2158:
EXPECT_EQ(default_turl->url(), result_turl->url());
Some of the above seem to be repeated a few times. Is this something we can wrap
in a helper?

https://codereview.chromium.org/12084076/diff/25001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service_sync_unittest.cc:2227: 
extra break here not needed.

https://codereview.chromium.org/12084076/diff/25001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service_sync_unittest.cc:2259: 
extra break here not needed

[beaudoin, 2013-02-04 20:01:31]

https://codereview.chromium.org/12084076/diff/25001/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url_service_sync_unittest.cc
(right):

https://codereview.chromium.org/12084076/diff/25001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service_sync_unittest.cc:229: // the
|url| and |guid|. The caller owns the returned TemplateURL*.
On 2013/02/04 19:00:18, SteveT wrote:
> Do you want to mention something about what you do to the timestamps?

Done.

https://codereview.chromium.org/12084076/diff/25001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service_sync_unittest.cc:2158:
EXPECT_EQ(default_turl->url(), result_turl->url());
On 2013/02/04 19:00:18, SteveT wrote:
> Some of the above seem to be repeated a few times. Is this something we can
wrap
> in a helper?

I usually prefer to keep the expectations right in the test, makes the code
easier to read and the failure message more explicit IMHO. I extracted most of
the common initialization code into CopyTemplateURL. I could maybe squeeze a bit
more out, but I'm not sure it would increase readability. Let me know if you
feel strongly about this and I will revisit.

https://codereview.chromium.org/12084076/diff/25001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service_sync_unittest.cc:2227: 
On 2013/02/04 19:00:18, SteveT wrote:
> extra break here not needed.

Done.

https://codereview.chromium.org/12084076/diff/25001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service_sync_unittest.cc:2259: 
On 2013/02/04 19:00:18, SteveT wrote:
> extra break here not needed

Done.

[Peter Kasting, 2013-02-04 20:35:32]

Are all the search_terms_replacement_key changes meant to go in this CL?  Your
description doesn't mention them.

https://codereview.chromium.org/12084076/diff/20002/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/12084076/diff/20002/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service.cc:1322:
UpdateTemplateURLIfPrepopulated(turl, profile);
Could this result in telling sync to go update these values to something else
for this entry on other clients?

If so, we could have endless fighting between two clients with different
prepopulate versions.

https://codereview.chromium.org/12084076/diff/20002/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service.cc:1763: 
Why add this?

https://codereview.chromium.org/12084076/diff/20002/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url_service_sync_unittest.cc
(right):

https://codereview.chromium.org/12084076/diff/20002/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service_sync_unittest.cc:234: time_t
last_mod = 100);
Google style guide bans default arguments

[SteveT, 2013-02-04 21:05:44]

https://codereview.chromium.org/12084076/diff/20002/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/12084076/diff/20002/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service.cc:1322:
UpdateTemplateURLIfPrepopulated(turl, profile);
At first glance, I don't believe this would be the case. What happens is that
the local TURL gets its URL updated/repaired, but it doesn't actually sync back
up to the server.

From the client's point of view, it looks like the server sent down a TURL which
already has the repaired URL.

However, this does mean that the client is momentarily out of sync with the
server (the client has an updated URL and the server doesn't know about it..
yet). It seems to me like the correct thing to do here is that, in the case of
upgrading a client's TURL, we want to send that new state back up to the server
via an ACTION_UPDATE.

However, the one issue here is that older clients synced to this account will
receive the updated URL, which it may not know how to handle. However, it sounds
like the TemplateURLService code knows what to do in this case (drop the
parameter entirely).

What's your take on this problem, here? I believe we do support such cases (not
necessarily the case of syncing between say Dev and Stable, but syncing between
one client that has updated recently, and one that has not).

[Peter Kasting, 2013-02-04 21:40:08]

https://codereview.chromium.org/12084076/diff/20002/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/12084076/diff/20002/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service.cc:1322:
UpdateTemplateURLIfPrepopulated(turl, profile);
On 2013/02/04 21:05:44, SteveT wrote:
> However, this does mean that the client is momentarily out of sync with the
> server (the client has an updated URL and the server doesn't know about it..
> yet). It seems to me like the correct thing to do here is that, in the case of
> upgrading a client's TURL, we want to send that new state back up to the
server
> via an ACTION_UPDATE.
> 
> However, the one issue here is that older clients synced to this account will
> receive the updated URL, which it may not know how to handle. However, it
sounds
> like the TemplateURLService code knows what to do in this case (drop the
> parameter entirely).

No, this is the very case I'm worried about.  If you were to send this back to
the server, it would then send that to the other clients, including the one who
triggered the original update here.  _That_ client must have differing
prepopulate data (in order for it to have sent said original update at all), and
the new update would run through this code, be reset to the existing local URL,
get bounced back to the server as an ACTION_UPDATE, repeat forever (well, until
both clients update to the same prepopulate version).

I think any changes we cause due to this new function must not get mirrored to
the server.  If they are later sent to the server because e.g. the user
subsequently edits the entry's keyword, that's fine; as long as other clients
obey this "don't auto-mirror updates due to enforcing the prepopulate data"
invariant, we won't chain-reaction.

[beaudoin, 2013-02-04 22:33:40]

I've discussed this offline with Steve and we agree with you that we must _not_
let the sync server of if that change.

This CL will not cause any ping-pong issue, however. The reason is that whenever
this code is called the processing_syncer_changes_ is true, which means update
will not ping the sync server.

This gating was required even before we introduced that change since the 
current logic causes the sync server to be pinged whenever a TemplateURL is
added or updated -- even if the update did not actually change anything.

https://codereview.chromium.org/12084076/diff/20002/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/12084076/diff/20002/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service.cc:1763: 
On 2013/02/04 20:35:32, Peter Kasting wrote:
> Why add this?

Unfortunate back-and-forth editing mistake.

Done.

https://codereview.chromium.org/12084076/diff/20002/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url_service_sync_unittest.cc
(right):

https://codereview.chromium.org/12084076/diff/20002/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service_sync_unittest.cc:234: time_t
last_mod = 100);
On 2013/02/04 20:35:32, Peter Kasting wrote:
> Google style guide bans default arguments

I got the idea from CreateTestTemplateURL above. :/  Should I correct this
everywhere?

I've removed this one as it's not really needed.

[Peter Kasting, 2013-02-04 23:52:29]

LGTM

https://codereview.chromium.org/12084076/diff/20002/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url_service_sync_unittest.cc
(right):

https://codereview.chromium.org/12084076/diff/20002/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service_sync_unittest.cc:234: time_t
last_mod = 100);
On 2013/02/04 22:33:40, beaudoin wrote:
> On 2013/02/04 20:35:32, Peter Kasting wrote:
> > Google style guide bans default arguments
> 
> I got the idea from CreateTestTemplateURL above. :/  Should I correct this
> everywhere?
> 
> I've removed this one as it's not really needed.

I wouldn't bother trying to fix existing usage, especially because
CreateTestTemplateURL() in particular is widely called.

[commit-bot: I haz the power, 2013-02-05 02:19:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/beaudoin@chromium.org/12084076/34001

[commit-bot: I haz the power, 2013-02-05 07:41:50]

Change committed as 180627