Transfer date/time value to the chooser as a double

Transfer date/time value to the chooser as a double

We want to avoid passing strings in IPCs. We also don't want string to date conversions in multiple places. Sending all types of values as double simplifies the code, since we already do it for min/max.

Adding WebDateTimeChooserParams::doubleValue.
Adding didChooseValue(double value)

BUG=None

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=162480

[keishi, 2013-11-07 18:05:57]

Chromium side is here: https://codereview.chromium.org/64913002/

[tkent, 2013-11-07 19:42:06]

Please write why you'd like to pass it in double in the CL description.

https://codereview.chromium.org/62083004/diff/20001/Source/core/html/forms/Ba...
File Source/core/html/forms/BaseChooserOnlyDateAndTimeInputType.cpp (right):

https://codereview.chromium.org/62083004/diff/20001/Source/core/html/forms/Ba...
Source/core/html/forms/BaseChooserOnlyDateAndTimeInputType.cpp:103:
element().setValue(String(), DispatchInputAndChangeEvent);
Are you sure this should be String(), not emptyString()?

https://codereview.chromium.org/62083004/diff/20001/Source/core/html/forms/Ba...
File Source/core/html/forms/BaseMultipleFieldsDateAndTimeInputType.cpp (right):

https://codereview.chromium.org/62083004/diff/20001/Source/core/html/forms/Ba...
Source/core/html/forms/BaseMultipleFieldsDateAndTimeInputType.cpp:268:
element().setValue(String(), DispatchInputAndChangeEvent);
Ditto.

https://codereview.chromium.org/62083004/diff/20001/Source/core/html/shadow/P...
File Source/core/html/shadow/PickerIndicatorElement.cpp (right):

https://codereview.chromium.org/62083004/diff/20001/Source/core/html/shadow/P...
Source/core/html/shadow/PickerIndicatorElement.cpp:105: if
(!m_pickerIndicatorOwner)
nit:

if (m_pickerIndicaterOwner)
    m_pickerIndicatorOwner->pickerIndicatorChooseValue(value);

is shoter.

https://codereview.chromium.org/62083004/diff/20001/Source/platform/DateTimeC...
File Source/platform/DateTimeChooser.h (right):

https://codereview.chromium.org/62083004/diff/20001/Source/platform/DateTimeC...
Source/platform/DateTimeChooser.h:49: double doubleValue;
Will you remove |String currentValue|?

https://codereview.chromium.org/62083004/diff/20001/Source/platform/DateTimeC...
File Source/platform/DateTimeChooserClient.h (right):

https://codereview.chromium.org/62083004/diff/20001/Source/platform/DateTimeC...
Source/platform/DateTimeChooserClient.h:46: virtual void didChooseValue(double)
= 0;
Will you remove didChooseValue(const String&)?

https://codereview.chromium.org/62083004/diff/20001/public/web/WebDateTimeCho...
File public/web/WebDateTimeChooserCompletion.h (right):

https://codereview.chromium.org/62083004/diff/20001/public/web/WebDateTimeCho...
public/web/WebDateTimeChooserCompletion.h:44: virtual void
didChooseValue(double) = 0;
Will you remove didChooseValue(const WebString&)?

https://codereview.chromium.org/62083004/diff/20001/public/web/WebDateTimeCho...
File public/web/WebDateTimeChooserParams.h (right):

https://codereview.chromium.org/62083004/diff/20001/public/web/WebDateTimeCho...
public/web/WebDateTimeChooserParams.h:44: WebString currentValue;
Will you remove |WebString currentValue|?

https://codereview.chromium.org/62083004/diff/20001/public/web/WebDateTimeCho...
public/web/WebDateTimeChooserParams.h:46: double doubleValue;
Can the value be NaN?

[keishi, 2013-11-07 20:04:10]

> Please write why you'd like to pass it in double in the CL description.
Done.

https://codereview.chromium.org/62083004/diff/20001/Source/core/html/forms/Ba...
File Source/core/html/forms/BaseChooserOnlyDateAndTimeInputType.cpp (right):

https://codereview.chromium.org/62083004/diff/20001/Source/core/html/forms/Ba...
Source/core/html/forms/BaseChooserOnlyDateAndTimeInputType.cpp:103:
element().setValue(String(), DispatchInputAndChangeEvent);
On 2013/11/07 19:42:07, tkent wrote:
> Are you sure this should be String(), not emptyString()?

Done.

https://codereview.chromium.org/62083004/diff/20001/Source/core/html/forms/Ba...
File Source/core/html/forms/BaseMultipleFieldsDateAndTimeInputType.cpp (right):

https://codereview.chromium.org/62083004/diff/20001/Source/core/html/forms/Ba...
Source/core/html/forms/BaseMultipleFieldsDateAndTimeInputType.cpp:268:
element().setValue(String(), DispatchInputAndChangeEvent);
On 2013/11/07 19:42:07, tkent wrote:
> Ditto.

Done.

https://codereview.chromium.org/62083004/diff/20001/Source/core/html/shadow/P...
File Source/core/html/shadow/PickerIndicatorElement.cpp (right):

