1146020 - Make APZCCallbackHandler extend from ChromeProcessController

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Android, defect)

Description

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

The world has moved along since I wrote the widget/android/APZCCallbackHandler code, and much of that is not needed any more. Instead that class should be modified to extend from gfx/layers/apz/util/ChromeProcessController and only override what it needs to. Looking at the current implementation, pretty much everything can be removed except for the PostDelayedTask override. That method will be called from the APZ "controller" thread (which on Fennec will be the Java UI thread), and should schedule a task for later execution on that same thread. The current implementation does that, and we'll need to keep that around. Pretty much everything else can be removed, and we should use the ChromeProcessController versions.

Comment 1

[Danilo Cesar Lemes de Paula]

Attachment: Bug-1146020-Make-APZCCallbackHandler-extend-from-Chr.patch

For the panzoomcontroller: SetNativePanZoomController() gets called by AndroidJNI before Initialization of the class.

Kats suggested to store the PZController as static and send it to the instance later. I didn't follow that as the APZCallbackHandler is basically a singleton and holding that as a static wouldn't do no harm but would keep the implementation simpler. 

For sWidget and sAPZEventState, I'm wondering if nsRefPtr is the right smartpointer to use here. Let me know if it's not.

Comment 2

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

Comment on attachment 8584143 [details] [diff] [review]
Bug-1146020-Make-APZCCallbackHandler-extend-from-Chr.patch

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

This looks pretty good, and it should work fine but I have some suggestions below just to tighten up the code a little more.

Also, since you're using git I suggest adding this to your ~/.gitconfig:

[alias]
    hgp = "show -M -C --binary --format=\"# HG changeset patch%n# User %an <%ae>%n%B\" -U8"

and then generate patches using |git hgp <SHA>| so that they get formatted like mercurial patches which the sheriffs can land more easily. It will also increase the diff context to 8 lines which makes it easier to review.

I'd like to see an updated patch with the changes before I r+. Thanks!

::: widget/android/APZCCallbackHandler.h
@@ +42,4 @@
>      static APZCCallbackHandler* GetInstance() {
>          if (sInstance.get() == nullptr) {
> +            if (sWidget.get() != nullptr && sAPZEventState != nullptr)
> +                sInstance = new APZCCallbackHandler(sWidget.get(), sAPZEventState.get());

It might be a little cleaner to just move this creation into the Initialize function. Then this GetInstance() function would simplify to this:

static APZCCallbackHandler* GetInstance() {
    MOZ_ASSERT(sInstance.get(), "Calling ...");
    return sInstance.get();
}

and in fact then you wouldn't need sWidget and sAPZEventState, because in Initialize(..) you can just do:

MOZ_ASSERT(!sInstance.get(), "Initializing multiple times");
sInstance = new APZCCallbackHandler(aWidget, aAPZEventState);

Comment 3

[Danilo Cesar Lemes de Paula]

Attachment: Bug_1146020_Make_APZCallbackHandler_extend_from.patch

Comment 4

[Danilo Cesar Lemes de Paula]

Attachment: Bug_1146020_Make_APZCallbackHandler_extend_from.patch

Comment 5

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

Comment on attachment 8584549 [details] [diff] [review]
Bug_1146020_Make_APZCallbackHandler_extend_from.patch

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

I think you got the asserts backwards, but r+ with that fixed.

::: widget/android/APZCCallbackHandler.h
@@ +34,4 @@
>  public:
> +    static void Initialize(nsIWidget* aWidget, mozilla::layers::APZEventState* aAPZEventState) {
> +
> +        MOZ_ASSERT(sInstance.get(), "APZCCallbackHandler.Initialize() got called twice");

I think this one should be !sInstance.get()

@@ +41,2 @@
>      static APZCCallbackHandler* GetInstance() {
> +        MOZ_ASSERT(!sInstance.get(), "Calling APZCCallbackHandler.GetInstance() before it's initialization");

and this one should be MOZ_ASSERT(sInstance.get(), ...)

Comment 6

[Danilo Cesar Lemes de Paula]

Attachment: Bug_1146020_Make_APZCallbackHandler_extend_from.patch

carrying r+ from previous review.

Comment 7

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

Try push for android:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aabd7b6a9eac

Comment 8

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

Actually I forgot tests don't really run on android debug builds. Here's a better try push:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd59707eaae4

Comment 9

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

Try was green, landed on inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/6e2c1ed29f14

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/6e2c1ed29f14