Bug 208492

Created attachment 392315 [details] Patch

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.

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.

Created attachment 392322 [details] For EWS

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.

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.

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

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.

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>

All reviewed patches have been landed. Closing bug.

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.

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

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.

(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.