Enable Chrome to tweak search engines for some locales

Enable Chrome to tweak search engines for some locales

In some locales we might want to show some extra search engine options.
This CL creates a JNI bridge, SpecialLocaleHandler, that will manage the
changes we do to the TURL service, when the device is in special locale.

BUG=638062
Committed: https://crrev.com/a27b1dfa7d0a8c7cb876d0bcd847555b9a1e87eb
Cr-Commit-Position: refs/heads/master@{#420770}

[Ian Wen, 2016-09-16 21:51:02]

ianwen@chromium.org changed reviewers:
+ mariakhomenko@chromium.org, pkasting@chromium.org

[Ian Wen, 2016-09-16 21:51:02]

mariakhomenko@chromium.org: Please review changes in browser.

pkasting@chromium.org: Please review changes in components/search_engine and
browser/search_engine.

Thank you both! :)

[Maria, 2016-09-16 23:44:02]

I think this change is missing tests.

Mostly nits -- and I did not really review search_engines change since I am not
too familiar with the code.

https://codereview.chromium.org/2347973002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/locale/LocaleManager.java
(right):

https://codereview.chromium.org/2347973002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/locale/LocaleManager.java:51:
* Overrides the default search engine to some engines we designate. This is a
no-op if the user
nit: Change to "Overrides the default search engine to a different search engine
we designate"

https://codereview.chromium.org/2347973002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/locale/SpecialLocaleHandler.java
(right):

https://codereview.chromium.org/2347973002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/locale/SpecialLocaleHandler.java:9:
* native object. Make sure to call Destroy() after this object is not used
anymore.
nit: Switch first sentence to "A Handler for changes in a given special locale".

https://codereview.chromium.org/2347973002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/locale/SpecialLocaleHandler.java:13:
private String mLocaleId;
final

https://codereview.chromium.org/2347973002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/locale/SpecialLocaleHandler.java:18:
* @param localeId Country id of the locale. Should be 2-character long.
nit: character -> characters

Are all locales really always two chars? There aren't any 3 char ones?

https://codereview.chromium.org/2347973002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/locale/SpecialLocaleHandler.java:36:
* Loads the template urls for this locale, and add it to template url service.
If the devices
nit: add -> adds

https://codereview.chromium.org/2347973002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/locale/SpecialLocaleHandler.java:37:
* was initialized in the given special locale, no-op here.
nit: was -> were

https://codereview.chromium.org/2347973002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/locale/SpecialLocaleHandler.java:42:
// If the locale is the same as the one set at install time, there will be
enough
Not sure I understand the comment

https://codereview.chromium.org/2347973002/diff/1/chrome/browser/android/loca...
File chrome/browser/android/locale/special_locale_handler.cc (right):

https://codereview.chromium.org/2347973002/diff/1/chrome/browser/android/loca...
chrome/browser/android/locale/special_locale_handler.cc:44:
SpecialLocaleHandler::SpecialLocaleHandler(std::string locale)
This should probably be passed by reference instead of copying

https://codereview.chromium.org/2347973002/diff/1/chrome/browser/android/loca...
chrome/browser/android/locale/special_locale_handler.cc:49:
SpecialLocaleHandler::~SpecialLocaleHandler() {}
Use the same order for methods here as you do in your .h file

https://codereview.chromium.org/2347973002/diff/1/chrome/browser/android/loca...
chrome/browser/android/locale/special_locale_handler.cc:107:
TemplateURLPrepopulateData::sogou.id);
Shouldn't this be abstracted away somewhere in a per-locale map?

https://codereview.chromium.org/2347973002/diff/1/chrome/browser/android/loca...
File chrome/browser/android/locale/special_locale_handler.h (right):

https://codereview.chromium.org/2347973002/diff/1/chrome/browser/android/loca...
chrome/browser/android/locale/special_locale_handler.h:32: std::string locale_;
add include for string

https://codereview.chromium.org/2347973002/diff/1/chrome/browser/android/loca...
chrome/browser/android/locale/special_locale_handler.h:35: std::vector<int>
prepopulate_ids_;
add include for vector

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

https://codereview.chromium.org/2347973002/diff/1/components/search_engines/t...
components/search_engines/template_url_prepopulate_data.h:46: // already loaded,
need_to-load will be set false.
nit: change to ", |need_to_load| will be set to false"

[Ian Wen, 2016-09-19 17:37:55]

PTAL the 2nd patchset.

I totally that there should be tests for special_locale* code. Maybe I should
mock the template url service and write some unittests? Not sure where to put
the test and I'm still learning about tests in C++. How about we first land this
one, and I'll work on a parallel CL to add tests?

https://chromiumcodereview.appspot.com/2347973002/diff/1/chrome/android/java/...
File
chrome/android/java/src/org/chromium/chrome/browser/locale/LocaleManager.java
(right):

https://chromiumcodereview.appspot.com/2347973002/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/locale/LocaleManager.java:51:
* Overrides the default search engine to some engines we designate. This is a
no-op if the user
On 2016/09/16 23:44:01, Maria wrote:
> nit: Change to "Overrides the default search engine to a different search
engine
> we designate"

Done.

https://chromiumcodereview.appspot.com/2347973002/diff/1/chrome/android/java/...
File
chrome/android/java/src/org/chromium/chrome/browser/locale/SpecialLocaleHandler.java
(right):

