232706 - nsHTMLValue must die too

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

nsHTMLValue supports a bunch of unneccesary types which we don't need any more.
nsAttrValue is smaller and has a better name :).

First off the types empty, null and colorname should be removed in favour of
normal strings. There is a risk with removing colorname since we wouldn't know
if the string is an invalid colorname and we'd waste time finding that out every
time we map the attribute into a rule. However in quirks pages all
attribute-values are parsed into either a colorname or a colorvalue so there
there's no such thing as an invalid colorname.
So we would only take a perfhit on strict pages with a lot of invalid
colornames, which i doubt is very common.

Second i'm pretty sure that we can remove the pixel type in favour of just
integers. The value itself doesn't have a unit, the attribute does.

Third enums are evils since they can't be converted into strings. Instead we
should just store atoms since they are supported by nsAttrValue anyway.


Hopefully after this is done we will be able to store most attributes in an
nsAttrValue without having to malloc an extra structure.

Another thing that we might want to do is to not use an nsCOMArray<nsIAtom> to
store the atomlist. The reason is that an nsCOMArray is basically just a pointer
to the actual buffer. So we end up with a pointer to a pointer to the buffer,
whereas we really want a pointer to a buffer. nsSmallVoidArray actually has the
same problem.
Ideally there would be some sort of buffer-manager-functions that operate on
just a bufferpointer and reallocate and modify it as appropriate. These
functions could then be used by nsVoidArray, nsSmallVoidArray and us.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Remove types from nsHTMLValue

This removes the following types from nsHTMLValue: eHTMLUnit_Empty,
eHTMLUnit_Null and eHTMLUnit_ColorName are all replaced by normal
eHTMLUnit_String. eHTMLUnit_Pixel is replaced by integers.

The patch also changes how we convert parsed attributes to strings: Instead of
first calling AttributeToString and then falling back to the default conversion
in  nsHTMLValue::ToString we instead use nsHTMLValue::ToString for everything
except enums and just call AttributeToString on enums.

There were only two cases where we handled other datatypes then enums in
AttributeToString: the code for the 'checked' attribute on <input> converted
all values into the string "checked". This was done as part of bug 53860.
However that should work fine anyway since we no longer convert all values to
eHTMLUnit_Empty in StringToAttribute.

The other case is for the size attribute of <font> elements where we need to
differentiate "+3" from "3". The old code would parse "+3" "-2" etc into
eHTMLUnit_Integer, and "1" "6" etc into eHTMLUnit_Enumerated. I switched this
so that "+3" "-2" are enumerated and "1" "6" are integers. That way the default
conversion works for integers and the relative units are treated like an
enumerated value that we convert to a string in AttributeToString.

Other then that it should be pretty straigtforward. I checked all places where
we use eHTMLUnit_String to make sure that all such places could now deal with
the old types being strings.

I'd like to get this patch checked in before i do more work on killing
nsHTMLValue since it feels like this is the most risky part and I want to have
as much testing as possible on it. If we can get it in 1.7a then that's great,
but early 1.7b should be enough too.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Remove types from nsHTMLValue -w

Same as previous diffed with -w

I forgot to mention, this patch applies on top of a tree with the latest patch
from bug 195350 and depends on that patch being checked in. (we would otherwise
have to deal with converting nsISupports values to strings too).

Comment 3

[Johnny Stenback (:jst)]

Comment on attachment 140491 [details] [diff] [review]
Remove types from nsHTMLValue  -w

- In nsGenericHTMLElement::GetAttr():

+  // Use subclass to convert enum to string.
   if (aNameSpaceID == kNameSpaceID_None && 
-      AttributeToString(aAttribute, *value, aResult) ==
+      AttributeToString(aAttribute, *attrValue->GetHTMLValue(), aResult) ==
       NS_CONTENT_ATTR_HAS_VALUE) {
     return NS_CONTENT_ATTR_HAS_VALUE;
   }

...
   NS_NOTREACHED("no value to string conversion found");
   return NS_CONTENT_ATTR_NOT_THERE;
 }

Flip that around such that the normal flow goes to the end of the function, and
the error case returns early.

- In nsHTMLInputElement::StringToAttribute():

+    // XXX ARG!! This is major evilness. StringToAttribute
+    // shouldn't set members. Override SetAttr instead

Is there a bug on file for this? :-)

sr=jst

Comment 4

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 140491 [details] [diff] [review]
Remove types from nsHTMLValue  -w

