Bug 208807 – [Cairo] Remove PlatformPathCairo

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Carlos Garcia Campos]

We have a class CairoPath defined in PlatformPathCairo.h that simply wraps a cairo_t. We can use the cairo_t directly as PlatformPath and simplify the cairo path implementation.

[Carlos Garcia Campos]

Created attachment 393028 [details]
Patch

[Darin Adler]

Comment on attachment 393028 [details]
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.

[Carlos Garcia Campos]

Created attachment 393142 [details]
Patch for landing

Added Darin's suggestions.

[Carlos Garcia Campos]

Committed r258204: <https://trac.webkit.org/changeset/258204>

[Radar WebKit Bug Importer]

<rdar://problem/60271083>