Bug 208807 - [Cairo] Remove PlatformPathCairo
Summary: [Cairo] Remove PlatformPathCairo
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
Keywords: Gtk, InRadar
Depends on:
Blocks: 208808
  Show dependency treegraph
Reported: 2020-03-09 07:13 PDT by Carlos Garcia Campos
Modified: 2020-03-10 06:51 PDT (History)
5 users (show)

See Also:

Patch (22.60 KB, patch)
2020-03-09 07:16 PDT, Carlos Garcia Campos
darin: review+
Details | Formatted Diff | Diff
Patch for landing (24.20 KB, patch)
2020-03-10 06:22 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2020-03-09 07:13:25 PDT
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.
Comment 1 Carlos Garcia Campos 2020-03-09 07:16:33 PDT
Created attachment 393028 [details]
Comment 2 Darin Adler 2020-03-09 13:02:29 PDT
Comment on attachment 393028 [details]

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();


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


> 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.
Comment 3 Carlos Garcia Campos 2020-03-10 06:22:41 PDT
Created attachment 393142 [details]
Patch for landing

Added Darin's suggestions.
Comment 4 Carlos Garcia Campos 2020-03-10 06:50:33 PDT
Committed r258204: <https://trac.webkit.org/changeset/258204>
Comment 5 Radar WebKit Bug Importer 2020-03-10 06:51:17 PDT