1349750 - APZ: Avoid blocking on the main thread to initiate APZ scrollbar dragging

Status: RESOLVED FIXED

(Core :: Panning and Zooming, enhancement)

Description

[Botond Ballo [:botond]]

In our current implementation of async scrollbar dragging, we still need to block on the main thread to kick off the drag (once that happens, the rest of the drag will be async).

A further improvement would be to avoid having to block on the main thread at all, such that the entire drag can happen in the compositor, as with other APZ input methods like wheel and touch.

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

We will look at this after the async keyboard scrolling (bug 1351783).  Parked until then.

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

Given the recent regressions in APZ scrollbar code, we will in fact look into this ahead of the keyboard scrolling, as we're in that code anyway.

Comment 3

[Botond Ballo [:botond]]

The first thing that occurs to me for this bug is that we can only avoid blocking on the main thread to initiate APZ scrollbar dragging for scroll frames that are already layerized (otherwise, we need to block on the main thread to layerize the scroll frame anyways).

I assume we don't want to change our policy regarding when we layerize scroll frames. (We currently layerize by default the "primary async-scrollable frame", which is the RCD-RSF if it's scrollable, and otherwise the first scrollable subframe that we encounter on the page. In addition, we layerize any scroll when it's scrolled, and keep it layerized until a timer expires.)

One implication of this is that the current codepath, where the main thread kicks off the scrollbar dragging, needs to stick around, and it needs to play nicely with the new codepath that will be added for async initiation of the drag.

Comment 4

[Botond Ballo [:botond]]

APZ currently waits for the main thread to provide two kinds of information before starting an async scrollbar drag:

  (1) Whether to proceed with the async drag at all. Codewise, this
      is represented in the various early-return conditions in
      nsSliderFrame::StartAPZDrag(), as well as in whether we get
      into that function at all.

  (2) Various metrics needed to perform the async drag, collected
      in AsyncDragMetrics.

Some of the information needed to determine (1), and some of the metrics in (2), are already available on the compositor thread. For the rest, we need to store it in the layer tree, so the information is "already there" when APZ processes a mouse event that can potentially start a drag, and we don't need to wait for the main thread to provide it.

I'm thinking of storing the relevant information on the scroll thumb's ContainerLayer. We already store some similar information (the thumb ratio) there.

Comment 5

[Botond Ballo [:botond]]

Attachment: Bug 1349750 - Group scroll thumb-related information in Layer into a ScrollThumbData structure.

The patch also renames Layer::SetScrollbarData() to Layer::SetScrollThumbData()
for clarity.

Review commit: https://reviewboard.mozilla.org/r/136018/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/136018/

Comment 6

[Botond Ballo [:botond]]

Attachment: Bug 1349750 - Remove the no-longer-used WebRenderScrollData[Wrapper]::GetScrollThumbLength().

Also remove a vestigial declaration of the previously removed method
HitTestingTreeNode::GetScrollThumbLength().

Review commit: https://reviewboard.mozilla.org/r/136020/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/136020/

Comment 7

[Botond Ballo [:botond]]

Attachment: Bug 1349750 - Introduce a PlainOldDataSerializer utility class and use it for SimpleLayerAttributes.

Review commit: https://reviewboard.mozilla.org/r/136022/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/136022/

Comment 8

[Botond Ballo [:botond]]

Attachment: Bug 1349750 - Store the entire ScrollThumbData in HitTestingTreeNode.

This is preparation for future changes that will add fields to ScrollThumbData
that will be accessed via HitTestingTreeNode.

To accomplish this, this patch also exposes the entire ScrollThumbData via
LayerMetricsWrapper and WebRenderScrollDataWrapper.

Review commit: https://reviewboard.mozilla.org/r/136024/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/136024/

Comment 9

[Botond Ballo [:botond]]

Attachment: Bug 1349750 - Move the scroll thumb length from AsyncDragMetrics to ScrollThumbData.

Review commit: https://reviewboard.mozilla.org/r/136026/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/136026/

Comment 10

