Bug 139721 - Unify the Mac and iOS implementations of FontPlatformData a bit
Summary: Unify the Mac and iOS implementations of FontPlatformData a bit
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-12-16 20:05 PST by Sam Weinig
Modified: 2014-12-18 14:36 PST (History)
2 users (show)

See Also:


Attachments
Patch (35.82 KB, patch)
2014-12-16 20:12 PST, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (38.46 KB, patch)
2014-12-17 14:03 PST, Sam Weinig
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2014-12-16 20:05:56 PST
Unify the Mac and iOS implementations of FontPlatformData a bit
Comment 1 Sam Weinig 2014-12-16 20:12:22 PST
Created attachment 243425 [details]
Patch
Comment 2 Darin Adler 2014-12-16 23:20:31 PST
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.
Comment 3 Sam Weinig 2014-12-17 14:03:11 PST
Created attachment 243458 [details]
Patch
Comment 4 Darin Adler 2014-12-18 09:37:38 PST
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.
Comment 5 Sam Weinig 2014-12-18 14:36:21 PST
Landed in r177527.