GTK: Clean up the gtk extension infobar implementation.

GTK: Clean up the gtk extension infobar implementation.

- Add the extension menu button.
- Normalize on the gray extension infobar colors (and always force UI elements
  inside the infobar to be in chrome-theme mode instead of gtk-theme mode).
- Fix ownership issues (so GtkContainers didn't still think they owned their
  widgets even after they had been unparrented).

BUG=39916
TEST=none

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

[Elliot Glaysher, 2011-08-30 23:23:09]

Want lgtms from both of you

finnur: This should handle everything except for setting the height correctly.

[Finnur, 2011-08-31 08:06:58]

Thank you Elliot, really appreciate this!

Infobar stuff LGTM.
GTK also LGTM, but would be good to have estade verify it also, as you
requested.
Rest is just minor nits and typos.

> finnur: This should handle everything except for setting
> the height correctly.

Remember, the delegate keeps track of how high the bar should be. See:
delegate->height().

http://codereview.chromium.org/7809007/diff/1006/chrome/browser/ui/gtk/custom...
File chrome/browser/ui/gtk/custom_button.h (right):

http://codereview.chromium.org/7809007/diff/1006/chrome/browser/ui/gtk/custom...
chrome/browser/ui/gtk/custom_button.h:161: void ForceBrowserTheme();
nit: comment.

http://codereview.chromium.org/7809007/diff/1006/chrome/browser/ui/gtk/infoba...
File chrome/browser/ui/gtk/infobars/extension_infobar_gtk.cc (right):

http://codereview.chromium.org/7809007/diff/1006/chrome/browser/ui/gtk/infoba...
chrome/browser/ui/gtk/infobars/extension_infobar_gtk.cc:33: // extension authros
are going to expect to match the declared gradient in
typo: authros -> authors.

http://codereview.chromium.org/7809007/diff/1006/chrome/browser/ui/gtk/infoba...
chrome/browser/ui/gtk/infobars/extension_infobar_gtk.cc:66: // icon of the
correct size; as is this is hueg.
typo: hueg -> huge.

http://codereview.chromium.org/7809007/diff/1006/chrome/browser/ui/gtk/infoba...
chrome/browser/ui/gtk/infobars/extension_infobar_gtk.cc:71: }
nit: Don't need curly braces on one-liners.

http://codereview.chromium.org/7809007/diff/1006/chrome/browser/ui/gtk/infoba...
File chrome/browser/ui/gtk/infobars/extension_infobar_gtk.h (right):

http://codereview.chromium.org/7809007/diff/1006/chrome/browser/ui/gtk/infoba...
chrome/browser/ui/gtk/infobars/extension_infobar_gtk.h:71: // A GtkImage.
nit: Which one? :)

http://codereview.chromium.org/7809007/diff/1006/chrome/browser/ui/gtk/infoba...
chrome/browser/ui/gtk/infobars/extension_infobar_gtk.h:77: // child properties.
Reparentting becomes easier too.
typo: Reparentting -> Reparenting

http://codereview.chromium.org/7809007/diff/1006/chrome/browser/ui/gtk/infoba...
chrome/browser/ui/gtk/infobars/extension_infobar_gtk.h:80: GtkWidget* button_;
nit: document.

http://codereview.chromium.org/7809007/diff/1006/chrome/browser/ui/gtk/infoba...
chrome/browser/ui/gtk/infobars/extension_infobar_gtk.h:83: // to which outlives
us.
nit: One usually monitors signals, as opposed to connecting to them. No? :)

[Elliot Glaysher, 2011-08-31 17:53:08]

http://codereview.chromium.org/7809007/diff/1006/chrome/browser/ui/gtk/infoba...
File chrome/browser/ui/gtk/infobars/extension_infobar_gtk.cc (right):