[Botond Ballo [:botond]]

This is a WIP patch series. No functional changes yet, just refactoring and laying the groundwork for future functional changes.

Comment 11

[Botond Ballo [:botond]]

Attachment: Bug 1349750 - Move the scroll track extents from AsyncDragMetrics to ScrollThumbData.

To conserve space in LayerAttributes, we only store the extents along the
relevant axis.

Review commit: https://reviewboard.mozilla.org/r/136692/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/136692/

Comment 12

[Botond Ballo [:botond]]

Attachment: Bug 1349750 - Add an 'is async-draggable' flag to ScrollThumbData.

This flag is set to false if there are any conditions that only the main
thread knows about that prevent the thumb from being async-dragged.

Review commit: https://reviewboard.mozilla.org/r/136694/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/136694/

Comment 13

[Botond Ballo [:botond]]

Attachment: Bug 1349750 - If APZ hit testing hits a scrollbar node, return the node for later use.

Review commit: https://reviewboard.mozilla.org/r/136696/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/136696/

Comment 14

[Botond Ballo [:botond]]

Attachment: Bug 1349750 - Have AsyncPanZoomController expose an IsScrollInfoLayer() method.

Review commit: https://reviewboard.mozilla.org/r/136698/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/136698/

Comment 15

[Botond Ballo [:botond]]

Attachment: Bug 1349750 - Add HitTestingTreeNode::IsScrollThumbNode().

Review commit: https://reviewboard.mozilla.org/r/136700/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/136700/

Comment 16

[Botond Ballo [:botond]]

Attachment: Bug 1349750 - Add a ConvertScrollbarPoint() helper to AsyncPanZoomController.

Review commit: https://reviewboard.mozilla.org/r/136702/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/136702/

Comment 17

[Botond Ballo [:botond]]

Attachment: Bug 1349750 - Have AsyncDragMetrics use ScrollDirection instead of rolling its own direction enum.

Review commit: https://reviewboard.mozilla.org/r/136704/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/136704/

Comment 18

[Botond Ballo [:botond]]

Attachment: Bug 1349750 - Have APZ initiate async scrollbar dragging when possible.

Review commit: https://reviewboard.mozilla.org/r/136706/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/136706/

Comment 19

[Botond Ballo [:botond]]

Comment on attachment 8864361 [details]
Bug 1349750 - Move the scroll thumb length from AsyncDragMetrics to ScrollThumbData.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136026/diff/1-2/

Comment 20

[Botond Ballo [:botond]]

