[cairo] implement clone_similar fallback

[Vladimir Vukicevic]

Carl Worth wrote:
> On Mon, 06 Aug 2007 22:38:59 -0700, Vladimir Vukicevic wrote:
>> Here's a patch that implements fallback for clone_similar; it uses
>> create_similar_scratch and then calls composite with OPERATOR_SOURCE.
> 
> Thanks. This looks quite reasonable. Two minor comments below. Feel
> free to address these as you see fit and then push the change.
> 
>> +    new_surface = _cairo_surface_create_similar_scratch (surface,
>> +							  cairo_surface_get_content (src),
>> +							  width, height);
> ...
>> +    if (status == CAIRO_STATUS_SUCCESS)
>> +	 *clone_out = new_surface;
>> +    else if (new_surface)
>> +	 cairo_surface_destroy (new_surface);
> 
> Part of the "never return NULL" approach means never having to test
> for NULL. So you can legitimately drop the "if (new_surface)"
> here. It's guaranteed to never be NULL, (this holds internally as well
> as across the public interface). Similarly, you should probably set
> *clone_out on failure as well as success. Those two points together
> mean you probably don't need the cairo_surface_destroy code at all,
> (since _create_similar_scratch should only be returning a nil surface
> on error). So I think I'd just replace these last four lines of code
> with simply:
> 
> 	/* If any error occurred, then return the nil surface we received. */
> 	*clone_out = new_surface;

If create_similar_scratch returns a nil surface, I just return its 
internal error directly right after the create_similar.  The problem I'm 
trying to avoid is if you got a valid non-nil surface from 
similar_scratch, but composite() failed for whatever reason -- I don't 
want to return a surface in clone_out *and* return an error (from 
composite), because then the caller would have to know to call 
surface_destroy() on clone_out even if the fallback method returns an 
error.  That seems wrong to me; it seems much simpler to have the 
function return an error only if there was an error, otherwise to return 
a valid surface.  But, the null-check can definitely go (I'm not sure 
what I was thinking when I wrote that, since if it was ever NULL it 
would've crashed in the status check!).

> Finally, isn't there a comment in _cairo_surface_clone_similar that
> should be removed along with this patch?

Yep; I've changed some other stuff in cairo_surface_clone_similar as 
well, I'll post a new patch when we get the above issue resolved.

    - Vlad