Put the dropdown menu button on the linux download shelf....

Put the dropdown menu button on the linux download shelf.

Also set the background color.

Also extend NineBox a little bit.


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

[Evan Martin, 2009-03-05 01:24:22]

http://codereview.chromium.org/40139/diff/1/6
File chrome/browser/gtk/download_item_gtk.cc (right):

http://codereview.chromium.org/40139/diff/1/6#newcode17
Line 17: const int kMenuButtonWidth = 16;
comment on this?

http://codereview.chromium.org/40139/diff/1/6#newcode167
Line 167: if (is_body) {
Why is this check necessary?

http://codereview.chromium.org/40139/diff/1/4
File chrome/browser/gtk/nine_box.cc (right):

http://codereview.chromium.org/40139/diff/1/4#newcode98
Line 98: }
no curlies needed here (to match the other calls)

[Evan Stade, 2009-03-05 01:42:06]

updated. I also made it set the background color. To do this I had to stuff the
hbox which was formerly known as shelf_ into an EventBox (the pygtk faq
instructed me to do this because certain widgets such as GtkBoxes don't have a
gdk window, so you have to stuff them in widget that does if you want to modify
the colors).

http://codereview.chromium.org/40139/diff/1/6
File chrome/browser/gtk/download_item_gtk.cc (right):

http://codereview.chromium.org/40139/diff/1/6#newcode17
Line 17: const int kMenuButtonWidth = 16;
On 2009/03/05 01:24:22, Evan Martin wrote:
> comment on this?

Done.

http://codereview.chromium.org/40139/diff/1/6#newcode167
Line 167: if (is_body) {
On 2009/03/05 01:24:22, Evan Martin wrote:
> Why is this check necessary?

the drop down menu button doesn't have any children, and gtk complains if you
call gtk_container_propagate_expose with a null pointer for the child.

http://codereview.chromium.org/40139/diff/1/4
File chrome/browser/gtk/nine_box.cc (right):

http://codereview.chromium.org/40139/diff/1/4#newcode98
Line 98: }
On 2009/03/05 01:24:22, Evan Martin wrote:
> no curlies needed here (to match the other calls)

Done.

http://codereview.chromium.org/40139/diff/13/1008
File chrome/browser/gtk/download_item_gtk.cc (right):

