Bug 208492 – Path::m_path should be a RetainPtr<CGMutablePathRef> on platforms that use CoreGraphics

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

[Wenson Hsieh]

This would allow us to remove all of the manual CFRetain/CFRelease calls in PathCG.cpp.

[Wenson Hsieh]

Created attachment 392315 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 392315 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/PathCG.cpp:220
>      CGMutablePathRef path = CGPathCreateMutable();

This would be better as RetainPtr<CGMutablePathRef> path = adoptCF(), and do a WTFMove below.

> Source/WebCore/platform/graphics/cg/PathCG.cpp:223
> +    CGMutablePathRef path = CGPathCreateMutableCopyByTransformingPath(m_path.get(), &transformCG);

Ditto.

[Wenson Hsieh]

Comment on attachment 392315 [details]
Patch

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

>> Source/WebCore/platform/graphics/cg/PathCG.cpp:220
>>      CGMutablePathRef path = CGPathCreateMutable();
> 
> This would be better as RetainPtr<CGMutablePathRef> path = adoptCF(), and do a WTFMove below.

Good call!

>> Source/WebCore/platform/graphics/cg/PathCG.cpp:223
>> +    CGMutablePathRef path = CGPathCreateMutableCopyByTransformingPath(m_path.get(), &transformCG);
> 
> Ditto.

Fixed.

[Wenson Hsieh]

Created attachment 392322 [details]
For EWS

[Said Abou-Hallawa]

Comment on attachment 392315 [details]
Patch

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

> Source/WebCore/platform/graphics/Path.h:178
> +#endif
> +#if USE(DIRECT2D) || USE(CG)

I do not think because COMPtr and RetainPtr have the method get(), we should have a single function serving them both. I would prefer adding the same function twice for clarity. It took me awhile to figure out why CG begins to use the same code path of DIRECT2D.

> Source/WebCore/platform/graphics/Path.h:232
> +#elif USE(CG)
> +    mutable RetainPtr<CGMutablePathRef> m_path;
>  #else
> -    mutable PlatformPathPtr m_path { nullptr };
> +    PlatformPathPtr m_path { nullptr };
>  #endif

I do not think we should do that. At the top of this file, we already have large #if...#endif structure to define PlatformPath. After that we define 

typedef PlatformPath* PlatformPathPtr;

And here you are ignoring all of that and adding a new #elif USE(CG) section which defines m_path with another CG new type different from PlatformPath and PlatformPathPtr. I know you are adding it after the DIRECT2D section but I think the DIRECT2D section is also incorrect.

I would expect adding all the platform dependent definitions in one section and then use the platform type without any additional #if's. 

What is the problem with defining PlatformPathPtr like that:

#if USE(CG)
using PlatformPathPtr = RetainPtr<CGMutablePathRef>;
#elif USE(CAIRO)
using PlatformPathPtr = RefPtr<CairoPath>;
#elif USE(DIRECT2D)
using PlatformPathPtr = COMPtr<ID2D1GeometryGroup>;
#endif

In this case m_path can be defined like this:

    PlatformPathPtr m_path;

And platformPath() can be defined like this:

    PlatformPathPtr platformPath() const { return m_path; }

This will be similar to what we do for NativeImagePtr.

[Wenson Hsieh]

Comment on attachment 392315 [details]
Patch

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

>> Source/WebCore/platform/graphics/Path.h:178
>> +#if USE(DIRECT2D) || USE(CG)
> 
> I do not think because COMPtr and RetainPtr have the method get(), we should have a single function serving them both. I would prefer adding the same function twice for clarity. It took me awhile to figure out why CG begins to use the same code path of DIRECT2D.

Okay, I'll un-merge them.

>> Source/WebCore/platform/graphics/Path.h:232
>>  #endif
> 
> I do not think we should do that. At the top of this file, we already have large #if...#endif structure to define PlatformPath. After that we define 
> 
> typedef PlatformPath* PlatformPathPtr;
> 
> And here you are ignoring all of that and adding a new #elif USE(CG) section which defines m_path with another CG new type different from PlatformPath and PlatformPathPtr. I know you are adding it after the DIRECT2D section but I think the DIRECT2D section is also incorrect.
> 
> I would expect adding all the platform dependent definitions in one section and then use the platform type without any additional #if's. 
> 
> What is the problem with defining PlatformPathPtr like that:
> 
> #if USE(CG)
> using PlatformPathPtr = RetainPtr<CGMutablePathRef>;
> #elif USE(CAIRO)
> using PlatformPathPtr = RefPtr<CairoPath>;
> #elif USE(DIRECT2D)
> using PlatformPathPtr = COMPtr<ID2D1GeometryGroup>;
> #endif
> 
> In this case m_path can be defined like this:
> 
>     PlatformPathPtr m_path;
> 
> And platformPath() can be defined like this:
> 
>     PlatformPathPtr platformPath() const { return m_path; }
> 
> This will be similar to what we do for NativeImagePtr.

