Closed Bug 1091283 Opened 10 years ago Closed 10 years ago

Refactor APZ test code to more easily allow dispatching input through APZCTM

Categories

(Core :: Panning and Zooming, defect)

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: kats, Assigned: kats)

Details

Attachments

(2 files)

Bug 1073250 added an ApzctmPan function which essentially duplicated the ApzcPan functionality, with a TODO to refactor stuff later. As part of APZ hit-testing (bug 918288 and friends) I'm going to be writing some tests that send input events through APZCTM, so now is a good time to clean that up.
Comment on attachment 8513893 [details] [diff] [review]
Part 1 - Ensure gesture events can get to APZC via ReceiveInputEvent

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

::: gfx/layers/apz/src/InputQueue.cpp
@@ +29,5 @@
>  InputQueue::ReceiveInputEvent(const nsRefPtr<AsyncPanZoomController>& aTarget, const InputData& aEvent, uint64_t* aOutInputBlockId) {
>    AsyncPanZoomController::AssertOnControllerThread();
>  
>    if (aEvent.mInputType != MULTITOUCH_INPUT) {
> +    // The return value for non-touch input isn't really used, so just pass

s/isn't really used/is only used by tests
Attachment #8513893 - Flags: review?(botond) → review+
Comment on attachment 8513894 [details] [diff] [review]
Part 2 - Templat-ify the input event helpers and reuse code

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

Looks pretty good overall!

I have two related requests:

  - For all functions that are made function templates in this patch,
    please name the template parameter something descriptive, such
    as 'EventTarget', rather than 'T'.

  - Add a comment saying what the requirements are for an EventTarget
    (has a method named ReceiveInputEvent() with a certain signature)
    and what classes currently meet these requirements (APZCTreeManager,
    TestAsyncPanZoomController).

r+ with these changes.

::: gfx/tests/gtest/TestAsyncPanZoomController.cpp
@@ +291,4 @@
>  }
>  
> +template<class T> static void
> +Tap(const nsRefPtr<T> aTarget, int aX, int aY, int& aTime, int aTapLength,

You probably meant to take |aTarget| by reference here too.

@@ +326,5 @@
>  {
> +  // Reduce the touch start tolerance to a tiny value.
> +  // We can't use a scoped pref because this value might be read at some later
> +  // time when the events are actually processed, rather than when we deliver
> +  // them.

Tricky!
Attachment #8513894 - Flags: review?(botond) → review+
(In reply to Botond Ballo [:botond] from comment #4)
>   - For all functions that are made function templates in this patch,
>     please name the template parameter something descriptive, such
>     as 'EventTarget', rather than 'T'.
> 
>   - Add a comment saying what the requirements are for an EventTarget
>     (has a method named ReceiveInputEvent() with a certain signature)
>     and what classes currently meet these requirements (APZCTreeManager,
>     TestAsyncPanZoomController).

Cool, made these changes (and the one in part 1).

> >  
> > +template<class T> static void
> > +Tap(const nsRefPtr<T> aTarget, int aX, int aY, int& aTime, int aTapLength,
> 
> You probably meant to take |aTarget| by reference here too.

Indeed, nice catch! :)

Will land once the trees reopen.
https://hg.mozilla.org/mozilla-central/rev/543860322c50
https://hg.mozilla.org/mozilla-central/rev/84c97cd075da
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: