[Webkit-unassigned] [Bug 50619] [GTK] Glyphs in vertical text tests are rotated 90 degrees clockwise
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Jul 16 12:39:41 PDT 2013
https://bugs.webkit.org/show_bug.cgi?id=50619
--- Comment #25 from Martin Robinson <mrobinson at webkit.org> 2013-07-16 12:39:42 PST ---
(From update of attachment 206748)
View in context: https://bugs.webkit.org/attachment.cgi?id=206748&action=review
Couple questions on this one. For starters, I don't think the configuration argument is necessary.
> Source/WebCore/ChangeLog:20
> + * GNUmakefile.list.am:
> + * platform/graphics/FontCache.h:
> + * platform/graphics/freetype/FontCustomPlatformDataFreeType.cpp:
> + (WebCore::FontCustomPlatformData::FontCustomPlatformData):
> + (WebCore::FontCustomPlatformData::fontPlatformData):
> + * platform/graphics/freetype/FontPlatformData.h:
> + (WebCore::FontPlatformData::FontPlatformData):
Please fill these out.
> Source/WebCore/GNUmakefile.list.am:6376
> +if ENABLE_OPENTYPE_VERTICAL
> +platformgtk_sources += \
> + Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp \
> + Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.h
> +endif
> +
Is there some risk to always enabling this. I think it makes sense to make the feature unconditional if not.
> Source/WebCore/platform/graphics/FontCache.h:46
> #if OS(WINDOWS)
> -#include <windows.h>
> -#include <objidl.h>
> #include <mlang.h>
> +#include <objidl.h>
> +#include <windows.h>
> #endif
This looks unrelated? Are you trying to run check-webkit-style against the entire file. Better to just run it against the diff.
> Source/WebCore/platform/graphics/freetype/FontCustomPlatformDataFreeType.cpp:54
> - static_cast<cairo_destroy_func_t>(releaseCustomFontData));
> + static_cast<cairo_destroy_func_t>(releaseCustomFontData));
>
> // Cairo doesn't do FreeType reference counting, so we need to ensure that when
> // this cairo_font_face_t is destroyed, it cleans up the FreeType face as well.
> static cairo_user_data_key_t freeTypeFaceKey;
> cairo_font_face_set_user_data(m_fontFace, &freeTypeFaceKey, freeTypeFace,
> - reinterpret_cast<cairo_destroy_func_t>(FT_Done_Face));
> + reinterpret_cast<cairo_destroy_func_t>(FT_Done_Face));
> }
Looks unrelated.
> Source/WebCore/platform/graphics/freetype/FontPlatformData.h:27
> -#ifndef FontPlatformDataFreeType_h
> -#define FontPlatformDataFreeType_h
> +#ifndef FontPlatformData_h
> +#define FontPlatformData_h
I think we want to keep the Freetype here since this file is included from FrontPlatformData.h in the parent directory. In any case, this isn't related.
> Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:47
> +#if ENABLE(OPENTYPE_VERTICAL)
> +static FontCache::FontFileKey lastFontID = 0;
> +#endif
> +
It seems that FontFileKey is an AtomicString on other platforms?
> Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:321
> - -fontConfigMatrix.xy, fontConfigMatrix.yy, 0, 0);
> + -fontConfigMatrix.xy, fontConfigMatrix.yy, 0, 0);
This style fix looks unrelated.
> Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:356
> + && FT_Select_Charmap(freeTypeFace, ft_encoding_symbol)
> + && FT_Select_Charmap(freeTypeFace, ft_encoding_apple_roman));
Ditto.
> Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:383
> + FT_Error error = FT_Load_Sfnt_Table(fontConfigFace, tag, 0, 0, &tableSize);
> + if (error)
> + return 0;
> +
You can just do:
if (FT_Load_Sfnt_Table(fontConfigFace, tag, 0, 0, &tableSize))
return 0;
and avoid the temporary.
> Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:386
> + RefPtr<SharedBuffer> buffer;
> + FT_ULong expectedTableSize = tableSize;
> + buffer = SharedBuffer::create(tableSize);
Why not just do:
RefPtr<SharedBuffer> buffer = SharedBuffer::create(tableSize);?
> Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:390
> + error = FT_Load_Sfnt_Table(fontConfigFace, tag, 0, (FT_Byte*)buffer->data(), &tableSize);
Please use C++ casts. Why is a cast necessary here?
> Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:399
> +void FontPlatformData::setOrientation(FontOrientation orientation)
Can you walk me through how this is called?
> Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:403
> + if (m_scaledFont && m_pattern && (m_orientation != orientation)) {
Please use an early return instead.
> Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:409
> + cairo_font_face_t* fontFace;
> + cairo_matrix_t fontMatrix;
> + cairo_matrix_t ctm;
> + cairo_matrix_init_identity(&ctm);
> +
Define these variables when you first use them.
> Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:104
> + FT_ULong vheaSize = 0;
> + FT_ULong vorgSize = 0;
> + FT_Error error = 0;
> + const FT_Tag vheaTag = FT_MAKE_TAG('v', 'h', 'e', 'a');
Please define these right before you use them.
> Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:105
> + FT_Face fontConfigFace = cairo_ft_scaled_font_lock_face(m_platformData.scaledFont());
This is actually a Freetype face, not a FontConfig face.
> Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:109
> + error = FT_Load_Sfnt_Table(fontConfigFace, vheaTag, 0, 0, &vheaSize);
> + if ((!error) && (vheaSize > 0))
> + m_hasVerticalGlyphs = true;
You can avoid the temporary.
> Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:115
> + const FT_Tag vorgTag = FT_MAKE_TAG('V', 'O', 'R', 'G');
> + error = FT_Load_Sfnt_Table(fontConfigFace, vorgTag, 0, 0, &vorgSize);
> + if ((!error) && (vorgSize > 0))
> + m_hasVerticalGlyphs = true;
> + }
I think you could move this entire code segment into a couple helper functions.
bool tableLengthIsNonZero(FT_Face face, FT_TAG table)
{
FT_ULong size;
return FT_Load_Sfnt_Table(face, table, 0, 0, &size) && size > 0;
}
bool hasVerticalGlyphs(FT_Face freeTypeFace)
{
return tableLengthIsNonZero(freeTypeFace, FT_MAKE_TAG('v', 'h', 'e', 'a')) || tableLengthIsNonZero(freeTypeFace, FT_MAKE_TAG('V', 'O', 'R', G'));
}
On a more general note, you are looking for these tables, but you are not reading data from them. Why?
> Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:215
> + if (platformData().orientation() == Horizontal) {
> + if (cairo_scaled_font_status(m_platformData.scaledFont()) == CAIRO_STATUS_SUCCESS && extents.x_advance)
> + w = (float)extents.x_advance;
> + } else {
> + if (cairo_scaled_font_status(m_platformData.scaledFont()) == CAIRO_STATUS_SUCCESS && extents.y_advance) {
> + // In case vertical orientation, width is based on the y_advance as fonts are rotated -90 degrees
> + w = (float)extents.y_advance;
> + }
> + }
> +#else
Please use C++ casts in new code.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list