Re: [PATCH] xcb: make use of _cairo_surface_is_xcb to check for surface type

Hi, 

Please find attached the updated patch incorporated with review comments
from Uli. 
Thanking him for taking time to review the patch. 
Please help me in reviewing the attached patch.

Thanks and Best Regards, 
N Ravoi

> -----Original Message-----
> From: Uli Schlachter [mailto:[email protected]]
> Sent: Monday, June 30, 2014 6:05 PM
> To: Ravi Nanjundappa; [email protected]
> Subject: Re: [cairo] [PATCH] xcb: make use of _cairo_surface_is_xcb to
check
> for surface type
> 
> Hi,
> 
> On 30.06.2014 14:03, Ravi Nanjundappa wrote:
> > Introduced a new inline function _cairo_surface_is_xcb() as similar to
> > _cairo_surface_is_image() and used the same to check for xcb surface
> > type
> >
> > Signed-off-by: Ravi Nanjundappa <[email protected]>
> > ---
> >  src/cairo-xcb-surface.c |   20 ++++++++++++++++++--
> >  1 file changed, 18 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/cairo-xcb-surface.c b/src/cairo-xcb-surface.c index
> > c900edc..e3230c7 100644
> > --- a/src/cairo-xcb-surface.c
> > +++ b/src/cairo-xcb-surface.c
> > @@ -78,6 +78,22 @@ slim_hidden_proto
> (cairo_xcb_surface_create_with_xrender_format);
> >   * Since: 1.12
> >   **/
> >
> > +
> > +/**
> > + * _cairo_surface_is_xcb:
> > + * @surface: a #cairo_surface_t
> > + *
> > + * Checks if a surface is an #cairo_xcb_surface_t
> > + *
> > + * Return value: %TRUE if the surface is an xcb surface  **/ static
> > +inline cairo_bool_t _cairo_surface_is_xcb (const cairo_surface_t
> > +*surface) {
> > +    /* _cairo_surface_nil sets a NULL backend so be safe */
> > +    return surface->backend && surface->backend->type ==
> > +CAIRO_SURFACE_TYPE_XCB; }
> 
> 
> Does this new "->backend != NULL" check make a difference (= does this fix
> any
> bugs?) or is it just copied from _cairo_surface_is_image()?
> 
> >  cairo_surface_t *
> >  _cairo_xcb_surface_create_similar (void
> 	*abstract_other,
> >  				   cairo_content_t	 content,
> > @@ -1432,7 +1448,7 @@ cairo_xcb_surface_set_size (cairo_surface_t
> *abstract_surface,
> >      }
> >
> >
> > -    if (abstract_surface->type != CAIRO_SURFACE_TYPE_XCB) {
> > +    if (! _cairo_surface_is_xcb (abstract_surface)) {
> >  	_cairo_surface_set_error (abstract_surface,
> >  				  _cairo_error
> (CAIRO_STATUS_SURFACE_TYPE_MISMATCH));
> >  	return;
> > @@ -1486,7 +1502,7 @@ cairo_xcb_surface_set_drawable
> (cairo_surface_t *abstract_surface,
> >      }
> >
> >
> > -    if (abstract_surface->type != CAIRO_SURFACE_TYPE_XCB) {
> > +    if (! _cairo_surface_is_xcb (abstract_surface)) {
> >  	_cairo_surface_set_error (abstract_surface,
> >  				  _cairo_error
> (CAIRO_STATUS_SURFACE_TYPE_MISMATCH));
> >  	return;
> 
> What's special about these two places?
> 
> According to git grep, there are lots of other matches for
> CAIRO_SURFACE_TYPE_XCB, e.g. in cairo-xcb-surface-core.c or cairo-xcb-
> surface-render.c (and in cairo-xlib-xcb-surface.c, but I don't think that
this
> counts). Some of these check ->type, others do source->backend->type.
> 
> Since I don't have much time right now:
> 
> subsurfaces/cairo_surface_create_for_rectangle() creates a surface where -
> >type is the "original" type and ->backend->type is
> CAIRO_SURFACE_TYPE_SUBSURFACE, right? So we should always use -
> >backend->type internally so that subsurfaces can't mess things up, right?
(=
> This patch fixes an actual bug, but doesn't fix all the places where we
have
> this bug)
> 
> Sorry for this chaotic mail, I don't have much time right now.
> 
> Cheers,
> Uli
> --
> Sent from my Game Boy.

-- 
cairo mailing list
[email protected]
http://lists.cairographics.org/mailman/listinfo/cairo

From bb0403d045b2742ad216d753fd2e9eda072600c5 Mon Sep 17 00:00:00 2001
From: Ravi Nanjundappa <[email protected]>
Date: Wed, 2 Jul 2014 14:13:58 +0530
Subject: [PATCH] xcb: make use of _cairo_surface_is_xcb to check for surface
 type

Introduced a new inline function _cairo_surface_is_xcb() as similar to
_cairo_surface_is_image() and used the same to check for xcb surface
type

Signed-off-by: Ravi Nanjundappa <[email protected]>
---
 src/cairo-xcb-private.h        |   15 +++++++++++++++
 src/cairo-xcb-surface-core.c   |    2 +-
 src/cairo-xcb-surface-render.c |    2 +-
 src/cairo-xcb-surface.c        |    4 ++--
 4 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/src/cairo-xcb-private.h b/src/cairo-xcb-private.h
index 5f04803..134100a 100644
--- a/src/cairo-xcb-private.h
+++ b/src/cairo-xcb-private.h
@@ -267,6 +267,21 @@ enum {
 
 cairo_private extern const cairo_surface_backend_t _cairo_xcb_surface_backend;
 
+/**
+ * _cairo_surface_is_xcb:
+ * @surface: a #cairo_surface_t
+ *
+ * Checks if a surface is an #cairo_xcb_surface_t
+ *
+ * Return value: %TRUE if the surface is an xcb surface
+ **/
+static inline cairo_bool_t
+_cairo_surface_is_xcb (const cairo_surface_t *surface)
+{
+    /* _cairo_surface_nil sets a NULL backend so be safe */
+    return surface->backend && surface->backend->type == CAIRO_SURFACE_TYPE_XCB;
+}
+
 cairo_private cairo_xcb_connection_t *
 _cairo_xcb_connection_get (xcb_connection_t *connection);
 
diff --git a/src/cairo-xcb-surface-core.c b/src/cairo-xcb-surface-core.c
index c608c9a..545fd28 100644
--- a/src/cairo-xcb-surface-core.c
+++ b/src/cairo-xcb-surface-core.c
@@ -406,7 +406,7 @@ _cairo_xcb_surface_pixmap (cairo_xcb_surface_t *target,
     if (pixmap != NULL && pixmap->screen == target->screen)
 	return (cairo_xcb_pixmap_t *) cairo_surface_reference (&pixmap->base);
 
-    if (source->type == CAIRO_SURFACE_TYPE_XCB &&
+    if (_cairo_surface_is_xcb(source) &&
 	((cairo_xcb_surface_t *) source)->screen == target->screen)
     {
 	cairo_xcb_surface_t *xcb_source = (cairo_xcb_surface_t *) source;
diff --git a/src/cairo-xcb-surface-render.c b/src/cairo-xcb-surface-render.c
index 75455e5..3f2fc43 100644
--- a/src/cairo-xcb-surface-render.c
+++ b/src/cairo-xcb-surface-render.c
@@ -1154,7 +1154,7 @@ _cairo_xcb_surface_picture (cairo_xcb_surface_t *target,
 
     if (source->type == CAIRO_SURFACE_TYPE_XCB)
     {
-	if (source->backend->type == CAIRO_SURFACE_TYPE_XCB) {
+	if (_cairo_surface_is_xcb(source)) {
 	    cairo_xcb_surface_t *xcb = (cairo_xcb_surface_t *) source;
 	    if (xcb->screen == target->screen && xcb->fallback == NULL) {
 		picture = _copy_to_picture ((cairo_xcb_surface_t *) source);
diff --git a/src/cairo-xcb-surface.c b/src/cairo-xcb-surface.c
index c900edc..3b0c422 100644
--- a/src/cairo-xcb-surface.c
+++ b/src/cairo-xcb-surface.c
@@ -1432,7 +1432,7 @@ cairo_xcb_surface_set_size (cairo_surface_t *abstract_surface,
     }
 
 
-    if (abstract_surface->type != CAIRO_SURFACE_TYPE_XCB) {
+    if (!_cairo_surface_is_xcb(abstract_surface)) {
 	_cairo_surface_set_error (abstract_surface,
 				  _cairo_error (CAIRO_STATUS_SURFACE_TYPE_MISMATCH));
 	return;
@@ -1486,7 +1486,7 @@ cairo_xcb_surface_set_drawable (cairo_surface_t *abstract_surface,
     }
 
 
-    if (abstract_surface->type != CAIRO_SURFACE_TYPE_XCB) {
+    if (!_cairo_surface_is_xcb(abstract_surface)) {
 	_cairo_surface_set_error (abstract_surface,
 				  _cairo_error (CAIRO_STATUS_SURFACE_TYPE_MISMATCH));
 	return;
-- 
1.7.9.5