Google Chrome doesn't remember settings for 'Clear browing data'. Added preferences for the clear b

Google Chrome doesn't remember settings for 'Clear browing data'. 
Added preferences for the clear browsing data so that it would be persistant.

Modified the code to meet coding styles.
Added a listener to the check boxes for change of state
changed comment
Added name to authors

BUG=1618

[tony, 2008-09-12 20:15:13]

http://codereview.chromium.org/3014/diff/1/2
File chrome/browser/views/clear_browsing_data.cc (right):

http://codereview.chromium.org/3014/diff/1/2#newcode378
Line 378:
profile_->GetPrefs()->SetBoolean(prefs::kDelBrowsingHistory,remove_mask  &
BrowsingDataRemover::REMOVE_HISTORY ? true : false);
It would be nice to only set this if the user changes the value.  Otherwise, we
write the defaults to the prefs file, which seems unnecessary.

So rather than setting the prefs here, you could add a listener to the
checkboxes and set the pref when there is a change to the checkbox.

http://codereview.chromium.org/3014/diff/1/4
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/3014/diff/1/4#newcode202
Line 202: const wchar_t kDelPasswords[] = L"browser.del_passwords";
Can you write out the word delete rather than using del_?  Also, you may just
want to nest this (e.g., browser.clear_data.*).

[Finnur, 2008-09-12 20:39:44]

I agree with Tony and have a few nits to add...

http://codereview.chromium.org/3014/diff/1/3
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/3014/diff/1/3#newcode181
Line 181: prefs->RegisterBooleanPref(prefs::kDelBrowsingHistory, true);
This constant should be named kDeleteBrowsingHistoryDefaultSetting, or something
like that (Same with the others)

http://codereview.chromium.org/3014/diff/1/2
File chrome/browser/views/clear_browsing_data.cc (right):

http://codereview.chromium.org/3014/diff/1/2#newcode75
Line 75: AddCheckbox(l10n_util::GetString(IDS_DEL_BROWSING_HISTORY_CHKBOX),
profile_->GetPrefs()->GetBoolean(prefs::kDelBrowsingHistory));
Our style guide states we have an 80 chars per line maximum.

http://codereview.chromium.org/3014/diff/1/2#newcode382
Line 382: profile_->GetPrefs()->SetBoolean(prefs::kDelCache,remove_mask  &
BrowsingDataRemover::REMOVE_CACHE ? true : false);
Also, you should take a look at the C++ coding style guide for Chrome. You need
a space after the comma and not two spaces before the & sign. And we have an 80
chars per line limit.

http://codereview.chromium.org/3014/diff/1/4
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/3014/diff/1/4#newcode197
Line 197: // clear browsing data options
Capitalize c and add a period at the end.

http://codereview.chromium.org/3014/diff/1/5
File chrome/common/pref_names.h (right):

http://codereview.chromium.org/3014/diff/1/5#newcode67
Line 67: // clear browsing options
Same here. Capitalize and end in period.

[Peter Kasting, 2008-09-12 20:57:03]

Note that the bug in question is still not one we've decided to actually change
anything for; in the future, I suggest holding off on patches until we agree we
want to change a particular behavior.

[developer0420, 2008-09-12 21:06:48]

On 2008/09/12 20:57:03, pkasting wrote:
> Note that the bug in question is still not one we've decided to actually
change
> anything for; in the future, I suggest holding off on patches until we agree
we
> want to change a particular behavior.


Thank you for the comments,  should I not move forward with the suggestions?

[Peter Kasting, 2008-09-13 01:15:28]

On 2008/09/12 21:06:48, developer0420 wrote:
> Thank you for the comments,  should I not move forward with the suggestions?

There's nothing wrong with updating the patch to take technical review feedback
into account, I'm just cautioning you that dealing with all the feedback doesn't
imply the patch will get landed in the end.  Just trying to prevent frustrating
and disappointment :).  (And, in the future, save your and your reviewers' time,
if possible.)

[developer0420, 2008-09-13 16:58:39]

I updated the code to meet Google C++ Style Guide

[Finnur, 2008-09-13 21:23:16]

That's good. However, you haven't addressed all the comments Tony gave you -
specifically, his suggestion about using a listener to detect when the values of
the checkboxes change (and possibly others I might have missed).

http://codereview.chromium.org/3014/diff/10/213
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/3014/diff/10/213#newcode196
Line 196: 
Don't need two empty lines, one will do just fine.

http://codereview.chromium.org/3014/diff/10/213#newcode197
Line 197: // Clear browsing data options.
This needs to be more verbose. I suggest replacing it with: "Boolean prefs that
define the default values for the checkboxes in the Clear Browsing Data dialog"

http://codereview.chromium.org/3014/diff/10/213#newcode198
Line 198: const wchar_t kDeleteBrowseHistory[] =
L"browser.clear_data.browse_history";
This should be browsing_history, not browse_history (both the variable and the
string constant).

http://codereview.chromium.org/3014/diff/10/214
File chrome/common/pref_names.h (right):

http://codereview.chromium.org/3014/diff/10/214#newcode67
Line 67: // Clear browsing data options.
Actually, remove this comment, it is duplicated from the .cc file. We only need
it there.

[developer0420, 2008-09-14 15:49:22]

I added in the listeners like Tony suggested

[Finnur, 2008-09-14 16:41:10]

Looks good as far as I am concerned. One nit below.

However, I just remembered that external contributors need to add their names to
the AUTHORS file as part of the changelist. Can you add your full name there?

http://codereview.chromium.org/3014/diff/14/223
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/3014/diff/14/223#newcode197
Line 197: // are checked.
This doesn't describe what the pref is used for, only how it gets set. I still
think this comment describes the purpose better:
"Boolean prefs that define the default values for the check boxes in the Clear
Browsing Data dialog"

[developer0420, 2008-09-15 14:24:52]

added name to authors changed comments to match suggestion.

Thanks for all help on understanding coding style, moving on to next issue.

[tony, 2008-09-15 15:59:24]

LGTM too.  I'll check this in today unless Finnur beats me to it.

[Finnur, 2008-09-17 18:47:31]

Arthur, since this has been checked in already, can you mark it as closed? 

On 2008/09/15 15:59:24, tony wrote:
> LGTM too.  I'll check this in today unless Finnur beats me to it.