Transfer date/time value to chooser as double

Transfer date/time value to chooser as double

BUG=None

[keishi, 2013-11-07 18:03:56]

Blink side is here: https://codereview.chromium.org/62083004/

[miu, 2013-11-08 01:48:02]

https://codereview.chromium.org/64913002/diff/1/content/common/view_messages.h
File content/common/view_messages.h (right):

https://codereview.chromium.org/64913002/diff/1/content/common/view_messages....
content/common/view_messages.h:392: IPC_STRUCT_MEMBER(double, current_value)
Drive-by comment: "current_value" isn't very descriptive here.  Is this seconds
since unix epoch?  Also, have you considered using base::Time instead of double?

[keishi, 2013-11-08 02:13:07]

https://codereview.chromium.org/64913002/diff/1/content/common/view_messages.h
File content/common/view_messages.h (right):

https://codereview.chromium.org/64913002/diff/1/content/common/view_messages....
content/common/view_messages.h:392: IPC_STRUCT_MEMBER(double, current_value)
On 2013/11/08 01:48:02, Yuri wrote:
> Drive-by comment: "current_value" isn't very descriptive here.  Is this
seconds
> since unix epoch?  Also, have you considered using base::Time instead of
double?

It depends on the dialog_type. If it is "date", "datetime-local", "week" then it
is ms since epoch, for "month" it is months since epoch, "time" is ms since
midnight.
The conversion to double is defined by the HTML spec so it is well defined and
we don't have to do any conversions when crossing the Blink/chromium barrier. We
were already transferring min/max as doubles so we are just sharing that code.
But technically we can use base::Time instead of double here.

[Miguel Garcia, 2013-11-11 13:40:05]

https://codereview.chromium.org/64913002/diff/1/content/browser/android/date_...
File content/browser/android/date_time_chooser_android.h (left):

