The following tests are failing on Chromium Leopard bots: fast/blockflow/broken-ideograph-small-caps.html = IMAGE+TEXT fast/blockflow/broken-ideographic-font.html = IMAGE fast/css/custom-font-xheight.html = TEXT In addition, fast/css/custom-font-xheight.html is failing on Apple's mac port (Leopard). I suspect this is a regression due to http://trac.webkit.org/changeset/91209.
I will build on Leopard and see if I can reproduce any of the failures.
Working on a fix.
(In reply to comment #2) > Working on a fix. Thanks!
Committed r91229.
In chromium we never set BUILDING_ON_LEOPARD, since we build the same binary for leopard and snow leopard, and rely on runtime checks. Do we need this code change for chromium-on-leopard as well? If so we'll have to convert this check to a runtime check, at least for chromium.
Then you will probably need a runtime check.
It seems like the patch broke Leopard build. /Volumes/Big/WebKit-BuildSlave/leopard-intel-debug/build/Source/WebCore/platform/graphics/cocoa/FontPlatformDataCocoa.mm: In member function 'const __CTFont* WebCore::FontPlatformData::ctFont() const': /Volumes/Big/WebKit-BuildSlave/leopard-intel-debug/build/Source/WebCore/platform/graphics/cocoa/FontPlatformDataCocoa.mm:207: error: expected `)' before ';' token
Build fix landed in http://trac.webkit.org/changeset/91231.
(In reply to comment #8) > Build fix landed in http://trac.webkit.org/changeset/91231. Sorry about that! I built and tested my fix on Leopard, but then retyped it on another machine in an attempt to expedite things :( Thank you for cleaning up after me!
Created attachment 101259 [details] fixes the bug for Chromium and all
I feel bad to duplicate code but I couldn't figure out a way to share code with hasBrokenCTFontGetVerticalTranslationsForGlyphs or OutOfProcessFontLoadingEnabled. We should figure out a way to share code between all these functions eventually.
I verified that this patch builds on Chromium Mac and Mac (both 10.6).
Comment on attachment 101259 [details] fixes the bug for Chromium and all View in context: https://bugs.webkit.org/attachment.cgi?id=101259&action=review I have a few comments but this is OK as-is. > Source/WebCore/platform/graphics/cocoa/FontPlatformDataCocoa.mm:195 > +static bool enableFontCascadeOptimization() The name is a little misleading, because this only controls the optimization for web fonts (the m_font == 0 case). > Source/WebCore/platform/graphics/cocoa/FontPlatformDataCocoa.mm:198 > + static SInt32 systemVersion = 0; No need to initialize a static to 0. > Source/WebCore/platform/graphics/cocoa/FontPlatformDataCocoa.mm:209 > +#elif defined(BUILDING_ON_SNOW_LEOPARD) > + return true; > +#else > + return false; > +#endif We usually try to order things such that the present and future codepath comes before the legacy codepath (as was in my change below). > Source/WebCore/platform/graphics/cocoa/FontPlatformDataCocoa.mm:223 > + else if (enableFontCascadeOptimization()) > m_CTFont.adoptCF(CTFontCreateWithGraphicsFont(m_cgFont.get(), m_size, 0, 0)); > -#endif > - } > + else > + m_CTFont.adoptCF(CTFontCreateWithGraphicsFont(m_cgFont.get(), m_size, 0, cascadeToLastResortFontDescriptor())); I think you can now just do something like m_CTFont.adoptCF(CTFontCreateWithGraphicsFont(m_cgFont.get(), m_size, 0, enableFontCascadeOptimization() ? cascadeToLastResortFontDescriptor() : 0));
Comment on attachment 101259 [details] fixes the bug for Chromium and all View in context: https://bugs.webkit.org/attachment.cgi?id=101259&action=review >> Source/WebCore/platform/graphics/cocoa/FontPlatformDataCocoa.mm:195 >> +static bool enableFontCascadeOptimization() > > The name is a little misleading, because this only controls the optimization for web fonts (the m_font == 0 case). Any suggestion? >> Source/WebCore/platform/graphics/cocoa/FontPlatformDataCocoa.mm:198 >> + static SInt32 systemVersion = 0; > > No need to initialize a static to 0. I copied this code from OutOfProcessFontLoadingEnabled so I'd rather keep = 0. It could be that there was some gcc bug that doesn't initialize this properly. >> Source/WebCore/platform/graphics/cocoa/FontPlatformDataCocoa.mm:209 >> +#endif > > We usually try to order things such that the present and future codepath comes before the legacy codepath (as was in my change below). I'm confused. We do return true first here for Snow Leopard, right? Or are you saying that we should put Chromium case after Mac port case? Mn... I've started to think that I should replace defined(BUILDING_ON_SNOW_LEOPARD) by !defined(BUILDING_ON_LEOPARD) >> Source/WebCore/platform/graphics/cocoa/FontPlatformDataCocoa.mm:223 >> + m_CTFont.adoptCF(CTFontCreateWithGraphicsFont(m_cgFont.get(), m_size, 0, cascadeToLastResortFontDescriptor())); > > I think you can now just do something like > m_CTFont.adoptCF(CTFontCreateWithGraphicsFont(m_cgFont.get(), m_size, 0, enableFontCascadeOptimization() ? cascadeToLastResortFontDescriptor() : 0)); Sure, that looks cleaner.
(In reply to comment #14) > (From update of attachment 101259 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=101259&action=review > > >> Source/WebCore/platform/graphics/cocoa/FontPlatformDataCocoa.mm:195 > >> +static bool enableFontCascadeOptimization() > > > > The name is a little misleading, because this only controls the optimization for web fonts (the m_font == 0 case). > > Any suggestion? canSetCascadeListForCustomFont() > > >> Source/WebCore/platform/graphics/cocoa/FontPlatformDataCocoa.mm:198 > >> + static SInt32 systemVersion = 0; > > > > No need to initialize a static to 0. > > I copied this code from OutOfProcessFontLoadingEnabled so I'd rather keep = 0. It could be that there was some gcc bug that doesn't initialize this properly. I very much doubt it, for two reasons: (1) there are many places in WebKit that rely on initialization of statics to 0 (or false) and this was never an issue; (2) Apple has never shipped a version of gcc with such a bug. > > >> Source/WebCore/platform/graphics/cocoa/FontPlatformDataCocoa.mm:209 > >> +#endif > > > > We usually try to order things such that the present and future codepath comes before the legacy codepath (as was in my change below). > > I'm confused. We do return true first here for Snow Leopard, right? Or are you saying that we should put Chromium case after Mac port case? Mn... I've started to think that I should replace defined(BUILDING_ON_SNOW_LEOPARD) by !defined(BUILDING_ON_LEOPARD) You’re right (and I missed the fact that the check was for Snow Leopard. That is the wrong thing to special-case, of course). > > >> Source/WebCore/platform/graphics/cocoa/FontPlatformDataCocoa.mm:223 > >> + m_CTFont.adoptCF(CTFontCreateWithGraphicsFont(m_cgFont.get(), m_size, 0, cascadeToLastResortFontDescriptor())); > > > > I think you can now just do something like > > m_CTFont.adoptCF(CTFontCreateWithGraphicsFont(m_cgFont.get(), m_size, 0, enableFontCascadeOptimization() ? cascadeToLastResortFontDescriptor() : 0)); > > Sure, that looks cleaner.
Comment on attachment 101259 [details] fixes the bug for Chromium and all r- because it’s wrong to special-case Snow Leopard here.
Created attachment 101265 [details] Fixed per comments
Created attachment 101266 [details] Fixed per comments
Comment on attachment 101266 [details] Fixed per comments Ugh... another mistake :(
Created attachment 101267 [details] More fixes
Comment on attachment 101267 [details] More fixes Clearing flags on attachment: 101267 Committed r91237: <http://trac.webkit.org/changeset/91237>
All reviewed patches have been landed. Closing bug.