Unbitrot APZ glue code for Fennec

RESOLVED FIXED in mozilla34

Status

()

Product:
Core
Component:
Widget: Android
Type:
defect
Status:
RESOLVED FIXED
Opened:
5 years ago
Updated:
5 years ago

People

(Reporter: kats, Assigned: kats)

Tracking

Version:
Trunk
Target:
mozilla34
Platform:
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 3 obsolete attachments)

Part 1 - Add a compile-time flag to enable APZ
5.33 KB, patch
snorp
: review+
Details | Diff | Splinter Review
Part 2 - General unbitrotting
28.98 KB, patch
snorp
: review+
Details | Diff | Splinter Review
Part 3 - Sprinkle some more MOZ_ANDROID_APZ in places
2.65 KB, patch
snorp
: review+
Details | Diff | Splinter Review
Part 4 (will be folded into part 2) - Move the RunDelayedTasks stuff out of NativePanZoomController
30.71 KB, patch
snorp
: review+
Details | Diff | Splinter Review 
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.
This is mostly just pushing code around. Some of the function implementations I updated as well.
Attachment #8464967 - Attachment is obsolete: true
Attachment #8467832 - Flags: review?(snorp)
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.
Attachment #8464968 - Attachment is obsolete: true
Attachment #8467830 - Flags: review?(snorp) → review+
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?
Attachment #8467832 - Flags: review?(snorp) → review+
Attachment #8467835 - Flags: review?(snorp) → review+
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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.
I moved the code out into GeckoAppShell. I'll squash this into part 2 on landing, just keeping it separate for easier review.
Attachment #8468000 - Flags: review?(snorp)
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
Attachment #8468000 - Flags: review?(snorp) → review+
You need to log in before you can comment on or make changes to this bug.