[Managed DWrite] Migrate FontCollectionLoader to managed

[ThomasGoulet73]

Contributes to #5305

Description

Migrate DWrite FontCollectionLoader to managed.

Customer Impact

It might be faster and should allow better support of trimming and the support of NativeAOT once everything is migrated to C#.

Regression

No.

Testing

Local build + CI + I tested a few apps that uses custom fonts to test this code path.

Risk

Low. For the most part, it is a copy of the C++ code manually rewritten to C# with as little changes as possible.

[ThomasGoulet73]

I rebased on main now that #6171 is merged.

[ThomasGoulet73]

I just pushed a rebase and reran my manual tests, everything seems to be working good.

@dotnet/wpf-developers: Could this be included in the next test pass ?

[ThomasGoulet73]

I rebased to fix the conflicts.

[dipeshmsft]

@ThomasGoulet73, sorry but it will take us time to get to this PR. We have started with the CTP's and will try to get to this PR as soon as possible.

[JeremyKuhne]

A few comments. The default constructor comment is the one I'm most concerned about.

[JeremyKuhne]

This should be a descriptive comment. Why should we not be creating this via the default constructor? Should this be private?

If we don't know of a case where the default constructor is needed, then this should probably be an InvalidOperationException.

[ThomasGoulet73]

I'm not sure what's the reasoning behind this, the original code was like this so I added it. Maybe C++/CLI doesn't/didn't support private constructor or maybe it's because it's a COM object and it was to ensure that it wasn't created through COM using the default constructor ?

[JeremyKuhne]

Suggested change

	public int CreateEnumeratorFromKey(IntPtr factory, [In] void* collectionKey, [In, MarshalAs(UnmanagedType.U4)] uint collectionKeySize, IntPtr* fontFileEnumerator)
	public int CreateEnumeratorFromKey(void* factory, void* collectionKey, uint collectionKeySize, void** fontFileEnumerator)

The attributes aren't needed, and as this is already using pointers it is probably better to just use void*. IntPtr doesn't make things safe.

[JeremyKuhne]

In general, it is much better to create a ReadOnlySpan<char>() over native input after doing basic validation. In this particular case, you're replicating pretty much exactly the same logic so I wouldn't necessarily do it.

new string((char*) terminates at the first null. If you create from a span, there is no walk of characters. If you know that embedded nulls are not a thing one will see, then it is a little more optimal to avoid the walking.

[ThomasGoulet73]

Thanks for the review @JeremyKuhne. My migration process is to stay as close as possible to the C++/CLI code (If the code is not wrong) and, if it's alright with you, I'd prefer to do that to avoid potential mistakes. Your suggestions are most likely correct and cleaner but I'm trying to avoid potential regression and to keep a clean Git history (Have the first commit of migrated files be as close as possible to the original and further cleanup later).

So if you agree, I don't think your comments are blocking and it can be done later on (I already planned on doing some cleanup in the future once when most or all the code is migrated). I'm open to doing your suggestions in this PR if that's what you prefer.

[lindexi]

I agree with @ThomasGoulet73 . This PR has been around for a long time and it's even more important to get it merged as soon as possible.

[JeremyKuhne]

Sorry, missed the fact that the assert was in the header, not the .cpp file. I have no concerns transliterating, just want to point out the weird so it is understood.