Currently we have nothing to test APZC code. We need to be able to add tests for non trivial cases for APZC that is very difficult to test by hand.

Here is a rebased version of attachment 750017 [details] [diff] [review]. It may contain build errors; I'm still building it.

Ok, this one is rebased *and* compiles *and* runs and passes. In my rebase I took out the -O0 optimize flags since it looked like those weren't meant to be left in.

Comment on attachment 765449 [details] [diff] [review] Add APZC tests, rebased Review of attachment 765449 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/tests/gtest/TestTiledLayerBuffer.cpp @@ +65,5 @@ > ASSERT_EQ(buffer.RoundDownToTileEdge(-10), -256); > } > > TEST(TiledLayerBuffer, EmptyUpdate) { > + nsRegion::InitStatic(); Since I wrote this patch I added ScopedXPCOM to GTestRunner so this init and shutdown is no longer needed.

Fixed up so we don't land the wrong version: https://tbpl.mozilla.org/?tree=Try&rev=63c0d5bafc57

Comment on attachment 765456 [details] [diff] [review] Add APZC tests, rebased v2 Review of attachment 765456 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comments addressed ::: gfx/layers/ipc/AsyncPanZoomController.cpp @@ +108,5 @@ > > +static TimeStamp sFrameTime; > + > +static TimeStamp > +GetFrameTime() { I don't think "GetFrameTime" is the best name for this method. Maybe "GetCurrentTime" instead? ::: gfx/layers/ipc/AsyncPanZoomController.h @@ +252,5 @@ > nsEventStatus HandleInputEvent(const InputData& aEvent); > > + /** > + * Sync panning and zomming animation using a fixed frame time. > + **/ s/zomming/zooming/. I don't know if this comment is that helpful; I would rather just say "Sets the current time used by APZC; used for testing purposes." or something. Also there should be only one star in the closing '*/' ::: gfx/tests/gtest/TestAsyncPanZoomController.cpp @@ +26,5 @@ > + MOCK_METHOD2(SendAsyncScrollDOMEvent, void(const CSSRect &aContentRect, const CSSSize &aScrollableSize)); > + MOCK_METHOD2(PostDelayedTask, void(Task* aTask, int aDelayMs)); > +}; > + > +class TestContainerLayer: public ContainerLayer { nit: space before ':' @@ +61,5 @@ > + > + mti = MultiTouchInput(MultiTouchInput::MULTITOUCH_START, aTime); > + aTime += TIME_BETWEEN_TOUCH_EVENT; > + // Make sure the move is large enough to not be handled as a tap > + mti.mTouches.AppendElement(SingleTouchData(0, ScreenIntPoint(10, aTouchStartY+OVERCOME_TOUCH_TOLERANCE), ScreenSize(0, 0), 0, 0)); Is this OVERCOME_TOUCH_TOLERANCE thing really needed here? I don't like hiding it inside a helper function like this because it's a large (100px) change to the values passed in. The caller might need to know about a change this large and that would break encapsulation.

Regarding the frame time. Right now this fix up is only for testing but later I think we need to propagate the concept of frame time. Preparing a frame might take longer then 1ms so if each substep of the compositing is grabbing it's time stamps each sub stage will have a slightly larger time stamp. Then for example the CSS animation and pan state wont all have the same timestamp causing them to drift in and out of sync causing type of problems similar to the janky panning on Fennec. This just takes us towards the right direction in fixing this.

Ok, that makes sense.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #8) > @@ +61,5 @@ > > + > > + mti = MultiTouchInput(MultiTouchInput::MULTITOUCH_START, aTime); > > + aTime += TIME_BETWEEN_TOUCH_EVENT; > > + // Make sure the move is large enough to not be handled as a tap > > + mti.mTouches.AppendElement(SingleTouchData(0, ScreenIntPoint(10, aTouchStartY+OVERCOME_TOUCH_TOLERANCE), ScreenSize(0, 0), 0, 0)); > > Is this OVERCOME_TOUCH_TOLERANCE thing really needed here? I don't like > hiding it inside a helper function like this because it's a large (100px) > change to the values passed in. The caller might need to know about a change > this large and that would break encapsulation. Regarding OVERCOME_TOUCH_TOLERANCE. This value is entirely ignored for now so there isn't a need for the outside to know it. Perhaps we should file a bug to fix this since I do believe it's a bug. That is when you transition between these two APZC state the delta position for the event that crosses this threshold is ignored. In practice I think events are so fine that this bug doesn't have any impact so I haven't prioritized fixing it. What do you think about leaving this as-is and filling a bug about it. IMO it falls outside the scope of multi-APZC so I don't plan on fixing it.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #8) > s/zomming/zooming/. I don't know if this comment is that helpful; I would > rather just say "Sets the current time used by APZC; used for testing > purposes." or something. Also there should be only one star in the closing > '*/' Extended to: 254 /** 255 * Sync panning and zooming animation using a fixed frame time. 256 * This will ensure that we animate the APZC correctly with other external 257 * animations to the same timestamp. 258 **/

Backed out for: https://tbpl.mozilla.org/php/getParsedLog.php?id=24429531&tree=Mozilla-Inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/511fab5a9157

For some reason Linux doesn't like it when gtest isn't included first. We will need to investigate why. In the meantime I reland this with a working include order.