linux_aura: Use GTK button borders in GTK theme mode.

linux_aura: Use GTK button borders in GTK theme mode.

Up until recently, the icons for back/forward/home/reload all used
borders drawn on the image resources. Then this was changed to use a
views::Border to draw on top of the icon. Gtk2UI wasn't similarly
changed. This patch adds a Gtk2Border, along with machinery for having
the views::LinuxUI vend instances of them to the views layer.

This border should theoretically be usable for other buttons, like the
bookmark buttons, which are still using the chrome theme resources even
in GTK theme mode.

BUG=333404
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=246444

[Elliot Glaysher, 2014-01-17 20:44:20]

msw: review
gbillock: fyi

[msw, 2014-01-21 19:18:19]

Sorry for the delay; most comments are just for simplifications and naming.

https://codereview.chromium.org/131513005/diff/1/chrome/browser/ui/libgtk2ui/...
File chrome/browser/ui/libgtk2ui/gtk2_border.cc (right):

https://codereview.chromium.org/131513005/diff/1/chrome/browser/ui/libgtk2ui/...
chrome/browser/ui/libgtk2ui/gtk2_border.cc:13: #include
"ui/views/controls/button/label_button.h"
nit: the button.h include should be sufficient; remove this.

https://codereview.chromium.org/131513005/diff/1/chrome/browser/ui/libgtk2ui/...
chrome/browser/ui/libgtk2ui/gtk2_border.cc:14: #include
"ui/views/controls/button/label_button_border.h"
nit: the border.h include in the header should be sufficient; remove this.

