gtk: Disable the "Import" button on Import Bookmarks dialog if there is no checkbox checked.

gtk: Disable the "Import" button on Import Bookmarks dialog if there is no checkbox checked. 

BUG=30401
TEST=open Import Bookmarks dialog, uncheck all checkboxes, see if the "Import" button is disabled. 

Patch from Thiago Farina <thiago.farina@gmail.com>;

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

[tfarina (gmail-do not use), 2010-04-09 23:54:45]

Hi Evan, could you review this to me?

[Evan Martin, 2010-04-09 23:58:37]

http://codereview.chromium.org/1604025/diff/1/2
File chrome/browser/gtk/import_dialog_gtk.cc (right):

http://codereview.chromium.org/1604025/diff/1/2#newcode21
chrome/browser/gtk/import_dialog_gtk.cc:21: gboolean IsChecked(GtkWidget*
widget) {
I don't think this helps readability much -- why not just call
gtk_toggle_button_get_active() for each of the widgets?

[tfarina (gmail-do not use), 2010-04-10 00:02:50]

http://codereview.chromium.org/1604025/diff/1/2
File chrome/browser/gtk/import_dialog_gtk.cc (right):

http://codereview.chromium.org/1604025/diff/1/2#newcode21
chrome/browser/gtk/import_dialog_gtk.cc:21: gboolean IsChecked(GtkWidget*
widget) {
On 2010/04/09 23:58:38, Evan Martin wrote:
> I don't think this helps readability much -- why not just call
> gtk_toggle_button_get_active() for each of the widgets?

I think this is much better to read and understand. And is much less than
gtk_toggle_button_get_active(GTK_TOGGLE_BUTTON(...)) compared to just
IsChecked(...).

[Evan Martin, 2010-04-10 00:04:34]

On 2010/04/10 00:02:50, tfarina wrote:
> http://codereview.chromium.org/1604025/diff/1/2
> File chrome/browser/gtk/import_dialog_gtk.cc (right):
> 
> http://codereview.chromium.org/1604025/diff/1/2#newcode21
> chrome/browser/gtk/import_dialog_gtk.cc:21: gboolean IsChecked(GtkWidget*
> widget) {
> On 2010/04/09 23:58:38, Evan Martin wrote:
> > I don't think this helps readability much -- why not just call
> > gtk_toggle_button_get_active() for each of the widgets?
> 
> I think this is much better to read and understand. And is much less than
> gtk_toggle_button_get_active(GTK_TOGGLE_BUTTON(...)) compared to just
> IsChecked(...).

But right next to it there is "gtk_dialog_set_response_sensitive"!
If IsChecked were the dominant function in the file, maybe it would make sense,
but it's only a few calls here...

[Evan Martin, 2010-04-10 00:05:41]

http://codereview.chromium.org/1604025/diff/1/2
File chrome/browser/gtk/import_dialog_gtk.cc (right):

http://codereview.chromium.org/1604025/diff/1/2#newcode201
chrome/browser/gtk/import_dialog_gtk.cc:201: IsChecked(history_);
I wonder if another way around this is refactoring GetDialogResponse's set of
tests into something like
  GetCheckedItems()
which returns a uint16.  Then here we can just test GetCheckedItems() != 0.

[tfarina (gmail-do not use), 2010-04-10 00:08:03]

On 2010/04/10 00:04:34, Evan Martin wrote:
> On 2010/04/10 00:02:50, tfarina wrote:
> > http://codereview.chromium.org/1604025/diff/1/2
> > File chrome/browser/gtk/import_dialog_gtk.cc (right):
> > 
> > http://codereview.chromium.org/1604025/diff/1/2#newcode21
> > chrome/browser/gtk/import_dialog_gtk.cc:21: gboolean IsChecked(GtkWidget*
> > widget) {
> > On 2010/04/09 23:58:38, Evan Martin wrote:
> > > I don't think this helps readability much -- why not just call
> > > gtk_toggle_button_get_active() for each of the widgets?
> > 
> > I think this is much better to read and understand. And is much less than
> > gtk_toggle_button_get_active(GTK_TOGGLE_BUTTON(...)) compared to just
> > IsChecked(...).
> 
> But right next to it there is "gtk_dialog_set_response_sensitive"!
> If IsChecked were the dominant function in the file, maybe it would make
sense,
> but it's only a few calls here...

OK, if you don't like the IsChecked I will revert this change. But just compare:

gboolean enabled = IsChecked(bookmarks_) ||
                   IsChecked(search_engines_) ||
                   IsChecked(passwords_) ||
                   IsChecked(history_);

to this:
gboolean enabled =  gtk_toggle_button_get_active(GTK_TOGGLE_BUTTON((bookmarks_))
||                  
gtk_toggle_button_get_active(GTK_TOGGLE_BUTTON((search_engines_)) ||
gtk_toggle_button_get_active(GTK_TOGGLE_BUTTON(passwords_)) ||
gtk_toggle_button_get_active(GTK_TOGGLE_BUTTON(history_));

[tfarina (gmail-do not use), 2010-04-10 00:11:44]

On 2010/04/10 00:05:41, Evan Martin wrote:
> http://codereview.chromium.org/1604025/diff/1/2
> File chrome/browser/gtk/import_dialog_gtk.cc (right):
> 
> http://codereview.chromium.org/1604025/diff/1/2#newcode201
> chrome/browser/gtk/import_dialog_gtk.cc:201: IsChecked(history_);
> I wonder if another way around this is refactoring GetDialogResponse's set of
> tests into something like
>   GetCheckedItems()
> which returns a uint16.  Then here we can just test GetCheckedItems() != 0.

There is a function like that in chrome/browser/views/importer_view.cc
Is like this function you have in mind?

[tfarina (gmail-do not use), 2010-04-10 00:29:25]

On 2010/04/10 00:08:03, tfarina wrote:
> On 2010/04/10 00:04:34, Evan Martin wrote:
> > On 2010/04/10 00:02:50, tfarina wrote:
> > > http://codereview.chromium.org/1604025/diff/1/2
> > > File chrome/browser/gtk/import_dialog_gtk.cc (right):
> > > 
> > > http://codereview.chromium.org/1604025/diff/1/2#newcode21
> > > chrome/browser/gtk/import_dialog_gtk.cc:21: gboolean IsChecked(GtkWidget*
> > > widget) {
> > > On 2010/04/09 23:58:38, Evan Martin wrote:
> > > > I don't think this helps readability much -- why not just call
> > > > gtk_toggle_button_get_active() for each of the widgets?
> > > 
> > > I think this is much better to read and understand. And is much less than
> > > gtk_toggle_button_get_active(GTK_TOGGLE_BUTTON(...)) compared to just
> > > IsChecked(...).
> > 
> > But right next to it there is "gtk_dialog_set_response_sensitive"!
> > If IsChecked were the dominant function in the file, maybe it would make
> sense,
> > but it's only a few calls here...
> 
> OK, if you don't like the IsChecked I will revert this change.
Sorry, I mean, I will use gtk_toggle_button_get_active(GTK_TOGGLE_BUTTON(...))
instead. :)

[Evan Martin, 2010-04-10 00:32:11]

On 2010/04/10 00:11:44, tfarina wrote:
> On 2010/04/10 00:05:41, Evan Martin wrote:
> > http://codereview.chromium.org/1604025/diff/1/2
> > File chrome/browser/gtk/import_dialog_gtk.cc (right):
> > 
> > http://codereview.chromium.org/1604025/diff/1/2#newcode201
> > chrome/browser/gtk/import_dialog_gtk.cc:201: IsChecked(history_);
> > I wonder if another way around this is refactoring GetDialogResponse's set
of
> > tests into something like
> >   GetCheckedItems()
> > which returns a uint16.  Then here we can just test GetCheckedItems() != 0.
> 
> There is a function like that in chrome/browser/views/importer_view.cc
> Is like this function you have in mind?

Ah, yes, exactly!

[tfarina (gmail-do not use), 2010-04-10 03:59:59]

On 2010/04/10 00:32:11, Evan Martin wrote:
> > > I wonder if another way around this is refactoring GetDialogResponse's set
> of
> > > tests into something like
> > >   GetCheckedItems()
> > > which returns a uint16.  Then here we can just test GetCheckedItems() !=
0.
> > 
Hi Evan, I did in this way, please take another look.

[tfarina (gmail-do not use), 2010-04-16 17:40:12]

Ping!

[Evan Martin, 2010-04-16 17:44:49]

http://codereview.chromium.org/1604025/diff/11001/12001
File chrome/browser/gtk/import_dialog_gtk.cc (right):

http://codereview.chromium.org/1604025/diff/11001/12001#newcode177
chrome/browser/gtk/import_dialog_gtk.cc:177: items |= importer::HISTORY;
I think you meant to call GetCheckedItems instead of this list of if statements
here :)

[tfarina (gmail-do not use), 2010-04-17 19:57:16]

http://codereview.chromium.org/1604025/diff/11001/12001
File chrome/browser/gtk/import_dialog_gtk.cc (right):

http://codereview.chromium.org/1604025/diff/11001/12001#newcode177
chrome/browser/gtk/import_dialog_gtk.cc:177: items |= importer::HISTORY;
On 2010/04/16 17:44:49, Evan Martin wrote:
> I think you meant to call GetCheckedItems instead of this list of if
statements
> here :)
Good catch (I'm so lazy :)).
Done.

[Evan Martin, 2010-04-17 21:26:19]

LGTM

[tfarina (gmail-do not use), 2010-04-20 03:01:30]

On 2010/04/17 21:26:19, Evan Martin wrote:
> LGTM

Could you land this to me?

[tfarina (gmail-do not use), 2010-04-26 21:22:06]

Ping!

[Evan Martin, 2010-04-26 23:50:39]

On 2010/04/26 21:22:06, tfarina wrote:
> Ping!

LGTM, committed