Bug 215057

Created attachment 405780 [details] Patch

Comment on attachment 405780 [details] Patch This business where we derive structure from platform structures seems a little strange. Seems like we could do this with some helper functions instead.

(In reply to Darin Adler from comment #2) > Comment on attachment 405780 [details] > Patch > > derive structure from platform structures Can you explain a little more what you mean by this?

Comment on attachment 405780 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405780&action=review > Source/WebCore/platform/graphics/GlyphBuffer.h:107 > +struct GlyphBufferOrigin : CGPoint { This is an example of deriving a structure from the Cocoa structure "CGPoint". That’s what I meant by "deriving structures from platform structures".

Comment on attachment 405780 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405780&action=review >> Source/WebCore/platform/graphics/GlyphBuffer.h:107 >> +struct GlyphBufferOrigin : CGPoint { > > This is an example of deriving a structure from the Cocoa structure "CGPoint". That’s what I meant by "deriving structures from platform structures". Ah, I understand now. What kind of helper functions did you have in mind?

Comment on attachment 405780 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405780&action=review >>> Source/WebCore/platform/graphics/GlyphBuffer.h:107 >>> +struct GlyphBufferOrigin : CGPoint { >> >> This is an example of deriving a structure from the Cocoa structure "CGPoint". That’s what I meant by "deriving structures from platform structures". > > Ah, I understand now. > > What kind of helper functions did you have in mind? I can think of three possibilities: 1) Typedef CGPoint to something like NativePoint, and make freestanding functions that accept and perform operations on NativePoint 2) Make GlyphBufferOrigin own a CGPoint, rather than be a CGPoint 3) Keep it as it is now: GlyphBufferOrigin is a CGPoint Which of these do you think is best? (And why?)

Created attachment 405902 [details] Rebased

Created attachment 405903 [details] Rebased

Comment on attachment 405780 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405780&action=review >>>> Source/WebCore/platform/graphics/GlyphBuffer.h:107 >>>> +struct GlyphBufferOrigin : CGPoint { >>> >>> This is an example of deriving a structure from the Cocoa structure "CGPoint". That’s what I meant by "deriving structures from platform structures". >> >> Ah, I understand now. >> >> What kind of helper functions did you have in mind? > > I can think of three possibilities: > > 1) Typedef CGPoint to something like NativePoint, and make freestanding functions that accept and perform operations on NativePoint > 2) Make GlyphBufferOrigin own a CGPoint, rather than be a CGPoint > 3) Keep it as it is now: GlyphBufferOrigin is a CGPoint > > Which of these do you think is best? (And why?) I would prefer (2). But also not sure of the bigger picture. Is GlyphBufferOrigin a cross-platform abstraction? If so, then does it’s platform-independent interface involve CGFloat? I’d like to see what the cross-platform abstraction is and see some examples of platform-independent code using it. I’ve noticed our platform layer current has a lot of cross-platform non-abstraction. Supposedly platform-independent types and typedefs that don’t actually enable any shared platform-independent code.

Comment on attachment 405903 [details] Rebased View in context: https://bugs.webkit.org/attachment.cgi?id=405903&action=review > Source/WebCore/platform/graphics/GlyphBuffer.h:319 > + GlyphBufferOrigin origin = m_origins[index1]; auto