This patch series is now is now complete, in the sense that all the parts (that I think we'll need) are there.

It passes some basic smoke tests, but I'd like to test it a bit more extensively before requesting review.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce3b916304934bdf5addb09b1ca2d0477989b8cd

Comment 21

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #20)
> Try push:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=ce3b916304934bdf5addb09b1ca2d0477989b8cd

This is showing an Android build failure, and two test failures:

  APZCTreeManagerTester.WheelInterruptedByMouseDrag
  test_group_mouseevents

Comment 22

[Botond Ballo [:botond]]

Attachment: Bug 1349750 - Store the thumb's start offset in ScrollThumbData.

Also add a comment to AsyncDragMetrics documenting that mScrollbarDragOffset
is relative to the thumb's start offset.

Review commit: https://reviewboard.mozilla.org/r/138184/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/138184/

Comment 23

[Botond Ballo [:botond]]

Attachment: Bug 1349750 - Put async initiation of scrollbar drags behind a pref.

This allows the feature to ride the trains later than async scrollbar
dragging itself, if desired.

Review commit: https://reviewboard.mozilla.org/r/138186/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/138186/

Comment 24

[Botond Ballo [:botond]]

Comment on attachment 8864357 [details]
Bug 1349750 - Group scroll thumb-related information in Layer into a ScrollThumbData structure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136018/diff/1-2/

Comment 25

[Botond Ballo [:botond]]

Comment on attachment 8864358 [details]
Bug 1349750 - Remove the no-longer-used WebRenderScrollData[Wrapper]::GetScrollThumbLength().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136020/diff/1-2/

Comment 26

[Botond Ballo [:botond]]

Comment on attachment 8864359 [details]
Bug 1349750 - Introduce a PlainOldDataSerializer utility class and use it for SimpleLayerAttributes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136022/diff/1-2/

Comment 27

[Botond Ballo [:botond]]

Comment on attachment 8864360 [details]
Bug 1349750 - Store the entire ScrollThumbData in HitTestingTreeNode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136024/diff/1-2/

Comment 28

[Botond Ballo [:botond]]

Comment on attachment 8864361 [details]
Bug 1349750 - Move the scroll thumb length from AsyncDragMetrics to ScrollThumbData.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136026/diff/2-3/

Comment 29

[Botond Ballo [:botond]]

Comment on attachment 8865040 [details]
Bug 1349750 - Move the scroll track extents from AsyncDragMetrics to ScrollThumbData.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136692/diff/1-2/

Comment 30

[Botond Ballo [:botond]]

Comment on attachment 8865041 [details]
Bug 1349750 - Add an 'is async-draggable' flag to ScrollThumbData.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136694/diff/1-2/

Comment 31

[Botond Ballo [:botond]]

Comment on attachment 8865042 [details]
Bug 1349750 - If APZ hit testing hits a scrollbar node, return the node for later use.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136696/diff/1-2/

Comment 32

[Botond Ballo [:botond]]

Comment on attachment 8865043 [details]
Bug 1349750 - Have AsyncPanZoomController expose an IsScrollInfoLayer() method.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136698/diff/1-2/

Comment 33

[Botond Ballo [:botond]]

Comment on attachment 8865044 [details]
Bug 1349750 - Add HitTestingTreeNode::IsScrollThumbNode().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136700/diff/1-2/

Comment 34

[Botond Ballo [:botond]]

Comment on attachment 8865045 [details]
Bug 1349750 - Add a ConvertScrollbarPoint() helper to AsyncPanZoomController.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136702/diff/1-2/

Comment 35

[Botond Ballo [:botond]]

Comment on attachment 8865046 [details]
Bug 1349750 - Have AsyncDragMetrics use ScrollDirection instead of rolling its own direction enum.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136704/diff/1-2/

Comment 36

[Botond Ballo [:botond]]

Comment on attachment 8865047 [details]
Bug 1349750 - Have APZ initiate async scrollbar dragging when possible.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136706/diff/1-2/

Comment 37

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #21)
> This is showing an Android build failure, and two test failures:
> 
>   APZCTreeManagerTester.WheelInterruptedByMouseDrag
>   test_group_mouseevents

These are all fixed in the updated patch series, along with various other issues found during manual testing.

I'm now satisfied enough with this patch series to put it up for review.

There are a few known limitations of the current implementation:

  1) When initiating an async drag, the APZCTM relies on the position 
     of the thumb in two places

      - during hit testing (to determine whether we hit the thumb 
        rather than another part of the scrollbar)

      - during computation of the drag start offset

     However, the position used for these computations does not take
     into account any async transform that may be applied to the
     thumb as a result of async-scrolling its target APZC (because
     that async transform is only computed and applied in
     AsyncCompositionManager).

  2) We don't handle scroll-to-click, which is a feature of scrollbars
     on some platforms where a mouse-down on a part of the scroll track
     outside the thumb will make the thumb jump to the mouse position,
     and you can drag the thumb from that point in the same gesture.

I plan to fix (1) in a follow-up bug. (2) I will probably put on the backlog.

Comment 38

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #37)
>   1) When initiating an async drag, the APZCTM relies on the position 
>      of the thumb in two places
> 
>       - during hit testing (to determine whether we hit the thumb 
>         rather than another part of the scrollbar)
> 
>       - during computation of the drag start offset
> 
>      However, the position used for these computations does not take
>      into account any async transform that may be applied to the
>      thumb as a result of async-scrolling its target APZC (because
>      that async transform is only computed and applied in
>      AsyncCompositionManager).

