Fix TemplateURLFetcherTest.* for Android.

Fix TemplateURLFetcherTest.* for Android.


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

[sky, 2012-11-19 22:50:00]

https://codereview.chromium.org/11348135/diff/1/chrome/browser/search_engines...
File chrome/browser/search_engines/template_url_service_test_util.cc (right):

https://codereview.chromium.org/11348135/diff/1/chrome/browser/search_engines...
chrome/browser/search_engines/template_url_service_test_util.cc:122:
TemplateURLPrepopulateData::InitCountryCode("US");
This seems like a more fundamental problem. Shouldn't the InitCountryCode be
done in a more central location?

[michaelbai, 2012-11-19 23:05:38]

https://codereview.chromium.org/11348135/diff/1/chrome/browser/search_engines...
File chrome/browser/search_engines/template_url_service_test_util.cc (right):

https://codereview.chromium.org/11348135/diff/1/chrome/browser/search_engines...
chrome/browser/search_engines/template_url_service_test_util.cc:122:
TemplateURLPrepopulateData::InitCountryCode("US");
Did you mean TestingProfile?

On 2012/11/19 22:50:01, sky wrote:
> This seems like a more fundamental problem. Shouldn't the InitCountryCode be
> done in a more central location?

[Peter Kasting, 2012-11-20 04:06:13]

https://codereview.chromium.org/11348135/diff/1/chrome/browser/search_engines...
File chrome/browser/search_engines/template_url_service_test_util.cc (right):

https://codereview.chromium.org/11348135/diff/1/chrome/browser/search_engines...
chrome/browser/search_engines/template_url_service_test_util.cc:122:
TemplateURLPrepopulateData::InitCountryCode("US");
On 2012/11/19 22:50:01, sky wrote:
> This seems like a more fundamental problem. Shouldn't the InitCountryCode be
> done in a more central location?

Yes, but really this is just wrong anyway.  No tests should depend on this.

It looks to me like the Android code in template_url_prepopulate_data.cc should
simply remove the entire concept of kCountryIDNotSet, init
g_country_code_at_install to kCountryIDUnknown, and not DCHECK in
GetCurrentCountryID.  That would mean the tests would run with the default
engine set, which is what we want.

[michaelbai, 2012-11-20 19:03:33]

PTAL

https://codereview.chromium.org/11348135/diff/1/chrome/browser/search_engines...
File chrome/browser/search_engines/template_url_service_test_util.cc (right):

https://codereview.chromium.org/11348135/diff/1/chrome/browser/search_engines...
chrome/browser/search_engines/template_url_service_test_util.cc:122:
TemplateURLPrepopulateData::InitCountryCode("US");
It looks like the original author want to be warned if the InitCountryCode()
wasn't called. Actually, it works in unit tests :), I'd like to keep this and
make the unit tests passed with kCountryIDUnknown at the same time, sounds good?

On 2012/11/20 04:06:13, Peter Kasting wrote:
> On 2012/11/19 22:50:01, sky wrote:
> > This seems like a more fundamental problem. Shouldn't the InitCountryCode be
> > done in a more central location?
> 
> Yes, but really this is just wrong anyway.  No tests should depend on this.
> 
> It looks to me like the Android code in template_url_prepopulate_data.cc
should
> simply remove the entire concept of kCountryIDNotSet, init
> g_country_code_at_install to kCountryIDUnknown, and not DCHECK in
> GetCurrentCountryID.  That would mean the tests would run with the default
> engine set, which is what we want.

[Peter Kasting, 2012-11-20 21:36:35]

No, don't do this.  Now you have a DLOG(WARNING) that gets hit every time we run
the unit tests.  Not OK.

The old code didn't "warn" anything, it DCHECKed, which meant "this shall never
happen", but also had no actual consequences.

The right solution is to do what I said at first: just nuke this DCHECK and the
"not set" constant.

[michaelbai, 2012-11-20 21:59:32]

Removed kCountryIDNotSet, PTAL

[Peter Kasting, 2012-11-20 22:01:51]

LGTM; I suggest also moving InitCountryCode() into that same #ifdef block with
the other Android functions.

[michaelbai, 2012-11-20 22:08:38]

Moved InitCountryCode(), thanks

[commit-bot: I haz the power, 2012-11-20 22:09:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaelbai@chromium.org/11348135/2002

[commit-bot: I haz the power, 2012-11-21 05:51:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaelbai@chromium.org/11348135/5003

[commit-bot: I haz the power, 2012-11-21 07:35:43]

Change committed as 169001