Make some of the search engine configuration data from master_preferences optional.

Make some of the search engine configuration data from master_preferences optional.

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

[Joao da Silva, 2012-10-16 14:46:13]

Hi Peter, please review this one.

I'm not sure you agree with the fix :-) I think this is correct because adding
new _mandatory_ settings break every existing deployment. We should instead
check for the mandatory settings and load the others optionally. WDYT?

[Peter Kasting, 2012-10-16 17:48:32]

LGTM

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

https://codereview.chromium.org/11170007/diff/1/chrome/browser/search_engines...
chrome/browser/search_engines/template_url_prepopulate_data.cc:3563: string16
name;
Nit: I suggest moving all these declarations inside the loop to scope them as
narrowly as possible.  We don't care about the tiny efficiency loss.

https://codereview.chromium.org/11170007/diff/1/chrome/browser/search_engines...
chrome/browser/search_engines/template_url_prepopulate_data.cc:3566: std::string
favicon;
Nit: Leave this named "favicon_url"

https://codereview.chromium.org/11170007/diff/1/chrome/browser/search_engines...
chrome/browser/search_engines/template_url_prepopulate_data.cc:3569: const
ListValue kEmptyList;
Nit: This one I'd declare just above |alternate_urls| as

  ListValue empty_list;
  const ListValue* alternate_urls = &empty_list;

[Joao da Silva, 2012-10-16 18:53:36]

Thanks for the review!

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

https://codereview.chromium.org/11170007/diff/1/chrome/browser/search_engines...
chrome/browser/search_engines/template_url_prepopulate_data.cc:3563: string16
name;
On 2012/10/16 17:48:32, Peter Kasting wrote:
> Nit: I suggest moving all these declarations inside the loop to scope them as
> narrowly as possible.  We don't care about the tiny efficiency loss.

Done.

https://codereview.chromium.org/11170007/diff/1/chrome/browser/search_engines...
chrome/browser/search_engines/template_url_prepopulate_data.cc:3566: std::string
favicon;
On 2012/10/16 17:48:32, Peter Kasting wrote:
> Nit: Leave this named "favicon_url"

But but but then the line is 81 chars! (Done!)

https://codereview.chromium.org/11170007/diff/1/chrome/browser/search_engines...
chrome/browser/search_engines/template_url_prepopulate_data.cc:3569: const
ListValue kEmptyList;
On 2012/10/16 17:48:32, Peter Kasting wrote:
> Nit: This one I'd declare just above |alternate_urls| as
> 
>   ListValue empty_list;
>   const ListValue* alternate_urls = &empty_list;

Done.

[commit-bot: I haz the power, 2012-10-16 18:54:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/11170007/7001

[commit-bot: I haz the power, 2012-10-16 21:03:36]

Change committed as 162243