[iOS] Expose TemplateURLPrepopulateData::GetCurrentCountryID()

[iOS] Expose TemplateURLPrepopulateData::GetCurrentCountryID()

Expose TemplateURLPrepopulateData::GetCurrentCountryID() to detect that the
user changed the device region in order to update the list of search engine
on iOS.

Note that when a region change is detected we remove all default search
engines except the one selected by the user and then insert all the new
engines. This is possible because user cannot customize search engines,
only select one of the default engines.

BUG=None

Committed: https://crrev.com/71c91039c5fb1a12f2f6920c1a63c0ae48a97c10
Cr-Commit-Position: refs/heads/master@{#330716}

[sdefresne, 2015-05-12 15:52:15]

sdefresne@chromium.org changed reviewers:
+ pkasting@chromium.org

[sdefresne, 2015-05-12 15:52:16]

Please take a look.

[Peter Kasting, 2015-05-12 18:49:14]

https://codereview.chromium.org/1130183004/diff/1/components/search_engines/t...
File components/search_engines/template_url_prepopulate_data.cc (right):

https://codereview.chromium.org/1130183004/diff/1/components/search_engines/t...
components/search_engines/template_url_prepopulate_data.cc:1283: return
GetCountryIDFromPrefs(nullptr);
This infinite loops, doesn't it?  The first thing GetCountryIDFromPrefs() does
is call GetCurrentCountryID() when the arg is null.

https://codereview.chromium.org/1130183004/diff/1/components/search_engines/t...
File components/search_engines/template_url_prepopulate_data.h (right):

https://codereview.chromium.org/1130183004/diff/1/components/search_engines/t...
components/search_engines/template_url_prepopulate_data.h:71: // of search
engines when user switch device region settings.
Nit: switches

https://codereview.chromium.org/1130183004/diff/1/components/search_engines/t...
components/search_engines/template_url_prepopulate_data.h:72: // TODO(ios):
Remove this method once user can customize search engine, see
Nit: engines

https://codereview.chromium.org/1130183004/diff/1/components/search_engines/t...
components/search_engines/template_url_prepopulate_data.h:73: //
http://crbug.com/144145
Nit: Trailing period

Also, the linked bug isn't obviously about adding search engine customization. 
I would either clarify on the bug what it covers or file a new bug.

[sdefresne, 2015-05-13 12:47:12]

Thank you for the review.

Please take another look.

https://codereview.chromium.org/1130183004/diff/1/components/search_engines/t...
File components/search_engines/template_url_prepopulate_data.cc (right):

https://codereview.chromium.org/1130183004/diff/1/components/search_engines/t...
components/search_engines/template_url_prepopulate_data.cc:1283: return
GetCountryIDFromPrefs(nullptr);
On 2015/05/12 18:49:13, Peter Kasting wrote:
> This infinite loops, doesn't it?  The first thing GetCountryIDFromPrefs() does
> is call GetCurrentCountryID() when the arg is null.

No because there are two methods called GetCurrentCountryID. This one in
TemplateURLPrepopulateData and one in TemplateURLPrepopulateData::{anonymous
namespace}.

Since the method in the anonymous namespace has different implementation on each
architecture it is not easy to move it out of the anonymous namespace. I've
renamed the exported method to make it easier to understand.

https://codereview.chromium.org/1130183004/diff/1/components/search_engines/t...
File components/search_engines/template_url_prepopulate_data.h (right):

https://codereview.chromium.org/1130183004/diff/1/components/search_engines/t...
components/search_engines/template_url_prepopulate_data.h:71: // of search
engines when user switch device region settings.
On 2015/05/12 18:49:13, Peter Kasting wrote:
> Nit: switches

Done.

https://codereview.chromium.org/1130183004/diff/1/components/search_engines/t...
components/search_engines/template_url_prepopulate_data.h:72: // TODO(ios):
Remove this method once user can customize search engine, see
On 2015/05/12 18:49:14, Peter Kasting wrote:
> Nit: engines

Done.

https://codereview.chromium.org/1130183004/diff/1/components/search_engines/t...
components/search_engines/template_url_prepopulate_data.h:73: //
http://crbug.com/144145
On 2015/05/12 18:49:14, Peter Kasting wrote:
> Nit: Trailing period
> 
> Also, the linked bug isn't obviously about adding search engine customization.

> I would either clarify on the bug what it covers or file a new bug.

Good catch. I've updated the bug reference.

[Peter Kasting, 2015-05-13 23:12:59]

https://codereview.chromium.org/1130183004/diff/20001/components/search_engin...
File components/search_engines/template_url_prepopulate_data.cc (right):

https://codereview.chromium.org/1130183004/diff/20001/components/search_engin...
components/search_engines/template_url_prepopulate_data.cc:1285: return
GetCurrentCountryID();
What implementation does this actually use?  OS_POSIX?  Make this clear in your
comment above instead of implying that iOS could use various ones on different
OSes (which would not make sense).

Why not just move the existing GetCurrentCountryID() implementations out of the
anonymous namespace instead of defining this alias?

https://codereview.chromium.org/1130183004/diff/20001/components/search_engin...
File components/search_engines/template_url_prepopulate_data.h (right):

https://codereview.chromium.org/1130183004/diff/20001/components/search_engin...
components/search_engines/template_url_prepopulate_data.h:74: int
GetDefaultCountryID();
This name seems misleading since users can't change from the "default" and the
"default" will change over time.

GetCurrentCountryID() was a better name; GetCurrentCountryIDIOS() might be a
unique name if you need such a thing (but see other comments).

[sdefresne, 2015-05-18 09:47:19]

PTAL

https://codereview.chromium.org/1130183004/diff/20001/components/search_engin...
File components/search_engines/template_url_prepopulate_data.cc (right):

https://codereview.chromium.org/1130183004/diff/20001/components/search_engin...
components/search_engines/template_url_prepopulate_data.cc:1285: return
GetCurrentCountryID();
On 2015/05/13 23:12:59, Peter Kasting wrote:
> What implementation does this actually use?  OS_POSIX?  Make this clear in
your
> comment above instead of implying that iOS could use various ones on different
> OSes (which would not make sense).
> 
> Why not just move the existing GetCurrentCountryID() implementations out of
the
> anonymous namespace instead of defining this alias?

Done.

https://codereview.chromium.org/1130183004/diff/20001/components/search_engin...
File components/search_engines/template_url_prepopulate_data.h (right):

https://codereview.chromium.org/1130183004/diff/20001/components/search_engin...
components/search_engines/template_url_prepopulate_data.h:74: int
GetDefaultCountryID();
On 2015/05/13 23:12:59, Peter Kasting wrote:
> This name seems misleading since users can't change from the "default" and the
> "default" will change over time.
> 
> GetCurrentCountryID() was a better name; GetCurrentCountryIDIOS() might be a
> unique name if you need such a thing (but see other comments).

I went back to GetCurrentCountryID().

[Peter Kasting, 2015-05-18 18:31:34]

LGTM

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

https://codereview.chromium.org/1130183004/diff/40001/components/search_engin...
components/search_engines/template_url_prepopulate_data.cc:1067: int
GeoIDToCountryID(GEOID geo_id) {
Nit: Leave this function (still #if'd) where it used to be within the anon
namespace; then move the non-namespaced GetCurrentCountryID() implementations
down to the place in this file that matches up with the declaration order in the
header.

https://codereview.chromium.org/1130183004/diff/40001/components/search_engin...
File components/search_engines/template_url_prepopulate_data.h (right):

https://codereview.chromium.org/1130183004/diff/40001/components/search_engin...
components/search_engines/template_url_prepopulate_data.h:72: // TODO(ios):
Remove this method once user can customize search engines, see
Nit: Perhaps:

TODO(ios): Once the user can customize search engines ( http://crbug.com/153047
) this declaration should be removed and the definition in the .cc file be moved
back to the anonymous namespace.

[sdefresne, 2015-05-19 09:04:22]

Thank you for the review.

I had to forward-declare the function on all OS because it is used in some of
the function in the anonymous namespace. I've done it in the .cc file but I do
not really like it.

I can see two other alternatives:
1. remove the #if defined(OS_IOS) in template_url_data.h and always
forward-declare the function,
2. close and re-open the anonymous namespace in the .cc file protected by #if
defined(OS_IOS)

Could you take another look and tell me if you want me to change to one of the
two other alternatives (or another one)?

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

https://codereview.chromium.org/1130183004/diff/40001/components/search_engin...
components/search_engines/template_url_prepopulate_data.cc:1067: int
GeoIDToCountryID(GEOID geo_id) {
On 2015/05/18 18:31:34, Peter Kasting wrote:
> Nit: Leave this function (still #if'd) where it used to be within the anon
> namespace; then move the non-namespaced GetCurrentCountryID() implementations
> down to the place in this file that matches up with the declaration order in
the
> header.

Done.

https://codereview.chromium.org/1130183004/diff/40001/components/search_engin...
File components/search_engines/template_url_prepopulate_data.h (right):

https://codereview.chromium.org/1130183004/diff/40001/components/search_engin...
components/search_engines/template_url_prepopulate_data.h:72: // TODO(ios):
Remove this method once user can customize search engines, see
On 2015/05/18 18:31:34, Peter Kasting wrote:
> Nit: Perhaps:
> 
> TODO(ios): Once the user can customize search engines (
http://crbug.com/153047
> ) this declaration should be removed and the definition in the .cc file be
moved
> back to the anonymous namespace.

Acknowledged.

[Peter Kasting, 2015-05-19 19:50:26]

I would make the declaration in the header un-#if'd but let the comment there
say that this is only used by iOS (and all the rest of the stuff your comment
currently says).

[sdefresne, 2015-05-20 08:20:43]

The CQ bit was checked by sdefresne@chromium.org

[sdefresne, 2015-05-20 08:20:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1130183004/#ps80001
(title: "Address comments")

[commit-bot: I haz the power, 2015-05-20 08:20:57]

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

[sdefresne, 2015-05-20 08:31:47]

On 2015/05/19 19:50:26, Peter Kasting wrote:
> I would make the declaration in the header un-#if'd but let the comment there
> say that this is only used by iOS (and all the rest of the stuff your comment
> currently says).

I've done it this way. Thank you for the reviews.

[commit-bot: I haz the power, 2015-05-20 09:14:49]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-05-20 09:15:36]

Patchset 5 (id:??) landed as
https://crrev.com/71c91039c5fb1a12f2f6920c1a63c0ae48a97c10
Cr-Commit-Position: refs/heads/master@{#330716}