WebUI for Profile Settings Reset

WebUI for Profile Settings Reset

This CL implements chrome://settings/profileSettingsReset, a feature to reset
profile settings that malware often modifies.

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

[battre, 2013-05-03 17:54:13]

Hi Evan,

may I ask you you review this CL? I'll share the design document separately with
you.

The code is mostly a replication of the clear browsing data UI.

Thanks,
Dominic

[battre, 2013-05-06 08:08:54]

CC: vasilii FYI

[Evan Stade, 2013-05-09 04:40:13]

sorry this CL fell off my radar. I've been focusing on preparing for i/o. Is it
possible to find another reviewer? If not, I'll have a look tomorrow.

[battre, 2013-05-09 08:54:49]

Hi James,

could you please review this CL?

Thanks,
Dominic

[battre, 2013-05-09 19:11:55]

Hi Dan,

passing this on to you. Could you please review the CL?

Thanks,
Dominic

[vasilii, 2013-05-13 09:13:06]

https://codereview.chromium.org/14924002/diff/9001/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/14924002/diff/9001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/browser_options_handler.cc:506:
values->SetBoolean("enableProfileSection",
Maybe enableResetProfileSection is a better name?

https://codereview.chromium.org/14924002/diff/9001/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/reset_profile_settings_handler.cc (right):

https://codereview.chromium.org/14924002/diff/9001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/reset_profile_settings_handler.cc:102:
reset_mask |= ProfileResetter::DEFAULT_SEARCH_ENGINE;
What about this?

struct {
  const char* flag_name;
  ProfileResetter::Resettable mask;
} name2flag[] = {
 {prefs::kResetDefaultSearchEngine, DEFAULT_SEARCH_ENGINE},
  ...
};
for (size_t i = 0; i < arraysize(name2flag); ++i)
  if (prefs->GetBoolean(name2flag[i].flag_name)
    reset_mask |= name2flag[i].mask;

https://codereview.chromium.org/14924002/diff/9001/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/reset_profile_settings_handler.h (right):

https://codereview.chromium.org/14924002/diff/9001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/reset_profile_settings_handler.h:8: #include
"base/compiler_specific.h"
Do we need this header?

[battre, 2013-05-13 15:54:06]

Thanks.

https://codereview.chromium.org/14924002/diff/9001/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/14924002/diff/9001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/browser_options_handler.cc:506:
values->SetBoolean("enableProfileSection",
On 2013/05/13 09:13:06, vasilii wrote:
> Maybe enableResetProfileSection is a better name?

Done.

https://codereview.chromium.org/14924002/diff/9001/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/reset_profile_settings_handler.cc (right):

https://codereview.chromium.org/14924002/diff/9001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/reset_profile_settings_handler.cc:102:
reset_mask |= ProfileResetter::DEFAULT_SEARCH_ENGINE;
On 2013/05/13 09:13:06, vasilii wrote:
> What about this?
> 
> struct {
>   const char* flag_name;
>   ProfileResetter::Resettable mask;
> } name2flag[] = {
>  {prefs::kResetDefaultSearchEngine, DEFAULT_SEARCH_ENGINE},
>   ...
> };
> for (size_t i = 0; i < arraysize(name2flag); ++i)
>   if (prefs->GetBoolean(name2flag[i].flag_name)
>     reset_mask |= name2flag[i].mask;

Done.

https://codereview.chromium.org/14924002/diff/9001/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/reset_profile_settings_handler.h (right):

https://codereview.chromium.org/14924002/diff/9001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/reset_profile_settings_handler.h:8: #include
"base/compiler_specific.h"
On 2013/05/13 09:13:06, vasilii wrote:
> Do we need this header?

Yes, we need this for OVERRIDE

[James Hawkins, 2013-05-13 17:31:32]

https://codereview.chromium.org/14924002/diff/20025/chrome/browser/resources/...
File chrome/browser/resources/options/browser_options.html (right):

https://codereview.chromium.org/14924002/diff/20025/chrome/browser/resources/...
chrome/browser/resources/options/browser_options.html:731: <button
id="reset-profile-settings-button"
nit: Don't append the element name to the ID, i.e., remove "-button".

https://codereview.chromium.org/14924002/diff/20025/chrome/browser/resources/...
File chrome/browser/resources/options/reset_profile_settings_overlay.html
(right):

https://codereview.chromium.org/14924002/diff/20025/chrome/browser/resources/...
chrome/browser/resources/options/reset_profile_settings_overlay.html:47:
data-type="number"></select>
nit: Close tag of wrapped element must be on a new line.

https://codereview.chromium.org/14924002/diff/20025/chrome/browser/resources/...
chrome/browser/resources/options/reset_profile_settings_overlay.html:54:
i18n-values="href:resetProfileSettingsLearnMoreUrl"></a>
nit: Close tag of wrapped element must be on a new line.

https://codereview.chromium.org/14924002/diff/20025/chrome/browser/resources/...
File chrome/browser/resources/options/reset_profile_settings_overlay.js (right):

https://codereview.chromium.org/14924002/diff/20025/chrome/browser/resources/...
chrome/browser/resources/options/reset_profile_settings_overlay.js:30: // Call
base class implementation to starts preference initialization.
Optional nit: I think this comment is superfluous.

https://codereview.chromium.org/14924002/diff/20025/chrome/browser/resources/...
chrome/browser/resources/options/reset_profile_settings_overlay.js:45: for (var
i = 0; i < checkboxes.length; i++) {
nit: No braces for single-line blocks.

https://codereview.chromium.org/14924002/diff/20025/chrome/browser/resources/...
chrome/browser/resources/options/reset_profile_settings_overlay.js:59: // Set
the enabled state of the commit button.
nit: Sets

https://codereview.chromium.org/14924002/diff/20025/chrome/browser/resources/...
chrome/browser/resources/options/reset_profile_settings_overlay.js:75: // Chrome
callbacks
nit: Unless you want to make these forward to documented, private methods,
document these methods.

https://codereview.chromium.org/14924002/diff/20025/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/reset_profile_settings_handler.cc (right):

https://codereview.chromium.org/14924002/diff/20025/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/reset_profile_settings_handler.cc:23: }
nit: }  // namespace

https://codereview.chromium.org/14924002/diff/20025/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/reset_profile_settings_handler.cc:93:
CHECK(resetter_);
s/CHECK/DCHECK/

[battre, 2013-05-21 09:25:08]

Thank you. PTAL.

Best regards,
Dominic

https://codereview.chromium.org/14924002/diff/20025/chrome/browser/resources/...
File chrome/browser/resources/options/browser_options.html (right):

https://codereview.chromium.org/14924002/diff/20025/chrome/browser/resources/...
chrome/browser/resources/options/browser_options.html:731: <button
id="reset-profile-settings-button"
On 2013/05/13 17:31:33, James Hawkins wrote:
> nit: Don't append the element name to the ID, i.e., remove "-button".

How extensively do you want this to be done? It seems to be pretty common
practice to have "...-section" ids. I think that the "-button" suffix is pretty
useful for distinction. Do you want me to modify the i18n label below as well?

https://codereview.chromium.org/14924002/diff/20025/chrome/browser/resources/...
File chrome/browser/resources/options/reset_profile_settings_overlay.html
(right):

https://codereview.chromium.org/14924002/diff/20025/chrome/browser/resources/...
chrome/browser/resources/options/reset_profile_settings_overlay.html:47:
data-type="number"></select>
On 2013/05/13 17:31:33, James Hawkins wrote:
> nit: Close tag of wrapped element must be on a new line.

Done.

https://codereview.chromium.org/14924002/diff/20025/chrome/browser/resources/...
chrome/browser/resources/options/reset_profile_settings_overlay.html:54:
i18n-values="href:resetProfileSettingsLearnMoreUrl"></a>
On 2013/05/13 17:31:33, James Hawkins wrote:
> nit: Close tag of wrapped element must be on a new line.

Done.

https://codereview.chromium.org/14924002/diff/20025/chrome/browser/resources/...
File chrome/browser/resources/options/reset_profile_settings_overlay.js (right):

https://codereview.chromium.org/14924002/diff/20025/chrome/browser/resources/...
chrome/browser/resources/options/reset_profile_settings_overlay.js:30: // Call
base class implementation to starts preference initialization.
On 2013/05/13 17:31:33, James Hawkins wrote:
> Optional nit: I think this comment is superfluous.

Done.

https://codereview.chromium.org/14924002/diff/20025/chrome/browser/resources/...
chrome/browser/resources/options/reset_profile_settings_overlay.js:45: for (var
i = 0; i < checkboxes.length; i++) {
On 2013/05/13 17:31:33, James Hawkins wrote:
> nit: No braces for single-line blocks.

Done.

https://codereview.chromium.org/14924002/diff/20025/chrome/browser/resources/...
chrome/browser/resources/options/reset_profile_settings_overlay.js:59: // Set
the enabled state of the commit button.
On 2013/05/13 17:31:33, James Hawkins wrote:
> nit: Sets

Done.

https://codereview.chromium.org/14924002/diff/20025/chrome/browser/resources/...
chrome/browser/resources/options/reset_profile_settings_overlay.js:75: // Chrome
callbacks
On 2013/05/13 17:31:33, James Hawkins wrote:
> nit: Unless you want to make these forward to documented, private methods,
> document these methods.

Done.

https://codereview.chromium.org/14924002/diff/20025/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/reset_profile_settings_handler.cc (right):

https://codereview.chromium.org/14924002/diff/20025/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/reset_profile_settings_handler.cc:23: }
On 2013/05/13 17:31:33, James Hawkins wrote:
> nit: }  // namespace

Done.

https://codereview.chromium.org/14924002/diff/20025/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/reset_profile_settings_handler.cc:93:
CHECK(resetter_);
On 2013/05/13 17:31:33, James Hawkins wrote:
> s/CHECK/DCHECK/

Done.

[battre, 2013-05-21 09:25:33]

+sky for OWNERS approval of chrome/browser/ui/browser_ui_prefs.cc

[sky, 2013-05-21 15:08:29]

https://codereview.chromium.org/14924002/diff/37001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_ui_prefs.cc (right):

https://codereview.chromium.org/14924002/diff/37001/chrome/browser/ui/browser...
chrome/browser/ui/browser_ui_prefs.cc:223:
user_prefs::PrefRegistrySyncable::SYNCABLE_PREF);
I haven't been involved with this feature, so forgive the question. Why do we
need prefs for these? If the user clicks the button don't we just reset the
values and not need to persist additional preferences? For example, if the user
clicks the button to reset the default home page shouldn't we just reset the
home page pref and nothing else?

[battre, 2013-05-21 16:29:31]

https://codereview.chromium.org/14924002/diff/37001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_ui_prefs.cc (right):

https://codereview.chromium.org/14924002/diff/37001/chrome/browser/ui/browser...
chrome/browser/ui/browser_ui_prefs.cc:223:
user_prefs::PrefRegistrySyncable::SYNCABLE_PREF);
On 2013/05/21 15:08:29, sky wrote:
> I haven't been involved with this feature, so forgive the question. Why do we
> need prefs for these? If the user clicks the button don't we just reset the
> values and not need to persist additional preferences? For example, if the
user
> clicks the button to reset the default home page shouldn't we just reset the
> home page pref and nothing else?

The reason for preferences is mostly to be consistent with the "Clear Browsing
Data" feature, where we do the same thing. If a user regularly wants to reset
only the default search engine (e.g. because some malware keeps overriding it),
it seems good to remember that preference.

[sky, 2013-05-21 17:32:28]

Ah, ok. That makes sense. LGTM

On Tue, May 21, 2013 at 9:29 AM,  <battre@chromium.org> wrote:
>
>
https://codereview.chromium.org/14924002/diff/37001/chrome/browser/ui/browser...
> File chrome/browser/ui/browser_ui_prefs.cc (right):
>
>
https://codereview.chromium.org/14924002/diff/37001/chrome/browser/ui/browser...
> chrome/browser/ui/browser_ui_prefs.cc:223:
> user_prefs::PrefRegistrySyncable::SYNCABLE_PREF);
> On 2013/05/21 15:08:29, sky wrote:
>>
>> I haven't been involved with this feature, so forgive the question.
>
> Why do we
>>
>> need prefs for these? If the user clicks the button don't we just
>
> reset the
>>
>> values and not need to persist additional preferences? For example, if
>
> the user
>>
>> clicks the button to reset the default home page shouldn't we just
>
> reset the
>>
>> home page pref and nothing else?
>
>
> The reason for preferences is mostly to be consistent with the "Clear
> Browsing Data" feature, where we do the same thing. If a user regularly
> wants to reset only the default search engine (e.g. because some malware
> keeps overriding it), it seems good to remember that preference.
>
> https://codereview.chromium.org/14924002/

[vasilii, 2013-05-23 08:46:58]

lgtm

[battre, 2013-05-23 08:51:13]

James, could you please have another look? Thanks.

Dominic

[James Hawkins, 2013-05-23 18:26:02]

LGTM with comments.

https://codereview.chromium.org/14924002/diff/20025/chrome/browser/resources/...
File chrome/browser/resources/options/browser_options.html (right):

https://codereview.chromium.org/14924002/diff/20025/chrome/browser/resources/...
chrome/browser/resources/options/browser_options.html:731: <button
id="reset-profile-settings-button"
On 2013/05/21 09:25:08, battre wrote:
> On 2013/05/13 17:31:33, James Hawkins wrote:
> > nit: Don't append the element name to the ID, i.e., remove "-button".
> 
> How extensively do you want this to be done? It seems to be pretty common
> practice to have "...-section" ids. I think that the "-button" suffix is
pretty
> useful for distinction. Do you want me to modify the i18n label below as well?

Yes; it suffices to call it 'resetProfileSettings'.

https://codereview.chromium.org/14924002/diff/37001/chrome/browser/resources/...
File chrome/browser/resources/options/browser_options.html (right):

https://codereview.chromium.org/14924002/diff/37001/chrome/browser/resources/...
chrome/browser/resources/options/browser_options.html:724:
i18n-content="resetProfileSettingsButton">
Missing closing </button>

[Dan Beam, 2013-05-23 19:46:42]

https://chromiumcodereview.appspot.com/14924002/diff/37001/chrome/browser/res...
File chrome/browser/resources/options/reset_profile_settings_overlay.js (right):

https://chromiumcodereview.appspot.com/14924002/diff/37001/chrome/browser/res...
chrome/browser/resources/options/reset_profile_settings_overlay.js:44: for (var
i = 0; i < checkboxes.length; i++)
nit: curlies

https://chromiumcodereview.appspot.com/14924002/diff/37001/chrome/browser/res...
chrome/browser/resources/options/reset_profile_settings_overlay.js:60:
'#reset-profile-settings-content-area input[type=checkbox]');
var sel = '#reset-profile-settings-content-area input[type=checkbox]:checked';
$('reset-profile-settings-commit').disabled =
    !!document.querySelector(sel);

