572896 - Split nsHTMLInputElement::Reset() to prevent calling it to set the default value to the current value

Status: RESOLVED FIXED

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

Description

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

It's a follow-up from bug 549475: it makes thing clearer when reading the code.

Comment 1

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

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

How does this make the code easier to read. Yet another switch-case.

Comment 2

[Mounir Lamouri (:mounir)]

(In reply to comment #1)
> (From update of attachment 452121 [details] [diff] [review])
> How does this make the code easier to read. Yet another switch-case.

Well, it prevents calling Reset when we don't really want to reset but set the value to the default value. In addition, the current call to reset is working only because we are calling it when the VALUE_CHANGED bit is false. For any reason, if that's done when VALUE_CHANGE is true, it would be wrong.

I admit a switch case isn't really readable but I think that worths it because when doing a first read of AfterAttrSet, you understand what's SetValueAsDefaultValue is doing and you don't have to ask yourself why Reset is called. In addition, with the ValueMode() function I've added in on of my patch, the size of the switch could be reduced to 4 lines (two cases). I could introduce this function (ValueMode()) in a separate patch if you think the patch in it's current shape is only making thing worse.

Comment 3

[Mounir Lamouri (:mounir)]

Attachment: Patch v1.1

Refreshed patch.

Comment 4

[Mounir Lamouri (:mounir)]

This is bug 549475 follow-up. Only refactorization. (for the approval)

Comment 5

[Mounir Lamouri (:mounir)]

Attachment: Patch v1.2

Updated patch for checkin.
r=smaug a=bsmedberg

Comment 6

[Mounir Lamouri (:mounir)]

If you push this patch, could you push the patch from bug 572897 with it? (this patch has to be applied before)

Comment 7

[(no longer active)]

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

Comment 8

[Mounir Lamouri (:mounir)]

Attachment: Fix bustage

Comment 9

[(no longer active)]

volkmar: when @value is set, it becomes the <input> value if !BF_VALUE_CHANGED
volkmar: it was done with Reset() which was doing SetValue() then resetting BF_VALUE_CHANGED
ehsan: hmm, ok, makes sense I guess
volkmar: so i've introduced a new argument to not set BF_VALUE_CHANGED when doing SetValue
volkmar: SetValueInternal
volkmar: but i forgot to do that for SetChecked

Comment 10

[Mounir Lamouri (:mounir)]

To be more accurate, the new "SetValueAsDefaultValue" method do not reset BF_VALUE_CHANGED because SetValueInternal has a new argument to not set it. Reset() do reset it because it has to.
However, I forgot to take into consideration DoSetChecked.

Sorry about that :(

Comment 11

[(no longer active)]

Bustage fix pushed: http://hg.mozilla.org/mozilla-central/rev/0dc03dd97a06