Week picker for android

Week picker for android

This adds support for <input type=week> on Android.

ViewHostMsg_DateTimeDialogValue_Params: Adding members for week_year and week. Adding week_year separately from year to avoid confusion.
WeekPicker.getWeekYearForDate(): Calendar.get(Calendar.YEAR) returns the year for the day and not the ISO week year. This returns the ISO week year.

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

[Miguel Garcia, 2013-05-10 15:05:24]

On 2013/05/09 09:45:07, keishi1 wrote:

I had an initial look and it looks pretty good! Can you explain what's the
difference between the year of the day  and the ISO week year? They seem the
same to me and having to pass two new parameters looks like a bit of an
overkill.

Some more nits coming

[Miguel Garcia, 2013-05-10 15:17:59]

https://chromiumcodereview.appspot.com/15057004/diff/4001/content/common/view...
File content/common/view_messages.h (right):

https://chromiumcodereview.appspot.com/15057004/diff/4001/content/common/view...
content/common/view_messages.h:363: IPC_STRUCT_MEMBER(int, week_year)
if we really need these two ints let's call them iso_week_year and let's rename
year to something more concrete (like iso_year?) otherwise it's really hard to
understand what's the difference (in fact I don't really know what the
difference is yet :) )

https://chromiumcodereview.appspot.com/15057004/diff/4001/content/public/andr...
File
content/public/android/java/src/org/chromium/content/browser/input/WeekPicker.java
(right):

https://chromiumcodereview.appspot.com/15057004/diff/4001/content/public/andr...
content/public/android/java/src/org/chromium/content/browser/input/WeekPicker.java:30:
public class WeekPicker extends FrameLayout {
Would it make sense to do some refactor here for the common functionality and
have MonthPicker and DatePicker extend from the same base class?

https://chromiumcodereview.appspot.com/15057004/diff/4001/content/public/andr...
File
content/public/android/java/src/org/chromium/content/browser/input/WeekPickerDialog.java
(right):

https://chromiumcodereview.appspot.com/15057004/diff/4001/content/public/andr...
content/public/android/java/src/org/chromium/content/browser/input/WeekPickerDialog.java:18:
public class WeekPickerDialog extends AlertDialog implements OnClickListener,
Same here (even more I think) both dialogs are essentially the same thing (they
display a number and a year) I think it'd make sense to have a more general
class that can do both or perhaps a base class with two very small subclasses.

https://chromiumcodereview.appspot.com/15057004/diff/4001/content/renderer/da...
File content/renderer/date_time_formatter.cc (right):

https://chromiumcodereview.appspot.com/15057004/diff/4001/content/renderer/da...
content/renderer/date_time_formatter.cc:116: if (type_ ==
ui::TEXT_INPUT_TYPE_WEEK) {
you probably want to check that success is still true right?

https://chromiumcodereview.appspot.com/15057004/diff/4001/content/renderer/da...
content/renderer/date_time_formatter.cc:117:
calendar.setMinimalDaysInFirstWeek(4);
I think some comment on why this is needed would be important.

https://chromiumcodereview.appspot.com/15057004/diff/4001/content/renderer/da...
content/renderer/date_time_formatter.cc:118:
calendar.setFirstDayOfWeek(UCAL_MONDAY);
Doesn't this depend on the locale?

https://chromiumcodereview.appspot.com/15057004/diff/4001/content/renderer/da...
content/renderer/date_time_formatter.cc:121: } else {
it seems you can anyway set year, month... in all cases (they will be 0
sometimes) and in addition to that set setMinimalDaysInFirstWeek and
setFirstDayOfWeek only if input type is week. I still don't understand why you
need those two things and why could they not be setup in all cases.

[keishi, 2013-05-13 03:34:52]

message: On 2013/05/10 15:05:24, Miguel Garcia wrote:
> On 2013/05/09 09:45:07, keishi1 wrote:
> 
> I had an initial look and it looks pretty good! Can you explain what's the
> difference between the year of the day  and the ISO week year? They seem the
> same to me and having to pass two new parameters looks like a bit of an
> overkill.
> 
> Some more nits coming

Sorry, I should have explained.

An ISO week always starts from Monday(no matter what locale). And the first ISO
week of a year is defined as the week with the year's first Thursday in it.
There are no fractional weeks.
Hence the "ISO week-numbering year number" sometimes doesn't match the Gregorian
year number.
For example: 2010-01-01 is 2009-W53-5
https://docs.google.com/file/d/0BwRi59l_ri74OGw2c25jTTFlU0E/edit?usp=sharing

The icu::Calendar has UCAL_YEAR_WOY but the java.util.Calendar doesn't seem to
have an equivalent.

I added the "week_year" parameter everywhere to avoid mistakes. But the usage of
"week_year" and "year" parameters are mutually exclusive, so we can get rid of
the "week_year" parameter.

[keishi, 2013-05-14 13:47:47]

https://codereview.chromium.org/15057004/diff/4001/content/common/view_messag...
File content/common/view_messages.h (right):

https://codereview.chromium.org/15057004/diff/4001/content/common/view_messag...
content/common/view_messages.h:363: IPC_STRUCT_MEMBER(int, week_year)
On 2013/05/10 15:17:59, Miguel Garcia wrote:
> if we really need these two ints let's call them iso_week_year and let's
rename
> year to something more concrete (like iso_year?) otherwise it's really hard to
> understand what's the difference (in fact I don't really know what the
> difference is yet :) )
In this patch I removed week_year and reused the year field.

