[Mac] Refactor the fullscreen toolbar animation

[Mac] Refactor the fullscreen toolbar animation

- Moved the animation logic from FullscreenToolbarController into a new
class
- Replaced DropdownAnimation with SlideAnimation

BUG=643418
Committed: https://crrev.com/aac459cb9f155cfe4f330d31616b5d6081042de7
Cr-Commit-Position: refs/heads/master@{#427137}

[spqchan, 2016-10-20 21:54:48]

Description was changed from

==========
FSTB Animation Refactor

BUG=
==========

to

==========
[Mac] Refactor the fullscreen toolbar animation

- Moved the animation logic from FullscreenToolbarController into a new 
class
- Replaced DropdownAnimation with SlideAnimation

BUG=643418
==========

[spqchan, 2016-10-20 21:54:48]

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

[spqchan, 2016-10-20 22:03:40]

PTAL

[erikchen, 2016-10-21 17:22:03]

https://codereview.chromium.org/2430403003/diff/60001/chrome/browser/ui/cocoa...
File
chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_animation_controller.h
(right):

https://codereview.chromium.org/2430403003/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_animation_controller.h:14:
namespace {
don't use anonymous namespaces in header files. Use FRIEND_TEST_ALL_PREFIXES
instead.

https://codereview.chromium.org/2430403003/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_animation_controller.h:69:
BOOL disable_animations_during_testing_;
I notice this isn't used yet. Given that this CL is part of a series of
refactors, I'm okay with the temporary presence of an unused variable.

That being said, changing behavior for tests to a large extension defeats the
point of testing [since you're no longer testing the same behavior]. I recommend
instead that all of your animate methods have a parameter BOOL:animated [This is
what appkit does] that changes the animation duration between N and 0. This way,
you can still test animations, just with a duration of 0.

https://codereview.chromium.org/2430403003/diff/60001/chrome/browser/ui/cocoa...
File
chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_animation_controller.mm
(right):

https://codereview.chromium.org/2430403003/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_animation_controller.mm:35:
base::Unretained(this)),
is it possible for there to be a task queued on the task queue after this object
is destroyed? Should we be using a weak ptr?

https://codereview.chromium.org/2430403003/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm (right):

https://codereview.chromium.org/2430403003/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:194:
animationController_->UpdateToolbarFraction([self toolbarFraction]);
please use a single source of truth for toolbarfraction - don't have an ivar in
both this class and animation controller.

In your design doc there's a "toolbarFraction" and a "currentToolbarFraction" -
can you describe the difference between the two?

[spqchan, 2016-10-21 18:26:55]

PTAL

https://codereview.chromium.org/2430403003/diff/60001/chrome/browser/ui/cocoa...
File
chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_animation_controller.h
(right):

https://codereview.chromium.org/2430403003/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_animation_controller.h:14:
namespace {
On 2016/10/21 17:22:03, erikchen wrote:
> don't use anonymous namespaces in header files. Use FRIEND_TEST_ALL_PREFIXES
> instead.

Removed

https://codereview.chromium.org/2430403003/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_animation_controller.h:69:
BOOL disable_animations_during_testing_;
On 2016/10/21 17:22:03, erikchen wrote:
> I notice this isn't used yet. Given that this CL is part of a series of
> refactors, I'm okay with the temporary presence of an unused variable.
> 
> That being said, changing behavior for tests to a large extension defeats the
> point of testing [since you're no longer testing the same behavior]. I
recommend
> instead that all of your animate methods have a parameter BOOL:animated [This
is
> what appkit does] that changes the animation duration between N and 0. This
way,
> you can still test animations, just with a duration of 0.

Removed this since it looks like the tests need to be change and I don't want to
have a lot of overhead.
I'm not sure what you mean. Do you mean something like AnimateToolbarIn(bool
should_animate)?

https://codereview.chromium.org/2430403003/diff/60001/chrome/browser/ui/cocoa...
File
chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_animation_controller.mm
(right):

https://codereview.chromium.org/2430403003/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_animation_controller.mm:35:
base::Unretained(this)),
On 2016/10/21 17:22:03, erikchen wrote:
> is it possible for there to be a task queued on the task queue after this
object
> is destroyed? Should we be using a weak ptr?

No, the Timer's deconstructor will stop and abandon the task.

https://codereview.chromium.org/2430403003/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm (right):

https://codereview.chromium.org/2430403003/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:194:
animationController_->UpdateToolbarFraction([self toolbarFraction]);
On 2016/10/21 17:22:03, erikchen wrote:
> please use a single source of truth for toolbarfraction - don't have an ivar
in
> both this class and animation controller.
> 
> In your design doc there's a "toolbarFraction" and a "currentToolbarFraction"
-
> can you describe the difference between the two?

Sorry, the design doc is currently outdated. I'm working on updating it.
Anyway, I renamed a few things so hopefully it's now more clear.
I'm storing the last toolbar fraction as the starting value for the animation.

[erikchen, 2016-10-21 19:36:59]

https://codereview.chromium.org/2430403003/diff/60001/chrome/browser/ui/cocoa...
File
chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_animation_controller.h
(right):

https://codereview.chromium.org/2430403003/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_animation_controller.h:69:
BOOL disable_animations_during_testing_;
yeah, but that wording is a little bit awkward. Maybe AnimateToolbarIn(bool
instantaneous)

https://codereview.chromium.org/2430403003/diff/80001/chrome/browser/ui/cocoa...
File
chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_animation_controller.h
(right):

https://codereview.chromium.org/2430403003/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_animation_controller.h:20:
// Called by |owner_| to when the fullscreen toolbar layout is updated.
s/to//

Also I'd recommend the name ToolbarDidUpdate, to indicate that this is reacting
to a change, rather than inducing one.

https://codereview.chromium.org/2430403003/diff/80001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm (right):

https://codereview.chromium.org/2430403003/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:254: if (slidingStyle_
== FullscreenSlidingStyle::OMNIBOX_TABS_NONE)
Don't we want toolbarFraction to return kHideFraction? Ditto for the other
places that return NO.

[spqchan, 2016-10-21 20:34:19]

PTAL

https://codereview.chromium.org/2430403003/diff/60001/chrome/browser/ui/cocoa...
File
chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_animation_controller.h
(right):

https://codereview.chromium.org/2430403003/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_animation_controller.h:69:
BOOL disable_animations_during_testing_;
On 2016/10/21 19:36:59, erikchen wrote:
> yeah, but that wording is a little bit awkward. Maybe AnimateToolbarIn(bool
> instantaneous)

Ah, gotcha. Parameters won't be necessary for this case since the friend tests
can directly set the duration.

https://codereview.chromium.org/2430403003/diff/80001/chrome/browser/ui/cocoa...
File
chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_animation_controller.h
(right):

https://codereview.chromium.org/2430403003/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_animation_controller.h:20:
// Called by |owner_| to when the fullscreen toolbar layout is updated.
On 2016/10/21 19:36:59, erikchen wrote:
> s/to//
> 
> Also I'd recommend the name ToolbarDidUpdate, to indicate that this is
reacting
> to a change, rather than inducing one.

Done.

https://codereview.chromium.org/2430403003/diff/80001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm (right):

https://codereview.chromium.org/2430403003/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:254: if (slidingStyle_
== FullscreenSlidingStyle::OMNIBOX_TABS_NONE)
On 2016/10/21 19:36:59, erikchen wrote:
> Don't we want toolbarFraction to return kHideFraction? Ditto for the other
> places that return NO.

