Bug 23028 - Enable web/dynamic font support for Chromium on Windows
Summary: Enable web/dynamic font support for Chromium on Windows
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Jungshik Shin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-12-29 17:41 PST by Jungshik Shin
Modified: 2009-03-03 11:54 PST (History)
4 users (show)

See Also:


Attachments
patch (49.70 KB, patch)
2008-12-29 17:50 PST, Jungshik Shin
no flags Details | Formatted Diff | Diff
updated patch (50.03 KB, patch)
2009-01-13 14:32 PST, Jungshik Shin
no flags Details | Formatted Diff | Diff
patch update (with UInt8 replaced by uint8_t) (55.78 KB, patch)
2009-01-28 13:45 PST, Jungshik Shin
eric: review-
Details | Formatted Diff | Diff
patch part1 (to be applied with svn-apply) (83.00 KB, patch)
2009-02-08 22:21 PST, Jungshik Shin
no flags Details | Formatted Diff | Diff
patch part 2 (to be applied with patch after applying part 1) (3.59 KB, patch)
2009-02-08 22:22 PST, Jungshik Shin
eric: review+
Details | Formatted Diff | Diff
patch updated (vcproj file updated to the trunk) part 1 (153.10 KB, patch)
2009-02-19 16:02 PST, Jungshik Shin
eric: review+
Details | Formatted Diff | Diff
patch part 1 updated without the failure-prone part of vcproj patch. (35.87 KB, patch)
2009-03-03 09:59 PST, Jungshik Shin
jshin: review+
Details | Formatted Diff | Diff
script for part 3 (vcproj file patch) (610 bytes, text/plain)
2009-03-03 10:09 PST, Jungshik Shin
no flags Details
files missed in the latest part 1 patch (5.79 KB, patch)
2009-03-03 11:48 PST, Jungshik Shin
jshin: 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-12-29 17:41:24 PST
Currently, Chromium port (not yet in the webkit trunk) does not support web/dynamic fonts. Using OpenTypeUtilities.cpp used for Windows port, Chromium can also support dynamic fonts. 

I moved OpenTypeUtilities.{h,cpp} to platform/graphics/opentype and changed #if for web font support to include Chromium on Windows.
Comment 1 Jungshik Shin 2008-12-29 17:50:33 PST
Created attachment 26306 [details]
patch 

In addition to changes mentioned in comment #0, I changed WebCore.vcproj.

What I'm not sure about is whether I have to add an 'xcopy' statement to copy header files in a newly added directory (platform/graphics/opentype) to $WebKitBuildDirectory/include/WebCore). Because I've already added it to AdditionalIncludeDirectory list in the vcproj file, the compilation went fine without 'xcopy'.
Comment 2 Jungshik Shin 2009-01-13 14:32:30 PST
Created attachment 26683 [details]
updated patch 

I missed one '#if' line in the previous patch (allClientsRemoved in CachedFont.cpp)
Comment 3 mitz 2009-01-13 14:40:12 PST
Comment on attachment 26683 [details]
updated patch 

> +#if !PLATFORM(CG)
> +typedef unsigned char UInt8;
> +#endif

I don't think this is the best thing to do. UInt8 is not a Core Graphics type. It just happens to be defined in the ports that use Core Graphics. I think it would be better to replace all uses of UInt8 in OpenTypeUtilities with a different type. Perhaps uint8_t. What do you think?
Comment 4 Jungshik Shin 2009-01-28 13:45:55 PST
Created attachment 27120 [details]
patch update (with UInt8 replaced by uint8_t)

Sorry for the late response ( I was on vacation) and thank you for the suggestion. I like your idea, Dan (I thought about it before making the previous patch, but took the path with the smallest change :-)).  

Now that platform/graphics/chromium is in the tree, this patch includes the corresponding changes in FontCustomPlatformData.cpp there as well as in win/FontCustomPlatformData.cpp.
Comment 5 Eric Seidel (no email) 2009-02-03 15:00:12 PST
Comment on attachment 27120 [details]
patch update (with UInt8 replaced by uint8_t)

This looks like you're just moving files and replacing UInt8 with uint8_t which looks fine to me.

Someone will probably need to check that the windows WebKit build still works after landing this, since I'm not 100% sure from reading if the project file changes are correct.
Comment 6 Eric Seidel (no email) 2009-02-04 15:41:16 PST
Comment on attachment 27120 [details]
patch update (with UInt8 replaced by uint8_t)

patch was malformed.
Comment 7 Jungshik Shin 2009-02-06 13:06:56 PST
Sorry that my patch could not be applied to the trunk. It has two problems. One is that WebCore.vcproj file patch got bit-rotten. The other is that even with WebCore.vcproj patch removed (or WebCore.vcproj patch updated to ToT), for some reason, 'svn-apply' fails when I tried to apply it to another tree (which is different from where I generated the patch with 'svn-create-patch'). 

This is very puzzling. I was under the impression that 'svn-apply' can deal with a file that is 'svn-moved' and edited in a single shot (my patch has two of them, win/OpenTypeUtilities.{h,cpp} svn-moved to opentype/Open....{h,cpp} and edited in their new location). 

I'll split the patch to two and see if they work. 

Comment 8 Jungshik Shin 2009-02-08 22:21:29 PST
Created attachment 27475 [details]
patch part1 (to be applied with svn-apply)

This is  part 1 of the patch. It should be applied with svn-apply. This will move OpenTypeUtilities.{h,cpp} from their current location to platform/graphics/opentype (new location) without changing them. 

