[webkit-reviews] review denied: [Bug 72996] [chromium] Add page-scale animation support to Impl thread : [Attachment 116469] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Alexandre Elias
<aelias at chromium.org>'s request for review:
Bug 72996: [chromium] Add page-scale animation support to Impl thread
https://bugs.webkit.org/show_bug.cgi?id=72996

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

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=116469&action=review


This is a great start. Left some comments that should be addressed before
landing, IMO

> Source/WebCore/ChangeLog:12
> +	   No new tests. (https://bugs.webkit.org/show_bug.cgi?id=71529 filed.)


I think it'd be really helpful to land at least some skeleton tests with this
patch, at least to show how the tests will be constructed and to verify that
there aren't any crazy explosions when the class is used in a straightforward
way.

> Source/WebCore/platform/graphics/chromium/cc/CCInputHandler.h:57
> +	   bool anchorPoint,
> +	   float pageScale,
> +	   double durationMs) = 0;

the line wrapping here is a bit strange - could you align with the opening "("
perhaps?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:114
> +    m_pageScaleAnimation = adoptPtr(new CCPageScaleAnimation(scrollTotal,
scaleTotal, m_viewportSize, currentTimeMs()));

don't use currentTimeMs() - we use monotonicallyIncreasingTime() for all
animation logic

even better would be taking a timestamp passed in from the caller so the caller
can adjust the time appropriately to reflect the event time or the next frame
time or whatnot

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:392
> +    if (!didMove || m_pinchGestureActive || m_pageScaleAnimation.get()) {

you don't need the .get() to null check, all WebKit smart pointers have an
operator bool() that does the right thing

> Source/WebCore/platform/graphics/chromium/cc/CCPageScaleAnimation.cpp:16
> + *	  * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + *	  * Redistributions in binary form must reproduce the above
> + * copyright notice, this list of conditions and the following disclaimer
> + * in the documentation and/or other materials provided with the
> + * distribution.
> + *	  * Neither the name of Google Inc. nor the names of its
> + * contributors may be used to endorse or promote products derived from
> + * this software without specific prior written permission.

we use a 2-clause license now. see the license header on (say)
http://trac.webkit.org/browser/trunk/Source/WebCore/html/HTMLCanvasElement.cpp,
modulo the date lines

> Source/WebCore/platform/graphics/chromium/cc/CCPageScaleAnimation.cpp:50
> +    m_scrollStart = scrollStart;
> +    m_pageScaleStart = pageScaleStart;
> +    m_windowSize = windowSize;
> +    m_anchorMode = false;
> +    m_scrollEnd = scrollStart;
> +    m_pageScaleEnd = pageScaleStart;
> +    m_startTime = startTime;

should be done with initializer list syntax

    : m_scrollStart(scrollStart)
    , m_pageScaleStart(pageScaleStart)

etc

> Source/WebCore/platform/graphics/chromium/cc/CCPageScaleAnimation.cpp:119
> +	   return 1.0f;

nit: we just say '1' for constants like this (see 'Floating point literals'
section in http://www.webkit.org/coding/coding-style.html)

> Source/WebCore/platform/graphics/chromium/cc/CCPageScaleAnimation.cpp:126
> +    if (ratio <= 0.0f)

0

> Source/WebCore/platform/graphics/chromium/cc/CCPageScaleAnimation.cpp:128
> +    if (ratio >= 1.0f)

1

> Source/WebCore/platform/graphics/chromium/cc/CCPageScaleAnimation.cpp:156
> +    if (ratio <= 0.0f)

0

> Source/WebCore/platform/graphics/chromium/cc/CCPageScaleAnimation.cpp:158
> +    if (ratio >= 1.0f)

1

> Source/WebCore/platform/graphics/chromium/cc/CCPageScaleAnimation.cpp:172
> +} // namespace WebKit

WebCore, no?

> Source/WebCore/platform/graphics/chromium/cc/CCPageScaleAnimation.h:8
> + *	  * Redistributions of source code must retain the above copyright

wrong license header here too

> Source/WebCore/platform/graphics/chromium/cc/CCPageScaleAnimation.h:47
> +    CCPageScaleAnimation(const IntSize& scrollStart, float pageScaleStart,
const IntSize& windowSize, double startTime);

what unit are all the times in this class' interface in? can you tag the
parameter and function names with the appropriate suffix?

another note: if it's intended that this class will be stored in an OwnPtr<>
it's better to make the c'tor private and expose a static PassOwnPtr<> create
function to ensure that the caller puts the object into a smart pointer and
doesn't leak it

> Source/WebCore/platform/graphics/chromium/cc/CCPageScaleAnimation.h:74
> +private:

nit: newline before the private: please

> Source/WebCore/platform/graphics/chromium/cc/CCPageScaleAnimation.h:92
> +} // namespace WebKit

WebCore, I think