Make nsRuleNode's use of font metrics for ch and ex units pass the correct language. (Bug 678671, patch 3) r=roc
authorL. David Baron <dbaron@dbaron.org>
Sun, 14 Aug 2011 10:08:04 -0700
changeset 75278 3a378e08192f9fcca5af00951674ee85898b0f90
parent 75277 f277fe9f70c39d760472ce223a60a78de64620a1
child 75279 83291ec2e28e44e466a819422c9850af8320b48b
push id20991
push userMs2ger@gmail.com
push dateMon, 15 Aug 2011 08:20:25 +0000
treeherdermozilla-central@e32b302039ca [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersroc
bugs678671
milestone8.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Make nsRuleNode's use of font metrics for ch and ex units pass the correct language. (Bug 678671, patch 3) r=roc This makes these users (which are exceptions within layout, although low-level enough that it won't matter for font inflation work) call through to GetMetricsFor explicitly with the correct language, rather than using the broken nsPresContext::GetMetricsFor and its charset-detected language. This improves the correctness of our behavior for 'ch' and 'ex' CSS units when the font selection (or defaults) are language-dependent. It should also reduce the number of unique sets of font metrics requested (which helps nsFontCache effectiveness).
layout/style/nsRuleNode.cpp
layout/style/nsRuleNode.h
layout/style/test/Makefile.in
layout/style/test/test_ch_ex_no_infloops.html
--- a/layout/style/nsRuleNode.cpp
+++ b/layout/style/nsRuleNode.cpp
@@ -179,77 +179,108 @@ static void EnsureBlockDisplay(PRUint8& 
     // make it a block
     display = NS_STYLE_DISPLAY_BLOCK;
   }
 }
 
 static nscoord CalcLengthWith(const nsCSSValue& aValue,
                               nscoord aFontSize,
                               const nsStyleFont* aStyleFont,
+                              const nsStyleVisibility* aStyleVisibility,
                               nsStyleContext* aStyleContext,
                               nsPresContext* aPresContext,
                               PRBool aUseProvidedRootEmSize,
                               PRBool aUseUserFontSet,
                               PRBool& aCanStoreInRuleTree);
 
 struct CalcLengthCalcOps : public css::BasicCoordCalcOps,
                            public css::NumbersAlreadyNormalizedOps
 {
   // All of the parameters to CalcLengthWith except aValue.
   const nscoord mFontSize;
   const nsStyleFont* const mStyleFont;
+  const nsStyleVisibility* const mStyleVisibility;
   nsStyleContext* const mStyleContext;
   nsPresContext* const mPresContext;
   const PRBool mUseProvidedRootEmSize;
   const PRBool mUseUserFontSet;
   PRBool& mCanStoreInRuleTree;
 
   CalcLengthCalcOps(nscoord aFontSize, const nsStyleFont* aStyleFont,
+                    const nsStyleVisibility* aStyleVisibility,
                     nsStyleContext* aStyleContext, nsPresContext* aPresContext,
                     PRBool aUseProvidedRootEmSize, PRBool aUseUserFontSet,
                     PRBool& aCanStoreInRuleTree)
     : mFontSize(aFontSize),
       mStyleFont(aStyleFont),
+      mStyleVisibility(aStyleVisibility),
       mStyleContext(aStyleContext),
       mPresContext(aPresContext),
       mUseProvidedRootEmSize(aUseProvidedRootEmSize),
       mUseUserFontSet(aUseUserFontSet),
       mCanStoreInRuleTree(aCanStoreInRuleTree)
   {
   }
 
   result_type ComputeLeafValue(const nsCSSValue& aValue)
   {
-    return CalcLengthWith(aValue, mFontSize, mStyleFont, mStyleContext,
-                          mPresContext, mUseProvidedRootEmSize,
+    return CalcLengthWith(aValue, mFontSize, mStyleFont, mStyleVisibility,
+                          mStyleContext, mPresContext, mUseProvidedRootEmSize,
                           mUseUserFontSet, mCanStoreInRuleTree);
   }
 };
 
 static inline nscoord ScaleCoord(const nsCSSValue &aValue, float factor)
 {
   return NSToCoordRoundWithClamp(aValue.GetFloatValue() * factor);
 }
 
+already_AddRefed<nsFontMetrics>
+GetMetricsFor(nsPresContext* aPresContext,
+              nsStyleContext* aStyleContext,
+              const nsStyleFont* aStyleFont,
+              const nsStyleVisibility* aStyleVisibility,
+              nscoord aFontSize, // overrides value from aStyleFont
+              PRBool aUseUserFontSet)
+{
+  nsFont font = aStyleFont->mFont;
+  font.size = aFontSize;
+  gfxUserFontSet *fs = nsnull;
+  if (aUseUserFontSet) {
+    fs = aPresContext->GetUserFontSet();
+  }
+  nsRefPtr<nsFontMetrics> fm;
+  if (!aStyleVisibility) {
+    aStyleVisibility = aStyleContext->GetStyleVisibility();
+  }
+  aPresContext->DeviceContext()->GetMetricsFor(font,
+                                               aStyleVisibility->mLanguage,
+                                               fs, *getter_AddRefs(fm));
+  return fm.forget();
+}
+
 static nscoord CalcLengthWith(const nsCSSValue& aValue,
                               nscoord aFontSize,
                               const nsStyleFont* aStyleFont,
+                              const nsStyleVisibility* aStyleVisibility,
                               nsStyleContext* aStyleContext,
                               nsPresContext* aPresContext,
                               PRBool aUseProvidedRootEmSize,
                               // aUseUserFontSet should always be PR_TRUE
                               // except when called from
                               // CalcLengthWithInitialFont.
                               PRBool aUseUserFontSet,
                               PRBool& aCanStoreInRuleTree)
 {
   NS_ASSERTION(aValue.IsLengthUnit() || aValue.IsCalcUnit(),
                "not a length or calc unit");
-  NS_ASSERTION(aStyleFont || aStyleContext, "Must have style data");
-  NS_ASSERTION(!aStyleFont || !aStyleContext, "Duplicate sources of data");
+  NS_ASSERTION((aStyleFont && aStyleVisibility) || aStyleContext,
+               "Must have style data");
+  NS_ASSERTION((!aStyleFont && !aStyleVisibility) || !aStyleContext,
+               "Duplicate sources of data");
   NS_ASSERTION(aPresContext, "Must have prescontext");
 
   if (aValue.IsFixedLengthUnit()) {
     return aValue.GetFixedLength(aPresContext);
   }
   if (aValue.IsPixelLengthUnit()) {
     return aValue.GetPixelLength();
   }
@@ -300,27 +331,25 @@ static nscoord CalcLengthWith(const nsCS
 
       return ScaleCoord(aValue, float(rootFontSize));
     }
     case eCSSUnit_EM: {
       return ScaleCoord(aValue, float(aFontSize));
       // XXX scale against font metrics height instead?
     }
     case eCSSUnit_XHeight: {
-      nsFont font = styleFont->mFont;
-      font.size = aFontSize;
       nsRefPtr<nsFontMetrics> fm =
-        aPresContext->GetMetricsFor(font, aUseUserFontSet);
+        GetMetricsFor(aPresContext, aStyleContext, styleFont,
+                      aStyleVisibility, aFontSize, aUseUserFontSet);
       return ScaleCoord(aValue, float(fm->XHeight()));
     }
     case eCSSUnit_Char: {
-      nsFont font = styleFont->mFont;
-      font.size = aFontSize;
       nsRefPtr<nsFontMetrics> fm =
-        aPresContext->GetMetricsFor(font, aUseUserFontSet);
+        GetMetricsFor(aPresContext, aStyleContext, styleFont,
+                      aStyleVisibility, aFontSize, aUseUserFontSet);
       gfxFloat zeroWidth = (fm->GetThebesFontGroup()->GetFontAt(0)
                             ->GetMetrics().zeroOrAveCharWidth);
 
       return ScaleCoord(aValue, ceil(aPresContext->AppUnitsPerDevPixel() *
                                      zeroWidth));
     }
     // For properties for which lengths are the *only* units accepted in
     // calc(), we can handle calc() here and just compute a final
@@ -328,17 +357,18 @@ static nscoord CalcLengthWith(const nsCS
     // properties by not calling CalcLength in those cases:  SetCoord
     // only calls CalcLength for a calc when it is appropriate to do so.
     case eCSSUnit_Calc:
     case eCSSUnit_Calc_Plus:
     case eCSSUnit_Calc_Minus:
     case eCSSUnit_Calc_Times_L:
     case eCSSUnit_Calc_Times_R:
     case eCSSUnit_Calc_Divided: {
-      CalcLengthCalcOps ops(aFontSize, aStyleFont, aStyleContext, aPresContext,
+      CalcLengthCalcOps ops(aFontSize, aStyleFont, aStyleVisibility,
+                            aStyleContext, aPresContext,
                             aUseProvidedRootEmSize, aUseUserFontSet,
                             aCanStoreInRuleTree);
       return css::ComputeCalc(aValue, ops);
     }
     default:
       NS_NOTREACHED("unexpected unit");
       break;
   }
@@ -348,17 +378,18 @@ static nscoord CalcLengthWith(const nsCS
 /* static */ nscoord
 nsRuleNode::CalcLength(const nsCSSValue& aValue,
                        nsStyleContext* aStyleContext,
                        nsPresContext* aPresContext,
                        PRBool& aCanStoreInRuleTree)
 {
   NS_ASSERTION(aStyleContext, "Must have style data");
 
-  return CalcLengthWith(aValue, -1, nsnull, aStyleContext, aPresContext,
+  return CalcLengthWith(aValue, -1, nsnull, nsnull,
+                        aStyleContext, aPresContext,
                         PR_FALSE, PR_TRUE, aCanStoreInRuleTree);
 }
 
 /* Inline helper function to redirect requests to CalcLength. */
 static inline nscoord CalcLength(const nsCSSValue& aValue,
                                  nsStyleContext* aStyleContext,
                                  nsPresContext* aPresContext,
                                  PRBool& aCanStoreInRuleTree)
@@ -367,18 +398,20 @@ static inline nscoord CalcLength(const n
                                 aPresContext, aCanStoreInRuleTree);
 }
 
 /* static */ nscoord
 nsRuleNode::CalcLengthWithInitialFont(nsPresContext* aPresContext,
                                       const nsCSSValue& aValue)
 {
   nsStyleFont defaultFont(aPresContext);
+  nsStyleVisibility defaultVisibility(aPresContext); // FIXME: best language?
   PRBool canStoreInRuleTree;
-  return CalcLengthWith(aValue, -1, &defaultFont, nsnull, aPresContext,
+  return CalcLengthWith(aValue, -1, &defaultFont, &defaultVisibility,
+                        nsnull, aPresContext,
                         PR_TRUE, PR_FALSE, canStoreInRuleTree);
 }
 
 struct LengthPercentPairCalcOps : public css::NumbersAlreadyNormalizedOps
 {
   typedef nsRuleNode::ComputedCalc result_type;
 
   LengthPercentPairCalcOps(nsStyleContext* aContext,
@@ -2437,39 +2470,43 @@ ComputeScriptLevelSize(const nsStyleFont
 }
 
 struct SetFontSizeCalcOps : public css::BasicCoordCalcOps,
                             public css::NumbersAlreadyNormalizedOps
 {
   // The parameters beyond aValue that we need for CalcLengthWith.
   const nscoord mParentSize;
   const nsStyleFont* const mParentFont;
+  const nsStyleVisibility* const mLanguageVisibility;
   nsPresContext* const mPresContext;
   const PRBool mAtRoot;
   PRBool& mCanStoreInRuleTree;
 
   SetFontSizeCalcOps(nscoord aParentSize, const nsStyleFont* aParentFont,
+                     const nsStyleVisibility* aLanguageVisibility,
                      nsPresContext* aPresContext, PRBool aAtRoot,
                      PRBool& aCanStoreInRuleTree)
     : mParentSize(aParentSize),
       mParentFont(aParentFont),
+      mLanguageVisibility(aLanguageVisibility),
       mPresContext(aPresContext),
       mAtRoot(aAtRoot),
       mCanStoreInRuleTree(aCanStoreInRuleTree)
   {
   }
 
   result_type ComputeLeafValue(const nsCSSValue& aValue)
   {
     nscoord size;
     if (aValue.IsLengthUnit()) {
       // Note that font-based length units use the parent's size
       // unadjusted for scriptlevel changes. A scriptlevel change
       // between us and the parent is simply ignored.
-      size = CalcLengthWith(aValue, mParentSize, mParentFont,
+      size = CalcLengthWith(aValue, mParentSize,
+                            mParentFont, mLanguageVisibility,
                             nsnull, mPresContext, mAtRoot,
                             PR_TRUE, mCanStoreInRuleTree);
       if (!aValue.IsRelativeLengthUnit()) {
         size = nsStyleFont::ZoomText(mPresContext, size);
       }
     }
     else if (eCSSUnit_Percent == aValue.GetUnit()) {
       mCanStoreInRuleTree = PR_FALSE;
@@ -2487,16 +2524,17 @@ struct SetFontSizeCalcOps : public css::
   }
 };
 
 /* static */ void
 nsRuleNode::SetFontSize(nsPresContext* aPresContext,
                         const nsRuleData* aRuleData,
                         const nsStyleFont* aFont,
                         const nsStyleFont* aParentFont,
+                        const nsStyleVisibility* aLanguageVisibility,
                         nscoord* aSize,
                         const nsFont& aSystemFont,
                         nscoord aParentSize,
                         nscoord aScriptLevelAdjustedParentSize,
                         PRBool aUsedStartStruct,
                         PRBool aAtRoot,
                         PRBool& aCanStoreInRuleTree)
 {
@@ -2545,18 +2583,18 @@ nsRuleNode::SetFontSize(nsPresContext* a
       }
     } else {
       NS_NOTREACHED("unexpected value");
     }
   }
   else if (sizeValue->IsLengthUnit() ||
            sizeValue->GetUnit() == eCSSUnit_Percent ||
            sizeValue->IsCalcUnit()) {
-    SetFontSizeCalcOps ops(aParentSize, aParentFont, aPresContext, aAtRoot,
-                           aCanStoreInRuleTree);
+    SetFontSizeCalcOps ops(aParentSize, aParentFont, aLanguageVisibility,
+                           aPresContext, aAtRoot, aCanStoreInRuleTree);
     *aSize = css::ComputeCalc(*sizeValue, ops);
     if (*aSize < 0) {
       NS_ABORT_IF_FALSE(sizeValue->IsCalcUnit(),
                         "negative lengths and percents should be rejected "
                         "by parser");
       *aSize = 0;
     }
     // Zoom is handled inside the calc ops when needed.
@@ -2617,16 +2655,22 @@ nsRuleNode::SetFont(nsPresContext* aPres
                     PRUint8 aGenericFontID, const nsRuleData* aRuleData,
                     const nsStyleFont* aParentFont,
                     nsStyleFont* aFont, PRBool aUsedStartStruct,
                     PRBool& aCanStoreInRuleTree)
 {
   const nsFont* defaultVariableFont =
     aPresContext->GetDefaultFont(kPresContext_DefaultVariableFont_ID);
   PRBool atRoot = !aContext->GetParent();
+  // We get '%', 'em', 'ex', and 'ch' units from aParentFont.  For 'ex'
+  // and 'ch' units we use font metrics, which depend on language.  It
+  // makes sense to use the language from the same style context as
+  // aParentFont.
+  const nsStyleVisibility *languageVisibility =
+    (atRoot ? aContext : aContext->GetParent())->GetStyleVisibility();
 
   // -moz-system-font: enum (never inherit!)
   nsFont systemFont;
   const nsCSSValue* systemFontValue = aRuleData->ValueForSystemFont();
   if (eCSSUnit_Enumerated == systemFontValue->GetUnit()) {
     nsSystemFontID sysID;
     switch (systemFontValue->GetIntValue()) {
       case NS_STYLE_FONT_CAPTION:       sysID = eSystemFont_Caption;      break;    // css2
@@ -2805,17 +2849,18 @@ nsRuleNode::SetFont(nsPresContext* aPres
 
   // -moz-script-min-size: length
   const nsCSSValue* scriptMinSizeValue = aRuleData->ValueForScriptMinSize();
   if (scriptMinSizeValue->IsLengthUnit()) {
     // scriptminsize in font units (em, ex) has to be interpreted relative
     // to the parent font, or the size definitions are circular and we
     //
     aFont->mScriptMinSize =
-      CalcLengthWith(*scriptMinSizeValue, aParentFont->mSize, aParentFont,
+      CalcLengthWith(*scriptMinSizeValue, aParentFont->mSize,
+                     aParentFont, languageVisibility,
                      nsnull, aPresContext, atRoot, PR_TRUE,
                      aCanStoreInRuleTree);
   }
 
   // -moz-script-size-multiplier: factor, inherit, initial
   SetFactor(*aRuleData->ValueForScriptSizeMultiplier(),
             aFont->mScriptSizeMultiplier,
             aCanStoreInRuleTree, aParentFont->mScriptSizeMultiplier,
@@ -2873,31 +2918,32 @@ nsRuleNode::SetFont(nsPresContext* aPres
   // font-size: enum, length, percent, inherit
   nscoord scriptLevelAdjustedParentSize = aParentFont->mSize;
   nscoord scriptLevelAdjustedUnconstrainedParentSize;
   scriptLevelAdjustedParentSize =
     ComputeScriptLevelSize(aFont, aParentFont, aPresContext,
                            &scriptLevelAdjustedUnconstrainedParentSize);
   NS_ASSERTION(!aUsedStartStruct || aFont->mScriptUnconstrainedSize == aFont->mSize,
                "If we have a start struct, we should have reset everything coming in here");
-  SetFontSize(aPresContext, aRuleData, aFont, aParentFont, &aFont->mSize,
+  SetFontSize(aPresContext, aRuleData, aFont, aParentFont,
+              languageVisibility, &aFont->mSize,
               systemFont, aParentFont->mSize, scriptLevelAdjustedParentSize,
               aUsedStartStruct, atRoot, aCanStoreInRuleTree);
   if (aParentFont->mSize == aParentFont->mScriptUnconstrainedSize &&
       scriptLevelAdjustedParentSize == scriptLevelAdjustedUnconstrainedParentSize) {
     // Fast path: we have not been affected by scriptminsize so we don't
     // need to call SetFontSize again to compute the
     // scriptminsize-unconstrained size. This is OK even if we have a
     // start struct, because if we have a start struct then 'font-size'
     // was specified and so scriptminsize has no effect.
     aFont->mScriptUnconstrainedSize = aFont->mSize;
   } else {
     SetFontSize(aPresContext, aRuleData, aFont, aParentFont,
-                &aFont->mScriptUnconstrainedSize, systemFont,
-                aParentFont->mScriptUnconstrainedSize,
+                languageVisibility, &aFont->mScriptUnconstrainedSize,
+                systemFont, aParentFont->mScriptUnconstrainedSize,
                 scriptLevelAdjustedUnconstrainedParentSize,
                 aUsedStartStruct, atRoot, aCanStoreInRuleTree);
   }
   NS_ASSERTION(aFont->mScriptUnconstrainedSize <= aFont->mSize,
                "scriptminsize should never be making things bigger");
 
   // enforce the user' specified minimum font-size on the value that we expose
   // (but don't change font-size:0)
@@ -4551,16 +4597,20 @@ nsRuleNode::ComputeVisibilityData(void* 
                                   nsStyleContext* aContext,
                                   nsRuleNode* aHighestNode,
                                   const RuleDetail aRuleDetail,
                                   const PRBool aCanStoreInRuleTree)
 {
   COMPUTE_START_INHERITED(Visibility, (mPresContext),
                           visibility, parentVisibility)
 
+  // IMPORTANT: No properties in this struct have lengths in them.  We
+  // depend on this since CalcLengthWith can call GetStyleVisibility()
+  // to get the language for resolving fonts!
+
   // direction: enum, inherit, initial
   SetDiscrete(*aRuleData->ValueForDirection(), visibility->mDirection,
               canStoreInRuleTree,
               SETDSC_ENUMERATED, parentVisibility->mDirection,
               (GET_BIDI_OPTION_DIRECTION(mPresContext->GetBidi())
                == IBMBIDI_TEXTDIRECTION_RTL)
               ? NS_STYLE_DIRECTION_RTL : NS_STYLE_DIRECTION_LTR,
               0, 0, 0, 0);
--- a/layout/style/nsRuleNode.h
+++ b/layout/style/nsRuleNode.h
@@ -595,16 +595,17 @@ protected:
                         RuleDetail aRuleDetail,
                         const PRBool aCanStoreInRuleTree);
 
   // helpers for |ComputeFontData| that need access to |mNoneBits|:
   static void SetFontSize(nsPresContext* aPresContext,
                           const nsRuleData* aRuleData,
                           const nsStyleFont* aFont,
                           const nsStyleFont* aParentFont,
+                          const nsStyleVisibility* aLanguageVisibility,
                           nscoord* aSize,
                           const nsFont& aSystemFont,
                           nscoord aParentSize,
                           nscoord aScriptLevelAdjustedParentSize,
                           PRBool aUsedStartStruct,
                           PRBool aAtRoot,
                           PRBool& aCanStoreInRuleTree);
 
--- a/layout/style/test/Makefile.in
+++ b/layout/style/test/Makefile.in
@@ -128,16 +128,17 @@ GARBAGE += css_properties.js
 		test_bug652486.html \
 		test_bug657143.html \
 		test_bug664955.html \
 		test_bug667520.html \
 		test_bug645998.html \
 		file_bug645998-1.css \
 		file_bug645998-2.css \
 		test_cascade.html \
+		test_ch_ex_no_infloops.html \
 		test_compute_data_with_start_struct.html \
 		test_computed_style.html \
 		test_computed_style_no_pseudo.html \
 		test_css_cross_domain.html \
 		test_css_eof_handling.html \
 		test_descriptor_storage.html \
 		test_descriptor_syntax_errors.html \
 		test_dont_use_document_colors.html \
new file mode 100644
--- /dev/null
+++ b/layout/style/test/test_ch_ex_no_infloops.html
@@ -0,0 +1,64 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=678671
+-->
+<head>
+  <title>Test for Bug 678671</title>
+  <script type="application/javascript" src="/MochiKit/packed.js"></script>
+  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <script type="text/javascript" src="property_database.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
+</head>
+<body>
+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=678671">Mozilla Bug 678671</a>
+<p id="display"></p>
+<div id="content"></div>
+<script type="application/javascript">
+
+/** Test for Bug 678671 **/
+
+/**
+ * Test 'ex' and 'ch' units in every place we possible can to make
+ * sure they don't cause an infinite loop.
+ */
+
+var content = document.getElementById("content");
+var cs = getComputedStyle(content, "");
+
+for (var prop in gCSSProperties) {
+  var info = gCSSProperties[prop];
+  if (info.backend_only) {
+    continue;
+  }
+  function test_val(v) {
+    content.style.setProperty(prop, v, "");
+    isnot(get_computed_value(cs, prop), "",
+      "Setting '" + prop + "' to '" + v + "' should not cause infinite loop");
+  }
+  test_val('3ex');
+  test_val('2ch');
+  function test_replaced_values(value_list) {
+    // For each item in value_list, if it looks like it has a dimension
+    // in it, replace those dimensions with 3ex and 2ch and test it.
+    for (var i = 0; i < value_list.length; ++i) {
+      var value = value_list[i];
+      function try_replace(withval) {
+        var rep = value.replace(/[0-9.]+[a-zA-Z]+/g, withval)
+        if (rep != value) {
+          test_val(rep);
+        }
+      }
+      try_replace('3ex');
+      try_replace('2ch');
+    }
+  }
+  test_replaced_values(info.initial_values);
+  test_replaced_values(info.other_values);
+  content.style.removeProperty(prop);
+}
+
+</script>
+</pre>
+</body>
+</html>