https://chromiumcodereview.appspot.com/2347973002/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/locale/SpecialLocaleHandler.java:9:
* native object. Make sure to call Destroy() after this object is not used
anymore.
On 2016/09/16 23:44:01, Maria wrote:
> nit: Switch first sentence to "A Handler for changes in a given special
locale".

Done.

https://chromiumcodereview.appspot.com/2347973002/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/locale/SpecialLocaleHandler.java:13:
private String mLocaleId;
On 2016/09/16 23:44:01, Maria wrote:
> final

Done.

https://chromiumcodereview.appspot.com/2347973002/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/locale/SpecialLocaleHandler.java:18:
* @param localeId Country id of the locale. Should be 2-character long.
On 2016/09/16 23:44:01, Maria wrote:
> nit: character -> characters
> 
> Are all locales really always two chars? There aren't any 3 char ones?

No it can't be. In [1] we compute the ID as integer of the two characters.

https://cs.chromium.org/chromium/src/components/search_engines/template_url_p...

https://chromiumcodereview.appspot.com/2347973002/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/locale/SpecialLocaleHandler.java:36:
* Loads the template urls for this locale, and add it to template url service.
If the devices
On 2016/09/16 23:44:01, Maria wrote:
> nit: add -> adds

Done.

https://chromiumcodereview.appspot.com/2347973002/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/locale/SpecialLocaleHandler.java:37:
* was initialized in the given special locale, no-op here.
On 2016/09/16 23:44:01, Maria wrote:
> nit: was -> were

Done.

https://chromiumcodereview.appspot.com/2347973002/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/locale/SpecialLocaleHandler.java:42:
// If the locale is the same as the one set at install time, there will be
enough
On 2016/09/16 23:44:01, Maria wrote:
> Not sure I understand the comment

Oops. Fixed it.

https://chromiumcodereview.appspot.com/2347973002/diff/1/chrome/browser/andro...
File chrome/browser/android/locale/special_locale_handler.cc (right):

https://chromiumcodereview.appspot.com/2347973002/diff/1/chrome/browser/andro...
chrome/browser/android/locale/special_locale_handler.cc:44:
SpecialLocaleHandler::SpecialLocaleHandler(std::string locale)
On 2016/09/16 23:44:01, Maria wrote:
> This should probably be passed by reference instead of copying

Done.

https://chromiumcodereview.appspot.com/2347973002/diff/1/chrome/browser/andro...
chrome/browser/android/locale/special_locale_handler.cc:49:
SpecialLocaleHandler::~SpecialLocaleHandler() {}
On 2016/09/16 23:44:01, Maria wrote:
> Use the same order for methods here as you do in your .h file

Done.

https://chromiumcodereview.appspot.com/2347973002/diff/1/chrome/browser/andro...
chrome/browser/android/locale/special_locale_handler.cc:107:
TemplateURLPrepopulateData::sogou.id);
On 2016/09/16 23:44:01, Maria wrote:
> Shouldn't this be abstracted away somewhere in a per-locale map?

I wanted to do this, but I found it very challenging as there is no native code
anymore in downstream.

Moved it to above to make it more clear.

https://chromiumcodereview.appspot.com/2347973002/diff/1/chrome/browser/andro...
File chrome/browser/android/locale/special_locale_handler.h (right):

https://chromiumcodereview.appspot.com/2347973002/diff/1/chrome/browser/andro...
chrome/browser/android/locale/special_locale_handler.h:32: std::string locale_;
On 2016/09/16 23:44:01, Maria wrote:
> add include for string

Done.

https://chromiumcodereview.appspot.com/2347973002/diff/1/chrome/browser/andro...
chrome/browser/android/locale/special_locale_handler.h:35: std::vector<int>
prepopulate_ids_;
On 2016/09/16 23:44:01, Maria wrote:
> add include for vector

Done.

https://chromiumcodereview.appspot.com/2347973002/diff/1/components/search_en...
File components/search_engines/template_url_prepopulate_data.h (right):

https://chromiumcodereview.appspot.com/2347973002/diff/1/components/search_en...
components/search_engines/template_url_prepopulate_data.h:46: // already loaded,
need_to-load will be set false.
On 2016/09/16 23:44:01, Maria wrote:
> nit: change to ", |need_to_load| will be set to false" 

Done.

[Peter Kasting, 2016-09-19 21:06:24]

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

https://codereview.chromium.org/2347973002/diff/20001/components/search_engin...
components/search_engines/template_url.h:502: // Installed only on this device.
Should not be sync'd.
Nit: synced

https://codereview.chromium.org/2347973002/diff/20001/components/search_engin...
components/search_engines/template_url.h:765: Type type_;
If you're going to elevate the "type" concept to not necessarily be connected
with an extension anymore, then it probably needs to be removed from
AssociatedExtensionInfo so it is set in one fundamental place on every
TemplateURL.  (That would also allow converting GetType() into an inline type()
accessor.)

Then TemplateURL's constructor could take a type argument (that can be defaulted
to NORMAL), instead of adding the setter you've added above, since you just want
to set your type at construction.

The problem is how this interacts with places that use set_extension_info()
today.  I'm a little uneasy about that function, since TemplateURLs are meant to
be mostly immutable, so I'm wondering whether this should similarly only be
happening at construction time, or whether there's some way existing callers use
this where they actually need to toggle the type of a TemplateURL after
creation.  I don't know how the extension code uses this enough to answer this;
I'm hoping you've looked at more of this than I have and can figure it out?

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

https://codereview.chromium.org/2347973002/diff/20001/components/search_engin...
components/search_engines/template_url_prepopulate_data.cc:617: }
I added this in https://codereview.chromium.org/2349213004/ ; you may want to
mark your patch as dependent on that (I added it in a slightly different place
FWIW)

