Allow the default search providers to be specified by the preferences files,...

Allow the default search providers to be specified by the preferences files,
overriding the built-in set.
- Per locale providers not allowed.

BUG=47440
TEST=unittest included, for manual testing see bug.



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

[cpu, 2010-07-12 17:04:30]

I wanted to preserve the const-ness of the existing structure so I had to do a
couple of unorthodox tricks.

I had to move the PrepoulatedEngine out of the anon namespace because gcc
complains. I could supress the warning as an alternative.

[Peter Kasting, 2010-07-12 18:01:35]

http://codereview.chromium.org/2974001/diff/6001/7001
File chrome/browser/search_engines/template_url_prepopulate_data.cc (left):

http://codereview.chromium.org/2974001/diff/6001/7001#oldcode27
chrome/browser/search_engines/template_url_prepopulate_data.cc:27: namespace {
Do not remove this.

http://codereview.chromium.org/2974001/diff/6001/7001#oldcode66
chrome/browser/search_engines/template_url_prepopulate_data.cc:66: const int id;
Do not un-const this.

http://codereview.chromium.org/2974001/diff/6001/7001
File chrome/browser/search_engines/template_url_prepopulate_data.cc (right):

http://codereview.chromium.org/2974001/diff/6001/7001#newcode65
chrome/browser/search_engines/template_url_prepopulate_data.cc:65: // IDs 33, 34
and 36 are reserved for distribution custom engines.
Don't do this.  Instead reserve all IDs above 1000 for distributions.  This also
removes the three-provider limit, which I don't think is reasonable.

http://codereview.chromium.org/2974001/diff/6001/7001#newcode2257
chrome/browser/search_engines/template_url_prepopulate_data.cc:2257: const
PrepopulatedEngine* engines_custom[] =
Do not add this.

http://codereview.chromium.org/2974001/diff/6001/7001#newcode2811
chrome/browser/search_engines/template_url_prepopulate_data.cc:2811: // This
class is a work arround to the PODness of |PrepopulatedEngine|
This implementation isn't acceptable.

Instead, change GetPrepopulationSetFromCountryID() to return a
std::vector<PrepopulatedEngine> -- it will grab the array of const pointers and
copy the contents into a vector.

Then you can write GetPrepopulationSetFromPreferences() without this memory
manager class and without things like "Foo***".

http://codereview.chromium.org/2974001/diff/6001/7001#newcode2926
chrome/browser/search_engines/template_url_prepopulate_data.cc:2926: &engines,
&num_engines)) {
Nit: No {}

http://codereview.chromium.org/2974001/diff/6001/7003
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/2974001/diff/6001/7003#newcode152
chrome/common/pref_names.cc:152: L"preferred_search_providers";
Nit: I would prefer to call this "search_provider_overrides" (and name the
constant identically).

[cpu, 2010-07-12 18:46:03]

http://codereview.chromium.org/2974001/diff/6001/7001
File chrome/browser/search_engines/template_url_prepopulate_data.cc (right):

http://codereview.chromium.org/2974001/diff/6001/7001#newcode2811
chrome/browser/search_engines/template_url_prepopulate_data.cc:2811: // This
class is a work arround to the PODness of |PrepopulatedEngine|
Ok.

On 2010/07/12 18:01:36, Peter Kasting wrote:
> This implementation isn't acceptable.
> 
> Instead, change GetPrepopulationSetFromCountryID() to return a
> std::vector<PrepopulatedEngine> -- it will grab the array of const pointers
and
> copy the contents into a vector.
> 
> Then you can write GetPrepopulationSetFromPreferences() without this memory
> manager class and without things like "Foo***".

[cpu, 2010-07-13 00:27:53]

changes made, please look again.

[Glenn Wilson, 2010-07-13 00:51:19]

http://codereview.chromium.org/2974001/diff/6/23001
File chrome/browser/search_engines/template_url_prepopulate_data.cc (right):

http://codereview.chromium.org/2974001/diff/6/23001#newcode2861
chrome/browser/search_engines/template_url_prepopulate_data.cc:2861:
eng->Get(L"suggest_url", &val) && val->GetAsString(&suggest_url) &&
What would this do if suggest_url, favicon_url, or encoding were not specified
(NULL)?  It doesn't look like these are initialized in the TemplateURL
constructor.

http://codereview.chromium.org/2974001/diff/6/23002
File chrome/browser/search_engines/template_url_prepopulate_data_unittest.cc
(right):

http://codereview.chromium.org/2974001/diff/6/23002#newcode95
chrome/browser/search_engines/template_url_prepopulate_data_unittest.cc:95: "]
}";
Would you mind adding a test where the favicon URL, suggest URL, or encoding are
omitted entirely from the entry?

[Peter Kasting, 2010-07-13 00:56:42]

http://codereview.chromium.org/2974001/diff/6/23001
File chrome/browser/search_engines/template_url_prepopulate_data.cc (right):