http://codereview.chromium.org/7809007/diff/1006/chrome/browser/ui/gtk/infoba...
chrome/browser/ui/gtk/infobars/extension_infobar_gtk.cc:66: // icon of the
correct size; as is this is hueg.
On 2011/08/31 08:06:59, Finnur wrote:
> typo: hueg -> huge.

Comment partially replaced.

Can we get an actual icon pre-scaled correctly? The gtk+ rendering matches the
Windows one from your email, but I don't think the scaling is very good while it
could have subpixel hinting, etc.

http://codereview.chromium.org/7809007/diff/1006/chrome/browser/ui/gtk/infoba...
File chrome/browser/ui/gtk/infobars/extension_infobar_gtk.h (right):

http://codereview.chromium.org/7809007/diff/1006/chrome/browser/ui/gtk/infoba...
chrome/browser/ui/gtk/infobars/extension_infobar_gtk.h:83: // to which outlives
us.
On 2011/08/31 08:06:59, Finnur wrote:
> nit: One usually monitors signals, as opposed to connecting to them. No? :)

I have never heard the word 'monitor' in the context of signals. Every signal
library I've ever used (gtk, boost::signals, qt) uses 'connect' for this action.

[Finnur, 2011-08-31 18:32:20]

> Can we get an actual icon pre-scaled correctly? 

See if Glen has one. I would bug him myself, but I only have a few minutes (I'm
cooking dinner). :)

> I have never heard the word 'monitor' in the context of signals. Every signal
> library I've ever used (gtk, boost::signals, qt) uses 'connect' for this
action.

OK. Just sounded weird. :)

Still LGTM.

[Glen Murphy, 2011-08-31 18:34:04]

>> Can we get an actual icon pre-scaled correctly?
>
> See if Glen has one. I would bug him myself, but I only have a few minutes
(I'm
> cooking dinner). :)

+alcor, for all icons.

[Evan Stade, 2011-08-31 20:00:12]

LGTM

http://codereview.chromium.org/7809007/diff/2006/chrome/browser/ui/gtk/custom...
File chrome/browser/ui/gtk/custom_button.h (right):

http://codereview.chromium.org/7809007/diff/2006/chrome/browser/ui/gtk/custom...
chrome/browser/ui/gtk/custom_button.h:230: // HTML extensions.
explaining the current use of this var is a little extraneous / prone to getting
stale

http://codereview.chromium.org/7809007/diff/2006/chrome/browser/ui/gtk/infoba...
File chrome/browser/ui/gtk/infobars/extension_infobar_gtk.cc (right):

