[Webkit-unassigned] [Bug 50619] Glyphs in vertical text tests are rotated 90 degrees clockwise on WebKitGtk

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=50619





--- Comment #9 from Koan-Sin Tan <koansin.tan at gmail.com>  2010-12-07 18:56:13 PST ---

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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.