Bug 94822 – OPENTYPE_VERTICAL support for Chromium Win

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Koji Ishii]

This is a split patch from bug 51450. As suggested by Tony in comment #50, this bug covers:
> 2) Land the code needed that is behind #if ENABLE(OPENTYPE_VERTICAL).
portion of the patch.

[Koji Ishii]

Created attachment 160187 [details]
OPENTYPE_VERTICAL support for Chromium Win

[Koji Ishii]

Tony, could you mind to review this one too?

[Tony Chang]

Comment on attachment 160187 [details]
OPENTYPE_VERTICAL support for Chromium Win

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

cq- for the changelog.  Otherwise seems fine.

At some point, we may want to add a perf test to the PerformanceTests directory.  I don't think the page cyclers actually have any pages with vertical text.

> Source/WebCore/ChangeLog:3
> +        [chromium] OPENTYPE_VERTICAL support for Chromium Win

I would remove [chromium] from the bug title since this patch modifies cross platform code.

> Source/WebCore/platform/graphics/chromium/FontChromiumWin.cpp:429
> +#if ENABLE(OPENTYPE_VERTICAL)
> +        if (verticalData) {

Nit: Checking verticalData for each glyph sounds slow. We can try it, but I bet it would be faster to have drawGlyphs check verticalData at the beginning of the function and call drawVerticalGlyphs that has all of the vertical code.

[Koji Ishii]

Created attachment 160980 [details]
Updated ChangeLog and Font::drawGlyphs in FontChromiumWin.cpp

[Koji Ishii]

Comment on attachment 160187 [details]
OPENTYPE_VERTICAL support for Chromium Win

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

>> Source/WebCore/ChangeLog:3
>> +        [chromium] OPENTYPE_VERTICAL support for Chromium Win
> 
> I would remove [chromium] from the bug title since this patch modifies cross platform code.

Fixed, thank you.

>> Source/WebCore/platform/graphics/chromium/FontChromiumWin.cpp:429
>> +        if (verticalData) {
> 
> Nit: Checking verticalData for each glyph sounds slow. We can try it, but I bet it would be faster to have drawGlyphs check verticalData at the beginning of the function and call drawVerticalGlyphs that has all of the vertical code.

Fixed. The check is actually every chunk (256 glyphs), but still you're right. I thought I could share more code but moving the if out of loop made the patch smaller.
It was marked as "nit", were I ok to fix this? Sorry for asking review against granted patch, hope it's not too much burden to you.

[Tony Chang]

Comment on attachment 160980 [details]
Updated ChangeLog and Font::drawGlyphs in FontChromiumWin.cpp

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

When I mark something as a nit, I mean you can commit with or without the change (it's more like a suggestion than a requirement).  If it's something small, then you don't need to ask for review again, although if you want me to, I don't mind reviewing again.

In this patch, since I'm setting cq+, you could make a follow up change to address the nit, but it's not a big deal either way.

> Source/WebCore/platform/graphics/chromium/FontChromiumWin.cpp:401
> +    const OpenTypeVerticalData* verticalData = font->verticalData();
> +    if (verticalData) {
> +        Vector<FloatPoint, kMaxBufferLength> translations;
> +        Vector<GOFFSET, kMaxBufferLength> offsets;

Nit: I would probably move all this code into a helper function, but this is OK too.

[WebKit Review Bot]

Comment on attachment 160980 [details]
Updated ChangeLog and Font::drawGlyphs in FontChromiumWin.cpp

Clearing flags on attachment: 160980

Committed r126907: <http://trac.webkit.org/changeset/126907>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Scott Graham]

Hi, having OpenTypeVerticalData.cpp in both webcore_remaining and webcore_platform seems to break incremental linking on Windows Chromium. Is it possible to remove it from one of those libraries?

[2734->326/3093 ~33] LINK(DLL) webkit.dll
FAILED: d:\src\depot_tools\python_bin\python.exe gyp-win-tool link-wrapper environment.x86 link.exe /nologo /IMPLIB:webkit.dll.lib /DLL /OUT:webkit.dll /PDB:webkit.dll.pdb @webkit.dll.rsp && d:\src\depot_tools\python_bin\python.exe gyp-win-tool manifest-wrapper environment.x86 mt.exe -nologo -manifest obj\third_party\WebKit\Source\WebKit\chromium\webkit.webkit.dll.intermediate.manifest -out:webkit.dll.manifest
webcore_remaining.OpenTypeVerticalData.obj : error LNK2005: "public: __thiscall WebCore::OpenTypeVerticalData::OpenTypeVerticalData(class WebCore::FontPlatformData const &)" (??0OpenTypeVerticalData@WebCore@@QAE@ABVFontPlatformData@1@@Z) already defined
in webcore_platform.OpenTypeVerticalData.obj
webcore_remaining.OpenTypeVerticalData.obj : error LNK2005: "private: void __thiscall WebCore::OpenTypeVerticalData::loadMetrics(class WebCore::FontPlatformData const &)" (?loadMetrics@OpenTypeVerticalData@WebCore@@AAEXABVFontPlatformData@2@@Z) already defined in webcore_platform.OpenTypeVerticalData.obj
webcore_remaining.OpenTypeVerticalData.obj : error LNK2005: "private: void __thiscall WebCore::OpenTypeVerticalData::loadVerticalGlyphSubstitutions(class WebCore::FontPlatformData const &)" (?loadVerticalGlyphSubstitutions@OpenTypeVerticalData@WebCore@@AAEXABVFontPlatformData@2@@Z) already defined in webcore_platform.OpenTypeVerticalData.obj
webcore_remaining.OpenTypeVerticalData.obj : error LNK2005: "public: float __thiscall WebCore::OpenTypeVerticalData::advanceHeight(class WebCore::SimpleFontData const *,unsigned short)const " (?advanceHeight@OpenTypeVerticalData@WebCore@@QBEMPBVSimpleFontData@2@G@Z) already defined in webcore_platform.OpenTypeVerticalData.obj
webcore_remaining.OpenTypeVerticalData.obj : error LNK2005: "public: void __thiscall WebCore::OpenTypeVerticalData::getVerticalTranslationsForGlyphs(class WebCore::SimpleFontData const *,unsigned short const *,unsigned int,float *)const " (?getVerticalTranslationsForGlyphs@OpenTypeVerticalData@WebCore@@QBEXPBVSimpleFontData@2@PBGIPAM@Z) already defined in webcore_platform.OpenTypeVerticalData.obj
webcore_remaining.OpenTypeVerticalData.obj : error LNK2005: "public: void __thiscall WebCore::OpenTypeVerticalData::substituteWithVerticalGlyphs(class WebCore::SimpleFontData const *,class WebCore::GlyphPage *,unsigned int,unsigned int)const " (?substituteWithVerticalGlyphs@OpenTypeVerticalData@WebCore@@QBEXPBVSimpleFontData@2@PAVGlyphPage@2@II@Z) already defined in webcore_platform.OpenTypeVerticalData.obj
webkit.dll : fatal error LNK1169: one or more multiply defined symbols found

[Tony Chang]

(In reply to comment #9)
> Hi, having OpenTypeVerticalData.cpp in both webcore_remaining and webcore_platform seems to break incremental linking on Windows Chromium. Is it possible to remove it from one of those libraries?

Looks like this was fixed in bug 95744.