Bug 157024 – Make FontPlatformData conceptually immutable

• Component: New Bugs
• Status: RESOLVED FIXED

[Myles C. Maxfield]

Make FontPlatformData immutable

[Myles C. Maxfield]

Created attachment 277336 [details]
Patch

[Myles C. Maxfield]

Created attachment 277345 [details]
Patch

[Darin Adler]

Comment on attachment 277345 [details]
Patch

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

> Source/WebCore/platform/graphics/Font.cpp:256
> +        FontPlatformData verticalRightPlatformData = FontPlatformData::cloneWithOrientation(m_platformData, Horizontal);

Can we use auto here instead of naming the type twice? Maybe we don’t need a local variable at all?

> Source/WebCore/platform/graphics/Font.cpp:323
> +        FontPlatformData nonSyntheticItalicFontPlatformData = FontPlatformData::cloneWithSyntheticOblique(m_platformData, false);

Would say the same thing except for the #if that might change things, I guess.

> Source/WebCore/platform/graphics/FontPlatformData.cpp:95
>  const FontPlatformData& FontPlatformData::operator=(const FontPlatformData& other)

Too bad we can’t just let the compiler generate this. Do any of the platformDataAssign functions do something other than what the compiler default would do? Seems like the only issue may be the lack of a smart pointer for m_scaledFont for Cairo!

Same thought about the copy constructor and platformDataInit.

> Source/WebCore/platform/graphics/FontPlatformData.h:233
> +    // vvvv These values are common to all ports vvvv

What’s with the "vvvv" thing? Not very attractive. Can we find another way to make this comment clear?

> Source/WebCore/platform/graphics/freetype/FontPlatformData.h:73
> +    static FontPlatformData cloneWithOrientation(const FontPlatformData&, FontOrientation);
> +    static FontPlatformData cloneWithSyntheticOblique(const FontPlatformData&, bool);
> +    static FontPlatformData cloneWithSize(const FontPlatformData&, float);

This seems strange. Why an entirely separate header for this one version of FontPlatformData, but not for any of the others?

[Myles C. Maxfield]

Comment on attachment 277345 [details]
Patch

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

>> Source/WebCore/platform/graphics/FontPlatformData.cpp:95
>>  const FontPlatformData& FontPlatformData::operator=(const FontPlatformData& other)
> 
> Too bad we can’t just let the compiler generate this. Do any of the platformDataAssign functions do something other than what the compiler default would do? Seems like the only issue may be the lack of a smart pointer for m_scaledFont for Cairo!
> 
> Same thought about the copy constructor and platformDataInit.

I intend to migrate the Cairo pointer to a smart pointer class in the future. That way, we can delete many of these boilerplate functions.

>> Source/WebCore/platform/graphics/freetype/FontPlatformData.h:73
>> +    static FontPlatformData cloneWithSize(const FontPlatformData&, float);
> 
> This seems strange. Why an entirely separate header for this one version of FontPlatformData, but not for any of the others?

I don't know. :(

[Myles C. Maxfield]

Committed r200094: <http://trac.webkit.org/changeset/200094>

[Gyuyoung Kim]

It looks GTK and EFL layout test have been broken since this commit.

- GTK : https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20%28Tests%29/builds/15352
- EFL : https://build.webkit.org/builders/EFL%20Linux%2064-bit%20Release%20WK2/builds/27735