http://codereview.chromium.org/2974001/diff/6/23001#newcode2802
chrome/browser/search_engines/template_url_prepopulate_data.cc:2802: int
GetDataVersion() {
We should read the data version from the prefs if there's a set of override
data.  That version number and this one don't need to correspond in any way.

http://codereview.chromium.org/2974001/diff/6/23001#newcode2841
chrome/browser/search_engines/template_url_prepopulate_data.cc:2841:
TemplateURL* GetPrepopulatedTemplatefromPrefs(PrefService* prefs, int index) {
Nit: It'll be a bit simpler (and more efficient, not that that matters) if you
combine the above function and this one into a single function that takes
|prefs| and |t_urls|, and wraps the TemplateURL construction block in a loop. 
Then the caller can unconditionally call this, and return immediately if that
causes |t_urls| to become non-empty.

http://codereview.chromium.org/2974001/diff/6/23001#newcode2858
chrome/browser/search_engines/template_url_prepopulate_data.cc:2858: if
(eng->Get(L"name", &val) && val->GetAsString(&name) &&
Nit: Convert to "!Get()" and "||" so you don't have an empty conditional arm (it
can be misleading if you're reading fast).

http://codereview.chromium.org/2974001/diff/6/23001#newcode2861
chrome/browser/search_engines/template_url_prepopulate_data.cc:2861:
eng->Get(L"suggest_url", &val) && val->GetAsString(&suggest_url) &&
On 2010/07/13 00:51:19, Glenn Wilson wrote:
> What would this do if suggest_url, favicon_url, or encoding were not specified
> (NULL)?  It doesn't look like these are initialized in the TemplateURL
> constructor.

We should require that the favicon URL and encoding be non-empty.  The suggest
URL should be allowed to be empty.

http://codereview.chromium.org/2974001/diff/6/23003
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/2974001/diff/6/23003#newcode152
chrome/common/pref_names.cc:152: L"preferred_search_providers";
Carried-over nit from patch set 2: I would prefer to call this
"search_provider_overrides" (and name the
constant identically).

[cpu, 2010-07-13 01:45:10]

code updated please look again.

http://codereview.chromium.org/2974001/diff/6/23001
File chrome/browser/search_engines/template_url_prepopulate_data.cc (right):

http://codereview.chromium.org/2974001/diff/6/23001#newcode2841
chrome/browser/search_engines/template_url_prepopulate_data.cc:2841:
TemplateURL* GetPrepopulatedTemplatefromPrefs(PrefService* prefs, int index) {
Ok, but that breaks the symmetry of GetPrepopulatedEngines, but whatever...

On 2010/07/13 00:56:43, Peter Kasting wrote:
> Nit: It'll be a bit simpler (and more efficient, not that that matters) if you
> combine the above function and this one into a single function that takes
> |prefs| and |t_urls|, and wraps the TemplateURL construction block in a loop. 
> Then the caller can unconditionally call this, and return immediately if that
> causes |t_urls| to become non-empty.

http://codereview.chromium.org/2974001/diff/6/23001#newcode2858
chrome/browser/search_engines/template_url_prepopulate_data.cc:2858: if
(eng->Get(L"name", &val) && val->GetAsString(&name) &&
I rather not. I am amenable to if(!(test)).

On 2010/07/13 00:56:43, Peter Kasting wrote:
> Nit: Convert to "!Get()" and "||" so you don't have an empty conditional arm
(it
> can be misleading if you're reading fast).

http://codereview.chromium.org/2974001/diff/6/23001#newcode2861
chrome/browser/search_engines/template_url_prepopulate_data.cc:2861:
eng->Get(L"suggest_url", &val) && val->GetAsString(&suggest_url) &&
Ok, I'll add that check.

On 2010/07/13 00:56:43, Peter Kasting wrote:
> On 2010/07/13 00:51:19, Glenn Wilson wrote:
> > What would this do if suggest_url, favicon_url, or encoding were not
specified
> > (NULL)?  It doesn't look like these are initialized in the TemplateURL
> > constructor.
> 
> We should require that the favicon URL and encoding be non-empty.  The suggest
> URL should be allowed to be empty.

[Peter Kasting, 2010-07-13 18:02:01]

http://codereview.chromium.org/2974001/diff/35001/1005
File chrome/browser/search_engines/template_url_prepopulate_data.cc (right):

http://codereview.chromium.org/2974001/diff/35001/1005#newcode2792
chrome/browser/search_engines/template_url_prepopulate_data.cc:2792: int
g_prepopulated_engine_data_version = 28;
Nit: I think this use of a global is more confusing than simply making
GetDataVersion() be:

int GetDataVersion(PrefService* prefs) {
  // Increment this if you change the above data in ways that mean users with
  // existing data should get a new version.
  const int kCurrentDataVersion = 28;

  if (!prefs->HasPrefPath(prefs::kPreferredSearchProvidersPrepopulatedVersion))
    return kCurrentDataVersion;
  return prefs->GetInteger(prefs::kPreferredSearchProvidersPrepopulatedVersion);
}

template_url_model.cc is the only caller of this function, and it can just call
GetDataVersion(GetPrefs()) since it has such a function.

http://codereview.chromium.org/2974001/diff/35001/1005#newcode2846
chrome/browser/search_engines/template_url_prepopulate_data.cc:2846: void
GetPrepopulatedTemplatefromPrefs(PrefService* prefs,
Nit: This arg should be on the next line

http://codereview.chromium.org/2974001/diff/35001/1005#newcode2862
chrome/browser/search_engines/template_url_prepopulate_data.cc:2862: for (size_t
ix = 0; ix != num_engines; ++ix) {
Nit: What does 'ix' mean? 'eng isn't fabulous either.  How about 'i' and
'engine'?

http://codereview.chromium.org/2974001/diff/35001/1005#newcode2873
chrome/browser/search_engines/template_url_prepopulate_data.cc:2873: // these
next fields are not allowed to be empty.
Nit: these -> These

http://codereview.chromium.org/2974001/diff/35001/1005#newcode2877
chrome/browser/search_engines/template_url_prepopulate_data.cc:2877: // Got a
parsing error. No big deal.
I think we should "continue" rather than "return" here.

http://codereview.chromium.org/2974001/diff/35001/1005#newcode2904
chrome/browser/search_engines/template_url_prepopulate_data.cc:2904:
TemplateURL* turl =
Nit: Could just be:

    t_urls->push_back(MakePrepopulatedTemplateURL(
        engines[i]->name,
        engines[i]->keyword,
        ...));

http://codereview.chromium.org/2974001/diff/35001/1007
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/2974001/diff/35001/1007#newcode152
chrome/common/pref_names.cc:152: L"preferred_search_providers";
Is there some reason you haven't even replied to my comments about the naming
here, let alone implemented them?

[cpu, 2010-07-13 20:36:16]

I think I got all the comments this time. please take a look again.

http://codereview.chromium.org/2974001/diff/35001/1005
File chrome/browser/search_engines/template_url_prepopulate_data.cc (right):

http://codereview.chromium.org/2974001/diff/35001/1005#newcode2792
chrome/browser/search_engines/template_url_prepopulate_data.cc:2792: int
g_prepopulated_engine_data_version = 28;
Ok, I'll change the interface then.

On 2010/07/13 18:02:01, Peter Kasting wrote:
> Nit: I think this use of a global is more confusing than simply making
> GetDataVersion() be:
> 
> int GetDataVersion(PrefService* prefs) {
>   // Increment this if you change the above data in ways that mean users with
>   // existing data should get a new version.
>   const int kCurrentDataVersion = 28;
> 
>   if
(!prefs->HasPrefPath(prefs::kPreferredSearchProvidersPrepopulatedVersion))
>     return kCurrentDataVersion;
>   return
prefs->GetInteger(prefs::kPreferredSearchProvidersPrepopulatedVersion);
> }
> 
> template_url_model.cc is the only caller of this function, and it can just
call
> GetDataVersion(GetPrefs()) since it has such a function.

http://codereview.chromium.org/2974001/diff/35001/1005#newcode2862
chrome/browser/search_engines/template_url_prepopulate_data.cc:2862: for (size_t
ix = 0; ix != num_engines; ++ix) {
On 2010/07/13 18:02:01, Peter Kasting wrote:
> Nit: What does 'ix' mean? 'eng isn't fabulous either.  How about 'i' and
> 'engine'?

I'll change eng for engine.
ix -> index, what does i stand for?

http://codereview.chromium.org/2974001/diff/35001/1005#newcode2877
chrome/browser/search_engines/template_url_prepopulate_data.cc:2877: // Got a
parsing error. No big deal.
On 2010/07/13 18:02:01, Peter Kasting wrote:
> I think we should "continue" rather than "return" here.

fine.

http://codereview.chromium.org/2974001/diff/35001/1005#newcode2904
chrome/browser/search_engines/template_url_prepopulate_data.cc:2904:
TemplateURL* turl =


On 2010/07/13 18:02:01, Peter Kasting wrote:
> Nit: Could just be:

I see no problem with the current form

> 
>     t_urls->push_back(MakePrepopulatedTemplateURL(
>         engines[i]->name,
>         engines[i]->keyword,
>         ...));

http://codereview.chromium.org/2974001/diff/35001/1007
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/2974001/diff/35001/1007#newcode152
chrome/common/pref_names.cc:152: L"preferred_search_providers";
Oversight. I saw them the first time and I did not like them, then I forgot. But
in the spirit of moving along I'll change them.

The're also shorter, which is nice.

On 2010/07/13 18:02:01, Peter Kasting wrote:
> Is there some reason you haven't even replied to my comments about the naming
> here, let alone implemented them?

[Peter Kasting, 2010-07-13 20:40:39]

LGTM

[Glenn Wilson, 2010-07-13 20:53:03]

LGTM

[cpu, 2010-07-13 21:08:09]

Thanks for your patience.

On 2010/07/13 20:40:39, Peter Kasting wrote:
> LGTM