Do not use NSAnimation to drive scrollbar animations.

Do not use NSAnimation to drive scrollbar animations.

NSAnimation requires a connection to WindowServer. Since it's only really being
used to drive an animation timer, simply reimplement this with Blink primitives.

BUG=397642, 306348, 325938
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181388

[Robert Sesek, 2014-09-02 20:07:45]

Patchset #2 (id:20001) has been deleted

[Robert Sesek, 2014-09-02 20:10:52]

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

[Robert Sesek, 2014-09-02 20:10:53]

asvitkine: Please review/WDYT?
ccameron: FYI

[Alexei Svitkine (slow), 2014-09-03 15:39:36]

Seems reasonable to me, LGTM.

https://codereview.chromium.org/529103002/diff/40001/Source/platform/mac/Scro...
File Source/platform/mac/ScrollAnimatorMac.mm (right):

https://codereview.chromium.org/529103002/diff/40001/Source/platform/mac/Scro...
Source/platform/mac/ScrollAnimatorMac.mm:299: class
WebScrollbarPartAnimationTimer {
Nit: Not sure on Blink style, but maybe this should be in an anon namespace?

https://codereview.chromium.org/529103002/diff/40001/Source/platform/mac/Scro...
Source/platform/mac/ScrollAnimatorMac.mm:299: class
WebScrollbarPartAnimationTimer {
Can you add a comment on the class briefly summarizing its purpose (i.e. to
provide functionality of NSAnimation without a connection to the window server).

https://codereview.chromium.org/529103002/diff/40001/Source/platform/mac/Scro...
Source/platform/mac/ScrollAnimatorMac.mm:316: m_timer.startRepeating(1.0 / 35.0,
FROM_HERE);
Nit: Can you add a comment about where this constant comes from? If it matches
what the system libraries do, have you checked whether it's consistent between
different OS versions?

[Robert Sesek, 2014-09-03 16:51:11]

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

[Robert Sesek, 2014-09-03 16:51:11]

+kbr for platform/OWNERS and review

Thanks, Alexei! Feedback addressed.

https://codereview.chromium.org/529103002/diff/40001/Source/platform/mac/Scro...
File Source/platform/mac/ScrollAnimatorMac.mm (right):

https://codereview.chromium.org/529103002/diff/40001/Source/platform/mac/Scro...
Source/platform/mac/ScrollAnimatorMac.mm:299: class
WebScrollbarPartAnimationTimer {
On 2014/09/03 15:39:35, Alexei Svitkine wrote:
> Can you add a comment on the class briefly summarizing its purpose (i.e. to
> provide functionality of NSAnimation without a connection to the window
server).

Done.

https://codereview.chromium.org/529103002/diff/40001/Source/platform/mac/Scro...
Source/platform/mac/ScrollAnimatorMac.mm:299: class
WebScrollbarPartAnimationTimer {
On 2014/09/03 15:39:36, Alexei Svitkine wrote:
> Nit: Not sure on Blink style, but maybe this should be in an anon namespace?

I codesearched and it seems mostly to be for tests that anon is used. I did move
this into the blink:: namespace below, though.

https://codereview.chromium.org/529103002/diff/40001/Source/platform/mac/Scro...
Source/platform/mac/ScrollAnimatorMac.mm:316: m_timer.startRepeating(1.0 / 35.0,
FROM_HERE);
On 2014/09/03 15:39:35, Alexei Svitkine wrote:
> Nit: Can you add a comment about where this constant comes from? If it matches
> what the system libraries do, have you checked whether it's consistent between
> different OS versions?

Done. All OS versions use a framerate of 60Hz, though I determined that 35 Hz is
sufficient for these animations.

[Robert Sesek, 2014-09-03 16:52:37]

Patchset #3 (id:60001) has been deleted

[Ken Russell (switch to Gerrit), 2014-09-03 23:06:49]

LGTM overall. One issue to be fixed, one question.

https://codereview.chromium.org/529103002/diff/80001/Source/platform/mac/Scro...
File Source/platform/mac/ScrollAnimatorMac.mm (right):

https://codereview.chromium.org/529103002/diff/80001/Source/platform/mac/Scro...
Source/platform/mac/ScrollAnimatorMac.mm:321: m_startTime =
CACurrentMediaTime();
Would it be possible to use existing APIs like WTF::currentTime() for this? Or
is it not accurate enough?

https://codereview.chromium.org/529103002/diff/80001/Source/platform/mac/Scro...
Source/platform/mac/ScrollAnimatorMac.mm:358: WebScrollbarPartAnimation*
m_animation;
Please document that this is a weak pointer.

https://codereview.chromium.org/529103002/diff/80001/Source/platform/mac/Scro...
Source/platform/mac/ScrollAnimatorMac.mm:359: CubicBezierTimingFunction*
m_timingFunction;
Should be RefPtr<CubicBezierTimingFunction>. (See other uses of
CubicBezierTimingFunction::preset.)

[Robert Sesek, 2014-09-03 23:25:30]

Thanks!

https://codereview.chromium.org/529103002/diff/80001/Source/platform/mac/Scro...
File Source/platform/mac/ScrollAnimatorMac.mm (right):

https://codereview.chromium.org/529103002/diff/80001/Source/platform/mac/Scro...
Source/platform/mac/ScrollAnimatorMac.mm:321: m_startTime =
CACurrentMediaTime();
On 2014/09/03 23:06:48, Ken Russell wrote:
> Would it be possible to use existing APIs like WTF::currentTime() for this? Or
> is it not accurate enough?

Yes, though I bumped the framerate. This doesn't require terribly high accuracy.

https://codereview.chromium.org/529103002/diff/80001/Source/platform/mac/Scro...
Source/platform/mac/ScrollAnimatorMac.mm:358: WebScrollbarPartAnimation*
m_animation;
On 2014/09/03 23:06:49, Ken Russell wrote:
> Please document that this is a weak pointer.

Done.

https://codereview.chromium.org/529103002/diff/80001/Source/platform/mac/Scro...
Source/platform/mac/ScrollAnimatorMac.mm:359: CubicBezierTimingFunction*
m_timingFunction;
On 2014/09/03 23:06:48, Ken Russell wrote:
> Should be RefPtr<CubicBezierTimingFunction>. (See other uses of
> CubicBezierTimingFunction::preset.)

Wow, thanks. I assumed that the STATIC_LOCAL thing meant it was just leaked.

[Ken Russell (switch to Gerrit), 2014-09-04 00:56:05]

https://codereview.chromium.org/529103002/diff/80001/Source/platform/mac/Scro...
File Source/platform/mac/ScrollAnimatorMac.mm (right):

https://codereview.chromium.org/529103002/diff/80001/Source/platform/mac/Scro...
Source/platform/mac/ScrollAnimatorMac.mm:359: CubicBezierTimingFunction*
m_timingFunction;
On 2014/09/03 23:25:30, rsesek wrote:
> On 2014/09/03 23:06:48, Ken Russell wrote:
> > Should be RefPtr<CubicBezierTimingFunction>. (See other uses of
> > CubicBezierTimingFunction::preset.)
> 
> Wow, thanks. I assumed that the STATIC_LOCAL thing meant it was just leaked.

I think your code was technically correct before, FWIW, because it didn't touch
the reference counts at all -- but it's better to follow the convention.

[Robert Sesek, 2014-09-04 13:39:33]

The CQ bit was checked by rsesek@chromium.org

[commit-bot: I haz the power, 2014-09-04 13:40:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rsesek@chromium.org/529103002/100001

[commit-bot: I haz the power, 2014-09-04 18:39:52]

Committed patchset #4 (id:100001) as 181388