Route all events through the APZCTreeManager on B2G

RESOLVED FIXED in Firefox OS v2.2

Status

()

Core
Widget: Gonk
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: kats, Assigned: kats)

Tracking

(Blocks: 1 bug, {perf})

Trunk
mozilla37
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(b2g-v2.2 fixed, b2g-master fixed)

Details

(Whiteboard: [c=handeye p=0 s=2014.08.15 u=])

Attachments

(2 attachments, 11 obsolete attachments)

6.83 KB, patch
mwu
: review+
Details | Diff | Splinter Review
11.60 KB, patch
mwu
: review+
smaug
: review+
dvander
: review+
Details | Diff | Splinter Review
+++ This bug was initially created as a clone of Bug #912657 +++

As part of the dynamic toolbar implementation for B2G, bug 835152 adds a scrollable <div> grouping the toolbar and the browser iframe. To get the desired behaviour, we need overscroll hand-off (bug 898478) to work between the iframe and the <div>. This means we need the <div> to have an APZC (bug 912657) and have input events wired up to be delivered to that APZC (this bug).

Currently the input events on the content process area in B2G go like so:
gonk/nsWindow -> gecko -> hit testing -> TabParent -> APZC -> child process

Input events on the root process area in B2G go through the first steps in that list, but then get sent directly to the frame/content element that the hit test finds.

What I would like to do is change the flow to this instead:
gonk/nsWindow -> APZC -> gecko -> hit testing -> child process

This will allow input events on the root process area to also go to the APZC so that we can do async panning for things in that process.

Note that with dzbarsky's work to move the browser into a child process, this may not actually be needed. The current set up will work fine for dynamic toolbar in the browser, if we move the browser into a separate process and turn on APZC in that process. However I think that for the long term it still makes sense to do this as it will result in a simpler overall flow that is consistent for both the root and child processes, and be more in line with what Fennec and Metro will be doing. Jim has already started the work to line up the Metro input events to the new model as part of bug 915213.
Summary: Create an APZC for the <div> that contains the toolbar and the browser iframe → Route all events through the APZCTreeManager on B2G
Assignee: nobody → bugmail.mozilla
Created attachment 809320 [details] [diff] [review]
(WIP) Part 1 - Update method signature
Created attachment 809321 [details] [diff] [review]
(WIP) Part 2 - Rip out capture fast-path
Created attachment 809408 [details] [diff] [review]
(WIP) Part 3 - Remove the two-parameter ReceiveInputEvent function now that it is redundant
Created attachment 809410 [details] [diff] [review]
(WIP) Part 4 - Move the APZC event routing to widget code
Created attachment 809411 [details] [diff] [review]
(WIP) Part 1 - Update method signature

Whoops, uploaded the wrong patch there.
Attachment #809320 - Attachment is obsolete: true

Comment 6

5 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #3)
> Created attachment 809408 [details] [diff] [review]
> (WIP) Part 3 - Remove the two-parameter ReceiveInputEvent function now that
> it is redundant

This great, although we'll have to come up with a better way to split off those mouse events based on widget backend. Here's the one param method in my landing - 

https://hg.mozilla.org/integration/mozilla-inbound/rev/401f078bbfe8

