Closed Bug 551670 Opened 14 years ago Closed 14 years ago

type attribute for button elements should be 'submit' when type is invalid or missing

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: mounir, Assigned: mounir)

References

(Blocks 2 open bugs, )

Details

(Keywords: dev-doc-complete, html5)

Attachments

(1 file, 14 obsolete files)

52.17 KB, patch
Details | Diff | Splinter Review
At the moment, button elements uses 'submit' has a default type value when initializing but if the type attribute is changed dynamically, the type attribute doesn't follow the rules of enumerated attributes [1] which say the default value should be used when the value used is empty or invalid. At the moment, if the value is empty or invalid, the type value is not changed.

[1] http://dev.w3.org/html5/spec/infrastructure.html#enumerated-attribute
So should just set mType if !res in ParseAttribute, right?  Or is this really about the _attribute_ as opposed to the behavior of the button (seems off if so)?

A testcase that has clear expected behavior would be good here.
Attached patch Tests (obsolete) — Splinter Review
Actually, as I am working on this, I quickly wrote this test.
The button behavior and the attribute are not correct.
Attached patch Tests (obsolete) — Splinter Review
Oups, there was a typo in the previous test.
The patch is nearly ready.
Attachment #431863 - Attachment is obsolete: true
Where exactly is the behavior for .type documented for unknown @type values?  The spec seems to say it should reflect the @type, no?

That is, why are the expected results of that test correct?
Attached patch Tests (obsolete) — Splinter Review
This test is testing more stuff
Attachment #431869 - Attachment is obsolete: true
Ah, looks like http://dev.w3.org/html5/spec/infrastructure.html#reflect has a special case for enumerated attrs:

"If a reflecting IDL attribute is a DOMString whose content attribute is an enumerated attribute, and the IDL attribute is limited to only known values, then, on getting, the IDL attribute must return the conforming value associated with the state the attribute is in (in its canonical case), or the empty string if the attribute is in a state that has no associated keyword value; and on setting, if the new value is an ASCII case-insensitive match for one of the keywords given for that attribute, then the content attribute must be set to the conforming value associated with the state that the attribute would be in if set to the given new value, otherwise, if the new value is the empty string, then the content attribute must be removed, otherwise, the setter must raise a SYNTAX_ERR  exception."

I'm going to raise that setter issue; it doesn't make sense.
Attached patch Patch v0.1 (obsolete) — Splinter Review
This patch is fixing the bug.
Maybe we can add a macro and a Parse*Value function ?
Attachment #431874 - Attachment is obsolete: true
The "patch" attachment looks just like the testcase.
Attached patch Patch v0.1 (obsolete) — Splinter Review
Oups, sorry about that.
Attachment #431878 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Ah, ok.  Yes, you want a macro; we have lots of other enumerated attributes around, no?
Attached patch Patch v0.2 (obsolete) — Splinter Review
This patch is creating macros, specific parse functions and refactoring between button and input code.
Boris, could you review it ?
Attachment #431880 - Attachment is obsolete: true
Attachment #432014 - Flags: review?(bzbarsky)
Attachment #432014 - Flags: review?(bzbarsky) → review-
Comment on attachment 432014 [details] [diff] [review]
Patch v0.2

>+nsAttrValue::SetEnumValue(const EnumTable& aTable,

Given that this is a protected method with only two callers, please make aCaseSensitive a required argument.

>+                            PRBool aCaseSensitive /*=PR_FALSE*/)

Spaces after /* and before */ ?

>+nsAttrValue::ParseEnumValueWithMissingDefault(const nsAString& aValue,

I'm not that happy with that method name.  Seems like it might be better to call this ParseEnumWithDefaultValue or something?

>+                                              PRInt16 aDefaultMissing)

That should just be aDefaultValue, right?

>+        nsAutoString tag;
>+        tag.AssignASCII(aTable->tag);
>+        SetEnumValue(*aTable, tag);

That seems like a significant change.  In particular, if I have:

  <input type="foopy">

and I then getAttribute("type"), this code will make it return "text" as far as I can tell.  That seems undesirable.

There are other issues in this method too, but not worth worrying about for now until this fundamental problem is resolved.

>+++ b/content/base/src/nsAttrValue.h	Fri Mar 12 01:20:22 2010 +0100
>+nsGenericHTMLElement::GetEnumAttr(PRInt16 aType, const nsAttrValue::EnumTable* aTable, nsAString& aResult)
>+    if (aType == aTable->value)
>+    {
>+      CopyUTF8toUTF16(aTable->tag, aResult);

Isn't aTable->tag always ASCII?  Please CopyASCII... here.

>+++ b/content/html/content/src/nsGenericHTMLElement.h	Fri Mar 12 01:20:22 2010 +0100

>+   * @return        PR_TRUE if the string has been found, PR_FALSE otherwise.
>+   */
>+  NS_HIDDEN_(nsresult) GetEnumAttr(PRInt16 aType, const nsAttrValue::EnumTable* aTable, nsAString& aResult);

That comment doesn't match the code.  And please wrap to 80 chars?

>+++ b/content/html/content/src/nsHTMLButtonElement.cpp	Fri Mar 12 01:20:22 2010 +0100
> PRBool
> nsHTMLButtonElement::ParseAttribute(PRInt32 aNamespaceID,
...
>+    return NS_OK;

I don't see how that makes sense.

>+++ b/content/html/content/src/nsHTMLInputElement.cpp	Fri Mar 12 01:20:22 2010 +0100
> //NS_IMPL_STRING_ATTR_DEFAULT_VALUE(nsHTMLInputElement, Type, type, "text")

That should be able to go away, right?

> PRBool
> nsHTMLInputElement::ParseAttribute(PRInt32 aNamespaceID,
...
>+      return NS_OK;

Once again, this makes no sense.

<input> tests needed here too, no?
(In reply to comment #12)
> (From update of attachment 432014 [details] [diff] [review])
> >+nsAttrValue::SetEnumValue(const EnumTable& aTable,
> 
> Given that this is a protected method with only two callers, please make
> aCaseSensitive a required argument.
> 
> >+                            PRBool aCaseSensitive /*=PR_FALSE*/)

Ok.

> Spaces after /* and before */ ?

Ok.

> >+nsAttrValue::ParseEnumValueWithMissingDefault(const nsAString& aValue,
> 
> I'm not that happy with that method name.  Seems like it might be better to
> call this ParseEnumWithDefaultValue or something?

Actually, I began with that name but there are three cases in the enum attribute parsing according to the specs:
1. no default value, missing and invalid value should be ignored
2. a missing default value and no invalid default value. The missing default value is used in case of the parsed value is empty or invalid.
3. a missing default value and a invalid default value

I thought it would be better to really underline this function is for case 2. I can create a function for case 3 with aInvalidDefaultValue optional but I think the default value may not be really easy to choose.

> 
> >+                                              PRInt16 aDefaultMissing)
> 
> That should just be aDefaultValue, right?

See previous above.

> >+        nsAutoString tag;
> >+        tag.AssignASCII(aTable->tag);
> >+        SetEnumValue(*aTable, tag);
> 
> That seems like a significant change.  In particular, if I have:
> 
>   <input type="foopy">
> 
> and I then getAttribute("type"), this code will make it return "text" as far as
> I can tell.  That seems undesirable.

It is not. The last test is testing that.

> There are other issues in this method too, but not worth worrying about for now
> until this fundamental problem is resolved.

Shoot :)

> >+++ b/content/base/src/nsAttrValue.h	Fri Mar 12 01:20:22 2010 +0100
> >+nsGenericHTMLElement::GetEnumAttr(PRInt16 aType, const nsAttrValue::EnumTable* aTable, nsAString& aResult)
> >+    if (aType == aTable->value)
> >+    {
> >+      CopyUTF8toUTF16(aTable->tag, aResult);
> 
> Isn't aTable->tag always ASCII?  Please CopyASCII... here.

Indeed. It cames from the older function that I moved.

> >+++ b/content/html/content/src/nsGenericHTMLElement.h	Fri Mar 12 01:20:22 2010 +0100
> 
> >+   * @return        PR_TRUE if the string has been found, PR_FALSE otherwise.
> >+   */
> >+  NS_HIDDEN_(nsresult) GetEnumAttr(PRInt16 aType, const nsAttrValue::EnumTable* aTable, nsAString& aResult);
> 
> That comment doesn't match the code.  And please wrap to 80 chars?

Indeed. By the way, is the 80 wraps a convention in the tree ? I don't had this feeling that why I'm not wrapping to 80 chars.

> >+++ b/content/html/content/src/nsHTMLButtonElement.cpp	Fri Mar 12 01:20:22 2010 +0100
> > PRBool
> > nsHTMLButtonElement::ParseAttribute(PRInt32 aNamespaceID,
> ...
> >+    return NS_OK;
> 
> I don't see how that makes sense.
> 
> >+++ b/content/html/content/src/nsHTMLInputElement.cpp	Fri Mar 12 01:20:22 2010 +0100
> > //NS_IMPL_STRING_ATTR_DEFAULT_VALUE(nsHTMLInputElement, Type, type, "text")
> 
> That should be able to go away, right?
> 
> > PRBool
> > nsHTMLInputElement::ParseAttribute(PRInt32 aNamespaceID,
> ...
> >+      return NS_OK;
> 
> Once again, this makes no sense.

Should be PR_TRUE.

> <input> tests needed here too, no?

I did not change the behavior for the input element, it already has the right behavior. I only refactor the existing code so, as far as I know, the tests for input element should exist and they will have to pass.
(In reply to comment #13)
> > That seems like a significant change.  In particular, if I have:
> > 
> >   <input type="foopy">
> > 
> > and I then getAttribute("type"), this code will make it return "text" as far as
> > I can tell.  That seems undesirable.
> 
> It is not. The last test is testing that.

Is that test still passing once you switch from NS_OK (which is == PR_FALSE) to PR_TRUE in your ParseAttribute implementations?

> I did not change the behavior for the input element, it already has the right
> behavior. I only refactor the existing code so, as far as I know, the tests for
> input element should exist and they will have to pass.

I rather doubt they exist.  That code almost certainly predates our test infrastructure.
Attached patch Patch v0.3 (obsolete) — Splinter Review
This patch should be better I hope.
Attachment #432014 - Attachment is obsolete: true
Attachment #432560 - Flags: review?(bzbarsky)
80 is a general convention, but a few areas use larger limits; assume 80 unless you know otherwise.
Comment on attachment 432560 [details] [diff] [review]
Patch v0.3

Changing the review to Olli because Boris is out until 22/03.
Attachment #432560 - Flags: review?(bzbarsky) → review?(Olli.Pettay)
Comment on attachment 432560 [details] [diff] [review]
Patch v0.3

>+nsAttrValue::ParseEnumValueWithMissingDefault(const nsAString& aValue,
>+                                              const EnumTable* aTable,
>+                                              const EnumTable* aDefaultMissing)
The "Missing" in the method name? Why not just ParseEnumValueWithDefault

>+{
>+  // If aValue is empty or invalid, we use aDefaultMissing.
>+  // aValue is known to be invalid when ParseEnumValue returns PR_FALSE.
>+  if (aValue.IsEmpty() || !ParseEnumValue(aValue, aTable))
>+  {
>+    PRInt32 tableValue;
>+    if (GetEnumTableGlobalValue(aDefaultMissing, tableValue))
>+    {
>+      SetIntValueAndType(tableValue, eEnum, &aValue);
>+    }
>+  }
The coding style should be
if (expr) {
  statement;
}
So '{' is in the same line as 'if'


>+  void ParseEnumValueWithMissingDefault(const nsAString& aValue,
>+                                        const EnumTable* aTable,
>+                                        const EnumTable* aDefaultMissing);
This could return PRBool, though I'm not sure if anyone really cares about the
PR_FALSE case.

>+  /**
>+   * Get the 'global' value of an EnumTable.
>+   * That means the value of the enum table using the index as an offset.
>+   *
>+   * @param aTable   the EnumTable to get the value of.
>+   * @param aResult  the global value of the EnumTable.
>+   * @return  whether the value has been found.
>+   */
>+  PRBool GetEnumTableGlobalValue(const EnumTable* aTable, PRInt32& aResult);
Perhaps GetIndexOfEnumTable(...)?

>+nsGenericHTMLElement::GetEnumAttr(PRInt16 aType, const nsAttrValue::EnumTable* aTable, nsAString& aResult)
>+{
>+  for(; aTable->tag; ++aTable)
>+  {
for (...) {
  ...
}

>+    if (aType == aTable->value)
>+    {
if (...) {

>+++ b/content/html/content/test/test_bug551670.html	Mon Mar 15 15:53:39 2010 +0100
Could you test all the possible types of input and button elements.
Attachment #432560 - Flags: review?(Olli.Pettay) → review-
Doesn't that last patch bloat the table-index list with the various default values?
(In reply to comment #19)
> Doesn't that last patch bloat the table-index list with the various default
> values?

I do not understand how. Can you explain what do you have in mind ?

(In reply to comment #18)
> (From update of attachment 432560 [details] [diff] [review])
> >+nsAttrValue::ParseEnumValueWithMissingDefault(const nsAString& aValue,
> >+                                              const EnumTable* aTable,
> >+                                              const EnumTable* aDefaultMissing)
> The "Missing" in the method name? Why not just ParseEnumValueWithDefault

As I said in comment 13, there are 2 types of default values : missing and invalid. I prefer to add 'Missing' so when the invalid default value will be used, a new function with 'Invalid' could be added.

What would you suggest ?

> >+  void ParseEnumValueWithMissingDefault(const nsAString& aValue,
> >+                                        const EnumTable* aTable,
> >+                                        const EnumTable* aDefaultMissing);
> This could return PRBool, though I'm not sure if anyone really cares about the
> PR_FALSE case.

I will add that.
 
> >+  /**
> >+   * Get the 'global' value of an EnumTable.
> >+   * That means the value of the enum table using the index as an offset.
> >+   *
> >+   * @param aTable   the EnumTable to get the value of.
> >+   * @param aResult  the global value of the EnumTable.
> >+   * @return  whether the value has been found.
> >+   */
> >+  PRBool GetEnumTableGlobalValue(const EnumTable* aTable, PRInt32& aResult);
> Perhaps GetIndexOfEnumTable(...)?

Ok.

> >+++ b/content/html/content/test/test_bug551670.html	Mon Mar 15 15:53:39 2010 +0100
> Could you test all the possible types of input and button elements.

I am not sure that is really needed as the code is mainly checking the default type is used as a fallback. 'otherType' is only used to check the parsing is case insensitive.
If the tests are failing with a type and not with another that would mean the 'otherType' is not recognized...

I will check the coding style issues.
Attached patch Patch v0.4 (obsolete) — Splinter Review
I didn't change the name of 'ParseEnumValueWithMissingDefault' and I didn't add tests you suggested for reasons I've mentioned in comment 20. Let me know if you think my reasons are not right.
Attachment #432560 - Attachment is obsolete: true
Attachment #433826 - Flags: review?(Olli.Pettay)
(In reply to comment #20)
> As I said in comment 13, there are 2 types of default values : missing and
> invalid. I prefer to add 'Missing' so when the invalid default value will be
> used, a new function with 'Invalid' could be added.
Could you explain you'd need too different methods to set default values?
Comment on attachment 433826 [details] [diff] [review]
Patch v0.4

Per IRC this will change.
Attachment #433826 - Flags: review?(Olli.Pettay)
Attached patch Patch v0.5 (obsolete) — Splinter Review
Attachment #433826 - Attachment is obsolete: true
Attachment #433859 - Flags: review?(Olli.Pettay)
Attachment #433859 - Flags: review?(Olli.Pettay) → review+
Comment on attachment 433859 [details] [diff] [review]
Patch v0.5


>   /**
>+   * Parse into an enum value using a default value if is invalid.
>+   *
>+   * @param aValue  the string to find the value for.
>+   * @param aTable  the enumeration to map with.
>+   * @param aDefaultMissing   the default value if aValue is invalid.
>+   */
Nit, could you align the parameter comments:
   * @param aValue          the string to find the value for.
   * @param aTable          the enumeration to map with.
   * @param aDefaultMissing the default value if aValue is invalid.


> nsHTMLButtonElement::ParseAttribute(PRInt32 aNamespaceID,
>                                     nsIAtom* aAttribute,
>                                     const nsAString& aValue,
>                                     nsAttrValue& aResult)
> {
>   if (aAttribute == nsGkAtoms::type && kNameSpaceID_None == aNamespaceID) {
>     // XXX ARG!! This is major evilness. ParseAttribute
>     // shouldn't set members. Override SetAttr instead
>-    PRBool res = aResult.ParseEnumValue(aValue, kButtonTypeTable);
>-    if (res) {
>+    PRBool success;
>+    success = aResult.ParseEnumValueWithDefault(aValue, kButtonTypeTable,
>+        kButtonDefaultType);
kButtonDefaultType should be aligned under aValue.

>@@ -760,16 +777,17 @@ NS_IMPL_STRING_ATTR(nsHTMLInputElement, 
> NS_IMPL_BOOL_ATTR(nsHTMLInputElement, ReadOnly, readonly)
> NS_IMPL_URI_ATTR(nsHTMLInputElement, Src, src)
> NS_IMPL_INT_ATTR_DEFAULT_VALUE(nsHTMLInputElement, TabIndex, tabindex, 0)
> NS_IMPL_STRING_ATTR(nsHTMLInputElement, UseMap, usemap)
> //NS_IMPL_STRING_ATTR(nsHTMLInputElement, Value, value)
> //NS_IMPL_INT_ATTR_DEFAULT_VALUE(nsHTMLInputElement, Size, size, 0)
> //NS_IMPL_STRING_ATTR_DEFAULT_VALUE(nsHTMLInputElement, Type, type, "text")
Could you remove this, since you're implementing type few lines under this.

> NS_IMPL_STRING_ATTR(nsHTMLInputElement, Placeholder, placeholder)
>+NS_IMPL_ENUM_ATTR(nsHTMLInputElement, Type, type, mType, kInputTypeTable)
> nsHTMLInputElement::ParseAttribute(PRInt32 aNamespaceID,
>                                    nsIAtom* aAttribute,
>                                    const nsAString& aValue,
>                                    nsAttrValue& aResult)
> {
>   if (aNamespaceID == kNameSpaceID_None) {
>     if (aAttribute == nsGkAtoms::type) {
>       // XXX ARG!! This is major evilness. ParseAttribute
>       // shouldn't set members. Override SetAttr instead
>       PRInt32 newType;
>       PRBool success;
>-      if ((success = aResult.ParseEnumValue(aValue, kInputTypeTable))) {
>+      success = aResult.ParseEnumValueWithDefault(aValue, kInputTypeTable,
>+          kInputDefaultType);
Align kInputDefaultType properly.

>+https://bugzilla.mozilla.org/show_bug.cgi?id=551670
>+-->
>+<head>
>+  <title>Test for Bug 551670</title>
>+  <script type="application/javascript" src="/MochiKit/packed.js"></script>
>+  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
>+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
>+</head>
>+<body>
>+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=551670">Mozilla Bug 551670</a>
>+<p id="display"></p>
>+<div id="content" style="display: none">
>+  <button type='badtype' id='b1'></button>
>+  <button id='b2'></button>
>+  <input type='badtype' id='i1'>
>+  <input id='i2'>
>+</div>
>+<pre id="test">
>+<script type="application/javascript">
>+
>+/** Test for Bug 551670 **/
>+
>+function checkType(e1, e2, defaultType, otherType)
>+{
>+  // When accessing to the type attribute from the IDL,
>+  // it should reflect the state of the button.
>+
>+  defaultType = defaultType.toLowerCase();
>+  otherType = otherType.toLowerCase();
>+
>+  is(e1.type, defaultType,
>+    "When the initial type attribute value is not valid, the state should be '" +
>+    defaultType + "'");
>+  is(e2.type, defaultType,
>+    "When the type attribute is not set, the state should be '" +
>+    defaultType + "'");
>+
>+  e1.type = otherType;
>+  e1.setAttribute('type', '');
>+  is(e1.type, defaultType,
>+    "When type attribute is set to an empty string, the state should be '" +
>+    defaultType + "'");
>+
>+  e1.type = otherType;
>+  e1.type = '';
>+  is(e1.type, defaultType,
>+    "When type attribute is set to an empty string, the state should be '" +
>+    defaultType + "'");
>+
>+  e1.type = otherType;
>+  e1.setAttribute('type', 'foo');
>+  is(e1.type, defaultType,
>+    "When type attribute is set to an invalid value, the state should be '" +
>+    defaultType + "'");
>+
>+  e1.type = otherType;
>+  e1.type = 'foo';
>+  is(e1.type, defaultType,
>+    "When type attribute is set to an invalid value, the state should be '" +
>+    defaultType + "'");
>+
>+  is(e1.getAttribute('type'), 'foo', "Type attribute content should not be changed");
>+
>+  e1.type = otherType.toUpperCase();
>+  is(e1.type, otherType, "Type attribute should be case insensitive");
>+
>+  e1.removeAttribute('type');
>+  is(e1.type, defaultType,
>+    "When type attribute is set to an empty string, the state should be '" +
>+    defaultType + "'");
>+}
>+
>+checkType(document.getElementById('b1'), document.getElementById('b2'), 'submit', 'reset');
>+checkType(document.getElementById('i1'), document.getElementById('i2'), 'text', 'password');

Could you still test all the input element types. Just to add more tests to prevent
future regressions.
Attached patch Patch v0.5.1 (obsolete) — Splinter Review
Attachment #433859 - Attachment is obsolete: true
Olli, I let you re-read the patches and commit them as said on IRC.
Keywords: checkin-needed
> I do not understand how. Can you explain what do you have in mind ?

Every call to nsAttrValue::GetEnumTableIndex does a linear search for the given const EnumTable* in sEnumTableArray.  If not found, it adds the given const EnumTable* to the array.

Your new code calls GetEnumTableIndex with your aDefault values as the const EnumTable*.  Which means they will now get added to the array if used, which will slow down all GetEnumTableIndex callers.

Then again, maybe this code was already completely broken and just added all sorts of random values into this array?  Based on how this is set up, I'd expect each enum table to end up in the array once (so should be doing the IndexOf on the original aTable value passed to ParseEnumValue, not on the modified aTable that we're iterating through), but it looks like we put each enum _value_ in the array once or something....

Maybe this should all be a followup bug, but if that was the intent then passing around const EnumTable* pointing into the middle of tables is a step in the wrong direction.

ccing sicking in case he remembers how this stuff is intended to work.
Oh, and the documentation in that patch for GetEnumTableIndex is confusing at best, but that might be due to the issues mentioned in comment 28.
A few more comments:

1)  The code that changes mType in nsHTMLButtonElement::UnsetAttr is almost
    certainly wrong in that it won't trigger the right style changes.  Changing
    mType changes states, and you need to notify on that change.
2)  While the out-of-band mType is possibly useful for the element impl, it
    doesn't seem to be needed for GetEnumAttr, since the nsAttrValue in question
    should have the right enum value already.  I guess an exception is when the
    attribute is not set...  in which case you need to tell GetEnumAttr what
    the default value should be.  Not sure whether the loop you have in
    GetEnumAttr is better or worse than the relevant GetAttr call (probably no
    worse), and whether other consumers that need to use this code too store
    the values out-of-band as well (almost certainly not).
3)  We could remove the need for more EnumTable*s floating around if we made the
    default values just come first in the tables, no?  I guess that could be
    more complicated if at some point we have multiple different defaults.

Sorry for the lag here; I was sort of off all last week....

Feel free to turn any of the issues above into followup bugs as desired.
(In reply to comment #28)
> > I do not understand how. Can you explain what do you have in mind ?
> 
> Every call to nsAttrValue::GetEnumTableIndex does a linear search for the given
> const EnumTable* in sEnumTableArray.  If not found, it adds the given const
> EnumTable* to the array.
> 
> Your new code calls GetEnumTableIndex with your aDefault values as the const
> EnumTable*.  Which means they will now get added to the array if used, which
> will slow down all GetEnumTableIndex callers.

But aDefault is a valid value. That means it is a value from the k{Input,Button}TypeTable. So, it will not be added randomly.

> Then again, maybe this code was already completely broken and just added all
> sorts of random values into this array?  Based on how this is set up, I'd
> expect each enum table to end up in the array once (so should be doing the
> IndexOf on the original aTable value passed to ParseEnumValue, not on the
> modified aTable that we're iterating through), but it looks like we put each
> enum _value_ in the array once or something....

As far as I understand it, sEnumTableArray is saving the list of EnumTable elements we have found. Elements of sEnumTableArray are never considered to be a table but only an EnumTable element.
By the way, I did not changed ParseEnumValue() to call GetEnumTableIndex() during the iteration of aTable, I've only moved some code to create a new function. It was already behaving like that.

> Maybe this should all be a followup bug, but if that was the intent then
> passing around const EnumTable* pointing into the middle of tables is a step in
> the wrong direction.
> 
> ccing sicking in case he remembers how this stuff is intended to work.
Keywords: checkin-needed
> Elements of sEnumTableArray are never considered to be a table but only an
> EnumTable element.

Then why are we storing both the value from that element _and_ its index in the enum table array in the nsAttrValue int?  Seems like just the latter would be enough to recover any information we might want.

> By the way, I did not changed ParseEnumValue() to call GetEnumTableIndex()
> during the iteration of aTable,

Yes, I realize that.  I'm just saying that your new code looked wrong to me, then I realized that the old code looks wrong in a very similar way.  That doesn't necessarily mean your code (or the old code) is right, but does mean that your patch is at least not making things immediately worse and that following up on this issue in a separate bug is ok by me....
(In reply to comment #30)
> A few more comments:
> 
> 1)  The code that changes mType in nsHTMLButtonElement::UnsetAttr is almost
>     certainly wrong in that it won't trigger the right style changes.  Changing
>     mType changes states, and you need to notify on that change.
I could be very wrong here, but I don't actually know any cases
where changing <button>'s type need to notify *state* changes.
(The attribute removal itself notifies the usual UPDATE_CONTENT_MODEL when needed)
Ah, ok, perhaps :default handling needs the state change.
> I don't actually know any cases where changing <button>'s type need to notify
> *state* changes.

Going from submit to one of the other types (or the reverse) can change the NS_EVENT_STATE_DEFAULT state of the button.  Note that we have code in BeforeSetAttr and AfterSetAttr to send the right notifications, last I checked, but there are also assertions in nsGenericHTMLFormElement::IntrinsicState that assume that the type matches what IsDefaultSubmitElement is doing, etc.  

The whole thing is somewhat tangled up; the safe thing to do is whatever nsHTMLInputElement is doing.
Though, seems like there are many cases when :default isn't updated properly
when there are dynamic changes.
(In reply to comment #36)
> Though, seems like there are many cases when :default isn't updated properly
> when there are dynamic changes.
Or not, Input element seems to notify lots of extra changes.
> Though, seems like there are many cases when :default isn't updated properly
> when there are dynamic changes.

We have some pretty extensive tests for :default with <input>, iirc.

> Or not, Input element seems to notify lots of extra changes.

Yep.
Ah, nsGenericHTMLFormElement::AfterSetAttr does actually notify
about NS_EVENT_STATE_DEFAULT, even with <button>
Mounir, could you add some test for :default handling.
I think changing button's type should update :default correctly.
Argh, my mistake
nsHTMLButtonElement::UnsetAttr certainly shouldn't call
nsGenericHTMLElement::UnsetAttr, but 
nsGenericHTMLFormElement::UnsetAttr.

And probably better to not use UnsetAttr, but the existing nsHTMLButtonElement::BeforeSetAttr.
Attached patch Patch v0.6 (obsolete) — Splinter Review
According to your comments, this patch should fix the issues #1.

About the issue #2 and #3, they are not linked to my patch (I used existing cod) so I think a follow up would be fine.
However, I think the issue #2 is not really an issue because we do not need a default value. mType should never be set to something not in the kButtonTypeTable so the exception should never be throw. In addition, my tests make sure every 'not set' situations are covered so we should never got into the exception.

I will write more tests to cover the points you have pointed later today.
Attachment #434018 - Attachment is obsolete: true
With that last patch, mType is updated in BeforeSetAttr, right?  I guess that works because nsGenericHTMLFormElement::BeforeSetAttr removes the node as a default submit as needed...  But it really would be safer to make this work the way nsHTMLInputElement does (that is, change mType in AfterSetAttr).  At least that way the behavior will be consistent if there are bugs either way.
Blame me for using BeforeSetAttr. I proposed that since based on how the code works atm it should be ok.
But sure using AfterSetAttr would good.
Attached patch Patch v0.7 (obsolete) — Splinter Review
I hope this patch is gonna make it :)
Attachment #434204 - Attachment is obsolete: true
Attachment #434890 - Flags: review?(bzbarsky)
Comment on attachment 434890 [details] [diff] [review]
Patch v0.7

Olli, as you've already reviewed an older version of this patch, I will no problem if you cancel this review.
Attachment #434890 - Flags: superreview?(bzbarsky)
Attachment #434890 - Flags: review?(bzbarsky)
Attachment #434890 - Flags: review?(Olli.Pettay)
I don't understand why we couldn't do for button elements what we do for input elements today. We're dealing with falling back to a default there.

That said, it does indeed look like the enum code is borked. It was intended that sEnumTableArray would contain a pointer to the first item in the enum table, but that isn't what is happening here clearly. I'm surprised this is working without running out of indexes into the table.

I'd definitely like to review the final patch here no matter what.
> I don't understand why we couldn't do for button elements what we do for input
> elements today

Jonas, what were doing for input today is having a custom GetType method.  We _could_ do that for buttons too, but since we actually need such custom methods for a all enumerated attributes it seems worthwhile to make it very easy to create them using our getter/setter macros, which is what this patch aims for.

If you plan to review the final patch, then I probably don't need to, right?  So you can just take over the sr request?
Two comments on that last patch, based on quick skim.

1)  The indentation on the nsHTMLButtonElement::AfterSetAttr declaration is
    broken.
2)  ParseAttribute should reset mType to the right thing even if
    ParseEnumValueWithDefault fails.
Comment on attachment 434890 [details] [diff] [review]
Patch v0.7

Yup, I'm happy to take the sr here.

Makes sense regarding the custom GetType method. Not sure yet which approach I like better, will have to review the patch to determine that.
Attachment #434890 - Flags: superreview?(bzbarsky) → superreview?(jonas)
Attached patch Patch v0.8 (obsolete) — Splinter Review
This patch should fix every issues mentioned.
It is no longer using the default value when parsing to prevent the issues mentioned in bug 551846 and bug 555711. The NS_IMPL_ENUM_ATTR_* macro is now specific to this case (with a default missing value). Others have not been implemented because they are not needed at the moment.
Attachment #434890 - Attachment is obsolete: true
Attachment #436622 - Flags: superreview?(jonas)
Attachment #436622 - Flags: review?(Olli.Pettay)
Attachment #434890 - Flags: superreview?(jonas)
Attachment #434890 - Flags: review?(Olli.Pettay)
Attachment #436622 - Flags: review?(Olli.Pettay) → review+
Comment on attachment 436622 [details] [diff] [review]
Patch v0.8


>+void
>+nsAttrValue::GetEnumString(nsAString& aResult) const
>+{
>+  NS_PRECONDITION(Type() == eEnum, "wrong type");
>+
>+  PRInt16 val = GetEnumValue();
>+  PRUint32 allEnumBits =
>+    (BaseType() == eIntegerBase) ? static_cast<PRUint32>(GetIntInternal())
>+                                   : GetMiscContainer()->mEnumValue;
>+  const EnumTable* table = sEnumTableArray->
>+    ElementAt(allEnumBits & NS_ATTRVALUE_ENUMTABLEINDEX_MASK);
>+  while (table->tag) {
>+    if (table->value == val) {
>+      aResult.AssignASCII(table->tag);
>+      return;
>+    }
>+    table++;
>+  }
>+
>+  NS_NOTREACHED("couldn't find value in EnumTable");
>+}
>+
Why you need this? You could just use nsAttrValue::ToString
This code is anyway copied from ToString.


>+nsGenericHTMLElement::GetEnumAttrWithDefaultMissing(nsIAtom* aAttr,
>+                                                    const nsAString& aDefaultMissing,
>+                                                    nsAString& aResult)
Strange method name "*WithDefaultMissing".
Perhaps GetEnumAttrWithDefaultValue, or just
GetEnumAttr(nsIAtom* aAttr, const nsAString& aDefaultValue, nsAString& aResult)


>+#define NS_IMPL_ENUM_ATTR_DEFAULT_MISSING_VALUE(_class, _method, _atom, _default) \
NS_IMPL_ENUM_ATTR_WITH_DEFAULT_VALUE

With those, r=me
And please file a followup bug to sort out how we handle enum attrs.
(In reply to comment #52)
> (From update of attachment 436622 [details] [diff] [review])
> 
> >+void
> >+nsAttrValue::GetEnumString(nsAString& aResult) const
> >+{
> >+  NS_PRECONDITION(Type() == eEnum, "wrong type");
> >+
> >+  PRInt16 val = GetEnumValue();
> >+  PRUint32 allEnumBits =
> >+    (BaseType() == eIntegerBase) ? static_cast<PRUint32>(GetIntInternal())
> >+                                   : GetMiscContainer()->mEnumValue;
> >+  const EnumTable* table = sEnumTableArray->
> >+    ElementAt(allEnumBits & NS_ATTRVALUE_ENUMTABLEINDEX_MASK);
> >+  while (table->tag) {
> >+    if (table->value == val) {
> >+      aResult.AssignASCII(table->tag);
> >+      return;
> >+    }
> >+    table++;
> >+  }
> >+
> >+  NS_NOTREACHED("couldn't find value in EnumTable");
> >+}
> >+
> Why you need this? You could just use nsAttrValue::ToString
> This code is anyway copied from ToString.

Because ToString does not call this code if the case wasn't the same. So, if I set type="SuBmIt", ToString() will return "SuBmIt" but I want to have "submit".

> >+nsGenericHTMLElement::GetEnumAttrWithDefaultMissing(nsIAtom* aAttr,
> >+                                                    const nsAString& aDefaultMissing,
> >+                                                    nsAString& aResult)
> Strange method name "*WithDefaultMissing".
> Perhaps GetEnumAttrWithDefaultValue, or just
> GetEnumAttr(nsIAtom* aAttr, const nsAString& aDefaultValue, nsAString& aResult)
> 
> 
> >+#define NS_IMPL_ENUM_ATTR_DEFAULT_MISSING_VALUE(_class, _method, _atom, _default) \
> NS_IMPL_ENUM_ATTR_WITH_DEFAULT_VALUE

Because at the moment, I do:
+  if (attrVal && attrVal->Type() == nsAttrValue::eEnum) {
+    attrVal->GetEnumString(aResult);
+  } else {
+    aResult.Append(aDefaultMissing);
+  }

But according to the specs, two other situations are possible:
+  if (!attrVal) {
+    aResult.Append(aDefaultMissing);
+  } else if (attrVal->Type() != nsAttrValue::eEnum) {
+    aResult.Append(aDefaultInvalid);
+  } else {
+    attrVal->GetEnumString(aResult);
+  }

and
+  if (!attrVal) {
+    aResult.Truncate();
+  } else if (attrVal->Type() != nsAttrValue::eEnum) {
+    aResult.Append(aDefaultInvalid);
+  } else {
+    attrVal->GetEnumString(aResult);
+  }

and
+  if (attrVal && attrVal->Type() == nsAttrValue::eEnum) {
+    attrVal->GetEnumString(aResult);
+  } else {
+    aResult.Truncate();
+  }

If you think my naming is not good, I can use a WithDefaults macro which will need two default values and if only one default value is needed, the other one will have to be "". Do you think it is better ?

> 
> With those, r=me
Blocks: 557179
(In reply to comment #54)
> Because ToString does not call this code if the case wasn't the same. So, if I
> set type="SuBmIt", ToString() will return "SuBmIt" but I want to have "submit".
Ok, but could you still make ToString code to use the method you added.
To do that you need to add support for NS_ATTRVALUE_ENUMTABLE_VALUE_NEEDS_TO_UPPER handling to your method.
We don't want almost the same code to be duplicated in several places.

> Because at the moment, I do:
> +  if (attrVal && attrVal->Type() == nsAttrValue::eEnum) {
> +    attrVal->GetEnumString(aResult);
> +  } else {
> +    aResult.Append(aDefaultMissing);
> +  }
Uh, this should be Assign, not Append.


When a method name ends with WithDefaultMissing
I read is somehow so that default value is actually missing, but that is not
quite the case here, since the default value is passed as a parameter.
So I prefer not using *DefaultMissing

In which cases aDefaultMissing value and aDefaultInvalid value are actually different per the HTML5 draft? Is the draft doing anything sane in those cases or should it be changed?
In any case, needing 2 default values in the method isn't a problem.
And there could be two different macros, one which take 1 and one which takes
2 default values.
Attached patch Patch v8.1 (obsolete) — Splinter Review
r=smaug

I had to add a boolean attribute for GetEnumString() because ToString needs ToUpper() but not the original need of GetEnumString().
Attachment #436622 - Attachment is obsolete: true
Attachment #437019 - Flags: superreview?
Attachment #436622 - Flags: superreview?(jonas)
Attachment #437019 - Flags: superreview? → superreview?(jonas)
Blocks: 557620
Blocks: 456229
Blocks: 557628
This bug is going to block other bugs which have r+.
Jonas, do you think you can sr it in the next few days ?
Comment on attachment 437019 [details] [diff] [review]
Patch v8.1

>+nsAttrValue::GetEnumString(nsAString& aResult, PRBool aRealTag) const
>+{
>+  NS_PRECONDITION(Type() == eEnum, "wrong type");
>+
>+  PRInt16 val = GetEnumValue();
>+  PRUint32 allEnumBits =
>+    (BaseType() == eIntegerBase) ? static_cast<PRUint32>(GetIntInternal())
>+                                   : GetMiscContainer()->mEnumValue;

You could save a bit of code by doing:

PRInt16 val = allEnumBits >> NS_ATTRVALUE_ENUMTABLEINDEX_BITS

(I know you copied this from the old code)

>+  const EnumTable* table = sEnumTableArray->
>+    ElementAt(allEnumBits & NS_ATTRVALUE_ENUMTABLEINDEX_MASK);

Hmm.. This looks wierd. This argument passed to ElementAt will always be a multiple of 16384 or 16 or something like that. I think.

This bit twiddling is too complex and too spread out. I take full responsibility for this fact.

Would you mind centralizing and cleaning it up? Something like the following defines:

#define NS_ATTRVALUE_ENUM_TABLEINDEX_BITS (32 - 16 - 1 - NS_ATTRVALUE_INTEGERTYPE_BITS)
#define NS_ATTRVALUE_ENUM_TABLEINDEX_OFFSET NS_ATTRVALUE_INTEGERTYPE_BITS
#define NS_ATTRVALUE_ENUM_TABLEINDEX_MASK \
 (PR_BITMASK(NS_ATTRVALUE_ENUM_TABLEINDEX_BITS) << NS_ATTRVALUE_ENUM_TABLEINDEX_OFFSET)
#define NS_ATTRVALUE_ENUM_GET_TABLEINDEX(_bits) \
 (((_bits) & NS_ATTRVALUE_ENUM_TABLEINDEX_MASK) >> NS_ATTRVALUE_ENUM_TABLEINDEX_OFFSET)

#define NS_ATTRVALUE_ENUM_VALUE_NEEDS_TO_UPPER_BITS 1
#define NS_ATTRVALUE_ENUM_VALUE_NEEDS_TO_UPPER_OFFSET \
 (NS_ATTRVALUE_ENUM_TABLEINDEX_OFFSET + NS_ATTRVALUE_ENUM_TABLEINDEX_BITS)
#define NS_ATTRVALUE_ENUM_VALUE_NEEDS_TO_UPPER_MASK \
 (PR_BITMASK(NS_ATTRVALUE_ENUM_VALUE_NEEDS_TO_UPPER_BITS) << NS_ATTRVALUE_ENUM_VALUE_NEEDS_TO_UPPER_OFFSET)
#define NS_ATTRVALUE_ENUM_VALUE_NEEDS_TO_UPPER(_bits) \
 ((_bits) & NS_ATTRVALUE_ENUM_VALUE_NEEDS_TO_UPPER_MASK)

#define NS_ATTRVALUE_ENUM_VALUE_BITS 16
#define NS_ATTRVALUE_ENUM_VALUE_OFFSET \
 (NS_ATTRVALUE_ENUM_TABLEINDEX_OFFSET + NS_ATTRVALUE_ENUM_TABLEINDEX_BITS)
#define NS_ATTRVALUE_ENUM_VALUE_MASK \
 (PR_BITMASK(NS_ATTRVALUE_ENUM_VALUE_BITS) << NS_ATTRVALUE_ENUM_VALUE_OFFSET)
#define NS_ATTRVALUE_ENUM_GET_VALUE(_bits) \
 (((_bits) & NS_ATTRVALUE_ENUM_VALUE_MASK) >> NS_ATTRVALUE_ENUM_VALUE_OFFSET)

And then use these defines throughout. Feel free to simplify these macros. Might actually be more readable to calculate the values by hand and #define to those values directly.

> nsAttrValue::ParseEnumValue(const nsAString& aValue,
>                             const EnumTable* aTable,
>-                            PRBool aCaseSensitive)
>+                            PRBool aCaseSensitive /* = PR_FALSE */)
> {
>   ResetIfSet();
>+  const EnumTable* firstTableEntry = aTable;

Nit: I think it would be cleaner to call this 'tableEntry' and use this variable to iterate over the table. And leave aTable untouched. In general I don't like modifying passed in arguments as it can easily lead to bugs just like the one you're fixing :)

Feel free to go either way though.

>diff -r c0bac5309fe2 content/html/content/src/nsGenericHTMLElement.cpp
>+nsGenericHTMLElement::GetEnumAttrWithDefault(nsIAtom* aAttr,
>+                                             const nsAString& aDefault,
>+                                             nsAString& aResult)

I think you can drop the 'WithDefault" as there is no enum getter without it. Also, making the default be a char* instead of an nsAString will make things nicer on the calling side. As it is now we always do the string conversion, weather needed or not.

>diff -r c0bac5309fe2 content/html/content/src/nsHTMLButtonElement.cpp

>+static const nsAttrValue::EnumTable kButtonTypeTable[] = {
>+  { "button", NS_FORM_BUTTON_BUTTON },
>+  { "reset", NS_FORM_BUTTON_RESET },
>+  { "submit", NS_FORM_BUTTON_SUBMIT },
>+  { 0 }
>+};
>+
>+// Default type is 'submit'.
>+static const nsAttrValue::EnumTable* kButtonDefaultType = &kButtonTypeTable[2];

I'm not really convinced that kButtonDefaultType is needed. It's very unlikely to ever change. I'm fine either way though. Same in nsHTMLInputElement.

>+nsHTMLButtonElement::AfterSetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,
>+                                  const nsAString* aValue, PRBool aNotify)
>+{
>+  if (aNameSpaceID == kNameSpaceID_None) {
>+    if (aName == nsGkAtoms::type && !aValue) {
>+      mType = kButtonDefaultType->value;
>+    }
>+  }
>+
>+  return nsGenericHTMLFormElement::AfterSetAttr(aNameSpaceID, aName,
>+                                                aValue, aNotify);
>+}

Please collapse the two if-statements into one.

Nit: If you put the !aValue test first then that will likely short-cut things most of the time as setAttribute is much more common than removeAttribute.

>diff -r c0bac5309fe2 content/html/content/src/nsHTMLInputElement.cpp

> nsHTMLInputElement::ParseAttribute(PRInt32 aNamespaceID,
>                                    nsIAtom* aAttribute,
>                                    const nsAString& aValue,
>                                    nsAttrValue& aResult)
> {
>   if (aNamespaceID == kNameSpaceID_None) {
>     if (aAttribute == nsGkAtoms::type) {
>       // XXX ARG!! This is major evilness. ParseAttribute
>       // shouldn't set members. Override SetAttr instead
>       PRInt32 newType;
>       PRBool success;
>-      if ((success = aResult.ParseEnumValue(aValue, kInputTypeTable))) {
>+      success = aResult.ParseEnumValue(aValue, kInputTypeTable, PR_FALSE);

Please combine the line declaring 'success' with the one setting its initial value.

P.S. if you are removing all places where we're not specifying the optional argument in the call to ParseEnumValue then it would rock if you make the argument required. I'm not a big fan of optional arguments as a rule of thumb :)

Thanks for a great patch!
Attachment #437019 - Flags: superreview?(jonas) → superreview+
(In reply to comment #58)
> >+  const EnumTable* table = sEnumTableArray->
> >+    ElementAt(allEnumBits & NS_ATTRVALUE_ENUMTABLEINDEX_MASK);
> 
> Hmm.. This looks wierd. This argument passed to ElementAt will always be a
> multiple of 16384 or 16 or something like that. I think.
> 
> This bit twiddling is too complex and too spread out. I take full
> responsibility for this fact.
> 
> Would you mind centralizing and cleaning it up? Something like the following
> defines:
> 
> #define NS_ATTRVALUE_ENUM_TABLEINDEX_BITS (32 - 16 - 1 -
> NS_ATTRVALUE_INTEGERTYPE_BITS)
> #define NS_ATTRVALUE_ENUM_TABLEINDEX_OFFSET NS_ATTRVALUE_INTEGERTYPE_BITS
> #define NS_ATTRVALUE_ENUM_TABLEINDEX_MASK \
>  (PR_BITMASK(NS_ATTRVALUE_ENUM_TABLEINDEX_BITS) <<
> NS_ATTRVALUE_ENUM_TABLEINDEX_OFFSET)
> #define NS_ATTRVALUE_ENUM_GET_TABLEINDEX(_bits) \
>  (((_bits) & NS_ATTRVALUE_ENUM_TABLEINDEX_MASK) >>
> NS_ATTRVALUE_ENUM_TABLEINDEX_OFFSET)
> 
> #define NS_ATTRVALUE_ENUM_VALUE_NEEDS_TO_UPPER_BITS 1
> #define NS_ATTRVALUE_ENUM_VALUE_NEEDS_TO_UPPER_OFFSET \
>  (NS_ATTRVALUE_ENUM_TABLEINDEX_OFFSET + NS_ATTRVALUE_ENUM_TABLEINDEX_BITS)
> #define NS_ATTRVALUE_ENUM_VALUE_NEEDS_TO_UPPER_MASK \
>  (PR_BITMASK(NS_ATTRVALUE_ENUM_VALUE_NEEDS_TO_UPPER_BITS) <<
> NS_ATTRVALUE_ENUM_VALUE_NEEDS_TO_UPPER_OFFSET)
> #define NS_ATTRVALUE_ENUM_VALUE_NEEDS_TO_UPPER(_bits) \
>  ((_bits) & NS_ATTRVALUE_ENUM_VALUE_NEEDS_TO_UPPER_MASK)
> 
> #define NS_ATTRVALUE_ENUM_VALUE_BITS 16
> #define NS_ATTRVALUE_ENUM_VALUE_OFFSET \
>  (NS_ATTRVALUE_ENUM_TABLEINDEX_OFFSET + NS_ATTRVALUE_ENUM_TABLEINDEX_BITS)
> #define NS_ATTRVALUE_ENUM_VALUE_MASK \
>  (PR_BITMASK(NS_ATTRVALUE_ENUM_VALUE_BITS) << NS_ATTRVALUE_ENUM_VALUE_OFFSET)
> #define NS_ATTRVALUE_ENUM_GET_VALUE(_bits) \
>  (((_bits) & NS_ATTRVALUE_ENUM_VALUE_MASK) >> NS_ATTRVALUE_ENUM_VALUE_OFFSET)
> 
> And then use these defines throughout. Feel free to simplify these macros.
> Might actually be more readable to calculate the values by hand and #define to
> those values directly.

I would prefer to do that in a follow-up because I think that's quite sensitive so it would be better to have a separate review for this.

> >diff -r c0bac5309fe2 content/html/content/src/nsGenericHTMLElement.cpp
> >+nsGenericHTMLElement::GetEnumAttrWithDefault(nsIAtom* aAttr,
> >+                                             const nsAString& aDefault,
> >+                                             nsAString& aResult)
> 
> I think you can drop the 'WithDefault" as there is no enum getter without it.
> Also, making the default be a char* instead of an nsAString will make things
> nicer on the calling side. As it is now we always do the string conversion,
> weather needed or not.

In general, I prefer to do all the conversions before calling a function. Now, if we have a nsAString, char* will have to be passed to the function then it will be changed to nsAString again. Anyway, I did the fix because we only use char* as default arguments and I don't think it's going to change in a near future.

> >diff -r c0bac5309fe2 content/html/content/src/nsHTMLButtonElement.cpp
> 
> >+static const nsAttrValue::EnumTable kButtonTypeTable[] = {
> >+  { "button", NS_FORM_BUTTON_BUTTON },
> >+  { "reset", NS_FORM_BUTTON_RESET },
> >+  { "submit", NS_FORM_BUTTON_SUBMIT },
> >+  { 0 }
> >+};
> >+
> >+// Default type is 'submit'.
> >+static const nsAttrValue::EnumTable* kButtonDefaultType = &kButtonTypeTable[2];
> 
> I'm not really convinced that kButtonDefaultType is needed. It's very unlikely
> to ever change. I'm fine either way though. Same in nsHTMLInputElement.

Using NS_FORM_INPUT_TEXT/NS_FORM_BUTTON_SUBMIT was fine but we need the tag string for the default value now so I prefer to use this static var.

> P.S. if you are removing all places where we're not specifying the optional
> argument in the call to ParseEnumValue then it would rock if you make the
> argument required. I'm not a big fan of optional arguments as a rule of thumb
> :)

The new patch will have everything else (even the PS).
Attached patch Patcd v9Splinter Review
r=smaug, sr=jonas
Attachment #437019 - Attachment is obsolete: true
sorry,
r=smaug, sr=sicking
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/15cece8f6369
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Keywords: checkin-needed
Flags: in-testsuite+
Blocks: 562421
You need to log in before you can comment on or make changes to this bug.