Comment on attachment 405780 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405780&action=review >>>>> Source/WebCore/platform/graphics/GlyphBuffer.h:107 >>>>> +struct GlyphBufferOrigin : CGPoint { >>>> >>>> This is an example of deriving a structure from the Cocoa structure "CGPoint". That’s what I meant by "deriving structures from platform structures". >>> >>> Ah, I understand now. >>> >>> What kind of helper functions did you have in mind? >> >> I can think of three possibilities: >> >> 1) Typedef CGPoint to something like NativePoint, and make freestanding functions that accept and perform operations on NativePoint >> 2) Make GlyphBufferOrigin own a CGPoint, rather than be a CGPoint >> 3) Keep it as it is now: GlyphBufferOrigin is a CGPoint >> >> Which of these do you think is best? (And why?) > > I would prefer (2). > > But also not sure of the bigger picture. Is GlyphBufferOrigin a cross-platform abstraction? If so, then does it’s platform-independent interface involve CGFloat? > > I’d like to see what the cross-platform abstraction is and see some examples of platform-independent code using it. > > I’ve noticed our platform layer current has a lot of cross-platform non-abstraction. Supposedly platform-independent types and typedefs that don’t actually enable any shared platform-independent code. > Is GlyphBufferOrigin a cross-platform abstraction? Yes. Cocoa ports use CGPoint so it can be passed directly to CTFontShapeGlyphs() or CTFontTransformGlyphsWIthLanguage(). Other ports use FloatPoint, and convert the FloatPoints to the platform-specific types (whatever Harfbuzz requires) later. > If so, then does its platform-independent interface involve CGFloat? Yes, but it shouldn't. I can fix it. > I’d like to see what the cross-platform abstraction is and see some examples of platform-independent code using it. FontCascade::drawGlyphBuffer() is one example.

Comment on attachment 405780 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405780&action=review >>>>>> Source/WebCore/platform/graphics/GlyphBuffer.h:107 >>>>>> +struct GlyphBufferOrigin : CGPoint { >>>>> >>>>> This is an example of deriving a structure from the Cocoa structure "CGPoint". That’s what I meant by "deriving structures from platform structures". >>>> >>>> Ah, I understand now. >>>> >>>> What kind of helper functions did you have in mind? >>> >>> I can think of three possibilities: >>> >>> 1) Typedef CGPoint to something like NativePoint, and make freestanding functions that accept and perform operations on NativePoint >>> 2) Make GlyphBufferOrigin own a CGPoint, rather than be a CGPoint >>> 3) Keep it as it is now: GlyphBufferOrigin is a CGPoint >>> >>> Which of these do you think is best? (And why?) >> >> I would prefer (2). >> >> But also not sure of the bigger picture. Is GlyphBufferOrigin a cross-platform abstraction? If so, then does it’s platform-independent interface involve CGFloat? >> >> I’d like to see what the cross-platform abstraction is and see some examples of platform-independent code using it. >> >> I’ve noticed our platform layer current has a lot of cross-platform non-abstraction. Supposedly platform-independent types and typedefs that don’t actually enable any shared platform-independent code. > > Aha! I tried (2) but got: error: static_cast from 'const WebCore::GlyphBufferAdvance *' to 'const CGSize *', which are not related by inheritance, is not allowed This makes total sense, and I think the weird inheritance thing less bad than a call to reinterpret_cast. I'll keep (3) and commit.

Comment on attachment 405780 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405780&action=review >>>>>>> Source/WebCore/platform/graphics/GlyphBuffer.h:107 >>>>>>> +struct GlyphBufferOrigin : CGPoint { >>>>>> >>>>>> This is an example of deriving a structure from the Cocoa structure "CGPoint". That’s what I meant by "deriving structures from platform structures". >>>>> >>>>> Ah, I understand now. >>>>> >>>>> What kind of helper functions did you have in mind? >>>> >>>> I can think of three possibilities: >>>> >>>> 1) Typedef CGPoint to something like NativePoint, and make freestanding functions that accept and perform operations on NativePoint >>>> 2) Make GlyphBufferOrigin own a CGPoint, rather than be a CGPoint >>>> 3) Keep it as it is now: GlyphBufferOrigin is a CGPoint >>>> >>>> Which of these do you think is best? (And why?) >>> >>> I would prefer (2). >>> >>> But also not sure of the bigger picture. Is GlyphBufferOrigin a cross-platform abstraction? If so, then does it’s platform-independent interface involve CGFloat? >>> >>> I’d like to see what the cross-platform abstraction is and see some examples of platform-independent code using it. >>> >>> I’ve noticed our platform layer current has a lot of cross-platform non-abstraction. Supposedly platform-independent types and typedefs that don’t actually enable any shared platform-independent code. >> >> > > Aha! I tried (2) but got: > > error: static_cast from 'const WebCore::GlyphBufferAdvance *' to 'const CGSize *', which are not related by inheritance, is not allowed > > This makes total sense, and I think the weird inheritance thing less bad than a call to reinterpret_cast. I'll keep (3) and commit. This shows that we have something very unsafe going on. We have an array of GlyphBufferAdvance and we pass it to something expecting an array of CGSize. This works only if we have added no data members. If I added a bool to GlyphBufferAdvance we’d have a lot of failures.

