Bug 23028 – Enable web/dynamic font support for Chromium on Windows

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Jungshik Shin]

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.

[Jungshik Shin]

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'.

[Jungshik Shin]

Created attachment 26683 [details]
updated patch 

I missed one '#if' line in the previous patch (allClientsRemoved in CachedFont.cpp)

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?

[Jungshik Shin]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Comment on attachment 27120 [details]
patch update (with UInt8 replaced by uint8_t)

patch was malformed.

[Jungshik Shin]

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. 

[Jungshik Shin]

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.

[Jungshik Shin]

Created attachment 27476 [details]
patch part 2 (to be applied with patch after applying part 1)

[Jungshik Shin]

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

[Jungshik Shin]

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.

[Eric Seidel (no email)]

Comment on attachment 27813 [details]
patch updated (vcproj file updated to the trunk) part 1

Still looks fine.

[Eric Seidel (no email)]

Comment on attachment 27476 [details]
patch part 2 (to be applied with patch after applying part 1)

Still looks fine.  Where are the ChangeLogs?

[Jungshik Shin]

(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? 

[Jungshik Shin]

(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).  

[Eric Seidel (no email)]

(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.

[Jungshik Shin]

Thank you, Eric. Then, I'll not upload a new set of patches. 

[Jungshik Shin]

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.

[Jungshik Shin]

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].

[Dimitri Glazkov (Google)]

Landed as http://trac.webkit.org/changeset/41390.

[Jungshik Shin]

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.

[Dimitri Glazkov (Google)]

Missing files landed as http://trac.webkit.org/changeset/41402.