[Webkit-unassigned] [Bug 72996] [chromium] Add page-scale animation support to Impl thread

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=72996


James Robinson <jamesr at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #116469|review?                     |review-
               Flag|                            |




--- Comment #14 from James Robinson <jamesr at chromium.org>  2011-11-27 12:49:28 PST ---
(From update of attachment 116469)
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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.