https://codereview.chromium.org/131513005/diff/1/chrome/browser/ui/libgtk2ui/...
chrome/browser/ui/libgtk2ui/gtk2_border.cc:20: int
ViewsToGtkState(views::Button::ButtonState state) {
nit: should this return a GtkStateType?

https://codereview.chromium.org/131513005/diff/1/chrome/browser/ui/libgtk2ui/...
chrome/browser/ui/libgtk2ui/gtk2_border.cc:26: default:
nit: handle Button::STATE_DISABLED / GTK_STATE_INSENSITIVE for completeness?

https://codereview.chromium.org/131513005/diff/1/chrome/browser/ui/libgtk2ui/...
chrome/browser/ui/libgtk2ui/gtk2_border.cc:34: : parent_(parent),
ditto nit: This isn't really the parent, should it be named gtk2_ui, or similar?

https://codereview.chromium.org/131513005/diff/1/chrome/browser/ui/libgtk2ui/...
chrome/browser/ui/libgtk2ui/gtk2_border.cc:50: int state_;
Should this be a GtkStateType?

https://codereview.chromium.org/131513005/diff/1/chrome/browser/ui/libgtk2ui/...
chrome/browser/ui/libgtk2ui/gtk2_border.cc:89: int state = ViewsToGtkState(
nit: should this be a GtkStateType?

https://codereview.chromium.org/131513005/diff/1/chrome/browser/ui/libgtk2ui/...
chrome/browser/ui/libgtk2ui/gtk2_border.cc:90: static_cast<const
views::LabelButton&>(view).state());
nit: DCHECK_EQ(view, owning_view_), and use owning_view_->state() (stored as a
views::Button).

https://codereview.chromium.org/131513005/diff/1/chrome/browser/ui/libgtk2ui/...
chrome/browser/ui/libgtk2ui/gtk2_border.cc:121: return gfx::Size(insets.left() +
insets.right(),
nit: use gfx::Insets::width() and height().

https://codereview.chromium.org/131513005/diff/1/chrome/browser/ui/libgtk2ui/...
File chrome/browser/ui/libgtk2ui/gtk2_border.h (right):

https://codereview.chromium.org/131513005/diff/1/chrome/browser/ui/libgtk2ui/...
chrome/browser/ui/libgtk2ui/gtk2_border.h:10: #include
"ui/views/controls/button/button.h"
nit: this include isn't needed.

https://codereview.chromium.org/131513005/diff/1/chrome/browser/ui/libgtk2ui/...
chrome/browser/ui/libgtk2ui/gtk2_border.h:27: Gtk2Border(Gtk2UI* parent,
nit: This isn't really the parent, should it be named gtk2_ui, or similar?

https://codereview.chromium.org/131513005/diff/1/chrome/browser/ui/libgtk2ui/...
chrome/browser/ui/libgtk2ui/gtk2_border.h:28: views::View* owning_view,
Pass in a views::Button here to avoid static casting later.

https://codereview.chromium.org/131513005/diff/1/chrome/browser/ui/libgtk2ui/...
chrome/browser/ui/libgtk2ui/gtk2_border.h:55:
scoped_ptr<views::LabelButtonBorder> label_button_border_;
This should just be a scoped_ptr<views::Border>.

https://codereview.chromium.org/131513005/diff/1/chrome/browser/ui/libgtk2ui/...
File chrome/browser/ui/libgtk2ui/gtk2_ui.h (right):

https://codereview.chromium.org/131513005/diff/1/chrome/browser/ui/libgtk2ui/...
chrome/browser/ui/libgtk2ui/gtk2_ui.h:102: views::LabelButtonBorder*
label_button_border) OVERRIDE;
Just use a views::Border here.

https://codereview.chromium.org/131513005/diff/1/ui/views/linux_ui/linux_ui.h
File ui/views/linux_ui/linux_ui.h (right):

https://codereview.chromium.org/131513005/diff/1/ui/views/linux_ui/linux_ui.h...
ui/views/linux_ui/linux_ui.h:29: class LabelButtonBorder;
nit: remove this.

https://codereview.chromium.org/131513005/diff/1/ui/views/linux_ui/linux_ui.h...
ui/views/linux_ui/linux_ui.h:107: views::LabelButtonBorder* label_button_border)
= 0;
Ditto, just use a views::Border here.

[Elliot Glaysher, 2014-01-21 23:06:54]

ptal

[msw, 2014-01-22 02:21:07]

LGTM with a couple nits.

https://codereview.chromium.org/131513005/diff/1/chrome/browser/ui/libgtk2ui/...
File chrome/browser/ui/libgtk2ui/gtk2_border.h (right):

https://codereview.chromium.org/131513005/diff/1/chrome/browser/ui/libgtk2ui/...
chrome/browser/ui/libgtk2ui/gtk2_border.h:10: #include
"ui/views/controls/button/button.h"
On 2014/01/21 19:18:19, msw wrote:
> nit: this include isn't needed.

PING!

https://codereview.chromium.org/131513005/diff/270001/chrome/browser/ui/libgt...
File chrome/browser/ui/libgtk2ui/gtk2_border.h (right):

https://codereview.chromium.org/131513005/diff/270001/chrome/browser/ui/libgt...
chrome/browser/ui/libgtk2ui/gtk2_border.h:18: class View;
nit: remove this; ui/views/border.h has a forward decl already.

https://codereview.chromium.org/131513005/diff/270001/chrome/browser/ui/libgt...
chrome/browser/ui/libgtk2ui/gtk2_border.h:54: // LabelButtonBorder and delegate
to it whenever we aren't in GTK theme mode.
nit: s/LabelButtonBorder/Border/

[Elliot Glaysher, 2014-01-22 18:18:32]

+sky: owners stamp

[sky, 2014-01-22 20:23:58]

LGTM with the following changes.

https://codereview.chromium.org/131513005/diff/390001/chrome/browser/ui/libgt...
File chrome/browser/ui/libgtk2ui/gtk2_border.cc (right):

https://codereview.chromium.org/131513005/diff/390001/chrome/browser/ui/libgt...
chrome/browser/ui/libgtk2ui/gtk2_border.cc:53: GtkStateType state_;
nit: const and this and size_.

https://codereview.chromium.org/131513005/diff/390001/ui/views/controls/butto...
File ui/views/controls/button/label_button.h (right):

https://codereview.chromium.org/131513005/diff/390001/ui/views/controls/butto...
ui/views/controls/button/label_button.h:102: void
UpdateThemedBorder(LabelButtonBorder* label_button_border);
Can this take a scoped_ptr so ownership is clear?

https://codereview.chromium.org/131513005/diff/390001/ui/views/linux_ui/linux...
File ui/views/linux_ui/linux_ui.h (right):

https://codereview.chromium.org/131513005/diff/390001/ui/views/linux_ui/linux...
ui/views/linux_ui/linux_ui.h:106: views::CustomButton* owning_button,
Same comments about scoped_ptrs here.

[Elliot Glaysher, 2014-01-22 21:12:15]

https://codereview.chromium.org/131513005/diff/390001/ui/views/controls/butto...
File ui/views/controls/button/label_button.h (right):

https://codereview.chromium.org/131513005/diff/390001/ui/views/controls/butto...
ui/views/controls/button/label_button.h:102: void
UpdateThemedBorder(LabelButtonBorder* label_button_border);
On 2014/01/22 20:23:59, sky wrote:
> Can this take a scoped_ptr so ownership is clear?

If this takes a scoped_ptr<>, I then have to change every instance of Border in
chrome, since it is a wrapper around set_border(). Doing this cleanly would be
much larger than the rest of this patch. (I tried: I'm up to fifteen files so
far and I'm still getting massive spew.)

[commit-bot: I haz the power, 2014-01-22 21:13:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erg@chromium.org/131513005/390001

[commit-bot: I haz the power, 2014-01-22 23:34:03]

Change committed as 246444