Bug 20531 – make alternativeFamilyName platform-dependent

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Jungshik Shin]

On Windows, Courier New, Times New Roman, Arial are 'guaranteed' to exist and they look good (TTF/OTFs) while 'Times', 'Courier' and 'Helvetica' are bitmap and their coverages are not so wide as the former group. Therefore, instead of having two-way mapping (as on Mac OS X), it's better to have a one-way mapping on Windows. 

Patch coming up.

[Jungshik Shin]

Created attachment 23010 [details]
patch (layout testing yet missing)

[Eric Seidel (no email)]

Comment on attachment 23010 [details]
patch (layout testing yet missing)

Hum... it seems instead of copying this function to each platform, there should just be a shared function which platforms can choose to call or not?  I assume part of this is that you're suggesting that some platforms don't want these mappings?

Or maybe this should just be a case-ignoring HashMap:

HashMap<AtomicString, AtomicString, CaseFoldingHash>

Then the "default" alternateFamilyName hash can be provided by a private function on FontCache (in FontCache.cpp) and platforms can provide their own alternateFamilyName implementation which either does a lookup in this provided HashMap, or modifies the HashMap, or just does their own lookup disregarding the HashMap.

[Eric Seidel (no email)]

(In reply to comment #2)
> (From update of attachment 23010 [details] [edit])

I guess at some level this function doesn't make any sense outside of Mac OS X.  So all of the non-mac implementations should probably be empty.  Except for the windows one.  

[Dimitri Glazkov (Google)]

Why not just use #if PLATFORM(WIN_OS) guards and keep the method in place? I can cook up a patch for this, if you'd like.

[Jungshik Shin]

Created attachment 26978 [details]
patch update (simpler)

Dimitri, yes, that's exactly what I was about to do.

[Eric Seidel (no email)]

Comment on attachment 26978 [details]
patch update (simpler)

This looks good to me.  Ideally steve or Adam would comment before I land, but as far as I can tell this is just fine.

[Dave Hyatt]

All bitmap fonts are blocked by all Windows ports so I'm not sure you need the Chromium part of the #ifdef.  These changes would be good for Safari also.

[Jungshik Shin]

Created attachment 27327 [details]
patch updated (removed CHROMIUM check)

Per David's comment, I got rid of PLATFORM(CHROMIUM) and changed the comment accordingly. I'm transferring Eric's r+ because nothing else has changed.

[Eric Seidel (no email)]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/platform/graphics/FontCache.cpp
Committed r40636

[Jungshik Shin]

Created attachment 27486 [details]
patch for Chromium only (missed in the previous patch)

I forgot to include changes necessary for platform/graphics/chromium. Chromium used to have the 1st patch to this bug (https://bugs.webkit.org/attachment.cgi?id=23010). That needs to be removed.

[Jungshik Shin]

Comment on attachment 27486 [details]
patch for Chromium only (missed in the previous patch)

This patch is pending review on the Chromium-side at 
http://codereview.chromium.org/21174

[Eric Seidel (no email)]

Comment on attachment 27486 [details]
patch for Chromium only (missed in the previous patch)

LGTM.

[Dimitri Glazkov (Google)]

Follow-up landed as http://trac.webkit.org/changeset/41082.