Some APZ gtests incorrectly cast AsyncPanZoomController* to TestAsyncPanZoomController*

RESOLVED FIXED in mozilla37

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: kats, Assigned: kats)

Tracking

unspecified
mozilla37
All
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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.
Blocks: 1109677
We could give APZCTreeManager a virtual CreateAPZC(...) function which creates a new AsyncPanZoomController, and override it in TestAPZCTreeManager to create TestAsyncPanZoomControllers.
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.
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?
Attachment #8534609 - Flags: review?(botond)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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 on attachment 8534609 [details] [diff] [review]
Patch

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

I'm fine with doing this for now.
Attachment #8534609 - Flags: review?(botond) → review+
(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
https://hg.mozilla.org/mozilla-central/rev/149096da07e5
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.