Closed Bug 1341724 Opened 8 years ago Closed 8 years ago

stylo: Add proper support for the "ch" unit and other font metrics

Categories

(Core :: CSS Parsing and Computation, defect, P1)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: bzbarsky, Assigned: manishearth)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

Bug 1341724 - Part 1: stylo: Refactor nsRuleNode::GetMetricsFor;
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1341724 - Part 2: stylo: Add bindings for fetching font metrics from Gecko;
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1341724 - Part 3: stylo: Use gecko's font metrics;
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1341724 - Part 4: stylo: Make font metrics usage threadsafe;
59 bytes, text/x-review-board-request
heycam
: review+
Details
followup - Update reftest expectations
28.99 KB, patch
Details | Diff | Splinter Review
This is causing reftest failures.
This basically requires font metrics, which we've discussed a bit in various places [1] [2]. A lot of uncertainty, so I think this should be P1. Over to Manish. [1] https://github.com/servo/servo/issues/14079 [2] https://groups.google.com/forum/#!topic/mozilla.dev.tech.layout/1Na7hdynWpg
Assignee: nobody → manishearth
Priority: -- → P1
Summary: stylo: Add proper support for the "ch" unit → stylo: Add proper support for the "ch" unit and other font metrics
Status: NEW → ASSIGNED
Comment on attachment 8856196 [details] Bug 1341724 - Part 1: stylo: Refactor nsRuleNode::GetMetricsFor; https://reviewboard.mozilla.org/r/128138/#review130624 ::: layout/style/nsRuleNode.h:32 (Diff revision 1) > class nsStyleCoord; > struct nsCSSRect; > struct nsCSSValueList; > struct nsCSSValuePairList; > struct nsRuleData; > +class nsFontMetrics; Nit: this list of forward declarations appears to be sorted (including the "class" and "struct"), so let's move it up a bit. ::: layout/style/nsRuleNode.cpp:408 (Diff revision 1) > + WritingMode wm(aStyleContext); > + if (wm.IsVertical() && !wm.IsSideways()) { > + isVertical = true; > + } > + } > + return nsRuleNode::GetMetricsFor(aPresContext, isVertical, aStyleFont, aFontSize, aUseUserFontSet); Nit: please keep to 80 columns. ::: layout/style/nsRuleNode.cpp:382 (Diff revision 2) > - nscoord aFontSize, // overrides value from aStyleFont > + nscoord aFontSize, > - bool aUseUserFontSet) > + bool aUseUserFontSet) > { > nsFont font = aStyleFont->mFont; > font.size = aFontSize; > - gfxFont::Orientation orientation = gfxFont::eHorizontal; > + gfxFont::Orientation orientation = aIsVertical? gfxFont::eVertical : gfxFont::eHorizontal; Nit: Put a space after the "?". Also please keep to 80 columns.
Attachment #8856196 - Flags: review?(cam) → review+
Depends on: 1351200
Comment on attachment 8856197 [details] Bug 1341724 - Part 2: stylo: Add bindings for fetching font metrics from Gecko; https://reviewboard.mozilla.org/r/128140/#review130626 r- for the FlushUserFontSet stuff. Not sure how we should solve that yet... ::: layout/style/ServoBindings.h:386 (Diff revision 1) > void Gecko_nsStyleFont_SetLang(nsStyleFont* font, nsIAtom* atom); > void Gecko_nsStyleFont_CopyLangFrom(nsStyleFont* aFont, const nsStyleFont* aSource); > FontSizePrefs Gecko_GetBaseSize(nsIAtom* lang); > > + > +struct GeckoFontMetrics { Nit: "{" on new line. And drop the blank line before it. (I don't see any other sectioning of declarations in this file using two blank lines.) ::: layout/style/ServoBindings.cpp:1552 (Diff revision 1) > sizes.mDefaultCursiveSize = prefs.mDefaultCursiveFont.size; > sizes.mDefaultFantasySize = prefs.mDefaultFantasyFont.size; > return sizes; > } > > +static Mutex sServoFontMetricsLock("Gecko_GetFontMetrics"); We shouldn't declare static variables whose initialization requires running code. (I think static analysis builds will flag this?) Instead, make this: static Mutex* sServoFontMetricsLock = nullptr; and then create the Mutex under nsLayoutStatics::Initialize, and destroy it under nsLayoutStatics::Shutdown. ::: layout/style/ServoBindings.cpp:1563 (Diff revision 1) > + // Safe because we are locked, and this function is only > + // ever called from Servo parallel traversal Please assert that we are in a traversal. ::: layout/style/ServoBindings.cpp:1566 (Diff revision 1) > + MutexAutoLock lock(sServoFontMetricsLock); > + GeckoFontMetrics ret; > + // Safe because we are locked, and this function is only > + // ever called from Servo parallel traversal > + nsPresContext* presContext = const_cast<nsPresContext*>(aPresContext); > + RefPtr<nsFontMetrics> fm = nsRuleNode::GetMetricsFor(presContext, aIsVertical, aFont, aFontSize, aUseUserFontSet); Nit: please keep to 80 columns. :-) ::: layout/style/ServoBindings.cpp:1567 (Diff revision 1) > + GeckoFontMetrics ret; > + // Safe because we are locked, and this function is only > + // ever called from Servo parallel traversal > + nsPresContext* presContext = const_cast<nsPresContext*>(aPresContext); > + RefPtr<nsFontMetrics> fm = nsRuleNode::GetMetricsFor(presContext, aIsVertical, aFont, aFontSize, aUseUserFontSet); > + ret.mXSize = float(fm->XHeight()); Why cast this to float? Both ret.mXSize and fm->XHeight() are nscoord. ::: layout/style/ServoBindings.cpp:1566 (Diff revision 2) > + MutexAutoLock lock(sServoFontMetricsLock); > + GeckoFontMetrics ret; > + // Safe because we are locked, and this function is only > + // ever called from Servo parallel traversal > + nsPresContext* presContext = const_cast<nsPresContext*>(aPresContext); > + RefPtr<nsFontMetrics> fm = nsRuleNode::GetMetricsFor(presContext, aIsVertical, aFont, aFontSize, aUseUserFontSet); Nit: please keep to 80 columns. Also, GetMetricsFor can eventually call into nsDocument::FlushUserFontSet, which I'm not convinced will like to run off the main thread, even with the lock here. (It interacts with DOM objects.)
Attachment #8856197 - Flags: review?(cam) → review-
Comment on attachment 8856198 [details] Bug 1341724 - Part 3: stylo: Use gecko's font metrics; https://reviewboard.mozilla.org/r/128142/#review130628 ::: servo/components/style/font_metrics.rs:17 (Diff revision 1) > /// Represents the font metrics that style needs from a font to compute the > /// value of certain CSS units like `ex`. > #[derive(Debug, PartialEq, Clone)] > pub struct FontMetrics { > /// The x-height of the font. > pub x_height: Au, > /// The zero advance. > - pub zero_advance_measure: Size2D<Au>, > + pub zero_advance_measure: Au, > } Might be worth mentioning in the comment that this FontMetrics is writing mode dependent, since zero_advance_measure could be (and usually is) different for horizontal and vertical text. ::: servo/components/style/gecko/wrapper.rs:483 (Diff revision 1) > + let gecko_metrics = unsafe { > + Gecko_GetFontMetrics(&*device.pres_context, > + wm.is_vertical() && !wm.is_sideways(), > + font.gecko(), > + font_size.0, > + in_media_query) Shouldn't this be "!in_media_query"? Please add a comment explaining why we're passing that in as the value of use_user_font_set.
Attachment #8856198 - Flags: review?(cam) → review+
Comment on attachment 8856199 [details] Bug 1341724 - Part 4: stylo: Make font metrics usage threadsafe; https://reviewboard.mozilla.org/r/128144/#review130630 Might be worth asserting we're on the main thread the destructors of the classes you're adding threadsafe refcounting too, with a message saying that style worker threads should never release these objects down to refcount 0? ::: gfx/src/nsDeviceContext.h:59 (Diff revision 1) > * @return error status > */ > nsresult Init(nsIWidget *aWidget); > > + /* > + * Initialize the font cache if it hasn't been initialized yet. Mention here that we need this for stylo. ::: gfx/src/nsDeviceContext.cpp:218 (Diff revision 1) > { > if (!mFontCache) { > mFontCache = new nsFontCache(); > mFontCache->Init(this); > } > - > +} Nit: blank line after this. ::: layout/style/ServoStyleSet.cpp:210 (Diff revision 1) > + mPresContext->DeviceContext()->InitFontCache(); > + gfxPlatformFontList::PlatformFontList()->InitLangService(); > + Can we do this in ServoStyleSet::Init instead?
Attachment #8856199 - Flags: review?(cam) → review+
Comment on attachment 8856197 [details] Bug 1341724 - Part 2: stylo: Add bindings for fetching font metrics from Gecko; https://reviewboard.mozilla.org/r/128140/#review130652 Also per IRC: <heycam> Manishearth: oh, one comment I forgot to add on that bug: CalcLengthWith calls nsPresContext::SetUsesExChUnits so that we know to restyle things when the FontFaceSet is updated (e.g. @font-face rules finish downloading) <heycam> we need to do that here/somewhere too Looks good apart from these issues. ::: layout/style/ServoBindings.cpp:1552 (Diff revision 3) > sizes.mDefaultCursiveSize = prefs.mDefaultCursiveFont.size; > sizes.mDefaultFantasySize = prefs.mDefaultFantasyFont.size; > return sizes; > } > > +static Mutex* sServoFontMetricsLock = nullptr;; Nit: one too many ";"s. ::: layout/style/ServoBindings.cpp:1582 (Diff revision 3) > + GeckoFontMetrics ret; > + // Safe because we are locked, and this function is only > + // ever called from Servo parallel traversal > + MOZ_ASSERT(ServoStyleSet::IsInServoTraversal()); > + nsPresContext* presContext = const_cast<nsPresContext*>(aPresContext); > + RefPtr<nsFontMetrics> fm= nsRuleNode::GetMetricsFor(presContext, aIsVertical, Nit: space before "=". But also, per the previous review comment, we have to do something about the fact that we can call into nsIDocument::FlushUserFontSet. Per IRC conversation, maybe it's sufficient to call that up front in PreTraverseSync? But I'm not sure what the performance implications of doing that on every restyle is, even if we didn't need to look up font metrics.
Attachment #8856197 - Flags: review?(cam) → review-
Blocks: stylo
(Reminder to myself to review part 4.)
Flags: needinfo?(cam)
Comment on attachment 8856197 [details] Bug 1341724 - Part 2: stylo: Add bindings for fetching font metrics from Gecko; https://reviewboard.mozilla.org/r/128140/#review130778
Attachment #8856197 - Flags: review?(cam) → review+
Comment on attachment 8856199 [details] Bug 1341724 - Part 4: stylo: Make font metrics usage threadsafe; https://reviewboard.mozilla.org/r/128144/#review130782 ::: gfx/thebes/gfxFcPlatformFontList.cpp:1070 (Diff revision 7) > if (skipped) { > aFontEntryList.TruncateLength(aFontEntryList.Length() - skipped); > } > } > > +/*virtual*/ Very small nit: we usually put spaces around the keyword, so "/* virtual */".
Flags: needinfo?(cam)
Pushed by manishearth@gmail.com: https://hg.mozilla.org/integration/autoland/rev/4f2dc1320f30 Part 1: stylo: Refactor nsRuleNode::GetMetricsFor; r=heycam https://hg.mozilla.org/integration/autoland/rev/c391d9514d9a Part 2: stylo: Add bindings for fetching font metrics from Gecko; r=heycam https://hg.mozilla.org/integration/autoland/rev/846fa6a6196e Part 3: stylo: Use gecko's font metrics; r=heycam https://hg.mozilla.org/integration/autoland/rev/4d87363051a3 Part 4: stylo: Make font metrics usage threadsafe; r=heycam
Pushed by kwierso@gmail.com: https://hg.mozilla.org/mozilla-central/rev/d7b81b700d75 followup - Adjust reftest expectations. a=merge
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: