Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) Reporter
	Description • 10 years ago

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.

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) Reporter
	Comment 1 • 10 years ago

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.

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) Reporter
	Comment 2 • 10 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 3 • 10 years ago

I support this initiative! \o/

	Michael Wu [:mwu]
	Comment 4 • 10 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 5 • 9 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 6 • 9 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 7 • 9 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 8 • 9 years ago

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?

	Botond Ballo [:botond] Assignee
	Comment 9 • 9 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 10 • 9 years ago

(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.

	Botond Ballo [:botond] Assignee
	Comment 11 • 9 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 17 • 9 years ago

Comment on attachment 8552779 [details] [diff] [review]
Part 2 - Store the widget in ChromeProcessController

Moved this patch to bug 1124452.

	Botond Ballo [:botond] Assignee
	Comment 18 • 9 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 19 • 9 years ago

(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.

	Botond Ballo [:botond] Assignee
	Comment 21 • 9 years ago

: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).

	Botond Ballo [:botond] Assignee
	Comment 23 • 9 years ago

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?

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 24 • 9 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 25 • 9 years ago

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().

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 26 • 9 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 27 • 9 years ago

(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)?

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 28 • 9 years ago

(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.

	Botond Ballo [:botond] Assignee
	Comment 29 • 9 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 30 • 9 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 31 • 9 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 32 • 9 years ago

(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

	Botond Ballo [:botond] Assignee
	Comment 33 • 9 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 34 • 9 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 35 • 9 years ago

(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

	Phil Ringnalda (:philor)
	Comment 36 • 9 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 37 • 9 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 38 • 9 years ago

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