Create TemplateUrlData to base::Dictionary utility functions

Create TemplateUrlData to base::Dictionary utility functions

This CL create utility functions that serialize TemplateURLData to and
from base::DictionaryValue. This is useful to store TemplateURLData
to preferences. Splitted from
https://codereview.chromium.org/2479113002/.

R=pkasting@chromium.org, stevet@chromium.org

Committed: https://crrev.com/6028ec2fcc7ca0d036e903ff702de77f60a69071
Cr-Commit-Position: refs/heads/master@{#433955}

[Alexander Yashkin, 2016-11-14 10:17:23]

Where is another TemplateURLData serialization function
GetPrepopulatedTemplateURLData
in components/search_engines/template_url_prepopulate_data.cc. It restores
TemplateURLData from kSearchProviderOverrides preference.
At first I thought about refactoring it to use common util function, but than I
found that its slightly different, it uses "name" instead of "short_name"
constant, "encoding" param is single string instead of "input_encodings" list of
strings as used in DefaultSearchManager.

So I decided to leave it as it is, WDYT?

[Peter Kasting, 2016-11-14 18:08:53]

On 2016/11/14 10:17:23, Alexander Yashkin wrote:
> Where is another TemplateURLData serialization function
> GetPrepopulatedTemplateURLData
> in components/search_engines/template_url_prepopulate_data.cc. It restores
> TemplateURLData from kSearchProviderOverrides preference.
> At first I thought about refactoring it to use common util function, but than
I
> found that its slightly different, it uses "name" instead of "short_name"
> constant, "encoding" param is single string instead of "input_encodings" list
of
> strings as used in DefaultSearchManager.
> 
> So I decided to leave it as it is, WDYT?

Can we find a way to combine these?  I'm really not keen on two long functions
which do almost but not exactly the same thing.

[Alexander Yashkin, 2016-11-16 11:05:32]

On 2016/11/14 at 18:08:53, pkasting wrote:
> On 2016/11/14 10:17:23, Alexander Yashkin wrote:
> > Where is another TemplateURLData serialization function
> > GetPrepopulatedTemplateURLData
> > in components/search_engines/template_url_prepopulate_data.cc. It restores
> > TemplateURLData from kSearchProviderOverrides preference.
> > At first I thought about refactoring it to use common util function, but
than I
> > found that its slightly different, it uses "name" instead of "short_name"
> > constant, "encoding" param is single string instead of "input_encodings"
list of
> > strings as used in DefaultSearchManager.
> > 
> > So I decided to leave it as it is, WDYT?
> 
> Can we find a way to combine these?  I'm really not keen on two long functions
which do almost but not exactly the same thing.

I tried to use same common function to deserialize TemplateURLData from
kSearchProviderOverrides and kDefaultSearchProviderDataPrefName yet the
difference is large enough.
Also, the set of data stored in kSearchProviderOverrides dictionary is closer to
PrepopulatedEngine struct, than to TemplateUrlData struct.

The difference between storage format of kSearchProviderOverrides and
kDefaultSearchProviderDataPrefName:
kSearchProviderOverrides dictionary uses "short_name" vs "name"
"suggest_url" vs "suggestions_url"
"suggest_url_post_params" vs "suggestions_url_post_params"
"encoding" string vs "input_encodings" list of string
kSearchProviderOverrides uses "id" field to store prepopulated_id,
kDefaultSearchProviderDataPrefName uses "id" field to store id and
"prepopulated_id" to store prepopulated_id.

Names used by kSearchProviderOverrides dictionary are closer to legacy
preference names like kDefaultSearchProviderSuggestURL.

I extracted deserialization of kSearchProviderOverrides dictionary to separate
util function - TemplateURLDataFromOverrideDictionary. 
As I see in code kSearchProviderOverrides is created from
BrandcodedDefaultSettings which is loaded by ProfileResetter or I assume can be
part of distributive.

I can think of another way to use common serialization algorithm for
kSearchProviderOverrides and kDefaultSearchProviderDataPrefName.
I can convert kSearchProviderOverrides pref to dictionary in new format when its
initialized from BrandcodedDefaultSettings. 
To distinguish converted and non converted value I can add some field, for
example "version" to new dictionary.
When reading kSearchProviderOverrides I will check for "version" field and read
accordingly old or new fields.
So, for some time both version will coexist.

WDYT? Is it worth the effort?

[Peter Kasting, 2016-11-17 00:41:03]

I am definitely a fan of using the same names in both cases long-term.

It sounds as if some of these names may be set by external data files created by
customers, which we can't control?  Is that interpretation correct?

If so, we can do a couple things here:

(1) Update the functions that deserialize these to read either name (if both
names for the same field occur, newer name wins).  Always serialize as the newer
names.  This should silently convert existing data.
(2) Update our docs to use the current names.  This should tell people the right
things for new files they create.
(3) See if there's some kind of deprecation warning we can emit somewhere if we
encounter old names, to warn people to update them.  Maybe this isn't necessary
depending on the scope of the problem?  I don't know how feasible it is either. 
Perhaps the policy folks could help.
(4) Eventually, rip out all support for the old names.

[Alexander Yashkin, 2016-11-17 20:19:08]

On 2016/11/17 at 00:41:03, pkasting wrote:
> I am definitely a fan of using the same names in both cases long-term.
> 
> It sounds as if some of these names may be set by external data files created
by customers, which we can't control?  Is that interpretation correct?

As I searched Chromium sources I found that kSearchProviderOverrides preference
is set only by ProfileResetter which downloads small file 
with most important settings from google server. ProfileResetter sends brandcode
to server while loading this file.

But this pref must already exist at browser first run to have effect on new
branded browser installation. My guess is - it is set by installer.

> 
> If so, we can do a couple things here:
> 
> (1) Update the functions that deserialize these to read either name (if both
names for the same field occur, newer name wins).  Always serialize as the newer
names.  This should silently convert existing data.
I tried to implement similar solution in latest commit.

> (2) Update our docs to use the current names.  This should tell people the
right things for new files they create.
I could not find any docs on creating chromium custom branding distributives. :(

> (3) See if there's some kind of deprecation warning we can emit somewhere if
we encounter old names, to warn people to update them.  Maybe this isn't
necessary depending on the scope of the problem?  I don't know how feasible it
is either.  Perhaps the policy folks could help.
> (4) Eventually, rip out all support for the old names.
Good plan.


Also.
Looking at DefaultSearchManager sources I can see that where was an attempt to
convert storage of DSE from several string/list preferences to single
Dictionary.
To me it seems like this work is already finished. I found such comments in
default_search_pref_migration files:

"// Migrates a DSE value stored in separate String/List/..Value preferences by
// M35 or earlier to the new single DictionaryValue used in M36.
// Operates immediately if |pref_service| is fully initialized. Otherwise, waits
// for the PrefService to load using an observer.

// TODO(erikwright): Remove this migration logic when this stat approaches
// zero.
UMA_HISTOGRAM_BOOLEAN("Search.MigratedPrefToDictionaryValue", true);"

Do you think its time to delete migration logic and legacy DSE prefs?
Currently, its very confusing to analyze code that works with two sets of prefs
for DSE - legacy and modern.

[Peter Kasting, 2016-11-17 20:26:58]

On 2016/11/17 20:19:08, Alexander Yashkin wrote:
> On 2016/11/17 at 00:41:03, pkasting wrote:
> > I am definitely a fan of using the same names in both cases long-term.
> > 
> > It sounds as if some of these names may be set by external data files
created
> by customers, which we can't control?  Is that interpretation correct?
> 
> As I searched Chromium sources I found that kSearchProviderOverrides
preference
> is set only by ProfileResetter which downloads small file 
> with most important settings from google server. ProfileResetter sends
brandcode
> to server while loading this file.

Does that mean we need to update a golden file on a server somewhere?

> But this pref must already exist at browser first run to have effect on new
> branded browser installation. My guess is - it is set by installer.

Maybe blame for the relevant code here would direct you to a person who might
know definitively?  If we need to update the installer code we'll want to make
sure that happens.

> > (2) Update our docs to use the current names.  This should tell people the
> right things for new files they create.
> I could not find any docs on creating chromium custom branding distributives.
:(

blumberg@chromium.org is a PM in charge of enterprise stuff.  I would reach out
to him with a concise summary of all your remaining questions around policy,
docs, what capabilities we expose, and/or making sure we change all the right
bits.

> Also.
> Looking at DefaultSearchManager sources I can see that where was an attempt to
> convert storage of DSE from several string/list preferences to single
> Dictionary.
> To me it seems like this work is already finished. I found such comments in
> default_search_pref_migration files:
> 
> "// Migrates a DSE value stored in separate String/List/..Value preferences by
> // M35 or earlier to the new single DictionaryValue used in M36.
> // Operates immediately if |pref_service| is fully initialized. Otherwise,
waits
> // for the PrefService to load using an observer.
> 
> // TODO(erikwright): Remove this migration logic when this stat approaches
> // zero.
> UMA_HISTOGRAM_BOOLEAN("Search.MigratedPrefToDictionaryValue", true);"
> 
> Do you think its time to delete migration logic and legacy DSE prefs?
> Currently, its very confusing to analyze code that works with two sets of
prefs
> for DSE - legacy and modern.

Yes, it is safe to rip out code that migrates a pre-M36 profile at this point.

[Alexander Yashkin, 2016-11-18 05:11:07]

On 2016/11/17 at 20:26:58, pkasting wrote:
> On 2016/11/17 20:19:08, Alexander Yashkin wrote:
> > On 2016/11/17 at 00:41:03, pkasting wrote:
> > > I am definitely a fan of using the same names in both cases long-term.
> > > 
> > > It sounds as if some of these names may be set by external data files
created
> > by customers, which we can't control?  Is that interpretation correct?
> > 
> > As I searched Chromium sources I found that kSearchProviderOverrides
preference
> > is set only by ProfileResetter which downloads small file 
> > with most important settings from google server. ProfileResetter sends
brandcode
> > to server while loading this file.
> 
> Does that mean we need to update a golden file on a server somewhere?
Yes, looks like it.

> 
> > But this pref must already exist at browser first run to have effect on new
> > branded browser installation. My guess is - it is set by installer.
> 
> Maybe blame for the relevant code here would direct you to a person who might
know definitively?  If we need to update the installer code we'll want to make
sure that happens.
> 
> > > (2) Update our docs to use the current names.  This should tell people the
> > right things for new files they create.
> > I could not find any docs on creating chromium custom branding
distributives. :(
> 
> blumberg@chromium.org is a PM in charge of enterprise stuff.  I would reach
out to him with a concise summary of all your remaining questions around policy,
docs, what capabilities we expose, and/or making sure we change all the right
bits.
> 
> > Also.
> > Looking at DefaultSearchManager sources I can see that where was an attempt
to
> > convert storage of DSE from several string/list preferences to single
> > Dictionary.
> > To me it seems like this work is already finished. I found such comments in
> > default_search_pref_migration files:
> > 
> > "// Migrates a DSE value stored in separate String/List/..Value preferences
by
> > // M35 or earlier to the new single DictionaryValue used in M36.
> > // Operates immediately if |pref_service| is fully initialized. Otherwise,
waits
> > // for the PrefService to load using an observer.
> > 
> > // TODO(erikwright): Remove this migration logic when this stat approaches
> > // zero.
> > UMA_HISTOGRAM_BOOLEAN("Search.MigratedPrefToDictionaryValue", true);"
> > 
> > Do you think its time to delete migration logic and legacy DSE prefs?
> > Currently, its very confusing to analyze code that works with two sets of
prefs
> > for DSE - legacy and modern.
> 
> Yes, it is safe to rip out code that migrates a pre-M36 profile at this point.

Sorry for inconstancy, by my current target goal is problem with extension DSE
in https://codereview.chromium.org/2479113002/
and I need only separate TemplateURLData serialization/deserialization function
for it.
And current CL becoming more and more complicated and broad.

Maybe I can return it to Patch2 state, where its still just a several functions
ripped out to common files?
When I can continue to https://codereview.chromium.org/2479113002/.
And after extension DSE bug is solved we can return to refactoring big scheme of
things? :)
Also deleting support for legacy prefs first would be more right before
introducing more chaos :)

[Peter Kasting, 2016-11-18 06:34:09]

On 2016/11/18 05:11:07, Alexander Yashkin wrote:
> On 2016/11/17 at 20:26:58, pkasting wrote:
> > On 2016/11/17 20:19:08, Alexander Yashkin wrote:
> > > On 2016/11/17 at 00:41:03, pkasting wrote:
> > > > I am definitely a fan of using the same names in both cases long-term.
> > > > 
> > > > It sounds as if some of these names may be set by external data files
> created
> > > by customers, which we can't control?  Is that interpretation correct?
> > > 
> > > As I searched Chromium sources I found that kSearchProviderOverrides
> preference
> > > is set only by ProfileResetter which downloads small file 
> > > with most important settings from google server. ProfileResetter sends
> brandcode
> > > to server while loading this file.
> > 
> > Does that mean we need to update a golden file on a server somewhere?
> Yes, looks like it.
> 
> > 
> > > But this pref must already exist at browser first run to have effect on
new
> > > branded browser installation. My guess is - it is set by installer.
> > 
> > Maybe blame for the relevant code here would direct you to a person who
might
> know definitively?  If we need to update the installer code we'll want to make
> sure that happens.
> > 
> > > > (2) Update our docs to use the current names.  This should tell people
the
> > > right things for new files they create.
> > > I could not find any docs on creating chromium custom branding
> distributives. :(
> > 
> > mailto:blumberg@chromium.org is a PM in charge of enterprise stuff.  I would
reach
> out to him with a concise summary of all your remaining questions around
policy,
> docs, what capabilities we expose, and/or making sure we change all the right
> bits.
> > 
> > > Also.
> > > Looking at DefaultSearchManager sources I can see that where was an
attempt
> to
> > > convert storage of DSE from several string/list preferences to single
> > > Dictionary.
> > > To me it seems like this work is already finished. I found such comments
in
> > > default_search_pref_migration files:
> > > 
> > > "// Migrates a DSE value stored in separate String/List/..Value
preferences
> by
> > > // M35 or earlier to the new single DictionaryValue used in M36.
> > > // Operates immediately if |pref_service| is fully initialized. Otherwise,
> waits
> > > // for the PrefService to load using an observer.
> > > 
> > > // TODO(erikwright): Remove this migration logic when this stat approaches
> > > // zero.
> > > UMA_HISTOGRAM_BOOLEAN("Search.MigratedPrefToDictionaryValue", true);"
> > > 
> > > Do you think its time to delete migration logic and legacy DSE prefs?
> > > Currently, its very confusing to analyze code that works with two sets of
> prefs
> > > for DSE - legacy and modern.
> > 
> > Yes, it is safe to rip out code that migrates a pre-M36 profile at this
point.
> 
> Sorry for inconstancy, by my current target goal is problem with extension DSE
> in https://codereview.chromium.org/2479113002/
> and I need only separate TemplateURLData serialization/deserialization
function
> for it.
> And current CL becoming more and more complicated and broad.
> 
> Maybe I can return it to Patch2 state, where its still just a several
functions
> ripped out to common files?
> When I can continue to https://codereview.chromium.org/2479113002/.
> And after extension DSE bug is solved we can return to refactoring big scheme
of
> things? :)
> Also deleting support for legacy prefs first would be more right before
> introducing more chaos :)

I'm a big fan of doing things in a series of smaller, targeted patches instead
of in single monolithic ones.  Even just moving a function to a new file and
changing it to be more broad sounds like two separate CLS to me.

Is it possible to do the series of patches that gets this stuff right first?  I
worry that if we don't address this now, it's not going to get addressed.  But
you're right that it's not really fair to hold you hostage to this.

[Alexander Yashkin, 2016-11-18 08:25:40]

On 2016/11/18 at 06:34:09, pkasting wrote:
> On 2016/11/18 05:11:07, Alexander Yashkin wrote:
> > On 2016/11/17 at 20:26:58, pkasting wrote:
> > > On 2016/11/17 20:19:08, Alexander Yashkin wrote:
> > > > On 2016/11/17 at 00:41:03, pkasting wrote:
> > > > > I am definitely a fan of using the same names in both cases long-term.
> > > > > 
> > > > > It sounds as if some of these names may be set by external data files
> > created
> > > > by customers, which we can't control?  Is that interpretation correct?
> > > > 
> > > > As I searched Chromium sources I found that kSearchProviderOverrides
> > preference
> > > > is set only by ProfileResetter which downloads small file 
> > > > with most important settings from google server. ProfileResetter sends
> > brandcode
> > > > to server while loading this file.
> > > 
> > > Does that mean we need to update a golden file on a server somewhere?
> > Yes, looks like it.
> > 
> > > 
> > > > But this pref must already exist at browser first run to have effect on
new
> > > > branded browser installation. My guess is - it is set by installer.
> > > 
> > > Maybe blame for the relevant code here would direct you to a person who
might
> > know definitively?  If we need to update the installer code we'll want to
make
> > sure that happens.
> > > 
> > > > > (2) Update our docs to use the current names.  This should tell people
the
> > > > right things for new files they create.
> > > > I could not find any docs on creating chromium custom branding
> > distributives. :(
> > > 
> > > mailto:blumberg@chromium.org is a PM in charge of enterprise stuff.  I
would reach
> > out to him with a concise summary of all your remaining questions around
policy,
> > docs, what capabilities we expose, and/or making sure we change all the
right
> > bits.
> > > 
> > > > Also.
> > > > Looking at DefaultSearchManager sources I can see that where was an
attempt
> > to
> > > > convert storage of DSE from several string/list preferences to single
> > > > Dictionary.
> > > > To me it seems like this work is already finished. I found such comments
in
> > > > default_search_pref_migration files:
> > > > 
> > > > "// Migrates a DSE value stored in separate String/List/..Value
preferences
> > by
> > > > // M35 or earlier to the new single DictionaryValue used in M36.
> > > > // Operates immediately if |pref_service| is fully initialized.
Otherwise,
> > waits
> > > > // for the PrefService to load using an observer.
> > > > 
> > > > // TODO(erikwright): Remove this migration logic when this stat
approaches
> > > > // zero.
> > > > UMA_HISTOGRAM_BOOLEAN("Search.MigratedPrefToDictionaryValue", true);"
> > > > 
> > > > Do you think its time to delete migration logic and legacy DSE prefs?
> > > > Currently, its very confusing to analyze code that works with two sets
of
> > prefs
> > > > for DSE - legacy and modern.
> > > 
> > > Yes, it is safe to rip out code that migrates a pre-M36 profile at this
point.
> > 
> > Sorry for inconstancy, by my current target goal is problem with extension
DSE
> > in https://codereview.chromium.org/2479113002/
> > and I need only separate TemplateURLData serialization/deserialization
function
> > for it.
> > And current CL becoming more and more complicated and broad.
> > 
> > Maybe I can return it to Patch2 state, where its still just a several
functions
> > ripped out to common files?
> > When I can continue to https://codereview.chromium.org/2479113002/.
> > And after extension DSE bug is solved we can return to refactoring big
scheme of
> > things? :)
> > Also deleting support for legacy prefs first would be more right before
> > introducing more chaos :)
> 
> I'm a big fan of doing things in a series of smaller, targeted patches instead
of in single monolithic ones.  Even just moving a function to a new file and
changing it to be more broad sounds like two separate CLS to me.
> 
> Is it possible to do the series of patches that gets this stuff right first? 
I worry that if we don't address this now, it's not going to get addressed.  But
you're right that it's not really fair to hold you hostage to this.

