[webkit-reviews] review granted: [Bug 208807] [Cairo] Remove PlatformPathCairo : [Attachment 393028] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 208807: [Cairo] Remove PlatformPathCairo
https://bugs.webkit.org/show_bug.cgi?id=208807

Attachment 393028: Patch

https://bugs.webkit.org/attachment.cgi?id=393028&action=review




--- Comment #2 from Darin Adler <darin at apple.com> ---
Comment on attachment 393028
  --> https://bugs.webkit.org/attachment.cgi?id=393028
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=393028&action=review

> Source/WebCore/platform/graphics/Path.h:61
> +// cairo_path_fixed_t isn't exposed in Cairo's public API, so we need to use
context.
> +typedef cairo_t PlatformPath;

I don’t see why we need this type at all, nor PlatformPathPtr, nor
PlatformPathStorageType. It’s OK for other platforms to use it for now but can
we fix Cairo to not? Just means putting m_path inside #if USE(CAIRO) I think.

> Source/WebCore/platform/graphics/Path.h:193
> +    PlatformPathPtr platformPath() const { return m_path.get(); }

I have two suggestions. First, I suggest a different name for this function:

    cairo_t* cairoPath() const { return m_path.get(); }

Second, I suggest not using this function inside the class. If it’s just a
getter I suggest we just use m_path directly.

There’s certainly no need for it to be named platformPath and no need for it to
return a PlatformPathPtr.

We are going to be renaming the CG platformPath function very soon. Also, I
think these functions should be considered separately and not part of a big
#if/#elif/#else sequence.

> Source/WebCore/platform/graphics/cairo/PathCairo.cpp:56
>  Path::Path(Path&& other)

This can just be "= default" now.

> Source/WebCore/platform/graphics/cairo/PathCairo.cpp:62
>  PlatformPathPtr Path::ensurePlatformPath()

I think this should be named ensureCairoPath() or ensurePath():

    cairo_t* Path::ensureCairoPath()

This will require a bit more #if in the header, but I think it’s the correct
direction.

> Source/WebCore/platform/graphics/cairo/PathCairo.cpp:66
> +	   RefPtr<cairo_surface_t> surface =
adoptRef(cairo_image_surface_create(CAIRO_FORMAT_A8, 1, 1));
> +	   m_path = adoptRef(cairo_create(surface.get()));

The one-line version of would be shorter than the first of these two lines. I
suggest doing it:

    m_path =
adoptRef(cairo_create(adoptRef(cairo_image_surface_create(CAIRO_FORMAT_A8, 1,
1)).get()));

> Source/WebCore/platform/graphics/cairo/PathCairo.cpp:83
> +    auto pathCopy = cairo_copy_path(other.platformPath());

Should use m_path here, I think.

> Source/WebCore/platform/graphics/cairo/PathCairo.cpp:90
>  Path& Path::operator=(Path&& other)

This can just be "= default" now.

> Source/WebCore/platform/graphics/cairo/PathCairo.cpp:197
> +    cairo_t* cr = platformPath();

m_path

> Source/WebCore/platform/graphics/cairo/PathCairo.cpp:307
> +    cairo_t* cr = path.platformPath();

m_path

> Source/WebCore/platform/graphics/cairo/PathCairo.cpp:364
> +    ASSERT(m_path);

Seems overkill since the code just above checks isNull().

> Source/WebCore/platform/graphics/cairo/PathCairo.cpp:375
> +    ASSERT(m_path);

In the CG version we chose to omit these from all the SlowCase functions since
it was caller responsibility to handle null in all those cases. No harm in
asserting a little more I suppose, but I suggest not doing it.