Linux Extensions Install Prompt

Linux Extensions Install Prompt

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

[rafaelw, 2009-08-25 22:16:06]

Thanks for your help. Here's my first linux CL. Let me know if you want to me to
send the review to someone else.

[Elliot Glaysher, 2009-08-25 22:29:13]

http://codereview.chromium.org/174462/diff/8/10
File chrome/browser/gtk/extension_install_prompt_gtk.cc (right):

http://codereview.chromium.org/174462/diff/8/10#newcode103
Line 103: gtk_widget_modify_fg(prompt_label, GTK_STATE_NORMAL, &black);
Don't do this. What happens if the user has a theme where the background is a
dark color (DarkRooms, High Contrast Inverse, etc.)?

http://codereview.chromium.org/174462/diff/8/10#newcode117
Line 117: gtk_widget_modify_fg(warning_label, GTK_STATE_NORMAL, &black);
Same.

http://codereview.chromium.org/174462/diff/8/10#newcode129
Line 129: gtk_widget_modify_fg(severe_label, GTK_STATE_NORMAL, &red);
(This one is almost OK, but modification of colors shouldn't be done lightly.
Again, what happens with the rest of the theme?)

[tony, 2009-08-25 22:49:10]

Drive by code review...

http://codereview.chromium.org/174462/diff/1002/1004
File chrome/browser/gtk/extension_install_prompt_gtk.cc (right):

http://codereview.chromium.org/174462/diff/1002/1004#newcode29
Line 29: const int kPanelVertMargin = 20;
Nit: This seems unused?

http://codereview.chromium.org/174462/diff/1002/1004#newcode92
Line 92: gtk_widget_set_size_request(dialog, width, -1);
We normally don't set the size of dialogs and let GTK do all the sizing for us. 
Since you set the width of the multiline text area below, I think it'll be able
to figure out the rest automatically.

http://codereview.chromium.org/174462/diff/1002/1004#newcode118
Line 118: gtk_widget_set_size_request(warning_label, kRightColumnWidth, -1);
Right, so in this case, it's fine to set the size since it's a multi-line text
area so the width can change.

http://codereview.chromium.org/174462/diff/1002/1004#newcode138
Line 138: gtk_window_set_resizable(GTK_WINDOW(dialog), FALSE);
Is there a reason to make the dialog non-resizable?

[rafaelw, 2009-08-25 23:21:54]

http://codereview.chromium.org/174462/diff/8/10
File chrome/browser/gtk/extension_install_prompt_gtk.cc (right):

http://codereview.chromium.org/174462/diff/8/10#newcode103
Line 103: gtk_widget_modify_fg(prompt_label, GTK_STATE_NORMAL, &black);
On 2009/08/25 22:29:13, Elliot Glaysher wrote:
> Don't do this. What happens if the user has a theme where the background is a
> dark color (DarkRooms, High Contrast Inverse, etc.)?

Done.

http://codereview.chromium.org/174462/diff/8/10#newcode117
Line 117: gtk_widget_modify_fg(warning_label, GTK_STATE_NORMAL, &black);
On 2009/08/25 22:29:13, Elliot Glaysher wrote:
> Same.

Done.

http://codereview.chromium.org/174462/diff/8/10#newcode129
Line 129: gtk_widget_modify_fg(severe_label, GTK_STATE_NORMAL, &red);
On 2009/08/25 22:29:13, Elliot Glaysher wrote:
> (This one is almost OK, but modification of colors shouldn't be done lightly.
> Again, what happens with the rest of the theme?)

Left as Red per offline conversation

http://codereview.chromium.org/174462/diff/1002/1004#newcode29
Line 29: // Top or bottom margin.
On 2009/08/25 22:49:11, tony wrote:
> Nit: This seems unused?

Done.

http://codereview.chromium.org/174462/diff/1002/1004#newcode92
Line 92: }
On 2009/08/25 22:49:11, tony wrote:
> We normally don't set the size of dialogs and let GTK do all the sizing for
us. 
> Since you set the width of the multiline text area below, I think it'll be
able
> to figure out the rest automatically.

Done.

http://codereview.chromium.org/174462/diff/1002/1004#newcode138
Line 138: g_signal_connect(dialog, "response", G_CALLBACK(OnDialogResponse),
delegate);
On 2009/08/25 22:49:11, tony wrote:
> Is there a reason to make the dialog non-resizable?

Left resizeable per offline conversation

[Elliot Glaysher, 2009-08-25 23:30:42]

LG