Bug 93514

Created attachment 157281 [details] patch

Comment on attachment 157281 [details] patch Attachment 157281 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13463111

Nate: What's the status of this patch? Are we waiting on jamesr to review?

(In reply to comment #3) > Nate: What's the status of this patch? Are we waiting on jamesr to review? I believe so, ergo... jamesr: ping :)

Comment on attachment 157281 [details] patch 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(flingCurve.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?

(In reply to comment #5) > (From update of attachment 157281 [details]) > 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. skyostil@ is the main author of the code, and I'm quite familiar with it too. > > 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(flingCurve.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. What scrollRange() is trying to achieve is that, for each of the 4 directions, try to find the innermost layer which can be scrolled for that direction. The idea is that we don't propagate up the fling when we hit the edge of a layer. The implementation works by starting from the current scrolling layer. We look at 4 direction of current scroll range, if all of them are scrollable already (isValidScrollRange) then we stop. Otherwise we continue to the parent layer and try to make the rest directions scrollable (expandScrollRange). That's why expandScrollRange only looks at one layer at a time, and it only updates the direction that is currently non-scrollable. I think the real problem with this implementation is that it ignores transformation. I believe the extra logic in scrollBy() takes care of the transformation. However, if we want to keep the same logic that a fling never propagates up while supporting transformation, then scroll range can become convex polygon instead of rectilinear rectangle. IMO the right way to do it is to pass in the fling velocity to scrollRange(), and the function returns the target scroll layer and the reverse-transformed fling velocity. Then later on we do everything in the layer's coordinates instead of window coordinates. > 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?

(In reply to comment #6) > (In reply to comment #5) > > 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. > > What scrollRange() is trying to achieve is that, for each of the 4 directions, try to find the innermost layer which can be scrolled for that direction. The idea is that we don't propagate up the fling when we hit the edge of a layer. > > The implementation works by starting from the current scrolling layer. We look at 4 direction of current scroll range, if all of them are scrollable already (isValidScrollRange) then we stop. Otherwise we continue to the parent layer and try to make the rest directions scrollable (expandScrollRange). > > That's why expandScrollRange only looks at one layer at a time, and it only updates the direction that is currently non-scrollable. > > I think the real problem with this implementation is that it ignores transformation. I believe the extra logic in scrollBy() takes care of the transformation. However, if we want to keep the same logic that a fling never propagates up while supporting transformation, then scroll range can become convex polygon instead of rectilinear rectangle. > > IMO the right way to do it is to pass in the fling velocity to scrollRange(), and the function returns the target scroll layer and the reverse-transformed fling velocity. Then later on we do everything in the layer's coordinates instead of window coordinates. I like this idea a lot. Could you take over this patch and try that, or is Sami a better owner?

(In reply to comment #7) > > I think the real problem with this implementation is that it ignores transformation. I believe the extra logic in scrollBy() takes care of the transformation. However, if we want to keep the same logic that a fling never propagates up while supporting transformation, then scroll range can become convex polygon instead of rectilinear rectangle. > > > > IMO the right way to do it is to pass in the fling velocity to scrollRange(), and the function returns the target scroll layer and the reverse-transformed fling velocity. Then later on we do everything in the layer's coordinates instead of window coordinates. Right, the original code doesn't work with transforms. Having scrollRange() transform the fling into local layer coordinates sounds good, although we need to be a little careful about selecting which layer is flung -- it could be m_currentlyScrollingLayer or any of its parents. I'm also wondering whether we can always guarantee that the fling only affects one layer. > I like this idea a lot. Could you take over this patch and try that, or is Sami a better owner? If anyone wants to go for it, be my guest. Otherwise I could look into this after 91117 et al are sorted.

*** Bug 77068 has been marked as a duplicate of this bug. ***

It looks like the fling upstreaming is not going to be finished before CC moves. Should we delete the downstream fling code to ensure an easy transition?

> It looks like the fling upstreaming is not going to be finished before CC moves. Should we delete the downstream fling code to ensure an easy transition? That's ok with me if that works for you.

Looks like this is obsolete.