Summary: | 1ex should equal .5em when the font has no x-height metric | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tab Atkins <tabatkins> | ||||||||
Component: | CSS | Assignee: | Tab Atkins <tabatkins> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | ap, cmarcelo, darin, davidbarr, efidler, eric, hyatt, macpherson, menard, mitz, senorblanco, thorton, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Tab Atkins
2012-03-05 17:24:42 PST
Created attachment 130249 [details]
Patch
The above patch definitely works with cr-linux, but I'm unsure of what the other platforms return from their various font libraries. I need to see bot results before I can be sure that my patch is complete. :/ Comment on attachment 130249 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=130249&action=review > Source/WebCore/platform/wx/wxcode/win/fontprops.cpp:61 > - m_xHeight = m_ascent * 0.56f; // Best guess for xHeight for non-Truetype fonts. > - m_lineGap = lroundf(tm.tmExternalLeading); > + m_xHeight = m_ascent * 0.56f; // Best guess for xHeight for non-Truetype fonts.0;lroundf(tm.tmExternalLeading); Did you mean to remove the assignment to m_lineGap and put its associated computation at the end of a comment? ...huh. I *thought* that comment looked weird. I have no idea how I managed to accidentally do that. I'm about to be OOO for the week; I'll fix that and resubmit when I get back next week. Thanks! Comment on attachment 130249 [details]
Patch
R-minusing based on Tim's comment to shorten the review queue. I'm looking for your next patch ;-)
Created attachment 133724 [details]
Patch
This looks reasonable to me. Comment on attachment 133724 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133724&action=review > Source/WebCore/platform/graphics/skia/SimpleFontDataSkia.cpp:109 > + m_fontMetrics.setHasXHeight(false); You might want to explain this line here with a comment. Created attachment 167168 [details]
Patch for landing
Comment on attachment 167168 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=167168&action=review > Source/WebCore/platform/graphics/FontMetrics.h:133 > + bool m_hasXHeight; Do we care about the size of this object such that we should be stealing a bit from another member instead of adding a whole int/byte/word/whatever (In reply to comment #10) > (From update of attachment 167168 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=167168&action=review > > > Source/WebCore/platform/graphics/FontMetrics.h:133 > > + bool m_hasXHeight; > > Do we care about the size of this object such that we should be stealing a bit from another member instead of adding a whole int/byte/word/whatever I have no idea! We shouldn't have many FontMetrics objects in a page, though, so I doubt it's much of an issue (unless I'm wrong). Comment on attachment 167168 [details]
Patch for landing
We'll have one for every instance of
DataRef<StyleInheritedData> inherited;
each element which has a different value for an inherited property than it's parent will have a unique StyleInheritedData object.
I suspect this isn't a huge deal. But I do suspect it will have a small memory effect. The solution here is for someone to look at the overall memory usage of the style tree and make it smaller if needed.
I don't think that needs to hold up this patch however.
Comment on attachment 167168 [details] Patch for landing Clearing flags on attachment: 167168 Committed r130437: <http://trac.webkit.org/changeset/130437> All reviewed patches have been landed. Closing bug. (In reply to comment #0) > Because the SVG font doesn't define an x-height metric, the 'ex' unit for elements that use it should be sized to half the 'em' unit. Where does that idea come from? A specification? (In reply to comment #15) > (In reply to comment #0) > > Because the SVG font doesn't define an x-height metric, the 'ex' unit for elements that use it should be sized to half the 'em' unit. > > Where does that idea come from? A specification? Yes: http://dev.w3.org/csswg/css3-values/#ex-unit |