[Android] Simplify logic for grabbing initial country-code.

[Android] Simplify logic for grabbing initial country-code.

The old way involved start-up code reading a value and setting a global
in template_url_prepopulate_data. Since the value can be computed
on-demand from device-information, just fetch it as needed instead.

BUG=169106
TBR=thakis

Committed: https://crrev.com/38b46138112e7077a41ab8dd4815a22a15ff0527
Cr-Commit-Position: refs/heads/master@{#300317}

[Yaron, 2014-10-06 18:21:29]

yfriedman@chromium.org changed reviewers:
+ jshin@chromium.org, newt@chromium.org, pkasting@chromium.org

[Yaron, 2014-10-06 18:21:30]

jshin:
  ui/base/l10n/l10n_util_android.cc
  ui/base/l10n/l10n_util_android.h
pkasting:
  chrome/browser/search_engines/search_provider_install_data_unittest.cc
  components/omnibox/autocomplete_result_unittest.cc
  components/search_engines/DEPS
  components/search_engines/template_url_prepopulate_data.cc
  components/search_engines/template_url_prepopulate_data.h
newt:
  ui/android/java/src/org/chromium/ui/base/LocalizationUtils.java

[Yaron, 2014-10-06 18:22:55]

oops. looks like i need to rebase.

[newt (away), 2014-10-06 18:34:03]

https://codereview.chromium.org/628263004/diff/40001/ui/android/java/src/org/...
File ui/android/java/src/org/chromium/ui/base/LocalizationUtils.java (right):

https://codereview.chromium.org/628263004/diff/40001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/base/LocalizationUtils.java:65:
context.getResources().getConfiguration().locale.getCountry();
Is context.getResources().getConfiguration().locale the same as
Locale.getDefault()? (I'd hope so, since we use the latter everywhere else.) If
so, let's just use Locale.getDefault() here.

[newt (away), 2014-10-06 18:36:48]

https://codereview.chromium.org/628263004/diff/40001/components/search_engine...
File components/search_engines/template_url_prepopulate_data.cc (right):

https://codereview.chromium.org/628263004/diff/40001/components/search_engine...
components/search_engines/template_url_prepopulate_data.cc:629: const
std::string& country_code = l10n_util::GetDefaultCountryCode();
Is it worth caching this value?

[Peter Kasting, 2014-10-06 20:28:27]

LGTM

https://codereview.chromium.org/628263004/diff/40001/chrome/browser/search_en...
File chrome/browser/search_engines/search_provider_install_data_unittest.cc
(right):

https://codereview.chromium.org/628263004/diff/40001/chrome/browser/search_en...
chrome/browser/search_engines/search_provider_install_data_unittest.cc:20:
#include "components/search_engines/template_url_prepopulate_data.h"
This #include can probably be removed.

https://codereview.chromium.org/628263004/diff/40001/components/omnibox/autoc...
File components/omnibox/autocomplete_result_unittest.cc (right):

https://codereview.chromium.org/628263004/diff/40001/components/omnibox/autoc...
components/omnibox/autocomplete_result_unittest.cc:21: #include
"components/search_engines/template_url_prepopulate_data.h"
This #include can probably be removed.

https://codereview.chromium.org/628263004/diff/40001/components/search_engine...
File components/search_engines/DEPS (right):

https://codereview.chromium.org/628263004/diff/40001/components/search_engine...
components/search_engines/DEPS:18: "+ui/base",
You might want to check with the folks that did the componentization of
search_engines/ as to whether this is problematic.  Also consider whether you
can scope it so it's only allowed for Android.

https://codereview.chromium.org/628263004/diff/40001/components/search_engine...
File components/search_engines/template_url_prepopulate_data.cc (right):

https://codereview.chromium.org/628263004/diff/40001/components/search_engine...
components/search_engines/template_url_prepopulate_data.cc:629: const
std::string& country_code = l10n_util::GetDefaultCountryCode();
On 2014/10/06 18:36:48, newt wrote:
> Is it worth caching this value?

No.  This is called at most once per run, I believe.

https://codereview.chromium.org/628263004/diff/40001/components/search_engine...
components/search_engines/template_url_prepopulate_data.cc:631: DLOG(ERROR) <<
"Invalid country code: " << country_code;
Unless you have a concrete plan to retrieve and act on these logs, don't log
this.

In fact, if this is a DCHECK-level failure -- i.e. it should never actually fail
-- don't handle it either.  Simply assume it succeeds, with a DCHECK to that
effect if necessary.

[Yaron, 2014-10-08 00:08:53]

yfriedman@chromium.org changed reviewers:
+ sky@chromium.org

[Yaron, 2014-10-08 00:08:54]

+sky to approve the DEP on ui/base

https://codereview.chromium.org/628263004/diff/40001/chrome/browser/search_en...
File chrome/browser/search_engines/search_provider_install_data_unittest.cc
(right):

https://codereview.chromium.org/628263004/diff/40001/chrome/browser/search_en...
chrome/browser/search_engines/search_provider_install_data_unittest.cc:20:
#include "components/search_engines/template_url_prepopulate_data.h"
On 2014/10/06 20:28:27, Peter Kasting wrote:
> This #include can probably be removed.

Done.

https://codereview.chromium.org/628263004/diff/40001/components/omnibox/autoc...
File components/omnibox/autocomplete_result_unittest.cc (right):

https://codereview.chromium.org/628263004/diff/40001/components/omnibox/autoc...
components/omnibox/autocomplete_result_unittest.cc:21: #include
"components/search_engines/template_url_prepopulate_data.h"
On 2014/10/06 20:28:27, Peter Kasting wrote:
> This #include can probably be removed.

Done.

https://codereview.chromium.org/628263004/diff/40001/components/search_engine...
File components/search_engines/template_url_prepopulate_data.cc (right):

https://codereview.chromium.org/628263004/diff/40001/components/search_engine...
components/search_engines/template_url_prepopulate_data.cc:629: const
std::string& country_code = l10n_util::GetDefaultCountryCode();
On 2014/10/06 20:28:27, Peter Kasting wrote:
> On 2014/10/06 18:36:48, newt wrote:
> > Is it worth caching this value?
> 
> No.  This is called at most once per run, I believe.

Agreed. Nobody else is caching these calls either and it'ss a short-hop to Java
and reading an in-memory variable.

https://codereview.chromium.org/628263004/diff/40001/components/search_engine...
components/search_engines/template_url_prepopulate_data.cc:631: DLOG(ERROR) <<
"Invalid country code: " << country_code;
On 2014/10/06 20:28:27, Peter Kasting wrote:
> Unless you have a concrete plan to retrieve and act on these logs, don't log
> this.
> 
> In fact, if this is a DCHECK-level failure -- i.e. it should never actually
fail
> -- don't handle it either.  Simply assume it succeeds, with a DCHECK to that
> effect if necessary.

it's not new (moved from below) but removed

https://codereview.chromium.org/628263004/diff/40001/ui/android/java/src/org/...
File ui/android/java/src/org/chromium/ui/base/LocalizationUtils.java (right):

https://codereview.chromium.org/628263004/diff/40001/ui/android/java/src/org/...
ui/android/java/src/org/chromium/ui/base/LocalizationUtils.java:65:
context.getResources().getConfiguration().locale.getCountry();
On 2014/10/06 18:34:03, newt wrote:
> Is context.getResources().getConfiguration().locale the same as
> Locale.getDefault()? (I'd hope so, since we use the latter everywhere else.)
If
> so, let's just use Locale.getDefault() here.

Done.

[Peter Kasting, 2014-10-08 00:23:08]

https://codereview.chromium.org/628263004/diff/40001/components/search_engine...
File components/search_engines/template_url_prepopulate_data.cc (right):

https://codereview.chromium.org/628263004/diff/40001/components/search_engine...
components/search_engines/template_url_prepopulate_data.cc:631: DLOG(ERROR) <<
"Invalid country code: " << country_code;
On 2014/10/08 00:08:54, Yaron wrote:
> On 2014/10/06 20:28:27, Peter Kasting wrote:
> > Unless you have a concrete plan to retrieve and act on these logs, don't log
> > this.
> > 
> > In fact, if this is a DCHECK-level failure -- i.e. it should never actually
> fail
> > -- don't handle it either.  Simply assume it succeeds, with a DCHECK to that
> > effect if necessary.
> 
> it's not new (moved from below) but removed

So is it actually possible for this to return anything other than 2?

If so, you should avoid the else after return here (violates Chromium style);
one way to do this would be

  return (country_code.size() == 2) ?
      CountryCharsToCountryIDWithUpdate(country_code[0], country_code[1]) :
      kCountryIDUnknown;

[newt (away), 2014-10-08 00:31:07]

lgtm

[sky, 2014-10-08 03:08:03]

Don't you need to update gyp/gn too? Presumably they aren't linking against ui.

https://codereview.chromium.org/628263004/diff/80001/ui/base/l10n/l10n_util_a...
File ui/base/l10n/l10n_util_android.h (right):

https://codereview.chromium.org/628263004/diff/80001/ui/base/l10n/l10n_util_a...
ui/base/l10n/l10n_util_android.h:18: UI_BASE_EXPORT std::string
GetDefaultLocale();
I'm surprised the local and country code are ui related. Any idea why this code
is here?

[Yaron, 2014-10-09 18:09:19]

https://codereview.chromium.org/628263004/diff/80001/ui/base/l10n/l10n_util_a...
File ui/base/l10n/l10n_util_android.h (right):

https://codereview.chromium.org/628263004/diff/80001/ui/base/l10n/l10n_util_a...
ui/base/l10n/l10n_util_android.h:18: UI_BASE_EXPORT std::string
GetDefaultLocale();
On 2014/10/08 03:08:03, sky wrote:
> I'm surprised the local and country code are ui related. Any idea why this
code
> is here?

Nope. Is base/i18n a better place?

[sky, 2014-10-10 02:51:08]

That seems better to me.

On Thu, Oct 9, 2014 at 11:09 AM,  <yfriedman@chromium.org> wrote:
>
>
https://codereview.chromium.org/628263004/diff/80001/ui/base/l10n/l10n_util_a...
> File ui/base/l10n/l10n_util_android.h (right):
>
>
https://codereview.chromium.org/628263004/diff/80001/ui/base/l10n/l10n_util_a...
> ui/base/l10n/l10n_util_android.h:18: UI_BASE_EXPORT std::string
> GetDefaultLocale();
> On 2014/10/08 03:08:03, sky wrote:
>>
>> I'm surprised the local and country code are ui related. Any idea why
>
> this code
>>
>> is here?
>
>
> Nope. Is base/i18n a better place?
>
> https://codereview.chromium.org/628263004/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jungshik at Google, 2014-10-13 21:46:18]

https://codereview.chromium.org/628263004/diff/80001/ui/base/l10n/l10n_util_a...
File ui/base/l10n/l10n_util_android.h (right):

https://codereview.chromium.org/628263004/diff/80001/ui/base/l10n/l10n_util_a...
ui/base/l10n/l10n_util_android.h:18: UI_BASE_EXPORT std::string
GetDefaultLocale();
On 2014/10/09 18:09:19, Yaron wrote:
> On 2014/10/08 03:08:03, sky wrote:
> > I'm surprised the local and country code are ui related. Any idea why this
> code
> > is here?
> 
> Nope. Is base/i18n a better place?

I like that better, too. A lot of stuffs in l10n_util had better be in
base/i18n. 

JFYI:
You don't have to change (given that CountryCode is used elsewhere in the code
you're touching), RegionCode would be slightly 'safer'.

[Yaron, 2014-10-17 18:23:59]

yfriedman@chromium.org changed reviewers:
+ boliu@chromium.org

[Yaron, 2014-10-17 18:24:00]

boliu: aw

https://codereview.chromium.org/628263004/diff/40001/components/search_engine...
File components/search_engines/template_url_prepopulate_data.cc (right):

https://codereview.chromium.org/628263004/diff/40001/components/search_engine...
components/search_engines/template_url_prepopulate_data.cc:631: DLOG(ERROR) <<
"Invalid country code: " << country_code;
On 2014/10/08 00:23:08, Peter Kasting wrote:
> On 2014/10/08 00:08:54, Yaron wrote:
> > On 2014/10/06 20:28:27, Peter Kasting wrote:
> > > Unless you have a concrete plan to retrieve and act on these logs, don't
log
> > > this.
> > > 
> > > In fact, if this is a DCHECK-level failure -- i.e. it should never
actually
> > fail
> > > -- don't handle it either.  Simply assume it succeeds, with a DCHECK to
that
> > > effect if necessary.
> > 
> > it's not new (moved from below) but removed
> 
> So is it actually possible for this to return anything other than 2?
> 
> If so, you should avoid the else after return here (violates Chromium style);
> one way to do this would be
> 
>   return (country_code.size() == 2) ?
>       CountryCharsToCountryIDWithUpdate(country_code[0], country_code[1]) :
>       kCountryIDUnknown;

I think it's possible, especially in the context of custom roms.

https://codereview.chromium.org/628263004/diff/80001/ui/base/l10n/l10n_util_a...
File ui/base/l10n/l10n_util_android.h (right):

https://codereview.chromium.org/628263004/diff/80001/ui/base/l10n/l10n_util_a...
ui/base/l10n/l10n_util_android.h:18: UI_BASE_EXPORT std::string
GetDefaultLocale();
On 2014/10/13 21:46:17, Jungshik Shin wrote:
> On 2014/10/09 18:09:19, Yaron wrote:
> > On 2014/10/08 03:08:03, sky wrote:
> > > I'm surprised the local and country code are ui related. Any idea why this
> > code
> > > is here?
> > 
> > Nope. Is base/i18n a better place?
> 
I ended up just moving to regular base as theres' an existing LocaleUtils there.
I also moved GetDefaultLocale there since this was just redirecting there.

> I like that better, too. A lot of stuffs in l10n_util had better be in
> base/i18n. 
> 
> JFYI:
> You don't have to change (given that CountryCode is used elsewhere in the code
> you're touching), RegionCode would be slightly 'safer'.

[boliu, 2014-10-17 19:41:27]

android_webview lgtm

[sky, 2014-10-20 16:13:37]

ui LGTM

[Yaron, 2014-10-20 18:00:34]

yfriedman@chromium.org changed reviewers:
+ thakis@chromium.org

[Yaron, 2014-10-20 18:00:35]

tbr thakis for trivial base gyp/gn changes

[Yaron, 2014-10-20 18:00:44]

The CQ bit was checked by yfriedman@chromium.org

[commit-bot: I haz the power, 2014-10-20 18:01:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/628263004/150001

[commit-bot: I haz the power, 2014-10-20 19:40:28]

Committed patchset #7 (id:150001)

[commit-bot: I haz the power, 2014-10-20 19:41:05]

Patchset 7 (id:??) landed as
https://crrev.com/38b46138112e7077a41ab8dd4815a22a15ff0527
Cr-Commit-Position: refs/heads/master@{#300317}