(I didn't include the special case since only metro was calling this.) What we can do here I think is something along the lines of:

#ifdef XP_WIN
default:
if (XRE_GetWindowsEnvironment() == WindowsEnvironmentType_Metro) {
  ProcessEvent()
} else {
  if (MouseEvent)
    ProcessMouseEvent()
  else
    ProcessEvent()
}
#else
  if (MouseEvent)
    ProcessMouseEvent()
  else
    ProcessEvent()
#endif

I can put together a patch for this on top of this work if you like.
That seems pretty complicated. I think a better approach would be to update the ReceiveInputEvent(InputData&) method to also untransform the passed-in InputData object, similar to your one-param version. And then instead of passing in an nsMouseEvent that gets converted to a MultiTouchInput, we can modify the B2G code to do the conversion at the call site and pass in a MultiTouchInput directly.

I can work this into my current patchset; I'll pull and rebase on top of your changes now that they've hit central and throw in a patch that does this as well.

Comment 8

5 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #7)
> That seems pretty complicated. I think a better approach would be to update
> the ReceiveInputEvent(InputData&) method to also untransform the passed-in
> InputData object, similar to your one-param version. And then instead of
> passing in an nsMouseEvent that gets converted to a MultiTouchInput, we can
> modify the B2G code to do the conversion at the call site and pass in a
> MultiTouchInput directly.

Sounds good to me. Just to confirm though we'll keep ReceiveInputEvent(nsInputEvent& aEvent) around yes? I'm currently working with this in handling wheel and content command events sent from MetroWidget.
(In reply to Jim Mathies [:jimm] from comment #8)
> Sounds good to me. Just to confirm though we'll keep
> ReceiveInputEvent(nsInputEvent& aEvent) around yes? I'm currently working
> with this in handling wheel and content command events sent from MetroWidget.

For now (and maybe forever), yes. I would like to move all input going into APZC off the gecko thread as much as I can, which would mean transitioning to the InputData version of the function but that's a much longer-term change. Even if we do that we'll probably keep the nsInputEvent& version since it doesn't hurt to have it.

Comment 10

5 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #9)
> (In reply to Jim Mathies [:jimm] from comment #8)
> > Sounds good to me. Just to confirm though we'll keep
> > ReceiveInputEvent(nsInputEvent& aEvent) around yes? I'm currently working
> > with this in handling wheel and content command events sent from MetroWidget.
> 
> For now (and maybe forever), yes. I would like to move all input going into
> APZC off the gecko thread as much as I can, which would mean transitioning
> to the InputData version of the function but that's a much longer-term
> change. Even if we do that we'll probably keep the nsInputEvent& version
> since it doesn't hurt to have it.

I can see how more generic storage would be useful particularly with the state WAITING_LISTENERS. For example wheel input has the same requirement that touch does in that content can cancel default actions. To address this I think mTouchQueue needs to become more generic and store InputData vs. MultiTouchInput. Sound reasonable?
(Just keep in mind that we should try to move to use pointer events, where touch-action let's
web page to opt-in to get the events, and by default events don't need to be dispatched to DOM.
Hopefully touch events will die at some point.)
Comment on attachment 809321 [details] [diff] [review]
(WIP) Part 2 - Rip out capture fast-path

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

Apparently this patch breaks stuff, even though it's supposed to be entirely a removable optimization. Dammit.
Blocks: 930939
Blocks: 950934
Assignee: bugmail.mozilla → nobody
Whiteboard: [TCP]
Assignee: nobody → mchang
Keywords: perf
Priority: -- → P1
Whiteboard: [TCP] → [c=effect p=4 s= u=] [TCP]
Note to self, look at patch 4 mostly.
Status: NEW → ASSIGNED
Whiteboard: [c=effect p=4 s= u=] [TCP] → [c=handeye p=4 s= u=] [TCP]
Whiteboard: [c=handeye p=4 s= u=] [TCP] → [c=handeye p=4 s= u=]
Since all the work I've been doing is in bug 930939, and there we route all touch events through APZCTreeManager, marking this as a dupe.
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → DUPLICATE
Whiteboard: [c=handeye p=4 s= u=] → [c=handeye p=0 s=2014.08.15 u=]
Duplicate of bug: 930939
Depends on: 918288
Un-duping as I believe this will need to be fixed before we can turn on parent-process APZ, which in turn will be needed before we can move input events off the main thread.
Assignee: mchang → nobody
Status: RESOLVED → REOPENED
Priority: P1 → --
Resolution: DUPLICATE → ---
Depends on: 1110540
Created attachment 8536252 [details] [diff] [review]
Part 1 (WIP) - Introduce InputEventContext
Attachment #809321 - Attachment is obsolete: true
Attachment #809408 - Attachment is obsolete: true
Attachment #809410 - Attachment is obsolete: true
Attachment #809411 - Attachment is obsolete: true
Created attachment 8536253 [details] [diff] [review]
Part 2 (WIP) - Add a layersId -> TabParent mapping
Created attachment 8536254 [details] [diff] [review]
Part 3 (WIP) - Split SendRealTouchEvent into two pieces
Created attachment 8536255 [details] [diff] [review]
Part 4 (WIP) - Move ToWidgetMouseEvent into MultiTouchInput
Created attachment 8536257 [details] [diff] [review]
Part 5 (WIP) - Reroute input events through APZ in widget

These patches mostly do the job. However there are issues with the event regions either not being respected or not being computed properly. If you bring down the notification tray, for example, the input events will still go through it to the content underneath. In this case I think the problem is the touch-event regions are not being respected properly. There's also the issue where the touch listeners in gaia/apps/system/js/edge_swipe_detector.js don't seem to generate touch-event regions in the layer tree, meaning that the edge swipe stuff doesn't work. :dvander convinced me that conceptually it should work if we iron out all the bugs.
Comment on attachment 8536252 [details] [diff] [review]
Part 1 (WIP) - Introduce InputEventContext

A variant of this patch landed as part of bug 1109985.
Attachment #8536252 - Attachment is obsolete: true
Comment on attachment 8536253 [details] [diff] [review]
Part 2 (WIP) - Add a layersId -> TabParent mapping

A variant of this landed as part of bug 1111873.
Attachment #8536253 - Attachment is obsolete: true
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #20)
> If you
> bring down the notification tray, for example, the input events will still
> go through it to the content underneath. In this case I think the problem is
> the touch-event regions are not being respected properly.

This is a bug in the APZC code, which attachment 8540882 [details] [diff] [review] fixes.
Depends on: 1115111
Depends on: 1116192
No longer depends on: 1116192
Created attachment 8542237 [details] [diff] [review]
Part 1 - Move ToWidgetMouseEvent into MultiTouchInput
Assignee: nobody → bugmail.mozilla
Attachment #8536255 - Attachment is obsolete: true
Attachment #8542237 - Flags: review?(mwu)
Created attachment 8542238 [details] [diff] [review]
Part 2 - Send touch inputs through APZ before sending them through gecko

Not sure who all needs to review this so flagging everybody who might be interested. Feel free to unflag yourself.
Attachment #8536257 - Attachment is obsolete: true
Attachment #8542238 - Flags: review?(mwu)
Attachment #8542238 - Flags: review?(dvander)
Attachment #8542238 - Flags: review?(bugs)
Created attachment 8542239 [details] [diff] [review]
Part 3 (WIP) - Direct-dispatch events to child process (optimization)

This code is #if 0 because it doesn't work yet. Conceptually it should, I think, but I need to iron out other bugs first. Just stashing it as a WIP here for now.
Attachment #8536254 - Attachment is obsolete: true
Comment on attachment 8542239 [details] [diff] [review]
Part 3 (WIP) - Direct-dispatch events to child process (optimization)

Just making it clear that this part 3 is meant to be an optimization and we should be fine without it as well. Landing parts 1 and 2 is dependent on turning on event-regions on B2G (bug 922833) as well as some APZ fixes (bug 1109873 and bug 1115111).
Attachment #8542239 - Attachment description: Part 3 (WIP) - Direct-dispatch events to child process → Part 3 (WIP) - Direct-dispatch events to child process (optimization)
Depends on: 928833
Blocks: 1116579
Comment on attachment 8542239 [details] [diff] [review]
Part 3 (WIP) - Direct-dispatch events to child process (optimization)

Moving this patch to bug 1116579 since there's other pieces that are blocking it, and we don't need it to complete this bug.
Attachment #8542239 - Attachment is obsolete: true
Comment on attachment 8542238 [details] [diff] [review]
Part 2 - Send touch inputs through APZ before sending them through gecko

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

lgtm
Attachment #8542238 - Flags: review?(dvander) → review+

Updated

4 years ago
Attachment #8542237 - Flags: review?(mwu) → review+

Comment 30

4 years ago
Comment on attachment 8542238 [details] [diff] [review]
Part 2 - Send touch inputs through APZ before sending them through gecko

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

::: widget/gonk/nsWindow.cpp
@@ +259,5 @@
> +
> +void
> +nsWindow::DispatchTouchInputViaAPZ(MultiTouchInput& aInput)
> +{
> +    if (!mAPZC) {

Hmmm... is this allowed to be null? If not, should be an assert. If it can be, it seems like it can do better than dropping the event. Or maybe this is just in case we receive events while the window is initializing?
(In reply to Michael Wu [:mwu] from comment #30)
> Hmmm... is this allowed to be null? If not, should be an assert. If it can
> be, it seems like it can do better than dropping the event. Or maybe this is
> just in case we receive events while the window is initializing?

In general it shouldn't be null, but you're right, it might be null during initial setup (before the compositor is up). Do we care about that case? I can add a comment there if that helps.

Comment 32

4 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #31)
> (In reply to Michael Wu [:mwu] from comment #30)
> > Hmmm... is this allowed to be null? If not, should be an assert. If it can
> > be, it seems like it can do better than dropping the event. Or maybe this is
> > just in case we receive events while the window is initializing?
> 
> In general it shouldn't be null, but you're right, it might be null during
> initial setup (before the compositor is up). Do we care about that case? I
> can add a comment there if that helps.

Sure, a comment would be fine.

Comment 33

4 years ago
Comment on attachment 8542238 [details] [diff] [review]
Part 2 - Send touch inputs through APZ before sending them through gecko

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

r=me for the widget/gonk parts.
Attachment #8542238 - Flags: review?(mwu) → review+
Depends on: 1119497
Comment on attachment 8542238 [details] [diff] [review]
Part 2 - Send touch inputs through APZ before sending them through gecko


>-    nsEventStatus status;
>-    gFocusedWindow->DispatchEvent(&aEvent, status);
>-    return status;
>+    // If it didn't get captured, dispatch the event into the gecko root process
>+    // for "normal" flow. The event might get sent to the child process still,
>+    // but if it doesn't we need to notify the APZ of various things. All of
>+    // that happens in DispatchEventForAPZ
>+    rv = DispatchEventForAPZ(&event, guid, inputBlockId);
Odd name for the method.
It is DispatchEvent + something else... but ok, that is what we have already.
Attachment #8542238 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/22f8aee2e0e0
https://hg.mozilla.org/mozilla-central/rev/8cae7aeb3a74
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Bug 1119497 caused the smoketest regression in bug 1121033.
This bug depended on bug 1119497.
So I backed out cset 8cae7aeb3a74 from this bug along with bug 1119497 to fix the regression.

https://hg.mozilla.org/integration/b2g-inbound/rev/86bcc35413b4
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 1121353
status-b2g-v2.2: --- → fixed
status-b2g-master: --- → affected
https://hg.mozilla.org/mozilla-central/rev/380b784bf8ec
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: mozilla37 → mozilla38
status-b2g-master: affected → fixed
Target Milestone: mozilla38 → mozilla37
Depends on: 1125422
Looks like the last patch on this bug breaks the screen reader.
We do preventDefault() from the top level window with the intention of capturing and redirecting all touch input. Unfortunately, after this patch landed, child remote iframes get the events nonetheless. Don't know enough about this code to know why this is.
Flags: needinfo?(bugmail.mozilla)
(In reply to Eitan Isaacson [:eeejay] from comment #40)
> Looks like the last patch on this bug breaks the screen reader.
> We do preventDefault() from the top level window with the intention of
> capturing and redirecting all touch input. Unfortunately, after this patch
> landed, child remote iframes get the events nonetheless. Don't know enough
> about this code to know why this is.

Let's take the discussion to bug 1125422.
Flags: needinfo?(bugmail.mozilla)
Depends on: 1123599
Depends on: 1128887
You need to log in before you can comment on or make changes to this bug.