Returned to common functions refactor out to util file.

My next goal is https://codereview.chromium.org/2479113002/
After that I'll try to delete legacy DSE prefs and maybe unite serialization
format of kSearchProviderOverrides and kDefaultSearchProviderDataPrefName.

[Peter Kasting, 2016-11-20 07:57:50]

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

https://codereview.chromium.org/2497853002/diff/60001/components/search_engin...
components/search_engines/default_search_manager.cc:165:
*TemplateURLDataToDictionary(data));
Nit: Args on subsequent lines must be indented matching the first arg.  git cl
format will handle this for you.

https://codereview.chromium.org/2497853002/diff/60001/components/search_engin...
components/search_engines/default_search_manager.cc:264:
TemplateURLDataFromDictionary(*url_dict));
Nit: Prefer assignment-style to constructor-style init for "simple"
initializations like this; see
https://sites.google.com/a/chromium.org/dev/developers/coding-style/cpp-dos-a...
.  (Applies in other files too)

https://codereview.chromium.org/2497853002/diff/60001/components/search_engin...
components/search_engines/default_search_manager.cc:267: return;
Don't handle DCHECK failure; see
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
.

Either the function cannot fail, in which case the conditional should disappear,
or it can, in which case the DCHECK should disappear.  In this case, it can;
we're reading from a dictionary on disk, which could be missing required fields,
e.g. due to hand-editing the profile.  So the DCHECK should go away.

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

