Updates to overrides of InkDropHost::AddInkDropLayer()

Updates to overrides of InkDropHost::AddInkDropLayer()

Add a call to SetFillsBoundsOpaquely(false) in
ToolbarButton::AddInkDropLayer() and
AppMenuButton::AddInkDropLayer(). This prevents
a black background appearing (indicating
non-painted regions) when the buttons show a
focus rect and ripple simultaneously.

Furthermore, in ToolbarButton::AddInkDropLayer(),
schedule a repaint on the parent layer once the
ink drop is added. This prevents a stale image
from remaining visible, which appears to overlap
with the current image drawn on the newly-created
layer for image().

BUG=551492, 546683
TEST=manual

Committed: https://crrev.com/649ed114438aa8ebd56904145ef540b61556357f
Cr-Commit-Position: refs/heads/master@{#360589}

[tdanderson, 2015-11-18 16:14:17]

tdanderson@chromium.org changed reviewers:
+ pkasting@chromium.org

[tdanderson, 2015-11-18 16:14:17]

Hi Peter, can you please take a look?

https://codereview.chromium.org/1460733002/diff/1/chrome/browser/ui/views/bar...
File chrome/browser/ui/views/bar_control_button.cc (right):

https://codereview.chromium.org/1460733002/diff/1/chrome/browser/ui/views/bar...
chrome/browser/ui/views/bar_control_button.cc:75: //                         
the button's image.
BarControlButton extends ImageButton, which does not have a child ImageView that
can be promoted to its own layer (as is done in ToolbarButton and
AppMenuButton). Is there a simple way to put the BarControlButton's SkImage into
its own layer for the duration of the ink drop animation without needing to have
a child ImageView? (And is BarControlButton even staying around for the long
term? I remember some talk about getting rid of it.)

[bruthig, 2015-11-18 16:26:16]

bruthig@chromium.org changed reviewers:
+ bruthig@chromium.org

[bruthig, 2015-11-18 16:26:17]

Adding sadrul@ to address my comment(s) in
chrome/browser/ui/views/toolbar/toolbar_button.cc

https://codereview.chromium.org/1460733002/diff/1/chrome/browser/ui/views/too...
File chrome/browser/ui/views/toolbar/toolbar_button.cc (right):

https://codereview.chromium.org/1460733002/diff/1/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar/toolbar_button.cc:238:
parent()->SchedulePaint();
sadrul@, would it make sense to move this parent()->SchedulePaint() call to
View::SetFillsBoundsOpaquely() and invoke it if |fills_bounds_opaquely| is
false?

I'm thinking this would solve the more general problem and I can't think of a
valid case where a View would not want to repaint it's parent when
|fills_bounds_opaquely| is false.

Also what do you think about updating View::SchedulePaint() to call
parent()->SchedulePaint() when layer()->fills_bounds_opaquely() is false.  Again
I can't think of a valid case where we wouldn't want to re-paint the parent if
layer()->fills_bounds_opaquely() is false.

[jonross, 2015-11-18 16:36:51]

jonross@chromium.org changed reviewers:
+ jonross@chromium.org

[jonross, 2015-11-18 16:36:52]

https://codereview.chromium.org/1460733002/diff/1/chrome/browser/ui/views/bar...
File chrome/browser/ui/views/bar_control_button.cc (right):

https://codereview.chromium.org/1460733002/diff/1/chrome/browser/ui/views/bar...
chrome/browser/ui/views/bar_control_button.cc:75: //                         
the button's image.
On 2015/11/18 16:14:17, tdanderson wrote:
> BarControlButton extends ImageButton, which does not have a child ImageView
that
> can be promoted to its own layer (as is done in ToolbarButton and
> AppMenuButton). Is there a simple way to put the BarControlButton's SkImage
into
> its own layer for the duration of the ink drop animation without needing to
have
> a child ImageView? (And is BarControlButton even staying around for the long
> term? I remember some talk about getting rid of it.)

We can follow the pattern used for CircleLayerDelegate::OnPaintLayer

https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/animation...

Create our own child layer for BarControlButton, and give it a LayerDelegate
that paints the SkImage

https://codereview.chromium.org/1460733002/diff/1/chrome/browser/ui/views/too...
File chrome/browser/ui/views/toolbar/toolbar_button.cc (right):

https://codereview.chromium.org/1460733002/diff/1/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar/toolbar_button.cc:238:
parent()->SchedulePaint();
On 2015/11/18 16:26:17, bruthig wrote:
> sadrul@, would it make sense to move this parent()->SchedulePaint() call to
> View::SetFillsBoundsOpaquely() and invoke it if |fills_bounds_opaquely| is
> false?
> 
> I'm thinking this would solve the more general problem and I can't think of a
> valid case where a View would not want to repaint it's parent when
> |fills_bounds_opaquely| is false.
> 
> Also what do you think about updating View::SchedulePaint() to call
> parent()->SchedulePaint() when layer()->fills_bounds_opaquely() is false. 
Again
> I can't think of a valid case where we wouldn't want to re-paint the parent if
> layer()->fills_bounds_opaquely() is false.

I'm slightly surprised that changing the layer tree with SetPaintToLayer(true)
doesn't cause parent->SchedulePaint already

I'd like this centralized to avoid implementation issues elsewhere in the future

[bruthig, 2015-11-18 16:51:19]

https://codereview.chromium.org/1460733002/diff/1/chrome/browser/ui/views/too...
File chrome/browser/ui/views/toolbar/toolbar_button.cc (right):

https://codereview.chromium.org/1460733002/diff/1/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar/toolbar_button.cc:238:
parent()->SchedulePaint();
On 2015/11/18 16:36:52, jonross wrote:
> On 2015/11/18 16:26:17, bruthig wrote:
> > sadrul@, would it make sense to move this parent()->SchedulePaint() call to
> > View::SetFillsBoundsOpaquely() and invoke it if |fills_bounds_opaquely| is
> > false?
> > 
> > I'm thinking this would solve the more general problem and I can't think of
a
> > valid case where a View would not want to repaint it's parent when
> > |fills_bounds_opaquely| is false.
> > 
> > Also what do you think about updating View::SchedulePaint() to call
> > parent()->SchedulePaint() when layer()->fills_bounds_opaquely() is false. 
> Again
> > I can't think of a valid case where we wouldn't want to re-paint the parent
if
> > layer()->fills_bounds_opaquely() is false.
> 
> I'm slightly surprised that changing the layer tree with SetPaintToLayer(true)
> doesn't cause parent->SchedulePaint already
> 
> I'd like this centralized to avoid implementation issues elsewhere in the
future

I've posted my propsed changes here: https://codereview.chromium.org/1461673002/

[Peter Kasting, 2015-11-18 21:04:53]

I don't understand the gory details of painting and layers enough to answer your
concerns about auto-scheduling paints.  Assuming you decide this route is
correct (either for now or for good), LGTM.

https://codereview.chromium.org/1460733002/diff/1/chrome/browser/ui/views/bar...
File chrome/browser/ui/views/bar_control_button.cc (right):

https://codereview.chromium.org/1460733002/diff/1/chrome/browser/ui/views/bar...
chrome/browser/ui/views/bar_control_button.cc:75: //                         
the button's image.
On 2015/11/18 16:14:17, tdanderson wrote:
> (And is BarControlButton even staying around for the long
> term? I remember some talk about getting rid of it.)

I recall proposing that we try to use it more widely and, if successful, rename
it something more general, but I don't know that anyone's suggested getting rid
of it.

[tdanderson, 2015-11-19 15:15:42]

https://codereview.chromium.org/1460733002/diff/1/chrome/browser/ui/views/bar...
File chrome/browser/ui/views/bar_control_button.cc (right):

https://codereview.chromium.org/1460733002/diff/1/chrome/browser/ui/views/bar...
chrome/browser/ui/views/bar_control_button.cc:75: //                         
the button's image.
On 2015/11/18 21:04:53, Peter Kasting wrote:
> On 2015/11/18 16:14:17, tdanderson wrote:
> > (And is BarControlButton even staying around for the long
> > term? I remember some talk about getting rid of it.)
> 
> I recall proposing that we try to use it more widely and, if successful,
rename
> it something more general, but I don't know that anyone's suggested getting
rid
> of it.

Thanks for the suggestion; I will address this in a follow-up CL.

https://codereview.chromium.org/1460733002/diff/1/chrome/browser/ui/views/too...
File chrome/browser/ui/views/toolbar/toolbar_button.cc (right):

https://codereview.chromium.org/1460733002/diff/1/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar/toolbar_button.cc:238:
parent()->SchedulePaint();
On 2015/11/18 16:51:19, bruthig wrote:
> On 2015/11/18 16:36:52, jonross wrote:
> > On 2015/11/18 16:26:17, bruthig wrote:
> > > sadrul@, would it make sense to move this parent()->SchedulePaint() call
to
> > > View::SetFillsBoundsOpaquely() and invoke it if |fills_bounds_opaquely| is
> > > false?
> > > 
> > > I'm thinking this would solve the more general problem and I can't think
of
> a
> > > valid case where a View would not want to repaint it's parent when
> > > |fills_bounds_opaquely| is false.
> > > 
> > > Also what do you think about updating View::SchedulePaint() to call
> > > parent()->SchedulePaint() when layer()->fills_bounds_opaquely() is false. 
> > Again
> > > I can't think of a valid case where we wouldn't want to re-paint the
parent
> if
> > > layer()->fills_bounds_opaquely() is false.
> > 
> > I'm slightly surprised that changing the layer tree with
SetPaintToLayer(true)
> > doesn't cause parent->SchedulePaint already
> > 
> > I'd like this centralized to avoid implementation issues elsewhere in the
> future
> 
> I've posted my propsed changes here:
https://codereview.chromium.org/1461673002/

Note I'm landing this CL as-is, feel free to modify afterwards pending the
outcome of your other CL.

[tdanderson, 2015-11-19 15:15:56]

The CQ bit was checked by tdanderson@chromium.org

[commit-bot: I haz the power, 2015-11-19 15:16:19]

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

[commit-bot: I haz the power, 2015-11-19 15:46:53]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-11-19 15:48:23]

Patchset 1 (id:??) landed as
https://crrev.com/649ed114438aa8ebd56904145ef540b61556357f
Cr-Commit-Position: refs/heads/master@{#360589}