1160642 - add reftest-async-zoom to apply an async zoom before taking snapshot

Status: RESOLVED FIXED

(Testing :: Reftest, defect)

Description

[Timothy Nikkel (:tnikkel)]

reftest-async-zoom is like reftest-async-scroll-x/y except for zoom. It is a handy addition to a sadly under-tested part of gecko: async panning/zooming.

Comment 1

[Timothy Nikkel (:tnikkel)]

Attachment: patch

Need at least an iid bump and some annotations to the reftest-sanity reftests that test the new feature to make them only run when we have an async pan zoom controller to do the requested zoom.

Comment 2

[Markus Stange [:mstange]]

So, do we also need something for testing stationary APZ-style zoom, if reftest-zoom is full page Desktop-style zoom?

Comment 3

[Timothy Nikkel (:tnikkel)]

(In reply to Markus Stange [:mstange] from comment #2)
> So, do we also need something for testing stationary APZ-style zoom, if
> reftest-zoom is full page Desktop-style zoom?

Yes, we do. But that will have to be another bug.

Comment 4

[Timothy Nikkel (:tnikkel)]

ni me so I remember to finish this up.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch v2

I made the changes from comment 1 in my local version

Comment 6

[Timothy Nikkel (:tnikkel)]

Attachment: add scrollbar tests using the new feature

I wrote these tests which use reftest-async-zoom.

Comment 7

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8607682 [details] [diff] [review]
add scrollbar tests using the new feature

A patch on bug 1151617 does this better. marking obsolete.

Comment 8

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8607605 [details] [diff] [review]
Patch v2

dbaron, do you want to review/okay adding this reftest feature?

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch v3

Updated this patch to address botond's comment at https://bugzilla.mozilla.org/show_bug.cgi?id=1151617#c21 - it's just a minor change to AsyncPanZoomController.cpp, the rest of the patch is unchanged. My r+ stands.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8608697 [details] [diff] [review]
Patch v3

Not sure which parts I'm being asked to review, so I'll just review the reftest bits.

>+Testing Async Zooming: reftest-async-zoom="<float>"
>+=========================================================
>+
>+When the "reftest-async-zoom" attribute is present on the root element then at
>+the end of the test take the snapshot with the given async zoom on top of any
>+existing zoom. Content is not re-rendered at the new zoom level. This
>+corresponds to the mobile style "pinch zoom" style of zoom. This is unsupported
>+in many configurations.

Maybe mention the asyncZoom in the condition sandbox here?

>+    var zoom = attrOrDefault(contentRootElement, "reftest-async-zoom", 0);
>+    if (zoom != 0) {

Seems like it would make more sense for both of these 0s to be 1s.


r=dbaron

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b5127b7f759

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9b5127b7f759

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

One other thought about this patch -- does anything need to happen to reset this after the test finishes so that things behave normally for the next test?

Comment 14

[Kartikaya Gupta (email:[e-mail])]

I don't think so, because the APZC that ends up with the reftest-asyc-zoom should get destroyed at the end of the test, and the next test will create a new set of APZCs without the extra zoom.