Bug 64767 - REGRESSION(91209?): fast/css/custom-font-xheight.html is failing on Leopard
Summary: REGRESSION(91209?): fast/css/custom-font-xheight.html is failing on Leopard
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-18 16:52 PDT by Ryosuke Niwa
Modified: 2011-07-18 21:14 PDT (History)
3 users (show)

See Also:


Attachments
fixes the bug for Chromium and all (2.45 KB, patch)
2011-07-18 19:31 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fixed per comments (2.52 KB, patch)
2011-07-18 20:40 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fixed per comments (2.52 KB, patch)
2011-07-18 20:46 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
More fixes (2.48 KB, patch)
2011-07-18 20:56 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2011-07-18 16:52:07 PDT
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.
Comment 1 mitz 2011-07-18 17:29:16 PDT
I will build on Leopard and see if I can reproduce any of the failures.
Comment 2 mitz 2011-07-18 17:52:39 PDT
Working on a fix.
Comment 3 Ryosuke Niwa 2011-07-18 17:58:28 PDT
(In reply to comment #2)
> Working on a fix.

Thanks!
Comment 4 mitz 2011-07-18 18:21:28 PDT
Committed r91229.
Comment 5 James Robinson 2011-07-18 18:24:58 PDT
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.
Comment 6 mitz 2011-07-18 18:27:47 PDT
Then you will probably need a runtime check.
Comment 7 Ryosuke Niwa 2011-07-18 18:34:24 PDT
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
Comment 8 Ryosuke Niwa 2011-07-18 18:43:05 PDT
Build fix landed in http://trac.webkit.org/changeset/91231.
Comment 9 mitz 2011-07-18 19:21:00 PDT
(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!
Comment 10 Ryosuke Niwa 2011-07-18 19:31:46 PDT
Created attachment 101259 [details]
fixes the bug for Chromium and all
Comment 11 Ryosuke Niwa 2011-07-18 19:34:08 PDT
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.
Comment 12 Ryosuke Niwa 2011-07-18 19:38:14 PDT
I verified that this patch builds on Chromium Mac and Mac (both 10.6).
Comment 13 mitz 2011-07-18 20:13:23 PDT
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 14 Ryosuke Niwa 2011-07-18 20:21:25 PDT
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.
Comment 15 mitz 2011-07-18 20:29:43 PDT
(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 16 mitz 2011-07-18 20:30:12 PDT
Comment on attachment 101259 [details]
fixes the bug for Chromium and all

r- because it’s wrong to special-case Snow Leopard here.
Comment 17 Ryosuke Niwa 2011-07-18 20:40:09 PDT
Created attachment 101265 [details]
Fixed per comments
Comment 18 Ryosuke Niwa 2011-07-18 20:46:51 PDT
Created attachment 101266 [details]
Fixed per comments
Comment 19 Ryosuke Niwa 2011-07-18 20:47:29 PDT
Comment on attachment 101266 [details]
Fixed per comments

Ugh... another mistake :(
Comment 20 Ryosuke Niwa 2011-07-18 20:56:01 PDT
Created attachment 101267 [details]
More fixes
Comment 21 Ryosuke Niwa 2011-07-18 21:14:44 PDT
Comment on attachment 101267 [details]
More fixes

Clearing flags on attachment: 101267

Committed r91237: <http://trac.webkit.org/changeset/91237>
Comment 22 Ryosuke Niwa 2011-07-18 21:14:49 PDT
All reviewed patches have been landed.  Closing bug.