Bug 208464 – Lazily generate CGPaths for some simple types of paths, such as arcs and lines

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Wenson Hsieh]

The final entry in the "Make Paths Fast" trilogy.

When creating certain types of paths such as arcs and lines, we can inline information about the path (e.g. the endpoints of a line segment, or the location and start/end angles of an arc) within the WebCore::Path itself.

This achieves two things:

1. IPC encoding and decoding is faster, since we don’t have to ask CGPath for its individual elements and encode them all.
2. We’ll avoid creating CGPaths for WebCore::Paths created in the GPU Process until they’re actually needed for painting. This means that we won’t end up with 512 individual CGPaths sitting around after we finish decoding the display list’s drawing items.

Combined, this gives us a roughly 36% improvement on Canvas Arcs, 13% improvement on Canvas Paths and 22% improvement on Canvas Lines when the GPU Process is used for canvas rendering.

[Radar WebKit Bug Importer]

<rdar://problem/59963226>

[Wenson Hsieh]

Created attachment 392634 [details]
Work in progress

[Wenson Hsieh]

Created attachment 392651 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 392651 [details]
Patch

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

This feels like it's 40% of the way towards just storing paths in our own representation. Maybe we should just do the whole thing.

> Source/WebCore/platform/graphics/Path.h:296
> +    encoder << static_cast<uint8_t>(m_inlineDataType);

Need to use the enum encoder.

> Source/WebCore/platform/graphics/Path.h:357
> +    path.m_inlineDataType = static_cast<InlineDataType>(inlineDataTypeValue);

No, you have to use the enum decoder.

> Source/WebCore/platform/graphics/cg/PathCG.cpp:68
> +static FloatRect rectContainingPoints(FloatPoint point1, FloatPoint point2)

I would expect we have this elsewhere. if not, please put in GeometryUtils.h

> Source/WebCore/platform/graphics/cg/PathCG.cpp:351
> +    if (m_inlineDataType <= InlineDataType::Move) {

There's an ordering dependency between the values of InlineDataType which is not apparent from its declaration. You should add a comment that makes that dependency super clear, or not rely on comparisons here.

[Sam Weinig]

Comment on attachment 392651 [details]
Patch

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

> Source/WebCore/platform/graphics/Path.h:155
> +#if USE(CG)
> +    WEBCORE_EXPORT bool isNull() const;
> +#else
>      bool isNull() const { return !m_path; }
> +#endif

I don't think it's worth keeping this inline for the non-USE(CG) case. Seems cleaner to make them both out of line.

[Said Abou-Hallawa]

Comment on attachment 392651 [details]
Patch

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

> Source/WebCore/ChangeLog:63
> +        The new member m_inlineDataType is an enum flag that indicates what kind of information is stored in
> +        m_inlineData. When a Path is initialized (or after it is cleared), it starts off in Cleared state. Moving the
> +        path transitions it into Move and just sets the `MoveData` member of the inline data; calling `addLineTo` or
> +        `addArc` transitions it to either Line or Arc.
> +
> +        If, at any point, the path changes in a different way (i.e. neither line, arc, nor move), we transition to
> +        the NotInline state and no longer attempt to store the path mutations as inline data on Path.

This state machine is hard to follow especially the difference between Cleared and NotInline is very subtle. I have the following suggestion. If Path class has the following data

PlatformPathStorageType m_path;
Optional<InlinePathData> m_inlineData;

Then we can have four states for these two members:

m_path          m_inlineData            Meaning

null            null                    Initial state
null            not null                Path is inline but is not applied to m_path
not null        null                    Path can't be inline
not null        not null                Path is inline and it is applied to m_path

> Source/WebCore/platform/graphics/Path.h:259
> +    struct LineData {
> +        FloatPoint start;
> +        FloatPoint end;
> +    };
> +
> +    struct ArcData {
> +        FloatPoint offset;
> +        FloatPoint center;
> +        float radius { 0 };
> +        float start { 0 };
> +        float end { 0 };
> +        bool clockwise { false };
> +        bool hasOffset { false };
> +    };
> +
> +    struct MoveData {
> +        FloatPoint location;
> +    };

I think these structures should also include methods for encoding, decoding and applying the data to m_path

> Source/WebCore/platform/graphics/Path.h:285
> +    InlineDataType m_inlineDataType { InlineDataType::Cleared };

I think this should be part of InlinePathData

struct InlinePathData {
    InlineDataType inlineDataType;
    MoveData move;
    LineData line;
    ArcData arc;
};

[Said Abou-Hallawa]

Comment on attachment 392651 [details]
Patch

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

>> Source/WebCore/platform/graphics/Path.h:285
>> +    InlineDataType m_inlineDataType { InlineDataType::Cleared };
> 
> I think this should be part of InlinePathData
> 
> struct InlinePathData {
>     InlineDataType inlineDataType;
>     MoveData move;
>     LineData line;
>     ArcData arc;
> };

I mean

struct InlinePathData {
    InlineDataType inlineDataType;
    union {
        MoveData move;
        LineData line;
        ArcData arc;
    };
};

[Wenson Hsieh]

Comment on attachment 392651 [details]
Patch

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

>> Source/WebCore/ChangeLog:63
>> +        the NotInline state and no longer attempt to store the path mutations as inline data on Path.
> 
> This state machine is hard to follow especially the difference between Cleared and NotInline is very subtle. I have the following suggestion. If Path class has the following data
> 
> PlatformPathStorageType m_path;
> Optional<InlinePathData> m_inlineData;
> 
> Then we can have four states for these two members:
> 
> m_path          m_inlineData            Meaning
> 
> null            null                    Initial state
> null            not null                Path is inline but is not applied to m_path
> not null        null                    Path can't be inline
> not null        not null                Path is inline and it is applied to m_path

Okay, I’ll give this a try.

>> Source/WebCore/platform/graphics/Path.h:155
>> +#endif
> 
> I don't think it's worth keeping this inline for the non-USE(CG) case. Seems cleaner to make them both out of line.

👍🏻

>> Source/WebCore/platform/graphics/Path.h:259
>> +    };
> 
> I think these structures should also include methods for encoding, decoding and applying the data to m_path

👍🏻

>>> Source/WebCore/platform/graphics/Path.h:285
>>> +    InlineDataType m_inlineDataType { InlineDataType::Cleared };
>> 
>> I think this should be part of InlinePathData
>> 
>> struct InlinePathData {
>>     InlineDataType inlineDataType;
>>     MoveData move;
>>     LineData line;
>>     ArcData arc;
>> };
> 
> I mean
> 
> struct InlinePathData {
>     InlineDataType inlineDataType;
>     union {
>         MoveData move;
>         LineData line;
>         ArcData arc;
>     };
> };

This sounds good! I’ll change it to this:

