572897 - Do not always call SetValueChanged(PR_TRUE) when SetValueInternal is called

Status: RESOLVED FIXED

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

Description

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

It's a follow-up from bug 549475: we have at least two calls to SetValueInternal vhich don't need SetValueChanged(PR_TRUE) to be called so it would be clearer to have a parameter to specify that instead of doing some hack around the call or just hoping everything is going to be allright.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

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

I'm having some trouble to review this since Bug 549475 hasn't landed yet, and I'm not sure how value-changed handling was changed there.

Are there tests for this all?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

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

Clearing the request until I get some answer to the question.

Comment 3

[Mounir Lamouri (:mounir)]

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

Actually, there is no change in value-changed in bug 549475 excepted it tries to sanitize the value when it's changed. This patch is here to prevent calling SetValue(something) then calling SetValueChanged(PR_FALSE).

It happens two times in the code:
 - when we change the type of the element and we need to sanitize the value with the new type sanitizing algorithm ;
 - when we call Reset(), SetValue() is called then SetValueChanged(PR_FALSE). Actually, I realize this change is in bug 572896. I will fix that and re-ask for a review after.

Comment 4

[Mounir Lamouri (:mounir)]

Attachment: Patch v1.1

Actually, the patch was correct: there are two calls with PR_FALSE.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 458637 [details] [diff] [review]
Patch v1.1


> 
>   if (IsSingleLineTextControlInternal(PR_FALSE, mType)) {
>     nsAutoString value;
>-    PRBool valueChanged = ValueChanged();
>     GetValue(value);
>     // SetValueInternal is going to sanitize the value.
>-    SetValueInternal(value, PR_FALSE);
>-    SetValueChanged(valueChanged);
>+    SetValueInternal(value, PR_FALSE, PR_FALSE);
>   }
> }
> 
So is this is
"- when we change the type of the element and we need to sanitize the value
with the new type sanitizing algorithm "

Do we have a testcase for this?

Comment 6

[Mounir Lamouri (:mounir)]

bug 549475 is introducing value sanitizing algorithm and testing it.

Comment 7

[Mounir Lamouri (:mounir)]

Comment on attachment 458637 [details] [diff] [review]
Patch v1.1

This is bug 549475 follow-up. Only refactorization.

Comment 8

[Mounir Lamouri (:mounir)]

Attachment: Patch v1.2

Re-based patch.

r=smaug a=bsmedberg

Comment 9

[Mounir Lamouri (:mounir)]

Could you push the patch from bug 572896 while pushing this one (this one as to be applied after).

Comment 10

[(no longer active)]

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