I forgot to mention the implication of this: if you async-scroll the page (e.g. with mousewheel), and then grab the thumb before the main thread has had a chance to repaint, *that* drag will not be initiated asynchronously.

Comment 39

[Botond Ballo [:botond]]

New Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d4d9d929ab2e32a04571c55e866837adc9fd652

Comment 40

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8864357 [details]
Bug 1349750 - Group scroll thumb-related information in Layer into a ScrollThumbData structure.

https://reviewboard.mozilla.org/r/136018/#review141590

Comment 41

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8864358 [details]
Bug 1349750 - Remove the no-longer-used WebRenderScrollData[Wrapper]::GetScrollThumbLength().

https://reviewboard.mozilla.org/r/136020/#review141592

Comment 42

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8864359 [details]
Bug 1349750 - Introduce a PlainOldDataSerializer utility class and use it for SimpleLayerAttributes.

https://reviewboard.mozilla.org/r/136022/#review141596

Comment 43

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8864360 [details]
Bug 1349750 - Store the entire ScrollThumbData in HitTestingTreeNode.

https://reviewboard.mozilla.org/r/136024/#review141600

Man, once we have IsPod working properly we should use the PlainOldDataSerializer in a lot more places. Forgetting to update the (de-)serializing code on adding new fields is something that has caused numerous bugs.

Comment 44

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8864361 [details]
Bug 1349750 - Move the scroll thumb length from AsyncDragMetrics to ScrollThumbData.

https://reviewboard.mozilla.org/r/136026/#review141602

::: gfx/layers/LayerAttributes.h:23
(Diff revision 3)
> -    : mDirection(ScrollDirection::NONE), mThumbRatio(0.0f) {}
> -  ScrollThumbData(ScrollDirection aDirection, float aThumbRatio)
> +    : mDirection(ScrollDirection::NONE),
> +      mThumbRatio(0.0f)

nit: line up the comma under the colon

::: gfx/layers/LayerAttributes.h:29
(Diff revision 3)
> +    : mDirection(aDirection),
> +      mThumbRatio(aThumbRatio),
> +      mThumbLength(aThumbLength) {}

ditto

Comment 45

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8865040 [details]
Bug 1349750 - Move the scroll track extents from AsyncDragMetrics to ScrollThumbData.

https://reviewboard.mozilla.org/r/136692/#review141606

Comment 46

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8865041 [details]
Bug 1349750 - Add an 'is async-draggable' flag to ScrollThumbData.

https://reviewboard.mozilla.org/r/136694/#review141610

::: layout/xul/nsSliderFrame.cpp:394
(Diff revision 2)
>        if (!scrollFrame) {
>          return;
>        }

You added this early return in the previous patch. I'm wondering if this is ever expected to hit, and if so, if it might affect the display list construction. It might be slightly safer to set isAsyncScrollable=false instead of doing an early return here (and skipping any calculations that require the scrollFrame). I'm not sure if you actually encountered the case where the scrollFrame is null and if it's ok to skip the display list building steps in that case.

Comment 47

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8865042 [details]
Bug 1349750 - If APZ hit testing hits a scrollbar node, return the node for later use.

https://reviewboard.mozilla.org/r/136696/#review141612

Comment 48

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8865043 [details]
Bug 1349750 - Have AsyncPanZoomController expose an IsScrollInfoLayer() method.

https://reviewboard.mozilla.org/r/136698/#review141614

Comment 49

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8865044 [details]
Bug 1349750 - Add HitTestingTreeNode::IsScrollThumbNode().

https://reviewboard.mozilla.org/r/136700/#review141616

Comment 50

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8865045 [details]
Bug 1349750 - Add a ConvertScrollbarPoint() helper to AsyncPanZoomController.

https://reviewboard.mozilla.org/r/136702/#review141618

Comment 51

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8865046 [details]
Bug 1349750 - Have AsyncDragMetrics use ScrollDirection instead of rolling its own direction enum.

https://reviewboard.mozilla.org/r/136704/#review141620

