[webkit-reviews] review denied: [Bug 57717] Convert use of raw pointers to RefPtr in using Cairo : [Attachment 89407] Proposed Patch7

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Apr 13 11:58:02 PDT 2011

Martin Robinson <mrobinson at webkit.org> has denied Joone Hur
<joone.hur at collabora.co.uk>'s request for review:
Bug 57717: Convert use of raw pointers to RefPtr in using Cairo

Attachment 89407: Proposed Patch7

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=89407&action=review

I apologize that this change has balooned from its original intent. I
understand if you wish to separate out the parts of the change into separate

> Source/WebCore/platform/graphics/cairo/CairoPath.cpp:18
> +/*
> +    Copyright (C) 2011 Collabora Ltd.
> +
> +    This library is free software; you can redistribute it and/or
> +    modify it under the terms of the GNU Library General Public
> +    License as published by the Free Software Foundation; either
> +    version 2 of the License, or (at your option) any later version.
> +
> +    This library is distributed in the hope that it will be useful,
> +    but WITHOUT ANY WARRANTY; without even the implied warranty of
> +    Library General Public License for more details.
> +
> +    You should have received a copy of the GNU Library General Public
> +    aint with this library; see the file COPYING.LIB.  If not, write to
> +    the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> +    Boston, MA 02110-1301, USA.
> +*/

I think it's good that you are moving this stuff into an actual implementation
file since it's becoming heftier. Please, please take this opportunity to
rename this to PlatformPathCairo though. Having CairoPath.{cpp,h} and
PathCairo.cpp is incredibly confusing!

Please format this license blocks with asterisks down the left side to match
the other files in this directory.

> Source/WebCore/platform/graphics/cairo/CairoPath.cpp:25
> +cairo_surface_t* CairoPath::m_pathSurface =
cairo_image_surface_create(CAIRO_FORMAT_A8, 1, 1);

Now that this is in a CPP file you can dispense with the static class member
altogether and just do:

static cairo_surface_t* getPathSurface()
    static pathSurface =  cairo_image_surface_create(CAIRO_FORMAT_A8, 1, 1);
    return pathSurface;

> Source/WebCore/platform/graphics/cairo/CairoPath.h:35
>      CairoPath()
>      {
> -	   static cairo_surface_t* pathSurface =
cairo_image_surface_create(CAIRO_FORMAT_A8, 1, 1);
> -	   m_cr = cairo_create(pathSurface);
> +	   m_cr = adoptRef(cairo_create(m_pathSurface));
>      }

Please move the implementation of the constructor into the CPP file so you can
remove the cairo.h include above. The assignment of m_cr should be an

    : m_cr(adoptRef(cairo_create(m_pathSurface))

> Source/WebCore/platform/graphics/cairo/CairoPath.h:40
>      ~CairoPath()
>      {
> -	   cairo_destroy(m_cr);
>      }

This can be one line now and perhaps even omitted.

> Source/WebCore/platform/graphics/cairo/CairoPath.h:45
> +    static cairo_surface_t* m_pathSurface;

You can remove this. See above.

More information about the webkit-reviews mailing list