Bug 235449 – REGRESSION(r282320): [Cocoa] User-installed fonts don't work in the GPU Process (in WKWebView)

• Component: New Bugs
• Status: RESOLVED FIXED

[Myles C. Maxfield]

[Cocoa] User-installed fonts don't work in the GPU Process (in WKWebView)

[Myles C. Maxfield]

Created attachment 449673 [details]
WIP

[Myles C. Maxfield]

<rdar://problem/84958961>

[Myles C. Maxfield]

Comment on attachment 449673 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=449673&action=review

> Source/WebCore/PAL/pal/spi/cf/CoreTextSPI.h:99
> +typedef CF_OPTIONS(uint32_t, CTFontDescriptorOptions) {
> +};

Apparently there has to be something inside the {}s :(

[Myles C. Maxfield]

Created attachment 449687 [details]
Patch

[Myles C. Maxfield]

Created attachment 449689 [details]
Patch

[Darin Adler]

Comment on attachment 449689 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=449689&action=review

> Source/WebCore/PAL/pal/spi/cf/CoreTextSPI.h:100
> +typedef CF_OPTIONS(uint32_t, CTFontDescriptorOptions) {
> +    kCTFontDescriptorOptionThisIsNotARealOption = 0xFFFFFFFF
> +};

Or we could just typedef it to uint32_t.

> Source/WebKit/Shared/Cocoa/WebCoreArgumentCodersCocoa.mm:468
>          encoder << String(string);

Seems a shame we have to convert CFString to String just to encode it on the wire. A repeated pattern.

[Myles C. Maxfield]

Comment on attachment 449689 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=449689&action=review

>> Source/WebCore/PAL/pal/spi/cf/CoreTextSPI.h:100
>> +};
> 
> Or we could just typedef it to uint32_t.

Do you think the typedef is preferable? I think I actually prefer the CF_OPTIONS thing, just because it more closely matches the original source.

I don't want to delay this bug fix any longer, so I'll land this as is and if you prefer the typedef I'll make a follow-up patch.

>> Source/WebKit/Shared/Cocoa/WebCoreArgumentCodersCocoa.mm:468
>>          encoder << String(string);
> 
> Seems a shame we have to convert CFString to String just to encode it on the wire. A repeated pattern.

I believe we send CFStrings in other places ... let me see if I can fix this...

[Darin Adler]

Comment on attachment 449689 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=449689&action=review

>>> Source/WebCore/PAL/pal/spi/cf/CoreTextSPI.h:100
>>> +};
>> 
>> Or we could just typedef it to uint32_t.
> 
> Do you think the typedef is preferable? I think I actually prefer the CF_OPTIONS thing, just because it more closely matches the original source.
> 
> I don't want to delay this bug fix any longer, so I'll land this as is and if you prefer the typedef I'll make a follow-up patch.

This is fine. I didn’t mean to cause delay!

[Myles C. Maxfield]

Comment on attachment 449689 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=449689&action=review

>>> Source/WebKit/Shared/Cocoa/WebCoreArgumentCodersCocoa.mm:468
>>>          encoder << String(string);
>> 
>> Seems a shame we have to convert CFString to String just to encode it on the wire. A repeated pattern.
> 
> I believe we send CFStrings in other places ... let me see if I can fix this...

Oh, yeah, of course - ArgumentCodersCF.h!

[Darin Adler]

Comment on attachment 449689 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=449689&action=review

>>>> Source/WebKit/Shared/Cocoa/WebCoreArgumentCodersCocoa.mm:468
>>>>          encoder << String(string);
>>> 
>>> Seems a shame we have to convert CFString to String just to encode it on the wire. A repeated pattern.
>> 
>> I believe we send CFStrings in other places ... let me see if I can fix this...
> 
> Oh, yeah, of course - ArgumentCodersCF.h!

Should find the others who are calling String() just to encode a CFStringRef. I bet this is not the only place.

[Myles C. Maxfield]

Created attachment 449891 [details]
Patch for committing

[Myles C. Maxfield]

Created attachment 449892 [details]
Patch for committing

[Myles C. Maxfield]

Created attachment 449893 [details]
Patch for committing

[Darin Adler]

Comment on attachment 449893 [details]
Patch for committing

View in context: https://bugs.webkit.org/attachment.cgi?id=449893&action=review

Fine to land as-is, but I see one further refinement.

> Source/WebKit/Shared/Cocoa/WebCoreArgumentCodersCocoa.mm:486
>          auto currentPostScriptName = adoptCF(static_cast<CFStringRef>(CTFontDescriptorCopyAttribute(fontDescriptor, kCTFontNameAttribute)));

Given we only call CFEqual on this, we don’t need the static_cast to CFStringRef. I suggest just taking it out.

[Myles C. Maxfield]

Comment on attachment 449893 [details]
Patch for committing

View in context: https://bugs.webkit.org/attachment.cgi?id=449893&action=review

>> Source/WebKit/Shared/Cocoa/WebCoreArgumentCodersCocoa.mm:486
>>          auto currentPostScriptName = adoptCF(static_cast<CFStringRef>(CTFontDescriptorCopyAttribute(fontDescriptor, kCTFontNameAttribute)));
> 
> Given we only call CFEqual on this, we don’t need the static_cast to CFStringRef. I suggest just taking it out.

🆒

I think I agree with you. This is more readable now.

[Myles C. Maxfield]

Committed r288543 (246375@trunk): <https://commits.webkit.org/246375@trunk>