GTK: Rounded corner on status bubble.

GTK: Rounded corner on status bubble.

http://crbug.com/18309

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

[tony, 2009-08-05 22:23:08]

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

http://codereview.chromium.org/164010/diff/1/2#newcode32
Line 32: // Factor to multiply degress by.
Nit: typo?

http://codereview.chromium.org/164010/diff/1/2#newcode184
Line 184: GdkGC* gc = gdk_gc_new(GDK_DRAWABLE(mask));
Does this need to be freed?

[Evan Martin, 2009-08-05 22:24:22]

+dean since I think he did something like this for the bookmarks buble

[Evan Stade, 2009-08-05 22:24:31]

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

http://codereview.chromium.org/164010/diff/1/2#newcode208
Line 208: gtk_widget_shape_combine_mask(widget(), mask, 0, 0);
Seems it would be better/faster to use a region rather than a mask (assuming
there are arc drawing primitives for regions). See info bubble shaping code.

Also it looks like you dropped the border (?) but on windows they do draw a
shadowy grey border.

[Elliot Glaysher, 2009-08-05 22:41:39]

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

http://codereview.chromium.org/164010/diff/1/2#newcode184
Line 184: GdkGC* gc = gdk_gc_new(GDK_DRAWABLE(mask));
On 2009/08/05 22:23:08, tony wrote:
> Does this need to be freed?

Done.

http://codereview.chromium.org/164010/diff/1/2#newcode208
Line 208: gtk_widget_shape_combine_mask(widget(), mask, 0, 0);
On 2009/08/05 22:24:31, Evan Stade wrote:
> Seems it would be better/faster to use a region rather than a mask (assuming
> there are arc drawing primitives for regions). See info bubble shaping code.

Didn't know about regions. I'll re-implement this.

> Also it looks like you dropped the border (?) but on windows they do draw a
> shadowy grey border.

I took it out because the border color is semi-opaque, which  I can't replicate.
(Also, the above implementation makes it hard.) I have an idea on how to fix
this.

[Evan Martin, 2009-08-05 22:52:08]

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

http://codereview.chromium.org/164010/diff/1/2#newcode208
Line 208: gtk_widget_shape_combine_mask(widget(), mask, 0, 0);
On 2009/08/05 22:41:40, Elliot Glaysher wrote:
> On 2009/08/05 22:24:31, Evan Stade wrote:
> > Seems it would be better/faster to use a region rather than a mask (assuming
> > there are arc drawing primitives for regions). See info bubble shaping code.
> 
> Didn't know about regions. I'll re-implement this.

I think they're both implemented as masks underneath -- might be worth skimming
the gtk source before you do more work on it.

[Evan Stade, 2009-08-05 22:56:47]

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

http://codereview.chromium.org/164010/diff/1/2#newcode208
Line 208: gtk_widget_shape_combine_mask(widget(), mask, 0, 0);
On 2009/08/05 22:41:40, Elliot Glaysher wrote:
> On 2009/08/05 22:24:31, Evan Stade wrote:
> > Seems it would be better/faster to use a region rather than a mask (assuming
> > there are arc drawing primitives for regions). See info bubble shaping code.
> 
> Didn't know about regions. I'll re-implement this.
> 
> > Also it looks like you dropped the border (?) but on windows they do draw a
> > shadowy grey border.
> 
> I took it out because the border color is semi-opaque, which  I can't
replicate.
> (Also, the above implementation makes it hard.) I have an idea on how to fix
> this.

well, I wouldn't say it's completely necessary to have the shadow be partially
trasparent. Our current border is not, but still looks better than not having a
border.

Just my 3 cents. I'm sure whatever you come up with will look great.

[Elliot Glaysher, 2009-08-05 22:57:51]

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

http://codereview.chromium.org/164010/diff/1/2#newcode208
Line 208: gtk_widget_shape_combine_mask(widget(), mask, 0, 0);
On 2009/08/05 22:52:08, Evan Martin wrote:
> On 2009/08/05 22:41:40, Elliot Glaysher wrote:
> > On 2009/08/05 22:24:31, Evan Stade wrote:
> > > Seems it would be better/faster to use a region rather than a mask
(assuming
> > > there are arc drawing primitives for regions). See info bubble shaping
code.
> > 
> > Didn't know about regions. I'll re-implement this.
> 
> I think they're both implemented as masks underneath -- might be worth
skimming
> the gtk source before you do more work on it.

Both implementation hand their respective chunks of data directly to the XShape
extension. (XShapeCombineMask vs. XShapeCombineRectangles).

[Elliot Glaysher, 2009-08-06 00:07:40]

Different implementation. Take another look?

[Evan Martin, 2009-08-06 18:05:38]

LGTM

http://codereview.chromium.org/164010/diff/1008/11
File chrome/browser/gtk/status_bubble_gtk.cc (right):

http://codereview.chromium.org/164010/diff/1008/11#newcode28
Line 28: // The roundedness of the edges of our bubble.
roundedness = radius, right?

http://codereview.chromium.org/164010/diff/1008/11#newcode38
Line 38: GdkPoint MakeBidiGdkPoint(gint x, gint y, gint width, bool ltr) {
docs on all functions

http://codereview.chromium.org/164010/diff/1008/11#newcode211
Line 211: // the base color counts twice.
clever!

http://codereview.chromium.org/164010/diff/1008/11#newcode259
Line 259: StatusBubbleGtk* bubble) {
Does this get called only when the allocation changes, or does it get called all
the time?  I'm not sure if it's worth worrying about, just a thought.

[Evan Stade, 2009-08-06 18:28:08]

lg