https://codereview.chromium.org/62083004/diff/20001/Source/core/html/shadow/P...
Source/core/html/shadow/PickerIndicatorElement.cpp:105: if
(!m_pickerIndicatorOwner)
On 2013/11/07 19:42:07, tkent wrote:
> nit:
> 
> if (m_pickerIndicaterOwner)
>     m_pickerIndicatorOwner->pickerIndicatorChooseValue(value);
> 
> is shoter.

Done.

https://codereview.chromium.org/62083004/diff/20001/Source/platform/DateTimeC...
File Source/platform/DateTimeChooser.h (right):

https://codereview.chromium.org/62083004/diff/20001/Source/platform/DateTimeC...
Source/platform/DateTimeChooser.h:49: double doubleValue;
On 2013/11/07 19:42:07, tkent wrote:
> Will you remove |String currentValue|?

Yes. Added comment

https://codereview.chromium.org/62083004/diff/20001/Source/platform/DateTimeC...
File Source/platform/DateTimeChooserClient.h (right):

https://codereview.chromium.org/62083004/diff/20001/Source/platform/DateTimeC...
Source/platform/DateTimeChooserClient.h:46: virtual void didChooseValue(double)
= 0;
On 2013/11/07 19:42:07, tkent wrote:
> Will you remove didChooseValue(const String&)?
yes. Added fixme comment.

https://codereview.chromium.org/62083004/diff/20001/public/web/WebDateTimeCho...
File public/web/WebDateTimeChooserCompletion.h (right):

https://codereview.chromium.org/62083004/diff/20001/public/web/WebDateTimeCho...
public/web/WebDateTimeChooserCompletion.h:44: virtual void
didChooseValue(double) = 0;
On 2013/11/07 19:42:07, tkent wrote:
> Will you remove didChooseValue(const WebString&)?

Yes. Added fixme comment.

https://codereview.chromium.org/62083004/diff/20001/public/web/WebDateTimeCho...
File public/web/WebDateTimeChooserParams.h (right):

https://codereview.chromium.org/62083004/diff/20001/public/web/WebDateTimeCho...
public/web/WebDateTimeChooserParams.h:44: WebString currentValue;
On 2013/11/07 19:42:07, tkent wrote:
> Will you remove |WebString currentValue|?

Yes. Added comment.

https://codereview.chromium.org/62083004/diff/20001/public/web/WebDateTimeCho...
public/web/WebDateTimeChooserParams.h:46: double doubleValue;
On 2013/11/07 19:42:07, tkent wrote:
> Can the value be NaN?

Yes. Added comment.

[tkent, 2013-11-07 20:10:08]

https://codereview.chromium.org/62083004/diff/120001/public/web/WebDateTimeCh...
File public/web/WebDateTimeChooserCompletion.h (right):

https://codereview.chromium.org/62083004/diff/120001/public/web/WebDateTimeCh...
public/web/WebDateTimeChooserCompletion.h:43: // is destroyed when this method
is called. If the value is Nan it means an
What about infinity values?

https://codereview.chromium.org/62083004/diff/120001/public/web/WebDateTimeCh...
File public/web/WebDateTimeChooserParams.h (right):

https://codereview.chromium.org/62083004/diff/120001/public/web/WebDateTimeCh...
public/web/WebDateTimeChooserParams.h:47: // NaN means empty value.
Ditto. Can it be infinity?

[Miguel Garcia, 2013-11-11 11:35:13]

Pass by review. It seems it would be very easy to add a unittest for this logic
and it'd be really worth it so we know we are not returning different times
tamps that in the current ICU based implementation inside chromium.

https://codereview.chromium.org/62083004/diff/120001/Source/platform/DateTime...
File Source/platform/DateTimeChooser.h (right):

https://codereview.chromium.org/62083004/diff/120001/Source/platform/DateTime...
Source/platform/DateTimeChooser.h:50: double doubleValue;
maybe timestampValue?

https://codereview.chromium.org/62083004/diff/120001/Source/web/DateTimeChoos...
File Source/web/DateTimeChooserImpl.cpp (right):

https://codereview.chromium.org/62083004/diff/120001/Source/web/DateTimeChoos...
Source/web/DateTimeChooserImpl.cpp:88:
components.setMillisecondsSinceEpochForDate(value);
perhaps consider early return in all these branches?

https://codereview.chromium.org/62083004/diff/120001/Source/web/DateTimeChoos...
Source/web/DateTimeChooserImpl.cpp:99: return components.type() ==
WebCore::DateComponents::Invalid ? String() : components.toString();
and then just return String() always here.

[keishi, 2013-11-19 03:28:04]

Added test WebViewTest.ChooseValueFromDateTimeChooser

https://codereview.chromium.org/62083004/diff/120001/Source/platform/DateTime...
File Source/platform/DateTimeChooser.h (right):

https://codereview.chromium.org/62083004/diff/120001/Source/platform/DateTime...
Source/platform/DateTimeChooser.h:50: double doubleValue;
On 2013/11/11 11:35:13, Miguel Garcia wrote:
> maybe timestampValue?