https://codereview.chromium.org/2347973002/diff/20001/components/search_engin...
components/search_engines/template_url_prepopulate_data.cc:976: void
GetPrepopulationSetFromPrefs(PrefService* prefs,
Nit: You only call this helper once, and it's simple, so I would just inline
this into the one place below and not add the helper.

https://codereview.chromium.org/2347973002/diff/20001/components/search_engin...
components/search_engines/template_url_prepopulate_data.cc:1120:
std::vector<std::unique_ptr<TemplateURLData>> GetLocalPrepopulatedEngines(
Nit: Function definition order should match .h declaration order.

https://codereview.chromium.org/2347973002/diff/20001/components/search_engin...
components/search_engines/template_url_prepopulate_data.cc:1127:
DCHECK(country_id != kCountryIDUnknown);
This DCHECK makes me nervous.  Are you sure the caller has sanitized the
incoming value?  I wonder if we should just modify the conditional below to
treat this case like the "same as current country" case.

https://codereview.chromium.org/2347973002/diff/20001/components/search_engin...
components/search_engines/template_url_prepopulate_data.cc:1137:
GetPrepopulationSetFromCountryID(country_id, &engines, &num_engines);
Nit: I suggest modifying GetPrepopulationSetFromCountryID() so that instead of
taking these two outparams, it constructs the vector of TemplateURLs internally.
 (This just involves changing the "return;" to a "break;" in the macro
definition for END_UNHANDLED_COUNTRIES and then adding some code on either side
of the switch.)

This will simplify the two functions here.

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

https://codereview.chromium.org/2347973002/diff/20001/components/search_engin...
components/search_engines/template_url_prepopulate_data.h:46: // already loaded,
need_to-load will be set false.
Nit: How about this (I rewrote both functions' comments for parallelism, focus
on the second one):

// Returns the prepopulated URLs for the current country.
// |default_search_provider_index| is set to the index of the default search
// provider within the returned vector.
std::vector<std::unique_ptr<TemplateURLData>> GetPrepopulatedEngines(
    PrefService* prefs,
    size_t* default_search_provider_index);

// Returns the prepopulated URLs associated with |locale|, if it differs from
// the current country.  |locale| should be a two-character uppercase ISO 3166-1
// country code.
std::vector<std::unique_ptr<TemplateURLData>>
GetPrepopulatedEnginesForNonDefaultLocale(const std::string& locale,
                                          PrefService* prefs);

This fixes these issues:
* Document the expected format of |locale|, since a "locale" can be many things.
* Document that this checks the current country and avoids doing anything if the
locale matches
* Avoid outparam entirely, since it's confusing, and callers can just check if
the returned vector is empty.

https://codereview.chromium.org/2347973002/diff/20001/components/search_engin...
components/search_engines/template_url_prepopulate_data.h:47:
std::vector<std::unique_ptr<TemplateURLData>> GetLocalPrepopulatedEngines(
Should this function be #if'd for Android-only?  It seems like that's the only
place it's called.

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

https://codereview.chromium.org/2347973002/diff/20001/components/search_engin...
components/search_engines/template_url_service.cc:619: DCHECK(!url ||
(!IsCreatedByExtension(url)));
Nit: Extra parens

[Ian Wen, 2016-09-20 04:48:04]

All addressed. PTAL. :)

https://chromiumcodereview.appspot.com/2347973002/diff/20001/components/searc...
File components/search_engines/template_url.h (right):

https://chromiumcodereview.appspot.com/2347973002/diff/20001/components/searc...
components/search_engines/template_url.h:502: // Installed only on this device.
Should not be sync'd.
On 2016/09/19 21:06:23, Peter Kasting wrote:
> Nit: synced

Done.

https://chromiumcodereview.appspot.com/2347973002/diff/20001/components/searc...
components/search_engines/template_url.h:765: Type type_;
On 2016/09/19 21:06:23, Peter Kasting wrote:
> If you're going to elevate the "type" concept to not necessarily be connected
> with an extension anymore, then it probably needs to be removed from
> AssociatedExtensionInfo so it is set in one fundamental place on every
> TemplateURL.  (That would also allow converting GetType() into an inline
type()
> accessor.)
> 
> Then TemplateURL's constructor could take a type argument (that can be
defaulted
> to NORMAL), instead of adding the setter you've added above, since you just
want
> to set your type at construction.
> 
> The problem is how this interacts with places that use set_extension_info()
> today.  I'm a little uneasy about that function, since TemplateURLs are meant
to
> be mostly immutable, so I'm wondering whether this should similarly only be
> happening at construction time, or whether there's some way existing callers
use
> this where they actually need to toggle the type of a TemplateURL after
> creation.  I don't know how the extension code uses this enough to answer
this;
> I'm hoping you've looked at more of this than I have and can figure it out?

In one case about omnibox extension I do need to change the turl's type. Other
than this TURL is pretty much immutable.

I followed your guide and moved type to TURL's constructor, and also makes
type() and set_type() inline methods.

Done.

https://chromiumcodereview.appspot.com/2347973002/diff/20001/components/searc...
File components/search_engines/template_url_prepopulate_data.cc (right):

https://chromiumcodereview.appspot.com/2347973002/diff/20001/components/searc...
components/search_engines/template_url_prepopulate_data.cc:617: }
On 2016/09/19 21:06:23, Peter Kasting wrote:
> I added this in https://codereview.chromium.org/2349213004/ ; you may want to
> mark your patch as dependent on that (I added it in a slightly different place
> FWIW)

