1031024 - gtests need to be able to flip prefs

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

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

Right now there's no easy way to write gtests for overscroll behaviour because overscroll is preffed off by default (and only enabled in a B2G environment). If the gtests had the ability to turn on prefs then we could test it.

Also, the OverScrollPanning test that's in there now seems kinda bogus because it never actually reaches an overscroll state, because overscroll is disabled. So I don't really know if that's testing anything useful.

Comment 1

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

Attachment: Part 1 - Add a scoped pref changer and fix the existing overscroll test

Comment 2

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

Attachment: Part 2 - Add a test for bug 1030221

Comment 3

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

Btw in part 1, the "non-overscrolling fling animation" is the one at [1] and the "overscrolling fling animation" is the one at [2]. It does seem kinda clunky that we do it this way but if we want to change it I'd rather do it in a separate bug. I can't think of an easy way to change it without breaking the handoff behaviour either.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp?rev=2b7a95f55ad0#1100
[2] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp?rev=2b7a95f55ad0#1682

Comment 4

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8447233 [details] [diff] [review]
Part 1 - Add a scoped pref changer and fix the existing overscroll test

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

::: gfx/tests/gtest/TestAsyncPanZoomController.cpp
@@ +59,5 @@
>    MOCK_METHOD2(PostDelayedTask, void(Task* aTask, int aDelayMs));
>  };
>  
> +template<typename T>
> +class ScopedPref {

What do you think of naming this "ScopedTestPref" or something instead? This seems a little too vague to me.

@@ +68,5 @@
> +    if (mozilla::IsSame<T, bool>::value) {
> +      mOldVal = Preferences::GetBool(aPref);
> +      Preferences::SetBool(aPref, aVal);
> +    } else {
> +      // TODO: add support for other types as needed

I think we should just add support for bools, ints, and strings now, since it won't take that long. We can deal with other types like "complex" later if we ever end up using those.

@@ +69,5 @@
> +      mOldVal = Preferences::GetBool(aPref);
> +      Preferences::SetBool(aPref, aVal);
> +    } else {
> +      // TODO: add support for other types as needed
> +      EXPECT_TRUE(false);

nit: I think it makes more sense to fatally error here, since this is basically equivalent to a compiler error, so we should use ASSERT_TRUE instead.

@@ +82,5 @@
> +      EXPECT_TRUE(false);
> +    }
> +  }
> +
> +private:

Maybe make these protected instead, in case we ever want to derive this.

@@ +913,5 @@
>    DoFlingStopTest(true);
>  }
>  
>  TEST_F(AsyncPanZoomControllerTester, OverScrollPanning) {
> +  ScopedPref<bool> overscrollPref("apz.overscroll.enabled", true);

nit: s/overscrollPref/overscrollEnabledPref/

@@ +944,5 @@
>    ViewTransform viewTransformOut;
>  
> +  // This sample will run to the end of the non-overscrolling fling animation
> +  // and will schedule the overscrolling fling animation.
> +  apzc->SampleContentTransformForFrame(testStartTime+TimeDuration::FromMilliseconds(10000), &viewTransformOut, pointOut);

nit: everywhere else in this file, we have spaces between additions of timestamps/durations.

Comment 5

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8447234 [details] [diff] [review]
Part 2 - Add a test for bug 1030221

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

I just want to discuss my second comment here and then this will get r+.

::: gfx/tests/gtest/TestAsyncPanZoomController.cpp
@@ +964,5 @@
>    apzc->Destroy();
>  }
>  
> +TEST_F(AsyncPanZoomControllerTester, OverScrollAbort) {
> +  ScopedPref<bool> overscrollPref("apz.overscroll.enabled", true);

nit: s/overscrollPref/overscrollEnabledPref/

@@ +984,5 @@
> +  ApzcPan(apzc, tm, time, touchStart, touchEnd);
> +  EXPECT_TRUE(apzc->IsOverscrolled());
> +
> +  // At this point, we are in the non-overscrolling fling animation but with
> +  // some overscroll already. Check that canceling the animtion clears the overscroll.

This comment is really unclear, and there are a couple of typos. I'm not sure what "non-overscrolling fling animation" means here. Do you mean the snapback animation? My understanding is that we can't enter a non-overscrolling fling unless we were handed off the fling, which isn't happening here.

Comment 6

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

(In reply to Doug Sherk (:drs) from comment #5)
> I'm not
> sure what "non-overscrolling fling animation" means here. Do you mean the
> snapback animation? My understanding is that we can't enter a
> non-overscrolling fling unless we were handed off the fling, which isn't
> happening here.

Same thing as I mentioned in comment 3. Basically, when we get a touch-end, we kick off a fling animation that is not allowed to overscroll (which I refer to as a "non-overscrolling fling animation"). It's not allowed to overscroll because it is instead supposed to hand off any excess fling amounts up the handoff chain. When it gets to the end of the handoff chain, the last APZC in the chain (which might be the same APZC as the one that started the fling) then initiates a fling animation that *is* allowed to overscroll ("overscrolling fling animation").

So right after the ApzcPan, we will have a non-overscrolling fling animation as the active animation. After we do a SampleContentTransformForFrame, that animation will terminate itself and initiate an overscrolling fling animation on the appropriate APZC. However this test cancels the non-overscrolling fling animation before it gets around to doing that.

Comment 7

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

Attachment: Part 1 - Add a scoped pref changer and fix the existing overscroll test (v2)

Updated as per IRC discussion, to TestScopedBoolPref because the template approach doesn't actually work. I left the members private for now since we'll probably have different classes for different types and we can easily change it to protected if we ever need to. Fixed the naming and spacing as well.

Comment 8

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

Attachment: Part 2 - Add a test for bug 1030221 (v2)

Updated so that we cancel the overscrolling fling animation instead of the non-overscrolling fling animation, to be more representative. Also updated comments.

Comment 9

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

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2c765580762d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/80f5c6e2282c

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2c765580762d
https://hg.mozilla.org/mozilla-central/rev/80f5c6e2282c