Bug 20531 - make alternativeFamilyName platform-dependent
Summary: make alternativeFamilyName platform-dependent
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 525.x (Safari 3.1)
Hardware: PC Windows XP
: P2 Normal
Assignee: Jungshik Shin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-08-26 15:49 PDT by Jungshik Shin
Modified: 2009-02-19 12:13 PST (History)
4 users (show)

See Also:


Attachments
patch (layout testing yet missing) (9.24 KB, patch)
2008-08-26 17:39 PDT, Jungshik Shin
eric: review-
Details | Formatted Diff | Diff
patch update (simpler) (2.25 KB, patch)
2009-01-23 12:25 PST, Jungshik Shin
eric: review+
Details | Formatted Diff | Diff
patch updated (removed CHROMIUM check) (2.23 KB, patch)
2009-02-04 14:30 PST, Jungshik Shin
jshin: review+
Details | Formatted Diff | Diff
patch for Chromium only (missed in the previous patch) (3.60 KB, patch)
2009-02-09 10:39 PST, Jungshik Shin
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jungshik Shin 2008-08-26 15:49:12 PDT
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.
Comment 1 Jungshik Shin 2008-08-26 17:39:00 PDT
Created attachment 23010 [details]
patch (layout testing yet missing)
Comment 2 Eric Seidel (no email) 2008-08-26 17:53:43 PDT
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.
Comment 3 Eric Seidel (no email) 2008-08-26 17:55:18 PDT
(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.  
Comment 4 Dimitri Glazkov (Google) 2009-01-23 10:05:29 PST
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.
Comment 5 Jungshik Shin 2009-01-23 12:25:09 PST
Created attachment 26978 [details]
patch update (simpler)

Dimitri, yes, that's exactly what I was about to do.
Comment 6 Eric Seidel (no email) 2009-02-03 15:02:23 PST
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.
Comment 7 Dave Hyatt 2009-02-03 15:18:13 PST
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.

Comment 8 Jungshik Shin 2009-02-04 14:30:58 PST
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.
Comment 9 Eric Seidel (no email) 2009-02-04 15:27:42 PST
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/platform/graphics/FontCache.cpp
Committed r40636
Comment 10 Jungshik Shin 2009-02-09 10:39:18 PST
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.
Comment 11 Jungshik Shin 2009-02-09 10:46:06 PST
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
Comment 12 Eric Seidel (no email) 2009-02-09 11:11:57 PST
Comment on attachment 27486 [details]
patch for Chromium only (missed in the previous patch)

LGTM.
Comment 13 Dimitri Glazkov (Google) 2009-02-19 12:13:36 PST
Follow-up landed as http://trac.webkit.org/changeset/41082.