SSIA
Created attachment 237998 [details] Patch
unofficial r=me
Created attachment 238004 [details] Patch
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.
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()); } \
Created attachment 238102 [details] Patch for landing
Comment on attachment 238102 [details] Patch for landing Clearing flags on attachment: 238102 Committed r173612: <http://trac.webkit.org/changeset/173612>
All reviewed patches have been landed. Closing bug.
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.
(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.