https://codereview.chromium.org/15057004/diff/4001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/input/WeekPicker.java
(right):

https://codereview.chromium.org/15057004/diff/4001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/input/WeekPicker.java:30:
public class WeekPicker extends FrameLayout {
On 2013/05/10 15:17:59, Miguel Garcia wrote:
> Would it make sense to do some refactor here for the common functionality and
> have MonthPicker and DatePicker extend from the same base class?

Done. Added BaseDatePicker.

https://codereview.chromium.org/15057004/diff/4001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/input/WeekPickerDialog.java
(right):

https://codereview.chromium.org/15057004/diff/4001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/input/WeekPickerDialog.java:18:
public class WeekPickerDialog extends AlertDialog implements OnClickListener,
On 2013/05/10 15:17:59, Miguel Garcia wrote:
> Same here (even more I think) both dialogs are essentially the same thing
(they
> display a number and a year) I think it'd make sense to have a more general
> class that can do both or perhaps a base class with two very small subclasses.

Most of the methods have a week/month parameter so I wasn't able to share much
code. I have left it separate classes in this patch.

https://codereview.chromium.org/15057004/diff/4001/content/renderer/date_time...
File content/renderer/date_time_formatter.cc (right):

https://codereview.chromium.org/15057004/diff/4001/content/renderer/date_time...
content/renderer/date_time_formatter.cc:116: if (type_ ==
ui::TEXT_INPUT_TYPE_WEEK) {
On 2013/05/10 15:17:59, Miguel Garcia wrote:
> you probably want to check that success is still true right?

Done.

https://codereview.chromium.org/15057004/diff/4001/content/renderer/date_time...
content/renderer/date_time_formatter.cc:117:
calendar.setMinimalDaysInFirstWeek(4);
On 2013/05/10 15:17:59, Miguel Garcia wrote:
> I think some comment on why this is needed would be important. 

Done.

https://codereview.chromium.org/15057004/diff/4001/content/renderer/date_time...
content/renderer/date_time_formatter.cc:118:
calendar.setFirstDayOfWeek(UCAL_MONDAY);
On 2013/05/10 15:17:59, Miguel Garcia wrote:
> Doesn't this depend on the locale?

ISO weeks always start from Monday regardless of locale.

https://codereview.chromium.org/15057004/diff/4001/content/renderer/date_time...
content/renderer/date_time_formatter.cc:121: } else {
On 2013/05/10 15:17:59, Miguel Garcia wrote:
> it seems you can anyway set year, month... in all cases (they will be 0
> sometimes) and in addition to that set setMinimalDaysInFirstWeek and
> setFirstDayOfWeek only if input type is week. I still don't understand why you
> need those two things and why could they not be setup in all cases.

I may not be understanding you correctly. Are you saying I should set all fields
without checking the type? Like this:

calendar.setMinimalDaysInFirstWeek(4);
calendar.setFirstDayOfWeek(UCAL_MONDAY);
calendar.set(UCAL_YEAR, year_);
calendar.set(UCAL_MONTH, month_);
calendar.set(UCAL_DATE, day_);
calendar.set(UCAL_HOUR_OF_DAY, hour_);
calendar.set(UCAL_MINUTE, minute_);
calendar.set(UCAL_SECOND, second_);
calendar.set(UCAL_YEAR_WOY, week_year_);
calendar.set(UCAL_WEEK_OF_YEAR, week_);

If there is an invalid value in even one of the fields calendar.getTime(success)
will fail, so it won't work.

[Miguel Garcia, 2013-05-17 09:21:58]

I don;t think getTime provides invalid values, it just provides 0s when a value
is not available.

On 2013/05/14 13:47:47, keishi1 wrote:
>
https://codereview.chromium.org/15057004/diff/4001/content/common/view_messag...
> File content/common/view_messages.h (right):
> 
>
https://codereview.chromium.org/15057004/diff/4001/content/common/view_messag...
> content/common/view_messages.h:363: IPC_STRUCT_MEMBER(int, week_year)
> On 2013/05/10 15:17:59, Miguel Garcia wrote:
> > if we really need these two ints let's call them iso_week_year and let's
> rename
> > year to something more concrete (like iso_year?) otherwise it's really hard
to
> > understand what's the difference (in fact I don't really know what the
> > difference is yet :) )
> In this patch I removed week_year and reused the year field.
> 
>
https://codereview.chromium.org/15057004/diff/4001/content/public/android/jav...
> File
>
content/public/android/java/src/org/chromium/content/browser/input/WeekPicker.java
> (right):
> 
>
https://codereview.chromium.org/15057004/diff/4001/content/public/android/jav...
>
content/public/android/java/src/org/chromium/content/browser/input/WeekPicker.java:30:
> public class WeekPicker extends FrameLayout {
> On 2013/05/10 15:17:59, Miguel Garcia wrote:
> > Would it make sense to do some refactor here for the common functionality
and
> > have MonthPicker and DatePicker extend from the same base class?
> 
> Done. Added BaseDatePicker.
> 
>
https://codereview.chromium.org/15057004/diff/4001/content/public/android/jav...
> File
>
content/public/android/java/src/org/chromium/content/browser/input/WeekPickerDialog.java
> (right):
> 
>
https://codereview.chromium.org/15057004/diff/4001/content/public/android/jav...
>
content/public/android/java/src/org/chromium/content/browser/input/WeekPickerDialog.java:18:
> public class WeekPickerDialog extends AlertDialog implements OnClickListener,
> On 2013/05/10 15:17:59, Miguel Garcia wrote:
> > Same here (even more I think) both dialogs are essentially the same thing
> (they
> > display a number and a year) I think it'd make sense to have a more general
> > class that can do both or perhaps a base class with two very small
subclasses.
> 
> Most of the methods have a week/month parameter so I wasn't able to share much
> code. I have left it separate classes in this patch.
> 
>
https://codereview.chromium.org/15057004/diff/4001/content/renderer/date_time...
> File content/renderer/date_time_formatter.cc (right):
> 
>
https://codereview.chromium.org/15057004/diff/4001/content/renderer/date_time...
> content/renderer/date_time_formatter.cc:116: if (type_ ==
> ui::TEXT_INPUT_TYPE_WEEK) {
> On 2013/05/10 15:17:59, Miguel Garcia wrote:
> > you probably want to check that success is still true right?
> 
> Done.
> 
>
https://codereview.chromium.org/15057004/diff/4001/content/renderer/date_time...
> content/renderer/date_time_formatter.cc:117:
> calendar.setMinimalDaysInFirstWeek(4);
> On 2013/05/10 15:17:59, Miguel Garcia wrote:
> > I think some comment on why this is needed would be important. 
> 
> Done.
> 
>
https://codereview.chromium.org/15057004/diff/4001/content/renderer/date_time...
> content/renderer/date_time_formatter.cc:118:
> calendar.setFirstDayOfWeek(UCAL_MONDAY);
> On 2013/05/10 15:17:59, Miguel Garcia wrote:
> > Doesn't this depend on the locale?
> 
> ISO weeks always start from Monday regardless of locale.
> 
>
https://codereview.chromium.org/15057004/diff/4001/content/renderer/date_time...
> content/renderer/date_time_formatter.cc:121: } else {
> On 2013/05/10 15:17:59, Miguel Garcia wrote:
> > it seems you can anyway set year, month... in all cases (they will be 0
> > sometimes) and in addition to that set setMinimalDaysInFirstWeek and
> > setFirstDayOfWeek only if input type is week. I still don't understand why
you
> > need those two things and why could they not be setup in all cases.
> 
> I may not be understanding you correctly. Are you saying I should set all
fields
> without checking the type? Like this:
> 
> calendar.setMinimalDaysInFirstWeek(4);
> calendar.setFirstDayOfWeek(UCAL_MONDAY);
> calendar.set(UCAL_YEAR, year_);
> calendar.set(UCAL_MONTH, month_);
> calendar.set(UCAL_DATE, day_);
> calendar.set(UCAL_HOUR_OF_DAY, hour_);
> calendar.set(UCAL_MINUTE, minute_);
> calendar.set(UCAL_SECOND, second_);
> calendar.set(UCAL_YEAR_WOY, week_year_);
> calendar.set(UCAL_WEEK_OF_YEAR, week_);
> 
> If there is an invalid value in even one of the fields
calendar.getTime(success)
> will fail, so it won't work.

[keishi, 2013-05-17 10:53:53]

On 2013/05/17 09:21:58, Miguel Garcia wrote:
> I don;t think getTime provides invalid values, it just provides 0s when a
value
> is not available.

I actually tried it and it turned out that calendar.getTime() doesn't fail but
returns the last week in year 1 BC.

This is the code I ran.

    calendar.setMinimalDaysInFirstWeek(4);
    calendar.setFirstDayOfWeek(UCAL_MONDAY);
    calendar.set(UCAL_YEAR, year_);
    calendar.set(UCAL_MONTH, month_);
    calendar.set(UCAL_DATE, day_);
    calendar.set(UCAL_HOUR_OF_DAY, hour_);
    calendar.set(UCAL_MINUTE, minute_);
    calendar.set(UCAL_SECOND, second_);
    calendar.set(UCAL_YEAR_WOY, week_year_);
    calendar.set(UCAL_WEEK_OF_YEAR, week_);
    UDate time = calendar.getTime(success);

    time == -62167943939000 // Thu, 23 Dec -1 14:41:01 GMT

[Miguel Garcia, 2013-05-17 11:23:38]

Looking pretty good, I have some comments but while we solve them let's start
adding owners I suggest bulach for the android specific piece and cpalmer for
the changes in the ipc messages. They have both reviewed code in this area
before

https://chromiumcodereview.appspot.com/15057004/diff/4001/content/common/view...
File content/common/view_messages.h (right):

https://chromiumcodereview.appspot.com/15057004/diff/4001/content/common/view...
content/common/view_messages.h:363: IPC_STRUCT_MEMBER(int, week_year)
Great, I think this makes more sense

On 2013/05/14 13:47:47, keishi1 wrote:
> On 2013/05/10 15:17:59, Miguel Garcia wrote:
> > if we really need these two ints let's call them iso_week_year and let's
> rename
> > year to something more concrete (like iso_year?) otherwise it's really hard
to
> > understand what's the difference (in fact I don't really know what the
> > difference is yet :) )
> In this patch I removed week_year and reused the year field.