Nice cleanup, thanks! :)

Comment 52

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8866581 [details]
Bug 1349750 - Store the thumb's start offset in ScrollThumbData.

https://reviewboard.mozilla.org/r/138184/#review141622

Comment 53

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8865047 [details]
Bug 1349750 - Have APZ initiate async scrollbar dragging when possible.

https://reviewboard.mozilla.org/r/136706/#review141624

::: gfx/layers/apz/src/APZCTreeManager.cpp:813
(Diff revision 2)
> +        if (apzDragEnabled && startsDrag && hitScrollbarNode &&
> +            hitScrollbarNode->IsScrollThumbNode() &&
> +            hitScrollbarNode->GetScrollThumbData().mIsAsyncDraggable &&
> +            // check that the scrollbar's target scroll frame is layerized
> +            hitScrollbarNode->GetScrollTargetId() == apzc->GetGuid().mScrollId &&
> +            !apzc->IsScrollInfoLayer() && mInputQueue->GetCurrentDragBlock()) {

That's quite the list of conditions! But it looks good :)

Comment 54

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8866582 [details]
Bug 1349750 - Put async initiation of scrollbar drags behind a pref.

https://reviewboard.mozilla.org/r/138186/#review141626

Great set of patches, thanks! It was actually fun to review :)

Comment 55

[Botond Ballo [:botond]]

Comment on attachment 8864357 [details]
Bug 1349750 - Group scroll thumb-related information in Layer into a ScrollThumbData structure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136018/diff/2-3/

Comment 56

[Botond Ballo [:botond]]

Comment on attachment 8864358 [details]
Bug 1349750 - Remove the no-longer-used WebRenderScrollData[Wrapper]::GetScrollThumbLength().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136020/diff/2-3/

Comment 57

[Botond Ballo [:botond]]

Comment on attachment 8864359 [details]
Bug 1349750 - Introduce a PlainOldDataSerializer utility class and use it for SimpleLayerAttributes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136022/diff/2-3/

Comment 58

[Botond Ballo [:botond]]

Comment on attachment 8864360 [details]
Bug 1349750 - Store the entire ScrollThumbData in HitTestingTreeNode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136024/diff/2-3/

Comment 59

[Botond Ballo [:botond]]

Comment on attachment 8864361 [details]
Bug 1349750 - Move the scroll thumb length from AsyncDragMetrics to ScrollThumbData.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136026/diff/3-4/

Comment 60

[Botond Ballo [:botond]]

Comment on attachment 8865041 [details]
Bug 1349750 - Add an 'is async-draggable' flag to ScrollThumbData.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136694/diff/2-3/

Comment 61

[Botond Ballo [:botond]]

Comment on attachment 8865040 [details]
Bug 1349750 - Move the scroll track extents from AsyncDragMetrics to ScrollThumbData.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136692/diff/2-3/

Comment 62

[Botond Ballo [:botond]]

Comment on attachment 8865042 [details]
Bug 1349750 - If APZ hit testing hits a scrollbar node, return the node for later use.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136696/diff/2-3/

Comment 63

[Botond Ballo [:botond]]

Comment on attachment 8865043 [details]
Bug 1349750 - Have AsyncPanZoomController expose an IsScrollInfoLayer() method.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136698/diff/2-3/

Comment 64

[Botond Ballo [:botond]]

Comment on attachment 8865044 [details]
Bug 1349750 - Add HitTestingTreeNode::IsScrollThumbNode().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136700/diff/2-3/

Comment 65

[Botond Ballo [:botond]]

Comment on attachment 8865045 [details]
Bug 1349750 - Add a ConvertScrollbarPoint() helper to AsyncPanZoomController.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136702/diff/2-3/

Comment 66

[Botond Ballo [:botond]]

Comment on attachment 8865046 [details]
Bug 1349750 - Have AsyncDragMetrics use ScrollDirection instead of rolling its own direction enum.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136704/diff/2-3/

Comment 67

[Botond Ballo [:botond]]

