Add WebView API for smoothScroll

Add WebView API for smoothScroll

This is implemented as a page scale animation with a target
position and duration, and no change to scale.

BUG=378984
Committed: https://crrev.com/3155fbd994ad80467c47149065f280ed0c2b1482
git-svn-id: svn://svn.chromium.org/blink/trunk@199617 bbb929c8-8fbe-4397-9dbb-9b2b20218538

[hush (inactive), 2015-07-23 01:59:53]

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

[hush (inactive), 2015-07-23 01:59:53]

PTAL, thanks!

By the way Chromium CL that uses the new fields is:
https://codereview.chromium.org/1251323002/

Jared/David:
Is this a good idea? Or is creating a brand new event type better?

[jdduke (slow), 2015-07-23 15:19:02]

jdduke@chromium.org changed reviewers:
+ jdduke@chromium.org

[jdduke (slow), 2015-07-23 15:19:02]

I'm not opposed to creating a new type, as it's nice to be explicit. However,
that would probably require a good deal more boilerplate, so I'm not opposed to
this approach either.

+aelias might have some thoughts.

https://codereview.chromium.org/1251473003/diff/1/public/web/WebInputEvent.h
File public/web/WebInputEvent.h (right):

https://codereview.chromium.org/1251473003/diff/1/public/web/WebInputEvent.h#...
public/web/WebInputEvent.h:500: float dx;
Do we need dx/dy if we have a velocity + duration?

I guess smooth scroll can have a slightly variable velocity at the
beginning/end, but if we're sharing the same event type I'd prefer not to have
"clashing" fields.

[aelias_OOO_until_Jul13, 2015-07-23 18:33:46]

aelias@chromium.org changed reviewers:
+ aelias@chromium.org

[aelias_OOO_until_Jul13, 2015-07-23 18:33:47]

Looking at the use cases for this, it's "pageUp", "pageDown" and
"requestChildRectangleOnScreen" APIs.  It seems like the user gesture input
pipeline is not quite a fit for this, as these are not user gestures.  I think a
more natural way to do that might be to create a PageScaleAnimation to the
target location (which can be a pure translation), as happens with double-tap
zoom, find-in-page and textbox zoom on Android.  You would send a message to the
Blink main thread, probably the most natural place to issue the call to the CC
API would be either WebViewImpl (like every other current use of
PageScaleAnimation) or possibly RenderViewImpl.

[jdduke (slow), 2015-07-23 18:49:19]

On 2015/07/23 18:33:47, aelias wrote:
> Looking at the use cases for this, it's "pageUp", "pageDown" and
> "requestChildRectangleOnScreen" APIs.  It seems like the user gesture input
> pipeline is not quite a fit for this, as these are not user gestures.  I think
a
> more natural way to do that might be to create a PageScaleAnimation to the
> target location (which can be a pure translation), as happens with double-tap
> zoom, find-in-page and textbox zoom on Android.  You would send a message to
the
> Blink main thread, probably the most natural place to issue the call to the CC
> API would be either WebViewImpl (like every other current use of
> PageScaleAnimation) or possibly RenderViewImpl.

Would be nice to avoid the main thread if we can void it, but maybe that's not
necessary? Do the initial side effects of these APIs need to be realized
synchronously?

[hush (inactive), 2015-07-23 18:58:01]

On 2015/07/23 18:49:19, jdduke wrote:
> On 2015/07/23 18:33:47, aelias wrote:
> > Looking at the use cases for this, it's "pageUp", "pageDown" and
> > "requestChildRectangleOnScreen" APIs.  It seems like the user gesture input
> > pipeline is not quite a fit for this, as these are not user gestures.  I
think
> a
> > more natural way to do that might be to create a PageScaleAnimation to the
> > target location (which can be a pure translation), as happens with
double-tap
> > zoom, find-in-page and textbox zoom on Android.  You would send a message to
> the
> > Blink main thread, probably the most natural place to issue the call to the
CC
> > API would be either WebViewImpl (like every other current use of
> > PageScaleAnimation) or possibly RenderViewImpl.
> 
> Would be nice to avoid the main thread if we can void it, but maybe that's not
> necessary? Do the initial side effects of these APIs need to be realized
> synchronously?

