[webkit-reviews] review denied: [Bug 93514] [chromium] Tie WebFlingAnimator into the compositor : [Attachment 157281] patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Nate Chapin
<japhet at chromium.org>'s request for review:
Bug 93514: [chromium] Tie WebFlingAnimator into the compositor
https://bugs.webkit.org/show_bug.cgi?id=93514

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

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


I can't tell what this patch's relationship with non-root layer scrolling is
supposed to be.  We have support for that in CCLayerTreeHostImpl and it seems
that this should have at least the same level of support.

Could someone familiar with this code on the android branch offer to answer
questions/etc?	I suspect one of aelias/trchen/skyostil is the right person for
this, but I'm not sure who.

> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:274
> +	   OwnPtr<PlatformGestureCurve> flingCurve =
PlatformGestureCurveFactory::get()->createCurve(FloatPoint(gestureEvent.deltaX,
gestureEvent.deltaY), m_inputHandlerClient->scrollRange());
>	   m_wheelFlingAnimation =
CCActiveGestureAnimation::create(PlatformGestureToCCGestureAdapter::create(flin
gCurve.release()), this);

OS(ANDROID) shouldn't be creating something called m_wheelFlingAnimation,
android doesn't have wheel flings.  If you want to reuse this logic (which
seems reasonable) I think you also need to figure out proper generic names for
things that express their cross-platform meaning.

> Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:68
> +    WebCore::IntRect scrollRange() const { return WebCore::IntRect(); }

this looks like an override of a virtual in the base class - can you add
virtual and OVERRIDE keywords?

> Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:128
> +	   m_velocity = WebCore::IntSize((int)velocity.x, (int)velocity.y);

use C++ casts (e.g. static_cast<type>()), not C-style.

a 'using WebCore::IntSize' at the top of this file would help make this file a
lot more readable

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:84
> +    virtual IntRect scrollRange() const OVERRIDE;

Could you document what this function does and what interface the OVERRIDE is
from? If it's from CCInputHandlerClient, why is there a blank line between it
and the rest of the functions? Could this go next to the other scroll-related
functions, perhaps?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:932
> +static void expandScrollRange(IntRect& scrollRange, float scale, IntPoint
scrollPosition, IntSize maxScrollPosition)

This function doesn't appear to be aware of scrollable sublayers. I think what
it's trying to do is figure out how far a fling would go if it went in a given
direction.  There's some not-so-trivial logic in ::scrollBy() to figure out
which layer should take a scroll in a given direction with a given layer tree -
it seems like this is too simple to be correct.

Should this instead take a given direction and then mimic (or hopefully reuse)
the scrollBy() logic to get a real extrema for a given direction?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:959
> +    const CCLayerImpl* layerImpl = m_rootScrollLayerImpl;

shouldn't this be looking at the m_currentlyScrollingLayerImpl, not the root?