1049136 - Hook up touch event flow for APZ usage on Fennec

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Android, defect)

Description

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

After bug 1046344 I want to hook up the touch events so that the flow correctly using APZ on Fennec. The current flow with JavaPanZoomController is something like this:

1) LayerView receives input events
2) LayerMarginsAnimator gets a crack at intercepting them
3) JavaPanZoomController is given any non-intercepted events. Goes through the TouchEventHandler/GestureDetector/SimpleScaleGestureDetector code as well here
4) The events are given to the TouchEventInterceptor registered in LayerView, which posts them to the Gecko thread via GeckoAppShell.sendEventToGecko(GeckoEvent.createMotionEvent(...)).
5) On the Gecko thread, the C++ code makes a WidgetTouchEvent from the AndroidGeckoEvent and dispatches it to the window
6) The C++ code checks the prevent-default status and calls GeckoAppShell::NotifyDefaultPrevented accordingly

With the changes I have in mind, the APZ flow will co-exist with this Java flow, and look something like this:

1) 2) Same as above
3) NativePanZoomController gets the events, and creates a MultiTouchInput from the AndroidGeckoEvent object
4) NPZC sends the event to the APZ via APZCTreeManager::ReceiveInputEvent, gets back the untransformed event, wraps it back up in an AndroidGeckoEvent, and posts it to the Gecko thread
5) Same as above
6) The C++ code checks the prevent-default status and calls APZCTreeManager::ContentReceivedTouch accordingly. This has to happen on the UI thread, and will use some of the code introduced in bug 1046344.

In theory I could reuse the same code for 6 as well but I would have to take a ScrollableLayerGuid object and pass that through Java and back into C++ which seems silly, so I plan on ifdef'ing that code based on whether or not APZ is enabled.

Comment 1

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

Attachment: Part 1 (WIP) - Remove the TouchEventInterceptor usage in LayerView

Comment 2

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

Attachment: Part 2 (WIP) - Add a function convert MultiTouchInput to WidgetTouchEvent

Stealing this from one of Mason's patches on bug 970751. Leaving author as Mason although there's a few changes from his version (notably, the name, return value, and nsIWidget* arg).

Comment 3

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

Attachment: Part 3 (WIP) - Hook up touch events as described in comment 0

Comment 4

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

Attachment: Part 1 - Remove the TouchEventInterceptor usage in LayerView

Comment 5

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

Attachment: Part 2 - Add a function convert MultiTouchInput to WidgetTouchEvent

Comment 6

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

Attachment: Part 3 - Hook up touch events as described in comment 0

I'm a little unhappy with having to create the APZ_INPUT_EVENT type but I think it's the solution that reuses the most code with the java input handling codepaths. Eventually when we kill off that old codepath we can make this better.

Comment 7

[Michael Wu [:mwu]]

Comment on attachment 8468642 [details] [diff] [review]
Part 2 - Add a function convert MultiTouchInput to WidgetTouchEvent

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

::: widget/xpwidgets/InputData.cpp
@@ +94,5 @@
> +  case MULTITOUCH_CANCEL:
> +    touchType = NS_TOUCH_CANCEL;
> +    break;
> +  default:
> +    NS_WARNING("Did not assign a type to WidgetTouchEvent in MultiTouchInput");

Could we do something like MOZ_ASSERT_UNREACHABLE here? It seems like there's no good reason we should hit this.

Comment 8

[Wesley Johnston (:wesj)]

Comment on attachment 8468640 [details] [diff] [review]
Part 1 - Remove the TouchEventInterceptor usage in LayerView

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

::: mobile/android/base/gfx/LayerView.java
@@ +229,5 @@
>          }
>          if (mPanZoomController != null && mPanZoomController.onTouchEvent(event)) {
>              return true;
>          }
> +        if (sendEventToGecko(event)) {

Might as well just return this.

@@ +237,5 @@
>      }
>  
>      @Override
>      public boolean onHoverEvent(MotionEvent event) {
> +        if (sendEventToGecko(event)) {

Same here.

Comment 9

[Wesley Johnston (:wesj)]

Comment on attachment 8468644 [details] [diff] [review]
Part 3 - Hook up touch events as described in comment 0

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

::: mobile/android/base/gfx/NativePanZoomController.java
@@ +69,5 @@
>  
> +    public void notifyDefaultActionPrevented(boolean prevented) {
> +        // this should never get called; there is a different
> +        // codepath that notifies the APZ code of this
> +        throw new IllegalStateException();

Add a little message to the exception. Also, sentence case in the comment is nice.

::: widget/android/nsWindow.cpp
@@ +1032,5 @@
>      if (sLastWasDownEvent && !sDefaultPreventedNotified) {
>          // if this event is a down event, that means it's the start of a new block, and the
>          // previous block should not be default-prevented
>          bool defaultPrevented = isDownEvent ? false : preventDefaultActions;
> +#ifdef MOZ_ANDROID_APZ

I kinda wish we could key this off the event type and avoid the ifdef.

Comment 10

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

(In reply to Michael Wu [:mwu] from comment #7)
> Could we do something like MOZ_ASSERT_UNREACHABLE here? It seems like
> there's no good reason we should hit this.

Yup, done. I also changed the corresponding NS_WARNING in the MultiTouchEvent::MultiTouchEvent(const WidgetTouchEvent&) constructor.

(In reply to Wesley Johnston (:wesj) from comment #8)
> Might as well just return this.
> Same here.

Done.

(In reply to Wesley Johnston (:wesj) from comment #9)
> Add a little message to the exception. Also, sentence case in the comment is
> nice.

Done

> ::: widget/android/nsWindow.cpp
> > +#ifdef MOZ_ANDROID_APZ
> 
> I kinda wish we could key this off the event type and avoid the ifdef.

We can, actually. I made the change locally. However I do think the ifdef is a little nicer because it makes it more obvious what code can be deleted later, and prevents libxul bloat. On the other hand it doesn't catch compile errors, so I don't feel too strongly about this either way.

Comment 11

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

https://tbpl.mozilla.org/?tree=Try&rev=8cbc1ca188fc
https://tbpl.mozilla.org/?tree=Try&rev=f6adb8f7c5b1

Comment 12

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8468644 [details] [diff] [review]
Part 3 - Hook up touch events as described in comment 0

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

::: widget/android/APZCCallbackHandler.cpp
@@ +47,5 @@
> +            aGuid, aDefaultPrevented), 0);
> +        return;
> +    }
> +
> +    // This should be running on the Java UI thread

Can we make an assertion about that?

Comment 13

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

Attachment: Part 4 - Add assertion for java ui thread

Best I could do. If this is too icky I can just leave it out.

Comment 14

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/ebdecbd4b669
https://hg.mozilla.org/integration/mozilla-inbound/rev/cabf76d115b6
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ffa223f053f
https://hg.mozilla.org/integration/mozilla-inbound/rev/6432653640cb

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ebdecbd4b669
https://hg.mozilla.org/mozilla-central/rev/cabf76d115b6
https://hg.mozilla.org/mozilla-central/rev/4ffa223f053f
https://hg.mozilla.org/mozilla-central/rev/6432653640cb