Closed Bug 864441 Opened 7 years ago Closed 7 years ago

Add APZC tests

Categories

(Core :: Graphics: Layers, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: BenWa, Assigned: BenWa)

References

Details

Attachments

(1 file, 5 obsolete files)

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.
Depends on: 844852
Blocks: multi-apzc
Depends on: 865824
Duplicate of this bug: 872370
Attached patch Add APZC tests. WIP v1 (obsolete) — Splinter Review
Assignee: nobody → bgirard
Attached patch Add APZC tests. WIP v1 (obsolete) — Splinter Review
Attachment #749644 - Attachment is obsolete: true
Attached patch Add APZC tests, rebased (obsolete) — Splinter Review
Here is a rebased version of attachment 750017 [details] [diff] [review]. It may contain build errors; I'm still building it.
Attached patch Add APZC tests, rebased (obsolete) — Splinter Review
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.
Attachment #750017 - Attachment is obsolete: true
Attachment #765408 - Attachment is obsolete: true
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.
Attachment #765449 - Flags: review?(bugmail.mozilla)
Attached patch Add APZC tests, rebased v2 (obsolete) — Splinter Review
Fixed up so we don't land the wrong version:

https://tbpl.mozilla.org/?tree=Try&rev=63c0d5bafc57
Attachment #765449 - Attachment is obsolete: true
Attachment #765449 - Flags: review?(bugmail.mozilla)
Attachment #765456 - Flags: review?(bugmail.mozilla)
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.
Attachment #765456 - Flags: review?(bugmail.mozilla) → review+
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.
(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    **/
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.
https://hg.mozilla.org/mozilla-central/rev/19df9215494e
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.