[webkit-reviews] review denied: [Bug 77872] [chromium] Add support for WebInputEvent::GestureDoubleTap on CC impl thread : [Attachment 126340] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied W. James MacLean
<wjmaclean at chromium.org>'s request for review:
Bug 77872: [chromium] Add support for WebInputEvent::GestureDoubleTap on CC
impl thread
https://bugs.webkit.org/show_bug.cgi?id=77872

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

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


I really don't like inventing new terminology for zoom animation when we
already have page scale and page scale animations.  Stick to the terminology we
already have.

> Source/WebCore/platform/CrossThreadCopier.h:44
> +class IntSize;

this whole file is indented incorrectly. I don't think fixing the indentation
for just a few lines at the top is an improvement - i'd rather be consistent
within this file than have some lines do one thing and some lines do another.

I would stick to the existing indentation in this case.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:191
> +    void zoomAnimation(const IntSize&, bool, float, double);

you need to name parameters when the meaning is not clear from the types. I've
no idea what the size, bool, float, and double could mean from this signature.

For time-based parameters please tag the name with the unit as well (seconds or
milliseconds). If it's milliseconds, use an integer type. I think we want to
converge on floating-point seconds everywhere if possible.

> Source/WebCore/platform/graphics/chromium/cc/CCProxy.h:60
> +    virtual void zoomAnimation(const IntSize&, bool, float, double) = 0;

again here, you need to provide useful parameter names in the header

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:146
> +void CCThreadProxy::requestZoomAnimationOnImplThread(IntSize targetPosition,
bool useAnchor, float scale, double duration)
> +{
> +    ASSERT(CCProxy::isImplThread());
> +    if (m_layerTreeHostImpl)
> +	   m_layerTreeHostImpl->startPageScaleAnimation(targetPosition,
useAnchor, scale, monotonicallyIncreasingTime() * 1000.0, duration);

why do we have to have inconsistent naming? what's wrong with calling it
pageScaleAnimation everywhere?

> Source/WebKit/chromium/src/WebViewImpl.h:319
> +    void zoomAnimation(const WebCore::IntPoint&, bool, float);

again - these parameters need useful names

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:958
> +    static void requestZoomAnimation(void * self)

no space between "void" and "*", it's a "void*"