Added Protector, hooked up DSE verification with error bubble.

Added Protector, hooked up DSE verification with error bubble.
Moved signing key to a separate cc file.

R=ivankr@chromium.org,sky@chromium.org
BUG=94447
TEST=Before launching Chrome modify Default Search Engine ID value in Web Data SQL db. Check that error bubble is shown on Chrome startup and its buttons work as expected.

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

[Ivan Korotkov, 2011-10-19 20:24:37]

http://codereview.chromium.org/8342049/diff/1/chrome/browser/protector/protec...
File chrome/browser/protector/protector.cc (right):

http://codereview.chromium.org/8342049/diff/1/chrome/browser/protector/protec...
chrome/browser/protector/protector.cc:22:
DefaultSearchProviderChange::DefaultSearchProviderChange(
Please move to a separate .cc file.

http://codereview.chromium.org/8342049/diff/1/chrome/browser/protector/protec...
chrome/browser/protector/protector.cc:80: void
Protector::AddChange(scoped_refptr<SettingChange> change) {
I don't think there is any need to store refptr's to changes -- let the
Protector instance take their ownership, store raw pointers and just
STLDeleteElements in ~

http://codereview.chromium.org/8342049/diff/1/chrome/browser/protector/protec...
chrome/browser/protector/protector.cc:89: if (changes_.empty())
DCHECK?

http://codereview.chromium.org/8342049/diff/1/chrome/browser/protector/protec...
chrome/browser/protector/protector.cc:130: protector->AddChange(change);
Please explicitly AddRef here and Release when we don't need Protector anymore
-- in Accept/DeclineChanges, for example.
Otherwise it's pretty unclear and up to the SettingChangeGlobalError's
implementation when the callbacks with their refs to Protector are destroyed.

http://codereview.chromium.org/8342049/diff/1/chrome/browser/protector/protec...
File chrome/browser/protector/protector.h (right):

http://codereview.chromium.org/8342049/diff/1/chrome/browser/protector/protec...
chrome/browser/protector/protector.h:27: class SettingChange : public
base::RefCountedThreadSafe<SettingChange> {
Please split all these classes into separate files.

http://codereview.chromium.org/8342049/diff/1/chrome/browser/protector/protec...
chrome/browser/protector/protector.h:67: class DefaultSearchProviderChange :
public SettingChange {
This is a pure implementation, so it can be declared in a .cc file with
something like 'SettingChange* CreateDefaultSearchProviderChange(...)' left in
this file.

http://codereview.chromium.org/8342049/diff/1/chrome/browser/protector/protec...
chrome/browser/protector/protector.h:137: void
NotifyUserOfChange(scoped_refptr<SettingChange> change);
I'd make this a static method of Protector.

http://codereview.chromium.org/8342049/diff/1/chrome/browser/protector/protec...
chrome/browser/protector/protector.h:140: std::string Sign(const std::string&
value);
Maybe give it a more descriptive name? Like 'SignSetting'.

http://codereview.chromium.org/8342049/diff/1/chrome/browser/protector/settin...
File chrome/browser/protector/settings_change_global_error.cc (right):

http://codereview.chromium.org/8342049/diff/1/chrome/browser/protector/settin...
chrome/browser/protector/settings_change_global_error.cc:73:
closed_by_button_(false),
browser_(NULL) after profile_

http://codereview.chromium.org/8342049/diff/1/chrome/browser/protector/settin...
File chrome/browser/protector/settings_change_global_error.h (right):

http://codereview.chromium.org/8342049/diff/1/chrome/browser/protector/settin...
chrome/browser/protector/settings_change_global_error.h:40: Browser* browser()
const { return browser_; }
Document these please.

http://codereview.chromium.org/8342049/diff/1/chrome/browser/webdata/keyword_...
File chrome/browser/webdata/keyword_table.cc (right):

http://codereview.chromium.org/8342049/diff/1/chrome/browser/webdata/keyword_...
chrome/browser/webdata/keyword_table.cc:14: #include
"chrome/browser/protector/keys.h"
Sort please.

[sky, 2011-10-19 21:09:12]

http://codereview.chromium.org/8342049/diff/3001/chrome/browser/webdata/keywo...
File chrome/browser/webdata/keyword_table.cc (right):

http://codereview.chromium.org/8342049/diff/3001/chrome/browser/webdata/keywo...
chrome/browser/webdata/keyword_table.cc:281:
scoped_refptr<protector::DefaultSearchProviderChange> change(
I don't like putting functionality like this in the DB or running it on this
thread. The DB should be a stateless as possible. Instead have this state
checked from WebDataServices call to Init and pass that back to the UI thread
for processing.

[whywhat, 2011-10-20 20:50:04]

http://codereview.chromium.org/8342049/diff/1/chrome/browser/protector/protec...
File chrome/browser/protector/protector.cc (right):

http://codereview.chromium.org/8342049/diff/1/chrome/browser/protector/protec...
chrome/browser/protector/protector.cc:22:
DefaultSearchProviderChange::DefaultSearchProviderChange(
On 2011/10/19 20:24:37, Ivan Korotkov wrote:
> Please move to a separate .cc file.

Done.

http://codereview.chromium.org/8342049/diff/1/chrome/browser/protector/protec...
chrome/browser/protector/protector.cc:80: void
Protector::AddChange(scoped_refptr<SettingChange> change) {
On 2011/10/19 20:24:37, Ivan Korotkov wrote:
> I don't think there is any need to store refptr's to changes -- let the
> Protector instance take their ownership, store raw pointers and just
> STLDeleteElements in ~

Done.

http://codereview.chromium.org/8342049/diff/1/chrome/browser/protector/protec...
chrome/browser/protector/protector.cc:89: if (changes_.empty())
On 2011/10/19 20:24:37, Ivan Korotkov wrote:
> DCHECK?

Done.

http://codereview.chromium.org/8342049/diff/1/chrome/browser/protector/protec...
chrome/browser/protector/protector.cc:130: protector->AddChange(change);
Discussed offline how to better manage Protector's lifetime

On 2011/10/19 20:24:37, Ivan Korotkov wrote:
> Please explicitly AddRef here and Release when we don't need Protector anymore
> -- in Accept/DeclineChanges, for example.
> Otherwise it's pretty unclear and up to the SettingChangeGlobalError's
> implementation when the callbacks with their refs to Protector are destroyed.

http://codereview.chromium.org/8342049/diff/1/chrome/browser/protector/protec...
File chrome/browser/protector/protector.h (right):

http://codereview.chromium.org/8342049/diff/1/chrome/browser/protector/protec...
chrome/browser/protector/protector.h:27: class SettingChange : public
base::RefCountedThreadSafe<SettingChange> {
On 2011/10/19 20:24:37, Ivan Korotkov wrote:
> Please split all these classes into separate files.

Done.

http://codereview.chromium.org/8342049/diff/1/chrome/browser/protector/protec...
chrome/browser/protector/protector.h:67: class DefaultSearchProviderChange :
public SettingChange {
On 2011/10/19 20:24:37, Ivan Korotkov wrote:
> This is a pure implementation, so it can be declared in a .cc file with
> something like 'SettingChange* CreateDefaultSearchProviderChange(...)' left in
> this file.

Done.

http://codereview.chromium.org/8342049/diff/1/chrome/browser/protector/protec...
chrome/browser/protector/protector.h:137: void
NotifyUserOfChange(scoped_refptr<SettingChange> change);
On 2011/10/19 20:24:37, Ivan Korotkov wrote:
> I'd make this a static method of Protector.

It would be protector::Protector::NotifyUserOfChange which is too long to my
taste. Left as it is.

http://codereview.chromium.org/8342049/diff/1/chrome/browser/protector/protec...
chrome/browser/protector/protector.h:140: std::string Sign(const std::string&
value);
On 2011/10/19 20:24:37, Ivan Korotkov wrote:
> Maybe give it a more descriptive name? Like 'SignSetting'.

Done.

http://codereview.chromium.org/8342049/diff/1/chrome/browser/protector/settin...
File chrome/browser/protector/settings_change_global_error.cc (right):

http://codereview.chromium.org/8342049/diff/1/chrome/browser/protector/settin...
chrome/browser/protector/settings_change_global_error.cc:73:
closed_by_button_(false),
On 2011/10/19 20:24:37, Ivan Korotkov wrote:
> browser_(NULL) after profile_

Done.

http://codereview.chromium.org/8342049/diff/1/chrome/browser/protector/settin...
File chrome/browser/protector/settings_change_global_error.h (right):

http://codereview.chromium.org/8342049/diff/1/chrome/browser/protector/settin...
chrome/browser/protector/settings_change_global_error.h:40: Browser* browser()
const { return browser_; }
On 2011/10/19 20:24:37, Ivan Korotkov wrote:
> Document these please.

Done.

http://codereview.chromium.org/8342049/diff/1/chrome/browser/webdata/keyword_...
File chrome/browser/webdata/keyword_table.cc (right):

http://codereview.chromium.org/8342049/diff/1/chrome/browser/webdata/keyword_...
chrome/browser/webdata/keyword_table.cc:14: #include
"chrome/browser/protector/keys.h"
On 2011/10/19 20:24:37, Ivan Korotkov wrote:
> Sort please.

Done.

[Ivan Korotkov, 2011-10-20 21:04:52]

LGTM after sky's comment is resolved.

http://codereview.chromium.org/8342049/diff/8001/chrome/browser/protector/pro...
File chrome/browser/protector/protector.cc (right):

http://codereview.chromium.org/8342049/diff/8001/chrome/browser/protector/pro...
chrome/browser/protector/protector.cc:69: OnDiscardChanges();
This means the settings page will be opened automagically after 10 minutes if
user ignores the menu item, doesn't it?

http://codereview.chromium.org/8342049/diff/8001/chrome/browser/protector/set...
File chrome/browser/protector/settings_change_global_error.h (right):

http://codereview.chromium.org/8342049/diff/8001/chrome/browser/protector/set...
chrome/browser/protector/settings_change_global_error.h:52: // Browser that we
have been shown for.
"... that the bubble has been last time shown for"

[whywhat, 2011-10-20 23:02:39]

Scott, there're a couple of questions to you in the comments. Could you answer
them, please?

http://codereview.chromium.org/8342049/diff/3001/chrome/browser/webdata/keywo...
File chrome/browser/webdata/keyword_table.cc (right):

http://codereview.chromium.org/8342049/diff/3001/chrome/browser/webdata/keywo...
chrome/browser/webdata/keyword_table.cc:281:
scoped_refptr<protector::DefaultSearchProviderChange> change(
On 2011/10/19 21:09:12, sky wrote:
> I don't like putting functionality like this in the DB or running it on this
> thread. The DB should be a stateless as possible. Instead have this state
> checked from WebDataServices call to Init and pass that back to the UI thread
> for processing.

Done.

http://codereview.chromium.org/8342049/diff/8001/chrome/browser/protector/pro...
File chrome/browser/protector/protector.cc (right):

http://codereview.chromium.org/8342049/diff/8001/chrome/browser/protector/pro...
chrome/browser/protector/protector.cc:69: OnDiscardChanges();
On 2011/10/20 21:04:52, Ivan Korotkov wrote:
> This means the settings page will be opened automagically after 10 minutes if
> user ignores the menu item, doesn't it?
Right, fixed.

http://codereview.chromium.org/8342049/diff/8001/chrome/browser/protector/set...
File chrome/browser/protector/settings_change_global_error.h (right):

http://codereview.chromium.org/8342049/diff/8001/chrome/browser/protector/set...
chrome/browser/protector/settings_change_global_error.h:52: // Browser that we
have been shown for.
On 2011/10/20 21:04:52, Ivan Korotkov wrote:
> "... that the bubble has been last time shown for"

Done.

http://codereview.chromium.org/8342049/diff/13001/chrome/browser/protector/de...
File chrome/browser/protector/default_search_provider_change.cc (right):

http://codereview.chromium.org/8342049/diff/13001/chrome/browser/protector/de...
chrome/browser/protector/default_search_provider_change.cc:82: LOG(WARNING) <<
"Didn't find search provider with id " << new_id_;
After moving the check to WebDataService, TemplateURLService is empty at this
point and we can't set DSE to the new value...

http://codereview.chromium.org/8342049/diff/13001/chrome/browser/webdata/web_...
File chrome/browser/webdata/web_data_service.cc (right):

http://codereview.chromium.org/8342049/diff/13001/chrome/browser/webdata/web_...
chrome/browser/webdata/web_data_service.cc:638:
keyword_table->GetDefaultSearchProviderIDBackup());
Scott, when the ID is set to 0, there's no default search engine and
TemplateURLService doesn't find a new one. Do you know how to reset default
search engine properly here?

[sky, 2011-10-20 23:40:59]

Sorry, I didn't see your comments.

http://codereview.chromium.org/8342049/diff/13001/chrome/browser/webdata/web_...
File chrome/browser/webdata/web_data_service.cc (right):

http://codereview.chromium.org/8342049/diff/13001/chrome/browser/webdata/web_...
chrome/browser/webdata/web_data_service.cc:638:
keyword_table->GetDefaultSearchProviderIDBackup());
On 2011/10/20 23:02:39, whywhat wrote:
> Scott, when the ID is set to 0, there's no default search engine and
> TemplateURLService doesn't find a new one. Do you know how to reset default
> search engine properly here? 

Don't expose the keyword table. Doing so breaks the encapsulation of
WebdataService and causes the problems you're running into.

Change NotifyDatabaseLoadedOnUIThread to take whatever you need to tell the
Protector.

If you need to go back to the default do it by way of TemplateURLService.
That'll make sure everything is reset correctly in the db (and other places).

[sky, 2011-10-20 23:57:31]

http://codereview.chromium.org/8342049/diff/13001/chrome/browser/webdata/keyw...
File chrome/browser/webdata/keyword_table.h (right):

http://codereview.chromium.org/8342049/diff/13001/chrome/browser/webdata/keyw...
chrome/browser/webdata/keyword_table.h:83: string16 GetKeywordShortName(int64
id);
Don't add this either. Get all the TemplateURLs and look for the one you care
about.

[whywhat, 2011-10-21 16:48:18]

http://codereview.chromium.org/8342049/diff/13001/chrome/browser/webdata/keyw...
File chrome/browser/webdata/keyword_table.h (right):

http://codereview.chromium.org/8342049/diff/13001/chrome/browser/webdata/keyw...
chrome/browser/webdata/keyword_table.h:83: string16 GetKeywordShortName(int64
id);
On 2011/10/20 23:57:31, sky wrote:
> Don't add this either. Get all the TemplateURLs and look for the one you care
> about.

Done.

http://codereview.chromium.org/8342049/diff/13001/chrome/browser/webdata/web_...
File chrome/browser/webdata/web_data_service.cc (right):

http://codereview.chromium.org/8342049/diff/13001/chrome/browser/webdata/web_...
chrome/browser/webdata/web_data_service.cc:638:
keyword_table->GetDefaultSearchProviderIDBackup());
On 2011/10/20 23:40:59, sky wrote:
> On 2011/10/20 23:02:39, whywhat wrote:
> > Scott, when the ID is set to 0, there's no default search engine and
> > TemplateURLService doesn't find a new one. Do you know how to reset default
> > search engine properly here? 
> 
> Don't expose the keyword table. Doing so breaks the encapsulation of
> WebdataService and causes the problems you're running into.
> 
> Change NotifyDatabaseLoadedOnUIThread to take whatever you need to tell the
> Protector.
> 
> If you need to go back to the default do it by way of TemplateURLService.
> That'll make sure everything is reset correctly in the db (and other places).

Done.

[sky, 2011-10-21 17:20:30]

http://codereview.chromium.org/8342049/diff/15001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.h (right):

http://codereview.chromium.org/8342049/diff/15001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.h:298: scoped_ptr<protector::Protector>
protector_;
You'll need to run this by Elliott. We're moving things off Profile.

http://codereview.chromium.org/8342049/diff/15001/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_service.h (right):

http://codereview.chromium.org/8342049/diff/15001/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.h:558: const TemplateURL*
backup_default_search_provider_;
I don't like persisting this and is_default_search_provider_verified. Can't
TemplateURLService::OnWebDataServiceRequestDone call off to the protector and
let the protected do whatever it wants.

http://codereview.chromium.org/8342049/diff/15001/chrome/browser/search_engin...
File chrome/browser/search_engines/util.cc (right):

http://codereview.chromium.org/8342049/diff/15001/chrome/browser/search_engin...
chrome/browser/search_engines/util.cc:216: DCHECK(result.GetType() ==
KEYWORDS_RESULT);
DCHECK_EQ

http://codereview.chromium.org/8342049/diff/15001/chrome/browser/search_engin...
chrome/browser/search_engines/util.cc:218: WDKeywordsResult keyword_result =
reinterpret_cast<
This should take a WDKeywordsResult.

http://codereview.chromium.org/8342049/diff/15001/chrome/browser/webdata/keyw...
File chrome/browser/webdata/keyword_table.h (right):

http://codereview.chromium.org/8342049/diff/15001/chrome/browser/webdata/keyw...
chrome/browser/webdata/keyword_table.h:90: bool
IsDefaultSearchProviderIDVerified();
This name is a bit confusing. How about: DidDefaultSearchProviderChange() with a
description something like: Returns true if the default search provider changed
out from under us. This can happen if another process modifies our database (or
we get corruption).

http://codereview.chromium.org/8342049/diff/15001/chrome/browser/webdata/web_...
File chrome/browser/webdata/web_data_service.h (right):

http://codereview.chromium.org/8342049/diff/15001/chrome/browser/webdata/web_...
chrome/browser/webdata/web_data_service.h:109: // Indicates if default search
provider valid.
Use name that I suggested in keyword_table.

[sky, 2011-10-21 17:59:29]

One other comment. Could we always store the default search provider encrypted?
That way we don't have two separate entries to worry about. It isn't as obscure,
but do we care?

  -Scott

[whywhat, 2011-10-21 18:52:19]

On 2011/10/21 17:59:29, sky wrote:
> One other comment. Could we always store the default search provider
encrypted?
> That way we don't have two separate entries to worry about. It isn't as
obscure,
> but do we care?
> 
>   -Scott

The idea is to be able to restore setting to the original value. So we have to
keep the second value. Otherwise if the orinal setting is corrupted by
third-party we'd be able only to show settings page.

[sky, 2011-10-21 19:18:21]

On Fri, Oct 21, 2011 at 11:52 AM,  <avayvod@chromium.org> wrote:
> On 2011/10/21 17:59:29, sky wrote:
>>
>> One other comment. Could we always store the default search provider
>
> encrypted?
>>
>> That way we don't have two separate entries to worry about. It isn't as
>
> obscure,
>>
>> but do we care?
>
>>   -Scott
>
> The idea is to be able to restore setting to the original value. So we have
> to
> keep the second value. Otherwise if the orinal setting is corrupted by
> third-party we'd be able only to show settings page.

Fair enough.

  -Scott

[whywhat, 2011-10-21 20:31:53]

http://codereview.chromium.org/8342049/diff/15001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.h (right):

http://codereview.chromium.org/8342049/diff/15001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.h:298: scoped_ptr<protector::Protector>
protector_;
On 2011/10/21 17:20:30, sky wrote:
> You'll need to run this by Elliott. We're moving things off Profile.

Move this off Profile

http://codereview.chromium.org/8342049/diff/15001/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_service.h (right):

http://codereview.chromium.org/8342049/diff/15001/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.h:558: const TemplateURL*
backup_default_search_provider_;
On 2011/10/21 17:20:30, sky wrote:
> I don't like persisting this and is_default_search_provider_verified. Can't
> TemplateURLService::OnWebDataServiceRequestDone call off to the protector and
> let the protected do whatever it wants.

Done.

http://codereview.chromium.org/8342049/diff/15001/chrome/browser/search_engin...
File chrome/browser/search_engines/util.cc (right):

http://codereview.chromium.org/8342049/diff/15001/chrome/browser/search_engin...
chrome/browser/search_engines/util.cc:216: DCHECK(result.GetType() ==
KEYWORDS_RESULT);
On 2011/10/21 17:20:30, sky wrote:
> DCHECK_EQ

Done.

http://codereview.chromium.org/8342049/diff/15001/chrome/browser/search_engin...
chrome/browser/search_engines/util.cc:218: WDKeywordsResult keyword_result =
reinterpret_cast<
On 2011/10/21 17:20:30, sky wrote:
> This should take a WDKeywordsResult.

Do you mean the fiunction? But I'd have to reinterpret_cast the structure
pointer in the caller. I think it's better to do it there (and more consistent
with the function above).

http://codereview.chromium.org/8342049/diff/15001/chrome/browser/webdata/keyw...
File chrome/browser/webdata/keyword_table.h (right):

http://codereview.chromium.org/8342049/diff/15001/chrome/browser/webdata/keyw...
chrome/browser/webdata/keyword_table.h:90: bool
IsDefaultSearchProviderIDVerified();
On 2011/10/21 17:20:30, sky wrote:
> This name is a bit confusing. How about: DidDefaultSearchProviderChange() with
a
> description something like: Returns true if the default search provider
changed
> out from under us. This can happen if another process modifies our database
(or
> we get corruption).

Done.

http://codereview.chromium.org/8342049/diff/15001/chrome/browser/webdata/web_...
File chrome/browser/webdata/web_data_service.h (right):

http://codereview.chromium.org/8342049/diff/15001/chrome/browser/webdata/web_...
chrome/browser/webdata/web_data_service.h:109: // Indicates if default search
provider valid.
On 2011/10/21 17:20:30, sky wrote:
> Use name that I suggested in keyword_table.

Done.

[sky, 2011-10-21 21:34:11]

http://codereview.chromium.org/8342049/diff/15001/chrome/browser/search_engin...
File chrome/browser/search_engines/util.cc (right):

http://codereview.chromium.org/8342049/diff/15001/chrome/browser/search_engin...
chrome/browser/search_engines/util.cc:218: WDKeywordsResult keyword_result =
reinterpret_cast<
On 2011/10/21 20:31:53, whywhat wrote:
> On 2011/10/21 17:20:30, sky wrote:
> > This should take a WDKeywordsResult.
> 
> Do you mean the fiunction? But I'd have to reinterpret_cast the structure
> pointer in the caller. I think it's better to do it there (and more consistent
> with the function above).

I thought the caller already was doing it. But as you say, it isn't so it's fine
like you have it.

http://codereview.chromium.org/8342049/diff/15003/chrome/browser/protector/de...
File chrome/browser/protector/default_search_provider_change.cc (right):

http://codereview.chromium.org/8342049/diff/15003/chrome/browser/protector/de...
chrome/browser/protector/default_search_provider_change.cc:31:
~DefaultSearchProviderChange();
virtual

http://codereview.chromium.org/8342049/diff/15003/chrome/browser/protector/de...
chrome/browser/protector/default_search_provider_change.cc:102: url =
urls.front();
The first may be a poor choice. Better to choose one of the prepopulated ones.

http://codereview.chromium.org/8342049/diff/15003/chrome/browser/protector/se...
File chrome/browser/protector/settings_change_global_error.h (right):

http://codereview.chromium.org/8342049/diff/15003/chrome/browser/protector/se...
chrome/browser/protector/settings_change_global_error.h:24: class Delegate {
This should be in its own header so that you don't have to include
settings_change_global_error.h to implement it.

http://codereview.chromium.org/8342049/diff/15003/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_service.cc (right):

http://codereview.chromium.org/8342049/diff/15003/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:447: void
TemplateURLService::ForceSetDefaultSearchProvider(
Having to expose a method like this is icky. Can you instead set it to NULL then
to what you want.

http://codereview.chromium.org/8342049/diff/15003/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:531:
default_search_provider,
Should we ignore this if managed?

Protector seems to be caching the TemplateURLs directly. There is nothing
stopping the TemplateURLService from deleting these. Either the protector should
observe the method and deal with the items being deleted, or cache the
information it needs in the constructor and not hold onto the TemplateURLs.

http://codereview.chromium.org/8342049/diff/15003/chrome/browser/search_engin...
File chrome/browser/search_engines/util.cc (right):

http://codereview.chromium.org/8342049/diff/15003/chrome/browser/search_engin...
chrome/browser/search_engines/util.cc:224: return
keyword_result.did_default_search_provider_change;
Can't we return right away if !did_default_Search_provider_change?

[whywhat, 2011-10-24 15:30:17]

Please, take a look.
Scott, could you also advise on the right way to trigger the check on Chrome
start up? Currently it looks like TemplateURLService is loaded when I type
something in Omnibox.

http://codereview.chromium.org/8342049/diff/15003/chrome/browser/protector/de...
File chrome/browser/protector/default_search_provider_change.cc (right):

http://codereview.chromium.org/8342049/diff/15003/chrome/browser/protector/de...
chrome/browser/protector/default_search_provider_change.cc:31:
~DefaultSearchProviderChange();
On 2011/10/21 21:34:11, sky wrote:
> virtual

Done.

http://codereview.chromium.org/8342049/diff/15003/chrome/browser/protector/de...
chrome/browser/protector/default_search_provider_change.cc:102: url =
urls.front();
On 2011/10/21 21:34:11, sky wrote:
> The first may be a poor choice. Better to choose one of the prepopulated ones.

Done.
I had to make FindNewDefaultSearchProvider method public not to copy its
implementation.
I'd add GetTemplateURLByID to TemplateURLService too (it's already used in two
places in utils.cc and would be welcome here too).

http://codereview.chromium.org/8342049/diff/15003/chrome/browser/protector/se...
File chrome/browser/protector/settings_change_global_error.h (right):

http://codereview.chromium.org/8342049/diff/15003/chrome/browser/protector/se...
chrome/browser/protector/settings_change_global_error.h:24: class Delegate {
On 2011/10/21 21:34:11, sky wrote:
> This should be in its own header so that you don't have to include
> settings_change_global_error.h to implement it.

Done.

http://codereview.chromium.org/8342049/diff/15003/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_service.cc (right):

http://codereview.chromium.org/8342049/diff/15003/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:447: void
TemplateURLService::ForceSetDefaultSearchProvider(
On 2011/10/21 21:34:11, sky wrote:
> Having to expose a method like this is icky. Can you instead set it to NULL
then
> to what you want.

Done.
I think using the fact that setting dsp to NULL and then to something else will
force save to db is also leak of abstraction here. Perhaps smth like
WebDataService or KeywordTable should cache backup and signature and decide
whether to set value in db or not while TemplateURLService should not check for
equality for dsp.

http://codereview.chromium.org/8342049/diff/15003/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:531:
default_search_provider,
On 2011/10/21 21:34:11, sky wrote:
> Should we ignore this if managed?
> 
> Protector seems to be caching the TemplateURLs directly. There is nothing
> stopping the TemplateURLService from deleting these. Either the protector
should
> observe the method and deal with the items being deleted, or cache the
> information it needs in the constructor and not hold onto the TemplateURLs.

Done.

http://codereview.chromium.org/8342049/diff/15003/chrome/browser/search_engin...
File chrome/browser/search_engines/util.cc (right):

http://codereview.chromium.org/8342049/diff/15003/chrome/browser/search_engin...
chrome/browser/search_engines/util.cc:224: return
keyword_result.did_default_search_provider_change;
On 2011/10/21 21:34:11, sky wrote:
> Can't we return right away if !did_default_Search_provider_change?

Done.

[sky, 2011-10-24 15:51:23]

On Mon, Oct 24, 2011 at 8:30 AM,  <avayvod@chromium.org> wrote:
> Please, take a look.
> Scott, could you also advise on the right way to trigger the check on Chrome
> start up? Currently it looks like TemplateURLService is loaded when I type
> something in Omnibox.

If you do it at startup, it means you're going to cause more disk
contention and slow down startup.

>
>
>
http://codereview.chromium.org/8342049/diff/15003/chrome/browser/protector/de...
> File chrome/browser/protector/default_search_provider_change.cc (right):
>
>
http://codereview.chromium.org/8342049/diff/15003/chrome/browser/protector/de...
> chrome/browser/protector/default_search_provider_change.cc:31:
> ~DefaultSearchProviderChange();
> On 2011/10/21 21:34:11, sky wrote:
>>
>> virtual
>
> Done.
>
>
http://codereview.chromium.org/8342049/diff/15003/chrome/browser/protector/de...
> chrome/browser/protector/default_search_provider_change.cc:102: url =
> urls.front();
> On 2011/10/21 21:34:11, sky wrote:
>>
>> The first may be a poor choice. Better to choose one of the
>
> prepopulated ones.
>
> Done.
> I had to make FindNewDefaultSearchProvider method public not to copy its
> implementation.
> I'd add GetTemplateURLByID to TemplateURLService too (it's already used
> in two places in utils.cc and would be welcome here too).
>
>
http://codereview.chromium.org/8342049/diff/15003/chrome/browser/protector/se...
> File chrome/browser/protector/settings_change_global_error.h (right):
>
>
http://codereview.chromium.org/8342049/diff/15003/chrome/browser/protector/se...
> chrome/browser/protector/settings_change_global_error.h:24: class
> Delegate {
> On 2011/10/21 21:34:11, sky wrote:
>>
>> This should be in its own header so that you don't have to include
>> settings_change_global_error.h to implement it.
>
> Done.
>
>
http://codereview.chromium.org/8342049/diff/15003/chrome/browser/search_engin...
> File chrome/browser/search_engines/template_url_service.cc (right):
>
>
http://codereview.chromium.org/8342049/diff/15003/chrome/browser/search_engin...
> chrome/browser/search_engines/template_url_service.cc:447: void
> TemplateURLService::ForceSetDefaultSearchProvider(
> On 2011/10/21 21:34:11, sky wrote:
>>
>> Having to expose a method like this is icky. Can you instead set it to
>
> NULL then
>>
>> to what you want.
>
> Done.
> I think using the fact that setting dsp to NULL and then to something
> else will force save to db is also leak of abstraction here. Perhaps
> smth like WebDataService or KeywordTable should cache backup and
> signature and decide whether to set value in db or not while
> TemplateURLService should not check for equality for dsp.

I agree with you. I'm find removing the == check in TemplateURLService
with a comment added as to why we don't do a check.

  -Scott

>
>
http://codereview.chromium.org/8342049/diff/15003/chrome/browser/search_engin...
> chrome/browser/search_engines/template_url_service.cc:531:
> default_search_provider,
> On 2011/10/21 21:34:11, sky wrote:
>>
>> Should we ignore this if managed?
>
>> Protector seems to be caching the TemplateURLs directly. There is
>
> nothing
>>
>> stopping the TemplateURLService from deleting these. Either the
>
> protector should
>>
>> observe the method and deal with the items being deleted, or cache the
>> information it needs in the constructor and not hold onto the
>
> TemplateURLs.
>
> Done.
>
>
http://codereview.chromium.org/8342049/diff/15003/chrome/browser/search_engin...
> File chrome/browser/search_engines/util.cc (right):
>
>
http://codereview.chromium.org/8342049/diff/15003/chrome/browser/search_engin...
> chrome/browser/search_engines/util.cc:224: return
> keyword_result.did_default_search_provider_change;
> On 2011/10/21 21:34:11, sky wrote:
>>
>> Can't we return right away if !did_default_Search_provider_change?
>
> Done.
>
> http://codereview.chromium.org/8342049/
>

[whywhat, 2011-10-24 16:49:24]

On 2011/10/24 15:51:23, sky wrote:
> On Mon, Oct 24, 2011 at 8:30 AM,  <mailto:avayvod@chromium.org> wrote:
> > Please, take a look.
> > Scott, could you also advise on the right way to trigger the check on Chrome
> > start up? Currently it looks like TemplateURLService is loaded when I type
> > something in Omnibox.
> 
> If you do it at startup, it means you're going to cause more disk
> contention and slow down startup.
> 
> >
> >
> >
>
http://codereview.chromium.org/8342049/diff/15003/chrome/browser/protector/de...
> > File chrome/browser/protector/default_search_provider_change.cc (right):
> >
> >
>
http://codereview.chromium.org/8342049/diff/15003/chrome/browser/protector/de...
> > chrome/browser/protector/default_search_provider_change.cc:31:
> > ~DefaultSearchProviderChange();
> > On 2011/10/21 21:34:11, sky wrote:
> >>
> >> virtual
> >
> > Done.
> >
> >
>
http://codereview.chromium.org/8342049/diff/15003/chrome/browser/protector/de...
> > chrome/browser/protector/default_search_provider_change.cc:102: url =
> > urls.front();
> > On 2011/10/21 21:34:11, sky wrote:
> >>
> >> The first may be a poor choice. Better to choose one of the
> >
> > prepopulated ones.
> >
> > Done.
> > I had to make FindNewDefaultSearchProvider method public not to copy its
> > implementation.
> > I'd add GetTemplateURLByID to TemplateURLService too (it's already used
> > in two places in utils.cc and would be welcome here too).
> >
> >
>
http://codereview.chromium.org/8342049/diff/15003/chrome/browser/protector/se...
> > File chrome/browser/protector/settings_change_global_error.h (right):
> >
> >
>
http://codereview.chromium.org/8342049/diff/15003/chrome/browser/protector/se...
> > chrome/browser/protector/settings_change_global_error.h:24: class
> > Delegate {
> > On 2011/10/21 21:34:11, sky wrote:
> >>
> >> This should be in its own header so that you don't have to include
> >> settings_change_global_error.h to implement it.
> >
> > Done.
> >
> >
>
http://codereview.chromium.org/8342049/diff/15003/chrome/browser/search_engin...
> > File chrome/browser/search_engines/template_url_service.cc (right):
> >
> >
>
http://codereview.chromium.org/8342049/diff/15003/chrome/browser/search_engin...
> > chrome/browser/search_engines/template_url_service.cc:447: void
> > TemplateURLService::ForceSetDefaultSearchProvider(
> > On 2011/10/21 21:34:11, sky wrote:
> >>
> >> Having to expose a method like this is icky. Can you instead set it to
> >
> > NULL then
> >>
> >> to what you want.
> >
> > Done.
> > I think using the fact that setting dsp to NULL and then to something
> > else will force save to db is also leak of abstraction here. Perhaps
> > smth like WebDataService or KeywordTable should cache backup and
> > signature and decide whether to set value in db or not while
> > TemplateURLService should not check for equality for dsp.
> 
> I agree with you. I'm find removing the == check in TemplateURLService
> with a comment added as to why we don't do a check.

Won't it overnotify observers? Can we just move the check after
SetDefaultSearchProviderNoNotify? 

> 
>   -Scott
> 
> >
> >
>
http://codereview.chromium.org/8342049/diff/15003/chrome/browser/search_engin...
> > chrome/browser/search_engines/template_url_service.cc:531:
> > default_search_provider,
> > On 2011/10/21 21:34:11, sky wrote:
> >>
> >> Should we ignore this if managed?
> >
> >> Protector seems to be caching the TemplateURLs directly. There is
> >
> > nothing
> >>
> >> stopping the TemplateURLService from deleting these. Either the
> >
> > protector should
> >>
> >> observe the method and deal with the items being deleted, or cache the
> >> information it needs in the constructor and not hold onto the
> >
> > TemplateURLs.
> >
> > Done.
> >
> >
>
http://codereview.chromium.org/8342049/diff/15003/chrome/browser/search_engin...
> > File chrome/browser/search_engines/util.cc (right):
> >
> >
>
http://codereview.chromium.org/8342049/diff/15003/chrome/browser/search_engin...
> > chrome/browser/search_engines/util.cc:224: return
> > keyword_result.did_default_search_provider_change;
> > On 2011/10/21 21:34:11, sky wrote:
> >>
> >> Can't we return right away if !did_default_Search_provider_change?
> >
> > Done.
> >
> > http://codereview.chromium.org/8342049/
> >

[sky, 2011-10-24 16:58:20]

On Mon, Oct 24, 2011 at 9:49 AM,  <avayvod@chromium.org> wrote:
> On 2011/10/24 15:51:23, sky wrote:
>>
>> On Mon, Oct 24, 2011 at 8:30 AM,  <mailto:avayvod@chromium.org> wrote:
>> > Please, take a look.
>> > Scott, could you also advise on the right way to trigger the check on
>> > Chrome
>> > start up? Currently it looks like TemplateURLService is loaded when I
>> > type
>> > something in Omnibox.
>
>> If you do it at startup, it means you're going to cause more disk
>> contention and slow down startup.
>
>> >
>> >
>> >
>
>
http://codereview.chromium.org/8342049/diff/15003/chrome/browser/protector/de...
>>
>> > File chrome/browser/protector/default_search_provider_change.cc (right):
>> >
>> >
>
>
http://codereview.chromium.org/8342049/diff/15003/chrome/browser/protector/de...
>>
>> > chrome/browser/protector/default_search_provider_change.cc:31:
>> > ~DefaultSearchProviderChange();
>> > On 2011/10/21 21:34:11, sky wrote:
>> >>
>> >> virtual
>> >
>> > Done.
>> >
>> >
>
>
http://codereview.chromium.org/8342049/diff/15003/chrome/browser/protector/de...
>>
>> > chrome/browser/protector/default_search_provider_change.cc:102: url =
>> > urls.front();
>> > On 2011/10/21 21:34:11, sky wrote:
>> >>
>> >> The first may be a poor choice. Better to choose one of the
>> >
>> > prepopulated ones.
>> >
>> > Done.
>> > I had to make FindNewDefaultSearchProvider method public not to copy its
>> > implementation.
>> > I'd add GetTemplateURLByID to TemplateURLService too (it's already used
>> > in two places in utils.cc and would be welcome here too).
>> >
>> >
>
>
http://codereview.chromium.org/8342049/diff/15003/chrome/browser/protector/se...
>>
>> > File chrome/browser/protector/settings_change_global_error.h (right):
>> >
>> >
>
>
http://codereview.chromium.org/8342049/diff/15003/chrome/browser/protector/se...
>>
>> > chrome/browser/protector/settings_change_global_error.h:24: class
>> > Delegate {
>> > On 2011/10/21 21:34:11, sky wrote:
>> >>
>> >> This should be in its own header so that you don't have to include
>> >> settings_change_global_error.h to implement it.
>> >
>> > Done.
>> >
>> >
>
>
http://codereview.chromium.org/8342049/diff/15003/chrome/browser/search_engin...
>>
>> > File chrome/browser/search_engines/template_url_service.cc (right):
>> >
>> >
>
>
http://codereview.chromium.org/8342049/diff/15003/chrome/browser/search_engin...
>>
>> > chrome/browser/search_engines/template_url_service.cc:447: void
>> > TemplateURLService::ForceSetDefaultSearchProvider(
>> > On 2011/10/21 21:34:11, sky wrote:
>> >>
>> >> Having to expose a method like this is icky. Can you instead set it to
>> >
>> > NULL then
>> >>
>> >> to what you want.
>> >
>> > Done.
>> > I think using the fact that setting dsp to NULL and then to something
>> > else will force save to db is also leak of abstraction here. Perhaps
>> > smth like WebDataService or KeywordTable should cache backup and
>> > signature and decide whether to set value in db or not while
>> > TemplateURLService should not check for equality for dsp.
>
>> I agree with you. I'm find removing the == check in TemplateURLService
>> with a comment added as to why we don't do a check.
>
> Won't it overnotify observers? Can we just move the check after
> SetDefaultSearchProviderNoNotify?

I don't think it'll matter, but I'm fine with moving the check (and
updating the comment).

  -Scott

>
>>   -Scott
>
>> >
>> >
>
>
http://codereview.chromium.org/8342049/diff/15003/chrome/browser/search_engin...
>>
>> > chrome/browser/search_engines/template_url_service.cc:531:
>> > default_search_provider,
>> > On 2011/10/21 21:34:11, sky wrote:
>> >>
>> >> Should we ignore this if managed?
>> >
>> >> Protector seems to be caching the TemplateURLs directly. There is
>> >
>> > nothing
>> >>
>> >> stopping the TemplateURLService from deleting these. Either the
>> >
>> > protector should
>> >>
>> >> observe the method and deal with the items being deleted, or cache the
>> >> information it needs in the constructor and not hold onto the
>> >
>> > TemplateURLs.
>> >
>> > Done.
>> >
>> >
>
>
http://codereview.chromium.org/8342049/diff/15003/chrome/browser/search_engin...
>>
>> > File chrome/browser/search_engines/util.cc (right):
>> >
>> >
>
>
http://codereview.chromium.org/8342049/diff/15003/chrome/browser/search_engin...
>>
>> > chrome/browser/search_engines/util.cc:224: return
>> > keyword_result.did_default_search_provider_change;
>> > On 2011/10/21 21:34:11, sky wrote:
>> >>
>> >> Can't we return right away if !did_default_Search_provider_change?
>> >
>> > Done.
>> >
>> > http://codereview.chromium.org/8342049/
>> >
>
>
>
> http://codereview.chromium.org/8342049/
>

[whywhat, 2011-10-24 17:00:46]

Done. I'll wait for your explicit approval before committing.

[sky, 2011-10-24 17:04:12]

http://codereview.chromium.org/8342049/diff/22020/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_service.cc (right):

http://codereview.chromium.org/8342049/diff/22020/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:469: if
(!template_urls_.empty()) {
change this to:
return template_urls_.empty() ? NULL : ...

http://codereview.chromium.org/8342049/diff/22020/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_service.h (right):

http://codereview.chromium.org/8342049/diff/22020/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.h:207: // empty.
Document ownership of returned pointer.

http://codereview.chromium.org/8342049/diff/22020/chrome/browser/webdata/web_...
File chrome/browser/webdata/web_data_service.cc (right):

http://codereview.chromium.org/8342049/diff/22020/chrome/browser/webdata/web_...
chrome/browser/webdata/web_data_service.cc:754:
result.did_default_search_provider_change =
Add a test to make sure did_default_search_provider_change isn't set to false on
a new profile.

http://codereview.chromium.org/8342049/diff/22020/chrome/browser/webdata/web_...
File chrome/browser/webdata/web_data_service.h (right):

http://codereview.chromium.org/8342049/diff/22020/chrome/browser/webdata/web_...
chrome/browser/webdata/web_data_service.h:109: // Indicates if default search
provider has been changed by something else
else -> other

[sky, 2011-10-24 17:23:21]

http://codereview.chromium.org/8342049/diff/22020/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_service.cc (right):

http://codereview.chromium.org/8342049/diff/22020/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:441: // We must always
persist the setting in the database since we have to
How about changing this comment to:
Always persist the setting in the database, that way if the backup signature has
changed out from under us it gets reset correctly.

[whywhat, 2011-10-25 10:30:41]

http://codereview.chromium.org/8342049/diff/22020/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_service.cc (right):

http://codereview.chromium.org/8342049/diff/22020/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:441: // We must always
persist the setting in the database since we have to
On 2011/10/24 17:23:21, sky wrote:
> How about changing this comment to:
> Always persist the setting in the database, that way if the backup signature
has
> changed out from under us it gets reset correctly.

Done.

http://codereview.chromium.org/8342049/diff/22020/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.cc:469: if
(!template_urls_.empty()) {
On 2011/10/24 17:04:13, sky wrote:
> change this to:
> return template_urls_.empty() ? NULL : ...

Done.

http://codereview.chromium.org/8342049/diff/22020/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_service.h (right):

http://codereview.chromium.org/8342049/diff/22020/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service.h:207: // empty.
On 2011/10/24 17:04:13, sky wrote:
> Document ownership of returned pointer.

Done.

http://codereview.chromium.org/8342049/diff/22020/chrome/browser/webdata/web_...
File chrome/browser/webdata/web_data_service.cc (right):

http://codereview.chromium.org/8342049/diff/22020/chrome/browser/webdata/web_...
chrome/browser/webdata/web_data_service.cc:754:
result.did_default_search_provider_change =
On 2011/10/24 17:04:13, sky wrote:
> Add a test to make sure did_default_search_provider_change isn't set to false
on
> a new profile.

Done.

http://codereview.chromium.org/8342049/diff/22020/chrome/browser/webdata/web_...
File chrome/browser/webdata/web_data_service.h (right):

http://codereview.chromium.org/8342049/diff/22020/chrome/browser/webdata/web_...
chrome/browser/webdata/web_data_service.h:109: // Indicates if default search
provider has been changed by something else
On 2011/10/24 17:04:13, sky wrote:
> else -> other

Done.

[sky, 2011-10-25 14:52:15]

LGTM

[whywhat, 2011-10-25 21:42:19]

Thanks for thorough review! Will submit as soon as I'm sure interactive_ui_tests
fail not because of the change (it passes locally).