Replace various ScrollableArea scroll methods with setScrollPosition

Replace various ScrollableArea scroll methods with setScrollPosition

Replaced scrollToOffsetWithoutAnimation and programaticallyScrollSmoothly
with setScrollPosition and a new enum describing the type of scroll. This
patch plumbs this ScrollType through to setScrollOffset to allow derived
classes to differ behavior based on the type of scroll.

BUG=

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196867

[bokan, 2015-06-04 13:32:12]

The CQ bit was checked by bokan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-06-04 13:33:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1158673006/20001

[commit-bot: I haz the power, 2015-06-04 13:38:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-04 13:38:06]

Dry run: Try jobs failed on following builders:
  mac_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/57696)

[bokan, 2015-06-04 13:45:00]

bokan@chromium.org changed reviewers:
+ skobes@chromium.org

[bokan, 2015-06-04 13:45:01]

Hey Steve, this is one of the major pieces of the mega patch split out and the
one that touches the most files. I basically consolidated all scrolling through
setScrollPosition and added a ScrollType to differentiate user, programmatic,
and compositor scrolls.

This type decides which path the scroll takes (which animator) but I've also
plumbed it through all the way to scroll offset so derived classes can use it
too. I need this for some viewport work we want to do, where depending on the
type of scroll, the viewport scroll order may change. In this patch though,
after setScrollPosition, the scrollType doesn't have any practical effect at the
moment.

[bokan, 2015-06-04 13:51:06]

The CQ bit was checked by bokan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-06-04 13:51:33]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1158673006/40001

[commit-bot: I haz the power, 2015-06-04 16:27:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-04 16:27:38]

Dry run: This issue passed the CQ dry run.

[skobes, 2015-06-04 23:35:47]

https://codereview.chromium.org/1158673006/diff/40001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/1158673006/diff/40001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:525: ProgrammaticScroll);
Why is this a programmatic scroll?  It seems conceptually similar to pinch zoom.

https://codereview.chromium.org/1158673006/diff/40001/Source/core/frame/RootF...
File Source/core/frame/RootFrameViewport.h (right):

https://codereview.chromium.org/1158673006/diff/40001/Source/core/frame/RootF...
Source/core/frame/RootFrameViewport.h:72: virtual ScrollBehavior
scrollBehaviorStyle() const override;
This should be public, to match its visibility in ScrollableArea.

https://codereview.chromium.org/1158673006/diff/40001/Source/core/paint/Depre...
File Source/core/paint/DeprecatedPaintLayerScrollableArea.cpp (right):

https://codereview.chromium.org/1158673006/diff/40001/Source/core/paint/Depre...
Source/core/paint/DeprecatedPaintLayerScrollableArea.cpp:637:
ScrollableArea::setScrollPosition(-origin + adjustedScrollOffset(),
ProgrammaticScroll);
Why does this call the base class instead of DPLSA::setScrollPosition?

https://codereview.chromium.org/1158673006/diff/40001/Source/platform/mac/Scr...
File Source/platform/mac/ScrollAnimatorMac.mm (right):

https://codereview.chromium.org/1158673006/diff/40001/Source/platform/mac/Scr...
Source/platform/mac/ScrollAnimatorMac.mm:774: // TODO(bokan): I'm no ObjC expert
but this method's only caller seems to be unused.
The caller is passed into NSScrollAnimationHelper, so maybe there is some Cocoa
stuff calling it?  But I am not an Objective C expert either. :)

https://codereview.chromium.org/1158673006/diff/40001/Source/platform/scroll/...
File Source/platform/scroll/ScrollableArea.cpp (right):

https://codereview.chromium.org/1158673006/diff/40001/Source/platform/scroll/...
Source/platform/scroll/ScrollableArea.cpp:211: // TODO(bokan)
Is ScrollableArea::userScroll eventually going to use this path?

[bokan, 2015-06-05 15:15:24]

https://codereview.chromium.org/1158673006/diff/40001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/1158673006/diff/40001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:525: ProgrammaticScroll);
On 2015/06/04 23:35:47, skobes wrote:
> Why is this a programmatic scroll?  It seems conceptually similar to pinch
zoom.