https://chromiumcodereview.appspot.com/14924002/diff/37001/chrome/browser/ui/...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://chromiumcodereview.appspot.com/14924002/diff/37001/chrome/browser/ui/...
chrome/browser/ui/webui/options/browser_options_handler.cc:288: {
"resetProfileSettingsButton", IDS_RESET_PROFILE_SETTINGS_BUTTON},
nit: s/}/ }/g

https://chromiumcodereview.appspot.com/14924002/diff/37001/chrome/browser/ui/...
File chrome/browser/ui/webui/options/profile_settings_reset_browsertest.js
(right):

https://chromiumcodereview.appspot.com/14924002/diff/37001/chrome/browser/ui/...
chrome/browser/ui/webui/options/profile_settings_reset_browsertest.js:16: *
Browse to the password manager.
^ fix

https://chromiumcodereview.appspot.com/14924002/diff/37001/chrome/browser/ui/...
File chrome/browser/ui/webui/options/reset_profile_settings_handler.cc (right):

https://chromiumcodereview.appspot.com/14924002/diff/37001/chrome/browser/ui/...
chrome/browser/ui/webui/options/reset_profile_settings_handler.cc:28: :
resetter_(NULL) {
^ not necessary

https://chromiumcodereview.appspot.com/14924002/diff/37001/chrome/browser/ui/...
chrome/browser/ui/webui/options/reset_profile_settings_handler.cc:63: ListValue*
reset_extensions_handling = new ListValue;
nit: scoped_ptr<ListValue> ptr -> ptr.release()

https://chromiumcodereview.appspot.com/14924002/diff/37001/chrome/browser/ui/...
chrome/browser/ui/webui/options/reset_profile_settings_handler.cc:76: ListValue*
option = new ListValue();
same

https://chromiumcodereview.appspot.com/14924002/diff/37001/chrome/browser/ui/...
chrome/browser/ui/webui/options/reset_profile_settings_handler.cc:92: const
ListValue* /*value*/) {
why commented?

https://chromiumcodereview.appspot.com/14924002/diff/37001/chrome/browser/ui/...
chrome/browser/ui/webui/options/reset_profile_settings_handler.cc:105:
{prefs::kResetDefaultSearchEngine, ProfileResetter::DEFAULT_SEARCH_ENGINE},
nit: { prefs:: ... ENGINE } (spaces before or after curlies)

https://chromiumcodereview.appspot.com/14924002/diff/37001/chrome/browser/ui/...
chrome/browser/ui/webui/options/reset_profile_settings_handler.cc:117:
(prefs->GetInteger(prefs::kResetExtensionsHandling) == 0)
nit: drop ()

https://chromiumcodereview.appspot.com/14924002/diff/37001/chrome/browser/ui/...
File chrome/browser/ui/webui/options/reset_profile_settings_handler.h (right):

https://chromiumcodereview.appspot.com/14924002/diff/37001/chrome/browser/ui/...
chrome/browser/ui/webui/options/reset_profile_settings_handler.h:1: // Copyright
(c) 2013 The Chromium Authors. All rights reserved.
nit: no (c)

[James Hawkins, 2013-05-23 20:06:42]

https://chromiumcodereview.appspot.com/14924002/diff/37001/chrome/browser/res...
File chrome/browser/resources/options/reset_profile_settings_overlay.js (right):

https://chromiumcodereview.appspot.com/14924002/diff/37001/chrome/browser/res...
chrome/browser/resources/options/reset_profile_settings_overlay.js:44: for (var
i = 0; i < checkboxes.length; i++)
On 2013/05/23 19:46:43, Dan Beam wrote:
> nit: curlies

Not required for single line blocks.

[Dan Beam, 2013-05-23 21:37:33]

https://chromiumcodereview.appspot.com/14924002/diff/37001/chrome/browser/res...
File chrome/browser/resources/options/reset_profile_settings_overlay.js (right):

https://chromiumcodereview.appspot.com/14924002/diff/37001/chrome/browser/res...
chrome/browser/resources/options/reset_profile_settings_overlay.js:44: for (var
i = 0; i < checkboxes.length; i++)
On 2013/05/23 20:06:42, James Hawkins wrote:
> On 2013/05/23 19:46:43, Dan Beam wrote:
> > nit: curlies
> 
> Not required for single line blocks.

I think that only applies to conditionals --
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Condit...

either way it's not a big deal

[battre, 2013-05-24 09:15:46]

https://codereview.chromium.org/14924002/diff/20025/chrome/browser/resources/...
File chrome/browser/resources/options/browser_options.html (right):

https://codereview.chromium.org/14924002/diff/20025/chrome/browser/resources/...
chrome/browser/resources/options/browser_options.html:731: <button
id="reset-profile-settings-button"
On 2013/05/23 18:26:02, James Hawkins wrote:
> On 2013/05/21 09:25:08, battre wrote:
> > On 2013/05/13 17:31:33, James Hawkins wrote:
> > > nit: Don't append the element name to the ID, i.e., remove "-button".
> > 
> > How extensively do you want this to be done? It seems to be pretty common
> > practice to have "...-section" ids. I think that the "-button" suffix is
> pretty
> > useful for distinction. Do you want me to modify the i18n label below as
well?
> 
> Yes; it suffices to call it 'resetProfileSettings'.

Done.

https://codereview.chromium.org/14924002/diff/37001/chrome/browser/resources/...
File chrome/browser/resources/options/browser_options.html (right):

https://codereview.chromium.org/14924002/diff/37001/chrome/browser/resources/...
chrome/browser/resources/options/browser_options.html:724:
i18n-content="resetProfileSettingsButton">
On 2013/05/23 18:26:02, James Hawkins wrote:
> Missing closing </button>

Done.

https://codereview.chromium.org/14924002/diff/37001/chrome/browser/resources/...
File chrome/browser/resources/options/reset_profile_settings_overlay.js (right):

https://codereview.chromium.org/14924002/diff/37001/chrome/browser/resources/...
chrome/browser/resources/options/reset_profile_settings_overlay.js:60:
'#reset-profile-settings-content-area input[type=checkbox]');
On 2013/05/23 19:46:43, Dan Beam wrote:
> var sel = '#reset-profile-settings-content-area input[type=checkbox]:checked';
> $('reset-profile-settings-commit').disabled =
>     !!document.querySelector(sel);