struct InlinePathData {
    InlineDataType type;
    union {
        MoveData move;
        LineData line;
        ArcData arc;
    };
};

(so that I can just use `m_inlineData.type` instead of `m_inlineData.inlineDataType`).

>> Source/WebCore/platform/graphics/Path.h:296
>> +    encoder << static_cast<uint8_t>(m_inlineDataType);
> 
> Need to use the enum encoder.

👍🏻

>> Source/WebCore/platform/graphics/Path.h:357
>> +    path.m_inlineDataType = static_cast<InlineDataType>(inlineDataTypeValue);
> 
> No, you have to use the enum decoder.

👍🏻

>> Source/WebCore/platform/graphics/cg/PathCG.cpp:68
>> +static FloatRect rectContainingPoints(FloatPoint point1, FloatPoint point2)
> 
> I would expect we have this elsewhere. if not, please put in GeometryUtils.h

I couldn’t find it elsewhere, so I made a new helper on GeometryUtils

>> Source/WebCore/platform/graphics/cg/PathCG.cpp:351
>> +    if (m_inlineDataType <= InlineDataType::Move) {
> 
> There's an ordering dependency between the values of InlineDataType which is not apparent from its declaration. You should add a comment that makes that dependency super clear, or not rely on comparisons here.

Good call; I’ll change this to compare directly.

[Daniel Bates]

Comment on attachment 392651 [details]
Patch

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

>>>> Source/WebCore/platform/graphics/Path.h:285
>>>> +    InlineDataType m_inlineDataType { InlineDataType::Cleared };
>>> 
>>> I think this should be part of InlinePathData
>>> 
>>> struct InlinePathData {
>>>     InlineDataType inlineDataType;
>>>     MoveData move;
>>>     LineData line;
>>>     ArcData arc;
>>> };
>> 
>> I mean
>> 
>> struct InlinePathData {
>>     InlineDataType inlineDataType;
>>     union {
>>         MoveData move;
>>         LineData line;
>>         ArcData arc;
>>     };
>> };
> 
> This sounds good! I’ll change it to this:
> 
> struct InlinePathData {
>     InlineDataType type;
>     union {
>         MoveData move;
>         LineData line;
>         ArcData arc;
>     };
> };
> 
> (so that I can just use `m_inlineData.type` instead of `m_inlineData.inlineDataType`).

This is ok as-is. No change is needed. A better solution  is to use std::variant (or WTF::variant if that exists) for the union because:

1. It is the modern way to represent a tagged union.
2. Because of (1) no need for InlineDataType, just need a bool for NoInline or Cleared state <-- I don't know what either of these are to say which could be represent by the empty variant, but one of them can

> Source/WebCore/platform/graphics/Path.h:298
> +    if (m_inlineDataType == InlineDataType::Move) {

This is ok as-is. No change is needed. The optimal solution is to implement support for encoding and decoding a variant that because:

1. Simplifies the code: variant encoder/decoder knows how to encode/decode each alternative.
2. Because of (1) can remove all the branches here.
3. Because of (1) it is type safe: compile failure if alternative has not defined encoder or decoder if in the future there is a new data type

[Daniel Bates]

Comment on attachment 392651 [details]
Patch

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

>>> Source/WebCore/platform/graphics/cg/PathCG.cpp:68
>>> +static FloatRect rectContainingPoints(FloatPoint point1, FloatPoint point2)
>> 
>> I would expect we have this elsewhere. if not, please put in GeometryUtils.h
> 
> I couldn’t find it elsewhere, so I made a new helper on GeometryUtils

This is ok as-is. No change is needed. This function duplicates what (FloatRect { }).fitToPoints(point1, point2) does.

[Wenson Hsieh]

Comment on attachment 392651 [details]
Patch

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

>>> Source/WebCore/ChangeLog:63
>>> +        the NotInline state and no longer attempt to store the path mutations as inline data on Path.
>> 
>> This state machine is hard to follow especially the difference between Cleared and NotInline is very subtle. I have the following suggestion. If Path class has the following data
>> 
>> PlatformPathStorageType m_path;
>> Optional<InlinePathData> m_inlineData;
>> 
>> Then we can have four states for these two members:
>> 
>> m_path          m_inlineData            Meaning
>> 
>> null            null                    Initial state
>> null            not null                Path is inline but is not applied to m_path
>> not null        null                    Path can't be inline
>> not null        not null                Path is inline and it is applied to m_path
> 
> Okay, I’ll give this a try.

Note that this would increase path from 48 to 56 bytes since the two bools and the 8-bit enum flag no longer bit inside the padding after the union. (I think Variant will have the same effect as well).

It’s slightly unfortunate, but probably not very impactful.

>>>>> Source/WebCore/platform/graphics/Path.h:285
>>>>> +    InlineDataType m_inlineDataType { InlineDataType::Cleared };
>>>> 
>>>> I think this should be part of InlinePathData
>>>> 
>>>> struct InlinePathData {
>>>>     InlineDataType inlineDataType;
>>>>     MoveData move;
>>>>     LineData line;
>>>>     ArcData arc;
>>>> };
>>> 
>>> I mean
>>> 
>>> struct InlinePathData {
>>>     InlineDataType inlineDataType;
>>>     union {
>>>         MoveData move;
>>>         LineData line;
>>>         ArcData arc;
>>>     };
>>> };
>> 
>> This sounds good! I’ll change it to this:
>> 
>> struct InlinePathData {
>>     InlineDataType type;
>>     union {
>>         MoveData move;
>>         LineData line;
>>         ArcData arc;
>>     };
>> };
>> 
>> (so that I can just use `m_inlineData.type` instead of `m_inlineData.inlineDataType`).
> 
> This is ok as-is. No change is needed. A better solution  is to use std::variant (or WTF::variant if that exists) for the union because:
> 
> 1. It is the modern way to represent a tagged union.
> 2. Because of (1) no need for InlineDataType, just need a bool for NoInline or Cleared state <-- I don't know what either of these are to say which could be represent by the empty variant, but one of them can

Yeah, I’ll explore using WTF::Variant for this.

>> Source/WebCore/platform/graphics/Path.h:298
>> +    if (m_inlineDataType == InlineDataType::Move) {
> 
> This is ok as-is. No change is needed. The optimal solution is to implement support for encoding and decoding a variant that because:
> 
> 1. Simplifies the code: variant encoder/decoder knows how to encode/decode each alternative.
> 2. Because of (1) can remove all the branches here.
> 3. Because of (1) it is type safe: compile failure if alternative has not defined encoder or decoder if in the future there is a new data type

Yes — will try to adopt WTF::Variant.

>>>> Source/WebCore/platform/graphics/cg/PathCG.cpp:68
>>>> +static FloatRect rectContainingPoints(FloatPoint point1, FloatPoint point2)
>>> 
>>> I would expect we have this elsewhere. if not, please put in GeometryUtils.h
>> 
>> I couldn’t find it elsewhere, so I made a new helper on GeometryUtils
> 
> This is ok as-is. No change is needed. This function duplicates what (FloatRect { }).fitToPoints(point1, point2) does.

It seems a bit odd to have to create an empty FloatRect just to fit it to two points, but this seems a bit cleaner than doing the math myself. I’ll use this instead of adding the helper.

[Wenson Hsieh]

Created attachment 392801 [details]
v2

[Tim Horton]

Comment on attachment 392801 [details]
v2

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

> Source/WebCore/platform/graphics/Path.h:233
> +    struct LineData {

Feels weird that this is in USE(CG). Should it be in a ENABLE(PLATFORM_INDEPENDENT_PATHS) (probably not that name) that is only on in CG for now? I assume the reason that you didn't is that it still needs the platform path to fall back on if it's not one of the simple types? But even that could be true for the other graphics frameworks.

[Tim Horton]

Comment on attachment 392801 [details]
v2

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

> Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:2528
> -    const Path m_path;
> +    mutable Path m_path;

This seems slightly unfortunate, I don't think we really expect display list items to mutate once created. Maybe we can hide the externally-invisible-mutability inside Path?

[Darin Adler]

Comment on attachment 392801 [details]
v2

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

I would like to see a refinement that puts more/most of this into the platform-independent source file, even if only used by CG at this time.

> Source/WebCore/platform/graphics/Path.h:228
> +    enum class NoInlineDataType : uint8_t { NoInlineData = 0 };

In many cases in other classes we std::nullptr_t for this. I don’t think "NoInlineData" is better than "nullptr".

>> Source/WebCore/platform/graphics/Path.h:233
>> +    struct LineData {
> 
> Feels weird that this is in USE(CG). Should it be in a ENABLE(PLATFORM_INDEPENDENT_PATHS) (probably not that name) that is only on in CG for now? I assume the reason that you didn't is that it still needs the platform path to fall back on if it's not one of the simple types? But even that could be true for the other graphics frameworks.

Related: I think it’s OK to define but not use cross-platform things like this that could be used later. This entire block could probably be unconditional even if at the moment it’s only used for CG. I feel that way even about the swap function. I’d like to see less of this inside #if USE(CG).

> Source/WebCore/platform/graphics/Path.h:268
> +    template <typename DataType>
> +    bool hasInlineDataOfType() const

My preferred formatting for this is:

    template<typename DataType> bool hasInlineDataOfType() const

Leaving out the space and line break that others often include in such cases. Would be nice to have a shorter name. Including "OfType" in the name seems inelegant.

> Source/WebCore/platform/graphics/Path.h:276
> +    bool hasInlineData() const
> +    {
> +        return !hasInlineDataOfType<NoInlineDataType>();
> +    }

Why inline in the class? How about putting this below with the other inlines?

> Source/WebCore/platform/graphics/cg/PathCG.cpp:103
> +    if (!m_path)
> +        m_path = adoptCF(CGPathCreateMutable());

This is the only platform-specfiic part. If we broke it out into a separate function the rest of this could be in Path.cpp.

> Source/WebCore/platform/graphics/displaylists/DisplayListItems.cpp:1197
> +    context.strokePath(std::exchange(m_path, { }));

Is this for correctness or performance? If for performance then use WTFMove. If correctness, then std::exchange is fine.

>> Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:2528
>> +    mutable Path m_path;
> 
> This seems slightly unfortunate, I don't think we really expect display list items to mutate once created. Maybe we can hide the externally-invisible-mutability inside Path?

I agree completely. We should do that. Just make a couple more functions const and use mutable *inside* Path.

[Wenson Hsieh]

Comment on attachment 392801 [details]
v2

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

>> Source/WebCore/platform/graphics/Path.h:228
>> +    enum class NoInlineDataType : uint8_t { NoInlineData = 0 };
> 
> In many cases in other classes we std::nullptr_t for this. I don’t think "NoInlineData" is better than "nullptr".

This is what I tried initially, but then I realized that std::nullptr_t isn’t currently supported when encoding/decoding. I’ve learned from Antti that I can just Monostate to represent the empty value in a Variant instead, so I think I’ll do that (and implement Encoder/Decoder support for it in ArgumentCoders).

>>> Source/WebCore/platform/graphics/Path.h:233
>>> +    struct LineData {
>> 
>> Feels weird that this is in USE(CG). Should it be in a ENABLE(PLATFORM_INDEPENDENT_PATHS) (probably not that name) that is only on in CG for now? I assume the reason that you didn't is that it still needs the platform path to fall back on if it's not one of the simple types? But even that could be true for the other graphics frameworks.
> 
> Related: I think it’s OK to define but not use cross-platform things like this that could be used later. This entire block could probably be unconditional even if at the moment it’s only used for CG. I feel that way even about the swap function. I’d like to see less of this inside #if USE(CG).

Sounds good. I’ll move this into a separate (platform-independent) file, InlinePathData.h

>> Source/WebCore/platform/graphics/Path.h:268
>> +    bool hasInlineDataOfType() const
> 
> My preferred formatting for this is:
> 
>     template<typename DataType> bool hasInlineDataOfType() const
> 
> Leaving out the space and line break that others often include in such cases. Would be nice to have a shorter name. Including "OfType" in the name seems inelegant.

Ah, this particular name choice was influenced by the -OfType suffix in childrenOfType/ancestorsOfType.

I’ll contract it to just hasInlineData<Foo>(), and rename what is currently hasInlineData() to hasAnyInlineData().

>> Source/WebCore/platform/graphics/Path.h:276
>> +    }
> 
> Why inline in the class? How about putting this below with the other inlines?

👍🏻 Moved below.

>> Source/WebCore/platform/graphics/cg/PathCG.cpp:103
>> +        m_path = adoptCF(CGPathCreateMutable());
> 
> This is the only platform-specfiic part. If we broke it out into a separate function the rest of this could be in Path.cpp.

Okay, I’ll add new private helpers then:

`static PlatformPathStorageType createPlatformPath();` (which returns a newly created path)
`static void applyInlinePathData(const InlinePathData&, PlatformPathPtr);` (which applies inline data to a platform path pointer).

…which I'll use in Path.cpp, to make this code platform-agnostic.

>> Source/WebCore/platform/graphics/displaylists/DisplayListItems.cpp:1197
>> +    context.strokePath(std::exchange(m_path, { }));
> 
> Is this for correctness or performance? If for performance then use WTFMove. If correctness, then std::exchange is fine.

It’s for performance, but if we’re going to keep `const Path m_path;`, then neither of these will work.

I’ve instead added `void deletePlatformPath() const` method on Path, which I’m calling here after I’m done with the path.

>>> Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:2528
>>> +    mutable Path m_path;
>> 
>> This seems slightly unfortunate, I don't think we really expect display list items to mutate once created. Maybe we can hide the externally-invisible-mutability inside Path?
> 
> I agree completely. We should do that. Just make a couple more functions const and use mutable *inside* Path.

Okay — I’ve added `void deletePlatformPath() const` instead.

[Darin Adler]

Comment on attachment 392801 [details]
v2

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

>>> Source/WebCore/platform/graphics/Path.h:268
>>> +    bool hasInlineDataOfType() const
>> 
>> My preferred formatting for this is:
>> 
>>     template<typename DataType> bool hasInlineDataOfType() const
>> 
>> Leaving out the space and line break that others often include in such cases. Would be nice to have a shorter name. Including "OfType" in the name seems inelegant.
> 
> Ah, this particular name choice was influenced by the -OfType suffix in childrenOfType/ancestorsOfType.
> 
> I’ll contract it to just hasInlineData<Foo>(), and rename what is currently hasInlineData() to hasAnyInlineData().

Antti, originator of ancestorsOfType, recently suggested that I remove the "OfType" from that function’s name. Part of why we haven’t done it yet is that we are not sure that childrenOfType should be renamed to just children.

>>> Source/WebCore/platform/graphics/displaylists/DisplayListItems.cpp:1197
>>> +    context.strokePath(std::exchange(m_path, { }));
>> 
>> Is this for correctness or performance? If for performance then use WTFMove. If correctness, then std::exchange is fine.
> 
> It’s for performance, but if we’re going to keep `const Path m_path;`, then neither of these will work.
> 
> I’ve instead added `void deletePlatformPath() const` method on Path, which I’m calling here after I’m done with the path.

That sounds wrong. A secret "destroy this while pretending it’s const"? Consider if this was a unique_ptr; is that what we’d want to do?

>>>> Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:2528
>>>> +    mutable Path m_path;
>>> 
>>> This seems slightly unfortunate, I don't think we really expect display list items to mutate once created. Maybe we can hide the externally-invisible-mutability inside Path?
>> 
>> I agree completely. We should do that. Just make a couple more functions const and use mutable *inside* Path.
> 
> Okay — I’ve added `void deletePlatformPath() const` instead.

Now I regret making this suggestion!

[Wenson Hsieh]

Comment on attachment 392801 [details]
v2

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

>>>> Source/WebCore/platform/graphics/displaylists/DisplayListItems.cpp:1197
>>>> +    context.strokePath(std::exchange(m_path, { }));
>>> 
>>> Is this for correctness or performance? If for performance then use WTFMove. If correctness, then std::exchange is fine.
>> 
>> It’s for performance, but if we’re going to keep `const Path m_path;`, then neither of these will work.
>> 
>> I’ve instead added `void deletePlatformPath() const` method on Path, which I’m calling here after I’m done with the path.
> 
> That sounds wrong. A secret "destroy this while pretending it’s const"? Consider if this was a unique_ptr; is that what we’d want to do?

Probably not!

>>>>> Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:2528
>>>>> +    mutable Path m_path;
>>>> 
>>>> This seems slightly unfortunate, I don't think we really expect display list items to mutate once created. Maybe we can hide the externally-invisible-mutability inside Path?
>>> 
>>> I agree completely. We should do that. Just make a couple more functions const and use mutable *inside* Path.
>> 
>> Okay — I’ve added `void deletePlatformPath() const` instead.
> 
> Now I regret making this suggestion!

Yeah, I’m not sure how to best go about this :/ I agree that making m_path mutable in the display list item is weird; I also agree that adding the const “delete my platform path” method is strange.

Maybe we could just make this `Path m_path`, and then change DisplayList::Item::apply() to be non-const (in other words, view individual display list items as disposable for the purposes of display list playback).

In any case, would you be okay with just making this `mutable Path m_path` for now and WTFMove-ing in StrokePath::apply?

[Darin Adler]

Comment on attachment 392801 [details]
v2

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

>>>>>> Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:2528
>>>>>> +    mutable Path m_path;
>>>>> 
>>>>> This seems slightly unfortunate, I don't think we really expect display list items to mutate once created. Maybe we can hide the externally-invisible-mutability inside Path?
>>>> 
>>>> I agree completely. We should do that. Just make a couple more functions const and use mutable *inside* Path.
>>> 
>>> Okay — I’ve added `void deletePlatformPath() const` instead.
>> 
>> Now I regret making this suggestion!
> 
> Yeah, I’m not sure how to best go about this :/ I agree that making m_path mutable in the display list item is weird; I also agree that adding the const “delete my platform path” method is strange.
> 
> Maybe we could just make this `Path m_path`, and then change DisplayList::Item::apply() to be non-const (in other words, view individual display list items as disposable for the purposes of display list playback).
> 
> In any case, would you be okay with just making this `mutable Path m_path` for now and WTFMove-ing in StrokePath::apply?

Yes.

[Wenson Hsieh]

Created attachment 392922 [details]
v3

[Wenson Hsieh]

Created attachment 392923 [details]
v3 (rebased on trunk)

[Wenson Hsieh]

Created attachment 392924 [details]
v3 (fix GTK/WPE builds)

[Wenson Hsieh]

Created attachment 392930 [details]
v3

[Darin Adler]

Comment on attachment 392930 [details]
v3

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

> Source/WebCore/platform/graphics/Path.cpp:209
> +#if ENABLE(INLINE_PATH_DATA)
> +
> +void Path::applyInlinePathDataIfNeeded() const
> +{
> +    if (!m_needsToApplyInlineData)
> +        return;
> +
> +    m_needsToApplyInlineData = false;
> +
> +    if (!hasAnyInlineData())
> +        return;
> +
> +    if (!m_path)
> +        m_path = createPlatformPath();
> +
> +    applyInlinePathData(m_inlineData, m_path.get());
> +}
> +
> +#endif // ENABLE(INLINE_PATH_DATA)

I was wrong to say that this function is platform-independent. It’s not. It should be deleted.

> Source/WebCore/platform/graphics/Path.h:252
> +    mutable bool m_needsToApplyInlineData { false };

This boolean is redundant. When we have inline data and m_path is null, this is always true. When we have inline data and m_path is non-null, this is always false. This function will always return the same thing as this boolean:

    bool needsToApplyInlineData() const { return !m_path && hasAnyInlineData(); }

So please get rid of this boolean and all the code that manages it.

> Source/WebCore/platform/graphics/cg/PathCG.cpp:90
> +RetainPtr<CGMutablePathRef> Path::createPlatformPath()
> +{
> +    return adoptCF(CGPathCreateMutable());
> +}
> +
> +void Path::applyInlinePathData(const InlinePathData& data, CGMutablePathRef path)
> +{

These functions should be merged into a single createPlatformPath function, a const function that does not take any arguments or return any values. It should start like this:

    if (m_path)
        return;
    m_path = adoptCF(CGPathCreateMutable());
    WTF::switchOn(m_inlinePathData,
        [&](Monostate) { }, // Start with an empty path.

> Source/WebCore/platform/graphics/cg/PathCG.cpp:92
> +        [&](auto) { ASSERT_NOT_REACHED(); },

This seems really peculiar. We want to get a compiler error if there’s any type of data we fail to handle. But including this "auto" case means we won’t get that. This should just cover Monostate, I think, not auto.

> Source/WebCore/platform/graphics/cg/PathCG.cpp:263
> +    CGPathAddPath(path.get(), &transformCG, ensurePlatformPath());

This should be platformPath(), not ensurePlatformPath(). And we should explicitly set m_inlineData = Monostate { } after setting m_path below.

> Source/WebCore/platform/graphics/cg/PathCG.cpp:265
> +    auto path = adoptCF(CGPathCreateMutableCopyByTransformingPath(ensurePlatformPath(), &transformCG));

Ditto.

> Source/WebCore/platform/graphics/cg/PathCG.cpp:274
>          return CGRectZero;

This should return { } rather than CGRectZero. I see no reason to make this different than the line below.

> Source/WebCore/platform/graphics/cg/PathCG.cpp:295
>          return CGRectZero;

This should return { } rather than CGRectZero. I see no reason to make this different than the line below.

> Source/WebCore/platform/graphics/cg/PathCG.cpp:308
> +    CGRect bound = CGPathGetBoundingBox(platformPath());
>      return CGRectIsNull(bound) ? CGRectZero : bound;

Why do we have two different boundingRect functions? It seems that the boundingRect and fastBoundingRect function bodies are exactly the same.

> Source/WebCore/platform/graphics/cg/PathCG.cpp:340
> +    if (isNull() || hasInlineData<MoveData>()) {
> +        m_inlineData = MoveData { point };
> +        m_needsToApplyInlineData = true;
> +        return;
> +    }

This is the platform-independent code that I would like to share if possible. If we did that it means that the general case of moveTo needs to be renamed to something like moveToSlowCase or whatever we want to call the non-inline-data case.

> Source/WebCore/platform/graphics/cg/PathCG.cpp:351
> +    if (isNull() || hasInlineData<MoveData>()) {
> +        m_inlineData = LineData { hasAnyInlineData() ? WTF::get<MoveData>(m_inlineData).location : FloatPoint(), p };
> +        m_needsToApplyInlineData = true;
> +        return;
> +    }

Ditto.

> Source/WebCore/platform/graphics/cg/PathCG.cpp:439
> +    if (isNull() || hasInlineData<MoveData>()) {
> +        m_inlineData = ArcData { hasAnyInlineData() ? WTF::get<MoveData>(m_inlineData).location : FloatPoint(), p, radius, startAngle, endAngle, clockwise, hasAnyInlineData() };
> +        m_needsToApplyInlineData = true;
> +        return;
> +    }

Ditto. Probably have to share the workaround too.

> Source/WebCore/platform/graphics/cg/PathCG.cpp:499
> +    if (isNull())
> +        return true;
> +
> +    if (hasAnyInlineData())
> +        return false;

Would be nice to share this part.

> Source/WebCore/platform/graphics/cg/PathCG.cpp:518
>      if (isNull())
> -        return FloatPoint();
> -    return CGPathGetCurrentPoint(m_path.get());
> +        return { };
> +
> +    if (hasInlineData<MoveData>())
> +        return WTF::get<MoveData>(m_inlineData).location;
> +
> +    if (hasInlineData<LineData>())
> +        return WTF::get<LineData>(m_inlineData).end;

Would be nice to share this part.

> Source/WebCore/platform/graphics/cg/PathCG.cpp:572
>      if (isNull())
>          return;
>  
> -    CGPathApply(m_path.get(), (void*)&function, CGPathApplierToPathApplier);
> +    if (hasInlineData<MoveData>()) {
> +        PathElement element;
> +        element.type = PathElement::Type::MoveToPoint;
> +        element.points[0] = WTF::get<MoveData>(m_inlineData).location;
> +        function(element);
> +        return;
> +    }
> +
> +    if (hasInlineData<LineData>()) {
> +        auto& line = WTF::get<LineData>(m_inlineData);
> +        PathElement element;
> +        element.type = PathElement::Type::MoveToPoint;
> +        element.points[0] = line.start;
> +        function(element);
> +        element.type = PathElement::Type::AddLineToPoint;
> +        element.points[0] = line.end;
> +        function(element);
> +        return;
> +    }

Would be nice to share this part.

> Source/WebCore/platform/graphics/cg/PathCG.cpp:585
> +    if (hasInlineData<MoveData>())
> +        return 1;
> +
> +    if (hasInlineData<LineData>())
> +        return 2;

And this part.

> Source/WebKit/Platform/IPC/ArgumentCoders.cpp:212
> +Optional<Monostate> ArgumentCoder<Monostate>::decode(Decoder& decoder)

Should omit the argument name "decoder" since the value is unused.

[Wenson Hsieh]

Comment on attachment 392930 [details]
v3

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

Thanks for the review!

I’m partway through addressing your latest comments, and should have a v4 posted soon (I just need to pull some of this inlining logic into platform-agnostic methods in Path now, and add platform-y doFooSlowCase() versions of a few methods).

Sorry for the back-and-forth.

>> Source/WebCore/platform/graphics/Path.cpp:209
>> +#endif // ENABLE(INLINE_PATH_DATA)
> 
> I was wrong to say that this function is platform-independent. It’s not. It should be deleted.

Ok, deleted this.

>> Source/WebCore/platform/graphics/Path.h:252
>> +    mutable bool m_needsToApplyInlineData { false };
> 
> This boolean is redundant. When we have inline data and m_path is null, this is always true. When we have inline data and m_path is non-null, this is always false. This function will always return the same thing as this boolean:
> 
>     bool needsToApplyInlineData() const { return !m_path && hasAnyInlineData(); }
> 
> So please get rid of this boolean and all the code that manages it.

Good catch! Done.

>> Source/WebCore/platform/graphics/cg/PathCG.cpp:90
>> +{
> 
> These functions should be merged into a single createPlatformPath function, a const function that does not take any arguments or return any values. It should start like this:
> 
>     if (m_path)
>         return;
>     m_path = adoptCF(CGPathCreateMutable());
>     WTF::switchOn(m_inlinePathData,
>         [&](Monostate) { }, // Start with an empty path.

Okay. For clarity, where did you envision createPlatformPath() being used? This is currently being used in applyInlinePathDataIfNeeded(), but that’s going away based on your above comment.

In my current WIP v4, I will be calling it in PathCG.cpp, in Path::platformPath() and Path::ensurePlatformPath().

>> Source/WebCore/platform/graphics/cg/PathCG.cpp:92
>> +        [&](auto) { ASSERT_NOT_REACHED(); },
> 
> This seems really peculiar. We want to get a compiler error if there’s any type of data we fail to handle. But including this "auto" case means we won’t get that. This should just cover Monostate, I think, not auto.

Okay, changed to `Monostate` instead of just auto.

>> Source/WebCore/platform/graphics/cg/PathCG.cpp:263
>> +    CGPathAddPath(path.get(), &transformCG, ensurePlatformPath());
> 
> This should be platformPath(), not ensurePlatformPath(). And we should explicitly set m_inlineData = Monostate { } after setting m_path below.

Done.

>> Source/WebCore/platform/graphics/cg/PathCG.cpp:265
>> +    auto path = adoptCF(CGPathCreateMutableCopyByTransformingPath(ensurePlatformPath(), &transformCG));
> 
> Ditto.

Done.

>> Source/WebCore/platform/graphics/cg/PathCG.cpp:274
>>          return CGRectZero;
> 
> This should return { } rather than CGRectZero. I see no reason to make this different than the line below.

Ok.

>> Source/WebCore/platform/graphics/cg/PathCG.cpp:295
>>          return CGRectZero;
> 
> This should return { } rather than CGRectZero. I see no reason to make this different than the line below.

Ok.

>> Source/WebCore/platform/graphics/cg/PathCG.cpp:308
>>      return CGRectIsNull(bound) ? CGRectZero : bound;
> 
> Why do we have two different boundingRect functions? It seems that the boundingRect and fastBoundingRect function bodies are exactly the same.

Pulled this out into a helper function.

>> Source/WebKit/Platform/IPC/ArgumentCoders.cpp:212
>> +Optional<Monostate> ArgumentCoder<Monostate>::decode(Decoder& decoder)
> 
> Should omit the argument name "decoder" since the value is unused.

Done.

[Wenson Hsieh]

Comment on attachment 392930 [details]
v3

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

I also noticed that hasCurrentPoint() was implemented on all three platforms as !isEmpty(), so I also pulled that out into platform-agnostic code in Path.cpp while I was at it.

>> Source/WebCore/platform/graphics/cg/PathCG.cpp:340
>> +    }
> 
> This is the platform-independent code that I would like to share if possible. If we did that it means that the general case of moveTo needs to be renamed to something like moveToSlowCase or whatever we want to call the non-inline-data case.

Done.

>> Source/WebCore/platform/graphics/cg/PathCG.cpp:351
>> +    }
> 
> Ditto.

👍🏻

>> Source/WebCore/platform/graphics/cg/PathCG.cpp:439
>> +    }
> 
> Ditto. Probably have to share the workaround too.

👍🏻

>> Source/WebCore/platform/graphics/cg/PathCG.cpp:499
>> +        return false;
> 
> Would be nice to share this part.

👍🏻

>> Source/WebCore/platform/graphics/cg/PathCG.cpp:518
>> +        return WTF::get<LineData>(m_inlineData).end;
> 
> Would be nice to share this part.

👍🏻

>> Source/WebCore/platform/graphics/cg/PathCG.cpp:572
>> +    }
> 
> Would be nice to share this part.

👍🏻

>> Source/WebCore/platform/graphics/cg/PathCG.cpp:585
>> +        return 2;
> 
> And this part.

👍🏻

[Wenson Hsieh]

Created attachment 392942 [details]
v4

[Wenson Hsieh]

Comment on attachment 392930 [details]
v3

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

>>> Source/WebCore/platform/graphics/cg/PathCG.cpp:308
>>>      return CGRectIsNull(bound) ? CGRectZero : bound;
>> 
>> Why do we have two different boundingRect functions? It seems that the boundingRect and fastBoundingRect function bodies are exactly the same.
> 
> Pulled this out into a helper function.

Oops, I just realized I misread your earlier comment.

It seems there are two different bounding rect functions because they compute different things; fastBoundingRect is the bounding rect of the path, expanded to include the path’s control points, whereas boundingRect is the bounding rect of the path without necessarily including the control points.

[Daniel Bates]

Comment on attachment 392942 [details]
v4

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

This patch looks good.

> Source/WebCore/platform/graphics/InlinePathData.h:48
> +    float start { 0 };

This is ok as-is. No change is needed. A better name would be startAngle and endAngle for these fields with a comment that these are in radians or degrees or whatever unit because:

1. Makes it a tiny bit more clear what these are.
2. It breaks symmetry with LineData's identically named,  but different purpose fields. <-- this is good because it matches intuition: different concepts should be given different names.

> Source/WebCore/platform/graphics/InlinePathData.h:51
> +    bool hasOffset { false };

This is ok as-is. No change is needed. The optimal solution is to use Optional<FloatPoint> for the offset because:

1. It is the modern C++ way
2. Can remove this bool (the Optional has one)
3. Because of (1) more likely a person will remember to check if offset is not nullopt over a separate bool due to API conveniences. This means less error prone to use this data.

> Source/WebCore/platform/graphics/InlinePathData.h:74
> +    return { data };

This is ok as-is. No change is needed. The braces are not needed.

> Source/WebCore/platform/graphics/InlinePathData.h:92
> +    return { WTFMove(data) };

This is ok as-is. No change is needed. The optimal solution is to just return data because:

1. Takes advantage of RVO

> Source/WebCore/platform/graphics/InlinePathData.h:130
> +    return { WTFMove(data) };

Ditto.

> Source/WebCore/platform/graphics/Path.cpp:253
> +        m_inlineData = ArcData { hasAnyInlineData() ? WTF::get<MoveData>(m_inlineData).location : FloatPoint(), point, radius, startAngle, endAngle, anticlockwise, hasAnyInlineData() };

This is ok-is. No change is needed. The optimal solution would be to initialize each struct member individually then move the struct into the the instance variable because:

1. The names of the fields are self describing so no need to ever have to consider argument order if in the future this struct is expanded.
2. Removes the need to EXPLICITLY initialize location if hasAnyInlineDate() is false.
3. There are many arguments to pass, which increases the chance of passing them in the wrong order in the future if the struct is expanded or layout changed.

> Source/WebCore/platform/graphics/Path.h:322
> +        return { WTFMove(path) };

This is ok as-is. No change needed. The optimal  solution is to just return path for the same reasons outlined earlier in this patch.

> Source/WebCore/platform/graphics/Path.h:386
> +    return { WTFMove(path) };

Ditto.

> Source/WebCore/platform/graphics/displaylists/DisplayListItems.cpp:1231
> +    context.strokePath(WTFMove(m_path));

Is it OK to move this? If so, why?

> Source/WebCore/platform/graphics/win/PathDirect2D.cpp:672
> +    HRESULT hr = GraphicsContext::systemFactory()->CreateGeometryGroup(D2D1_FILL_MODE_WINDING, nullptr, 0, &m_path);

This is ok as-is. No change needed. A better solution is to either:
1. Add inline comment to describe 0

Or

2. Define local with value 0 and pass it.

Either one because it makes the purpose of the 0 clear.

[Darin Adler]

Comment on attachment 392942 [details]
v4

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

Looks great

> Source/WebCore/platform/graphics/Path.cpp:196
> +    applySlowCase(function);

I think the paragraphing of all these functions with #if ENABLE(INLINE_PATH_DATA) sections works better if they have blank lines after #endif.

> Source/WebCore/platform/graphics/Path.h:230
> +    void createPlatformPath() const;

I don’t think this is a cross-platform function. I suggest naming this createCGPath and making it CG-specific. The one for CG is quite different from the ones for Cairo and Direct2D. I believe those two aren’t called anywhere and also they don’t check m_path for null so they are not really the same kind of function.

I know I keep harping on this, but the platformPath name is not really doing us any good.

> Source/WebCore/platform/graphics/Path.h:233
> +    static void applyInlinePathData(const InlinePathData&, PlatformPathPtr);

Need to remove this. We don’t have this function any more.

> Source/WebCore/platform/graphics/Path.h:234
> +    template <typename DataType> bool hasInlineData() const;

I prefer the format without a space after the word "template".

> Source/WebCore/platform/graphics/Path.h:237
> +    bool needsToApplyInlineData() const { return !m_path && hasAnyInlineData(); }

Not sure we need this function. It’s called in only one place.

> Source/WebCore/platform/graphics/cairo/PathCairo.cpp:450
> +void Path::createPlatformPath() const
> +{
> +    m_path = new CairoPath;
> +}

Dead code, lets not add this.

> Source/WebCore/platform/graphics/cg/PathCG.cpp:94
> +        CGPathMoveToPoint(m_path.get(), nullptr, move.location.x(), move.location.y());

This needs to be indented four more spaces.

> Source/WebCore/platform/graphics/cg/PathCG.cpp:276
>      // CGPathGetBoundingBox includes the path's control points, CGPathGetPathBoundingBox does not.

Ah, I see the comment now.

> Source/WebCore/platform/graphics/cg/PathCG.cpp:278
> +    CGRect bound = CGPathGetPathBoundingBox(platformPath());

Seems like these functions could have used the "slow case" pattern.

> Source/WebCore/platform/graphics/cg/PathCG.cpp:279
>      return CGRectIsNull(bound) ? CGRectZero : bound;

I think we should make a static inline helper function to do this to tidy up the two places we do it. Convert a CGRect to a FloatRect, converting null CGRect into zero floating rect.

> Source/WebCore/platform/graphics/win/PathDirect2D.cpp:674
> +void Path::createPlatformPath() const
> +{
> +    HRESULT hr = GraphicsContext::systemFactory()->CreateGeometryGroup(D2D1_FILL_MODE_WINDING, nullptr, 0, &m_path);
> +    ASSERT(SUCCEEDED(hr));
> +}

Dead code; lets not add this.

[Wenson Hsieh]

Comment on attachment 392942 [details]
v4

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

>> Source/WebCore/platform/graphics/InlinePathData.h:48
>> +    float start { 0 };
> 
> This is ok as-is. No change is needed. A better name would be startAngle and endAngle for these fields with a comment that these are in radians or degrees or whatever unit because:
> 
> 1. Makes it a tiny bit more clear what these are.
> 2. It breaks symmetry with LineData's identically named,  but different purpose fields. <-- this is good because it matches intuition: different concepts should be given different names.

Good point! Renamed to startAngle and endAngle.

>> Source/WebCore/platform/graphics/InlinePathData.h:51
>> +    bool hasOffset { false };
> 
> This is ok as-is. No change is needed. The optimal solution is to use Optional<FloatPoint> for the offset because:
> 
> 1. It is the modern C++ way
> 2. Can remove this bool (the Optional has one)
> 3. Because of (1) more likely a person will remember to check if offset is not nullopt over a separate bool due to API conveniences. This means less error prone to use this data.

Ah, so I tried this at some point (Optional<FloatPoint> offset;), but found that it bumped the size of Path up from 48 to 56 bytes :(

With the optional:

whsieh@Wenson-Desktop-2 OpenSource % dump-class-layout ~/Build/Release/WebCore.framework/WebCore Path
  +0 < 56> WebCore::Path
  +0 <  8>     PlatformPathStorageType m_path
  +0 <  8>       StorageType m_ptr
  +8 < 40>     WebCore::InlinePathData m_inlineData
  +8 <  1>         WTF::__variant_base<WTF::Variant<WTF::Monostate, WebCore::MoveData, WebCore::LineData, WebCore::ArcData>, true> WTF::__variant_base<WTF::Variant<WTF::Monostate, WebCore::MoveData, WebCore::LineData, WebCore::ArcData>, true>
  +8 < 36>       WTF::Variant<WTF::Monostate, WebCore::MoveData, WebCore::LineData, WebCore::ArcData>::__storage_type __storage
 +44 <  1>       WTF::__discriminator_type<4, false, false, false>::__type __index
 +45 <  3>   <PADDING: 3 bytes>
 +48 <  1>   bool m_copyPathBeforeMutation
 +49 <  7>   <PADDING: 7 bytes>
Total byte size: 56
Total pad bytes: 10
Padding percentage: 17.86 %

…and with the bool:

whsieh@Wenson-Desktop-2 OpenSource % dump-class-layout ~/Build/Release/WebCore.framework/WebCore Path
  +0 < 48> WebCore::Path
  +0 <  8>     PlatformPathStorageType m_path
  +0 <  8>       StorageType m_ptr
  +8 < 36>     WebCore::InlinePathData m_inlineData
  +8 <  1>         WTF::__variant_base<WTF::Variant<WTF::Monostate, WebCore::MoveData, WebCore::LineData, WebCore::ArcData>, true> WTF::__variant_base<WTF::Variant<WTF::Monostate, WebCore::MoveData, WebCore::LineData, WebCore::ArcData>, true>
  +8 < 32>       WTF::Variant<WTF::Monostate, WebCore::MoveData, WebCore::LineData, WebCore::ArcData>::__storage_type __storage
 +40 <  1>       WTF::__discriminator_type<4, false, false, false>::__type __index
 +41 <  3>   <PADDING: 3 bytes>
 +44 <  1>   bool m_copyPathBeforeMutation
 +45 <  3>   <PADDING: 3 bytes>
Total byte size: 48
Total pad bytes: 6
Padding percentage: 12.50 %

The problem seems to be that ArcData’s optional offset internally has 3 byte of padding, in which I unfortunately can’t fit any other members of the struct:

  +0 < 36> WebCore::ArcData
  +0 < 12>     WTF::Optional<WebCore::FloatPoint> offset
  +0 < 12>         WTF::OptionalBase<WebCore::FloatPoint> WTF::OptionalBase<WebCore::FloatPoint>
  +0 <  1>           bool init_
  +1 <  3>           <PADDING: 3 bytes>
  +4 <  8>           WTF::constexpr_storage_t<WebCore::FloatPoint> storage_
 +12 <  8>     WebCore::FloatPoint center
 +12 <  4>       float m_x
 +16 <  4>       float m_y
 +20 <  4>   float radius
 +24 <  4>   float startAngle
 +28 <  4>   float endAngle
 +32 <  1>   bool clockwise
 +33 <  3>   <PADDING: 3 bytes>

I certainly agree that the Optional is more idiomatic, and it’s possible an extra 8 bytes per Path isn’t going to have any noticeable effect. For the time being, I’ll stick with the bool and FloatPoint, but I’ll give this some more thought.

>> Source/WebCore/platform/graphics/InlinePathData.h:74
>> +    return { data };
> 
> This is ok as-is. No change is needed. The braces are not needed.

Removed the braces.

>> Source/WebCore/platform/graphics/InlinePathData.h:92
>> +    return { WTFMove(data) };
> 
> This is ok as-is. No change is needed. The optimal solution is to just return data because:
> 
> 1. Takes advantage of RVO

Removed the braces and WTFMove.

>> Source/WebCore/platform/graphics/InlinePathData.h:130
>> +    return { WTFMove(data) };
> 
> Ditto.

Removed the braces and WTFMove.

>> Source/WebCore/platform/graphics/Path.cpp:196
>> +    applySlowCase(function);
> 
> I think the paragraphing of all these functions with #if ENABLE(INLINE_PATH_DATA) sections works better if they have blank lines after #endif.

Sounds good! Added newlines after these #endifs

>> Source/WebCore/platform/graphics/Path.cpp:253
>> +        m_inlineData = ArcData { hasAnyInlineData() ? WTF::get<MoveData>(m_inlineData).location : FloatPoint(), point, radius, startAngle, endAngle, anticlockwise, hasAnyInlineData() };
> 
> This is ok-is. No change is needed. The optimal solution would be to initialize each struct member individually then move the struct into the the instance variable because:
> 
> 1. The names of the fields are self describing so no need to ever have to consider argument order if in the future this struct is expanded.
> 2. Removes the need to EXPLICITLY initialize location if hasAnyInlineDate() is false.
> 3. There are many arguments to pass, which increases the chance of passing them in the wrong order in the future if the struct is expanded or layout changed.

Fair point! Fixed.

This also made me realize that there’s a naming discrepancy between the PathCG and the other platforms for the final argument to addArc (anticlockwise or clockwise). Before this patch, the last argument to addArc for CG ports is clockwise, whereas it is anticlockwise for Direct2D and Cairo.

In this patch, I opted to not change any behavior and just plumb the flag through, but it looks like this should be followed up. I added a FIXME for this.

>> Source/WebCore/platform/graphics/Path.h:230
>> +    void createPlatformPath() const;
> 
> I don’t think this is a cross-platform function. I suggest naming this createCGPath and making it CG-specific. The one for CG is quite different from the ones for Cairo and Direct2D. I believe those two aren’t called anywhere and also they don’t check m_path for null so they are not really the same kind of function.
> 
> I know I keep harping on this, but the platformPath name is not really doing us any good.

Ah, okay, that makes much more sense. Changed to createCGPath and make it only for USE(CG).

I agree with the platformPath naming not being great, since nothing outside of Path.h uses platformPath without already knowing exactly what kind of path it’s going to be — I still intend to follow up with a cleanup patch for that!

>> Source/WebCore/platform/graphics/Path.h:233
>> +    static void applyInlinePathData(const InlinePathData&, PlatformPathPtr);
> 
> Need to remove this. We don’t have this function any more.

Whoops, good catch! Removed.

>> Source/WebCore/platform/graphics/Path.h:234
>> +    template <typename DataType> bool hasInlineData() const;
> 
> I prefer the format without a space after the word "template".

Fixed.

>> Source/WebCore/platform/graphics/Path.h:237
>> +    bool needsToApplyInlineData() const { return !m_path && hasAnyInlineData(); }
> 
> Not sure we need this function. It’s called in only one place.

Okay — removed the function.

>> Source/WebCore/platform/graphics/Path.h:322
>> +        return { WTFMove(path) };
> 
> This is ok as-is. No change needed. The optimal  solution is to just return path for the same reasons outlined earlier in this patch.

Okay, changed to just `return path;`

>> Source/WebCore/platform/graphics/Path.h:386
>> +    return { WTFMove(path) };
> 
> Ditto.

Fixed.

>> Source/WebCore/platform/graphics/cairo/PathCairo.cpp:450
>> +}
> 
> Dead code, lets not add this.

👍🏻

>> Source/WebCore/platform/graphics/cg/PathCG.cpp:94
>> +        CGPathMoveToPoint(m_path.get(), nullptr, move.location.x(), move.location.y());
> 
> This needs to be indented four more spaces.

Whoops, fixed.

>> Source/WebCore/platform/graphics/cg/PathCG.cpp:278
>> +    CGRect bound = CGPathGetPathBoundingBox(platformPath());
> 
> Seems like these functions could have used the "slow case" pattern.

Sounds good — pulled the implementation of these into Path.cpp, with private -SlowCase methods per platform.

>> Source/WebCore/platform/graphics/cg/PathCG.cpp:279
>>      return CGRectIsNull(bound) ? CGRectZero : bound;
> 
> I think we should make a static inline helper function to do this to tidy up the two places we do it. Convert a CGRect to a FloatRect, converting null CGRect into zero floating rect.

Yep, pulled this out into a static inline helper.

>> Source/WebCore/platform/graphics/displaylists/DisplayListItems.cpp:1231
>> +    context.strokePath(WTFMove(m_path));
> 
> Is it OK to move this? If so, why?

It’s okay to move this, because we don’t need the path anymore after applying the display list item.

>> Source/WebCore/platform/graphics/win/PathDirect2D.cpp:672
>> +    HRESULT hr = GraphicsContext::systemFactory()->CreateGeometryGroup(D2D1_FILL_MODE_WINDING, nullptr, 0, &m_path);
> 
> This is ok as-is. No change needed. A better solution is to either:
> 1. Add inline comment to describe 0
> 
> Or
> 
> 2. Define local with value 0 and pass it.
> 
> Either one because it makes the purpose of the 0 clear.

Ah, I’ve removed this method altogether.

>> Source/WebCore/platform/graphics/win/PathDirect2D.cpp:674
>> +}
> 
> Dead code; lets not add this.

Removed!

[Wenson Hsieh]

Created attachment 392991 [details]
For EWS

[Wenson Hsieh]

Committed r258118: <https://trac.webkit.org/changeset/258118>