Skip to content

Commit

Permalink
Revert "[Cocoa] objectForEqualityCheck() is no longer necessary"
Browse files Browse the repository at this point in the history
https://bugs.webkit.org/show_bug.cgi?id=263136
rdar://116934547

Reviewed by Cameron McCormack.

This reverts commit dc989b3.

267312@main [1] broke the following test:
imported/w3c/web-platform-tests/css/css-text/line-break/line-break-anywhere-overrides-uax-behavior-011.html

Before [1] we used to define a derived object from the Fonts we were trying to compare,
because we considered that CFEqual() would do a shallow comparison.

Since it is possible to generate two font objects (with different pointers) representing
the same font object, a shallow comparison wouldn’t work for us.

[1] states that CFEqual() "now” does a deep comparison, and
the derived object would have no use anymore.

However, at Source/WebCore/platform/graphics/mac/ComplexTextControllerCoreText.mm:
ComplexTextController::collectComplexTextRunsForCharacters:

We observe that the comparisons before and after [1] produce different results,
for the specific test that is failing. More specifically we can reduce it to
the render of “X͏”:

-> Without [1] the following is never true:
 if (!safeCFEqual(runFontEqualityObject.get(), font->platformData().objectForEqualityCheck().get()))

This means that the fonts are considered equal. However:

-> With [1] the following is always true:
 if (!safeCFEqual(runCTFont, font->platformData().ctFont()))

This means that the run’s font, acquired after shaping is not the same as the font passed,
so we will try to add it to the fallback font list.

* Source/WebCore/platform/graphics/FontPlatformData.h:
* Source/WebCore/platform/graphics/coretext/FontPlatformDataCoreText.cpp:
(WebCore::FontPlatformData::objectForEqualityCheck):
(WebCore::FontPlatformData::objectForEqualityCheck const):
* Source/WebCore/platform/graphics/mac/ComplexTextControllerCoreText.mm:
(WebCore::ComplexTextController::collectComplexTextRunsForCharacters):

Canonical link: https://commits.webkit.org/273377@main
  • Loading branch information
vitorroriz committed Jan 23, 2024
1 parent 7123a96 commit d295764
Show file tree
Hide file tree
Showing 3 changed files with 21 additions and 3 deletions.
2 changes: 2 additions & 0 deletions Source/WebCore/platform/graphics/FontPlatformData.h
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,8 @@ class FontPlatformData {

#if USE(CORE_TEXT)
WEBCORE_EXPORT CTFontRef registeredFont() const; // Returns nullptr iff the font is not registered, such as web fonts (otherwise returns font()).
static RetainPtr<CFTypeRef> objectForEqualityCheck(CTFontRef);
RetainPtr<CFTypeRef> objectForEqualityCheck() const;
bool hasCustomTracking() const { return isSystemFont(); }

CTFontRef font() const { return m_font.get(); }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,21 @@ CTFontRef FontPlatformData::ctFont() const
return m_ctFont.get();
}

RetainPtr<CFTypeRef> FontPlatformData::objectForEqualityCheck(CTFontRef ctFont)
{
auto fontDescriptor = adoptCF(CTFontCopyFontDescriptor(ctFont));
// FIXME: https://bugs.webkit.org/show_bug.cgi?id=138683 This is a shallow pointer compare for web fonts
// because the URL contains the address of the font. This means we might erroneously get false negatives.
auto object = adoptCF(CTFontDescriptorCopyAttribute(fontDescriptor.get(), kCTFontReferenceURLAttribute));
ASSERT(!object || CFGetTypeID(object.get()) == CFURLGetTypeID());
return object;
}

RetainPtr<CFTypeRef> FontPlatformData::objectForEqualityCheck() const
{
return objectForEqualityCheck(ctFont());
}

RefPtr<SharedBuffer> FontPlatformData::openTypeTable(uint32_t table) const
{
if (RetainPtr<CFDataRef> data = adoptCF(CTFontCopyTable(ctFont(), table, kCTFontTableOptionNoOptions)))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -229,15 +229,16 @@ static CFDictionaryRef typesetterOptions()
// Therefore, we only need to inspect which font was actually used if isSystemFallback is true.
if (isSystemFallback) {
CFDictionaryRef runAttributes = CTRunGetAttributes(ctRun);
auto runCTFont = static_cast<CTFontRef>(CFDictionaryGetValue(runAttributes, kCTFontAttributeName));
CTFontRef runCTFont = static_cast<CTFontRef>(CFDictionaryGetValue(runAttributes, kCTFontAttributeName));
ASSERT(runCTFont && CFGetTypeID(runCTFont) == CTFontGetTypeID());
if (!safeCFEqual(runCTFont, font->platformData().ctFont())) {
RetainPtr<CFTypeRef> runFontEqualityObject = FontPlatformData::objectForEqualityCheck(runCTFont);
if (!safeCFEqual(runFontEqualityObject.get(), font->platformData().objectForEqualityCheck().get())) {
// Begin trying to see if runFont matches any of the fonts in the fallback list.
for (unsigned i = 0; !m_font.fallbackRangesAt(i).isNull(); ++i) {
runFont = m_font.fallbackRangesAt(i).fontForCharacter(baseCharacter);
if (!runFont)
continue;
if (safeCFEqual(runFont->platformData().ctFont(), runCTFont))
if (safeCFEqual(runFont->platformData().objectForEqualityCheck().get(), runFontEqualityObject.get()))
break;
runFont = nullptr;
}
Expand Down

0 comments on commit d295764

Please sign in to comment.