https://codereview.chromium.org/2497853002/diff/60001/components/search_engin...
components/search_engines/template_url_data_util.cc:20: 
Nit: No blank line

https://codereview.chromium.org/2497853002/diff/60001/components/search_engin...
components/search_engines/template_url_data_util.cc:21: std::string search_url;
Is everything from here on just taken verbatim from the other files, or do I
need to actually review anything?  If it's pure copy-and-paste I won't bother
looking.

https://codereview.chromium.org/2497853002/diff/60001/components/search_engin...
components/search_engines/template_url_data_util.cc:174:
std::unique_ptr<TemplateURLData> MakePrepopulatedTemplateURLData(
I think this should just be an additional constructor on TemplateURLData.

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

https://codereview.chromium.org/2497853002/diff/60001/components/search_engin...
components/search_engines/template_url_data_util.h:20: // Restore
TemplateURLData structure from base::DictionaryValue as stored by
Nit: Function overview comments should be descriptive ("restores") rather than
imperative ("restore"); see
http://google.github.io/styleguide/cppguide.html#Function_Comments .

Also, prefer to keep comments brief by not repeating information (e.g. type
names, or the word "structure") that's clear from the function declaration; for
example, here we could just say "Deserializes a TemplateURLData from |dict|. 
The serialization is assumed to be the one used by
TemplateURLDataToDictionary()."

Note that I wouldn't say "DefaultSearchManager format" because that's a class
that calls these functions, it's not really a documented format.  By definition,
its format is whatever these do.  Hence the phrasing above.

(Several places)

https://codereview.chromium.org/2497853002/diff/60001/components/search_engin...
components/search_engines/template_url_data_util.h:35: //
kSearchProviderOverrides pref.
Nit: Here you might clarify that this differs from the format above for
historical reasons, and possibly add a TODO to eliminate this via migration.

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

https://codereview.chromium.org/2497853002/diff/60001/components/search_engin...
components/search_engines/template_url_prepopulate_data.cc:995: (t_url =
TemplateURLDataFromOverrideDictionary(*engine)))
Nit: Don't use statements with side effects in conditionals.  Fixing this will
also allow you to narrow the scope of |t_url| (see
http://google.github.io/styleguide/cppguide.html#Local_Variables ):

    if (list->GetDictionary(i, &engine)) {
      auto t_url = TemplateURLDataFromOverrideDictionary(*engine);
      if (t_url)
        t_urls.push_back(std::move(t_url));
    }

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

https://codereview.chromium.org/2497853002/diff/60001/components/search_engin...
components/search_engines/template_url_prepopulate_data_unittest.cc:157: 
Nit: Why this blank line?

https://codereview.chromium.org/2497853002/diff/60001/components/search_engin...
components/search_engines/template_url_prepopulate_data_unittest.cc:161:
EXPECT_TRUE(t_urls[0]->last_modified.is_null());
Is it important to check these?  If so, should we be checking them below as well
(around line 190)?  Or do they not really have anything to do with what this
particular test is trying to verify?  (For example, if we're just checking the
default values of these fields, and that's tested by another unittest, then
having these here doesn't really increase test coverage, it just means more
places to fix if we change something.)

[Alexander Yashkin, 2016-11-20 20:24:05]

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

https://codereview.chromium.org/2497853002/diff/60001/components/search_engin...
components/search_engines/default_search_manager.cc:165:
*TemplateURLDataToDictionary(data));
On 2016/11/20 at 07:57:50, Peter Kasting wrote:
> Nit: Args on subsequent lines must be indented matching the first arg.  git cl
format will handle this for you.

