Bug 83512 – FontMetrics.unitsPerEm(), FontPlatformData.orientation(), SimpleFontData::platformBoundsForGlyph are not implemented on Chromium Windows

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Koji Ishii]

As summary says, the value isn't set on Chromium platform and is always gDefaultUnitsPerEm = 1000.

It looks like the bug 75961 is the one for Skia, but Chromium Windows use Win32 API instead of Skia for the metrics and since the data is available there, I think we should set the value from Win32 on Windows.

Note that bug 51450 requires this fix (or 75961 + switching to Skia on Windows.)

[Koji Ishii]

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

[Kenichi Ishibashi]

LGTM. Tony, could you do formal review?

[Koji Ishii]

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.

[Eric Seidel (no email)]

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.

[Mike Reed]

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

[Koji Ishii]

(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 [e-mail] 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.

[Mike Reed]

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)

[Koji Ishii]

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.

[Koji Ishii]

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.

[Koji Ishii]

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

[Tony Chang]

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.

[Koji Ishii]

Created attachment 159802 [details]
Updated ChangeLog

[Koji Ishii]

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

[WebKit Review Bot]

Comment on attachment 159802 [details]
Updated ChangeLog

Clearing flags on attachment: 159802

Committed r126312: <http://trac.webkit.org/changeset/126312>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.