Transfer date/time value to chooser as double

Transfer date/time value to chooser as double

BUG=324151
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=238594

[Miguel Garcia, 2013-11-26 16:07:08]

So I still don't understand why can you have more than one dialog opened at
once.

I do see that we should be removing the reference in the renderer when we remove
it from blink though.

In any case I strongly prefer this version over the one where an identifier is
generated.

Can you update the subject and the description? This is doing much more than
announced (using millis since epoch from instead of year/month/day... tuples for
example).

Can you also update the bug number?

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

https://codereview.chromium.org/85643002/diff/1/content/browser/android/date_...
content/browser/android/date_time_chooser_android.cc:44: host_->Send(new
ViewMsg_ReplaceDateTime(host_->GetRoutingID(),
I think this fits in one line.

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

https://codereview.chromium.org/85643002/diff/1/content/browser/android/date_...
content/browser/android/date_time_chooser_android.h:28: double current_value,
keep dialog_value to be consistent with the cc file.
Document the parameters now that they are not self explanatory (value is the
dialog value in millis since epoch except for month... or maybe refer to where
are they declared in WebKit).

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

https://codereview.chromium.org/85643002/diff/1/content/common/view_messages....
content/common/view_messages.h:969: double /* dialog_value */)
so if you are passing milliseconds since epoch why are you using a double
(floating point) instead of simply an int64? I guess the same applies for min
and max.

https://codereview.chromium.org/85643002/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/85643002/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java:95:
if (Double.isNaN(dialogValue)) {
I would really like to see a test that checks that for a given dialog type and
dialog value the right YEAR MONTH DAY HOUR... combination is generated. We used
to a similar thing in date_time_formatter_unittest.cc but this has been removed
now.

https://codereview.chromium.org/85643002/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java:96:
cal = Calendar.getInstance();
How do you pass Nan via JNI BTW?

https://codereview.chromium.org/85643002/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java:100:
if (dialogType == sTextInputTypeMonth) {
please document why month is special.

https://codereview.chromium.org/85643002/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java:221:
(double) ((hourOfDay * 60 + minute) * 60 + second) * 1000 + milli);
Use TimeUnit(HOUR).toMillis() + TimeUnit.MINUTE(minute).toMillis +
TimeUnit.SECOND(second).toMillis() + millis

https://codereview.chromium.org/85643002/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java:222:
}
You are not setting mDialogAlreadyDismissed in these cases now since you don't
call setFieldDateTimeValue. 

 I recommend you always call setFieldDateTimeValue and modify that methods so it
takes a dialog type. Then you can generate the right double. If you make the
method static and public and make it return the right double you can actually
unittest it as well.

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

https://codereview.chromium.org/85643002/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/input/MonthPicker.java:48:
protected long valueFromDate(int year, int month) {
shouldn't this return a double? That's what you are using all the time..

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

https://codereview.chromium.org/85643002/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/input/WeekPicker.java:53:
protected long valueFromDate(int year, int week) {
another double vs long here

https://codereview.chromium.org/85643002/diff/1/content/renderer/render_view_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/85643002/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:6381: if (date_time_picker_client_)
Please add a comment stating when can this happen, I can't see how you can have
more than one dialog open since they are full screen. Specially within the same
renderer.

https://codereview.chromium.org/85643002/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:6388: void
RenderViewImpl::DidDismissDateTimeDialog() {
This is such a simple implementation that perhaps you can inline it in the .h
file. See my comment about the signature as well.

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

https://codereview.chromium.org/85643002/diff/1/content/renderer/renderer_dat...
content/renderer/renderer_date_time_picker.cc:72:
static_cast<RenderViewImpl*>(render_view())->DidDismissDateTimeDialog();
The method signature is confusing. It seems it should be
DismissDateTimeDialog();

I agree that if the dialog is dissmissed in WebKit the reference should also be
cleared in the renderer which it seems was not happening before.

In the browser we also hold a pointer to the dialog chooser 
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we...

Is that cleared somehow?

[keishi, 2013-11-27 06:58:37]

Thanks for reviewing!

> So I still don't understand why can you have more than one dialog opened at
once.

JavaScript can open the dialog by calling element.click()
and JavaScript will keep running while the dialog is open.

Blink has restrictions in place to avoid popup dialog unless a user gesture was
involved.
So docuemnt.body.onload="element.click()" won't work but
docuemnt.body.onclick="element.click()" will open the dialog.

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

https://codereview.chromium.org/85643002/diff/1/content/browser/android/date_...
content/browser/android/date_time_chooser_android.cc:44: host_->Send(new
ViewMsg_ReplaceDateTime(host_->GetRoutingID(),
On 2013/11/26 16:07:08, Miguel Garcia wrote:
> I think this fits in one line.

Done.

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

https://codereview.chromium.org/85643002/diff/1/content/browser/android/date_...
content/browser/android/date_time_chooser_android.h:28: double current_value,
On 2013/11/26 16:07:08, Miguel Garcia wrote:
> keep dialog_value to be consistent with the cc file.
> Document the parameters now that they are not self explanatory (value is the
> dialog value in millis since epoch except for month... or maybe refer to where
> are they declared in WebKit).

Done. Added comment.

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

https://codereview.chromium.org/85643002/diff/1/content/common/view_messages....
content/common/view_messages.h:969: double /* dialog_value */)
On 2013/11/26 16:07:08, Miguel Garcia wrote:
> so if you are passing milliseconds since epoch why are you using a double
> (floating point) instead of simply an int64? I guess the same applies for min
> and max.
> 

The HTML spec defines min/max for the date/time values. dialog_value will always
be between -62135596800000.0 and 8640000000000000.0 so we can use double without
loss in precision. We can also express an empty value as NaN.

https://codereview.chromium.org/85643002/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/85643002/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java:95:
if (Double.isNaN(dialogValue)) {
On 2013/11/26 16:07:08, Miguel Garcia wrote:
> I would really like to see a test that checks that for a given dialog type and
> dialog value the right YEAR MONTH DAY HOUR... combination is generated. We
used
> to a similar thing in date_time_formatter_unittest.cc but this has been
removed
> now.

I separated out the double to year/month/date... conversion so we can unit test
it. I couldn't find examples of Java unit tests so I'm not sure how to write
one.

https://codereview.chromium.org/85643002/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java:96:
cal = Calendar.getInstance();
On 2013/11/26 16:07:08, Miguel Garcia wrote:
> How do you pass Nan via JNI BTW?

I'm not sure what you mean. NaN is passed from C++ to Java using double.

https://codereview.chromium.org/85643002/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java:100:
if (dialogType == sTextInputTypeMonth) {
On 2013/11/26 16:07:08, Miguel Garcia wrote:
> please document why month is special.

Done.

https://codereview.chromium.org/85643002/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java:221:
(double) ((hourOfDay * 60 + minute) * 60 + second) * 1000 + milli);
On 2013/11/26 16:07:08, Miguel Garcia wrote:
> Use TimeUnit(HOUR).toMillis() + TimeUnit.MINUTE(minute).toMillis +
> TimeUnit.SECOND(second).toMillis() + millis

Done.

https://codereview.chromium.org/85643002/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java:222:
}
On 2013/11/26 16:07:08, Miguel Garcia wrote:
> You are not setting mDialogAlreadyDismissed in these cases now since you don't
> call setFieldDateTimeValue. 
> 
>  I recommend you always call setFieldDateTimeValue and modify that methods so
it
> takes a dialog type. Then you can generate the right double. If you make the
> method static and public and make it return the right double you can actually
> unittest it as well.

Done.

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

https://codereview.chromium.org/85643002/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/input/MonthPicker.java:48:
protected long valueFromDate(int year, int month) {
On 2013/11/26 16:07:08, Miguel Garcia wrote:
> shouldn't this return a double? That's what you are using all the time..

Done. Changed to double.

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

https://codereview.chromium.org/85643002/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/input/WeekPicker.java:53:
protected long valueFromDate(int year, int week) {
On 2013/11/26 16:07:08, Miguel Garcia wrote:
> another double vs long here

Done.

https://codereview.chromium.org/85643002/diff/1/content/renderer/render_view_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/85643002/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:6381: if (date_time_picker_client_)
On 2013/11/26 16:07:08, Miguel Garcia wrote:
> Please add a comment stating when can this happen, I can't see how you can
have
> more than one dialog open since they are full screen. Specially within the
same
> renderer.

Done.
JavaScript runs while the dialog is open, and JavaScript can open date/time
dialogs.

https://codereview.chromium.org/85643002/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:6388: void
RenderViewImpl::DidDismissDateTimeDialog() {
On 2013/11/26 16:07:08, Miguel Garcia wrote:
> This is such a simple implementation that perhaps you can inline it in the .h
> file. See my comment about the signature as well.

Done.

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

https://codereview.chromium.org/85643002/diff/1/content/renderer/renderer_dat...
content/renderer/renderer_date_time_picker.cc:72:
static_cast<RenderViewImpl*>(render_view())->DidDismissDateTimeDialog();
On 2013/11/26 16:07:08, Miguel Garcia wrote:
> The method signature is confusing. It seems it should be
> DismissDateTimeDialog();
> 
> I agree that if the dialog is dissmissed in WebKit the reference should also
be
> cleared in the renderer which it seems was not happening before.
> 
> In the browser we also hold a pointer to the dialog chooser 
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we...
> 
> Is that cleared somehow?

That seems to be leaking too. I've also found other leaks. I'll fix them in
another CL.

[keishi, 2013-11-27 07:07:03]

jam@ could you review the files below? Thanks!
content/renderer/date_time_formatter.cc
content/renderer/date_time_formatter.h
content/renderer/date_time_formatter_unittest.cc
content/renderer/render_view_impl.cc
content/renderer/render_view_impl.h
content/renderer/renderer_date_time_picker.cc
content/renderer/renderer_date_time_picker.h
content/browser/android/date_time_chooser_android.cc
content/browser/android/date_time_chooser_android.h
content/browser/web_contents/web_contents_impl.cc
content/content_renderer.gypi
content/content_tests.gypi

joth@ could you review these files? Thanks!
content/public/android/java/src/org/chromium/content/browser/input/DateTimeChooserAndroid.java
content/public/android/java/src/org/chromium/content/browser/input/DateTimePickerDialog.java
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java
content/public/android/java/src/org/chromium/content/browser/input/MonthPicker.java
content/public/android/java/src/org/chromium/content/browser/input/MonthPickerDialog.java
content/public/android/java/src/org/chromium/content/browser/input/TwoFieldDatePicker.java
content/public/android/java/src/org/chromium/content/browser/input/TwoFieldDatePickerDialog.java
content/public/android/java/src/org/chromium/content/browser/input/WeekPicker.java
content/public/android/java/src/org/chromium/content/browser/input/WeekPickerDialog.java
content/browser/android/date_time_chooser_android.cc
content/browser/android/date_time_chooser_android.h

kenrb@ could you review view_messages.h? Thanks!

jochen@ could you review web_contents_impl.cc? Thanks!

[jochen (gone - plz use gerrit), 2013-11-27 08:59:04]

https://codereview.chromium.org/85643002/diff/30001/content/browser/android/d...
File content/browser/android/date_time_chooser_android.h (right):

https://codereview.chromium.org/85643002/diff/30001/content/browser/android/d...
content/browser/android/date_time_chooser_android.h:29: int dialog_type,
why don't you pass in ui::TextInputType?

https://codereview.chromium.org/85643002/diff/30001/content/browser/android/d...
content/browser/android/date_time_chooser_android.h:30: double dialog_value,
dialog_value is not very descriptive. what about milliseconds_since_epoch?

[keishi, 2013-11-27 10:11:52]

https://codereview.chromium.org/85643002/diff/30001/content/browser/android/d...
File content/browser/android/date_time_chooser_android.h (right):

https://codereview.chromium.org/85643002/diff/30001/content/browser/android/d...
content/browser/android/date_time_chooser_android.h:29: int dialog_type,
On 2013/11/27 08:59:04, jochen wrote:
> why don't you pass in ui::TextInputType?

Done.

https://codereview.chromium.org/85643002/diff/30001/content/browser/android/d...
content/browser/android/date_time_chooser_android.h:30: double dialog_value,
On 2013/11/27 08:59:04, jochen wrote:
> dialog_value is not very descriptive. what about milliseconds_since_epoch?

dialog_value mean different things depending on dialog_type.
For TextInputTypeMonth it will be months since epoch, for TextInputTypeTime it
will be milliseconds since midnight.

The same thing was pointed out in a previous iteration of this patch here:
https://codereview.chromium.org/64913002/#msg3

[jochen (gone - plz use gerrit), 2013-11-27 10:23:54]

What is the value called in the spec?

Can you please also update the CL description and reference the tracking bug (or
file one if there is none)?

[Miguel Garcia, 2013-11-27 11:17:24]

Just a small concern with casting from double to long plus a request for the
extra test.

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

https://codereview.chromium.org/85643002/diff/1/content/common/view_messages....
content/common/view_messages.h:969: double /* dialog_value */)
Ok, would be nice to know what happens if we actually pass a value that is not
within those boundaries, can we get an overflow? or is blink taking care of
that?

On 2013/11/27 06:58:37, keishi1 wrote:
> On 2013/11/26 16:07:08, Miguel Garcia wrote:
> > so if you are passing milliseconds since epoch why are you using a double
> > (floating point) instead of simply an int64? I guess the same applies for
min
> > and max.
> > 
> 
> The HTML spec defines min/max for the date/time values. dialog_value will
always
> be between -62135596800000.0 and 8640000000000000.0 so we can use double
without
> loss in precision. We can also express an empty value as NaN.

https://codereview.chromium.org/85643002/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/85643002/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java:95:
if (Double.isNaN(dialogValue)) {
Here is an example you can base the test on

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav...

I think it'd be important to test the edge cases (max value, min value...) this
way.




On 2013/11/27 06:58:37, keishi1 wrote:
> On 2013/11/26 16:07:08, Miguel Garcia wrote:
> > I would really like to see a test that checks that for a given dialog type
and
> > dialog value the right YEAR MONTH DAY HOUR... combination is generated. We
> used
> > to a similar thing in date_time_formatter_unittest.cc but this has been
> removed
> > now.
> 
> I separated out the double to year/month/date... conversion so we can unit
test
> it. I couldn't find examples of Java unit tests so I'm not sure how to write
> one.

https://codereview.chromium.org/85643002/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java:96:
cal = Calendar.getInstance();
Nevermind this works out of the box as you say

On 2013/11/27 06:58:37, keishi1 wrote:
> On 2013/11/26 16:07:08, Miguel Garcia wrote:
> > How do you pass Nan via JNI BTW?
> 
> I'm not sure what you mean. NaN is passed from C++ to Java using double.

https://codereview.chromium.org/85643002/diff/50001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/input/DateTimePickerDialog.java
(right):

https://codereview.chromium.org/85643002/diff/50001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/DateTimePickerDialog.java:65:

This is fishy right? you mention that the max timestamp can be 8640000000000000
whereas a signed java long in 32 bits (which is what we have in android) will
only hold 2147483647

[keishi, 2013-11-28 01:45:58]

On 2013/11/27 10:23:54, jochen wrote:
> What is the value called in the spec?

There is no name for it in the spec.
 It is defined as "value of the element, interpreted as a number"
http://www.whatwg.org/specs/web-apps/current-work/multipage/common-input-elem...
And the conversion is referred as "algorithm to convert a string to a number".
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-input-element...

> Can you please also update the CL description and reference the tracking bug
(or
> file one if there is none)?

Done.

[kenrb, 2013-11-28 02:43:24]

lgtm

[keishi, 2013-11-28 16:27:26]

https://codereview.chromium.org/85643002/diff/50001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/input/DateTimePickerDialog.java
(right):

https://codereview.chromium.org/85643002/diff/50001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/DateTimePickerDialog.java:65:

On 2013/11/27 11:17:24, Miguel Garcia wrote:
> This is fishy right? you mention that the max timestamp can be
8640000000000000
> whereas a signed java long in 32 bits (which is what we have in android) will
> only hold 2147483647

I've confirmed by inserting bit count code here but long seems to be 64 bits and
holds up to 9223372036854775807.

[keishi, 2013-11-29 03:30:13]

miguelg@ joth@ jam@ jochen@ please take a look again. Thanks!

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

https://codereview.chromium.org/85643002/diff/1/content/common/view_messages....
content/common/view_messages.h:969: double /* dialog_value */)
On 2013/11/27 11:17:24, Miguel Garcia wrote:
> Ok, would be nice to know what happens if we actually pass a value that is not
> within those boundaries, can we get an overflow? or is blink taking care of
> that?
> 
> On 2013/11/27 06:58:37, keishi1 wrote:
> > On 2013/11/26 16:07:08, Miguel Garcia wrote:
> > > so if you are passing milliseconds since epoch why are you using a double
> > > (floating point) instead of simply an int64? I guess the same applies for
> min
> > > and max.
> > > 
> > 
> > The HTML spec defines min/max for the date/time values. dialog_value will
> always
> > be between -62135596800000.0 and 8640000000000000.0 so we can use double
> without
> > loss in precision. We can also express an empty value as NaN.
> 

Blink only send valid values to chromium so the value should always be in this
range.

https://codereview.chromium.org/85643002/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/85643002/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java:95:
if (Double.isNaN(dialogValue)) {
On 2013/11/27 11:17:24, Miguel Garcia wrote:
> Here is an example you can base the test on
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav...
> 
> I think it'd be important to test the edge cases (max value, min value...)
this
> way.
> 
> 
> 
> 
> On 2013/11/27 06:58:37, keishi1 wrote:
> > On 2013/11/26 16:07:08, Miguel Garcia wrote:
> > > I would really like to see a test that checks that for a given dialog type
> and
> > > dialog value the right YEAR MONTH DAY HOUR... combination is generated. We
> > used
> > > to a similar thing in date_time_formatter_unittest.cc but this has been
> > removed
> > > now.
> > 
> > I separated out the double to year/month/date... conversion so we can unit
> test
> > it. I couldn't find examples of Java unit tests so I'm not sure how to write
> > one.
> 

Thanks I was able to add a test (InputDialogContainerTest.java).

[Miguel Garcia, 2013-11-29 11:20:25]

lgtm

Thanks for all the changes and the test!!

[joth, 2013-11-29 18:56:39]

lgtm

https://codereview.chromium.org/85643002/diff/280001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/input/DateTimePickerDialog.java
(right):

https://codereview.chromium.org/85643002/diff/280001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/DateTimePickerDialog.java:15:
import android.util.Log;
Nit: remove

[jochen (gone - plz use gerrit), 2013-12-02 10:35:22]

https://codereview.chromium.org/85643002/diff/280001/content/renderer/render_...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/85643002/diff/280001/content/renderer/render_...
content/renderer/render_view_impl.h:269: void DismissDateTimeDialog() {
please move body to .cc file

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

https://codereview.chromium.org/85643002/diff/280001/content/renderer/rendere...
content/renderer/renderer_date_time_picker.cc:19: static ui::TextInputType
toTextInputType(int type) {
us COMPILE_ASSERT() that the enum values are equal and just static_cast them

See e.g. render_widget.cc for an example

[keishi, 2013-12-02 11:25:56]

https://codereview.chromium.org/85643002/diff/280001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/input/DateTimePickerDialog.java
(right):

https://codereview.chromium.org/85643002/diff/280001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/DateTimePickerDialog.java:15:
import android.util.Log;
On 2013/11/29 18:56:40, joth wrote:
> Nit: remove

Done.

https://codereview.chromium.org/85643002/diff/280001/content/renderer/render_...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/85643002/diff/280001/content/renderer/render_...
content/renderer/render_view_impl.h:269: void DismissDateTimeDialog() {
On 2013/12/02 10:35:22, jochen wrote:
> please move body to .cc file

Done.

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

https://codereview.chromium.org/85643002/diff/280001/content/renderer/rendere...
content/renderer/renderer_date_time_picker.cc:19: static ui::TextInputType
toTextInputType(int type) {
On 2013/12/02 10:35:22, jochen wrote:
> us COMPILE_ASSERT() that the enum values are equal and just static_cast them
> 
> See e.g. render_widget.cc for an example

Done.

[jochen (gone - plz use gerrit), 2013-12-03 08:44:48]

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

https://codereview.chromium.org/85643002/diff/320001/content/renderer/rendere...
content/renderer/renderer_date_time_picker.cc:58: message.dialog_type =
ToTextInputType(chooser_params_.type);
with the compile asserts, you can now just use dialog_type =
static_cast<ui::TextInputType>(chooser_params_.type)

If the enum value is out of bounds, the IPC system should kill the renderer.

[jochen (gone - plz use gerrit), 2013-12-03 08:46:08]

lgtm with that change

[keishi, 2013-12-03 09:14:57]

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

https://codereview.chromium.org/85643002/diff/320001/content/renderer/rendere...
content/renderer/renderer_date_time_picker.cc:58: message.dialog_type =
ToTextInputType(chooser_params_.type);
On 2013/12/03 08:44:48, jochen wrote:
> with the compile asserts, you can now just use dialog_type =
> static_cast<ui::TextInputType>(chooser_params_.type)
> 
> If the enum value is out of bounds, the IPC system should kill the renderer.

Done.

[Miguel Garcia, 2013-12-03 15:14:22]

lgtm again module Marcus's comments

[jam, 2013-12-03 23:47:11]

lgtm

[commit-bot: I haz the power, 2013-12-04 01:07:06]

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

[commit-bot: I haz the power, 2013-12-04 01:07:17]

Failed to apply patch for content/renderer/render_view_impl.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/renderer/render_view_impl.h
  Hunk #2 FAILED at 153.
  Hunk #3 succeeded at 273 (offset 7 lines).
  1 out of 3 hunks FAILED -- saving rejects to file
content/renderer/render_view_impl.h.rej

Patch:       content/renderer/render_view_impl.h
Index: content/renderer/render_view_impl.h
diff --git a/content/renderer/render_view_impl.h
b/content/renderer/render_view_impl.h
index
5f3e8912048421134259e0f6d3ef01ee53707e49..da5cbc139ffbfc620a658292f55fa81b15cc8154
100644
--- a/content/renderer/render_view_impl.h
+++ b/content/renderer/render_view_impl.h
@@ -39,6 +39,7 @@
 #include "content/renderer/mouse_lock_dispatcher.h"
 #include "content/renderer/render_frame_impl.h"
 #include "content/renderer/render_widget.h"
+#include "content/renderer/renderer_date_time_picker.h"
 #include "content/renderer/renderer_webcookiejar_impl.h"
 #include "content/renderer/stats_collection_observer.h"
 #include "ipc/ipc_platform_file.h"
@@ -152,7 +153,6 @@ class PepperPluginInstanceImpl;
 class RenderViewObserver;
 class RenderViewTest;
 class RendererAccessibility;
-class RendererDateTimePicker;
 class RendererPpapiHost;
 class RendererWebColorChooserImpl;
 class RenderWidgetFullscreenPepper;
@@ -265,6 +265,10 @@ class CONTENT_EXPORT RenderViewImpl
       const std::string& html,
       bool replace);
 
+#if defined(OS_ANDROID)
+  void DismissDateTimeDialog();
+#endif
+
   // Plugin-related functions
--------------------------------------------------
 
 #if defined(ENABLE_PLUGINS)

[commit-bot: I haz the power, 2013-12-04 02:36:19]

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

[commit-bot: I haz the power, 2013-12-04 03:03:07]

Retried try job too often on linux_rel for step(s) base_unittests,
browser_tests, interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-12-04 03:55:52]

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

[commit-bot: I haz the power, 2013-12-04 06:04:16]

Change committed as 238594