We have never used "timestamp" to describe date/time input values. I'll use
doubleValue for now and rename it to currentValue when removing the string
version.

https://codereview.chromium.org/62083004/diff/120001/Source/web/DateTimeChoos...
File Source/web/DateTimeChooserImpl.cpp (right):

https://codereview.chromium.org/62083004/diff/120001/Source/web/DateTimeChoos...
Source/web/DateTimeChooserImpl.cpp:88:
components.setMillisecondsSinceEpochForDate(value);
On 2013/11/11 11:35:13, Miguel Garcia wrote:
> perhaps consider early return in all these branches?

When the set*() method fails the components.type() is set to Invalid so we need
to check that.
I think early return will increase the number of lines.

https://codereview.chromium.org/62083004/diff/120001/public/web/WebDateTimeCh...
File public/web/WebDateTimeChooserCompletion.h (right):

https://codereview.chromium.org/62083004/diff/120001/public/web/WebDateTimeCh...
public/web/WebDateTimeChooserCompletion.h:43: // is destroyed when this method
is called. If the value is Nan it means an
On 2013/11/07 20:10:08, tkent wrote:
> What about infinity values?

It shouldn't be infinity. I added assertions to
BaseChooserOnlyDateAndTimeInputType::didChooseValue and
BaseMultipleFieldsDateAndTimeInputType::pickerIndicatorChooseValue

[tkent, 2013-11-21 00:09:42]

https://codereview.chromium.org/62083004/diff/230001/Source/web/tests/WebView...
File Source/web/tests/WebViewTest.cpp (right):

https://codereview.chromium.org/62083004/diff/230001/Source/web/tests/WebView...
Source/web/tests/WebViewTest.cpp:1270: static bool openDateTimeChooser(WebView*
webView, WebCore::HTMLInputElement* inputElement)
The return value is not used.  It should be void.

https://codereview.chromium.org/62083004/diff/230001/Source/web/tests/WebView...
Source/web/tests/WebViewTest.cpp:1295:
page->chrome().client().setPagePopupDriver(mockPagePopupDriver.get());
Is MockPagePopupDriver needed though it's for Android?

Anyway, this environment change is not good for test stability, and we should
restore non-mock page popup driver.  We should override testing::Test::SetUp()
and TearDown().

https://codereview.chromium.org/62083004/diff/230001/public/web/WebDateTimeCh...
File public/web/WebDateTimeChooserCompletion.h (right):

https://codereview.chromium.org/62083004/diff/230001/public/web/WebDateTimeCh...
public/web/WebDateTimeChooserCompletion.h:44: // empty value.
should document that infinity should not be passed.

https://codereview.chromium.org/62083004/diff/230001/public/web/WebDateTimeCh...
File public/web/WebDateTimeChooserParams.h (right):

https://codereview.chromium.org/62083004/diff/230001/public/web/WebDateTimeCh...
public/web/WebDateTimeChooserParams.h:47: // NaN means empty value.
Ditto.

[keishi, 2013-11-21 15:07:12]

https://codereview.chromium.org/62083004/diff/230001/Source/web/tests/WebView...
File Source/web/tests/WebViewTest.cpp (right):

https://codereview.chromium.org/62083004/diff/230001/Source/web/tests/WebView...
Source/web/tests/WebViewTest.cpp:1270: static bool openDateTimeChooser(WebView*
webView, WebCore::HTMLInputElement* inputElement)
On 2013/11/21 00:09:43, tkent wrote:
> The return value is not used.  It should be void.

Done.

https://codereview.chromium.org/62083004/diff/230001/Source/web/tests/WebView...
Source/web/tests/WebViewTest.cpp:1295:
page->chrome().client().setPagePopupDriver(mockPagePopupDriver.get());
On 2013/11/21 00:09:43, tkent wrote:
> Is MockPagePopupDriver needed though it's for Android?
> 
> Anyway, this environment change is not good for test stability, and we should
> restore non-mock page popup driver.  We should override testing::Test::SetUp()
> and TearDown().

This was left over from my efforts to run this test on non Android. Removed.

https://codereview.chromium.org/62083004/diff/230001/public/web/WebDateTimeCh...
File public/web/WebDateTimeChooserCompletion.h (right):

https://codereview.chromium.org/62083004/diff/230001/public/web/WebDateTimeCh...
public/web/WebDateTimeChooserCompletion.h:44: // empty value.
On 2013/11/21 00:09:43, tkent wrote:
> should document that infinity should not be passed.
> 

Done.

https://codereview.chromium.org/62083004/diff/230001/public/web/WebDateTimeCh...
File public/web/WebDateTimeChooserParams.h (right):

https://codereview.chromium.org/62083004/diff/230001/public/web/WebDateTimeCh...
public/web/WebDateTimeChooserParams.h:47: // NaN means empty value.
On 2013/11/21 00:09:43, tkent wrote:
> Ditto.

Done.

[tkent, 2013-11-21 15:21:26]

lgtm

[commit-bot: I haz the power, 2013-11-21 15:21:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/keishi@chromium.org/62083004/380001

[commit-bot: I haz the power, 2013-11-21 16:18:21]

Change committed as 162480