linux_aura: Fixes a couple of issues with the button border integration.

linux_aura: Fixes a couple of issues with the button border integration.

- Animate in like the normal views LabelButtonBorders do.
- Actually check whether we want to render the borders in different
  hover states. (Fixes the buttons in the bookmark bubble.)
- Support tab focus borders.

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

[msw, 2014-01-24 00:22:37]

https://codereview.chromium.org/132023010/diff/70001/chrome/browser/ui/libgtk...
File chrome/browser/ui/libgtk2ui/gtk2_border.cc (right):

https://codereview.chromium.org/132023010/diff/70001/chrome/browser/ui/libgtk...
chrome/browser/ui/libgtk2ui/gtk2_border.cc:27: Button::ButtonState
GetViewsButtonState(ui::NativeTheme::State state) {
nit: Rather than duplicate this label_button_border.cc function, make it a
Button class static helper function or similar.

https://codereview.chromium.org/132023010/diff/70001/chrome/browser/ui/libgtk...
chrome/browser/ui/libgtk2ui/gtk2_border.cc:116: gfx::Rect
rect(native_theme_delegate->GetThemePaintRect());
Rather than copy this from LabelButtonBorder, does it make any sense to add a
virtual LabelButtonBorder::PaintState, and then make this a LabelButtonBorder
subclass which overloads that function (or similar)?

https://codereview.chromium.org/132023010/diff/70001/chrome/browser/ui/libgtk...
chrome/browser/ui/libgtk2ui/gtk2_border.cc:123: // animation.
nit: fits on line above, just like where this is copied from.

https://codereview.chromium.org/132023010/diff/70001/chrome/browser/ui/libgtk...
chrome/browser/ui/libgtk2ui/gtk2_border.cc:166: // If the LabelButtonBorder has
a painter for our focused/state combination,
Why do you check this? Perhaps just check the button's style (STYLE_BUTTON for
bubbles/dialogs, and STYLE_TEXTBUTTON for back/forward/reload) to know what
focused/state images to skip? But doesn't GTK draw something for
back/forward/reload when they are unfocused and normal (not hovered or pressed),
while views doesn't? It seems like you should just always try to get the GTK
appearance.

[Elliot Glaysher, 2014-01-24 21:55:59]

https://codereview.chromium.org/132023010/diff/70001/chrome/browser/ui/libgtk...
File chrome/browser/ui/libgtk2ui/gtk2_border.cc (right):

https://codereview.chromium.org/132023010/diff/70001/chrome/browser/ui/libgtk...
chrome/browser/ui/libgtk2ui/gtk2_border.cc:27: Button::ButtonState
GetViewsButtonState(ui::NativeTheme::State state) {
On 2014/01/24 00:22:37, msw wrote:
> nit: Rather than duplicate this label_button_border.cc function, make it a
> Button class static helper function or similar.

Done.

https://codereview.chromium.org/132023010/diff/70001/chrome/browser/ui/libgtk...
chrome/browser/ui/libgtk2ui/gtk2_border.cc:116: gfx::Rect
rect(native_theme_delegate->GetThemePaintRect());
On 2014/01/24 00:22:37, msw wrote:
> Rather than copy this from LabelButtonBorder, does it make any sense to add a
> virtual LabelButtonBorder::PaintState, and then make this a LabelButtonBorder
> subclass which overloads that function (or similar)?

I don't think so. Several people working on views have a strong preference for
composition over inheritance, and I'm not sure that a Gtk2Border is-a
LabelButtonBorder.

https://codereview.chromium.org/132023010/diff/70001/chrome/browser/ui/libgtk...
chrome/browser/ui/libgtk2ui/gtk2_border.cc:166: // If the LabelButtonBorder has
a painter for our focused/state combination,
Taking these two questions out of order...

On 2014/01/24 00:22:37, msw wrote:
> But doesn't GTK draw something for
> back/forward/reload when they are unfocused and normal (not hovered or
pressed),
> while views doesn't? It seems like you should just always try to get the GTK
> appearance.

Not on toolbars. On toolbars, buttons don't have borders in the normal state.

> Why do you check this? Perhaps just check the button's style (STYLE_BUTTON for
> bubbles/dialogs, and STYLE_TEXTBUTTON for back/forward/reload) to know what
> focused/state images to skip?

My recollection was that there were cases where this was set outside of the
LabelButtonBorder constructor; this patch started as a fix for crbug.com/337081,
before I realized that the real fix was to never show GTK-ish rendering in the
content area. However, I can't find any places where it would actually make a
difference.

I've changed this, but that said, this feels fragile. It seems to me to be
slightly more robust to ask the LabelButtonBorder about whether a border would
be drawn in this case.

[msw, 2014-01-24 23:57:16]

LGTM with a q and a couple nits.

https://codereview.chromium.org/132023010/diff/70001/chrome/browser/ui/libgtk...
File chrome/browser/ui/libgtk2ui/gtk2_border.cc (right):

