[MD] Use same focus ring on BarControlButton as MdTextButton.

[MD] Use same focus ring on BarControlButton as MdTextButton.

BUG=596033
Committed: https://crrev.com/ee3ec60a1260a0c72edaae3a4b72c90bae93a2d9
Cr-Commit-Position: refs/heads/master@{#382759}

[Evan Stade, 2016-03-22 01:21:01]

estade@chromium.org changed reviewers:
+ sky@chromium.org

[sky, 2016-03-22 15:31:40]

https://codereview.chromium.org/1817253003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/bar_control_button.cc (right):

https://codereview.chromium.org/1817253003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/bar_control_button.cc:56: SizeToPreferredSize();
What view has BarControlButton in it? I get nervous with random
SizeToPreferredSize as that is generally better down in the parent doing the
layout.

https://codereview.chromium.org/1817253003/diff/20001/ui/views/controls/butto...
File ui/views/controls/button/custom_button.cc (right):

https://codereview.chromium.org/1817253003/diff/20001/ui/views/controls/butto...
ui/views/controls/button/custom_button.cc:485: md_focus_ring_ = new
MdFocusRing();
DCHECK(!md_focus_ring_), or perhaps this code should early out if md_focus_ring_
is non-null?

[Evan Stade, 2016-03-22 17:35:44]

https://codereview.chromium.org/1817253003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/bar_control_button.cc (right):

https://codereview.chromium.org/1817253003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/bar_control_button.cc:56: SizeToPreferredSize();
On 2016/03/22 15:31:40, sky wrote:
> What view has BarControlButton in it? I get nervous with random
> SizeToPreferredSize as that is generally better down in the parent doing the
> layout.

infobarview, find bar, download shelf. The alternative is to reimplement
GetPreferredSize so it returns the right thing even before OnThemeChanged, but
that seemed like slightly duplicate logic to me and I couldn't think of a way
sizing this here would cause harm. The button is not likely to do the right
thing/look right if you don't size it to its preferred size so in some sense
it's an error if a parent doesn't do this.

https://codereview.chromium.org/1817253003/diff/20001/ui/views/controls/butto...
File ui/views/controls/button/custom_button.cc (right):

https://codereview.chromium.org/1817253003/diff/20001/ui/views/controls/butto...
ui/views/controls/button/custom_button.cc:485: md_focus_ring_ = new
MdFocusRing();
On 2016/03/22 15:31:40, sky wrote:
> DCHECK(!md_focus_ring_), or perhaps this code should early out if
md_focus_ring_
> is non-null?

Done.

[sky, 2016-03-22 18:02:27]

https://codereview.chromium.org/1817253003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/bar_control_button.cc (right):

https://codereview.chromium.org/1817253003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/bar_control_button.cc:56: SizeToPreferredSize();
On 2016/03/22 17:35:44, Evan Stade wrote:
> On 2016/03/22 15:31:40, sky wrote:
> > What view has BarControlButton in it? I get nervous with random
> > SizeToPreferredSize as that is generally better down in the parent doing the
> > layout.
> 
> infobarview, find bar, download shelf. The alternative is to reimplement
> GetPreferredSize so it returns the right thing even before OnThemeChanged, but
> that seemed like slightly duplicate logic to me and I couldn't think of a way
> sizing this here would cause harm. The button is not likely to do the right
> thing/look right if you don't size it to its preferred size so in some sense
> it's an error if a parent doesn't do this.

Doesn't find bar and the like need to relayout in this case? What triggers that?

[Evan Stade, 2016-03-22 18:16:11]

https://codereview.chromium.org/1817253003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/bar_control_button.cc (right):

https://codereview.chromium.org/1817253003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/bar_control_button.cc:56: SizeToPreferredSize();
On 2016/03/22 18:02:27, sky wrote:
> On 2016/03/22 17:35:44, Evan Stade wrote:
> > On 2016/03/22 15:31:40, sky wrote:
> > > What view has BarControlButton in it? I get nervous with random
> > > SizeToPreferredSize as that is generally better down in the parent doing
the
> > > layout.
> > 
> > infobarview, find bar, download shelf. The alternative is to reimplement
> > GetPreferredSize so it returns the right thing even before OnThemeChanged,
but
> > that seemed like slightly duplicate logic to me and I couldn't think of a
way
> > sizing this here would cause harm. The button is not likely to do the right
> > thing/look right if you don't size it to its preferred size so in some sense
> > it's an error if a parent doesn't do this.
> 
> Doesn't find bar and the like need to relayout in this case? What triggers
that?

This code path gets hit as soon as BarControlButton is added to the child
hierarchy (AddChildView -> OnNativeThemeChanged -> OnThemeChanged). The parent
will necessarily call Layout() after this.

Granted, this won't work if subsequent theme changes were to change the size of
the button, which they currently don't. Another way to write this would be:

if (parent() && size() != GetPreferredSize())
  parent()->Layout();

but then the parent has to call SizeToPreferredSize (in the sense that it's
probably an error to size it to anything else).

[sky, 2016-03-22 23:25:38]

On 2016/03/22 18:16:11, Evan Stade wrote:
>
https://codereview.chromium.org/1817253003/diff/20001/chrome/browser/ui/views...
> File chrome/browser/ui/views/bar_control_button.cc (right):
> 
>
https://codereview.chromium.org/1817253003/diff/20001/chrome/browser/ui/views...
> chrome/browser/ui/views/bar_control_button.cc:56: SizeToPreferredSize();
> On 2016/03/22 18:02:27, sky wrote:
> > On 2016/03/22 17:35:44, Evan Stade wrote:
> > > On 2016/03/22 15:31:40, sky wrote:
> > > > What view has BarControlButton in it? I get nervous with random
> > > > SizeToPreferredSize as that is generally better down in the parent doing
> the
> > > > layout.
> > > 
> > > infobarview, find bar, download shelf. The alternative is to reimplement
> > > GetPreferredSize so it returns the right thing even before OnThemeChanged,
> but
> > > that seemed like slightly duplicate logic to me and I couldn't think of a
> way
> > > sizing this here would cause harm. The button is not likely to do the
right
> > > thing/look right if you don't size it to its preferred size so in some
sense
> > > it's an error if a parent doesn't do this.
> > 
> > Doesn't find bar and the like need to relayout in this case? What triggers
> that?
> 
> This code path gets hit as soon as BarControlButton is added to the child
> hierarchy (AddChildView -> OnNativeThemeChanged -> OnThemeChanged). The parent
> will necessarily call Layout() after this.
> 
> Granted, this won't work if subsequent theme changes were to change the size
of
> the button, which they currently don't. Another way to write this would be:
> 
> if (parent() && size() != GetPreferredSize())
>   parent()->Layout();
> 
> but then the parent has to call SizeToPreferredSize (in the sense that it's
> probably an error to size it to anything else).

PreferredSizeChanged() was added with this in mind. Can you use it here?
In general the parent should be the one doing the layout. I would expect the
parent to be calling SizeToPreferredSize (If that makes sense), and not the
actual view.

[Evan Stade, 2016-03-22 23:47:01]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[Evan Stade, 2016-03-22 23:47:19]

On 2016/03/22 23:25:38, sky wrote:
> On 2016/03/22 18:16:11, Evan Stade wrote:
> >
>
https://codereview.chromium.org/1817253003/diff/20001/chrome/browser/ui/views...
> > File chrome/browser/ui/views/bar_control_button.cc (right):
> > 
> >
>
https://codereview.chromium.org/1817253003/diff/20001/chrome/browser/ui/views...
> > chrome/browser/ui/views/bar_control_button.cc:56: SizeToPreferredSize();
> > On 2016/03/22 18:02:27, sky wrote:
> > > On 2016/03/22 17:35:44, Evan Stade wrote:
> > > > On 2016/03/22 15:31:40, sky wrote:
> > > > > What view has BarControlButton in it? I get nervous with random
> > > > > SizeToPreferredSize as that is generally better down in the parent
doing
> > the
> > > > > layout.
> > > > 
> > > > infobarview, find bar, download shelf. The alternative is to reimplement
> > > > GetPreferredSize so it returns the right thing even before
OnThemeChanged,
> > but
> > > > that seemed like slightly duplicate logic to me and I couldn't think of
a
> > way
> > > > sizing this here would cause harm. The button is not likely to do the
> right
> > > > thing/look right if you don't size it to its preferred size so in some
> sense
> > > > it's an error if a parent doesn't do this.
> > > 
> > > Doesn't find bar and the like need to relayout in this case? What triggers
> > that?
> > 
> > This code path gets hit as soon as BarControlButton is added to the child
> > hierarchy (AddChildView -> OnNativeThemeChanged -> OnThemeChanged). The
parent
> > will necessarily call Layout() after this.
> > 
> > Granted, this won't work if subsequent theme changes were to change the size
> of
> > the button, which they currently don't. Another way to write this would be:
> > 
> > if (parent() && size() != GetPreferredSize())
> >   parent()->Layout();
> > 
> > but then the parent has to call SizeToPreferredSize (in the sense that it's
> > probably an error to size it to anything else).
> 
> PreferredSizeChanged() was added with this in mind. Can you use it here?
> In general the parent should be the one doing the layout. I would expect the
> parent to be calling SizeToPreferredSize (If that makes sense), and not the
> actual view.

I don't find PreferredSizeChanged() all that elegant since the default
implementation of ChildPreferredSizeChanged() is empty. It's clunky to force
anyone who adds a BarControlButton to also implement this function.

I've updated the patch so all parents call SizeToPreferredSize in Layout.

[commit-bot: I haz the power, 2016-03-22 23:47:28]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1817253003/50001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1817253003/50001

[sky, 2016-03-22 23:57:06]

LGTM

https://codereview.chromium.org/1817253003/diff/50001/chrome/browser/ui/views...
File chrome/browser/ui/views/find_bar_view.cc (right):

https://codereview.chromium.org/1817253003/diff/50001/chrome/browser/ui/views...
chrome/browser/ui/views/find_bar_view.cc:338: // First we draw the close button
on the far right.
draw->position.

[Evan Stade, 2016-03-23 00:00:26]

https://codereview.chromium.org/1817253003/diff/50001/chrome/browser/ui/views...
File chrome/browser/ui/views/find_bar_view.cc (right):

https://codereview.chromium.org/1817253003/diff/50001/chrome/browser/ui/views...
chrome/browser/ui/views/find_bar_view.cc:338: // First we draw the close button
on the far right.
On 2016/03/22 23:57:06, sky wrote:
> draw->position.

Done.

[Evan Stade, 2016-03-23 00:00:29]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2016-03-23 00:00:29]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1817253003/#ps70001
(title: "update docs")

[commit-bot: I haz the power, 2016-03-23 00:00:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1817253003/70001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1817253003/70001

[commit-bot: I haz the power, 2016-03-23 01:23:04]

Committed patchset #5 (id:70001)

[commit-bot: I haz the power, 2016-03-23 01:25:47]

Description was changed from

==========
[MD] Use same focus ring on BarControlButton as MdTextButton.

BUG=596033
==========

to

==========
[MD] Use same focus ring on BarControlButton as MdTextButton.

BUG=596033

Committed: https://crrev.com/ee3ec60a1260a0c72edaae3a4b72c90bae93a2d9
Cr-Commit-Position: refs/heads/master@{#382759}
==========

[commit-bot: I haz the power, 2016-03-23 01:25:48]

Patchset 5 (id:??) landed as
https://crrev.com/ee3ec60a1260a0c72edaae3a4b72c90bae93a2d9
Cr-Commit-Position: refs/heads/master@{#382759}