Simplify the gtk AutoFill dialog and bring it more in line with the mocks....

Simplify the gtk AutoFill dialog and bring it more in line with the mocks.

BUG=none
TEST=none

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

[mattm, 2010-01-15 00:49:59]

could you also send a screenshot or two of what the new dialog looks like?

http://codereview.chromium.org/551034/diff/1/4
File chrome/browser/autofill/autofill_dialog_gtk.cc (right):

http://codereview.chromium.org/551034/diff/1/4#newcode28
chrome/browser/autofill/autofill_dialog_gtk.cc:28: const int
kAutoFillDialogIndent = 5;
Where does 5 come from?  Generally we try to follow the Gnome HIG in laying out
dialogs
(http://library.gnome.org/devel/hig-book/stable/design-window.html.en#layout-d...)

http://codereview.chromium.org/551034/diff/1/4#newcode121
chrome/browser/autofill/autofill_dialog_gtk.cc:121: GTK_RESPONSE_APPLY,
I thought we generally preferred settings being immediately applied over this
cancel/apply?

http://codereview.chromium.org/551034/diff/1/4#newcode121
chrome/browser/autofill/autofill_dialog_gtk.cc:121: GTK_RESPONSE_APPLY,
Also isn't it usually cancel/apply/ok?

http://codereview.chromium.org/551034/diff/1/4#newcode139
chrome/browser/autofill/autofill_dialog_gtk.cc:139:
gtk_widget_modify_bg(frame_event_box, GTK_STATE_NORMAL, &gfx::kGdkWhite);
This isn't going to play well with some gtk themes.

Best bet is to choose some widget that has colors you want to emulate and get
the styles from it (both fg and bg) and set those on your widgets.
Ex: CookiesView::InitStylesAndShow in
/chrome/browser/gtk/options/cookies_view.cc

http://codereview.chromium.org/551034/diff/1/4#newcode153
chrome/browser/autofill/autofill_dialog_gtk.cc:153:
gtk_box_pack_start_defaults(GTK_BOX(outer_vbox), addresses_vbox_);
The new design seems to assume that you'll never add enough entries to make the
dialog too tall to fit in the window? Should there still be a scrolled window
around outer_vbox or something?

http://codereview.chromium.org/551034/diff/1/4#newcode188
chrome/browser/autofill/autofill_dialog_gtk.cc:188: new_address, position);
Is the reordering just that you want it to go at the end of the list but before
the add button?  You could just add the button with pack_end instead of
pack_start, then you wouldn't need to reorder anything here.

http://codereview.chromium.org/551034/diff/1/9
File chrome/common/gtk_util.h (right):

http://codereview.chromium.org/551034/diff/1/9#newcode91
chrome/common/gtk_util.h:91: int width_id, int height_id, bool resizable);
fyi, looks like there are a number of places doing GetWidgetSizeFromResources &
gtk_window_set_default_size that could be converted to use this, if you want to
update those too.

[James Hawkins, 2010-01-15 02:20:30]

http://codereview.chromium.org/551034/diff/1/4
File chrome/browser/autofill/autofill_dialog_gtk.cc (right):

http://codereview.chromium.org/551034/diff/1/4#newcode28
chrome/browser/autofill/autofill_dialog_gtk.cc:28: const int
kAutoFillDialogIndent = 5;
On 2010/01/15 00:49:59, mattm wrote:
> Where does 5 come from?  Generally we try to follow the Gnome HIG in laying
out
> dialogs
>
(http://library.gnome.org/devel/hig-book/stable/design-window.html.en#layout-d...)

This dialog follows the hig for control and content spacing.  The indentation is
to match the mock.  The label, expander arrow, and button edge all line up.

http://codereview.chromium.org/551034/diff/1/4#newcode121
chrome/browser/autofill/autofill_dialog_gtk.cc:121: GTK_RESPONSE_APPLY,
On 2010/01/15 00:49:59, mattm wrote:
> I thought we generally preferred settings being immediately applied over this
> cancel/apply?
> 

The thinking (from the UI group) is that we're dealing with a lot of information
in this dialog, most of which is not memorized and painstaking to enter.  If the
user accidentally deletes a profile, we need a way to cancel the save.

http://codereview.chromium.org/551034/diff/1/4#newcode121
chrome/browser/autofill/autofill_dialog_gtk.cc:121: GTK_RESPONSE_APPLY,
On 2010/01/15 00:49:59, mattm wrote:
> Also isn't it usually cancel/apply/ok?

I'm just following the mocks on this one.  Also, I don't see the need for an OK
button.

http://codereview.chromium.org/551034/diff/1/4#newcode139
chrome/browser/autofill/autofill_dialog_gtk.cc:139:
gtk_widget_modify_bg(frame_event_box, GTK_STATE_NORMAL, &gfx::kGdkWhite);
On 2010/01/15 00:49:59, mattm wrote:
> This isn't going to play well with some gtk themes.
> 
> Best bet is to choose some widget that has colors you want to emulate and get
> the styles from it (both fg and bg) and set those on your widgets.
> Ex: CookiesView::InitStylesAndShow in
> /chrome/browser/gtk/options/cookies_view.cc

Done.

http://codereview.chromium.org/551034/diff/1/4#newcode153
chrome/browser/autofill/autofill_dialog_gtk.cc:153:
gtk_box_pack_start_defaults(GTK_BOX(outer_vbox), addresses_vbox_);
On 2010/01/15 00:49:59, mattm wrote:
> The new design seems to assume that you'll never add enough entries to make
the
> dialog too tall to fit in the window? Should there still be a scrolled window
> around outer_vbox or something?

I was going to add it later, but it was simple enough to add to this CL.

http://codereview.chromium.org/551034/diff/1/4#newcode188
chrome/browser/autofill/autofill_dialog_gtk.cc:188: new_address, position);
On 2010/01/15 00:49:59, mattm wrote:
> Is the reordering just that you want it to go at the end of the list but
before
> the add button?  You could just add the button with pack_end instead of
> pack_start, then you wouldn't need to reorder anything here.