https://codereview.chromium.org/64913002/diff/1/content/browser/android/date_...
content/browser/android/date_time_chooser_android.h:41: void
ReplaceDateTime(JNIEnv* env,
nit: give names to the different values so they match the cc file. Also I think
they all fit in one line now

https://codereview.chromium.org/64913002/diff/1/content/browser/android/date_...
content/browser/android/date_time_chooser_android.h:43: jint dialog_type,
don't you need dialog_type?

https://codereview.chromium.org/64913002/diff/1/content/browser/android/date_...
File content/browser/android/date_time_chooser_android.h (right):

https://codereview.chromium.org/64913002/diff/1/content/browser/android/date_...
content/browser/android/date_time_chooser_android.h:33: // Replaces the current
value with the one passed the different fields
nit: change comment

https://codereview.chromium.org/64913002/diff/1/content/common/view_messages.h
File content/common/view_messages.h (right):

https://codereview.chromium.org/64913002/diff/1/content/common/view_messages....
content/common/view_messages.h:392: IPC_STRUCT_MEMBER(double, current_value)
since you have dialog_type already how about naming this dialog_value?

On 2013/11/08 02:13:07, keishi1 wrote:
> On 2013/11/08 01:48:02, Yuri wrote:
> > Drive-by comment: "current_value" isn't very descriptive here.  Is this
> seconds
> > since unix epoch?  Also, have you considered using base::Time instead of
> double?
> 
> It depends on the dialog_type. If it is "date", "datetime-local", "week" then
it
> is ms since epoch, for "month" it is months since epoch, "time" is ms since
> midnight.
> The conversion to double is defined by the HTML spec so it is well defined and
> we don't have to do any conversions when crossing the Blink/chromium barrier.
We
> were already transferring min/max as doubles so we are just sharing that code.
> But technically we can use base::Time instead of double here.

https://codereview.chromium.org/64913002/diff/1/content/public/android/java/s...
File
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java
(right):

https://codereview.chromium.org/64913002/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java:108:
Time time;
can you perhaps stick to Calendar through the whole file instead of using Time
sometimes and calendar some other times?

https://codereview.chromium.org/64913002/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java:110:
if (Double.isNaN(value)) {
can you document somewhere what is supposed to happen when you pass nan?

https://codereview.chromium.org/64913002/diff/1/content/renderer/date_time_fo...
File content/renderer/date_time_formatter.cc (right):

https://codereview.chromium.org/64913002/diff/1/content/renderer/date_time_fo...
content/renderer/date_time_formatter.cc:8: #include
"base/strings/utf_string_conversions.h"
is this still needed?

https://codereview.chromium.org/64913002/diff/1/content/renderer/date_time_fo...
content/renderer/date_time_formatter.cc:79: if (type_ ==
ui::TEXT_INPUT_TYPE_NONE) {
so won't this break for empty inputs of a correct type? How do you clear the
value of a form now?

https://codereview.chromium.org/64913002/diff/1/content/renderer/date_time_fo...
content/renderer/date_time_formatter.cc:86: switch (type_) {
why? it seems the previous version would try to set all fields. This seems to
complicate the logic and actually not set some fields some times without a good
reason.

https://codereview.chromium.org/64913002/diff/1/content/renderer/renderer_dat...
File content/renderer/renderer_date_time_picker.cc (right):

https://codereview.chromium.org/64913002/diff/1/content/renderer/renderer_dat...
content/renderer/renderer_date_time_picker.cc:11: 
don't you need to include ui/base/ime/text_input_type.h  for the enum constants?

https://codereview.chromium.org/64913002/diff/1/content/renderer/renderer_dat...
content/renderer/renderer_date_time_picker.cc:20: static ui::TextInputType
toTextInputType(int type) {
ui::TextInputType is an enum right? wouldn't it be better to have a method that
checks boundaries and then simply cast?

https://codereview.chromium.org/64913002/diff/1/content/renderer/renderer_dat...
content/renderer/renderer_date_time_picker.cc:24: break;
no need to break if you return

https://codereview.chromium.org/64913002/diff/1/content/renderer/renderer_dat...
content/renderer/renderer_date_time_picker.cc:40: case
WebKit::WebDateTimeInputTypeNone:
NOTREACHED?

[keishi, 2013-11-19 12:51:20]

https://codereview.chromium.org/64913002/diff/1/content/browser/android/date_...
File content/browser/android/date_time_chooser_android.h (left):

https://codereview.chromium.org/64913002/diff/1/content/browser/android/date_...
content/browser/android/date_time_chooser_android.h:41: void
ReplaceDateTime(JNIEnv* env,
On 2013/11/11 13:40:05, Miguel Garcia wrote:
> nit: give names to the different values so they match the cc file. Also I
think
> they all fit in one line now

Done.

https://codereview.chromium.org/64913002/diff/1/content/browser/android/date_...
content/browser/android/date_time_chooser_android.h:43: jint dialog_type,
On 2013/11/11 13:40:05, Miguel Garcia wrote:
> don't you need dialog_type?

The dialog_type would be useful when the replaceDateTime call is recieved by a
different RendererDateTimePicker than the one that opened the dialog. To be more
accurate I added an identifier to RendererDateTimePicker. The identifier is sent
to the browser process in ViewHostMsg_OpenDateTimeDialog and is sent back in
ViewMsg_ReplaceDateTime. RendererDateTimePicker can ignore the replaceDateTime
message if the identifier doesn't match.

I had to do the same thing for renderer_webcolorchooser_impl

https://codereview.chromium.org/64913002/diff/1/content/browser/android/date_...
File content/browser/android/date_time_chooser_android.h (right):

https://codereview.chromium.org/64913002/diff/1/content/browser/android/date_...
content/browser/android/date_time_chooser_android.h:33: // Replaces the current
value with the one passed the different fields
On 2013/11/11 13:40:05, Miguel Garcia wrote:
> nit: change comment

Done.

https://codereview.chromium.org/64913002/diff/1/content/common/view_messages.h
File content/common/view_messages.h (right):

https://codereview.chromium.org/64913002/diff/1/content/common/view_messages....
content/common/view_messages.h:392: IPC_STRUCT_MEMBER(double, current_value)
On 2013/11/11 13:40:05, Miguel Garcia wrote:
> since you have dialog_type already how about naming this dialog_value?
> 
> On 2013/11/08 02:13:07, keishi1 wrote:
> > On 2013/11/08 01:48:02, Yuri wrote:
> > > Drive-by comment: "current_value" isn't very descriptive here.  Is this
> > seconds
> > > since unix epoch?  Also, have you considered using base::Time instead of
> > double?
> > 
> > It depends on the dialog_type. If it is "date", "datetime-local", "week"
then
> it
> > is ms since epoch, for "month" it is months since epoch, "time" is ms since
> > midnight.
> > The conversion to double is defined by the HTML spec so it is well defined
and
> > we don't have to do any conversions when crossing the Blink/chromium
barrier.
> We
> > were already transferring min/max as doubles so we are just sharing that
code.
> > But technically we can use base::Time instead of double here.
> 

Done.

https://codereview.chromium.org/64913002/diff/1/content/public/android/java/s...
File
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java
(right):

https://codereview.chromium.org/64913002/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java:108:
Time time;
On 2013/11/11 13:40:05, Miguel Garcia wrote:
> can you perhaps stick to Calendar through the whole file instead of using Time
> sometimes and calendar some other times?

Done.

https://codereview.chromium.org/64913002/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java:110:
if (Double.isNaN(value)) {
On 2013/11/11 13:40:05, Miguel Garcia wrote:
> can you document somewhere what is supposed to happen when you pass nan?

Done. Added comment here.

https://codereview.chromium.org/64913002/diff/1/content/renderer/date_time_fo...
File content/renderer/date_time_formatter.cc (right):

https://codereview.chromium.org/64913002/diff/1/content/renderer/date_time_fo...
content/renderer/date_time_formatter.cc:79: if (type_ ==
ui::TEXT_INPUT_TYPE_NONE) {
On 2013/11/11 13:40:05, Miguel Garcia wrote:
> so won't this break for empty inputs of a correct type? How do you clear the
> value of a form now?

NaN represents empty value. I added code to handle it in blink.

I removed date_time_formatter files completely because it isn't used anymore.

https://codereview.chromium.org/64913002/diff/1/content/renderer/renderer_dat...
File content/renderer/renderer_date_time_picker.cc (right):

https://codereview.chromium.org/64913002/diff/1/content/renderer/renderer_dat...
content/renderer/renderer_date_time_picker.cc:11: 
On 2013/11/11 13:40:05, Miguel Garcia wrote:
> don't you need to include ui/base/ime/text_input_type.h  for the enum
constants?

Done.

https://codereview.chromium.org/64913002/diff/1/content/renderer/renderer_dat...
content/renderer/renderer_date_time_picker.cc:20: static ui::TextInputType
toTextInputType(int type) {
On 2013/11/11 13:40:05, Miguel Garcia wrote:
> ui::TextInputType is an enum right? wouldn't it be better to have a method
that
> checks boundaries and then simply cast?

Done.

https://codereview.chromium.org/64913002/diff/1/content/renderer/renderer_dat...
content/renderer/renderer_date_time_picker.cc:40: case
WebKit::WebDateTimeInputTypeNone:
On 2013/11/11 13:40:05, Miguel Garcia wrote:
> NOTREACHED?

Done.

[Miguel Garcia, 2013-11-20 23:05:52]

https://codereview.chromium.org/64913002/diff/160001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/input/DateTimeChooserAndroid.java
(right):

https://codereview.chromium.org/64913002/diff/160001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/DateTimeChooserAndroid.java:43:
private void showDialog(int dialogType, double value,
dialogValue

https://codereview.chromium.org/64913002/diff/160001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/DateTimeChooserAndroid.java:75:
double value);
dialogValue

https://codereview.chromium.org/64913002/diff/160001/content/renderer/rendere...
File content/renderer/renderer_date_time_picker.cc (right):

https://codereview.chromium.org/64913002/diff/160001/content/renderer/rendere...
content/renderer/renderer_date_time_picker.cc:70: if (identifier != identifier_)
what is this new identifier thing? It seems very fishy (and not thread safe) to
calculate the identifier should a static method.

https://codereview.chromium.org/64913002/diff/160001/content/renderer/rendere...
File content/renderer/renderer_date_time_picker.h (right):

https://codereview.chromium.org/64913002/diff/160001/content/renderer/rendere...
content/renderer/renderer_date_time_picker.h:11: #include
"ui/base/ime/text_input_type.h"
why do you need this here?

[keishi, 2013-11-21 15:03:08]

https://codereview.chromium.org/64913002/diff/160001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/input/DateTimeChooserAndroid.java
(right):

https://codereview.chromium.org/64913002/diff/160001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/DateTimeChooserAndroid.java:43:
private void showDialog(int dialogType, double value,
On 2013/11/20 23:05:53, Miguel Garcia wrote:
> dialogValue

Done.

https://codereview.chromium.org/64913002/diff/160001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/DateTimeChooserAndroid.java:75:
double value);
On 2013/11/20 23:05:53, Miguel Garcia wrote:
> dialogValue

Done.

https://codereview.chromium.org/64913002/diff/160001/content/renderer/rendere...
File content/renderer/renderer_date_time_picker.cc (right):

https://codereview.chromium.org/64913002/diff/160001/content/renderer/rendere...
content/renderer/renderer_date_time_picker.cc:70: if (identifier != identifier_)
On 2013/11/20 23:05:53, Miguel Garcia wrote:
> what is this new identifier thing? It seems very fishy (and not thread safe)
to
> calculate the identifier should a static method.

RendererDateTimePicker::Open() is called by blink so it is always called from
the main thread.
The replaceDateTime and openDateTimeDialog IPC messages can pass each other
during flight, causing the replaceDateTime to be received by a different input
element than the one that opened the dialog. We need an identifier to match them
up.

For example we have an HTML like this. 
This will open two date time dialogs when you tap the body.

<input type=date id=a>
<input type=date id=b>
<script>
a=document.getElementById('a');
b=document.getElementById('b');
document.body.onclick = function() {a.click();b.click(); }
</script>

If the IPC queue is busy something like this could happen.
1. OpenDateTimeDialog is sent for element "a"
2. OpenDateTimeDialog is sent for element "b"
3. OpenDateTimeDialog for element "a" is received by the browser. Dialog opens.
4. User selects date. ReplaceDateTime is sent from the browser to the renderer.
5. OpenDateTimeDialog for element "b" is received by the browser.
6. ReplaceDateTime is received by the renderer. value is set to element b

Therefore it is insufficient to check the type and we should be matching up the
object on the renderer side that triggered the dialog.

https://codereview.chromium.org/64913002/diff/160001/content/renderer/rendere...
File content/renderer/renderer_date_time_picker.h (right):

https://codereview.chromium.org/64913002/diff/160001/content/renderer/rendere...
content/renderer/renderer_date_time_picker.h:11: #include
"ui/base/ime/text_input_type.h"
On 2013/11/20 23:05:53, Miguel Garcia wrote:
> why do you need this here?

Removed. It was left over.

[keishi, 2013-11-25 12:18:05]

On 2013/11/21 15:03:08, keishi1 wrote:
>
https://codereview.chromium.org/64913002/diff/160001/content/public/android/j...
> File
>
content/public/android/java/src/org/chromium/content/browser/input/DateTimeChooserAndroid.java
> (right):
> 
>
https://codereview.chromium.org/64913002/diff/160001/content/public/android/j...
>
content/public/android/java/src/org/chromium/content/browser/input/DateTimeChooserAndroid.java:43:
> private void showDialog(int dialogType, double value,
> On 2013/11/20 23:05:53, Miguel Garcia wrote:
> > dialogValue
> 
> Done.
> 
>
https://codereview.chromium.org/64913002/diff/160001/content/public/android/j...
>
content/public/android/java/src/org/chromium/content/browser/input/DateTimeChooserAndroid.java:75:
> double value);
> On 2013/11/20 23:05:53, Miguel Garcia wrote:
> > dialogValue
> 
> Done.
> 
>
https://codereview.chromium.org/64913002/diff/160001/content/renderer/rendere...
> File content/renderer/renderer_date_time_picker.cc (right):
> 
>
https://codereview.chromium.org/64913002/diff/160001/content/renderer/rendere...
> content/renderer/renderer_date_time_picker.cc:70: if (identifier !=
identifier_)
> On 2013/11/20 23:05:53, Miguel Garcia wrote:
> > what is this new identifier thing? It seems very fishy (and not thread safe)
> to
> > calculate the identifier should a static method.
> 
> RendererDateTimePicker::Open() is called by blink so it is always called from
> the main thread.
> The replaceDateTime and openDateTimeDialog IPC messages can pass each other
> during flight, causing the replaceDateTime to be received by a different input
> element than the one that opened the dialog. We need an identifier to match
them
> up.
> 
> For example we have an HTML like this. 
> This will open two date time dialogs when you tap the body.
> 
> <input type=date id=a>
> <input type=date id=b>
> <script>
> a=document.getElementById('a');
> b=document.getElementById('b');
> document.body.onclick = function() {a.click();b.click(); }
> </script>
> 
> If the IPC queue is busy something like this could happen.
> 1. OpenDateTimeDialog is sent for element "a"
> 2. OpenDateTimeDialog is sent for element "b"
> 3. OpenDateTimeDialog for element "a" is received by the browser. Dialog
opens.
> 4. User selects date. ReplaceDateTime is sent from the browser to the
renderer.
> 5. OpenDateTimeDialog for element "b" is received by the browser.
> 6. ReplaceDateTime is received by the renderer. value is set to element b
> 
> Therefore it is insufficient to check the type and we should be matching up
the
> object on the renderer side that triggered the dialog.
> 
>
https://codereview.chromium.org/64913002/diff/160001/content/renderer/rendere...
> File content/renderer/renderer_date_time_picker.h (right):
> 
>
https://codereview.chromium.org/64913002/diff/160001/content/renderer/rendere...
> content/renderer/renderer_date_time_picker.h:11: #include
> "ui/base/ime/text_input_type.h"
> On 2013/11/20 23:05:53, Miguel Garcia wrote:
> > why do you need this here?
> 
> Removed. It was left over.

migueal@ if you still don't like the identifier, how about something like this?
https://codereview.chromium.org/85643002
The RenderViewImpl ensures that there is only one date time picker open at a
time and the openDateTImeChooser call fails while one is already open.