Wire up the AsyncPanZoomController to be usable from java

RESOLVED FIXED in Firefox 23

Status

()

Firefox for Android
Toolbar
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: kats, Assigned: kats)

Tracking

unspecified
Firefox 23
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 7 obsolete attachments)

23.67 KB, patch
kats
: review+
Details | Diff | Splinter Review
11.26 KB, patch
kats
: review+
Details | Diff | Splinter Review
3.23 KB, patch
kats
: review+
Details | Diff | Splinter Review
21.05 KB, patch
cwiiis
: review+
Details | Diff | Splinter Review
3.18 KB, patch
kats
: review+
Details | Diff | Splinter Review
13.13 KB, patch
kats
: review+
Details | Diff | Splinter Review
15.58 KB, patch
kats
: review+
Details | Diff | Splinter Review
With bug 777468 out of the way, we now need an implementation of the PanZoomController interface that delegates to an instance of the AsyncPanZoomController.
Depends on: 840721
Created attachment 739196 [details] [diff] [review]
Part 1 - Add a stub NativePanZoomController in java

The FIXME functions will be filled in subsequent patches.
Attachment #739196 - Flags: review?(chrislord.net)
Created attachment 739197 [details] [diff] [review]
Part 2 - Forward touch events to APZC
Attachment #739197 - Flags: review?(chrislord.net)
Attachment #739197 - Flags: feedback?(wjohnston)
Created attachment 739199 [details] [diff] [review]
Part 3 - (Cleanup) add mozilla::layers namespace to nsWindow

This just makes the code a bit easier to read.
Attachment #739199 - Flags: review?(chrislord.net)
Created attachment 739200 [details] [diff] [review]
Part 4 - Add a syncFrameMetrics code path between APZC <-> java

This is the combination of SetFirstPaintViewport, PageRectUpdated, and SyncViewportInfo, all rolled into one, using the APZC codepath. r? BenWa for the gfx/ bits and r? Cwiiis for the rest.
Attachment #739200 - Flags: review?(chrislord.net)
Attachment #739200 - Flags: review?(bgirard)
Created attachment 739201 [details] [diff] [review]
Part 5 - Handle APZC-reported gestures
Attachment #739201 - Flags: review?(chrislord.net)
Created attachment 739202 [details] [diff] [review]
Part 6 - Implement RequestContentRepaint callback
Attachment #739202 - Flags: review?(chrislord.net)
Created attachment 739203 [details] [diff] [review]
Part 7 - Implement the PostDelayedTask callback
Attachment #739203 - Flags: review?(chrislord.net)
Attachment #739203 - Flags: review?(blassey.bugs)
Try build in progress at https://tbpl.mozilla.org/?tree=Try&rev=296a113c8462

These patches, combined with a fix for bug 860162 (which I have yet to figure out how to fix for Fennec without breaking B2G), get the Fennec support for APZC to a barely-usable state. By "barely-usable" I mean you can pan and zoom content, click, and long-press on things. However the experience is pretty bad, because the panning/zooming physics in APZC is much jerkier, and often clicks/long-presses get dropped entirely. I don't know why this is, but it will take some work to figure out.

Note that all this code adds support for APZC but doesn't actually start using it; I will add a patch to bug 776030 that allows the user to switch to APZC via a pref. Actually turning it on by default will take much longer, I think.
Comment on attachment 739200 [details] [diff] [review]
Part 4 - Add a syncFrameMetrics code path between APZC <-> java

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