Duh, silly me.  I've fixed this.

http://codereview.chromium.org/551034/diff/1/9
File chrome/common/gtk_util.h (right):

http://codereview.chromium.org/551034/diff/1/9#newcode91
chrome/common/gtk_util.h:91: int width_id, int height_id, bool resizable);
On 2010/01/15 00:49:59, mattm wrote:
> fyi, looks like there are a number of places doing GetWidgetSizeFromResources
&
> gtk_window_set_default_size that could be converted to use this, if you want
to
> update those too.

Done.

[mattm, 2010-01-15 02:40:45]

http://codereview.chromium.org/551034/diff/1/4
File chrome/browser/autofill/autofill_dialog_gtk.cc (right):

http://codereview.chromium.org/551034/diff/1/4#newcode28
chrome/browser/autofill/autofill_dialog_gtk.cc:28: const int
kAutoFillDialogIndent = 5;
On 2010/01/15 02:20:30, James Hawkins wrote:
> On 2010/01/15 00:49:59, mattm wrote:
> > Where does 5 come from?  Generally we try to follow the Gnome HIG in laying
> out
> > dialogs
> >
>
(http://library.gnome.org/devel/hig-book/stable/design-window.html.en#layout-d...)
> 
> This dialog follows the hig for control and content spacing.  The indentation
is
> to match the mock.  The label, expander arrow, and button edge all line up.

Well, just that HIG usually uses multiples of 6

http://codereview.chromium.org/551034/diff/1/4#newcode121
chrome/browser/autofill/autofill_dialog_gtk.cc:121: GTK_RESPONSE_APPLY,
On 2010/01/15 02:20:30, James Hawkins wrote:
> On 2010/01/15 00:49:59, mattm wrote:
> > Also isn't it usually cancel/apply/ok?
> 
> I'm just following the mocks on this one.  Also, I don't see the need for an
OK
> button.

Well, Apply is always "apply and leave window open" and Ok is "apply and close
window".   Having only Apply seems weird.

Here's the HIG entry on it:
http://library.gnome.org/devel/hig-book/stable/windows-utility.html.en#window...

http://codereview.chromium.org/551034/diff/1/4#newcode121
chrome/browser/autofill/autofill_dialog_gtk.cc:121: GTK_RESPONSE_APPLY,
On 2010/01/15 02:20:30, James Hawkins wrote:
> On 2010/01/15 00:49:59, mattm wrote:
> > I thought we generally preferred settings being immediately applied over
this
> > cancel/apply?
> > 
> 
> The thinking (from the UI group) is that we're dealing with a lot of
information
> in this dialog, most of which is not memorized and painstaking to enter.  If
the
> user accidentally deletes a profile, we need a way to cancel the save.

Ok

http://codereview.chromium.org/551034/diff/18/5003#newcode44
chrome/browser/autofill/autofill_dialog_gtk.cc:44: 
I think the widget needs to be realized for the style to be initialized (or at
least gtk_widget_ensure_style? Haven't ever tried that)

[James Hawkins, 2010-01-15 18:37:07]

http://codereview.chromium.org/551034/diff/1/4
File chrome/browser/autofill/autofill_dialog_gtk.cc (right):

http://codereview.chromium.org/551034/diff/1/4#newcode28
chrome/browser/autofill/autofill_dialog_gtk.cc:28: const int
kAutoFillDialogIndent = 5;
On 2010/01/15 02:40:45, mattm wrote:
> On 2010/01/15 02:20:30, James Hawkins wrote:
> > On 2010/01/15 00:49:59, mattm wrote:
> > > Where does 5 come from?  Generally we try to follow the Gnome HIG in
laying
> > out
> > > dialogs
> > >
> >
>
(http://library.gnome.org/devel/hig-book/stable/design-window.html.en#layout-d...)
> > 
> > This dialog follows the hig for control and content spacing.  The
indentation
> is
> > to match the mock.  The label, expander arrow, and button edge all line up.
> 
> Well, just that HIG usually uses multiples of 6

I understand, but I believe this is not a case that really applies.  The
indentation is singular, as there are no other widgets on the same horizontal
plane for these, and it looks rather ugly if the widgets don't line up.  We
can't change how far out the arrow on the expander widget is placed.

http://codereview.chromium.org/551034/diff/1/4#newcode121
chrome/browser/autofill/autofill_dialog_gtk.cc:121: GTK_RESPONSE_APPLY,
On 2010/01/15 02:40:45, mattm wrote:
> On 2010/01/15 02:20:30, James Hawkins wrote:
> > On 2010/01/15 00:49:59, mattm wrote:
> > > Also isn't it usually cancel/apply/ok?
> > 
> > I'm just following the mocks on this one.  Also, I don't see the need for an
> OK
> > button.
> 
> Well, Apply is always "apply and leave window open" and Ok is "apply and close
> window".   Having only Apply seems weird.
> 
> Here's the HIG entry on it:
>
http://library.gnome.org/devel/hig-book/stable/windows-utility.html.en#window...
> 
> 

Done.

http://codereview.chromium.org/551034/diff/18/5003#newcode44
chrome/browser/autofill/autofill_dialog_gtk.cc:44: 
On 2010/01/15 02:40:45, mattm wrote:
> I think the widget needs to be realized for the style to be initialized (or at
> least gtk_widget_ensure_style? Haven't ever tried that)

I used gtk_widget_ensure_style.  We can't realize the widget because it's not
anchored, and we don't have an easy way to anchor a random widget.

[mattm, 2010-01-15 20:44:51]

lgtm

http://codereview.chromium.org/551034/diff/1/4
File chrome/browser/autofill/autofill_dialog_gtk.cc (right):

http://codereview.chromium.org/551034/diff/1/4#newcode28
chrome/browser/autofill/autofill_dialog_gtk.cc:28: const int
kAutoFillDialogIndent = 5;
On 2010/01/15 18:37:07, James Hawkins wrote:
> On 2010/01/15 02:40:45, mattm wrote:
> > On 2010/01/15 02:20:30, James Hawkins wrote:
> > > On 2010/01/15 00:49:59, mattm wrote:
> > > > Where does 5 come from?  Generally we try to follow the Gnome HIG in
> laying
> > > out
> > > > dialogs
> > > >
> > >
> >
>
(http://library.gnome.org/devel/hig-book/stable/design-window.html.en#layout-d...)
> > > 
> > > This dialog follows the hig for control and content spacing.  The
> indentation
> > is
> > > to match the mock.  The label, expander arrow, and button edge all line
up.
> > 
> > Well, just that HIG usually uses multiples of 6
> 
> I understand, but I believe this is not a case that really applies.  The
> indentation is singular, as there are no other widgets on the same horizontal
> plane for these, and it looks rather ugly if the widgets don't line up.  We
> can't change how far out the arrow on the expander widget is placed.

Ah, missed that it was to align with the expander.  Still a little uneasy that
it might depend on the theme, but I guess there isn't really a good way to deal
with this.

http://codereview.chromium.org/551034/diff/7002/4004#newcode125
chrome/browser/autofill/autofill_dialog_gtk.cc:125:
gtk_util::SetWindowSizeFromResources(GTK_WINDOW(dialog_),
order should be apply, cancel, ok

[James Hawkins, 2010-01-15 20:53:22]

http://codereview.chromium.org/551034/diff/7002/4004
File chrome/browser/autofill/autofill_dialog_gtk.cc (right):

http://codereview.chromium.org/551034/diff/7002/4004#newcode125
chrome/browser/autofill/autofill_dialog_gtk.cc:125: GTK_RESPONSE_OK,
On 2010/01/15 20:44:51, mattm wrote:
> order should be apply, cancel, ok

Done.