Refactored AppMenuAnimation

Refactored AppMenuAnimation

Refactored and moved AppMenuAnimation out of Views so
that it can be used in cross platforms.

AppMenuAnimation no longer paints the dot, instead it will
handle the animation and delegate the painting to
AppMenuAnimationDelegate.

BUG=704786
Review-Url: https://codereview.chromium.org/2858313002
Cr-Commit-Position: refs/heads/master@{#469885}
Committed: https://chromium.googlesource.com/chromium/src/+/6dc6477fbfd23d601d72dc1ad941699c65a47353

[spqchan, 2017-05-04 17:33:44]

Description was changed from

==========
Refactored AppMenuAnimation so that it can be used by Cocoa

BUG=704786
==========

to

==========
Refactored AppMenuAnimation

Moved AppMenuAnimation out of views so that it
can be used by the Mac platform.

BUG=704786
==========

[spqchan, 2017-05-04 17:33:45]

spqchan@chromium.org changed reviewers:
+ msw@chromium.org

[spqchan, 2017-05-04 17:35:00]

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

[commit-bot: I haz the power, 2017-05-04 17:35:37]

Dry run: CQ is trying da patch.

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

[spqchan, 2017-05-04 18:04:14]

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

[spqchan, 2017-05-04 18:04:51]

Description was changed from

==========
Refactored AppMenuAnimation

Moved AppMenuAnimation out of views so that it
can be used by the Mac platform.

BUG=704786
==========

to

==========
Refactored AppMenuAnimation

Moved AppMenuAnimation out of Views so that it
can be shared between platforms.

BUG=704786
==========

[commit-bot: I haz the power, 2017-05-04 18:05:00]

Dry run: CQ is trying da patch.

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

[spqchan, 2017-05-04 18:05:04]

Description was changed from

==========
Refactored AppMenuAnimation

Moved AppMenuAnimation out of Views so that it
can be shared between platforms.

BUG=704786
==========

to

==========
Refactored AppMenuAnimation

Moved AppMenuAnimation out of Views so that it can be 
shared between platforms.

BUG=704786
==========

[spqchan, 2017-05-04 18:11:29]

Patchset #2 (id:20001) has been deleted

[spqchan, 2017-05-04 18:14:20]

Description was changed from

==========
Refactored AppMenuAnimation

Moved AppMenuAnimation out of Views so that it can be 
shared between platforms.

BUG=704786
==========

to

==========
Refactored AppMenuAnimation

Refactored and moved AppMenuAnimation out of Views so
that it can be used in cross platforms.

AppMenuAnimation no longer paints the dot, instead it will
handle the animation and delegate the painting to
AppMenuAnimationDelegate.

BUG=704786
==========

[spqchan, 2017-05-04 18:14:24]

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

[commit-bot: I haz the power, 2017-05-04 18:14:50]

Dry run: 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-05-04 19:20:22]

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

[commit-bot: I haz the power, 2017-05-04 19:20:23]

Dry run: This issue passed the CQ dry run.

[spqchan, 2017-05-04 21:24:04]

Hey msw, PTAL. I’m moving this out of views so that this can also be used for
Mac.

To have an idea on how this will used in Mac, please look at
app_toolbar_button.mm in https://codereview.chromium.org/2859903003/ (WIP, will
land after this one). Let me know if you have any questions!

[msw, 2017-05-05 18:47:39]

Thanks for pointing me at the mac cl too, that's helpful.
Mostly lg, but I do have some questions and nits.

https://codereview.chromium.org/2858313002/diff/40001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/app_menu_animation.cc (right):

https://codereview.chromium.org/2858313002/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/app_menu_animation.cc:116: delegate->PaintDot(point,
color, gfx::SizeF(dot_width, dot_height), canvas);
Would #if defined(OS_MACOSX) && !BUILDFLAG(MAC_VIEWS_BROWSER) here be simpler?

https://codereview.chromium.org/2858313002/diff/40001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/app_menu_animation.h (right):

