Make prepopulated-TemplateURL-de-duper heuristic smarter. Instead of blindly preserving the first U…

Make prepopulated-TemplateURL-de-duper heuristic smarter. Instead of blindly preserving the first URL we get back from the keyword table, use the default search engine, or the URL that matches the prepopulate keyword, only after that falling back to using the lowest ID.

BUG=128216
TEST=unit_tests TemplateURLServiceUtilTest.*


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

[SteveT, 2012-05-17 13:34:54]

Hey Peter,

I've ensured that the first patchset is your original code, and the second
include my changes plus the tests, so you can see what I've been up to. There
are a bunch of comments inline discussing various decisions I've made.

One high level comment about the new template_url_service_util_unittest.cc file.
I noticed that template_url_service_unittest.cc has some basic tests for
handling prepopulate data. However, none of their tests allow you to set the
prepopulate data from template_url_prepopulate_data.*.

Instead of doing a ton of surgery to make this possible for our one test, I
created this new file that can be used in the future to test more of util.*
(which will be renamed).  Let me know your thoughts on this choice.

We'll need sky@ on OWNERS after I get your lg.tm.


Steve

https://chromiumcodereview.appspot.com/10409002/diff/4002/chrome/browser/sear...
File chrome/browser/search_engines/template_url_service_util_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10409002/diff/4002/chrome/browser/sear...
chrome/browser/search_engines/template_url_service_util_unittest.cc:29:
TEST(TemplateURLServiceUtilTest, RemoveDuplicatePrepopulateIDs) {
This covers all the heuristics used by RemoveDuplicatePrepopulateIDs. Let me
know if there are more edge cases worth testing.

https://chromiumcodereview.appspot.com/10409002/diff/4002/chrome/browser/sear...
chrome/browser/search_engines/template_url_service_util_unittest.cc:80: delete
*itr;
Manual cleanups suck, but sometimes they're necessary. Another possible method
is having the vector<TemplateURL>s instead be vector<scoped_ptr<TemplateURL> >s
instead, but that feels a bit wrong in that I haven't seen this pattern used
elsewhere.

Another possibility, if we add more test cases, is to make a Test class with two
methods to add to member-vectors of TemplateURLs. The TearDown() can then do the
manual delete for us (or use the scoped_ptr method).

https://chromiumcodereview.appspot.com/10409002/diff/4002/chrome/browser/sear...
File chrome/browser/search_engines/util.cc (right):

https://chromiumcodereview.appspot.com/10409002/diff/4002/chrome/browser/sear...
chrome/browser/search_engines/util.cc:103: if (service)
Since we're allowing the service to be NULL (as required by that other
callsite), we'll have to do the Remove only if it's provided, as before.

https://chromiumcodereview.appspot.com/10409002/diff/4002/chrome/browser/sear...
File chrome/browser/search_engines/util.h (right):

https://chromiumcodereview.appspot.com/10409002/diff/4002/chrome/browser/sear...
chrome/browser/search_engines/util.h:60: TemplateURLService::TemplateURLVector*
template_urls);
I'm exposing RemoveDuplicatePrepopulateIDs for direct testing by creating this
testing wrapper. I've been told this is an acceptable pattern, but do let me
know if you know a cleaner way of exposing a static helper for testing.

BTW it totally sucks that the methods in util.* are not part of a named
namespace. Happy to change this in the followup patch that renames util to
something better (proposing: template_url_service_util.[cc|h]).

[SteveT, 2012-05-17 15:01:45]

One more comment inline.

I've merged and fixed merge issues with my PreSyncDelete patch.

http://codereview.chromium.org/10409002/diff/9002/chrome/browser/search_engin...
File chrome/browser/search_engines/util.cc (right):

http://codereview.chromium.org/10409002/diff/9002/chrome/browser/search_engin...
chrome/browser/search_engines/util.cc:49: DCHECK(template_urls);
FYI you used to have a DCHECK(default_search_provider) here (not sure how it
left the diff) but it turns out it's possible for the caller to not have the DSP
yet. This was causing TemplateURLServiceTests and Chrome startup to crash.

[Peter Kasting, 2012-05-17 20:12:17]

https://chromiumcodereview.appspot.com/10409002/diff/4002/chrome/browser/sear...
File chrome/browser/search_engines/template_url_service_util_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10409002/diff/4002/chrome/browser/sear...
chrome/browser/search_engines/template_url_service_util_unittest.cc:43:
local_turls.push_back(CreatePrepopulateTemplateURL(0, "winner3", 6));
If after this block you add a "size_t num_non_prepopulated_urls =
local_turls.size();", then after the Remove call below you can do
"EXPECT_EQ(prepopulated_turls.size() + num_non_prepopulated_urls,
local_turls.size());".

