The widget/ backend for Gonk send mouse events as well as touch events for the main process. On such a device mouse events should happens only if there a click. This also makes some core interactions a bit slower as all touch move actions are also doing a mouse move action and are hits the fluffing code.

This patch just remove the widget/gonk support for mouse events, and send simulated mouse events from dom/browser-element/BrowserElementPanning.js. It probably needs to bake a little bit before asking for review though.

Comment on attachment 8417277 [details] [diff] [review] no.mouse.events.for.system.app.patch Flagging mwu as it's unclear to me if there is any reasons to keep those mouse events into the widget code.

I support this initiative! \o/

Comment on attachment 8417277 [details] [diff] [review] no.mouse.events.for.system.app.patch Sorry for the long response on this one. I think disabling all mouse events is fine for events that originate from the touchscreen. Unfortunately, we can't take all mouse event support away since that'll break the little actual mouse support we currently have. It's actually used when setting up barebones dev boards.

Going to start looking at this to advance the APZ-in-parent-process work.

As discussed with Kats, the general idea here to is implement ChromeProcessController::HandleSingleTap, and remove the code that currently generates mouse events from touch events going to the system process. Looking at the code that generates these mouse events, though, I see that it also generates them for child processes, at least in the case where we're not going through the TabParent::TryCapture path (so, at least for touch-start events). The justification for this is described as [1]: // Technically we should not need to do this if the touch event was routed // to the child process, but that seems to expose a bug in B2G where the // keyboard doesn't go away in some cases. Is this bug still around (and if so, does that mean we can't get rid of this code for child processes yet)? [1] http://mxr.mozilla.org/mozilla-central/source/widget/gonk/nsWindow.cpp?rev=8cae7aeb3a74#302

We should be able to get rid of that code. I investigated that further at some point and posted my findings to https://bugzilla.mozilla.org/show_bug.cgi?id=1116192#c1

For child processes, touch events go through TabChild as they are dispatched. In addition to dispatching them, TabChild observes them, and uses them to maintain some state (things like mTouchEndCancelled, and the state inside mActiveElementManager). In turn, it uses some of this state when handling single-taps. To handle single-taps in a functionally similar way in ChromeProcessController, am I correct to say that it, too, will need to observe touch events and maintain state based on them? If so, do you know what would be a good way to route the touch events to it?

As a strategy for reusing code between TabChild and ChromeProcessController, what do you think about extracting a WidgetHelper (or APZWidgetHelper, or something) class that would store an nsIWidget*, and moving the DispatchWidgetEvent(), DispatchSynthesizedMouseEvent(), and FireSingleTapEvent() methods (for starters) into it? I *think* the implementations of these methods are also applicable to a parent process widget, except perhaps for the capture check here [1], which I assume is for capturing by nested child processes; perhaps we can condition that on being in a child process? [1] http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp?rev=9ddc69b3693a#635

(In reply to Botond Ballo [:botond] from comment #8) > > To handle single-taps in a functionally similar way in > ChromeProcessController, am I correct to say that it, too, will need to > observe touch events and maintain state based on them? If so, do you know > what would be a good way to route the touch events to it? Yes it will need to do that. We can pass the information needed to it from the widget which created the ChromeProcessController and can keep a reference to it if it doesn't already. (In reply to Botond Ballo [:botond] from comment #9) > As a strategy for reusing code between TabChild and ChromeProcessController, > what do you think about extracting a WidgetHelper (or APZWidgetHelper, or > something) class that would store an nsIWidget*, and moving the > DispatchWidgetEvent(), DispatchSynthesizedMouseEvent(), and > FireSingleTapEvent() methods (for starters) into it? I *think* the > implementations of these methods are also applicable to a parent process > widget, except perhaps for the capture check here [1], which I assume is for > capturing by nested child processes; perhaps we can condition that on being > in a child process? My initial thoughts for code reuse here generally involved making more static helper functions in APZCCallbackHelper that could then be called from both TabChild and ChromeProcessController. However there might be a better organization for it; what you suggested sounds fine as well. The only thing I would like to make sure is that the shared code lives in apz/util. And yes the TryCapture code you linked to is for nested child processes but note that TryCapture only captures touch events so for the SingleTap mouse events that part would always be a no-op.

I have some initial patches that implement a basic version of ChromeProcessController::HandleSingleTap. It's basic in the sense that I haven't implemented passing ChromeProcessController information from touch events yet, so the bits that depend on such information (specifically, not generating a tap if the touch-end has been cancelled, and delaying the tap if the active element is visually affected by the :active style) aren't implemented yet. I wasn't able to meaningfully test these patches yet due to bug 1124452; I'll fix that next, and then carry on with this.

Comment on attachment 8552779 [details] [diff] [review] Part 2 - Store the widget in ChromeProcessController Moved this patch to bug 1124452.

Tested this with the patches in bug 1124452 applied, and it seems to be working pretty well! I'll implement the bits that rely on touch event state before putting the patches here up for review.

(In reply to Botond Ballo [:botond] from comment #18) > I'll implement the bits that rely on touch event state before putting the > patches here up for review. On second thought, those bits will need quite a bit of infrastructure from TabChild ported to the chrome process code, and I don't believe that functionality is implemented in the current code anyways, so I think we might as well get what's done so far reviewed and landed.

:smaug, could you have a look at this as well, with an eye for lifetime issues? I'm changing DelayedFireSingleTapEvent to store an nsWeakPtr to the TabChild's widget rather than the TabChild itself (and in FireSingleTapEvent(), checking whether the widget is Destroyed(), while previously we were checking if the TabChild was destroyed).

We can't actually land this until bug 950934 lands. Kats, do you know if bug 950934 is ready to land, or if it needs to wait for some ChromeProcessController infrastructure (such as that in Parts 1-3 of this bug, and others) to be implemented?

Comment on attachment 8553984 [details] [diff] [review] Part 1 - Store the main thread's MessageLoop in ChromeProcessController Review of attachment 8553984 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/apz/util/ChromeProcessController.cpp @@ +10,5 @@ > #include "nsView.h" > #include "nsIPresShell.h" > #include "nsIDocument.h" > +#include "MainThreadUtils.h" // for NS_IsMainThread() > +#include "base/message_loop.h" // for MessageLoop Insert these in alphabetical order

Comment on attachment 8553989 [details] [diff] [review] Part 2 - Extract TabChild::FireSingleTapEvent and its helpers into APZCCallbackHelper Review of attachment 8553989 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/apz/util/APZCCallbackHelper.cpp @@ +11,5 @@ > #include "nsIInterfaceRequestorUtils.h" > #include "nsIContent.h" > #include "nsIDocument.h" > #include "nsIDOMWindow.h" > +#include "mozilla/dom/TabParent.h" Insert in alphabetical order @@ +374,5 @@ > + > + // In a content process, a nested content process may be capturing events. > + // In a chrome process, the check for capture is done elsewhere (for gonk, > + // in nsWindow::DispatchTouchInputViaAPZ). > + if (XRE_GetProcessType() == GeckoProcessType_Content) { I don't think you need this check, as TryCapture will only return true for touch events, and no parent-process touch events will get routed through here. If you still want to keep this check, use !XRE_IsParentProcess().

Comment on attachment 8554007 [details] [diff] [review] Part 4 - Do not generate mouse events from touch events at the gonk widget layer Review of attachment 8554007 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Botond Ballo [:botond] from comment #23) > Kats, do you know if bug 950934 is ready to land, or if it needs to wait for > some ChromeProcessController infrastructure (such as that in Parts 1-3 of > this bug, and others) to be implemented? As long as it lands simultaneously with the stuff in this bug everything should be ok, I think. Worth doing some local testing with all the patches applied and making sure things like the notification tray, keyboard, rocketbar search, root process dialogs, etc. all work as expected.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #26) > (In reply to Botond Ballo [:botond] from comment #23) > > Kats, do you know if bug 950934 is ready to land, or if it needs to wait for > > some ChromeProcessController infrastructure (such as that in Parts 1-3 of > > this bug, and others) to be implemented? > > As long as it lands simultaneously with the stuff in this bug everything > should be ok, I think. What about ChromeProcessController bits that aren't implemented in this bug (e.g. long taps, double taps)?

(In reply to Botond Ballo [:botond] from comment #27) > What about ChromeProcessController bits that aren't implemented in this bug > (e.g. long taps, double taps)? We can file a follow-up for that. That stuff needs to be done, but that stuff never existed in the parent process before anyway (at least double-tap didn't) so it shouldn't be a regression.

Comment on attachment 8553984 [details] [diff] [review] Part 1 - Store the main thread's MessageLoop in ChromeProcessController Moved to bug 1124452. The version there addresses the review comments.

Updated to address review comments. Carrying r+. (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #25) > @@ +374,5 @@ > > + > > + // In a content process, a nested content process may be capturing events. > > + // In a chrome process, the check for capture is done elsewhere (for gonk, > > + // in nsWindow::DispatchTouchInputViaAPZ). > > + if (XRE_GetProcessType() == GeckoProcessType_Content) { > > I don't think you need this check, as TryCapture will only return true for > touch events, and no parent-process touch events will get routed through > here. If you still want to keep this check, use !XRE_IsParentProcess(). I asserted !XRE_IsParentProcess() if TryCapture() returns true instead.

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

(In reply to Botond Ballo [:botond] from comment #31) > Try push: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6c447a063ad Had Android bustage due to missing includes in APZCCallbackHelper.h. Fixed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=10afa1347bae

New Try push that includes a fix for failures caused by the dependent patches for bug 1124452: https://treeherder.mozilla.org/#/jobs?repo=try&revision=254129b325f5

Comment on attachment 8554007 [details] [diff] [review] Part 4 - Do not generate mouse events from touch events at the gonk widget layer Moved this patch to bug 950934, since it's tied to it pretty tightly.

(In reply to Botond Ballo [:botond] from comment #33) > New Try push that includes a fix for failures caused by the dependent > patches for bug 1124452: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=254129b325f5 Try push is clean, and bug 1124452 has landed, so we're good to go: https://hg.mozilla.org/integration/mozilla-inbound/rev/7ee5dfb97ff6 https://hg.mozilla.org/integration/mozilla-inbound/rev/0038cfaf847a

https://hg.mozilla.org/mozilla-central/rev/7ee5dfb97ff6 https://hg.mozilla.org/mozilla-central/rev/0038cfaf847a

Comment on attachment 8553990 [details] [diff] [review] Part 3 - Basic implementation of ChromeProcessController::HandleSingleTap 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

remote: https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/8598e63f2652 remote: https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/32a4b08d4464