Bug 33825 – HTMLInputElement::valueAsDate setter support for type=time.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Kent Tamura]

HTMLInputElement::valueAsDate setter support for type=time.

[Kent Tamura]

Created attachment 46883 [details]
Proposed patch

[Kent Tamura]

Add Darin to CC because he reviewed the valueAsDate getter part and valueAsDate setter for type=month.

[Darin Adler]

Comment on attachment 46883 [details]
Proposed patch

> +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

[Kent Tamura]

(In reply to comment #3)
> (From update of attachment 46883 [details])
> > +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.

Done.
 It's great. I had no idea to write multiple expressions there.

> > +    // FIXME: We should specify SecondFormat.
> 
> Can you add a test case that demonstrates this is not working?

Done.

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

Done.

> 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.

Right." >= msPerDay" was my intent.
We can enforce the input value is always correct.  So I changed it to ASSERT() and  this function is void now.

> > +    m_millisecond = static_cast<int>(fmod(msInDay, msPerSecond));
> 
> Should this be a truncation rather than rounding? Could you make a test case
> for that?

Good point. Though I don't think we need rounding with the current code, we might change the time representation to second-based value instead of millisecond-base value as you recommended in another bug.
I added round() to the caller of this function.

Well, the change from the last patch will be the following.  I'll commit it soon.

diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
index 4dc801e..1898749 100644
--- a/LayoutTests/ChangeLog
+++ b/LayoutTests/ChangeLog
@@ -8,6 +8,9 @@
         Add setter tests to input-valueasdate-time.js, and update the
         expectation.
 
+        Note: the expectation file contains some FAIL lines. They are
+        intentional because they test a unimplemented feature.
+
         * fast/forms/input-valueasdate-time-expected.txt:
         * fast/forms/script-tests/input-valueasdate-time.js:
 
diff --git a/LayoutTests/fast/forms/input-valueasdate-time-expected.txt b/LayoutTests/fast/forms/input-valueasdate-time-expected.txt
index b93fe2f..9d02aaf 100644
--- a/LayoutTests/fast/forms/input-valueasdate-time-expected.txt
+++ b/LayoutTests/fast/forms/input-valueasdate-time-expected.txt
@@ -17,10 +17,13 @@ PASS setValueAsDateAndGetValue(24, 0, 0, 0) is "00:00"
 PASS setValueAsDateAndGetValue(48, 0, 13, 0) is "00:00:13"
 PASS setValueAsDateAndGetValue(-23, -59, -59, 0) is "00:00:01"
 Invalid Date:
-PASS input.value is ""
+PASS var date = new Date(); date.setTime(8.65E15); input.valueAsDate = date; input.value is ""
 Invalid objects:
-PASS input.value is ""
-PASS input.value is ""
+PASS input.value = "00:00"; input.valueAsDate = document; input.value is ""
+PASS input.value = "00:00"; input.valueAsDate = null; input.value is ""
+Step attribute value and string representation:
+FAIL input.step = "1"; setValueAsDateAndGetValue(0, 0, 0, 0) should be 00:00:00. Was 00:00.
+FAIL input.step = "0.001"; setValueAsDateAndGetValue(0, 0, 0, 0) should be 00:00:00.000. Was 00:00.
 PASS successfullyParsed is true
 
 TEST COMPLETE
diff --git a/LayoutTests/fast/forms/script-tests/input-valueasdate-time.js b/LayoutTests/fast/forms/script-tests/input-valueasdate-time.js
index e08b3f4..583b3a0 100644
--- a/LayoutTests/fast/forms/script-tests/input-valueasdate-time.js
+++ b/LayoutTests/fast/forms/script-tests/input-valueasdate-time.js
@@ -31,17 +31,16 @@ shouldBe('setValueAsDateAndGetValue(48, 0, 13, 0)', '"00:00:13"');
 shouldBe('setValueAsDateAndGetValue(-23, -59, -59, 0)', '"00:00:01"');
 
 debug('Invalid Date:');
-var date = new Date();
-date.setTime(8.65E15); // Date of JavaScript can't represent this.
-input.valueAsDate = date;
-shouldBe('input.value', '""');
-
+// Date of JavaScript can't represent 8.65E15.
+shouldBe('var date = new Date(); date.setTime(8.65E15); input.valueAsDate = date; input.value', '""');
 debug('Invalid objects:');
-input.value = '00:00';
-input.valueAsDate = document;
-shouldBe('input.value', '""');
-input.value = '00:00';
-input.valueAsDate = null;
-shouldBe('input.value', '""');
+shouldBe('input.value = "00:00"; input.valueAsDate = document; input.value', '""');
+shouldBe('input.value = "00:00"; input.valueAsDate = null; input.value', '""');
+
+debug('Step attribute value and string representation:');
+// If the step attribute value is 1 second and the second part is 0, we should show the second part.
+shouldBe('input.step = "1"; setValueAsDateAndGetValue(0, 0, 0, 0)', '"00:00:00"');
+// If the step attribute value is 0.001 second and the millisecond part is 0, we should show the millisecond part.
+shouldBe('input.step = "0.001"; setValueAsDateAndGetValue(0, 0, 0, 0)', '"00:00:00.000"');
 
 var successfullyParsed = true;
diff --git a/WebCore/html/ISODateTime.cpp b/WebCore/html/ISODateTime.cpp
index d266fd2..6fbc64a 100644
--- a/WebCore/html/ISODateTime.cpp
+++ b/WebCore/html/ISODateTime.cpp
@@ -453,20 +453,18 @@ bool ISODateTime::parseDateTime(const UChar* src, unsigned length, unsigned star
 static inline double positiveFmod(double value, double divider)
 {
     double remainder = fmod(value, divider);
-    return remainder < 0.0 ? remainder + divider : remainder;
+    return remainder < 0 ? remainder + divider : remainder;
 }
 
-bool ISODateTime::setMillisecondsSinceMidnightInternal(double msInDay)
+void ISODateTime::setMillisecondsSinceMidnightInternal(double msInDay)
 {
-    if (msInDay < 0.0 || msInDay > msPerDay)
-        return false;
+    ASSERT(msInDay >= 0 && msInDay < msPerDay);
     m_millisecond = static_cast<int>(fmod(msInDay, msPerSecond));
     double value = floor(msInDay / msPerSecond);
     m_second = static_cast<int>(fmod(value, secondsPerMinute));
     value = floor(value / secondsPerMinute);
     m_minute = static_cast<int>(fmod(value, minutesPerHour));
     m_hour = static_cast<int>(value / minutesPerHour);
-    return true;
 }
 
 bool ISODateTime::setMillisecondsSinceEpochForDateInternal(double ms)
@@ -495,9 +493,7 @@ bool ISODateTime::setMillisecondsSinceMidnight(double ms)
 {
     if (!isfinite(ms))
         return false;
-    double msInDay = positiveFmod(ms, msPerDay);
-    if (!setMillisecondsSinceMidnightInternal(msInDay))
-        return false;
+    setMillisecondsSinceMidnightInternal(positiveFmod(round(ms), msPerDay));
     m_type = Time;
     return true;
 }
diff --git a/WebCore/html/ISODateTime.h b/WebCore/html/ISODateTime.h
index 15e56b2..f0dc25f 100644
--- a/WebCore/html/ISODateTime.h
+++ b/WebCore/html/ISODateTime.h
@@ -131,7 +131,7 @@ private:
     double millisecondsSinceEpochForTime() const;
     // Helpers for setMillisecondsSinceEpochFor*().
     bool setMillisecondsSinceEpochForDateInternal(double ms);
-    bool setMillisecondsSinceMidnightInternal(double ms);
+    void setMillisecondsSinceMidnightInternal(double ms);
     // Helper for toString().
     String toStringForTime(SecondFormat) const;

[Kent Tamura]

Landed as r53551.