555711 - nsAttrValue::ParseNonNegativeIntValue should return PR_FALSE when the value is not valid

Status: RESOLVED FIXED

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

Description

[Mounir Lamouri (:mounir)]

Attachment: Patch v0.1

At the moment, if the value could not be parsed, it returns PR_FALSE but if the value is not valid (< 0), it is returning PR_TRUE. This can lead to issues like we got in bug 551846 (see comments 27 and 29).
NS_IMPL_NON_NEGATIVE_INT_ATTR_DEFAULT_VALUE should manage the default value alone we do not need to manage that in the parsing function.

"content/html/content/test/test_bug536891.html" is the test to check regressions.

Comment 1

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

Comment on attachment 435613 [details] [diff] [review]
Patch v0.1

Could you update test_bug536891.html to test also that getAttribute does the right thing.
Something like :
>+ var nonNumericValue = 'non-numerical-value';
>+ element.setAttribute('maxLength', nonNumericValue);
>- element.setAttribute('maxLength', 'non-numerical-value');
>  is(element.maxLength, -1, "non-numerical value should be considered invalid and represented as -1");
>+ is(element.getAttribute('maxlength'), nonNumericValue, "getAttribute should ");

If you add that to the test, r=me.

Comment 2

[Mounir Lamouri (:mounir)]

Attachment: Patch v0.1.1

r=smaug

Boris, may you sr this small patch ?

Comment 3

[Mounir Lamouri (:mounir)]

Olli, can we consider your previous review as an sr or I need to wait from a sr even for a so small patch ? It looks like everyone is busy with reviews....

Comment 4

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

Comment on attachment 435957 [details] [diff] [review]
Patch v0.1.1

I don't think this small patches (which aren't really API changes) need sr.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 435957 [details] [diff] [review]
Patch v0.1.1

fwiw, the test changes don't make sense.  The getAttribute test should be for -2147483648, not for "non-numerical-value".  The code change looks fine to me, though.

Comment 6

[Mounir Lamouri (:mounir)]

Attachment: Patcd v0.1.2

r=smaug

I think no additional tests was actually needed. The change has been made for negative value and the tests are already checking getAttribute for '-15'. No need to use PR_INT32_MIN.

Still waiting for checkin.

Comment 7

[Mounir Lamouri (:mounir)]

Could someone checkin this patch before it bitrots ?

Comment 8

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

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

Comment 9

[Mounir Lamouri (:mounir)]

Status hasn't been updated.