Comment on attachment 405780 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405780&action=review >>>>>>>> Source/WebCore/platform/graphics/GlyphBuffer.h:107 >>>>>>>> +struct GlyphBufferOrigin : CGPoint { >>>>>>> >>>>>>> This is an example of deriving a structure from the Cocoa structure "CGPoint". That’s what I meant by "deriving structures from platform structures". >>>>>> >>>>>> Ah, I understand now. >>>>>> >>>>>> What kind of helper functions did you have in mind? >>>>> >>>>> I can think of three possibilities: >>>>> >>>>> 1) Typedef CGPoint to something like NativePoint, and make freestanding functions that accept and perform operations on NativePoint >>>>> 2) Make GlyphBufferOrigin own a CGPoint, rather than be a CGPoint >>>>> 3) Keep it as it is now: GlyphBufferOrigin is a CGPoint >>>>> >>>>> Which of these do you think is best? (And why?) >>>> >>>> I would prefer (2). >>>> >>>> But also not sure of the bigger picture. Is GlyphBufferOrigin a cross-platform abstraction? If so, then does it’s platform-independent interface involve CGFloat? >>>> >>>> I’d like to see what the cross-platform abstraction is and see some examples of platform-independent code using it. >>>> >>>> I’ve noticed our platform layer current has a lot of cross-platform non-abstraction. Supposedly platform-independent types and typedefs that don’t actually enable any shared platform-independent code. >>> >>> >> >> Aha! I tried (2) but got: >> >> error: static_cast from 'const WebCore::GlyphBufferAdvance *' to 'const CGSize *', which are not related by inheritance, is not allowed >> >> This makes total sense, and I think the weird inheritance thing less bad than a call to reinterpret_cast. I'll keep (3) and commit. > > This shows that we have something very unsafe going on. > > We have an array of GlyphBufferAdvance and we pass it to something expecting an array of CGSize. This works only if we have added no data members. If I added a bool to GlyphBufferAdvance we’d have a lot of failures. We should use (1). GlyphBufferAdvance should be: using GlyphBufferAdvance = FloatSize; and using GlyphBufferAdvance = CGSize; on different platforms. Then we should use non-member functions for the things we need to do in cross-platform code that are different between these two.

Comment on attachment 405780 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405780&action=review >>>>>>>>> Source/WebCore/platform/graphics/GlyphBuffer.h:107 >>>>>>>>> +struct GlyphBufferOrigin : CGPoint { >>>>>>>> >>>>>>>> This is an example of deriving a structure from the Cocoa structure "CGPoint". That’s what I meant by "deriving structures from platform structures". >>>>>>> >>>>>>> Ah, I understand now. >>>>>>> >>>>>>> What kind of helper functions did you have in mind? >>>>>> >>>>>> I can think of three possibilities: >>>>>> >>>>>> 1) Typedef CGPoint to something like NativePoint, and make freestanding functions that accept and perform operations on NativePoint >>>>>> 2) Make GlyphBufferOrigin own a CGPoint, rather than be a CGPoint >>>>>> 3) Keep it as it is now: GlyphBufferOrigin is a CGPoint >>>>>> >>>>>> Which of these do you think is best? (And why?) >>>>> >>>>> I would prefer (2). >>>>> >>>>> But also not sure of the bigger picture. Is GlyphBufferOrigin a cross-platform abstraction? If so, then does it’s platform-independent interface involve CGFloat? >>>>> >>>>> I’d like to see what the cross-platform abstraction is and see some examples of platform-independent code using it. >>>>> >>>>> I’ve noticed our platform layer current has a lot of cross-platform non-abstraction. Supposedly platform-independent types and typedefs that don’t actually enable any shared platform-independent code. >>>> >>>> >>> >>> Aha! I tried (2) but got: >>> >>> error: static_cast from 'const WebCore::GlyphBufferAdvance *' to 'const CGSize *', which are not related by inheritance, is not allowed >>> >>> This makes total sense, and I think the weird inheritance thing less bad than a call to reinterpret_cast. I'll keep (3) and commit. >> >> This shows that we have something very unsafe going on. >> >> We have an array of GlyphBufferAdvance and we pass it to something expecting an array of CGSize. This works only if we have added no data members. If I added a bool to GlyphBufferAdvance we’d have a lot of failures. > > We should use (1). > > GlyphBufferAdvance should be: > > using GlyphBufferAdvance = FloatSize; > > and > > using GlyphBufferAdvance = CGSize; > > on different platforms. Then we should use non-member functions for the things we need to do in cross-platform code that are different between these two. Right, the current code is unsafe. I agree that (1) is better than (3). I did what you suggested, but it turned out to be quite a lot of changes - more than the rest of the patch. So, rather than pollute this one, I'm going to post a second patch to a new bug.

