Closed Bug 970578 Opened 6 years ago Closed 6 years ago

Add tests for preventDefault and long tap behavior

Categories

(Core :: Panning and Zooming, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: drs, Assigned: drs)

References

Details

Attachments

(1 file, 3 obsolete files)

Currently, we're not testing APZC's long tap or preventDefault behavior at all. These tests will go in gfx/tests/gtest/TestAsyncPanZoomController.cpp.
Depends on: 964421
QA Contact: drs+bugzilla
Assignee: nobody → drs+bugzilla
QA Contact: drs+bugzilla
I added tests for panning with preventDefault, long press with preventDefault, and I made the regular long press test more precise. Thanks for being patient and letting me do this as a follow-up.
Attachment #8375583 - Flags: review?(bugmail.mozilla)
Comment on attachment 8375583 [details] [diff] [review]
Add tests for preventDefault and long tap behavior to gtest.

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

When I apply this patch to latest m-c code and run it I get this warning:

 0:31.02 TEST-START | AsyncPanZoomController.PanWithPreventDefault
 0:31.02 [81983] WARNING: Received impossible touch in OnTouchStart: file /Users/kats/zspace/mozilla-git/gfx/layers/ipc/AsyncPanZoomController.cpp, line 656
 0:31.02 TEST-PASS | AsyncPanZoomController.PanWithPreventDefault | test completed (time: 0ms)

and also this failure:

 0:31.02 TEST-START | AsyncPanZoomController.LongPressPreventDefault
 0:31.02 Assertion failure: !IsNull() (Cannot compute with a null value), at ../../../dist/include/mozilla/TimeStamp.h:283
 0:31.02 make: *** [gtest] Error 1

Is this the right version of the patch? It applied cleanly for me so I don't think it's a rebase/merge problem on my end. Also I haven't tried it on 1.3, is that where you tested it?
Attachment #8375583 - Flags: review?(bugmail.mozilla) → review-
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #2)
> Is this the right version of the patch? It applied cleanly for me so I don't
> think it's a rebase/merge problem on my end. Also I haven't tried it on 1.3,
> is that where you tested it?

Yep! I just derped and didn't test with debug enabled. I'll clean it up and come back with it.
Cleaned up, shouldn't have any warnings or assertions anymore.
Attachment #8375583 - Attachment is obsolete: true
Attachment #8376469 - Flags: review?(bugmail.mozilla)
Comment on attachment 8376469 [details] [diff] [review]
Add tests for preventDefault and long tap behavior to gtest.

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

r=me with comments addressed.

::: gfx/tests/gtest/TestAsyncPanZoomController.cpp
@@ +580,5 @@
> +
> +  FrameMetrics frameMetrics(TestFrameMetrics());
> +  frameMetrics.mMayHaveTouchListeners = true;
> +
> +  apzc->SetTouchActionEnabled(true);

nit: move this line down to where you call SetAllowedTouchBehavior. (Or move that stuff up here). Since the touch action enabling goes hand-in-hand with the allowed touch behavior i'd like those to be together.

@@ +727,5 @@
>  
>    nsEventStatus status = ApzcDown(apzc, 10, 10, time);
>    EXPECT_EQ(nsEventStatus_eConsumeNoDefault, status);
>  
> +  MockFunction<void(string check_point_name)> check;

nit: checkPointName (camelcase).

Also leave a blank line between this and the opening brace on next line and/or add a comment that the brace is scoping for the InSequence object. Otherwise this looks like some sort of crazy c++ inner function definition using magical template things.

Nice work with this pre/post stuff though - it definitely tightens the checks on when the longtap/longtapup calls are supposed to happen.

@@ +790,5 @@
> +  EXPECT_EQ(nsEventStatus_eConsumeNoDefault, status);
> +
> +  MockFunction<void(string check_point_name)> check;
> +  {
> +    InSequence s;

ditto here about the brace/scoping comment.

@@ +808,5 @@
> +  mcc->ClearDelayedTask();
> +
> +  // Send the signal that content has handled this long tap. This takes the
> +  // place of the "contextmenu" event.
> +  apzc->ContentReceivedTouch(true);

Move the ClearDelayedTask to be grouped with this line, and update the comment to say that we have to clear the waiting-for-content timeout task when we simulate the response from content.

@@ +813,5 @@
> +
> +  time += 1000;
> +
> +  MultiTouchInput mti = MultiTouchInput(MultiTouchInput::MULTITOUCH_MOVE, time, 0);
> +  mti.mTouches.AppendElement(SingleTouchData(0, ScreenIntPoint(touchStart, touchEnd), ScreenSize(0, 0), 0, 0));

It's a little confusing to be using (touchStart, touchEnd) as x/y coordinates. I'd rather you just replaced all the x-coordinates with "10", and only use touchStart/touchEnd for the y-coordinates.

@@ +820,5 @@
> +
> +  status = ApzcUp(apzc, touchStart, touchEnd, time);
> +  EXPECT_EQ(nsEventStatus_eIgnore, status);
> +
> +  // Flush the event queue. Even though we're not marketing the queue as

s/marketing/marking/. I don't fully understand this comment though.
Attachment #8376469 - Flags: review?(bugmail.mozilla) → review+
Review comments addressed, carrying r+.
Attachment #8376469 - Attachment is obsolete: true
Attachment #8376545 - Flags: review+
string -> std::string
Attachment #8376545 - Attachment is obsolete: true
Attachment #8376715 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/3305c1509a28
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Depends on: 973660
You need to log in before you can comment on or make changes to this bug.