>--- xulattr/mozilla/content/html/content/src/nsHTMLFontElement.cpp	2004-01-26 13:22:04.000000000 -0600
>+++ trunk/mozilla/content/html/content/src/nsHTMLFontElement.cpp	2004-02-03 00:07:28.000000000 -0600
>@@ -174,14 +174,14 @@ nsHTMLFontElement::StringToAttribute(nsI
>       //rickg: fixed flaw where ToInteger error code was not being checked.
>       //       This caused wrong default value for font size.
>     PRInt32 ec, v = tmp.ToInteger(&ec);
>     if(NS_SUCCEEDED(ec)){
>       tmp.CompressWhitespace(PR_TRUE, PR_FALSE);
>       PRUnichar ch = tmp.IsEmpty() ? 0 : tmp.First();
>-      aResult.SetIntValue(v, ((ch == '+') || (ch == '-')) ?
>-                          eHTMLUnit_Integer : eHTMLUnit_Enumerated);
>+      aResult.SetIntValue(v, (ch == '+' || ch == '-') ?
>+                             eHTMLUnit_Enumerated : eHTMLUnit_Integer);

Did you mean to swap these around?

Ok I see that you did after reading further.

r=caillon.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Move parsing and storage from nsHTMLValue to nsAttrValue, v1

This patch makes nsAttrValue able to store all the datatypes that nsHTMLValue
can rather then storing an nsHTMLValue internally. This way storage is a lot
more optimal and a lot faster.

It also makes all the parsingcode (StringToAttribute) use nsAttrValue rather
then nsHTMLValue. Since I had to touch all the signatures of StringToAttribute
i cleaned up it's signature by renaming it ParseAttribute and making it return
a bool rather then an nsresult.

I also fixed few users of nsHTMLValue that used nsHTMLValue to convert nscolors
to strings by adding an NS_RGBToHex function to nsColor.h and killed off a lot
of code that ended up no being used any more in nsHTMLValue.


Note: In the new StringToAttribute code there is a lot of else-after-returns.
The reason for that is that I didn't want to touch those lines since that would
make the diff a lot harder to read. I'd happily fix them, either in this patch
or later, but I wanted to give you a chance to review the code this way first.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 140491 [details] [diff] [review]
Remove types from nsHTMLValue  -w

sorry, i should mention that this one was checked in february 10th

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I did some thinking on this and i think i've limited the values we can store a
bit too much.

Right now ParseWithBounds limits the bounds to 16-bit integers, meaning we'll
only store values between -32768 and 32767. However there are enough bits
available in nsAttrValue to store values between -67108863 and 67108864, which
should be plenty for anyone. So I'm going to change the bounds back to 32-bit
integers and instead put an assert in ParseIntWithBounds to make sure that we
keep within the bounds.

Comment 8

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 142467 [details] [diff] [review]
Move parsing and storage from nsHTMLValue to nsAttrValue, v1

Since you're going to rename |Type| anyway, I think that |AttrType| is a much
better name than |TypeEnum|.

ResetIfSet() is kind of unnecessary and kind of obscures the code IMO.	I think
it would be better to just write out

  if (mBits) {
    Reset();
  }

It's not a lot of code, and it makes it very explicit what you are checking
against.


>Index: content/base/src/nsAttrValue.cpp

>+nsAttrValue::TypeEnum
>+nsAttrValue::Type() const
> {
>-  Reset();
>+  switch (BaseType()) {
>+    case eIntegerBase:
>+    {
>+      return NS_STATIC_CAST(TypeEnum, mBits & NS_ATTRVALUE_INTEGERTYPE_MASK);
>+    }
>+    case eOtherBase:
>+    {
>+      return GetMiscContainer()->mType;
>+    }
>+    default:
>+    {
>+      return NS_STATIC_CAST(TypeEnum, BaseType());
>+    }
>+  }
>+
>+  NS_NOTREACHED("shouldn't get there");
>+  return eString;

Not only shouldn't control get here, but it is also impossible.  Compilers
nowadays are smart enough to realize that (and not warn), so just remove the
above two lines.


> }
> 

> void
> nsAttrValue::SetTo(const nsAttrValue& aOther)
> {
...
>+    case eAtomBase:
>     {
>-      SetTo(*aOther.GetHTMLValue());
>+      ResetIfSet();
>+      nsIAtom* atom = aOther.GetAtomValue();
>+      NS_ADDREF(atom);
>+      SetPtrValueAndType(atom, eAtomBase);

Maybe this should happen in a separate patch but I think SetPtrValueAndType()
should probably handle the addref itself.  Same for the CSSStyleRule version of
this.

>+      return;
>+    }
>+    case eIntegerBase:
>+    {
>+      ResetIfSet();
>+      mBits = aOther.mBits;
>+      return;      

Uber nit: You have a bunch of whitespace after the above return statement. 
Like, don't put that there.

...

>+    default:
>     {
>-      SetTo(aOther.GetAtomValue());
>+      NS_NOTREACHED("unknown type stored in other-struct");

Just call it "aOther" (in the assert text) to make it clearer what you're
talking about.

>       break;
>     }
>   }
> }




>@@ -194,111 +241,615 @@ nsAttrValue::SwapValueWith(nsAttrValue& 
>   mBits = tmp;
> }
> 
> void
> nsAttrValue::ToString(nsAString& aResult) const
> {
>-  void* ptr = GetPtr();
>-
>-  switch(GetType()) {
>+  switch(Type()) {
...
>+    case eAtomArray:
>+    {
>+      MiscContainer* cont = GetMiscContainer();
>+      PRInt32 count = cont->mAtomArray->Count();
>+      if (count) {
>+        cont->mAtomArray->ObjectAt(0)->ToString(aResult);
>+        nsAutoString tmp;
>+        PRInt32 i;
>+        for (i = 1; i < count; ++i) {
>+          cont->mAtomArray->ObjectAt(i)->ToString(tmp);
>+          aResult.Append(NS_LITERAL_STRING(" ") + tmp);
>+        }
>+      }
>+      break;
>+    }

You probably want to truncate aResult if count is 0.




> PRUint32
> nsAttrValue::HashValue() const
> {
>-  void* ptr = GetPtr();
>-  switch(GetType()) {
>-    case eString:
>+  switch(BaseType()) {
>+    case eStringBase:
>     {
>-      PRUnichar* str = NS_STATIC_CAST(PRUnichar*, ptr);
>+      PRUnichar* str = NS_STATIC_CAST(PRUnichar*, GetPtr());
>       return str ? nsCheapStringBufferUtils::HashCode(str) : 0;
>     }
>-    case eHTMLValue:
>+    case eOtherBase:
>     {
>-      return NS_STATIC_CAST(nsHTMLValue*, ptr)->HashValue();
>+      break;
>     }
>-    case eAtom:
>+    case eAtomBase:
>+    case eIntegerBase:
>     {
>-      return NS_PTR_TO_INT32(ptr);
>+      // mBits and PRUint32 might have different size. This should silence
>+      // any warnings or compile-errors. This is what the implementation of
>+      // NS_PTR_TO_INT32 does to take care of the same problem.
>+      return mBits - 0;

I don't really see a reason for this change.  You don't need to explain it if
you just use the macro.  If you think that the intermediate cast to char* in
the macro is unnecessary, then perhaps you should file a bug to fix the macro.



> PRBool
> nsAttrValue::Equals(const nsAttrValue& aOther) const
> {
...
>+  switch (thisCont->mType) {
>+    case eColor:
>+    {
>+      return thisCont->mColor == otherCont->mColor;
>+    }
>+    case eCSSStyleRule:
>+    {
>+      return thisCont->mCSSStyleRule == otherCont->mCSSStyleRule;
>+    }
>+    case eAtomArray:
>+    {
...
>+      return PR_TRUE;
>+    }
>+    default:
>+    {
>+      break;
>+    }
>+  }
>+  
>+  NS_NOTREACHED("unknown type stored in other-struct");
>   return PR_FALSE;

You can just move the above into the default case, so you don't need the
additional break statement (though it should optimize out anyway).



>+void
>+nsAttrValue::ParseAtomArray(const nsAString& aValue)
>+{
>+  nsAString::const_iterator iter, end;
>+  aValue.BeginReading(iter);
>+  aValue.EndReading(end);
>+
>+  // skip initial whitespace
>+  while (iter != end && nsCRT::IsAsciiSpace(*iter)) {
>+    ++iter;
>+  }
>+
>+  if (iter == end) {
>+    ResetIfSet();
>+
>+    return;
>+  }
>+
>+  nsAString::const_iterator start(iter);
>+
>+  // get first, and often only, atom

This comment took me a while to parse.	I suppose it works, but it would
definitely be clearer here IMO if you used a sentence: "Get the first - and
often the only - atom."


>+void
>+nsAttrValue::ParseStringOrAtom(const nsAString& aValue)
>+{
>+  PRUint32 len = aValue.Length();
>+  // Don't bother with atoms if it's an empty string since
>+  // we can store those efficently anyway.
>+  if (len && len <= NS_ATTRVALUE_MAX_STRINGLENGTH_ATOM) {
>+    ParseAtom(aValue);
>+  }
>+  else {
>+    SetTo(aValue);
>+  }
>+}
>+
>+PRBool
>
>+nsAttrValue::ParseEnumValue(const nsAString& aValue,

Get rid of the empty new line in between the type and the signature.


>+PRBool
>+nsAttrValue::ParseSpecialIntValue(const nsAString& aString,
>+                                  PRBool aCanBePercent,
>+                                  PRBool aCanBeProportional)
>+{
>+  PRInt32 ec;
>+  nsAutoString tmp(aString);
>+  PRInt32 val = tmp.ToInteger(&ec);
>+
>+  if (NS_FAILED(ec) && aCanBeProportional) {
>+    // Even if the integer could not be parsed, it might just be "*"
>+    tmp.CompressWhitespace(PR_TRUE, PR_TRUE);
>+    if (tmp.Length() == 1 && tmp.Last() == '*') {
>+      // special case: HTML spec says a value '*' == '1*'
>+      // see http://www.w3.org/TR/html4/types.html#type-multi-length
>+      // bug 29061
>+      SetIntValueAndType(1, eProportional);
>+      return PR_TRUE;
>+    }
>+  }
>+
>+  if (val < 0) {
>+    val = 0;
>+  }
>+
>+  // % (percent) (XXX RFindChar means that 5%x will be parsed!)

Try to avoid putting an XXX comment on the same line as a valid comment?  It
can get to be too confusing...

>+  if (aCanBePercent && tmp.RFindChar('%') >= 0) {
>+    if (val > 100) {
>+      val = 100;
>+    }
>+    SetIntValueAndType(val, ePercent);
>+    return PR_TRUE;
>+  }
>+
>+  // * (proportional) (XXX RFindChar means that 5*x will be parsed!)

Same.

>+  if (aCanBeProportional && tmp.RFindChar('*') >= 0) {
>+    SetIntValueAndType(val, eProportional);
>+    return PR_TRUE;
>+  }
>+
>+  // Straight number is interpreted as integer
>+  SetIntValueAndType(val, eInteger);
>+  return PR_TRUE;
>+}
>+
>+PRBool
>+nsAttrValue::ParseIntWithBounds(const nsAString& aString,
>+                                PRInt16 aMin, PRInt16 aMax)
>+{
>+  NS_PRECONDITION(aMin < aMax, "bad boundrys");

boundaries

>+
>+  PRInt32 ec;
>+  PRInt32 val = PromiseFlatString(aString).ToInteger(&ec);
>+  if (NS_SUCCEEDED(ec)) {
>+    val = PR_MAX(val, aMin);
>+    val = PR_MIN(val, aMax);
>+    SetIntValueAndType(val, eInteger);
>+
>+    return PR_TRUE;
>+  }
>+
>+  return PR_FALSE;

Change this to return early if NS_FAILED(ec).

>+}
>+
>+PRBool
>+nsAttrValue::ParseColor(const nsAString& aString, nsIDocument* aDocument)
>+{
>+  static const char* kWhitespace = " \n\r\t\b";

What is this for?  It doesn't seem to be used at all.

>+
>+  nsAutoString colorStr(aString);
>+  colorStr.CompressWhitespace(PR_TRUE, PR_TRUE);
>+  if (colorStr.IsEmpty()) {
>+    return PR_FALSE;
>+  }
>+
>+  nscolor color;
>+  // No color names begin with a '#', but numerical colors do so
>+  // it is a very common first char
>+  if ((colorStr.CharAt(0) != '#') && NS_ColorNameToRGB(colorStr, &color)) {
>+    SetTo(colorStr);
>+    return PR_TRUE;
>+  }
>+
>+  // Check if we are in compatibility mode
>+  // XXX evil NS_HexToRGB and NS_LooseHexToRGB takes nsString as argument!

Another language nit, plural subjects get a singular verb form, so NS_HexToRGB
and NS_LooseHexToRGB take, not takes.


> class nsAttrValue {
> public:
>   nsAttrValue();
>   nsAttrValue(const nsAttrValue& aOther);
>   explicit nsAttrValue(const nsAString& aValue);
>-  explicit nsAttrValue(const nsHTMLValue& aValue);
>-  explicit nsAttrValue(nsIAtom* aValue);
>+  //explicit nsAttrValue(nsIAtom* aValue);
>+  explicit nsAttrValue(nsICSSStyleRule* aValue);
>+  //explicit SetTo(PRInt16 aInt, TypeEnum aType);

Why comment?  Just remove them.


>+  /**
>+   * Map a string to its enum value and return result as HTMLValue

return?  you mean store, right?

>+   * (case-insensitive matching)
>+   *
>+   * @param aValue the string to find the value for
>+   * @param aTable the enumeration to map with
>+   * @param aResult the enum mapping [OUT]
>+   * @return whether the enum value was found or not
>+   */
>+  PRBool ParseEnumValue(const nsAString& aValue,
>+                        const nsHTMLValue::EnumTable* aTable,
>+                        PRBool aCaseSensitive = PR_FALSE);
>+
>+  /**
>+   * Parse a string value into an int HTMLValue with minimum
>+   * value and maximum value (can optionally parse percent (n%) and
>+   * proportional (n*).  This method explicitly sets a lower bound of zero on
>+   * the element, whether it be proportional or percent or raw integer.
>+   *
>+   * @param aString the string to parse
>+   * @param aCanBePercent true if it can be a percent value (%)

PR_TRUE

>+   * @param aCanBeProportional true if it can be a proportional value (*)

PR_TRUE

>+   * @return whether the value could be parsed
>+   */
>+  PRBool ParseSpecialIntValue(const nsAString& aString,
>+                              PRBool aCanBePercent,
>+                              PRBool aCanBeProportional);
>+
>+  /**
>+   * Parse a string value into an integer with minimum value and maximum value.
>+   *
>+   * @param aString the string to parse
>+   * @param aMin the minimum value (if value is less it will be bumped up)
>+   * @param aMax the maximum value (if value is greater it will be chopped down)
>+   * @return whether the value could be parsed
>+   */

Looks like you are commenting the wrong method here.  Also, why just 16 bit out
of curiosity?

>+  PRBool ParseIntValue(const nsAString& aString) {
>+    return ParseIntWithBounds(aString, PR_INT16_MIN, PR_INT16_MAX);
>   }
>+  PRBool ParseIntWithBounds(const nsAString& aString,
>+                            PRInt16 aMin, PRInt16 aMax = PR_INT16_MAX);
>+
>+  /**
>+   * Parse a string into a color (with hexes or color names)

I'd like it better if you said "hex color values" or something.  Hexes are
spells you cast on people.

>+   *
>+   * @param aString the string to parse
>+   * @param aDocument the document (to find out whether we're in quirks mode)
>+   * @return whether the value could be parsed
>+   */
>+  PRBool ParseColor(const nsAString& aString, nsIDocument* aDocument);



>Index: content/html/content/src/nsGenericHTMLElement.h
>===================================================================
>RCS file: /cvsroot/mozilla/content/html/content/src/nsGenericHTMLElement.h,v
>retrieving revision 1.175
>diff -u -6 -p -r1.175 nsGenericHTMLElement.h
>--- content/html/content/src/nsGenericHTMLElement.h	24 Feb 2004 23:55:17 -0000	1.175
>+++ content/html/content/src/nsGenericHTMLElement.h	27 Feb 2004 22:04:55 -0000
>@@ -232,20 +232,19 @@ public:
>   /**
>    * Convert an attribute string value to attribute type based on the type of
>    * attribute.  Called by SetAttr().
>    *
>    * @param aAttribute to attribute to convert
>    * @param aValue the string value to convert
>-   * @param aResult the HTMLValue [OUT]
>-   * @return NS_CONTENT_ATTR_HAS_VALUE if the string was successfully converted
>-   *         NS_CONTENT_ATTR_NOT_THERE if the string could not be converted
>+   * @param aResult the nsAttrValue [OUT]
>+   * @return PR_TRUE if the parsing was successfull, PR_FALSE otherwise

successful





>Index: content/html/content/src/nsHTMLInputElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/html/content/src/nsHTMLInputElement.cpp,v
>retrieving revision 1.331
>diff -u -6 -p -r1.331 nsHTMLInputElement.cpp
>--- content/html/content/src/nsHTMLInputElement.cpp	24 Feb 2004 23:55:17 -0000	1.331
>+++ content/html/content/src/nsHTMLInputElement.cpp	27 Feb 2004 22:04:56 -0000
>@@ -174,15 +174,15 @@ public:
>   virtual PRBool AllowDrop();
> 
>   // nsIContent
>   virtual void SetFocus(nsIPresContext* aPresContext);
>   virtual void RemoveFocus(nsIPresContext* aPresContext);
> 
>-  NS_IMETHOD StringToAttribute(nsIAtom* aAttribute,
>-                               const nsAString& aValue,
>-                               nsHTMLValue& aResult);
>+  virtual PRBool ParseAttribute(nsIAtom* aAttribute,
>+                                const nsAString& aValue,
>+                                nsAttrValue& aResult);
>   NS_IMETHOD AttributeToString(nsIAtom* aAttribute,
>                                const nsHTMLValue& aValue,
>                                nsAString& aResult) const;
>   NS_IMETHOD GetAttributeChangeHint(const nsIAtom* aAttribute,
>                                     PRInt32 aModType,
>                                     nsChangeHint& aHint) const;
>@@ -1699,85 +1699,64 @@ static const nsHTMLValue::EnumTable kInp
>   { "radio", NS_FORM_INPUT_RADIO },
>   { "submit", NS_FORM_INPUT_SUBMIT },
>   { "text", NS_FORM_INPUT_TEXT },
>   { 0 }
> };
> 
>-NS_IMETHODIMP
>-nsHTMLInputElement::StringToAttribute(nsIAtom* aAttribute,
>-                                      const nsAString& aValue,
>-                                      nsHTMLValue& aResult)
>+PRBool
>+nsHTMLInputElement::ParseAttribute(nsIAtom* aAttribute,
>+                                   const nsAString& aValue,
>+                                   nsAttrValue& aResult)
> {
>   if (aAttribute == nsHTMLAtoms::type) {
>-    // XXX ARG!! This is major evilness. StringToAttribute
>+    // XXX ARG!! This is major evilness. ParseAttribute
>     // shouldn't set members. Override SetAttr instead
>-    const nsHTMLValue::EnumTable *table = kInputTypeTable;
>-    nsAutoString valueStr(aValue);
>-    while (nsnull != table->tag) { 
>-      if (valueStr.EqualsIgnoreCase(table->tag)) {
>+    PRBool res = aResult.ParseEnumValue(aValue, kInputTypeTable);
>+    if (res) {
>+      mType = aResult.GetEnumValue();
>+      if (mType == NS_FORM_INPUT_FILE) {
>         // If the type is being changed to file, set the element value
>         // to the empty string. This is for security.
>-        if (table->value == NS_FORM_INPUT_FILE) {
>-          SetValue(EmptyString());
>-        }
>-        aResult.SetIntValue(table->value, eHTMLUnit_Enumerated);
>-        mType = table->value;  // set the type of this input
>-        return NS_CONTENT_ATTR_HAS_VALUE;
>+        SetValue(EmptyString());
>       }
>-      table++;
>     }
>-    // Unknown type.  We treat this as "text", but we set it as a string, not
>-    // as an HTMLValue.
>-    mType = NS_FORM_INPUT_TEXT;
>+    else {
>+      mType = NS_FORM_INPUT_TEXT;
>+    }
>+    return res;


How about:

    if (!aResult.ParseEnumValue(aValue, kInputTypeTable)) {
      mType = NS_FORM_INPUT_TEXT;
      return PR_FALSE;
    }

    mType = aResult.GetEnumValue();
    if (mType == NS_FORM_INPUT_FILE) {
      ...
    }

    return PR_TRUE;


And all these "else-if-after-return" following can be turned into simply
if-then-return statements.

>   }
>   else if (aAttribute == nsHTMLAtoms::width) {
>-    if (aResult.ParseSpecialIntValue(aValue, eHTMLUnit_Integer, PR_TRUE, PR_FALSE)) {
>-      return NS_CONTENT_ATTR_HAS_VALUE;
>-    }
>+    return aResult.ParseSpecialIntValue(aValue, PR_TRUE, PR_FALSE);
>   }
>   else if (aAttribute == nsHTMLAtoms::height) {
>-    if (aResult.ParseSpecialIntValue(aValue, eHTMLUnit_Integer, PR_TRUE, PR_FALSE)) {
>-      return NS_CONTENT_ATTR_HAS_VALUE;
>-    }
>+    return aResult.ParseSpecialIntValue(aValue, PR_TRUE, PR_FALSE);
>   }
>   else if (aAttribute == nsHTMLAtoms::maxlength) {
>-    if (aResult.ParseIntWithBounds(aValue, eHTMLUnit_Integer, 0)) {
>-      return NS_CONTENT_ATTR_HAS_VALUE;
>-    }
>+    return aResult.ParseIntWithBounds(aValue, 0);
>   }
>   else if (aAttribute == nsHTMLAtoms::size) {
>-    if (aResult.ParseIntWithBounds(aValue, eHTMLUnit_Integer, 0)) {
>-      return NS_CONTENT_ATTR_HAS_VALUE;
>-    }
>+    return aResult.ParseIntWithBounds(aValue, 0);
>   }
>   else if (aAttribute == nsHTMLAtoms::tabindex) {
>-    if (aResult.ParseIntWithBounds(aValue, eHTMLUnit_Integer, 0)) {
>-      return NS_CONTENT_ATTR_HAS_VALUE;
>-    }
>+    return aResult.ParseIntWithBounds(aValue, 0, 32767);
>   }
>   else if (aAttribute == nsHTMLAtoms::border) {
>-    if (aResult.ParseIntWithBounds(aValue, eHTMLUnit_Integer, 0)) {
>-      return NS_CONTENT_ATTR_HAS_VALUE;
>-    }
>+    return aResult.ParseIntWithBounds(aValue, 0);
>   }
>   else if (aAttribute == nsHTMLAtoms::align) {
>-    if (ParseAlignValue(aValue, aResult)) {
>-      return NS_CONTENT_ATTR_HAS_VALUE;
>-    }
>+    return ParseAlignValue(aValue, aResult);
>   }
>-  else {
>+  else if (ParseImageAttribute(aAttribute, aValue, aResult)) {
>     // We have to call |ParseImageAttribute| unconditionally since we
>     // don't know if we're going to have a type="image" attribute yet,
>     // (or could have it set dynamically in the future).  See bug
>     // 214077.
>-    if (ParseImageAttribute(aAttribute, aValue, aResult)) {
>-      return NS_CONTENT_ATTR_HAS_VALUE;
>-    }
>+    return PR_TRUE;
>   }
> 
>-  return NS_CONTENT_ATTR_NOT_THERE;
>+  return nsGenericHTMLElement::ParseAttribute(aAttribute, aValue, aResult);
> }
> 
> NS_IMETHODIMP
> nsHTMLInputElement::AttributeToString(nsIAtom* aAttribute,
>                                       const nsHTMLValue& aValue,
>                                       nsAString& aResult) const











>Index: content/html/content/src/nsHTMLTableCellElement.cpp
>-#define MAX_COLSPAN 8190
>-#define MAX_ROWSPAN 8190 // celldata.h can not handle more
>+#define MAX_COLROWSPAN 8190 // celldata.h can not handle more

In theory, one could want to decrease one or the other but not both.  Or
someone could fix celldata.h to handle more, and then we may want to increase
one of the numbers or something. It probably doesn't matter that much but I'd
rather make this change in a different bug, really and not here.









>Index: content/shared/public/nsHTMLValue.h
> class nsHTMLValue {
>@@ -246,95 +228,31 @@ public:
>    * }
>    */
>   struct EnumTable {
>     /** The string the value maps to */
>     const char* tag;
>     /** The enum value that maps to this string */
>-    PRInt32 value;
>+    PRInt16 value;

As you already noted, ack :)


>Index: gfx/public/nsColor.h
>===================================================================
>RCS file: /cvsroot/mozilla/gfx/public/nsColor.h,v
>retrieving revision 1.17
>diff -u -6 -p -r1.17 nsColor.h
>--- gfx/public/nsColor.h	18 Mar 2003 05:43:11 -0000	1.17
>+++ gfx/public/nsColor.h	27 Feb 2004 22:05:02 -0000
>@@ -90,12 +90,16 @@ extern "C" NS_GFX_(PRBool) NS_HexToRGB(c
> 
> // Translate a hex string to a color. Return true if it parses ok,
> // otherwise return false.
> // This version accepts 1 to 9 digits (missing digits are 0)
> extern "C" NS_GFX_(PRBool) NS_LooseHexToRGB(const nsString& aBuf, nscolor* aResult);
> 
>+// Translate a color to a hex string and prepend a '#'.
>+// The returned string is always 7 digits including '#' character.

This comment is wrong for two reasons:

1) the '#' character is not a digit.  you probably meant 'character'...

>+extern "C" NS_GFX_(void) NS_RGBToHex(nscolor aColor, nsAString& aResult)
>+{
>+  char buf[10];
>+  PR_snprintf(buf, sizeof(buf), "#%02x%02x%02x",
>+              NS_GET_R(aColor), NS_GET_G(aColor), NS_GET_B(aColor));

and 2)

  nsAutoString foo = NS_LITERAL_STRING("foo");
  NS_RGBToHex(0, foo); // foo contains 10 characters, not 7.

you should probably truncate here, or something...

>+  AppendASCIItoUTF16(buf, aResult);
> }
> 
> extern "C" NS_GFX_(PRBool) NS_ColorNameToRGB(const nsAString& aColorName, nscolor* aResult)
> {
>   nsColorName id = nsColorNames::LookupName(aColorName);
>   if (eColorName_UNKNOWN < id) {

Also, please clear the additional GFX method with someone who can lay claim to
GFX, like pavlov or blizzard or someone.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> ResetIfSet() is kind of unnecessary and kind of obscures the code IMO.	
> I think it would be better to just write out
> 
>   if (mBits) {
>     Reset();
>   }
> 
> It's not a lot of code, and it makes it very explicit what you are checking
> against.

It also spreads out the logic for what qualifies as an 'unset' value. I'd rather
keep things the way they are.

> Maybe this should happen in a separate patch but I think SetPtrValueAndType()
> should probably handle the addref itself.  Same for the CSSStyleRule version 
> of this.

Note that CSSStyleRules don't go through SetPtrValueAndType since it only lives
in the MiscContainer. So only one out of 3 types needs addreffing. I'd rather
keep it the way it is.

> I don't really see a reason for this change.  You don't need to explain it if
> you just use the macro.  If you think that the intermediate cast to char* in
> the macro is unnecessary, then perhaps you should file a bug to fix the macro.

I don't want to call GetPtr on integer-types, so i can't call it at the top. And
this way I can reuse the code for integers and atoms.

> Get rid of the empty new line in between the type and the signature.

Hmmm.. strange. This doesn't appear in the attachment or locally when i look at it.

> Looks like you are commenting the wrong method here.  Also, why just 16 bit 
> out of curiosity?

I wanted a compiletime check if someone tries to pass too large values. However
I realized that that'll limit the values we can store too much so i'll change it
back to 32-bit. See comment 7.

> >Index: content/html/content/src/nsHTMLTableCellElement.cpp
> >-#define MAX_COLSPAN 8190
> >-#define MAX_ROWSPAN 8190 // celldata.h can not handle more
> >+#define MAX_COLROWSPAN 8190 // celldata.h can not handle more
> 
> In theory, one could want to decrease one or the other but not both.  Or
> someone could fix celldata.h to handle more, and then we may want to increase
> one of the numbers or something. It probably doesn't matter that much but I'd
> rather make this change in a different bug, really and not here.

We can always switch back if that happens, such a change would be minimal. It
feels so unneccesary to have code to choose between two numbers that are the same.
Is it ok if I keep this in the patch?

> >   struct EnumTable {
> >     /** The string the value maps to */
> >     const char* tag;
> >     /** The enum value that maps to this string */
> >-    PRInt32 value;
> >+    PRInt16 value;
> 
> As you already noted, ack :)

Actually, i want to keep this change. There should be no need for enum-values
bigger then 32767 so a compiletime check is nice to have.

Fixed everything else.

Did you want to look at a new patch before you mark r=caillon, or did you just
forget to mark it?

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Move parsing and storage from nsHTMLValue to nsAttrValue, v2

Addresses comment 7 and comments from caillon

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Move parsing and storage from nsHTMLValue to nsAttrValue, v2.1

This one fixes the else-after-returns too. I'd still recommend reviewing the
previous patch, it should be a lot easier to read.

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 142719 [details] [diff] [review]
Move parsing and storage from nsHTMLValue to nsAttrValue, v2.1

+PRBool
+nsAttrValue::GetColorValue(nscolor& aColor) const

This method always returns PR_TRUE, make it return void, or nscolor?

- In nsAttrValue::ParseSpecialIntValue():

+  nsAutoString tmp(aString);
+  PRInt32 val = tmp.ToInteger(&ec);

nsAttrValue::ParseIntWithBounds() does the same using:

  PRInt32 val = PromiseFlatString(aString).ToInteger(&ec);

which I believe will be faster, assuming the incoming string is flat.

sr=jst

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

GetColorValue can return false when BaseType() returns eString.
I need to get call RFind on the string in ParseSpecialIntValue, which doesn't
exist on nsAString, so i'll keep the current code.

I did add 'inline' qualifiers on the inlined functions in nsAttrValue though
(comment i got over irc).

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch that was checked in

Checked in. Thanks for reviews!!

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 142719 [details] [diff] [review]
Move parsing and storage from nsHTMLValue to nsAttrValue, v2.1

got r=caillon over jst/irc/tcp/ip

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This caused bug 237071

Comment 17

[Boris Zbarsky [:bzbarsky]]

This also caused bug 237090

Comment 18

[Boris Zbarsky [:bzbarsky]]

I just checked over the patch using 

   egrep "^Index:|^[^-].*return NS_CONTENT" patch

and it looks like <ol start=""> and <tr height=""> were the only cases that got
messed up in the various nsHTML*Element classes (for * != Generic).

Comment 19

[Daniel Veditz [:dveditz]]

Apparently caused bug 241924

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The last bits of nsHTMLValue died with the patch in bug 236476. May it rest in
piece.