Fixed.

https://codereview.chromium.org/2497853002/diff/60001/components/search_engin...
components/search_engines/default_search_manager.cc:264:
TemplateURLDataFromDictionary(*url_dict));
On 2016/11/20 at 07:57:50, Peter Kasting wrote:
> Nit: Prefer assignment-style to constructor-style init for "simple"
initializations like this; see
https://sites.google.com/a/chromium.org/dev/developers/coding-style/cpp-dos-a...
.  (Applies in other files too)

Fixed, thanks for link.

https://codereview.chromium.org/2497853002/diff/60001/components/search_engin...
components/search_engines/default_search_manager.cc:267: return;
On 2016/11/20 at 07:57:50, Peter Kasting wrote:
> Don't handle DCHECK failure; see
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
.
> 
> Either the function cannot fail, in which case the conditional should
disappear, or it can, in which case the DCHECK should disappear.  In this case,
it can; we're reading from a dictionary on disk, which could be missing required
fields, e.g. due to hand-editing the profile.  So the DCHECK should go away.

DCHECK deleted.

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

https://codereview.chromium.org/2497853002/diff/60001/components/search_engin...
components/search_engines/template_url_data_util.cc:20: 
On 2016/11/20 at 07:57:50, Peter Kasting wrote:
> Nit: No blank line

Fixed

https://codereview.chromium.org/2497853002/diff/60001/components/search_engin...
components/search_engines/template_url_data_util.cc:21: std::string search_url;
On 2016/11/20 at 07:57:50, Peter Kasting wrote:
> Is everything from here on just taken verbatim from the other files, or do I
need to actually review anything?  If it's pure copy-and-paste I won't bother
looking.

I have added check for short_name field to be required, following comments in
TemplateURLData:
// Private so we can enforce using the setters and thus enforce that these
// fields are never empty.
Yet I am not sure that this change does not break something.


Also I have added serialization/deserialization of contextual_search_url field
to TemplateURLDataFromDictionary/TemplateURLDataToDictionary which I believe was
forgotten.
It was deserialized from override engines pref but not from default_search pref.
I don't think such behaviour was intentional.

Other code is just copy/pasted.