Unlike NativeImagePtr, there isn't actually any platform-agnostic code that takes advantage of a platform-agnostic PlatformPathPtr in WebKit (all the call sites that handle platform path pointers all already know what they want).

We chatted in person, and you recommended adding something like this to the top of the file:

```
#if USE(CG)
using PlatformPathStorageType = RetainPtr<CGMutablePathPtr>;
#elif USE(DIRECT2D)
using PlatformPathStorageType = COMPtr<ID2D1GeometryGroup>;
#else
using PlatformPathStorageType = PlatformPathPtr;
#endif
```

...which would allow us to just define this in the private section of Path:

```
mutable PlatformPathStorageType m_path;
```

(PathCairo.cpp starts its m_path as 0 anyways, so we don't need the { nullptr } at the end).

We could additionally consider making PlatformPathPtr a Retain/COMPtr on CG and Direct2D platforms respectively, but I think it's a bit cumbersome for all call sites to have to take ownership of the platform path, or call `.get()` to obtain the raw pointer. In any case, I'll pursue this second item as a followup.

[Wenson Hsieh]

Created attachment 392340 [details]
Address Said's comments + fix Win build

[Darin Adler]

Comment on attachment 392340 [details]
Address Said's comments + fix Win build

This is OK but I don’t really see a point in having all these types defined when there is no platform independent code using them. Seriously the entire point of the type names seems to be the so we can share one line to defining a private data member.

[WebKit Commit Bot]

Comment on attachment 392340 [details]
Address Said's comments + fix Win build

Clearing flags on attachment: 392340

Committed r257820: <https://trac.webkit.org/changeset/257820>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

I suggest removing PlatformPath, PlatformPathPtr. PlatformPathStorageType, platformPath, ensuurePlatformPath, and m_path, replacing them with separate platform-specific function and data members.

None of these are used in platform-independent code. The platform-specific code is made less clear by these confusing names. What does "platform" even mean in these names?

I believe that the NativeImagePtr pattern isn’t a good one and shouldn’t be replicated.

But aside from that, this case is different from NativeImagePtr because we are not using any of those things for platform-independent code.

[Wenson Hsieh]

(In reply to Darin Adler from comment #11)
> I suggest removing PlatformPath, PlatformPathPtr. PlatformPathStorageType,
> platformPath, ensuurePlatformPath, and m_path, replacing them with separate
> platform-specific function and data members.
> 
> None of these are used in platform-independent code. The platform-specific
> code is made less clear by these confusing names. What does "platform" even
> mean in these names?
> 
> I believe that the NativeImagePtr pattern isn’t a good one and shouldn’t be
> replicated.
> 
> But aside from that, this case is different from NativeImagePtr because we
> are not using any of those things for platform-independent code.

Sounds good — I’ll do it in a followup here: https://bugs.webkit.org/show_bug.cgi?id=208579.

Would you suggest we rename platformPath, ensurePlatformPath, and m_path to something along the lines of nativePath, ensureNativePath, and m_nativePath (respectively)?

[Darin Adler]

As I understand it, our pattern for the future for security’s sake it to use RefPtr and RetainPtr for return values all the time. So I think that pathCG (what is currently called platform path) should be returning RetainPtr<CGPathRef> even though that means writing get() at most of the call sites. This would be done to decrease the chance of subtle object lifetime bugs.

Not related to this refactoring, related to a general new direction across all of WebKit.

[Darin Adler]

(In reply to Wenson Hsieh from comment #12)
> Would you suggest we rename platformPath, ensurePlatformPath, and m_path to
> something along the lines of nativePath, ensureNativePath, and m_nativePath
> (respectively)?

For CG, I would suggesting calling them something like this:

pathCG or immutableCGPath or cgPath
mutableCGPath
m_path or m_pathCG or m_cgPath

No abstract name for "what CG is", because the code that uses this will all be CG-specific.