I'm confused on what you're getting at.

[erikchen, 2016-10-21 20:39:28]

https://codereview.chromium.org/2430403003/diff/80001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm (right):

https://codereview.chromium.org/2430403003/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:238: return 1.0;
probably kShowFraction?

https://codereview.chromium.org/2430403003/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:254: if (slidingStyle_
== FullscreenSlidingStyle::OMNIBOX_TABS_NONE)
On 2016/10/21 20:34:18, spqchan wrote:
> On 2016/10/21 19:36:59, erikchen wrote:
> > Don't we want toolbarFraction to return kHideFraction? Ditto for the other
> > places that return NO.
> 
> I'm confused on what you're getting at.

Oh, I see what happened. The slidingStyle_ conditionals are duplicated here and
in lines 232-235. Semantically, the checks here make sense, but it's the
conditionals in lines 232-235 that actually cause kShowFraction and
kHideFraction to be returned.

[erikchen, 2016-10-21 20:56:52]

On 2016/10/21 20:39:28, erikchen wrote:
>
https://codereview.chromium.org/2430403003/diff/80001/chrome/browser/ui/cocoa...
> File chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm (right):
> 
>
https://codereview.chromium.org/2430403003/diff/80001/chrome/browser/ui/cocoa...
> chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:238: return 1.0;
> probably kShowFraction?
> 
>
https://codereview.chromium.org/2430403003/diff/80001/chrome/browser/ui/cocoa...
> chrome/browser/ui/cocoa/fullscreen_toolbar_controller.mm:254: if
(slidingStyle_
> == FullscreenSlidingStyle::OMNIBOX_TABS_NONE)
> On 2016/10/21 20:34:18, spqchan wrote:
> > On 2016/10/21 19:36:59, erikchen wrote:
> > > Don't we want toolbarFraction to return kHideFraction? Ditto for the other
> > > places that return NO.
> > 
> > I'm confused on what you're getting at.
> 
> Oh, I see what happened. The slidingStyle_ conditionals are duplicated here
and
> in lines 232-235. Semantically, the checks here make sense, but it's the
> conditionals in lines 232-235 that actually cause kShowFraction and
> kHideFraction to be returned.