https://codereview.chromium.org/2858313002/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/app_menu_animation.h:21: class
AppMenuAnimationDelegate {
It'd be nice if we could use Skia on Cocoa too and avoid [some] delegation.

https://codereview.chromium.org/2858313002/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/app_menu_animation.h:23: // Paints a dot according to
the given parameters. |canvas| is null
nit: specify the coordinates for |point|? (screen space? button space?)

https://codereview.chromium.org/2858313002/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/app_menu_animation.h:25: virtual void PaintDot(const
gfx::PointF point,
nit: pass a rect instead of a point and a size?

https://codereview.chromium.org/2858313002/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/app_menu_animation.h:27: gfx::SizeF size,
nit: pass a const ref here (and for the point, or for the rect)

https://codereview.chromium.org/2858313002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/app_menu_button.cc (right):

https://codereview.chromium.org/2858313002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/app_menu_button.cc:44: constexpr float
kDotRadius = 2.0f;
nit: maybe this should be owned by app_menu_animation.cc and passed as an arg to
the PaintDot function (so we don't need to duplicate this in the non-views UI?)
That said, I'm not sure why it's 2.0f here and 8.0f in
https://codereview.chromium.org/2859903003/diff/100001/chrome/browser/ui/coco...
(ignore this if the radii should really be different per-platform)

https://codereview.chromium.org/2858313002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/app_menu_button.cc:176:
flags.setAntiAlias(true);
aside: would avoiding anti aliasing avoid the blur mentioned in the bug?

[spqchan, 2017-05-05 21:18:56]

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

[commit-bot: I haz the power, 2017-05-05 21:20:26]

Dry run: CQ is trying da patch.

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

[spqchan, 2017-05-05 21:20:54]

Thanks for pointing out using Skia on Cocoa, I wasn't aware that we have
something like that. I removed the PaintDot() function from the delegate class
and it's much easier. The mac CL has also been updated.

https://codereview.chromium.org/2858313002/diff/40001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/app_menu_animation.cc (right):

https://codereview.chromium.org/2858313002/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/app_menu_animation.cc:116: delegate->PaintDot(point,
color, gfx::SizeF(dot_width, dot_height), canvas);
On 2017/05/05 18:47:38, msw wrote:
> Would #if defined(OS_MACOSX) && !BUILDFLAG(MAC_VIEWS_BROWSER) here be simpler?

Removed the delegate method

https://codereview.chromium.org/2858313002/diff/40001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/app_menu_animation.h (right):

https://codereview.chromium.org/2858313002/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/app_menu_animation.h:21: class
AppMenuAnimationDelegate {
On 2017/05/05 18:47:38, msw wrote:
> It'd be nice if we could use Skia on Cocoa too and avoid [some] delegation.

Thanks for pointing that out! I had no idea that we already have something where
Skia can paint on Cocoa.
I got rid of the PaintDot function

https://codereview.chromium.org/2858313002/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/app_menu_animation.h:23: // Paints a dot according to
the given parameters. |canvas| is null
On 2017/05/05 18:47:38, msw wrote:
> nit: specify the coordinates for |point|? (screen space? button space?)

The function is gone

https://codereview.chromium.org/2858313002/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/app_menu_animation.h:25: virtual void PaintDot(const
gfx::PointF point,
On 2017/05/05 18:47:38, msw wrote:
> nit: pass a rect instead of a point and a size?

gone

https://codereview.chromium.org/2858313002/diff/40001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/app_menu_animation.h:27: gfx::SizeF size,
On 2017/05/05 18:47:38, msw wrote:
> nit: pass a const ref here (and for the point, or for the rect)

gone

https://codereview.chromium.org/2858313002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/app_menu_button.cc (right):

https://codereview.chromium.org/2858313002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/app_menu_button.cc:44: constexpr float
kDotRadius = 2.0f;
On 2017/05/05 18:47:38, msw wrote:
> nit: maybe this should be owned by app_menu_animation.cc and passed as an arg
to
> the PaintDot function (so we don't need to duplicate this in the non-views
UI?)
> That said, I'm not sure why it's 2.0f here and 8.0f in
>
https://codereview.chromium.org/2859903003/diff/100001/chrome/browser/ui/coco...
> (ignore this if the radii should really be different per-platform)

Removed the delegate's PaintDot function so this is no longer applicable.

https://codereview.chromium.org/2858313002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/app_menu_button.cc:176:
flags.setAntiAlias(true);
On 2017/05/05 18:47:38, msw wrote:
> aside: would avoiding anti aliasing avoid the blur mentioned in the bug?

That's a really good point. I tried to remove it and the dots look choppy
without it. It looks like I'll need to explore this option a little more.

[msw, 2017-05-05 21:26:25]

lgtm, fingers crossed someday we'll ship Views on Mac!

https://codereview.chromium.org/2858313002/diff/60001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/app_menu_animation.cc (right):

https://codereview.chromium.org/2858313002/diff/60001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/app_menu_animation.cc:51: // The radius of each dot in
the icon.
optional nit: reorder up, just above kDotYOffset or similar

[spqchan, 2017-05-05 21:37:34]

Thanks!

https://codereview.chromium.org/2858313002/diff/60001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/app_menu_animation.cc (right):

https://codereview.chromium.org/2858313002/diff/60001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/app_menu_animation.cc:51: // The radius of each dot in
the icon.
On 2017/05/05 21:26:24, msw wrote:
> optional nit: reorder up, just above kDotYOffset or similar

Done.

[spqchan, 2017-05-05 21:37:49]

The CQ bit was checked by spqchan@chromium.org

[spqchan, 2017-05-05 21:37:49]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2858313002/#ps80001
(title: "Fix for msw")

[commit-bot: I haz the power, 2017-05-05 21:38:56]

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-05-06 19:23:20]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1494020269348460,
"parent_rev": "e002d2d5d13051f259effd9cf98980d80e7c812e", "commit_rev":
"6dc6477fbfd23d601d72dc1ad941699c65a47353"}

[commit-bot: I haz the power, 2017-05-06 19:23:31]

Description was changed from

==========
Refactored AppMenuAnimation

Refactored and moved AppMenuAnimation out of Views so
that it can be used in cross platforms.

AppMenuAnimation no longer paints the dot, instead it will
handle the animation and delegate the painting to
AppMenuAnimationDelegate.

BUG=704786
==========

to

==========
Refactored AppMenuAnimation

Refactored and moved AppMenuAnimation out of Views so
that it can be used in cross platforms.

AppMenuAnimation no longer paints the dot, instead it will
handle the animation and delegate the painting to
AppMenuAnimationDelegate.

BUG=704786

Review-Url: https://codereview.chromium.org/2858313002
Cr-Commit-Position: refs/heads/master@{#469885}
Committed:
https://chromium.googlesource.com/chromium/src/+/6dc6477fbfd23d601d72dc1ad941...
==========

[commit-bot: I haz the power, 2017-05-06 19:23:32]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/6dc6477fbfd23d601d72dc1ad941...