Closed Bug 1355656 Opened 8 years ago Closed 6 years ago

Write gtests for momentum scrolling after a two-fingered pan

Categories

(Core :: Panning and Zooming, enhancement, P3)

Product:
Core
Component:
Panning and Zooming
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: botond, Assigned: jlogandavison, Mentored)

References

Details

(Whiteboard: [gfx-noted] [lang=c++])

Attachments

(1 file, 5 obsolete files)

patch.diff
(deleted), patch
botond
: review+
Details | Diff | Splinter Review 
In bug 1180799, we implemented momentum scrolling after a two-fingered pan, on pages where zooming is not allowed.

As a follow-up, we would like to write some gtests for this feature.

I think the tests we'd like to have are:

  (1) On a page where zooming is allowed, check that we do NOT
      get momentum scrolling after a two-fingered pan.

  (2) On a page where zooming is not allowed, check that we get
      momentum scrolling after a two-fingered pan:

       (a) in the case where the two fingers are lifted at the
           same time

       (b) in the case where the two fingers are lifted at
           different times

(1) can just be a new assertion in an existing test in TestPinching.cpp [1]. (2a) and (2b) should be new tests, also in TestPinching.cpp.

[1] http://searchfox.org/mozilla-central/source/gfx/layers/apz/test/gtest/TestPinching.cpp
An additional scenario for which it would be good to write a test is the one that regressed in bug 1355944 (see bug 1180799 comment 38 for a diagnosis that helps pin down the STR for that regression).
I would like to work on this bug if it is possible. Thank you.
(In reply to Jaewoongyu from comment #2)
> I would like to work on this bug if it is possible. Thank you.

Great, thanks for your interest!

The first thing to do is to build the Firefox codebase locally, as described here:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build

Let me know once you've done that and I'll post some further guidance.
I built the Firefox codebase locally as described
Great! I'm assigning the bug to you.

The next step is to make the modifications to TestPinching.cpp described in comment 0, and then test them by running the new / modified test cases.

The tests in that file use the GTest (Google Test) test framework. See [1] for documentation on how to run test cases. There's also a link there to Google's own documentation for the GTest framework which goes into more detail.

You may also want to review bug 1180799 to get a better understanding of the functionality we want to test. There's a fair amount of technical discussion in that bug; don't worry if you don't follow it all, as a lot of it pertains to the implementation of the functionality, which doesn't really matter for writing the test.

Let me know if you have any questions or run into any issues! I'm happy to provide further help / guidance as needed.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/GTest
Assignee: nobody → yuj25
Should I do gtest it before I create a patch file?
I mean that after I have some modifications based on comment 0.
Yes, please check that the new / modified tests pass before uploading the patch. Thanks!
So, basically what test should I do? there are three tests which are Running test, Debugging a gtest unit Test, Writing a gtest unit test
As you can see in comment 0, there are three tasks here, numbered (1), (2a) and (2b) in that comment.

Let's start with (1). That involves modifying an existing test.

  - Pick a test from TestPinching.cpp that involves a page where 
    zooming is allowed. Any test that calls the MakeApzcZoomable() 
    function should do.

  - Modify the test to check that momentum scrolling has not
    occurred. Figuring out how to do that requires thinking a bit
    about how momentum scrolling works; this is why I suggested
    reviewing bug 1180799 first. Give it a try, if you're not
    getting anywhere I can give you a hint.

  - Run the modified test to check that it still passes.

That's probably a good time to post your modifications for review. We can do parts (2a) and (2b) later.

Debugging the test is only necessary if you're getting an unexpected result (e.g. the test is failing when you're expecting it to pass) and you need to figure out why.
I get a little bit confused, so basically when I implement gtest, should I do it within the Firefox build environment?
(In reply to Jaewoongyu from comment #11)
> I get a little bit confused, so basically when I implement gtest, should I
> do it within the Firefox build environment?

You do need to build before you can test your modifications. So you would:

  - make modifications to TestPinching.cpp

  - run "./mach build binaries" to build the modified test code

  - run "./mach gtest <testname>" to run your modified test, substituting
    "<testname>" with the name of the test you want to run (an example 
    might be "APZCPinchTester.Pinch_DefaultGestures_NoTouchAction")

Let me know if that answers your question.
So I need to build it within the Firefox build environment right?
I have been trying many times to modify, but I am still struggling. Where do you use this code?
(In reply to Jaewoongyu from comment #13)
> So I need to build it within the Firefox build environment right?

You run the commands mentioned in comment 12 in the same directory where you ran './mach build' to build Firefox originally (the source directory), if that's what you mean.
(In reply to Jaewoongyu from comment #14)
> I have been trying many times to modify, but I am still struggling.

I've been thinking some more about what I wrote earlier:

(In reply to Botond Ballo [:botond] from comment #10)
>   - Pick a test from TestPinching.cpp that involves a page where 
>     zooming is allowed. Any test that calls the MakeApzcZoomable() 
>     function should do.

I don't think that any test that calls MakeApzcZoomable() is suitable for this purpose. Most of the tests in TestPinching.cpp generate pinch gestures intended to trigger zooming (the distance between the fingers changes but the focus point does not), but we want a test that generates pinch gestures intended to trigger two-finger panning (the distance between the fingers stays the same but the focus point changes).

We appear to have two such tests, Panning_TwoFinger_ZoomDisabled and Pinch_TwoFinger_APZZoom_Disabled_Bug1354185, and in both of them, zooming is disabled.

So, we do actually need to write a new test for this case, rather than modifying an existing test.

Let's start with something simple: let's make a new test, similar to Panning_TwoFinger_ZoomDisabled, where zooming is allowed. We can call it Panning_TwoFinger_ZoomEnabled. The only difference from Panning_TwoFinger_ZoomDisabled should be that instead of calling MakeApzcUnzoomable(), we call MakeApzcZoomable().

Let's start with that. We can then worry about checking that momentum scrolling doesn't happen, as the next step.
 I do not think it will be able to run without the class files and header files used here. Can you upload all the files?
(In reply to Jaewoongyu from comment #17)
>  I do not think it will be able to run without the class files and header
> files used here. Can you upload all the files?

All the necessary files are in the source directory that you checked out when doing your initial build (comment 3).

What files do you think you're missing?
Hi Jaewoongyu, are you still working on this?
Flags: needinfo?(yuj25)
I'm going to unassign this bug and make it available for someone else who may be interested to take it on.

Jaewoongyu, if you're still working on this, or would like to, feel free to say so and I'll assign it back to you if no one else has taken it.
Assignee: yuj25 → nobody
Flags: needinfo?(yuj25)
Hi Botond,

For detecting the momentum scrolling, should we *expect* the `APZCTreeManager::DispatchFling()` to be called? Thanks!
Flags: needinfo?(botond)
(In reply to Dhi Aurrahman from comment #21)
> For detecting the momentum scrolling, should we *expect* the
> `APZCTreeManager::DispatchFling()` to be called? Thanks!

Good question! We do expect APZCTreeManager::DispatchFling() to be called if momentum scrolling is triggered.

If you mean using EXPECT_CALL to detect whether or not DispatchFling() has been called, I don't think we can do that, because we don't have a mock object for APZCTreeManager the way we do for GeckoContentController.

An alternative method for detecting whether momentum scrolling has been triggered might be to check the state of the AsyncPanZoomController after the pinch gesture has been processed; methods like TestAsyncPanZoomController::AssertStateIsFling() [1] may be helpful.

[1] https://searchfox.org/mozilla-central/rev/d2b4b40901c15614fad2fa34718eea428774306e/gfx/layers/apz/test/gtest/APZTestCommon.h#263
Flags: needinfo?(botond)
Thanks (In reply to Botond Ballo [:botond] from comment #22)
> (In reply to Dhi Aurrahman from comment #21)
> > For detecting the momentum scrolling, should we *expect* the
> > `APZCTreeManager::DispatchFling()` to be called? Thanks!
> 
> Good question! We do expect APZCTreeManager::DispatchFling() to be called if
> momentum scrolling is triggered.
> 
> If you mean using EXPECT_CALL to detect whether or not DispatchFling() has
> been called, I don't think we can do that, because we don't have a mock
> object for APZCTreeManager the way we do for GeckoContentController.
> 
> An alternative method for detecting whether momentum scrolling has been
> triggered might be to check the state of the AsyncPanZoomController after
> the pinch gesture has been processed; methods like
> TestAsyncPanZoomController::AssertStateIsFling() [1] may be helpful.
> 
> [1]
> https://searchfox.org/mozilla-central/rev/
> d2b4b40901c15614fad2fa34718eea428774306e/gfx/layers/apz/test/gtest/
> APZTestCommon.h#263

Thanks for the hint, yes, I tried that yesterday. However, I guess I need to be sure about: `the pinch gesture has been processed`, since blindly calling checking the `TestAsyncPanZoomController::mState` after `PinchWithPinchInput(...)` gives me `PINCHING`. Digging...
Hi Botond,

I'm sorry if it is actually obvious, I couldn't find a way to make sure the event is processed. I guessed it is mcc related but apparently it is not. Would you give me another hint? Thanks!
Flags: needinfo?(botond)
Depends on: 1180799
Flags: needinfo?(botond)
(In reply to Dhi Aurrahman from comment #23)
> However, I guess I need to
> be sure about: `the pinch gesture has been processed`, since blindly calling
> checking the `TestAsyncPanZoomController::mState` after
> `PinchWithPinchInput(...)` gives me `PINCHING`.

You're right; there is a complication that I forgot about.

First, copying some background information from bug 1180799 comment 26:

> To give a bit of background here: what happens in production code is the OS
> sends us touch events, we run them through a gesture detector to see if the
> touch events represent a pinch gesture, and if so we synthesize "pinch
> gesture events" which we then handle (e.g. in OnScaleBegin()/OnScaleEnd()).
> 
> The test suite, however, can also just create and send pinch gesture events
> directly

The test suite can trigger the pinch gesture handling code in two ways:

  - PinchWithTouchInput(), which creates touch events that represent
    a pinch gesture. These go to the gesture detector and are turned
    into pinch gesture events.

  - PinchWithPinchInput(), which creates the pinch gesture events
    directly.

In bug 1180799, we only hooked up the momentum scrolling for pinch gesture events that are created from touch events (for simplicity, and because this is what happens in production).

That means that tests that use PinchWithPinchInput() will not trigger the momentum scrolling code.

(That the state is still PINCHING after calling PinchWithPinchInput(), rather than NOTHING which is what you would expect if the gesture has been processed and momentum scrolling has not been triggered, is a bug. I filed bug 1443231 for that.)

So, for the tests in this bug, we need to use PinchWithTouchInput(), rather than PinchWithPinchInput().

Note also the following discussion from bug 1180799 comment 28 which is relevant:

> > Is it possible to use PinchWithTouchInput() directly for the behavior we
> > want? It seems like it just does a horizontal pinch, and both fingers move
> > at the same rate so the focus point will remain constant. Would we need to
> > modify the function or create a similar function? Also,
> > PinchWithTouchInput() seems to release the two fingers at the same time, so
> > it seems like we would need to have the ability to change that as well.
> 
> Good point, we'd need to either make some changes to PinchWithTouchInput(),
> or just send the touch events (similar how to how PinchWithTouchInput() does
> it, but with our desired modifications) in our test body directly.
Hi Dhi, could you clarify whether you are interested in taking this bug?

I ask because if not, I have another contributor who may be interested in working on it.
Flags: needinfo?(diorahman)
Hi Botond, please go ahead assign it to another contributor. Sorry for not telling it clearly before.
Flags: needinfo?(diorahman)
Attached patch patch.diff (obsolete) (deleted) — — Splinter Review 
Hi, I've had a crack at this. This is a preliminary patch w.out the required tests, just some per-requisite work.

I've opted to slightly refactor "PinchWithTouchInput" to allow for different starting/ending focus points. I've overloaded the original method signature so as to not break any existing tests that rely on "PinchWithTouchInput".

(We'll probably still need to implement the more advanced gestures directly in the body of the 2nd and 3rd test cases, but I felt this would be a nice addition to the existing test utilities)

I'm having trouble actually triggering a fling gesture with this implementation however. Even in a test where the apzc is set to unzoomable which if I understand correctly SHOULD allow for fling gestures on two finger pan.

With the 4 input events in PinchWithTouchInput (Start -> Move -> Move -> End), I'm seeing the following state transition: (Touching -> Panning -> Panning -> Nothing)

I also can't seem to pan in the y-axis at all, so perhaps my implementation is buggy. Is there anything obvious that you can see is missing here?
Attachment #8970038 - Flags: review?(botond)
Comment on attachment 8970038 [details] [diff] [review]
patch.diff

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

Thanks for working on this! I'll assign the bug to you.

This patch so far looks good.

(In reply to jlogandavison from comment #28)
> I've opted to slightly refactor "PinchWithTouchInput" to allow for different
> starting/ending focus points. I've overloaded the original method signature
> so as to not break any existing tests that rely on "PinchWithTouchInput".
> 
> (We'll probably still need to implement the more advanced gestures directly
> in the body of the 2nd and 3rd test cases, but I felt this would be a nice
> addition to the existing test utilities)

Sounds good.

> I'm having trouble actually triggering a fling gesture with this
> implementation however. Even in a test where the apzc is set to unzoomable
> which if I understand correctly SHOULD allow for fling gestures on two
> finger pan.
> 
> With the 4 input events in PinchWithTouchInput (Start -> Move -> Move ->
> End), I'm seeing the following state transition: (Touching -> Panning ->
> Panning -> Nothing)
> 
> I also can't seem to pan in the y-axis at all, so perhaps my implementation
> is buggy. Is there anything obvious that you can see is missing here?

It would be easier for me to answer these questions if you also posted the portion of the new test that you have written so far, that is exhibiting the behaviour you are describing.
Attachment #8970038 - Flags: review?(botond) → feedback+
Assignee: nobody → jlogandavison
Attached patch patch.diff (obsolete) (deleted) — — Splinter Review 
Yes, apologies. This would have been much clearer had I shown the code that was calling to PinchWithTouchInput.

I've added an example test case including assertions.
Attachment #8970038 - Attachment is obsolete: true
Attachment #8971826 - Flags: review?(botond)
Comment on attachment 8971826 [details] [diff] [review]
patch.diff

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

Ah, I see what the problem is: in order for PinchWithTouchInput() to work, the gesture detector which converts the touch events into pinch gesture events needs to be enabled. This requires an additional setup step. We have a fixture called APZCPinchGestureDetectorTester that handles this; the test should use that fixture rather than APZCPinchTester.
Attachment #8971826 - Flags: review?(botond)
There is actually one other problem: I broke momentum scrolling in the case where the two fingers are lifted at the same time, in bug 1443231 :(

(See, this is why we need tests. In the absence of tests, the functionality will break over time.)

I filed bug 1457743 and posted a fix there. You can apply my fix locally to unblock you from working on this.
Finally, there's a third issue: the momentum scrolling codepath is conditioned on some calculations related to velocity (the speed at which the page is moving at the time the fingers are released). In order for the code to be able to calculate the velocity correctly, the synthesized events need to have timestamps assigned to them. Right now, PinchWithTouchInput() is giving each of them a timestamp of zero, which doesn't work.

Have a look at how Pan(), the utility function for generating a one-finger pan gesture [1], assigns timestamps to the events it synthesizes. We'll need to do something similar in PinchWithTouchInput(). 

[1] https://searchfox.org/mozilla-central/rev/12af4303ffd384bc2406b67f54ba68d8264d72c8/gfx/layers/apz/test/gtest/APZTestCommon.h#445
Attached patch patch.diff (obsolete) (deleted) — — Splinter Review 
Ok, quick status update.

I've implemented the changes you suggested and it works! The example test now successfully triggers a FLING state.

In order to get a pointer to the content controller in |PinchWithTouchInput| (for the purposes of calling |mcc->Time()| and |mcc->AdvanceBy()|) I added a |getContentController()| method to the MockAPZC. 

In this patch I've gone with an alternative approach. I've moved the implementation for |PinchWithTouchInput()| inside |APZCTesterBase| alongside |Pan()|, |Tap()|, and |DoubleTap()|. This way |PinchWithTouchInput()| can access the |mcc| member of |APZCTesterBase|.

This is a bit of a bold refactoring, however it doesn't actually break any existing calls to |PinchWithTouchInput()| because all calls are from within subclasses of |APZCTesterBase|.

There are a couple wrinkles though. Firstly, the name |PinchWithTouchInput()| is now kind of inconsistent when placed next to its counterparts |Pan|, |Tap|, etc.

Secondly the compiler is complaining that it cannot find the method |CreateSingleTouchData()| (note that I've commented out the offending lines in this patch), which is a bit of a head scratcher because other methods from |InputUtils.h| are used throughout |APZCTesterBase|.

If you run the tests currently an exception is thrown when |PinchWithTouchInput()| is called. This is because of the missing lines that I've commented out. Any insight on how to resolve these compiler errors?
Attachment #8971826 - Attachment is obsolete: true
Attachment #8973030 - Flags: review?(botond)
Regardless of what we decide to do about the issue above, I'm now in a position be able to implement the test cases.

I noticed that the |Pan()| method had an optional argument to pass a |PanOptions| enum. This option modifies the behavior of the pan gesture. For example [1]:

> if (!(aOptions & PanOptions::KeepFingerDown)) {
>     status = TouchUp(aTarget, aTouchEnd, mcc->Time());
> } else {
>     status = nsEventStatus_eIgnore;
> }

Allows you to specify that the finger should not be lifted at the end of the gesture.

What are your thoughts on a similar |PinchOptions| so that we can specify which fingers should be lifted at the end of the pinch gesture? This would be useful for test case 2b.

[1]: https://searchfox.org/mozilla-central/rev/c0d81882c7941c4ff13a50603e37095cdab0d1ea/gfx/layers/apz/test/gtest/APZTestCommon.h#517
Flags: needinfo?(botond)
(In reply to jlogandavison from comment #34)
> In this patch I've gone with an alternative approach. I've moved the
> implementation for |PinchWithTouchInput()| inside |APZCTesterBase| alongside
> |Pan()|, |Tap()|, and |DoubleTap()|. This way |PinchWithTouchInput()| can
> access the |mcc| member of |APZCTesterBase|.

I like this approach!

> There are a couple wrinkles though. Firstly, the name
> |PinchWithTouchInput()| is now kind of inconsistent when placed next to its
> counterparts |Pan|, |Tap|, etc.

I think that's fine. Normally we'd just call the function Pinch(), but since there are two ways to pinch, we disambiguate by having PinchWithTouchInput() and PinchWithPinchInput().

> Secondly the compiler is complaining that it cannot find the method
> |CreateSingleTouchData()| (note that I've commented out the offending lines
> in this patch), which is a bit of a head scratcher because other methods
> from |InputUtils.h| are used throughout |APZCTesterBase|.

InputUtils.h includes APZTestCommon.h, not the other way around, so code in APZTestCommon.h is seen by the compiler before code in InputUtils.h. CreateSingleTouchData() is defined in InputUtils.h, and we are trying to call it from APZTestCommon.h

The reason this is not an issue for certain other functions like TouchDown(), is that the calls to those in APZTestCommon.h are inside function templates, and the arguments are dependent (on the type of the InputReceiver template parameter), and name lookup for function calls with dependent arguments is deferred until the point when the template is instantiated. The instantiation happens later (in the .cpp files that define the tests), by which time InputUtils.h has been seen.

A reasonable solution would be to move the CreateSingleTouchData() function to APZTestCommon.h.

(In reply to jlogandavison from comment #35)
> I noticed that the |Pan()| method had an optional argument to pass a
> |PanOptions| enum. This option modifies the behavior of the pan gesture. For
> example [1]:
> 
> > if (!(aOptions & PanOptions::KeepFingerDown)) {
> >     status = TouchUp(aTarget, aTouchEnd, mcc->Time());
> > } else {
> >     status = nsEventStatus_eIgnore;
> > }
> 
> Allows you to specify that the finger should not be lifted at the end of the
> gesture.
> 
> What are your thoughts on a similar |PinchOptions| so that we can specify
> which fingers should be lifted at the end of the pinch gesture? This would
> be useful for test case 2b.

Sounds good!
Flags: needinfo?(botond)
Comment on attachment 8973030 [details] [diff] [review]
patch.diff

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

Looks good so far!

::: gfx/layers/apz/test/gtest/TestPinching.cpp
@@ +281,5 @@
> +  uint64_t blockId = 0;
> +  PinchWithTouchInput(apzc, ScreenIntPoint(100, 200), ScreenIntPoint(100, 100),
> +      1, touchInputId, nullptr, nullptr, &blockId);
> +
> +  // Expect to be in a flining state

typo: flinging
Attachment #8973030 - Flags: review?(botond) → feedback+
Hi jlogandavison, how are things going here? Anything else I can help with?
Attached patch patch.diff (obsolete) (deleted) — — Splinter Review 
Hi Botond, my apologies for not putting this together earlier.

I've added the optional PinchOptions argument to PinchWithTouchInput as discussed. It works as a bit mask allowing you to specify which fingers to lift at the end of the gesture. The default is to lift both fingers.

I've written the three test cases (1, 2a, and 2b) that were discussed. 1 and 2a pass as expected. But 2b currently leaves the APZC in a FLING state when we are expecting NOTHING state. This might be due to my implementation. I'm using PinchWithTouchInput (with PinchOptions to specify that only one finger is lifted) followed by a call to Pan.

However I have noticed that a two-then-one finger pan gesture *does* appear to trigger a FLING state on the version of Firefox on my phone [1]. Are you able to reproduce this behavior?

[1]: Firefox nightly 62.0a1 (2018-05-09)
Attachment #8973030 - Attachment is obsolete: true
Attachment #8982059 - Flags: review?(botond)
(In reply to jlogandavison from comment #39)
> I've written the three test cases (1, 2a, and 2b) that were discussed. 1 and
> 2a pass as expected. But 2b currently leaves the APZC in a FLING state when
> we are expecting NOTHING state. This might be due to my implementation. I'm
> using PinchWithTouchInput (with PinchOptions to specify that only one finger
> is lifted) followed by a call to Pan.
> 
> However I have noticed that a two-then-one finger pan gesture *does* appear
> to trigger a FLING state on the version of Firefox on my phone [1]. Are you
> able to reproduce this behavior?

So this is actually expected: if you've transitioned to a one-finger pan gesture, then it doesn't matter whether that used to be a two-finger gesture before or not, it always flings.

The scenario I had in mind for (2b) was where you release one finger, do not move the second finger (at least, not enough to trigger a pan gesture), and then release the second finger. The intention is to test the scenario where you intend to release both fingers, but the touchscreen hardware is sufficiently sensitive that it picks up a small difference in time between the two fingers lifting, and reports them as two separate events.

In terms of how to express this in the test, we could do a PinchWithTouchInput(LiftFinger1) call followed by a separate touch-end event for the second finger. (Or, if you prefer, you can add a LiftFingersSeparately flag to PinchWithTouchInput and have it send to the second touch-end.)
Comment on attachment 8982059 [details] [diff] [review]
patch.diff

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

Looks good, besides the issue with the (2b) testcase which I discussed.
Attachment #8982059 - Flags: review?(botond) → feedback+
Attached patch patch.diff (obsolete) (deleted) — — Splinter Review 
(In reply to Botond Ballo [:botond] from comment #40) 
> The scenario I had in mind for (2b) was where you release one finger, do not
> move the second finger (at least, not enough to trigger a pan gesture), and
> then release the second finger.

Aah, that makes a lot more sense. I've updated the test case for 2b.
Attachment #8982059 - Attachment is obsolete: true
Attachment #8982828 - Flags: review?(botond)
Comment on attachment 8982828 [details] [diff] [review]
patch.diff

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

::: gfx/layers/apz/test/gtest/TestPinching.cpp
@@ +315,5 @@
> +      1, touchInputId, nullptr, nullptr, &blockId, PinchOptions::LiftFinger1);
> +
> +  // Lift second finger after a pause
> +  mcc->AdvanceBy(TimeDuration::FromMilliseconds(50));
> +  TouchUp(apzc, ScreenIntPoint(100, 100), mcc->Time());

Thanks, this is almost what we want! Just one thing: PinchWithTouchInput() when called with touchInputId=0 generates two touches with ids of 0 and 1. LiftFinger1 causes the touch with id=0 to be lifted.

TouchUp() is hardcoded to generate a touch-up for id=0, so the events we are generating are saying we are lifting the same finger twice, which does not reflect a real scenario.

This is easy to fix: if we change the PinchWithTouchInput() call to use LiftFinger2 instead, it's going to lift the touch with id=1, and then the TouchUp() will be correct to lift id=0.

(The other alternative is to add an argument to TouchUp() specifying which touch id to use.)
Attachment #8982828 - Flags: review?(botond) → feedback+
Attached patch patch.diff (deleted) — — Splinter Review 
(In reply to Botond Ballo [:botond] from comment #43)
> TouchUp() is hardcoded to generate a touch-up for id=0, so the events we are
> generating are saying we are lifting the same finger twice, which does not
> reflect a real scenario.

Oops, nice catch. I didn't think to check the implementation of |TouchUp()|. Fixed by using the LiftFinger2 option.
Attachment #8982828 - Attachment is obsolete: true
Attachment #8983370 - Flags: review?(botond)
Comment on attachment 8983370 [details] [diff] [review]
patch.diff

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

Looks good, thanks!
Attachment #8983370 - Flags: review?(botond) → review+
(In reply to Botond Ballo [:botond] from comment #46)
> Try push:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=f5245470a32d525e6969e2126396571a0d19f532

Sorry, that has tests that are failing due to an unrelated patch I had applied as well. Let's Try that again with just your patch:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa55d99edb10e296850b89159bc9be1320368019
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/21861ee0ddb8
Implement test cases for two finger fling gestures. r=botond
The updated try push looks good, so I went ahead and landed. Thanks!

I updated the first line of the commit message to include the bug number and the reviewer, as per convention:

  Bug 1355656 - Implement test cases for two finger fling gestures. r=botond

In the future, feel free to add these yourself :)
https://hg.mozilla.org/mozilla-central/rev/21861ee0ddb8
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
(In reply to Botond Ballo [:botond] from comment #49)
> The updated try push looks good, so I went ahead and landed. Thanks!

Awesome! Thank you very much for your help with this.

> I updated the first line of the commit message to include the bug number and
> the reviewer, as per convention:
> 
>   Bug 1355656 - Implement test cases for two finger fling gestures. r=botond
> 
> In the future, feel free to add these yourself :)

I'll keep this in mind for next time, thanks.

----

With regards to "next time", Bug 1451461 appears to be still available. If that's the case then I'm happy to jump onto it. I'll need to do a bit of investigation and formulate my thoughts about a good solution for that one.

In the mean time, if you have any outstanding bugs that you feel would be suitable then I'm all ears!
Flags: needinfo?(botond)
(In reply to jlogandavison from comment #51)
> With regards to "next time", Bug 1451461 appears to be still available. If
> that's the case then I'm happy to jump onto it. I'll need to do a bit of
> investigation and formulate my thoughts about a good solution for that one.

Great, please go ahead!

> In the mean time, if you have any outstanding bugs that you feel would be
> suitable then I'm all ears!

Bug 1451461 is the only mentored bug I have open at the moment, apart from bug 1282245 which is more of a larger, open-ended project (and for which the plan has gotten out of date due to newer developments, though it could be resurrected). New mentored bugs get filed from time to time, so if you check back after bug 1451461 is fixed, there may be more by that time.

If you'd like to cast your net a bit wider, this site has a list of all mentored bugs in the codebase:

https://www.joshmatthews.net/bugsahoy/?cpp=1
Flags: needinfo?(botond)
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: