Bug 94822

Created attachment 160187 [details] OPENTYPE_VERTICAL support for Chromium Win

Tony, could you mind to review this one too?

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.

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

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.

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.

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>

All reviewed patches have been landed. Closing bug.

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

(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.