::: mobile/android/base/gfx/GeckoLayerClient.java
@@ +881,5 @@
>  
>      /*
>       * You must hold the monitor while calling this.
>       */
> +    private void setViewportMetrics(ImmutableViewportMetrics metrics, boolean notifyGecko, boolean render) {

enum please.
Attachment #739200 - Flags: review?(bgirard) → review+
Comment on attachment 739196 [details] [diff] [review]
Part 1 - Add a stub NativePanZoomController in java

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

LGTM, would like to know the answer to my comment below.

::: widget/android/AndroidBridge.cpp
@@ +44,5 @@
>  using namespace mozilla;
>  
>  NS_IMPL_THREADSAFE_ISUPPORTS0(nsFilePickerCallback)
>  
> +nsRefPtr<AndroidBridge> AndroidBridge::sBridge = nullptr;

This seems kind of unrelated - is there a reason for switching this in this patch?
Attachment #739196 - Flags: review?(chrislord.net) → review+
Comment on attachment 739197 [details] [diff] [review]
Part 2 - Forward touch events to APZC

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

LGTM with two comments addressed/justified.

::: mobile/android/base/GeckoEvent.java
@@ +328,5 @@
>              return null;
>          }
>      }
>  
> +    public static GeckoEvent createMotionEvent(MotionEvent m, boolean keepInViewCoordinates) {

I think it's worth documenting this function, to explain what keepInViewCoordinates does and why you'd want to do it.

::: widget/android/AndroidJNI.cpp
@@ +899,5 @@
> +    AsyncPanZoomController *controller = nsWindow::GetPanZoomController();
> +    if (controller) {
> +        AndroidGeckoEvent* wrapper = AndroidGeckoEvent::MakeFromJavaObject(env, event);
> +        const MultiTouchInput& input = wrapper->MakeMultiTouchInput(nsWindow::TopWindow());
> +        delete wrapper;

This seems a bit of a wasteful thing to do for every event - can we not reuse a wrapper?
Attachment #739197 - Flags: review?(chrislord.net) → review+
Comment on attachment 739199 [details] [diff] [review]
Part 3 - (Cleanup) add mozilla::layers namespace to nsWindow

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

Yes, that is nicer.
Attachment #739199 - Flags: review?(chrislord.net) → review+
Comment on attachment 739201 [details] [diff] [review]
Part 5 - Handle APZC-reported gestures

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

LGTM, but you may want to get a second opinion on the string functions you use (I'm no expert).
Attachment #739201 - Flags: review?(chrislord.net) → review+
Comment on attachment 739202 [details] [diff] [review]
Part 6 - Implement RequestContentRepaint callback

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

LGTM.
Attachment #739202 - Flags: review?(chrislord.net) → review+
Comment on attachment 739200 [details] [diff] [review]
Part 4 - Add a syncFrameMetrics code path between APZC <-> java

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

r- for the code duplication and commenting in GLC.

::: mobile/android/base/gfx/GeckoLayerClient.java
@@ +713,5 @@
>          return mCurrentViewTransform;
>      }
>  
> +    /* Invoked by JNI from the compositor thread */
> +    public ViewTransform syncFrameMetrics(float offsetX, float offsetY, float zoom,

This seems to be a copy of setFirstPaintViewport and syncViewportInfo? These need to be factored into sensible separate functions and the code shared - also, what this function does needs documenting.
Attachment #739200 - Flags: review?(chrislord.net) → review-
Comment on attachment 739203 [details] [diff] [review]
Part 7 - Implement the PostDelayedTask callback

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

LGTM.

::: widget/android/AndroidBridge.cpp
@@ +2657,5 @@
> +    // add the new task into the mDelayedTaskQueue, sorted with
> +    // the earliest task first in the queue
> +    DelayedTask* newTask = new DelayedTask(aTask, aDelayMs);
> +    uint32_t i = 0;
> +    while (i < mDelayedTaskQueue.Length()) {

Maybe it'd be nice to do a binary search here, unless we generally expect tasks to always be at the front of the queue?
Attachment #739203 - Flags: review?(chrislord.net) → review+
(In reply to Chris Lord [:cwiiis] from comment #10)
> 
> This seems kind of unrelated - is there a reason for switching this in this
> patch?

Yeah, making AndroidBridge extend from the GeckoContentController interface ends up requiring this because it declares the NS_INLINE_DECL_THREADSAFE_REFCOUNTING macro. I might be able to do some jujitsu to avoid it but it seemed simpler and safer to just do it.

> > +    public static GeckoEvent createMotionEvent(MotionEvent m, boolean keepInViewCoordinates) {
> 
> I think it's worth documenting this function, to explain what
> keepInViewCoordinates does and why you'd want to do it.

Will do.

> > +        AndroidGeckoEvent* wrapper = AndroidGeckoEvent::MakeFromJavaObject(env, event);
> > +        const MultiTouchInput& input = wrapper->MakeMultiTouchInput(nsWindow::TopWindow());
> > +        delete wrapper;
> 
> This seems a bit of a wasteful thing to do for every event - can we not
> reuse a wrapper?

I'd have to make one of the private things in AndroidGeckoEvent non-private, but yeah we can reuse it. I'll make that change (although it feels just a tad like premature optimization...)

(In reply to Chris Lord [:cwiiis] from comment #15)
> > +    /* Invoked by JNI from the compositor thread */
> > +    public ViewTransform syncFrameMetrics(float offsetX, float offsetY, float zoom,
> 
> This seems to be a copy of setFirstPaintViewport and syncViewportInfo? These
> need to be factored into sensible separate functions and the code shared -
> also, what this function does needs documenting.

Yeah, my intention was that the setFirstPaintViewport and syncViewportInfo functions would be removed once we switch over to APZC but I don't know exactly when that'll happen so I might as well refactor/reuse the code.

(In reply to Chris Lord [:cwiiis] from comment #16)
> > +    DelayedTask* newTask = new DelayedTask(aTask, aDelayMs);
> > +    uint32_t i = 0;
> > +    while (i < mDelayedTaskQueue.Length()) {
> 
> Maybe it'd be nice to do a binary search here, unless we generally expect
> tasks to always be at the front of the queue?

The queue is rarely more than 1 or 2 elements long so binary search seems unnecessary (and probably less efficient).
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #17)
> (In reply to Chris Lord [:cwiiis] from comment #10)
> > > +        AndroidGeckoEvent* wrapper = AndroidGeckoEvent::MakeFromJavaObject(env, event);
> > > +        const MultiTouchInput& input = wrapper->MakeMultiTouchInput(nsWindow::TopWindow());
> > > +        delete wrapper;
> > 
> > This seems a bit of a wasteful thing to do for every event - can we not
> > reuse a wrapper?
> 
> I'd have to make one of the private things in AndroidGeckoEvent non-private,
> but yeah we can reuse it. I'll make that change (although it feels just a
> tad like premature optimization...)

I'm happy to leave it as it is in that case.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #17)
> (In reply to Chris Lord [:cwiiis] from comment #16)
> > > +    DelayedTask* newTask = new DelayedTask(aTask, aDelayMs);
> > > +    uint32_t i = 0;
> > > +    while (i < mDelayedTaskQueue.Length()) {
> > 
> > Maybe it'd be nice to do a binary search here, unless we generally expect
> > tasks to always be at the front of the queue?
> 
> The queue is rarely more than 1 or 2 elements long so binary search seems
> unnecessary (and probably less efficient).
Could use IndexOfFirstElementGt() though
Attachment #739203 - Flags: review?(blassey.bugs) → review+
Created attachment 742009 [details] [diff] [review]
Part 1 - Add a stub NativePanZoomController in java

Rebased, carrying r+
Attachment #739196 - Attachment is obsolete: true
Attachment #742009 - Flags: review+
Created attachment 742011 [details] [diff] [review]
Part 2 - Forward touch events to APZC

Rebased, added documentation about the createMotionEvent parameter, carrying r+ and f?
Attachment #739197 - Attachment is obsolete: true
Attachment #739197 - Flags: feedback?(wjohnston)
Attachment #742011 - Flags: review+
Attachment #742011 - Flags: feedback?(wjohnston)
Created attachment 742012 [details] [diff] [review]
Part 3 - (Cleanup) add mozilla::layers namespace to nsWindow

Rebased, carrying r+
Attachment #739199 - Attachment is obsolete: true
Attachment #742012 - Flags: review+
Created attachment 742013 [details] [diff] [review]
Part 4 - Add a syncFrameMetrics code path between APZC <-> java (v2)

Rebased and modified syncFrameMetrics to just reuse the existing setFirstPaintViewport and syncViewportInfo methods. This is much simpler, and doesn't require the extra "boolean render" flag being passed around either.
Attachment #739200 - Attachment is obsolete: true
Attachment #742013 - Flags: review?(chrislord.net)
Created attachment 742014 [details] [diff] [review]
Part 5 - Handle APZC-reported gestures

Rebased, carrying r+
Attachment #739201 - Attachment is obsolete: true
Attachment #742014 - Flags: review+
Created attachment 742016 [details] [diff] [review]
Part 6 - Implement RequestContentRepaint callback

Rebased, carrying r+
Attachment #739202 - Attachment is obsolete: true
Attachment #742016 - Flags: review+
Created attachment 742019 [details] [diff] [review]
Part 7 - Implement the PostDelayedTask callback

Rebased, carrying r+. I decided not to use IndexOfFirstElementGt because that would require making the array of type DelayedTask rather DelayedTask* and overloading operators which makes the rest of the code more complicated.
Attachment #739203 - Attachment is obsolete: true
Attachment #742019 - Flags: review+
Comment on attachment 742013 [details] [diff] [review]
Part 4 - Add a syncFrameMetrics code path between APZC <-> java (v2)

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

Much better! Bonus points if you changed SyncViewportInfo and SyncFrameMetrics to use a gfx::Point instead of float references (I forgot to do this in my patch :/)
Attachment #742013 - Flags: review?(chrislord.net) → review+
Depends on: 866184
Attachment #742011 - Flags: feedback?(wjohnston)
You need to log in before you can comment on or make changes to this bug.