[webkit-reviews] review granted: [Bug 33825] HTMLInputElement::valueAsDate setter support for type=time. : [Attachment 46883] Proposed patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted TAMURA, Kent <tkent at chromium.org>'s
request for review:
Bug 33825: HTMLInputElement::valueAsDate setter support for type=time.
https://bugs.webkit.org/show_bug.cgi?id=33825

Attachment 46883: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=46883&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +var date = new Date();
> +date.setTime(8.65E15); // Date of JavaScript can't represent this.
> +input.valueAsDate = date;
> +shouldBe('input.value', '""');

A test like this can be written as follows:

    shouldBe('var date = new Date(); date.setTime(8.65E15); input.valueAsDate =
date; input.value', '""');

Then the test result output is a lot easier to read.

> +input.value = '00:00';
> +input.valueAsDate = document;
> +shouldBe('input.value', '""');

> +input.value = '00:00';
> +input.valueAsDate = null;
> +shouldBe('input.value', '""');

Similarly:

    shouldBe('input.value = "00:00"; input.valueAsDate = document;
input.value', '""');
    shouldBe('input.value = "00:00"; input.valueAsDate = null; input.value',
'""');

> +    // FIXME: We should specify SecondFormat.

Can you add a test case that demonstrates this is not working?

> +static inline double positiveFmod(double value, double divider)
> +{
> +    double remainder = fmod(value, divider);
> +    return remainder < 0.0 ? remainder + divider : remainder;
> +}

Could just be "0" rather than "0.0". Normally we do that.

> +    if (msInDay < 0.0 || msInDay > msPerDay)
> +	   return false;

Ditto.

Also, I think this should be >= msPerDay, not > msPerDay. Can we make a test
case that exercises that code path? Or maybe these should be assertions rather
than a return statement. The long caller seems to already guarantee a good
value, so I'm not sure why this function needs both a return value and a
failure case rather than just an assertion. Or the fmod could just be moved in
here.

Note that this expression will not return in the case of an NaN. Because
expressions involving NaN return false, it's best to check things you want to
be true rather than things you want to be false when it comes to floating point
numbers.

> +    m_millisecond = static_cast<int>(fmod(msInDay, msPerSecond));

Should this be a truncation rather than rounding? Could you make a test case
for that?

Otherwise looks OK. r=me