Closed Bug 1037591 Opened 6 years ago Closed 6 years ago

Clean up APZ gtests

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

(9 files, 11 obsolete files)

1.80 KB, patch
botond
: review+
Details | Diff | Splinter Review
1.80 KB, patch
kats
: review+
Details | Diff | Splinter Review
10.76 KB, patch
kats
: review+
Details | Diff | Splinter Review
8.42 KB, patch
kats
: review+
Details | Diff | Splinter Review
9.71 KB, patch
botond
: review+
Details | Diff | Splinter Review
12.59 KB, patch
botond
: review+
Details | Diff | Splinter Review
5.58 KB, patch
kats
: review+
Details | Diff | Splinter Review
45.07 KB, patch
botond
: review+
Details | Diff | Splinter Review
2.69 KB, patch
botond
: review+
Details | Diff | Splinter Review
There's a bunch of miscellaneous cleanup I want to do in the APZ gtests.
Assignee: nobody → bugmail.mozilla
Not sure if this is better long-term but it's more consistent at least.
Attachment #8454636 - Flags: feedback?(botond) → review?(botond)
Attachment #8454693 - Attachment is obsolete: true
Attachment #8455315 - Flags: review?(botond)
I can remove an extra #include with this patch.
Attachment #8454612 - Attachment is obsolete: true
Attachment #8454612 - Flags: review?(botond)
Attachment #8455342 - Flags: review?(botond)
Slight update to make the macros more consistent.
Attachment #8454636 - Attachment is obsolete: true
Attachment #8454636 - Flags: review?(botond)
Attachment #8455343 - Flags: review?(botond)
Attachment #8454611 - Flags: review?(botond) → review+
As per our IRC discussion
Attachment #8455342 - Attachment is obsolete: true
Attachment #8455342 - Flags: review?(botond)
Attachment #8455463 - Flags: review?(botond)
Comment on attachment 8455463 [details] [diff] [review]
Part 2 - Merge TestScopedBoolPref and the TouchActionEnabledTester into a general pref solution

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

Thanks!

The macro could deduce the type of the pref, but it's a bit more trouble that it's worth so let's not go there. (If you're interested, the stackoverflow question I asked [1] might make for an interesting read.)

[1] http://stackoverflow.com/questions/24743328/raii-and-deduced-template-arguments

::: gfx/tests/gtest/TestAsyncPanZoomController.cpp
@@ +65,3 @@
>  };
>  
> +#define SCOPED_GFX_PREF(prefBase, prefType, prefValue) \

nit about naming convention consistency: since both SCOPED_GFX_PREF and TestScopedGfxPref are in this test file, either both should be prefixed with "test", or neither. (My preference is neither.)
Attachment #8455463 - Flags: review?(botond) → review+
Comment on attachment 8455343 [details] [diff] [review]
Part 3 - Refactor boilerplate macros

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

To summarize our IRC discussion:

  - Gtest creates a different instance of the XXXTester class for each test.
    Therefore:   

      - The variables declared by the boilerplate macros can be made members of 
        the XXXTester class, as described here [1].

      - The initialization performed by the macros can be done in the
        constructor of the tester class, or in SetUp() which is called by
        the framework immediately after the constructor.

  - The variance between different flavours of the boilerplate macros can
    be handled by using different XXXTester classes, with a common base
    for shared elements.

[1] https://code.google.com/p/googletest/wiki/Primer#Test_Fixtures:_Using_the_Same_Data_Configuration_for_Multiple_Te
Attachment #8455343 - Flags: review?(botond) → review-
Attachment #8454692 - Flags: review?(botond) → review+
Comment on attachment 8455315 [details] [diff] [review]
Part 5 - Split ApzcPan into two parts

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

Is not checking the statuses in other tests that call ApzcPan (Fling, FlingStop, OverScrollPanning etc.) intentional?

