673230 - Refactor thebes surface classes to use static creation functions instead of constructors

Status: RESOLVED WONTFIX

(Core :: Graphics, defect)

Comment 1

[Josh Matthews [:jdm]]

Ok, so I've been experimenting with just changing gfxImageSurface right now to make Open and OpenInfallible available, with Open returning null if CheckSurfaceSize fails or the resulting surface has a non-zero CairoStatus result. OpenInfallible attempts to create a 0x0 cairo surface, which I assumed would be an error surface.

First, I discovered that places that look like they're set up to handle errors (proper checks of CairoStatus already in place) are not necessarily ready to be converted to use Open. The first time I tried this, the libpr0n delaytest crashtest timed out because it wasn't expecting canvas.drawImage to throw an error, which occurs because several places now returned null pointers instead of seemingly-valid gfxImageSurfaces.

When I made the relevant places use OpenInfallible instead, it passed, but it turns out that a 0x0 surface gets turned into 1x1 by cairo, so they actually were merrily carrying around perfectly valid surfaces. I suspect this is not the outcome we want.

I tried making OpenInfallible create -1x-1 surfaces in the case when OpenInfallible is used to create surfaces that fail CheckSurfaceSize, and that gave us properly invalid but existing cairo surfaces, and the crashtest merrily passed (now lacking two assertions about unsing null cairo surfaces, to boot).

The last option was promising, but I feel uneasy about simply changing instances of Open to OpenInfallible until tests pass. I also feel uneasy about deleting failure checks that break tests when failures do occur.

Thoughts?

Comment 2

[Josh Matthews [:jdm]]

Oh, I forgot, the -1x-1 panacea is not actually that. Creating a -1x-1 surface does create a valid error cairo surface, but the gfxASurface Init function checks for that and early-returns, which leaves us exactly where we were before. Bleah.

The other thing I've noticed is that some functions in gfxImageSurface check mSurfaceValid, while others don't. I guess that's something that should be corrected. I feel like I'm kind of going in circles here.

Comment 3

[Josh Matthews [:jdm]]

I tried to do this and failed. We decided that it was better to simply ensure that cairo surfaces aren't null instead of trying to avoid situations in which they could be.