https://codereview.chromium.org/2497853002/diff/60001/components/search_engin...
components/search_engines/template_url_data_util.cc:174:
std::unique_ptr<TemplateURLData> MakePrepopulatedTemplateURLData(
On 2016/11/20 at 07:57:50, Peter Kasting wrote:
> I think this should just be an additional constructor on TemplateURLData.

What I don't like about an idea making it a constructor:
This function takes in arguments not all fields of TemplateURLData but only
those that exist in PrepopulatedEngine struct. It also initializes fields
safe_for_autoreplace, date_created, last_modified with values that are
appropriate for TemplateURLData created from PrepopulatedEngine struct but not
from other sources. Its name reflects this specifics -
MakePrepopulatedTemplateURLData.
 
Alternative would be creating universal constructor for TemplateURLData that
must get all fields in arguments - huge list of more than 20 values.
And it would be used from same places only to create TemplateURLData for
prepopulated engines - from override preference or from internal list of
PrepopulatedEngine structs.

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

https://codereview.chromium.org/2497853002/diff/60001/components/search_engin...
components/search_engines/template_url_data_util.h:20: // Restore
TemplateURLData structure from base::DictionaryValue as stored by
On 2016/11/20 at 07:57:50, Peter Kasting wrote:
> Nit: Function overview comments should be descriptive ("restores") rather than
imperative ("restore"); see
http://google.github.io/styleguide/cppguide.html#Function_Comments .
> 
> Also, prefer to keep comments brief by not repeating information (e.g. type
names, or the word "structure") that's clear from the function declaration; for
example, here we could just say "Deserializes a TemplateURLData from |dict|. 
The serialization is assumed to be the one used by
TemplateURLDataToDictionary()."
> 
> Note that I wouldn't say "DefaultSearchManager format" because that's a class
that calls these functions, it's not really a documented format.  By definition,
its format is whatever these do.  Hence the phrasing above.
> 
> (Several places)

Fixed, thanks.

https://codereview.chromium.org/2497853002/diff/60001/components/search_engin...
components/search_engines/template_url_data_util.h:35: //
kSearchProviderOverrides pref.
On 2016/11/20 at 07:57:50, Peter Kasting wrote:
> Nit: Here you might clarify that this differs from the format above for
historical reasons, and possibly add a TODO to eliminate this via migration.

Comment fixed.

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

https://codereview.chromium.org/2497853002/diff/60001/components/search_engin...
components/search_engines/template_url_prepopulate_data.cc:995: (t_url =
TemplateURLDataFromOverrideDictionary(*engine)))
On 2016/11/20 at 07:57:50, Peter Kasting wrote:
> Nit: Don't use statements with side effects in conditionals.  Fixing this will
also allow you to narrow the scope of |t_url| (see
http://google.github.io/styleguide/cppguide.html#Local_Variables ):
> 
>     if (list->GetDictionary(i, &engine)) {
>       auto t_url = TemplateURLDataFromOverrideDictionary(*engine);
>       if (t_url)
>         t_urls.push_back(std::move(t_url));
>     }

Thanks, fixed.

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

https://codereview.chromium.org/2497853002/diff/60001/components/search_engin...
components/search_engines/template_url_prepopulate_data_unittest.cc:157: 
On 2016/11/20 at 07:57:50, Peter Kasting wrote:
> Nit: Why this blank line?

Deleted.

https://codereview.chromium.org/2497853002/diff/60001/components/search_engin...
components/search_engines/template_url_prepopulate_data_unittest.cc:161:
EXPECT_TRUE(t_urls[0]->last_modified.is_null());
On 2016/11/20 at 07:57:50, Peter Kasting wrote:
> Is it important to check these?  If so, should we be checking them below as
well (around line 190)?  Or do they not really have anything to do with what
this particular test is trying to verify?  (For example, if we're just checking
the default values of these fields, and that's tested by another unittest, then
having these here doesn't really increase test coverage, it just means more
places to fix if we change something.)

I saw that fields safe_for_autoreplace, date_created and last_modified are
initialized with non default values for TemplateURLData initialized from
prepopulated engines. So I decided to check this in tests too.
They are not tested anywhere, also I added check for their values below in
TemplateURLPrepopulateDataTest.ProvidersFromPrepopulated

[Peter Kasting, 2016-11-21 02:17:08]

LGTM

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

https://codereview.chromium.org/2497853002/diff/60001/components/search_engin...
components/search_engines/template_url_data_util.cc:21: std::string search_url;
On 2016/11/20 20:24:05, Alexander Yashkin wrote:
> On 2016/11/20 at 07:57:50, Peter Kasting wrote:
> > Is everything from here on just taken verbatim from the other files, or do I
> need to actually review anything?  If it's pure copy-and-paste I won't bother
> looking.
> 
> I have added check for short_name field to be required, following comments in
> TemplateURLData:
> // Private so we can enforce using the setters and thus enforce that these
> // fields are never empty.
> Yet I am not sure that this change does not break something.

Good catch!  Yes, that would have resulted in a DCHECK failure at runtime. 
Better to catch and handle it here.

> Also I have added serialization/deserialization of contextual_search_url field
> to TemplateURLDataFromDictionary/TemplateURLDataToDictionary which I believe
was
> forgotten.
> It was deserialized from override engines pref but not from default_search
pref.
> I don't think such behaviour was intentional.

Probably just never noticed since I doubt anyone has tried to use it.  Adding
that seems like a good change.

Thanks for the summary!

https://codereview.chromium.org/2497853002/diff/60001/components/search_engin...
components/search_engines/template_url_data_util.cc:174:
std::unique_ptr<TemplateURLData> MakePrepopulatedTemplateURLData(
On 2016/11/20 20:24:05, Alexander Yashkin wrote:
> On 2016/11/20 at 07:57:50, Peter Kasting wrote:
> > I think this should just be an additional constructor on TemplateURLData.
> 
> What I don't like about an idea making it a constructor:
> This function takes in arguments not all fields of TemplateURLData but only
> those that exist in PrepopulatedEngine struct. It also initializes fields
> safe_for_autoreplace, date_created, last_modified with values that are
> appropriate for TemplateURLData created from PrepopulatedEngine struct but not
> from other sources. Its name reflects this specifics -
> MakePrepopulatedTemplateURLData.

It seems OK, and probably even preferable, that |date_created| and
|last_modified| be set to Time::Now() instead of Time() for these engines.  If
these are going to be considered "prepopulated" engines, then they'll be created
once during first run, and setting their creation and modification dates to that
time is reasonable.

If we changed these, the only fields whose values would be explicitly
initialized differently from the current null constructor are
show_in_default_list, which is gone as of
https://chromium.googlesource.com/chromium/src/+/cf06dd01d2516e1509acd1e99b20...
, and safe_for_autoreplace.  There are several ways to deal with that:

(1) Change the null constructor to also set this field to true, and then change
callers of that constructor so that the ones which wanted it true anyway do
nothing, and the ones which wanted it false set that explicitly.
(2) Set this field to false in this constructor, but set it true explicitly in
the callers here.
(3) Just have these two fields differ, and add a comment on the constructor
declaration a la:

// Creates a TemplateURLData suitable for prepopulated engines.  Note that
unlike in the default constructor, |safe_for_autoreplace| will be set to true.
TemplateURLData(...);

(4) Take this field's value as an argument to the constructor.

I'd be OK with any of these routes, personally.

Would doing any of these be satisfactory to you?

The other option, as you mentioned, is to just take all the fields in the
constructor argument list.  We're already passing almost everything anyway, so
this wouldn't be hugely worse, and I'd be OK with that too, although it doesn't
seem like we need the flexibility; but maybe some test code could be simplified
using such a constructor?

https://codereview.chromium.org/2497853002/diff/60001/components/search_engin...
components/search_engines/template_url_data_util.cc:177: const
base::StringPiece& search_url,
Nit: Since most of these are being used as strings below, and in some cases
passed in as strings as well, I'd change them to be string&s instead of
StringPiece&s, to avoid converting back and forth in some callers.

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

https://codereview.chromium.org/2497853002/diff/60001/components/search_engin...
components/search_engines/template_url_prepopulate_data_unittest.cc:161:
EXPECT_TRUE(t_urls[0]->last_modified.is_null());
On 2016/11/20 20:24:05, Alexander Yashkin wrote:
> On 2016/11/20 at 07:57:50, Peter Kasting wrote:
> > Is it important to check these?  If so, should we be checking them below as
> well (around line 190)?  Or do they not really have anything to do with what
> this particular test is trying to verify?  (For example, if we're just
checking
> the default values of these fields, and that's tested by another unittest,
then
> having these here doesn't really increase test coverage, it just means more
> places to fix if we change something.)
> 
> I saw that fields safe_for_autoreplace, date_created and last_modified are
> initialized with non default values for TemplateURLData initialized from
> prepopulated engines. So I decided to check this in tests too.
> They are not tested anywhere, also I added check for their values below in
> TemplateURLPrepopulateDataTest.ProvidersFromPrepopulated

Seems fine, but note that if you follow my suggestion elsewhere to make this
functionality set the two time fields to Now() that the EXPECTs on these fields
should probably just disappear from both these places.

https://codereview.chromium.org/2497853002/diff/80001/components/search_engin...
File components/search_engines/template_url_data_util.h (right):

https://codereview.chromium.org/2497853002/diff/80001/components/search_engin...
components/search_engines/template_url_data_util.h:24: // Serializes a
TemplateURLData to |dict|
Nit: Trailing period

https://codereview.chromium.org/2497853002/diff/80001/components/search_engin...
components/search_engines/template_url_data_util.h:33: //
kSearchProviderOverrides pref. Name of values in |dict| are different from
Nit: Maybe this second sentence would be slightly more grammatical? "The field
names in |dict| differ from those used in the To/FromDictionary functions above
for historical reasons."

[Alexander Yashkin, 2016-11-21 13:43:53]

On 2016/11/21 at 02:17:08, pkasting wrote:
> LGTM
> 
>
https://codereview.chromium.org/2497853002/diff/60001/components/search_engin...
> File components/search_engines/template_url_data_util.cc (right):
> 
>
https://codereview.chromium.org/2497853002/diff/60001/components/search_engin...
> components/search_engines/template_url_data_util.cc:21: std::string
search_url;
> On 2016/11/20 20:24:05, Alexander Yashkin wrote:
> > On 2016/11/20 at 07:57:50, Peter Kasting wrote:
> > > Is everything from here on just taken verbatim from the other files, or do
I
> > need to actually review anything?  If it's pure copy-and-paste I won't
bother
> > looking.
> > 
> > I have added check for short_name field to be required, following comments
in
> > TemplateURLData:
> > // Private so we can enforce using the setters and thus enforce that these
> > // fields are never empty.
> > Yet I am not sure that this change does not break something.
> 
> Good catch!  Yes, that would have resulted in a DCHECK failure at runtime. 
Better to catch and handle it here.
> 
> > Also I have added serialization/deserialization of contextual_search_url
field
> > to TemplateURLDataFromDictionary/TemplateURLDataToDictionary which I believe
was
> > forgotten.
> > It was deserialized from override engines pref but not from default_search
pref.
> > I don't think such behaviour was intentional.
> 
> Probably just never noticed since I doubt anyone has tried to use it.  Adding
that seems like a good change.
> 
> Thanks for the summary!
> 
>
https://codereview.chromium.org/2497853002/diff/60001/components/search_engin...
> components/search_engines/template_url_data_util.cc:174:
std::unique_ptr<TemplateURLData> MakePrepopulatedTemplateURLData(
> On 2016/11/20 20:24:05, Alexander Yashkin wrote:
> > On 2016/11/20 at 07:57:50, Peter Kasting wrote:
> > > I think this should just be an additional constructor on TemplateURLData.
> > 
> > What I don't like about an idea making it a constructor:
> > This function takes in arguments not all fields of TemplateURLData but only
> > those that exist in PrepopulatedEngine struct. It also initializes fields
> > safe_for_autoreplace, date_created, last_modified with values that are
> > appropriate for TemplateURLData created from PrepopulatedEngine struct but
not
> > from other sources. Its name reflects this specifics -
> > MakePrepopulatedTemplateURLData.
> 
> It seems OK, and probably even preferable, that |date_created| and
|last_modified| be set to Time::Now() instead of Time() for these engines.  If
these are going to be considered "prepopulated" engines, then they'll be created
once during first run, and setting their creation and modification dates to that
time is reasonable.

I looked at code and what I found:

date_created is used, I mean compared, in RemoveAutoGeneratedForUrlsBetween and
similar functions in template_url_service.cc
This functions call CanReplace which eventually returns false for TemplateUrl if
prepopulate_id > 0. 
So it seems safe to change date_created field to any value for TemplateUrls
created from prepopulated_engines.

last_modified field is compared in MergeDataAndStartSyncing and
IsLocalTemplateURLBetter in template urls syncing.
I am not an expert in sync, but after looking at this functions I think that
setting last_modified for prepopulated engines to browser first run time can
lead 
to problem. For example - changes from last installed browser will override user
modifications for search engines that were performed earlier in another browser
installation.

WDYT? Maybe we need to consult someone from sync side?


> 
> If we changed these, the only fields whose values would be explicitly
initialized differently from the current null constructor are
show_in_default_list, which is gone as of
https://chromium.googlesource.com/chromium/src/+/cf06dd01d2516e1509acd1e99b20...
, and safe_for_autoreplace.  There are several ways to deal with that:
> 
> (1) Change the null constructor to also set this field to true, and then
change callers of that constructor so that the ones which wanted it true anyway
do nothing, and the ones which wanted it false set that explicitly.
> (2) Set this field to false in this constructor, but set it true explicitly in
the callers here.
> (3) Just have these two fields differ, and add a comment on the constructor
declaration a la:
> 
> // Creates a TemplateURLData suitable for prepopulated engines.  Note that
unlike in the default constructor, |safe_for_autoreplace| will be set to true.
> TemplateURLData(...);
> 
> (4) Take this field's value as an argument to the constructor.
> 
> I'd be OK with any of these routes, personally.
> 
> Would doing any of these be satisfactory to you?

Ok, so many choices :).
I implemented number 3, also initializing date_created and last_modified times
to null, to be on safe side until situation with them is clear. 
See my comments above.

> 
> The other option, as you mentioned, is to just take all the fields in the
constructor argument list.  We're already passing almost everything anyway, so
this wouldn't be hugely worse, and I'd be OK with that too, although it doesn't
seem like we need the flexibility; but maybe some test code could be simplified
using such a constructor?
> 
>
https://codereview.chromium.org/2497853002/diff/60001/components/search_engin...
> components/search_engines/template_url_data_util.cc:177: const
base::StringPiece& search_url,
> Nit: Since most of these are being used as strings below, and in some cases
passed in as strings as well, I'd change them to be string&s instead of
StringPiece&s, to avoid converting back and forth in some callers.

I tried to use std::string first but I encountered that PrepopulatedEngine
structure contains pointers to const char* data, and some of this pointers can
be nullptr.
Initializing std::string with nullptr does not work, so StringPiece is more
convenient in this case.

> 
>
https://codereview.chromium.org/2497853002/diff/60001/components/search_engin...
> File components/search_engines/template_url_prepopulate_data_unittest.cc
(right):
> 
>
https://codereview.chromium.org/2497853002/diff/60001/components/search_engin...
> components/search_engines/template_url_prepopulate_data_unittest.cc:161:
EXPECT_TRUE(t_urls[0]->last_modified.is_null());
> On 2016/11/20 20:24:05, Alexander Yashkin wrote:
> > On 2016/11/20 at 07:57:50, Peter Kasting wrote:
> > > Is it important to check these?  If so, should we be checking them below
as
> > well (around line 190)?  Or do they not really have anything to do with what
> > this particular test is trying to verify?  (For example, if we're just
checking
> > the default values of these fields, and that's tested by another unittest,
then
> > having these here doesn't really increase test coverage, it just means more
> > places to fix if we change something.)
> > 
> > I saw that fields safe_for_autoreplace, date_created and last_modified are
> > initialized with non default values for TemplateURLData initialized from
> > prepopulated engines. So I decided to check this in tests too.
> > They are not tested anywhere, also I added check for their values below in
> > TemplateURLPrepopulateDataTest.ProvidersFromPrepopulated
> 
> Seems fine, but note that if you follow my suggestion elsewhere to make this
functionality set the two time fields to Now() that the EXPECTs on these fields
should probably just disappear from both these places.
> 
>
https://codereview.chromium.org/2497853002/diff/80001/components/search_engin...
> File components/search_engines/template_url_data_util.h (right):
> 
>
https://codereview.chromium.org/2497853002/diff/80001/components/search_engin...
> components/search_engines/template_url_data_util.h:24: // Serializes a
TemplateURLData to |dict|
> Nit: Trailing period
Fixed.

> 
>
https://codereview.chromium.org/2497853002/diff/80001/components/search_engin...
> components/search_engines/template_url_data_util.h:33: //
kSearchProviderOverrides pref. Name of values in |dict| are different from
> Nit: Maybe this second sentence would be slightly more grammatical? "The field
names in |dict| differ from those used in the To/FromDictionary functions above
for historical reasons."
Thanks, fixed.

PTAL, see if I understood your intentions right.

[Peter Kasting, 2016-11-21 20:12:36]

On 2016/11/21 13:43:53, Alexander Yashkin wrote:
> last_modified field is compared in MergeDataAndStartSyncing and
> IsLocalTemplateURLBetter in template urls syncing.
> I am not an expert in sync, but after looking at this functions I think that
> setting last_modified for prepopulated engines to browser first run time can
> lead 
> to problem. For example - changes from last installed browser will override
user
> modifications for search engines that were performed earlier in another
browser
> installation.

That seems like a fair concern.  We block users from editing the URLs of
prepopulated engines, but they can still edit the names and keywords, and such
changes could be overridden.

I would init the created time to the current time (as you noted, that seems
safe) and leave the last modified time blank.  That should prevent this.

That raises the question: should the last modified time be left blank for the
null constructor as well?  It seems like it might make sense to have that only
be set when an entry is actually modified.  This is the approach I'm also
advocating for the "last visited" timestamp that another engineer is in the
process of adding.

However, changing that is probably out of scope for this patch.

> >
>
https://codereview.chromium.org/2497853002/diff/60001/components/search_engin...
> > components/search_engines/template_url_data_util.cc:177: const
> base::StringPiece& search_url,
> > Nit: Since most of these are being used as strings below, and in some cases
> passed in as strings as well, I'd change them to be string&s instead of
> StringPiece&s, to avoid converting back and forth in some callers.
> 
> I tried to use std::string first but I encountered that PrepopulatedEngine
> structure contains pointers to const char* data, and some of this pointers can
> be nullptr.
> Initializing std::string with nullptr does not work, so StringPiece is more
> convenient in this case.

I see.  I didn't think of that.

What if we simply changed the prepopulate data to use "" instead of NULL in
these cases?  That seems just as readable and would avoid these extra
conversions.

[Peter Kasting, 2016-11-21 21:03:54]

BTW, current code still LGTM, pending resolution of outstanding discussion
points.

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

https://codereview.chromium.org/2497853002/diff/100001/components/search_engi...
components/search_engines/template_url_data.h:32: const base::StringPiece&
search_url,
Prefer to pass StringPiece by value (see
https://cs.chromium.org/chromium/src/base/strings/string_piece.h?rcl=0&l=14 ).

Obviously, if we are able to switch to something that passes these as
std::string, this becomes moot.  If not, consider adding a comment about why
these are StringPieces and not strings.

[Alexander Yashkin, 2016-11-22 06:29:27]

On 2016/11/21 at 20:12:36, pkasting wrote:
> On 2016/11/21 13:43:53, Alexander Yashkin wrote:
> > last_modified field is compared in MergeDataAndStartSyncing and
> > IsLocalTemplateURLBetter in template urls syncing.
> > I am not an expert in sync, but after looking at this functions I think that
> > setting last_modified for prepopulated engines to browser first run time can
> > lead 
> > to problem. For example - changes from last installed browser will override
user
> > modifications for search engines that were performed earlier in another
browser
> > installation.
> 
> That seems like a fair concern.  We block users from editing the URLs of
prepopulated engines, but they can still edit the names and keywords, and such
changes could be overridden.
> 
> I would init the created time to the current time (as you noted, that seems
safe) and leave the last modified time blank.  That should prevent this.

The more I think of this, the more I incline to leave both times null
initialized for prepopulated_engines. 
Whats the profit of knowing creation time for prpepopulated engine if we still
ignore its value? 
And how we could use it if it depends just on browser first run time, and does
not change afterwards?

> 
> That raises the question: should the last modified time be left blank for the
null constructor as well?  It seems like it might make sense to have that only
be set when an entry is actually modified.  This is the approach I'm also
advocating for the "last visited" timestamp that another engineer is in the
process of adding.
> 
> However, changing that is probably out of scope for this patch.
> 
> > >
> >
https://codereview.chromium.org/2497853002/diff/60001/components/search_engin...
> > > components/search_engines/template_url_data_util.cc:177: const
> > base::StringPiece& search_url,
> > > Nit: Since most of these are being used as strings below, and in some
cases
> > passed in as strings as well, I'd change them to be string&s instead of
> > StringPiece&s, to avoid converting back and forth in some callers.
> > 
> > I tried to use std::string first but I encountered that PrepopulatedEngine
> > structure contains pointers to const char* data, and some of this pointers
can
> > be nullptr.
> > Initializing std::string with nullptr does not work, so StringPiece is more
> > convenient in this case.
> 
> I see.  I didn't think of that.
> 
> What if we simply changed the prepopulate data to use "" instead of NULL in
these cases?  That seems just as readable and would avoid these extra
conversions.

Its not that easy to change PrepopulatedEngine to use std::string and this way
has its own drawbacks. 
PrepopulatedEngine is generated by json_to_struct.py from
prepopulated_engines_schema.json and is used just to hold references to compile
time constant strings. 
Changing its member to std::string will greatly inflate size of browser working
set, IMHO, because std::strings are using dynamic memory to hold values. 
And where are a lot of prepopulated engine structs declared in browser.

[Alexander Yashkin, 2016-11-22 06:36:42]

On 2016/11/21 at 21:03:54, pkasting wrote:
> BTW, current code still LGTM, pending resolution of outstanding discussion
points.
> 
>
https://codereview.chromium.org/2497853002/diff/100001/components/search_engi...
> File components/search_engines/template_url_data.h (right):
> 
>
https://codereview.chromium.org/2497853002/diff/100001/components/search_engi...
> components/search_engines/template_url_data.h:32: const base::StringPiece&
search_url,
> Prefer to pass StringPiece by value (see
https://cs.chromium.org/chromium/src/base/strings/string_piece.h?rcl=0&l=14 ).
> 
> Obviously, if we are able to switch to something that passes these as
std::string, this becomes moot.  If not, consider adding a comment about why
these are StringPieces and not strings.

Added comment for StringPieces usage, thanks for links.

[Alexander Yashkin, 2016-11-22 06:46:14]

The CQ bit was checked by a-v-y@yandex-team.ru

[Alexander Yashkin, 2016-11-22 06:46:14]

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/2497853002/#ps120001
(title: "Minor fixes")

[commit-bot: I haz the power, 2016-11-22 06:46:24]

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-11-22 07:04:37]

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

[commit-bot: I haz the power, 2016-11-22 07:04:39]

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

[Alexander Yashkin, 2016-11-22 12:59:19]

The CQ bit was checked by a-v-y@yandex-team.ru

[Alexander Yashkin, 2016-11-22 12:59:19]

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/2497853002/#ps140001
(title: "Add forgotten TemplateURLDats fields initialization")

[commit-bot: I haz the power, 2016-11-22 12:59:32]

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-11-22 13:29:42]

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

[commit-bot: I haz the power, 2016-11-22 13:29:46]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Alexander Yashkin, 2016-11-22 13:42:54]

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

[Alexander Yashkin, 2016-11-22 13:43:58]

On 2016/11/22 at 13:29:46, commit-bot wrote:
> Try jobs failed on following builders:
>   android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

PTAL ianwen@chromium.org 
android specific unittest

[Alexander Yashkin, 2016-11-22 13:44:08]

The CQ bit was checked by a-v-y@yandex-team.ru

[Alexander Yashkin, 2016-11-22 13:44:08]

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/2497853002/#ps160001
(title: "Fixed android compilation")

[commit-bot: I haz the power, 2016-11-22 13:44:28]

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-11-22 13:50:03]

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

