Bug 64767 – REGRESSION(91209?): fast/css/custom-font-xheight.html is failing on Leopard

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

(In reply to comment #2)
> Working on a fix.

Thanks!

Committed r91229.

[James Robinson]

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.

[Ryosuke Niwa]

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

[Ryosuke Niwa]

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!

[Ryosuke Niwa]

Created attachment 101259 [details]
fixes the bug for Chromium and all

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

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));

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Created attachment 101265 [details]
Fixed per comments

[Ryosuke Niwa]

Created attachment 101266 [details]
Fixed per comments

[Ryosuke Niwa]

Comment on attachment 101266 [details]
Fixed per comments

Ugh... another mistake :(

[Ryosuke Niwa]

Created attachment 101267 [details]
More fixes

[Ryosuke Niwa]

Comment on attachment 101267 [details]
More fixes

Clearing flags on attachment: 101267

Committed r91237: <http://trac.webkit.org/changeset/91237>

[Ryosuke Niwa]

All reviewed patches have been landed.  Closing bug.