GTK: HTTP Auth dialogs under linux.

GTK: HTTP Auth dialogs under linux.

- Implements a ConstrainedWindowGtk which positions itself in the center of its corresponding TabContentsViewGtk.
- Implements LoginPromptGtk. HTTP Auth now works under Linux.
- Renames ConstrainedWindowImpl to ConstrainedWindowWin

http://crbug.com/11512

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

[Elliot Glaysher, 2009-06-19 17:28:00]

I'd like one OK from a windows person and one from a linux person.

In light of the recent email from brettw, I am aware that LoginPrompt{Gtk,Win}
need a redesign since they have so much shared model/controller code. That's my
next task, but I'd prefer not to hold up that refactoring so that this feature
can go out next week on the dev channel.

[Evan Martin, 2009-06-19 17:40:33]

Comments on the GTK btis

http://codereview.chromium.org/132047/diff/1001/37
File chrome/browser/gtk/constrained_window_gtk.cc (right):

http://codereview.chromium.org/132047/diff/1001/37#newcode25
Line 25: GtkWidget* frame = gtk_frame_new(NULL);
Perhaps you can set the shadow type of the frame explicitly?
http://library.gnome.org/devel/gtk/unstable/GtkFrame.html#gtk-frame-set-shado...
I thought it'd do the ugly win3.1 style etched line by default, and I think
you're using it here to give it a raised edge, so it might be nice to be
explicit.

http://codereview.chromium.org/132047/diff/1001/37#newcode27
Line 27: gtk_alignment_set_padding(GTK_ALIGNMENT(alignment),
Since GtkFrame is a subclass of GtkContainer, you can set the padding on the
frame directly and not use this alignment.

http://codereview.chromium.org/132047/diff/1001/39
File chrome/browser/login_prompt_gtk.cc (right):

http://codereview.chromium.org/132047/diff/1001/39#newcode59
Line 59: root_.Own(gtk_vbox_new(NULL, 12));
This 12 can be one of the constants in chrome/common/gtk_util.h.

http://codereview.chromium.org/132047/diff/1001/39#newcode67
Line 67: password_entry_ = gtk_entry_new();
Put this one in "password mode".
http://library.gnome.org/devel/gtk/unstable/GtkEntry.html#gtk-entry-set-visib...

http://codereview.chromium.org/132047/diff/1001/39#newcode81
Line 81: ok_ = gtk_button_new_from_stock(GTK_STOCK_OK);
I think you need to mark this as "default" so that it is activated when you hit
enter in the other bits.

[Elliot Glaysher, 2009-06-19 18:20:41]

Please take another look?

http://codereview.chromium.org/132047/diff/1001/37
File chrome/browser/gtk/constrained_window_gtk.cc (right):

http://codereview.chromium.org/132047/diff/1001/37#newcode27
Line 27: gtk_alignment_set_padding(GTK_ALIGNMENT(alignment),
On 2009/06/19 17:40:33, Evan Martin wrote:
> Since GtkFrame is a subclass of GtkContainer, you can set the padding on the
> frame directly and not use this alignment.

We talked offline about why this is needed.

[Evan Martin, 2009-06-19 19:07:07]

http://codereview.chromium.org/132047/diff/3001/3010
File chrome/browser/tab_contents/tab_contents_view_gtk.cc (right):

http://codereview.chromium.org/132047/diff/3001/3010#newcode422
Line 422: // Look at the size request of the status bubble and tell the
Is "status" bubble correct here?

[Elliot Glaysher, 2009-06-19 19:30:29]

http://codereview.chromium.org/132047/diff/3001/3010
File chrome/browser/tab_contents/tab_contents_view_gtk.cc (right):

http://codereview.chromium.org/132047/diff/3001/3010#newcode422
Line 422: // Look at the size request of the status bubble and tell the
On 2009/06/19 19:07:08, Evan Martin wrote:
> Is "status" bubble correct here?

Done.

[Ben Goodger (Google), 2009-06-19 21:44:17]

OK for the viewsey windowsey changes

[Evan Martin, 2009-06-19 21:55:48]

LGTM,suggestions below

http://codereview.chromium.org/132047/diff/5002/4004
File chrome/browser/gtk/constrained_window_gtk.h (right):

http://codereview.chromium.org/132047/diff/5002/4004#newcode19
Line 19: // Returns the top of the widget root that will be put in the
constrained
"top of the widget root" is weird.  You just mean "the widget" I think.

http://codereview.chromium.org/132047/diff/5002/4004#newcode28
Line 28: // Constrained window implementation for the GTK port. Unlike the
windows
capital Windows here, to reduce confusion with lowercase windows below

http://codereview.chromium.org/132047/diff/5002/4004#newcode41
Line 41: // Returns the event box we draw on.
Maybe make it more clear why we expose this?  Maybe the caller doesn't care that
it's an eventbox but rather this is just "the widget that contains the junk" or
whatever.

http://codereview.chromium.org/132047/diff/5002/4005
File chrome/browser/login_prompt_gtk.cc (right):

http://codereview.chromium.org/132047/diff/5002/4005#newcode50
Line 50: ~LoginHandlerGtk() {
virtual

http://codereview.chromium.org/132047/diff/5002/4005#newcode159
Line 159: "Why is OnRequestCancelled called from the UI thread?";
I guess this text is a bit redundant with the DCHECK.  *shrug*

http://codereview.chromium.org/132047/diff/5002/4005#newcode199
Line 199: void CancelAuthDeferred() {
It's a bit weird that all of these are inside the class, rather than void
LoginHandlerGtk::CancelAuthDeferred() etc

http://codereview.chromium.org/132047/diff/5002/4005#newcode309
Line 309: GtkWidget* cancel_;
These don't need to be members, it seems -- they're only used in one function.