Refactor ScrollableArea::setScrollPosition.

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

 /*
  * Copyright (C) 2003, 2009, 2012 Apple Inc. All rights reserved.
  *
  * Portions are Copyright (C) 1998 Netscape Communications Corporation.
  *
  * Other contributors:
  *   Robert O'Callahan <roc+@cs.cmu.edu>
  *   David Baron <dbaron@fas.harvard.edu>
  *   Christian Biesinger <cbiesinger@web.de>
  *   Randall Jesup <rjesup@wgate.com>
@@ skipping 292 matching lines @@
   void scrollbarVisibilityChanged() override;
   IntRect scrollableAreaBoundingBox() const override;
   void registerForAnimation() override;
   void deregisterForAnimation() override;
   bool userInputScrollable(ScrollbarOrientation) const override;
   bool shouldPlaceVerticalScrollbarOnLeft() const override;
   int pageStep(ScrollbarOrientation) const override;
   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);
+  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 scrollToOffsetFromOrigin(
+      const DoubleSize& scrollOffset,
+      ScrollBehavior scrollBehavior = ScrollBehaviorInstant,
+      ScrollType scrollType = ProgrammaticScroll) {
+    setScrollPosition(-scrollOrigin() + scrollOffset, scrollType,
+                      scrollBehavior);
   }
 
-  void scrollToXOffset(double x,
-                       ScrollOffsetClamping clamp = ScrollOffsetUnclamped,
-                       ScrollBehavior scrollBehavior = ScrollBehaviorInstant) {
-    scrollToOffset(DoubleSize(x, scrollYOffset()), clamp, scrollBehavior);
-  }
-
-  void scrollToYOffset(double y,
-                       ScrollOffsetClamping clamp = ScrollOffsetUnclamped,
-                       ScrollBehavior scrollBehavior = ScrollBehaviorInstant) {
-    scrollToOffset(DoubleSize(scrollXOffset(), y), clamp, 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
   // document layout is still happening.
   void updateAfterLayout();
   void clampScrollPositionsAfterLayout();
 
   void updateAfterStyleChange(const ComputedStyle*);
   void updateAfterOverflowRecalc();
 
@@ skipping 21 matching lines @@
   LayoutUnit scrollWidth() const;
   LayoutUnit scrollHeight() const;
   int pixelSnappedScrollWidth() const;
   int pixelSnappedScrollHeight() const;
 
   int verticalScrollbarWidth(
       OverlayScrollbarClipBehavior = IgnoreOverlayScrollbarSize) const;
   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
   // area.
   bool isPointInResizeControl(const IntPoint& absolutePoint,
                               ResizerHitTestType) const;
   bool hitTestOverflowControls(HitTestResult&, const IntPoint& localPoint);
 
   bool hitTestResizerInFragments(const PaintLayerFragments&,
                                  const HitTestLocation&) const;
@@ skipping 107 matching lines @@
   bool hasVerticalOverflow() const;
   bool hasScrollableHorizontalOverflow() const;
   bool hasScrollableVerticalOverflow() const;
   bool visualViewportSuppliesScrollbars() const;
 
   bool needsScrollbarReconstruction() const;
 
   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;
   IntSize scrollbarOffset(const Scrollbar&) const;
 
   void setHasHorizontalScrollbar(bool hasScrollbar);
   void setHasVerticalScrollbar(bool hasScrollbar);
 
   void updateScrollCornerStyle();
 
@@ skipping 41 matching lines @@
 
   // The width/height of our scrolled area.
   // This is OverflowModel's layout overflow translated to physical
   // coordinates. See OverflowModel for the different overflow and
   // LayoutBoxModelObject for the coordinate systems.
   LayoutRect m_overflowRect;
 
   // 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;
 
   // LayoutObject to hold our custom scroll corner.
   LayoutScrollbarPart* m_scrollCorner;
 
   // LayoutObject to hold our custom resizer.
   LayoutScrollbarPart* m_resizer;
 
   ScrollAnchor m_scrollAnchor;
 
   std::unique_ptr<PaintLayerScrollableAreaRareData> m_rareData;
 
 #if ENABLE(ASSERT)
   bool m_hasBeenDisposed;
 #endif
 };
 
 DEFINE_TYPE_CASTS(PaintLayerScrollableArea,
                   ScrollableArea,
                   scrollableArea,
                   scrollableArea->isPaintLayerScrollableArea(),
                   scrollableArea.isPaintLayerScrollableArea());
 
 }  // namespace blink
 
 #endif  // LayerScrollableArea_h