[commit-bot: I haz the power, 2016-11-22 13:50:04]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Alexander Yashkin, 2016-11-22 15:04:20]

The CQ bit was checked by a-v-y@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2016-11-22 15:04:33]

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-11-22 15:07:40]

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

[commit-bot: I haz the power, 2016-11-22 15:07:41]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Alexander Yashkin, 2016-11-22 17:33:22]

The CQ bit was checked by a-v-y@yandex-team.ru

[Alexander Yashkin, 2016-11-22 17:33:23]

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/2497853002/#ps180001
(title: "Fixed android compilation")

[commit-bot: I haz the power, 2016-11-22 17:33:40]

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-11-22 17:43:25]

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

[commit-bot: I haz the power, 2016-11-22 17:43:26]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Ian Wen, 2016-11-22 18:52:46]

lgtm

[Alexander Yashkin, 2016-11-22 19:41:14]

The CQ bit was checked by a-v-y@yandex-team.ru

[commit-bot: I haz the power, 2016-11-22 19:41:50]

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

[Peter Kasting, 2016-11-22 19:57:24]

Note: Please try to leave replies on my comments themselves rather than replying
via email or the "send message" box in code review, as it's much easier to
follow conversation threads later that way.

On 2016/11/22 06:29:27, Alexander Yashkin wrote:
> On 2016/11/21 at 20:12:36, pkasting wrote:
> > I would init the created time to the current time (as you noted, that seems
> safe) and leave the last modified time blank.  That should prevent this.
> 
> The more I think of this, the more I incline to leave both times null
> initialized for prepopulated_engines. 
> Whats the profit of knowing creation time for prpepopulated engine if we still
> ignore its value? 
> And how we could use it if it depends just on browser first run time, and does
> not change afterwards?