Comment on attachment 8866581 [details]
Bug 1349750 - Store the thumb's start offset in ScrollThumbData.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138184/diff/1-2/

Comment 68

[Botond Ballo [:botond]]

Comment on attachment 8865047 [details]
Bug 1349750 - Have APZ initiate async scrollbar dragging when possible.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136706/diff/2-3/

Comment 69

[Botond Ballo [:botond]]

Comment on attachment 8866582 [details]
Bug 1349750 - Put async initiation of scrollbar drags behind a pref.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138186/diff/1-2/

Comment 70

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #44)
> nit: line up the comma under the colon

Fixed.

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #46)
> You added this early return in the previous patch. I'm wondering if this is
> ever expected to hit, and if so, if it might affect the display list
> construction. It might be slightly safer to set isAsyncScrollable=false
> instead of doing an early return here (and skipping any calculations that
> require the scrollFrame). I'm not sure if you actually encountered the case
> where the scrollFrame is null and if it's ok to skip the display list
> building steps in that case.

Good point, to be safe I revised the code as suggested.

The updated patch series is also rebased onto the latest m-c tip.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=422f2874c52bd2829975b5902965b1d86aef17ac

Comment 71

[Botond Ballo [:botond]]

While testing this patch on a page with artificial jank to see the async initiation in action, I realized that it wasn't actually achieving async initiation. It improved things in that we were no longer waiting for a target confirmation from content, but we still were waiting for a content response.

Thankfully, this is easy to fix. I verified that content can't prevent scrollbar dragging by registering a mouse event listener and preventDefault()ing the event, so we can mark the drag block as having its content response received at the same time we confirm it.

Comment 72

[Botond Ballo [:botond]]

Attachment: Bug 1349750 - When APZ initiates an async drag, do not wait for a content response.

Review commit: https://reviewboard.mozilla.org/r/138940/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/138940/

Comment 73

[Botond Ballo [:botond]]

Comment on attachment 8865040 [details]
Bug 1349750 - Move the scroll track extents from AsyncDragMetrics to ScrollThumbData.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136692/diff/3-4/

Comment 74

[Botond Ballo [:botond]]

Comment on attachment 8865042 [details]
Bug 1349750 - If APZ hit testing hits a scrollbar node, return the node for later use.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136696/diff/3-4/

Comment 75

[Botond Ballo [:botond]]

Comment on attachment 8865043 [details]
Bug 1349750 - Have AsyncPanZoomController expose an IsScrollInfoLayer() method.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136698/diff/3-4/

Comment 76

[Botond Ballo [:botond]]

Comment on attachment 8865044 [details]
Bug 1349750 - Add HitTestingTreeNode::IsScrollThumbNode().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136700/diff/3-4/

Comment 77

[Botond Ballo [:botond]]

Comment on attachment 8865045 [details]
Bug 1349750 - Add a ConvertScrollbarPoint() helper to AsyncPanZoomController.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136702/diff/3-4/

Comment 78

[Botond Ballo [:botond]]

Comment on attachment 8865046 [details]
Bug 1349750 - Have AsyncDragMetrics use ScrollDirection instead of rolling its own direction enum.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136704/diff/3-4/

Comment 79

[Botond Ballo [:botond]]

Comment on attachment 8866581 [details]
Bug 1349750 - Store the thumb's start offset in ScrollThumbData.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138184/diff/2-3/

Comment 80

[Botond Ballo [:botond]]

Comment on attachment 8865047 [details]
Bug 1349750 - Have APZ initiate async scrollbar dragging when possible.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136706/diff/3-4/

Comment 81

[Botond Ballo [:botond]]

Comment on attachment 8866582 [details]
Bug 1349750 - Put async initiation of scrollbar drags behind a pref.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138186/diff/2-3/

Comment 82

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8867396 [details]
Bug 1349750 - When APZ initiates an async drag, do not wait for a content response.

https://reviewboard.mozilla.org/r/138940/#review142290

