Bug 50619

Created attachment 75790 [details] a patch to make vertical text work on WebKitGtk initial patch to make vertical text work on WebKitGtk

Created attachment 75796 [details] a patch to make vertical text work on WebKitGtk+ remove two printf()

Created attachment 75816 [details] a patch to make vertical text work on WebKitGtk A patch to make vertical text work on WebKitGtk+. changed to modifying an existing FontPlatformData constructor rather than creating a new one.

Comment on attachment 75796 [details] a patch to make vertical text work on WebKitGtk+ View in context: https://bugs.webkit.org/attachment.cgi?id=75796&action=review Great start. I think we should initialize things properly up front instead of waiting until the render method. Is it possible to have Fontconfig to deliver a face with vertical glyps? That might avoid the need to call substituteWithVerticalGlyphs. Why must we call substituteWithVerticalGlyphs and also rotate the font matrix? > WebCore/platform/graphics/Font.cpp:348 > + // 0x2C7 Caron, Mandrin Chinese 3rd Tone > + // 0x2CA Modifier Letter Acute Accent, Mandarin Chinese 2nd Tone > + // 0x2CB Modifier Letter Grave Access, Mandarin Chinese 4th Tone > + // 0x2D9 Dot Above, Mandarin Chinese 5th Tone > + if ((c == 0x2C7) || (c == 0x2CA) || (c == 0x2CB) || (c == 0x2D9)) > + return true; > + > // The basic CJK Unified Ideographs block. I think the changes in Font.cpp should be separated out into a separate patch. This is logically a different problem than the lack of support in WebKItGTK+. > WebCore/platform/graphics/cairo/FontCairo.cpp:109 > for (int i = 0; i < numGlyphs; i++) { > glyphs[i].x = offset; > glyphs[i].y = 0.0f; > offset += glyphBuffer.advanceAt(from + i); > + if (isVertical) > + glyphs[i].x = offset; Don't we need to update the y offset because we're laying the text out vertically? why are you setting the x-offset to the offset of the next offset? > WebCore/platform/graphics/cairo/FontCairo.cpp:146 > + if (isVertical) { > + cairo_get_font_matrix(cr, &matrix); > + oldMatrix = matrix; > + cairo_matrix_rotate(&matrix, M_PI*1.5); > + cairo_set_font_matrix(cr, &matrix); > + } Why not rotate this matrix only once when the font is created? > WebCore/platform/graphics/freetype/FontPlatformData.h:62 > FontPlatformData(cairo_font_face_t* fontFace, float size, bool bold, bool italic); > + FontPlatformData(cairo_font_face_t* fontFace, float size, bool bold, bool italic, FontOrientation orientation = Horizontal); Please replace the old constructor instead of adding a new one, if possible. > WebCore/platform/graphics/freetype/FontPlatformData.h:76 > + FontOrientation orientation() const { return m_orientation; } // FIXME: Implement. > You should remove the FIXME now. :) > WebCore/platform/graphics/freetype/FontPlatformData.h:102 > + FontOrientation m_orientation; > cairo_scaled_font_t* m_scaledFont; This will generate warnings because the field initializers in the constructors are initializing things in the wrong order. It's probably better to add this last. Can't it be private? > WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:252 > && m_scaledFont == other.m_scaledFont && m_size == other.m_size > - && m_syntheticOblique == other.m_syntheticOblique && m_syntheticBold == other.m_syntheticBold; > + && m_syntheticOblique == other.m_syntheticOblique && m_syntheticBold == other.m_syntheticBold && m_orientation == other.m_orientation; Nit: please even out the line lengths here. > WebCore/platform/graphics/freetype/GlyphPageTreeNodeFreeType.cpp:45 > +static bool substituteWithVerticalGlyphs(unsigned offset, unsigned length, const SimpleFontData* fontData, FT_Face face, GlyphPage *glyphPage) The asterisk should be with GlyphPage. > WebCore/platform/graphics/freetype/GlyphPageTreeNodeFreeType.cpp:50 > + guint scriptIndex; > + guint featureIndex; Define these near where you first use them and initialize them. > WebCore/platform/graphics/freetype/GlyphPageTreeNodeFreeType.cpp:56 > + RefPtr<FcPattern> pattern = fontData->platformData().m_pattern; It's not necessary to use a RefPtr here since we do not want to take a reference. > WebCore/platform/graphics/freetype/GlyphPageTreeNodeFreeType.cpp:64 > + PangoFontMap* pangoFM; > + PangoContext* context; > + PangoFontDescription* description; > + PangoFont* pangoFont; Define these when you first use them, so it's easy to see that they are not unintialized. > WebCore/platform/graphics/freetype/GlyphPageTreeNodeFreeType.cpp:66 > + pangoFM = pango_ft2_font_map_new(); Do not use abbreviations for the PangoFontMap variable name. pangoFontMap is fine. > WebCore/platform/graphics/freetype/GlyphPageTreeNodeFreeType.cpp:68 > + description = pango_font_description_from_string((char *)fontConfigFamilyName); Please use static_cast here. > WebCore/platform/graphics/freetype/GlyphPageTreeNodeFreeType.cpp:71 > + return false; Don't you need to clean up the context and fontmap here? > WebCore/platform/graphics/freetype/GlyphPageTreeNodeFreeType.cpp:74 > + if (!fcFont) > + return false; Don't you need to clean up the pangoFont here? > WebCore/platform/graphics/freetype/GlyphPageTreeNodeFreeType.cpp:85 > + pango_ot_buffer_get_glyphs(pangoBuffer, &foo, &glyphLength); Where is foo from? Wouldn't a more descriptive name be better here? > WebCore/platform/graphics/freetype/GlyphPageTreeNodeFreeType.cpp:88 > + return true; Don't you need to clean up all of the objects you created here? > WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:79 > + FT_ULong length = 0; > + FT_Load_Sfnt_Table(face, TTAG_vhea, 0, 0, &length); > + if (length <= 0) > + m_orientation = Horizontal; > + } I'm not sure if this is the proper way to handle errors. Shouldn't you instead load the original table? Also, maybe it's better to check the return value of the freetype function.

Attachment 75816 [details] did not pass style-queue: Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128 error: Could not write new index file. fatal: Could not reset index file to revision 'refs/remotes/trunk'. If any of these errors are false positives, please file a bug against check-webkit-style.

Attachment 75816 [details] did not pass style-queue: Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128 error: Could not write new index file. fatal: Could not reset index file to revision 'refs/remotes/trunk'. If any of these errors are false positives, please file a bug against check-webkit-style.

Attachment 75816 [details] did not pass style-queue: Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128 error: Could not write new index file. fatal: Could not reset index file to revision 'refs/remotes/trunk'. If any of these errors are false positives, please file a bug against check-webkit-style.

Attachment 75816 [details] did not pass style-queue: Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128 error: Could not write new index file. fatal: Could not reset index file to revision 'refs/remotes/trunk'. If any of these errors are false positives, please file a bug against check-webkit-style.

Martin, thanks for the comments (In reply to comment #4) > > Great start. I think we should initialize things properly up front instead of waiting until the render method. Is it possible to have Fontconfig to deliver a face with vertical glyps? That might avoid the need to call substituteWithVerticalGlyphs. Why must we call substituteWithVerticalGlyphs and also rotate the font matrix? > substituteWithVerticalGlyphs() is not to get vertical glyphs but to substitute some glyphs, such as, s/U+300C/U+FE41/, see [1]. [1] http://en.wikipedia.org/wiki/Non-English_usage_of_quotation_mark > I think the changes in Font.cpp should be separated out into a separate patch. This is logically a different problem than the lack of support in WebKItGTK+. > Will separate it to another patch, thanks > > WebCore/platform/graphics/cairo/FontCairo.cpp:109 > > for (int i = 0; i < numGlyphs; i++) { > > glyphs[i].x = offset; > > glyphs[i].y = 0.0f; > > offset += glyphBuffer.advanceAt(from + i); > > + if (isVertical) > > + glyphs[i].x = offset; > > Don't we need to update the y offset because we're laying the text out vertically? why are you setting the x-offset to the offset of the next offset? > I know this looks a bit weird, but, as far as I can tell, the way vertical text is implemented is rotating a block clockwise and the coordinate inside the block is not changed. That is, vertical offset is along the x axis of the block. > > Why not rotate this matrix only once when the font is created? > I don't know how to do it. I think the a rotation matrix is associated with a cairo context rather than a font > > Please replace the old constructor instead of adding a new one, if possible. > OK > > + FontOrientation orientation() const { return m_orientation; } // FIXME: Implement. > > You should remove the FIXME now. :) > OK > > + FontOrientation m_orientation; > > cairo_scaled_font_t* m_scaledFont; > > This will generate warnings because the field initializers in the constructors are initializing things in the wrong order. It's probably better to add this last. Can't it be private? > OK. changed the order and made it private without problem > > WebCore/platform/graphics/freetype/GlyphPageTreeNodeFreeType.cpp:56 > > + RefPtr<FcPattern> pattern = fontData->platformData().m_pattern; > > It's not necessary to use a RefPtr here since we do not want to take a reference. Argh, I am a nobrainer, just set type to whatever fontData->platformData().m_pattern is :-) > Don't you need to clean up the context and fontmap here? > Don't you need to clean up the pangoFont here? > > WebCore/platform/graphics/freetype/GlyphPageTreeNodeFreeType.cpp:88 > > + return true; > > Don't you need to clean up all of the objects you created here? Yes, I should!! I can think of 2 ways to handle this. Which one is the preferred way. The first one is something like if (!pangoFontMap) return false; PangoContext* pangoContext = pango_font_map_create_context(PANGO_FONT_MAP(pangoFontMap)); if (!pangoContext) { g_object_unref(pangoFontMap); return false; } PangoFontDescription* pangoDescription = pango_font_description_from_string(reinterpret_cast<char*>(fontConfigFamilyName)); if (!pangoDescription) { g_object_unref(pangoContext); g_object_unref(pangoFontMap); return false; } ..... And the second one is, with goto: pango_ot_buffer_destroy(pangoBuffer); error4: g_object_unref(pangoFont); error3: pango_font_description_free(pangoDescription); error2: g_object_unref(pangoContext); error1: g_object_unref(pangoFontMap); if (success) return true; return false; > > WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:79 > > + FT_ULong length = 0; > > + FfT_Load_Sfnt_Table(face, TTAG_vhea, 0, 0, &length); > > + if (length <= 0) > > + m_orientation = Horizontal; > > + } > > I'm not sure if this is the proper way to handle errors. Shouldn't you instead load the original table? Also, maybe it's better to check the return value of the freetype function. according to freetype manual, passing length with value zero doesn't load the table, it's used to get the real length. Anyway, you are right that I should check the return value. Will do it.

Created attachment 75865 [details] a patch to make vertical text work on WebKitGtk+ Modified according to Martin's comments except rotating font when it is created, 'cause I don't know how to do that. Error handling in substituteWithVerticalGlyphs() has some redundant code, because if I use goto, then there will be "jump to label crosses initialization" problems.

Attachment 75816 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2 Updating OpenSource Incomplete data: Delta source ended unexpectedly at /usr/lib/git-core/git-svn line 5061 Died at WebKitTools/Scripts/update-webkit line 132. If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 76002 [details] a patch to make vertical text work on WebKitGtk+ Same as 75865, remove unnecessary RefPtr<FcPattern>

Created attachment 76327 [details] proposed patch to make vertical text work on WebKitGtk+ 1. Use the font matrix in cairo_scaled_font_t instead of the one with cairo_context 2. Use PlatformRefPtr to get rid of repetitive g_object_unref()

Created attachment 77168 [details] proposed patch to make vertical text work on WebKitGtk+ improvement over 76327: 1. remove recreateScaledFont() 2. avoid unnecessary vertical glyph lookup 3. handle shadow text

Comment on attachment 77168 [details] proposed patch to make vertical text work on WebKitGtk+ View in context: https://bugs.webkit.org/attachment.cgi?id=77168&action=review This patch is looking better and better! I still have a few concerns though. > WebCore/platform/graphics/cairo/FontCairo.cpp:56 > +static void rotateBackIfNecessary(cairo_t *context, const SimpleFontData* font) The placement of the asterisk on "context" is incorrect. > WebCore/platform/graphics/cairo/FontCairo.cpp:60 > + if (isHorizontalOnly) { It's probably better to use an early return here like (untested!): if ((!font->platformData().orientation() == Vertical) || (!font->orientation() == Horizontal)) return; > WebCore/platform/graphics/cairo/FontCairo.cpp:63 > + cairo_matrix_rotate(&fontMatrix, M_PI*0.5); Is there any reason this is better than M_PI / 2? > WebCore/platform/graphics/cairo/FontCairo.cpp:70 > + rotateBackIfNecessary(context, font); I think this can be avoided. > WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:278 > + cairo_matrix_rotate(&fontMatrix, -M_PI*0.5); Again, is this better in some way than "/ 2"? > WebCore/platform/graphics/freetype/GlyphPageTreeNodeFreeType.cpp:48 > +static bool substituteWithVerticalGlyphs(unsigned offset, unsigned length, const SimpleFontData* fontData, FT_Face face, GlyphPage* glyphPage) Is this called for many glyphs? If so is there a lot of overhead for doing this over and over again for each one? > WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:59 > + // if the font doesn't have vertical metrics, we don't rotate it 90 degress counterclockwise > + if (m_orientation == Vertical) { > + FT_Face face = cairo_ft_scaled_font_lock_face(m_platformData.scaledFont()); > + if (!FT_HAS_VERTICAL(face)) > + m_orientation = Horizontal; > + cairo_ft_scaled_font_unlock_face(m_platformData.scaledFont()); > + } > + I still think it might be better to do this check in FontPlatformData::initializeWithFontFace so that we can avoid undoing the rotation in drawGlyphs above.

(In reply to comment #15) > > WebCore/platform/graphics/cairo/FontCairo.cpp:63 > > + cairo_matrix_rotate(&fontMatrix, M_PI*0.5); > Is there any reason this is better than M_PI / 2? I think multiplication is cheaper than division :-) How about M_PI_2? it seems most unix platforms have M_PI_2, and I can add #if !defined(M_PI_2) #define M_PI_2 1.57079632679489661923132169163975144 /* pi/2 */ #endif > > WebCore/platform/graphics/freetype/GlyphPageTreeNodeFreeType.cpp:48 > > +static bool substituteWithVerticalGlyphs(unsigned offset, unsigned length, const SimpleFontData* fontData, FT_Face face, GlyphPage* glyphPage) > > Is this called for many glyphs? If so is there a lot of overhead for doing this over and over again for each one? > Yes, I already avoid those in isCJKIdeographs. Many glyphs have vertical forms, esp. in Japanese fonts. To reduce the overhead, the only way I can think of right now is to load and parse the table using freetype, cache it in memory. > > WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:59 > > + // if the font doesn't have vertical metrics, we don't rotate it 90 degress counterclockwise > > + if (m_orientation == Vertical) { > > + FT_Face face = cairo_ft_scaled_font_lock_face(m_platformData.scaledFont()); > > + if (!FT_HAS_VERTICAL(face)) > > + m_orientation = Horizontal; > > + cairo_ft_scaled_font_unlock_face(m_platformData.scaledFont()); > > + } > > + > > I still think it might be better to do this check in FontPlatformData::initializeWithFontFace so that we can avoid undoing the rotation in drawGlyphs above. OK, I'll try that.

(In reply to comment #15) > > WebCore/platform/graphics/cairo/FontCairo.cpp:70 > > + rotateBackIfNecessary(context, font); > > I think this can be avoided. I tried to check if a font has vertical metrics by creating FT_Face with FcPattern in FontPlatformData::initializeWithFontFace(). Yes, it works. But I cannot find a way to deal with some broken font cases, such as LayoutTests/fast/blockflow/broken-ideographic-font.html. It seems this cannot be avoided. These cases display CJK+non-CJK glyphs with "broken" fonts (no vertical metrics), but CJK ideographs still need be displayed correctly. Let me explain what I know, see if the this make sense. 1. there are two orientations. One is SimpleFontData's; the other is FontPlatformData's 2. by the WebCore/platform/graphics/SimpleFontData.h, the one in SimpleFontData should be the supported orientation according to the tables in the font. FontPlatformData will always have the desired orientation. (This is for 3.) 3. the broken font case is handled by Font::glyphDataForCharacter in WebCore/platform/graphics/FontFastPath.cpp (if (data.fontData->platformData().orientation() == Vertical && data.fontData->orientation() == Horizontal && Font::isCJKIdeographOrSymbol(c))) 4. here comes the tricky part 4.1 if all the unicode characters to be displayed are !Font::isCJKIdeographOrSymbol(), then setting FontPlatformData's orientation to be Horizontal is good. 4.2 if there are some Font::isCJKIdeographOrSymbol() characters, we need something like 3., or we have to use 3. > > > WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:59 > > + // if the font doesn't have vertical metrics, we don't rotate it 90 degress counterclockwise > > + if (m_orientation == Vertical) { > > + FT_Face face = cairo_ft_scaled_font_lock_face(m_platformData.scaledFont()); > > + if (!FT_HAS_VERTICAL(face)) > > + m_orientation = Horizontal; > > + cairo_ft_scaled_font_unlock_face(m_platformData.scaledFont()); > > + } > > + > > I still think it might be better to do this check in FontPlatformData::initializeWithFontFace so that we can avoid undoing the rotation in drawGlyphs above.

Created attachment 78369 [details] proposed patch to make vertical text work on WebKitGtk+ update 77168 1. rebase: WebCore -> SourceWebCore 2. move substituteWithVerticalGlyphs() to platform/graphics/gtk/PangoFontHelpers.cpp 3. use FT_Face->family_name and FT_Face->style_name instead of family name from FcPattern

Created attachment 163161 [details] Patch

I have been working on a new patch for this issue, which has a similar approach as the one proposed by Koan-Sin Tan. The main difference is that FontPlatformData::setOrientation() has also to rotate the fonts, and there is no font substitutions in Pango.

Comment on attachment 163161 [details] Patch I'll modify this patch to have a 90-rotate font in the context rather than rotating fonts when needed. It should give a better performance for long texts.

Created attachment 206748 [details] WIP Patch based on OPENTYPE_VERTICAL

(In reply to comment #22) > Created an attachment (id=206748) [details] > WIP Patch based on OPENTYPE_VERTICAL I have modified my previous patch to use OPENTYPE_VERTICAL. This feature is not currently implemented in the GTK+ port, but since it is not very complicated, I thought it was worth giving it a try. Although this patch is complete, it has been marked as a WIP because there are some assumptions to be discussed: - The solution is based on OPENTYPE_VERTICAL. There is currently no maintainer for this feature in the GTK+ port, but I am happy to help on this - This feature is disabled by default in this patch and a compilation option --enable-opentype-vertical has been introduced - The layout tests related to vertical texts are not re-based yet because OPENTYPE_VERTICAL is disabled by default - This patch also includes some minor coding style fixes, which are not related to the code for this fix, and if necessary I can move them to another patch to make it easier to review - Another option for this patch is to raise a new bug for implementing OPENTYPE_VERTICAL and add it to the "Depends on" list. Then we can keep this one open until OPENTYPE_VERTICAL gets enabled by default. - Since the code is only executed when FontPlatformData::orientation() == Vertical, the performance of horizontal text rendering should not be affected by this change I have tested this patch against several Japanese and Chinese pages with vertical text including the ones in LayoutTests. The results are good and visually the same as shown by Chrome. editing/selection/vertical-lr-ltr-extend-line-* fast/repaint/japanese-rl-selection-* fast/writing-mode/japanese-* fast/writing-mode/vertical-*

Comment on attachment 78369 [details] proposed patch to make vertical text work on WebKitGtk+ Marking this patch as obsolete on behalf of Denis, who doesn't have EditBug permissions yet. He says that he contacted the original author and he's no longer working on this.

Comment on attachment 206748 [details] WIP Patch based on OPENTYPE_VERTICAL 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.

Comment on attachment 206748 [details] WIP Patch based on OPENTYPE_VERTICAL Attachment 206748 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/1123005 New failing tests: webaudio/delaynode-max-default-delay.html

Created attachment 206826 [details] Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-16 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.3

(In reply to comment #25) Many thanks for reviewing the patch. I have modified it according to the comments. Initially I added a configuration argument just to be on the safe side since this is my first time adding a feature guarded by ENABLE_*. I also agree this feature can be unconditional mainly because it is only trigged when the content has vertical text. This type of scenario is currently always incorrect, and horizontal texts should not be affected by this change. I have removed all changes done on unrelated code to fix coding style problems. I just found a problem when handling the glyph extents of vertical ghyphs. In particular, y_advance is negative and CairoGetGlyphWidthAndExtents() in the complex path had to be modified as it uses Cairo scaled fonts from SimpleFontData. > > 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? I had similar concerns about this. The idea of defining FontFileKey as integer is based on Chromium, where the ID was provided by Skia. On the plus side, it should be more efficient than a string. I have also tried to find out what other ports do, but I could not find any other code that calls FontCache::getVerticalData(). Strange. > > 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? It is a sign/const convertion issue. error: invalid conversion from 'const char*' to 'FT_Byte* {aka unsigned char*}' I have modified the code to use C++ casts. My bad.. > > Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:399 > > +void FontPlatformData::setOrientation(FontOrientation orientation) > > Can you walk me through how this is called? In my investigations this function is only called to change the orientation from vertical to horizontal from SimpleFontData::verticalRightOrientationFontData(), which is called by glyphDataAndPageForNonCJKCharacterWithGlyphOrientation() when non-CJK characters need to be displayed among other CJK characters. It can happen when the vertical text is composed by CJK characters with some non-CJK characters as well. > > 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? Ops, great catch. m_hasVerticalGlyphs was initialized in two places, first at SimpleFontData::platformInit() and later on at SimpleFontData::SimpleFontData(). I have removed the first one, which I introduced.

Created attachment 206896 [details] Patch

Comment on attachment 206896 [details] Patch Attachment 206896 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/1097208

Comment on attachment 206896 [details] Patch Attachment 206896 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/1103227

Created attachment 206976 [details] Patch

(In reply to comment #32) > Created an attachment (id=206976) [details] In the previous patch, I forgot OPENTYPE_VERTICAL code will be shared with the EFL port, which won't have it enabled by default for now. I have put the guards back in this new patch then other ports can compile it without enabling OPENTYPE_VERTICAL.

Comment on attachment 206976 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=206976&action=review I found few improvements to this patch, and hopefully it will help to review it. They are described below and I will soon upload a new patch with them. I will also rebase the patch to the latest code. > Source/WebCore/platform/graphics/FontCache.h:152 > +#if PLATFORM(GTK) > + typedef uint32_t FontFileKey; > +#else > typedef AtomicString FontFileKey; > +#endif As Martin mentioned before, this change is not convenient since GTK will be the only port with a different type for FontFileKey. I will revert this change and will improve the definition of HashMap<FontFileKey,...> in FontCache.cpp. The current implementation of this HashMap assumes that FontFileKey is an integer and it doesn't allow FontFileKey to be an AtomicString (I got a compilation error). I think this problem happened after the Chromium/Skia code clean up. The Chromium port used to define FontFileKey as an integer, which was given by Skia. > Source/WebCore/platform/graphics/freetype/FontPlatformData.h:93 > + FontCache::FontFileKey uniqueID() const { return m_fontID; }; uniqueID()/m_fontID are only used once in FontPlatformDataFreeType.cpp, and this can be removed/optimised. > Source/WebCore/platform/graphics/freetype/FontPlatformData.h:132 > + FontCache::FontFileKey m_fontID; Ditto. > Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:45 > +static FontCache::FontFileKey lastFontID = 0; This is ugly. Instead of generating ID's for FontFileKey, I would like to propose hash() instead. hash() also gives an unique value based on m_scaledFont. > Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:367 > +static uint32_t reverseByteOrder(uint32_t tableTag) This function is only called once. Perhaps this can be optimised.

Created attachment 212584 [details] Patch Modified patch according to review comments.

Comment on attachment 212584 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=212584&action=review Please find a couple of comments below. I am not an expert, so this is not even an informal review, just an effort on my side to understand the patch :) > Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:323 > + cairo_matrix_translate(&fontMatrix, 0.0, 1.0); Could you explain why this makes the translation in the y-axis, while in FontPlatformData::setOrientation you translate in the x-axis? > Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:363 > + uint32_t tag = (table >> 24) | ((table >> 8) & 0xff00) | ((table & 0xff00) << 8) | ((table & 0xff) << 24); I am not sure, but maybe you could try to use http://www.freetype.org/freetype2/docs/reference/ft2-basic_types.html#FT_MAKE_TAG for this, and it would increase readability. > Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:388 > + cairo_matrix_t ctm; Maybe you could use a more verbose name here? > Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:408 > + This is what I meant: here you do the translation on the other x-axis.

(In reply to comment #36) > (From update of attachment 212584 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=212584&action=review > > Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:323 > > + cairo_matrix_translate(&fontMatrix, 0.0, 1.0); > > Could you explain why this makes the translation in the y-axis, while in FontPlatformData::setOrientation you translate in the x-axis? Sure. Here is my understanding, please correct me if I'm wrong. In the Cairo coordinate system, glyphs are drawn in the quadrant where x and y are positive. For example, in the figure (a) below, the glyph is drawn in quadrant 4. Assuming texts are rotated 90 degrees in the user space because the text orientation is vertical, then the coordinates are rotated as in (b) and the glyph is located at quadrant 1. So the idea behind the fix is to keep the glyph in quadrant 1 (b) but rotated anti-clockwise 90 degrees. Therefore, rotation angle = -90 degrees, and translation = (0,1). Where (0,1) means a translation of font size towards y-axis. Assuming the system is scaled by font size. (a) HORIZONTAL ORIENTATION (b) VERTICAL ORIENTATION | | 3 | 4 2 | 3 | x y | -----------> <----------- | | 2 | 1 1 | 4 | | v y v x Translation: (0,0) Translation: (0,1) Rotation: 0 Rotation: -90 degrees | 1 0 0 | | 1 0 0 | | 1 0 0 | | 0 -1 0 | | 0 -1 0 | | 0 1 0 | | 0 1 0 | x | 0 1 0 | x | 1 0 0 | = | 1 0 0 | | 0 0 1 | | 0 0 1 | | 0 1 1 | | 0 0 1 | | 1 0 1 | Horizontal Matrix Horizontal Translation Rotation Vertical (Identity) Matrix Matrix Matrix Matrix Where translation matrix = | cos sin 0 | cos - cosine of the rotation angle | -sin cos 0 | sin - sine of the rotation angle | tx ty 1 | rotation matrix = | 1 0 0 | tx,ty - translation in x and y, | 0 1 0 | respectively | tx ty 1 | At FontPlatformData::setOrientation(), there are two situations: 1) From vertical orientation to horizontal orientation When changing the orientation from vertical to horizontal, the coordinate system changes from (c) to (d) below, and the glyph is located in quadrant 1. So this time, we want to keep the glyph in quadrant 1 (d) but rotated clockwise 90 degrees. Therefore, rotation angle = 90 degrees and translation = (-1,0). The peculiar Cairo coordinate system is the reason why translation is done in x-axis and not in y-axis as in (b). The resulting rotation angle and translation in (d) should be the same as in (a). (c) VERTICAL ORIENTATION (d) HORIZONTAL ORIENTATION | | 2 | 3 3 | 4 y | | x <----------- -----------> | | 1 | 4 2 | 1 | | v x v y Translation: (0,1) Translation: (-1,0) Rotation: -90 degrees Rotation: 90 degrees | 0 -1 0 | | 0 -1 0 | | 1 0 0 | | 0 1 0 | | 1 0 0 | | 1 0 0 | | 1 0 0 | x | 0 1 0 | x |-1 0 0 | = | 0 1 0 | | 1 0 1 | | 1 0 1 | |-1 0 1 | | 0 0 1 | | 0 0 1 | Vertical Vertical Translation Rotation Horizontal Matrix Matrix Matrix Matrix Matrix 2) From horizontal orientation to vertical orientation This is the same case as in (a) to (b). I realised this last case is incorrect in my patch, which incorrectly translates in the x-axis. This case is not currently used in the code and that explains why this bug wasn't spotted before. I will fix it. > > Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:363 > > + uint32_t tag = (table >> 24) | ((table >> 8) & 0xff00) | ((table & 0xff00) << 8) | ((table & 0xff) << 24); > > I am not sure, but maybe you could try to use http://www.freetype.org/freetype2/docs/reference/ft2-basic_types.html#FT_MAKE_TAG for this, and it would increase readability. Nice one :) That line can be simplified a bit: uint32_t tag = FT_MAKE_TAG(table & 0xff, table & 0xff00, table & 0xff0000, table & 0xff000000); > > Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:388 > > + cairo_matrix_t ctm; > > Maybe you could use a more verbose name here? Sure. I'll do it in the next patch.

Comment on attachment 212584 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=212584&action=review Thanks a lot for your time preparing the diagram! I thought about it, and I proposed an alternative that maybe feels a bit easier to understand. >>> Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:323 >>> + cairo_matrix_translate(&fontMatrix, 0.0, 1.0); >> >> Could you explain why this makes the translation in the y-axis, while in FontPlatformData::setOrientation you translate in the x-axis? > > Sure. Here is my understanding, please correct me if I'm wrong. > In the Cairo coordinate system, glyphs are drawn in the quadrant where x and y are positive. For example, in the figure (a) below, the glyph is drawn in quadrant 4. > Assuming texts are rotated 90 degrees in the user space because the text orientation is vertical, then the coordinates are rotated as in (b) and the glyph is located at quadrant 1. > So the idea behind the fix is to keep the glyph in quadrant 1 (b) but rotated anti-clockwise 90 degrees. Therefore, rotation angle = -90 degrees, and translation = (0,1). Where (0,1) means a translation of font size towards y-axis. Assuming the system is scaled by font size. > > (a) HORIZONTAL ORIENTATION (b) VERTICAL ORIENTATION > | | > 3 | 4 2 | 3 > | x y | > -----------> <----------- > | | > 2 | 1 1 | 4 > | | > v y v x > > Translation: (0,0) Translation: (0,1) > Rotation: 0 Rotation: -90 degrees > > | 1 0 0 | | 1 0 0 | | 1 0 0 | | 0 -1 0 | | 0 -1 0 | > | 0 1 0 | | 0 1 0 | x | 0 1 0 | x | 1 0 0 | = | 1 0 0 | > | 0 0 1 | | 0 0 1 | | 0 1 1 | | 0 0 1 | | 1 0 1 | > Horizontal Matrix Horizontal Translation Rotation Vertical > (Identity) Matrix Matrix Matrix Matrix > > Where translation matrix = | cos sin 0 | cos - cosine of the rotation angle > | -sin cos 0 | sin - sine of the rotation angle > | tx ty 1 | > > rotation matrix = | 1 0 0 | tx,ty - translation in x and y, > | 0 1 0 | respectively > | tx ty 1 | > > At FontPlatformData::setOrientation(), there are two situations: > > 1) From vertical orientation to horizontal orientation > When changing the orientation from vertical to horizontal, the coordinate system changes from (c) to (d) below, and the glyph is located in quadrant 1. > So this time, we want to keep the glyph in quadrant 1 (d) but rotated clockwise 90 degrees. > Therefore, rotation angle = 90 degrees and translation = (-1,0). The peculiar Cairo coordinate system is the reason why translation is done in x-axis and not in y-axis as in (b). > The resulting rotation angle and translation in (d) should be the same as in (a). > > (c) VERTICAL ORIENTATION (d) HORIZONTAL ORIENTATION > | | > 2 | 3 3 | 4 > y | | x > <----------- -----------> > | | > 1 | 4 2 | 1 > | | > v x v y > > Translation: (0,1) Translation: (-1,0) > Rotation: -90 degrees Rotation: 90 degrees > > | 0 -1 0 | | 0 -1 0 | | 1 0 0 | | 0 1 0 | | 1 0 0 | > | 1 0 0 | | 1 0 0 | x | 0 1 0 | x |-1 0 0 | = | 0 1 0 | > | 1 0 1 | | 1 0 1 | |-1 0 1 | | 0 0 1 | | 0 0 1 | > Vertical Vertical Translation Rotation Horizontal > Matrix Matrix Matrix Matrix Matrix > > 2) From horizontal orientation to vertical orientation > This is the same case as in (a) to (b). > I realised this last case is incorrect in my patch, which incorrectly translates in the x-axis. This case is not currently used in the code and that explains why this bug wasn't spotted before. > I will fix it. If we start from a Horizontal matrix H and we want to get to the Vertical matrix V, we are doing it like: V = H · R · T So, if we want to go back to the original Horizontal matrix, we should be able to do H · (R · T) · (R · T)^(-1) = V · (R · T)^(-1) So H = V · T^(-1) · R^(-1) In the case of the rotation matrix, if we invert it, we get to a Rotation matrix of M_PI_2: the opposite to the previous angle. Then, when we invert the translation matrix, it turns out that we are translating back (0, -1): the opposite amount of the previous translation. When going from horizontal to vertical, we do cairo_matrix_rotate(&fontMatrix, -M_PI_2); cairo_matrix_translate(&fontMatrix, 0.0, 1.0); so we would, when going from vertical to horizontal, do cairo_matrix_translate(&fontMatrix, 0.0, -1.0); cairo_matrix_rotate(&fontMatrix, M_PI_2); It could be that keeping the order you suggest is also fine, but I think it feels more natural to think about inverting the matrices for going back. >>> Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:363 >>> + uint32_t tag = (table >> 24) | ((table >> 8) & 0xff00) | ((table & 0xff00) << 8) | ((table & 0xff) << 24); >> >> I am not sure, but maybe you could try to use http://www.freetype.org/freetype2/docs/reference/ft2-basic_types.html#FT_MAKE_TAG for this, and it would increase readability. > > Nice one :) That line can be simplified a bit: > > uint32_t tag = FT_MAKE_TAG(table & 0xff, table & 0xff00, table & 0xff0000, table & 0xff000000); Cool! I think it's more readable!

(In reply to comment #38) > (From update of attachment 212584 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=212584&action=review > > Thanks a lot for your time preparing the diagram! I thought about it, and I proposed an alternative that maybe feels a bit easier to understand. Pleasure and many thanks for helping on this :) > > 2) From horizontal orientation to vertical orientation > > This is the same case as in (a) to (b). > > I realised this last case is incorrect in my patch, which incorrectly translates in the x-axis. This case is not currently used in the code and that explains why this bug wasn't spotted before. > > I will fix it. > > If we start from a Horizontal matrix H and we want to get to the Vertical matrix V, we are doing it like: > > V = H • R • T > > So, if we want to go back to the original Horizontal matrix, we should be able to do > > H • (R • T) • (R • T)^(-1) = V • (R • T)^(-1) > > So > > H = V • T^(-1) • R^(-1) > > In the case of the rotation matrix, if we invert it, we get to a Rotation matrix of M_PI_2: the opposite to the previous angle. Then, when we invert the translation matrix, it turns out that we are translating back (0, -1): the opposite amount of the previous translation. > > When going from horizontal to vertical, we do > > cairo_matrix_rotate(&fontMatrix, -M_PI_2); > cairo_matrix_translate(&fontMatrix, 0.0, 1.0); > > so we would, when going from vertical to horizontal, do > > cairo_matrix_translate(&fontMatrix, 0.0, -1.0); > cairo_matrix_rotate(&fontMatrix, M_PI_2); > > It could be that keeping the order you suggest is also fine, but I think it feels more natural to think about inverting the matrices for going back. Great approach. I completely agree and the code will be much easier to understand. I will modify it in the next patch. And also add some comments based on your explanation.

Created attachment 213358 [details] Patch Modified the patch according to comment #36 and comment #38.

(In reply to comment #40) > Created an attachment (id=213358) [details] > Patch > > Modified the patch according to comment #36 and comment #38. Thanks! It looks nice to me. Let's see what the reviewers have to say.

Comment on attachment 213358 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=213358&action=review Okay. Apologies for the *very* late review. This is looking a lot better. I think you could reduce the repetition quite a bit. Let's also just turn on OPENTYPE_VERTICAL by default on all Freetype ports. I don't think it's useful to support multiple configurations. Another option is just to keep one code path, but ensure that the orientation is never vertical when OPENTYPE_VERTICAL is disabled. > Source/WebCore/platform/graphics/FontCache.cpp:241 > +struct FontVerticalDataCacheKeyHash { > + static unsigned hash(const FontCache::FontFileKey& fontFileKey) > + { > + return PtrHash<const FontCache::FontFileKey*>::hash(&fontFileKey); > + } > + > + static bool equal(const FontCache::FontFileKey& a, const FontCache::FontFileKey& b) > + { > + return a == b; > + } > + > + static const bool safeToCompareToEmptyOrDeleted = true; > +}; You probably want to describe in the ChangeLog why the new hash function is necessary. > Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:366 > + // Reverse the byte order. I think it makes sense to expand this commend to explain *why* you are reversing the byte order as well. > Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:367 > + uint32_t tag = FT_MAKE_TAG(table & 0xff, table & 0xff00, table & 0xff0000, table & 0xff000000); Extra space here. > Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:385 > +void FontPlatformData::setOrientation(FontOrientation orientation) Is this ever called for us? The only place I see this called is in SimpleFontData::verticalRightOrientationFontData, but that appears to only be called by the Mac port. > Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:396 > + cairo_font_face_t* fontFace; > + fontFace = cairo_scaled_font_get_font_face(m_scaledFont); Should be one line. > Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:402 > + cairo_font_options_t* options = getDefaultFontOptions(); > + setCairoFontOptionsFromFontConfigPattern(options, m_pattern.get()); Not sure I understand why this is called again. > Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:410 > + if (orientation == Vertical) { > + // This transformation is the same as the one done at initializeWithFontFace(). > + // V = H · R · T, where R rotates by -90 degrees and T translates by font > + // size towards y axis. > + cairo_matrix_rotate(&fontMatrix, -M_PI_2); > + cairo_matrix_translate(&fontMatrix, 0.0, 1.0); If this is necessary (see above), then this should move into a helper function/method. > Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:419 > + // The transformation matrix (H) for reversing the changes applied for vertical > + // orientation can be obtained by re-arranging the equation used to calculate > + // the matrix (V) for vertical orientation (e.g. V = H · R · T). > + // V = H · R · T ==> V · (R · T)^-1 = H · (R · T) · (R · T)^-1 ==> H = V · T^-1 · R^-1 > + // where R^-1 rotates by 90 degrees and T^-1 translates by font size against y axis. > + cairo_matrix_translate(&fontMatrix, 0.0, -1.0); > + cairo_matrix_rotate(&fontMatrix, M_PI_2); > + } Instead of trying to undo the rotation, why not either save the original matrix or produce it again via a helper. That would allow you to eliminate this code. > Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:98 > + cairo_scaled_font_text_extents(m_platformData.scaledFont(), "x", &textExtents); > + // In case of vertical orientation, glyphs are rotated and height is actually width. > + m_fontMetrics.setXHeight(narrowPrecisionToFloat(textExtents.width)); > + > + cairo_scaled_font_text_extents(m_platformData.scaledFont(), " ", &textExtents); > + // In case of vertical orientation, width is based on the y_advance as fonts are rotated -90 degrees > + m_spaceWidth = narrowPrecisionToFloat(-textExtents.y_advance); > + This looks like a lot of repetition, so why not add two small helpers, heightFromTextExtentsAndOrientation and widthFromTextExtentsAndOrientation. > Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:103 > + if (!isTextOrientationFallback()) { > + FT_Face freeTypeFace = cairo_ft_scaled_font_lock_face(m_platformData.scaledFont()); > + m_fontMetrics.setUnitsPerEm(freeTypeFace->units_per_EM); > + cairo_ft_scaled_font_unlock_face(m_platformData.scaledFont()); > + } Not sure I understand why this is necessary now... > Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:198 > + if (platformData().orientation() == Horizontal) { > + if (cairo_scaled_font_status(m_platformData.scaledFont()) == CAIRO_STATUS_SUCCESS && extents.x_advance) > + w = static_cast<float>(extents.x_advance); > + } else { > + if (cairo_scaled_font_status(m_platformData.scaledFont()) == CAIRO_STATUS_SUCCESS && extents.y_advance) { > + // In case of vertical orientation, width is based on y_advance as fonts are rotated -90 degrees > + w = static_cast<float>(-extents.y_advance); > + } > + } Or: if (cairo_scaled_font_status(m_platformData.scaledFont()) == CAIRO_STATUS_SUCCESS && extents.x_advance) w = platformData().orientation() == Horizontal ? extents.x_advance : -extents.y_advance; or use the helper described above. :) > Source/WebCore/platform/graphics/harfbuzz/HarfBuzzFaceCairo.cpp:112 > +#if ENABLE(OPENTYPE_VERTICAL) > + if (!(*advance)) { > + // There is no advance in x, so assuming this font has vertical ghyphs. > + *advance = doubleToHarfBuzzPosition(-glyphExtents.y_advance); > + if (extents) { > + extents->x_bearing = doubleToHarfBuzzPosition(glyphExtents.x_bearing); > + extents->y_bearing = doubleToHarfBuzzPosition(-glyphExtents.y_bearing); > + extents->width = doubleToHarfBuzzPosition(-glyphExtents.height); > + extents->height = doubleToHarfBuzzPosition(glyphExtents.width); > + } > + } else { > + if (extents) { > + extents->x_bearing = doubleToHarfBuzzPosition(glyphExtents.x_bearing); > + extents->y_bearing = doubleToHarfBuzzPosition(glyphExtents.y_bearing); > + extents->width = doubleToHarfBuzzPosition(glyphExtents.width); > + extents->height = doubleToHarfBuzzPosition(glyphExtents.height); > + } > + } There's a lot of repetition here as well.

Comment on attachment 213358 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=213358&action=review Never late, thank you very much! I will modify the patch to enable OPENTYPE_VERTICAL by default on all Freetype ports. I've also noticed there are some characters in my comments that don't display properly by Bugzilla (e.g. matrix multiplication operator), so I will replace them. >> Source/WebCore/platform/graphics/FontCache.cpp:241 >> +}; > > You probably want to describe in the ChangeLog why the new hash function is necessary. Sure, I will do it. >> Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:366 >> + // Reverse the byte order. > > I think it makes sense to expand this commend to explain *why* you are reversing the byte order as well. Good point. I will do it. >> Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:367 >> + uint32_t tag = FT_MAKE_TAG(table & 0xff, table & 0xff00, table & 0xff0000, table & 0xff000000); > > Extra space here. Ops sorry. >> Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:385 >> +void FontPlatformData::setOrientation(FontOrientation orientation) > > Is this ever called for us? The only place I see this called is in SimpleFontData::verticalRightOrientationFontData, but that appears to only be called by the Mac port. The code changed quite significantly since the initial patch, in particular after the Chromium code clean up. I will re-test this function and will double check the use cases. >> Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:98 >> + > > This looks like a lot of repetition, so why not add two small helpers, heightFromTextExtentsAndOrientation and widthFromTextExtentsAndOrientation. Ok I'll add them. >> Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:198 >> + } > > Or: > > if (cairo_scaled_font_status(m_platformData.scaledFont()) == CAIRO_STATUS_SUCCESS && extents.x_advance) > w = platformData().orientation() == Horizontal ? extents.x_advance : -extents.y_advance; > > or use the helper described above. :) Cool nice one. I'll change it. >> Source/WebCore/platform/graphics/harfbuzz/HarfBuzzFaceCairo.cpp:112 >> + } > > There's a lot of repetition here as well. Sure, I'll simplify this.

Comment on attachment 213358 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=213358&action=review >>> Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:367 >>> + uint32_t tag = FT_MAKE_TAG(table & 0xff, table & 0xff00, table & 0xff0000, table & 0xff000000); >> >> Extra space here. > > Ops sorry. I found a bug in my simplification with FT_MAKE_TAG. I forgot to shift the bits when applying the bit mask. >>> Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:385 >>> +void FontPlatformData::setOrientation(FontOrientation orientation) >> >> Is this ever called for us? The only place I see this called is in SimpleFontData::verticalRightOrientationFontData, but that appears to only be called by the Mac port. > > The code changed quite significantly since the initial patch, in particular after the Chromium code clean up. I will re-test this function and will double check the use cases. SimpleFontData::verticalRightOrientationFontData() is used by glyphDataAndPageForNonCJKCharacterWithGlyphOrientation() when displaying non-CJK characters in vertical mode, which should stay rotated 90 degrees. >> Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:402 >> + setCairoFontOptionsFromFontConfigPattern(options, m_pattern.get()); > > Not sure I understand why this is called again. Good catch, this call is not needed. I'll remove it. >>> Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:98 >>> + >> >> This looks like a lot of repetition, so why not add two small helpers, heightFromTextExtentsAndOrientation and widthFromTextExtentsAndOrientation. > > Ok I'll add them. I'll simplify this code and helpers won't be necessary. >> Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:103 >> + } > > Not sure I understand why this is necessary now... EM is used by OpenTypeVerticalData. In my tests without setting it, the space between glyphs in vertical text gets wrong.

Created attachment 216038 [details] Patch Modified patch according to review comments.

Comment on attachment 216038 [details] Patch Attachment 216038 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/21218006

Comment on attachment 216038 [details] Patch Attachment 216038 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/20998157

(In reply to comment #46) > (From update of attachment 216038 [details]) > Attachment 216038 [details] did not pass efl-ews (efl): > Output: http://webkit-queues.appspot.com/results/21218006 I just realised that graphics/opentype/OpenTypeVerticalData.cpp needs to be added to WebCore/PlatformEfl.cmake to enable this feature on EFL. Unfortunately I don't have an EFL environment to test this, and I'm wondering if I should raise a separate bug to include OpenTypeVerticalData.cpp on EFL?

(In reply to comment #48) > I just realised that graphics/opentype/OpenTypeVerticalData.cpp needs to be added to WebCore/PlatformEfl.cmake to enable this feature on EFL. > > Unfortunately I don't have an EFL environment to test this, and I'm wondering if I should raise a separate bug to include OpenTypeVerticalData.cpp on EFL? If it's just a question of adding the file to the list of source files, try doing it without testing locally and the EWS will ensure that it builds properly.

Created attachment 216051 [details] Patch Modified patch according to review comments and fixed EFL build.

Comment on attachment 216051 [details] Patch Attachment 216051 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/19478162

Comment on attachment 216051 [details] Patch Attachment 216051 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/21178052

Comment on attachment 216051 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=216051&action=review Looking pretty good. I have some stylistic concerns, but you should also try to regenerate pixel/test results for tests that use vertical writing modes. > Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:42 > +namespace { > + This anonymous namespace is unnecessary. > Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:106 > -static cairo_font_options_t* getDefaultFontOptions() > +cairo_font_options_t* getDefaultFontOptions() This should remain static. > Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:118 > +void rotateCairoMatrixForVerticalOrientation(cairo_matrix_t* matrix) This should be static. > Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:173 > - cairo_glyph_t cglyph = { glyph, 0, 0 }; > - cairo_text_extents_t extents; > - cairo_scaled_font_glyph_extents(m_platformData.scaledFont(), &cglyph, 1, &extents); > + float w = 0.0; > + if (cairo_scaled_font_status(m_platformData.scaledFont()) == CAIRO_STATUS_SUCCESS) { > + cairo_glyph_t cglyph = { glyph, 0, 0 }; > + cairo_text_extents_t extents; > + cairo_scaled_font_glyph_extents(m_platformData.scaledFont(), &cglyph, 1, &extents); > + w = static_cast<float>(platformData().orientation() == Horizontal ? extents.x_advance : -extents.y_advance); > + } > > - float w = (float)m_spaceWidth; > - if (cairo_scaled_font_status(m_platformData.scaledFont()) == CAIRO_STATUS_SUCCESS && extents.x_advance) > - w = (float)extents.x_advance; > + if (!static_cast<unsigned>(w)) > + w = static_cast<float>(m_spaceWidth); > Please use early returns, avoid casting, and abbreviated variable names. For instance: if (cairo_scaled_font_status(m_platformData.scaledFont()) != CAIRO_STATUS_SUCCESS) return m_spaceWidth; cairo_glyph_t cairoGlyph = { glyph, 0, 0 }; cairo_text_extents_t extents; cairo_scaled_font_glyph_extents(m_platformData.scaledFont(), &cairoGlyph, 1, &extents); float width = platformData().orientation() == Horizontal ? extents.x_advance : -extents.y_advance; return width ? width : m_spaceWidth; > Source/WebCore/platform/graphics/harfbuzz/HarfBuzzFaceCairo.cpp:92 > + // If no advance in x, assume font has vertical ghyphs. ghyphs -> glyphs > Source/WebCore/platform/graphics/harfbuzz/HarfBuzzFaceCairo.cpp:101 > - *advance = doubleToHarfBuzzPosition(glyphExtents.x_advance); > + *advance = doubleToHarfBuzzPosition(static_cast<unsigned>(glyphExtents.x_advance) ? glyphExtents.x_advance : -glyphExtents.y_advance); > + > if (extents) { > extents->x_bearing = doubleToHarfBuzzPosition(glyphExtents.x_bearing); > - extents->y_bearing = doubleToHarfBuzzPosition(glyphExtents.y_bearing); > - extents->width = doubleToHarfBuzzPosition(glyphExtents.width); > - extents->height = doubleToHarfBuzzPosition(glyphExtents.height); > + extents->y_bearing = doubleToHarfBuzzPosition(static_cast<unsigned>(glyphExtents.x_advance) ? glyphExtents.y_bearing : -glyphExtents.y_bearing); > + extents->width = doubleToHarfBuzzPosition(static_cast<unsigned>(glyphExtents.x_advance) ? glyphExtents.width : -glyphExtents.height); > + extents->height = doubleToHarfBuzzPosition(static_cast<unsigned>(glyphExtents.x_advance) ? glyphExtents.height : glyphExtents.width); > } Certainly these casts can be avoided? It's probably best to save the results of the test into a boolean: bool hasVerticalGlyphs = glyphExtents.x_advance; This will allow you to remove the comment too.

Comment on attachment 216051 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=216051&action=review Thanks again :) My comments below. I noticed EFL still doesn't compile but I think know the problem. And I'll regenerate the test results. >> Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:42 >> + > > This anonymous namespace is unnecessary. Ok. I will remove it. >> Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:106 >> +cairo_font_options_t* getDefaultFontOptions() > > This should remain static. Ok >> Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:118 >> +void rotateCairoMatrixForVerticalOrientation(cairo_matrix_t* matrix) > > This should be static. Ok >> Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:173 >> > > Please use early returns, avoid casting, and abbreviated variable names. For instance: > > if (cairo_scaled_font_status(m_platformData.scaledFont()) != CAIRO_STATUS_SUCCESS) > return m_spaceWidth; > > cairo_glyph_t cairoGlyph = { glyph, 0, 0 }; > cairo_text_extents_t extents; > cairo_scaled_font_glyph_extents(m_platformData.scaledFont(), &cairoGlyph, 1, &extents); > float width = platformData().orientation() == Horizontal ? extents.x_advance : -extents.y_advance; > return width ? width : m_spaceWidth; Just beautiful! I'll change it. >> Source/WebCore/platform/graphics/harfbuzz/HarfBuzzFaceCairo.cpp:92 >> + // If no advance in x, assume font has vertical ghyphs. > > ghyphs -> glyphs Ops >> Source/WebCore/platform/graphics/harfbuzz/HarfBuzzFaceCairo.cpp:101 >> } > > Certainly these casts can be avoided? It's probably best to save the results of the test into a boolean: > > bool hasVerticalGlyphs = glyphExtents.x_advance; > > This will allow you to remove the comment too. Unfortunately there are corner cases where cairo_scaled_font_glyph_extents() returns x_advance with values like 3.0e-16 instead of zero, and they aren't converted into zero without casting. To void this problem, I will use y_advance instead, which has not shown this problem.

(In reply to comment #54) > (From update of attachment 216051 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=216051&action=review > > > bool hasVerticalGlyphs = glyphExtents.x_advance; > > > > This will allow you to remove the comment too. > > Unfortunately there are corner cases where cairo_scaled_font_glyph_extents() returns x_advance with values like 3.0e-16 instead of zero, and they aren't converted into zero without casting. > To void this problem, I will use y_advance instead, which has not shown this problem. Yikes! Interesting problem. Even if you use y_advance it might be good to make the test something like bool hasVerticalGlyphs = glyphExtents.x_advance > epsilon;

Created attachment 216222 [details] Patch Modified according to review comments. Layout tests are too big and will be uploaded separately.

Created attachment 216223 [details] Layout tests - part 1

Created attachment 216224 [details] Layout tests - part 2

Comment on attachment 216222 [details] Patch Great!

(In reply to comment #59) > (From update of attachment 216222 [details]) > Great! Brilliant! Thank you very much. This bug has been epic :) I'll miss it.

(In reply to comment #60) > (In reply to comment #59) > > (From update of attachment 216222 [details] [details]) > > Great! > > Brilliant! Thank you very much. This bug has been epic :) I'll miss it. As agreed with Denis and the WebKitEFL guys in webkit-efl, I'll help landing this manually (Denis is not a committer) and we'll monitor the bots closely to spot any issues. Hopefully, it will also be a matter of generating new baselines for EFL, something Gyuyoung already volunteered to help with (thanks!).

(In reply to comment #61) > (In reply to comment #60) > > (In reply to comment #59) > > > (From update of attachment 216222 [details] [details] [details]) > > > Great! > > > > Brilliant! Thank you very much. This bug has been epic :) I'll miss it. > > As agreed with Denis and the WebKitEFL guys in webkit-efl, I'll help landing > this manually (Denis is not a committer) and we'll monitor the bots closely to > spot any issues. > > Hopefully, it will also be a matter of generating new baselines for EFL, > something Gyuyoung already volunteered to help with (thanks!). It's landed: http://trac.webkit.org/changeset/158848 Now I'll update bug 123985 and ping EFL guys to let them know about it