Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Handle missing font files more gracefully #1486

Merged
merged 5 commits into from
Dec 2, 2016
Merged

Handle missing font files more gracefully #1486

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
04cd65c
Handle missing font files more gracefully
aballway Dec 1, 2016
ee98009
More tests, use ComPtr like a grown up
aballway Dec 1, 2016
53503a9
return comment
aballway Dec 1, 2016
01f91d0
Maybe one of these days I will stop leaking memory... some day
aballway Dec 1, 2016
583f639
someday
aballway Dec 1, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion Frameworks/CoreGraphics/CGDataProvider.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,9 @@ - (NSUInteger)length {
CGDataProviderRef CGDataProviderCreateWithURL(CFURLRef url) {
NSString* path = [static_cast<NSURL*>(url) path];
CGDataProvider* ret = [[CGDataProvider alloc] initWithContentsOfFile:path];
ret->filename = path;
if (ret) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this and CGDataProviderCreateWithFilename are pretty much the same logic.
re-use/refactor. rather than introducing the same code path.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will all be changed for #1450 #WontFix

ret->filename = path;
}

return ret;
}
Expand Down
30 changes: 18 additions & 12 deletions Frameworks/CoreGraphics/DWriteWrapper.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -408,9 +408,6 @@ HRESULT _DWriteCreateFontFaceWithName(CFStringRef name, IDWriteFontFace** outFon
// Eg: Bold, Condensed, Light, Italic
std::shared_ptr<_DWriteFontProperties> properties = _DWriteGetFontPropertiesFromName(name);

// TODO: #1250: Need to be able to load fonts from the app's bundle
// For now return a default font to avoid crashes in case of missing fonts
// When #1250 is completed, remove this
if (!properties->familyName.get()) {
name = CFSTR("Segoe UI");
properties = _DWriteGetFontPropertiesFromName(name);
Expand Down Expand Up @@ -667,7 +664,7 @@ HRESULT STDMETHODCALLTYPE GetCurrentFontFile(_Out_ IDWriteFontFile** fontFile) {
RETURN_HR_IF(E_ILLEGAL_METHOD_CALL, m_location < 0 || m_location > CFArrayGetCount(m_fontDatas.get()));

if (0 <= m_location && m_location < m_previouslyCreatedFiles->size()) {
*fontFile = m_previouslyCreatedFiles->at(m_location).Get();
m_previouslyCreatedFiles->at(m_location).CopyTo(fontFile);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

opyTo(fontFile); [](start = 54, length = 16)

:D

} else {
ComPtr<IDWriteFactory> dwriteFactory;
RETURN_IF_FAILED(DWriteCreateFactory(DWRITE_FACTORY_TYPE_SHARED, __uuidof(IDWriteFactory), &dwriteFactory));
Expand Down Expand Up @@ -716,8 +713,9 @@ HRESULT RuntimeClassInitialize() {
return S_OK;
}

void AddDatas(CFArrayRef fontDatas, CFArrayRef* errors) {
HRESULT AddDatas(CFArrayRef fontDatas, CFArrayRef* errors) {
CFMutableArrayRef outErrors = nil;
HRESULT ret = S_OK;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ret [](start = 16, length = 3)

nit: result

if (errors) {
outErrors = CFArrayCreateMutable(nullptr, 0, &kCFTypeArrayCallBacks);
*errors = outErrors;
Expand All @@ -729,19 +727,24 @@ void AddDatas(CFArrayRef fontDatas, CFArrayRef* errors) {
if (data) {
if (CFSetContainsValue(m_fontDatasSet.get(), data)) {
__AppendErrorIfExists(outErrors, kCTFontManagerErrorAlreadyRegistered);
ret = S_FALSE;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why does this have to be S_FALSE?
Are you expecting this to succeed, where you have failures?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is created with a list of font files, so should one fail we need to return that it failed but still un(register) whatever fonts were valid

} else {
CFArrayAppendValue(m_fontDatas.get(), data);
CFSetAddValue(m_fontDatasSet.get(), data);
__AppendNullptrIfExists(outErrors);
}
} else {
__AppendErrorIfExists(outErrors, kCTFontManagerErrorInvalidFontData);
ret = S_FALSE;
}
}

return ret;
}

void RemoveDatas(CFArrayRef fontDatas, CFArrayRef* errors) {
HRESULT RemoveDatas(CFArrayRef fontDatas, CFArrayRef* errors) {
CFMutableArrayRef outErrors = nil;
HRESULT ret = S_OK;
if (errors) {
outErrors = CFArrayCreateMutable(nullptr, 0, &kCFTypeArrayCallBacks);
*errors = outErrors;
Expand All @@ -759,11 +762,15 @@ void RemoveDatas(CFArrayRef fontDatas, CFArrayRef* errors) {
__AppendNullptrIfExists(outErrors);
} else {
__AppendErrorIfExists(outErrors, kCTFontManagerErrorNotRegistered);
ret = S_FALSE;
}
} else {
__AppendErrorIfExists(outErrors, kCTFontManagerErrorInvalidFontData);
ret = S_FALSE;
}
}

return ret;
}

HRESULT STDMETHODCALLTYPE CreateEnumeratorFromKey(_In_ IDWriteFactory* factory,
Expand Down Expand Up @@ -802,7 +809,7 @@ static void __AppendNullptrIfExists(CFMutableArrayRef errors) {
};

// TLambda is a member function of our FontCollectionLoader which updates the internal state of the loader
// TLambda :: (DWriteFontCollectionLoader -> CFArrayRef -> CFArrayRef*) -> void
// TLambda :: (DWriteFontCollectionLoader -> CFArrayRef -> CFArrayRef*) -> HRESULT
template <typename TLambda>
static HRESULT __DWriteUpdateUserCreatedFontCollection(CFArrayRef datas, CFArrayRef* errors, TLambda&& func) {
ComPtr<IDWriteFactory> dwriteFactory;
Expand All @@ -820,21 +827,20 @@ static HRESULT __DWriteUpdateUserCreatedFontCollection(CFArrayRef datas, CFArray
RETURN_IF_FAILED(createdLoader);

// Call member function to update loader's font data
func(loader.Get(), datas, errors);
// Still want to update font collection with whatever fonts succeeded, so return ret at end
HRESULT ret = func(loader.Get(), datas, errors);

// DWrite won't create a new font collection unless we provide a new key each time
// So every time we modify the font collection increment the key
static size_t collectionKey = 0;
++collectionKey;

std::lock_guard<std::mutex> guard(_userCreatedFontCollectionMutex);
RETURN_IF_FAILED(
dwriteFactory->CreateCustomFontCollection(loader.Get(), &(collectionKey), sizeof(collectionKey), &_userCreatedFontCollection));
dwriteFactory->CreateCustomFontCollection(loader.Get(), &(++collectionKey), sizeof(collectionKey), &_userCreatedFontCollection));

// Update s_userFontPropertiesMap with new values
s_userFontPropertiesMap = __CreatePropertiesMapForFontCollection(_userCreatedFontCollection.Get());

return S_OK;
return ret;
}

// Registers user defined fonts to a collection so they can be created later
Expand Down
11 changes: 6 additions & 5 deletions Frameworks/CoreText/CTFontManager.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,12 +59,11 @@ static bool __CTFontManagerUpdateWithFonts(CFArrayRef fontURLs, CTFontManagerSco
woc::unique_cf<CFMutableArrayRef> fontDatas{ CFArrayCreateMutable(nullptr, count, &kCFTypeArrayCallBacks) };
for (size_t i = 0; i < count; ++i) {
NSData* data = [NSData dataWithContentsOfURL:static_cast<NSURL*>(CFArrayGetValueAtIndex(fontURLs, i))];
if (data != nullptr) {
CFArrayAppendValue(fontDatas.get(), (CFDataRef)data);
}
CFArrayAppendValue(fontDatas.get(), (CFDataRef)data);
}

return SUCCEEDED(func(fontDatas.get(), errors));
// S_FALSE represents partial failure so cannot use SUCCEEDED macro
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why use S_FALSE?
Are you expecting partial failure?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It made more sense to use S_FALSE than E_* that might mean a more fatal failure than what we're experiencing

return func(fontDatas.get(), errors) == S_OK;
}

// Gets CFData from CGFontRef if available, which are passed into DWriteWrapper methods
Expand All @@ -84,7 +83,9 @@ static bool __CTFontManagerUpdateWithGraphicsFont(CGFontRef font, CFErrorRef _Nu

woc::unique_cf<CFArrayRef> fontDatas{ CFArrayCreate(nullptr, (const void**)&data, 1, &kCFTypeArrayCallBacks) };
CFArrayRef errors = nil;
bool ret = SUCCEEDED(func(fontDatas.get(), &errors));

// S_FALSE represents partial failure so cannot use SUCCEEDED macro
bool ret = func(fontDatas.get(), &errors) == S_OK;
if (error != nil && errors != nil && CFArrayGetCount(errors) > 0L) {
*error = (CFErrorRef)CFRetain(CFArrayGetValueAtIndex(errors, 0));
}
Expand Down
78 changes: 77 additions & 1 deletion tests/unittests/CoreText/CTFontManagerTests.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,83 @@
TEST(CTFontManager, ShouldFailToUnregisterNonregisteredFonts) {
NSURL* testFileURL = __GetURLFromPathRelativeToModuleDirectory(@"/data/WinObjC.ttf");
CFErrorRef error = nullptr;
EXPECT_TRUE(CTFontManagerUnregisterFontsForURL((__bridge CFURLRef)testFileURL, kCTFontManagerScopeSession, &error));
EXPECT_FALSE(CTFontManagerUnregisterFontsForURL((__bridge CFURLRef)testFileURL, kCTFontManagerScopeSession, &error));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happened here?
How does this behave on the ref platform?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should return false, this was an error

EXPECT_NE(nullptr, error);
EXPECT_EQ(kCTFontManagerErrorNotRegistered, CFErrorGetCode(error));
}

TEST(CTFontManager, ShouldFailToRegisterNonexistentFont) {
NSURL* testFileURL = __GetURLFromPathRelativeToModuleDirectory(@"/data/BlatantlyNonexistentFont.ttf");
CFErrorRef error = nullptr;
EXPECT_FALSE(CTFontManagerRegisterFontsForURL((__bridge CFURLRef)testFileURL, kCTFontManagerScopeSession, &error));
EXPECT_NE(nullptr, error);
EXPECT_EQ(kCTFontManagerErrorInvalidFontData, CFErrorGetCode(error));

EXPECT_FALSE(CTFontManagerUnregisterFontsForURL((__bridge CFURLRef)testFileURL, kCTFontManagerScopeSession, &error));
EXPECT_NE(nullptr, error);
EXPECT_EQ(kCTFontManagerErrorInvalidFontData, CFErrorGetCode(error));
}

TEST(CTFontManager, ShouldFailToRegisterSameFontTwice) {
woc::unique_cf<CFStringRef> fontName{ CFSTR("WinObjC") };
NSURL* testFileURL = __GetURLFromPathRelativeToModuleDirectory(@"/data/WinObjC.ttf");
CFErrorRef error = nullptr;
EXPECT_TRUE(CTFontManagerRegisterFontsForURL((__bridge CFURLRef)testFileURL, kCTFontManagerScopeSession, &error));
EXPECT_EQ(nullptr, error);

woc::unique_cf<CTFontRef> font{ CTFontCreateWithName(fontName.get(), 20, nullptr) };
EXPECT_NE(nullptr, font);

StrongId<NSString> familyName = (NSString*)CTFontCopyFamilyName(font.get());
EXPECT_OBJCEQ(@"WinObjC", familyName);

EXPECT_FALSE(CTFontManagerRegisterFontsForURL((__bridge CFURLRef)testFileURL, kCTFontManagerScopeSession, &error));
EXPECT_NE(nullptr, error);
EXPECT_EQ(kCTFontManagerErrorAlreadyRegistered, CFErrorGetCode(error));

EXPECT_TRUE(CTFontManagerUnregisterFontsForURL((__bridge CFURLRef)testFileURL, kCTFontManagerScopeSession, &error));
EXPECT_EQ(nullptr, error);

woc::unique_cf<CTFontRef> failedFont{ CTFontCreateWithName(fontName.get(), 20, nullptr) };
EXPECT_NE(nullptr, failedFont);

familyName = (NSString*)CTFontCopyFullName(failedFont.get());
EXPECT_OBJCEQ(@"Segoe UI", familyName);
}

TEST(CTFontManager, ShouldRegisterMultipleFonts) {
woc::unique_cf<CFStringRef> firstFontName{ CFSTR("WinObjC") };
woc::unique_cf<CFStringRef> secondFontName{ CFSTR("WinObjC-Italic") };
NSArray* urls = @[
__GetURLFromPathRelativeToModuleDirectory(@"/data/WinObjC.ttf"),
__GetURLFromPathRelativeToModuleDirectory(@"/data/WinObjC-Italic.ttf")
];
CFArrayRef errors = nullptr;
EXPECT_TRUE(CTFontManagerRegisterFontsForURLs((__bridge CFArrayRef)urls, kCTFontManagerScopeSession, &errors));
EXPECT_NE(nullptr, errors);
EXPECT_EQ(nullptr, CFArrayGetValueAtIndex(errors, 0));
EXPECT_EQ(nullptr, CFArrayGetValueAtIndex(errors, 1));
CFRelease(errors);

woc::unique_cf<CTFontRef> firstFont{ CTFontCreateWithName(firstFontName.get(), 20, nullptr) };
EXPECT_NE(nullptr, firstFont);
StrongId<NSString> firstName = (NSString*)CTFontCopyFullName(firstFont.get());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tring> firstName = (NSString*)CTFontCopyFullName(firstFont.get()); [](start = 16, length = 66)

this feels wrong. The general = causes a retain??

EXPECT_OBJCEQ(@"WinObjC", firstName);

woc::unique_cf<CTFontRef> secondFont{ CTFontCreateWithName(secondFontName.get(), 20, nullptr) };
EXPECT_NE(nullptr, secondFont);
StrongId<NSString> secondName = (NSString*)CTFontCopyFullName(secondFont.get());
EXPECT_OBJCEQ(@"WinObjC Italic", secondName);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you have repeated code here and in few other tests that do the same thing.
Consider using a general function. This can help add more tests in the future.


EXPECT_TRUE(CTFontManagerUnregisterFontsForURLs((__bridge CFArrayRef)urls, kCTFontManagerScopeSession, &errors));
EXPECT_NE(nullptr, errors);
EXPECT_EQ(nullptr, CFArrayGetValueAtIndex(errors, 0));
EXPECT_EQ(nullptr, CFArrayGetValueAtIndex(errors, 1));
CFRelease(errors);

woc::unique_cf<CTFontRef> failedFont{ CTFontCreateWithName(secondFontName.get(), 20, nullptr) };
EXPECT_NE(nullptr, failedFont);

secondName = (NSString*)CTFontCopyFullName(failedFont.get());
EXPECT_OBJCEQ(@"Segoe UI", secondName);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addition of registering/unregistering multiple fonts test would be useful.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, this is a good point that I missed in the previous PR

In reply to: 90536678 [](ancestors = 90536678)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a test to attempt to register the same font.