1109855 - Some APZ gtests incorrectly cast AsyncPanZoomController* to TestAsyncPanZoomController*

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

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

Some gtests, such as HitTesting1, incorrectly cast a AsyncPanZoomController to a TestAsyncPanZoomController. This happens because UpdatePanZoomControllerTree is called which directly instantiates AsyncPanZoomController instances. The test code however assumes that pretty much all the APZCs in sight are TestAsyncPanZoomControllers, and the ApzcOf function does a cast which relies on this.

Comment 1

[Botond Ballo [:botond]]

We could give APZCTreeManager a virtual CreateAPZC(...) function which creates a new AsyncPanZoomController, and override it in TestAPZCTreeManager to create TestAsyncPanZoomControllers.

Comment 2

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

Yes, I already have a patch for this (and the other bugs I'm filing). I just need bug numbers to put in commit messages.

Comment 3

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

Attachment: Patch

I wanted to put the GestureBehaviour parameter on MakeAPZCInstance as well but it seemed to require #include'ing AsyncPanZoomController.h into APZCTreeManager.h which I didn't really want to do. Is there some way around that?

Comment 4

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #3)
> Created attachment 8534609 [details] [diff] [review]
> Patch
> 
> I wanted to put the GestureBehaviour parameter on MakeAPZCInstance as well
> but it seemed to require #include'ing AsyncPanZoomController.h into
> APZCTreeManager.h which I didn't really want to do. Is there some way around
> that?

I don't have a great solution, but a couple of ideas for the record:

  1) Move GestureBehavior out into namespace scope and into a separate header 
     (or an existing header you're willing to include in APZCTreeManager.h).

  2) Introduce an APZCFactory interface and have APZCTreeManager's constructor
     take a pointer to that. The signature that includes GestureBehaviour then
     goes into the definition of APZCFactory, which APZCTreeManager.h doesn't
     need to see.

Comment 5

[Botond Ballo [:botond]]

Comment on attachment 8534609 [details] [diff] [review]
Patch

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

I'm fine with doing this for now.

Comment 6

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

(In reply to Botond Ballo [:botond] from comment #4)
>   1) Move GestureBehavior out into namespace scope and into a separate
> header 
>      (or an existing header you're willing to include in APZCTreeManager.h).

This would be my preferred alternative of the two, but for now I just left it as-is. Maybe later if we have more dependencies we can revisit it.

https://hg.mozilla.org/integration/mozilla-inbound/rev/149096da07e5

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/149096da07e5