[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]

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
https://bugs.webkit.org/show_bug.cgi?id=57717

Attachment 89407: Proposed Patch7
https://bugs.webkit.org/attachment.cgi?id=89407&action=review

------- 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
bugs.

> 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
> +    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +    Library General Public License for more details.
> +
> +    You should have received a copy of the GNU Library General Public
License
> +    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
initializer.

CairoPath()
    : 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.