lgtm % my last comment.

[spqchan, 2016-10-21 21:15:59]

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

[spqchan, 2016-10-21 21:15:59]

thanks!

+rsesek for ownership

[Robert Sesek, 2016-10-24 18:17:33]

https://codereview.chromium.org/2430403003/diff/120001/chrome/browser/ui/coco...
File
chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_animation_controller.h
(right):

https://codereview.chromium.org/2430403003/diff/120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_animation_controller.h:16:
class FullscreenToolbarAnimationController : public gfx::AnimationDelegate {
Class-level comment?

https://codereview.chromium.org/2430403003/diff/120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_animation_controller.h:18:
FullscreenToolbarAnimationController(FullscreenToolbarController* owner);
explicit

https://codereview.chromium.org/2430403003/diff/120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_animation_controller.h:35:
// Returns the toolbar fraction calculated from the animation progress.
Explain what the toolbar fraction is here.

https://codereview.chromium.org/2430403003/diff/120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_animation_controller.h:39:
BOOL IsAnimationRunning() const;
Use C++ bool here since the rest of the interface is C++y.

https://codereview.chromium.org/2430403003/diff/120001/chrome/browser/ui/coco...
File
chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_animation_controller.mm
(right):

https://codereview.chromium.org/2430403003/diff/120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_animation_controller.mm:53:
if ([owner_ mustShowFullscreenToolbar])
What's the logic behind this line?

[spqchan, 2016-10-24 18:55:19]

PTAL

https://codereview.chromium.org/2430403003/diff/120001/chrome/browser/ui/coco...
File
chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_animation_controller.h
(right):

https://codereview.chromium.org/2430403003/diff/120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_animation_controller.h:16:
class FullscreenToolbarAnimationController : public gfx::AnimationDelegate {
On 2016/10/24 18:17:32, Robert Sesek wrote:
> Class-level comment?

Done.

https://codereview.chromium.org/2430403003/diff/120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_animation_controller.h:18:
FullscreenToolbarAnimationController(FullscreenToolbarController* owner);
On 2016/10/24 18:17:32, Robert Sesek wrote:
> explicit

Done.

https://codereview.chromium.org/2430403003/diff/120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_animation_controller.h:35:
// Returns the toolbar fraction calculated from the animation progress.
On 2016/10/24 18:17:32, Robert Sesek wrote:
> Explain what the toolbar fraction is here.

Done.

https://codereview.chromium.org/2430403003/diff/120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_animation_controller.h:39:
BOOL IsAnimationRunning() const;
On 2016/10/24 18:17:32, Robert Sesek wrote:
> Use C++ bool here since the rest of the interface is C++y.

Done.

https://codereview.chromium.org/2430403003/diff/120001/chrome/browser/ui/coco...
File
chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_animation_controller.mm
(right):

https://codereview.chromium.org/2430403003/diff/120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_animation_controller.mm:53:
if ([owner_ mustShowFullscreenToolbar])
On 2016/10/24 18:17:33, Robert Sesek wrote:
> What's the logic behind this line?

Added a comment

[Robert Sesek, 2016-10-24 18:58:43]

LGTM

[spqchan, 2016-10-24 19:02:10]

On 2016/10/24 18:58:43, Robert Sesek wrote:
> LGTM

thanks!

[spqchan, 2016-10-24 19:02:14]

The CQ bit was checked by spqchan@chromium.org

[spqchan, 2016-10-24 19:02:15]

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

[commit-bot: I haz the power, 2016-10-24 19:03:25]

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, 2016-10-24 20:34:41]

Description was changed from

==========
[Mac] Refactor the fullscreen toolbar animation

- Moved the animation logic from FullscreenToolbarController into a new 
class
- Replaced DropdownAnimation with SlideAnimation

BUG=643418
==========

to

==========
[Mac] Refactor the fullscreen toolbar animation

- Moved the animation logic from FullscreenToolbarController into a new 
class
- Replaced DropdownAnimation with SlideAnimation

BUG=643418
==========

[commit-bot: I haz the power, 2016-10-24 20:34:42]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-10-24 20:36:51]

Description was changed from

==========
[Mac] Refactor the fullscreen toolbar animation

- Moved the animation logic from FullscreenToolbarController into a new 
class
- Replaced DropdownAnimation with SlideAnimation

BUG=643418
==========

to

==========
[Mac] Refactor the fullscreen toolbar animation

- Moved the animation logic from FullscreenToolbarController into a new
class
- Replaced DropdownAnimation with SlideAnimation

BUG=643418

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

[commit-bot: I haz the power, 2016-10-24 20:36:52]

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