I guess it could be argued either way, my view is this (and pinch-zoom actually)
is the browser keeping the view anchored so it's not really the user scrolling.
In this case more so, the user action doesn't really relate to scrolling. i.e.
If the user changes the zoom level, we shouldn't set the FrameView's
scrolledByUser and prevent resetting the scroll offset on a history navigation.

It does fall into an "in between" type though, maybe we want a separate type?
AnchorScroll or something like that? I was already thinking about that since
we'll want to do some special behavior for scrolling in RootFrameViewport if
it's from a pinch-zoom gesture. If you don't feel strongly though I'd leave that
for the RootFrameViewport/pinchzoom patch.

https://codereview.chromium.org/1158673006/diff/40001/Source/core/frame/RootF...
File Source/core/frame/RootFrameViewport.h (right):

https://codereview.chromium.org/1158673006/diff/40001/Source/core/frame/RootF...
Source/core/frame/RootFrameViewport.h:72: virtual ScrollBehavior
scrollBehaviorStyle() const override;
On 2015/06/04 23:35:47, skobes wrote:
> This should be public, to match its visibility in ScrollableArea.

Done.

https://codereview.chromium.org/1158673006/diff/40001/Source/core/paint/Depre...
File Source/core/paint/DeprecatedPaintLayerScrollableArea.cpp (right):

https://codereview.chromium.org/1158673006/diff/40001/Source/core/paint/Depre...
Source/core/paint/DeprecatedPaintLayerScrollableArea.cpp:637:
ScrollableArea::setScrollPosition(-origin + adjustedScrollOffset(),
ProgrammaticScroll);
On 2015/06/04 23:35:47, skobes wrote:
> Why does this call the base class instead of DPLSA::setScrollPosition?

DPLSA::setScrollPosition calls scrollToOffset. That method only does something
if the scroll offset changed. Update after layout changes the scroll origin, so
when we go to call scrollToOffset with the new offset it looks like it hasn't
changed so it early-outs. We need to call into SA::setScrollPosition to update
scrollbars and the like. This needs cleanup but it's no worse than calling
SA::scrollToOffsetWithoutAnimation. I've added a comment explaining this. One of
the follow up patches changes this to ScrollableArea::scrollPositionChanged
since we really just want to tell the base class to update itself.

https://codereview.chromium.org/1158673006/diff/40001/Source/platform/mac/Scr...
File Source/platform/mac/ScrollAnimatorMac.mm (right):

https://codereview.chromium.org/1158673006/diff/40001/Source/platform/mac/Scr...
Source/platform/mac/ScrollAnimatorMac.mm:774: // TODO(bokan): I'm no ObjC expert
but this method's only caller seems to be unused.
On 2015/06/04 23:35:47, skobes wrote:
> The caller is passed into NSScrollAnimationHelper, so maybe there is some
Cocoa
> stuff calling it?  But I am not an Objective C expert either. :)

Doesn't look like it, it's one of the CLs I split out (just forgot this TODO).
It compiled fine on my Mac and passed the CQ dry run. This'll go away when I
land https://codereview.chromium.org/1165883002/ and rebase.

https://codereview.chromium.org/1158673006/diff/40001/Source/platform/scroll/...
File Source/platform/scroll/ScrollableArea.cpp (right):

https://codereview.chromium.org/1158673006/diff/40001/Source/platform/scroll/...
Source/platform/scroll/ScrollableArea.cpp:211: // TODO(bokan)
On 2015/06/04 23:35:47, skobes wrote:
> Is ScrollableArea::userScroll eventually going to use this path?

That's my hope. It can't right now since it doesn't work on just a simple delta.
Untangling that will be a CL in its own right.

[bokan, 2015-06-05 15:17:25]

https://codereview.chromium.org/1158673006/diff/40001/Source/core/frame/RootF...
File Source/core/frame/RootFrameViewport.h (right):

