Refactor some DWriteWrapper code by ms-jihua · Pull Request #1504 · microsoft/WinObjC

ms-jihua · 2016-12-02T23:04:37Z

Split DWriteWrapper, DWriteWrapper_CoreText among more files
- DWriteFontBinaryDataLoader is now in its own .h + .mm along with BinaryDataStream
- DWriteFontCollectionLoader is now in its own .h + .mm along with FileEnumerator
  - Renamed to DWriteFontBinaryDataCollectionLoader to get the name farther from the interface name (IDWriteFontCollectionLoader)
- Implementation for _DWriteFontCreatePathForGlyph(), and the __CGPathGeometrySink class,
  which was only used in this function, are now in a separate .mm file
- Implementation for DWriteFont_CoreText functions relating to CTFont, CTFontDescriptor are now in a separate .mm file
- Refactored _userFontCollection, s_userFontPropertiesMap, systemFontPropertiesMap to be behind an abstraction layer,
  DWriteFontCollectionHelper, which wraps around either a user or system IDWriteFontCollection and provides
  font name -> other name/family name, properties mappings.
  - This is in its own .h + .mm file
  - As a result, many _DWrite functions now reduce cleanly to
    __GetSystemFontCollectionHelper()->Foo(), lock, __GetUserFontCollectionHelper()->Foo()
Refactored __CreateDWriteTextFormat(), __CreateDWriteTextLayout()
- Removed export of _DWriteCreateTextFormat(), this is now done directly in DWriteWrapper_CoreText.mm
- Now return HRESULT instead of ComPtr, now create through out-pointers, changed naming convention
  - As a result, __CreateDWriteTextFormat no longer always returns a nullptr if an error occurs -
    this should not impact anything, as this was an internal function, and return values were never checked for nullptr anyway
- Use CFAttributedStringGetAttribute directly instead of copying to a dictionary first,
  this should save some time on redundant copies/copies of unused attributes
- Parts of these functions were moved into separate inline functions for easier reading
Reversed order of i, j in _DWriteGetFrame(), such that i is on the outside and j on the inside
- Replaced the inner two while loops, with a single for loop
Exported _GetDefaultUserLocaleName()
Broke parts of _DWriteCreateFontFaceWithFontDescriptor() into separate functions for easier readability (less nesting)
Moved CustomDWriteTextRenderer member functions bodies to inside the class body
_CFStringCreateUppercaseCopy now uses a nullptr CFLocale (faster, font names are not localized anyway)
- Removed a redundant call to CFStringCreateUppercaseCopy() in the initialization of a font property map
Changed most remaining instances where CFAutorelease was used to use woc::unique_cf instead
Standardized naming convention of functions that translate a DWrite constant to CT/CG, or vice-versa
Changed members of most C++ classes to use the m_ naming convention
Standardized most file-private functions to be static
Standardized most instances of loops that call functions that return HRESULTs to call continue on failure
Changed CTTypesetterCreateLineWithOffset() to log and return null instead of throw
Added a bunch of SAL annotations
Added a string helper for copying a CFString to a vector, this was a pattern that occurred multiple times
Added comments in various places

This change is

msft-Jeyaram · 2016-12-02T23:07:26Z

:D

msft-Jeyaram · 2016-12-02T23:09:20Z

+        return S_OK;
+    }
+
+    HRESULT STDMETHODCALLTYPE GetCurrentFontFile(_Out_ IDWriteFontFile** fontFile) {


fontFile [](start = 73, length = 8)

validity check. #Resolved

msft-Jeyaram · 2016-12-02T23:09:54Z

+        return S_OK;
+    }
+
+    HRESULT STDMETHODCALLTYPE MoveNext(_Out_ BOOL* hasCurrentFile) {


validity check? #ByDesign

msft-Jeyaram · 2016-12-02T23:12:11Z

++ Also thank you for draining the nestlings :)

msft-Jeyaram · 2016-12-02T23:13:40Z

+    // Deliberate no-op
+}
+
+HRESULT DWriteFontBinaryDataLoader::RuntimeClassInitialize(CFDataRef data) {


i'm being too picky here, but what if data was null.
Is this a valid success? #Resolved

msft-Jeyaram · 2016-12-02T23:15:51Z

+    DWriteFontBinaryDataStream() {
+    }
+
+    HRESULT RuntimeClassInitialize(CFDataRef data) {


same question as below.
I assume null data is not valid? #ByDesign

yeah, but since this is only used from the below class, it's sufficient that we validate below.

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

ms-jihua · 2016-12-02T23:15:59Z

+HRESULT STDMETHODCALLTYPE DWriteFontBinaryDataLoader::CreateStreamFromKey(_In_ const void* fontFileReferenceKey,
+                                                                          uint32_t fontFileReferenceKeySize,
+                                                                          _Out_ IDWriteFontFileStream** fontFileStream) {
+    RETURN_HR_IF_NULL(E_INVALIDARG, fontFileStream);


E_INVALIDARG [](start = 22, length = 12)

debating whether this should be E_POINTER instead... #Resolved

Usually OUT invalid args are pretty much: E_POINTER
used prominently in windows. use E_POINTER

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

msft-Jeyaram · 2016-12-02T23:19:11Z

+    ComPtr<DWriteFontBinaryDataStream> ret;
+    RETURN_IF_FAILED(MakeAndInitialize<DWriteFontBinaryDataStream>(&ret, m_data.get()));
+
+    *fontFileStream = ret.Detach();


ret.Detach() [](start = 22, length = 12)

CopyTo() #Resolved

the CopyTos are only truly necessary if the copy-from-er will be retaining a reference. it does add an unnecessary +1 -1 pair otherwise.

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

That's likely acceptable :)

In reply to: 90742277 [](ancestors = 90742277,90737178)

msft-Jeyaram · 2016-12-02T23:19:41Z

+        RETURN_IF_FAILED(textLayout->SetTypography(typography.Get(), dwriteRange));
    }

-    return textLayout;


xtLayout.Detach(); [](start = 23, length = 18)

use CopyTo #Resolved

same as above #ByDesign

msft-Jeyaram · 2016-12-02T23:20:15Z

+static HRESULT __DWriteTextLayoutCreate(CFAttributedStringRef string,
+                                        CFRange range,
+                                        CGRect frameSize,
+                                        _Out_ IDWriteTextLayout** outTextLayout) {


E_INVALIDARG [](start = 17, length = 12)

invalid pointer. #Resolved

msft-Jeyaram · 2016-12-02T23:20:32Z

+        RETURN_IF_FAILED(__DWriteTextFormatApplyParagraphStyle(textFormat, settings));
    }

-    return textFormat;


Detach [](start = 32, length = 6)

use CopyTo(...) #Resolved

still not CopyTo? #ByDesign

@aballway CopyTo adds an unnecessary +1 on the pointed-to COM object that's -1'd when the method returns immediately after. *x = y.Detach() is the idiomatic way to commute a +1 reference to a caller. #ByDesign

msft-Jeyaram · 2016-12-02T23:25:08Z

+
+        if (fragmentContext) {
+            // Deliberately unused: this is meant to be passed to ReleaseFileFragment() below to free part of the data,
+            // but this stream frees all the underlying CFData at once


nit: As mentioned earlier, have the comments to indicate something is unused at the top of the function.
This makes it much easier for usage. Pretty much call it out at the top. #Resolved

Given it's an internal class.. meh i'll leave it up to you.

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

msft-Jeyaram · 2016-12-02T23:26:19Z

+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+// THE SOFTWARE.
+//
+//******************************************************************************


all of the headers need #pragma once #Resolved

oh yeah! i knew i forgot something...

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

msft-Jeyaram · 2016-12-02T23:29:19Z

+    CFMutableArrayRef CopyFontFamilyNames();
+    CFMutableArrayRef CopyFontNamesForFamilyName(CFStringRef familyName);
+    std::shared_ptr<_DWriteFontProperties> GetFontPropertiesFromUppercaseFontName(const woc::unique_cf<CFStringRef>& upperFontName);
+    HRESULT CreateFontFamilyWithName(_In_ const wchar_t* unicharFamilyName, _Out_ IDWriteFontFamily** outFontFamily);


Out [](start = 76, length = 5)

as per SAL Outptr is the most appropriate here.
use Outptr #Resolved

just curious why we decided to use SAL here. Do we have uniform conventions about this?

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

It seemed more preferable than not using SAL, and we're dealing with COM out pointers which typically uses SAL in function signatures. I don't have a strong opinion on this, I can remove them if you feel it's better that way.

In reply to: 90743039 [](ancestors = 90743039,90738161)

It fine to use SAL I guess (we don't get the benefit of actually doing anything useful with it except for reading clarity) but it seems to be inconsistent about when you are using it. Take the below _GetFontLIstForFamilyName method that doesn't annotate its in parameter. If we are going to do it, lets make it consistent so that COM style HRESULT returning functions all use SAL or whatever ...

In reply to: 90746813 [](ancestors = 90746813,90743039,90738161)

? That function does annotate in its parameter..., or do you just mean that it doesn't annotate the CFStringRef?

In reply to: 90917892 [](ancestors = 90917892,90746813,90743039,90738161)

well seeing as I mentioned an in parameter, which one do you think I meant? If you don't think a CF type should be annotated, it begs the question why and what value you are seeing from this process ...

In reply to: 90928031 [](ancestors = 90928031,90917892,90746813,90743039,90738161)

ah, I missed a word. I think I'll just get rid of the SAL then. it was a nice idea but I think the maintenance cost on it going forward won't be worth it for what little we gain in terms of readability.

In reply to: 90931524 [](ancestors = 90931524,90928031,90917892,90746813,90743039,90738161)

msft-Jeyaram · 2016-12-02T23:31:42Z

// #1207: Do not move this block, it has to come first for some reason

given the task is closed, doesn't make sense to have it here anymore.
this mystery might be solved one day. #Resolved

Refers to: Frameworks/CoreGraphics/DWriteWrapper.mm:17 in 4fad654. [](commit_id = 4fad654, deletion_comment = False)

bbowman · 2016-12-03T00:17:06Z

-COREGRAPHICS_EXPORT CFStringRef _CFStringFromLocalizedString(IDWriteLocalizedStrings* localizedString);
+#ifdef __cplusplus
+extern "C++" std::wstring _GetUserDefaultLocaleName();
+#endif


not sure how I feel about this. Why are we cross module exporting c++ things? #Resolved

I guess I don't have a better answer for you than 'convenience'. The alternative would be to make the caller allocate a char buffer for us to dump into. Same below, it's more convenient to just pass a shared_ptr around than to define (iirc) a copy constructor for that struct. What are the downsides of cross module exporting c++?

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

Discussed offline. It's a little sketch still but we're letting it pass in this case.

In reply to: 90993784 [](ancestors = 90993784,90742313)

bbowman · 2016-12-03T00:17:19Z

+// Font name <-> font family, font properties
 #ifdef __cplusplus
 extern "C++" std::shared_ptr<_DWriteFontProperties> _DWriteGetFontPropertiesFromName(CFStringRef fontName);
 #endif


also here. #Closed

continuing discussion above

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

bbowman · 2016-12-03T00:18:49Z

    return ContainerFromDelimitedString<std::vector<std::wstring>>(delimitedString, delimiter);
 }

+#ifdef __OBJC__


OBJC [](start = 7, length = 8)

is this actually needed? CF should be useable from straight c / c++ right? #Resolved

it's actually needed. do you want to grab the build error for you?

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

I want you to find the error and think about it more. I have a sneaking suspicion that the error is more related to our #defs etc for clang than for objective c as a language. I bet if you built for c++ in clang it would work fine. This mostly points to a failing in our sbclang.props / .targets in that its not symmetric building clang and cl but i'd like us to not overindex on this difference as it makes fixing it even harder as each of these things needs to be re-evaluated for what they really are trying to do.

perhaps an ifdef for clang? or something else related to having CoreFoundation includes available?

In reply to: 90938607 [](ancestors = 90938607,90742443)

1>f:\winobjc\frameworks\include\stringhelpers.h(21): fatal error C1083: Cannot open type library file: 'f:\winobjc\include\corefoundation\cfstring.h': Error loading type library/DLL.

i'm not actually sure what to make of this??

In reply to: 90942112 [](ancestors = 90942112,90938607,90742443)

bbowman · 2016-12-03T00:19:49Z

+// Note also that Thin and Ultra/Extra-Light are in opposite order in DWrite and CoreText/UIKit constants
+// (However, "Thin" fonts on the reference platform have UIFontWeightUltraLight...)
+// clang-format off
+static const struct __WeightMapping c_weightMap[] = { { DWRITE_FONT_WEIGHT_THIN, kCTFontWeightUltraLight },


__WeightMapping [](start = 20, length = 15)

could make this an anonymous definition instead of using weird __ syntax to try to indicate its super private? #Resolved

bbowman · 2016-12-03T00:21:35Z

    }
 }

+static DWRITE_WORD_WRAPPING __CTLineBreakModeToDWrite(CTLineBreakMode lineBreakMode) {


__CTLineBreakModeToDWrite [](start = 28, length = 25)

whats the coding convention on _ vs __ as a prefix? #Resolved

__ is file-private, _ is framework-"private" (not intended to be used by general consumers but exported nonetheless)

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

you used the word export there and I want to make sure we actually know what the relationships are there. Did you mean exported, as in a dll export, or extern, as in visible outside the current translation unit (.mm or .cpp)

In reply to: 90746929 [](ancestors = 90746929,90742638)

both, I think? we have unit tests that test _ functions

In reply to: 90918451 [](ancestors = 90918451,90746929,90742638)

hmmm. Seems like we should be more confident with this. How do the unit tests build? Are these items in a .def file? Would a private dll export have any other potential pitfalls with regards to encapsulation and versioning?

In reply to: 90936774 [](ancestors = 90936774,90918451,90746929,90742638)

How do the unit tests build?
not sure what this question means, or what answer you're fishing for. probably something like "CoreGraphics.UnitTests links agains CoreGraphics dll"?

Are these items in a .def file?
yes

Would a private dll export have any other potential pitfalls with regards to encapsulation and versioning?
i mean, we deliberately broke encapsulation here. versioning risks... _ is intended to communicate the same level of privacy as the stuff in CFFoundationInternal.h, so I figured we'd just continue whatever risk we accepted already, but yeah because of this we'd have to make sure unit tests, coregraphics, coretext, and foundation, all get a version update if the contract of these functions changes.

In reply to: 90937900 [](ancestors = 90937900,90936774,90918451,90746929,90742638)

Yep thats basically what I wanted to know. I was being on purposely obstinate here to make sure we understand the risks and implications. I'm fine if we are ok making that assertion and understand how we need to ship these things together as a result. Just wanted to make sure we are thinking about it and clearly communicating it.

In reply to: 90940630 [](ancestors = 90940630,90937900,90936774,90918451,90746929,90742638)

bbowman · 2016-12-03T00:25:36Z

+    RETURN_HR_IF_NULL(S_FALSE, fontCollection);
+
+    size_t fontFamilyIndex;
+    BOOL fontFamilyExists;


uninitialized #ByDesign

same as the other uninitialized comment (https://msdn.microsoft.com/en-us/library/windows/desktop/dd368217(v=vs.85).aspx)

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

bbowman · 2016-12-03T00:26:11Z

+            auto info = std::make_shared<_DWriteFontProperties>();
+            ComPtr<IDWriteLocalizedStrings> displayName;
+            ComPtr<IDWriteLocalizedStrings> postScriptName;
+            BOOL exist;


exist [](start = 17, length = 5)

uninitialized. #ByDesign

does this actually matter? I would expect GetInformationalStrings() to always populate this.

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

does it? Is that a safe assumption from the caller's point of view?

In reply to: 90747010 [](ancestors = 90747010,90742971)

https://msdn.microsoft.com/en-us/library/windows/desktop/dd371147(v=vs.85).aspx
exists [out]
Type: BOOL*
When this method returns, TRUE if the font contains the specified string ID; otherwise, FALSE.

I think it's probably safe to interpret that as saying 'this will always be populated'?

In reply to: 90931716 [](ancestors = 90931716,90747010,90742971)

probably. I was trying to get you to think about a little more defensive programming but whatever.

In reply to: 90936195 [](ancestors = 90936195,90931716,90747010,90742971)

bbowman · 2016-12-03T00:29:18Z

+
+    HRESULT RuntimeClassInitialize(CFDataRef data) {
+        CFRetain(data);
+        m_data.reset(data);


this pattern suggests an issue in the design here. Should we create a shared_cf? or maybe more correctly just auto_cf? #WontFix

mm, I don't think you need to go that far. this is still one-lineable, I just picked this way of stating it since CFRetain returns CFTypeRef, and I find casting back rather ugly. If we wanted, we could add like a 'retain_and_reset' to unique_cf and it'd make this neater.

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

bbowman · 2016-12-03T00:30:04Z

+        m_lastWriteTime |= static_cast<uint64_t>(fileTime.dwLowDateTime);
+        m_lastWriteTime |= static_cast<uint64_t>(fileTime.dwHighDateTime) << 32;
+        return S_OK;
+    }


RuntimeClassInitialize is useful if failure is possible. That doesn't seem to be the case here. Why not just make a ctor with args? #Resolved

bbowman · 2016-12-03T00:30:50Z

+        return S_OK;
+    };
+
+    void STDMETHODCALLTYPE ReleaseFileFragment(void* fragmentContext){


ReleaseFileFragment [](start = 27, length = 19)

I see what you are doing but should you reset m_data here? What is the contract supposed to be? #ByDesign

The IDWriteFontFileStream is that the stream is intended to be releasable little-by-little as it's consumed. ReadFileFragment() is expected to return information about the data that was read, through fragmentContext. ReleaseFileFragment() is supposed to then take this, to release part of the stream. Resetting m_data would do the wrong thing by releasing all the data here, instead of just some of it. (I've verified that callers actually do the bit-by-bit read/release)

CFDataRef doesn't have a clean mapping to this. We could engineer one using CFMutableData but I don't think we care enough. It's much easier to just go through normal CF-lifetime patterns and release the data all at once.

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

would CFStream afford you anything nicer / different / more efficient?

In reply to: 90747118 [](ancestors = 90747118,90743300)

Doesn't seem like it, it doesn't offer a little-by-little release either.

In reply to: 90916971 [](ancestors = 90916971,90747118,90743300)

it offers little by little reading which allows you to build releasable chunks no? This would improve memory consumption right?

In reply to: 90935815 [](ancestors = 90935815,90916971,90747118,90743300)

@bbowman I think the correct abstraction here is CGDataProvider. Right now, it is incomplete, and can't serve this purpose.

In reply to: 90937378 [](ancestors = 90937378,90935815,90916971,90747118,90743300)

@bbowman I'm not sure what you mean by 'build releasable chunks'. Are you suggesting that we write an extension/subclass to CFReadStream that releases bytes as it goes? That seems like a bit heavy in terms of engineering cost for this.

@DHowett-MSFT I'm not certain if CGDataProvider would fare better either, its release callback appears to want to free all the data at once as well https://developer.apple.com/reference/coregraphics/cgdataproviderreleasedatacallback?language=objc

In reply to: 90945909 [](ancestors = 90945909,90937378,90935815,90916971,90747118,90743300)

Talked offline but the basic idea was that CFReadStream can read into arbitrary buffers which we could manage and release when they tell us to. The real kicker is that we are stealing the whole data from CGDataProvider at the beginning nullifying this benefit. A later change that actually implements CGDataProvider correctly should do better.

In reply to: 90956518 [](ancestors = 90956518,90945909,90937378,90935815,90916971,90747118,90743300)

bbowman · 2016-12-03T00:32:00Z

+    ComPtr<DWriteFontBinaryDataStream> ret;
+    RETURN_IF_FAILED(MakeAndInitialize<DWriteFontBinaryDataStream>(&ret, m_data.get()));
+
+    return ret.CopyTo(fontFileStream);


CopyTo [](start = 15, length = 6)

prefer detach here if you aren't saving the reference internally for later. CopyTo adds an extra AddRef that technically isn't needed since you are transferring ownership. #Resolved

bbowman · 2016-12-03T00:34:54Z

+    int m_location = -1;
+
+    woc::unique_cf<CFArrayRef> m_fontDatas;
+    std::vector<ComPtr<IDWriteFontFile>>* m_previouslyCreatedFiles;


[](start = 40, length = 1)

interesting decision to use a raw * here. What is the ownership contract of this vector? #Resolved

This is owned by the FontCollectionLoader which creates this FontFileEnumerator. FileEnumerator will never exist without the CollectionLoader. #Resolved

The fact that this class, DWriteFontFileEnumerator, is a COM class makes this a dubious assumption. COM describes a shared ownership of objects. Presumably this guy is being handed out somewhere and the caller is free to hang on to it for as long as he/she wants. This could be longer than DWriteFontBinaryDataCollectionLoader is held onto. And in fact there is nothing else linking the Enumerator to the Loader which raises serious concerns about the assumptions you made in that comment. Can you explain more about how you are ensuring the validity of this pointer?

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

Our FontCollectionLoader is a static singleton, so it will always exist #Resolved

seems like this guy deserves a comment about this assumption or a move to a shared_ptr between the two objects to make the ownership clear from the code at all locations and increase the flexibility of the future design (i.e. there is no real good reason to have it as a singleton except that it makes life somewhat easier / faster but that shouldn't be a forcing function on the rest of the design)

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

I'll look into making this a shared_ptr or something similar, I think...

In reply to: 90943527 [](ancestors = 90943527,90917725)

Documentation for the interface recommends they be singletons, and it seems silly to move it to a shared_ptr when it shouldn't be owning it at all. Maybe a comment would be helpful but I think wrapping it in a shared_ptr would suggest the opposite of what we want.

In reply to: 90943752 [](ancestors = 90943752,90943527,90917725)

"shouldn't be owning at all" ... the class blindly uses the pointer which implies something about the lifetime of that memory. This is pretty much the definition of shared ownership. It is saying, "I didn't create the thing, but I have a reasonable expectation that whenever I want, I can use it and therefore store it among my member variables, the set of objects I own and manage". To correctly make that work, you need something that enforces that. Right now its a tenuous contract with the singleton parent object that requires a future code maintainer to read and understand all of the dwrite loader classes and uses as there is nothing else linking the two. The "child" class is not a child in any traditional sense. Its lifetime is not tied to the parent, its definition is not private to the parent, etc.

This makes the contract hard to continue in the future as someone may invalidate the baseline assumption and then be real confused. One of the amazing things about shared_ptr and friends is that the type itself implies the ownership so its much harder to do the wrong thing and at all times the use is clear and communicated.

My 2 cents is that code should be correct first, maintainable second, and fast/clever/whatever third. If you think that the shared_ptr is overly cumbersome, please take performance measurements to show its impact and we can evaluate from there. Until then, lets make it as easy as possible for the next person.

In reply to: 90944931 [](ancestors = 90944931,90943752,90943527,90917725)

settled on a pattern where the enumerator delegates to the loader, which owns the data.

In reply to: 90948737 [](ancestors = 90948737,90944931,90943752,90943527,90917725)

bbowman · 2016-12-03T00:37:21Z

+    HRESULT ret = S_OK;
+    if (errors) {
+        outErrors = CFArrayCreateMutable(nullptr, 0, &kCFTypeArrayCallBacks);
+        *errors = outErrors;


*errors = outErrors; [](start = 8, length = 20)

strange pattern. Typically you want to wait until the end to assign out parameters as returning a "half baked" structure in the case of failure can create confusion on the part of the caller. #Resolved

bbowman · 2016-12-03T00:38:25Z

+            }
+        } else {
+            __AppendErrorIfExists(outErrors, kCTFontManagerErrorInvalidFontData);
+            ret = S_FALSE;


S_FALSE [](start = 18, length = 7)

This is my least favorite return ever. It is still a success value (S_* is success) so all the macros like RETURN_IF_FAILED won't trip. I'd recommend against a pattern that relies on this. #ByDesign

Mm, pushing back on this. I'm not convinced that a single iteration failing here should fail the whole function, and I don't think that RETURN_IF_FAILED should be tripped in such a case. S_FALSE has a connotation of 'partial success', and we have the errors out-array if we need more specific error information anyway.

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

bbowman · 2016-12-03T00:38:49Z

aballway

Awesome job on this, sorry I complicated everything right before you did this :D

aballway · 2016-12-05T17:01:13Z

+    int m_location = -1;
+
+    woc::unique_cf<CFArrayRef> m_fontDatas;
+    std::vector<ComPtr<IDWriteFontFile>>* m_previouslyCreatedFiles;


This is owned by the FontCollectionLoader which creates this FontFileEnumerator. FileEnumerator will never exist without the CollectionLoader. #Resolved

aballway · 2016-12-05T17:06:38Z

+
+std::shared_ptr<_DWriteFontProperties> DWriteFontCollectionHelper::GetFontPropertiesFromUppercaseFontName(
+    const woc::unique_cf<CFStringRef>& upperFontName) {
+    if (!m_propertiesMap) {


should this be thread-safe? #Resolved

was thinking about this over the weekend and probably yes

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

aballway · 2016-12-05T17:08:05Z

+        return {};
+    }
+
+    _DWriteFontProperties ret = {};


super nit: move this above and return if !traits #Resolved

aballway · 2016-12-05T17:15:56Z

+    HRESULT result =
+        __GetSystemFontCollectionHelper()->CreateFontFamilyWithName(reinterpret_cast<const wchar_t*>(familyNameUnichars.data()),
+                                                                    outFontFamily);
+    if (result != S_FALSE) {


nit: == S_OK? #ByDesign

no, this is also intended to be an early return if the call failed above.

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

bbowman · 2016-12-05T19:20:19Z

    // Create singleton Font Collection loader, which will be shared for registering/unregistering fonts
-    static ComPtr<DWriteFontCollectionLoader> loader;
-    static HRESULT createdLoader = [](DWriteFontCollectionLoader** collectionLoader) {
+    static ComPtr<DWriteFontBinaryDataCollectionLoader> loader;


loader [](start = 56, length = 6)

s_loader? #Resolved

do we do this naming convention for function-local statics?

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

hence the question mark, lol. I personally think its more clear but don't care too much

In reply to: 90938094 [](ancestors = 90938094,90935689)

bbowman · 2016-12-05T19:22:29Z

-    // Update s_userFontPropertiesMap with new values
-    s_userFontPropertiesMap = __CreatePropertiesMapForFontCollection(_userCreatedFontCollection.Get());
+    // Update the user font collection
+    std::lock_guard<std::recursive_mutex> guard(s_userFontCollectionLock);


s_userFontCollectionLock [](start = 48, length = 24)

what is this protecting? Seems like the encapsulation is potentially wrong if the FontCollectionHelper is the one actually manipulating the data that needs protecting. Why not lock in that UpdateCollection function? #Resolved

read-write access to the whole font collection helper. if we locked in UpdateCollection(), we could still call ->CopyFontFamilyNames() from it, for instance. (we could alleviate that by overriding CopyFontFamilyNames() and the other read functions to also lock, but I don't find that to be preferable to the current pattern of a lock around the whole data structure)

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

The key thing you said is "lock around the whole data structure"

This points to a problem where you don't have the ownership quite correct. The data structure should be able to manage itself (hooray object oriented programming) and therefore the responsibility of locking should be encapsulated to the owning class / object. The pattern as is presents maintainability issues as every new callsite needs to remember to lock otherwise the whole lock is useless. Presumably the problem space for the class itself is much smaller and isolated which makes this a simpler maintainability problem.

In reply to: 90937664 [](ancestors = 90937664,90936157)

That makes sense. I'll move the locks to inside the class.

In reply to: 90938541 [](ancestors = 90938541,90937664,90936157)

bbowman · 2016-12-05T19:28:00Z

+    RETURN_IF_FAILED(DWriteCreateFactory(DWRITE_FACTORY_TYPE_SHARED, __uuidof(IDWriteFactory), &dwriteFactory));
+
+    size_t index = 0;
+    BOOL exists = FALSE;


LOL consistency. #Resolved

o>

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

msft-Jeyaram · 2016-12-05T20:01:23Z

+// DWriteFontBinaryDataCollectionLoader member functions
+
+HRESULT DWriteFontBinaryDataCollectionLoader::DWriteFontBinaryDataCollectionLoader::RuntimeClassInitialize() {
+    m_fontDatas.reset(CFArrayCreateMutable(kCFAllocatorDefault, 0, &kCFTypeArrayCallBacks));


kCFAllocatorDefault [](start = 43, length = 19)

consistent usage. #Resolved

DHowett-MSFT · 2016-12-05T20:07:30Z

+                                          IDWriteFontCollectionLoader> {
+protected:
+    InspectableClass(L"Windows.Bridge.DirectWrite", TrustLevel::BaseTrust);
+


Our runtime class names should have the final class name in them as well; right now they will collide since they're just namespaces #ByDesign

very true (its not ever going to be used since we aren't registering our runtime classes to be activatable in a RoActivateInstance type scenario) but yes would be great to do.

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

Thinking about this more, is there any benefit we gain from having these classes be IInspectable and not simply IUnknown? It looks like DWrite uses Classic COM only.

In reply to: 90946927 [](ancestors = 90946927,90944936)

be wary of classic com as the runtime flags. We should make ours mix because it implies the implementation is Agile (can be called on any thread). Unless we want to cause sta marshalling of calls, we probably want to stay with IInspectable and be agile.

In reply to: 91176606 [](ancestors = 91176606,90946927,90944936)

ms-jihua · 2016-12-06T21:16:32Z

Ping! #Closed

DHowett-MSFT · 2016-12-06T21:32:07Z

+                CFSetRemoveValue(m_fontDatasSet.get(), data);
+
+                for (auto it = m_fontDatas.begin(); it != m_fontDatas.end(); ++it) {
+                    if (CFEqual(it->first.get(), data)) {


O(N*M)! While this is fairly low-bounded for the common case, I'm worried about not using something more hashtabley here. CFData supports hashing, and we could make do with an adapter like CFStringRef's? #Resolved

Oh, thanks for noticing this (little bit glad that the changes made the O(N*M) more apparent). Thinking about how to change m_fontDatasSet to be something better for this purpose...

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

Probably rather than the current setup (CFSet and vector), we should just use an std::unordered_map that uses CFData's hash/equal (unless there's some good way to use CFDictionary instead, but I don't know if I trust CFDictionary to take a ComPtr...)

In reply to: 91189304 [](ancestors = 91189304,91176040)

DHowett-MSFT · 2016-12-06T21:39:50Z

+        m_lastWriteTime |= static_cast<uint64_t>(fileTime.dwHighDateTime) << 32;
+    }
+
+    HRESULT RuntimeClassInitialize() {


RuntimeClassInitialize [](start = 12, length = 22)

If we use Make I don't think we even need RuntimeClassInitialize as a stub! #Resolved

aballway · 2016-12-06T21:26:55Z

+    const woc::unique_cf<CFStringRef>& upperFontName) {
+    std::lock_guard<std::recursive_mutex> lock(m_lock);
+
+    if (!m_propertiesMap) {


nit: maybe a dispatch once here rather than flag and a mutex #ByDesign

it can happen more than once, eg UserFontCollectionHelper clears this ptr

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

aballway · 2016-12-06T21:30:35Z

-    return wstring(defaultLocaleSuccess ? localeName : c_defaultUserLanguage);
-}
+        ComPtr<IDWriteFontCollection> fontCollection;
+        THROW_IF_FAILED(dwriteFactory->GetSystemFontCollection(&fontCollection));


nit: Would prefer to keep this static to only pull once rather than getting every time this is called #WontFix

Mm, but then we have to keep the whole font collection in memory, which I think I hate more than getting it every time. I'll possibly revisit this in #1374 but I don't think it'll be big enough of an issue to come up.

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

aballway · 2016-12-06T21:35:02Z

    RETURN_NULL_IF(!frame);
    THROW_NS_IF_FALSE(E_UNEXPECTED, [frame->_lines count] == 1);

+    if ([frame->_lines count] != 1) {


remove the throw above #Resolved

oops, I thought I did that...

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

aballway · 2016-12-06T21:38:26Z

+    ComPtr<IDWriteFontFace2> fontFace2;
+    RETURN_NULL_IF_FAILED(fontFace.As(&fontFace2));
+    ComPtr<IDWriteFontFace3> fontFace3;
+    RETURN_NULL_IF_FAILED(fontFace.As(&fontFace3));


IDWriteFontFace3 inherits from IDWriteFontFace2, so just use that one #Resolved

aballway · 2016-12-06T21:39:13Z

+        symbolicTraits |= kCTFontColorGlyphsTrait;
+    }
+
+    // TODO: The symbolic traits below are poorly documented/have no clear DWrite mapping


No commented out code #Resolved

aballway · 2016-12-06T21:39:48Z

+
+    DWRITE_INFORMATIONAL_STRING_ID informationalStringId;
+
+    if (CFEqual(nameKey, kCTFontCopyrightNameKey)) {


is this making enough of an impact to convert to dictionary-lookup? #WontFix

let's investigate that in #1374 lol

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

aballway · 2016-12-06T21:41:44Z

 static const wchar_t* TAG = L"_DWriteWrapper_CoreText";

-static DWRITE_TEXT_ALIGNMENT __CoreTextAlignmentToDwrite(CTTextAlignment alignment) {
+static DWRITE_TEXT_ALIGNMENT __CTAlignmentToDWrite(CTTextAlignment alignment) {


nit: should make these inline too :) #Resolved

aballway · 2016-12-06T21:42:44Z

+        RETURN_IF_FAILED(__DWriteTextFormatApplyParagraphStyle(textFormat, settings));
    }

-    return textFormat;


still not CopyTo? #ByDesign

aballway · 2016-12-06T21:43:32Z

+        RETURN_IF_FAILED(textLayout->SetTypography(typography.Get(), dwriteRange));
    }

-    return textLayout;


same as above #ByDesign

- Split DWriteWrapper, DWriteWrapper_CoreText among more files - DWriteFontBinaryDataLoader is now in its own .h + .mm along with BinaryDataStream - DWriteFontCollectionLoader is now in its own .h + .mm along with FileEnumerator - Renamed to DWriteFontBinaryDataCollectionLoader to get the name farther from the interface name (IDWriteFontCollectionLoader) - Implementation for _DWriteFontCreatePathForGlyph(), and the __CGPathGeometrySink class, which was only used in this function, are now in a separate .mm file - Implementation for DWriteFont_CoreText functions relating to CTFont, CTFontDescriptor are now in a separate .mm file - Refactored _userFontCollection, s_userFontPropertiesMap, systemFontPropertiesMap to be behind an abstraction layer, DWriteFontCollectionHelper, which wraps around either a user or system IDWriteFontCollection and provides font name -> other name/family name, properties mappings. - This is in its own .h + .mm file - As a result, many _DWrite functions now reduce cleanly to __GetSystemFontCollectionHelper()->Foo(), lock, __GetUserFontCollectionHelper()->Foo() - Refactored __CreateDWriteTextFormat(), __CreateDWriteTextLayout() - Removed export of _DWriteCreateTextFormat(), this is now done directly in DWriteWrapper_CoreText.mm - Now return HRESULT instead of ComPtr, now create through out-pointers, changed naming convention - As a result, __CreateDWriteTextFormat no longer always returns a nullptr if an error occurs - this should not impact anything, as this was an internal function, and return values were never checked for nullptr anyway - Use CFAttributedStringGetAttribute directly instead of copying to a dictionary first, this should save some time on redundant copies/copies of unused attributes - Parts of these functions were moved into separate inline functions for easier reading - Reversed order of i, j in _DWriteGetFrame(), such that i is on the outside and j on the inside - Replaced the inner two while loops, with a single for loop - Exported _GetDefaultUserLocaleName() - Broke parts of _DWriteCreateFontFaceWithFontDescriptor() into separate functions for easier readability (less nesting) - Moved CustomDWriteTextRenderer member functions bodies to inside the class body - _CFStringCreateUppercaseCopy now uses a nullptr CFLocale (faster, font names are not localized anyway) - Removed a redundant call to CFStringCreateUppercaseCopy() in the initialization of a font property map - Changed most remaining instances where CFAutorelease was used to use woc::unique_cf instead - Standardized naming convention of functions that translate a DWrite constant to CT/CG, or vice-versa - Changed members of most C++ classes to use the m_ naming convention - Standardized most file-private functions to be static - Standardized most instances of loops that call functions that return HRESULTs to call continue on failure - Changed CTTypesetterCreateLineWithOffset() to log and return null instead of throw - Added a bunch of SAL annotations - Added a string helper for copying a CFString to a vector, this was a pattern that occurred multiple times - Added comments in various places Fixes microsoft#1212

- Changed instances of RETURN_HR_IF(E_FOO, !ptr) to use RETURN_HR_IF_NULL instead - Changed instances where the ptr was an out pointer to return E_POINTER instead of E_INVALIDARG - Added #pragma once to .h files - Changed various places to use ComPtr.CopyTo()

… in DWriteFontBinaryDataCollectionLoader.mm - CollectionLoader now owns these completely, FileEnumerator keeps a reference to the one that created it, and delegates to it. - Updated DWriteFontBinaryDataCollectionLoader to only populate outErrors when it's ready to return - Updated DWriteFontCollectionHelper to be thread-safe. UserFontCollectionHelper now no longer requires a lock around it. - Moved some classes' RuntimeInitialize() body, where possible, to the class's ctor. - Made __WeightMapping an anonymous struct. - Changed an #ifdef in StringHelpers.h from __OBJC__ to the more permissive __clang__ - Reverted some instances of CopyTo() to Detach() to avoid an AddRef - Reverted SAL changes. - Updated function-local statics to use s_ naming convention. - Updated runtime class names to have actual names instead of just namespaces - Standardized usage of kCFAllocatorDefault vs kCFAllocatorSystemDefault - Standardized initialization/naming of BOOL exists - Misc CR feedback

- Moved part of DWriteTextFormat creation back into DWriteWrapper, so that the user font collection can also be used - Changed DWriteFontBinaryDataCollectionLoader to use just one (hashed) data structure instead of two. - This speeds up RemoveDatas() from O(n*m) to O(n) - Removed some unnecessary RuntimeClassInitialize()s - Removed some commented out code - Misc CR feedback

ms-jihua · 2016-12-07T21:53:37Z

CI builds both completed for 4th commit, skipping for 5th commit as it is very minor.

msftclas added the cla-not-required label Dec 2, 2016

msft-Jeyaram reviewed Dec 2, 2016

View reviewed changes

ms-jihua commented Dec 2, 2016

View reviewed changes

msft-Jeyaram reviewed Dec 2, 2016

View reviewed changes

bbowman reviewed Dec 3, 2016

View reviewed changes

aballway approved these changes Dec 5, 2016

View reviewed changes

bbowman reviewed Dec 5, 2016

View reviewed changes

msft-Jeyaram reviewed Dec 5, 2016

View reviewed changes

DHowett-MSFT reviewed Dec 5, 2016

View reviewed changes

DHowett-MSFT mentioned this pull request Dec 6, 2016

Implement CGPathApply #1507

Merged

DHowett-MSFT reviewed Dec 6, 2016

View reviewed changes

aballway reviewed Dec 6, 2016

View reviewed changes

ms-jihua added 4 commits December 7, 2016 11:43

ms-jihua force-pushed the ct_refactor4 branch from 81122b9 to 636512c Compare December 7, 2016 20:28

msft-Jeyaram approved these changes Dec 7, 2016

View reviewed changes

- Changed most usages of _DWriteFontProperties to be const

aaefacc

ms-jihua merged commit b06f7b3 into microsoft:develop Dec 7, 2016

aballway mentioned this pull request Dec 8, 2016

CoreText: AV in WoCCatalog text display sample when quickly scrolling around #1508

Closed


		DWRITE_INFORMATIONAL_STRING_ID informationalStringId;

		if (CFEqual(nameKey, kCTFontCopyrightNameKey)) {

Conversation

ms-jihua commented Dec 2, 2016 • edited by rajsesh Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msft-Jeyaram commented Dec 2, 2016

Uh oh!

msft-Jeyaram Dec 2, 2016 • edited by ms-jihua Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msft-Jeyaram Dec 2, 2016 • edited by ms-jihua Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msft-Jeyaram commented Dec 2, 2016

Uh oh!

msft-Jeyaram Dec 2, 2016 • edited by ms-jihua Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msft-Jeyaram Dec 2, 2016 • edited by ms-jihua Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ms-jihua Dec 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msft-Jeyaram Dec 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msft-Jeyaram Dec 2, 2016 • edited by ms-jihua Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msft-Jeyaram Dec 2, 2016 • edited by ms-jihua Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aballway Dec 6, 2016 • edited by ms-jihua Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msft-Jeyaram Dec 2, 2016 • edited by ms-jihua Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msft-Jeyaram Dec 2, 2016 • edited by ms-jihua Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aballway Dec 6, 2016 • edited by ms-jihua Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DHowett-MSFT Dec 6, 2016 • edited by ms-jihua Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msft-Jeyaram Dec 2, 2016 • edited by ms-jihua Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msft-Jeyaram Dec 2, 2016 • edited by ms-jihua Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msft-Jeyaram Dec 2, 2016 • edited by ms-jihua Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ms-jihua commented Dec 2, 2016 •

edited by rajsesh

Loading

msft-Jeyaram Dec 2, 2016 •

edited by ms-jihua

Loading

msft-Jeyaram Dec 2, 2016 •

edited by ms-jihua

Loading

msft-Jeyaram Dec 2, 2016 •

edited by ms-jihua

Loading

msft-Jeyaram Dec 2, 2016 •

edited by ms-jihua

Loading

ms-jihua Dec 2, 2016 •

edited

Loading

msft-Jeyaram Dec 2, 2016 •

edited

Loading

msft-Jeyaram Dec 2, 2016 •

edited by ms-jihua

Loading

msft-Jeyaram Dec 2, 2016 •

edited by ms-jihua

Loading

aballway Dec 6, 2016 •

edited by ms-jihua

Loading

msft-Jeyaram Dec 2, 2016 •

edited by ms-jihua

Loading

msft-Jeyaram Dec 2, 2016 •

edited by ms-jihua

Loading

aballway Dec 6, 2016 •

edited by ms-jihua

Loading

DHowett-MSFT Dec 6, 2016 •

edited by ms-jihua

Loading

msft-Jeyaram Dec 2, 2016 •

edited by ms-jihua

Loading

msft-Jeyaram Dec 2, 2016 •

edited by ms-jihua

Loading

msft-Jeyaram Dec 2, 2016 •

edited by ms-jihua

Loading

msft-Jeyaram commented Dec 2, 2016 •

edited by ms-jihua

Loading

bbowman Dec 3, 2016 •

edited by ms-jihua

Loading

bbowman Dec 3, 2016 •

edited by ms-jihua

Loading

bbowman Dec 3, 2016 •

edited by ms-jihua

Loading

bbowman Dec 5, 2016 •

edited

Loading

bbowman Dec 3, 2016 •

edited by ms-jihua

Loading