::: gfx/tests/gtest/TestAsyncPanZoomController.cpp
@@ +326,5 @@
> +                      bool hasTouchListeners,
> +                      nsTArray<uint32_t>* aAllowedTouchBehaviors)
> +{
> +  nsTArray<nsEventStatus> statuses;
> +  ApzcPan(aApzc, aTreeManager, aTime, aTouchStartY, aTouchEndY, aAllowedTouchBehaviors, &statuses);

Let's add MOZ_ASSERT(statuses.size() == 3);
(In reply to Botond Ballo [:botond] from comment #14)
> Is not checking the statuses in other tests that call ApzcPan (Fling,
> FlingStop, OverScrollPanning etc.) intentional?

Yes. I think the statuses should be checked only in tests that are explicitly checking the status for some purpose. For example, in some touch-action tests the status will actually be different depending on the value of the allowed behavior, and so it makes sense to check the status there. For other tests that don't really care about what happens internally as long as the right user-visible behavior is observed, I think checking the status just makes the test more brittle in that they might break with internal code refactorings. The status is not really relevant in FlingStop tests as long as the fling stops, so why bother checking it? It's just more code that will need to be updated if we change how the status works.

> ::: gfx/tests/gtest/TestAsyncPanZoomController.cpp
> @@ +326,5 @@
> > +                      bool hasTouchListeners,
> > +                      nsTArray<uint32_t>* aAllowedTouchBehaviors)
> > +{
> > +  nsTArray<nsEventStatus> statuses;
> > +  ApzcPan(aApzc, aTreeManager, aTime, aTouchStartY, aTouchEndY, aAllowedTouchBehaviors, &statuses);
> 
> Let's add MOZ_ASSERT(statuses.size() == 3);

Sure, will do.
This is definitely much nicer. I moved this patch to the end of the queue since it was an easier rebase this way.
Attachment #8455343 - Attachment is obsolete: true
Attachment #8455602 - Flags: review?(botond)
Comment on attachment 8455315 [details] [diff] [review]
Part 5 - Split ApzcPan into two parts

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

::: gfx/tests/gtest/TestAsyncPanZoomController.cpp
@@ +326,5 @@
> +                      bool hasTouchListeners,
> +                      nsTArray<uint32_t>* aAllowedTouchBehaviors)
> +{
> +  nsTArray<nsEventStatus> statuses;
> +  ApzcPan(aApzc, aTreeManager, aTime, aTouchStartY, aTouchEndY, aAllowedTouchBehaviors, &statuses);

Possibly even better (because it avoids a dynamic allocation and is explicit that this is a fixed-size array) is to change the parameter of ApzcPan to:

  nsEventStatus (*aOutEventStatuses)[3]

Writing to the array is then as follows:

  if (aOutEventStatuses) {
    (*aOutEventStatuses)[0] = status;  // note the parentheses
  }

And calling the function:

  nsEventStatus statuses[3];
  ApzcPan(..., &statuses);
  EXPECT_EQ(whatever, statuses[0]);
   
If you find the syntax "nsEventStatus (*aOutEventStatuses)[3]" offensive, you can hide it behind a typedef:

  typedef nsEventStatus nsEventStatusArray[3];
  ...
  nsEventStatusArray* aOutEventStatuses
Comment on attachment 8455315 [details] [diff] [review]
Part 5 - Split ApzcPan into two parts

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

r+ since you clarified that not checking the status after some pans is intentional.

I'll leave the nsEventStatus[3] thing up to you.
Attachment #8455315 - Flags: review?(botond) → review+
Comment on attachment 8455316 [details] [diff] [review]
Part 6 - Split ApzcPinchWith* into two parts each

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

::: gfx/tests/gtest/TestAsyncPanZoomController.cpp
@@ +398,5 @@
>  }
>  
> +static void
> +ApzcPinchWithPinchInput(AsyncPanZoomController* aApzc,
> +                        int aFocusX, int aFocusY, float aScale,

I liked having one parameter per line when we can't fit them all on one line (but I don't care very much).

@@ +493,5 @@
>    inputId += 2;
>  }
>  
> +static void
> +ApzcPinchWithTouchInputAndCheckStatus(AsyncPanZoomController* aApzc,

The following pattern (borrowed from the standard library) can cut down on method name length:

struct check_status_t {};
check_status_t check_status;

// the non-checking version
void ApzcPinchWithTouchInput(/* parameters */) { ... }

// the checking version
void ApzcPinchWithTouchInput(/* parameters */, check_status_t) { ... }

// call site for non-checking version
ApzcPinchWithTouchInput(/* arguments */);

// call site for checking version
ApzcPinchWithTouchInput(/* arguments */, check_status);

'check_status' can be reused across the various functions that have checking and non-checking versions. It can also be made the first parameter instead of the last.

Up to you if you like this pattern.
Attachment #8455316 - Flags: review?(botond) → review+
Comment on attachment 8455349 [details] [diff] [review]
Part 7 - Move DoPanTest down to satisfy my OCD

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

::: gfx/tests/gtest/TestAsyncPanZoomController.cpp
@@ +723,5 @@
>  
> +static void
> +DoPanTest(bool aShouldTriggerScroll, uint32_t aBehavior)
> +{
> +  BOILERPLATE_FOR_BASIC_TEST;

This will need to be adjusted.
Attachment #8455349 - Flags: review?(botond) → review+
Comment on attachment 8455602 [details] [diff] [review]
Part 8 (formerly part 3) - Refactor boilerplate code into tester classes

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

::: gfx/tests/gtest/TestAsyncPanZoomController.cpp
@@ +189,5 @@
> +  }
> +
> +protected:
> +  virtual void
> +  SetUp()

I don't think I've seen the return value on a different line for an inline method definition anywhere else in the code.

@@ +220,5 @@
> +  {
> +    apzc->UpdateZoomConstraints(ZoomConstraints(false, false, CSSToScreenScale(1.0f), CSSToScreenScale(1.0f)));
> +  }
> +
> +  AsyncPanZoomController::GestureBehavior mBehavior;

'behavior' is very generic. I'd call this mGestureBehavior.

@@ +596,1 @@
>  TEST_F(AsyncPanZoomControllerTester, Pinch_UseGestureDetector_TouchActionNotAllowZoom) {

Can we use APZCBasicTester for these tests, too (and thus avoid repeating the boilerplate in DoPinchTest)?

If so, I believe we can then unify AsyncPanZoomControllerTester and APZCBasicTester.

@@ +746,5 @@
> +    } else {
> +      EXPECT_CALL(*mcc, SendAsyncScrollDOMEvent(_,_,_)).Times(0);
> +      EXPECT_CALL(*mcc, RequestContentRepaint(_)).Times(0);
> +    }
> +  

If I choose to ignore the fact that there's trailing whitespace here, will you return the favour in future reviews? :p

@@ +776,4 @@
>      EXPECT_EQ(ScreenPoint(), pointOut);
>      EXPECT_EQ(ViewTransform(), viewTransformOut);
> +  
> +    apzc->Destroy();

Can we move apzc->Destroy() to APZCBasicTester::TearDown()?
(In reply to Botond Ballo [:botond] from comment #19)
> The following pattern (borrowed from the standard library) can cut down on
> method name length:

It'll cut down on the method name length, but the call site then has to tack on check_status in the arguments, so I feel like it doesn't add much value. Interesting pattern though.

(In reply to Botond Ballo [:botond] from comment #20)
> > +{
> > +  BOILERPLATE_FOR_BASIC_TEST;
> 
> This will need to be adjusted.

Yup, the latest local version of the patch that I have has the right thing there.

(In reply to Botond Ballo [:botond] from comment #21)
> > +protected:
> > +  virtual void
> > +  SetUp()
> 
> I don't think I've seen the return value on a different line for an inline
> method definition anywhere else in the code.

Hm, I guess you're right. I'll move it all into one line. And I should see if clang-format is usable yet, because I really hate not knowing what the right style is...

> > +
> > +  AsyncPanZoomController::GestureBehavior mBehavior;
> 
> 'behavior' is very generic. I'd call this mGestureBehavior.

Will do.

> @@ +596,1 @@
> >  TEST_F(AsyncPanZoomControllerTester, Pinch_UseGestureDetector_TouchActionNotAllowZoom) {
> 
> Can we use APZCBasicTester for these tests, too (and thus avoid repeating
> the boilerplate in DoPinchTest)?

Yes we can! Another advantage of this method over the macro method. I'll update that.

> 
> If I choose to ignore the fact that there's trailing whitespace here, will
> you return the favour in future reviews? :p

Not a chance :) I already fixed the trailing whitespace in my local copy after I noticed it in the splinter view.

> @@ +776,4 @@
> >      EXPECT_EQ(ScreenPoint(), pointOut);
> >      EXPECT_EQ(ViewTransform(), viewTransformOut);
> > +  
> > +    apzc->Destroy();
> 
> Can we move apzc->Destroy() to APZCBasicTester::TearDown()?

Yup, will do.
Rebased with comments addressed, re-requesting review just as a sanity check.
Attachment #8455315 - Attachment is obsolete: true
Attachment #8455667 - Flags: review?(botond)
Rebased, carrying r+
Attachment #8455349 - Attachment is obsolete: true
Attachment #8455671 - Flags: review+
Rebased and updated, carrying r?

There's only one AsyncPanZoomControllerTester test left, the "Constructor" test. I'm not sure that provides much value actually, I'd be fine with just killing it off.
Attachment #8455602 - Attachment is obsolete: true
Attachment #8455602 - Flags: review?(botond)
Attachment #8455673 - Flags: review?(botond)
Comment on attachment 8455667 [details] [diff] [review]
Part 4 - Split ApzcPan into two parts

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

LGTM!
Attachment #8455667 - Flags: review?(botond) → review+
Comment on attachment 8455670 [details] [diff] [review]
Part 5 - Split the ApzcPinchWith* into two parts each

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

::: gfx/tests/gtest/TestAsyncPanZoomController.cpp
@@ +413,5 @@
>  
>    nsEventStatus expectedStatus = aShouldTriggerPinch
> +      ? nsEventStatus_eConsumeNoDefault
> +      : nsEventStatus_eIgnore;
> +  EXPECT_EQ(statuses[0], expectedStatus);

Let's take this opportunity to correct the order of arguments here (expected is first).
Attachment #8455670 - Flags: review?(botond) → review+
Comment on attachment 8455673 [details] [diff] [review]
Part 7 - Refactor boilerplate code into tester classes

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

I like this very much!

I'd be in favour of removing the Constructor test and unifying APZCBasicTester and AsyncPanZoomControllerTester.
Attachment #8455673 - Flags: review?(botond) → review+
Attachment #8455706 - Flags: review?(botond) → review+
You need to log in before you can comment on or make changes to this bug.