Bug 136766 – Introduce FONT_DATA_TYPE_CASTS, and use it

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Gyuyoung Kim]

SSIA

[Gyuyoung Kim]

Created attachment 237998 [details]
Patch

[Myles C. Maxfield]

unofficial r=me

[Gyuyoung Kim]

Created attachment 238004 [details]
Patch

[Darin Adler]

Comment on attachment 238004 [details]
Patch

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

> Source/WebCore/platform/graphics/FontGlyphs.cpp:74
> +        fontCache().releaseFontData(toSimpleFontData(m_realizedFontData[i]));

I don’t understand why the .get() isn’t needed any more.

> Source/WebCore/platform/graphics/FontGlyphs.cpp:82
> -        m_pitch = static_cast<const SimpleFontData*>(fontData)->pitch();
> +        m_pitch = toSimpleFontData(fontData)->pitch();

Should be toSimpleFontData(*fontData).pitch()

> Source/WebCore/platform/graphics/FontGlyphs.cpp:84
> -        const SegmentedFontData* segmentedFontData = static_cast<const SegmentedFontData*>(fontData);
> +        const SegmentedFontData* segmentedFontData = toSegmentedFontData(fontData);

Should be a reference, not a pointer, since it can’t be null.

[Gyuyoung Kim]

Comment on attachment 238004 [details]
Patch

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

>> Source/WebCore/platform/graphics/FontGlyphs.cpp:74
>> +        fontCache().releaseFontData(toSimpleFontData(m_realizedFontData[i]));
> 
> I don’t understand why the .get() isn’t needed any more.

By adding a below line, FONT_DATA_TYPE_CASTS generates "toSimpleFontData(const RefPtr<FontData>& fontData)" as well. Thanks to the function, FONT_DATA_TYPE_CASTS can handle .get(). It seems to me it would be nicer to delegate .get() handling to toFoo(). This idea comes from blink.

template<typename T> inline ToValueTypeName* to##ToValueTypeName(const RefPtr<T>& fontData) { return to##ToValueTypeName(fontData.get()); } \

[Gyuyoung Kim]

Created attachment 238102 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 238102 [details]
Patch for landing

Clearing flags on attachment: 238102

Committed r173612: <http://trac.webkit.org/changeset/173612>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 238004 [details]
Patch

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

>>> Source/WebCore/platform/graphics/FontGlyphs.cpp:74
>>> +        fontCache().releaseFontData(toSimpleFontData(m_realizedFontData[i]));
>> 
>> I don’t understand why the .get() isn’t needed any more.
> 
> By adding a below line, FONT_DATA_TYPE_CASTS generates "toSimpleFontData(const RefPtr<FontData>& fontData)" as well. Thanks to the function, FONT_DATA_TYPE_CASTS can handle .get(). It seems to me it would be nicer to delegate .get() handling to toFoo(). This idea comes from blink.
> 
> template<typename T> inline ToValueTypeName* to##ToValueTypeName(const RefPtr<T>& fontData) { return to##ToValueTypeName(fontData.get()); } \

I’m not sure this is a good idea. It’s random that some toXXX are overloaded to take RefPtr and others are not, and that this works on RefPtr but not PassRefPtr, and that the result is a raw pointer rather than another RefPtr. In some cases all of those things could be drawbacks. And in this case, the thing can never be nil so it would be nicer to use a reference rather than a pointer.

[Gyuyoung Kim]

(In reply to comment #9)
> (From update of attachment 238004 [details])

so it would be nicer to use a reference rather than a pointer.

Darin, ok, I see. Let me adjust your suggestion into new patch on Bug 136809.