Removed main-thread one dimensional scrolling paths.

third_party/WebKit/Source/core/frame/RootFrameViewport.cpp

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "core/frame/RootFrameViewport.h"
 
 #include "core/frame/FrameView.h"
 #include "core/layout/ScrollAlignment.h"
 #include "platform/geometry/DoubleRect.h"
 #include "platform/geometry/FloatRect.h"
@@ skipping 214 matching lines @@
 GraphicsLayer* RootFrameViewport::layerForHorizontalScrollbar() const
 {
     return layoutViewport().layerForHorizontalScrollbar();
 }
 
 GraphicsLayer* RootFrameViewport::layerForVerticalScrollbar() const
 {
     return layoutViewport().layerForVerticalScrollbar();
 }
 
-ScrollResultOneDimensional RootFrameViewport::userScroll(ScrollDirectionPhysical direction, ScrollGranularity granularity, float delta)
+ScrollResult RootFrameViewport::userScroll(ScrollGranularity granularity, const FloatSize& delta)
 {
+    // TODO(bokan/ymalik): This method specialization can likely go away as a
+    // result of the smooth scrolling work.

[tdresser, 2016/02/29 14:53:43]

I may just be lacking context, but it isn't clear to me what this comment is
referring to.

[bokan, 2016/03/01 05:56:24]

I think it should be clearer now.

+
     updateScrollAnimator();
 
-    ScrollbarOrientation orientation = scrollbarOrientationFromDirection(direction);
+    // Distribute the scroll between the visual and layout viewport.
 
-    if (layoutViewport().userInputScrollable(orientation) && visualViewport().userInputScrollable(orientation)) {
-        // Distribute the scroll between the visual and layout viewport.
-        float step = scrollStep(granularity, orientation);
+    float stepX = scrollStep(granularity, HorizontalScrollbar);
+    float stepY = scrollStep(granularity, VerticalScrollbar);
 
-        if (direction == ScrollUp || direction == ScrollLeft)
-            delta = -delta;
+    FloatSize pixelDelta(delta);
+    pixelDelta.scale(stepX, stepY);
 
-        // This is the total amount we need to scroll. Instead of passing step
-        // to the scroll animator and letting it compute the total delta, we
-        // give it the delta it should scroll. This way we can apply the
-        // unused delta from the visual viewport to the layout viewport.
-        delta *= step;
+    if (!layoutViewport().userInputScrollable(HorizontalScrollbar))
+        pixelDelta.setWidth(0);

[tdresser, 2016/02/29 14:53:43]

Can we avoid calling scrollStep in cases where we're going to ignore its output?

[bokan, 2016/03/01 05:56:24]

Actually, we'll still want to call scrollStep so we can return the
unusedScrollDelta in pixels, though I realized I don't do that here or in
ScrollableArea::userScroll. I've updated both.

+    if (!layoutViewport().userInputScrollable(VerticalScrollbar))
+        pixelDelta.setHeight(0);
 
-        cancelProgrammaticScrollAnimation();
+    // If the layoutViewport isn't scrollable, just let the visual viewport
+    // handle the scroll since it's always userInputScrollable.
+    if (pixelDelta.isZero())
+        return visualViewport().userScroll(granularity, delta);

[tdresser, 2016/02/29 14:53:43]

If one axis didn't scroll, but the other did, this will cause a change in
behavior, won't it? Shouldn't we allow scrolling the layout viewport on one
axis, but the visual viewport on the other?

[bokan, 2016/03/01 05:56:24]

I'm not sure I understand. If pixelDelta.isZero() is true then neither axis is
scrollable and we just let the visual viewport handle it (visual viewport is
always scrollable so I removed the checks). Otherwise we drop down and do the
distribution logic which I think should handle the case you mention.

I've actually rewritten this method to correctly handle the return type. It's a
little complicated (no way around it I think - the viewport distribution
requires a bit of bookkeeping for the return value) but I've added plenty of
comments and hopefully made it more clear.

 
-        float visualUsedDelta = visualViewport().scrollAnimator().computeDeltaToConsume(orientation, delta);
-        ScrollResultOneDimensional visualResult = visualViewport().scrollAnimator().userScroll(
-            orientation, granularity, visualUsedDelta);
+    cancelProgrammaticScrollAnimation();
 
-        // Scroll the layout viewport if all of the scroll was not applied to the
-        // visual viewport.
-        if (visualUsedDelta == delta)
-            return visualResult;
+    // TODO(bokan): Why do we call userScroll on the animators directly and
+    // not through the ScrollableAreas?
 
-        ScrollResultOneDimensional layoutResult = layoutViewport().scrollAnimator().userScroll(
-            orientation, granularity, delta - visualUsedDelta);
+    // Precompute the amount of possible scrolling since, when animated,
+    // ScrollAnimator::userScroll will consume the total scroll delta even
+    // beyond the scrolling bounds.
+    FloatSize visualUsedDelta = visualViewport().scrollAnimator().computeDeltaToConsume(pixelDelta);
+    ScrollResult visualResult = visualViewport().scrollAnimator().userScroll(granularity, visualUsedDelta);
 
-        return ScrollResultOneDimensional(visualResult.didScroll || layoutResult.didScroll,
-            layoutResult.unusedScrollDelta);
-    }
+    // Scroll the layout viewport if all of the scroll was not applied to the
+    // visual viewport.
+    if (visualUsedDelta == pixelDelta)
+        return visualResult;
 
-    if (visualViewport().userInputScrollable(orientation))
-        return visualViewport().userScroll(direction, granularity, delta);
+    ScrollResult layoutResult = layoutViewport().scrollAnimator().userScroll(granularity, pixelDelta - visualUsedDelta);

[tdresser, 2016/02/29 14:53:43]

I'm a bit confused about how this works. If I understand correctly:
1. If the layout viewport is user scrollable, scroll it.
2. If the layout viewport didn't scroll, try to scroll the visual viewport.
3. If the visual viewport didn't consume all the scroll, scroll the layout
viewport.

Why would we ever do anything in step #3? Is it for cases where the layout
viewport isn't user scrollable?

[bokan, 2016/03/01 05:56:24]

It's actually:

1. Try to scroll the visual viewport
2. If there's still unused delta remaining and the layout viewport is scrollable
in that direction, scroll it.

Were you looking at the old version? Admittedly, the various scrollability
checks and cases make it seem a little confusing...Take a look at the updated
version. Hopefully we can remove this as the TODO at the top says and use the
much simpler ScrollableArea::userScroll (it does the distribution once the
offset is actually applied).

 
-    if (layoutViewport().userInputScrollable(orientation))
-        return layoutViewport().userScroll(direction, granularity, delta);
-
-    return ScrollResultOneDimensional(false, delta);
+    return ScrollResult(
+        visualResult.didScrollX || layoutResult.didScrollX,
+        visualResult.didScrollY || layoutResult.didScrollY,
+        layoutResult.unusedScrollDeltaX,
+        layoutResult.unusedScrollDeltaY);
 }
 
 bool RootFrameViewport::scrollAnimatorEnabled() const
 {
     return layoutViewport().scrollAnimatorEnabled();
 }
 
 HostWindow* RootFrameViewport::hostWindow() const
 {
     return layoutViewport().hostWindow();
@@ skipping 26 matching lines @@
 }
 
 DEFINE_TRACE(RootFrameViewport)
 {
     visitor->trace(m_visualViewport);
     visitor->trace(m_layoutViewport);
     ScrollableArea::trace(visitor);
 }
 
 } // namespace blink