Done.

https://chromiumcodereview.appspot.com/2347973002/diff/20001/components/searc...
components/search_engines/template_url_prepopulate_data.cc:976: void
GetPrepopulationSetFromPrefs(PrefService* prefs,
On 2016/09/19 21:06:23, Peter Kasting wrote:
> Nit: You only call this helper once, and it's simple, so I would just inline
> this into the one place below and not add the helper.

Done.

https://chromiumcodereview.appspot.com/2347973002/diff/20001/components/searc...
components/search_engines/template_url_prepopulate_data.cc:1120:
std::vector<std::unique_ptr<TemplateURLData>> GetLocalPrepopulatedEngines(
On 2016/09/19 21:06:23, Peter Kasting wrote:
> Nit: Function definition order should match .h declaration order.

Done.

https://chromiumcodereview.appspot.com/2347973002/diff/20001/components/searc...
components/search_engines/template_url_prepopulate_data.cc:1127:
DCHECK(country_id != kCountryIDUnknown);
On 2016/09/19 21:06:23, Peter Kasting wrote:
> This DCHECK makes me nervous.  Are you sure the caller has sanitized the
> incoming value?  I wonder if we should just modify the conditional below to
> treat this case like the "same as current country" case.

Done.

https://chromiumcodereview.appspot.com/2347973002/diff/20001/components/searc...
File components/search_engines/template_url_prepopulate_data.h (right):

https://chromiumcodereview.appspot.com/2347973002/diff/20001/components/searc...
components/search_engines/template_url_prepopulate_data.h:46: // already loaded,
need_to-load will be set false.
On 2016/09/19 21:06:23, Peter Kasting wrote:
> Nit: How about this (I rewrote both functions' comments for parallelism, focus
> on the second one):
> 
> // Returns the prepopulated URLs for the current country.
> // |default_search_provider_index| is set to the index of the default search
> // provider within the returned vector.
> std::vector<std::unique_ptr<TemplateURLData>> GetPrepopulatedEngines(
>     PrefService* prefs,
>     size_t* default_search_provider_index);
> 
> // Returns the prepopulated URLs associated with |locale|, if it differs from
> // the current country.  |locale| should be a two-character uppercase ISO
3166-1
> // country code.
> std::vector<std::unique_ptr<TemplateURLData>>
> GetPrepopulatedEnginesForNonDefaultLocale(const std::string& locale,
>                                           PrefService* prefs);
> 
> This fixes these issues:
> * Document the expected format of |locale|, since a "locale" can be many
things.
> * Document that this checks the current country and avoids doing anything if
the
> locale matches
> * Avoid outparam entirely, since it's confusing, and callers can just check if
> the returned vector is empty.

Done.

https://chromiumcodereview.appspot.com/2347973002/diff/20001/components/searc...
components/search_engines/template_url_prepopulate_data.h:47:
std::vector<std::unique_ptr<TemplateURLData>> GetLocalPrepopulatedEngines(
On 2016/09/19 21:06:23, Peter Kasting wrote:
> Should this function be #if'd for Android-only?  It seems like that's the only
> place it's called.

Done.

https://chromiumcodereview.appspot.com/2347973002/diff/20001/components/searc...
File components/search_engines/template_url_service.cc (right):

https://chromiumcodereview.appspot.com/2347973002/diff/20001/components/searc...
components/search_engines/template_url_service.cc:619: DCHECK(!url ||
(!IsCreatedByExtension(url)));
On 2016/09/19 21:06:24, Peter Kasting wrote:
> Nit: Extra parens

Done.

[Ian Wen, 2016-09-20 05:23:06]

The CQ bit was checked by ianwen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 05:23:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 05:35:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-20 05:35:48]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ian Wen, 2016-09-20 05:42:31]

The CQ bit was checked by ianwen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 05:42:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 07:20:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-20 07:20:56]

Dry run: This issue passed the CQ dry run.

[Peter Kasting, 2016-09-21 21:45:08]

https://chromiumcodereview.appspot.com/2347973002/diff/60001/chrome/browser/e...
File chrome/browser/extensions/api/settings_overrides/settings_overrides_api.cc
(right):

https://chromiumcodereview.appspot.com/2347973002/diff/60001/chrome/browser/e...
chrome/browser/extensions/api/settings_overrides/settings_overrides_api.cc:272:
new TemplateURL::AssociatedExtensionInfo(extension->id()));
Nit: While here, use MakeUnique():

  auto info =
      base::MakeUnique<TemplateURL::AssociatedExtensionInfo>(extension->id());

