Botond Ballo [:botond] Reporter
	Description • 7 years ago

In bug 1180865 we added a "pinch locking" mechanism where, if a two-finger gesture starts off looking like it's mostly a two-fingered pan rather than a pinch, we lock it into doing panning only and not zooming.

It would be good to write some tests for this feature. We have an existing suite of tests for pinching [1], written using the Google Test (gtest) framework, that we could add some test cases to.

[1] https://searchfox.org/mozilla-central/rev/652fbd6270de0d3ec424d2b88f8375ff546f949f/gfx/layers/apz/test/gtest/TestPinching.cpp

	jlogandavison Assignee
	Comment 1 • 7 years ago

I worked on bug 1180865 and I'm happy to have a look into this :)

	Botond Ballo [:botond] Reporter
	Comment 2 • 7 years ago

Sorry, I just realized I never followed up on this!

jlogandavison, are you still interested in working on this? If so, I'll write up some details on how to get started.

	jlogandavison Assignee
	Comment 3 • 7 years ago

(In reply to Botond Ballo [:botond] from comment #2)

As it happens, I had another look at this the other evening. I've managed to get the test suite up and running on my machine and I've had a browse of the existing test cases in "TestPinching.cpp".

( I noticed that the test cases from "TestPinching.cpp" don't run when "./mach gtest" is ran with no filter. I guessed that the default test suite is some sort of smoke test that includes a subset of the full suite. Is this accurate? )

From what I can gather from reading the code, there are a number of util methods defined in |InputUtil| that I can use to supply inputs to the APZC. Then I can assert on |APZC->GetFrameMetrics()| to confirm that the expected behavior actually happened.

Do you think it's worth defining a method similar to the existing |DoPinchTest()| (|DoPinchLockTest()| maybe?) that supplies a canned pan-then-pinch (or pinch-then-pan) gesture that can be invoked with different preconditions in multiple tests?

	Botond Ballo [:botond] Reporter
	Comment 4 • 7 years ago

Great! I assigned the bug to you.

(In reply to jlogandavison from comment #3)
> ( I noticed that the test cases from "TestPinching.cpp" don't run when
> "./mach gtest" is ran with no filter. I guessed that the default test suite
> is some sort of smoke test that includes a subset of the full suite. Is this
> accurate? )

They should. I just ran "./mach gtest", and while it took a while to complete, I see the following in the output:

[----------] 3 tests from APZCPinchTester
[ RUN      ] APZCPinchTester.Pinch_DefaultGestures_NoTouchAction
[       OK ] APZCPinchTester.Pinch_DefaultGestures_NoTouchAction (0 ms)
[ RUN      ] APZCPinchTester.Panning_TwoFinger_ZoomDisabled
[       OK ] APZCPinchTester.Panning_TwoFinger_ZoomDisabled (0 ms)
[ RUN      ] APZCPinchTester.Pinch_TwoFinger_APZZoom_Disabled_Bug1354185
[       OK ] APZCPinchTester.Pinch_TwoFinger_APZZoom_Disabled_Bug1354185 (0 ms)
[----------] 3 tests from APZCPinchTester (0 ms total)

[----------] 9 tests from APZCPinchGestureDetectorTester
[ RUN      ] APZCPinchGestureDetectorTester.Pinch_UseGestureDetector_NoTouchAction
[       OK ] APZCPinchGestureDetectorTester.Pinch_UseGestureDetector_NoTouchAction (0 ms)
[ RUN      ] APZCPinchGestureDetectorTester.Pinch_UseGestureDetector_TouchActionNone
[       OK ] APZCPinchGestureDetectorTester.Pinch_UseGestureDetector_TouchActionNone (0 ms)
[ RUN      ] APZCPinchGestureDetectorTester.Pinch_UseGestureDetector_TouchActionZoom
[       OK ] APZCPinchGestureDetectorTester.Pinch_UseGestureDetector_TouchActionZoom (1 ms)
[ RUN      ] APZCPinchGestureDetectorTester.Pinch_UseGestureDetector_TouchActionNotAllowZoom
[       OK ] APZCPinchGestureDetectorTester.Pinch_UseGestureDetector_TouchActionNotAllowZoom (0 ms)
[ RUN      ] APZCPinchGestureDetectorTester.Pinch_UseGestureDetector_TouchActionNone_NoAPZZoom
[       OK ] APZCPinchGestureDetectorTester.Pinch_UseGestureDetector_TouchActionNone_NoAPZZoom (0 ms)
[ RUN      ] APZCPinchGestureDetectorTester.Pinch_PreventDefault
[       OK ] APZCPinchGestureDetectorTester.Pinch_PreventDefault (0 ms)
[ RUN      ] APZCPinchGestureDetectorTester.Pinch_PreventDefault_NoAPZZoom
[       OK ] APZCPinchGestureDetectorTester.Pinch_PreventDefault_NoAPZZoom (0 ms)
[ RUN      ] APZCPinchGestureDetectorTester.Pinch_APZZoom_Disabled
[       OK ] APZCPinchGestureDetectorTester.Pinch_APZZoom_Disabled (1 ms)
[ RUN      ] APZCPinchGestureDetectorTester.Pinch_NoSpan
[       OK ] APZCPinchGestureDetectorTester.Pinch_NoSpan (0 ms)
[----------] 9 tests from APZCPinchGestureDetectorTester (2 ms total)

That said, for running these tests locally I don't recommend |./mach gtest|, since it takes a long time and runs a bunch of unrelated test suites. I would suggest |./mach gtest "APZCPinch*"| to run all the tests in the file, or a more specific filter to run a particular test.

> From what I can gather from reading the code, there are a number of util
> methods defined in |InputUtil| that I can use to supply inputs to the APZC.
> Then I can assert on |APZC->GetFrameMetrics()| to confirm that the expected
> behavior actually happened.

Yep, that's the general idea.

> Do you think it's worth defining a method similar to the existing
> |DoPinchTest()| (|DoPinchLockTest()| maybe?) that supplies a canned
> pan-then-pinch (or pinch-then-pan) gesture that can be invoked with
> different preconditions in multiple tests?

Sounds good to me!

It seems like you have things under control here, so I'll leave you to it. Please feel free to ask if you have any questions or run into any problems / challenges.

	Botond Ballo [:botond] Reporter
	Comment 5 • 7 years ago

How is the test coming along? Anything I can help with?

	jlogandavison Assignee
	Comment 6 • 7 years ago

(In reply to Botond Ballo [:botond] from comment #5)
> How is the test coming along? Anything I can help with?

Thanks for the reminder @botond. I've had a look at this this week and thought I should post a preliminary patch, so you can see the stage that I'm at.

I've written 5 tests total that cover the 3 pinch locking modes. They all perform some combination of two-finger panning and two-finger zooming before verifying that the lock is active/inactive as expected. The pinch-lock status is checked by performing a small pinch operation and checking the "FrameMetrics" to see if the zoom changed (see "APZCPinchLockingTester::isPinchLockActive()").

At least one of the tests is currently failing. This is because I'm struggling to find suitable gestures. I need:

  * Pan/Zoom gestures that are large enough to engage/break the pinch lock
  * A zoom gesture that is small enough to verify the lock status without
    breaking it.

Either I'm coming up against panning/zooming limits that are causing "isPinchLockActive()" to fail when it shouldn't and/or the gestures are sensitive to the APZC's current FrameMetrics. Either way whatever I try seems to leave me with one or more test failure.

Seeing as the GFXPrefs that define the pinch-locking thresholds are defined in screen inches. Maybe I would have more success if I worked in screen inches rather than in screen pixels. To do that I would have to multiply the inch values by the DPI of the test canvas before passing to the "CreatePinchGestureInput" method.

Maybe it's worth stepping back and trying a different approach entirely.

Any thoughts?

	Botond Ballo [:botond] Reporter
	Comment 7 • 7 years ago

Comment on attachment 8955640 [details] [diff] [review]
patch.diff

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

Thanks, this generally looks great!

I played around with the tests a bit and noticed a couple of issues.

First, from the comment "Send a small scale input to the APZC" in isPinchLockActive(), I gather that the intention is for this function to send a scale input that's small enough not to itself cause a sticky pinch lock to be broken (which is good!). Unfortunately, it doesn't deliver on that intention: it passes 'mScale + 1' as the current span, and '10.0' as the previous span. If it's called after twoFingerZoom() has been called, which sets mScale to 15, then the span difference will be 6, which is large enough to break the pinch lock. Perhaps you intended to pass 'mScale' as the previous span, to ensure that the difference will be 1?

The second issue is that, depending on the DPI, the scale change from 10 to 15 in twoFingerZoom() may not be enough to break the pinch lock. (On my computer, the DPI is 160, which results in a threshold of '5' pixels, so the scale change is just too small (it needs to be larger than the threshold)). We can fix this by increasing the scale change in twoFingerZoom(), but I would also like to avoid making assumptions about what the DPI is, since (I believe) it can change from computer to computer. In cases like this, I prefer that the test suite override settings like the DPI with a known value, so that we can be sure that our specified scale changes are large enough. The DPI can be overridden using APZCTreeManager::SetDPI(); the test fixture has access to the APZCTreeManager instance via the protected field APZCBasicTester::tm, which can be accessed from a derived class like APZCPinchLockingTester.

Finally, I have a suggestion for one additional test case: for the PINCH_NORMAL locking mode, we can test that the pinch lock remains active even after a larger pinch (such as the one done in twoFingerZoom()) has been attempted.

	Botond Ballo [:botond] Reporter
	Comment 8 • 7 years ago

While playing around with the tests, I also realized that our pinch lock implementation has a deficiency: it makes the lock / unlock decisions based on the span change and focus change between one event and the next. If you have sufficiently sensitive hardware, it might send events frequently enough that the change between one event and the next won't be larger than our thresholds even if the movement (over multiple events) is relatively large. It may be better to make the decisions based on some sort of *cumulative* span and focus changes over multiple events.

Let's not worry about that in this bug, but we could enhance our implementation to deal with this in a follow-up.

	jlogandavison Assignee
	Comment 9 • 7 years ago

(In reply to Botond Ballo [:botond] from comment #7)
> Unfortunately, it doesn't deliver on that intention: it passes
> 'mScale + 1' as the current span, and '10.0' as the previous span.

Doh! I really should have double checked those arguments. I'm now passing the original mScale as the previous span.

> In cases like this, I prefer that the test suite override settings
> like the DPI with a known value, so that we can be sure that our
> specified scale changes are large enough.

I've set the DPI to a fixed 160. I've also changed the pan/pinch distances to expressions like:

    "panDistance = (1.0/32.0) * 1.2 * tm->GetDPI()"

so it's clear which distances are less/greater than the thresholds (defined in inches in gfxPrefs).

> Finally, I have a suggestion for one additional test case: ...

Done and done.

-------------------------------------------

I had a play with the idea of parameterizing these tests, but ultimately left it out of this patch. It was easy enough to create parameterized test cases for each of the existing test scenarios (using the TEST_P macro) and I was able to instantiate the test cases with FREE, NORMAL, and STICKY locking modes.

The problem was that I couldn't figure out a nice way to map the locking modes to the expected outcomes of each test case.

I think parameterization would be a nice way to ensure that all cases are covered for all modes. Do you think it's worth trying to figure it out?

	Botond Ballo [:botond] Reporter
	Comment 10 • 7 years ago

Comment on attachment 8959396 [details] [diff] [review]
patch.diff

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

Looks good! Just a couple of minor suggestions:

::: gfx/layers/apz/test/gtest/TestPinching.cpp
@@ +156,5 @@
> +  }
> +
> +  void twoFingerPan() {
> +    // Greater than 1/32 inches
> +    ScreenCoord panDistance = (1.0 / 32.0) * 1.2 * tm->GetDPI();

We can make the test a bit more robust in the presence of changes to the pinch-locking preferences, by changing the (1.0 / 32.0) to gfxPrefs::APZPinchLockScrollLockThreshold().

@@ +171,5 @@
> +  }
> +
> +  void twoFingerZoom() {
> +    // Greater than 1/32 inches
> +    float pinchDistance = (1.0 / 32.0) * 1.2 * tm->GetDPI();

And here, gfxPrefs::APZPinchLockSpanBreakoutThreshold().

@@ +187,5 @@
> +    FrameMetrics originalMetrics = apzc->GetFrameMetrics();
> +
> +    // Send a small scale input to the APZC
> +    // Less than 1/32 inches
> +    float pinchDistance = (1.0 / 32.0) * 0.8 * tm->GetDPI();

Here also gfxPrefs::APZPinchLockSpanBreakoutThreshold().

	Botond Ballo [:botond] Reporter
	Comment 11 • 7 years ago

(In reply to jlogandavison from comment #9)
> I had a play with the idea of parameterizing these tests, but ultimately
> left it out of this patch. It was easy enough to create parameterized test
> cases for each of the existing test scenarios (using the TEST_P macro) and I
> was able to instantiate the test cases with FREE, NORMAL, and STICKY locking
> modes.
> 
> The problem was that I couldn't figure out a nice way to map the locking
> modes to the expected outcomes of each test case.
> 
> I think parameterization would be a nice way to ensure that all cases are
> covered for all modes. Do you think it's worth trying to figure it out?

It's an interesting idea, but I think our efforts are better spent improving test coverage for other scenarios.

I'm mentoring another bug that involves writing a gtest, as it happens it's also related to two-finger gestures: bug 1355656. You could work on that if you're interested, seems like it would be right up your alley given your experience with bug 1180865 and this bug.

There's also the issue described in comment 8; we could pursue a fix for that as well (in a new bug) if you're interested.

	jlogandavison Assignee
	Comment 12 • 7 years ago

Adjustments made as requested.

	jlogandavison Assignee
	Comment 13 • 7 years ago

(In reply to Botond Ballo [:botond] from comment #11)
> It's an interesting idea, but I think our efforts are better spent improving
> test coverage for other scenarios.
>
> I'm mentoring another bug that involves writing a gtest, as it happens it's
> also related to two-finger gestures: bug 1355656. You could work on that if
> you're interested, seems like it would be right up your alley given your
> experience with bug 1180865 and this bug.

Yep, fair enough. I'm definitely available to jump onto something new, so anything you can throw my way is great!

Reading through the comments on that bug, although it isn't assigned, it seems there was somebody working on it not too long ago. Are we sure it's fine for me to jump onto that one?

> There's also the issue described in comment 8; we could pursue a fix for
> that as well (in a new bug) if you're interested.

Absolutely! I'll take a look.

	Botond Ballo [:botond] Reporter
	Comment 14 • 7 years ago

(In reply to jlogandavison from comment #13)
> (In reply to Botond Ballo [:botond] from comment #11)
> > It's an interesting idea, but I think our efforts are better spent improving
> > test coverage for other scenarios.
> >
> > I'm mentoring another bug that involves writing a gtest, as it happens it's
> > also related to two-finger gestures: bug 1355656. You could work on that if
> > you're interested, seems like it would be right up your alley given your
> > experience with bug 1180865 and this bug.
> 
> Yep, fair enough. I'm definitely available to jump onto something new, so
> anything you can throw my way is great!
> 
> Reading through the comments on that bug, although it isn't assigned, it
> seems there was somebody working on it not too long ago. Are we sure it's
> fine for me to jump onto that one?

I double-checked with the other potential contributor in the bug, it's free to be taken.

	Botond Ballo [:botond] Reporter
	Comment 15 • 7 years ago

Comment on attachment 8961196 [details] [diff] [review]
patch.diff

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

Thanks!

	Botond Ballo [:botond] Reporter
	Comment 16 • 7 years ago

Pushed to Try to make sure the patch builds and the new gtests are passing:

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

	Pulsebot
	Comment 17 • 7 years ago

Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3b0b2b6c4a1
Tests for APZC pinch locking mechanism. r=botond

	Botond Ballo [:botond] Reporter
	Comment 18 • 7 years ago

(In reply to Botond Ballo [:botond] from comment #16)
> Pushed to Try to make sure the patch builds and the new gtests are passing:
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=651065e10746ac284619da422a282a8c994998d5

Looks good, I went ahead and pushed the patch to mozilla-inbound.

	Narcis Beleuzu [:NarcisB]
	Comment 19 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/a3b0b2b6c4a1

	Botond Ballo [:botond] Reporter
	Comment 20 • 7 years ago

All right, looks like we're all done here. Thanks for your work here, jlogandavison!

We've already discussed possible next steps in comment 11 / comment 13, so I'll leave you to try one of those if you'd like :)

	Botond Ballo [:botond] Reporter
	Comment 21 • 7 years ago

(In reply to Botond Ballo [:botond] from comment #8)
> While playing around with the tests, I also realized that our pinch lock
> implementation has a deficiency: it makes the lock / unlock decisions based
> on the span change and focus change between one event and the next. If you
> have sufficiently sensitive hardware, it might send events frequently enough
> that the change between one event and the next won't be larger than our
> thresholds even if the movement (over multiple events) is relatively large.
> It may be better to make the decisions based on some sort of *cumulative*
> span and focus changes over multiple events.
> 
> Let's not worry about that in this bug, but we could enhance our
> implementation to deal with this in a follow-up.

I filed bug 1451461 for this.