1199885 - Let nsSliderFrame trigger an async APZ scroll

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Benoit Girard (:BenWa)]

Loosely based on the approach of bug 1198966, mstange suggests that we should have the content thread initiate the start of an async scroll. The compositor could then take over the interaction.

This means that the content thread needs to response to start the interaction but afterwards it's entirely async. This has the benefit that we don't need to duplicate some of the nsSliderFrame (scrollbar) logic in the compositor. However I imagine from this patch we could incrementally make the scroll start async if we wanted to duplicate that code.

Comment 1

[Benoit Girard (:BenWa)]

Attachment: MozReview Request: Bug 1199885 - Part 1: Add MouseInput InputData. r=kats

Bug 1199885 - Let nsSliderFrame trigger an async APZ scroll

Comment 2

[Benoit Girard (:BenWa)]

Comment on attachment 8654469 [details]
MozReview Request: Bug 1199885 - Part 1: Add MouseInput InputData. r=kats

Bug 1199885 - Let nsSliderFrame trigger an async APZ scroll

Comment 3

[Benoit Girard (:BenWa)]

This works much better than bug 1198966 as expected. This version of the patch works, but there's work remaining:
- Properly calculate the scroll position (transform the event)
- Handle vertical scrolling
- Clamp the scrolling value

Comment 4

[Benoit Girard (:BenWa)]

Comment on attachment 8654469 [details]
MozReview Request: Bug 1199885 - Part 1: Add MouseInput InputData. r=kats

Bug 1199885 - Let nsSliderFrame trigger an async APZ scroll

Comment 5

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

https://reviewboard.mozilla.org/r/17707/#review15895

::: layout/xul/nsSliderFrame.cpp:925
(Diff revision 3)
> +    tabChild->SendStartScrollbarDrag(PresContext()->PresShell()->GetPresShellId(), scrollTargetId, 0, mDragStart / 60);

I think it would be better to route this through the widget rather than the tabchild directly. That way scrollbars in the parent process will also work the same way.

Comment 6

[Benoit Girard (:BenWa)]

Thanks for the early feedback. I'll do that before posting it for review. It might take a few days at least. I only handle the trivial case ATM.

Comment 7

[Benoit Girard (:BenWa)]

Attachment: js_scroll_hijack.html

Here's a simple testcase where JS will move your scroll position each 2 seconds.

Right now scroll bars don't take priority. Neither does it in Chrome/Safari. So I don't think we will implement giving the drag scroll priority.

Comment 8

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

Sounds good, that should keep the implementation simpler too.

Comment 9

[Benoit Girard (:BenWa)]

So I need to know the size of the scroll thumb to compute the new scroll position. I will need to notify the APZC instance each time a transaction comes to tell the controller what the size of the scroll thumbs are. :(

Comment 10

[Benoit Girard (:BenWa)]

Comment on attachment 8654469 [details]
MozReview Request: Bug 1199885 - Part 1: Add MouseInput InputData. r=kats

Bug 1199885 - Let nsSliderFrame trigger an async APZ scroll

Comment 11

[Benoit Girard (:BenWa)]

Mostly works now. The units conversion aren't 100% correct yet but almost. Need to test corner cases as well.

Comment 12

[Benoit Girard (:BenWa)]

Comment on attachment 8654469 [details]
MozReview Request: Bug 1199885 - Part 1: Add MouseInput InputData. r=kats

Bug 1199885 - Let nsSliderFrame trigger an async APZ scroll

Comment 13

[Benoit Girard (:BenWa)]

Here's a good working snapshot. Deals well with offsets, content scales, CSS transforms and windows scrollbars.

We decided we want to implemented input blocks.

Comment 14

[Benoit Girard (:BenWa)]

Comment on attachment 8654469 [details]
MozReview Request: Bug 1199885 - Part 1: Add MouseInput InputData. r=kats

Bug 1199885 - Let nsSliderFrame trigger an async APZ scroll. r?kats

Comment 15

[Benoit Girard (:BenWa)]

This is behind a proper flag. I think it's a good idea to get this into the tree while we decide how to deal with the input block ordering.

Comment 16

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

I gave the input block discussion we had some thought and I think we can make it work without too much trouble. Basically this is what I envision:

- We create a new input block type of "drag"
- A "drag" block is created either when we get a mousedown or a drag event [1]. It would be terminated by a mouseup or non-drag event. So for an input sequence like "mousedown, drag, wheel, drag, mouseup", we would get a drag block for the first "mousedown, drag", then a wheel block for the wheel event, and a second (independent) drag block for "drag, mouseup".
- This means that the main thread will need to send notifications to the APZ code not just on mousedown but also on drag events that start a new input block. This was the point at which we stopped our discussion - but handling this is actually pretty easy. The main thread gets an input block id with each event (see for example [2]) and any time that input block id changes we need to send a notification. So to handle this we just need add something like this at [3] and the equivalent TabChild mouse-handling code:

} else if (aEvent is mousedown || aEvent is drag) {
  static uint64_t lastBlockId = 0;
  if (lastBlockId != aInputBlockId) {
    lastBlockId = aInputBlockId;
    APZCCallbackHelper::SendSetTargetAPZCNotification(...)
    // whatever other notifications
  }
} ...

Does that sound reasonable? I looked over the shape of your patch quickly and most of it looks pretty good, although there seems to be a bunch of stuff like StartScrollbarDragTimeout that I don't think we need if we use the input blocks properly. I'd also like to see that patch split up into smaller patches since it's kind of hard to review. I'll do a pass over it anyway.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/APZCTreeManager.cpp?rev=3906b2dab080#881
[2] http://mxr.mozilla.org/mozilla-central/source/widget/nsBaseWidget.cpp?rev=3906b2dab080#967
[3] http://mxr.mozilla.org/mozilla-central/source/widget/nsBaseWidget.cpp?rev=3906b2dab080#1002

Comment 17

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

Comment on attachment 8654469 [details]
MozReview Request: Bug 1199885 - Part 1: Add MouseInput InputData. r=kats

https://reviewboard.mozilla.org/r/17707/#review16901

This is a really big patch and introduces a pretty large change. Thanks for taking it on! While I do want to see this go forward I feel like it's going to take a bunch more work and it's not really something we can just whip up in a day. Honestly if you have spare cycles to work on APZ stuff I'd rather see that effort go towards all the existing bugs we have with wheel scrolling before trying to introduce a whole new set of bugs with scrollbar dragging. I realize scrollbar dragging is just as valid a use case as wheel but looking at the size and complexity of this and the number of edge cases I can think of, I'd much rather wait on this until we have all the other major bugs hammered out.

If you want to continue working on this though at the very least I'd like to see this patch split up into a queue of smaller patches. I think the first few patches would be (1) adding the new class to InputData.h, (2) Updating the APZTreeManager and InputQueue to create proper drag blocks out of the relevant mouse events, even though the blocks will have no special behaviour (i.e. AsyncPanZoomController will just ignore them), (3) hook up the nsSliderFrame machinery so that it notifies APZTreeManager as whether or not the drag input block should be handled in APZ.

::: dom/ipc/PBrowser.ipdl:565
(Diff revision 6)
> +    async StartScrollbarDragTimeout(uint64_t aBlockId);

If we use input blocks properly I don't think we will need the StartScrollbarDragTimeout function at all

::: gfx/layers/apz/src/AsyncDragMetrics.h:11
(Diff revision 6)
> +#include "mozilla/TimeStamp.h"

I don't think you need this or the forward-declaration for TimeStamp (and certainly not both)

::: gfx/layers/apz/src/AsyncDragMetrics.h:34
(Diff revision 6)
> +  AsyncDragMetrics()