https://codereview.chromium.org/1158673006/diff/40001/Source/core/frame/RootF...
Source/core/frame/RootFrameViewport.h:72: virtual ScrollBehavior
scrollBehaviorStyle() const override;
On 2015/06/04 23:35:47, skobes wrote:
> This should be public, to match its visibility in ScrollableArea.

Should mention: also did this in DPLSA and made DPLSA::setScrollOffset private
for the same reason.

[skobes, 2015-06-05 15:53:57]

lgtm

https://codereview.chromium.org/1158673006/diff/40001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/1158673006/diff/40001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:525: ProgrammaticScroll);
On 2015/06/05 15:15:23, bokan wrote:
> I guess it could be argued either way, my view is this (and pinch-zoom
actually)
> is the browser keeping the view anchored so it's not really the user
scrolling.
> In this case more so, the user action doesn't really relate to scrolling. i.e.
> If the user changes the zoom level, we shouldn't set the FrameView's
> scrolledByUser and prevent resetting the scroll offset on a history
navigation.
> 
> It does fall into an "in between" type though, maybe we want a separate type?
> AnchorScroll or something like that? I was already thinking about that since
> we'll want to do some special behavior for scrolling in RootFrameViewport if
> it's from a pinch-zoom gesture. If you don't feel strongly though I'd leave
that
> for the RootFrameViewport/pinchzoom patch.

It would be nice to have a comment on the ScrollType enum that documents what
the intent and consequences of each value are.  Can be a separate patch.

[bokan, 2015-06-05 20:35:36]

https://codereview.chromium.org/1158673006/diff/40001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/1158673006/diff/40001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:525: ProgrammaticScroll);
On 2015/06/05 15:53:57, skobes wrote:
> On 2015/06/05 15:15:23, bokan wrote:
> > I guess it could be argued either way, my view is this (and pinch-zoom
> actually)
> > is the browser keeping the view anchored so it's not really the user
> scrolling.
> > In this case more so, the user action doesn't really relate to scrolling.
i.e.
> > If the user changes the zoom level, we shouldn't set the FrameView's
> > scrolledByUser and prevent resetting the scroll offset on a history
> navigation.
> > 
> > It does fall into an "in between" type though, maybe we want a separate
type?
> > AnchorScroll or something like that? I was already thinking about that since
> > we'll want to do some special behavior for scrolling in RootFrameViewport if
> > it's from a pinch-zoom gesture. If you don't feel strongly though I'd leave
> that
> > for the RootFrameViewport/pinchzoom patch.
> 
> It would be nice to have a comment on the ScrollType enum that documents what
> the intent and consequences of each value are.  Can be a separate patch.

Good point, I'll add it once the rest of my patches land and it's more clear
what the differences are.

[bokan, 2015-06-05 20:41:12]

bokan@chromium.org changed reviewers:
+ dmazzoni@chromium.org, vollick@chromium.org

[bokan, 2015-06-05 20:41:13]

+dmazzoni@ for modules/accessibility
+vollick@ for platform and core/{dom|html|page|paint}

For context: this is a piece of a larger cleanup of ScrollableArea. The whole
thing was posted in https://codereview.chromium.org/1150003003/ but split up for
easier reviewing/landing.

[bokan, 2015-06-05 20:41:41]

The CQ bit was checked by bokan@chromium.org to run a CQ dry run

[bokan, 2015-06-05 20:41:41]

The patchset sent to the CQ was uploaded after l-g-t-m from skobes@chromium.org
Link to the patchset: https://codereview.chromium.org/1158673006/#ps100001
(title: "Rebase")

[commit-bot: I haz the power, 2015-06-05 20:42:13]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1158673006/100001

[commit-bot: I haz the power, 2015-06-05 21:52:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-05 21:52:44]

Dry run: This issue passed the CQ dry run.

[bokan, 2015-06-09 12:30:39]

On 2015/06/05 21:52:44, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

Ping

[Ian Vollick, 2015-06-09 19:29:13]

On 2015/06/09 at 12:30:39, bokan wrote:
> On 2015/06/05 21:52:44, commit-bot: I haz the power wrote:
> > Dry run: This issue passed the CQ dry run.
> 
> Ping

lgtm

[dmazzoni, 2015-06-09 19:40:23]