https://chromiumcodereview.appspot.com/10409002/diff/4002/chrome/browser/sear...
chrome/browser/search_engines/template_url_service_util_unittest.cc:55:
local_turls.push_back(CreatePrepopulateTemplateURL(2, "loser4", 11));
Nit: Make this have keyword "xxx" to validate that default outranks keyword
match.

https://chromiumcodereview.appspot.com/10409002/diff/4002/chrome/browser/sear...
chrome/browser/search_engines/template_url_service_util_unittest.cc:70:
local_turls.begin(); itr != local_turls.end(); ++itr) {
Nit: {} not necessary

https://chromiumcodereview.appspot.com/10409002/diff/4002/chrome/browser/sear...
chrome/browser/search_engines/template_url_service_util_unittest.cc:80: delete
*itr;
On 2012/05/17 13:34:54, SteveT wrote:
> Manual cleanups suck, but sometimes they're necessary.

Use ScopedVector for |prepopulated_turls|.  Use STLElementDeleter (in
stl_util.h) for |local_turls|.

https://chromiumcodereview.appspot.com/10409002/diff/4002/chrome/browser/sear...
File chrome/browser/search_engines/util.cc (right):

https://chromiumcodereview.appspot.com/10409002/diff/4002/chrome/browser/sear...
chrome/browser/search_engines/util.cc:99: // Add it to the checked group; remove
it from elsewhere.
Nit: Let's just say "Add the best URL to the checked group and delete the rest."

https://chromiumcodereview.appspot.com/10409002/diff/4002/chrome/browser/sear...
chrome/browser/search_engines/util.cc:101: unchecked_urls.erase(best);
Instead of this, let's put an "if (i == best) continue" in the loop below.  This
way we won't actually touch change the map except in the single erase() call at
the end.  While doing the erase() here happens to be safe with a map since it
won't invalidate other map iterators, it _looks_ scary, and would break if
someone changed this to be a different container type.

https://chromiumcodereview.appspot.com/10409002/diff/4002/chrome/browser/sear...
File chrome/browser/search_engines/util.h (right):

https://chromiumcodereview.appspot.com/10409002/diff/4002/chrome/browser/sear...
chrome/browser/search_engines/util.h:60: TemplateURLService::TemplateURLVector*
template_urls);
On 2012/05/17 13:34:54, SteveT wrote:
> I'm exposing RemoveDuplicatePrepopulateIDs for direct testing by creating this
> testing wrapper.

Seems like at this point we might as well just declare the real helper method in
this file.  We can comment what it does, and then at the end say something like
"this is used as a helper by GetSearchProvidersUsingKeywordResult(), but
declared here so it's accessible by unittests."

https://chromiumcodereview.appspot.com/10409002/diff/9002/chrome/browser/sear...
File chrome/browser/search_engines/util.cc (right):

https://chromiumcodereview.appspot.com/10409002/diff/9002/chrome/browser/sear...
chrome/browser/search_engines/util.cc:49: DCHECK(template_urls);
On 2012/05/17 15:01:45, SteveT wrote:
> FYI you used to have a DCHECK(default_search_provider) here (not sure how it
> left the diff) but it turns out it's possible for the caller to not have the
DSP
> yet. This was causing TemplateURLServiceTests and Chrome startup to crash.

Yeah, this was a mistake.  I was thinking one level of indirection too many,
because I was thinking about how on the caller side we have a TemplateURL**, and
I wanted to DCHECK() that the first level of pointer was non-NULL.  But we deref
before calling this function, so that assertion is implied.  Removing the DCHECK
entirely here was the right thing to do.

[SteveT, 2012-05-17 21:27:22]

Okay, I believe I've addressed everything. PTAL and let me know if there is
anything else to take care of.

http://codereview.chromium.org/10409002/diff/4002/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_service_util_unittest.cc
(right):

http://codereview.chromium.org/10409002/diff/4002/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_util_unittest.cc:43:
local_turls.push_back(CreatePrepopulateTemplateURL(0, "winner3", 6));
On 2012/05/17 20:12:17, Peter Kasting wrote:
> If after this block you add a "size_t num_non_prepopulated_urls =
> local_turls.size();", then after the Remove call below you can do
> "EXPECT_EQ(prepopulated_turls.size() + num_non_prepopulated_urls,
> local_turls.size());".