pageUp, pageDown and requestChildRectangleOnScreen(with "immediate" == false)
are not synchronous so it is okay for them to go through blink thread

[aelias_OOO_until_Jul13, 2015-07-23 18:59:07]

On 2015/07/23 at 18:49:19, jdduke wrote:
> Would be nice to avoid the main thread if we can void it, but maybe that's not
necessary? Do the initial side effects of these APIs need to be realized
synchronously?

The final result obviously isn't synchronously set, and if any user of these
APIs relies on the scroll offset synchronously having changed a little bit as
soon as the call returns, the input event message wouldn't get you that either. 
(I think it's unlikely that any app relies on that property anyway, I don't see
the use case.)

I think these are largely useless and rarely used APIs so I don't think we
should go out of our way to give them optimal performance either.  Let's just
implement them as simply as possible.

[jdduke (slow), 2015-07-23 19:06:15]

On 2015/07/23 18:59:07, aelias wrote:
> On 2015/07/23 at 18:49:19, jdduke wrote:
> > Would be nice to avoid the main thread if we can void it, but maybe that's
not
> necessary? Do the initial side effects of these APIs need to be realized
> synchronously?
> 
> The final result obviously isn't synchronously set, and if any user of these
> APIs relies on the scroll offset synchronously having changed a little bit as
> soon as the call returns, the input event message wouldn't get you that
either. 
> (I think it's unlikely that any app relies on that property anyway, I don't
see
> the use case.)

Right, I meant more do we need a synchronous return result of some kind that
indicates if scrolling is possible or has been started.

[aelias_OOO_until_Jul13, 2015-07-23 19:09:01]

On 2015/07/23 at 19:06:15, jdduke wrote:
> Right, I meant more do we need a synchronous return result of some kind that
indicates if scrolling is possible or has been started.

They do have such return values, but we can probably just compare the current
scroll offset with the target scroll offset and return true if unequal.

[jdduke (slow), 2015-07-23 19:14:50]

I guess the advantage of using an input event type is that it respects the
ordering of other input event types, but as you said I suspect these APIs are
legacy enough that it probably won't matter. I'm happy with either approach,
particularly if we can leverage existing page scale animation support.

[hush (inactive), 2015-07-23 19:19:43]

On 2015/07/23 19:14:50, jdduke wrote:
> I guess the advantage of using an input event type is that it respects the
> ordering of other input event types, but as you said I suspect these APIs are
> legacy enough that it probably won't matter. I'm happy with either approach,
> particularly if we can leverage existing page scale animation support.

I will have a try with PageScaleAnimation then

[hush (inactive), 2015-07-25 00:22:25]

PTAL ps2

[bokan, 2015-07-27 15:39:59]

lgtm

[jdduke (slow), 2015-07-27 15:49:02]

https://codereview.chromium.org/1251473003/diff/20001/public/web/WebView.h
File public/web/WebView.h (right):

https://codereview.chromium.org/1251473003/diff/20001/public/web/WebView.h#ne...
public/web/WebView.h:181: virtual void smoothScroll(int targetX, int targetY,
long durationMs);
I guess this handles all of pageDown/pageUp/requestChildRectOnScreen? Just
wondering if targetX/targetY is more convenient than deltaX/deltaY?

[hush (inactive), 2015-07-28 00:16:06]

https://codereview.chromium.org/1251473003/diff/20001/public/web/WebView.h
File public/web/WebView.h (right):

https://codereview.chromium.org/1251473003/diff/20001/public/web/WebView.h#ne...
public/web/WebView.h:181: virtual void smoothScroll(int targetX, int targetY,
long durationMs);
On 2015/07/27 15:49:02, jdduke wrote:
> I guess this handles all of pageDown/pageUp/requestChildRectOnScreen? Just
> wondering if targetX/targetY is more convenient than deltaX/deltaY?

yes. This handles all three.
I use target position instead of delta location because that's what
startPageScaleAnimation needs.

Actually, there is no need for "deltaX" and "deltaY" to exist in the first
place. "AwScrollOffsetManager#animateScrollTo" simply passes the target position
to content view core.

