Bug 139721 – Unify the Mac and iOS implementations of FontPlatformData a bit

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

[Sam Weinig]

Unify the Mac and iOS implementations of FontPlatformData a bit

[Sam Weinig]

Created attachment 243425 [details]
Patch

[Darin Adler]

Comment on attachment 243425 [details]
Patch

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

> Source/WebCore/platform/graphics/FontPlatformData.cpp:50
> +    , m_ctFont(nullptr)

Smart pointer, so doesn’t need explicit initialization.

> Source/WebCore/platform/graphics/FontPlatformData.cpp:53
> +    , m_cgFont(nullptr)

Smart pointer, so doesn’t need explicit initialization.

> Source/WebCore/platform/graphics/FontPlatformData.cpp:78
> +    , m_font(nullptr)

I suggest we use the newfangled syntax in the header:

    CTFontRef m_font { nullptr };

Then avoid lines of code like the one above.

> Source/WebCore/platform/graphics/FontPlatformData.cpp:79
> +    , m_ctFont(nullptr)

Smart pointer, so doesn’t need explicit initialization.

> Source/WebCore/platform/graphics/FontPlatformData.cpp:82
> +    , m_cgFont(nullptr)

Smart pointer, so doesn’t need explicit initialization.

> Source/WebCore/platform/graphics/FontPlatformData.cpp:107
> +    , m_font(nullptr)

I suggest we use the newfangled syntax in the header:

    CTFontRef m_font { nullptr };

Then avoid lines of code like the one above.

> Source/WebCore/platform/graphics/FontPlatformData.cpp:108
> +    , m_ctFont(nullptr)

Smart pointer, so doesn’t need explicit initialization.

> Source/WebCore/platform/graphics/FontPlatformData.cpp:111
> +    , m_cgFont(nullptr)

Smart pointer, so doesn’t need explicit initialization.

> Source/WebCore/platform/graphics/FontPlatformData.cpp:113
> +    , m_scaledFont(nullptr)

I suggest we use the newfangled syntax in the header:

    cairo_scaled_font_t* m_scaledFont { nullptr };

Then avoid lines of code like the one above.

> Source/WebCore/platform/graphics/FontPlatformData.h:46
>  #if PLATFORM(IOS)

I like it better if we don’t nest #if like this.

> Source/WebCore/platform/graphics/FontPlatformData.h:49
> +#if PLATFORM(MAC)

I like it better if we don’t nest #if like this.

Can we use USE(APPKIT) for this?

> Source/WebCore/platform/graphics/FontPlatformData.h:75
>  

Extra blank line here, I think.

> Source/WebCore/platform/graphics/FontPlatformData.h:92
> +#if PLATFORM(MAC)

Why did you switch from USE(APPKIT) to PLATFORM(MAC)?

> Source/WebCore/platform/graphics/FontPlatformData.h:120
> +#if PLATFORM(MAC)

USE(APPKIT)?

> Source/WebCore/platform/graphics/cocoa/FontPlatformDataCocoa.mm:49
> +#if PLATFORM(IOS)
>      , m_isEmoji(false)
> +#endif

Could use in-class initialization for this:

    bool m_isEmoji { false };

Unless there is a good reason not to.

[Sam Weinig]

Created attachment 243458 [details]
Patch

[Darin Adler]

Comment on attachment 243458 [details]
Patch

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

> Source/WebCore/platform/graphics/FontPlatformData.h:118
> +    // 

Oops, probably want to go and remove this or add the comment that was intended to be here.

> Source/WebCore/platform/graphics/FontPlatformData.h:139
> +#if PLATFORM(COCOA)
>  #endif

I suggest removing this.

[Sam Weinig]

Landed in r177527.