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

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
P1
normal
RESOLVED FIXED
6 months ago
3 months ago

People

(Reporter: bz, Assigned: manishearth)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments)

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
(Assignee)

Updated

5 months ago
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 10

4 months ago
mozreview-review
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+
(Assignee)

Updated

4 months ago
Depends on: 1351200

Comment 11

4 months ago
mozreview-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/#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 12

4 months ago
mozreview-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 13

4 months ago
mozreview-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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 18

4 months ago
mozreview-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-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

4 months ago
Blocks: 1243581
(Reminder to myself to review part 4.)
Flags: needinfo?(cam)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 27

4 months ago
mozreview-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/#review130778
Attachment #8856197 - Flags: review?(cam) → review+

Comment 28

4 months ago
mozreview-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)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 31

4 months ago
servo bug @ https://github.com/servo/servo/pull/16333

Comment 32

4 months ago
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
Created attachment 8856489 [details] [diff] [review]
followup - Update reftest expectations
https://hg.mozilla.org/integration/autoland/rev/1ab3c4b1b127f5e62d5607bf7151ea5997a57e75
Bug 1341724 followup - Adjust reftest expectations.

Comment 35

4 months ago
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/mozilla-central/rev/d7b81b700d75
followup - Adjust reftest expectations. a=merge

Comment 36

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4f2dc1320f30
https://hg.mozilla.org/mozilla-central/rev/c391d9514d9a
https://hg.mozilla.org/mozilla-central/rev/846fa6a6196e
https://hg.mozilla.org/mozilla-central/rev/4d87363051a3
https://hg.mozilla.org/mozilla-central/rev/d7b81b700d75
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Comment 37

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1ab3c4b1b127
status-firefox54: affected → ---
You need to log in before you can comment on or make changes to this bug.