Closed Bug 1146024 Opened 5 years ago Closed 5 years ago

Fix up input routing for APZ on Fennec

Categories

(Core :: Widget: Android, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: kats, Assigned: danilo.eu)

References

Details

Attachments

(2 files, 6 obsolete files)

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
Attached patch wip_patch.patch (obsolete) — Splinter Review
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)
(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 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.
Attached patch bug1146024_apz_input.patch (obsolete) — Splinter Review
Changed to current code to use TopWindow instead of looking for a target.
Also reimplemented ConfigureAPZControllerThread as suggested by Kats.
Attachment #8590063 - Attachment is obsolete: true
Attachment #8590810 - Flags: review?(bugmail.mozilla)
Attached patch bug1146024_apz_input.patch (obsolete) — Splinter Review
Defining nsEventStatus as nsEventStatus_eIgnore to avoid a possible undefined behavior.
Attachment #8590810 - Attachment is obsolete: true
Attachment #8590810 - Flags: review?(bugmail.mozilla)
Attachment #8590811 - Flags: review?(bugmail.mozilla)
Attached patch bug1146024_apz_input.patch (obsolete) — Splinter Review
Attachment #8590811 - Attachment is obsolete: true
Attachment #8590811 - Flags: review?(bugmail.mozilla)
Attached patch PatchSplinter Review
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.
Attached patch bug1146024_apz_input.patch (obsolete) — Splinter Review
Attachment #8591039 - Attachment is obsolete: true
Attachment #8594861 - Flags: review?(bugmail.mozilla)
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
Attachment #8594861 - Flags: review?(bugmail.mozilla) → review+
r+ from latest review.
Attachment #8594861 - Attachment is obsolete: true
Attachment #8594872 - Flags: review+
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
Thanks

Should I push it again to try server?
Attachment #8594872 - Attachment is obsolete: true
Attachment #8594876 - Flags: review+
Kats, would you mind to add checkin-needed as keyworkd? I don't have the privs to do it.
^ landed it directly instead :)
https://hg.mozilla.org/mozilla-central/rev/085956254fe4
https://hg.mozilla.org/mozilla-central/rev/9bfb68d5230a
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
No longer blocks: 1156392
Depends on: 1156392
Assignee: nobody → danilo.eu
You need to log in before you can comment on or make changes to this bug.