Comment 83

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/649084d26dad
Group scroll thumb-related information in Layer into a ScrollThumbData structure. r=kats
https://hg.mozilla.org/integration/autoland/rev/2383e9189274
Remove the no-longer-used WebRenderScrollData[Wrapper]::GetScrollThumbLength(). r=kats
https://hg.mozilla.org/integration/autoland/rev/c744347a18a5
Introduce a PlainOldDataSerializer utility class and use it for SimpleLayerAttributes. r=kats
https://hg.mozilla.org/integration/autoland/rev/b7f383b496b9
Store the entire ScrollThumbData in HitTestingTreeNode. r=kats
https://hg.mozilla.org/integration/autoland/rev/1c1eb8a23e87
Move the scroll thumb length from AsyncDragMetrics to ScrollThumbData. r=kats
https://hg.mozilla.org/integration/autoland/rev/9b08e1862d70
Add an 'is async-draggable' flag to ScrollThumbData. r=kats
https://hg.mozilla.org/integration/autoland/rev/a243424febf2
Move the scroll track extents from AsyncDragMetrics to ScrollThumbData. r=kats
https://hg.mozilla.org/integration/autoland/rev/1cbbe0c51147
If APZ hit testing hits a scrollbar node, return the node for later use. r=kats
https://hg.mozilla.org/integration/autoland/rev/1fea41757d52
Have AsyncPanZoomController expose an IsScrollInfoLayer() method. r=kats
https://hg.mozilla.org/integration/autoland/rev/aa5b12b989f9
Add HitTestingTreeNode::IsScrollThumbNode(). r=kats
https://hg.mozilla.org/integration/autoland/rev/62d950c3bcca
Add a ConvertScrollbarPoint() helper to AsyncPanZoomController. r=kats
https://hg.mozilla.org/integration/autoland/rev/523311854f7f
Have AsyncDragMetrics use ScrollDirection instead of rolling its own direction enum. r=kats
https://hg.mozilla.org/integration/autoland/rev/eb3028eb3132
Store the thumb's start offset in ScrollThumbData. r=kats
https://hg.mozilla.org/integration/autoland/rev/f20207f554ab
Have APZ initiate async scrollbar dragging when possible. r=kats
https://hg.mozilla.org/integration/autoland/rev/85cd9d40c8e2
Put async initiation of scrollbar drags behind a pref. r=kats
https://hg.mozilla.org/integration/autoland/rev/eee03f85bdad
When APZ initiates an async drag, do not wait for a content response. r=kats

Comment 84

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/649084d26dad
https://hg.mozilla.org/mozilla-central/rev/2383e9189274
https://hg.mozilla.org/mozilla-central/rev/c744347a18a5
https://hg.mozilla.org/mozilla-central/rev/b7f383b496b9
https://hg.mozilla.org/mozilla-central/rev/1c1eb8a23e87
https://hg.mozilla.org/mozilla-central/rev/9b08e1862d70
https://hg.mozilla.org/mozilla-central/rev/a243424febf2
https://hg.mozilla.org/mozilla-central/rev/1cbbe0c51147
https://hg.mozilla.org/mozilla-central/rev/1fea41757d52
https://hg.mozilla.org/mozilla-central/rev/aa5b12b989f9
https://hg.mozilla.org/mozilla-central/rev/62d950c3bcca
https://hg.mozilla.org/mozilla-central/rev/523311854f7f
https://hg.mozilla.org/mozilla-central/rev/eb3028eb3132
https://hg.mozilla.org/mozilla-central/rev/f20207f554ab
https://hg.mozilla.org/mozilla-central/rev/85cd9d40c8e2
https://hg.mozilla.org/mozilla-central/rev/eee03f85bdad

Comment 85

[avada]

There's something very wrong about the scrollbar.

If I click an empty space on the scrollbar and hold the button, the thumb doesn't stop at that point. It overshoots it.
With default settings it only overshoots by abou the height of the thumb


But it's a lot worse if you have have the scrolling settings customized like have. This time the position is overshot by a ridiculous amount. Might be roughly as much as the the starting distance between the thumb and the clicked position. So it might be half a page or more.


