839641 - Wire up the AsyncPanZoomController to be usable from java

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

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

With bug 777468 out of the way, we now need an implementation of the PanZoomController interface that delegates to an instance of the AsyncPanZoomController.

Comment 1

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

Attachment: Part 1 - Add a stub NativePanZoomController in java

The FIXME functions will be filled in subsequent patches.

Comment 2

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

Attachment: Part 2 - Forward touch events to APZC

Comment 3

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

Attachment: Part 3 - (Cleanup) add mozilla::layers namespace to nsWindow

This just makes the code a bit easier to read.

Comment 4

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

Attachment: 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.

Comment 5

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

Attachment: Part 5 - Handle APZC-reported gestures

Comment 6

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

Attachment: Part 6 - Implement RequestContentRepaint callback

Comment 7

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

Attachment: Part 7 - Implement the PostDelayedTask callback

Comment 8

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

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 9

[Benoit Girard (:BenWa)]

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.

Comment 10

[Chris Lord [:cwiiis]]

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?

Comment 11

[Chris Lord [:cwiiis]]

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?

Comment 12

[Chris Lord [:cwiiis]]

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.

Comment 13

[Chris Lord [:cwiiis]]

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).

Comment 14

[Chris Lord [:cwiiis]]

Comment on attachment 739202 [details] [diff] [review]
Part 6 - Implement RequestContentRepaint callback

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

LGTM.

Comment 15

[Chris Lord [:cwiiis]]

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.

Comment 16

[Chris Lord [:cwiiis]]

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?

Comment 17

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

(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).

Comment 18

[Chris Lord [:cwiiis]]

(In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 19

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Kartikaya Gupta (email:[e-mail]) 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

Comment 20

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

Attachment: Part 1 - Add a stub NativePanZoomController in java

Rebased, carrying r+

Comment 21

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

Attachment: Part 2 - Forward touch events to APZC

Rebased, added documentation about the createMotionEvent parameter, carrying r+ and f?

Comment 22

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

Attachment: Part 3 - (Cleanup) add mozilla::layers namespace to nsWindow

Rebased, carrying r+

Comment 23

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

Attachment: 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.

Comment 24

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

Attachment: Part 5 - Handle APZC-reported gestures

Rebased, carrying r+

Comment 25

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

Attachment: Part 6 - Implement RequestContentRepaint callback

Rebased, carrying r+

Comment 26

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

Attachment: 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.

Comment 27

[Chris Lord [:cwiiis]]

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 :/)

Comment 28

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

I filed bug 866184 for the gfx::Point thing, I'll do it as a follow-up. The try build on these patches was clean:

https://tbpl.mozilla.org/?tree=Try&rev=63d18a0847d7

So landed on inbound:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b5ffa6b0d927
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7e093f07dd05
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/984977c00f07
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4b2b6c39adf9
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e2152cf53805
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d892316a8519
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5884664a5961

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b5ffa6b0d927
https://hg.mozilla.org/mozilla-central/rev/7e093f07dd05
https://hg.mozilla.org/mozilla-central/rev/984977c00f07
https://hg.mozilla.org/mozilla-central/rev/4b2b6c39adf9
https://hg.mozilla.org/mozilla-central/rev/e2152cf53805
https://hg.mozilla.org/mozilla-central/rev/d892316a8519
https://hg.mozilla.org/mozilla-central/rev/5884664a5961