http://codereview.chromium.org/40139/diff/13/1008#newcode41
Line 41: GtkWidget* label =
gtk_label_new(download_model->download()->GetFilePath()
This is supposed to be GetFileName(). I changed it to this function because in
download_manager.h it has this to say about it:

  // Returns the file-name that should be reported to the user, which is
  // file_name_ for safe downloads and original_name_ for dangerous ones with
  // the uniquifier number.

[Evan Martin, 2009-03-05 01:47:46]

http://codereview.chromium.org/40139/diff/1/6
File chrome/browser/gtk/download_item_gtk.cc (right):

http://codereview.chromium.org/40139/diff/1/6#newcode167
Line 167: if (is_body) {
On 2009/03/05 01:42:06, estade wrote:
> On 2009/03/05 01:24:22, Evan Martin wrote:
> > Why is this check necessary?
> 
> the drop down menu button doesn't have any children, and gtk complains if you
> call gtk_container_propagate_expose with a null pointer for the child.

Maybe you could just check if (gtk_bin_get_child()), then?  It's weird at a
glance that we only propagate the events to certain widgets, because you'd
expect for them to always be propagated.

http://codereview.chromium.org/40139/diff/13/1005
File chrome/browser/gtk/download_shelf_gtk.cc (right):

http://codereview.chromium.org/40139/diff/13/1005#newcode41
Line 41: gdk_color_parse("#E6EDF4", &color);
Why not the same as kBorderColor?

http://codereview.chromium.org/40139/diff/13/1005#newcode61
Line 61: shelf_ = gtk_event_box_new();
maybe comment here as to why you need an event box (like you did in the mail)

[Evan Stade, 2009-03-05 02:04:14]

http://codereview.chromium.org/40139/diff/1/6
File chrome/browser/gtk/download_item_gtk.cc (right):

http://codereview.chromium.org/40139/diff/1/6#newcode167
Line 167: if (is_body) {
On 2009/03/05 01:47:46, Evan Martin wrote:
> On 2009/03/05 01:42:06, estade wrote:
> > On 2009/03/05 01:24:22, Evan Martin wrote:
> > > Why is this check necessary?
> > 
> > the drop down menu button doesn't have any children, and gtk complains if
you
> > call gtk_container_propagate_expose with a null pointer for the child.
> 
> Maybe you could just check if (gtk_bin_get_child()), then?  It's weird at a
> glance that we only propagate the events to certain widgets, because you'd
> expect for them to always be propagated.

ok then

http://codereview.chromium.org/40139/diff/13/1005
File chrome/browser/gtk/download_shelf_gtk.cc (right):

http://codereview.chromium.org/40139/diff/13/1005#newcode41
Line 41: gdk_color_parse("#E6EDF4", &color);
On 2009/03/05 01:47:46, Evan Martin wrote:
> Why not the same as kBorderColor?

I meant to delete the border color line, it's not used anywhere yet. If you're
wondering, I use parse instead of directly setting the members because it takes
the more standard 24-bit notation (members in GdkColor are 2 bytes each).

http://codereview.chromium.org/40139/diff/13/1005#newcode61
Line 61: shelf_ = gtk_event_box_new();
On 2009/03/05 01:47:46, Evan Martin wrote:
> maybe comment here as to why you need an event box (like you did in the mail)

Is the comment in the header not enough?

[Evan Martin, 2009-03-05 02:10:02]

http://codereview.chromium.org/40139/diff/13/1005
File chrome/browser/gtk/download_shelf_gtk.cc (right):

http://codereview.chromium.org/40139/diff/13/1005#newcode41
Line 41: gdk_color_parse("#E6EDF4", &color);
On 2009/03/05 02:04:15, estade wrote:
> On 2009/03/05 01:47:46, Evan Martin wrote:
> > Why not the same as kBorderColor?
> 
> I meant to delete the border color line, it's not used anywhere yet. If you're
> wondering, I use parse instead of directly setting the members because it
takes
> the more standard 24-bit notation (members in GdkColor are 2 bytes each).
> 

I guess you could could do it like this if you wanted to be clean:
 GdkColor c = { 0xE6 << 8, 0xED << 8, etc };

I just think it's a lot of code for maintaining something that should be a
compile-time static.  :)

http://codereview.chromium.org/40139/diff/13/1005#newcode61
Line 61: shelf_ = gtk_event_box_new();
On 2009/03/05 02:04:15, estade wrote:
> On 2009/03/05 01:47:46, Evan Martin wrote:
> > maybe comment here as to why you need an event box (like you did in the
mail)
> 
> Is the comment in the header not enough?

ah, ok

[Evan Stade, 2009-03-05 02:30:25]

> I guess you could could do it like this if you wanted to be clean:
>  GdkColor c = { 0xE6 << 8, 0xED << 8, etc };


whydidntithinkofthat

[Evan Martin, 2009-03-05 03:07:59]

SHIP IT

[Dean McNamee, 2009-03-05 14:16:01]

http://codereview.chromium.org/40139/diff/1016/1021
File chrome/browser/gtk/download_item_gtk.cc (right):

http://codereview.chromium.org/40139/diff/1016/1021#newcode21
Line 21: }
  // namespace

http://codereview.chromium.org/40139/diff/1016/1021#newcode42
Line 42: GtkWidget* label =
gtk_label_new(download_model->download()->GetFileName()
I would have wrapped this at the first ( if it's possible.

http://codereview.chromium.org/40139/diff/1016/1018
File chrome/browser/gtk/download_shelf_gtk.cc (right):

http://codereview.chromium.org/40139/diff/1016/1018#newcode37
Line 37: }
  // namespace

http://codereview.chromium.org/40139/diff/1016/1022
File chrome/browser/gtk/download_shelf_gtk.h (right):

http://codereview.chromium.org/40139/diff/1016/1022#newcode44
Line 44: GtkWidget* shelf_;
shelf_box_ -> shelf_ ?