Centered flood fill style ink drop ripples on mouse/touch points.

Centered flood fill style ink drop ripples on LocatedEvents.

BUG=597273
TEST=InkDropHostViewTest.*
TEST=FloodFillInkDropRipple.MaxDistanceToCorners

Committed: https://crrev.com/854a6b15453fce54325ecfe2611a4c854ad7573c
Cr-Commit-Position: refs/heads/master@{#400666}

[bruthig, 2016-06-14 20:31:10]

Description was changed from

==========
Centered flood fill style ink drop ripples on mouse/touch points.

Depends on: https://codereview.chromium.org/2041033002/

BUG=597273
==========

to

==========
Centered flood fill style ink drop ripples on LocatedEvents.

BUG=597273

TEST=InkDropHostViewTest.*
TEST=RectTest.MaxDistanceToCorners
==========

[bruthig, 2016-06-14 20:31:46]

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

[bruthig, 2016-06-14 20:31:47]

estade@, can you take a first look?

[Evan Stade, 2016-06-14 21:13:24]

https://codereview.chromium.org/2042073002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/download/download_item_view_md.cc (right):

https://codereview.chromium.org/2042073002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view_md.cc:759:
AnimateInkDrop(views::InkDropState::ACTION_TRIGGERED, &event);
I'd just as soon move this out to the two OpenDownload callsites.

https://codereview.chromium.org/2042073002/diff/20001/ui/views/animation/ink_...
File ui/views/animation/ink_drop_host_view.h (right):

https://codereview.chromium.org/2042073002/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_host_view.h:43: const ui::Event*
last_ripple_triggering_event() const {
is this used somewhere

https://codereview.chromium.org/2042073002/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_host_view.h:55: // Returns the point of the
|last_ripple_triggereing_event_| if it was a
nit: spelling

https://codereview.chromium.org/2042073002/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_host_view.h:66: void AnimateInkDrop(InkDropState
state, const ui::Event* event);
|event| should be a located event to make it more explicit that non-located
events are totally ignored

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

https://codereview.chromium.org/2042073002/diff/20001/ui/views/controls/butto...
ui/views/controls/button/label_button.cc:444: ? CreateDefaultInkDropRipple(
if you make this InkDropHostView::CreateInkDropRipple then I think you can make
CreateDefaultInkDropRipple private

(actually I don't know why this has to change from
CustomButton::CreateInkDropRipple in the first place)

[bruthig, 2016-06-14 21:57:01]

estade@, PTAL

https://codereview.chromium.org/2042073002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/download/download_item_view_md.cc (right):

https://codereview.chromium.org/2042073002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view_md.cc:759:
AnimateInkDrop(views::InkDropState::ACTION_TRIGGERED, &event);
On 2016/06/14 21:13:24, Evan Stade wrote:
> I'd just as soon move this out to the two OpenDownload callsites.

Done.

https://codereview.chromium.org/2042073002/diff/20001/ui/views/animation/ink_...
File ui/views/animation/ink_drop_host_view.h (right):

https://codereview.chromium.org/2042073002/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_host_view.h:43: const ui::Event*
last_ripple_triggering_event() const {
On 2016/06/14 21:13:24, Evan Stade wrote:
> is this used somewhere

Removed.

https://codereview.chromium.org/2042073002/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_host_view.h:55: // Returns the point of the
|last_ripple_triggereing_event_| if it was a
On 2016/06/14 21:13:24, Evan Stade wrote:
> nit: spelling

Done.

https://codereview.chromium.org/2042073002/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_host_view.h:66: void AnimateInkDrop(InkDropState
state, const ui::Event* event);
On 2016/06/14 21:13:24, Evan Stade wrote:
> |event| should be a located event to make it more explicit that non-located
> events are totally ignored

Done.

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

https://codereview.chromium.org/2042073002/diff/20001/ui/views/controls/butto...
ui/views/controls/button/label_button.cc:444: ? CreateDefaultInkDropRipple(
On 2016/06/14 21:13:24, Evan Stade wrote:
> if you make this InkDropHostView::CreateInkDropRipple then I think you can
make
> CreateDefaultInkDropRipple private
> 
> (actually I don't know why this has to change from
> CustomButton::CreateInkDropRipple in the first place)

The center point is different, CustomButton::CreateInkDropRipple() puts the
centerpoint at the local bounds center whereas this puts the center behind the
image i.e. 'image()->GetMirroredBounds()'. this is needed because the 'Back'
button and AppMenuButtons extend their bounds and aren't square when the Window
is maximized.

[Evan Stade, 2016-06-15 18:14:09]

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

https://codereview.chromium.org/2042073002/diff/20001/ui/views/controls/butto...
ui/views/controls/button/label_button.cc:444: ? CreateDefaultInkDropRipple(
On 2016/06/14 21:57:01, bruthig wrote:
> On 2016/06/14 21:13:24, Evan Stade wrote:
> > if you make this InkDropHostView::CreateInkDropRipple then I think you can
> make
> > CreateDefaultInkDropRipple private
> > 
> > (actually I don't know why this has to change from
> > CustomButton::CreateInkDropRipple in the first place)
> 
> The center point is different, CustomButton::CreateInkDropRipple() puts the
> centerpoint at the local bounds center whereas this puts the center behind the
> image i.e. 'image()->GetMirroredBounds()'. this is needed because the 'Back'
> button and AppMenuButtons extend their bounds and aren't square when the
Window
> is maximized.

ah I see that this logic used to be in GetInkDropCenter. Why didn't you change
InkDropHostView::GetInkDropCenter() to return based on the last event? Instead
you added a new method.

https://codereview.chromium.org/2042073002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/download/download_item_view_md.cc (right):

https://codereview.chromium.org/2042073002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view_md.cc:453: // OpenDownload
may delete this, so don't add any code after this line.
nit: comment should be lower

[bruthig, 2016-06-15 18:52:03]

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

https://codereview.chromium.org/2042073002/diff/20001/ui/views/controls/butto...
ui/views/controls/button/label_button.cc:444: ? CreateDefaultInkDropRipple(
On 2016/06/15 18:14:09, Evan Stade wrote:
> On 2016/06/14 21:57:01, bruthig wrote:
> > On 2016/06/14 21:13:24, Evan Stade wrote:
> > > if you make this InkDropHostView::CreateInkDropRipple then I think you can
> > make
> > > CreateDefaultInkDropRipple private
> > > 
> > > (actually I don't know why this has to change from
> > > CustomButton::CreateInkDropRipple in the first place)
> > 
> > The center point is different, CustomButton::CreateInkDropRipple() puts the
> > centerpoint at the local bounds center whereas this puts the center behind
the
> > image i.e. 'image()->GetMirroredBounds()'. this is needed because the 'Back'
> > button and AppMenuButtons extend their bounds and aren't square when the
> Window
> > is maximized.
> 
> ah I see that this logic used to be in GetInkDropCenter. Why didn't you change
> InkDropHostView::GetInkDropCenter() to return based on the last event? Instead
> you added a new method.

Because the buttons using the SquareInkDropRipple shouldn't be centered on the
last event and currently the SquareInkDropRipple is the default one created by
InkDropHostView.

https://codereview.chromium.org/2042073002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/download/download_item_view_md.cc (right):

https://codereview.chromium.org/2042073002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view_md.cc:453: // OpenDownload
may delete this, so don't add any code after this line.
On 2016/06/15 18:14:09, Evan Stade wrote:
> nit: comment should be lower

Done.

[Evan Stade, 2016-06-15 20:34:42]

lg aside from one comment

https://codereview.chromium.org/2042073002/diff/80001/ui/views/animation/ink_...
File ui/views/animation/ink_drop_host_view.cc (right):

https://codereview.chromium.org/2042073002/diff/80001/ui/views/animation/ink_...
ui/views/animation/ink_drop_host_view.cc:144: const ui::LocatedEvent*
InkDropHostView::AsLocatedEvent(
I'm not a big fan of putting this here. It's not particularly relevant to
InkDropHostView. I'd add it to Event ("MaybeAsLocatedEvent"/"ToLocatedEvent" or
something.). If the owners of Event don't think it's worth having such a
function, then you'll just have to insert the ternary everywhere.

[bruthig, 2016-06-15 21:43:18]

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

[bruthig, 2016-06-15 21:43:20]

sky@, can you PTAL?

https://codereview.chromium.org/2042073002/diff/80001/ui/views/animation/ink_...
File ui/views/animation/ink_drop_host_view.cc (right):

https://codereview.chromium.org/2042073002/diff/80001/ui/views/animation/ink_...
ui/views/animation/ink_drop_host_view.cc:144: const ui::LocatedEvent*
InkDropHostView::AsLocatedEvent(
On 2016/06/15 20:34:42, Evan Stade wrote:
> I'm not a big fan of putting this here. It's not particularly relevant to
> InkDropHostView. I'd add it to Event ("MaybeAsLocatedEvent"/"ToLocatedEvent"
or
> something.). If the owners of Event don't think it's worth having such a
> function, then you'll just have to insert the ternary everywhere.

Moved to event_utils(h|cc) as ToLocatedEventOrNull(...).

[Evan Stade, 2016-06-15 23:04:09]

On 2016/06/15 21:43:20, bruthig wrote:
> sky@, can you PTAL?
> 
>
https://codereview.chromium.org/2042073002/diff/80001/ui/views/animation/ink_...
> File ui/views/animation/ink_drop_host_view.cc (right):
> 
>
https://codereview.chromium.org/2042073002/diff/80001/ui/views/animation/ink_...
> ui/views/animation/ink_drop_host_view.cc:144: const ui::LocatedEvent*
> InkDropHostView::AsLocatedEvent(
> On 2016/06/15 20:34:42, Evan Stade wrote:
> > I'm not a big fan of putting this here. It's not particularly relevant to
> > InkDropHostView. I'd add it to Event ("MaybeAsLocatedEvent"/"ToLocatedEvent"
> or
> > something.). If the owners of Event don't think it's worth having such a
> > function, then you'll just have to insert the ternary everywhere.
> 
> Moved to event_utils(h|cc) as ToLocatedEventOrNull(...).

I don't know why it wouldn't belong alongside Event::AsLocatedEvent. gfx::Image
has both AsImageSkia and ToImageSkia, this seems similar. (The other option
would be to relax the "must be a located event" restriction on AsLocatedEvent,
but I assume someone felt that was a valuable check.)

[sky, 2016-06-16 16:50:01]

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

[sky, 2016-06-16 16:50:03]

https://codereview.chromium.org/2042073002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/icon_label_bubble_view.h (right):

https://codereview.chromium.org/2042073002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:79: virtual bool
OnActivate(const ui::Event* event);
Can this be a const ui::Event& ? AFAICT event is never null, but perhaps I'm
missing a call?

https://codereview.chromium.org/2042073002/diff/100001/ui/events/event_utils.h
File ui/events/event_utils.h (right):

https://codereview.chromium.org/2042073002/diff/100001/ui/events/event_utils....
ui/events/event_utils.h:184: EVENTS_EXPORT const ui::LocatedEvent*
ToLocatedEventOrNull(
+sadrul for his thoughts. I prefer a static on LocatedEvent, and I would go for
a more verbose name to reinforce it checks the type e.g.
LocatedEvent::AsLocatedEventIfLocatedEvent().

https://codereview.chromium.org/2042073002/diff/100001/ui/gfx/geometry/rect.h
File ui/gfx/geometry/rect.h (right):

https://codereview.chromium.org/2042073002/diff/100001/ui/gfx/geometry/rect.h...
ui/gfx/geometry/rect.h:181: float MaxDistanceToCorners(const gfx::Point& point)
const;
This is very specialized and AFAICT only needed in one place. Can you move to
where it is used?

[bruthig, 2016-06-16 19:50:02]

sky@, can you PTAL?

sadrul@, can you take a look at:
 - LocatedEvent::AsLocatedEventIfLocatedEvent()

https://codereview.chromium.org/2042073002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/icon_label_bubble_view.h (right):

https://codereview.chromium.org/2042073002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:79: virtual bool
OnActivate(const ui::Event* event);
On 2016/06/16 16:50:02, sky wrote:
> Can this be a const ui::Event& ? AFAICT event is never null, but perhaps I'm
> missing a call?

Done.

https://codereview.chromium.org/2042073002/diff/100001/ui/events/event_utils.h
File ui/events/event_utils.h (right):

https://codereview.chromium.org/2042073002/diff/100001/ui/events/event_utils....
ui/events/event_utils.h:184: EVENTS_EXPORT const ui::LocatedEvent*
ToLocatedEventOrNull(
On 2016/06/16 16:50:02, sky wrote:
> +sadrul for his thoughts. I prefer a static on LocatedEvent, and I would go
for
> a more verbose name to reinforce it checks the type e.g.
> LocatedEvent::AsLocatedEventIfLocatedEvent().

Moved to LocatedEvent::AsLocatedEventIfLocatedEvent(), will wait for sadrul@'s
input though.

https://codereview.chromium.org/2042073002/diff/100001/ui/gfx/geometry/rect.h
File ui/gfx/geometry/rect.h (right):

https://codereview.chromium.org/2042073002/diff/100001/ui/gfx/geometry/rect.h...
ui/gfx/geometry/rect.h:181: float MaxDistanceToCorners(const gfx::Point& point)
const;
On 2016/06/16 16:50:02, sky wrote:
> This is very specialized and AFAICT only needed in one place. Can you move to
> where it is used?

Done.

[sky, 2016-06-16 22:03:09]

https://codereview.chromium.org/2042073002/diff/120001/ui/views/controls/butt...
File ui/views/controls/button/custom_button.cc (right):

https://codereview.chromium.org/2042073002/diff/120001/ui/views/controls/butt...
ui/views/controls/button/custom_button.cc:167:
SetState(ShouldEnterPushedState(event) ? STATE_PRESSED : STATE_HOVERED);
Why can't you use should_enter_pushed here?

[bruthig, 2016-06-17 13:39:59]

sky@, PTAL

https://codereview.chromium.org/2042073002/diff/120001/ui/views/controls/butt...
File ui/views/controls/button/custom_button.cc (right):

https://codereview.chromium.org/2042073002/diff/120001/ui/views/controls/butt...
ui/views/controls/button/custom_button.cc:167:
SetState(ShouldEnterPushedState(event) ? STATE_PRESSED : STATE_HOVERED);
On 2016/06/16 22:03:09, sky wrote:
> Why can't you use should_enter_pushed here?

Whoops,  fixed.

[sky, 2016-06-17 14:58:51]

LGTM

[sadrul, 2016-06-17 16:03:22]

https://codereview.chromium.org/2042073002/diff/100001/ui/events/event_utils.h
File ui/events/event_utils.h (right):

https://codereview.chromium.org/2042073002/diff/100001/ui/events/event_utils....
ui/events/event_utils.h:184: EVENTS_EXPORT const ui::LocatedEvent*
ToLocatedEventOrNull(
On 2016/06/16 19:50:02, bruthig wrote:
> On 2016/06/16 16:50:02, sky wrote:
> > +sadrul for his thoughts. I prefer a static on LocatedEvent, and I would go
> for
> > a more verbose name to reinforce it checks the type e.g.
> > LocatedEvent::AsLocatedEventIfLocatedEvent().
> 
> Moved to LocatedEvent::AsLocatedEventIfLocatedEvent(), will wait for sadrul@'s
> input though.

We could have LocatedEvent::From(ui::Event*); which could return null (we have
some code in other parts that use similar mechanism, e.g.
RenderViewHost::From(RenderWidgetHost*)). We could be a bit more verbose though,
e.g. MaybeFrom(ui::Event*), or FromIfValid(ui::Event*).

[bruthig, 2016-06-17 17:33:16]

Description was changed from

==========
Centered flood fill style ink drop ripples on LocatedEvents.

BUG=597273

TEST=InkDropHostViewTest.*
TEST=RectTest.MaxDistanceToCorners
==========

to

==========
Centered flood fill style ink drop ripples on LocatedEvents.

BUG=597273

TEST=InkDropHostViewTest.*
TEST=FloodFillInkDropRipple.MaxDistanceToCorners
==========

[bruthig, 2016-06-17 17:36:31]

On 2016/06/17 16:03:22, sadrul wrote:
> https://codereview.chromium.org/2042073002/diff/100001/ui/events/event_utils.h
> File ui/events/event_utils.h (right):
> 
>
https://codereview.chromium.org/2042073002/diff/100001/ui/events/event_utils....
> ui/events/event_utils.h:184: EVENTS_EXPORT const ui::LocatedEvent*
> ToLocatedEventOrNull(
> On 2016/06/16 19:50:02, bruthig wrote:
> > On 2016/06/16 16:50:02, sky wrote:
> > > +sadrul for his thoughts. I prefer a static on LocatedEvent, and I would
go
> > for
> > > a more verbose name to reinforce it checks the type e.g.
> > > LocatedEvent::AsLocatedEventIfLocatedEvent().
> > 
> > Moved to LocatedEvent::AsLocatedEventIfLocatedEvent(), will wait for
sadrul@'s
> > input though.
> 
> We could have LocatedEvent::From(ui::Event*); which could return null (we have
> some code in other parts that use similar mechanism, e.g.
> RenderViewHost::From(RenderWidgetHost*)). We could be a bit more verbose
though,
> e.g. MaybeFrom(ui::Event*), or FromIfValid(ui::Event*).

Updated to LocatedEvent::FromIfValid().

No need to review but do so if you wish, I plan to submit on Monday.

[bruthig, 2016-06-20 12:09:17]

The CQ bit was checked by bruthig@chromium.org

[bruthig, 2016-06-20 12:09:17]

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/2042073002/#ps180001
(title: "Renamed LocatedEvent::AsLocatedIfLocatedEvent() as FromIfValid().")

[commit-bot: I haz the power, 2016-06-20 12:09:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2042073002/180001

[commit-bot: I haz the power, 2016-06-20 13:15:53]

Description was changed from

==========
Centered flood fill style ink drop ripples on LocatedEvents.

BUG=597273

TEST=InkDropHostViewTest.*
TEST=FloodFillInkDropRipple.MaxDistanceToCorners
==========

to

==========
Centered flood fill style ink drop ripples on LocatedEvents.

BUG=597273

TEST=InkDropHostViewTest.*
TEST=FloodFillInkDropRipple.MaxDistanceToCorners
==========

[commit-bot: I haz the power, 2016-06-20 13:15:54]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-06-20 13:17:21]

Description was changed from

==========
Centered flood fill style ink drop ripples on LocatedEvents.

BUG=597273

TEST=InkDropHostViewTest.*
TEST=FloodFillInkDropRipple.MaxDistanceToCorners
==========

to

==========
Centered flood fill style ink drop ripples on LocatedEvents.

BUG=597273

TEST=InkDropHostViewTest.*
TEST=FloodFillInkDropRipple.MaxDistanceToCorners

Committed: https://crrev.com/854a6b15453fce54325ecfe2611a4c854ad7573c
Cr-Commit-Position: refs/heads/master@{#400666}
==========

[commit-bot: I haz the power, 2016-06-20 13:17:22]

Patchset 10 (id:??) landed as
https://crrev.com/854a6b15453fce54325ecfe2611a4c854ad7573c
Cr-Commit-Position: refs/heads/master@{#400666}