Bug 83512 - FontMetrics.unitsPerEm(), FontPlatformData.orientation(), SimpleFontData::platformBoundsForGlyph are not implemented on Chromium Windows
Summary: FontMetrics.unitsPerEm(), FontPlatformData.orientation(), SimpleFontData::pla...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 94735 51450
  Show dependency treegraph
 
Reported: 2012-04-09 14:58 PDT by Koji Ishii
Modified: 2012-08-22 11:30 PDT (History)
7 users (show)

See Also:


Attachments
set FontMetrics.unitsPerEm on Chromium Windows (1.42 KB, patch)
2012-04-09 15:05 PDT, Koji Ishii
eric: review-
Details | Formatted Diff | Diff
Implement FontMetrics.unitsPerEm(), FontPlatformData.orientation(), SimpleFontData::platformBoundsForGlyph for Chromium Win (20.51 KB, patch)
2012-08-21 08:35 PDT, Koji Ishii
no flags Details | Formatted Diff | Diff
Updated ChangeLog (20.74 KB, patch)
2012-08-21 16:48 PDT, Koji Ishii
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Koji Ishii 2012-04-09 14:58:17 PDT
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.)
Comment 1 Koji Ishii 2012-04-09 15:05:38 PDT
Created attachment 136312 [details]
set FontMetrics.unitsPerEm on Chromium Windows
Comment 2 Kenichi Ishibashi 2012-04-09 19:34:04 PDT
LGTM. Tony, could you do formal review?
Comment 3 Koji Ishii 2012-04-10 03:14:00 PDT
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 4 Eric Seidel (no email) 2012-04-19 16:00:45 PDT
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.
Comment 5 Mike Reed 2012-04-20 07:13:49 PDT
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)?
Comment 6 Koji Ishii 2012-04-20 08:52:51 PDT
(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.
Comment 7 Mike Reed 2012-04-20 09:15:08 PDT
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)
Comment 8 Koji Ishii 2012-04-25 05:44:37 PDT
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.
Comment 9 Koji Ishii 2012-08-21 06:22:49 PDT
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.
Comment 10 Koji Ishii 2012-08-21 08:35:02 PDT
Created attachment 159696 [details]
Implement FontMetrics.unitsPerEm(), FontPlatformData.orientation(), SimpleFontData::platformBoundsForGlyph for Chromium Win
Comment 11 Tony Chang 2012-08-21 13:25:59 PDT
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.
Comment 12 Koji Ishii 2012-08-21 16:48:30 PDT
Created attachment 159802 [details]
Updated ChangeLog
Comment 13 Koji Ishii 2012-08-21 16:51:27 PDT
(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 14 WebKit Review Bot 2012-08-22 09:53:29 PDT
Comment on attachment 159802 [details]
Updated ChangeLog

Clearing flags on attachment: 159802

Committed r126312: <http://trac.webkit.org/changeset/126312>
Comment 15 WebKit Review Bot 2012-08-22 09:53:33 PDT
All reviewed patches have been landed.  Closing bug.