Protector strings and bubble/SettingsChange code refactoring.

Protector strings and bubble/SettingsChange code refactoring.

BUG=102765, 103317
TEST=Manual

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

[Ivan Korotkov, 2011-11-17 18:08:22]

PTA close L!

[whywhat, 2011-11-18 08:18:19]

LGTM with extra credits for:
- My Awesome Blog
- old setting is old
- server and client in X11

http://codereview.chromium.org/8590036/diff/2001/chrome/browser/protector/def...
File chrome/browser/protector/default_search_provider_change.cc (right):

http://codereview.chromium.org/8590036/diff/2001/chrome/browser/protector/def...
chrome/browser/protector/default_search_provider_change.cc:26: }
// namespace

http://codereview.chromium.org/8590036/diff/2001/chrome/browser/protector/def...
chrome/browser/protector/default_search_provider_change.cc:152: // Old setting
is old, offer to go to settings.
is old -> is lost

http://codereview.chromium.org/8590036/diff/2001/chrome/browser/protector/def...
chrome/browser/protector/default_search_provider_change.cc:178: fallback_id_ =
url->id();
This code is logically part of initialization, not setting dsp to a value. I'd
prefer fallback_id_ and fallback_name_ being set explicitly in Init.

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

http://codereview.chromium.org/8590036/diff/2001/chrome/browser/protector/pro...
chrome/browser/protector/protector.cc:68: VLOG(1) << "Timeout";
Add a todo for histogram here, please.

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

http://codereview.chromium.org/8590036/diff/2001/chrome/browser/protector/set...
chrome/browser/protector/settings_change_global_error.h:39: SettingChange*
change() { return change_.get(); }
Should be mutable_change() since the returned pointer is non-const.

[Ivan Korotkov, 2011-11-18 10:44:25]

http://codereview.chromium.org/8590036/diff/2001/chrome/browser/protector/def...
File chrome/browser/protector/default_search_provider_change.cc (right):

http://codereview.chromium.org/8590036/diff/2001/chrome/browser/protector/def...
chrome/browser/protector/default_search_provider_change.cc:26: }
On 2011/11/18 08:18:20, whywhat wrote:
> // namespace

Done.

http://codereview.chromium.org/8590036/diff/2001/chrome/browser/protector/def...
chrome/browser/protector/default_search_provider_change.cc:152: // Old setting
is old, offer to go to settings.
On 2011/11/18 08:18:20, whywhat wrote:
> is old -> is lost

Done.

http://codereview.chromium.org/8590036/diff/2001/chrome/browser/protector/def...
chrome/browser/protector/default_search_provider_change.cc:178: fallback_id_ =
url->id();
Makes sense.

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

http://codereview.chromium.org/8590036/diff/2001/chrome/browser/protector/pro...
chrome/browser/protector/protector.cc:68: VLOG(1) << "Timeout";
On 2011/11/18 08:18:20, whywhat wrote:
> Add a todo for histogram here, please.

Done.

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

http://codereview.chromium.org/8590036/diff/2001/chrome/browser/protector/set...
chrome/browser/protector/settings_change_global_error.h:39: SettingChange*
change() { return change_.get(); }
On 2011/11/18 08:18:20, whywhat wrote:
> Should be mutable_change() since the returned pointer is non-const.

Done.