https://chromiumcodereview.appspot.com/15057004/diff/4001/content/public/andr...
File
content/public/android/java/src/org/chromium/content/browser/input/WeekPickerDialog.java
(right):

https://chromiumcodereview.appspot.com/15057004/diff/4001/content/public/andr...
content/public/android/java/src/org/chromium/content/browser/input/WeekPickerDialog.java:18:
public class WeekPickerDialog extends AlertDialog implements OnClickListener,
I still don't understand what the problem is, just make those parameters generic
(like year and position_in_year) and you can practically reuse the same file.
In any case if you see it's not straight forward I don't feel too strongly about
it. so It's fine



On 2013/05/14 13:47:47, keishi1 wrote:
> On 2013/05/10 15:17:59, Miguel Garcia wrote:
> > Same here (even more I think) both dialogs are essentially the same thing
> (they
> > display a number and a year) I think it'd make sense to have a more general
> > class that can do both or perhaps a base class with two very small
subclasses.
> 
> Most of the methods have a week/month parameter so I wasn't able to share much
> code. I have left it separate classes in this patch.

https://chromiumcodereview.appspot.com/15057004/diff/4001/content/renderer/da...
File content/renderer/date_time_formatter.cc (right):

https://chromiumcodereview.appspot.com/15057004/diff/4001/content/renderer/da...
content/renderer/date_time_formatter.cc:121: } else {
I'm still hesitant to special case week here, can we simply check wether the
value returned by calendar.getTime() is > 0 and set it to 0 otherwise? The date
spec does not allow values below 0 for anything

http://www.w3.org/TR/html-markup/datatypes.html#form.data.date

On 2013/05/14 13:47:47, keishi1 wrote:
> On 2013/05/10 15:17:59, Miguel Garcia wrote:
> > it seems you can anyway set year, month... in all cases (they will be 0
> > sometimes) and in addition to that set setMinimalDaysInFirstWeek and
> > setFirstDayOfWeek only if input type is week. I still don't understand why
you
> > need those two things and why could they not be setup in all cases.
> 
> I may not be understanding you correctly. Are you saying I should set all
fields
> without checking the type? Like this:
> 
> calendar.setMinimalDaysInFirstWeek(4);
> calendar.setFirstDayOfWeek(UCAL_MONDAY);
> calendar.set(UCAL_YEAR, year_);
> calendar.set(UCAL_MONTH, month_);
> calendar.set(UCAL_DATE, day_);
> calendar.set(UCAL_HOUR_OF_DAY, hour_);
> calendar.set(UCAL_MINUTE, minute_);
> calendar.set(UCAL_SECOND, second_);
> calendar.set(UCAL_YEAR_WOY, week_year_);
> calendar.set(UCAL_WEEK_OF_YEAR, week_);
> 
> If there is an invalid value in even one of the fields
calendar.getTime(success)
> will fail, so it won't work.

[Miguel Garcia, 2013-05-17 15:59:40]

https://chromiumcodereview.appspot.com/15057004/diff/13001/content/renderer/d...
File content/renderer/date_time_formatter.cc (left):

https://chromiumcodereview.appspot.com/15057004/diff/13001/content/renderer/d...
content/renderer/date_time_formatter.cc:137: case
WebKit::WebDateTimeInputTypeWeek: // Not implemented
nit: remove the // Not implemented comment

[Miguel Garcia, 2013-05-21 15:02:23]

Oh and you will also need jam@ for the 
web_contents changes
On 2013/05/17 15:59:40, Miguel Garcia wrote:
>
https://chromiumcodereview.appspot.com/15057004/diff/13001/content/renderer/d...
> File content/renderer/date_time_formatter.cc (left):
> 
>
https://chromiumcodereview.appspot.com/15057004/diff/13001/content/renderer/d...
> content/renderer/date_time_formatter.cc:137: case
> WebKit::WebDateTimeInputTypeWeek: // Not implemented
> nit: remove the // Not implemented comment

[commit-bot: I haz the power, 2013-05-21 18:08:07]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[keishi, 2013-05-22 07:33:26]

https://codereview.chromium.org/15057004/diff/4001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/input/WeekPickerDialog.java
(right):

https://codereview.chromium.org/15057004/diff/4001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/input/WeekPickerDialog.java:18:
public class WeekPickerDialog extends AlertDialog implements OnClickListener,
I've made the dialog base class by making the parameter generic.

On 2013/05/17 11:23:38, Miguel Garcia wrote:
> I still don't understand what the problem is, just make those parameters
generic
> (like year and position_in_year) and you can practically reuse the same file.
> In any case if you see it's not straight forward I don't feel too strongly
about
> it. so It's fine
> 
> 
> 
> On 2013/05/14 13:47:47, keishi1 wrote:
> > On 2013/05/10 15:17:59, Miguel Garcia wrote:
> > > Same here (even more I think) both dialogs are essentially the same thing
> > (they
> > > display a number and a year) I think it'd make sense to have a more
general
> > > class that can do both or perhaps a base class with two very small
> subclasses.
> > 
> > Most of the methods have a week/month parameter so I wasn't able to share
much
> > code. I have left it separate classes in this patch.
>

https://codereview.chromium.org/15057004/diff/4001/content/renderer/date_time...
File content/renderer/date_time_formatter.cc (right):

https://codereview.chromium.org/15057004/diff/4001/content/renderer/date_time...
content/renderer/date_time_formatter.cc:121: } else {
On 2013/05/17 11:23:38, Miguel Garcia wrote:
> I'm still hesitant to special case week here, can we simply check wether the
> value returned by calendar.getTime() is > 0 and set it to 0 otherwise? The
date
> spec does not allow values below 0 for anything
> 
> http://www.w3.org/TR/html-markup/datatypes.html#form.data.date

calendar.getTime() will return milliseconds since epoch(1970-01-01) so we can't
check if its greater than 0. According to the spec we need to support dates
between 0001-01-01 and 275760-09-12.

https://codereview.chromium.org/15057004/diff/13001/content/renderer/date_tim...
File content/renderer/date_time_formatter.cc (left):

https://codereview.chromium.org/15057004/diff/13001/content/renderer/date_tim...
content/renderer/date_time_formatter.cc:137: case
WebKit::WebDateTimeInputTypeWeek: // Not implemented
On 2013/05/17 15:59:40, Miguel Garcia wrote:
> nit: remove the // Not implemented comment

Done.

[Miguel Garcia, 2013-05-23 18:38:48]

Two comments and we are done I think, I think once you merge with my cl you can
add the OWNERS for the different pieces as suggested in a comment before so they
can rubberstamp

BTW I am using http://jsbin.com/ejudos/24/edit to test date/value changes. Feel
free to upgrade it with more use cases for week if you need to.

https://codereview.chromium.org/15057004/diff/4001/content/renderer/date_time...
File content/renderer/date_time_formatter.cc (right):

https://codereview.chromium.org/15057004/diff/4001/content/renderer/date_time...
content/renderer/date_time_formatter.cc:121: } else {
Ok, I think you've convinced me :)

On 2013/05/22 07:33:26, keishi1 wrote:
> On 2013/05/17 11:23:38, Miguel Garcia wrote:
> > I'm still hesitant to special case week here, can we simply check wether the
> > value returned by calendar.getTime() is > 0 and set it to 0 otherwise? The
> date
> > spec does not allow values below 0 for anything
> > 
> > http://www.w3.org/TR/html-markup/datatypes.html#form.data.date
> 
> calendar.getTime() will return milliseconds since epoch(1970-01-01) so we
can't
> check if its greater than 0. According to the spec we need to support dates
> between 0001-01-01 and 275760-09-12.

https://codereview.chromium.org/15057004/diff/23001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/input/BaseDatePicker.java
(right):

https://codereview.chromium.org/15057004/diff/23001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/BaseDatePicker.java:28:
public class BaseDatePicker extends FrameLayout {
DatePicker suggest its a date, let's go with TwoFieldDatePicker?

https://codereview.chromium.org/15057004/diff/23001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/BaseDatePicker.java:29:

I've made a significant simplification of the month picker class as part of
https://chromiumcodereview.appspot.com/15533003/ I think it's worth waiting so
the base class will be much simpler and it will give you support of min and max
for free.

[Miguel Garcia, 2013-06-06 17:48:11]

So now that min/max has landed (together with a much simpler Month picker) do
you want to rebase and ping me once it's ready for another look? We can still
make M29 with this I think


On 2013/05/23 18:38:48, Miguel Garcia wrote:
> Two comments and we are done I think, I think once you merge with my cl you
can
> add the OWNERS for the different pieces as suggested in a comment before so
they
> can rubberstamp
> 
> BTW I am using http://jsbin.com/ejudos/24/edit to test date/value changes.
Feel
> free to upgrade it with more use cases for week if you need to.
> 
>
https://codereview.chromium.org/15057004/diff/4001/content/renderer/date_time...
> File content/renderer/date_time_formatter.cc (right):
> 
>
https://codereview.chromium.org/15057004/diff/4001/content/renderer/date_time...
> content/renderer/date_time_formatter.cc:121: } else {
> Ok, I think you've convinced me :)
> 
> On 2013/05/22 07:33:26, keishi1 wrote:
> > On 2013/05/17 11:23:38, Miguel Garcia wrote:
> > > I'm still hesitant to special case week here, can we simply check wether
the
> > > value returned by calendar.getTime() is > 0 and set it to 0 otherwise? The
> > date
> > > spec does not allow values below 0 for anything
> > > 
> > > http://www.w3.org/TR/html-markup/datatypes.html#form.data.date
> > 
> > calendar.getTime() will return milliseconds since epoch(1970-01-01) so we
> can't
> > check if its greater than 0. According to the spec we need to support dates
> > between 0001-01-01 and 275760-09-12.
> 
>
https://codereview.chromium.org/15057004/diff/23001/content/public/android/ja...
> File
>
content/public/android/java/src/org/chromium/content/browser/input/BaseDatePicker.java
> (right):
> 
>
https://codereview.chromium.org/15057004/diff/23001/content/public/android/ja...
>
content/public/android/java/src/org/chromium/content/browser/input/BaseDatePicker.java:28:
> public class BaseDatePicker extends FrameLayout {
> DatePicker suggest its a date, let's go with TwoFieldDatePicker?
> 
>
https://codereview.chromium.org/15057004/diff/23001/content/public/android/ja...
>
content/public/android/java/src/org/chromium/content/browser/input/BaseDatePicker.java:29:
> 
> I've made a significant simplification of the month picker class as part of
> https://chromiumcodereview.appspot.com/15533003/ I think it's worth waiting so
> the base class will be much simpler and it will give you support of min and
max
> for free.

[keishi, 2013-06-07 11:08:51]

I've rebased the patch and added support for min/max to the weekPicker too.

https://codereview.chromium.org/15057004/diff/23001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/input/BaseDatePicker.java
(right):

https://codereview.chromium.org/15057004/diff/23001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/BaseDatePicker.java:28:
public class BaseDatePicker extends FrameLayout {
On 2013/05/23 18:38:49, Miguel Garcia wrote:
> DatePicker suggest its a date, let's go with TwoFieldDatePicker?

Done.

https://codereview.chromium.org/15057004/diff/23001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/BaseDatePicker.java:29:

On 2013/05/23 18:38:49, Miguel Garcia wrote:
> I've made a significant simplification of the month picker class as part of
> https://chromiumcodereview.appspot.com/15533003/ I think it's worth waiting so
> the base class will be much simpler and it will give you support of min and
max
> for free.

Done.

[keishi, 2013-06-07 11:10:41]

jam@ could you review the change to web_contents? Thanks.

[Miguel Garcia, 2013-06-10 12:29:22]

On 2013/06/07 11:10:41, keishi1 wrote:
> jam@ could you review the change to web_contents? Thanks.

I think the rebase was not really successful since all the try bots are failing.
Can you have a look?

I had a cursory look and it seems that the TwoFieldDatePicker has a lot of stuff
that is no longer required (and was removed in the MonthPicker refactor) I
suggest you take a look at the new MonthPicker file as it is in head Today and
refactor from there.

Thanks!

[keishi, 2013-06-10 15:15:55]

On 2013/06/10 12:29:22, Miguel Garcia wrote:
> On 2013/06/07 11:10:41, keishi1 wrote:
> > jam@ could you review the change to web_contents? Thanks.
> 
> I think the rebase was not really successful since all the try bots are
failing.
> Can you have a look?
> 
> I had a cursory look and it seems that the TwoFieldDatePicker has a lot of
stuff
> that is no longer required (and was removed in the MonthPicker refactor) I
> suggest you take a look at the new MonthPicker file as it is in head Today and
> refactor from there.
> 
> Thanks!

The patch was missing a file so it should build now.
I removed code to match the current MonthPicker.java

[jam, 2013-06-10 17:26:24]

On 2013/06/07 11:10:41, keishi1 wrote:
> jam@ could you review the change to web_contents? Thanks.

that file lgtm. one comment on the renderer

https://codereview.chromium.org/15057004/diff/35001/content/renderer/render_t...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/15057004/diff/35001/content/renderer/render_t...
content/renderer/render_thread_impl.cc:653: // 
WebRuntimeFeatures::enableInputTypeWeek(false);
just take it out

[keishi, 2013-06-11 02:27:16]

Thanks.

https://codereview.chromium.org/15057004/diff/35001/content/renderer/render_t...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/15057004/diff/35001/content/renderer/render_t...
content/renderer/render_thread_impl.cc:653: // 
WebRuntimeFeatures::enableInputTypeWeek(false);
On 2013/06/10 17:26:24, jam wrote:
> just take it out

Done.

[Miguel Garcia, 2013-06-11 13:02:51]

I've been playing a bit with it and functionality wise it looks good. I also
upgraded http://chromiumexperiments.appspot.com/static/forms.html with some test
cases for input type=week. 

I think however that we can reuse more common code. All the layout is
essentially the same thing so the new TwoFieldDatePicker and
TwoFieldDatePickerDialog can do more common stuff. Please have a look at my
comments and let me know what you think ok?

https://codereview.chromium.org/15057004/diff/35001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/input/TwoFieldDatePicker.java
(right):

https://codereview.chromium.org/15057004/diff/35001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/TwoFieldDatePicker.java:141:
* This method should be subclassed to update the spinners based on mCurrentDate.
I think this should be abstract since we need and actual implementation

https://codereview.chromium.org/15057004/diff/45001/content/public/android/ja...
File content/public/android/java/res/layout/week_picker.xml (right):

https://codereview.chromium.org/15057004/diff/45001/content/public/android/ja...
content/public/android/java/res/layout/week_picker.xml:24:
android:id="@+id/week"
I don't think you need a new xml template for week, just make the one for month
more generic by renaming it and changing the changing the ids.

https://codereview.chromium.org/15057004/diff/45001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/input/DateTimeChooserAndroid.java
(right):

https://codereview.chromium.org/15057004/diff/45001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/DateTimeChooserAndroid.java:45:
int hour, int minute, int second, int week,
nit: I think this fits in 100 cols

https://codereview.chromium.org/15057004/diff/45001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/DateTimeChooserAndroid.java:56:
int hour, int minute, int second, int week,
nit: I think this fits in 100 cols

https://codereview.chromium.org/15057004/diff/45001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java
(right):

https://codereview.chromium.org/15057004/diff/45001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java:22:
import
org.chromium.content.browser.input.TwoFieldDatePickerDialog.OnMonthOrWeekSetListener;
Can you call the listener OnValueSelectedListener? Also don't import it like
this just import org.chromium.content.browser.input.TwoFieldDatePickerDialog
here and then use TwoFieldDatePickerDialog.OnValueSelectedListener in the code.
It makes it more readable.

https://codereview.chromium.org/15057004/diff/45001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java:122:
if (dialogType == sTextInputTypeWeek) {
please add this directly as an else if like all the previous types.

https://codereview.chromium.org/15057004/diff/45001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java:122:
if (dialogType == sTextInputTypeWeek) {
Move this as a new else if block instead of doing having it as a separate if. If
you don't need the normalized time just use the original ones.

https://codereview.chromium.org/15057004/diff/45001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java:133:
DatePickerDialog dialog = new DatePickerDialog(mContext, new
DateListener(dialogType),
nit, line over 100 cols

https://codereview.chromium.org/15057004/diff/45001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java:257:
public void onMonthSet(MonthPicker view, int year, int month) {
I think you don't need two methods here just create a single onValueSet with all
the arguments and then fill in the datetime value based on the dialog type. You
also don't need to pass the view actually since it's not used anywhere

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

https://codereview.chromium.org/15057004/diff/45001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/MonthPicker.java:112:
mPositionInYearSpinner.setDisplayedValues(null);
This logic is exactly the same for month and week right? Why not share the
implementation and just change how min/max is calculated?

https://codereview.chromium.org/15057004/diff/45001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/input/TwoFieldDatePicker.java
(right):

https://codereview.chromium.org/15057004/diff/45001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/TwoFieldDatePicker.java:28:
// having dates over 2038.
no protected members please, make accessors if you need them but I don't think
you will need them, as per the remaining comments.

https://codereview.chromium.org/15057004/diff/45001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/TwoFieldDatePicker.java:141:
* This method should be subclassed to update the spinners based on mCurrentDate.
make this abstract, it does require an implementation.

https://codereview.chromium.org/15057004/diff/45001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/input/TwoFieldDatePickerDialog.java
(right):

https://codereview.chromium.org/15057004/diff/45001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/TwoFieldDatePickerDialog.java:102:
protected void tryNotifyDateSet() {
abstract

https://codereview.chromium.org/15057004/diff/45001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/TwoFieldDatePickerDialog.java:132:
@Override
I think all this was removed as part of the refactor

https://codereview.chromium.org/15057004/diff/45001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/input/WeekPicker.java
(right):

https://codereview.chromium.org/15057004/diff/45001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/WeekPicker.java:22:
LayoutInflater inflater = (LayoutInflater) context
yeah it seems you can move the inflation to the parent class since all you need
to change is the text. Just pass the text and the min/max dates to the inner
constructor

https://codereview.chromium.org/15057004/diff/45001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/input/WeekPickerDialog.java
(right):

https://codereview.chromium.org/15057004/diff/45001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/WeekPickerDialog.java:1:
// Copyright (c) 2012 The Chromium Authors. All rights reserved.
nit 2013

[keishi, 2013-06-12 13:23:07]

https://codereview.chromium.org/15057004/diff/45001/content/public/android/ja...
File content/public/android/java/res/layout/week_picker.xml (right):

https://codereview.chromium.org/15057004/diff/45001/content/public/android/ja...
content/public/android/java/res/layout/week_picker.xml:24:
android:id="@+id/week"
On 2013/06/11 13:02:52, Miguel Garcia wrote:
> I don't think you need a new xml template for week, just make the one for
month
> more generic by renaming it and changing the changing the ids.

Done.

https://codereview.chromium.org/15057004/diff/45001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/input/DateTimeChooserAndroid.java
(right):

https://codereview.chromium.org/15057004/diff/45001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/DateTimeChooserAndroid.java:45:
int hour, int minute, int second, int week,
On 2013/06/11 13:02:52, Miguel Garcia wrote:
> nit: I think this fits in 100 cols

Done.

https://codereview.chromium.org/15057004/diff/45001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/DateTimeChooserAndroid.java:56:
int hour, int minute, int second, int week,
On 2013/06/11 13:02:52, Miguel Garcia wrote:
> nit: I think this fits in 100 cols

Done.

https://codereview.chromium.org/15057004/diff/45001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java
(right):

https://codereview.chromium.org/15057004/diff/45001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java:22:
import
org.chromium.content.browser.input.TwoFieldDatePickerDialog.OnMonthOrWeekSetListener;
On 2013/06/11 13:02:52, Miguel Garcia wrote:
> Can you call the listener OnValueSelectedListener? Also don't import it like
> this just import org.chromium.content.browser.input.TwoFieldDatePickerDialog
> here and then use TwoFieldDatePickerDialog.OnValueSelectedListener in the
code.
> It makes it more readable.

Done.

https://codereview.chromium.org/15057004/diff/45001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java:257:
public void onMonthSet(MonthPicker view, int year, int month) {
On 2013/06/11 13:02:52, Miguel Garcia wrote:
> I think you don't need two methods here just create a single onValueSet with
all
> the arguments and then fill in the datetime value based on the dialog type.
You
> also don't need to pass the view actually since it's not used anywhere

Done.

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

https://codereview.chromium.org/15057004/diff/45001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/MonthPicker.java:112:
mPositionInYearSpinner.setDisplayedValues(null);
On 2013/06/11 13:02:52, Miguel Garcia wrote:
> This logic is exactly the same for month and week right? Why not share the
> implementation and just change how min/max is calculated?

Done.

https://codereview.chromium.org/15057004/diff/45001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/input/TwoFieldDatePicker.java
(right):

https://codereview.chromium.org/15057004/diff/45001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/TwoFieldDatePicker.java:28:
// having dates over 2038.
On 2013/06/11 13:02:52, Miguel Garcia wrote:
> no protected members please, make accessors if you need them but I don't think
> you will need them, as per the remaining comments.

Done. I had to add some accessors.

https://codereview.chromium.org/15057004/diff/45001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/TwoFieldDatePicker.java:141:
* This method should be subclassed to update the spinners based on mCurrentDate.
On 2013/06/11 13:02:52, Miguel Garcia wrote:
> make this abstract, it does require an implementation.

Done.

https://codereview.chromium.org/15057004/diff/45001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/input/TwoFieldDatePickerDialog.java
(right):

https://codereview.chromium.org/15057004/diff/45001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/TwoFieldDatePickerDialog.java:102:
protected void tryNotifyDateSet() {
On 2013/06/11 13:02:52, Miguel Garcia wrote:
> abstract

Done.

https://codereview.chromium.org/15057004/diff/45001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/TwoFieldDatePickerDialog.java:132:
@Override
On 2013/06/11 13:02:52, Miguel Garcia wrote:
> I think all this was removed as part of the refactor

Done.

https://codereview.chromium.org/15057004/diff/45001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/input/WeekPicker.java
(right):

https://codereview.chromium.org/15057004/diff/45001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/WeekPicker.java:22:
LayoutInflater inflater = (LayoutInflater) context
On 2013/06/11 13:02:52, Miguel Garcia wrote:
> yeah it seems you can move the inflation to the parent class since all you
need
> to change is the text. Just pass the text and the min/max dates to the inner
> constructor

Done.

https://codereview.chromium.org/15057004/diff/45001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/input/WeekPickerDialog.java
(right):

https://codereview.chromium.org/15057004/diff/45001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/WeekPickerDialog.java:1:
// Copyright (c) 2012 The Chromium Authors. All rights reserved.
On 2013/06/11 13:02:52, Miguel Garcia wrote:
> nit 2013

Done.

[Miguel Garcia, 2013-06-18 07:54:44]

lgtm

Thanks for putting up with my comments.

Marcus can you have a look for the files requiring OWNERS?

Thanks

[bulach, 2013-06-18 10:38:20]

lgtm, thanks! just some small nits:

https://codereview.chromium.org/15057004/diff/80001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/input/TwoFieldDatePicker.java
(right):

https://codereview.chromium.org/15057004/diff/80001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/TwoFieldDatePicker.java:121:
protected abstract Calendar createDateFromValue(long value);
please comment on what subclasses are responsible to do, like:

/**
 * Subclasses know the semantics of @value, and need to return
 * a Calendar corresponding to it.
 */

https://codereview.chromium.org/15057004/diff/80001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/TwoFieldDatePicker.java:138:
protected abstract void setCurrentDate(int year, int positionInYear);
as above, docstring.

https://codereview.chromium.org/15057004/diff/80001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/TwoFieldDatePicker.java:141:
{
nit: move { to EOL at 140

https://codereview.chromium.org/15057004/diff/80001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/TwoFieldDatePicker.java:173:
protected abstract int getMaxYear();
nit: ditto, docstrings so subclasses know what to do.

https://codereview.chromium.org/15057004/diff/80001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/input/TwoFieldDatePickerDialog.java
(right):

https://codereview.chromium.org/15057004/diff/80001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/TwoFieldDatePickerDialog.java:93:
protected abstract void tryNotifyDateSet();
nit: docstring

[keishi, 2013-06-19 04:39:36]

Thanks!

https://codereview.chromium.org/15057004/diff/80001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/input/TwoFieldDatePicker.java
(right):

https://codereview.chromium.org/15057004/diff/80001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/TwoFieldDatePicker.java:121:
protected abstract Calendar createDateFromValue(long value);
On 2013/06/18 10:38:20, bulach wrote:
> please comment on what subclasses are responsible to do, like:
> 
> /**
>  * Subclasses know the semantics of @value, and need to return
>  * a Calendar corresponding to it.
>  */

Done.

https://codereview.chromium.org/15057004/diff/80001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/TwoFieldDatePicker.java:138:
protected abstract void setCurrentDate(int year, int positionInYear);
On 2013/06/18 10:38:20, bulach wrote:
> as above, docstring.

Done.

https://codereview.chromium.org/15057004/diff/80001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/TwoFieldDatePicker.java:141:
{
On 2013/06/18 10:38:20, bulach wrote:
> nit: move { to EOL at 140

Done.

https://codereview.chromium.org/15057004/diff/80001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/input/TwoFieldDatePickerDialog.java
(right):

https://codereview.chromium.org/15057004/diff/80001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/TwoFieldDatePickerDialog.java:93:
protected abstract void tryNotifyDateSet();
On 2013/06/18 10:38:20, bulach wrote:
> nit: docstring

Done.

[commit-bot: I haz the power, 2013-06-19 04:41:50]

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

[commit-bot: I haz the power, 2013-06-19 04:54:08]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[keishi, 2013-06-19 05:28:51]

palmer@ could you review the change to view_messages.h

[palmer, 2013-06-19 20:39:28]

IPC security LGTM. Note nits.

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

https://codereview.chromium.org/15057004/diff/85001/content/renderer/renderer...
content/renderer/renderer_date_time_picker.cc:41: message.month = 
parser.GetMonth();
Nits: Use only 1 space.

[commit-bot: I haz the power, 2013-06-20 02:21:18]

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

[commit-bot: I haz the power, 2013-06-20 04:27:46]

Retried try job too often on android_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2013-06-20 05:05:46]

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

[commit-bot: I haz the power, 2013-06-20 06:37:02]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-06-20 12:13:14]

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

[commit-bot: I haz the power, 2013-06-20 16:29:10]

Change committed as 207444