Closed Bug 1049136 Opened 5 years ago Closed 5 years ago

Hook up touch event flow for APZ usage on Fennec

Categories

(Core :: Widget: Android, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(4 files, 3 obsolete files)

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.
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).
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.
Attachment #8468036 - Attachment is obsolete: true
Attachment #8468644 - Flags: review?(wjohnston)
Attachment #8468644 - Flags: review?(snorp)
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.
Attachment #8468642 - Flags: review?(mwu) → review+
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.
Attachment #8468640 - Flags: review?(wjohnston) → review+
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.
Attachment #8468644 - Flags: review?(wjohnston) → review+
(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 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?
Attachment #8468644 - Flags: review?(snorp) → review+
Best I could do. If this is too icky I can just leave it out.
Attachment #8470288 - Flags: review?(snorp)
Attachment #8470288 - Flags: review?(snorp) → review+
You need to log in before you can comment on or make changes to this bug.