Bug 160895 – [Cocoa] Migrate off of deprecated CoreGraphics API CGContextSelectFont() and CGContextShowTextAtPoint()

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

[Cocoa] Migrate off of deprecated CoreGraphics API CGContextSelectFont() and CGContextShowTextAtPoint()

[Myles C. Maxfield]

Created attachment 286164 [details]
Patch

[Sam Weinig]

Comment on attachment 286164 [details]
Patch

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

> Source/WebCore/platform/graphics/ca/PlatformCALayer.cpp:137
>  // This function is needed to work around a bug in Windows CG <rdar://problem/22703470>
>  void PlatformCALayer::drawTextAtPoint(CGContextRef context, CGFloat x, CGFloat y, CGSize scale, CGFloat fontSize, const char* text, size_t length) const
>  {
> -#pragma clang diagnostic push
> -#pragma clang diagnostic ignored "-Wdeprecated-declarations"
> -    CGContextSetTextMatrix(context, CGAffineTransformMakeScale(scale.width, scale.height));
> -    CGContextSelectFont(context, "Helvetica", fontSize, kCGEncodingMacRoman);
> -    CGContextShowTextAtPoint(context, x, y, text, length);
> -#pragma clang diagnostic pop
> +    CGAffineTransform matrix = CGAffineTransformMakeScale(scale.width, scale.height);
> +    RetainPtr<CTFontRef> font = adoptCF(CTFontCreateWithName(CFSTR("Helvetica"), fontSize, &matrix));
> +    CFTypeRef keys[] = { kCTFontAttributeName };
> +    CFTypeRef values[] = { font.get() };
> +    RetainPtr<CFDictionaryRef> attributes = adoptCF(CFDictionaryCreate(kCFAllocatorDefault, keys, values, WTF_ARRAY_LENGTH(keys), &kCFTypeDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks));
> +    RetainPtr<CFStringRef> string = adoptCF(CFStringCreateWithBytesNoCopy(kCFAllocatorDefault, reinterpret_cast<const UInt8*>(text), length, kCFStringEncodingUTF8, false, kCFAllocatorNull));
> +    RetainPtr<CFAttributedStringRef> attributedString = adoptCF(CFAttributedStringCreate(kCFAllocatorDefault, string.get(), attributes.get()));
> +    RetainPtr<CTLineRef> line = adoptCF(CTLineCreateWithAttributedString(attributedString.get()));
> +    CGPoint textPosition = CGContextGetTextPosition(context);
> +    CGContextSetTextPosition(context, x, y);
> +    CTLineDraw(line.get(), context);

I don't think CoreText exists on Windows, so this probably won't compile.

[Myles C. Maxfield]

Comment on attachment 286164 [details]
Patch

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

>> Source/WebCore/platform/graphics/ca/PlatformCALayer.cpp:137
>> +    CTLineDraw(line.get(), context);
> 
> I don't think CoreText exists on Windows, so this probably won't compile.

Hrm. I see CoreText.dll inside the Apple Application Support folder (next to CoreGraphics.dll, etc.), and CoreText.lib inside WebKitSupportLibrary. It looks like the headers inside WebKitSupportLibrary are incomplete, but running dumpbin.exe on CoreText.lib makes it seem that the functions do exist.

[Myles C. Maxfield]

Created attachment 286256 [details]
WIP

[Myles C. Maxfield]

Created attachment 286291 [details]
Patch

[Myles C. Maxfield]

Created attachment 286294 [details]
Patch

[Myles C. Maxfield]

Created attachment 286302 [details]
Patch

[Myles C. Maxfield]

Created attachment 286321 [details]
Patch

[Dean Jackson]

Comment on attachment 286321 [details]
Patch

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

> Source/WebCore/page/cocoa/ResourceUsageOverlayCocoa.mm:260
>  #if PLATFORM(IOS)
> -    CGContextSelectFont(context, "Courier", 10, kCGEncodingMacRoman);
> +    CFStringRef fontName = CFSTR("Courier");
> +    CGFloat fontSize = 10;

Why Courier on iOS? And why Menlo elsewhere? Can't we use SF on iOS and where available on macOS?

> Source/WebCore/platform/ios/LegacyTileCache.mm:598
> +        RetainPtr<CTFontRef> font = adoptCF(CTFontCreateWithName(CFSTR("Helvetica"), 25, &matrix));

We can probably use SF or Menlo here too.

> Source/WebCore/platform/spi/win/CoreTextSPIWin.h:2
> +/*
> + * Copyright (C) 2016 Apple Inc. All rights reserved.

Why spi/win/CoreTextSPIWin.h?

Can we just merge this with an existing CoreTextSPI.h?

[Myles C. Maxfield]

Comment on attachment 286321 [details]
Patch

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

>> Source/WebCore/page/cocoa/ResourceUsageOverlayCocoa.mm:260
>> +    CGFloat fontSize = 10;
> 
> Why Courier on iOS? And why Menlo elsewhere? Can't we use SF on iOS and where available on macOS?

That choice was in the existing code; this patch simply migrates the existing code to non-deprecated calls.

>> Source/WebCore/platform/ios/LegacyTileCache.mm:598
>> +        RetainPtr<CTFontRef> font = adoptCF(CTFontCreateWithName(CFSTR("Helvetica"), 25, &matrix));
> 
> We can probably use SF or Menlo here too.

Ditto.

>> Source/WebCore/platform/spi/win/CoreTextSPIWin.h:2
>> + * Copyright (C) 2016 Apple Inc. All rights reserved.
> 
> Why spi/win/CoreTextSPIWin.h?
> 
> Can we just merge this with an existing CoreTextSPI.h?

CoreTextSPI.h is inside cocoa/

I could move it out, but there would be a ton of new things needing to be forward-declared in order to just get CoreTextSPI.h to compile on Windows. I think that keeping this simple is a good idea, given the very different requirements of Windows and macOS/iOS.

[Myles C. Maxfield]

Comment on attachment 286321 [details]
Patch

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

>>> Source/WebCore/page/cocoa/ResourceUsageOverlayCocoa.mm:260
>>> +    CGFloat fontSize = 10;
>> 
>> Why Courier on iOS? And why Menlo elsewhere? Can't we use SF on iOS and where available on macOS?
> 
> That choice was in the existing code; this patch simply migrates the existing code to non-deprecated calls.

What I mean to say is: switching out the fonts is a good idea, but not for this patch. This patch has no behavior changes.

[Myles C. Maxfield]

Comment on attachment 286321 [details]
Patch

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

> Source/WebCore/page/cocoa/ResourceUsageOverlayCocoa.mm:266
> +    CFTypeRef keys[] = { kCTFontAttributeName };

kCTForegroundColorFromContextAttributeName

> Source/WebCore/platform/graphics/ca/PlatformCALayer.cpp:133
> +    CFTypeRef keys[] = { kCTFontAttributeName };

kCTForegroundColorFromContextAttributeName

> Source/WebCore/platform/ios/LegacyTileCache.mm:599
> +        CFTypeRef keys[] = { kCTFontAttributeName };

kCTForegroundColorFromContextAttributeName

[Myles C. Maxfield]

Created attachment 286363 [details]
Patch for committing

[WebKit Commit Bot]

Comment on attachment 286363 [details]
Patch for committing

Clearing flags on attachment: 286363

Committed r204592: <http://trac.webkit.org/changeset/204592>

[Csaba Osztrogonác]

(In reply to comment #14)
> Comment on attachment 286363 [details]
> Patch for committing
> 
> Clearing flags on attachment: 286363
> 
> Committed r204592: <http://trac.webkit.org/changeset/204592>

It broke the WinCairo build, see build.webkit.org for details.

[Darin Adler]

Comment on attachment 286363 [details]
Patch for committing

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

Some comments post-landing for future patches like this one.

> Source/WebCore/page/cocoa/ResourceUsageOverlayCocoa.mm:257
> +    CGAffineTransform matrix = CGAffineTransformMakeScale(1, -1);

Might be nicer to use auto for a line like this.

> Source/WebCore/page/cocoa/ResourceUsageOverlayCocoa.mm:265
> +    RetainPtr<CTFontRef> font = adoptCF(CTFontCreateWithName(fontName, fontSize, &matrix));

In future code, I suggest auto instead of writing out the type for the result of adoptCF calls.

> Source/WebCore/page/cocoa/ResourceUsageOverlayCocoa.mm:268
> +    RetainPtr<CFDictionaryRef> attributes = adoptCF(CFDictionaryCreate(kCFAllocatorDefault, keys, values, WTF_ARRAY_LENGTH(keys), &kCFTypeDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks));

And this.

> Source/WebCore/page/cocoa/ResourceUsageOverlayCocoa.mm:272
> +    RetainPtr<CFStringRef> string = adoptCF(CFStringCreateWithBytesNoCopy(kCFAllocatorDefault, reinterpret_cast<const UInt8*>(cstr.data()), cstr.length(), kCFStringEncodingASCII, false, kCFAllocatorNull));
> +    RetainPtr<CFAttributedStringRef> attributedString = adoptCF(CFAttributedStringCreate(kCFAllocatorDefault, string.get(), attributes.get()));
> +    RetainPtr<CTLineRef> line = adoptCF(CTLineCreateWithAttributedString(attributedString.get()));

And these.

> Source/WebCore/platform/graphics/ca/PlatformCALayer.cpp:36
> +#include <CoreFoundation/CoreFoundation.h>
> +#include <CoreText/CoreText.h>
> +
>  #include "GraphicsContextCG.h"
>  #include "LayerPool.h"
>  #include "PlatformCALayerClient.h"
>  #include "TextStream.h"
>  #include <wtf/StringExtras.h>

WebKit coding style has these sorted in a single paragraph, not in two paragraphs like this.

> Source/WebCore/platform/graphics/ca/PlatformCALayer.cpp:132
> +    CGAffineTransform matrix = CGAffineTransformMakeScale(scale.width, scale.height);
> +    RetainPtr<CTFontRef> font = adoptCF(CTFontCreateWithName(CFSTR("Helvetica"), fontSize, &matrix));

auto

> Source/WebCore/platform/graphics/ca/PlatformCALayer.cpp:138
> +    RetainPtr<CFDictionaryRef> attributes = adoptCF(CFDictionaryCreate(kCFAllocatorDefault, keys, values, WTF_ARRAY_LENGTH(keys), &kCFTypeDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks));
> +    RetainPtr<CFStringRef> string = adoptCF(CFStringCreateWithBytesNoCopy(kCFAllocatorDefault, reinterpret_cast<const UInt8*>(text), length, kCFStringEncodingUTF8, false, kCFAllocatorNull));
> +    RetainPtr<CFAttributedStringRef> attributedString = adoptCF(CFAttributedStringCreate(kCFAllocatorDefault, string.get(), attributes.get()));
> +    RetainPtr<CTLineRef> line = adoptCF(CTLineCreateWithAttributedString(attributedString.get()));

auto

> Source/WebCore/platform/ios/LegacyTileCache.mm:35
> +#include <CoreText/CoreText.h>
> +
>  #include "FontAntialiasingStateSaver.h"
>  #include "LegacyTileGrid.h"
>  #include "LegacyTileGridTile.h"

Sorted in a single paragraph.

> Source/WebCore/platform/ios/LegacyTileCache.mm:598
> +        CGAffineTransform matrix = CGAffineTransformMakeScale(1, -1);
> +        RetainPtr<CTFontRef> font = adoptCF(CTFontCreateWithName(CFSTR("Helvetica"), 25, &matrix));

auto

> Source/WebCore/platform/ios/LegacyTileCache.mm:604
> +        RetainPtr<CFDictionaryRef> attributes = adoptCF(CFDictionaryCreate(kCFAllocatorDefault, keys, values, WTF_ARRAY_LENGTH(keys), &kCFTypeDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks));
> +        RetainPtr<CFStringRef> string = adoptCF(CFStringCreateWithBytesNoCopy(kCFAllocatorDefault, reinterpret_cast<const UInt8*>(text), strlen(text), kCFStringEncodingUTF8, false, kCFAllocatorNull));
> +        RetainPtr<CFAttributedStringRef> attributedString = adoptCF(CFAttributedStringCreate(kCFAllocatorDefault, string.get(), attributes.get()));
> +        RetainPtr<CTLineRef> line = adoptCF(CTLineCreateWithAttributedString(attributedString.get()));

auto

> Source/WebCore/platform/spi/win/CoreTextSPIWin.h:35
> +typedef const struct __CTFont* CTFontRef;
> +typedef const struct __CTLine* CTLineRef;

Are these really needed even if we include the headers above?

[Myles C. Maxfield]

Comment on attachment 286363 [details]
Patch for committing

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

>> Source/WebCore/platform/spi/win/CoreTextSPIWin.h:35
>> +typedef const struct __CTLine* CTLineRef;
> 
> Are these really needed even if we include the headers above?

Yes, because the Windows CoreText headers are incomplete. However, all (many) of the symbols do exist in the .dll, and I verified by hand that they actually do work.

[Myles C. Maxfield]

Committed r204658: <http://trac.webkit.org/changeset/204658>