Bug 83512

Created attachment 136312 [details] set FontMetrics.unitsPerEm on Chromium Windows

LGTM. Tony, could you do formal review?

Note that I searched for unitsPerEm() within chrome.sln. All usages of unitsPerEm() are either Mac, CGWin, or ENABLE(SVG_FONTS), and all such source files are under _excluded_files in the sln.

Comment on attachment 136312 [details] set FontMetrics.unitsPerEm on Chromium Windows What is the purpose if there is no behavior change? Please state in your ChangeLog.

Koji, Is unitsperem all that is needed in SkTypeface to switch over to using Skia for metrics (something we really want to do, so we can change Skia's backend to directwrite)?

(In reply to comment #5) > Koji, > > Is unitsperem all that is needed in SkTypeface to switch over to using Skia for metrics (something we really want to do, so we can change Skia's backend to directwrite)? Hi Mike, Unfortunately, no. There are bunch of code in SimpleFontDataChromiumWin.cpp and FontPlatformDataChromiumWin.cpp that use GDI for font metrics as you might know, and I'm adding a couple of more because functions I need for 51450 are missing in Skia Win today. The reason I put this bug was that, my on-going patches for 51450 and 69282 require unitsPerEm, neither Chromium Windows nor Skia sets it today, and I thought having a separate bug from 51450 works better for you guys to be aware that I'm adding more GDI code. As it goes, 51450 has more GDI code than this bug that I'm talking with bashi@google.com to resolve this one as INVALID and merge this patch to 51450. It sounds great to me that that you're planning to switch over to DirectWrite. Do you have a bug for these GDI font code to move to Skia? If so, and if the bug is going to land earlier than this/51450, I'm happy to add the functions to Skia Win instead of these files. Another function I need is to get OpenType table, which is available for Linux version of Skia but not for Windows, so I'm trying to add it in 51450. If it's going to take a bit more, I might resolve this one as INVALID and merge with 51450, then you guys can look into current code plus 51450 to see which functions you need to move to Skia. Please let me know your preferences and I'm very happy to help you. Another thing to keep in mind is whether you're supporting old Windows that don't run DirectWrite or not. If you do, then you need a run-time switch between GDI and DirectWrite, and you'll need GDI code anyway, and it might make sense for me to keep adding GDI code and for you to move them to Skia.

http://code.google.com/p/chromium/issues/detail?id=124406 1. We are a ways from DW support, so I wouldn't wait for it if you have a quick fix for something now. 2. We will support runtime switching between GDI and DW backends (in skia) Please use the above crbug to list any missing APIs in Skia that you identify (ala unitsperem)

After discussing with Chrome guys, I think it's more appropriate to merge this fix into bug 51450, and therefore I'm resolving this as INVALID.

Given the review of bug 51450 by Tony, it was recommended to split the patch for the code that is not behind #if ENABLE(OPENTYPE_VERTICAL). This bug was originally entered for that purpose, so re-opening, and changed title to reflect what I'm going to do better.

Created attachment 159696 [details] Implement FontMetrics.unitsPerEm(), FontPlatformData.orientation(), SimpleFontData::platformBoundsForGlyph for Chromium Win

Comment on attachment 159696 [details] Implement FontMetrics.unitsPerEm(), FontPlatformData.orientation(), SimpleFontData::platformBoundsForGlyph for Chromium Win View in context: https://bugs.webkit.org/attachment.cgi?id=159696&action=review The code change seems fine. Did you mean to set r? for this patch? > Source/WebCore/ChangeLog:11 > + 1. FontMetrics.unitsPerEm() always returns the default value (gDefaultUnitsPerEm = 1000). > + 2. FontPlatformData.orientation() always returns Horizontal. > + 3. SimpleFontData::platformBoundsForGlyph() always returns FloatRect(). It would be good to note what each of these changes does. For example, I assume (2) does nothing right now (doesn't change any layout tests). (1) and (3) probably causes some text to change in size, but it would be good to mention which tests demonstrate this.

Created attachment 159802 [details] Updated ChangeLog

(In reply to comment #11) > (From update of attachment 159696 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=159696&action=review > It would be good to note what each of these changes does. For example, I assume (2) does nothing right now (doesn't change any layout tests). (1) and (3) probably causes some text to change in size, but it would be good to mention which tests demonstrate this. Thank you again, added.

Comment on attachment 159802 [details] Updated ChangeLog Clearing flags on attachment: 159802 Committed r126312: <http://trac.webkit.org/changeset/126312>

All reviewed patches have been landed. Closing bug.