[Webkit-unassigned] [Bug 208807] [Cairo] Remove PlatformPathCairo

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Mar 9 13:02:29 PDT 2020


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
                 CC|                            |darin at apple.com
 Attachment #393028|review?                     |review+
              Flags|                            |

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

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.

You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20200309/51996d8b/attachment-0001.htm>

More information about the webkit-unassigned mailing list