930939 - (input-thread) Separate the threads that run gecko and that receive input events from the OS

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Gonk, defect, P1)

Description

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

One of the goals for future FxOS versions is to improve the responsiveness so that the latency between a user touching the screen and a visual effect happening on the screen is 120ms or less [1].

Currently, the fastest codepath we can do this on has input events getting received from the OS on the gecko thread (which may be busy doing things like running JS or GC'ing), sending that input event through gecko for hit testing, then sending it to the AsyncPanZoomController and having it update some state. At the next frame composite, the compositor thread would pick up the updated transform from the APZC and show it on the screen. This path involves (1) waiting for gecko, (2) hit testing in gecko, (3) more hit testing in APZC, and (4) transfer of state from the gecko thread to the compositor thread.

I had previously filed bug 920036 to move (2) off this codepath, but I think we can get rid of (1) as well by moving gecko off the process main thread. The process main thread should be used strictly for dispatching the events received from the OS to the right thread (either the gecko thread or the compositor thread) as fast as possible. Fennec already uses this model, because the gecko thread is different from the Android UI main thread where events are received, and motion events can result in compositor updates without going through gecko.

Note that one option that would get rid of (4) is to run the compositor on the process main thread. However on B2G the compositor blocks on vsync, so if the compositor is blocked there it will hold up processing of other events being received by the OS. On platforms where the compositor never blocks it might be possible to do this but that's outside the scope of this bug.

Another constraint is that for scenarios like full-screen games, we want touch events delivered to content JS with the same (or even lower) latency. For this case it would be counterproductive to always routing the events through the compositor for this; it would be better if the input-handling thread had sufficient information to send the input directly to the content process so that it could be dispatched to JS there quicker.

[1] https://etherpad.mozilla.org/graphics-work-week (in case this is gone, this is coming from the FxOS UX team)

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You're talking specifically about touch events that trigger panning and zooming in APZC, right? Not, say, touching a button? That's implied by your comment but not stated.

On B2G, couldn't we modify Gonk to receive touch events on any thread we want?

Comment 2

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

According to vlad getting touch events to content has an even tighter responsiveness requirement because of games. This means we want both touch events for panning/zooming in APZC and touch events for buttons to be fast.

And yes, I assume we can modify Gonk to receive touch events on any thread, but I don't think that any of existing threads we have on B2G's current architecture are good candidates because they all result in trading off latency to APZC vs latency to content. That's why I'm suggesting a new thread/architecture for this purpose.

Comment 3

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

Attachment: Current event flow on B2G

For the record, this is what the current input event flow looks like on B2G. I drew this up on the flight back from the rendering work week and meant to blog about it but in case I end up not doing that I still want people to see it.

Comment 4

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

(The red lines are cross-process messaging and the yellow lines are cross-thread)

Comment 5

[Benoit Girard (:BenWa)]

Very nice. Which thread is the are all the first steps done in? The b2g process' main thread? That would mean events waits for the status bar to paint before we can dispatch them to content processes.

Comment 6

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

Yeah, the first steps are all on the b2g process main thread, which runs gecko.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Nice diagram.

It's not clear to me how you plan to avoid going through the compositor thread. I suppose once we have touch regions hooked up (I have a patch for computing the regions now) the compositor could, on every frame, push to some event-handling thread a map of regions indicating for each point, which thread a touchstart event for that point should go to (the compositor thread, or the content thread of some process). But it might be difficult to dispatch events to the content thread of some subprocess without going through the content thread of the b2g process due to synchronization issues.

Comment 8

[Vincent Lin[:vilin]]

Just would like to know if this bug would be helpful for mine.
https://bugzilla.mozilla.org/show_bug.cgi?id=944564#c0 (2nd problem)

The current input event flow of swiping Homescreen on B2G looks like ...
(process/thread, -> cross-thread, --> cross-process)
b2g/InputReader -> b2g/b2g --> content/Homescreen --> b2g/Compositor

I noticed that there're 3 problems make transmission of input event blocked.
b2g/b2g costs ViewportFrame::BuildDisplayList() for touch_start.
b2g/b2g is doing GC.
content/Homescreen is re-flowing or painting layer. (BenWa has implemented pre-render to prevent from painting for homescreen, but other apps may block receiving input event for some reasons.)

So to overcome these blocks, are you going to move all gecko stuff off the main thread ?
I'm also concerned about synchronization problem.

Comment 9

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

This bug should in theory get rid of the b2g/b2g step in your flow, which will help alleviate some of the problems. However since we don't have a concrete plan yet for how to organize the input event flow I can't say that for certain.

For the record, I would like come up with a modified version of attachment 827404 [details] and have everybody sign off on it before any code changes are made. If anybody has the time and is willing to try doing that please feel free. (It's a hand-written SVG so you you can even just grab it and edit it manually).

Comment 10

[Vincent Lin[:vilin]]

Does GeckoInputDispatcher::dispatchOnce() have to be called in main thread for some reasons now ? Or why not just simply take advantage of InputManager class in libui ? Then there will be a dedicated "InputDispatcher" thread for running it.

Comment 11

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

The current implementation of GeckoInputDispatcher::dispatchOnce() invokes nsWindow::DispatchInputEvent which delivers the event to gecko so it must run on the gecko thread. This bug is about changing that. using the InputManager class in libui is one possible option, yes.

Comment 12

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

Attachment: Update event flow on B2G with edge-gestures implemented

Yay more complicated event flow!

Comment 13

[Mason Chang [Inactive] [:mchang]]

Attachment: Touch start flow

Hey Kats, here's a first stab of the overall flow for what happens on a touch start only in the new architecture where we process the input events off main thread. Any comments or is this accurate from what we discussed? Thanks!

Comment 14

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

Hey, sorry for the delay. I got swamped with some tricky bugs that I wanted to get out of the way first. So the main problem in your flow is that the "Dispatch touch even to main thread" happens before the APZ untransform, but it needs to happen after. Also after the event goes through the main process main thread it needs to go through the child process steps as well.

I like the way you used the colors in your diagram, it helps with understanding what happens where. I might update my SVG to do that as well so maybe things will be clearer. Also I think the "Gecko untransform" box is obsolete now and can probably be removed. At least I don't remember what that is anymore.

Comment 15

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #14)
> Also I think the "Gecko untransform" box is
> obsolete now and can probably be removed. At least I don't remember what
> that is anymore.

It sounds like the untransformation done in TabParent::MapEventCoordinatesForChildProcess to get the events into the child process' coordinate system, although that's done in the APZ case as well.

Comment 16

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

Attachment: Existing event flow (v3)

Comment 17

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

Attachment: Desired event flow (v1)

Comment 18

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

(In reply to Botond Ballo [:botond] from comment #15)
> It sounds like the untransformation done in
> TabParent::MapEventCoordinatesForChildProcess to get the events into the
> child process' coordinate system, although that's done in the APZ case as
> well.

Yeah that sounds right. I put it on both paths in the updated flow.

Comment 19

[Mason Chang [Inactive] [:mchang]]

Attachment: WIP: Create GeckoTouchDispatcher and convert gonk input to APZ Input

Comment 20

[Mason Chang [Inactive] [:mchang]]

Attachment: WIP: Send TouchEvents to APZ on input thread on TabParent::Capture path

Just to test that I was sending the right events, on TabParent::Capture paths, we directly send the touch events to the APZC controller. It actually all works and we can scroll! It completely crashes on non-scroll events such as a tap, but one step at a time.

I also flashed two flames, one with and one without this patch. There is a very subtle but nice noticeable improvement in scrolling responsiveness! The other fallout is that we see low res tiles a lot more often, probably because I'm not correctly notifying Gecko to paint.

Either way, I think this patch at least translates touch input scroll events correctly.

Comment 21

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

Comment on attachment 8443824 [details] [diff] [review]
WIP: Send TouchEvents to APZ on input thread on TabParent::Capture path

Review of attachment 8443824 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/apz/src/APZCTreeManager.h
@@ +129,5 @@
>    nsEventStatus ReceiveInputEvent(const InputData& aEvent,
>                                    ScrollableLayerGuid* aOutTargetGuid);
>  
> +  nsEventStatus ReceiveInputEventOnInputThread(const MultiTouchInput& aMultiTouchInput,
> +                                  ScrollableLayerGuid* aOutTargetGuid);

You don't need this function. The ReceiveInputEvent(const InputData& aEvent, ScrollableLayerGuid* aOutTargetGuid) is meant for exactly this purpose.

Comment 22

[Mason Chang [Inactive] [:mchang]]

Attachment: WIP: Send Touch Start/End on Main thread, Moves on input thread

Changing the touch start/ends to be on the main thread and the touch moves on the input thread makes tapping work again. 

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #21)
> Comment on attachment 8443824 [details] [diff] [review]
> You don't need this function. The ReceiveInputEvent(const InputData& aEvent,
> ScrollableLayerGuid* aOutTargetGuid) is meant for exactly this purpose.

Yeah, I thought I needed to change more things so I was playing with the code. Looks like I probably won't need this function for now.

Comment 23

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

(In reply to Mason Chang [:mchang] from comment #22)
> Created attachment 8444504 [details] [diff] [review]
> WIP: Send Touch Start/End on Main thread, Moves on input thread
> 
> Changing the touch start/ends to be on the main thread and the touch moves
> on the input thread makes tapping work again. 
> 

Except we still want to deal with touch start/ends on the input thread, not the main thread. The likely culprit for tapping breaking is that when the APZ code tries to schedule a delayed task (by invoking [1]) then it doesn't work because MessageLoop::current is the input thread's message loop, or something.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/ipc/RenderFrameParent.cpp?rev=5da74b54a472#642

Comment 24

[Mason Chang [Inactive] [:mchang]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #23)
> (In reply to Mason Chang [:mchang] from comment #22)
> > Created attachment 8444504 [details] [diff] [review]
> > WIP: Send Touch Start/End on Main thread, Moves on input thread
> > 
> > Changing the touch start/ends to be on the main thread and the touch moves
> > on the input thread makes tapping work again. 
> > 
> 
> Except we still want to deal with touch start/ends on the input thread, not
> the main thread. The likely culprit for tapping breaking is that when the
> APZ code tries to schedule a delayed task (by invoking [1]) then it doesn't
> work because MessageLoop::current is the input thread's message loop, or
> something.
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/layout/ipc/RenderFrameParent.
> cpp?rev=5da74b54a472#642

Thanks for the hint on where it is breaking. Yeah I still want to move all touch events to the input thread, was just seeing why it broke so I have a grounding in the code. Just tracing through the code some more :)

Comment 25

[Mason Chang [Inactive] [:mchang]]

Attachment: WIP: Touch events on input thread

Seems to work sometimes, at least for the vertical home screen. When I load an app, the app crashes when I scroll. The Input thread's message loop always seems to crash when we post an event to it, so I have to investigate and see if the input thread even has a message loop.

Another interesting thing, once I move my finger, there is quite a bit of delay until we start scrolling, more so than without this patch. I think it's because we're processing touch events but waiting for the GeckoContent to callback saying or to create scrollable layers, which I think occurs on the child main thread. Still have to investigate.

Comment 26

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

(In reply to Mason Chang [:mchang] from comment #25)
> Another interesting thing, once I move my finger, there is quite a bit of
> delay until we start scrolling, more so than without this patch. I think
> it's because we're processing touch events but waiting for the GeckoContent
> to callback saying or to create scrollable layers, which I think occurs on
> the child main thread. Still have to investigate.

You can verify this by looking at the layer dumps before and after. It might also be that the APZ code is having to wait longer before it hears back from content about touch listeners via the ContentReceivedTouch callback.

Comment 27

[Mason Chang [Inactive] [:mchang]]

Attachment: WIP: Touch Events on Input Thread

Ugly ugly hack. The android input thread doesn't have it's own message loop, so I added one since APZ needs to post tasks. Unfortunately, the input thread is always asleep unless an input event comes in, so the delayed tasks don't run. This is an ugly hack to wake the thread up every 16 ms (to align with vsync) so that our delayed tasks do run.

In the future, we'll run APZ on the vsync thread which should be a full nsThread with a MessageLoop.

Things that probably break: any gestures (long taps, etc), multi touch, anything other than scrolling. But pure scroll events are now on the input thread.

Comment 28

[Mason Chang [Inactive] [:mchang]]

Best validation that this works. I went to nytimes.com, which loads to the desktop site. I tried with two flames, one with and one without this patch. nytimes.com runs a video and lots of JS. I could barely scroll on the flame off tip without this patch. I could scroll mostly pretty well (and I'm sure it's because I broke touch start in some cases) with this patch. hoorah!

Comment 29

[Mason Chang [Inactive] [:mchang]]

Attachment: Patch on a flame on nytimes.com

Comment 30

[Mason Chang [Inactive] [:mchang]]

Attachment: m-c on a nexus 4 on nytimes.com

Comment 31

[Mason Chang [Inactive] [:mchang]]

Attachment: WIP: Touch Events on Input Thread

Updated WIP. This depends on the vsync thread from bug 987527, and a few tweaks to that patch that will be removed later. We also have an InputThreadLoop, which should be removed once 987529 lands and we have a vsync thread with a MessageLoop.

Currently, we get touch events on the input thread and put them in a queue. Every vsync, we notify the main thread and process the touch events through APZ on the vsync thread. Most of the changes involved in APZC are making sure we update the transformToGecko in APZCTreeManager::ReceiveInputEvent and in the RenderFrameParent/GeckoContentController to make sure we post the callbacks on the input thread rather than the main thread. The guiding assumption is callbacks which directly call TabParent/TabChild go on the main thread, all other callbacks should go on the input thread. This is because TabParent/TabChild adjust things based on the DOM which can only be accessed on the main thread. Sending IPDL messages on different threads but on the same channel results in sporadic crashes. 

It all seems to work, including edge gestures. It's still quite dirty and I will clean it up after some more testing.

Comment 32

[Mason Chang [Inactive] [:mchang]]

Attachment: WIP: Touch Events on Input Thread

Cleaned up a bit more and we also send key events correctly now.

Comment 33

[Mason Chang [Inactive] [:mchang]]

Attachment: WIP: Touch Events on vsync Thread

WIP to get touch events on the vsync thread. This fixes issues with overscroll animation getting stuck and the overflow APZC hit test (http://people.mozilla.org/~kgupta/tmp/overflow.html#). Currently, after APZC processes the touch event, we send a clone of the original touch data through the main thread as a WidgeTouchEvent. When TabParent::MaybeForwardToRenderFrame comes around, we update the touch event with the Gecko Untransform.

A few things until this patch should hopefully be ready for review:
1) We need the full vsync thread / dispatcher in bug 987529. Jerry is working on that.
2) Cleaned up in a few places with a lot fewer printfs and removal of the InputThreadLoop. The Vsync thread is a chromium::Thread and will have a MessageLoop.
3) One thing I'm concerned about is that we process touch events on the Vsync thread, then dispatch the same touch event through the main thread. When that same touch event comes around to the APZCTreeManager::ReceiveInputEvent, we currently check if we have an mApzcForInputBlock and return a status based on mInOverscrolledApzc, which might be sending different results to TabParent for the same touch. For example, if we get a TOUCH_END on the vsync thread before the parent process sees the TOUCH_END on the main thread, we already set mApzcForInputBlock to null. This hasn't seemed to be a problem in practice, but maybe we should feed the result of ReceiveInputThread on from the input thread all the way through Gecko?

Kats, any thoughts on #3? Thanks!

Comment 34

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

(In reply to Mason Chang [:mchang] from comment #33)
> maybe we should
> feed the result of ReceiveInputThread on from the input thread all the way
> through Gecko?

Yes, exactly. In bug 1035356 which I landed recently the ReceiveInputEvent(InputData) function now applies the gecko untransform as well. So if you create the WidgetInputEvent clone from the InputData *after* calling ReceiveInputEvent, get the gecko-untransformed version, and you can just send that through gecko. That way you won't need to send the input event again to the APZC in MaybeForwardToRenderFrame at all. Does that make sense?

Comment 35

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #34)
> calling ReceiveInputEvent, get the gecko-untransformed version, and you can

s/get/you will get/

Comment 36

[Mason Chang [Inactive] [:mchang]]

Attachment: WIP: Touch Events on Vsync Thread

Rebased with the vsync work from Jerry. I was able to delete the InputThreadLoop. All 3 things from Comment 33 are done. At the moment, after APZ Processes the event, we store both the original touch data and the APZ untransformed data and put it into a WidgetAPZTouchEvent, which carries both touch sets. We pass that through Gecko with the original touch data, then at TabParent, send the APZ untransformed data through TabChild. It's a lot of data to carry around through the pipeline, not sure how I like it as a solution.

Another side bug I found was while having some overlays, APZ sees the data as well as the overlay, so we get something like two events. For example, in the Settings app, the ADB and devtools setting + the developer hud button overlay each other. When choosing one option, the touch event gets sent to both the button overlay + the developer hud.

Comment 37

[Mason Chang [Inactive] [:mchang]]

Attachment: WIP: Touch Events on Vsync Thread

Updated to return true/false if we processed an input event on a vsync. This helps Jerry do some potential optimizations. Otherwise it's the same.

Comment 38

[Mason Chang [Inactive] [:mchang]]

Attachment: WIP: Touch Events on Vsync Thread

Updated to compile on desktop linux / os x platforms. Cleaned up to use the new WIP from Jerry's vsync bug. After bug 973096 lands, I don't see any other major bugs. I will test and clean up, then start asking for feedback. Reviews will come after Jerry's patches land as we need the vsync thread there.

Comment 39

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 1: Modify APZ to process events on vsync thread

Comment 40

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 2: Add WidgetAPZTouchEvent to hold APZ untransformed touches

Comment 41

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 3: Enable conversions to / from MultiTouchEvent - WidgetTouchEvent

Comment 42

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 4: Add Gecko Touch Dispatcher

After talking with :mwu, we want to go from nsAppShell -> Gecko Touch Dispatcher -> APZ. The Gecko Touch Dispatcher mostly converts android touch data -> MultiTouchInput and is the glue between gonk / APZ.

Comment 43

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 5: Dispatch Touch Event from gonk to APZ

Comment 44

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 6: Hook Input Dispatcher into Vsync Dispatcher

Just some changes required to hook into vsync from bug 987529. No need to review this, this part is just some glue work.

Comment 45

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

Comment on attachment 8460281 [details] [diff] [review]
Part 1: Modify APZ to process events on vsync thread

Review of attachment 8460281 [details] [diff] [review]:
-----------------------------------------------------------------

For future patches, please make sure you include more lines of context (usually at least 8) so that it's easier to see where your changes.

Also, in general you should try to order the patches so that each patch only depends on previous ones in the sequence. This patch won't even compile by itself.

::: dom/ipc/TabParent.cpp
@@ +956,5 @@
> +bool
> +TabParent::SendWidgetTouchEvent(WidgetTouchEvent& aEvent)
> +{
> +  // Might not be true, probably still need for other platforms?
> +  NS_RUNTIMEABORT("Shouldn't get here anymore");

This code might be needed for e10s-enabled platforms without APZ. I would remove the runtimeabort.

@@ +981,5 @@
> +    return false;
> +  }
> +
> +  WidgetTouchEvent apzProcessedTouch(true, aApzTouchEvent->message, nullptr);
> +  aApzTouchEvent->ReplaceWithAPZTransforms(apzProcessedTouch);

ReplaceWithAPZTransforms is a very confusing name. Without looking at a future patch on this bug it's very hard for me to tell what this is doing.

@@ +1070,5 @@
>  
> +  const WidgetAPZTouchEvent* apzEvent = aEvent.AsAPZTouchEvent();
> +  if (apzEvent) {
> +    WidgetAPZTouchEvent apzTouch(*apzEvent);
> +    return SendRealTouchEvent(apzTouch);

Does this work? You're passing a WidgetAPZTouchEvent to a function that takes a WidgetTouchEvent&. Even if it does work, what's the point of doing this? Why can't you just leave it as always passing |event|?

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +659,4 @@
>  
>    switch (aEvent.eventStructType) {
>      case NS_TOUCH_EVENT: {
> +      NS_RUNTIMEABORT("WE SHOULDNT GET HERE ANYMORE");

Leave this function as-is. It's still used on Metro.

::: layout/ipc/RenderFrameParent.cpp
@@ +668,5 @@
>    virtual void PostDelayedTask(Task* aTask, int aDelayMs) MOZ_OVERRIDE
>    {
> +    if (NS_IsMainThread()) {
> +      MessageLoop::current()->PostDelayedTask(FROM_HERE, aTask, aDelayMs);
> +      NS_RUNTIMEABORT("SHOULD NOT BE ON MAIN THREAD\n");

Remove.

@@ +670,5 @@
> +    if (NS_IsMainThread()) {
> +      MessageLoop::current()->PostDelayedTask(FROM_HERE, aTask, aDelayMs);
> +      NS_RUNTIMEABORT("SHOULD NOT BE ON MAIN THREAD\n");
> +    } else if (MessageLoop::current()) {
> +      // Probably the Compositor thread

Remove this comment, since it's not actually the compositor thread that will be running here.

@@ +1172,5 @@
>      return;
>    }
> +
> +  #ifdef MOZ_WIDGET_GONK
> +  mContentController->RequestContentReceivedTouch(aGuid, aPreventDefault);

This seems unnecessarily convoluted. All you should need to do is, at the top of RenderFrameParent::ContentReceivedTouch, add this:

#ifdef MOZ_WIDGET_GONK
  // For gonk this needs to run on the vsync thread, so re-dispatch to there.
  if (NS_IsMainThread()) {
    GonkVsyncDispatcher::GetInstance()->GetMessageLoop()->PostDelayedTask(FROM_HERE, NewRunnableMethod(this, &RenderFrameParent::ContentReceivedTouch, aGuid, aPreventDefault), 0);
  }
#endif

Comment 46

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

Comment on attachment 8460282 [details] [diff] [review]
Part 2: Add WidgetAPZTouchEvent to hold APZ untransformed touches

Review of attachment 8460282 [details] [diff] [review]:
-----------------------------------------------------------------

This patch is incomplete and missing changes to InputData.cpp. This set of changes (adding WidgetAPZTouchEvent and conversion stuff) should be patch #1

::: widget/TouchEvents.h
@@ +214,5 @@
>  
> +/******************************************************************************
> + * mozilla::WidgetAPZTouchEvent
> + * For touch events already processed by APZ. Contains two sets of transforms
> + * Those from the InputReader in Gonk and those already processed by APZ

This comment doesn't seem to match the code. I don't see anything that looks like "transforms" stored in the class, and there is only one TouchArray.

Comment 47

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

Comment on attachment 8460283 [details] [diff] [review]
Part 3: Enable conversions to / from MultiTouchEvent - WidgetTouchEvent

Review of attachment 8460283 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/xpwidgets/InputData.cpp
@@ +123,5 @@
> +
> +void
> +MultiTouchInput::ConvertToWidgetTouchEvent(WidgetTouchEvent& aWidgetTouchEvent) const
> +{
> +  uint32_t touchType;

This should probably assert it's on the main thread. Ditto for the other functions

Comment 48

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

Comment on attachment 8460286 [details] [diff] [review]
Part 4: Add Gecko Touch Dispatcher

Review of attachment 8460286 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/gonk/GeckoTouchDispatcher.cpp
@@ +226,5 @@
> +                                    aApzStatus, aGuid);
> +    aGeckoTouch.ConvertToWidgetAPZTouchEvent(touchEvent, aApzTouch);
> +    nsEventStatus status = nsWindow::DispatchInputEvent(touchEvent, &wasCaptured);
> +
> +    if (!wasCaptured) {

I think we'll have to get rid of the capture code path because it will break the contract about APZ receiving content responses. That is, for every touch block the APZ code receives, it MUST receive exactly one call to ContentReceivedTouch(). The capture code path means that the APZ is still getting input events but then they are captured by the root process and so it never receives the corresponding ContentReceivedTouch() calls.

Actually I'm not sure even if that is enough, because the root process might do a preventDefault on the event regardless of the capture code path, and we would have to notify the APZ of that.

Comment 49

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

Comment on attachment 8460287 [details] [diff] [review]
Part 5: Dispatch Touch Event from gonk to APZ

Review of attachment 8460287 [details] [diff] [review]:
-----------------------------------------------------------------

I mostly don't understand this patch. There's a lot of unrelated whitespace/formatting changes mixed together it looks like. Those types of non-functional changes should be in their own patch. There's also a bunch of vsync stuff in here that I'm not sure about, I think the only part I care about is the nsWindow::DispatchInputEventToAPZ function.

Comment 50

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

Overall I think we're heading in the right direction. The only "big" concern I had was ensuring that any time the APZ code processes events (i.e. APZCTreeManager::ReceiveInputEvent returns nsEventStatus_eConsumeDoDefault) that we make sure that touch block gets a ContentReceivedResponse(). To be fair that's a requirement from bug 1009733 which I landed pretty recently so it didn't cross my mind before that or I would have brought it up sooner. If we can sort that out (which shouldn't be too hard, I think) and clean up the patches I think we're good.

Comment 51

[Mason Chang [Inactive] [:mchang]]

I just had a discussion with kats regarding his comments. Overall:

> Does this work? You're passing a WidgetAPZTouchEvent to a function that takes a WidgetTouchEvent&. Even 
> if it does work, what's the point of doing this? Why can't you just leave it as always passing |event|?

We are given a WidgetGUIEvent and convert it to a WidgetTouchEvent. However, the WidgetAPZTouchEvent contains two sets of touch events, so converting it to a WidgetTouchEvent will lose one set of touch events. Thus, we'll change SendRealTouchEvent to take a WidgetTouchEvent*.

> This seems unnecessarily convoluted. All you should need to do is, at the top of 
> RenderFrameParent::ContentReceivedTouch, add this:

RenderFrameParent is not ref counted so we cannot create tasks and post to MessageLoops. We can get around this by moving the functionality out of RenderFrameParent and into the GeckoContentController.

> This comment doesn't seem to match the code. I don't see anything that looks like "transforms" stored in 
> the class, and there is only one TouchArray.

Because WidgetAPZTouchEvent inherits from WidgetTouchEvent, we have two touch arrays. One defined in WidgetTouchEvent and one defined in WidgetAPZTouchEvent.

> I think we'll have to get rid of the capture code path because it will break
> the contract about APZ receiving content responses. That is, for every touch
> block the APZ code receives, it MUST receive exactly one call to
> ContentReceivedTouch(). The capture code path means that the APZ is still
> getting input events but then they are captured by the root process and so
> it never receives the corresponding ContentReceivedTouch() calls.

For now we will keep the capture path for two reasons. One is that even on the capture path, we send the touch event to the content so we should still receive the ContentReceivedTouch. The second reason is that currently, if we do not capture a touch event, we also send a mouse event in addition to the touch event. This is probably some optimization. For now, we will keep the capture path but we would like to rip it out after the parent process has APZ enabled.


> I mostly don't understand this patch. There's a lot of unrelated
> whitespace/formatting changes mixed together it looks like. Those types of
> non-functional changes should be in their own patch. There's also a bunch of
> vsync stuff in here that I'm not sure about, I think the only part I care
> about is the nsWindow::DispatchInputEventToAPZ function.

I will clean this patch up. Most of the work to extract out a GeckoTouchDispatcher will be done in bug 970751.

Comment 52

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 1: Add WidgetAPZTouchEvent and methods to convert to/from MultiTouchInput

This part enables us to have the WidgetAPZTouchEvent, which contains two sets of touch events. One from the original gonk touch data and one after APZ has already transformed the touch events. It also enables support to transition between MultiTouchInput and WidgetAPZTouchEvent.

Comment 53

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 2: Enable nsWindow to dispatch input event to APZ

This portion sets up the code so that we have a reference in nsWindow to the APZCTreeManager where we can send touch events from gonk to APZ.

Comment 54

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 3: Modify TabParent to send APZ transformed touch events to content

This portion changes TabParent to not send touch events to APZ and instead, use the WidgetAPZTouchEvent data to directly send both the return value from APZ and the transformed touch data to the content.

Comment 55

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 4: Modify GeckoTouchDispatcher to listen to Vsync

Nothing to review here for now.

Comment 56

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 5: Dispatch Touch events from gonk to APZ

Modified the GeckoTouchDispatcher to convert gonk touch data into MultiTouchInput data. Then we send the touch event through to APZ and redispatch the touch event to the main thread on the parent process. There are some printfs that I'm using to debug because I think there is bug in APZ here:

http://dxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp?from=AsyncPanZoomController.cpp&case=true#2565

If we scroll and we have the content timeout, we process the touch events but never decrement mTouchBlockBalance. Then if we scroll again, when the content receives the touch, mTouchBlockBalance is > 0 so we never process the touch event:

http://dxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp?from=AsyncPanZoomController.cpp&case=true#2578

Thus we are in a state of perpetual timeouts. Is this a bug or am I misunderstanding the code? Thanks!

Comment 57

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 6: Make sure all tasks from APZ are run on the vsync thread

When content tells APZ that it received a touch, it occurs on the main thread. This patch ensures that we process the touch event in APZ on the vsync thread and not on both the main thread and vsync thread. We've had IPC crashes when before if APZ processes touch events on two different threads.

Comment 58

[Mason Chang [Inactive] [:mchang]]

For part 6, I will delete a lot of the runtime aborts after some more testing.

Comment 59

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

(In reply to Mason Chang [:mchang] from comment #56)
> If we scroll and we have the content timeout, we process the touch events
> but never decrement mTouchBlockBalance. Then if we scroll again, when the
> content receives the touch, mTouchBlockBalance is > 0 so we never process
> the touch event:

I haven't looked at the patches, but mTouchBlockBalance should be decremented as part of the ContentReceivedTouch callback. Every touch block MUST have exactly one ContentReceivedTouch callback, see [1]. The most likely problem here is that the ContentReceivedTouch callback is getting skipped somehow. Once I look through the patches I might have a better idea of what's happening.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.h?rev=02c5fd1fe559#734

Comment 60

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

Comment on attachment 8464373 [details] [diff] [review]
Part 3: Modify TabParent to send APZ transformed touch events to content

Review of attachment 8464373 [details] [diff] [review]:
-----------------------------------------------------------------

These patches are looking a million times better than the last set, thanks!

Comment 61

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

Comment on attachment 8464375 [details] [diff] [review]
Part 5: Dispatch Touch events from gonk to APZ

Review of attachment 8464375 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/gonk/GeckoTouchDispatcher.cpp
@@ +318,5 @@
> +
> +  NS_IMETHOD Run()
> +  {
> +    MOZ_ASSERT(NS_IsMainThread());
> +    mTouchDispatcher->DispatchInputEvent(mGeckoTouch,

Is the GeckoTouchDispatcher ever destroyed? Just want to make sure we don't end up with a bad ref here.

Comment 62

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

Comment on attachment 8464380 [details] [diff] [review]
Part 6: Make sure all tasks from APZ are run on the vsync thread

Review of attachment 8464380 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/ipc/RenderFrameParent.cpp
@@ +523,5 @@
> +  {
> +    // This happens on the main thread, so repost the content received touch onto
> +    // the vsync thread.
> +    Task* contentRecvTouch = NewRunnableMethod(this,
> +                           &RemoteContentController::DoContentReceivedTouch,

You can merge these two functions into a single one like so:

if (NS_IsMainThread()) {
  // stuff from RequestContentReceivedTouch, but s/DoContentReceivedTouch/RequestContentReceivedTouch/
} else {
  // stuff from DoContentReceivedTouch
}

I find that a little more self-documenting and robust, because then you don't need to assert which thread you're on, the code just handles it automatically.

@@ +664,5 @@
> +    if (NS_IsMainThread()) {
> +      MessageLoop::current()->PostDelayedTask(FROM_HERE, aTask, aDelayMs);
> +      NS_RUNTIMEABORT("SHOULD NOT BE ON MAIN THREAD\n");
> +    } else if (MessageLoop::current()) {
> +      // Probably the Compositor thread

s/Compositor/input/ but really you can combine the if/else branches because they do the same thing.

@@ +1168,5 @@
>    }
> +
> +  if (NS_IsMainThread()) {
> +    mContentController->RequestContentReceivedTouch(aGuid, aPreventDefault);
> +  } else if (GetApzcTreeManager()) {

These conditions will need some work, I think. You want to make sure that GetApzcTreeManager()->ContentReceivedTouch is called on the same thread that ReceiveInputEvent is called on. With your changes, on B2G, that will be the input thread. This code however might be used in e10s on non-B2G platforms in which case it should remain on the main thread (or whatever thread input events are delivered on).

Comment 63

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 6: Make sure all tasks from APZ are run on the vsync thread

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #62)
> Comment on attachment 8464380 [details] [diff] [review]
> Part 6: Make sure all tasks from APZ are run on the vsync thread
> 
> Review of attachment 8464380 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/ipc/RenderFrameParent.cpp
> @@ +523,5 @@
> > +  {
> > +    // This happens on the main thread, so repost the content received touch onto
> > +    // the vsync thread.
> > +    Task* contentRecvTouch = NewRunnableMethod(this,
> > +                           &RemoteContentController::DoContentReceivedTouch,
> 
> You can merge these two functions into a single one like so:
> 
> if (NS_IsMainThread()) {
>   // stuff from RequestContentReceivedTouch, but
> s/DoContentReceivedTouch/RequestContentReceivedTouch/
> } else {
>   // stuff from DoContentReceivedTouch
> }
> 
> I find that a little more self-documenting and robust, because then you
> don't need to assert which thread you're on, the code just handles it
> automatically.
> 

Updated to do that in this version. 

> @@ +664,5 @@
> > +    if (NS_IsMainThread()) {
> > +      MessageLoop::current()->PostDelayedTask(FROM_HERE, aTask, aDelayMs);
> > +      NS_RUNTIMEABORT("SHOULD NOT BE ON MAIN THREAD\n");
> > +    } else if (MessageLoop::current()) {
> > +      // Probably the Compositor thread
> 
> s/Compositor/input/ but really you can combine the if/else branches because
> they do the same thing.
> 

I will do that, I just want to leave the runtime abort there to test more corner cases to ensure that we really are processing all events on the vsync thread.

> @@ +1168,5 @@
> >    }
> > +
> > +  if (NS_IsMainThread()) {
> > +    mContentController->RequestContentReceivedTouch(aGuid, aPreventDefault);
> > +  } else if (GetApzcTreeManager()) {
> 
> These conditions will need some work, I think. You want to make sure that
> GetApzcTreeManager()->ContentReceivedTouch is called on the same thread that
> ReceiveInputEvent is called on. With your changes, on B2G, that will be the
> input thread. This code however might be used in e10s on non-B2G platforms
> in which case it should remain on the main thread (or whatever thread input
> events are delivered on).

For the moment, I put the call to mContentController in an #ifdef MOZ_WIDGET_GONK until I think of a better way to ensure that the thread that calls ReceiveInputEvent == the thread that handles content touch received.

Comment 64

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 1: Add WidgetAPZTouchEvent and methods to convert to/from MultiTouchInput

Updated to merge cleanly with bug 970751, which contains GeckoTouchDispatcher work.

Comment 65

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 2: Enable nsWindow to dispatch input event to APZ

Carrying f+ over.

Comment 66

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 3: Modify TabParent to send APZ transformed touch events to content

Comment 67

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 4: Dispatch Touch Events from Gonk to APZ

Modifications to take touch events from GeckoTouchDispatcher in bug 970751 and dispatch the resampled event to APZ. 

> Is the GeckoTouchDispatcher ever destroyed? Just want to make sure we don't 
> end up with a bad ref here.

From :mwu's feedback, we now use an nsRefPtr<GeckoTouchDispatcher>. However, this is only destroyed when GeckoInputDispatcher is destroyed, which I'm not sure it ever is.

Comment 68

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 5: Ensure all APZ tasks are run on the vsync thread

Same from comment 6. Will again remove all printfs / runtime aborts after more testing.

Comment 70

[Mason Chang [Inactive] [:mchang]]

Attachment: APZ Process Touch Events on Vsync Thread

Rebased off master and with bug 970751. Will split back into multiple parts once bug 970751 lands. Carrying f+=kats.

Comment 71

[Mason Chang [Inactive] [:mchang]]

Attachment: APZ Process Touch Events on Vsync Thread

Rebased off master. Renamed MultiTouchInput::ConvertToWidgetAPZTouchEvent to use the same naming style as ToWidgetTouchEvent. Now use MultiTouchInput::ToWidgetAPZTouchEvent() const; Carrying f+=kats.

Comment 72

[Mason Chang [Inactive] [:mchang]]

Attachment: APZ Process Touch Events on Vsync Thread

Rebased off master and off 970751. Lots of printfs which I want to keep in to debug bug 1053892 and other regressions that probably come up.

Comment 73

[Mason Chang [Inactive] [:mchang]]

Looks like until we get APZ hit testing working and in the parent process, this will have to bitrot :(.

Comment 74

[Kai-Chih Hu [:khu]]

Hi, Milan, do we know who could be the best one to own this bug? Thanks.

Comment 75

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

(In reply to Kevin Hu [:khu] from comment #74)
> Hi, Milan, do we know who could be the best one to own this bug? Thanks.

I don't understand the question - the bug is assigned, let me know what more details you need.

Comment 76

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

Attachment: Feed input to APZ on the compositor thread (WIP)

This moves the touch input to the compositor thread, although we could bump it to a different thread entirely if so desired. The only requirement is that we be able to post tasks to that thread, and unfortunately doing that with either the vsync thread or the libui thread is not easy.

This patch seems to work fine on top of my local patch queue but needs more testing.

Comment 77

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

Attachment: Part 1 - Introduce APZThreadUtils

Comment 78

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

Attachment: Part 2 - Encapsulate running things on the controller thread

Comment 79

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

Attachment: Part 3 - Switch the B2G controller thread to the compositor thread

Comment 80

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

Comment on attachment 8555484 [details] [diff] [review]
Part 1 - Introduce APZThreadUtils

Might as well get this one reviewed since it would be good to land regardless.

Comment 81

[Mason Chang [Inactive] [:mchang]]

from irc: setting a need info on myself to test this.

Comment 82

[Mason Chang [Inactive] [:mchang]]

I got to test this out, great stuff. With just master, so only the vsync compositor enabled, it works pretty well. There is one bug though:

1) Load contacts app
2) Load a contact
3) push back to go back to the contacts list
4) Cannot scroll, bug

I also see that the notification bar is lot jankier. But super heavy websites, like nytimes and reddit work really well. I can scroll up/down (into checkerboarding), but it's responsive. Edge swipes are a little worse, but the "bad" cases are already bad and even though they feel jankier, it's not that bad. I think overall it's a nice win.

I also tested with touch resampling enabled. Most of the jankiness on the main thread go away with touch resampling. The notification bar is smoother while tracking my finger than just master without silk or this patch. Edge swipes are still janky, but I've never seen them smooth generally. There was also the occasional super jank with touch resampling while scrolling the homescreen due to the main thread being busy. With this patch, I can't get it to jank on the homescreen and the two combined really improved the responsiveness by far! Scrolling text heavy sites used to be really bad, now it's pretty good.

Comment 83

[Mason Chang [Inactive] [:mchang]]

Attachment: Video of patch + resampling vs master

Just showing a very nice perf win!

Comment 84

[Mason Chang [Inactive] [:mchang]]

Hmm the bug from comment 82 seems to be in master without this patch. Might be related to 1126579 then.

Comment 85

[Botond Ballo [:botond]]

Comment on attachment 8555484 [details] [diff] [review]
Part 1 - Introduce APZThreadUtils

Review of attachment 8555484 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +137,5 @@
>  void
>  APZCTreeManager::SetAllowedTouchBehavior(uint64_t aInputBlockId,
>                                           const nsTArray<TouchBehaviorFlags> &aValues)
>  {
> +  APZThreadUtils::AssertOnControllerThread();

InputQueue::SetAllowedTouchBehavior already asserts this. Why double-assert?

@@ +691,5 @@
>  APZCTreeManager::ProcessTouchInput(MultiTouchInput& aInput,
>                                     ScrollableLayerGuid* aOutTargetGuid,
>                                     uint64_t* aOutInputBlockId)
>  {
> +  APZThreadUtils::AssertOnControllerThread();

I think this assertion would make more sense in APZCTreeManager::ReceiveInputEvent(WidgetInputEvent), as the other overload of ReceiveInputEvent has it, and this is a helper function for both.

Comment 86

[Botond Ballo [:botond]]

Comment on attachment 8555485 [details] [diff] [review]
Part 2 - Encapsulate running things on the controller thread

Review of attachment 8555485 [details] [diff] [review]:
-----------------------------------------------------------------

Does RenderFrameParent::NotifyInputEvent need to use RunOnControllerThread?

::: layout/ipc/RenderFrameParent.cpp
@@ +564,5 @@
> +    void (APZCTreeManager::*setTargetApzcFunc)(uint64_t, const nsTArray<ScrollableLayerGuid>&)
> +        = &APZCTreeManager::SetTargetAPZC;
> +    APZThreadUtils::RunOnControllerThread(NewRunnableMethod(
> +        GetApzcTreeManager(), setTargetApzcFunc,
> +        aInputBlockId, aTargets));

Yuck!

Now that we can use C++11 lambdas in our code, you may want to consider using one:

nsRefPtr<layers::APZCTreeManager> treeManager = GetApzcTreeManager();
APZThreadUtils::RunOnControllerThread(NewRunnableFunction([=]() {
  treeManager->SetTargetAPZC(aInputBlockId, aTargets);
}));

(The purpose of the local variable is to avoid having the lambda capture the 'this' pointer (to the RenderFrameParent object), which it would if you called a method like GetApzcTreeManager() inside the lambda.)

::: widget/gonk/nsWindow.cpp
@@ +291,5 @@
> +    // we need more. Also we can't pass in |this| to the task because nsWindow
> +    // refcounting is not threadsafe. Instead we just use the gFocusedWindow
> +    // static ptr inside the task.
> +    NS_DispatchToMainThread(new DispatchTouchInputOnMainThread(
> +        aInput, guid, inputBlockId));

template <typename Lambda>
class LambdaRunnable : public nsIRunnable {
  Lambda mLambda;
  LambdaRunnable(const Lambda& aLambda) : mLambda(aLambda) {}
  NS_IMETHOD Run() {
    mLambda();
  }
};

...

NS_DispatchToMainThread(new LambdaRunnable([=]() {
  if (gFocusedWindow) {
    gFocusedWindow->DispatchTouchEventForApz(aInput, guid, inputBlockId);
  }
}));

@@ +427,5 @@
> +    // non-const ref. At this callsite we don't care about the mutations that
> +    // the function performs so this is fine. Also we can't pass |this| to the
> +    // task because nsWindow refcounting is not threadsafe. Instead we just use
> +    // the gFocusedWindow static ptr instead the task.
> +    APZThreadUtils::RunOnControllerThread(new DispatchTouchInputOnControllerThread(inputToDispatch));

NewRunnableFunction([=]() mutable {
  if (gFocusedWindow) {
    gFocusedWindow->DispatchTouchInputViaAPZ(inputToDispatch);
  }
});

Note the use of 'mutable' to allow the lambda to mutate the captured variable 'inputToDispatch'.

::: widget/nsBaseWidget.cpp
@@ +962,5 @@
> +    // need a local var to disambiguate between the SetTargetAPZC overloads.
> +    void (APZCTreeManager::*setTargetApzcFunc)(uint64_t, const ScrollableLayerGuid&)
> +        = &APZCTreeManager::SetTargetAPZC;
> +    APZThreadUtils::RunOnControllerThread(NewRunnableMethod(
> +        mAPZC.get(), setTargetApzcFunc, aInputBlockId, aGuid));

nsRefPtr<APZCTreeManager> treeManager = mAPZC;
NewRunnableFunction([=]() {
  treeManager->SetTargetAPZC(aInputBlockId, aGuid);
})

Comment 87

[Botond Ballo [:botond]]

Comment on attachment 8555488 [details] [diff] [review]
Part 3 - Switch the B2G controller thread to the compositor thread

Review of attachment 8555488 [details] [diff] [review]:
-----------------------------------------------------------------

I didn't realize the controller thread would be the compositor thread! I thought it would be a new thread.

Does this mean that some of what we do in APZ for sharing data between the compositor thread and the controller thread (for example, making a copy of some things stored in the layer tree for access on the controller thread) is now technically unnecessary?

Comment 88

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #86)
> Now that we can use C++11 lambdas in our code

Actually, we can only use lambdas in 38 and later, but we want to uplift this patch to 37. Never mind then :)

Comment 89

[Botond Ballo [:botond]]

Would the changes to nsWindow in the Part 2 patch be more appropriate in Part 3?

Comment 90

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

(In reply to Botond Ballo [:botond] from comment #85)
> ::: gfx/layers/apz/src/APZCTreeManager.cpp
> @@ +137,5 @@
> >  void
> >  APZCTreeManager::SetAllowedTouchBehavior(uint64_t aInputBlockId,
> >                                           const nsTArray<TouchBehaviorFlags> &aValues)
> >  {
> > +  APZThreadUtils::AssertOnControllerThread();
> 
> InputQueue::SetAllowedTouchBehavior already asserts this. Why double-assert?

I was mostly just adding them locally to be able to trace all the APZCTM functions that have this requirement. I can remove this instance.

> @@ +691,5 @@
> >  APZCTreeManager::ProcessTouchInput(MultiTouchInput& aInput,
> >                                     ScrollableLayerGuid* aOutTargetGuid,
> >                                     uint64_t* aOutInputBlockId)
> >  {
> > +  APZThreadUtils::AssertOnControllerThread();
> 
> I think this assertion would make more sense in
> APZCTreeManager::ReceiveInputEvent(WidgetInputEvent), as the other overload
> of ReceiveInputEvent has it, and this is a helper function for both.

Sounds good, I'll move it over.

(In reply to Botond Ballo [:botond] from comment #86)
> 
> Does RenderFrameParent::NotifyInputEvent need to use RunOnControllerThread?

In theory, yes, but that function will go away entirely. All input should be entering the APZ from widget rather than in RenderFrameParent. I verified that on B2G this is the case now. Other platforms might still have some stray events routing through here, but once we turn on parent-process APZ those will be wrong and will need to be removed. (This goes hand in hand with increasing the number of events that early-return out of TabParent::MaybeForwardEventToRenderFrame).

> Now that we can use C++11 lambdas in our code, you may want to consider
> using one:
> 
> nsRefPtr<layers::APZCTreeManager> treeManager = GetApzcTreeManager();
> APZThreadUtils::RunOnControllerThread(NewRunnableFunction([=]() {
>   treeManager->SetTargetAPZC(aInputBlockId, aTargets);
> }));
> 
> (The purpose of the local variable is to avoid having the lambda capture the
> 'this' pointer (to the RenderFrameParent object), which it would if you
> called a method like GetApzcTreeManager() inside the lambda.)

That's pretty neat! I'm disappointed now that I can't use it just yet. Any other changes I can make that are upliftable to 37 to clean this up?

(In reply to Botond Ballo [:botond] from comment #87)
> I didn't realize the controller thread would be the compositor thread! I
> thought it would be a new thread.

Actually I wanted it to be the libui thread that we receive the input events on, but we can't easily post tasks to that thread which is a requirement of the controller thread. Mason had a hacky approach to do that in attachment 8446256 [details] [diff] [review] but I'd rather use the compositor thread for now and then revisit this in the future. Mason would also prefer it on the compositor thread for now since then we don't have to worry as much about racing with composition.

> Does this mean that some of what we do in APZ for sharing data between the
> compositor thread and the controller thread (for example, making a copy of
> some things stored in the layer tree for access on the controller thread) is
> now technically unnecessary?

For B2G right now, yes. However I would still like to in the long run switch the controller thread to be another thread. Even if we don't, platforms like Fennec are definitely not going to have the controller thread be the same as the compositor thread because that would involve running the compositor on the Java UI thread (which would jank the UI) or redispatching events from the Java UI thread to the compositor thread needlessly. So the machinery we put in place for the hit-testing tree is still needed, and makes it easy for us to decide on a controller thread on a per-platform basis.

(In reply to Botond Ballo [:botond] from comment #89)
> Would the changes to nsWindow in the Part 2 patch be more appropriate in
> Part 3?

Yeah, that makes sense. When I split those patches initially I was hoping to make part 3 trivial but it turned out to not be as I imagined. I'll split and merge those bits.

Comment 91

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

Attachment: Part 1 - Introduce APZThreadUtils

Updated, carrying r=botond

Comment 92

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

Attachment: Part 2 - Introduce APZThreadUtils::RunOnControllerThread (v2)

Comment 93

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

Attachment: Part 3 - Switch the B2G controller thread to the compositor thread (v2)

Comment 94

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #90)
> (In reply to Botond Ballo [:botond] from comment #86)
> > 
> > Does RenderFrameParent::NotifyInputEvent need to use RunOnControllerThread?
> 
> In theory, yes, but that function will go away entirely. All input should be
> entering the APZ from widget rather than in RenderFrameParent. I verified
> that on B2G this is the case now.

Can we assert to this effect?

> > Now that we can use C++11 lambdas in our code, you may want to consider
> > using one:
> > 
> > nsRefPtr<layers::APZCTreeManager> treeManager = GetApzcTreeManager();
> > APZThreadUtils::RunOnControllerThread(NewRunnableFunction([=]() {
> >   treeManager->SetTargetAPZC(aInputBlockId, aTargets);
> > }));
> > 
> > (The purpose of the local variable is to avoid having the lambda capture the
> > 'this' pointer (to the RenderFrameParent object), which it would if you
> > called a method like GetApzcTreeManager() inside the lambda.)
> 
> That's pretty neat! I'm disappointed now that I can't use it just yet. Any
> other changes I can make that are upliftable to 37 to clean this up?

The best I can think of is to add an overload to NewRunnableMethod that looks like this:

  template <class Return, class T, class Arg1, class Arg2>
  CancelableTask* NewRunnableMethod(T* object, const Arg1& arg1, const Arg2& arg2, Return (T::*method)(Arg1, Arg2));

and then call it like this:

  NewRunnableMethod<void>(GetApzcTreeManager(), aInputBlockId, aTargets, &APZCTreeManager::SetTargetAPZC);

This signature makes the following changes compared to the one we would call if we didn't have an overloaded function:

  - Instead of making the method pointer its own template parameter, we write out the
    method pointer type in terms of the class, parameter, and return types.

  - |arg1| and |arg2| now come before |method|.

  - We introduce a template parameter for the return type, which needs to be explicitly
    specified (hence the '<void>' in the call).

The idea is that *if* each of the component types in 'Return (T::*)(Arg1, Arg2)' have been deduced by the time the compiler processes that type, the compiler will deduce the overload of SetTargetAPZC that matches this signature. The reason we had to move |arg1| and |arg2| before |method| was so that |Arg1| and |Arg2| got deduced in time. The reason we have to explicitly specify |Return| is because there isn't any place we can deduce it from.

Up to you if you like this. We could also rename one of the overloads, or leave things as in the current patch. I'm OK with either.

Comment 95

[Botond Ballo [:botond]]

Comment on attachment 8556156 [details] [diff] [review]
Part 2 - Introduce APZThreadUtils::RunOnControllerThread (v2)

Review of attachment 8556156 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the assertion added in NotifyInputEvent.

Comment 96

[Botond Ballo [:botond]]

Comment on attachment 8556157 [details] [diff] [review]
Part 3 - Switch the B2G controller thread to the compositor thread (v2)

Review of attachment 8556157 [details] [diff] [review]:
-----------------------------------------------------------------

Aesthetics aside, this looks good to me :)

Comment 97

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

(In reply to Botond Ballo [:botond] from comment #94)
> > In theory, yes, but that function will go away entirely. All input should be
> > entering the APZ from widget rather than in RenderFrameParent. I verified
> > that on B2G this is the case now.
> 
> Can we assert to this effect?

We can, what kind of assert would you like to see? I was thinking that the assert inside APZCTreeManager::ReceiveInputEvent already took care of pretty much everything.

> The best I can think of is to add an overload to NewRunnableMethod that
> looks like this:
>
> [snip]
>
> Up to you if you like this. We could also rename one of the overloads, or
> leave things as in the current patch. I'm OK with either.

Hm, interesting. I'm not really sure I like this approach particularly since we should be able to switch to lambdas (which I find cleaner) going forward. I also don't want to uplift new versions of NewRunnableMethod to 37. Given that I'd rather leave things as-is and then switch to lambdas in the future.

Comment 98

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #97)
> (In reply to Botond Ballo [:botond] from comment #94)
> > > In theory, yes, but that function will go away entirely. All input should be
> > > entering the APZ from widget rather than in RenderFrameParent. I verified
> > > that on B2G this is the case now.
> > 
> > Can we assert to this effect?
> 
> We can, what kind of assert would you like to see? I was thinking that the
> assert inside APZCTreeManager::ReceiveInputEvent already took care of pretty
> much everything.

Oh, good point! Ignore that then :)

Comment 99

[Florian Bender]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #90)
> (In reply to Botond Ballo [:botond] from comment #87)
> > I didn't realize the controller thread would be the compositor thread! I
> > thought it would be a new thread.
> 
> Actually I wanted it to be the libui thread that we receive the input events
> on, but we can't easily post tasks to that thread which is a requirement of
> the controller thread. Mason had a hacky approach to do that in attachment
> 8446256 [details] [diff] [review] but I'd rather use the compositor thread
> for now and then revisit this in the future. Mason would also prefer it on
> the compositor thread for now since then we don't have to worry as much
> about racing with composition.


Can you please file a followup bug for this and post it here? Just to be sure this gets noted in Bugzilla.

Comment 100

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

(In reply to Florian Bender from comment #99)
> Can you please file a followup bug for this and post it here? Just to be
> sure this gets noted in Bugzilla.

Yup, I'll do that if mwu gives an r+ for the patch pending review. He might prefer just hacking up libui as part of this bug...

Comment 101

[Botond Ballo [:botond]]

Comment on attachment 8556156 [details] [diff] [review]
Part 2 - Introduce APZThreadUtils::RunOnControllerThread (v2)

Review of attachment 8556156 [details] [diff] [review]:
-----------------------------------------------------------------

The assumption:

  // On B2G the controller thread is the compositor thread, and this function
  // is always called from the libui thread or the main thread.

does not seem to hold. Stack trace:

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 2963.3009]
0xb470cf2e in mozilla::layers::APZThreadUtils::RunOnControllerThread (aTask=0xab584580) at ../../../gfx/layers/apz/util/APZThreadUtils.cpp:59
warning: Source file is more recent than executable.
59        MOZ_ASSERT(MessageLoop::current() != loop);
(gdb) i s
#0  0xb470cf2e in mozilla::layers::APZThreadUtils::RunOnControllerThread (aTask=0xab584580) at ../../../gfx/layers/apz/util/APZThreadUtils.cpp:59
#1  0xb509073e in mozilla::GeckoTouchDispatcher::NotifyVsync (aVsyncTimestamp=...) at ../../../widget/gonk/GeckoTouchDispatcher.cpp:106
#2  0xb4748ba6 in DispatchTouchEvents (aVsyncTimestamp=..., this=<optimized out>) at ../../../gfx/layers/ipc/CompositorParent.cpp:371
#3  mozilla::layers::CompositorVsyncObserver::Composite (this=0xae191000, aVsyncTimestamp=...) at ../../../gfx/layers/ipc/CompositorParent.cpp:341
#4  0xb4736a22 in DispatchToMethod<mozilla::layers::CompositorParent, void (mozilla::layers::CompositorParent::*)(mozilla::TimeStamp), mozilla::TimeStamp> (arg=..., method=
    (void (mozilla::layers::CompositorParent::*)(mozilla::layers::CompositorParent * const, mozilla::TimeStamp)) 0xb4748b29 <mozilla::layers::CompositorVsyncObserver::Composite(mozilla::TimeStamp)>, obj=<optimized out>) at ../../../ipc/chromium/src/base/tuple.h:393
#5  RunnableMethod<mozilla::layers::CompositorParent, void (mozilla::layers::CompositorParent::*)(mozilla::TimeStamp), Tuple1<mozilla::TimeStamp> >::Run (this=<optimized out>)
    at ../../../ipc/chromium/src/base/task.h:310
#6  0xb43ff0ec in MessageLoop::RunTask (this=0xb06c3cc0, task=0xab2430a0) at ../../../ipc/chromium/src/base/message_loop.cc:361
#7  0xb4402222 in MessageLoop::DeferOrRunPendingTask (this=<optimized out>, pending_task=...) at ../../../ipc/chromium/src/base/message_loop.cc:369
#8  0xb4403fd8 in DoWork (this=<optimized out>) at ../../../ipc/chromium/src/base/message_loop.cc:447
#9  MessageLoop::DoWork (this=0xb06c3cc0) at ../../../ipc/chromium/src/base/message_loop.cc:426
#10 0xb440015c in base::MessagePumpDefault::Run (this=0xb0999d80, delegate=0xb06c3cc0) at ../../../ipc/chromium/src/base/message_pump_default.cc:34
#11 0xb44000d8 in MessageLoop::RunInternal (this=this@entry=0xb06c3cc0) at ../../../ipc/chromium/src/base/message_loop.cc:233
#12 0xb44000f2 in RunHandler (this=0xb06c3cc0) at ../../../ipc/chromium/src/base/message_loop.cc:226
#13 MessageLoop::Run (this=this@entry=0xb06c3cc0) at ../../../ipc/chromium/src/base/message_loop.cc:200
#14 0xb4405700 in base::Thread::ThreadMain (this=0xb09c8040) at ../../../ipc/chromium/src/base/thread.cc:170
#15 0xb43f9280 in ThreadFunc (closure=<optimized out>) at ../../../ipc/chromium/src/base/platform_thread_posix.cc:39
#16 0xb6e5f22c in __thread_entry (func=0xb43f9279 <ThreadFunc(void*)>, arg=0xb09c8040, tls=0xb06c3dd0) at bionic/libc/bionic/pthread_create.cpp:105
#17 0xb6e5f3c4 in pthread_create (thread_out=0xb09c8048, attr=<optimized out>, start_routine=0xb43f9279 <ThreadFunc(void*)>, arg=0x78) at bionic/libc/bionic/pthread_create.cpp:224
#18 0x00000000 in ?? ()

Comment 102

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

Comment on attachment 8556157 [details] [diff] [review]
Part 3 - Switch the B2G controller thread to the compositor thread (v2)

This has rotted a bit, will post new version.

Comment 103

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

Attachment: Assertion failure

Need to fix this too.

Comment 104

[Botond Ballo [:botond]]

Comment on attachment 8556156 [details] [diff] [review]
Part 2 - Introduce APZThreadUtils::RunOnControllerThread (v2)

Review of attachment 8556156 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/apz/util/APZThreadUtils.cpp
@@ +61,5 @@
> +#else
> +  // On non-B2G platforms this is only ever called from the controller thread
> +  // itself.
> +  AssertOnControllerThread();
> +  aTask->Run();

On a different note, if we Run() a task without posting it to a message loop, aren't we responsible for deleting it?

Comment 105

[Botond Ballo [:botond]]

Attachment: Workaround for assertion in comment 101

This patch fixes the assertion in comment 101 for me locally, and allows me to continue developing bug 1127066 on top of this bug for now.

Comment 106

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

Attachment: Part 2 - Introduce APZThreadUtils::RunOnControllerThread (v3)

(In reply to Botond Ballo [:botond] from comment #104)
> On a different note, if we Run() a task without posting it to a message
> loop, aren't we responsible for deleting it?

Indeed, good catch! Updated patch to do that.

Comment 107

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

Attachment: Part 3 - Switch the B2G controller thread to the compositor thread (v3)

Updated part 3 to deal with the assertion.

Comment 108

[No-Jun Park [:njpark]]

I tried those 3 patches, and found some issues that I don't see in latest nightly:
- system messages, like the geolocation permission or crash notification messages do not accept UI input (i.e., I can't press the buttons
- Going to www.youtube.com from the browser, it crashes.
- In gallery app, when I tap the thumbnail of a picture, it shows black screen.

Not sure whether this is due to the patch, or the tip of master branch actually contains some other bugs at the time, but they all seem to be UI related (except the last one) so might worth taking a look.  let me know if you want me to perform specific steps.

Comment 109

[Martijn Wargers (dead)]

(In reply to No-Jun Park [:njpark] from comment #108)
> I tried those 3 patches, and found some issues that I don't see in latest
> nightly:
> - system messages, like the geolocation permission or crash notification

Related to bug 1130067?

> messages do not accept UI input (i.e., I can't press the buttons
> - Going to www.youtube.com from the browser, it crashes.

This looks like bug 1130086.
I guess the latest tinderbox builds contain these fixes?

Comment 110

[No-Jun Park [:njpark]]

(In reply to No-Jun Park [:njpark] from comment #108)
> - In gallery app, when I tap the thumbnail of a picture, it shows black
> screen.

And this one is Bug 1130403 it seems.  I'll be running another check on the latest nightly today just to make sure.

Comment 111

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

Thanks no-jun! It does look like all of those issues are unrelated to my patches.

I did some more local testing on a debug build and found an error though.

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #90)
> (In reply to Botond Ballo [:botond] from comment #86)
> > 
> > Does RenderFrameParent::NotifyInputEvent need to use RunOnControllerThread?
> 
> In theory, yes, but that function will go away entirely. All input should be
> entering the APZ from widget rather than in RenderFrameParent. I verified
> that on B2G this is the case now. Other platforms might still have some
> stray events routing through here, but once we turn on parent-process APZ
> those will be wrong and will need to be removed. (This goes hand in hand
> with increasing the number of events that early-return out of
> TabParent::MaybeForwardEventToRenderFrame).

Turns out this wasn't entirely accurate. I ran into a case where the mouse events dispatched from ChromeProcessController::HandleSingleTap were getting dispatched to the child process and therefore entering RenderFrameParent::NotifyInputEvent. This caused a crash on debug builds because of the AssertOnControllerThread violation. I'll attach another patch to remove this codepath entirely since I think it's safe to do at this point.

Comment 112

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

Try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=bad983ec228b is green.

Comment 113

[Michael Wu [:mwu]]

Comment on attachment 8560015 [details] [diff] [review]
Part 3 - Switch the B2G controller thread to the compositor thread (v3)

Review of attachment 8560015 [details] [diff] [review]:
-----------------------------------------------------------------

I don't have a good feel for the entire patch, but things seem mostly right locally. Don't think I'm gonna get any good insights by reading this again (for probably the 4th time) tomorrow, so r=me.

::: widget/gonk/GeckoTouchDispatcher.h
@@ +47,5 @@
>  
>  public:
>    GeckoTouchDispatcher();
>    void NotifyTouch(MultiTouchInput& aTouch, TimeStamp aEventTime);
> +  void DispatchTouchEvent(MultiTouchInput aMultiTouch);

Is this change intentional?

Comment 114

[Botond Ballo [:botond]]

Comment on attachment 8560015 [details] [diff] [review]
Part 3 - Switch the B2G controller thread to the compositor thread (v3)

Review of attachment 8560015 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/gonk/GeckoTouchDispatcher.h
@@ +47,5 @@
>  
>  public:
>    GeckoTouchDispatcher();
>    void NotifyTouch(MultiTouchInput& aTouch, TimeStamp aEventTime);
> +  void DispatchTouchEvent(MultiTouchInput aMultiTouch);

I believe so. The patch changes the way of invoking this method from direct invocation to being scheduled on the controller thread via NewRunnableMethod. Digging a bit in the implementation of NewRunnableMethod reveals that the when it runs the method, it passes all arguments by const reference [1], so the call wouldn't compile if the method took the argument by nonconst reference.

[1] https://dxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/tuple.h#392

Comment 115

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

Yup, botond is right, it was intentional for exactly that reason.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d805db38cd5f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e2b0f9037728
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5a35ca7c0adc

Comment 116

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d805db38cd5f
https://hg.mozilla.org/mozilla-central/rev/e2b0f9037728
https://hg.mozilla.org/mozilla-central/rev/5a35ca7c0adc

Comment 117

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

Comment on attachment 8556155 [details] [diff] [review]
Part 1 - Introduce APZThreadUtils

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): required for parent-process APZ (bug 950934) and input-thread (bug 930939) which are 2.2+ features. I'm tracking the full set of bugs that will need uplifting together at http://mzl.la/1zvKgmk; requesting approval on bugs individually but will wait until everything has approval before I uplift any of it.
User impact if declined: can't have parent-process APZ or input-thread
Testing completed: on master. some of these bugs have been baking for a while; others are more recent.
Risk to taking this patch (and alternatives if risky): there's likely some edge cases that we haven't run into yet and will be uncovered with further testing. Bug 1128887 is the only known issue that we don't yet have a fix for but I consider that relatively minor and something we can fix after uplifting everything else.
String or UUID changes made by this patch: none

Comment 118

[zhaoyang]

Hi Kartikaya Gupta，

   whether bug 930939 will land on 2.1&2.1s？

   Thanks a lot

Comment 119

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

No, 2.2 and later.  There is a large number of large changes involved in enabling this functionality, I would not recommend attempting somebody trying to uplift to 2.1.

Comment 120

[bhavana bajaj [:bajaj]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #117)
> Comment on attachment 8556155 [details] [diff] [review]
> Part 1 - Introduce APZThreadUtils
> 
> NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to
> better understand the B2G approval process and landings.
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): required for parent-process APZ
> (bug 950934) and input-thread (bug 930939) which are 2.2+ features. I'm
> tracking the full set of bugs that will need uplifting together at
> http://mzl.la/1zvKgmk; requesting approval on bugs individually but will
> wait until everything has approval before I uplift any of it.
> User impact if declined: can't have parent-process APZ or input-thread
> Testing completed: on master. some of these bugs have been baking for a
> while; others are more recent.
> Risk to taking this patch (and alternatives if risky): there's likely some
> edge cases that we haven't run into yet and will be uncovered with further
> testing. Bug 1128887 is the only known issue that we don't yet have a fix
> for but I consider that relatively minor and something we can fix after
> uplifting everything else.
> String or UUID changes made by this patch: none

Eric, once kats land this on 2.2 please make sure to verify some of the graphic tests cases ASAP and schedule a more extensive testing as well. These have baked on m-c and kats did address the fallouts we had, so hopefully once uplifted to the branch we do not run into major failure.

:kats, depending on when you land these, can you flag njpark for a graphics sanity run on 2.2, thanks!

Comment 121

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

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/1046c97bfd7d
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/7768b0596a5c
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/ee7bef814675

Comment 122

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

I landed this (long with the other bugs it depends on) on 2.2, so flagging njpark for a sanity run as per comment 120.

Comment 123

[No-Jun Park [:njpark]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #122)
> I landed this (long with the other bugs it depends on) on 2.2, so flagging
> njpark for a sanity run as per comment 120.

Hi Kats, I ran some sanity checks on 2.2, and found the following.  Maybe you can quickly check whether they are related to this patch?  I ni?ed milan since they could be graphics bugs.
https://bugzilla.mozilla.org/show_bug.cgi?id=1133966
https://bugzilla.mozilla.org/show_bug.cgi?id=1133961
https://bugzilla.mozilla.org/show_bug.cgi?id=1133955

Comment 124

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

Thanks, No-Jun! I'll take a look at them today or (more likely) tomorrow.