[cairo] [cairo-commit] 8 commits - configure.ac src/cairo-debug.c src/cairo.h src/cairoint.h src/cairo-jpeg-info.c src/cairo-jpeg-info-private.h src/cairo-misc.c src/cairo-mutex-list-private.h src/cairo-pdf-surface.c src/cairo-ps-surface.c src/cairo-surface.c src/cairo-surface-fallback.c src/cairo-surface-private.h src/cairo-types-private.h test/Makefile.am test/mime-data.c test/mime-data.ref.png test/romedalen.jpg util/cairo-trace

[Chris Wilson]

On Mon, 2008-11-03 at 18:30 -0500, Behdad Esfahbod wrote:
> Chris Wilson wrote:
> >     [trace] Autodetect -lbfd during configure
> >     Stop being lazy and detect libbfd during configure.
> 
> Maybe update util/Makefile.am accordingly to use this info?
> 
> > +AC_CHECK_LIB(bfd, bfd_openr,
> > +	 [AC_CHECK_HEADER(bfd.h, [have_bfd=yes],
> > +	 [have_bfd="no (requires binutils-dev)"])],
> > +	 [have_bfd="no (requires binutils-dev)"])
> > +if test "x$have_bfd" = "xyes"; then
> > +    AC_DEFINE([HAVE_BFD], [1], [Define to 1 if you have binutils-dev installed])
> 
> "binutils-dev" -> Avoid Debianism.

;-) Sorry 'bout that.


> > +    _cairo_intern_string_reset_static_data ();
> 
> This is unsafe, but we can't do better without adding refcounts to the
> interned strings.

AIUI, this is no less safe than all the other callees of
cairo_debug_reset_static_data() - the user must guarantee that he holds
no live cairo objects, so all we have to worry about is whether any of
the cached objects might accidentally construct a intern string during
their user_data destroy notifiers - which we can cease to worry about if
we order the reset carefully.

> > +    /* XXX Need to copy all known image representations...
> > +     * For now, just copy "image/jpeg", but in future we should construct
> > +     * an array of known types and iterate.
> > +     */
> 
> If we add ref counts to mime_data objects, we don't have to copy, right?
Right, that still complies with the expected lifetime semantics.

> > @@ -638,23 +658,56 @@ cairo_surface_get_mime_data (cairo_surface_t		*surface,
> >   * and %NULL for @data.
> >   *
> >   * Since: 1.10
> > + *
> > + * Return value: %CAIRO_STATUS_SUCCESS or %CAIRO_STATUS_NO_MEMORY if a
> > + * slot could not be allocated for the user data.
> >   **/
> > -void
> > +cairo_status_t
> >  cairo_surface_set_mime_data (cairo_surface_t		*surface,
> >                               const char			*mime_type,
> >                               const unsigned char	*data,
> >                               unsigned int		 length,
> >  			     cairo_destroy_func_t	 destroy)
> >  {
> > +    cairo_status_t status;
> > +    cairo_mime_data_t *mime_data;
> > +
> > +    if (surface->status)
> > +	return surface->status;
> >  
> > +    status = _cairo_intern_string (&mime_type, -1);
> > +    if (status)
> > +	return _cairo_surface_set_error (surface, status);
> > +
> > +    mime_data = _cairo_user_data_array_get_data (&surface->user_data,
> > +						 (cairo_user_data_key_t *) mime_type);
> > +    if (mime_data != NULL) {
> > +	if (mime_data->destroy && mime_data->data)
> > +	    mime_data->destroy (mime_data->data);
> 
> The freeing should be automatically handled by user_data facilities when
> setting a new value, no?

As it stands, we didn't replace (and reallocate) the user data slot, but
reused the existing mime data. However, after switching to a ref-counted
mime data, it will be cleaner just to use the replace the user data slot
and perform the destroy from the user data notifier.

Thanks for the review and further suggestions.
-- 
Chris