Allow a LocatedEvent to be passed to MenuButton, so the ink drop animation is centered on the click…

Allow a LocatedEvent to be passed to MenuButton, so the ink drop animation is centered on the click location.

The new constructor overload is used in https://codereview.chromium.org/2832823002/

BUG=635699
Review-Url: https://codereview.chromium.org/2831353002
Cr-Commit-Position: refs/heads/master@{#466922}
Committed: https://chromium.googlesource.com/chromium/src/+/b1d346cc88d41c6a5b1dd5ac47327c9ee7c6d7e7

[emx, 2017-04-21 14:12:01]

Description was changed from

==========
Allow a LocatedEvent to be passed to MenuButton, so the ink drop animation is
centered on the click location

BUG=635699
==========

to

==========
Allow a LocatedEvent to be passed to MenuButton, so the ink drop animation is
centered on the click location.

The new constructor overload is used in
https://codereview.chromium.org/2832823002/

BUG=635699
==========

[emx, 2017-04-21 15:32:01]

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

[emx, 2017-04-21 15:32:02]

Please take a look

[sky, 2017-04-21 20:32:33]

This looks fine to me. How about test coverage?

https://codereview.chromium.org/2831353002/diff/1/ui/views/controls/button/me...
File ui/views/controls/button/menu_button.h (right):

https://codereview.chromium.org/2831353002/diff/1/ui/views/controls/button/me...
ui/views/controls/button/menu_button.h:37: const ui::LocatedEvent* event);
Document event.

https://codereview.chromium.org/2831353002/diff/1/ui/views/controls/button/me...
ui/views/controls/button/menu_button.h:115: const ui::LocatedEvent* event);
Please document |event| and say it may be null.

[emx, 2017-04-24 13:49:06]

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

[commit-bot: I haz the power, 2017-04-24 13:49:37]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[emx, 2017-04-24 13:50:59]

Changes made, please take another look.

[sky, 2017-04-24 15:07:13]

LGTM with the following.

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

https://codereview.chromium.org/2831353002/diff/20001/ui/views/controls/butto...
ui/views/controls/button/menu_button_unittest.cc:348: auto click_point =
gfx::Point(6, 8);
IMO auto here is overkill:

gfx::Point click_point(6, 8) is shorter. Same comment on 363 and 364 below.

https://codereview.chromium.org/2831353002/diff/20001/ui/views/controls/butto...
ui/views/controls/button/menu_button_unittest.cc:367:
std::unique_ptr<MenuButton::PressedLock> pressed_lock(
There is no need for the unique_ptr here. Declared pressed_lock on the stack.

[commit-bot: I haz the power, 2017-04-24 15:16:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-24 15:16:41]

Dry run: This issue passed the CQ dry run.

[emx, 2017-04-24 16:30:42]

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

[commit-bot: I haz the power, 2017-04-24 16:31:12]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[emx, 2017-04-24 16:32:07]

OK, I've addressed your comments, thanks!

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

https://codereview.chromium.org/2831353002/diff/20001/ui/views/controls/butto...
ui/views/controls/button/menu_button_unittest.cc:348: auto click_point =
gfx::Point(6, 8);
On 2017/04/24 15:07:13, sky wrote:
> IMO auto here is overkill:
> 
> gfx::Point click_point(6, 8) is shorter. Same comment on 363 and 364 below.

Oh, yes, of course. :)

https://codereview.chromium.org/2831353002/diff/20001/ui/views/controls/butto...
ui/views/controls/button/menu_button_unittest.cc:367:
std::unique_ptr<MenuButton::PressedLock> pressed_lock(
On 2017/04/24 15:07:13, sky wrote:
> There is no need for the unique_ptr here. Declared pressed_lock on the stack.

Done.

[commit-bot: I haz the power, 2017-04-24 17:30:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-24 17:30:42]

Dry run: This issue passed the CQ dry run.

[sky, 2017-04-24 19:55:45]

SLGTM

[emx, 2017-04-25 08:19:12]

The CQ bit was checked by emx@chromium.org

[emx, 2017-04-25 08:19:12]

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/2831353002/#ps40001
(title: "Addressed review comments")

[commit-bot: I haz the power, 2017-04-25 08:19:23]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-25 08:22:39]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493108352352100,
"parent_rev": "393c304c284a4f1e79c55e1547d4399a63eb2ff9", "commit_rev":
"b1d346cc88d41c6a5b1dd5ac47327c9ee7c6d7e7"}

[commit-bot: I haz the power, 2017-04-25 08:22:48]

Description was changed from

==========
Allow a LocatedEvent to be passed to MenuButton, so the ink drop animation is
centered on the click location.

The new constructor overload is used in
https://codereview.chromium.org/2832823002/

BUG=635699
==========

to

==========
Allow a LocatedEvent to be passed to MenuButton, so the ink drop animation is
centered on the click location.

The new constructor overload is used in
https://codereview.chromium.org/2832823002/

BUG=635699

Review-Url: https://codereview.chromium.org/2831353002
Cr-Commit-Position: refs/heads/master@{#466922}
Committed:
https://chromium.googlesource.com/chromium/src/+/b1d346cc88d41c6a5b1dd5ac4732...
==========

[commit-bot: I haz the power, 2017-04-25 08:22:50]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/b1d346cc88d41c6a5b1dd5ac4732...