Done.

http://codereview.chromium.org/10409002/diff/4002/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_util_unittest.cc:55:
local_turls.push_back(CreatePrepopulateTemplateURL(2, "loser4", 11));
On 2012/05/17 20:12:17, Peter Kasting wrote:
> Nit: Make this have keyword "xxx" to validate that default outranks keyword
> match.

Done.

http://codereview.chromium.org/10409002/diff/4002/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_util_unittest.cc:70:
local_turls.begin(); itr != local_turls.end(); ++itr) {
On 2012/05/17 20:12:17, Peter Kasting wrote:
> Nit: {} not necessary

Done.

http://codereview.chromium.org/10409002/diff/4002/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_util_unittest.cc:80: delete
*itr;
Awesome. Done.

http://codereview.chromium.org/10409002/diff/4002/chrome/browser/search_engin...
File chrome/browser/search_engines/util.cc (right):

http://codereview.chromium.org/10409002/diff/4002/chrome/browser/search_engin...
chrome/browser/search_engines/util.cc:99: // Add it to the checked group; remove
it from elsewhere.
On 2012/05/17 20:12:17, Peter Kasting wrote:
> Nit: Let's just say "Add the best URL to the checked group and delete the
rest."

Done.

http://codereview.chromium.org/10409002/diff/4002/chrome/browser/search_engin...
chrome/browser/search_engines/util.cc:101: unchecked_urls.erase(best);
Makes sense. Done.

http://codereview.chromium.org/10409002/diff/4002/chrome/browser/search_engin...
File chrome/browser/search_engines/util.h (right):

http://codereview.chromium.org/10409002/diff/4002/chrome/browser/search_engin...
chrome/browser/search_engines/util.h:60: TemplateURLService::TemplateURLVector*
template_urls);
Okay that's fair. Done.

[Peter Kasting, 2012-05-18 23:03:38]

LGTM

http://codereview.chromium.org/10409002/diff/13001/chrome/browser/search_engi...
File chrome/browser/search_engines/template_url_service_util_unittest.cc
(right):

http://codereview.chromium.org/10409002/diff/13001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_service_util_unittest.cc:47: 
Nit: Unnecessary newline

http://codereview.chromium.org/10409002/diff/13001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_service_util_unittest.cc:73:
EXPECT_EQ(local_turls.size(),
Nit: This can go in the block with the loop, just below the next comment

[SteveT, 2012-05-20 13:30:14]

Hey Peter - wanted your opinion on my latest patchset. Your change pulls the
retrieval of Pre-populated TURLs out of MergeEnginesFromPrepopulateData and into
its caller. So now if MergeEnginesFromPrepopulateData isn't called (whenever a
version number isn't updated), we end up with a leak. I resolved this by
manually deleting the engines in the case where MergeEnginesFromPrepopulateData
isn't called.

An alternative is to make |prepopulated_urls| a ScopedVector and passing a
pointer to that into MergeEnginesFromPrepopulateData. The object lifetime might
be a _bit_ clearer?

Do you have a preference?

http://codereview.chromium.org/10409002/diff/13001/chrome/browser/search_engi...
File chrome/browser/search_engines/template_url_service_util_unittest.cc
(right):

http://codereview.chromium.org/10409002/diff/13001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_service_util_unittest.cc:47: 
On 2012/05/18 23:03:38, Peter Kasting wrote:
> Nit: Unnecessary newline

Done.

http://codereview.chromium.org/10409002/diff/13001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_service_util_unittest.cc:73:
EXPECT_EQ(local_turls.size(),
On 2012/05/18 23:03:38, Peter Kasting wrote:
> Nit: This can go in the block with the loop, just below the next comment

Done.

[Peter Kasting, 2012-05-21 18:07:25]

Using scoping objects if possible is always good.

[commit-bot: I haz the power, 2012-05-24 17:40:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/10409002/11

[Peter Kasting, 2012-05-24 17:43:53]

https://chromiumcodereview.appspot.com/10409002/diff/11/chrome/browser/search...
File chrome/browser/search_engines/util.cc (right):

https://chromiumcodereview.appspot.com/10409002/diff/11/chrome/browser/search...
chrome/browser/search_engines/util.cc:144: ScopedVector<TemplateURL>&
prepopulated_urls,
Nit: Google style prohibits non-const refs; pass by pointer if you need to
modify.

[commit-bot: I haz the power, 2012-05-29 14:41:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/10409002/22002

[commit-bot: I haz the power, 2012-05-29 15:58:27]

Change committed as 139300