The benefit is that I'd like to implement one constructor in terms of the other
and not have their behaviors differ when there's no obvious reason for the
difference.  In the future someone is going to have this same conversation about
why these differ and whether they can be changed because we didn't resolve the
difference now.

Functionally, there is no difference between setting this to the real creation
time versus setting it to nothing, since what we use the creation time for is to
remove autogenerated engines, which wouldn't apply here.  So it's not critical
that we set it "correctly", but it's not wrong either, and it's nice not to
veary for the reason given above.

> > What if we simply changed the prepopulate data to use "" instead of NULL in
> these cases?  That seems just as readable and would avoid these extra
> conversions.
> 
> Its not that easy to change PrepopulatedEngine to use std::string and this way
> has its own drawbacks.

I didn't say to change it to std::string (which would violate the style guide). 
Leave it as char*, just use "" for empty strings instead of NULL.  "" is a valid
char*.

The thing we'd change to std::string is the constructor itself.

[commit-bot: I haz the power, 2016-11-22 20:09:53]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1479843674685220,
"parent_rev": "b06c995101189ad3aac0bf3f29fb3b477586d44d", "commit_rev":
"98979265b7ee89affa00f16dd15ecb5cb018e8a9"}

[commit-bot: I haz the power, 2016-11-22 20:10:24]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-11-22 20:11:59]