[jdduke (slow), 2015-07-28 15:53:27]

On 2015/07/28 00:16:06, hush wrote:
> https://codereview.chromium.org/1251473003/diff/20001/public/web/WebView.h
> File public/web/WebView.h (right):
> 
>
https://codereview.chromium.org/1251473003/diff/20001/public/web/WebView.h#ne...
> public/web/WebView.h:181: virtual void smoothScroll(int targetX, int targetY,
> long durationMs);
> On 2015/07/27 15:49:02, jdduke wrote:
> > I guess this handles all of pageDown/pageUp/requestChildRectOnScreen? Just
> > wondering if targetX/targetY is more convenient than deltaX/deltaY?
> 
> yes. This handles all three.
> I use target position instead of delta location because that's what
> startPageScaleAnimation needs.
> 
> Actually, there is no need for "deltaX" and "deltaY" to exist in the first
> place. "AwScrollOffsetManager#animateScrollTo" simply passes the target
position
> to content view core.

My thinking was that deltaX/deltaY might be slightly more robust to main thread
latency, e.g., it won't "destroy" any interim scrolls that occur, either from JS
or from gestures. I guess you could compute targetX/targetY at the last possible
second in the renderer, but maybe that fear is premature. Don't have a strong
preference either way, what you have is fine if others have no objections.

[aelias_OOO_until_Jul13, 2015-07-28 17:21:13]

On 2015/07/28 at 15:53:27, jdduke wrote:
> On 2015/07/28 00:16:06, hush wrote:
> > https://codereview.chromium.org/1251473003/diff/20001/public/web/WebView.h
> > File public/web/WebView.h (right):
> > 
> >
https://codereview.chromium.org/1251473003/diff/20001/public/web/WebView.h#ne...
> > public/web/WebView.h:181: virtual void smoothScroll(int targetX, int
targetY,
> > long durationMs);
> > On 2015/07/27 15:49:02, jdduke wrote:
> > > I guess this handles all of pageDown/pageUp/requestChildRectOnScreen? Just
> > > wondering if targetX/targetY is more convenient than deltaX/deltaY?
> > 
> > yes. This handles all three.
> > I use target position instead of delta location because that's what
> > startPageScaleAnimation needs.
> > 
> > Actually, there is no need for "deltaX" and "deltaY" to exist in the first
> > place. "AwScrollOffsetManager#animateScrollTo" simply passes the target
position
> > to content view core.
> 
> My thinking was that deltaX/deltaY might be slightly more robust to main
thread latency, e.g., it won't "destroy" any interim scrolls that occur, either
from JS or from gestures. I guess you could compute targetX/targetY at the last
possible second in the renderer, but maybe that fear is premature. Don't have a
strong preference either way, what you have is fine if others have no
objections.

Well, delta is more natural for pageUp/pageDown and target is more natural for
requestChildRectangleOnScreen, and since this is used for both APIs there's no
strong reason to prefer one over the other.  The target maps directly to to
PageScaleAnimation builder so we went with that one.

[hush (inactive), 2015-07-28 17:57:15]

The CQ bit was checked by hush@chromium.org

