Bug 157024

Summary: Make FontPlatformData conceptually immutable
Product: WebKit Reporter: Myles C. Maxfield <mmaxfield>
Component: New BugsAssignee: Myles C. Maxfield <mmaxfield>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, gyuyoung.kim
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch darin: review+

Description Myles C. Maxfield 2016-04-25 21:58:07 PDT
Make FontPlatformData immutable
Comment 1 Myles C. Maxfield 2016-04-25 22:00:26 PDT
Created attachment 277336 [details]
Patch
Comment 2 Myles C. Maxfield 2016-04-26 00:04:46 PDT
Created attachment 277345 [details]
Patch
Comment 3 Darin Adler 2016-04-26 08:52:01 PDT
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?
Comment 4 Myles C. Maxfield 2016-04-26 10:18:15 PDT
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. :(
Comment 5 Myles C. Maxfield 2016-04-26 10:20:04 PDT
Committed r200094: <http://trac.webkit.org/changeset/200094>
Comment 6 Gyuyoung Kim 2016-04-26 21:43:38 PDT
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