1046344 - Unbitrot APZ glue code for Fennec

Status: RESOLVED FIXED

(Core :: Widget: Android, defect)

Description

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

The glue code that I wrote eons ago for getting APZ working with Fennec has fallen into a state of severe disrepair. It needs to be cleaned up and gotten working again as a prerequisite for bug 776030.

Comment 1

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

Attachment: Part 1 (WIP) - General unbitrotting

Comment 2

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

Attachment: Part 2 (WIP) - Some touch event handling

Comment 3

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

Attachment: Part 3 (WIP) - GetRootZoomConstraints hookup

Comment 4

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

Attachment: Part 1 - Add a compile-time flag to enable APZ

Comment 5

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

Attachment: Part 2 - General unbitrotting

This is mostly just pushing code around. Some of the function implementations I updated as well.

Comment 6

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

Comment on attachment 8464968 [details] [diff] [review]
Part 2 (WIP) - Some touch event handling

These other two patches need some more work so I'll move them to another bug.

Comment 7

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

Attachment: Part 3 - Sprinkle some more MOZ_ANDROID_APZ in places

Comment 8

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

Comment on attachment 8467832 [details] [diff] [review]
Part 2 - General unbitrotting

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

::: widget/android/APZCCallbackHandler.cpp
@@ +137,5 @@
> +APZCCallbackHandler::SendAsyncScrollDOMEvent(bool aIsRoot,
> +                                             const CSSRect& aContentRect,
> +                                             const CSSSize& aScrollableSize)
> +{
> +    // no-op, we don't want to support this event on fennec

What is this and why don't we want to support it?

@@ +162,5 @@
> +    if (i == 0) {
> +        // if we're inserting it at the head of the queue, notify Java because
> +        // we need to get a callback at an earlier time than the last scheduled
> +        // callback
> +        mNativePanZoomController->PostDelayedCallbackWrapper((int64_t)aDelayMs);

Will this cancel/replace the existing callback?

@@ +167,5 @@
> +    }
> +}
> +
> +int64_t
> +APZCCallbackHandler::RunDelayedTasks()

It seems like all of this task stuff should be abstracted out into some kind of DelayedTaskQueue type of thing that is separate from APZC.

::: widget/android/AndroidBridge.cpp
@@ +45,5 @@
>  using namespace mozilla;
>  using namespace mozilla::widget::android;
>  using namespace mozilla::gfx;
>  
> +AndroidBridge* AndroidBridge::sBridge;

Why do we need to do this?

Comment 9

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

(In reply to James Willcox (:snorp) ([e-mail]) from comment #8)
> > +    // no-op, we don't want to support this event on fennec
> 
> What is this and why don't we want to support it?

It's basically the same as the scroll event. It gets fired about as often while the user is scrolling around. We want to get rid of it entirely (bug 898075) because it provides no advantage over just listening for the scroll event. We can't just yet because some B2G code uses it. I can update the comment to point to bug 898075.

> @@ +162,5 @@
> > +    if (i == 0) {
> > +        // if we're inserting it at the head of the queue, notify Java because
> > +        // we need to get a callback at an earlier time than the last scheduled
> > +        // callback
> > +        mNativePanZoomController->PostDelayedCallbackWrapper((int64_t)aDelayMs);
> 
> Will this cancel/replace the existing callback?

No. So if this called twice, with the second task having an earlier run-at time than the first task, there will be two callbacks posted in the java-side code, and they will both run. The first one will probably schedule yet another one (see [1]) but whichever runs last will be a no-op because it will find no tasks waiting to be run. I can add code to cancel the existing callback but it seems like a bunch of complexity for not much benefit. It doesn't even allocate more memory because the same CallbackRunnable instance is used on the java side.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/gfx/NativePanZoomController.java?rev=36853e536d68#100

> @@ +167,5 @@
> > +int64_t
> > +APZCCallbackHandler::RunDelayedTasks()
> 
> It seems like all of this task stuff should be abstracted out into some kind
> of DelayedTaskQueue type of thing that is separate from APZC.

It is separate from APZC, but it requires going back and forth across the JNI barrier into java code. I might be able to abstract it out I guess but it won't really be a general-purpose thing since it's only useful for posting tasks from C++ running on the android UI thread.

> ::: widget/android/AndroidBridge.cpp
> > +AndroidBridge* AndroidBridge::sBridge;
> 
> Why do we need to do this?

Now that AndroidBridge no longer extends from GeckoContentController it no longer implements AddRef() and friends, so we can't keep a RefPtr to it. This restores the code to the state it was in before I landed bug 839641.

Comment 10

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

Attachment: Part 4 (will be folded into part 2) - Move the RunDelayedTasks stuff out of NativePanZoomController

I moved the code out into GeckoAppShell. I'll squash this into part 2 on landing, just keeping it separate for easier review.

Comment 11

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

Try push at https://tbpl.mozilla.org/?tree=Try&rev=654e95fa1c09

Comment 12

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

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

Comment 13

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

Comment on attachment 8468000 [details] [diff] [review]
Part 4 (will be folded into part 2) - Move the RunDelayedTasks stuff out of NativePanZoomController

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

Thanks, looks good

Comment 14

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/9ff86c2ca064
https://hg.mozilla.org/integration/mozilla-inbound/rev/3cb6a9651e0a
https://hg.mozilla.org/integration/mozilla-inbound/rev/fcc3d8857f00

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9ff86c2ca064
https://hg.mozilla.org/mozilla-central/rev/3cb6a9651e0a
https://hg.mozilla.org/mozilla-central/rev/fcc3d8857f00