[commit-bot: I haz the power, 2015-07-28 17:57:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1251473003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1251473003/20001

[commit-bot: I haz the power, 2015-07-28 18:02:55]

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

[commit-bot: I haz the power, 2015-07-28 18:02:56]

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

[hush (inactive), 2015-07-28 18:42:16]

hush@chromium.org changed reviewers:
+ dcheng@chromium.org

[hush (inactive), 2015-07-28 18:42:16]

dcheng@
can you take a look at public/? Thanks!

[dcheng, 2015-07-28 18:50:15]

https://codereview.chromium.org/1251473003/diff/80001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/1251473003/diff/80001/Source/web/WebViewImpl....
Source/web/WebViewImpl.cpp:2879: LocalFrame* frame = page()->mainFrame() &&
page()->mainFrame()->isLocalFrame()
Who calls this? Will this be covered by a test on the waterfall so we'll know to
fix this later for OOPI?

[aelias_OOO_until_Jul13, 2015-07-28 19:00:21]

https://codereview.chromium.org/1251473003/diff/80001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/1251473003/diff/80001/Source/web/WebViewImpl....
Source/web/WebViewImpl.cpp:2879: LocalFrame* frame = page()->mainFrame() &&
page()->mainFrame()->isLocalFrame()
On 2015/07/28 at 18:50:14, dcheng wrote:
> Who calls this? Will this be covered by a test on the waterfall so we'll know
to fix this later for OOPI?

Actually, why is this code here at all?  The code below doesn't use the frame. 
startPageScaleFactor doesn't rely on it either.

[hush (inactive), 2015-07-28 19:18:11]

https://codereview.chromium.org/1251473003/diff/80001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/1251473003/diff/80001/Source/web/WebViewImpl....
Source/web/WebViewImpl.cpp:2879: LocalFrame* frame = page()->mainFrame() &&
page()->mainFrame()->isLocalFrame()
On 2015/07/28 19:00:21, aelias wrote:
> On 2015/07/28 at 18:50:14, dcheng wrote:
> > Who calls this? Will this be covered by a test on the waterfall so we'll
know
> to fix this later for OOPI?
> 
> Actually, why is this code here at all?  The code below doesn't use the frame.

> startPageScaleFactor doesn't rely on it either.

I was copying the logic of WebViewImpl::scrollFocusedNodeIntoRect. I initially
thought scrollFocusedNodeIntoRect does not need frame either, simply using it as
a way to determine valid status. Sorry I missed the part that the frame is used
for retrieving the focused element.
Anyways, I will remove this block.

dcheng@:
This api will eventually be called by Android WebView to implement
pageUp/pageDown.
The callpath is like WebView#pageUp -> ContentViewCore->SmoothScroll ->
WebContents->SmoothScroll -> IPC to blink InputMsg_SmoothScroll ->
render_view_impl->SmoothScroll
I have another CL in progress here https://codereview.chromium.org/1251323002/

[dcheng, 2015-07-28 23:00:06]

Have you considered how this API will work with OOPI? Will it ever need to
scroll subframes?

[hush (inactive), 2015-07-28 23:09:17]

On 2015/07/28 23:00:06, dcheng wrote:
> Have you considered how this API will work with OOPI? Will it ever need to
> scroll subframes?
I'm familiar with OOPI. Is it just regular iframe with a different process
model?

I only want to scroll the root layer with this API, so it is mainframe only. So
if a page contains an iframe, then smooth scroll won't scroll it.
On a desktop Chrome, the user could just put the focus inside the iframe and
press pageUp/Down key. There is no focus element on Android Chrome, so there is
no appropriate hit test location. Only root layer scroll makes sense in this
case.

[dcheng, 2015-07-28 23:12:38]

On 2015/07/28 at 23:09:17, hush wrote:
> On 2015/07/28 23:00:06, dcheng wrote:
> > Have you considered how this API will work with OOPI? Will it ever need to
> > scroll subframes?
> I'm familiar with OOPI. Is it just regular iframe with a different process
model?
> 
> I only want to scroll the root layer with this API, so it is mainframe only.
So if a page contains an iframe, then smooth scroll won't scroll it.
> On a desktop Chrome, the user could just put the focus inside the iframe and
press pageUp/Down key. There is no focus element on Android Chrome, so there is
no appropriate hit test location. Only root layer scroll makes sense in this
case.

https://www.chromium.org/developers/design-documents/oop-iframes covers the
design for OOPI at a high level. For now, this should be fine. I'll have to
think about how we should treat WebView methods that are only valid in the
process hosting the main frame.

lgtm

[hush (inactive), 2015-07-28 23:27:30]

The CQ bit was checked by hush@chromium.org

[hush (inactive), 2015-07-28 23:27:30]

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

[commit-bot: I haz the power, 2015-07-28 23:27:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1251473003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1251473003/100001

[commit-bot: I haz the power, 2015-07-29 00:42:25]

Committed patchset #6 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=199617

[commit-bot: I haz the power, 2015-09-23 11:52:05]

Patchset 6 (id:??) landed as
https://crrev.com/3155fbd994ad80467c47149065f280ed0c2b1482