Fix the issue in 'Clear browsing data' dialog with Cancel vs Close button.

Fix the issue in 'Clear browsing data' dialog with Cancel vs Close button.

We should *only* save the state of the checkboxes when the user click on the button to clear the data,
otherwise we shouldn't change the preferences of the checkboxes.

Along the way, rename the button from 'Close' to 'Cancel'.

BUG=35098
TEST=open the dialog, see the current state of the checkboxes, click 'Cancel'.
reopen the dialog, the state should be the same. Now uncheck some checkbox,
click 'Cancel', reopen the dialog the unchecked checkbox should be checked.
Open the dialog again, uncheck a checkbox and click 'Clear browsing data',
reopen the dialog, that checkbox should be *unchecked*.

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

[Evan Martin, 2010-09-26 05:19:03]

LGTM

http://codereview.chromium.org/3389033/diff/5001/6001
File chrome/browser/gtk/clear_browsing_data_dialog_gtk.cc (right):

http://codereview.chromium.org/3389033/diff/5001/6001#newcode48
chrome/browser/gtk/clear_browsing_data_dialog_gtk.cc:48: GtkWidget* close_button
= gtk_dialog_add_button(GTK_DIALOG(dialog_),
maybe rename this variable to cancel_button now?

[tfarina, 2010-09-26 06:59:02]

http://codereview.chromium.org/3389033/diff/5001/6001
File chrome/browser/gtk/clear_browsing_data_dialog_gtk.cc (right):

http://codereview.chromium.org/3389033/diff/5001/6001#newcode48
chrome/browser/gtk/clear_browsing_data_dialog_gtk.cc:48: GtkWidget* close_button
= gtk_dialog_add_button(GTK_DIALOG(dialog_),
On 2010/09/26 05:19:03, Evan Martin wrote:
> maybe rename this variable to cancel_button now?

Done.

[Peter Kasting, 2010-09-27 17:02:52]

LGTM

http://codereview.chromium.org/3389033/diff/10001/11001
File chrome/browser/gtk/clear_browsing_data_dialog_gtk.cc (right):

http://codereview.chromium.org/3389033/diff/10001/11001#newcode48
chrome/browser/gtk/clear_browsing_data_dialog_gtk.cc:48: GtkWidget*
cancel_button = gtk_dialog_add_button(GTK_DIALOG(dialog_),
Nit: The old wrapping was fine.

http://codereview.chromium.org/3389033/diff/10001/11001#newcode192
chrome/browser/gtk/clear_browsing_data_dialog_gtk.cc:192:
profile_->GetPrefs()->SetBoolean(prefs::kDeleteBrowsingHistory,
Nit: Factor the pref service out to a temp so you aren't calling GetPrefs()
every time.

http://codereview.chromium.org/3389033/diff/10001/11002
File chrome/browser/views/clear_browsing_data.cc (right):

http://codereview.chromium.org/3389033/diff/10001/11002#newcode246
chrome/browser/views/clear_browsing_data.cc:246: } else if (button ==
MessageBoxFlags::DIALOGBUTTON_CANCEL) {
Nit: While here, get rid of "else" after "return".

In fact, I'd just simplify this as:

  DCHECK((button == MessageBoxFlags::DIALOGBUTTON_OK) ||
         (button == MessageBoxFlags::DIALOGBUTTON_CANCEL));
  return l10n_util::GetString((button == MessageBoxFlags::DIALOGBUTTON_OK) ?
      IDS_CLEAR_BROWSING_DATA_COMMIT : IDS_CANCEL);

http://codereview.chromium.org/3389033/diff/10001/11002#newcode298
chrome/browser/views/clear_browsing_data.cc:298:
profile_->GetPrefs()->SetBoolean(prefs::kDeleteBrowsingHistory,
Nit: Factor the prefservice out to a temp so you aren't calling GetPrefs() on
every line.

[tfarina, 2010-09-28 02:53:26]

http://codereview.chromium.org/3389033/diff/10001/11001
File chrome/browser/gtk/clear_browsing_data_dialog_gtk.cc (right):

http://codereview.chromium.org/3389033/diff/10001/11001#newcode48
chrome/browser/gtk/clear_browsing_data_dialog_gtk.cc:48: GtkWidget*
cancel_button = gtk_dialog_add_button(GTK_DIALOG(dialog_),
On 2010/09/27 17:02:53, Peter Kasting wrote:
> Nit: The old wrapping was fine.

Done.

http://codereview.chromium.org/3389033/diff/10001/11001#newcode192
chrome/browser/gtk/clear_browsing_data_dialog_gtk.cc:192:
profile_->GetPrefs()->SetBoolean(prefs::kDeleteBrowsingHistory,
On 2010/09/27 17:02:53, Peter Kasting wrote:
> Nit: Factor the pref service out to a temp so you aren't calling GetPrefs()
> every time.

Done.

http://codereview.chromium.org/3389033/diff/10001/11002
File chrome/browser/views/clear_browsing_data.cc (right):

http://codereview.chromium.org/3389033/diff/10001/11002#newcode246
chrome/browser/views/clear_browsing_data.cc:246: } else if (button ==
MessageBoxFlags::DIALOGBUTTON_CANCEL) {
On 2010/09/27 17:02:53, Peter Kasting wrote:
> Nit: While here, get rid of "else" after "return".
> 
> In fact, I'd just simplify this as:
> 
>   DCHECK((button == MessageBoxFlags::DIALOGBUTTON_OK) ||
>          (button == MessageBoxFlags::DIALOGBUTTON_CANCEL));
>   return l10n_util::GetString((button == MessageBoxFlags::DIALOGBUTTON_OK) ?
>       IDS_CLEAR_BROWSING_DATA_COMMIT : IDS_CANCEL);

Done.

http://codereview.chromium.org/3389033/diff/10001/11002#newcode298
chrome/browser/views/clear_browsing_data.cc:298:
profile_->GetPrefs()->SetBoolean(prefs::kDeleteBrowsingHistory,
On 2010/09/27 17:02:53, Peter Kasting wrote:
> Nit: Factor the prefservice out to a temp so you aren't calling GetPrefs() on
> every line.

Done.

[twittermoo_googlemail.com, 2010-10-09 02:08:27]

> We should *only* save the state of the 
> checkboxes when the user click on 
> the button to clear the data, 
> otherwise we shouldn't change 
> the preferences of the checkboxes.

Please explain your logic? We think this is illogical, preferences must be saved
when user click on [X][Close] button or press Alt+F4.  

Now the users of the Click&Clean extension can't clean up!
https://chrome.google.com/extensions/detail/ghgabhipcejejjmhhchfonmamedcbeod
They can't change "Clear Browsing Data" options, Click&Clean can't save
preferences without run "Clear Browsing Data"!
Preferences must be saved when user click on [X] or [Close] button or press
Alt+F4.

[tfarina, 2010-10-09 02:11:40]

On 2010/10/09 02:08:27, twittermoo wrote:
> > We should *only* save the state of the 
> > checkboxes when the user click on 
> > the button to clear the data, 
> > otherwise we shouldn't change 
> > the preferences of the checkboxes.
> 
> Please explain your logic? We think this is illogical

It is illogical saving the state of the checkboxes when clicking in Cancel.

Also this is not the right place to discuss these things. If you think it's
wrong, please file a bug at crbug.com/new.

[tfarina, 2010-10-09 02:43:34]

On Fri, Oct 8, 2010 at 11:37 PM, Mixesoft <omnisat.support@gmail.com> wrote:
>  Hello,
>
>> It is illogical saving the state of the checkboxes when clicking in
>> Cancel.
>
> Your logic does not coincide with the logic in the options. Try to change
> any settings in
> the Options dialog and close it. All settings are saved. You understand that
> you've done?

Yes, and that is the expected behavior. When I click on Cancel I don't
expect that the changes I did on the dialog should be preserved, since
I'm CANCELING what I have done. And that is what the BUG is saying.

> Now many, many users,  can't save settings!
>
Sorry about that.



-- 
A: Because it messes up the order in which people normally read text.
 See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

-Thiago

[Peter Kasting, 2010-10-09 03:27:16]

Please do not harass Thiago for implementing the design we specified on bug
35098 comment 5.  If you want someone to blame, you can blame me.  I have a
thick skin, I don't mind.  Just don't do that here.  Code review threads are for
reviewing code, not for debating things.  I suggest whining on
chromium-discuss@chromium.org if you desperately need an outlet.

You can file bugs if you'd like, but I will dupe them against bug 58592.  This
behavior is intentional (and has been long needing a fix), not a bug.

[tfarina, 2010-10-09 03:29:31]

On Sat, Oct 9, 2010 at 12:27 AM,  <pkasting@chromium.org> wrote:
> You can file bugs if you'd like, but I will dupe them against bug 58592.
>  This
> behavior is intentional (and has been long needing a fix), not a bug.
>

He filed a bug here http://code.google.com/p/chromium/issues/detail?id=58593

-- 
A: Because it messes up the order in which people normally read text.
 See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

-Thiago

[twittermoo_googlemail.com, 2010-10-09 03:32:20]

Fix the issue in 'Clear browsing data' dialog when user click [x] - Close button
or press ALT+F4:
http://code.google.com/p/chromium/issues/detail?id=58592