https://codereview.chromium.org/132023010/diff/70001/chrome/browser/ui/libgtk...
chrome/browser/ui/libgtk2ui/gtk2_border.cc:166: // If the LabelButtonBorder has
a painter for our focused/state combination,
On 2014/01/24 21:56:00, Elliot Glaysher wrote:
> Taking these two questions out of order...
> 
> On 2014/01/24 00:22:37, msw wrote:
> > But doesn't GTK draw something for
> > back/forward/reload when they are unfocused and normal (not hovered or
> pressed),
> > while views doesn't? It seems like you should just always try to get the GTK
> > appearance.
> 
> Not on toolbars. On toolbars, buttons don't have borders in the normal state.
> 
> > Why do you check this? Perhaps just check the button's style (STYLE_BUTTON
for
> > bubbles/dialogs, and STYLE_TEXTBUTTON for back/forward/reload) to know what
> > focused/state images to skip?
> 
> My recollection was that there were cases where this was set outside of the
> LabelButtonBorder constructor; this patch started as a fix for
crbug.com/337081,
> before I realized that the real fix was to never show GTK-ish rendering in the
> content area. However, I can't find any places where it would actually make a
> difference.
> 
> I've changed this, but that said, this feels fragile. It seems to me to be
> slightly more robust to ask the LabelButtonBorder about whether a border would
> be drawn in this case.

To clarify, will back/forward/reload behave differently than bookmarks? Views
just has the two button styles. I like the new explicit ShouldDrawBorder code.

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

https://codereview.chromium.org/132023010/diff/120001/chrome/browser/ui/libgt...
chrome/browser/ui/libgtk2ui/gtk2_border.cc:172: focused == false &&
nit: consider making the focused/state conditions part of the return statement
in this STYLE_TEXTBUTTON branch.

https://codereview.chromium.org/132023010/diff/120001/ui/views/controls/butto...
File ui/views/controls/button/label_button_border.cc (right):

https://codereview.chromium.org/132023010/diff/120001/ui/views/controls/butto...
ui/views/controls/button/label_button_border.cc:42:
Button::GetButtonStateFrom(state));
nit: drop the Button:: qualifiers here and below and fit this on one line.

[Elliot Glaysher, 2014-01-25 00:37:35]

msw's return statement comment

[Elliot Glaysher, 2014-01-25 00:38:19]

+sky for owner's stamp

https://codereview.chromium.org/132023010/diff/70001/chrome/browser/ui/libgtk...
File chrome/browser/ui/libgtk2ui/gtk2_border.cc (right):

https://codereview.chromium.org/132023010/diff/70001/chrome/browser/ui/libgtk...
chrome/browser/ui/libgtk2ui/gtk2_border.cc:166: // If the LabelButtonBorder has
a painter for our focused/state combination,
On 2014/01/24 23:57:16, msw wrote:
> On 2014/01/24 21:56:00, Elliot Glaysher wrote:
> > Taking these two questions out of order...
> > 
> > On 2014/01/24 00:22:37, msw wrote:
> > > But doesn't GTK draw something for
> > > back/forward/reload when they are unfocused and normal (not hovered or
> > pressed),
> > > while views doesn't? It seems like you should just always try to get the
GTK
> > > appearance.
> > 
> > Not on toolbars. On toolbars, buttons don't have borders in the normal
state.
> > 
> > > Why do you check this? Perhaps just check the button's style (STYLE_BUTTON
> for
> > > bubbles/dialogs, and STYLE_TEXTBUTTON for back/forward/reload) to know
what
> > > focused/state images to skip?
> > 
> > My recollection was that there were cases where this was set outside of the
> > LabelButtonBorder constructor; this patch started as a fix for
> crbug.com/337081,
> > before I realized that the real fix was to never show GTK-ish rendering in
the
> > content area. However, I can't find any places where it would actually make
a
> > difference.
> > 
> > I've changed this, but that said, this feels fragile. It seems to me to be
> > slightly more robust to ask the LabelButtonBorder about whether a border
would
> > be drawn in this case.
> 
> To clarify, will back/forward/reload behave differently than bookmarks? Views
> just has the two button styles. I like the new explicit ShouldDrawBorder code.

In the current GTK port, there is no visual difference between the
back/forward/reload buttons and the bookmark buttons. My hope is that in the gtk
mode of linux_aura, there will be no visual difference either.

https://codereview.chromium.org/132023010/diff/120001/ui/views/controls/butto...
File ui/views/controls/button/label_button_border.cc (right):

https://codereview.chromium.org/132023010/diff/120001/ui/views/controls/butto...
ui/views/controls/button/label_button_border.cc:42:
Button::GetButtonStateFrom(state));
On 2014/01/24 23:57:17, msw wrote:
> nit: drop the Button:: qualifiers here and below and fit this on one line.

I verified that I can't do this. Not in scope.

[sky, 2014-01-25 00:41:32]

LGTM

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

https://codereview.chromium.org/132023010/diff/120002/chrome/browser/ui/libgt...
chrome/browser/ui/libgtk2ui/gtk2_border.cc:85: for (int i = 0; i < 2; ++i)
arraysize or a constant? I would also add parens around this for.

[commit-bot: I haz the power, 2014-01-25 00:52:06]

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

[commit-bot: I haz the power, 2014-01-25 07:18:30]

Change committed as 247082