Implemented 'Reset Search engines' feature.

New methods allow to
- delete prepopulated search engines from user prefs
- delete the default search engine from user prefs
- reset unmanaged and non-extension URLs to prepopulated list. 

New unit tests:
- TemplateURLPrepopulateDataTest.ClearProvidersFromPrefs
- TemplateURLServiceTest.ResetNonExtensionURLs
- TemplateURLServiceTest.ResetURLsWithManagedDefault

BUG=235037, 244291
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202869

[vasilii, 2013-05-21 17:38:30]

Hello Dominic and Scott,

Could you review the following:

Dominic: profile_resetter.cc [+ other stuff]
Scott: search_engines/*

[sky, 2013-05-21 17:46:05]

sky->pkasting

[battre, 2013-05-22 14:03:37]

https://codereview.chromium.org/15572002/diff/18001/chrome/browser/profile_re...
File chrome/browser/profile_resetter/profile_resetter_unittest.cc (left):

https://codereview.chromium.org/15572002/diff/18001/chrome/browser/profile_re...
chrome/browser/profile_resetter/profile_resetter_unittest.cc:24: resetter_(NULL)
{}
I think that we should actually go in the opposite direction here: Instead of
deleting the message loop we should make the whole thing truly asynchronous.
I.e. in any case, the callback should be triggered via a PostTask.

This is what you can do instead of relying on the MockObject:

content::BrowserThread::PostTask(
    content::BrowserThread::UI, FROM_HERE,
    base::Bind(&ProfileResetter::Reset,
               base::Unretained(&resetter),
               ProfileResetter::DEFAULT_SEARCH_ENGINE,
               ProfileResetter::DISABLE_EXTENSIONS,
               MessageLoop::QuitClosure()));
message_loop_.Run();

ProfileResetter::MarkAsDone should post the callback on the message loop.

Sorry, I should have pointed this out before.

https://codereview.chromium.org/15572002/diff/18001/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url_prepopulate_data.cc (right):

https://codereview.chromium.org/15572002/diff/18001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_prepopulate_data.cc:1210:
prefs->Set(prefs::kSearchProviderOverrides, empty_list);
Should we use prefs->ClearPref here and in the following line?

https://codereview.chromium.org/15572002/diff/18001/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url_prepopulate_data.h (right):

https://codereview.chromium.org/15572002/diff/18001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_prepopulate_data.h:57: // Removes
prepopulated engines stored in user prefs and overwrites its version.
what does "its" refer to?

https://codereview.chromium.org/15572002/diff/18001/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url_prepopulate_data_unittest.cc
(right):

https://codereview.chromium.org/15572002/diff/18001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_prepopulate_data_unittest.cc:203:
overrides->Append(entry->DeepCopy());
nit: Either entry.Pass() or allocate entry on the stack?

https://codereview.chromium.org/15572002/diff/18001/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/15572002/diff/18001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service.cc:2527: i !=
template_urls_.end(); ++i) {
nit: +1 space

https://codereview.chromium.org/15572002/diff/18001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service.cc:2535: i !=
entries_to_process.end(); ++i) {
nit: +1 space

https://codereview.chromium.org/15572002/diff/18001/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url_service.h (right):

https://codereview.chromium.org/15572002/diff/18001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service.h:597: TemplateURLVector*
template_urls,
pass as const-ref or indicate that this is an optional parameter that may be
NULL? (dito for the following paramter).

Please indicate whether you transfer the ownership of elements in
|template_urls|.

[vasilii, 2013-05-22 18:54:08]

Done, Dominic. 
Peter, please review search_engines/ part.

https://codereview.chromium.org/15572002/diff/18001/chrome/browser/profile_re...
File chrome/browser/profile_resetter/profile_resetter_unittest.cc (left):

https://codereview.chromium.org/15572002/diff/18001/chrome/browser/profile_re...
chrome/browser/profile_resetter/profile_resetter_unittest.cc:24: resetter_(NULL)
{}
On 2013/05/22 14:03:37, battre wrote:
> I think that we should actually go in the opposite direction here: Instead of
> deleting the message loop we should make the whole thing truly asynchronous.
> I.e. in any case, the callback should be triggered via a PostTask.
> 
> This is what you can do instead of relying on the MockObject:
> 
> content::BrowserThread::PostTask(
>     content::BrowserThread::UI, FROM_HERE,
>     base::Bind(&ProfileResetter::Reset,
>                base::Unretained(&resetter),
>                ProfileResetter::DEFAULT_SEARCH_ENGINE,
>                ProfileResetter::DISABLE_EXTENSIONS,
>                MessageLoop::QuitClosure()));
> message_loop_.Run();
> 
> ProfileResetter::MarkAsDone should post the callback on the message loop.
> 
> Sorry, I should have pointed this out before.

I modified the tests in another way.

https://codereview.chromium.org/15572002/diff/18001/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url_prepopulate_data.cc (right):

https://codereview.chromium.org/15572002/diff/18001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_prepopulate_data.cc:1210:
prefs->Set(prefs::kSearchProviderOverrides, empty_list);
On 2013/05/22 14:03:37, battre wrote:
> Should we use prefs->ClearPref here and in the following line? 

Done.

https://codereview.chromium.org/15572002/diff/18001/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url_prepopulate_data.h (right):

https://codereview.chromium.org/15572002/diff/18001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_prepopulate_data.h:57: // Removes
prepopulated engines stored in user prefs and overwrites its version.
On 2013/05/22 14:03:37, battre wrote:
> what does "its" refer to?

Done.

https://codereview.chromium.org/15572002/diff/18001/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url_prepopulate_data_unittest.cc
(right):

https://codereview.chromium.org/15572002/diff/18001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_prepopulate_data_unittest.cc:203:
overrides->Append(entry->DeepCopy());
On 2013/05/22 14:03:37, battre wrote:
> nit: Either entry.Pass() or allocate entry on the stack?

Done.

https://codereview.chromium.org/15572002/diff/18001/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/15572002/diff/18001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service.cc:2527: i !=
template_urls_.end(); ++i) {
On 2013/05/22 14:03:37, battre wrote:
> nit: +1 space

Done.

https://codereview.chromium.org/15572002/diff/18001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service.cc:2535: i !=
entries_to_process.end(); ++i) {
On 2013/05/22 14:03:37, battre wrote:
> nit: +1 space

Done.

https://codereview.chromium.org/15572002/diff/18001/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url_service.h (right):

https://codereview.chromium.org/15572002/diff/18001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service.h:597: TemplateURLVector*
template_urls,
On 2013/05/22 14:03:37, battre wrote:
> pass as const-ref or indicate that this is an optional parameter that may be
> NULL? (dito for the following paramter).
> 
> Please indicate whether you transfer the ownership of elements in
> |template_urls|.

The method consumes |template_urls| and modifies it internally.

[Peter Kasting, 2013-05-22 21:22:57]

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/profile_re...
File chrome/browser/profile_resetter/profile_resetter_unittest.cc (right):

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/profile_re...
chrome/browser/profile_resetter/profile_resetter_unittest.cc:39:
ProfileResetterTest() {
Nit: Don't inline constructors and destructors

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/profile_re...
chrome/browser/profile_resetter/profile_resetter_unittest.cc:42: //
testing::Test
Nit: Add trailing colon

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/profile_re...
chrome/browser/profile_resetter/profile_resetter_unittest.cc:46:
~ProfileResetterTest() {}
Nit: This goes right below the constructor

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url_prepopulate_data.cc (right):

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_prepopulate_data.cc:1207:
prefs->ClearPref(prefs::kSearchProviderOverridesVersion);
These prefs are supposed to only be set by tests and by the master prefs.  Are
you sure clearing them is correct?  You may want to ping joaodasilva about the
master prefs side of this.

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url_prepopulate_data_unittest.cc
(right):

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_prepopulate_data_unittest.cc:188:
switches::kCountry, "US");
Why is this needed?  It looks like the test ought to work for any locale.

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service.cc:1601: void
TemplateURLService::ClearDefaultProviderFromPrefs() {
Function definition order must match declaration order (several places)

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service.cc:1606: if (PrefService*
prefs = GetPrefs())
Nit: Do not use side-effect statements as conditions.  Do the assignment and
then the conditional.

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service.cc:2522: bool
TemplateURLService::ResetNonExtensionURLs() {
This function never returns false.  Why does it return a bool?

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service.cc:2525: TemplateURLVector
entries_to_process;
Nit: In general, this function could do with separating distinct functional
blocks with blank lines.

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service.cc:2528: if
(!(**i).IsExtensionKeyword())
Nit: (*i)->

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service.cc:2532:
default_search_provider_ = NULL;
If the DSP was managed, don't we need to preserve the TURL it pointed to, in
case that one isn't in the prepopulated list?  Or are we guaranteed the data on
that engine is recoverable from somewhere?

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service.cc:2533: // Remove
non-extension keywords
Nit: Trailing period

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service.cc:2535: i !=
entries_to_process.end(); ++i) {
Nit: No {}

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service.cc:2546: // This will force
to merge prepopulated engines.
Nit: Grammar (I don't actually know what this sentence is supopsed to mean so I
can't suggest a better one)

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service.cc:2558: // Make sure the
default search engine != NULL;
Nit: This comment adds nothing to the code.

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url_service.h (right):

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service.h:220: void
ClearDefaultProviderFromPrefs();
If these two methods get called one after the other, why aren't they just one
method?

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service.h:223: // Resets the default
engine unless it's managed.
Nit: Grammar.  How about:

Resets the search providers to the prepopulated engines plus any
extension-supplied engines.  Also resets the default search engine unless it's
managed.

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service.h:596: //
|default_search_provider| specifies URL inside |*template_urls|.
Nit: How about just:

Adds |template_urls| to |template_urls_| and sets up the default search
provider.  If |default_search_provider| is non-NULL, it must refer to one of the
|template_urls|, and will be used as the new default if there is no current
default.

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service.h:602: // if it's unmanaged
and NULL.
Nit: How about:

If there is no current default search provider, sets the default to the result
of calling FindNewDefaultSearchProvider().

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/search_eng...
File chrome/browser/search_engines/util.cc (right):

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/search_eng...
chrome/browser/search_engines/util.cc:152: // Loads engines from prepopulate
data and merges them in with the existing
Nit: While here, please fix this comment: "Merges the provided prepopulated
engines with the provided existing engines. ..."

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/search_eng...
File chrome/browser/search_engines/util.h (right):

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/search_eng...
chrome/browser/search_engines/util.h:67: // |*template_urls|.
This comment is confusing.  How about just this:

Like GetSearchProvidersUsingKeywordResult(), but allows the caller to pass in
engines in |template_urls| instead of getting them via processing a web data
service request.

[Joao da Silva, 2013-05-23 08:54:10]

Thanks for the heads up, see inline.

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url_prepopulate_data.cc (right):

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_prepopulate_data.cc:1207:
prefs->ClearPref(prefs::kSearchProviderOverridesVersion);
On 2013/05/22 21:22:57, Peter Kasting wrote:
> These prefs are supposed to only be set by tests and by the master prefs.  Are
> you sure clearing them is correct?  You may want to ping joaodasilva about the
> master prefs side of this.

As Peter mentioned, these prefs can only come from the master_preferences. The
master_preferences are sort-of a template of the user Preferences file, that is
imported only once during the first run. It is used by administrators to
preconfigure settings, including the list of search providers.

So these prefs should *not* be cleared when the user cleans up his profile. IMO
the right thing to do is to clean up the search providers database, and then
populate it again as if the profile is new; that should import the
"search_provider_overrides" again, if they exist in that Profile's preferences;
otherwise it should import the default initial providers.

Peter, does this make sense to you too? :-)

[Joao da Silva, 2013-05-23 09:12:50]

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url_prepopulate_data.cc (right):

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_prepopulate_data.cc:1207:
prefs->ClearPref(prefs::kSearchProviderOverridesVersion);
On 2013/05/23 08:54:10, Joao da Silva wrote:
> On 2013/05/22 21:22:57, Peter Kasting wrote:
> > These prefs are supposed to only be set by tests and by the master prefs. 
Are
> > you sure clearing them is correct?  You may want to ping joaodasilva about
the
> > master prefs side of this.
> 
> As Peter mentioned, these prefs can only come from the master_preferences. The
> master_preferences are sort-of a template of the user Preferences file, that
is
> imported only once during the first run. It is used by administrators to
> preconfigure settings, including the list of search providers.
> 
> So these prefs should *not* be cleared when the user cleans up his profile.
IMO
> the right thing to do is to clean up the search providers database, and then
> populate it again as if the profile is new; that should import the
> "search_provider_overrides" again, if they exist in that Profile's
preferences;
> otherwise it should import the default initial providers.
> 
> Peter, does this make sense to you too? :-)

I wasn't looking at this from the malware prevention POV. After discussing with
Dominic I now think it's fine to clean it up.

Whatever is imported from master_preferences can easily be modified by the user,
so there is no strong enforcement of those settings like we do for policies. And
malware can modify the master_preferences file just as easily as the
preferences; so I'm fine with cleaning this up.

On the enterprise side, we may come to deprecate master_preferences if they
become an attack vector. We can add a few new policies to do the same that the
master_prefs are usually used for.

[battre, 2013-05-23 16:19:14]

LGTM @ chrome/browser/profile_resetter/*

[vasilii, 2013-05-23 17:03:41]

I rely in my code on TemplateURLService has loaded. But I see that Search
Engines dialog triggers the loading and refreshes once TemplateURLService has
loaded. Peter, does it mean that it's possible for a user to open
chrome://settings before TemplateURLService is ready?

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/profile_re...
File chrome/browser/profile_resetter/profile_resetter_unittest.cc (right):

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/profile_re...
chrome/browser/profile_resetter/profile_resetter_unittest.cc:39:
ProfileResetterTest() {
On 2013/05/22 21:22:57, Peter Kasting wrote:
> Nit: Don't inline constructors and destructors

Done.

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/profile_re...
chrome/browser/profile_resetter/profile_resetter_unittest.cc:42: //
testing::Test
On 2013/05/22 21:22:57, Peter Kasting wrote:
> Nit: Add trailing colon

Done.

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/profile_re...
chrome/browser/profile_resetter/profile_resetter_unittest.cc:46:
~ProfileResetterTest() {}
On 2013/05/22 21:22:57, Peter Kasting wrote:
> Nit: This goes right below the constructor

Done.

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url_prepopulate_data.cc (right):

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_prepopulate_data.cc:1207:
prefs->ClearPref(prefs::kSearchProviderOverridesVersion);
On 2013/05/22 21:22:57, Peter Kasting wrote:
> These prefs are supposed to only be set by tests and by the master prefs.  Are
> you sure clearing them is correct?  You may want to ping joaodasilva about the
> master prefs side of this.

The purpose of this feature is to recover Chrome after some malware's spoiled
user settings. Therefore we definitely want to reset them for current profile.

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url_prepopulate_data_unittest.cc
(right):

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_prepopulate_data_unittest.cc:188:
switches::kCountry, "US");
On 2013/05/22 21:22:57, Peter Kasting wrote:
> Why is this needed?  It looks like the test ought to work for any locale.

Done. I copied that from the next test.

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service.cc:1601: void
TemplateURLService::ClearDefaultProviderFromPrefs() {
On 2013/05/22 21:22:57, Peter Kasting wrote:
> Function definition order must match declaration order (several places)

Done.

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service.cc:1606: if (PrefService*
prefs = GetPrefs())
On 2013/05/22 21:22:57, Peter Kasting wrote:
> Nit: Do not use side-effect statements as conditions.  Do the assignment and
> then the conditional.

Done.

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service.cc:2522: bool
TemplateURLService::ResetNonExtensionURLs() {
On 2013/05/22 21:22:57, Peter Kasting wrote:
> This function never returns false.  Why does it return a bool?

Done.

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service.cc:2525: TemplateURLVector
entries_to_process;
On 2013/05/22 21:22:57, Peter Kasting wrote:
> Nit: In general, this function could do with separating distinct functional
> blocks with blank lines.

Done.

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service.cc:2528: if
(!(**i).IsExtensionKeyword())
On 2013/05/22 21:22:57, Peter Kasting wrote:
> Nit: (*i)->

Done.

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service.cc:2532:
default_search_provider_ = NULL;
On 2013/05/22 21:22:57, Peter Kasting wrote:
> If the DSP was managed, don't we need to preserve the TURL it pointed to, in
> case that one isn't in the prepopulated list?  Or are we guaranteed the data
on
> that engine is recoverable from somewhere?

Yes. It's stored in managed prefs. AddTemplateURLsAndSetupDefaultEngine loads,
adds and set it as default. See lines 2471, 2497, 2505. This case is covered by
TemplateURLServiceTest.ResetURLsWithManagedDefault.

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service.cc:2533: // Remove
non-extension keywords
On 2013/05/22 21:22:57, Peter Kasting wrote:
> Nit: Trailing period

Done.

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service.cc:2535: i !=
entries_to_process.end(); ++i) {
On 2013/05/22 21:22:57, Peter Kasting wrote:
> Nit: No {}

Done.

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service.cc:2546: // This will force
to merge prepopulated engines.
On 2013/05/22 21:22:57, Peter Kasting wrote:
> Nit: Grammar (I don't actually know what this sentence is supopsed to mean so
I
> can't suggest a better one)

Done.

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service.cc:2558: // Make sure the
default search engine != NULL;
On 2013/05/22 21:22:57, Peter Kasting wrote:
> Nit: This comment adds nothing to the code.

Done.

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url_service.h (right):

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service.h:220: void
ClearDefaultProviderFromPrefs();
On 2013/05/22 21:22:57, Peter Kasting wrote:
> If these two methods get called one after the other, why aren't they just one
> method?

Done.

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service.h:223: // Resets the default
engine unless it's managed.
On 2013/05/22 21:22:57, Peter Kasting wrote:
> Nit: Grammar.  How about:
> 
> Resets the search providers to the prepopulated engines plus any
> extension-supplied engines.  Also resets the default search engine unless it's
> managed.

Done.

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service.h:596: //
|default_search_provider| specifies URL inside |*template_urls|.
On 2013/05/22 21:22:57, Peter Kasting wrote:
> Nit: How about just:
> 
> Adds |template_urls| to |template_urls_| and sets up the default search
> provider.  If |default_search_provider| is non-NULL, it must refer to one of
the
> |template_urls|, and will be used as the new default if there is no current
> default.

Done. Actually we set the default even if it's not NULL. But in both our
use-cases it's NULL.

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service.h:602: // if it's unmanaged
and NULL.
On 2013/05/22 21:22:57, Peter Kasting wrote:
> Nit: How about:
> 
> If there is no current default search provider, sets the default to the result
> of calling FindNewDefaultSearchProvider().

Done.

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/search_eng...
File chrome/browser/search_engines/util.cc (right):

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/search_eng...
chrome/browser/search_engines/util.cc:152: // Loads engines from prepopulate
data and merges them in with the existing
On 2013/05/22 21:22:57, Peter Kasting wrote:
> Nit: While here, please fix this comment: "Merges the provided prepopulated
> engines with the provided existing engines. ..."

Done.

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/search_eng...
File chrome/browser/search_engines/util.h (right):

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/search_eng...
chrome/browser/search_engines/util.h:67: // |*template_urls|.
On 2013/05/22 21:22:57, Peter Kasting wrote:
> This comment is confusing.  How about just this:
> 
> Like GetSearchProvidersUsingKeywordResult(), but allows the caller to pass in
> engines in |template_urls| instead of getting them via processing a web data
> service request.

Done.

[Peter Kasting, 2013-05-23 19:32:51]

On 2013/05/23 09:12:50, Joao da Silva wrote:
> I wasn't looking at this from the malware prevention POV. After discussing
with
> Dominic I now think it's fine to clean it up.
> 
> Whatever is imported from master_preferences can easily be modified by the
user,
> so there is no strong enforcement of those settings like we do for policies.
And
> malware can modify the master_preferences file just as easily as the
> preferences; so I'm fine with cleaning this up.

There's a distinction between the local prefs (which may have been set based on
the master prefs) and the master prefs themselves, right?

I'm OK clearing the local prefs but then at that point I think we should
re-import the master prefs.  Any malware which is mucking with the master prefs
is also probably savvy enough to re-inject itself into the user's profile
anyway.

[Peter Kasting, 2013-05-23 19:33:11]

On 2013/05/23 17:03:41, vasilii wrote:
> I rely in my code on TemplateURLService has loaded. But I see that Search
> Engines dialog triggers the loading and refreshes once TemplateURLService has
> loaded. Peter, does it mean that it's possible for a user to open
> chrome://settings before TemplateURLService is ready?

Probably.

[vasilii, 2013-05-24 14:04:53]

We assume that user has uninstalled malware. Otherwise, it can tamper with
settings again immediately.
Master prefs is just a file. Therefore it's probably damaged as well as profile
prefs.

[Peter Kasting, 2013-05-24 18:29:55]

On 2013/05/24 14:04:53, vasilii wrote:
> Master prefs is just a file. Therefore it's probably damaged as well as
profile
> prefs.

I don't think I agree.

The common use case for this is for people to reset their own changes (e.g.
deleting Google), not those of malware.

Even for malware, I doubt most malware today tries to muck with the master
prefs.

Not including the master prefs basically means the reset button will always do
the wrong thing for anyone who really has master prefs, because that indicates
that the administrator was trying to set the profile's default state to
something that won't be reflected in our reset.  I don't think we can do worse
than this by assuming the master prefs are valid.

[Peter Kasting, 2013-05-27 22:07:40]

I just filed bug 244291 for another facet that should be considered for this
change or a subsequent one.

[vasilii, 2013-05-28 14:30:35]

On 2013/05/27 22:07:40, Peter Kasting wrote:
> I just filed bug 244291 for another facet that should be considered for this
> change or a subsequent one.

Implemented.

[Peter Kasting, 2013-05-29 04:04:41]

LGTM

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/15572002/diff/31001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service.cc:2532:
default_search_provider_ = NULL;
On 2013/05/23 17:03:42, vasilii wrote:
> On 2013/05/22 21:22:57, Peter Kasting wrote:
> > If the DSP was managed, don't we need to preserve the TURL it pointed to, in
> > case that one isn't in the prepopulated list?  Or are we guaranteed the data
> on
> > that engine is recoverable from somewhere?
> 
> Yes. It's stored in managed prefs. AddTemplateURLsAndSetupDefaultEngine loads,
> adds and set it as default. See lines 2471, 2497, 2505. This case is covered
by
> TemplateURLServiceTest.ResetURLsWithManagedDefault.

OK, good.  Consider adding a comment about this here for the benefit of future
readers of the code.

https://codereview.chromium.org/15572002/diff/71001/chrome/browser/profile_re...
File chrome/browser/profile_resetter/profile_resetter.h (right):

https://codereview.chromium.org/15572002/diff/71001/chrome/browser/profile_re...
chrome/browser/profile_resetter/profile_resetter.h:73: //
content::NotificationObserver implementation.
Nit: Just "// BaseClassName:"

https://codereview.chromium.org/15572002/diff/71001/chrome/browser/profile_re...
File chrome/browser/profile_resetter/profile_resetter_unittest.cc (right):

https://codereview.chromium.org/15572002/diff/71001/chrome/browser/profile_re...
chrome/browser/profile_resetter/profile_resetter_unittest.cc:42: //
testing::Test:
Nit: Add blank line above

https://codereview.chromium.org/15572002/diff/71001/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/15572002/diff/71001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service.cc:701: // Prepopulated
engines version isn't 0. Thus they will be merged in for sure.
Nit: Ah, now I see what you're trying to say.  How about "Force
GetSearchProvidersUsingLoadedEngines() to include the prepopulated engines in
the list by claiming we are currently on version 0, ensuring that the
prepopulate data version will be newer."

https://codereview.chromium.org/15572002/diff/71001/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url_service_unittest.cc (right):

https://codereview.chromium.org/15572002/diff/71001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service_unittest.cc:994: // The
following call has no effect on managed search engine.
Nit: on -> on the?  on any?

https://codereview.chromium.org/15572002/diff/71001/chrome/browser/search_eng...
File chrome/browser/search_engines/util.cc (right):

https://codereview.chromium.org/15572002/diff/71001/chrome/browser/search_eng...
chrome/browser/search_engines/util.cc:320: if (*new_resource_keyword_version !=
resource_keyword_version) {
Nit: Really, this should use < instead of !=.  This prevents strange things from
happening if the user attempts to downgrade versions (which we don't really
support, but regardless...).

https://codereview.chromium.org/15572002/diff/71001/chrome/browser/search_eng...
File chrome/browser/search_engines/util.h (right):

https://codereview.chromium.org/15572002/diff/71001/chrome/browser/search_eng...
chrome/browser/search_engines/util.h:57: // |*template_urls|.
Nit: How about we name it |resource_keyword_version| and have the comment here
say:

|resource_keyword_version| should contain the version number of the current
keyword data, i.e. the version number of the most recent prepopulate data that
has been merged into the current keyword data.  On exit, this will be set as in
GetSearchProvidersUsingKeywordResult().

[vasilii, 2013-05-29 08:30:18]

Thanks

https://codereview.chromium.org/15572002/diff/71001/chrome/browser/profile_re...
File chrome/browser/profile_resetter/profile_resetter.h (right):

https://codereview.chromium.org/15572002/diff/71001/chrome/browser/profile_re...
chrome/browser/profile_resetter/profile_resetter.h:73: //
content::NotificationObserver implementation.
On 2013/05/29 04:04:41, Peter Kasting wrote:
> Nit: Just "// BaseClassName:"

Done.

https://codereview.chromium.org/15572002/diff/71001/chrome/browser/profile_re...
File chrome/browser/profile_resetter/profile_resetter_unittest.cc (right):

https://codereview.chromium.org/15572002/diff/71001/chrome/browser/profile_re...
chrome/browser/profile_resetter/profile_resetter_unittest.cc:42: //
testing::Test:
On 2013/05/29 04:04:41, Peter Kasting wrote:
> Nit: Add blank line above

Done.

https://codereview.chromium.org/15572002/diff/71001/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/15572002/diff/71001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service.cc:701: // Prepopulated
engines version isn't 0. Thus they will be merged in for sure.
On 2013/05/29 04:04:41, Peter Kasting wrote:
> Nit: Ah, now I see what you're trying to say.  How about "Force
> GetSearchProvidersUsingLoadedEngines() to include the prepopulated engines in
> the list by claiming we are currently on version 0, ensuring that the
> prepopulate data version will be newer."

Done.

https://codereview.chromium.org/15572002/diff/71001/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url_service_unittest.cc (right):

https://codereview.chromium.org/15572002/diff/71001/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_service_unittest.cc:994: // The
following call has no effect on managed search engine.
On 2013/05/29 04:04:41, Peter Kasting wrote:
> Nit: on -> on the?  on any?

Done.

https://codereview.chromium.org/15572002/diff/71001/chrome/browser/search_eng...
File chrome/browser/search_engines/util.cc (right):

https://codereview.chromium.org/15572002/diff/71001/chrome/browser/search_eng...
chrome/browser/search_engines/util.cc:320: if (*new_resource_keyword_version !=
resource_keyword_version) {
On 2013/05/29 04:04:41, Peter Kasting wrote:
> Nit: Really, this should use < instead of !=.  This prevents strange things
from
> happening if the user attempts to downgrade versions (which we don't really
> support, but regardless...).

Done.

https://codereview.chromium.org/15572002/diff/71001/chrome/browser/search_eng...
File chrome/browser/search_engines/util.h (right):

https://codereview.chromium.org/15572002/diff/71001/chrome/browser/search_eng...
chrome/browser/search_engines/util.h:57: // |*template_urls|.
On 2013/05/29 04:04:41, Peter Kasting wrote:
> Nit: How about we name it |resource_keyword_version| and have the comment here
> say:
> 
> |resource_keyword_version| should contain the version number of the current
> keyword data, i.e. the version number of the most recent prepopulate data that
> has been merged into the current keyword data.  On exit, this will be set as
in
> GetSearchProvidersUsingKeywordResult().

Done.

[commit-bot: I haz the power, 2013-05-29 08:30:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vasilii@chromium.org/15572002/87001

[commit-bot: I haz the power, 2013-05-29 13:35:47]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is android_dbg, revision is HEAD

[commit-bot: I haz the power, 2013-05-29 13:48:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vasilii@chromium.org/15572002/87001

[commit-bot: I haz the power, 2013-05-29 14:44:57]

Change committed as 202869