Closed Bug 1146020 Opened 5 years ago Closed 5 years ago

Make APZCCallbackHandler extend from ChromeProcessController


(Core :: Widget: Android, defect)

Not set



Tracking Status
firefox39 --- fixed


(Reporter: kats, Assigned:




(1 file, 3 obsolete files)

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.
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.
Attachment #8584143 - Flags: review?(bugmail.mozilla)
Comment on attachment 8584143 [details] [diff] [review]

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:

    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);
Attachment #8584143 - Flags: review?(bugmail.mozilla) → feedback+
Assignee: nobody →
Attachment #8584548 - Flags: review?(bugmail.mozilla)
Attachment #8584143 - Attachment is obsolete: true
Attachment #8584548 - Attachment is obsolete: true
Attachment #8584548 - Flags: review?(bugmail.mozilla)
Attachment #8584549 - Flags: review?(bugmail.mozilla)
Comment on attachment 8584549 [details] [diff] [review]

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(), ...)
Attachment #8584549 - Flags: review?(bugmail.mozilla) → review+
carrying r+ from previous review.
Attachment #8584549 - Attachment is obsolete: true
Attachment #8584567 - Flags: review+
Actually I forgot tests don't really run on android debug builds. Here's a better try push:
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.