Description was changed from

==========
Create TemplateUrlData to base::Dictionary utility functions

This CL create utility functions that serialize TemplateURLData to and
from base::DictionaryValue. This is useful to store TemplateURLData
to preferences. Splitted from
https://codereview.chromium.org/2479113002/.

R=pkasting@chromium.org, stevet@chromium.org
==========

to

==========
Create TemplateUrlData to base::Dictionary utility functions

This CL create utility functions that serialize TemplateURLData to and
from base::DictionaryValue. This is useful to store TemplateURLData
to preferences. Splitted from
https://codereview.chromium.org/2479113002/.

R=pkasting@chromium.org, stevet@chromium.org

Committed: https://crrev.com/6028ec2fcc7ca0d036e903ff702de77f60a69071
Cr-Commit-Position: refs/heads/master@{#433955}
==========

[commit-bot: I haz the power, 2016-11-22 20:12:00]

Patchset 10 (id:??) landed as
https://crrev.com/6028ec2fcc7ca0d036e903ff702de77f60a69071
Cr-Commit-Position: refs/heads/master@{#433955}

[Alexander Yashkin, 2016-11-23 05:16:00]

On 2016/11/22 at 19:57:24, pkasting wrote:
> Note: Please try to leave replies on my comments themselves rather than
replying via email or the "send message" box in code review, as it's much easier
to follow conversation threads later that way.
> 
> On 2016/11/22 06:29:27, Alexander Yashkin wrote:
> > On 2016/11/21 at 20:12:36, pkasting wrote:
> > > I would init the created time to the current time (as you noted, that
seems
> > safe) and leave the last modified time blank.  That should prevent this.
> > 
> > The more I think of this, the more I incline to leave both times null
> > initialized for prepopulated_engines. 
> > Whats the profit of knowing creation time for prpepopulated engine if we
still
> > ignore its value? 
> > And how we could use it if it depends just on browser first run time, and
does
> > not change afterwards?
> 
> The benefit is that I'd like to implement one constructor in terms of the
other and not have their behaviors differ when there's no obvious reason for the
difference.  In the future someone is going to have this same conversation about
why these differ and whether they can be changed because we didn't resolve the
difference now.
> 
> Functionally, there is no difference between setting this to the real creation
time versus setting it to nothing, since what we use the creation time for is to
remove autogenerated engines, which wouldn't apply here.  So it's not critical
that we set it "correctly", but it's not wrong either, and it's nice not to
veary for the reason given above.

Already commited, when I noticed this comment. Agree, I''l try to fix it later
on.

> 
> > > What if we simply changed the prepopulate data to use "" instead of NULL
in
> > these cases?  That seems just as readable and would avoid these extra
> > conversions.
> > 
> > Its not that easy to change PrepopulatedEngine to use std::string and this
way
> > has its own drawbacks.
> 
> I didn't say to change it to std::string (which would violate the style
guide).  Leave it as char*, just use "" for empty strings instead of NULL.  ""
is a valid char*.
> 
> The thing we'd change to std::string is the constructor itself.

Do you mean to change autogeneration of structures in such way that they will
contain "" instead of NULL for undefined fields?
This generation is done in json_to_struct/element_generator.py. It could be
done, but effect of such change could be very wide. 
One need to track all usage of autogenerated structs in chromium to be sure all
places that use such structs do not compare for NULL explicitly.
Is it worth it if as result we can only use std::string instead of StringPiece
when initializing from such fields?

Thanks for review, your comments are always useful and thoughtful.

[Peter Kasting, 2016-11-23 05:53:59]

On 2016/11/23 05:16:00, Alexander Yashkin wrote:
> On 2016/11/22 at 19:57:24, pkasting wrote:
> > I didn't say to change it to std::string (which would violate the style
> guide).  Leave it as char*, just use "" for empty strings instead of NULL.  ""
> is a valid char*.
> > 
> > The thing we'd change to std::string is the constructor itself.
> 
> Do you mean to change autogeneration of structures in such way that they will
> contain "" instead of NULL for undefined fields?

Yes.

> This generation is done in json_to_struct/element_generator.py. It could be
> done, but effect of such change could be very wide.

Looks like only fieldtrial_testing_config.* and prepopulated_engines.* use this,
so hopefully it would be fairly easy to check the effects of such a change.

> Is it worth it if as result we can only use std::string instead of StringPiece
> when initializing from such fields?

In all cases, we'll be converting to a string later anyway, so when initializing
from these fields we'll save construction of a StringPiece, and when converting
from a string to call these, we'll save both the StringPiece and later string
constructions.  That seems worth doing to me.

[Alexander Yashkin, 2016-11-23 05:58:18]

On 2016/11/23 at 05:53:59, pkasting wrote:
> On 2016/11/23 05:16:00, Alexander Yashkin wrote:
> > On 2016/11/22 at 19:57:24, pkasting wrote:
> > > I didn't say to change it to std::string (which would violate the style
> > guide).  Leave it as char*, just use "" for empty strings instead of NULL. 
""
> > is a valid char*.
> > > 
> > > The thing we'd change to std::string is the constructor itself.
> > 
> > Do you mean to change autogeneration of structures in such way that they
will
> > contain "" instead of NULL for undefined fields?
> 
> Yes.
> 
> > This generation is done in json_to_struct/element_generator.py. It could be
> > done, but effect of such change could be very wide.
> 
> Looks like only fieldtrial_testing_config.* and prepopulated_engines.* use
this, so hopefully it would be fairly easy to check the effects of such a
change.
> 
> > Is it worth it if as result we can only use std::string instead of
StringPiece
> > when initializing from such fields?
> 
> In all cases, we'll be converting to a string later anyway, so when
initializing from these fields we'll save construction of a StringPiece, and
when converting from a string to call these, we'll save both the StringPiece and
later string constructions.  That seems worth doing to me.

Ok, I'll try to fix it.