Currently, we're not testing APZC's long tap or preventDefault behavior at all. These tests will go in gfx/tests/gtest/TestAsyncPanZoomController.cpp.

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.

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?

(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.

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.

Review comments addressed, carrying r+.

backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/7cd180ea79fe for not compiling on a decent percentage of platforms: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=31bc303caa56

string -> std::string

Try push: https://tbpl.mozilla.org/?tree=Try&rev=2062a4751212