Not sure why you have this default constructor, it leaves stuff uninitialized and so I'd get rid of it if possible.

::: gfx/layers/apz/src/AsyncPanZoomController.h:394
(Diff revision 6)
> +  void ScheduleComposite();

Why did you move this declaration?

::: gfx/layers/apz/src/InputQueue.cpp:196
(Diff revision 6)
> +          prevBlock->ShareTarget(block);

This won't always work, because the old mouse block can get dropped from the queue after is done being processed. That's the main reason I think it is better to just start a new independent block with its own notifications from the main thread.

::: gfx/layers/moz.build:10
(Diff revision 6)
> +CXXFLAGS += ['-O0', '-g']

Drop this

::: widget/InputData.h:255
(Diff revision 6)
> +    MOUSE_MOVE,

We'll need a MOUSE_DRAG type as well

::: widget/InputData.h:285
(Diff revision 6)
> +  ScreenPoint mOrigin;

Will need a ParentLayerPoint mLocalOrigin for the untransformed coordinate

Comment 18

[Benoit Girard (:BenWa)]

https://reviewboard.mozilla.org/r/17707/#review16901

I knew it would be significantly longer when we stopped doing the first trivial approach. I agree it will take longer than a day, well over a week of work left in fact but I'm happy to do it. In the mean time I'd like to get this landed behind a preference. This touches a lot of the tree and will rot if it's not checked in. As far I know the approach is good so there shouldn't be any adverse effects from checking this in under a flag. I can understand not being in a rush to turn this on and have more bugs to deal with so that can wait. If your concern is review bandwidth then let me know.

Last I synced up with mstange he was building on a patch queue to address several issue. There wasn't really anything I could jump into effectively without contenting. If there's bugs I can help with now effectively let me know. I was also asked to look into painting performance and after Talos and fixing the DP perf issues this was the next low hanging fruit. While it's not simple, it's not that hard either and is a fairly big win.

I'm happy to gate turning this on against a high quality bar.

I can split out the parts if you'd like. The partition is mostly at a file level so I didn't think it was neccessary.

> Not sure why you have this default constructor, it leaves stuff uninitialized and so I'd get rid of it if possible.

It's for IPC. I though I left a comment in the code to point that out. I'll use friend and make it private.

> Why did you move this declaration?

I think I can clean this up now.

Comment 19

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