(I like using "auto" in this sort of case to avoid duplicating the type name;
you're welcome to do it for |turl| too.)

https://chromiumcodereview.appspot.com/2347973002/diff/60001/chrome/browser/e...
chrome/browser/extensions/api/settings_overrides/settings_overrides_api.cc:281: 
Nit: I'd put this blank line below the next statement rather than above it, so
the blocks are basically "build each arg for the function call, then finally
call the function".

https://chromiumcodereview.appspot.com/2347973002/diff/60001/chrome/browser/s...
File chrome/browser/search_engines/chrome_template_url_service_client.cc
(right):

https://chromiumcodereview.appspot.com/2347973002/diff/60001/chrome/browser/s...
chrome/browser/search_engines/chrome_template_url_service_client.cc:74:
template_url->set_type(TemplateURL::OMNIBOX_API_EXTENSION);
This was the thing that worried me, looking through the existing calls.  I feel
like there should be a way to avoid doing this.

Looking at how this function is called, it seems like maybe we can just nuke all
this stuff because it's associated with some stuff vasilii TODOd to remove?  I
sent him an email and CCed you on it.  In the meantime, let's not block on that,
but if he's able to clean this up, then hopefully with the other comments I
suggest in this review we can nuke set_type().

https://chromiumcodereview.appspot.com/2347973002/diff/60001/components/searc...
File components/search_engines/template_url.cc (right):

https://chromiumcodereview.appspot.com/2347973002/diff/60001/components/searc...
components/search_engines/template_url.cc:1492: DCHECK(type_ !=
OMNIBOX_API_EXTENSION);
Nit: DCHECK_NE(OMNIBOX_API_EXTENSION, type_)?

https://chromiumcodereview.appspot.com/2347973002/diff/60001/components/searc...
File components/search_engines/template_url.h (right):

https://chromiumcodereview.appspot.com/2347973002/diff/60001/components/searc...
components/search_engines/template_url.h:508: AssociatedExtensionInfo(const
std::string& extension_id);
Nit: explicit

https://chromiumcodereview.appspot.com/2347973002/diff/60001/components/searc...
components/search_engines/template_url.h:521: TemplateURL(const TemplateURLData&
data, Type type = NORMAL);
Nit: I think this still needs "explicit"?  Or does the compiler not allow an
implicit conversion of TemplateURLData to TemplateURL with this declaration?

https://chromiumcodereview.appspot.com/2347973002/diff/60001/components/searc...
components/search_engines/template_url.h:761: Type type_;
Nit: I'd probably place this above |engine_type_|.

https://chromiumcodereview.appspot.com/2347973002/diff/60001/components/searc...
File components/search_engines/template_url_prepopulate_data.cc (right):

https://chromiumcodereview.appspot.com/2347973002/diff/60001/components/searc...
components/search_engines/template_url_prepopulate_data.cc:620:
std::vector<std::unique_ptr<TemplateURLData>>& t_urls) {
Nit: Don't take an outparam by non-const ref.  If we wanted an outparam we'd
want to use a pointer (see
http://google.github.io/styleguide/cppguide.html#Reference_Arguments ).

But in this case it makes more sense to me to simply construct and return a new
vector.

https://chromiumcodereview.appspot.com/2347973002/diff/60001/components/searc...
components/search_engines/template_url_prepopulate_data.cc:973: for (size_t i =
0; i < num_engines; ++i) {
Nit: Blank line above this.

No {}.

https://chromiumcodereview.appspot.com/2347973002/diff/60001/components/searc...
File components/search_engines/template_url_prepopulate_data.h (right):

https://chromiumcodereview.appspot.com/2347973002/diff/60001/components/searc...
components/search_engines/template_url_prepopulate_data.h:49:
std::vector<std::unique_ptr<TemplateURLData>> GetLocalPrepopulatedEngines(
Nit: The only thing I worry about with this function name is that I don't know
whether it's sufficiently obvious that, if the locale is the same as the current
one, this returns an empty vector.  The function name kinda sounds like it just
gets the engines for a particular locale.

However, the comment does say this, and my suggested name was pretty awkward. 
So maybe this is good enough.  I don't know.

https://chromiumcodereview.appspot.com/2347973002/diff/60001/components/searc...
File components/search_engines/template_url_service.cc (right):

https://chromiumcodereview.appspot.com/2347973002/diff/60001/components/searc...
components/search_engines/template_url_service.cc:486:
template_url->set_type(type);
I don't think we should do this.  Callers should set the right type on
|template_url| when creating it.

The call site in this file is easy to do this with, and
settings_overrides_api.cc does it already.  That just leaves the unittest code,
which can be fixed by changing the helpers that we use to create the passed-in
TemplateURLs so they have the right type set.

This leaves usage of set_type() to just the RestoreExtensionInfoIfNecessary()
case, which hopefully can be done away with separately.

[Ian Wen, 2016-09-21 23:09:48]

The CQ bit was checked by ianwen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-21 23:10:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ian Wen, 2016-09-21 23:12:38]

Thanks for the comments. All addressed. PTAL :)

https://codereview.chromium.org/2347973002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/settings_overrides/settings_overrides_api.cc
(right):

https://codereview.chromium.org/2347973002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/settings_overrides/settings_overrides_api.cc:272:
new TemplateURL::AssociatedExtensionInfo(extension->id()));
On 2016/09/21 21:45:08, Peter Kasting wrote:
> Nit: While here, use MakeUnique():
> 
>   auto info =
>       base::MakeUnique<TemplateURL::AssociatedExtensionInfo>(extension->id());
> 
> (I like using "auto" in this sort of case to avoid duplicating the type name;
> you're welcome to do it for |turl| too.)

Done.

https://codereview.chromium.org/2347973002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/settings_overrides/settings_overrides_api.cc:281: 
On 2016/09/21 21:45:08, Peter Kasting wrote:
> Nit: I'd put this blank line below the next statement rather than above it, so
> the blocks are basically "build each arg for the function call, then finally
> call the function".

Done.

https://codereview.chromium.org/2347973002/diff/60001/chrome/browser/search_e...
File chrome/browser/search_engines/chrome_template_url_service_client.cc
(right):

https://codereview.chromium.org/2347973002/diff/60001/chrome/browser/search_e...
chrome/browser/search_engines/chrome_template_url_service_client.cc:74:
template_url->set_type(TemplateURL::OMNIBOX_API_EXTENSION);
On 2016/09/21 21:45:08, Peter Kasting wrote:
> This was the thing that worried me, looking through the existing calls.  I
feel
> like there should be a way to avoid doing this.
> 
> Looking at how this function is called, it seems like maybe we can just nuke
all
> this stuff because it's associated with some stuff vasilii TODOd to remove?  I
> sent him an email and CCed you on it.  In the meantime, let's not block on
that,
> but if he's able to clean this up, then hopefully with the other comments I
> suggest in this review we can nuke set_type().

Added a todo track this. Agreed that we shouldn't block on it. :)

https://codereview.chromium.org/2347973002/diff/60001/components/search_engin...
File components/search_engines/template_url.cc (right):

https://codereview.chromium.org/2347973002/diff/60001/components/search_engin...
components/search_engines/template_url.cc:1492: DCHECK(type_ !=
OMNIBOX_API_EXTENSION);
On 2016/09/21 21:45:08, Peter Kasting wrote:
> Nit: DCHECK_NE(OMNIBOX_API_EXTENSION, type_)?

Done.

https://codereview.chromium.org/2347973002/diff/60001/components/search_engin...
File components/search_engines/template_url.h (right):

https://codereview.chromium.org/2347973002/diff/60001/components/search_engin...
components/search_engines/template_url.h:508: AssociatedExtensionInfo(const
std::string& extension_id);
On 2016/09/21 21:45:08, Peter Kasting wrote:
> Nit: explicit

Done.

https://codereview.chromium.org/2347973002/diff/60001/components/search_engin...
components/search_engines/template_url.h:521: TemplateURL(const TemplateURLData&
data, Type type = NORMAL);
On 2016/09/21 21:45:08, Peter Kasting wrote:
> Nit: I think this still needs "explicit"?  Or does the compiler not allow an
> implicit conversion of TemplateURLData to TemplateURL with this declaration?

Compiler allows it. Yet I agree explicit should be here.

https://codereview.chromium.org/2347973002/diff/60001/components/search_engin...
components/search_engines/template_url.h:761: Type type_;
On 2016/09/21 21:45:08, Peter Kasting wrote:
> Nit: I'd probably place this above |engine_type_|.

Done.

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

https://codereview.chromium.org/2347973002/diff/60001/components/search_engin...
components/search_engines/template_url_prepopulate_data.cc:620:
std::vector<std::unique_ptr<TemplateURLData>>& t_urls) {
On 2016/09/21 21:45:08, Peter Kasting wrote:
> Nit: Don't take an outparam by non-const ref.  If we wanted an outparam we'd
> want to use a pointer (see
> http://google.github.io/styleguide/cppguide.html#Reference_Arguments ).
> 
> But in this case it makes more sense to me to simply construct and return a
new
> vector.

Done.

https://codereview.chromium.org/2347973002/diff/60001/components/search_engin...
components/search_engines/template_url_prepopulate_data.cc:973: for (size_t i =
0; i < num_engines; ++i) {
On 2016/09/21 21:45:08, Peter Kasting wrote:
> Nit: Blank line above this.
> 
> No {}.

Done.

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

https://codereview.chromium.org/2347973002/diff/60001/components/search_engin...
components/search_engines/template_url_prepopulate_data.h:49:
std::vector<std::unique_ptr<TemplateURLData>> GetLocalPrepopulatedEngines(
On 2016/09/21 21:45:08, Peter Kasting wrote:
> Nit: The only thing I worry about with this function name is that I don't know
> whether it's sufficiently obvious that, if the locale is the same as the
current
> one, this returns an empty vector.  The function name kinda sounds like it
just
> gets the engines for a particular locale.
> 
> However, the comment does say this, and my suggested name was pretty awkward. 
> So maybe this is good enough.  I don't know.

I added a clarification to make it more clear that vector should be empty if
locale is already loaded.

https://codereview.chromium.org/2347973002/diff/60001/components/search_engin...
File components/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/2347973002/diff/60001/components/search_engin...
components/search_engines/template_url_service.cc:486:
template_url->set_type(type);
On 2016/09/21 21:45:08, Peter Kasting wrote:
> I don't think we should do this.  Callers should set the right type on
> |template_url| when creating it.
> 
> The call site in this file is easy to do this with, and
> settings_overrides_api.cc does it already.  That just leaves the unittest
code,
> which can be fixed by changing the helpers that we use to create the passed-in
> TemplateURLs so they have the right type set.
> 
> This leaves usage of set_type() to just the RestoreExtensionInfoIfNecessary()
> case, which hopefully can be done away with separately.

Done.

[commit-bot: I haz the power, 2016-09-21 23:25:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-21 23:25:50]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ian Wen, 2016-09-22 01:27:22]

The CQ bit was checked by ianwen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-22 01:27:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-22 02:51:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-22 02:51:43]

Dry run: This issue passed the CQ dry run.

[Maria, 2016-09-22 17:17:55]

lgtm

https://codereview.chromium.org/2347973002/diff/80001/chrome/browser/android/...
File chrome/browser/android/locale/special_locale_handler.h (right):

https://codereview.chromium.org/2347973002/diff/80001/chrome/browser/android/...
chrome/browser/android/locale/special_locale_handler.h:16: using std::vector;
nit: I feel like it's really uncommon to have using statements in header files,
not sure whether there's something in the style guide about this

[Peter Kasting, 2016-09-23 01:28:36]

LGTM

https://codereview.chromium.org/2347973002/diff/60001/components/search_engin...
File components/search_engines/template_url.h (right):

https://codereview.chromium.org/2347973002/diff/60001/components/search_engin...
components/search_engines/template_url.h:521: TemplateURL(const TemplateURLData&
data, Type type = NORMAL);
On 2016/09/21 23:12:38, Ian Wen wrote:
> On 2016/09/21 21:45:08, Peter Kasting wrote:
> > Nit: I think this still needs "explicit"?  Or does the compiler not allow an
> > implicit conversion of TemplateURLData to TemplateURL with this declaration?
> 
> Compiler allows it. Yet I agree explicit should be here.

What I meant was, if the compiler allows it, we definitely need it, because we
don't want to allow it :)  Anyway, I think we're good now.

https://codereview.chromium.org/2347973002/diff/80001/chrome/browser/android/...
File chrome/browser/android/locale/special_locale_handler.h (right):

https://codereview.chromium.org/2347973002/diff/80001/chrome/browser/android/...
chrome/browser/android/locale/special_locale_handler.h:16: using std::vector;
On 2016/09/22 17:17:55, Maria wrote:
> nit: I feel like it's really uncommon to have using statements in header
files,
> not sure whether there's something in the style guide about this

I don't know that there's anything explicit, but yes, this is a bad move, don't
do it.

In this case, there's no reason for the using directives anyway, because they
cost lines of code rather than making things less noisy.  And "using std::XYZ"
is frowned-on in Chromium anyway, since the std:: qualifier is short and makes
it clear you're using something from the STL instead of e.g. a weird custom
string class (cough, google3, cough).

https://codereview.chromium.org/2347973002/diff/100001/components/search_engi...
File components/search_engines/template_url_prepopulate_data.cc (right):

https://codereview.chromium.org/2347973002/diff/100001/components/search_engi...
components/search_engines/template_url_prepopulate_data.cc:1121:
*default_search_provider_index = 0;
Nit: Not for this CL, but wanna write a separate cleanup CL that just removes
this arg entirely?  Always setting it to zero is silly, and I think a lot of the
callers don't even care about it, so let's just kill it and make callers who
care assume the default provider is the first.

I'd do this myself but I don't have time right this moment :/

https://codereview.chromium.org/2347973002/diff/100001/components/search_engi...
components/search_engines/template_url_prepopulate_data.cc:1134:
std::vector<std::unique_ptr<TemplateURLData>> t_urls;
Nit: This is only used to return the empty object below.  Just "return
std::vector<std::unique_ptr<TemplateURLData>>();" below and eliminate this temp.

https://codereview.chromium.org/2347973002/diff/100001/components/search_engi...
File components/search_engines/template_url_prepopulate_data.h (right):

https://codereview.chromium.org/2347973002/diff/100001/components/search_engi...
components/search_engines/template_url_prepopulate_data.h:48: // country code.
If the given locale is the same as the existing locale, return
Nit: returns

[Ian Wen, 2016-09-23 07:56:29]

ianwen@chromium.org changed reviewers:
+ asargent@chromium.org, dbeam@chromium.org

[Ian Wen, 2016-09-23 07:56:30]

dbeam@chromium.org: Please review changes in web ui.

asargent@ Please review changes in extensions

[asargent_no_longer_on_chrome, 2016-09-23 16:38:33]

chrome/browser/extensions/* lgtm

[Dan Beam, 2016-09-23 21:51:04]

lgtm

[Ian Wen, 2016-09-23 22:01:57]

https://codereview.chromium.org/2347973002/diff/80001/chrome/browser/android/...
File chrome/browser/android/locale/special_locale_handler.h (right):

https://codereview.chromium.org/2347973002/diff/80001/chrome/browser/android/...
chrome/browser/android/locale/special_locale_handler.h:16: using std::vector;
On 2016/09/23 01:28:36, Peter Kasting wrote:
> On 2016/09/22 17:17:55, Maria wrote:
> > nit: I feel like it's really uncommon to have using statements in header
> files,
> > not sure whether there's something in the style guide about this
> 
> I don't know that there's anything explicit, but yes, this is a bad move,
don't
> do it.
> 
> In this case, there's no reason for the using directives anyway, because they
> cost lines of code rather than making things less noisy.  And "using std::XYZ"
> is frowned-on in Chromium anyway, since the std:: qualifier is short and makes
> it clear you're using something from the STL instead of e.g. a weird custom
> string class (cough, google3, cough).

Done.

https://codereview.chromium.org/2347973002/diff/100001/components/search_engi...
File components/search_engines/template_url_prepopulate_data.cc (right):

https://codereview.chromium.org/2347973002/diff/100001/components/search_engi...
components/search_engines/template_url_prepopulate_data.cc:1121:
*default_search_provider_index = 0;
On 2016/09/23 01:28:36, Peter Kasting wrote:
> Nit: Not for this CL, but wanna write a separate cleanup CL that just removes
> this arg entirely?  Always setting it to zero is silly, and I think a lot of
the
> callers don't even care about it, so let's just kill it and make callers who
> care assume the default provider is the first.
> 
> I'd do this myself but I don't have time right this moment :/

Sure. Let me do it next week.

https://codereview.chromium.org/2347973002/diff/100001/components/search_engi...
components/search_engines/template_url_prepopulate_data.cc:1134:
std::vector<std::unique_ptr<TemplateURLData>> t_urls;
On 2016/09/23 01:28:36, Peter Kasting wrote:
> Nit: This is only used to return the empty object below.  Just "return
> std::vector<std::unique_ptr<TemplateURLData>>();" below and eliminate this
temp.

Done.

https://codereview.chromium.org/2347973002/diff/100001/components/search_engi...
File components/search_engines/template_url_prepopulate_data.h (right):

https://codereview.chromium.org/2347973002/diff/100001/components/search_engi...
components/search_engines/template_url_prepopulate_data.h:48: // country code.
If the given locale is the same as the existing locale, return
On 2016/09/23 01:28:36, Peter Kasting wrote:
> Nit: returns

Done.

[Ian Wen, 2016-09-23 22:02:08]

The CQ bit was checked by ianwen@chromium.org

[Ian Wen, 2016-09-23 22:02:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, asargent@chromium.org, dbeam@chromium.org,
mariakhomenko@chromium.org
Link to the patchset: https://codereview.chromium.org/2347973002/#ps120001
(title: "final nits")

[commit-bot: I haz the power, 2016-09-23 22:02:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-23 23:08:47]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-09-23 23:11:26]

Description was changed from

==========
Enable Chrome to tweak search engines for some locales

In some locales we might want to show some extra search engine options.
This CL creates a JNI bridge, SpecialLocaleHandler, that will manage the
changes we do to the TURL service, when the device is in special locale.

BUG=638062
==========

to

==========
Enable Chrome to tweak search engines for some locales

In some locales we might want to show some extra search engine options.
This CL creates a JNI bridge, SpecialLocaleHandler, that will manage the
changes we do to the TURL service, when the device is in special locale.

BUG=638062

Committed: https://crrev.com/a27b1dfa7d0a8c7cb876d0bcd847555b9a1e87eb
Cr-Commit-Position: refs/heads/master@{#420770}
==========

[commit-bot: I haz the power, 2016-09-23 23:11:27]

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

[Alexander Yashkin, 2016-10-21 06:34:40]

a-v-y@yandex-team.ru changed reviewers:
+ a-v-y@yandex-team.ru

[Alexander Yashkin, 2016-10-21 06:34:41]

https://codereview.chromium.org/2347973002/diff/120001/components/search_engi...
File components/search_engines/template_url_prepopulate_data.cc (right):

https://codereview.chromium.org/2347973002/diff/120001/components/search_engi...
components/search_engines/template_url_prepopulate_data.cc:1140: return
GetPrepopulationSetFromCountryID(country_id);
Is it ok, that GetLocalPrepopulatedEngines returns list of prepopulated engines
for locale and do not take in consideration 
engines that were overriden from preference file like its done in
GetPrepopulatedEngines?

[Peter Kasting, 2016-10-21 06:44:42]

https://codereview.chromium.org/2347973002/diff/120001/components/search_engi...
File components/search_engines/template_url_prepopulate_data.cc (right):

https://codereview.chromium.org/2347973002/diff/120001/components/search_engi...
components/search_engines/template_url_prepopulate_data.cc:1140: return
GetPrepopulationSetFromCountryID(country_id);
On 2016/10/21 06:34:41, Alexander Yashkin wrote:
> Is it ok, that GetLocalPrepopulatedEngines returns list of prepopulated
engines
> for locale and do not take in consideration 
> engines that were overriden from preference file like its done in
> GetPrepopulatedEngines?

What do you mean "is it OK"?  This is a utility function, so the question is
what the overall system is doing, not just what this function is doing.  What
does the caller need to take into account?  What preferences are in question
here?  Or to step back: what problem are you seeing that needs solving?

[Alexander Yashkin, 2016-10-21 08:32:21]

On 2016/10/21 at 06:44:42, pkasting wrote:
>
https://codereview.chromium.org/2347973002/diff/120001/components/search_engi...
> File components/search_engines/template_url_prepopulate_data.cc (right):
> 
>
https://codereview.chromium.org/2347973002/diff/120001/components/search_engi...
> components/search_engines/template_url_prepopulate_data.cc:1140: return
GetPrepopulationSetFromCountryID(country_id);
> On 2016/10/21 06:34:41, Alexander Yashkin wrote:
> > Is it ok, that GetLocalPrepopulatedEngines returns list of prepopulated
engines
> > for locale and do not take in consideration 
> > engines that were overriden from preference file like its done in
> > GetPrepopulatedEngines?
> 
> What do you mean "is it OK"?  This is a utility function, so the question is
what the overall system is doing, not just what this function is doing.  What
does the caller need to take into account?  What preferences are in question
here?  Or to step back: what problem are you seeing that needs solving?

I think I finally understand logic behind this code. 
I have no access to https://bugs.chromium.org/p/chromium/issues/detail?id=638062
so have to guess from code:

I was worried that two functions - GetPrepopulatedEngines and
GetLocalPrepopulatedEngines can return different sets of engines for same
country. 
GetPrepopulatedEngines uses content prefs::kSearchProviderOverrides and country
id, and GetLocalPrepopulatedEngines uses only country id.

I believe prefs::kSearchProviderOverrides is used to add some ability to
override search engines set and default search by branded Chrome distributives.
If I understand this commit correctly it adds ability to dynamically add/remove
search engines if locale is changed during browser work.

My worry was that default search engine already overriden by
prefs::kSearchProviderOverrides could again be overriden by locale change.
Yet, i see that SpecialLocaleHandler::OverrideDefaultSearchProvider already
checks and does nothing for non-google dsp.

So, it seems there is nothing to worry about.