Committed r265261: <https://trac.webkit.org/changeset/265261>

<rdar://problem/66543482>

Comment on attachment 405780 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405780&action=review >>>>>>>>>> Source/WebCore/platform/graphics/GlyphBuffer.h:107 >>>>>>>>>> +struct GlyphBufferOrigin : CGPoint { >>>>>>>>> >>>>>>>>> This is an example of deriving a structure from the Cocoa structure "CGPoint". That’s what I meant by "deriving structures from platform structures". >>>>>>>> >>>>>>>> Ah, I understand now. >>>>>>>> >>>>>>>> What kind of helper functions did you have in mind? >>>>>>> >>>>>>> I can think of three possibilities: >>>>>>> >>>>>>> 1) Typedef CGPoint to something like NativePoint, and make freestanding functions that accept and perform operations on NativePoint >>>>>>> 2) Make GlyphBufferOrigin own a CGPoint, rather than be a CGPoint >>>>>>> 3) Keep it as it is now: GlyphBufferOrigin is a CGPoint >>>>>>> >>>>>>> Which of these do you think is best? (And why?) >>>>>> >>>>>> I would prefer (2). >>>>>> >>>>>> But also not sure of the bigger picture. Is GlyphBufferOrigin a cross-platform abstraction? If so, then does it’s platform-independent interface involve CGFloat? >>>>>> >>>>>> I’d like to see what the cross-platform abstraction is and see some examples of platform-independent code using it. >>>>>> >>>>>> I’ve noticed our platform layer current has a lot of cross-platform non-abstraction. Supposedly platform-independent types and typedefs that don’t actually enable any shared platform-independent code. >>>>> >>>>> >>>> >>>> Aha! I tried (2) but got: >>>> >>>> error: static_cast from 'const WebCore::GlyphBufferAdvance *' to 'const CGSize *', which are not related by inheritance, is not allowed >>>> >>>> This makes total sense, and I think the weird inheritance thing less bad than a call to reinterpret_cast. I'll keep (3) and commit. >>> >>> This shows that we have something very unsafe going on. >>> >>> We have an array of GlyphBufferAdvance and we pass it to something expecting an array of CGSize. This works only if we have added no data members. If I added a bool to GlyphBufferAdvance we’d have a lot of failures. >> >> We should use (1). >> >> GlyphBufferAdvance should be: >> >> using GlyphBufferAdvance = FloatSize; >> >> and >> >> using GlyphBufferAdvance = CGSize; >> >> on different platforms. Then we should use non-member functions for the things we need to do in cross-platform code that are different between these two. > > Right, the current code is unsafe. I agree that (1) is better than (3). > > I did what you suggested, but it turned out to be quite a lot of changes - more than the rest of the patch. So, rather than pollute this one, I'm going to post a second patch to a new bug. https://bugs.webkit.org/show_bug.cgi?id=215143