Part 2 (which will be uploaded after this one)  should be applied with 'patch' command rather than 'svn-apply' after applying part 1.
Comment 9 Jungshik Shin 2009-02-08 22:22:30 PST
Created attachment 27476 [details]
patch part 2 (to be applied with patch after applying part 1)
Comment 10 Jungshik Shin 2009-02-19 16:02:24 PST
Created attachment 27813 [details]
patch updated (vcproj file updated to the trunk) part 1

attachment 27475 [details] (vcproj file) got bit-rotten. This updates WebCore.vcproj file. Everything else remains the same. For a strange reason, I can't apply this patch on Windows (even though I made this patch on Windows) because WebCore.vcproj file patch fails (it's not due to an EOL issue). As of now, this patch can be cleanly applied with 'svn-apply' on Mac. Windows developers should be ok (I just built it on Windows).
Comment 11 Jungshik Shin 2009-02-19 16:03:46 PST
Comment on attachment 27813 [details]
patch updated (vcproj file updated to the trunk) part 1

after applying this patch, attachment 27476 [details] should be applied with 'patch -p0' to actually change OpenTypeUtilities.{h,cpp} in their new locations.
Comment 12 Eric Seidel (no email) 2009-02-19 17:01:58 PST
Comment on attachment 27813 [details]
patch updated (vcproj file updated to the trunk) part 1

Still looks fine.
Comment 13 Eric Seidel (no email) 2009-02-19 17:02:12 PST
Comment on attachment 27476 [details]
patch part 2 (to be applied with patch after applying part 1)

Still looks fine.  Where are the ChangeLogs?
Comment 14 Jungshik Shin 2009-02-23 11:48:32 PST
(In reply to comment #13)
> (From update of attachment 27476 [details] [review])
> Still looks fine.  Where are the ChangeLogs?

Thank you for the review. attachment 27476 [details] (part 2) is meant to be landed at the same time as attachment 27813 [details] (part 1). And, attachment 27813 [details] has the ChangeLogs for both. Wouldn't it work that way? 

Comment 15 Jungshik Shin 2009-02-23 11:57:26 PST
(In reply to comment #14)
> (In reply to comment #13)
> > (From update of attachment 27476 [details] [review] [review])
> > Still looks fine.  Where are the ChangeLogs?
> 
> Thank you for the review. attachment 27476 [details] [review] (part 2) is meant to be landed at
> the same time as attachment 27813 [details] [review] (part 1). And, attachment 27813 [details] [review] has the
> ChangeLogs for both. Wouldn't it work that way? 

Well, to make it easier for checking in these patches, I'll make new patches (identical except different sets of files will be in part 1 and part 2 patch with separate ChangeLogs).  

Comment 16 Eric Seidel (no email) 2009-02-23 12:01:39 PST
(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > (From update of attachment 27476 [details] [review] [review] [review])
> > > Still looks fine.  Where are the ChangeLogs?
> > 
> > Thank you for the review. attachment 27476 [details] [review] [review] (part 2) is meant to be landed at
> > the same time as attachment 27813 [details] [review] [review] (part 1). And, attachment 27813 [details] [review] [review] has the
> > ChangeLogs for both. Wouldn't it work that way? 
> 
> Well, to make it easier for checking in these patches, I'll make new patches
> (identical except different sets of files will be in part 1 and part 2 patch
> with separate ChangeLogs).  
> 

They're fine as is.  I just didn't notice the ChangeLog for some reason.
Comment 17 Jungshik Shin 2009-02-23 12:52:05 PST
Thank you, Eric. Then, I'll not upload a new set of patches. 
Comment 18 Jungshik Shin 2009-03-03 09:59:57 PST
Created attachment 28227 [details]
patch part 1 updated  without the failure-prone part of vcproj patch.

Carrying over Eric's r+ because nothing of substance has changed (only vcproj file was changed). I gave this updated patch to Dimitri off-line, but I'm uploading it here as well in case somebody tries to land it and encounters a patch failure. 

This does not have a part of WebCore.vcproj patch that tends to fail to be applied AND is hard/tedious to apply by hand. I'm gonna upload a script for that part.
Comment 19 Jungshik Shin 2009-03-03 10:09:50 PST
Created attachment 28228 [details]
script for part 3 (vcproj file patch)

This script needs to be run AFTER applying attachment 28227 [details] with svn-apply and attachment 27476 [details] with 'patch -p0' on Mac. 

A patch for adding entries to AdditionalInclude and PostBuild* tends to be bit-rotten quickly and is tedious/hard to apply by hand. This script should be less failure-prone than the patch. 

$ python vcproj.py2 < WebCore/WebCore.vcproj/WebCore.vcproj > WebCore.vcproj.new
$ mv WebCore.vcproj.new WebCore/WebCore.vcproj/WebCore.vcproj

All three parts should be landed in a single shot and ChangeLog is only included in attachment 28227 [details].
Comment 20 Dimitri Glazkov (Google) 2009-03-03 10:22:12 PST
Landed as http://trac.webkit.org/changeset/41390.
Comment 21 Jungshik Shin 2009-03-03 11:48:50 PST
Created attachment 28233 [details]
files missed in the latest part 1 patch

While making the latest part 1 patch, I forgot to include 3 files (in platform/graphics and loader). This should fix the build bustage.
Comment 22 Dimitri Glazkov (Google) 2009-03-03 11:54:06 PST
Missing files landed as http://trac.webkit.org/changeset/41402.