http://codereview.chromium.org/7809007/diff/2006/chrome/browser/ui/gtk/infoba...
chrome/browser/ui/gtk/infobars/extension_infobar_gtk.cc:50: double* r, double*
g, double *b) {
s/ */* /

http://codereview.chromium.org/7809007/diff/2006/chrome/browser/ui/gtk/infoba...
chrome/browser/ui/gtk/infobars/extension_infobar_gtk.cc:52: *r = *g = *b = 233.0
/ 255.0;
huh? aren't they drawn by a renderer, basically making this value irrelevant?

http://codereview.chromium.org/7809007/diff/2006/chrome/browser/ui/gtk/infoba...
chrome/browser/ui/gtk/infobars/extension_infobar_gtk.cc:78: image_size +
kDropArrowLeftMargin + drop_image->width(), image_size,
I think there are more readable ways to distribute the newlines in function
calls than this

http://codereview.chromium.org/7809007/diff/2006/chrome/browser/ui/gtk/infoba...
chrome/browser/ui/gtk/infobars/extension_infobar_gtk.cc:161: new MenuGtk(this,
context_menu_model_.get()));
this doesn't fit on one line?

http://codereview.chromium.org/7809007/diff/2006/chrome/browser/ui/gtk/infoba...
File chrome/browser/ui/gtk/infobars/extension_infobar_gtk.h (right):

http://codereview.chromium.org/7809007/diff/2006/chrome/browser/ui/gtk/infoba...
chrome/browser/ui/gtk/infobars/extension_infobar_gtk.h:87: // The context menu
view and model for this extension action.
I don't think it's technically a context menu unless it's triggered by right
click (which is what I would assume by reading this, but it's not?)

[Elliot Glaysher, 2011-08-31 20:37:32]

http://codereview.chromium.org/7809007/diff/2006/chrome/browser/ui/gtk/infoba...
File chrome/browser/ui/gtk/infobars/extension_infobar_gtk.cc (right):

http://codereview.chromium.org/7809007/diff/2006/chrome/browser/ui/gtk/infoba...
chrome/browser/ui/gtk/infobars/extension_infobar_gtk.cc:50: double* r, double*
g, double *b) {
On 2011/08/31 20:00:12, Evan Stade wrote:
> s/ */* /

Fixed here and in every InfobarGtk subclass that has it copied.

http://codereview.chromium.org/7809007/diff/2006/chrome/browser/ui/gtk/infoba...
chrome/browser/ui/gtk/infobars/extension_infobar_gtk.cc:52: *r = *g = *b = 233.0
/ 255.0;
On 2011/08/31 20:00:12, Evan Stade wrote:
> huh? aren't they drawn by a renderer, basically making this value irrelevant?

Only the render area is. If we don't do this, there's a big area to the left and
right of the render area that uses gtk default colors in gtk-mode.

[Evan Stade, 2011-08-31 20:47:49]

http://codereview.chromium.org/7809007/diff/2006/chrome/browser/ui/gtk/infoba...
File chrome/browser/ui/gtk/infobars/extension_infobar_gtk.cc (right):

http://codereview.chromium.org/7809007/diff/2006/chrome/browser/ui/gtk/infoba...
chrome/browser/ui/gtk/infobars/extension_infobar_gtk.cc:52: *r = *g = *b = 233.0
/ 255.0;
On 2011/08/31 20:37:32, Elliot Glaysher wrote:
> On 2011/08/31 20:00:12, Evan Stade wrote:
> > huh? aren't they drawn by a renderer, basically making this value
irrelevant?
> 
> Only the render area is. If we don't do this, there's a big area to the left
and
> right of the render area that uses gtk default colors in gtk-mode.

shouldn't we at least match the chrome theme then? i.e.
theme_service_->GetColor(ThemeService::COLOR_TOOLBAR)

[Elliot Glaysher, 2011-08-31 21:06:05]

On 2011/08/31 20:47:49, Evan Stade wrote:
> shouldn't we at least match the chrome theme then? i.e.
> theme_service_->GetColor(ThemeService::COLOR_TOOLBAR)

But it's not the toolbar color and it's subtly not the normal page action
infobar color, either. In extensions.css, it's set as a gradient from #E9E9E9
(233,233,233) to #DADADA (218,218,218).

The page action colors are (237,237,237) to (217,217,217) and the slight
difference is noticeable. :(

[alcor, 2011-09-06 19:17:46]

What icon is this for? I didn't get the rest of the thread :)

On Wed, Aug 31, 2011 at 11:33 AM, Glen Murphy <glen@chromium.org> wrote:

> >> Can we get an actual icon pre-scaled correctly?
> >
> > See if Glen has one. I would bug him myself, but I only have a few
> minutes (I'm
> > cooking dinner). :)
>
> +alcor, for all icons.
>

[Elliot Glaysher, 2011-09-06 19:24:51]

On Tue, Sep 6, 2011 at 12:16 PM, Nicholas Jitkoff <alcor@google.com> wrote:
> What icon is this for? I didn't get the rest of the thread :)

There are new infobars that can be created by extensions. If the
extension doesn't provide a 16x16 icon, we use a default. However, our
current default is the big icon at the top of the extensions page
scaled down to 16x16. I don't think the in-chrome scaling looks very
good.

-- Elliot