Refactor ScrollableArea::setScrollPosition.

third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h

Index: third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h
diff --git a/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h b/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h
index 7f1f1ea597075844a9a3a614a7a1dc63580c6326..bf2766edf1621bba431d124f46d26e1ef88a3701 100644
--- a/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h
+++ b/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h
@@ -310,50 +310,24 @@ class CORE_EXPORT PaintLayerScrollableArea final
   ScrollBehavior scrollBehaviorStyle() const override;
   CompositorAnimationTimeline* compositorAnimationTimeline() const override;
 
-  double scrollXOffset() const {
-    return m_scrollOffset.width() + scrollOrigin().x();
-  }
-  double scrollYOffset() const {
-    return m_scrollOffset.height() + scrollOrigin().y();
-  }
-
-  DoubleSize scrollOffset() const { return m_scrollOffset; }
-
   // FIXME: We shouldn't allow access to m_overflowRect outside this class.
   LayoutRect overflowRect() const { return m_overflowRect; }
 
-  void scrollToPosition(const DoublePoint& scrollPosition,
-                        ScrollOffsetClamping = ScrollOffsetUnclamped,
-                        ScrollBehavior = ScrollBehaviorInstant,
-                        ScrollType = ProgrammaticScroll);
-
-  void scrollToOffset(const DoubleSize& scrollOffset,
-                      ScrollOffsetClamping clamp = ScrollOffsetUnclamped,
-                      ScrollBehavior scrollBehavior = ScrollBehaviorInstant,
-                      ScrollType scrollType = ProgrammaticScroll) {
-    scrollToPosition(-scrollOrigin() + scrollOffset, clamp, scrollBehavior,
-                     scrollType);
-  }
-
-  void scrollToXOffset(double x,
-                       ScrollOffsetClamping clamp = ScrollOffsetUnclamped,
-                       ScrollBehavior scrollBehavior = ScrollBehaviorInstant) {
-    scrollToOffset(DoubleSize(x, scrollYOffset()), clamp, scrollBehavior);
+  DoubleSize offsetFromOrigin() const {

[bokan, 2016/10/01 20:32:47]

I find this naming to be confusing, I think what this method is doing is
returning the distance of the ScrollableArea's top-left corner from the overflow
content's top-left corner. It took me a while to realize that "offsetFromOrigin"
isn't "offsetFrom*Scroll*Origin".

Perhaps we should call it "offsetFromTopLeft"? Alternatively, leave it so that
the origin means the overflow content's top left corner and rename
scrollOrigin() to writingModeAdjustment() or something like that. Or am I
completely misunderstanding?

[szager1, 2016/10/02 19:35:28]

In discussion with skobes, we had different ideas about how to interpret
"origin": I imagined it as the top/left of the overflow rect, skobes imagines it
as the top/left of the beginning of flow position.  The only consensus, I think,
is that "origin" is a universally confusing term.

These names are temporary, the subsequent patch changes them to:

"offset": Distance from beginning of flow
"absolutePosition": Distance from top/left

... which avoids the term "origin".  I can add documentation to that patch to
clarify their meaning.

Changing the names is part of what makes the subsequent patch so big, so in this
patch, I just did s/adjustedScrollOffset/offsetFromOrigin/ because it was a
relatively small change and I thought it added clarity.

+    return toDoubleSize(DoublePoint(scrollOrigin())) + m_scrollPosition;
   }
-
-  void scrollToYOffset(double y,
-                       ScrollOffsetClamping clamp = ScrollOffsetUnclamped,
-                       ScrollBehavior scrollBehavior = ScrollBehaviorInstant) {
-    scrollToOffset(DoubleSize(scrollXOffset(), y), clamp, scrollBehavior);
+  void scrollToOffsetFromOrigin(
+      const DoubleSize& scrollOffset,
+      ScrollBehavior scrollBehavior = ScrollBehaviorInstant,
+      ScrollType scrollType = ProgrammaticScroll) {
+    setScrollPosition(-scrollOrigin() + scrollOffset, scrollType,
+                      scrollBehavior);
   }
 
-  void setScrollPosition(
-      const DoublePoint& position,
-      ScrollType scrollType,
-      ScrollBehavior scrollBehavior = ScrollBehaviorInstant) override {
-    scrollToOffset(toDoubleSize(position), ScrollOffsetClamped, scrollBehavior,
-                   scrollType);
-  }
+  // This will set the scroll position without clamping, and it will do all post-update
+  // work even if the scroll position didn't change.
+  void setScrollPositionUnconditionally(const DoublePoint&,
+                                        ScrollType = ProgrammaticScroll);
 
   // TODO(szager): Actually run these after all of layout is finished.  Currently, they
   // run at the end of box()'es layout (or after all flexbox layout has finished) but while
@@ -395,10 +369,6 @@ class CORE_EXPORT PaintLayerScrollableArea final
   int horizontalScrollbarHeight(
       OverlayScrollbarClipBehavior = IgnoreOverlayScrollbarSize) const;
 
-  DoubleSize adjustedScrollOffset() const {
-    return DoubleSize(scrollXOffset(), scrollYOffset());
-  }
-
   void positionOverflowControls();
 
   // isPointInResizeControl() is used for testing if a pointer/touch position is in the resize control
@@ -526,7 +496,7 @@ class CORE_EXPORT PaintLayerScrollableArea final
   void updateScrollOrigin();
   void updateScrollDimensions();
 
-  void setScrollOffset(const DoublePoint&, ScrollType) override;
+  void updateScrollPosition(const DoublePoint&, ScrollType) override;
 
   int verticalScrollbarStart(int minX, int maxX) const;
   int horizontalScrollbarStart(int minX) const;
@@ -588,8 +558,8 @@ class CORE_EXPORT PaintLayerScrollableArea final
   // ScrollbarManager holds the Scrollbar instances.
   ScrollbarManager m_scrollbarManager;
 
-  // This is the (scroll) offset from scrollOrigin().
-  DoubleSize m_scrollOffset;
+  // This is the absolute scroll position, *not* relative to m_scrollOrigin.

[bokan, 2016/10/01 20:32:47]

Ok, I think I'm missing something. Absolute would mean relative to the top left?
This seems to be  the opposite implied in the description:
Is this supposed to mean "position (0, 0) is scrolled all the way up and left in
LTR and down and right in RTL"? In that case, that does seem to be relative to
m_scrollOrigin.
offsetFromOrigin() is defined as:

  return toDoubleSize(DoublePoint(scrollOrigin())) + m_scrollPosition;

So (referring to the ASCII art in the README.md) if we're RTL and assume
scrollOrigin is (200, 0) and scrollPosition is (0, 0), offsetFromOrigin will be
(200, 0) which implies it's not relative to the origin but the top left.

[szager1, 2016/10/02 19:35:28]

Yeah, this is a bad comment; I changed it.

+  DoubleSize m_scrollPosition;
 
   IntPoint m_cachedOverlayScrollbarOffset;