[Android] Implement min/max on all supported date types

- pass the min/max attribute from Blink into the browser
- use them in the different components
- minor cleanup of some of the elements while at it

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

[Miguel Garcia, 2013-05-21 15:06:26]

keishi1@ and bulach@ as general reviewers
palmer@ for the changes in the message
jam@ for the changes in web_contents

Thanks!

[Miguel Garcia, 2013-05-21 15:20:44]

Throughly tested manually on this
test page http://jsbin.com/ejudos/23


On 2013/05/21 15:06:26, Miguel Garcia wrote:
> keishi1@ and bulach@ as general reviewers
> palmer@ for the changes in the message
> jam@ for the changes in web_contents
> 
> Thanks!

[bulach, 2013-05-21 17:26:35]

just a quick drive-be, won't have time to go into much details today..

https://codereview.chromium.org/15533003/diff/5001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/input/DateDialogNormalizer.java
(right):

https://codereview.chromium.org/15533003/diff/5001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/input/DateDialogNormalizer.java:3:
// found in the LICENSE file.
nit: add a \n

https://codereview.chromium.org/15533003/diff/5001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/input/DateDialogNormalizer.java:14:
public class DateDialogNormalizer {
nit: it looks like it can be package-private, ditto for the methods

https://codereview.chromium.org/15533003/diff/5001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/input/DateDialogNormalizer.java:15:
static long ONE_DAY_MILLIS = 3600 * 24 * 1000;
nit: final, and also I think it'd be clearer in hours * minutes * seconds *
millis, i.e., 24 * 60 * 60 * 1000;

https://codereview.chromium.org/15533003/diff/5001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/input/DateDialogNormalizer.java:19:
long minTime = min > 0 ? min -1 : 0;
nit: space after -

https://codereview.chromium.org/15533003/diff/5001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/input/DateDialogNormalizer.java:27:
* Create a verified DateDialogPicker
I don't think it "creates" a dialog, but rather, ensures DateDialogPicker is
within the boundaries.

https://codereview.chromium.org/15533003/diff/5001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/input/DateDialogNormalizer.java:29:
public static void normalize(DatePicker picker, OnDateChangedListener callBack,
nit: s/callBack/listener/

https://codereview.chromium.org/15533003/diff/5001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/input/DateTimeChooserAndroid.java
(right):

https://codereview.chromium.org/15533003/diff/5001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/input/DateTimeChooserAndroid.java:54:
int year, int month, int day    ,
nit: remove spaces after day

https://codereview.chromium.org/15533003/diff/5001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/input/DateTimePickerDialog.java
(right):

https://codereview.chromium.org/15533003/diff/5001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/input/DateTimePickerDialog.java:38:
private final Time mMaxTime;
these seem to be only ever used in the millis form, perhaps use that directly?

https://codereview.chromium.org/15533003/diff/5001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/input/DateTimePickerDialog.java:95:
mDatePicker.init(year, monthOfYear, dayOfMonth, this);
init is being called twice..

https://codereview.chromium.org/15533003/diff/5001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/input/DateTimePickerDialog.java:146:
time.set(0, mTimePicker.getCurrentMinute(),
should this use hourOfDay and minute params?

https://codereview.chromium.org/15533003/diff/5001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java
(right):

https://codereview.chromium.org/15533003/diff/5001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java:120:
DateDialogNormalizer.normalize(dialog.getDatePicker(), dialog,
could this be done inside DatePickerDialog constructor itself? I'm not quite
sure I understand why it's done for DateTimePickerDialog..

https://codereview.chromium.org/15533003/diff/5001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/input/MonthPicker.java
(right):

https://codereview.chromium.org/15533003/diff/5001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/input/MonthPicker.java:29:
private static final int MONTHS_NUMBER = 12;
is it true for all calendars? I thought the idea here is that the number of
months would be locale dependent... which also brings the question, is the param
coming from blink guaranteed to be in a 12-months calendar?

https://codereview.chromium.org/15533003/diff/5001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/input/MonthPicker.java:86:
} else if (oldVal == 0 && newVal == 11) {
I don't fully appreciate the logic here, but it seems this is also broken for
different calendars..

https://codereview.chromium.org/15533003/diff/5001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/input/MonthPicker.java:106:
mMonthSpinner.setDisplayedValues(mShortMonths);
so here, mShortMonths could potentially be different from MONTHS_NUMBER..

https://codereview.chromium.org/15533003/diff/5001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/input/MonthPicker.java:146:
}
nit: perhaps
int year = (int)Math.min(months / 12 + 1970, Integer.MAX_VALUE);

but as above, need to double check the months we received is not localized..
perhaps worth adding a comment.

https://codereview.chromium.org/15533003/diff/5001/content/renderer/renderer_...
File content/renderer/renderer_date_time_picker.cc (right):

https://codereview.chromium.org/15533003/diff/5001/content/renderer/renderer_...
content/renderer/renderer_date_time_picker.cc:42: if(chooser_params_.minimum >
0)
nit: space after if

[palmer, 2013-05-21 18:55:17]

IPC security LGTM

[Miguel Garcia, 2013-05-23 12:16:48]

Thanks for the review Marcus!

https://chromiumcodereview.appspot.com/15533003/diff/5001/content/public/andr...
File
content/public/android/java/src/org/chromium/content/browser/input/DateDialogNormalizer.java
(right):

https://chromiumcodereview.appspot.com/15533003/diff/5001/content/public/andr...
content/public/android/java/src/org/chromium/content/browser/input/DateDialogNormalizer.java:3:
// found in the LICENSE file.
On 2013/05/21 17:26:35, bulach wrote:
> nit: add a \n

Done.

https://chromiumcodereview.appspot.com/15533003/diff/5001/content/public/andr...
content/public/android/java/src/org/chromium/content/browser/input/DateDialogNormalizer.java:14:
public class DateDialogNormalizer {
On 2013/05/21 17:26:35, bulach wrote:
> nit: it looks like it can be package-private, ditto for the methods

Done.

https://chromiumcodereview.appspot.com/15533003/diff/5001/content/public/andr...
content/public/android/java/src/org/chromium/content/browser/input/DateDialogNormalizer.java:15:
static long ONE_DAY_MILLIS = 3600 * 24 * 1000;
On 2013/05/21 17:26:35, bulach wrote:
> nit: final, and also I think it'd be clearer in hours * minutes * seconds *
> millis, i.e., 24 * 60 * 60 * 1000;

Done.

https://chromiumcodereview.appspot.com/15533003/diff/5001/content/public/andr...
content/public/android/java/src/org/chromium/content/browser/input/DateDialogNormalizer.java:19:
long minTime = min > 0 ? min -1 : 0;
On 2013/05/21 17:26:35, bulach wrote:
> nit: space after -

Done.

https://chromiumcodereview.appspot.com/15533003/diff/5001/content/public/andr...
content/public/android/java/src/org/chromium/content/browser/input/DateDialogNormalizer.java:27:
* Create a verified DateDialogPicker
On 2013/05/21 17:26:35, bulach wrote:
> I don't think it "creates" a dialog, but rather, ensures DateDialogPicker is
> within the boundaries.

Done.

https://chromiumcodereview.appspot.com/15533003/diff/5001/content/public/andr...
content/public/android/java/src/org/chromium/content/browser/input/DateDialogNormalizer.java:29:
public static void normalize(DatePicker picker, OnDateChangedListener callBack,
On 2013/05/21 17:26:35, bulach wrote:
> nit: s/callBack/listener/

Done.

https://chromiumcodereview.appspot.com/15533003/diff/5001/content/public/andr...
File
content/public/android/java/src/org/chromium/content/browser/input/DateTimeChooserAndroid.java
(right):

https://chromiumcodereview.appspot.com/15533003/diff/5001/content/public/andr...
content/public/android/java/src/org/chromium/content/browser/input/DateTimeChooserAndroid.java:54:
int year, int month, int day    ,
On 2013/05/21 17:26:35, bulach wrote:
> nit: remove spaces after day

Done.

https://chromiumcodereview.appspot.com/15533003/diff/5001/content/public/andr...
File
content/public/android/java/src/org/chromium/content/browser/input/DateTimePickerDialog.java
(right):

https://chromiumcodereview.appspot.com/15533003/diff/5001/content/public/andr...
content/public/android/java/src/org/chromium/content/browser/input/DateTimePickerDialog.java:38:
private final Time mMaxTime;
On 2013/05/21 17:26:35, bulach wrote:
> these seem to be only ever used in the millis form, perhaps use that directly?

Done.

https://chromiumcodereview.appspot.com/15533003/diff/5001/content/public/andr...
File
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java
(right):

https://chromiumcodereview.appspot.com/15533003/diff/5001/content/public/andr...
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java:120:
DateDialogNormalizer.normalize(dialog.getDatePicker(), dialog,
On 2013/05/21 17:26:35, bulach wrote:
> could this be done inside DatePickerDialog constructor itself? I'm not quite
> sure I understand why it's done for DateTimePickerDialog..

Well DatePickerDialog is an Android system class, I'd really love to be able to
modify that constructor :)

I could add a wrapper on top but then it would complicate the our
AndroidDateTimePicker class since it loads the component from xml directly.

https://chromiumcodereview.appspot.com/15533003/diff/5001/content/public/andr...
File
content/public/android/java/src/org/chromium/content/browser/input/MonthPicker.java
(right):

https://chromiumcodereview.appspot.com/15533003/diff/5001/content/public/andr...
content/public/android/java/src/org/chromium/content/browser/input/MonthPicker.java:29:
private static final int MONTHS_NUMBER = 12;
I think what is locale dependendent is only the short name of the months. The
spec does not support  chinese calendars and or other variations.


From http://www.w3.org/TR/html-markup/input.month.html

value = month
A string representing a month.
The following parts, in exactly the following order:
Four or more digits representing a number greater than 0.
The literal string "-".
Two digits, representing the month month, in the range 1 ≤ month, ≤ 12.


On 2013/05/21 17:26:35, bulach wrote:
> is it true for all calendars? I thought the idea here is that the number of
> months would be locale dependent... which also brings the question, is the
param
> coming from blink guaranteed to be in a 12-months calendar?

https://chromiumcodereview.appspot.com/15533003/diff/5001/content/public/andr...
content/public/android/java/src/org/chromium/content/browser/input/MonthPicker.java:86:
} else if (oldVal == 0 && newVal == 11) {
I think this had in mind a much more complex spec than what is really accepted,
see above.

On 2013/05/21 17:26:35, bulach wrote:
> I don't fully appreciate the logic here, but it seems this is also broken for
> different calendars..

https://chromiumcodereview.appspot.com/15533003/diff/5001/content/public/andr...
content/public/android/java/src/org/chromium/content/browser/input/MonthPicker.java:106:
mMonthSpinner.setDisplayedValues(mShortMonths);
On 2013/05/21 17:26:35, bulach wrote:
> so here, mShortMonths could potentially be different from MONTHS_NUMBER..

Done.

https://chromiumcodereview.appspot.com/15533003/diff/5001/content/public/andr...
content/public/android/java/src/org/chromium/content/browser/input/MonthPicker.java:146:
}
On 2013/05/21 17:26:35, bulach wrote:
> nit: perhaps
> int year = (int)Math.min(months / 12 + 1970, Integer.MAX_VALUE);
> 
> but as above, need to double check the months we received is not localized..
> perhaps worth adding a comment.

Great simplification, thanks!
See above about the different calendars and localization

https://chromiumcodereview.appspot.com/15533003/diff/5001/content/renderer/re...
File content/renderer/renderer_date_time_picker.cc (right):

https://chromiumcodereview.appspot.com/15533003/diff/5001/content/renderer/re...
content/renderer/renderer_date_time_picker.cc:42: if(chooser_params_.minimum >
0)
On 2013/05/21 17:26:35, bulach wrote:
> nit: space after if

Done.

[bulach, 2013-05-24 09:23:22]

thanks miguel! I think the issues here are bridging the locale-agnostic with the
locale-specific APIs, I guess there are a few places where this is being mixed
up below.. please let me know your thoughts:

https://codereview.chromium.org/15533003/diff/17001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/input/DateDialogNormalizer.java
(right):

https://codereview.chromium.org/15533003/diff/17001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/DateDialogNormalizer.java:16:
static final long ONE_DAY_MILLIS = 24 * 60 * 60 * 1000;
private?

https://codereview.chromium.org/15533003/diff/17001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/DateDialogNormalizer.java:19:
// DatePicker intervals are non inclusive
nit: end with .

https://codereview.chromium.org/15533003/diff/17001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/DateDialogNormalizer.java:22:
? Long.MAX_VALUE : max + ONE_DAY_MILLIS;
nit: move ? to the previous line

https://codereview.chromium.org/15533003/diff/17001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/input/MonthPicker.java
(left):

https://codereview.chromium.org/15533003/diff/17001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/MonthPicker.java:315:
protected Parcelable onSaveInstanceState() {
why is this being removed? iirc, this was necessary for when the dialog goes to
the background, is rotated, etc..

https://codereview.chromium.org/15533003/diff/17001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/input/MonthPicker.java
(right):

https://codereview.chromium.org/15533003/diff/17001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/MonthPicker.java:105:
mMonthSpinner.setMaxValue(MONTHS_NUMBER - 1);
afaict, mShortMonths is calendar-dependent, right? so it does not necessarily
contain MONTHS_NUMBER, which is what I think it was before..
also, 84 / 86 should probably check mShortMonths.lenght rather than hardcode 11
to bump "year", or even better, use Calendar.roll?

so from the comment in the previous patch, whilst the web-api is
locale-agnostic, and afaict the UI and java Calendar is not, so we need to adapt
them both here..

https://codereview.chromium.org/15533003/diff/17001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/MonthPicker.java:129:
if (mCurrentDate.getTimeInMillis() < mMinDate.getTimeInMillis()) {
maybe something like:

if (mCurrentDate.compareTo(mMinDate) < 0) {
  mCurrentDate = mMinDate.clone();
} else if (mCurrentDate.compareTo(mMaxDate) > 0) {
  mCurrentDate = mMaxDate.clone();
}

https://codereview.chromium.org/15533003/diff/17001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/MonthPicker.java:143:
Calendar cal = Calendar.getInstance();
again, I think this would return the default locale, which may not be 12-months
based..

[jam, 2013-05-28 17:59:48]

On 2013/05/21 15:06:26, Miguel Garcia wrote:
> keishi1@ and bulach@ as general reviewers
> palmer@ for the changes in the message
> jam@ for the changes in web_contents

lgtm

[Miguel Garcia, 2013-05-30 10:06:30]

All comments addressed, PTAL

I also fixed two minor bugs as a result or more in depth testing. I also
uploaded a test page to http://chromiumexperiments.appspot.com/static/forms.html
that contains all the test cases I tried.

https://codereview.chromium.org/15533003/diff/5001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/input/DateTimePickerDialog.java
(right):

https://codereview.chromium.org/15533003/diff/5001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/input/DateTimePickerDialog.java:95:
mDatePicker.init(year, monthOfYear, dayOfMonth, this);
On 2013/05/21 17:26:35, bulach wrote:
> init is being called twice..

True. Removed one call now.

https://codereview.chromium.org/15533003/diff/5001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/input/DateTimePickerDialog.java:146:
time.set(0, mTimePicker.getCurrentMinute(),
On 2013/05/21 17:26:35, bulach wrote:
> should this use hourOfDay and minute params?

It seems more consistent to get everything from the pickers right?

https://codereview.chromium.org/15533003/diff/17001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/input/DateDialogNormalizer.java
(right):

https://codereview.chromium.org/15533003/diff/17001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/DateDialogNormalizer.java:16:
static final long ONE_DAY_MILLIS = 24 * 60 * 60 * 1000;
On 2013/05/24 09:23:22, bulach wrote:
> private?

Done.

https://codereview.chromium.org/15533003/diff/17001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/DateDialogNormalizer.java:19:
// DatePicker intervals are non inclusive
On 2013/05/24 09:23:22, bulach wrote:
> nit: end with .

Done.

https://codereview.chromium.org/15533003/diff/17001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/DateDialogNormalizer.java:22:
? Long.MAX_VALUE : max + ONE_DAY_MILLIS;
On 2013/05/24 09:23:22, bulach wrote:
> nit: move ? to the previous line

Done.

https://codereview.chromium.org/15533003/diff/17001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/input/MonthPicker.java
(left):

https://codereview.chromium.org/15533003/diff/17001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/MonthPicker.java:315:
protected Parcelable onSaveInstanceState() {
On 2013/05/24 09:23:22, bulach wrote:
> why is this being removed? iirc, this was necessary for when the dialog goes
to
> the background, is rotated, etc..

Solved in a separate CL

https://codereview.chromium.org/15533003/diff/17001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/input/MonthPicker.java
(right):

https://codereview.chromium.org/15533003/diff/17001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/MonthPicker.java:105:
mMonthSpinner.setMaxValue(MONTHS_NUMBER - 1);
Solved in a separate CL now.

On 2013/05/24 09:23:22, bulach wrote:
> afaict, mShortMonths is calendar-dependent, right? so it does not necessarily
> contain MONTHS_NUMBER, which is what I think it was before..
> also, 84 / 86 should probably check mShortMonths.lenght rather than hardcode
11
> to bump "year", or even better, use Calendar.roll?
> 
> so from the comment in the previous patch, whilst the web-api is
> locale-agnostic, and afaict the UI and java Calendar is not, so we need to
adapt
> them both here..

https://codereview.chromium.org/15533003/diff/17001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/MonthPicker.java:143:
Calendar cal = Calendar.getInstance();
On 2013/05/24 09:23:22, bulach wrote:
> again, I think this would return the default locale, which may not be
12-months
> based..
Solved in a different CL

[bulach, 2013-05-30 14:16:30]

lgtm, just some nits and a few small comments from the previous patchset..

https://codereview.chromium.org/15533003/diff/29001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/input/DateDialogNormalizer.java
(right):

https://codereview.chromium.org/15533003/diff/29001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/DateDialogNormalizer.java:19:
// DatePicker intervals are non inclusive
nit: end with .

https://codereview.chromium.org/15533003/diff/29001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/DateDialogNormalizer.java:24:
// set max/min Date accepts a time so while the widget is only able to
this sentence is a bit confusing, how about:
// While the widget is only able to display date, min/max can also contain time
information.
// Trim min/max to date before adjusting the picker.

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

https://codereview.chromium.org/15533003/diff/29001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/DateTimePickerDialog.java:132:

nit: remove extra \n

https://codereview.chromium.org/15533003/diff/29001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/input/MonthPicker.java
(right):

https://codereview.chromium.org/15533003/diff/29001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/MonthPicker.java:126:
if (mCurrentDate.getTimeInMillis() < mMinDate.getTimeInMillis()) {
maybe something like:

if (mCurrentDate.compareTo(mMinDate) < 0) {
  mCurrentDate = mMinDate.clone();
} else if (mCurrentDate.compareTo(mMaxDate) > 0) {
  mCurrentDate = mMaxDate.clone();
}

https://codereview.chromium.org/15533003/diff/29001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/input/TimeDialog.java
(right):

https://codereview.chromium.org/15533003/diff/29001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/TimeDialog.java:53:
this.mYear = year;
nit: s/this.//

https://codereview.chromium.org/15533003/diff/29001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/TimeDialog.java:67:
}
60-67 and 27-32 are the same, maybe have something like:

Time getBoundedTime(int year, int month, int day, int hour,
       int minute, long min, long max) {
  ...
}

https://codereview.chromium.org/15533003/diff/29001/content/renderer/renderer...
File content/renderer/renderer_date_time_picker.cc (right):

https://codereview.chromium.org/15533003/diff/29001/content/renderer/renderer...
content/renderer/renderer_date_time_picker.cc:42: if (chooser_params_.minimum >
0)
iirc, the ViewHostMsg_DateTimeDialogValue_Params members are not zero
initialized, so message.minimum / maximum would potentially end up with
undefined data in the other end.. so, is this if necessary?

[Miguel Garcia, 2013-06-03 13:08:57]

Thanks for the review! committing now.

https://codereview.chromium.org/15533003/diff/29001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/input/DateDialogNormalizer.java
(right):

https://codereview.chromium.org/15533003/diff/29001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/DateDialogNormalizer.java:19:
// DatePicker intervals are non inclusive
On 2013/05/30 14:16:30, bulach wrote:
> nit: end with .

Done.

https://codereview.chromium.org/15533003/diff/29001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/DateDialogNormalizer.java:24:
// set max/min Date accepts a time so while the widget is only able to
On 2013/05/30 14:16:30, bulach wrote:
> this sentence is a bit confusing, how about:
> // While the widget is only able to display date, min/max can also contain
time
> information.
> // Trim min/max to date before adjusting the picker.

Done.

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

https://codereview.chromium.org/15533003/diff/29001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/DateTimePickerDialog.java:132:

On 2013/05/30 14:16:30, bulach wrote:
> nit: remove extra \n

Done.

https://codereview.chromium.org/15533003/diff/29001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/input/MonthPicker.java
(right):

https://codereview.chromium.org/15533003/diff/29001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/MonthPicker.java:126:
if (mCurrentDate.getTimeInMillis() < mMinDate.getTimeInMillis()) {
Changed to clone.

About the comparison. Is there any advantage to that than simply checking the
millis since epoch?

On 2013/05/30 14:16:30, bulach wrote:
> maybe something like:
> 
> if (mCurrentDate.compareTo(mMinDate) < 0) {
>   mCurrentDate = mMinDate.clone();
> } else if (mCurrentDate.compareTo(mMaxDate) > 0) {
>   mCurrentDate = mMaxDate.clone();
> }

https://codereview.chromium.org/15533003/diff/29001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/input/TimeDialog.java
(right):

https://codereview.chromium.org/15533003/diff/29001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/TimeDialog.java:53:
this.mYear = year;
On 2013/05/30 14:16:30, bulach wrote:
> nit: s/this.//

Done.

https://codereview.chromium.org/15533003/diff/29001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/TimeDialog.java:67:
}
Good idea! done, also made the private method static

On 2013/05/30 14:16:30, bulach wrote:
> 60-67 and 27-32 are the same, maybe have something like:
> 
> Time getBoundedTime(int year, int month, int day, int hour,
>        int minute, long min, long max) {
>   ...
> }

https://codereview.chromium.org/15533003/diff/29001/content/renderer/renderer...
File content/renderer/renderer_date_time_picker.cc (right):

https://codereview.chromium.org/15533003/diff/29001/content/renderer/renderer...
content/renderer/renderer_date_time_picker.cc:42: if (chooser_params_.minimum >
0)
Good point, let's better be safe than sorry here.

On 2013/05/30 14:16:30, bulach wrote:
> iirc, the ViewHostMsg_DateTimeDialogValue_Params members are not zero
> initialized, so message.minimum / maximum would potentially end up with
> undefined data in the other end.. so, is this if necessary?

[commit-bot: I haz the power, 2013-06-03 13:09:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miguelg@chromium.org/15533003/39001

[commit-bot: I haz the power, 2013-06-03 20:21:36]

The commit queue went berserk retrying too often for a
seemingly flaky test on builder android_dbg:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

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

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miguelg@chromium.org/15533003/39001

[commit-bot: I haz the power, 2013-06-04 14:54:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miguelg@chromium.org/15533003/39001

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

Change committed as 203975