[webkit-reviews] review denied: [Bug 68035] [chromium] Zoom animator front-end : [Attachment 109482] Patch

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has denied W. James MacLean
<wjmaclean at chromium.org>'s request for review:
Bug 68035: [chromium] Zoom animator front-end
https://bugs.webkit.org/show_bug.cgi?id=68035

Attachment 109482: Patch
https://bugs.webkit.org/attachment.cgi?id=109482&action=review

------- Additional Comments from Kenneth Russell <kbr at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=109482&action=review


The overall structure looks OK to me but there are a few things that should be
cleaned up. Please also delete the comments in the ChangeLog about its not
being for review yet.

> LayoutTests/platform/chromium/compositing/zoom-animator-scale-test2.html:29
> +		   // Check every 100 mS for animation completion.

This comment is out of sync with the value being passed to setInterval.

> LayoutTests/platform/chromium/compositing/zoom-animator-scale-test2.html:30
> +		   timer = setInterval("finishTest()", 250);

Shouldn't you add some provision for the test completing within a certain
period of time?

> Source/WebCore/page/FrameView.h:173
> +    virtual void zoomAnimatorTransformChanged(double, double, double, bool);


In general WebKit style is to use enums instead of bool arguments so the call
site is more self-explanatory. If you can find a good place to define one for
this, that would be good.

> Source/WebCore/platform/ScrollAnimator.h:93
> +    virtual void testZoomParameters(float, float, float);

This method name is unclear. It sounds like it is supposed to test the incoming
values and return a bool. I think you should rename it to
setZoomParametersForTesting if that's what it's for, or just call it
setZoomParameters.

> Source/WebCore/platform/ScrollAnimatorNone.cpp:370
> +ScrollAnimatorNone::ZoomData::ZoomData(WebCore::ScrollAnimatorNone* parent,
float* scale, float* transX, float* transY)

Passing these pointers is pretty gross. Since you're already passing the parent
ScrollAnimatorNone (and ignoring it right now, which is poor), why not just
make ZoomData a friend and set its fields directly?

> Source/WebCore/platform/ScrollAnimatorNone.cpp:390
> +    *m_currentScale = deltaTime / m_animationTime * (m_desiredScale -
m_startScale) + m_startScale;

Compute deltaTime / m_animationTime into a local variable for clarity.

> Source/WebCore/platform/ScrollAnimatorNone.cpp:469
> +    // FIXME(wjmaclean): modify this so we can start even if the timer is
active.

WebKit style doesn't put user names into FIXMEs. Here and below.