[cairo] [PATCH] OS/2 surface fixes

[M Joonas Pihlaja]

Hi Dave,

On Sat, 17 Jul 2010, Dave Yeo wrote:

> Hi, as part of https://bugzilla.mozilla.org/show_bug.cgi?id=557159 Rich Walsh
> has fixed some issues in cairo-os2-surface. It would be nice to get these
> applied to master before the next release so they can propagate downstream.

I skimmed OS/2 surface code and the patch this morning.  I don't know 
anything about OS/2 interfaces so will defer on those bits and leave 
it at general comments.

I'd really prefer for the patch to be split up into several logically 
independent patches at least.  Code cleanups, optimizations, bug 
fixes, new API... these are all different things and shouldn't be 
rolled into one.

In the small, there are a few bits in the patch I'd not be comfortable 
with commiting:

- The introduction of an unindented do {} while (0) block so that new 
code can break out of the nonloop.  Please use a goto and a 
descriptive label instead.

- There's a case of s/status = CAIRO_STATUS_SUCCESS/status = 0/.

- Missing "Since: 1.10" in the docs (once, it says "Since: 1.6" 
instead.)

- The set_hps() method should return cairo_status_t since it can fail 
rather than some bogus old-HPS.  Do we even need to provide access to 
the old HPS at all?  If we really do, then we should use a separate 
getter instead of conflating setting and getting.

- The set_hps() method doesn't check for the surface being finished.  
Granted, neither do any of the other os2 surface methods, but they 
ought to too.

- It would be good to have some documentation about the ownership 
contracts of the OS/2 objects in the surface constructors and setters.  

The whole per-surface mutex and event semaphore thing in 
cairo-os2-surface.c looks out of place in the backend, since cairo's 
internals don't really support concurrent use of a surface, yet 
cairo_os2_surface_t seems to encourage and even expect it (cf. 
comments concerning the application's PM rendering thread in the 
code).  While this doesn't have anything to do with the patch itself, 
I'm left wondering what kind of concurrency uses the OS/2 surfaces 
actually see which would necessitate the semaphores.

Cheers,

Joonas