general.smoothScroll.lines.durationMaxMS;450
general.smoothScroll.lines.durationMinMS;450
general.smoothScroll.mouseWheel.durationMaxMS;1500
general.smoothScroll.pages.durationMaxMS;385
general.smoothScroll.pages.durationMinMS;385

(I posted all my customized settings for completeness' sake)


Naturally I have these customized since with default it doesn't feel smooth at all, especially the scrollbar press and hold scrolling.

Comment 86

[Botond Ballo [:botond]]

avada, what OS are you testing on? 

On Linux, I see two behaviours when clicking on empty space in the scrollbar, on Firefox Release:

  1) Plain left click ==> The thumb immediately jumps to the clicked position
 
  2) Shift + left click, or right click ==> The thumb starts moving in the
     direction of your click, but if you keep holding down it doesn't stop
     until it reaches the end of the scroll range.

I am seeing both of these behaviours preserved in Firefox Nightly.

Comment 87

[avada]

(In reply to Botond Ballo [:botond] from comment #86)
> avada, what OS are you testing on? 
> 
> On Linux, I see two behaviours when clicking on empty space in the
> scrollbar, on Firefox Release:
> 
>   1) Plain left click ==> The thumb immediately jumps to the clicked position
>  
>   2) Shift + left click, or right click ==> The thumb starts moving in the
>      direction of your click, but if you keep holding down it doesn't stop
>      until it reaches the end of the scroll range.
> 
> I am seeing both of these behaviours preserved in Firefox Nightly.

I'm on Windows (8.1). And as far as I remember it never worked like that. Right click does nothing at all.
Left click normally scrolls by one screen height and on hold keeps doing it until it reaches the cursor, or at least it should, but it overshoots.
Shift + leftclick (which I didn't know about until now) jumps instantly to the cursor position. (Accurately)

Here are some screencasts showing the behavior with default and customized general.smoothScroll.pages.* values:


https://drive.google.com/open?id=0ByfdfPvnoDuzSFpNT0VRdVRpUFk

Comment 88

[Botond Ballo [:botond]]

(In reply to avada from comment #87)
> I'm on Windows (8.1). And as far as I remember it never worked like that.
> Right click does nothing at all.
> Left click normally scrolls by one screen height and on hold keeps doing it
> until it reaches the cursor, or at least it should, but it overshoots.
> Shift + leftclick (which I didn't know about until now) jumps instantly to
> the cursor position. (Accurately)

Yeah, some of this behaviour varies by platform.

I'll be near a Windows machine on Friday, I'll test the behaviour on Windows then. Leaving needinfo on me until then.

Comment 89

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #88)
> I'll be near a Windows machine on Friday, I'll test the behaviour on Windows
> then. Leaving needinfo on me until then.

I can reproduce the overshooting described in comment 85 on Windows. However, it doesn't seem to be related to the changes in this bug, and in fact it doesn't seem to be a recent regression at all. Mozregression gave me a regression window in August 2014.

Comment 90

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #89)
> I can reproduce the overshooting described in comment 85 on Windows.
> However, it doesn't seem to be related to the changes in this bug, and in
> fact it doesn't seem to be a recent regression at all. Mozregression gave me
> a regression window in August 2014.

Filed bug 1369847 for this.

Comment 91

[avada]

(In reply to Botond Ballo [:botond] from comment #89)
> 
> I can reproduce the overshooting described in comment 85 on Windows.
> However, it doesn't seem to be related to the changes in this bug, and in
> fact it doesn't seem to be a recent regression at all. Mozregression gave me
> a regression window in August 2014.

I see. I wasn't sure how old it is, I thought I'd mention it anyway since you were touching scrollbar code.

Comment 92

[Botond Ballo [:botond]]

I think it's misleading to alias this bug as "apz-scrollbar-dragging". The bug that tracks APZ scrollbar dragging is bug 1211610. This bug concerns avoiding the initial round-trip to the main thread to kick off APZ scrollbar dragging, which is just a piece of the puzzle.