Protector bubble cancels itself if user changes default search engine manually.

Protector bubble cancels itself if user changes default search engine manually.

BUG=None
TEST=Manual: trigger protector bubble, change search engine manually; bubble should vanish.

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

[Ivan Korotkov, 2011-11-21 13:53:21]

PTAL

[whywhat, 2011-11-21 14:51:21]

lgtm

http://codereview.chromium.org/8612002/diff/1/chrome/browser/protector/base_s...
File chrome/browser/protector/base_setting_change.h (right):

http://codereview.chromium.org/8612002/diff/1/chrome/browser/protector/base_s...
chrome/browser/protector/base_setting_change.h:32: // call the base method.
This method cries to be a ctor...

http://codereview.chromium.org/8612002/diff/1/chrome/browser/protector/defaul...
File chrome/browser/protector/default_search_provider_change.cc (right):

http://codereview.chromium.org/8612002/diff/1/chrome/browser/protector/defaul...
chrome/browser/protector/default_search_provider_change.cc:191:
default_search_provider_) {
Note: at this moment default_search_provider_ may point to deallocated memory.
So we might better set it to NULL if the condition is true.

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

http://codereview.chromium.org/8612002/diff/1/chrome/browser/protector/protec...
chrome/browser/protector/protector.cc:82: delete this;
Do we have to delete this?

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

http://codereview.chromium.org/8612002/diff/1/chrome/browser/protector/protec...
chrome/browser/protector/protector.h:58: // middle of some operations on
settings that have changed.
You should document that object might be self-destroyed here...

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

http://codereview.chromium.org/8612002/diff/1/chrome/browser/protector/settin...
chrome/browser/protector/settings_change_global_error.h:28: // deleted until
|OnRemovedFromProfile| is called. Uses |delegate| to notify
Did you mean RemoveFromProfile?

[Ivan Korotkov, 2011-11-21 15:27:38]

http://codereview.chromium.org/8612002/diff/1/chrome/browser/protector/base_s...
File chrome/browser/protector/base_setting_change.h (right):

http://codereview.chromium.org/8612002/diff/1/chrome/browser/protector/base_s...
chrome/browser/protector/base_setting_change.h:32: // call the base method.
Kind of, but it's called asynchronously (to reset default SE).
We can call pass Protector to the real ctor so there will be no need to call it
from base.

http://codereview.chromium.org/8612002/diff/1/chrome/browser/protector/defaul...
File chrome/browser/protector/default_search_provider_change.cc (right):

http://codereview.chromium.org/8612002/diff/1/chrome/browser/protector/defaul...
chrome/browser/protector/default_search_provider_change.cc:191:
default_search_provider_) {
Nice catch, thanks.

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

http://codereview.chromium.org/8612002/diff/1/chrome/browser/protector/protec...
chrome/browser/protector/protector.cc:82: delete this;
This is called asynchronously at the moment when nobody should have a reference
to the Protector (because Change is deleted, Error not yet created and the
function that created Protector already exited).
So I'd rather not do async deletion from more than one place.

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

http://codereview.chromium.org/8612002/diff/1/chrome/browser/protector/protec...
chrome/browser/protector/protector.h:58: // middle of some operations on
settings that have changed.
On 2011/11/21 14:51:21, whywhat wrote:
> You should document that object might be self-destroyed here...

Done.

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

http://codereview.chromium.org/8612002/diff/1/chrome/browser/protector/settin...
chrome/browser/protector/settings_change_global_error.h:28: // deleted until
|OnRemovedFromProfile| is called. Uses |delegate| to notify
On 2011/11/21 14:51:21, whywhat wrote:
> Did you mean RemoveFromProfile?

I meant |Delegate::OnRemovedFromProfile|