Bug 157464 – [OS X] Migrate our Font classes entirely off of NSFont

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

[Myles C. Maxfield]

[OS X] Migrate our Font classes entirely off of NSFont

[Myles C. Maxfield]

Created attachment 278367 [details]
Patch

[Darin Adler]

Comment on attachment 278367 [details]
Patch

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

How safe is this switch? Is there any risk of behavior change?

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:1019
> +        NSDictionary *dict = [NSDictionary dictionaryWithObjectsAndKeys:
> +            static_cast<NSString *>(adoptCF(CTFontCopyPostScriptName(font)).get()) , NSAccessibilityFontNameKey,
> +            static_cast<NSString *>(adoptCF(CTFontCopyFamilyName(font)).get())     , NSAccessibilityFontFamilyKey,
> +            static_cast<NSString *>(adoptCF(CTFontCopyDisplayName(font)).get())    , NSAccessibilityVisibleNameKey,
> +            [NSNumber numberWithFloat:CTFontGetSize(font)]                         , NSAccessibilityFontSizeKey,
> +            nil];

WebKit coding style specifically asks us to avoid this style, where we line things up in columns.

> Source/WebCore/bindings/objc/DOM.mm:728
> -    RenderObject* renderer = core(self)->renderer();
> -    if (!renderer)
> -        return nil;
> -    return renderer->style().fontCascade().primaryFont().getCTFont();
> +    if (RenderObject* renderer = core(self)->renderer())
> +        return renderer->style().fontCascade().primaryFont().getCTFont();
> +    return nil;

I prefer early return to nested code like this, except where there are multiple cases.

Also, I suggest we use auto instead of RenderObject* for the type. We may actually get more efficient code because I am pretty sure Element::renderer returns something more specific than RenderObject.

> Source/WebCore/editing/cocoa/HTMLConverter.mm:1041
> +    if (auto renderer = element.renderer())
> +        return (PlatformFont *)renderer->style().fontCascade().primaryFont().getCTFont();
> +    return nil;

Same comment about early return.

Why is the explicit cast to PlatformFont * needed?

> Source/WebKit/mac/WebCoreSupport/PopupMenuMac.mm:175
> +        RetainPtr<CTFontRef> defaultFont = adoptCF(CTFontCreateUIFontForLanguage(kCTFontSystemFontType, CTFontGetSize(font), nil));

I suggest using auto here instead of explicit RetainPtr<CTFontRef>. The type would be implicit based on the right side expression.

> Source/WebKit2/WebProcess/WebCoreSupport/mac/WebPopupMenuMac.mm:46
> +    RetainPtr<CTFontDescriptorRef> fontDescriptor = adoptCF(CTFontCopyFontDescriptor(font));

Ditto.

> Source/WebKit2/WebProcess/WebCoreSupport/mac/WebPopupMenuMac.mm:50
> +    RetainPtr<CFDictionaryRef> attributes = adoptCF(CTFontDescriptorCopyAttributes(fontDescriptor.get()));

Ditto.

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:370
> +        if (CTFontRef ctFont = font ? font->getCTFont() : nullptr) {
> +            fontName = adoptCF(CTFontCopyPostScriptName(ctFont)).get();
> +            fontSize = CTFontGetSize(ctFont);
>          }

How about nesting a second if rather than using ?: here?

[Myles C. Maxfield]

Created attachment 278371 [details]
Patch

[Myles C. Maxfield]

(In reply to comment #2)
> Comment on attachment 278367 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=278367&action=review
> 
> How safe is this switch? Is there any risk of behavior change?

Almost no risk. There are two slight changes, but they shouldn't have any effect.

[Myles C. Maxfield]

Created attachment 278373 [details]
Patch for committing

[WebKit Commit Bot]

Comment on attachment 278373 [details]
Patch for committing

Rejecting attachment 278373 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 278373, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/1289552

[Myles C. Maxfield]

Created attachment 278374 [details]
Patch for committing

[Darin Adler]

Comment on attachment 278374 [details]
Patch for committing

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

More ideas for refinement (after landing, presumably).

> Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:548
> +    m_treatAsFixedPitch = (CTFontGetSymbolicTraits(ctFont) & kCTFontMonoSpaceTrait) || CGFontIsFixedPitch(m_platformData.cgFont()) || (fullName && (CFStringCompare(fullName.get(), CFSTR("Osaka-Mono"), kCFCompareCaseInsensitive) == kCFCompareEqualTo || CFStringCompare(fullName.get(), CFSTR("MS-PGothic"), kCFCompareCaseInsensitive) == kCFCompareEqualTo || CFStringCompare(fullName.get(), CFSTR("MonotypeCorsiva"), kCFCompareCaseInsensitive) == kCFCompareEqualTo));

These calls to CFStringCompare really call for a helper function to make this expression readable.

> Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:598
> +        CGFontRenderingStyle renderingStyle = kCGFontRenderingStyleAntialiasing | kCGFontRenderingStyleSubpixelPositioning | kCGFontRenderingStyleSubpixelQuantization | kCGFontAntialiasingStyleUnfiltered;

Probably could have named this just style instead of renderingStyle.

> Source/WebKit/mac/WebCoreSupport/PopupMenuMac.mm:87
> +                font = style.font().weight() < FontWeightBold ? adoptCF(CTFontCreateUIFontForLanguage(kCTFontUIFontSystem, size, nullptr)) : adoptCF(CTFontCreateUIFontForLanguage(kCTFontUIFontEmphasizedSystem, size, nullptr));

Seems like we could use the ? : just for the kCTFontUIFont constants instead of repeating the whole expression twice.

> Source/WebKit/mac/WebView/WebHTMLView.mm:5650
> +        auto aName = [a fontName];
> +        if (!fontName || !aName || !CFEqual(fontName.get(), static_cast<CFStringRef>([a fontName])))

Forgot to use aName here inside the static_cast.

> Source/WebKit2/WebProcess/WebCoreSupport/mac/WebPopupMenuMac.mm:54
> +    data.fontInfo.fontAttributeDictionary = attributes;

Surprised you don’t need to call get() here.

[WebKit Commit Bot]

Comment on attachment 278374 [details]
Patch for committing

Rejecting attachment 278374 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
ntBase.dia -c /Volumes/Data/EWS/WebKit/Source/WebCore/html/HTMLFrameElementBase.cpp -o /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/HTMLFrameElementBase.o

** BUILD FAILED **


The following build commands failed:
	CompileC /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/HTMLConverter.o editing/cocoa/HTMLConverter.mm normal x86_64 objective-c++ com.apple.compilers.llvm.clang.1_0.compiler
(1 failure)

Full output: http://webkit-queues.webkit.org/results/1289610

[Myles C. Maxfield]

Created attachment 278377 [details]
Patch for committing

[WebKit Commit Bot]

Comment on attachment 278377 [details]
Patch for committing

Clearing flags on attachment: 278377

Committed r200563: <http://trac.webkit.org/changeset/200563>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.