Done.

https://codereview.chromium.org/14924002/diff/37001/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/14924002/diff/37001/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/browser_options_handler.cc:288: {
"resetProfileSettingsButton", IDS_RESET_PROFILE_SETTINGS_BUTTON},
On 2013/05/23 19:46:43, Dan Beam wrote:
> nit: s/}/ }/g

Done.

https://codereview.chromium.org/14924002/diff/37001/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/profile_settings_reset_browsertest.js
(right):

https://codereview.chromium.org/14924002/diff/37001/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/profile_settings_reset_browsertest.js:16: *
Browse to the password manager.
On 2013/05/23 19:46:43, Dan Beam wrote:
> ^ fix

Done.

https://codereview.chromium.org/14924002/diff/37001/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/reset_profile_settings_handler.cc (right):

https://codereview.chromium.org/14924002/diff/37001/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/reset_profile_settings_handler.cc:28: :
resetter_(NULL) {
On 2013/05/23 19:46:43, Dan Beam wrote:
> ^ not necessary

Done.

https://codereview.chromium.org/14924002/diff/37001/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/reset_profile_settings_handler.cc:63: ListValue*
reset_extensions_handling = new ListValue;
On 2013/05/23 19:46:43, Dan Beam wrote:
> nit: scoped_ptr<ListValue> ptr -> ptr.release()

Done.

https://codereview.chromium.org/14924002/diff/37001/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/reset_profile_settings_handler.cc:76: ListValue*
option = new ListValue();
On 2013/05/23 19:46:43, Dan Beam wrote:
> same

Done.

https://codereview.chromium.org/14924002/diff/37001/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/reset_profile_settings_handler.cc:92: const
ListValue* /*value*/) {
On 2013/05/23 19:46:43, Dan Beam wrote:
> why commented?

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
asks to comment out unused parameters.

https://codereview.chromium.org/14924002/diff/37001/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/reset_profile_settings_handler.cc:105:
{prefs::kResetDefaultSearchEngine, ProfileResetter::DEFAULT_SEARCH_ENGINE},
On 2013/05/23 19:46:43, Dan Beam wrote:
> nit: { prefs:: ... ENGINE } (spaces before or after curlies)

Done.

https://codereview.chromium.org/14924002/diff/37001/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/reset_profile_settings_handler.cc:117:
(prefs->GetInteger(prefs::kResetExtensionsHandling) == 0)
On 2013/05/23 19:46:43, Dan Beam wrote:
> nit: drop ()

I think this is a case of "Feel free to insert extra parentheses judiciously
because they can be very helpful in increasing readability when used
appropriately"
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Boolea...
also
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Return...
indicates that adding paraentheses is fine.

https://codereview.chromium.org/14924002/diff/37001/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/reset_profile_settings_handler.h (right):

https://codereview.chromium.org/14924002/diff/37001/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/reset_profile_settings_handler.h:1: // Copyright
(c) 2013 The Chromium Authors. All rights reserved.
On 2013/05/23 19:46:43, Dan Beam wrote:
> nit: no (c)

Done.

[commit-bot: I haz the power, 2013-05-24 09:16:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/battre@chromium.org/14924002/60001

[commit-bot: I haz the power, 2013-05-24 11:59:39]

Change committed as 202064

[Dan Beam, 2013-05-24 19:13:03]

https://codereview.chromium.org/14924002/diff/37001/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/reset_profile_settings_handler.cc (right):

https://codereview.chromium.org/14924002/diff/37001/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/reset_profile_settings_handler.cc:92: const
ListValue* /*value*/) {
On 2013/05/24 09:15:46, battre wrote:
> On 2013/05/23 19:46:43, Dan Beam wrote:
> > why commented?
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
> asks to comment out unused parameters.

"The more you know..."

[Dan Beam, 2013-05-24 19:13:15]

lgtm