1146024 - Fix up input routing for APZ on Fennec

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Android, defect)

Description

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

The touch input event routing for APZ on Fennec should end up pretty similar to what we are doing on B2G. Some of this is already done but some will need to be fixed up.

In the code as it exists on m-c right now (with --enable-android-apz), touch events will enter C++ code at [1], on the Java UI thread. That function sends the input event to APZ, and discards it if APZ says to do so. Otherwise, it creates an event of type APZ_INPUT_EVENT and dispatches it for processing on the main thread. The equivalent to this code on B2G lives at [2].

What happens next on Fennec is that the APZ_INPUT_EVENT event is processed at [3] and there's a bunch of stuff that happens with detecting prevent-default notifications inside the OnMultitouch function and so on. There's no need for this code to run anymore on APZ_INPUT_EVENT events (although for now we still want to keep it for MOTION_EVENT events). Instead, what we want to happen is similar to what happens on B2G at [4] - create a WidgetTouchEvent and call nsBaseWidget::ProcessUntransformedAPZEvent on it. That will take care of tracking all of the prevent-default state along with other notifications that need to be sent to the APZ. Existing code for APZ_INPUT_EVENT handling in OnMultitouch can be removed.

Note that ProcessUntransformedAPZEvent relies on APZThreadUtils::RunOnControllerThread [5] to make sure various tasks are run on the correct thread. This function will need updating for Fennec (add another #ifdef block) to run things on the Java UI thread (which is the "controller" thread on Fennec).

[1] http://mxr.mozilla.org/mozilla-central/source/widget/android/AndroidJNI.cpp?rev=e706eac29b22#912
[2] http://mxr.mozilla.org/mozilla-central/source/widget/gonk/nsWindow.cpp?rev=3d8fb4f958b5#282
[3] http://mxr.mozilla.org/mozilla-central/source/widget/android/nsWindow.cpp?rev=7e279cc6cb7a#842
[4] http://mxr.mozilla.org/mozilla-central/source/widget/gonk/nsWindow.cpp?rev=3d8fb4f958b5#309
[5] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/util/APZThreadUtils.cpp?rev=e2b0f9037728#53

Comment 1

[Danilo Cesar Lemes de Paula]

Attachment: wip_patch.patch

So, there was a change in the current code that it's not "only add another #ifdef block there" anymore.

AFAIR, the way to run tasks on the UI thread on Android is by calling AndroidBridge::PostTaskToUiThread().

The current APZCThreadUtils code was changed (https://bugzilla.mozilla.org/show_bug.cgi?id=1147681, part 1) to receive a MessageLoop instance and then call the PostTask method from it.
I'm not quite sure if I should  write a wrapper class that only pass the PostTask() call to androidbridge::posTasktoUiThread, still use the #ifdef or check if there's another way to find a messageLoop that posts to the UI thread on Android.

About the rest of the changes, I would like to check if that's what kats had in mind. I couldn't test it yet as the app keeps crashing in random places (I will check again tomorrow morning as MC can be broken for other reasons)

Comment 2

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

(In reply to Danilo Cesar Lemes de Paula from comment #1)
> So, there was a change in the current code that it's not "only add another
> #ifdef block there" anymore.

Ah, sorry, I should have warned you about that. For now you probably don't need to worry about implementing APZThreadUtils::RunOnControllerThread. The way it's set up I don't think it will ever get called on Android. However you will need to port bug 1151890 to Fennec as well, to avoid getting that same assertion in Fennec debug builds. It should be trivial to do, just override the function in widget/android/nsWindow.cpp.

> About the rest of the changes, I would like to check if that's what kats had
> in mind. I couldn't test it yet as the app keeps crashing in random places
> (I will check again tomorrow morning as MC can be broken for other reasons)

Yeah, it looks good. I'll post another comment from the splinter review tool in a sec.

Comment 3

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

Comment on attachment 8590063 [details] [diff] [review]
wip_patch.patch

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

::: widget/android/nsWindow.cpp
@@ +830,5 @@
> +
> +            if (target) {
> +                WidgetTouchEvent touchEvent = ae->MakeTouchEvent(target); // XXX: Do we want the topWindow 
> +                                                                          // TouchEvent, target's or
> +                                                                          // something else?

In practice, target should always be the same as the TopWindow() so using target here should be fine. To be precise we always want "target" here to be the same as the window on which CreateRootContentController() gets called. If you run into weird problems it would be worth checking for that here, and if it's not the case then something is wrong that we will need to fix.

@@ +837,5 @@
> +                nsEventStatus result;
> +
> +                // XXX: as this is an static method, we can't call directly from nsBaseWidget::
> +                // I think that the instance we want is the target, but I'm note really sure.
> +                target->ProcessUntransformedAPZEvent(&touchEvent, guid, inputBlockId, result);

Here instead of creating local variables for the guid and inputBlockId you should use ae->mApzGuid and ae->mApzInputBlockId.

Comment 4

[Danilo Cesar Lemes de Paula]

Attachment: bug1146024_apz_input.patch

Changed to current code to use TopWindow instead of looking for a target.
Also reimplemented ConfigureAPZControllerThread as suggested by Kats.

Comment 5

[Danilo Cesar Lemes de Paula]

Attachment: bug1146024_apz_input.patch

Defining nsEventStatus as nsEventStatus_eIgnore to avoid a possible undefined behavior.

Comment 6

[Danilo Cesar Lemes de Paula]

Attachment: bug1146024_apz_input.patch

Comment 7

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

Attachment: Patch

Danilo emailed an updated version of the patch to me and said there were issues with clicking and scrolling. I was able to debug it somewhat and I think this updated version fixes the problem. However at some point I started getting just blank white instead of rendered content so I'm not 100% sure this fixes it. Gecko logging seems to indicate it does, but I can't verify visually.

The root cause is that in Fennec we draw scrollbars in Java, and have set the xul scrollbars to be display:none in the Fennec content.css. This means that the code in nsGfxScrollFrame::WantAsyncScroll() always returns false because there's no scrollboxes. My updates to that function just bypass that if MOZ_ANDROID_APZ is set so that WantAsyncScroll() doesn't return false for things that would otherwise be scrollable.

Comment 8

[Danilo Cesar Lemes de Paula]

Attachment: bug1146024_apz_input.patch

Comment 9

[Danilo Cesar Lemes de Paula]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a1470422ab7c

Comment 10

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

Comment on attachment 8594861 [details] [diff] [review]
bug1146024_apz_input.patch

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

::: gfx/layers/apz/util/APZThreadUtils.cpp
@@ +53,5 @@
>  /*static*/ void
>  APZThreadUtils::RunOnControllerThread(Task* aTask)
>  {
> +// This is needed while nsWindow::ConfigureAPZControllerThread is not propper
> +// implemented.

Move this comment inside the ifdef

@@ +57,5 @@
> +// implemented.
> +#ifdef MOZ_ANDROID_APZ
> +  if (AndroidBridge::IsJavaUiThread()) {
> +      aTask->Run();
> +      delete aTask;

Use 2-space indents here

::: layout/generic/nsGfxScrollFrame.cpp
@@ +1073,5 @@
>      mOuter->PresContext()->ThemeChanged();
>    }
>  }
>  
> +#if defined(MOZ_B2G) || (defined(MOZ_WIDGET_ANDROID) && !defined(MOZ_ANDROID_APZ))

I'll file a follow-up bug to enable the gecko scrollbars in the MOZ_ANDROID_APZ configuration, which will let us get rid of this change. For now it's fine though.

::: widget/android/nsWindow.cpp
@@ +75,5 @@
>  // The dimensions of the current android view
>  static gfxIntSize gAndroidBounds = gfxIntSize(0, 0);
>  static gfxIntSize gAndroidScreenBounds;
>  
> +nsWindow *nsWindow::sAPZWindow = nullptr;

We can take this out now, it was useful for debugging but we don't need to check it in.

@@ +844,5 @@
> +            ALOG("AndroidGeckoEvent::APZ_INPUT_EVENT\n");
> +            win->UserActivity();
> +
> +            MOZ_ASSERT(win == sAPZWindow,
> +                       "APZ_INPUT_EVENT window is not the same from ConfigureAPZTreeManager");

Remove this assertion as well

Comment 11

[Danilo Cesar Lemes de Paula]

Attachment: bug_1146024_Fix_up_input_routing_for_APZ_on_Fennec.patch

r+ from latest review.

Comment 12

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

Comment on attachment 8594872 [details] [diff] [review]
bug_1146024_Fix_up_input_routing_for_APZ_on_Fennec.patch

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

::: widget/android/nsWindow.h
@@ +35,5 @@
>  {
>  private:
>      virtual ~nsWindow();
>  
> +    static nsWindow *sAPZWindow;

Don't forget this one

Comment 13

[Danilo Cesar Lemes de Paula]

Attachment: bug_1146024_Fix_up_input_routing_for_APZ_on_Fennec.patch

Thanks

Should I push it again to try server?

Comment 14

[Danilo Cesar Lemes de Paula]

any case: https://treeherder.mozilla.org/#/jobs?repo=try&revision=920fe9e4caee

Comment 15

[Danilo Cesar Lemes de Paula]

Kats, would you mind to add checkin-needed as keyworkd? I don't have the privs to do it.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/085956254fe4

Comment 17

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

^ landed it directly instead :)

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9bfb68d5230a

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/085956254fe4
https://hg.mozilla.org/mozilla-central/rev/9bfb68d5230a