Closed Bug 677036 Opened 13 years ago Closed 12 years ago

Unify parsing length and mpadded attributes

Categories

(Core :: MathML, defect)

Product:
Core
Component:
MathML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla15

People

(Reporter: fredw, Assigned: fredw)

References

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 3 obsolete files)

Patch V2
51.98 KB, patch
karlt
: review+
Details | Diff | Splinter Review
patch for the torture test
723 bytes, patch
karlt
: review+
karlt
: checkin+
Details | Diff | Splinter Review 
We can merge nsMathMLmpaddedFrame::ParseAttribute into nsMathMLElement::ParseNumericValue.

See bug 398038 comment 23.
Assignee: nobody → hage.jonathan
Blocks: 398038, 411227
Depends on: namedspaceOverriding
Attached patch Patch (obsolete) — — Splinter Review 

    
    

    
  
(In reply to Jonathan Hage from comment #1)
> Created attachment 552049 [details] [diff] [review]
> Patch

I think the general idea about how pseudounit and sign members/arguments are merged and used is good. However, nsMathMLElement::ParseNumericValue is not quite correct in its present form, one will have to read carefully what the REC says about the mpadded/length values and how we can unify their parsing. The patch remains a good starting point, though.

    
  
Assignee: hage.jonathan → fred.wang

    
    

    
  
Finally, I think I'm going to fix bug 673759 here.

http://www.w3.org/TR/MathML3/chapter2.html#type.length
http://www.w3.org/TR/MathML3/chapter3.html#id.3.3.6.2
Blocks: namedspaceOverriding
No longer depends on: namedspaceOverriding
Summary: Merge nsMathMLmpaddedFrame::ParseAttribute into nsMathMLElement::ParseNumericValue → Unify parsing length and mpadded attributes

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch V1 - part 1 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #552049 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 586803 [details] [diff] [review]
Patch V1 - part 1

># HG changeset patch
># Parent 82fe146ec9449c35d2d78c91863191b1c426d18d
># User Frédéric Wang <fred.wang@free.fr>
>Bug 677036 - Unify parsing length and mpadded attributes (bug 673759) - part1. r=karlt
>
>diff --git a/content/mathml/content/src/nsMathMLElement.cpp b/content/mathml/content/src/nsMathMLElement.cpp
>--- a/content/mathml/content/src/nsMathMLElement.cpp
>+++ b/content/mathml/content/src/nsMathMLElement.cpp
>@@ -227,16 +227,63 @@ nsMapRuleToAttributesFunc
> nsMathMLElement::GetAttributeMappingFunction() const
> {
>   // It doesn't really matter what our tag is here, because only attributes
>   // that satisfy IsAttributeMapped will be stored in the mapped attributes
>   // list and available to the mapping function
>   return &MapMathMLAttributesInto;
> }
> 
>+/* static */ bool
>+nsMathMLElement::ParseNamedSpaceValue(const nsString& aString,
>+                                      nsCSSValue&     aCSSValue,
>+                                      PRUint32        aFlags)
>+{
>+   PRInt32 i = 0;
>+   // See if it is one of the 'namedspace' (ranging 1/18em...7/18em)
>+   if (aString.EqualsLiteral("veryverythinmathspace")) {
>+     i = 1;
>+   } else if (aString.EqualsLiteral("verythinmathspace")) {
>+     i = 2;
>+   } else if (aString.EqualsLiteral("thinmathspace")) {
>+     i = 3;
>+   } else if (aString.EqualsLiteral("mediummathspace")) {
>+     i = 4;
>+   } else if (aString.EqualsLiteral("thickmathspace")) {
>+     i = 5;
>+   } else if (aString.EqualsLiteral("verythickmathspace")) {
>+     i = 6;
>+   } else if (aString.EqualsLiteral("veryverythickmathspace")) {
>+     i = 7;
>+   } else if (aFlags & PARSE_ALLOW_NEGATIVE) {
>+     if (aString.EqualsLiteral("negativeveryverythinmathspace")) {
>+       i = -1;
>+     } else if (aString.EqualsLiteral("negativeverythinmathspace")) {
>+       i = -2;
>+     } else if (aString.EqualsLiteral("negativethinmathspace")) {
>+       i = -3;
>+     } else if (aString.EqualsLiteral("negativemediummathspace")) {
>+       i = -4;
>+     } else if (aString.EqualsLiteral("negativethickmathspace")) {
>+       i = -5;
>+     } else if (aString.EqualsLiteral("negativeverythickmathspace")) {
>+       i = -6;
>+     } else if (aString.EqualsLiteral("negativeveryverythickmathspace")) {
>+       i = -7;
>+     }
>+   }
>+   if (0 != i) { 
>+     // fall back to the default value
>+     aCSSValue.SetFloatValue(float(i)/float(18), eCSSUnit_EM);
>+     return true;
>+   }
>+   
>+   return false;
>+}
>+ 
> // ================
> // Utilities for parsing and retrieving numeric values
> 
> /*
> The REC says:
>   An explicit plus sign ('+') is not allowed as part of a numeric value
>   except when it is specifically listed in the syntax (as a quoted '+'
>   or "+"),
>@@ -258,23 +305,32 @@ Implementation here:
>   [h/v-unit]
> */
> 
> /* static */ bool
> nsMathMLElement::ParseNumericValue(const nsString& aString,
>                                    nsCSSValue&     aCSSValue,
>                                    PRUint32        aFlags)
> {
>+  if (aFlags & PARSE_MPADDED_SYNTAX) {
>+    // XXXfw To reimplement! (bug 677036)
>+    return false;
>+  }
>+
>   nsAutoString str(aString);
>   str.CompressWhitespace(); // aString is const in this code...
> 
>   PRInt32 stringLength = str.Length();
>   if (!stringLength)
>     return false;
> 
>+  if (ParseNamedSpaceValue(aString, aCSSValue, aFlags)) {
>+    return true;
>+  }
>+
>   nsAutoString number, unit;
> 
>   // see if the negative sign is there
>   PRInt32 i = 0;
>   PRUnichar c = str[0];
>   if (c == '-') {
>     number.Append(c);
>     i++;
>diff --git a/content/mathml/content/src/nsMathMLElement.h b/content/mathml/content/src/nsMathMLElement.h
>--- a/content/mathml/content/src/nsMathMLElement.h
>+++ b/content/mathml/content/src/nsMathMLElement.h
>@@ -82,18 +82,23 @@ public:
>                                 const nsAString& aValue,
>                                 nsAttrValue& aResult);
> 
>   NS_IMETHOD_(bool) IsAttributeMapped(const nsIAtom* aAttribute) const;
>   virtual nsMapRuleToAttributesFunc GetAttributeMappingFunction() const;
> 
>   enum {
>     PARSE_ALLOW_UNITLESS = 0x01, // unitless 0 will be turned into 0px
>-    PARSE_ALLOW_NEGATIVE = 0x02
>+    PARSE_ALLOW_NEGATIVE = 0x02,
>+    PARSE_MPADDED_SYNTAX = 0x04
>   };
>+  static bool ParseNamedSpaceValue(const nsString& aString,
>+                                   nsCSSValue&     aCSSValue,
>+                                   PRUint32        aFlags);
>+
>   static bool ParseNumericValue(const nsString& aString,
>                                   nsCSSValue&     aCSSValue,
>                                   PRUint32        aFlags);
> 
>   static void MapMathMLAttributesInto(const nsMappedAttributes* aAttributes, 
>                                       nsRuleData* aRuleData);
>   
>   virtual nsresult PreHandleEvent(nsEventChainPreVisitor& aVisitor);
>diff --git a/layout/mathml/nsMathMLFrame.cpp b/layout/mathml/nsMathMLFrame.cpp
>--- a/layout/mathml/nsMathMLFrame.cpp
>+++ b/layout/mathml/nsMathMLFrame.cpp
>@@ -342,138 +342,64 @@ nsMathMLFrame::GetAxisHeight(nsRendering
>   nsBoundingMetrics bm = aRenderingContext.GetBoundingMetrics(&minus, 1);
>   aAxisHeight = bm.ascent - (bm.ascent + bm.descent)/2;
>   if (aAxisHeight <= 0 || aAxisHeight >= xHeight) {
>     // fall-back to the other version
>     GetAxisHeight(aFontMetrics, aAxisHeight);
>   }
> }
> 
>-/* static */ nscoord
>-nsMathMLFrame::CalcLength(nsPresContext*   aPresContext,
>-                          nsStyleContext*   aStyleContext,
>-                          const nsCSSValue& aCSSValue)
>+/* static */ void
>+nsMathMLFrame::ParseNumericValue(const nsString&   aString,
>+                                 nscoord&          aLengthValue,
>+                                 PRUint32          aFlags,
>+                                 nsPresContext*    aPresContext,
>+                                 nsStyleContext*   aStyleContext)
> {
>-  NS_ASSERTION(aCSSValue.IsLengthUnit(), "not a length unit");
>+  nsCSSValue cssValue;
> 
>-  if (aCSSValue.IsFixedLengthUnit()) {
>-    return aCSSValue.GetFixedLength(aPresContext);
>-  }
>-  if (aCSSValue.IsPixelLengthUnit()) {
>-    return aCSSValue.GetPixelLength();
>+  if (!nsMathMLElement::ParseNumericValue(aString, cssValue, aFlags)) {
>+    return;
>   }
> 
>-  nsCSSUnit unit = aCSSValue.GetUnit();
>+  nsCSSUnit unit = cssValue.GetUnit();
>+
>+  if (unit == eCSSUnit_Percent || unit == eCSSUnit_Number) {
>+    aLengthValue = NSToCoordRound(cssValue.GetFloatValue() * aLengthValue);
>+    return;
>+  }
>+
>+  if (cssValue.IsFixedLengthUnit()) {
>+    aLengthValue = cssValue.GetFixedLength(aPresContext);
>+    return;
>+  }
>+
>+  if (cssValue.IsPixelLengthUnit()) {
>+    aLengthValue = cssValue.GetPixelLength();
>+    return;
>+  }
> 
>   if (eCSSUnit_EM == unit) {
>     const nsStyleFont* font = aStyleContext->GetStyleFont();
>-    return NSToCoordRound(aCSSValue.GetFloatValue() * (float)font->mFont.size);
>+    aLengthValue = NSToCoordRound(cssValue.GetFloatValue() *
>+                                  (float)font->mFont.size);
>+    return;
>   }
>   else if (eCSSUnit_XHeight == unit) {
>     nsRefPtr<nsFontMetrics> fm;
>     nsLayoutUtils::GetFontMetricsForStyleContext(aStyleContext,
>                                                  getter_AddRefs(fm));
>     nscoord xHeight = fm->XHeight();
>-    return NSToCoordRound(aCSSValue.GetFloatValue() * (float)xHeight);
>+    aLengthValue = NSToCoordRound(cssValue.GetFloatValue() * (float)xHeight);
>+    return;
>   }
> 
>   // MathML doesn't specify other CSS units such as rem or ch
>-  NS_ERROR("Unsupported unit");
>-  return 0;
>-}
>-
>-/* static */ bool
>-nsMathMLFrame::ParseNamedSpaceValue(nsIFrame*   aMathMLmstyleFrame,
>-                                    nsString&   aString,
>-                                    nsCSSValue& aCSSValue)
>-{
>-  aCSSValue.Reset();
>-  aString.CompressWhitespace(); //  aString is not a const in this code...
>-  if (!aString.Length()) return false;
>-
>-  // See if it is one of the 'namedspace' (ranging 1/18em...7/18em)
>-  PRInt32 i = 0;
>-  nsIAtom* namedspaceAtom = nsnull;
>-  if (aString.EqualsLiteral("veryverythinmathspace")) {
>-    i = 1;
>-    namedspaceAtom = nsGkAtoms::veryverythinmathspace_;
>-  }
>-  else if (aString.EqualsLiteral("verythinmathspace")) {
>-    i = 2;
>-    namedspaceAtom = nsGkAtoms::verythinmathspace_;
>-  }
>-  else if (aString.EqualsLiteral("thinmathspace")) {
>-    i = 3;
>-    namedspaceAtom = nsGkAtoms::thinmathspace_;
>-  }
>-  else if (aString.EqualsLiteral("mediummathspace")) {
>-    i = 4;
>-    namedspaceAtom = nsGkAtoms::mediummathspace_;
>-  }
>-  else if (aString.EqualsLiteral("thickmathspace")) {
>-    i = 5;
>-    namedspaceAtom = nsGkAtoms::thickmathspace_;
>-  }
>-  else if (aString.EqualsLiteral("verythickmathspace")) {
>-    i = 6;
>-    namedspaceAtom = nsGkAtoms::verythickmathspace_;
>-  }
>-  else if (aString.EqualsLiteral("veryverythickmathspace")) {
>-    i = 7;
>-    namedspaceAtom = nsGkAtoms::veryverythickmathspace_;
>-  }
>-  else if (aString.EqualsLiteral("negativeveryverythinmathspace")) {
>-    i = -1;
>-    namedspaceAtom = nsGkAtoms::negativeveryverythinmathspace_;
>-  }
>-  else if (aString.EqualsLiteral("negativeverythinmathspace")) {
>-    i = -2;
>-    namedspaceAtom = nsGkAtoms::negativeverythinmathspace_;
>-  }
>-  else if (aString.EqualsLiteral("negativethinmathspace")) {
>-    i = -3;
>-    namedspaceAtom = nsGkAtoms::negativethinmathspace_;
>-  }
>-  else if (aString.EqualsLiteral("negativemediummathspace")) {
>-    i = -4;
>-    namedspaceAtom = nsGkAtoms::negativemediummathspace_;
>-  }
>-  else if (aString.EqualsLiteral("negativethickmathspace")) {
>-    i = -5;
>-    namedspaceAtom = nsGkAtoms::negativethickmathspace_;
>-  }
>-  else if (aString.EqualsLiteral("negativeverythickmathspace")) {
>-    i = -6;
>-    namedspaceAtom = nsGkAtoms::negativeverythickmathspace_;
>-  }
>-  else if (aString.EqualsLiteral("negativeveryverythickmathspace")) {
>-    i = -7;
>-    namedspaceAtom = nsGkAtoms::negativeveryverythickmathspace_;
>-  }
>-
>-  if (0 != i) {
>-    if (aMathMLmstyleFrame) {
>-      // see if there is a <mstyle> that has overriden the default value
>-      // GetAttribute() will recurse all the way up into the <mstyle> hierarchy
>-      nsAutoString value;
>-      GetAttribute(nsnull, aMathMLmstyleFrame, namedspaceAtom, value);
>-      if (!value.IsEmpty()) {
>-        if (ParseNumericValue(value, aCSSValue) &&
>-            aCSSValue.IsLengthUnit()) {
>-          return true;
>-        }
>-      }
>-    }
>-
>-    // fall back to the default value
>-    aCSSValue.SetFloatValue(float(i)/float(18), eCSSUnit_EM);
>-    return true;
>-  }
>-
>-  return false;
>+  NS_NOTREACHED("Unsupported unit");
>+  return;
> }
> 
> // ================
> // Utils to map attributes into CSS rules (work-around to bug 69409 which
> // is not scheduled to be fixed anytime soon)
> //
> 
> static const PRInt32 kMathMLversion1 = 1;
>diff --git a/layout/mathml/nsMathMLFrame.h b/layout/mathml/nsMathMLFrame.h
>--- a/layout/mathml/nsMathMLFrame.h
>+++ b/layout/mathml/nsMathMLFrame.h
>@@ -181,33 +181,21 @@ public:
>   static bool
>   GetAttribute(nsIContent* aContent,
>                nsIFrame*   aMathMLmstyleFrame,          
>                nsIAtom*    aAttributeAtom,
>                nsString&   aValue);
> 
>   // utilities to parse and retrieve numeric values in CSS units
>   // All values are stored in twips.
>-  static bool
>-  ParseNumericValue(const nsString& aString,
>-                    nsCSSValue&     aCSSValue) {
>-    return nsMathMLElement::ParseNumericValue(aString, aCSSValue,
>-            nsMathMLElement::PARSE_ALLOW_NEGATIVE |
>-            nsMathMLElement::PARSE_ALLOW_UNITLESS);
>-  }
>-
>-  static nscoord 
>-  CalcLength(nsPresContext*   aPresContext,
>-             nsStyleContext*   aStyleContext,
>-             const nsCSSValue& aCSSValue);
>-
>-  static bool
>-  ParseNamedSpaceValue(nsIFrame*   aMathMLmstyleFrame,
>-                       nsString&   aString,
>-                       nsCSSValue& aCSSValue);
>+  static void ParseNumericValue(const nsString&   aString,
>+                                nscoord&          aLengthValue,
>+                                PRUint32          aFlags,
>+                                nsPresContext*    aPresContext,
>+                                nsStyleContext*   aStyleContext);
> 
>   static eMathMLFrameType
>   GetMathMLFrameTypeFor(nsIFrame* aFrame)
>   {
>     if (aFrame->IsFrameOfType(nsIFrame::eMathML)) {
>       nsIMathMLFrame* mathMLFrame = do_QueryFrame(aFrame);
>       if (mathMLFrame)
>         return mathMLFrame->GetMathMLFrameType();
>diff --git a/layout/mathml/nsMathMLmfracFrame.cpp b/layout/mathml/nsMathMLmfracFrame.cpp
>--- a/layout/mathml/nsMathMLmfracFrame.cpp
>+++ b/layout/mathml/nsMathMLmfracFrame.cpp
>@@ -119,16 +119,24 @@ nsMathMLmfracFrame::CalcLineThickness(ns
>                                       nsString&        aThicknessAttribute,
>                                       nscoord          onePixel,
>                                       nscoord          aDefaultRuleThickness)
> {
>   nscoord defaultThickness = aDefaultRuleThickness;
>   nscoord lineThickness = aDefaultRuleThickness;
>   nscoord minimumThickness = onePixel;
> 
>+  // linethickness
>+  //
>+  // "Specifies the thickness of the horizontal 'fraction bar', or 'rule'. The
>+  // default value is 'medium', 'thin' is thinner, but visible, 'thick' is
>+  // thicker; the exact thickness of these is left up to the rendering agent."
>+  //
>+  // syntax: length | "thin" | "medium" | "thick"
>+  //
>   if (!aThicknessAttribute.IsEmpty()) {
>     if (aThicknessAttribute.EqualsLiteral("thin")) {
>       lineThickness = NSToCoordFloor(defaultThickness * THIN_FRACTION_LINE);
>       minimumThickness = onePixel * THIN_FRACTION_LINE_MINIMUM_PIXELS;
>       // should visually decrease by at least one pixel, if default is not a pixel
>       if (defaultThickness > onePixel && lineThickness > defaultThickness - onePixel)
>         lineThickness = defaultThickness - onePixel;
>     }
>@@ -137,27 +145,22 @@ nsMathMLmfracFrame::CalcLineThickness(ns
>     }
>     else if (aThicknessAttribute.EqualsLiteral("thick")) {
>       lineThickness = NSToCoordCeil(defaultThickness * THICK_FRACTION_LINE);
>       minimumThickness = onePixel * THICK_FRACTION_LINE_MINIMUM_PIXELS;
>       // should visually increase by at least one pixel
>       if (lineThickness < defaultThickness + onePixel)
>         lineThickness = defaultThickness + onePixel;
>     }
>-    else { // see if it is a plain number, or a percentage, or a h/v-unit like 1ex, 2px, 1em
>-      nsCSSValue cssValue;
>-      if (ParseNumericValue(aThicknessAttribute, cssValue)) {
>-        nsCSSUnit unit = cssValue.GetUnit();
>-        if (eCSSUnit_Number == unit)
>-          lineThickness = nscoord(float(defaultThickness) * cssValue.GetFloatValue());
>-        else if (eCSSUnit_Percent == unit)
>-          lineThickness = nscoord(float(defaultThickness) * cssValue.GetPercentValue());
>-        else if (eCSSUnit_Null != unit)
>-          lineThickness = CalcLength(aPresContext, aStyleContext, cssValue);
>-      }
>+    else {
>+      // length value
>+      lineThickness = defaultThickness;
>+      ParseNumericValue(aThicknessAttribute, lineThickness,
>+                        nsMathMLElement::PARSE_ALLOW_UNITLESS,
>+                        aPresContext, aStyleContext);
>     }
>   }
> 
>   // use minimum if the lineThickness is a non-zero value less than minimun
>   if (lineThickness && lineThickness < minimumThickness) 
>     lineThickness = minimumThickness;
> 
>   return lineThickness;
>diff --git a/layout/mathml/nsMathMLmmultiscriptsFrame.cpp b/layout/mathml/nsMathMLmmultiscriptsFrame.cpp
>--- a/layout/mathml/nsMathMLmmultiscriptsFrame.cpp
>+++ b/layout/mathml/nsMathMLmmultiscriptsFrame.cpp
>@@ -115,34 +115,46 @@ nsMathMLmmultiscriptsFrame::TransmitAuto
> }
> 
> void
> nsMathMLmmultiscriptsFrame::ProcessAttributes()
> {
>   mSubScriptShift = 0;
>   mSupScriptShift = 0;
> 
>-  // check if the subscriptshift attribute is there
>+  // subscriptshift
>+  //
>+  // "Specifies the minimum amount to shift the baseline of subscript down; the
>+  // default is for the rendering agent to use its own positioning rules."
>+  //
>+  // We use 0 as the default value so unitless values can be ignored.
>+  // XXXfw Should we forbid negative values? (bug 411227)
>+  //
>   nsAutoString value;
>   GetAttribute(mContent, mPresentationData.mstyle,
>                nsGkAtoms::subscriptshift_, value);
>   if (!value.IsEmpty()) {
>-    nsCSSValue cssValue;
>-    if (ParseNumericValue(value, cssValue) && cssValue.IsLengthUnit()) {
>-      mSubScriptShift = CalcLength(PresContext(), mStyleContext, cssValue);
>-    }
>+    ParseNumericValue(value, mSubScriptShift,
>+                      nsMathMLElement::PARSE_ALLOW_NEGATIVE,
>+                      PresContext(), mStyleContext);
>   }
>-  // check if the superscriptshift attribute is there
>+  // superscriptshift
>+  //
>+  // "Specifies the minimum amount to shift the baseline of superscript up; the
>+  // default is for the rendering agent to use its own positioning rules."
>+  //
>+  // We use 0 as the default value so unitless values can be ignored.
>+  // XXXfw Should we forbid negative values? (bug 411227)
>+  //
>   GetAttribute(mContent, mPresentationData.mstyle,
>                nsGkAtoms::superscriptshift_, value);
>   if (!value.IsEmpty()) {
>-    nsCSSValue cssValue;
>-    if (ParseNumericValue(value, cssValue) && cssValue.IsLengthUnit()) {
>-      mSupScriptShift = CalcLength(PresContext(), mStyleContext, cssValue);
>-    }
>+    ParseNumericValue(value, mSupScriptShift,
>+                      nsMathMLElement::PARSE_ALLOW_NEGATIVE,
>+                      PresContext(), mStyleContext);
>   }
> }
> 
> /* virtual */ nsresult
> nsMathMLmmultiscriptsFrame::Place(nsRenderingContext& aRenderingContext,
>                                   bool                 aPlaceOrigin,
>                                   nsHTMLReflowMetrics& aDesiredSize)
> {
>diff --git a/layout/mathml/nsMathMLmoFrame.cpp b/layout/mathml/nsMathMLmoFrame.cpp
>--- a/layout/mathml/nsMathMLmoFrame.cpp
>+++ b/layout/mathml/nsMathMLmoFrame.cpp
>@@ -408,48 +408,44 @@ nsMathMLmoFrame::ProcessOperatorData()
>         }
>       }
>     }
>   }
> 
>   // If we are an accent without explicit lspace="." or rspace=".",
>   // we will ignore our default leading/trailing space
> 
>-  // lspace = number h-unit | namedspace
>+  // lspace
>+  //
>+  // "Specifies the leading space appearing before the operator"
>+  //
>   nscoord leadingSpace = mEmbellishData.leadingSpace;
>   GetAttribute(mContent, mPresentationData.mstyle, nsGkAtoms::lspace_,
>                value);
>   if (!value.IsEmpty()) {
>-    nsCSSValue cssValue;
>-    if (ParseNumericValue(value, cssValue) ||
>-        ParseNamedSpaceValue(mPresentationData.mstyle, value, cssValue))
>-    {
>-      if ((eCSSUnit_Number == cssValue.GetUnit()) && !cssValue.GetFloatValue())
>-        leadingSpace = 0;
>-      else if (cssValue.IsLengthUnit())
>-        leadingSpace = CalcLength(presContext, mStyleContext, cssValue);
>-      mFlags |= NS_MATHML_OPERATOR_LSPACE_ATTR;
>-    }
>+    ParseNumericValue(value, leadingSpace, 
>+                      nsMathMLElement::PARSE_ALLOW_UNITLESS |
>+                      nsMathMLElement::PARSE_ALLOW_NEGATIVE,
>+                      presContext, mStyleContext);
>+    mFlags |= NS_MATHML_OPERATOR_LSPACE_ATTR;
>   }
> 
>-  // rspace = number h-unit | namedspace
>+  // rspace
>+  //
>+  // "Specifies the trailing space appearing after the operator"
>+  //
>   nscoord trailingSpace = mEmbellishData.trailingSpace;
>   GetAttribute(mContent, mPresentationData.mstyle, nsGkAtoms::rspace_,
>                value);
>   if (!value.IsEmpty()) {
>-    nsCSSValue cssValue;
>-    if (ParseNumericValue(value, cssValue) ||
>-        ParseNamedSpaceValue(mPresentationData.mstyle, value, cssValue))
>-    {
>-      if ((eCSSUnit_Number == cssValue.GetUnit()) && !cssValue.GetFloatValue())
>-        trailingSpace = 0;
>-      else if (cssValue.IsLengthUnit())
>-        trailingSpace = CalcLength(presContext, mStyleContext, cssValue);
>-      mFlags |= NS_MATHML_OPERATOR_RSPACE_ATTR;
>-    }
>+    ParseNumericValue(value, trailingSpace, 
>+                      nsMathMLElement::PARSE_ALLOW_UNITLESS |
>+                      nsMathMLElement::PARSE_ALLOW_NEGATIVE,
>+                      presContext, mStyleContext);
>+    mFlags |= NS_MATHML_OPERATOR_RSPACE_ATTR;
>   }
> 
>   // little extra tuning to round lspace & rspace to at least a pixel so that
>   // operators don't look as if they are colliding with their operands
>   if (leadingSpace || trailingSpace) {
>     nscoord onePixel = nsPresContext::CSSPixelsToAppUnits(1);
>     if (leadingSpace && leadingSpace < onePixel)
>       leadingSpace = onePixel;
>@@ -498,84 +494,35 @@ nsMathMLmoFrame::ProcessOperatorData()
>   }
>   GetAttribute(mContent, mPresentationData.mstyle, nsGkAtoms::symmetric_,
>                value);
>   if (value.EqualsLiteral("false"))
>     mFlags &= ~NS_MATHML_OPERATOR_SYMMETRIC;
>   else if (value.EqualsLiteral("true"))
>     mFlags |= NS_MATHML_OPERATOR_SYMMETRIC;
> 
>-  // minsize = number [ v-unit | h-unit ] | namedspace
>+  // minsize
>+  //
>+  // 
>+  //
>   mMinSize = 0.0;
>   GetAttribute(mContent, mPresentationData.mstyle, nsGkAtoms::minsize_,
>                value);
>   if (!value.IsEmpty()) {
>-    nsCSSValue cssValue;
>-    if (ParseNumericValue(value, cssValue) ||
>-        ParseNamedSpaceValue(mPresentationData.mstyle, value, cssValue))
>-    {
>-      nsCSSUnit unit = cssValue.GetUnit();
>-      if (eCSSUnit_Number == unit)
>-        mMinSize = cssValue.GetFloatValue();
>-      else if (eCSSUnit_Percent == unit)
>-        mMinSize = cssValue.GetPercentValue();
>-      else if (eCSSUnit_Null != unit) {
>-        mMinSize = float(CalcLength(presContext, mStyleContext, cssValue));
>-        mFlags |= NS_MATHML_OPERATOR_MINSIZE_ABSOLUTE;
>-      }
>-
>-      if ((eCSSUnit_Number == unit) || (eCSSUnit_Percent == unit)) {
>-        // see if the multiplicative inheritance should be from <mstyle>
>-        GetAttribute(nsnull, mPresentationData.mstyle,
>-                     nsGkAtoms::minsize_, value);
>-        if (!value.IsEmpty()) {
>-          if (ParseNumericValue(value, cssValue)) {
>-            if (cssValue.IsLengthUnit()) {
>-              mMinSize *= float(CalcLength(presContext, mStyleContext, cssValue));
>-              mFlags |= NS_MATHML_OPERATOR_MINSIZE_ABSOLUTE;
>-            }
>-          }
>-        }
>-      }
>-    }
>+    // XXXfw TODO
>   }
> 
>-  // maxsize = number [ v-unit | h-unit ] | namedspace | infinity
>+  // maxsize
>+  //
>+  // "Specifies the maximum size of the operator when stretchy"
>   mMaxSize = NS_MATHML_OPERATOR_SIZE_INFINITY;
>   GetAttribute(mContent, mPresentationData.mstyle, nsGkAtoms::maxsize_,
>                value);
>   if (!value.IsEmpty()) {
>-    nsCSSValue cssValue;
>-    if (ParseNumericValue(value, cssValue) ||
>-        ParseNamedSpaceValue(mPresentationData.mstyle, value, cssValue))
>-    {
>-      nsCSSUnit unit = cssValue.GetUnit();
>-      if (eCSSUnit_Number == unit)
>-        mMaxSize = cssValue.GetFloatValue();
>-      else if (eCSSUnit_Percent == unit)
>-        mMaxSize = cssValue.GetPercentValue();
>-      else if (eCSSUnit_Null != unit) {
>-        mMaxSize = float(CalcLength(presContext, mStyleContext, cssValue));
>-        mFlags |= NS_MATHML_OPERATOR_MAXSIZE_ABSOLUTE;
>-      }
>-
>-      if ((eCSSUnit_Number == unit) || (eCSSUnit_Percent == unit)) {
>-        // see if the multiplicative inheritance should be from <mstyle>
>-        GetAttribute(nsnull, mPresentationData.mstyle,
>-                     nsGkAtoms::maxsize_, value);
>-        if (!value.IsEmpty()) {
>-          if (ParseNumericValue(value, cssValue)) {
>-            if (cssValue.IsLengthUnit()) {
>-              mMaxSize *= float(CalcLength(presContext, mStyleContext, cssValue));
>-              mFlags |= NS_MATHML_OPERATOR_MAXSIZE_ABSOLUTE;
>-            }
>-          }
>-        }
>-      }
>-    }
>+    // XXXfw TODO    
>   }
> }
> 
> static PRUint32
> GetStretchHint(nsOperatorFlags aFlags, nsPresentationData aPresentationData,
>                bool aIsVertical)
> {
>   PRUint32 stretchHint = NS_STRETCH_NONE;
>diff --git a/layout/mathml/nsMathMLmpaddedFrame.cpp b/layout/mathml/nsMathMLmpaddedFrame.cpp
>--- a/layout/mathml/nsMathMLmpaddedFrame.cpp
>+++ b/layout/mathml/nsMathMLmpaddedFrame.cpp
>@@ -262,29 +262,29 @@ nsMathMLmpaddedFrame::ParseAttribute(nsS
>     */
>   }
>   else if (unit.EqualsLiteral("width"))  aPseudoUnit = NS_MATHML_PSEUDO_UNIT_WIDTH;
>   else if (unit.EqualsLiteral("height")) aPseudoUnit = NS_MATHML_PSEUDO_UNIT_HEIGHT;
>   else if (unit.EqualsLiteral("depth"))  aPseudoUnit = NS_MATHML_PSEUDO_UNIT_DEPTH;
>   else if (!gotPercent) { // percentage can only apply to a pseudo-unit
> 
>     // see if the unit is a named-space
>-    // XXX nsnull in ParseNamedSpacedValue()? don't access mstyle?
>-    if (ParseNamedSpaceValue(nsnull, unit, aCSSValue)) {
>+    if (false) {
>       // re-scale properly, and we know that the unit of the named-space is 'em'
>       floatValue *= aCSSValue.GetFloatValue();
>       aCSSValue.SetFloatValue(floatValue, eCSSUnit_EM);
>       aPseudoUnit = NS_MATHML_PSEUDO_UNIT_NAMEDSPACE;
>       return true;
>     }
> 
>     // see if the input was just a CSS value
>     number.Append(unit); // leave the sign out if it was there
>-    if (ParseNumericValue(number, aCSSValue))
>+    if (false) {
>       return true;
>+    }
>   }
> 
>   // if we enter here, we have a number that will act as a multiplier on a pseudo-unit
>   if (aPseudoUnit != NS_MATHML_PSEUDO_UNIT_UNSPECIFIED) {
>     if (gotPercent)
>       aCSSValue.SetPercentValue(floatValue / 100.0f);
>     else
>       aCSSValue.SetFloatValue(floatValue, eCSSUnit_Number);
>@@ -335,17 +335,18 @@ nsMathMLmpaddedFrame::UpdateValue(PRInt3
>       }
>     }
> 
>     if (eCSSUnit_Number == unit)
>       amount = NSToCoordRound(float(scaler) * aCSSValue.GetFloatValue());
>     else if (eCSSUnit_Percent == unit)
>       amount = NSToCoordRound(float(scaler) * aCSSValue.GetPercentValue());
>     else
>-      amount = CalcLength(PresContext(), mStyleContext, aCSSValue);
>+      // XXXfw TODO
>+      amount = 0; //CalcLength(PresContext(), mStyleContext, aCSSValue);
> 
>     if (NS_MATHML_SIGN_PLUS == aSign)
>       aValueToUpdate += amount;
>     else if (NS_MATHML_SIGN_MINUS == aSign)
>       aValueToUpdate -= amount;
>     else
>       aValueToUpdate  = amount;
>   }
>diff --git a/layout/mathml/nsMathMLmspaceFrame.cpp b/layout/mathml/nsMathMLmspaceFrame.cpp
>--- a/layout/mathml/nsMathMLmspaceFrame.cpp
>+++ b/layout/mathml/nsMathMLmspaceFrame.cpp
>@@ -66,61 +66,66 @@ bool
> nsMathMLmspaceFrame::IsLeaf() const
> {
>   return true;
> }
> 
> void
> nsMathMLmspaceFrame::ProcessAttributes(nsPresContext* aPresContext)
> {
>-  /*
>-  parse the attributes
>-
>-  width  = number h-unit 
>-  height = number v-unit 
>-  depth  = number v-unit 
>-  */
>-
>   nsAutoString value;
>-  nsCSSValue cssValue;
> 
>   // width 
>+  //
>+  // "Specifies the desired width of the space."
>+  //
>+  // The default value is "0em", so unitless values can be ignored.
>+  // <mspace/> is listed among MathML elements allowing negative spacing and
>+  // the MathML test suite contains "Presentation/TokenElements/mspace/mspace2" 
>+  // as an example. Hence we allow negative values.
>+  //
>   mWidth = 0;
>   GetAttribute(mContent, mPresentationData.mstyle, nsGkAtoms::width,
>                value);
>   if (!value.IsEmpty()) {
>-    if ((ParseNumericValue(value, cssValue) ||
>-         ParseNamedSpaceValue(mPresentationData.mstyle, value, cssValue)) &&
>-         cssValue.IsLengthUnit()) {
>-      mWidth = CalcLength(aPresContext, mStyleContext, cssValue);
>-    }
>+    ParseNumericValue(value, mWidth,
>+                      nsMathMLElement::PARSE_ALLOW_NEGATIVE,
>+                      aPresContext, mStyleContext);
>   }
> 
>   // height
>+  //
>+  // "Specifies the desired height (above the baseline) of the space."
>+  //
>+  // The default value is "0ex", so unitless values can be ignored.
>+  // XXXfw Should we forbid negative values? (bugs 411227, 716349)
>+  //
>   mHeight = 0;
>   GetAttribute(mContent, mPresentationData.mstyle, nsGkAtoms::height,
>                value);
>   if (!value.IsEmpty()) {
>-    if ((ParseNumericValue(value, cssValue) ||
>-         ParseNamedSpaceValue(mPresentationData.mstyle, value, cssValue)) &&
>-         cssValue.IsLengthUnit()) {
>-      mHeight = CalcLength(aPresContext, mStyleContext, cssValue);
>-    }
>+    ParseNumericValue(value, mHeight,
>+                      nsMathMLElement::PARSE_ALLOW_NEGATIVE,
>+                      aPresContext, mStyleContext);
>   }
> 
>   // depth
>+  //
>+  // "Specifies the desired depth (below the baseline) of the space."
>+  //
>+  // The default value is "0ex", so unitless values can be ignored.
>+  // XXXfw Should we forbid negative values? (bugs 411227, 716349)
>+  //
>   mDepth = 0;
>   GetAttribute(mContent, mPresentationData.mstyle, nsGkAtoms::depth_,
>                value);
>   if (!value.IsEmpty()) {
>-    if ((ParseNumericValue(value, cssValue) ||
>-         ParseNamedSpaceValue(mPresentationData.mstyle, value, cssValue)) &&
>-         cssValue.IsLengthUnit()) {
>-      mDepth = CalcLength(aPresContext, mStyleContext, cssValue);
>-    }
>+    ParseNumericValue(value, mDepth,
>+                      nsMathMLElement::PARSE_ALLOW_NEGATIVE,
>+                      aPresContext, mStyleContext);
>   }
> }
> 
> NS_IMETHODIMP
> nsMathMLmspaceFrame::Reflow(nsPresContext*          aPresContext,
>                             nsHTMLReflowMetrics&     aDesiredSize,
>                             const nsHTMLReflowState& aReflowState,
>                             nsReflowStatus&          aStatus)
>diff --git a/layout/mathml/nsMathMLmsubFrame.cpp b/layout/mathml/nsMathMLmsubFrame.cpp
>--- a/layout/mathml/nsMathMLmsubFrame.cpp
>+++ b/layout/mathml/nsMathMLmsubFrame.cpp
>@@ -84,26 +84,32 @@ nsMathMLmsubFrame::TransmitAutomaticData
> /* virtual */ nsresult
> nsMathMLmsubFrame::Place (nsRenderingContext& aRenderingContext,
>                           bool                 aPlaceOrigin,
>                           nsHTMLReflowMetrics& aDesiredSize)
> {
>   // extra spacing after sup/subscript
>   nscoord scriptSpace = nsPresContext::CSSPointsToAppUnits(0.5f); // 0.5pt as in plain TeX
> 
>-  // check if the subscriptshift attribute is there
>+  // subscriptshift
>+  //
>+  // "Specifies the minimum amount to shift the baseline of subscript down; the
>+  // default is for the rendering agent to use its own positioning rules."
>+  //
>+  // We use 0 as the default value so unitless values can be ignored.
>+  // XXXfw Should we forbid negative values? (bug 411227)
>+  //
>   nscoord subScriptShift = 0;
>   nsAutoString value;
>   GetAttribute(mContent, mPresentationData.mstyle,
>                nsGkAtoms::subscriptshift_, value);
>   if (!value.IsEmpty()) {
>-    nsCSSValue cssValue;
>-    if (ParseNumericValue(value, cssValue) && cssValue.IsLengthUnit()) {
>-      subScriptShift = CalcLength(PresContext(), mStyleContext, cssValue);
>-    }
>+    ParseNumericValue(value, subScriptShift,
>+                      nsMathMLElement::PARSE_ALLOW_NEGATIVE,
>+                      PresContext(), mStyleContext);
>   }
> 
>   return nsMathMLmsubFrame::PlaceSubScript(PresContext(), 
>                                            aRenderingContext,
>                                            aPlaceOrigin,
>                                            aDesiredSize,
>                                            this,
>                                            subScriptShift,
>diff --git a/layout/mathml/nsMathMLmsubsupFrame.cpp b/layout/mathml/nsMathMLmsubsupFrame.cpp
>--- a/layout/mathml/nsMathMLmsubsupFrame.cpp
>+++ b/layout/mathml/nsMathMLmsubsupFrame.cpp
>@@ -91,35 +91,48 @@ nsMathMLmsubsupFrame::TransmitAutomaticD
> nsMathMLmsubsupFrame::Place(nsRenderingContext& aRenderingContext,
>                             bool                 aPlaceOrigin,
>                             nsHTMLReflowMetrics& aDesiredSize)
> {
>   // extra spacing between base and sup/subscript
>   nscoord scriptSpace = 0;
> 
>   // check if the subscriptshift attribute is there
>+  //
>+  // The REC defines subscriptshift that way:
>+  // "Specifies the minimum amount to shift the baseline of subscript down; the
>+  // default is for the rendering agent to use its own positioning rules."
>+  //
>+  // We use 0 as the default value so unitless values can be ignored.
>+  // XXXfw Should we forbid negative values? (bug 411227)
>+  //
>   nsAutoString value;
>   nscoord subScriptShift = 0;
>   GetAttribute(mContent, mPresentationData.mstyle,
>                nsGkAtoms::subscriptshift_, value);
>   if (!value.IsEmpty()) {
>-    nsCSSValue cssValue;
>-    if (ParseNumericValue(value, cssValue) && cssValue.IsLengthUnit()) {
>-      subScriptShift = CalcLength(PresContext(), mStyleContext, cssValue);
>-    }
>+    ParseNumericValue(value, subScriptShift,
>+                      nsMathMLElement::PARSE_ALLOW_NEGATIVE,
>+                      PresContext(), mStyleContext);
>   }
>-  // check if the superscriptshift attribute is there
>+  // superscriptshift
>+  //
>+  // "Specifies the minimum amount to shift the baseline of superscript up; the
>+  // default is for the rendering agent to use its own positioning rules."
>+  //
>+  // We use 0 as the default value so unitless values can be ignored.
>+  // XXXfw Should we forbid negative values? (bug 411227)
>+  //
>   nscoord supScriptShift = 0;
>   GetAttribute(mContent, mPresentationData.mstyle,
>                nsGkAtoms::superscriptshift_, value);
>   if (!value.IsEmpty()) {
>-    nsCSSValue cssValue;
>-    if (ParseNumericValue(value, cssValue) && cssValue.IsLengthUnit()) {
>-      supScriptShift = CalcLength(PresContext(), mStyleContext, cssValue);
>-    }
>+    ParseNumericValue(value, supScriptShift,
>+                      nsMathMLElement::PARSE_ALLOW_NEGATIVE,
>+                      PresContext(), mStyleContext);
>   }
> 
>   return nsMathMLmsubsupFrame::PlaceSubSupScript(PresContext(),
>                                                  aRenderingContext,
>                                                  aPlaceOrigin,
>                                                  aDesiredSize,
>                                                  this,
>                                                  subScriptShift,
>diff --git a/layout/mathml/nsMathMLmsupFrame.cpp b/layout/mathml/nsMathMLmsupFrame.cpp
>--- a/layout/mathml/nsMathMLmsupFrame.cpp
>+++ b/layout/mathml/nsMathMLmsupFrame.cpp
>@@ -84,26 +84,32 @@ nsMathMLmsupFrame::TransmitAutomaticData
> /* virtual */ nsresult
> nsMathMLmsupFrame::Place(nsRenderingContext& aRenderingContext,
>                          bool                 aPlaceOrigin,
>                          nsHTMLReflowMetrics& aDesiredSize)
> {
>   // extra spacing after sup/subscript
>   nscoord scriptSpace = nsPresContext::CSSPointsToAppUnits(0.5f); // 0.5pt as in plain TeX
> 
>-  // check if the superscriptshift attribute is there
>+  // superscriptshift
>+  //
>+  // "Specifies the minimum amount to shift the baseline of superscript up; the
>+  // default is for the rendering agent to use its own positioning rules."
>+  //
>+  // We use 0 as the default value so unitless values can be ignored.
>+  // XXXfw Should we forbid negative values? (bug 411227)
>+  //
>   nsAutoString value;
>   nscoord supScriptShift = 0;
>   GetAttribute(mContent, mPresentationData.mstyle,
>                nsGkAtoms::superscriptshift_, value);
>   if (!value.IsEmpty()) {
>-    nsCSSValue cssValue;
>-    if (ParseNumericValue(value, cssValue) && cssValue.IsLengthUnit()) {
>-      supScriptShift = CalcLength(PresContext(), mStyleContext, cssValue);
>-    }
>+    ParseNumericValue(value, supScriptShift,
>+                      nsMathMLElement::PARSE_ALLOW_NEGATIVE,
>+                      PresContext(), mStyleContext);
>   }
> 
>   return nsMathMLmsupFrame::PlaceSuperScript(PresContext(), 
>                                              aRenderingContext,
>                                              aPlaceOrigin,
>                                              aDesiredSize,
>                                              this,
>                                              supScriptShift,

        Attachment #586803 -
        Attachment is obsolete: true

    
    

    
  
https://raw.github.com/fred-wang/MozillaCentralPatches/master/unify-length-and-mpadded-parsing-1.diff

I've spent some time to analyse which kinds of values should be accepted for attribute of type "length" and added descriptions from the spec as well various comments, as a first step to solve bug 411227.

This patch also merges the calls to ParseNumericValue, ParseNamedSpaceValue and CalcLength for most of the attributes in the layout/ side and handles better default values. The only exceptions are minsize/maxsize as well as mpadded, that will need more code refactoring and will be treated in separate patches. But I assume this first patch could be taken without waiting for the remaining work.

Of course, the merge has been allowed by the removal of namespace value overriding, fixing bug 673759. Also, namedspace values should now be accepted by most of the length attributes, as described by the MathML3 REC.

    
    

    
  
So if I understand correctly the code for minsize/maxsize:
http://mxr.mozilla.org/mozilla-central/source/layout/mathml/nsMathMLmoFrame.cpp#506

The attribute is parsed and can be either a relative (unitless or %) or an absolute value.
In the former case, contrary to what is done with other MathML "lengths", we don't multiply by the default value. Instead, we store the multipliers as a non-absolute value to use it later in Stretch. That seems correct. (See http://lists.w3.org/Archives/Public/www-math/2012Jan/0003.html)

However, I'm not sure to understand what is done just after:
mxr.mozilla.org/mozilla-central/source/layout/mathml/nsMathMLmoFrame.cpp#525

We look for a potential absolute minsize in an innermost parent mstyle if there is one, we multiply it by the relative value on the mo element. I can't find where this is described in the spec. It seems to me that this second part should be removed?

    
    

    
  
(In reply to Frédéric Wang from comment #7)
> We look for a potential absolute minsize in an innermost parent mstyle if
> there is one, we multiply it by the relative value on the mo element.

Perhaps an interpretation of "multipliers of the operator's normal size", but I agree that this seems odd.

> can't find where this is described in the spec. It seems to me that this
> second part should be removed?

Yes, makes sense to me, but check there's no mention in MathML2.

    
    

    
  
> > can't find where this is described in the spec. It seems to me that this
> > second part should be removed?
> 
> Yes, makes sense to me, but check there's no mention in MathML2.

Already done, but I didn't find anything in MathML1 or MathML2 either.

    
    

    
  
No bug report for that change but it is old.
"Make maxsize and minsize inherit from <mstyle> if appropriate"
http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/layout/mathml/base/src/nsMathMLmoFrame.cpp&rev=1.20

So feel free to remove it.

    
    

    
  
So finally I don't think it is worth to merge any further the parsing code. Hence I handled the case of minsize/maxsize and mpadded in the same patch. 

For minsize/maxsize, I removed the portion of code mentioned in comment 7. Thus it only remained to merge the call to ParseNumericValue and ParseNamedSpace.

To follow the MathML3 REC and prevent issues like bug 398038, whitespaces are no longer allowed in nsMathMLElement::ParseNumericValue and nsMathMLmpaddedFrame::ParseAttribute. I also removed the case "0" for mpadded, which is not conformed either. The mpadded function call nsMathMLElement::ParseNumericValue with a specific flag to enable a restricted syntax (I think the restriction on '%' should not be necessary, but I let it for clarity).

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch V1 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #587413 -
        Flags: review?(karlt)

    
  
Status: NEW → ASSIGNED

    
    

    
  
Karl, any update on this review? I think it's the remaining part from Jonathan's job and I would like it to be finished, so that it does not bother the students for this summer. Indeed, one of the student I talked with is probably going to work on bug 685628 and moreover this patch touches several files in the MathML code. Also, this bug is blocking three other bugs that can easily be fixed with these changes.

    
    

    
  
Comment on attachment 587413 [details] [diff] [review]
Patch V1

I looked at much of this today.  Hopefully I can finish tomorrow.

>+       i = -7;
>+     }
>+   }
>+   if (0 != i) { 
>+     // fall back to the default value
>+     aCSSValue.SetFloatValue(float(i)/float(18), eCSSUnit_EM);

This comment should be removed now.  This is no longer falling back because
there are no non-default values.

>+  static void ParseNumericValue(const nsString&   aString,
>+                                nscoord&          aLengthValue,

Usually we use pointers instead of non-const references for output parameters
so that it is clear at the call site which parameter gets (or may get)
modified.

Either

  static void ParseNumericValue(const nsString&   aString,
                                nscoord*          aLengthValue,

or

  static nscoord ParseNumericValue(const nsString&   aString,
                                   nscoord           aDefaultValue,

would be options for the interface.
The second option allows specifying a const default value and might work
particularly well if the the value.IsEmpty() optimizations are moved into one
of the ParseNumericValue functions, but ...

>   if (!value.IsEmpty()) {
>-    nsCSSValue cssValue;
>-    if (ParseNumericValue(value, cssValue) ||
>-        ParseNamedSpaceValue(mPresentationData.mstyle, value, cssValue))
>-    {
>-      if ((eCSSUnit_Number == cssValue.GetUnit()) && !cssValue.GetFloatValue())
>-        trailingSpace = 0;
>-      else if (cssValue.IsLengthUnit())
>-        trailingSpace = CalcLength(presContext, mStyleContext, cssValue);
>-      mFlags |= NS_MATHML_OPERATOR_RSPACE_ATTR;
>-    }
>+    ParseNumericValue(value, trailingSpace, 
>+                      nsMathMLElement::PARSE_ALLOW_UNITLESS,
>+                      presContext, mStyleContext);
>+    mFlags |= NS_MATHML_OPERATOR_RSPACE_ATTR;
>   }

... NS_MATHML_OPERATOR_RSPACE_ATTR is now set even when the attribute can't be
parsed.  Similarly for LSPACE.  Perhaps that doesn't matter and perhaps the
default values should really have already been set to zero here for accent,
but I don't even know if that's possible.  If we want to know whether
ParseNumericValue succeeded, then the second interface option is not so good.

Do you know what situation the ((eCSSUnit_Number == cssValue.GetUnit()) &&
!cssValue.GetFloatValue()) in the old code was detecting?

    
    

    
  
(In reply to Karl Tomlinson (:karlt) from comment #15)
> Comment on attachment 587413 [details] [diff] [review]
> Patch V1
> 
> I looked at much of this today.  Hopefully I can finish tomorrow.
> 
Thanks!

> Do you know what situation the ((eCSSUnit_Number == cssValue.GetUnit()) &&
> !cssValue.GetFloatValue()) in the old code was detecting?

The only case I see from the parsing functions is lspace="3". This should be treated as lspace="300%" but seems to be set to 0 instead. That's what the new code does, although I'm not sure what is done after is correct. The rounding operation seems to assume they are always absolute values.

    
    

    
  
Comment on attachment 587413 [details] [diff] [review]
Patch V1

This approach looks good, thanks.

Some key behavior changes (intentional I assume) that I see are:
No longer permit space after unary negative operator.
No longer permit space between +, a number, %, and unit in mpadded.
Removes non-default named space values.
linethickness, subscriptshift, superscriptshift now support named space values.

>+    // This flag enables a restricted syntax when ParseNumericValue is called
>+    // from nsMathMLmpaddedFrame.cpp.
>+    PARSE_CALLED_FROM_MPADDED = 0x04

I'd prefer a flag that indicated the change in function, rather than the
particular caller, but I don't think we need this flag at all.
CompressWhitepace is harmless.  As you point out above the unit won't be %.
Similarly, the number.unit concatenation won't match a named space.
For most code, simplicity is more important than speed.  Optimization is only
necessary for the code that runs the slowest.

>+  if (unit == eCSSUnit_Percent || unit == eCSSUnit_Number) {
>+    // Relative units. A multiple of the default length value is used.
>+    aLengthValue = NSToCoordRound(cssValue.GetFloatValue() *
>+                                  (float)aLengthValue);

Looks like nsCSSValue::GetFloatValue() will (?now) abort in debug builds if
unit == eCSSUnit_Percent.  Seems a shame to have a separate GetPercentValue()
call but looks like need to.

The (float) cast is unnecessary.

>+  // lspace

>+  // XXXfw Should we allow negative values? (bug 411227) They will be made
>+  // positive by the rounding below.

If the spec allows them, then I guess we should get as close as we can, which
may mean setting these to zero.  But no need to do that in this patch, as the
behavior is already strange, as you point out.

>+  // XXXfw A relative unit gives a multiple of the current leading space, which
>+  // is not necessarily the default one.

Wouldn't it be better to not set PARSE_ALLOW_UNITLESS until that is sorted
out?  Having the factor applied each time ProcessOperatorData() is called
doesn't sound good.

        Attachment #587413 -
        Flags: review?(karlt)

        Attachment #587413 -
        Flags: review-

        Attachment #587413 -
        Flags: feedback+

    
    

    
  
(In reply to Karl Tomlinson (:karlt) from comment #17)
> Comment on attachment 587413 [details] [diff] [review]
> Patch V1
> 
> This approach looks good, thanks.
> 
> Some key behavior changes (intentional I assume) that I see are:
> No longer permit space after unary negative operator.
> No longer permit space between +, a number, %, and unit in mpadded.
> Removes non-default named space values.
> linethickness, subscriptshift, superscriptshift now support named space
> values.

Yes, all of these changes conform with the MathML3 spec.

> >+  // XXXfw A relative unit gives a multiple of the current leading space, which
> >+  // is not necessarily the default one.
> 
> Wouldn't it be better to not set PARSE_ALLOW_UNITLESS until that is sorted
> out?  Having the factor applied each time ProcessOperatorData() is called
> doesn't sound good.

Note that relative units can be "unitless" numbers or "%". So not setting PARSE_ALLOW_UNITLESS will only prevent issues with the first one, I think.

    
    

    
  
(In reply to Karl Tomlinson (:karlt) from comment #15)
> >+  static void ParseNumericValue(const nsString&   aString,
> >+                                nscoord&          aLengthValue,
> 
> Usually we use pointers instead of non-const references for output parameters
> so that it is clear at the call site which parameter gets (or may get)
> modified.
> 
> Either
> 
>   static void ParseNumericValue(const nsString&   aString,
>                                 nscoord*          aLengthValue,
> 
> or
> 
>   static nscoord ParseNumericValue(const nsString&   aString,
>                                    nscoord           aDefaultValue,
> 
> would be options for the interface.
> The second option allows specifying a const default value and might work
> particularly well if the the value.IsEmpty() optimizations are moved into one
> of the ParseNumericValue functions, but ...
> 
> >   if (!value.IsEmpty()) {
> >-    nsCSSValue cssValue;
> >-    if (ParseNumericValue(value, cssValue) ||
> >-        ParseNamedSpaceValue(mPresentationData.mstyle, value, cssValue))
> >-    {
> >-      if ((eCSSUnit_Number == cssValue.GetUnit()) && !cssValue.GetFloatValue())
> >-        trailingSpace = 0;
> >-      else if (cssValue.IsLengthUnit())
> >-        trailingSpace = CalcLength(presContext, mStyleContext, cssValue);
> >-      mFlags |= NS_MATHML_OPERATOR_RSPACE_ATTR;
> >-    }
> >+    ParseNumericValue(value, trailingSpace, 
> >+                      nsMathMLElement::PARSE_ALLOW_UNITLESS,
> >+                      presContext, mStyleContext);
> >+    mFlags |= NS_MATHML_OPERATOR_RSPACE_ATTR;
> >   }
> 
> ... NS_MATHML_OPERATOR_RSPACE_ATTR is now set even when the attribute can't
> be
> parsed.  Similarly for LSPACE.  Perhaps that doesn't matter and perhaps the
> default values should really have already been set to zero here for accent,
> but I don't even know if that's possible.  If we want to know whether
> ParseNumericValue succeeded, then the second interface option is not so good.
> 

I'm going to choose the second interface, as I don't want to think too much about how we set these NS_MATHML_OPERATOR_*_ATTR flags for the moment. Maybe future improvements could be made when we fix bug 411227.

    
    

    
  
(In reply to Frédéric Wang from comment #18)
> > >+  // XXXfw A relative unit gives a multiple of the current leading space, which
> > >+  // is not necessarily the default one.
> > 
> > Wouldn't it be better to not set PARSE_ALLOW_UNITLESS until that is sorted
> > out?  Having the factor applied each time ProcessOperatorData() is called
> > doesn't sound good.
> 
> Note that relative units can be "unitless" numbers or "%". So not setting
> PARSE_ALLOW_UNITLESS will only prevent issues with the first one, I think.

Ah, yes.
Can you disable % too, then please?
The easiest way to do that might be to call nsMathMLElement::ParseNumericValue() and leave the old cssValue.IsLengthUnit() code.

(In reply to Frédéric Wang from comment #19)
> I'm going to choose the second interface, as I don't want to think too much
> about how we set these NS_MATHML_OPERATOR_*_ATTR flags for the moment. Maybe
> future improvements could be made when we fix bug 411227.

Sounds good.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch V2
        — 
          — Splinter Review
      

    


  

        Attachment #587413 -
        Attachment is obsolete: true

        Attachment #623646 -
        Flags: review?(karlt)

        Attachment #623646 -
        Flags: review?(karlt) → review+

    
    

    
  
https://tbpl.mozilla.org/?tree=Try&rev=9545b7e7c463
Keywords: checkin-needed
Whiteboard: [please push with the patch from bug 398038]

    
    

    
  
https://hg.mozilla.org/integration/mozilla-inbound/rev/a073a2fd27ec
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [please push with the patch from bug 398038]
Target Milestone: --- → mozilla15

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/a073a2fd27ec
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED

    
    

    
  
We should a dev doc note, mentioning that the syntax for mpadded and length attributes is now closer to the one of the MathML rec:

http://www.w3.org/TR/MathML3/chapter2.html#fund.units
http://www.w3.org/TR/MathML3/chapter3.html#id.3.3.6.2

We now only allow spaces at the beginning and at the end of the attribute value. See also comment 17.

I left some XXXfredw comments to be address in bug 411227...
Keywords: dev-doc-needed

    
    

    
  
Another important change: "0" is no longer allowed in mpadded (that's what the MathML rec says). However, we'll have to update our demo/test pages that use this syntax (e.g. the MathML torture test uses it twice).
No longer blocks: 411227, namedspaceOverriding, 398038

    
    

    
  


  
Apparently, we only use <mpadded> in the torture test. I'm not sure what it the purpose of these <mpadded> elements but this patch replaces width="0" by width="0em".

        Attachment #625700 -
        Flags: review?(karlt)

    
    

    
  
Comment on attachment 625700 [details] [diff] [review]
patch for the torture test

I expect the mpadded is to use the height of the mphantom without the width.

I wrote to www-math to confirm to that "0" should not be accepted.
http://lists.w3.org/Archives/Public/www-math/2012May/0026.html

        Attachment #625700 -
        Flags: review?(karlt) → review+

    
    

    
  
Comment on attachment 625700 [details] [diff] [review]
patch for the torture test

r105325

        Attachment #625700 -
        Flags: checkin+
Depends on: 757703

    
    

    
  
This has been documented here:

https://developer.mozilla.org/en/Firefox_15_for_developers#MathML

but I didn't give all the details. I don't know if the exact syntax is explained in MDN's MathML attribute doc.

    
    

    
  
(In reply to Frédéric Wang (:fredw) from comment #30)
> This has been documented here:
> 
> https://developer.mozilla.org/en/Firefox_15_for_developers#MathML
> 
> but I didn't give all the details. I don't know if the exact syntax is
> explained in MDN's MathML attribute doc.

I think I don't understand all the changes in detail made in this bug and in bug 757703 in order to write proper documentation notes about it. I guess this won't need so much docs, but  probably it's way more easier for you to dev-doc-complete this bug and bug 757703 if we really need more explanation about this out there.

    
    

    
  
I think "Supported syntax for Length and <mpadded> values have been made closer to the one specified in MathML3 spec." is enough. As I said, the details are not given in

https://developer.mozilla.org/en-US/docs/MathML/Attributes/Values

or

https://developer.mozilla.org/en-US/docs/MathML/Element/mpadded

anyway.

    
    

    
  
Marking as complete then.
Keywords: dev-doc-needed → dev-doc-complete

    
  
Blocks: 1274789

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: