|
|
Created:
12 years, 6 months ago by efidler Modified:
12 years, 5 months ago CC:
skia-review_googlegroups.com, evan Base URL:
http://skia.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionAdd Windows-compatible font metrics to SkPaint::FontMetrics. This is needed to
get Windows-compatible line spacing for fonts using VDMX table metrics in
Chromium Linux and BlackBerry WebKit.
BUG=webkit:83279
Patch Set 1 #
Total comments: 4
Patch Set 2 : #
MessagesTotal messages: 14
Adding bungeman, who has familiarity with Windows font issues... https://codereview.appspot.com/5985052/diff/1/include/core/SkPaint.h File include/core/SkPaint.h (right): https://codereview.appspot.com/5985052/diff/1/include/core/SkPaint.h#newcode4 include/core/SkPaint.h:4: * Copyright (C) 2012 Research In Motion Limited. All rights reserved. I don't know what the implication of adding this copyright would be. Anyone know a good person to ask about this? https://codereview.appspot.com/5985052/diff/1/src/core/SkPaint.cpp File src/core/SkPaint.cpp (right): https://codereview.appspot.com/5985052/diff/1/src/core/SkPaint.cpp#newcode4 src/core/SkPaint.cpp:4: * Copyright (C) 2012 Research In Motion Limited. All rights reserved. another copyright change https://codereview.appspot.com/5985052/diff/1/src/ports/SkFontHost_FreeType.cpp File src/ports/SkFontHost_FreeType.cpp (right): https://codereview.appspot.com/5985052/diff/1/src/ports/SkFontHost_FreeType.c... src/ports/SkFontHost_FreeType.cpp:4: * Copyright (C) 2012 Research In Motion Limited. All rights reserved. ...and another
Sign in to reply to this message.
https://codereview.appspot.com/5985052/diff/1/include/core/SkPaint.h File include/core/SkPaint.h (right): https://codereview.appspot.com/5985052/diff/1/include/core/SkPaint.h#newcode4 include/core/SkPaint.h:4: * Copyright (C) 2012 Research In Motion Limited. All rights reserved. On 2012/04/05 19:48:01, epoger wrote: > I don't know what the implication of adding this copyright would be. Anyone > know a good person to ask about this? I think the first thing to do is make sure that someone at RIM has filled out the Corporate Contributor License Agreement: http://source.android.com/source/cla-corporate.html to make them part of the Android open source project. Once that is done, the single Android copyright should suffice, since RIM will be included as a contributor.
Sign in to reply to this message.
> On 2012/04/05 19:48:01, epoger wrote: > > I don't know what the implication of adding this copyright would be. Anyone > > know a good person to ask about this? > > I think the first thing to do is make sure that someone at RIM has filled out > the Corporate Contributor License Agreement: > http://source.android.com/source/cla-corporate.html to make them part of the > Android open source project. I don't think there's any way RIM will sign that. This change is so minimal that it should be fine to land it with no copyright change.
Sign in to reply to this message.
Please re-upload without the copyright changes then, and let's get bungeman or reed to take a look...
Sign in to reply to this message.
On 2012/04/05 19:40:04, tony wrote: I'm not sure if the reasoning on the WebKit bug is even correct. Have you tried with FreeType 2.4.6 or later (after b0962ac34e66052ccfee7996e5468f30d4bd5a72)? This change made the metrics line up with the glyphs which FreeType is actually producing. It also causes several other headaches which Skia will need to clean up. Even if we do want exactly this, all of the other FontHosts will need to be updated as well. I don't think this change is what we want though, it would be more reasonable to have the caller be able to request which kind of metrics they desire, Mac, Win, Typo, WinButTypoIfFlaged, etc and return the desired values in the existing fAscent and fDescent fields. If the user needs more than that, they probably need to just look in the tables themselves. This is actually quite an invasive change either way and I would prefer to make sure Reed gets a good look first.
Sign in to reply to this message.
Since we're doing the VDMX handling in WebKit, we explicitly need the Ascent/Descent/LineGap from the HHEA table and the WinAscent/WinDescent from the OS/2 table. I suppose we could do OS/2 parsing in WebKit, but that's a lot of duplicate work, both in code and at runtime. If Skia wants to hide all Windows metrics handling including VDMX, that's fine with me too, but I think that's potentially more invasive to Skia.
Sign in to reply to this message.
(sorry, late to the party) There are at least 3 sets of these values (unfortunately), since there are typo variants in the OS/2 table. Given this, and other misc. uses, I'd like to formally add getTable() functions to SkTypeface. Once those land, would we still need to expand SkPaint::FontMetrics?
Sign in to reply to this message.
For this use case, I need: HEAD unitsPerEm, yMin, yMax HHEA yAscender, yDescender, yLineGap OS/2 usWinAscent, usWinDescent VDMX yMin and yMax (if they exist) at the current pel size I don't need (for now, but probably soon will): OS/2 sTypoAscender, sTypoDescender, sTypoLineGap They don't necessarily need to be in the SkPaint::FontMetrics object, as long as I can get them.
Sign in to reply to this message.
Further thoughts? We're going to be forked for both Skia and WebKit until we can get this data. I'm happy to adapt our code to whatever Skia API you prefer, but I don't think I can write the whole patch myself.
Sign in to reply to this message.
https://codereview.appspot.com/6050046 My suggestion is to use these APIs directly, once that CL lands. From those you can get to whatever specific metrics you want, and tailor those querys per-platform if you wish. Do you think that will meet your needs?
Sign in to reply to this message.
I don't currently have code in WebKit to parse HEAD, HHEA and OS/2 table data. Wouldn't it make sense for Skia to have that code?
Sign in to reply to this message.
On 2012/04/17 17:17:49, efidler wrote: > I don't currently have code in WebKit to parse HEAD, HHEA and OS/2 table data. > Wouldn't it make sense for Skia to have that code? Feel free to copy (with all appropriate attributions, of course) from Skia's src/sfnt directory. This directory contains some header files with rather detailed definitions of the tables in question. Skia currently is not exposing these publicly since we don't want to have to support them if we don't have to, but they're rather well researched, at least. Since we will be exposing the table data getters, it may be possible to make the argument that these headers should be exposed.
Sign in to reply to this message.
|