Closed Bug 1031024 Opened 6 years ago Closed 6 years ago

gtests need to be able to flip prefs

Categories

(Core :: Panning and Zooming, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(2 files, 2 obsolete files)

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.
Assignee: nobody → bugmail.mozilla
Attachment #8447233 - Flags: review?(drs+bugzilla)
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 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.
Attachment #8447233 - Flags: review?(drs+bugzilla)
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.
Attachment #8447234 - Flags: review?(drs+bugzilla)
(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.
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.
Attachment #8447233 - Attachment is obsolete: true
Attachment #8447386 - Flags: review?(drs+bugzilla)
Updated so that we cancel the overscrolling fling animation instead of the non-overscrolling fling animation, to be more representative. Also updated comments.
Attachment #8447234 - Attachment is obsolete: true
Attachment #8447387 - Flags: review?(drs+bugzilla)
Attachment #8447386 - Flags: review?(drs+bugzilla) → review+
Attachment #8447387 - Flags: review?(drs+bugzilla) → review+
https://hg.mozilla.org/mozilla-central/rev/2c765580762d
https://hg.mozilla.org/mozilla-central/rev/80f5c6e2282c
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.