https://codereview.chromium.org/1158673006/diff/100001/Source/modules/accessi...
File Source/modules/accessibility/AXObject.cpp (right):

https://codereview.chromium.org/1158673006/diff/100001/Source/modules/accessi...
Source/modules/accessibility/AXObject.cpp:1089:
area->setScrollPosition(DoublePoint(offset.x(), offset.y()),
ProgrammaticScroll);
What are the implications of this being a programmatic scroll vs user scroll?

Typically we assume accessibility is the agent of the user - for example if a
user with a motor impairment issues a voice command to scroll down the page and
that triggers using accessibility APIs, we should treat that the same as if they
pressed PageDown or flicked their mouse scroll wheel.

For example, we call this before triggering a button press via accessibility:

UserGestureIndicator gestureIndicator(DefinitelyProcessingNewUserGesture);

Does that sound okay or am I misunderstanding the difference between user and
programmatic here?

[bokan, 2015-06-09 20:03:01]

https://codereview.chromium.org/1158673006/diff/100001/Source/modules/accessi...
File Source/modules/accessibility/AXObject.cpp (right):

https://codereview.chromium.org/1158673006/diff/100001/Source/modules/accessi...
Source/modules/accessibility/AXObject.cpp:1089:
area->setScrollPosition(DoublePoint(offset.x(), offset.y()),
ProgrammaticScroll);
On 2015/06/09 19:40:23, dmazzoni wrote:
> What are the implications of this being a programmatic scroll vs user scroll?
> 
> Typically we assume accessibility is the agent of the user - for example if a
> user with a motor impairment issues a voice command to scroll down the page
and
> that triggers using accessibility APIs, we should treat that the same as if
they
> pressed PageDown or flicked their mouse scroll wheel.
> 
> For example, we call this before triggering a button press via accessibility:
> 
> UserGestureIndicator gestureIndicator(DefinitelyProcessingNewUserGesture);
> 
> Does that sound okay or am I misunderstanding the difference between user and
> programmatic here?

Hmm...right now the only implication here is which ScrollAnimator gets used. We
have separate animators for user and programmatic scrolls (I'm not sure why) and
a scroll through one cancels any animations on the other.

The call I'm replacing here was taking the programmatic scroll path. In that
case, should this and AXScrollbar be a UserScroll? Functionally, I don't think
it makes much difference right now but we may key more info off this in the
future.

[dmazzoni, 2015-06-09 21:15:04]

lgtm

OK, if this is not changing any existing behavior that's fine. It wasn't clear
to me since the enum was new in this change.

I'll look into changing it to a user scroll as a follow-up.

[bokan, 2015-06-10 11:12:37]

On 2015/06/09 21:15:04, dmazzoni wrote:
> lgtm
> 
> OK, if this is not changing any existing behavior that's fine. It wasn't clear
> to me since the enum was new in this change.
> 
> I'll look into changing it to a user scroll as a follow-up.

Thanks, I've left TODOs so we don't forget.

[bokan, 2015-06-10 11:13:28]

The CQ bit was checked by bokan@chromium.org

[bokan, 2015-06-10 11:13:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
vollick@chromium.org, dmazzoni@chromium.org, skobes@chromium.org
Link to the patchset: https://codereview.chromium.org/1158673006/#ps120001
(title: "Rebase + Accessibility TODOs")

[commit-bot: I haz the power, 2015-06-10 11:13:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1158673006/120001

[commit-bot: I haz the power, 2015-06-10 11:23:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-10 11:23:41]

Try jobs failed on following builders:
  mac_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)

[bokan, 2015-06-10 12:05:29]

The CQ bit was checked by bokan@chromium.org

[bokan, 2015-06-10 12:05:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
vollick@chromium.org, dmazzoni@chromium.org, skobes@chromium.org
Link to the patchset: https://codereview.chromium.org/1158673006/#ps160001
(title: "Build fix")

[commit-bot: I haz the power, 2015-06-10 12:06:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1158673006/160001

[commit-bot: I haz the power, 2015-06-10 13:49:19]

Committed patchset #9 (id:160001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196867