Fix smallcaps #68
Fix smallcaps #68
Conversation
| FontPlatformData FontPlatformData::cloneWithSize(const FontPlatformData& source, float size) | ||
| { | ||
| FontPlatformData copy(source); | ||
| copy.m_font = std::make_shared<BFont>(source.font()); |
There was a problem hiding this comment.
shouldn't FontPlatformData copy constructor already do that? Then we could use the "default" method (although, as far as I can see, no one does, since it is disabled for both freetype and cocoa already).
There was a problem hiding this comment.
shouldn't FontPlatformData copy constructor already do that? Then we could use the "default" method (although, as far as I can see, no one does, since it is disabled for both freetype and cocoa already).
That then implement their version as an exact copy of the default one, and they don't use it anyway, because they use the full constructor with the new size, instead of cloneWithSize, which, apart from Haiku, it is only used in Windows, and even then only for remote fonts.
Updated to do it in the copy constructor. Notice that, apart from the clone methods here, that's also used in the Font constructor. I opened a handful of pages and got about 100 to 1 calls to copy constructor versus updateSize, which in my inexperienced view feels a more natural place than this or the one from the previous patch, specially given
haikuwebkit/Source/WebCore/platform/graphics/FontPlatformData.h
Lines 463 to 464 in dc4276d
I have the updateSize version in lonemadmax@85c6166, in case you change your mind. All three versions fix the issue.
There was a problem hiding this comment.
The BFont copy constructor is trivial, so I think it shouldn't be a problem to invoke it more often?
The default implementation to get a derived font with a different size just copies the platform data and sets the size. We need a new BFont, though, to avoid the size change in other text that is using the original.
|
I don't have time this week but I think I should discuss this with webkit devs as well.
|
|
I confirmed with WebKit devs that some other platforms do the font copy in updateSize:
Core Text for OSX: https://github.com/WebKit/WebKit/blob/cce33018ccdd163e1c5e15931ad3d6e3fb7a0642/Source/WebCore/platform/graphics/coretext/FontPlatformDataCoreText.cpp#L211
FreeType: https://github.com/WebKit/WebKit/blob/cce33018ccdd163e1c5e15931ad3d6e3fb7a0642/Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp#L168
Skia does not, in their case the default copy constructor apparently already makes a copy of the font: https://github.com/WebKit/WebKit/blob/cce33018ccdd163e1c5e15931ad3d6e3fb7a0642/Source/WebCore/platform/graphics/skia/FontPlatformDataSkia.cpp
Let's go with the majority and use MadMax's initial proposal then.
22 octobre 2025 à 17:55 "waddlesplash" ***@***.*** ***@***.***?to=%22waddlesplash%22%20%3Cnotifications%40github.com%3E > a écrit:
…
***@***.***** commented on this pull request.
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
In Source/WebCore/platform/graphics/haiku/FontPlatformDataHaiku.cpp #68 (comment) :
> @@ -132,6 +132,15 @@ FontPlatformData::FontPlatformData(const BFont& font, const FontDescription& fon
}
+FontPlatformData FontPlatformData::cloneWithSize(const FontPlatformData& source, float size)
+{
+ FontPlatformData copy(source);
+ copy.m_font = std::make_shared<BFont>(source.font());
The BFont copy constructor is trivial, so I think it shouldn't be a problem to invoke it more often?
—
Reply to this email directly, view it on GitHub #68 (comment) , or unsubscribe https://github.com/notifications/unsubscribe-auth/AAWTWSN325VFYS3U76ATT533Y6SFXAVCNFSM6AAAAACJSBTMC2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTGNRWGQ3DSNJTGM .
You are receiving this because you commented.
|
3 participants
The default implementation to get a derived font with a different size just copies the platform data and sets the size. We need a new BFont, though, to avoid the size change in other text that is using the original.