97027 - Misuse of ParseValueOrPercentOrProportional

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Bernd]

http://lxr.mozilla.org/seamonkey/ident?i=ParseValueOrPercentOrProportional
shows that we use the parse mechanism for multilength variables (
http://www.w3.org/TR/html4/types.html#type-multi-length )
 also for widths of
tables,
cells and
rows !!!

Later we ignore the proportional values as required by the spec.
We should replace ParseValueOrPercentOrProportional with ParseValueOrPercent in
the table and the cell case and remove the statement for the row at all. I see
two   advantages:
- we will use faster routines in the most common cases
- code removal

Comment 1

[Bernd]

Attachment: patch

Comment 2

[Bernd]

looks like the diff is a little bit confusing

old stuff including a nasty hack 


 PRBool
 nsGenericHTMLElement::ParseValueOrPercentOrProportional(const
nsAReadableString& aString,
 nsHTMLValue& aResult,
 nsHTMLUnit aValueUnit)
 {
 nsAutoString tmp(aString);
 tmp.CompressWhitespace(PR_TRUE, PR_TRUE);
 PRInt32 ec, val = tmp.ToInteger(&ec);
 if ((NS_ERROR_ILLEGAL_VALUE == ec) && (tmp.Length() > 0)) {
 // NOTE: we need to allow non-integer values for the '*' case,
 //       so we allow for the ILLEGAL_VALUE error and set val to 0
  val = 0;
  ec = NS_OK;
 }
 if (NS_OK == ec) {
   if (val < 0) val = 0;
   if (tmp.Length() && tmp.RFindChar('%') >= 0) {/* XXX not 100% compatible with
ebina's code */
     if (val > 100) val = 100;
       aResult.SetPercentValue(float(val)/100.0f);
     } else if (tmp.Length() && tmp.Last() == '*') {
     if (tmp.Length() == 1) {
       // special case: HTML spec says a value '*' == '1*'
       // see http://www.w3.org/TR/html4/types.html#type-multi-length
       // b=29061
       val = 1;
     }
     aResult.SetIntValue(val, eHTMLUnit_Proportional); // proportional values
are integers
   } else {
     if (eHTMLUnit_Pixel == aValueUnit) {
      aResult.SetPixelValue(val);
     }
     else {
      aResult.SetIntValue(val, aValueUnit);
     }
   }
   return PR_TRUE;
  }
 return PR_FALSE;
}


rewritten function



PRBool
nsGenericHTMLElement::ParseValueOrPercentOrProportional(const nsAReadableString&
aString,
                                                        nsHTMLValue& aResult,
                                                        nsHTMLUnit aValueUnit)
{
  nsAutoString tmp(aString);
  tmp.CompressWhitespace(PR_TRUE, PR_TRUE);
  PRInt32  ec, val = tmp.ToInteger(&ec);
  if (NS_OK == ec) {
    if (val < 0) val = 0;
    if (tmp.Length() && tmp.RFindChar('%') >= 0) {/* XXX not 100% compatible
with ebina's code */
      if (val > 100) val = 100;
      aResult.SetPercentValue(float(val)/100.0f);
    } else if (tmp.Length() && tmp.Last() == '*') {
      if (tmp.Length() == 1) {
        // special case: HTML spec says a value '*' == '1*'
        // see http://www.w3.org/TR/html4/types.html#type-multi-length
        // b=29061
        val = 1;
      }
      aResult.SetIntValue(val, eHTMLUnit_Proportional); // proportional values
are integers
    } else if (eHTMLUnit_Pixel == aValueUnit) {
        aResult.SetPixelValue(val);
      }
      else {
        aResult.SetIntValue(val, aValueUnit);
    } 
    return PR_TRUE;
  }
  else if (tmp.Length()==1 && tmp.Last()== '*') {
    aResult.SetIntValue(1, eHTMLUnit_Proportional);
    return PR_TRUE;
  }  
  
  return PR_FALSE;
}

Comment 3

[karnaze (gone)]

Comment on attachment 53471 [details] [diff] [review]
patch

Bernd, please make sure that no other elements having proportional values are advesely affected.

Comment 4

[Bernd]

http://lxr.mozilla.org/seamonkey/search?string=PercentOrProportional

shows that only table uses this function

Comment 5

[Marc Attinasi]

Comment on attachment 53471 [details] [diff] [review]
patch

sr=attinasi - you could clean up the tabbing of the bracket right before the 'else' too if you want:
+    return PR_TRUE;
     }
+  else if ...

Comment 6

[Bernd]

fix checked in