549475 - Implement HTML5-specified value sanitization algorithm for HTMLInputElement

Status: RESOLVED FIXED

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

Description

[(no longer active)]

HTML5 says that <input type=text> (and <input type=search>, which we're not implementing yet) should implement a "value sanitization algorithm", which basically means that they should be removing \r and \n from their values.  This should happen when the control is being instantiated, and when the type value is changed to text.

This behavior should not happen for password fields, though.

What happens right now is that the value is modified according to the editor.singleLine.pasteNewlines pref, which should really only affect newline handling when pasting.

CCing Jonas because this is about the discussion that we had last week with regard to bug 221820.  CCing Mounir so that we can make sure that the same behavior happens in <input type=search> controls if we implement them.

Comment 1

[(no longer active)]

Oh, and the patch that I'm going to attach on this bug will also correct the behavior of the editor.singleLine.pasteNewlines to only affect pasted text, which is what it should have been doing.

Comment 2

[Alex Vincent [:WeirdAl]]

Isn't type mutable?  That's going to make this... interesting.

Comment 3

[(no longer active)]

(In reply to comment #2)
> Isn't type mutable?  That's going to make this... interesting.

It is.  The spec says [1] that if the type is changed to text or search states, the value should be sanitized, which will be what my patch will accomplish.

[1] <http://www.whatwg.org/specs/web-apps/current-work/multipage/the-input-element.html#value-sanitization-algorithm>

Comment 4

[(no longer active)]

This depends on bug 534785, which would move the ownership of the value to the content node (the current case is that the value is owned by the frame when there is one around, and the ownership is being constantly moved between content and frame when frames are constructed, destroyed.

Comment 6

[Mounir Lamouri (:mounir)]

Ehsan, are you going to add the dirty value flag with this patch or I should open a new bug for that ?

Comment 7

[(no longer active)]

(In reply to comment #6)
> Ehsan, are you going to add the dirty value flag with this patch or I should
> open a new bug for that ?

I think a new bug is fine.  I assume that bug 561640 is going to handle this, right?

Comment 8

[(no longer active)]

Mounir told me on IRC that he's willing to take this.

Comment 9

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

I had to add |StripChars| to nsAString_internal. I hope that was not a bad idea. I thought it's really annoying to need nsString just for |StripChars|.

Some choices may be stupid^W discussed.

Comment 10

[(no longer active)]

Comment on attachment 451741 [details] [diff] [review]
Patch v1

>   nsresult SetValueInternal(const nsAString& aValue,
>-                            PRBool aUserInput);
>+                            PRBool aUserInput,
>+                            PRBool aSetValueChanged);

I don't like this.  Why is this necessary?  Most callers would set aSetValueChanged to true, so why not adjust the changed bit in those callers which don't want to set that flag?

>     if (aName == nsGkAtoms::value &&
>-        !GET_BOOLBIT(mBitField, BF_VALUE_CHANGED) &&
>-        (mType == NS_FORM_INPUT_TEXT ||
>-         mType == NS_FORM_INPUT_SEARCH ||
>-         mType == NS_FORM_INPUT_PASSWORD ||
>-         mType == NS_FORM_INPUT_TEL ||
>-         mType == NS_FORM_INPUT_FILE)) {
>-      Reset();
>+        !GET_BOOLBIT(mBitField, BF_VALUE_CHANGED)) {
>+      SetDefaultValueAsValue();
>     }

This is changing the behavior for inputs other than text, search, password, tel and file.  Why?

>@@ -2372,16 +2357,41 @@ nsHTMLInputElement::HandleTypeChange(PRU
>     FreeData();
>     mInputData.mState = new nsTextEditorState(this);
>     NS_ADDREF(mInputData.mState);
>   } else if (isCurrentTypeSingleLine && !isNewTypeSingleLine) {
>     FreeData();
>   }
> 
>   mType = aNewType;
>+
>+  if (IsSingleLineTextControlInternal(PR_FALSE, mType)) {

You can just use IsSingleLineTextControl.

>+    nsAutoString value;
>+    GetValue(value);
>+    // SetValueInternal is going to sanitize the value.
>+    SetValueInternal(value, PR_FALSE, PR_FALSE);
>+  }
>+}

This is doing unneeded work, for example, when changing from type=text to type=search.  Why?

>+void
>+nsHTMLInputElement::SanitizeValue(nsAString& aValue)
>+{
>+  switch (mType) {
>+    case NS_FORM_INPUT_TEXT:
>+    case NS_FORM_INPUT_SEARCH:
>+    case NS_FORM_INPUT_TEL:
>+    case NS_FORM_INPUT_PASSWORD:

Why didn't you just use IsSingleLineTextControl?

> nsresult
>-nsHTMLInputElement::Reset()
>+nsHTMLInputElement::SetDefaultValueAsValue()

I don't really understand why you broke up Reset into two parts like this.  Could you please explain what you're trying to achieve?

> NS_IMETHODIMP_(void)
> nsHTMLInputElement::GetDefaultValueFromContent(nsAString& aValue)
> {
>   nsTextEditorState *state = GetEditorState();
>   if (state) {
>     GetDefaultValue(aValue);
>+    // This is called by the frame to show the value.
>+    // We have to sanitize it.
>+    SanitizeValue(aValue);

This is sanitizing the value for all input types.  It seems wrong to me.

>-  nsTextEditRules::HandleNewLines(value, -1);

Can you make sure that this is the only usecase for nsTextEditRules in nsTextControlFrame, and remove the related #include's and Makefile.in stuff?  (In a separate patch!)


It would be very good if you could move the refactoring parts into their own patches.  It would make the review very easy.

I haven't looked at the tests yet.

Comment 11

[Mounir Lamouri (:mounir)]

(In reply to comment #10)
> (From update of attachment 451741 [details] [diff] [review])
> >   nsresult SetValueInternal(const nsAString& aValue,
> >-                            PRBool aUserInput);
> >+                            PRBool aUserInput,
> >+                            PRBool aSetValueChanged);
> 
> I don't like this.  Why is this necessary?  Most callers would set
> aSetValueChanged to true, so why not adjust the changed bit in those callers
> which don't want to set that flag?

I don't really like having a call to SetValueInternal then a call to SetValueChanged(PR_FALSE) or even worst, get the ValueChanged() and reset it after the call of SetValueInternal.

> >     if (aName == nsGkAtoms::value &&
> >-        !GET_BOOLBIT(mBitField, BF_VALUE_CHANGED) &&
> >-        (mType == NS_FORM_INPUT_TEXT ||
> >-         mType == NS_FORM_INPUT_SEARCH ||
> >-         mType == NS_FORM_INPUT_PASSWORD ||
> >-         mType == NS_FORM_INPUT_TEL ||
> >-         mType == NS_FORM_INPUT_FILE)) {
> >-      Reset();
> >+        !GET_BOOLBIT(mBitField, BF_VALUE_CHANGED)) {
> >+      SetDefaultValueAsValue();
> >     }
> 
> This is changing the behavior for inputs other than text, search, password, tel
> and file.  Why?

This is the behavior requested by the specifications.

> >@@ -2372,16 +2357,41 @@ nsHTMLInputElement::HandleTypeChange(PRU
> >     FreeData();
> >     mInputData.mState = new nsTextEditorState(this);
> >     NS_ADDREF(mInputData.mState);
> >   } else if (isCurrentTypeSingleLine && !isNewTypeSingleLine) {
> >     FreeData();
> >   }
> > 
> >   mType = aNewType;
> >+
> >+  if (IsSingleLineTextControlInternal(PR_FALSE, mType)) {
> 
> You can just use IsSingleLineTextControl.

I thought you've added IsSingleLineTextControl to prevent calling the virtual GetType(). That wasn't the reason ?

> >+    nsAutoString value;
> >+    GetValue(value);
> >+    // SetValueInternal is going to sanitize the value.
> >+    SetValueInternal(value, PR_FALSE, PR_FALSE);
> >+  }
> >+}
> 
> This is doing unneeded work, for example, when changing from type=text to
> type=search.  Why?

I could check that we are not moving to a type with the same sanitizing algorithm but I'm wondering if that really worth it.

> >+void
> >+nsHTMLInputElement::SanitizeValue(nsAString& aValue)
> >+{
> >+  switch (mType) {
> >+    case NS_FORM_INPUT_TEXT:
> >+    case NS_FORM_INPUT_SEARCH:
> >+    case NS_FORM_INPUT_TEL:
> >+    case NS_FORM_INPUT_PASSWORD:
> 
> Why didn't you just use IsSingleLineTextControl?

Other types have sanitizing algorithm. Using a switch seems to be the best way to go.

> > nsresult
> >-nsHTMLInputElement::Reset()
> >+nsHTMLInputElement::SetDefaultValueAsValue()
> 
> I don't really understand why you broke up Reset into two parts like this. 
> Could you please explain what you're trying to achieve?

Make the code more readable and do not call |Reset| when we don't really care about resetting. We were re-setting the CHANGED bit to false even if that wasn't needed. Hopefully, |Reset| was called only when CHANGED bit was already false. I think having a code more clear can only help.

> > NS_IMETHODIMP_(void)
> > nsHTMLInputElement::GetDefaultValueFromContent(nsAString& aValue)
> > {
> >   nsTextEditorState *state = GetEditorState();
> >   if (state) {
> >     GetDefaultValue(aValue);
> >+    // This is called by the frame to show the value.
> >+    // We have to sanitize it.
> >+    SanitizeValue(aValue);
> 
> This is sanitizing the value for all input types.  It seems wrong to me.

Why does that seem wrong ? If the type doesn't have a sanitizing algorithm, that will do nothing.

> >-  nsTextEditRules::HandleNewLines(value, -1);
> 
> Can you make sure that this is the only usecase for nsTextEditRules in
> nsTextControlFrame, and remove the related #include's and Makefile.in stuff? 
> (In a separate patch!)

Ok.

> It would be very good if you could move the refactoring parts into their own
> patches.  It would make the review very easy.

Are you talking about the |Reset| and SetValueInternal changes ?

> I haven't looked at the tests yet.

Comment 12

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

I haven't read the patch here yet, but I wanted to point out that nsHTMLInputElement is extremely complex. So I really think we should optimize all code for readability and simplicity. Very little of this code is performance critical.

Comment 13

[(no longer active)]

(In reply to comment #11)
> (In reply to comment #10)
> > (From update of attachment 451741 [details] [diff] [review] [details])
> > >   nsresult SetValueInternal(const nsAString& aValue,
> > >-                            PRBool aUserInput);
> > >+                            PRBool aUserInput,
> > >+                            PRBool aSetValueChanged);
> > 
> > I don't like this.  Why is this necessary?  Most callers would set
> > aSetValueChanged to true, so why not adjust the changed bit in those callers
> > which don't want to set that flag?
> 
> I don't really like having a call to SetValueInternal then a call to
> SetValueChanged(PR_FALSE) or even worst, get the ValueChanged() and reset it
> after the call of SetValueInternal.

Fair enough.  But let's do that in a separate patch, as discussed on IRC.

> > >     if (aName == nsGkAtoms::value &&
> > >-        !GET_BOOLBIT(mBitField, BF_VALUE_CHANGED) &&
> > >-        (mType == NS_FORM_INPUT_TEXT ||
> > >-         mType == NS_FORM_INPUT_SEARCH ||
> > >-         mType == NS_FORM_INPUT_PASSWORD ||
> > >-         mType == NS_FORM_INPUT_TEL ||
> > >-         mType == NS_FORM_INPUT_FILE)) {
> > >-      Reset();
> > >+        !GET_BOOLBIT(mBitField, BF_VALUE_CHANGED)) {
> > >+      SetDefaultValueAsValue();
> > >     }
> > 
> > This is changing the behavior for inputs other than text, search, password, tel
> > and file.  Why?
> 
> This is the behavior requested by the specifications.

Oh, OK.  Do the tests in that patch test this?

> > >@@ -2372,16 +2357,41 @@ nsHTMLInputElement::HandleTypeChange(PRU
> > >     FreeData();
> > >     mInputData.mState = new nsTextEditorState(this);
> > >     NS_ADDREF(mInputData.mState);
> > >   } else if (isCurrentTypeSingleLine && !isNewTypeSingleLine) {
> > >     FreeData();
> > >   }
> > > 
> > >   mType = aNewType;
> > >+
> > >+  if (IsSingleLineTextControlInternal(PR_FALSE, mType)) {
> > 
> > You can just use IsSingleLineTextControl.
> 
> I thought you've added IsSingleLineTextControl to prevent calling the virtual
> GetType(). That wasn't the reason ?

Not really, I added it to be able to check types before setting them as the active type.  But now that you've brought it up, why would we need the GetType() call at all?  Is it going to return something different than mType?

> > >+    nsAutoString value;
> > >+    GetValue(value);
> > >+    // SetValueInternal is going to sanitize the value.
> > >+    SetValueInternal(value, PR_FALSE, PR_FALSE);
> > >+  }
> > >+}
> > 
> > This is doing unneeded work, for example, when changing from type=text to
> > type=search.  Why?
> 
> I could check that we are not moving to a type with the same sanitizing
> algorithm but I'm wondering if that really worth it.

Well, we can leave it for readability purposes, but let's at least say that this might not be needed with a comment?

> > >+void
> > >+nsHTMLInputElement::SanitizeValue(nsAString& aValue)
> > >+{
> > >+  switch (mType) {
> > >+    case NS_FORM_INPUT_TEXT:
> > >+    case NS_FORM_INPUT_SEARCH:
> > >+    case NS_FORM_INPUT_TEL:
> > >+    case NS_FORM_INPUT_PASSWORD:
> > 
> > Why didn't you just use IsSingleLineTextControl?
> 
> Other types have sanitizing algorithm. Using a switch seems to be the best way
> to go.

OK.

> > > nsresult
> > >-nsHTMLInputElement::Reset()
> > >+nsHTMLInputElement::SetDefaultValueAsValue()
> > 
> > I don't really understand why you broke up Reset into two parts like this. 
> > Could you please explain what you're trying to achieve?
> 
> Make the code more readable and do not call |Reset| when we don't really care
> about resetting. We were re-setting the CHANGED bit to false even if that
> wasn't needed. Hopefully, |Reset| was called only when CHANGED bit was already
> false. I think having a code more clear can only help.

That's also fair, but it belongs in a separate patch.

> > > NS_IMETHODIMP_(void)
> > > nsHTMLInputElement::GetDefaultValueFromContent(nsAString& aValue)
> > > {
> > >   nsTextEditorState *state = GetEditorState();
> > >   if (state) {
> > >     GetDefaultValue(aValue);
> > >+    // This is called by the frame to show the value.
> > >+    // We have to sanitize it.
> > >+    SanitizeValue(aValue);
> > 
> > This is sanitizing the value for all input types.  It seems wrong to me.
> 
> Why does that seem wrong ? If the type doesn't have a sanitizing algorithm,
> that will do nothing.

Oh, OK.  s/sanitize it/sanitize it if needed/ to avoid the confusion, please.

> > It would be very good if you could move the refactoring parts into their own
> > patches.  It would make the review very easy.
> 
> Are you talking about the |Reset| and SetValueInternal changes ?

Yep!

Comment 14

[Mounir Lamouri (:mounir)]

Attachment: Patch v2

I cleaned the patch as much as possible.
I will open the follow-ups for the refactorisation.

Comment 15

[Mounir Lamouri (:mounir)]

(In reply to comment #13)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > (From update of attachment 451741 [details] [diff] [review] [details] [details])
> > > >   nsresult SetValueInternal(const nsAString& aValue,
> > > >-                            PRBool aUserInput);
> > > >+                            PRBool aUserInput,
> > > >+                            PRBool aSetValueChanged);
> > > 
> > > I don't like this.  Why is this necessary?  Most callers would set
> > > aSetValueChanged to true, so why not adjust the changed bit in those callers
> > > which don't want to set that flag?
> > 
> > I don't really like having a call to SetValueInternal then a call to
> > SetValueChanged(PR_FALSE) or even worst, get the ValueChanged() and reset it
> > after the call of SetValueInternal.
> 
> Fair enough.  But let's do that in a separate patch, as discussed on IRC.

bug 572897

> > > >     if (aName == nsGkAtoms::value &&
> > > >-        !GET_BOOLBIT(mBitField, BF_VALUE_CHANGED) &&
> > > >-        (mType == NS_FORM_INPUT_TEXT ||
> > > >-         mType == NS_FORM_INPUT_SEARCH ||
> > > >-         mType == NS_FORM_INPUT_PASSWORD ||
> > > >-         mType == NS_FORM_INPUT_TEL ||
> > > >-         mType == NS_FORM_INPUT_FILE)) {
> > > >-      Reset();
> > > >+        !GET_BOOLBIT(mBitField, BF_VALUE_CHANGED)) {
> > > >+      SetDefaultValueAsValue();
> > > >     }
> > > 
> > > This is changing the behavior for inputs other than text, search, password, tel
> > > and file.  Why?
> > 
> > This is the behavior requested by the specifications.
> 
> Oh, OK.  Do the tests in that patch test this?

Yes, they should.

> > > >@@ -2372,16 +2357,41 @@ nsHTMLInputElement::HandleTypeChange(PRU
> > > >     FreeData();
> > > >     mInputData.mState = new nsTextEditorState(this);
> > > >     NS_ADDREF(mInputData.mState);
> > > >   } else if (isCurrentTypeSingleLine && !isNewTypeSingleLine) {
> > > >     FreeData();
> > > >   }
> > > > 
> > > >   mType = aNewType;
> > > >+
> > > >+  if (IsSingleLineTextControlInternal(PR_FALSE, mType)) {
> > > 
> > > You can just use IsSingleLineTextControl.
> > 
> > I thought you've added IsSingleLineTextControl to prevent calling the virtual
> > GetType(). That wasn't the reason ?
> 
> Not really, I added it to be able to check types before setting them as the
> active type.  But now that you've brought it up, why would we need the
> GetType() call at all?  Is it going to return something different than mType?

Actually, IsSingleLineTextControl is a method declared in nsIFormControl and defined in nsGenericHTMLFormElement. None of them have mType. nsIFormControl declares GetType which is defined in every for element classes. Only button and input have mType other elements return a constant. We could add mType in nsIFormControl but it will make us waste 8bits per object.

> > > >+    nsAutoString value;
> > > >+    GetValue(value);
> > > >+    // SetValueInternal is going to sanitize the value.
> > > >+    SetValueInternal(value, PR_FALSE, PR_FALSE);
> > > >+  }
> > > >+}
> > > 
> > > This is doing unneeded work, for example, when changing from type=text to
> > > type=search.  Why?
> > 
> > I could check that we are not moving to a type with the same sanitizing
> > algorithm but I'm wondering if that really worth it.
> 
> Well, we can leave it for readability purposes, but let's at least say that
> this might not be needed with a comment?

Ok.

> > > > nsresult
> > > >-nsHTMLInputElement::Reset()
> > > >+nsHTMLInputElement::SetDefaultValueAsValue()
> > > 
> > > I don't really understand why you broke up Reset into two parts like this. 
> > > Could you please explain what you're trying to achieve?
> > 
> > Make the code more readable and do not call |Reset| when we don't really care
> > about resetting. We were re-setting the CHANGED bit to false even if that
> > wasn't needed. Hopefully, |Reset| was called only when CHANGED bit was already
> > false. I think having a code more clear can only help.
> 
> That's also fair, but it belongs in a separate patch.

bug 572896

Comment 16

[Mounir Lamouri (:mounir)]

Attachment: Patch v2.1

This is fixing an editor reftest and adding one.
Except for this test, it went fine on the try server.

Comment 17

[(no longer active)]

Comment on attachment 452206 [details] [diff] [review]
Patch v2.1

>+void
>+nsHTMLInputElement::SanitizeValue(nsAString& aValue)
>+{
>+  switch (mType) {
>+    case NS_FORM_INPUT_TEXT:
>+    case NS_FORM_INPUT_SEARCH:
>+    case NS_FORM_INPUT_TEL:
>+    case NS_FORM_INPUT_PASSWORD:
>+      {
>+        PRUnichar crlf[] = { PRUnichar('\r'), PRUnichar('\n'), 0 };
>+        aValue.StripChars(crlf);
>+      }
>+      break;
>+    default:
>+      break;

Drop the default case?

>-nsresult
>+NS_IMETHODIMP
> nsHTMLInputElement::Reset()

This is probably not necessary.

>+var form = document.createElement("form");
>+var element = document.createElement("input");
>+form.appendChild(element);

I'd very much rather see all the tests here work on an input with and without a frame, and with and without an editor, just to make sure that the presence of the frame and the editor won't affect things.

>+  switch (aType) {
>+    case "text":
>+    case "password":
>+    case "search":
>+    case "telephone":
>+    //case "email":
>+      return aValue.replace(/[\n\r]/g, "");
>+    case "email":
>+      // TODO: uncomment the previous email case when email is implemented.
>+      return "";
>+    case "url":
>+      // TODO: uncomment the next line when url is implemented.
>+      //return aValue.replace(/[\n\r]/g, "").replace(/^\s+|\s+$/g, ""');
>+      return "";

Can you actually put todo calls in these places?  (and elsewhere in this test).

r=me, but you would probably want someone from the content team look at the content changes as well.  And also you need to get review for the xpcom part.

Comment 18

[Mounir Lamouri (:mounir)]

(In reply to comment #17)
> (From update of attachment 452206 [details] [diff] [review])
> >+void
> >+nsHTMLInputElement::SanitizeValue(nsAString& aValue)
> >+{
> >+  switch (mType) {
> >+    case NS_FORM_INPUT_TEXT:
> >+    case NS_FORM_INPUT_SEARCH:
> >+    case NS_FORM_INPUT_TEL:
> >+    case NS_FORM_INPUT_PASSWORD:
> >+      {
> >+        PRUnichar crlf[] = { PRUnichar('\r'), PRUnichar('\n'), 0 };
> >+        aValue.StripChars(crlf);
> >+      }
> >+      break;
> >+    default:
> >+      break;
> 
> Drop the default case?

Not needed indeed. I'm used to add a default at the end of my switch. I can remove it when I will update the patch.

> >-nsresult
> >+NS_IMETHODIMP
> > nsHTMLInputElement::Reset()
> 
> This is probably not necessary.

Probably not but isn't that better to have NS_IMETHODIMP when the method is declared NS_IMETHOD ?

> >+var form = document.createElement("form");
> >+var element = document.createElement("input");
> >+form.appendChild(element);
> 
> I'd very much rather see all the tests here work on an input with and without a
> frame, and with and without an editor, just to make sure that the presence of
> the frame and the editor won't affect things.

Ok for the frame but how could I check with a frame and without an editor ?

> >+  switch (aType) {
> >+    case "text":
> >+    case "password":
> >+    case "search":
> >+    case "telephone":
> >+    //case "email":
> >+      return aValue.replace(/[\n\r]/g, "");
> >+    case "email":
> >+      // TODO: uncomment the previous email case when email is implemented.
> >+      return "";
> >+    case "url":
> >+      // TODO: uncomment the next line when url is implemented.
> >+      //return aValue.replace(/[\n\r]/g, "").replace(/^\s+|\s+$/g, ""');
> >+      return "";
> 
> Can you actually put todo calls in these places?  (and elsewhere in this test).

I've added some todo tests at the end so the test will break when a new input will be added. I suppose that's enough and we don't have to add fake todo's everywhere.

> r=me, but you would probably want someone from the content team look at the
> content changes as well.  And also you need to get review for the xpcom part.

Mike, could you review the XPCOM change and Jonas the content ?

Thank you for your review Ehsan :)

Comment 19

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 452206 [details] [diff] [review]
Patch v2.1

string changes over to bsmedberg.

Comment 20

[(no longer active)]

(In reply to comment #18)
> > Drop the default case?
> 
> Not needed indeed. I'm used to add a default at the end of my switch. I can
> remove it when I will update the patch.

OK, please do! :-)

> > >-nsresult
> > >+NS_IMETHODIMP
> > > nsHTMLInputElement::Reset()
> > 
> > This is probably not necessary.
> 
> Probably not but isn't that better to have NS_IMETHODIMP when the method is
> declared NS_IMETHOD ?

Ah, well, yes.  I didn't look into where Reset is coming from, but if it's not declared in an interface, then I think you can make it not an NS_IMETHOD.

> > >+var form = document.createElement("form");
> > >+var element = document.createElement("input");
> > >+form.appendChild(element);
> > 
> > I'd very much rather see all the tests here work on an input with and without a
> > frame, and with and without an editor, just to make sure that the presence of
> > the frame and the editor won't affect things.
> 
> Ok for the frame but how could I check with a frame and without an editor ?

Here's a life-cycle of a text box:

<input id=x style=display:none> // no frame, no editor
x.style.display = ""; // frame, no editor
x.focus();x.blur(); // frame, editor
x.style.display = "none"; // no frame, editor

> > Can you actually put todo calls in these places?  (and elsewhere in this test).
> 
> I've added some todo tests at the end so the test will break when a new input
> will be added. I suppose that's enough and we don't have to add fake todo's
> everywhere.

OK, I guess so.

Comment 21

[Benjamin Smedberg]

Comment on attachment 452206 [details] [diff] [review]
Patch v2.1

I'd like a unit test for StripChars, please. You can add it in TestStrings.cpp

Comment 22

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

Comment on attachment 452206 [details] [diff] [review]
Patch v2.1

>+nsTSubstring_CharT::StripChars( char_type* aChars, PRInt32 aOffset )

Make the first argument be of type PRUint32?

>+  {
>+    if (mLength == 0 || aOffset >= PRInt32(mLength))
>+      return;

If so, you can remove the |== 0| check here.

r=me, though I too would like to see StripChars tested in unit tests.

Comment 23

[Mounir Lamouri (:mounir)]

(In reply to comment #22)
> Comment on attachment 452206 [details] [diff] [review]
> Patch v2.1
> 
> >+nsTSubstring_CharT::StripChars( char_type* aChars, PRInt32 aOffset )
> 
> Make the first argument be of type PRUint32?
> 
> >+  {
> >+    if (mLength == 0 || aOffset >= PRInt32(mLength))
> >+      return;
> 
> If so, you can remove the |== 0| check here.

I don't really understand. I guess you meant the second argument but the check for == 0 is done against a class member (mLength). mLength's type is |size_type| so we can't even remove the conversion to PRInt32 if we make aOffset a PRUint32.

I agree PRUint32 would be better but I don't get how it will help removing some code. So, maybe we could keep PRint32 for consistency with StripChar ?

> r=me, though I too would like to see StripChars tested in unit tests.

I will do that.

Comment 24

[Mounir Lamouri (:mounir)]

Attachment: Patch v2.2

This should fix all the requests from the reviews.

Comment 25

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

(In reply to comment #23)
> (In reply to comment #22)
> > Comment on attachment 452206 [details] [diff] [review] [details]
> > Patch v2.1
> > 
> > >+nsTSubstring_CharT::StripChars( char_type* aChars, PRInt32 aOffset )
> > 
> > Make the first argument be of type PRUint32?
> > 
> > >+  {
> > >+    if (mLength == 0 || aOffset >= PRInt32(mLength))
> > >+      return;
> > 
> > If so, you can remove the |== 0| check here.
> 
> I don't really understand. I guess you meant the second argument but the check
> for == 0 is done against a class member (mLength). mLength's type is
> |size_type| so we can't even remove the conversion to PRInt32 if we make
> aOffset a PRUint32.

If you make aOffset be of unsigned type, then the aOffset >= mLength test will always be true if mLength is 0, and so the explicit test for mLength == 0 is not needed. So I stand by my statement :)

Comment 26

[Mounir Lamouri (:mounir)]

Attachment: Patch v2.3

That's right Jonas. Sorry for my stupid comment. That's now fixed :)

I've also changed the |strip| string type to const and I've fixed the tests which were not really testing the new method...

Comment 27

[Johnny Stenback (:jst)]

Comment on attachment 458639 [details] [diff] [review]
Patch v2.3

- In content/html/content/src/nsHTMLInputElement.cpp:

+    // TODO: |Reset| should not be used as it's not really meant for that.

Is there a bug on file for this?

- In nsHTMLInputElement::SetValueInternal():

+    nsAutoString value;
+    value.Assign(aValue);

nsAutoString value(aValue) here, instead of an explicit assign?

sr=jst

Comment 28

[Mounir Lamouri (:mounir)]

(In reply to comment #27)
> Comment on attachment 458639 [details] [diff] [review]
> Patch v2.3
> 
> - In content/html/content/src/nsHTMLInputElement.cpp:
> 
> +    // TODO: |Reset| should not be used as it's not really meant for that.
> 
> Is there a bug on file for this?

Yes, it's a follow-up (bug 572896) and a patch is ready.

> - In nsHTMLInputElement::SetValueInternal():
> 
> +    nsAutoString value;
> +    value.Assign(aValue);
> 
> nsAutoString value(aValue) here, instead of an explicit assign?

Ok

Comment 29

[Mounir Lamouri (:mounir)]

Attachment: Patch v2.5

r=sicking,ehsan,bsmedberg sr=jst

Comment 30

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/e0f9c67979d4