(In reply to Benoit Girard (:BenWa) from comment #18)
> I knew it would be significantly longer when we stopped doing the first
> trivial approach. I agree it will take longer than a day, well over a week
> of work left in fact but I'm happy to do it. In the mean time I'd like to
> get this landed behind a preference. This touches a lot of the tree and will
> rot if it's not checked in. As far I know the approach is good so there
> shouldn't be any adverse effects from checking this in under a flag. I can
> understand not being in a rush to turn this on and have more bugs to deal
> with so that can wait. If your concern is review bandwidth then let me know.

Ok, fair enough. Let's clean it up and get it landed. I would still like to bring up the quality of the patch somewhat, and avoid landing code that we will need to rip out later when we do proper input blocks - there's no point to landing that because it will just clutter the code and make other changes harder, and even if it rots it doesn't matter because we won't be needing it later. Are you ok with that?

> Last I synced up with mstange he was building on a patch queue to address
> several issue. There wasn't really anything I could jump into effectively
> without contenting. If there's bugs I can help with now effectively let me
> know.

Something like bug 1189565 would be a really great one to pick up since it's not tangled with anything else anybody is working on.

> I can split out the parts if you'd like. The partition is mostly at a file
> level so I didn't think it was neccessary.

Yes please. MozReview makes it really annoying to review a monolithic patch because jumping around between specific files is hard (I filed bug 1203969 for this as I was looking at your patch). Also if you split it up we can get stuff landed incrementally to prevent rot. Thanks!

Comment 20

[Benoit Girard (:BenWa)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #19)
> (In reply to Benoit Girard (:BenWa) from comment #18)
> > I knew it would be significantly longer when we stopped doing the first
> > trivial approach. I agree it will take longer than a day, well over a week
> > of work left in fact but I'm happy to do it. In the mean time I'd like to
> > get this landed behind a preference. This touches a lot of the tree and will
> > rot if it's not checked in. As far I know the approach is good so there
> > shouldn't be any adverse effects from checking this in under a flag. I can
> > understand not being in a rush to turn this on and have more bugs to deal
> > with so that can wait. If your concern is review bandwidth then let me know.
> 
> Ok, fair enough. Let's clean it up and get it landed. I would still like to
> bring up the quality of the patch somewhat, and avoid landing code that we
> will need to rip out later when we do proper input blocks - there's no point
> to landing that because it will just clutter the code and make other changes
> harder, and even if it rots it doesn't matter because we won't be needing it
> later. Are you ok with that?
> 

Yes absolutely. My intention here isn't to strong arm you to accept a bad patch under the excuse 'it's behind a preference'. Let's make sure we're happy with the patch.

I'd like to make sure everything about this patch, expect the input block handling, to be great. If you want I can remove that portion of the code entirely from the patch since that is tangled.

> > Last I synced up with mstange he was building on a patch queue to address
> > several issue. There wasn't really anything I could jump into effectively
> > without contenting. If there's bugs I can help with now effectively let me
> > know.
> 
> Something like bug 1189565 would be a really great one to pick up since it's
> not tangled with anything else anybody is working on.
> 

I'll take a look. I think this was one of the bugs I discussed with mstange but it was landing when I last looked.

> > I can split out the parts if you'd like. The partition is mostly at a file
> > level so I didn't think it was neccessary.
> 
> Yes please. MozReview makes it really annoying to review a monolithic patch
> because jumping around between specific files is hard (I filed bug 1203969
> for this as I was looking at your patch). Also if you split it up we can get
> stuff landed incrementally to prevent rot. Thanks!

Will do.

Comment 21

[Benoit Girard (:BenWa)]

Comment on attachment 8654469 [details]
MozReview Request: Bug 1199885 - Part 1: Add MouseInput InputData. r=kats

Bug 1199885 - Part 1: Add MouseInput InputData. r?kats

Comment 22

[Benoit Girard (:BenWa)]

Attachment: MozReview Request: Bug 1199885 - Part 2: Add AsyncDragMetrics. r=kats

Bug 1199885 - Part 2: Add AsyncDragMetrics. r?kats

Comment 23

[Benoit Girard (:BenWa)]

Attachment: MozReview Request: Bug 1199885 - Part 3: Add 'apz.drag.enabled' preference for async scrollbars. r=kats

Bug 1199885 - Part 3: Add 'apz.drag.enabled' preference for async scrollbars. r?kats

Comment 24

[Benoit Girard (:BenWa)]

Attachment: MozReview Request: Bug 1199885 - Part 4: Let the hit testing tree know about the scroll thumb. r=kats

Bug 1199885 - Part 4: Let the hit testing tree know about the scroll thumb. r?kats

Comment 25

[Benoit Girard (:BenWa)]

Attachment: MozReview Request: Bug 1199885 - Part 5: Make mRootLayerTreeID const to prove that there's no data races. r?kats

Bug 1199885 - Part 5: Let nsSliderFrame trigger async scrolling via StartScrollbarDrag. r?kats

Comment 26

[Benoit Girard (:BenWa)]

Alright here's a proper patch queue. I could split part 5 more but IMO this block really needs to be reviewed together.

Note that I didn't restore my previous intermediate 'works but gets input blocks wrong' state, now this patch doesn't work at all but it will all be behind a preference. We've got a plan for input blocks so I'd rather just work on that.

So ideally we should be fully satisfied with this queue, minus it not dealing with input blocks.

Comment 27

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

Comment on attachment 8654469 [details]
MozReview Request: Bug 1199885 - Part 1: Add MouseInput InputData. r=kats

https://reviewboard.mozilla.org/r/17707/#review16931

::: widget/InputData.h:254
(Diff revision 7)
> +  enum MouseType {

nit: brace on new line

::: widget/InputData.cpp:63
(Diff revision 7)
> +      mType = MOUSE_DRAG_START;

missing break;

::: widget/InputData.cpp:64
(Diff revision 7)
> +    case eDragOver:

From my reading of https://developer.mozilla.org/en-US/docs/Web/Guide/HTML/Drag_and_drop#events I think you want eDragEnd instead of eDragOver, and you want to map eDrag to something like MOUSE_DRAG.

::: widget/InputData.cpp:68
(Diff revision 7)
> +      MOZ_ASSERT_UNREACHABLE("Mouse event type not supported");

Crashing here seems not that great, but I guess it depends on what we do at the call sites of this function. Once I look at the rest of the queue I might come back to this.

Comment 28

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

Comment on attachment 8660105 [details]
MozReview Request: Bug 1199885 - Part 2: Add AsyncDragMetrics. r=kats

https://reviewboard.mozilla.org/r/19063/#review16933

Comment 29

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

Comment on attachment 8660106 [details]
MozReview Request: Bug 1199885 - Part 3: Add 'apz.drag.enabled' preference for async scrollbars. r=kats

https://reviewboard.mozilla.org/r/19065/#review16935

::: toolkit/locales/en-US/chrome/global/aboutSupport.properties:107
(Diff revision 1)
> +dragEnabled = drag input enabled

Maybe "scrollbar drag enabled" would be more meaningful since this is a user-visible string?

Comment 30

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

Comment on attachment 8660107 [details]
MozReview Request: Bug 1199885 - Part 4: Let the hit testing tree know about the scroll thumb. r=kats

https://reviewboard.mozilla.org/r/19067/#review16937

::: gfx/layers/apz/src/HitTestingTreeNode.h:16
(Diff revision 1)
> +#include "Layers.h"

Can we get away with forward-declarations instead of including Layers.h? It's just such a big header to be including here :/

If we do need to include it, please move it up to between FrameMetrics.h and Matrix.h so that it's alphabetical.

::: gfx/layers/apz/src/HitTestingTreeNode.h:95
(Diff revision 1)
> +  FrameMetrics::ViewID GetScrollViewId() const { return mScrollViewId; }

In general I prefer if the body of even simple getters are in the .cpp file, because it's easier to set breakpoints and inserting printfs doesn't cause a massive recompile.

In this case I would also get rid of the GetScrollViewId and GetScrollDirection getters entirely since they don't need to be exposed and break encapsulation. Instead add a |bool MatchesScrollDragMetrics(const AsyncDragMetrics& aDragMetrics) const;| which you can call from FindScrollNode(...)

The GetScrollSize() can stay, but move the body into the .cpp file.

Comment 31

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

Comment on attachment 8660108 [details]
MozReview Request: Bug 1199885 - Part 5: Make mRootLayerTreeID const to prove that there's no data races. r?kats

https://reviewboard.mozilla.org/r/19073/#review16941

I have to head out soon - couple of comments below but if you're splitting this into smaller patches I'd rather wait for those. If you end up not doing that just re-flag me for review on the bugzilla attachment and I'll pick this up on Monday.

::: layout/base/Units.h:179
(Diff revision 1)
> +  static CSSIntRect FromAppUnits(const nsIntRect& aRect) {

nsIntRect is usually not in app units, it's usually LayoutDevicePixel units. Please remove this and see my other comment at the call site.

::: layout/xul/nsSliderFrame.cpp:911
(Diff revision 1)
> +  nsIntRect sliderTrack(GetRect().x - scrollbarBox->GetRect().x,

This should be an nsRect. In fact I would rewrite this as nsRect sliderTrack = GetRect() - scrollbarBox.GetPosition();

Comment 32

[Benoit Girard (:BenWa)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #30)
> Comment on attachment 8660107 [details]
> MozReview Request: Bug 1199885 - Part 4: Let the hit testing tree know about
> the scroll thumb. r?kats
> 
> https://reviewboard.mozilla.org/r/19067/#review16937
> 
> ::: gfx/layers/apz/src/HitTestingTreeNode.h:16
> (Diff revision 1)
> > +#include "Layers.h"
> 
> Can we get away with forward-declarations instead of including Layers.h?
> It's just such a big header to be including here :/
> 
> If we do need to include it, please move it up to between FrameMetrics.h and
> Matrix.h so that it's alphabetical.

It's not ideal I agree but we can't forward declare ScrollDirection otherwise. We'd have to move ScrollDirection outside of Layers.h but I don't think it's worth paying that cost just to minimize includes.

Comment 33

[Benoit Girard (:BenWa)]

Comment on attachment 8654469 [details]
MozReview Request: Bug 1199885 - Part 1: Add MouseInput InputData. r=kats

Bug 1199885 - Part 1: Add MouseInput InputData. r?kats

Comment 34

[Benoit Girard (:BenWa)]

Comment on attachment 8660105 [details]
MozReview Request: Bug 1199885 - Part 2: Add AsyncDragMetrics. r=kats

Bug 1199885 - Part 2: Add AsyncDragMetrics. r=kats

Comment 35

[Benoit Girard (:BenWa)]

Comment on attachment 8660106 [details]
MozReview Request: Bug 1199885 - Part 3: Add 'apz.drag.enabled' preference for async scrollbars. r=kats

Bug 1199885 - Part 3: Add 'apz.drag.enabled' preference for async scrollbars. r=kats

Comment 36

[Benoit Girard (:BenWa)]

Comment on attachment 8660107 [details]
MozReview Request: Bug 1199885 - Part 4: Let the hit testing tree know about the scroll thumb. r=kats

Bug 1199885 - Part 4: Let the hit testing tree know about the scroll thumb. r?kats

Comment 37

[Benoit Girard (:BenWa)]

Comment on attachment 8660108 [details]
MozReview Request: Bug 1199885 - Part 5: Make mRootLayerTreeID const to prove that there's no data races. r?kats

Bug 1199885 - Part 5: Make mRootLayerTreeID const to prove that there's no data races. r?kats

Comment 38

[Benoit Girard (:BenWa)]

Attachment: MozReview Request: Bug 1199885 - Part 8: Add FindScrollNode to locate the scrollbar thumb. r?kats

Bug 1199885 - Part 6: Let events know which input blocks to belong to confirm them. r?kats

Comment 39

[Benoit Girard (:BenWa)]

Attachment: MozReview Request: Bug 1199885 - Part 9: Let APZC handle the drag events. r=kats

Bug 1199885 - Part 7: Add ScrollTo to FrameMetrics for absolute scrolling. r?kats

Comment 40

[Benoit Girard (:BenWa)]

Attachment: MozReview Request: Bug 1199885 - Part 9.5: Make the mouse events APZC aware. r?kats

Bug 1199885 - Part 8: Add FindScrollNode to locate the scrollbar thumb. r?kats

Comment 41

[Benoit Girard (:BenWa)]

Attachment: MozReview Request: Bug 1199885 - Part 10: Add APZTeeManager API to start an async scroll. r?kats

Bug 1199885 - Part 9: Let APZC handle the drag events. r?kats

Comment 42

[Benoit Girard (:BenWa)]

Attachment: MozReview Request: Bug 1199885 - Part 12: Add StartScrollbarDrag IPC message. r?kats

Bug 1199885 - Part 10: Add APZTeeManager API to start an async scroll. r?kats

Comment 43

[Benoit Girard (:BenWa)]

Attachment: MozReview Request: Bug 1199885 - Part 13: Let nsSliderFrame trigger async scrolling via StartScrollbarDrag. r?kats

Bug 1199885 - Part 11: Let APZCCallbackHelper dispatch StartScrollbarDrag to the APZ controller thread. r?kats

Comment 44

[Benoit Girard (:BenWa)]

Attachment: MozReview Request: Bug 1199885 - Part 12: Add StartScrollbarDrag IPC message. r?kats

Bug 1199885 - Part 12: Add StartScrollbarDrag IPC message. r?kats

Comment 45

[Benoit Girard (:BenWa)]

Attachment: MozReview Request: Bug 1199885 - Part 13: Let nsSliderFrame trigger async scrolling via StartScrollbarDrag. r?kats

Bug 1199885 - Part 13: Let nsSliderFrame trigger async scrolling via StartScrollbarDrag. r?kats

Comment 46

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

Comment on attachment 8654469 [details]
MozReview Request: Bug 1199885 - Part 1: Add MouseInput InputData. r=kats

https://reviewboard.mozilla.org/r/17707/#review17063

Comment 47

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

https://reviewboard.mozilla.org/r/19065/#review17065

::: toolkit/locales/en-US/chrome/global/aboutSupport.properties:107
(Diff revisions 1 - 2)
> -dragEnabled = drag input enabled
> +dragEnabled = scroolbar drag enabled

typo: s/scroolbar/scrollbar/

Comment 48

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

Comment on attachment 8660107 [details]
MozReview Request: Bug 1199885 - Part 4: Let the hit testing tree know about the scroll thumb. r=kats

https://reviewboard.mozilla.org/r/19067/#review17067

::: gfx/layers/apz/src/HitTestingTreeNode.h:16
(Diff revision 2)
> +#include "Layers.h"

Please move this #include up to between FrameMetrics.h and Matrix.h to keep it alphabetical

Comment 49

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

Comment on attachment 8660108 [details]
MozReview Request: Bug 1199885 - Part 5: Make mRootLayerTreeID const to prove that there's no data races. r?kats

https://reviewboard.mozilla.org/r/19073/#review17069

Comment 50

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

Comment on attachment 8660859 [details]
MozReview Request: Bug 1199885 - Part 9: Let APZC handle the drag events. r=kats

https://reviewboard.mozilla.org/r/19169/#review17071

::: gfx/layers/FrameMetrics.h:201
(Diff revision 1)
> +  void ScrollTo(const CSSPoint& aPoint)

We already have SetScrollOffset which does the exact same thing. Either use that or file a follow-up to remove that one and convert all the uses to ScrollTo

Comment 51

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

Comment on attachment 8660860 [details]
MozReview Request: Bug 1199885 - Part 9.5: Make the mouse events APZC aware. r?kats

https://reviewboard.mozilla.org/r/19171/#review17073

::: gfx/layers/apz/src/APZCTreeManager.h:303
(Diff revision 1)
> +  HitTestingTreeNode* FindScrollNode(const AsyncDragMetrics& aDragMetrics);

This can't be made public as-is. Either it needs to return a nsRefPtr<HitTestingTreeNode>, and the implementation has to acquire the tree lock, or you have to make it private.

::: gfx/layers/apz/src/APZCTreeManager.cpp:1513
(Diff revision 1)
> +    if (((node->GetScrollDirection() == Layer::HORIZONTAL && aScrollDir == AsyncDragMetrics::HORIZONTAL) ||

This doesn't look like it's been rebased properly, need to use node->MatchesScrollDragMetrics instead. It will probably also change the signature of this function since you need to pass in the AsyncDragMetrics object itself.

Comment 52

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

Comment on attachment 8660858 [details]
MozReview Request: Bug 1199885 - Part 8: Add FindScrollNode to locate the scrollbar thumb. r?kats

https://reviewboard.mozilla.org/r/19167/#review17077

This adds an extra field to every single WidgetEvent when you only really need it in a small subset of cases. Instead of doing this, you should:

(1) route mouse events through TabParent/TabChild in the same way that mousewheel events are routed. Specifically, in TabParent::SendRealMouseEvent call ApzAwareEventRoutingToChild to get the guid/blockId and send that to the child process (see how it's done in SendMouseWheelEvent). Calling ApzAwareEventRoutingToChild is something you'll need to do in the future for this mouse events anyway, so might as well do it now as it helps solve this problem.

(2) Then in TabChild::RecvRealMouseButtonEvent, which actually dispatches the event to content (and which will invoke nsSliderFrame::HandleEvent), create another InputAPZContext object on the stack (before the call APZCCallbackHelper::DispatchWidgetEvent). InputAPZContext is a RAII helper which will provide access to the input block, so the code in nsSliderFrame::HandleEvent can access it via InputAPZContext::GetInputBlockId().

Doing these two things will allow the nsSliderFrame to know the input block id of the current mouse event, regardless of whether the code is in the child or parent process. (For the parent process, the InputAPZContext on the stack will be the one created in nsBaseWidget::ProcessUntransformedAPZEvent). And you won't need to add the extra field to WidgetEvent.

Comment 53

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

Comment on attachment 8660865 [details]
MozReview Request: Bug 1199885 - Part 13: Let nsSliderFrame trigger async scrolling via StartScrollbarDrag. r?kats

https://reviewboard.mozilla.org/r/19181/#review17085

::: layout/xul/nsSliderFrame.cpp:899
(Diff revision 1)
> +bool

unused return value, did you intend to use that somehow?

::: layout/xul/nsSliderFrame.cpp:914
(Diff revision 1)
> +  nsIContent* scrollableContent = GetScrollbar()->GetParent()->GetContent();

Can GetScrollbar()->GetParent() return null here? I assume yes; you may want to null-check for that also.

::: layout/xul/nsSliderFrame.cpp:920
(Diff revision 1)
> +  bool hasAPZView = (scrollTargetId != layers::FrameMetrics::NULL_SCROLL_ID);

hasAPZView will always be true, because FindOrCreateIDFor will never return NULL_SCROLL_ID. I think what you meant to do was use FindIDFor instead of FindOrCreateIDFor

Comment 54

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

Comment on attachment 8660864 [details]
MozReview Request: Bug 1199885 - Part 12: Add StartScrollbarDrag IPC message. r?kats

https://reviewboard.mozilla.org/r/19179/#review17087

::: dom/ipc/TabParent.cpp:2954
(Diff revision 1)
> +TabParent::RecvStartScrollbarDrag(const AsyncDragMetrics& aDragMetrics)

Move the meat of this function into RenderFrameParent so you don't need to expose the APZCTreeManager as public.

Also this function is busted because it does the nullcheck for GetRenderFrame() after it calls GetApzcTreeManager on it...

This function should just look like:

if (RenderFrameParent* rfp = GetRenderFrame()) {
  rfp->StartScrollbarDrag(aDragMetrics);
}
return true;

::: widget/nsBaseWidget.cpp:1683
(Diff revision 1)
> +nsBaseWidget::StartAsyncScrollbarDrag(const AsyncDragMetrics& aDragMetrics)

Add a |if (!AsyncPanZoomEnabled()) return| in here somewhere (preferably right at the top of the function). It's possible to have widgets which don't have an APZ instance even if the global APZ pref is enabled (non-e10s widgets for example). In those cases mAPZC will be null.

Comment 55

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

Comment on attachment 8660863 [details]
MozReview Request: Bug 1199885 - Part 13: Let nsSliderFrame trigger async scrolling via StartScrollbarDrag. r?kats

https://reviewboard.mozilla.org/r/19177/#review17089

::: gfx/layers/apz/util/APZCCallbackHelper.cpp:847
(Diff revision 1)
> +class StartScrollbarDragTask : public Task

You don't need this task at all. You can inline this trivially into the two call sites that call APZCCallbackHelper::StartScrollbarDrag:

APZThreadUtils::RunOnControllerThread(NewRunnableMethod(apzctm, &APZCTreeManager::StartScrollbarDrag, aGuid, aDragMetrics));

Comment 56

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

Comment on attachment 8660862 [details]
MozReview Request: Bug 1199885 - Part 12: Add StartScrollbarDrag IPC message. r?kats

https://reviewboard.mozilla.org/r/19175/#review17095

::: gfx/layers/apz/src/APZCTreeManager.h:544
(Diff revision 1)
> +  AsyncDragMetrics mDragMetrics;

These two fields aren't used yet so we can take them out for now.

::: gfx/layers/apz/src/APZCTreeManager.cpp:330
(Diff revision 1)
> +  // TODO Confirm the input block

FWIW yes, this is pretty much what I had in mind for the architecture

::: gfx/layers/apz/src/APZCTreeManager.cpp:658
(Diff revision 1)
> +          mRootNode->GetApzc(),

If you hard-code the root node's APZC here, it won't work for subframes. (Or worse, dragging a subframe's scrollbar will end up scrolling the root document). You should call GetTargetAPZC to get an APZC, much the way APZCTreeManager::ProcessEvent does it. But see my next comment.

::: gfx/layers/apz/src/APZCTreeManager.cpp:662
(Diff revision 1)
> +      break;

Hmm. You'll need apply an APZC's untransform to mouseInput.mOrigin after this call to ReceiveInputEvent, so that when gecko sees this event it has the right coordinates. However dragging on a scrollbar probably behaves differently from the other input types in that the gecko coordinates are the same as the screen coordinates even though the async transform on the APZC is changing. I'm not really sure at this point and it'll take some though to figure out properly. For now this code won't get hit unless the pref is enabled, so we can just stick in a TODO and leave it as-is. (This goes for the previous comment as well)

::: gfx/layers/apz/src/APZCTreeManager.cpp:984
(Diff revision 1)
> +  uint64_t outInputBlockId;

If you get rid of the inputBlockId member on WidgetEvent, you can get rid of this also, and just pass aOutInputBlockId directly to ReceiveInputEvent.

::: gfx/layers/apz/src/APZCTreeManager.cpp:986
(Diff revision 1)
> +  // REVIEW: aEvent.refPoint type safety?

Assigning the refPoint to mOrigin here is (I believe) correct, although perhaps it would be cleaner to do it as part of the MouseInput constructor.

However, *after* the call to ReceiveInputEvent, you need to overwrite aEvent.refPoint with the input.mOrigin value, because ReceiveInputEvent is supposed to do an in-place modification on that. That isn't happening yet (see my previous comments) but I'd like to see the code here updated to assume it is happening, so that we don't forget to do it later. See ProcessWheelEvent for an example of how.

Comment 57

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

Comment on attachment 8660861 [details]
MozReview Request: Bug 1199885 - Part 10: Add APZTeeManager API to start an async scroll. r?kats

https://reviewboard.mozilla.org/r/19173/#review17083

This code is mostly not invoked at the moment so I didn't go over it detail. We can land it as-is for now and revisit it later.

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:68
(Diff revision 1)
> +#include "HitTestingTreeNode.h"

Move up to alphabetize

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:993
(Diff revision 1)
> +  ParentLayerPoint geckoPoint = TransformTo<ParentLayerPixel>(transformToGecko, point);

So what you're doing here will give you the right math but is labelled wrong. GetScreenToApzcTransform(this) will return a screen-to-apzc transform, so calling it transformToGecko is incorrect. Calling this variable "geckoPoint" is also incorrect. For other input events we have a TransformToLocal function on the input object (see for example ScrollWheelInput::TransformToLocal) which does this conversion from screen space to APZC space and puts it in the "local" field of the input event. We can follow the same pattern here.

Comment 58

[Benoit Girard (:BenWa)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #56)
> ::: gfx/layers/apz/src/APZCTreeManager.cpp:658
> (Diff revision 1)
> > +          mRootNode->GetApzc(),
> 
> If you hard-code the root node's APZC here, it won't work for subframes. (Or
> worse, dragging a subframe's scrollbar will end up scrolling the root
> document). You should call GetTargetAPZC to get an APZC, much the way
> APZCTreeManager::ProcessEvent does it. But see my next comment.
> 

This part here is not clear. If I call GetTargetAPZC I'll get an approximate APZC for the event which may not be the one that the content confirms.

It feels like I should leave the APZC as null until the content can confirm but this would require some refactoring of ReceiveInputEvent. This is what I had tried first but I didn't want to start refactoring this on a hunch and I was not sure how deep the assumption that an input event always has an APZC.

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #53)
> ::: layout/xul/nsSliderFrame.cpp:899
> (Diff revision 1)
> > +bool
> 
> unused return value, did you intend to use that somehow?

I'm not using it but it felt useful to include. I did use it a bit for testing. I can remove it if we don't want it but I didn't think there was any harm in leaving it.

Comment 59

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

(In reply to Benoit Girard (:BenWa) from comment #58)
> This part here is not clear. If I call GetTargetAPZC I'll get an approximate
> APZC for the event which may not be the one that the content confirms.

That's correct, and that should be fine.

> It feels like I should leave the APZC as null until the content can confirm
> but this would require some refactoring of ReceiveInputEvent. This is what I
> had tried first but I didn't want to start refactoring this on a hunch and I
> was not sure how deep the assumption that an input event always has an APZC.

The assumption that an input block has a non-null APZC on creation is pretty deeply baked in. The APZC can be changed to null later if content says so.

> (In reply to Kartikaya Gupta (email:[e-mail]) from comment #53)
> > ::: layout/xul/nsSliderFrame.cpp:899
> > (Diff revision 1)
> > > +bool
> > 
> > unused return value, did you intend to use that somehow?
> 
> I'm not using it but it felt useful to include. I did use it a bit for
> testing. I can remove it if we don't want it but I didn't think there was
> any harm in leaving it.

Ok, that's fine.

Comment 60

[Benoit Girard (:BenWa)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #59)
> (In reply to Benoit Girard (:BenWa) from comment #58)
> > This part here is not clear. If I call GetTargetAPZC I'll get an approximate
> > APZC for the event which may not be the one that the content confirms.
> 
> That's correct, and that should be fine.
> 

Then how good does the 'approximate guess' have to be then? Always picking mRootNode is an approximate guess for some very weak accuracy / high precision criterion. What property does GetTargetAPZC provide that we need? Or will it be incorrect sometimes too if the wrong subframe is picked?

Comment 61

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

The guess is used to compute the async untransform for the input event, so that it gets converted from screen space into the layout-device space that Gecko is expecting. It's true that in the cases we guess wrong this untransform will be wrong but in practice the guess is "good enough" that errors are not noticeable. If the guess is less accurate, such as with always using the root node, the errors may become more noticeable.

Comment 62

[Benoit Girard (:BenWa)]

Ok thanks!

Comment 63

[Benoit Girard (:BenWa)]

Comment on attachment 8654469 [details]
MozReview Request: Bug 1199885 - Part 1: Add MouseInput InputData. r=kats

Bug 1199885 - Part 1: Add MouseInput InputData. r=kats

Comment 64

[Benoit Girard (:BenWa)]

Comment on attachment 8660105 [details]
MozReview Request: Bug 1199885 - Part 2: Add AsyncDragMetrics. r=kats

Bug 1199885 - Part 2: Add AsyncDragMetrics. r=kats

Comment 65

[Benoit Girard (:BenWa)]

Comment on attachment 8660106 [details]
MozReview Request: Bug 1199885 - Part 3: Add 'apz.drag.enabled' preference for async scrollbars. r=kats

Bug 1199885 - Part 3: Add 'apz.drag.enabled' preference for async scrollbars. r=kats

Comment 66

[Benoit Girard (:BenWa)]

Comment on attachment 8660107 [details]
MozReview Request: Bug 1199885 - Part 4: Let the hit testing tree know about the scroll thumb. r=kats

Bug 1199885 - Part 4: Let the hit testing tree know about the scroll thumb. r=kats

Comment 67

[Benoit Girard (:BenWa)]

Comment on attachment 8660108 [details]
MozReview Request: Bug 1199885 - Part 5: Make mRootLayerTreeID const to prove that there's no data races. r?kats

Bug 1199885 - Part 5: Make mRootLayerTreeID const to prove that there's no data races. r?kats

Comment 68

[Benoit Girard (:BenWa)]

Comment on attachment 8660858 [details]
MozReview Request: Bug 1199885 - Part 8: Add FindScrollNode to locate the scrollbar thumb. r?kats

Bug 1199885 - Part 8: Add FindScrollNode to locate the scrollbar thumb. r?kats

Comment 69

[Benoit Girard (:BenWa)]

Comment on attachment 8660859 [details]
MozReview Request: Bug 1199885 - Part 9: Let APZC handle the drag events. r=kats

Bug 1199885 - Part 9: Let APZC handle the drag events. r=kats

Comment 70

[Benoit Girard (:BenWa)]

Comment on attachment 8660860 [details]
MozReview Request: Bug 1199885 - Part 9.5: Make the mouse events APZC aware. r?kats

Bug 1199885 - Part 9.5: Make the mouse events APZC aware. r?kats

Comment 71

[Benoit Girard (:BenWa)]

Comment on attachment 8660861 [details]
MozReview Request: Bug 1199885 - Part 10: Add APZTeeManager API to start an async scroll. r?kats

Bug 1199885 - Part 10: Add APZTeeManager API to start an async scroll. r?kats

Comment 72

[Benoit Girard (:BenWa)]

Comment on attachment 8660862 [details]
MozReview Request: Bug 1199885 - Part 12: Add StartScrollbarDrag IPC message. r?kats

Bug 1199885 - Part 12: Add StartScrollbarDrag IPC message. r?kats

Comment 73

[Benoit Girard (:BenWa)]

Comment on attachment 8660863 [details]
MozReview Request: Bug 1199885 - Part 13: Let nsSliderFrame trigger async scrolling via StartScrollbarDrag. r?kats

Bug 1199885 - Part 13: Let nsSliderFrame trigger async scrolling via StartScrollbarDrag. r?kats

Comment 74

[Benoit Girard (:BenWa)]

Part 6 and 7 has been deleted, part 9.5 has been introduced between 9 and 10.

Comment 75

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

Comment on attachment 8660858 [details]
MozReview Request: Bug 1199885 - Part 8: Add FindScrollNode to locate the scrollbar thumb. r?kats

https://reviewboard.mozilla.org/r/19167/#review17837

::: gfx/layers/apz/src/APZCTreeManager.h:19
(Diff revision 2)
> +#include "mozilla/layers/AsyncDragMetrics.h" // for AsyncDragMetrics

Shouldn't need this include here

Comment 76

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

Comment on attachment 8660859 [details]
MozReview Request: Bug 1199885 - Part 9: Let APZC handle the drag events. r=kats

https://reviewboard.mozilla.org/r/19169/#review17839

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:1010
(Diff revision 2)
> +                            GetFrameMetrics().GetViewport().height;

GetViewport() is the wrong thing to be using here, that's the CSS viewport. The composition bounds is what you want. Add a TODO for it.

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:1013
(Diff revision 2)
> +  scrollPosition = std::max(0.f, scrollPosition);

Instead of 0 we should be using the GetScrollableRect()'s left/top value. For RTL pages the leftmost scroll position may be nonzero. Add a TODO for this.

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:1016
(Diff revision 2)
> +  mFrameMetrics.SetScrollOffset(CSSPoint(0, scrollPosition));

Half of this code is axis-independent and the other half assumes the y-axis. Please add a TODO so that this is corrected before we start using this code.

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:1077
(Diff revision 2)
> +    if (!scrollInput.TransformToLocal(aTransformToApzc)) { 

nit: trailing whitespace

Comment 77

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

Comment on attachment 8660860 [details]
MozReview Request: Bug 1199885 - Part 9.5: Make the mouse events APZC aware. r?kats

https://reviewboard.mozilla.org/r/19171/#review17841

::: dom/ipc/TabParent.cpp:1462
(Diff revision 2)
> +  event.refPoint += GetChildProcessOffset();

Remove this line; it is already done earlier in the function and doing it twice will result in incorrect mouse events.

::: dom/ipc/TabParent.cpp:3556
(Diff revision 2)
> +

remove spurious blank lines

Comment 78

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

Comment on attachment 8660862 [details]
MozReview Request: Bug 1199885 - Part 12: Add StartScrollbarDrag IPC message. r?kats

https://reviewboard.mozilla.org/r/19175/#review17843

::: dom/ipc/TabParent.cpp:2961
(Diff revision 2)
> +      rfp->StartScrollbarDrag(aDragMetrics);

2-space indent

::: layout/ipc/RenderFrameParent.h:108
(Diff revision 2)
> +  layers::APZCTreeManager* GetApzcTreeManager();

Move this back to where it was in the protected section

::: layout/ipc/RenderFrameParent.cpp:592
(Diff revision 2)
> +  layers::APZCTreeManager* apzctm = GetApzcTreeManager();

GetApzcTreeManager() could return null, please null check like all the other functions

Comment 79

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

Comment on attachment 8660863 [details]
MozReview Request: Bug 1199885 - Part 13: Let nsSliderFrame trigger async scrolling via StartScrollbarDrag. r?kats

https://reviewboard.mozilla.org/r/19177/#review17845

::: layout/xul/nsSliderFrame.cpp:910
(Diff revision 2)
> +  scrollbar = GetContentOfBox(scrollbarBox);

combine this assignment into the declaration on the previous line

::: layout/xul/nsSliderFrame.cpp:913
(Diff revision 2)
> +  nsRect sliderTrack = GetRect() - scrollbarBox->GetPosition();

Move these declarations down to after the early returns, no point computing these if we're not going to use them.

::: layout/xul/nsSliderFrame.cpp:929
(Diff revision 2)
> +  uint64_t inputblockId = InputAPZContext::GetInputBlockId();

Move this down to past the early return as well.

::: layout/xul/nsSliderFrame.cpp:937
(Diff revision 2)
> +                               NSAppUnitsToIntPixels(mDragStart,

This makes me think the AsyncDragMetrics should store this value as a CSSIntCoord rather than an int32_t, so that we have it strongly typed. Stick in a TODO for it or something.

Comment 80

[Benoit Girard (:BenWa)]

https://reviewboard.mozilla.org/r/19169/#review17839

> GetViewport() is the wrong thing to be using here, that's the CSS viewport. The composition bounds is what you want. Add a TODO for it.

fixed instead of adding a todo.

> Instead of 0 we should be using the GetScrollableRect()'s left/top value. For RTL pages the leftmost scroll position may be nonzero. Add a TODO for this.

fixed instead

> Half of this code is axis-independent and the other half assumes the y-axis. Please add a TODO so that this is corrected before we start using this code.

fixed instead

Comment 81

[Benoit Girard (:BenWa)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=12052ff8fb29

Comment 82

[Benoit Girard (:BenWa)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bee96d724c8f

Comment 83

[Benoit Girard (:BenWa)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e7949ba0976

Comment 84

[Benoit Girard (:BenWa)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=360eca98aa80

Comment 85

[Benoit Girard (:BenWa)]

Comment on attachment 8654469 [details]
MozReview Request: Bug 1199885 - Part 1: Add MouseInput InputData. r=kats

Bug 1199885 - Part 1: Add MouseInput InputData. r=kats

Comment 86

[Benoit Girard (:BenWa)]

Comment on attachment 8660105 [details]
MozReview Request: Bug 1199885 - Part 2: Add AsyncDragMetrics. r=kats

Bug 1199885 - Part 2: Add AsyncDragMetrics. r=kats

Comment 87

[Benoit Girard (:BenWa)]

Comment on attachment 8660106 [details]
MozReview Request: Bug 1199885 - Part 3: Add 'apz.drag.enabled' preference for async scrollbars. r=kats

Bug 1199885 - Part 3: Add 'apz.drag.enabled' preference for async scrollbars. r=kats

Comment 88

[Benoit Girard (:BenWa)]

Comment on attachment 8660107 [details]
MozReview Request: Bug 1199885 - Part 4: Let the hit testing tree know about the scroll thumb. r=kats

Bug 1199885 - Part 4: Let the hit testing tree know about the scroll thumb. r=kats

Comment 89

[Benoit Girard (:BenWa)]

Comment on attachment 8660108 [details]
MozReview Request: Bug 1199885 - Part 5: Make mRootLayerTreeID const to prove that there's no data races. r?kats

Bug 1199885 - Part 5: Make mRootLayerTreeID const to prove that there's no data races. r?kats

Comment 90

[Benoit Girard (:BenWa)]

Comment on attachment 8660858 [details]
MozReview Request: Bug 1199885 - Part 8: Add FindScrollNode to locate the scrollbar thumb. r?kats

Bug 1199885 - Part 8: Add FindScrollNode to locate the scrollbar thumb. r?kats

Comment 91

[Benoit Girard (:BenWa)]

Comment on attachment 8660859 [details]
MozReview Request: Bug 1199885 - Part 9: Let APZC handle the drag events. r=kats

Bug 1199885 - Part 9: Let APZC handle the drag events. r=kats

Comment 92

[Benoit Girard (:BenWa)]

Comment on attachment 8660860 [details]
MozReview Request: Bug 1199885 - Part 9.5: Make the mouse events APZC aware. r?kats

Bug 1199885 - Part 9.5: Make the mouse events APZC aware. r?kats

Comment 93

[Benoit Girard (:BenWa)]

Comment on attachment 8660861 [details]
MozReview Request: Bug 1199885 - Part 10: Add APZTeeManager API to start an async scroll. r?kats

Bug 1199885 - Part 10: Add APZTeeManager API to start an async scroll. r?kats

Comment 94

[Benoit Girard (:BenWa)]

Comment on attachment 8660862 [details]
MozReview Request: Bug 1199885 - Part 12: Add StartScrollbarDrag IPC message. r?kats

Bug 1199885 - Part 12: Add StartScrollbarDrag IPC message. r?kats

Comment 95

[Benoit Girard (:BenWa)]

Comment on attachment 8660863 [details]
MozReview Request: Bug 1199885 - Part 13: Let nsSliderFrame trigger async scrolling via StartScrollbarDrag. r?kats

Bug 1199885 - Part 13: Let nsSliderFrame trigger async scrolling via StartScrollbarDrag. r?kats

Comment 96

[Benoit Girard (:BenWa)]

Attachment: MozReview Request: Bug 1199885 - Part 14: Add input blocks for mouse events. r?kats

Bug 1199885 - Part 14: Add input blocks for mouse events. r?kats

Comment 97

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

https://reviewboard.mozilla.org/r/19169/#review18985

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:1001
(Diff revision 3)
> +  CSSPoint scrollFramePoint = aEvent.mLocalOrigin / GetFrameMetrics().GetZoom();

This function calls GetFrameMetrics() a lot, would be good to cache it locally, or to just use mFrameMetrics directly.

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:1005
(Diff revision 3)
> +  CSSRect cssCompositionBound = GetFrameMetrics().GetCompositionBounds() / GetFrameMetrics().GetZoom();

You can use GetFrameMetrics().CalculateCompositedRectInCssPixels()

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:1031
(Diff revision 3)
> +    scrollOffset = CSSPoint(scrollPosition, 0);

Using 0 here I think means that if you're halfway down a page and then start scrolling horizontally, you'll snap back up to the top. That's not desirable, you should use the existing scroll offset's y coord.

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:1039
(Diff revision 3)
> +  // Here we consume the events. This means that the content scrollbars

This comment is wrong; the return value from this function is eventually dropped and has no bearing on whether the content scrollbars see this event or not.

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:1095
(Diff revision 3)
> +    ScrollWheelInput scrollInput = aEvent.AsScrollWheelInput();

This is wrong, it's converting MOUSE_INPUT types to ScrollWheelInput. Copy/paste error most likely. Since this hunk is removed in part 14 anyway, you can just take it out in this patch instead.

Comment 98

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

https://reviewboard.mozilla.org/r/19171/#review18987

::: dom/ipc/TabChild.cpp:1940
(Diff revision 3)
> +  nsEventStatus unused;

You should still initialize this to something sane so it's not garbage.

Comment 99

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

https://reviewboard.mozilla.org/r/19173/#review18989

::: gfx/layers/apz/src/APZCTreeManager.cpp:643
(Diff revision 3)
> +  return aEvent.mMessage == eMouseMove ||

I'm assuming here that you've exercised this code locally with the pref enabled and that it does work. In which case I'm wondering when does the input mouse event have a message type of eDragStart or eDragEnd? Those types are handled in the WidgetMouseEvent -> MouseInput conversion but I would expect them to be handled here as well.

Comment 100

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

Comment on attachment 8668668 [details]
MozReview Request: Bug 1199885 - Part 14: Add input blocks for mouse events. r?kats

https://reviewboard.mozilla.org/r/21011/#review18991

::: gfx/layers/apz/src/InputBlockState.cpp:243
(Diff revision 1)
> +  return "mouse wheel";

copy/paste error

::: gfx/layers/apz/src/InputQueue.h:11
(Diff revision 1)
> +#include "TouchCounter.h"

Not sure why these includes are needed here, doesn't seem like they should be.

::: gfx/layers/apz/src/InputQueue.cpp:179
(Diff revision 1)
> +  bool newBlock = aEvent.mType == MouseInput::MOUSE_DOWN && aEvent.IsLeftButton();

So the way I understand this code, it's going to create a new block any time the user pushes the left button down, and all start putting mouse events into that block until either the next mouse-down happens, or some other input type comes along and breaks the block. This means things like mouse-move events that are outside a drag can also end up in blocks, and I'm not a fan of this.

In comment 16 I described what I would like the input blocks to be like - specific to drag gestures, rather than generic mouse input blocks. Converting the way these blocks work from "mouse blocks" to "drag blocks" will simplify a lot of stuff. You probably won't need things like mIsInDrag, and won't need to un-const functions like DispatchEvent. I looked over the bits that aren't directly related to the input block implementation (i.e. mostly the conversion in the widget/ code) and that looks ok, but I'm holding off on reviewing the bulk of this patch because changing it to drag blocks will be significant.

I also thought we decided in subsequent comments (19, 20, 26) to not do the input block stuff now, and I wasn't aware you were going to implement this. Sorry if you told me and I missed it.

I'm happy with parts 1-13 landing as-is (modulo the comments I just made above) but this part 14 will need a bunch more work to get landed. If you want to continue cleaning this up I'd suggest forking a separate bug to do this and land parts 1-13 before they rot. What do you think? Please also note that I'll be on PTO October 6-22 so if you want to land it all together this part would need to be made landable before October 6, or it will have to wait until after I'm back.

::: widget/cocoa/nsChildView.mm
(Diff revision 1)
> -        DispatchEvent(&event, status);

Why did you remove this line?

Comment 101

[Benoit Girard (:BenWa)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #98)
> https://reviewboard.mozilla.org/r/19171/#review18987
> 
> ::: dom/ipc/TabChild.cpp:1940
> (Diff revision 3)
> > +  nsEventStatus unused;
> 
> You should still initialize this to something sane so it's not garbage.

I actually just had a similar discussion with Jeff today about another problem. In this case we don't really have any value that are meaningful when initialized. By initializing this to an incorrect value it means that tools that warn for 'read without initializing' wont work and thus we might start using a bad value. Leaving this as garbage is better IMO.

Perhaps the best fix here is to add an overload of InputAPZContext that doesn't take this out param.

Comment 102

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

(In reply to Benoit Girard (:BenWa) from comment #101)
> I actually just had a similar discussion with Jeff today about another
> problem. In this case we don't really have any value that are meaningful
> when initialized. By initializing this to an incorrect value it means that
> tools that warn for 'read without initializing' wont work and thus we might
> start using a bad value. Leaving this as garbage is better IMO.

That's a good point. Ok, we can leave it as garbage then.

Comment 103

[Benoit Girard (:BenWa)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #100)
> ::: gfx/layers/apz/src/InputQueue.cpp:179
> (Diff revision 1)
> > +  bool newBlock = aEvent.mType == MouseInput::MOUSE_DOWN && aEvent.IsLeftButton();
> 
> So the way I understand this code, it's going to create a new block any time
> the user pushes the left button down, and all start putting mouse events
> into that block until either the next mouse-down happens, or some other
> input type comes along and breaks the block. This means things like
> mouse-move events that are outside a drag can also end up in blocks, and I'm
> not a fan of this.

As we discussed on IRC we're going to change this to be called a drag block and only consider events between a mouse down and mouse up block.

> 
> In comment 16 I described what I would like the input blocks to be like -
> specific to drag gestures, rather than generic mouse input blocks.
> Converting the way these blocks work from "mouse blocks" to "drag blocks"
> will simplify a lot of stuff. You probably won't need things like mIsInDrag,
> and won't need to un-const functions like DispatchEvent. I looked over the
> bits that aren't directly related to the input block implementation (i.e.
> mostly the conversion in the widget/ code) and that looks ok, but I'm
> holding off on reviewing the bulk of this patch because changing it to drag
> blocks will be significant.

Yes, I think you're right and it will make this easier. I'll have a shot at it.

> 
> I also thought we decided in subsequent comments (19, 20, 26) to not do the
> input block stuff now, and I wasn't aware you were going to implement this.
> Sorry if you told me and I missed it.
> 
> I'm happy with parts 1-13 landing as-is (modulo the comments I just made
> above) but this part 14 will need a bunch more work to get landed. If you
> want to continue cleaning this up I'd suggest forking a separate bug to do
> this and land parts 1-13 before they rot. What do you think? Please also
> note that I'll be on PTO October 6-22 so if you want to land it all together
> this part would need to be made landable before October 6, or it will have
> to wait until after I'm back.
> 

My aim was to get this patch set working since as the review progressed, the feature no longer worked without input blocks. I don't want to land a huge patch queue that's effectively dead code so I start to work on the last part now. I'll move it to a separate bug. We're still going to need to a follow up to deal with wheel and mouse/drag blocks mixing. Right now if you click during a wheel scroll the wheel scroll stops abruptly. That will likely be a follow-up.

> ::: widget/cocoa/nsChildView.mm
> (Diff revision 1)
> > -        DispatchEvent(&event, status);
> 
> Why did you remove this line?

I'm not sure, maybe a vim accident while searching. It's restored now.

Comment 104

[Benoit Girard (:BenWa)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=abad97dcf097

Comment 105

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c267e8cc7952253892b062498d21afd7b10559dd
Bug 1199885 - Part 1: Add MouseInput InputData. r=kats

https://hg.mozilla.org/integration/mozilla-inbound/rev/60cbca9b66500267658a551be21c6ae7a0f5b213
Bug 1199885 - Part 2: Add AsyncDragMetrics. r=kats

https://hg.mozilla.org/integration/mozilla-inbound/rev/853289b175764b66f72d5557bb84697934678fdf
Bug 1199885 - Part 3: Add 'apz.drag.enabled' preference for async scrollbars. r=kats

https://hg.mozilla.org/integration/mozilla-inbound/rev/012607cf35d59a3bf6b3e0c14d91adec87b1773e
Bug 1199885 - Part 4: Let the hit testing tree know about the scroll thumb. r=kats

https://hg.mozilla.org/integration/mozilla-inbound/rev/f650905bf686fa0139c7c96ada68c6590eb34809
Bug 1199885 - Part 5: Make mRootLayerTreeID const to prove that there's no data races. r=kats

https://hg.mozilla.org/integration/mozilla-inbound/rev/c6f50e9286b663bf41da5ba01f9a54f45782c06e
Bug 1199885 - Part 8: Add FindScrollNode to locate the scrollbar thumb. r=kats

https://hg.mozilla.org/integration/mozilla-inbound/rev/0c27f0511f8893bc673024d32317dcc76d0e8489
Bug 1199885 - Part 9: Let APZC handle the drag events. r=kats

https://hg.mozilla.org/integration/mozilla-inbound/rev/f69028be4a423c5c56fc7101b33e921a3e4c2c28
Bug 1199885 - Part 9.5: Make the mouse events APZC aware. r=kats

https://hg.mozilla.org/integration/mozilla-inbound/rev/266c7e4df84e81a5f13cb14a1caa389df92b7ee6
Bug 1199885 - Part 10: Add APZTeeManager API to start an async scroll. r=kats

https://hg.mozilla.org/integration/mozilla-inbound/rev/c9999ccc9a8b7f77ad54b3540196c2725a82bb7b
Bug 1199885 - Part 12: Add StartScrollbarDrag IPC message. r=kats

https://hg.mozilla.org/integration/mozilla-inbound/rev/ccaad97f4f64210abd20219d9466e4f589accb53
Bug 1199885 - Part 13: Let nsSliderFrame trigger async scrolling via StartScrollbarDrag. r=kats

Comment 106

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c267e8cc7952
https://hg.mozilla.org/mozilla-central/rev/60cbca9b6650
https://hg.mozilla.org/mozilla-central/rev/853289b17576
https://hg.mozilla.org/mozilla-central/rev/012607cf35d5
https://hg.mozilla.org/mozilla-central/rev/f650905bf686
https://hg.mozilla.org/mozilla-central/rev/c6f50e9286b6
https://hg.mozilla.org/mozilla-central/rev/0c27f0511f88
https://hg.mozilla.org/mozilla-central/rev/f69028be4a42
https://hg.mozilla.org/mozilla-central/rev/266c7e4df84e
https://hg.mozilla.org/mozilla-central/rev/c9999ccc9a8b
https://hg.mozilla.org/mozilla-central/rev/ccaad97f4f64