Support datalist for date/time input types on Android

Support datalist for date/time input types on Android

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

[keishi, 2013-10-07 13:00:15]

This shows a dialog with a list of datalist suggestions first, and when the user
taps "Other" at the end of the list it shows the existing date/time picker
dialog.

Here is a
screenshot:https://docs.google.com/file/d/0BwRi59l_ri74NlZHaUc5MHprNU0/edit?usp=sharing
The design was approved by roma@

Blink side patch: https://codereview.chromium.org/23623017

[Miguel Garcia, 2013-10-08 17:44:39]

Some initial comments, thanks for working on this!

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

https://codereview.chromium.org/23623019/diff/9001/content/common/view_messag...
content/common/view_messages.h:769:
IPC_MESSAGE_ROUTED1(ViewMsg_AcceptDataListSuggestion,
I would not allow the suggestion to crop back, just return the actual date or
time selected regardless of wether it comes as a suggestion or as a an actual
value selected by the user.

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

https://codereview.chromium.org/23623019/diff/9001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/input/DateTimeChooserAndroid.java:78:
private static DateTimeSuggestion[] createDateTimeSuggestionArray(int size) {
why do you need to preallocate the array in java? You can simply created in C++
and pass it over.

https://codereview.chromium.org/23623019/diff/9001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/input/DateTimeChooserAndroid.java:83:
* @param array ColorSuggestion array that should get a new suggestion added.
color suggestion? :)

No need to add suggestions one by one you can pass the array from C++ directly

https://codereview.chromium.org/23623019/diff/9001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/input/DateTimeChooserAndroid.java:112:
private native void nativeAcceptDataListSuggestion(
don't add an extra JNI method, I think you can just pass the right date/time
selected to C++

https://codereview.chromium.org/23623019/diff/9001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/input/DateTimeSuggestion.java
(right):

https://codereview.chromium.org/23623019/diff/9001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/input/DateTimeSuggestion.java:27:
}
please implement equals() and hashcode() (and maybe toString()) since this is
meant to be used in data structures. Currently just an ArrayList but that could
change to say a hashMap at any point. In general it's good to have these methods
implemented.

https://codereview.chromium.org/23623019/diff/9001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/input/DateTimeSuggestionListAdapter.java
(right):

https://codereview.chromium.org/23623019/diff/9001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/input/DateTimeSuggestionListAdapter.java:22:
* Autofill suggestion adapter for AutofillWindow.
please fix doc

https://codereview.chromium.org/23623019/diff/9001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/input/DateTimeSuggestionListAdapter.java:56:
return super.getCount() + 1;
why +1? getCount should give you the actual size right?

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

https://codereview.chromium.org/23623019/diff/9001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java:8:
import android.app.AlertDialog;
double import

https://codereview.chromium.org/23623019/diff/9001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java:124:
void showSuggestionDialog(final int dialogType,
Add javadoc.
Can this be private?

I think this should be moved to it's own class, something called
DateTimeSuggestionDialog with a show method or something like that.

https://codereview.chromium.org/23623019/diff/9001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java:137:
new DateTimeSuggestionListAdapter(mContext, suggestionList);
nit: no need for the suggestionList temp variable, just do
final DateTimeSuggestionListAdapter adapter = new
DateTimeSuggestionListAdapter(mContext, Arrays.asList(suggestions));

https://codereview.chromium.org/23623019/diff/9001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java:147:
mInputActionDelegate.acceptDataListSuggestion(adapter.getItem(position).mValue);
can you call replaceDateTime with the right values instead of adding another
method?

https://codereview.chromium.org/23623019/diff/9001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java:153:
int dialogTitleId = R.string.date_picker_dialog_title;
How does this layout on a phone, it seems the design will work on tablets but
with smaller screens it will probably overflow. Can you send a screenshot on
portrait mode on a phone?

https://codereview.chromium.org/23623019/diff/9001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java:209:
if (isDialogShowing()) mDialog.dismiss();
can you move this check to the showDialog call instead of repeating it in both
showPickerDialog and showSuggestionDialog?

https://codereview.chromium.org/23623019/diff/9001/content/public/common/date...
File content/public/common/date_time_suggestion.h (right):

https://codereview.chromium.org/23623019/diff/9001/content/public/common/date...
content/public/common/date_time_suggestion.h:15: namespace content {
can you point to the java version of this (and the other way around) so they are
kept in sync?

https://codereview.chromium.org/23623019/diff/9001/content/public/common/date...
content/public/common/date_time_suggestion.h:24: base::string16 value;
I got very negative feedback for passing strings on IPCs but I am not sure how
to fix that really can you include chris palmer on this review now?

https://codereview.chromium.org/23623019/diff/9001/content/public/common/date...
content/public/common/date_time_suggestion.h:24: base::string16 value;
can you document all 3 values?

[newt (away), 2013-10-09 07:20:52]

https://codereview.chromium.org/23623019/diff/9001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/input/DateTimeSuggestion.java
(right):

https://codereview.chromium.org/23623019/diff/9001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/input/DateTimeSuggestion.java:12:
final String mValue;
these should presumably have the same visibility as the class (e.g. public).
also, public fields shouldn't have "m" prefix.

https://codereview.chromium.org/23623019/diff/9001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/input/DateTimeSuggestionListAdapter.java
(right):

https://codereview.chromium.org/23623019/diff/9001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/input/DateTimeSuggestionListAdapter.java:5:

remove blank line

https://codereview.chromium.org/23623019/diff/9001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/input/DateTimeSuggestionListAdapter.java:10:

remove blank line

https://codereview.chromium.org/23623019/diff/9001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/input/DateTimeSuggestionListAdapter.java:36:
LayoutInflater inflater =
LayoutInflater inflater = LayoutInflater.from(mContext);

https://codereview.chromium.org/23623019/diff/9001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/input/DateTimeSuggestionListAdapter.java:38:
layout = inflater.inflate(R.layout.date_time_suggestion_item, null);
do this:

    layout = inflater.inflate(R.layout.date_time_suggestion_item, parent,
false);

telling the inflate what the parent is going to be allows it to attach the
correct subclass of ViewGroup.LayoutParams to the inflated view

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

https://codereview.chromium.org/23623019/diff/9001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java:132:
LayoutInflater inflater =
unused variable

https://codereview.chromium.org/23623019/diff/9001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java:181:
mDialogAlreadyDismissed = true;
Can you replace lines 181-182 with "mDialog.dismiss()"?

https://codereview.chromium.org/23623019/diff/9001/content/public/android/jav...
File content/public/android/java/strings/android_content_strings.grd (right):

https://codereview.chromium.org/23623019/diff/9001/content/public/android/jav...
content/public/android/java/strings/android_content_strings.grd:23: <message
desc="Label for 'other' item in date picker dialog and time picker dialog, used
to close the list of suggestions and open the the full date/time picker
[CHAR-LIMIT=24]" name="IDS_DATE_PICKER_DIALOG_OTHER_ITEM_LABEL">
item -> button, and CHAR-LIMIT should be 32

https://codereview.chromium.org/23623019/diff/9001/ui/android/java/res/layout...
File ui/android/java/res/layout/date_time_suggestion_item.xml (right):

https://codereview.chromium.org/23623019/diff/9001/ui/android/java/res/layout...
ui/android/java/res/layout/date_time_suggestion_item.xml:1: <?xml version="1.0"
encoding="utf-8"?>
IMO, this could be renamed just date_time_suggestion.xml. I don't think "item"
adds anything useful.

https://codereview.chromium.org/23623019/diff/9001/ui/android/java/res/layout...
ui/android/java/res/layout/date_time_suggestion_item.xml:8: <LinearLayout
xmlns:android="http://schemas.android.com/apk/res/android"
indentation should be:

    <LinearLayout xmlns:android="..."
        android:id="...
        ... >
        <TextView
            android:id="..."
            ... />

https://codereview.chromium.org/23623019/diff/9001/ui/android/java/res/layout...
ui/android/java/res/layout/date_time_suggestion_item.xml:10:
android:layout_width="fill_parent"
s/fill_parent/match_parent/g

https://codereview.chromium.org/23623019/diff/9001/ui/android/java/res/layout...
ui/android/java/res/layout/date_time_suggestion_item.xml:12:
android:minHeight="44dp"
don't need minHeight="44dp" if you also have layout_height="44dp"

https://codereview.chromium.org/23623019/diff/9001/ui/android/java/res/layout...
ui/android/java/res/layout/date_time_suggestion_item.xml:21:
android:includeFontPadding="false"
why make this false? when this is false, the text's vertical alignment is
affected by whether it contains any descenders or ascenders. as a result, the
values and the labels often won't have their baselines aligned

https://codereview.chromium.org/23623019/diff/9001/ui/android/java/res/layout...
ui/android/java/res/layout/date_time_suggestion_item.xml:22:
android:layout_marginStart="10dp"
layout_* attributes should be group together at the top (after android:id)

https://codereview.chromium.org/23623019/diff/9001/ui/android/java/res/layout...
ui/android/java/res/layout/date_time_suggestion_item.xml:25:
android:layout_width="wrap_content"
use layout_width="0dp". this'll ensure the date_time_suggestion_value gets as
much space as it needs, and this TextView takes up an leftover space

https://codereview.chromium.org/23623019/diff/9001/ui/android/java/res/layout...
ui/android/java/res/layout/date_time_suggestion_item.xml:32:
android:minWidth="500dp"
you shouldn't need minWidth here

[keishi, 2013-10-21 17:00:57]

I've changed it to use double to exchange the value.

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

https://codereview.chromium.org/23623019/diff/9001/content/common/view_messag...
content/common/view_messages.h:769:
IPC_MESSAGE_ROUTED1(ViewMsg_AcceptDataListSuggestion,
On 2013/10/08 17:44:40, Miguel Garcia wrote:
> I would not allow the suggestion to crop back, just return the actual date or
> time selected regardless of wether it comes as a suggestion or as a an actual
> value selected by the user.

Removed, used ViewMsg_ReplaceDateTime

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

https://codereview.chromium.org/23623019/diff/9001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/input/DateTimeChooserAndroid.java:78:
private static DateTimeSuggestion[] createDateTimeSuggestionArray(int size) {
On 2013/10/08 17:44:40, Miguel Garcia wrote:
> why do you need to preallocate the array in java? You can simply created in
C++
> and pass it over.

Done.

https://codereview.chromium.org/23623019/diff/9001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/input/DateTimeChooserAndroid.java:83:
* @param array ColorSuggestion array that should get a new suggestion added.
On 2013/10/08 17:44:40, Miguel Garcia wrote:
> color suggestion? :)
> 
> No need to add suggestions one by one you can pass the array from C++ directly

Done.

https://codereview.chromium.org/23623019/diff/9001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/input/DateTimeChooserAndroid.java:112:
private native void nativeAcceptDataListSuggestion(
On 2013/10/08 17:44:40, Miguel Garcia wrote:
> don't add an extra JNI method, I think you can just pass the right date/time
> selected to C++

Done.

https://codereview.chromium.org/23623019/diff/9001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/input/DateTimeSuggestion.java
(right):

https://codereview.chromium.org/23623019/diff/9001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/input/DateTimeSuggestion.java:12:
final String mValue;
On 2013/10/09 07:20:52, newt wrote:
> these should presumably have the same visibility as the class (e.g. public).
> also, public fields shouldn't have "m" prefix.

Done.

https://codereview.chromium.org/23623019/diff/9001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/input/DateTimeSuggestionListAdapter.java
(right):

https://codereview.chromium.org/23623019/diff/9001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/input/DateTimeSuggestionListAdapter.java:36:
LayoutInflater inflater =
On 2013/10/09 07:20:52, newt wrote:
> LayoutInflater inflater = LayoutInflater.from(mContext);

Done.

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

https://codereview.chromium.org/23623019/diff/9001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java:8:
import android.app.AlertDialog;
On 2013/10/08 17:44:40, Miguel Garcia wrote:
> double import

Done.

https://codereview.chromium.org/23623019/diff/9001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java:132:
LayoutInflater inflater =
On 2013/10/09 07:20:52, newt wrote:
> unused variable

Done.

https://codereview.chromium.org/23623019/diff/9001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java:137:
new DateTimeSuggestionListAdapter(mContext, suggestionList);
On 2013/10/08 17:44:40, Miguel Garcia wrote:
> nit: no need for the suggestionList temp variable, just do
> final DateTimeSuggestionListAdapter adapter = new
> DateTimeSuggestionListAdapter(mContext, Arrays.asList(suggestions));

Done.

https://codereview.chromium.org/23623019/diff/9001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java:147:
mInputActionDelegate.acceptDataListSuggestion(adapter.getItem(position).mValue);
On 2013/10/08 17:44:40, Miguel Garcia wrote:
> can you call replaceDateTime with the right values instead of adding another
> method?

Done.

https://codereview.chromium.org/23623019/diff/9001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java:153:
int dialogTitleId = R.string.date_picker_dialog_title;
On 2013/10/08 17:44:40, Miguel Garcia wrote:
> How does this layout on a phone, it seems the design will work on tablets but
> with smaller screens it will probably overflow. Can you send a screenshot on
> portrait mode on a phone?

I don't have a device with a small screen so here is a screenshot with very long
strings. I think it looks good.
https://docs.google.com/file/d/0BwRi59l_ri74bExaTGg4aFE3NzQ/edit?usp=sharing

https://codereview.chromium.org/23623019/diff/9001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java:181:
mDialogAlreadyDismissed = true;
On 2013/10/09 07:20:52, newt wrote:
> Can you replace lines 181-182 with "mDialog.dismiss()"?

Done.

https://codereview.chromium.org/23623019/diff/9001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java:209:
if (isDialogShowing()) mDialog.dismiss();
On 2013/10/08 17:44:40, Miguel Garcia wrote:
> can you move this check to the showDialog call instead of repeating it in both
> showPickerDialog and showSuggestionDialog?

Done.

https://codereview.chromium.org/23623019/diff/9001/content/public/android/jav...
File content/public/android/java/strings/android_content_strings.grd (right):

https://codereview.chromium.org/23623019/diff/9001/content/public/android/jav...
content/public/android/java/strings/android_content_strings.grd:23: <message
desc="Label for 'other' item in date picker dialog and time picker dialog, used
to close the list of suggestions and open the the full date/time picker
[CHAR-LIMIT=24]" name="IDS_DATE_PICKER_DIALOG_OTHER_ITEM_LABEL">
On 2013/10/09 07:20:52, newt wrote:
> item -> button, and CHAR-LIMIT should be 32

Done.

https://codereview.chromium.org/23623019/diff/9001/content/public/common/date...
File content/public/common/date_time_suggestion.h (right):

https://codereview.chromium.org/23623019/diff/9001/content/public/common/date...
content/public/common/date_time_suggestion.h:15: namespace content {
On 2013/10/08 17:44:40, Miguel Garcia wrote:
> can you point to the java version of this (and the other way around) so they
are
> kept in sync?

Done.

https://codereview.chromium.org/23623019/diff/9001/content/public/common/date...
content/public/common/date_time_suggestion.h:24: base::string16 value;
On 2013/10/08 17:44:40, Miguel Garcia wrote:
> can you document all 3 values?

Done.

https://codereview.chromium.org/23623019/diff/9001/ui/android/java/res/layout...
File ui/android/java/res/layout/date_time_suggestion_item.xml (right):

https://codereview.chromium.org/23623019/diff/9001/ui/android/java/res/layout...
ui/android/java/res/layout/date_time_suggestion_item.xml:1: <?xml version="1.0"
encoding="utf-8"?>
On 2013/10/09 07:20:52, newt wrote:
> IMO, this could be renamed just date_time_suggestion.xml. I don't think "item"
> adds anything useful.

Done.

https://codereview.chromium.org/23623019/diff/9001/ui/android/java/res/layout...
ui/android/java/res/layout/date_time_suggestion_item.xml:8: <LinearLayout
xmlns:android="http://schemas.android.com/apk/res/android"
On 2013/10/09 07:20:52, newt wrote:
> indentation should be:
> 
>     <LinearLayout xmlns:android="..."
>         android:id="...
>         ... >
>         <TextView
>             android:id="..."
>             ... />

Done.

https://codereview.chromium.org/23623019/diff/9001/ui/android/java/res/layout...
ui/android/java/res/layout/date_time_suggestion_item.xml:10:
android:layout_width="fill_parent"
On 2013/10/09 07:20:52, newt wrote:
> s/fill_parent/match_parent/g

Done.

https://codereview.chromium.org/23623019/diff/9001/ui/android/java/res/layout...
ui/android/java/res/layout/date_time_suggestion_item.xml:10:
android:layout_width="fill_parent"
On 2013/10/09 07:20:52, newt wrote:
> s/fill_parent/match_parent/g

Done.

https://codereview.chromium.org/23623019/diff/9001/ui/android/java/res/layout...
ui/android/java/res/layout/date_time_suggestion_item.xml:21:
android:includeFontPadding="false"
On 2013/10/09 07:20:52, newt wrote:
> why make this false? when this is false, the text's vertical alignment is
> affected by whether it contains any descenders or ascenders. as a result, the
> values and the labels often won't have their baselines aligned

Removed. I copied this file from Android Studio and forgot to fix it.

https://codereview.chromium.org/23623019/diff/9001/ui/android/java/res/layout...
ui/android/java/res/layout/date_time_suggestion_item.xml:22:
android:layout_marginStart="10dp"
On 2013/10/09 07:20:52, newt wrote:
> layout_* attributes should be group together at the top (after android:id)

Done.

https://codereview.chromium.org/23623019/diff/9001/ui/android/java/res/layout...
ui/android/java/res/layout/date_time_suggestion_item.xml:25:
android:layout_width="wrap_content"
On 2013/10/09 07:20:52, newt wrote:
> use layout_width="0dp". this'll ensure the date_time_suggestion_value gets as
> much space as it needs, and this TextView takes up an leftover space

Done.

https://codereview.chromium.org/23623019/diff/9001/ui/android/java/res/layout...
ui/android/java/res/layout/date_time_suggestion_item.xml:32:
android:minWidth="500dp"
On 2013/10/09 07:20:52, newt wrote:
> you shouldn't need minWidth here

Done.

[keishi, 2013-11-01 01:59:49]

miguelg@, newt@
Could you please take a look again? Thanks.

[newt (away), 2013-11-04 17:35:54]

a couple comments inline, mostly looks good though.

I find encoding dates as doubles a bit odd, but if that's what blink already
does, I guess it's OK to use here.

https://codereview.chromium.org/23623019/diff/61001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/input/DateTimeSuggestionListAdapter.java
(right):

https://codereview.chromium.org/23623019/diff/61001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/DateTimeSuggestionListAdapter.java:10:

remove empty line

https://codereview.chromium.org/23623019/diff/61001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/DateTimeSuggestionListAdapter.java:22:
* Autofill suggestion adapter for AutofillWindow.
Fix javadoc

https://codereview.chromium.org/23623019/diff/61001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/DateTimeSuggestionListAdapter.java:25:
private Context mContext;
might as well be final

https://codereview.chromium.org/23623019/diff/61001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/DateTimeSuggestionListAdapter.java:37:
layout = inflater.inflate(R.layout.date_time_suggestion, null);
do this:

    layout = inflater.inflate(R.layout.date_time_suggestion, parent,
false);

telling the inflater what the parent is going to be allows it to attach the
correct subclass of ViewGroup.LayoutParams to the inflated view

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

https://codereview.chromium.org/23623019/diff/61001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java:174:
showSuggestionDialog(type, value,
can this fit on one line?

https://codereview.chromium.org/23623019/diff/61001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java:197:
milli = (int) value % 1000;
This should be:

    milli = ((long) value) % 1000;

(casting to int will return an arbitrary and different result)

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

https://codereview.chromium.org/23623019/diff/61001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/TwoFieldDatePicker.java:137:
/**
javadoc

https://codereview.chromium.org/23623019/diff/61001/content/public/common/dat...
File content/public/common/date_time_suggestion.h (right):

https://codereview.chromium.org/23623019/diff/61001/content/public/common/dat...
content/public/common/date_time_suggestion.h:26: // The suggestion value
localized.
is this the localized *value* or the localized *label*?

https://codereview.chromium.org/23623019/diff/61001/ui/android/java/res/layou...
File ui/android/java/res/layout/date_time_suggestion.xml (right):

https://codereview.chromium.org/23623019/diff/61001/ui/android/java/res/layou...
ui/android/java/res/layout/date_time_suggestion.xml:11:
android:layout_height="44dp"
change layout_height to wrap_content, in case the user's font size is huge and
44dp is not enough vertical space

https://codereview.chromium.org/23623019/diff/61001/ui/android/java/res/layou...
ui/android/java/res/layout/date_time_suggestion.xml:35:
android:includeFontPadding="false" />
remove includeFontPadding attribute

[keishi, 2013-11-05 07:23:42]

https://codereview.chromium.org/23623019/diff/61001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/input/DateTimeSuggestionListAdapter.java
(right):

https://codereview.chromium.org/23623019/diff/61001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/DateTimeSuggestionListAdapter.java:10:

On 2013/11/04 17:35:55, newt wrote:
> remove empty line

Done.

https://codereview.chromium.org/23623019/diff/61001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/DateTimeSuggestionListAdapter.java:22:
* Autofill suggestion adapter for AutofillWindow.
On 2013/11/04 17:35:55, newt wrote:
> Fix javadoc

Done.

https://codereview.chromium.org/23623019/diff/61001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/DateTimeSuggestionListAdapter.java:25:
private Context mContext;
On 2013/11/04 17:35:55, newt wrote:
> might as well be final

Done.

https://codereview.chromium.org/23623019/diff/61001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/DateTimeSuggestionListAdapter.java:37:
layout = inflater.inflate(R.layout.date_time_suggestion, null);
On 2013/11/04 17:35:55, newt wrote:
> do this:
> 
>     layout = inflater.inflate(R.layout.date_time_suggestion, parent,
> false);
> 
> telling the inflater what the parent is going to be allows it to attach the
> correct subclass of ViewGroup.LayoutParams to the inflated view

Done.

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

https://codereview.chromium.org/23623019/diff/61001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java:174:
showSuggestionDialog(type, value,
On 2013/11/04 17:35:55, newt wrote:
> can this fit on one line?

Done.

https://codereview.chromium.org/23623019/diff/61001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java:197:
milli = (int) value % 1000;
On 2013/11/04 17:35:55, newt wrote:
> This should be:
> 
>     milli = ((long) value) % 1000;
> 
> (casting to int will return an arbitrary and different result)

Done.

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

https://codereview.chromium.org/23623019/diff/61001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/input/TwoFieldDatePicker.java:137:
/**
On 2013/11/04 17:35:55, newt wrote:
> javadoc

Done.

https://codereview.chromium.org/23623019/diff/61001/content/public/common/dat...
File content/public/common/date_time_suggestion.h (right):

https://codereview.chromium.org/23623019/diff/61001/content/public/common/dat...
content/public/common/date_time_suggestion.h:26: // The suggestion value
localized.
On 2013/11/04 17:35:55, newt wrote:
> is this the localized *value* or the localized *label*?

Its the localized value.

<datalist>
<option label="New year's day">2014-01-01</option>
</datalist>

localizedValue will be something like "Jan. 1, 2014 " depending on the locale.
label will be "New year's day"

https://codereview.chromium.org/23623019/diff/61001/ui/android/java/res/layou...
File ui/android/java/res/layout/date_time_suggestion.xml (right):

https://codereview.chromium.org/23623019/diff/61001/ui/android/java/res/layou...
ui/android/java/res/layout/date_time_suggestion.xml:11:
android:layout_height="44dp"
On 2013/11/04 17:35:55, newt wrote:
> change layout_height to wrap_content, in case the user's font size is huge and
> 44dp is not enough vertical space

Done.

https://codereview.chromium.org/23623019/diff/61001/ui/android/java/res/layou...
ui/android/java/res/layout/date_time_suggestion.xml:35:
android:includeFontPadding="false" />
On 2013/11/04 17:35:55, newt wrote:
> remove includeFontPadding attribute

Done.

[keishi, 2013-11-05 07:25:13]

+palmer Could you review the change to the IPC? Thanks.

[newt (away), 2013-11-05 15:37:05]

Java/ui bits lgtm

https://codereview.chromium.org/23623019/diff/61001/content/public/common/dat...
File content/public/common/date_time_suggestion.h (right):

https://codereview.chromium.org/23623019/diff/61001/content/public/common/dat...
content/public/common/date_time_suggestion.h:26: // The suggestion value
localized.
On 2013/11/05 07:23:42, keishi1 wrote:
> On 2013/11/04 17:35:55, newt wrote:
> > is this the localized *value* or the localized *label*?
> 
> Its the localized value.
> 
> <datalist>
> <option label="New year's day">2014-01-01</option>
> </datalist>
> 
> localizedValue will be something like "Jan. 1, 2014 " depending on the locale.
> label will be "New year's day"

gotcha, thanks

https://codereview.chromium.org/23623019/diff/241001/build/android/envsetup.sh
File build/android/envsetup.sh (right):

https://codereview.chromium.org/23623019/diff/241001/build/android/envsetup.s...
build/android/envsetup.sh:148: let "goma_threads=24"
remove this :)

[keishi, 2013-11-06 02:20:42]

Thanks!

https://codereview.chromium.org/23623019/diff/241001/build/android/envsetup.sh
File build/android/envsetup.sh (right):

https://codereview.chromium.org/23623019/diff/241001/build/android/envsetup.s...
build/android/envsetup.sh:148: let "goma_threads=24"
On 2013/11/05 15:37:05, newt wrote:
> remove this :)

Done.

[palmer, 2013-11-06 02:29:00]

https://codereview.chromium.org/23623019/diff/371001/content/public/common/da...
File content/public/common/date_time_suggestion.h (right):

https://codereview.chromium.org/23623019/diff/371001/content/public/common/da...
content/public/common/date_time_suggestion.h:27: base::string16 localizedValue;
We should localize it on the browser (trusted) side if possible.

https://codereview.chromium.org/23623019/diff/371001/content/public/common/da...
content/public/common/date_time_suggestion.h:29: base::string16 label;
Does the browser side check this for sanity in some way? (Probably: contains
only printable characters, is shorter than some reasonable max length.)

[keishi, 2013-11-06 04:28:46]

https://codereview.chromium.org/23623019/diff/371001/content/public/common/da...
File content/public/common/date_time_suggestion.h (right):

https://codereview.chromium.org/23623019/diff/371001/content/public/common/da...
content/public/common/date_time_suggestion.h:27: base::string16 localizedValue;
On 2013/11/06 02:29:00, Chromium Palmer wrote:
> We should localize it on the browser (trusted) side if possible.

I don't think we can do that.

We want to show the same localized value in the dialog and in the web page.
We have written a LDML parser inside Blink to do the localization properly and
it would be hard to bring it over to the browser process.

https://codereview.chromium.org/23623019/diff/371001/content/public/common/da...
content/public/common/date_time_suggestion.h:29: base::string16 label;
On 2013/11/06 02:29:00, Chromium Palmer wrote:
> Does the browser side check this for sanity in some way? (Probably: contains
> only printable characters, is shorter than some reasonable max length.)

We don't do anything right now.

In my next patch I'll trim the length down to 256 characters in the browser and
renderer.

How do I check for printable characters? "label" will contain non ascii
characters.

[palmer, 2013-11-07 00:19:33]

> How do I check for printable characters? "label" will contain non ascii
> characters.

In ICU, there is a function called u_isgraph.

[lgombos, 2013-11-27 14:32:06]

Would be great to have a bugID for this. Can you just use
https://code.google.com/p/chromium/issues/detail?id=242455 ?

[keishi, 2013-11-29 02:32:08]

@palmer I added the sanitization. I used isprint because we want to keep spaces.

I've separated out the changing value to double part to another patch.
https://codereview.chromium.org/85643002/

@miguelg please take a look.

@kenrb could you review view_messages.h?

jochen@ could you review web_contents_impl.cc?

jam@ could you review the files below?
content/content_common.gypi
content/public/common/date_time_suggestion.h
content/public/common/date_time_suggestion.cc
content/renderer/renderer_date_time_picker.cc

joth@ could you review these files?
content/public/android/java/resource_map/org/chromium/content/R.java
content/public/android/java/src/org/chromium/content/browser/input/DateTimeChooserAndroid.java
content/public/android/java/src/org/chromium/content/browser/input/DateTimeSuggestion.java
content/public/android/java/src/org/chromium/content/browser/input/DateTimeSuggestionListAdapter.java
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java
content/public/android/java/strings/android_content_strings.grd
content/public/android/javatests/src/org/chromium/content/browser/input/InputDialogContainerTest.java
content/browser/android/date_time_chooser_android.cc
content/browser/android/date_time_chooser_android.h



On 2013/11/27 14:32:06, lgombos wrote:
> Would be great to have a bugID for this. Can you just use
> https://code.google.com/p/chromium/issues/detail?id=242455 ?

Done.

[keishi, 2013-11-29 02:33:05]

@kenrb could you review view_messages.h?

jochen@ could you review web_contents_impl.cc?

jam@ could you review the files below?
content/content_common.gypi
content/public/common/date_time_suggestion.h
content/public/common/date_time_suggestion.cc
content/renderer/renderer_date_time_picker.cc

joth@ could you review these files?
content/public/android/java/resource_map/org/chromium/content/R.java
content/public/android/java/src/org/chromium/content/browser/input/DateTimeChooserAndroid.java
content/public/android/java/src/org/chromium/content/browser/input/DateTimeSuggestion.java
content/public/android/java/src/org/chromium/content/browser/input/DateTimeSuggestionListAdapter.java
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java
content/public/android/java/strings/android_content_strings.grd
content/public/android/javatests/src/org/chromium/content/browser/input/InputDialogContainerTest.java
content/browser/android/date_time_chooser_android.cc
content/browser/android/date_time_chooser_android.h

[jam, 2013-12-02 17:27:21]

https://codereview.chromium.org/23623019/diff/461001/content/public/common/da...
File content/public/common/date_time_suggestion.h (right):

https://codereview.chromium.org/23623019/diff/461001/content/public/common/da...
content/public/common/date_time_suggestion.h:1: // Copyright 2013 The Chromium
Authors. All rights reserved.
why is this in content/public? Per
http://www.chromium.org/developers/content-module/content-api, stuff should only
go into content/public if embedders of content need it. If this is only used
inside content, it should be in content/common

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

https://codereview.chromium.org/23623019/diff/461001/content/renderer/rendere...
content/renderer/renderer_date_time_picker.cc:19: static ui::TextInputType
toTextInputType(int type) {
nit: per style guide, this needs to be ToTextInputType

[joth, 2013-12-02 20:48:54]

+ bulach to have a look at content/browser/android/date_time_chooser_android

lgtm rubberstamp for the .java files based on newt's review

https://codereview.chromium.org/23623019/diff/461001/content/browser/android/...
File content/browser/android/date_time_chooser_android.cc (right):

https://codereview.chromium.org/23623019/diff/461001/content/browser/android/...
content/browser/android/date_time_chooser_android.cc:87:
"org/chromium/content/browser/input/DateTimeSuggestion");
+bulach could you double check this? I know we normally try and avoid looking up
classes by name like this, but I don't know of a better pattern for creating an
object array.

[palmer, 2013-12-02 20:53:37]

LGTM. Thanks!

[keishi, 2013-12-03 03:21:25]

https://codereview.chromium.org/23623019/diff/461001/content/public/common/da...
File content/public/common/date_time_suggestion.h (right):

https://codereview.chromium.org/23623019/diff/461001/content/public/common/da...
content/public/common/date_time_suggestion.h:1: // Copyright 2013 The Chromium
Authors. All rights reserved.
On 2013/12/02 17:27:22, jam wrote:
> why is this in content/public? Per
> http://www.chromium.org/developers/content-module/content-api, stuff should
only
> go into content/public if embedders of content need it. If this is only used
> inside content, it should be in content/common

I moved this file to content/common but including third_party/WebKit was
prohibited so I had to remove the constructor using
blink::WebDateTimeSuggestion.

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

https://codereview.chromium.org/23623019/diff/461001/content/renderer/rendere...
content/renderer/renderer_date_time_picker.cc:19: static ui::TextInputType
toTextInputType(int type) {
On 2013/12/02 17:27:22, jam wrote:
> nit: per style guide, this needs to be ToTextInputType

Done.

This chunk shouldn't be in this CL. I made a mistake with the diff base.
This chunk is in this prerequisite CL. Please review it too, jam@
https://codereview.chromium.org/85643002/

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

I defer to jam@

[bulach, 2013-12-03 14:22:28]

thanks keishi! I have a few nits and comments below, most important ones about
the CharIterator and a small cleanup in the JNI usage:

https://codereview.chromium.org/23623019/diff/501001/content/browser/android/...
File content/browser/android/date_time_chooser_android.cc (right):

https://codereview.chromium.org/23623019/diff/501001/content/browser/android/...
content/browser/android/date_time_chooser_android.cc:28: string16
SanitizeSuggestionString(string16 string) {
nit: parameter as const&?

https://codereview.chromium.org/23623019/diff/501001/content/browser/android/...
content/browser/android/date_time_chooser_android.cc:30:
std::remove_if(string.begin(), string.end(), &IsNonPrintableChar),
shouldn't this be using base::i18n::UTF16CharIterator instead?
if not, nit: wrong indent :)

https://codereview.chromium.org/23623019/diff/501001/content/browser/android/...
content/browser/android/date_time_chooser_android.cc:87:
"org/chromium/content/browser/input/DateTimeSuggestion");
we try as much as possible to use GetClass, NewObjectArray, etc... directly...

this will be *much* clearer as:

ScopedJavaLocalRef<jobject> j_suggestions =
Java_DateTimeChooserAndroid_createSuggestionsArray(env, suggestions.size);
...
then in 97, rather than env->SetObjectArrayElement, call:


Java_DateTimeChooserAndroid_setDateTimeSuggestionAt(env,
   j_suggestions, i, suggestion.value, localized_value.obj(), label.obj());

https://codereview.chromium.org/23623019/diff/501001/content/browser/android/...
content/browser/android/date_time_chooser_android.cc:93:
ScopedJavaLocalRef<jstring> localizedValue = ConvertUTF16ToJavaString(
nit: localized_value

https://codereview.chromium.org/23623019/diff/501001/content/browser/android/...
File content/browser/android/date_time_chooser_android.h (right):

https://codereview.chromium.org/23623019/diff/501001/content/browser/android/...
content/browser/android/date_time_chooser_android.h:37:
std::vector<DateTimeSuggestion> suggestions);
nit: pass as const&?

https://codereview.chromium.org/23623019/diff/501001/content/common/date_time...
File content/common/date_time_suggestion.h (right):

https://codereview.chromium.org/23623019/diff/501001/content/common/date_time...
content/common/date_time_suggestion.h:22: // The value to suggest.
nit: these comments are fairly redundant :)

https://codereview.chromium.org/23623019/diff/501001/content/common/date_time...
content/common/date_time_suggestion.h:25: base::string16 localizedValue;
nit: localized_value

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

https://codereview.chromium.org/23623019/diff/501001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/DateTimeChooserAndroid.java:67:
private static DateTimeSuggestion createDateTimeSuggestion(
as above, have two methods:
1) private static DateTimeSuggestion[] createDateTimeSuggestionsArray
2) private static void setDateTimeSuggestionAt(DateTimeSuggestion[] array,
double value, String localizedValue, String label)

it'll simplify the implementation and remove all env-> and GetClass..

https://codereview.chromium.org/23623019/diff/501001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/input/DateTimeSuggestion.java
(right):

https://codereview.chromium.org/23623019/diff/501001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/DateTimeSuggestion.java:11:
public class DateTimeSuggestion {
nit: can this be package private?

https://codereview.chromium.org/23623019/diff/501001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/DateTimeSuggestion.java:12:
public final double value;
nit: I think we normally use private fields and getters for accessors.

https://codereview.chromium.org/23623019/diff/501001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/input/DateTimeSuggestionListAdapter.java
(right):

https://codereview.chromium.org/23623019/diff/501001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/DateTimeSuggestionListAdapter.java:23:
public class DateTimeSuggestionListAdapter extends
ArrayAdapter<DateTimeSuggestion> {
nit: as above, perhaps package-private?

https://codereview.chromium.org/23623019/diff/501001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java
(right):

https://codereview.chromium.org/23623019/diff/501001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java:211:
void showPickerDialog(final int dialogType,
nit: wrong indent.

[keishi, 2013-12-03 16:52:33]

https://codereview.chromium.org/23623019/diff/501001/content/browser/android/...
File content/browser/android/date_time_chooser_android.cc (right):

https://codereview.chromium.org/23623019/diff/501001/content/browser/android/...
content/browser/android/date_time_chooser_android.cc:28: string16
SanitizeSuggestionString(string16 string) {
On 2013/12/03 14:22:29, bulach wrote:
> nit: parameter as const&?

Done.

https://codereview.chromium.org/23623019/diff/501001/content/browser/android/...
content/browser/android/date_time_chooser_android.cc:30:
std::remove_if(string.begin(), string.end(), &IsNonPrintableChar),
On 2013/12/03 14:22:29, bulach wrote:
> shouldn't this be using base::i18n::UTF16CharIterator instead?
> if not, nit: wrong indent :)

Done.

https://codereview.chromium.org/23623019/diff/501001/content/browser/android/...
content/browser/android/date_time_chooser_android.cc:87:
"org/chromium/content/browser/input/DateTimeSuggestion");
On 2013/12/03 14:22:29, bulach wrote:
> we try as much as possible to use GetClass, NewObjectArray, etc... directly...
> 
> this will be *much* clearer as:
> 
> ScopedJavaLocalRef<jobject> j_suggestions =
> Java_DateTimeChooserAndroid_createSuggestionsArray(env, suggestions.size);
> ...
> then in 97, rather than env->SetObjectArrayElement, call:
> 
> 
> Java_DateTimeChooserAndroid_setDateTimeSuggestionAt(env,
>    j_suggestions, i, suggestion.value, localized_value.obj(), label.obj());

Done.

https://codereview.chromium.org/23623019/diff/501001/content/browser/android/...
content/browser/android/date_time_chooser_android.cc:93:
ScopedJavaLocalRef<jstring> localizedValue = ConvertUTF16ToJavaString(
On 2013/12/03 14:22:29, bulach wrote:
> nit: localized_value

Done.

https://codereview.chromium.org/23623019/diff/501001/content/browser/android/...
File content/browser/android/date_time_chooser_android.h (right):

https://codereview.chromium.org/23623019/diff/501001/content/browser/android/...
content/browser/android/date_time_chooser_android.h:37:
std::vector<DateTimeSuggestion> suggestions);
On 2013/12/03 14:22:29, bulach wrote:
> nit: pass as const&?

Done.

https://codereview.chromium.org/23623019/diff/501001/content/common/date_time...
File content/common/date_time_suggestion.h (right):

https://codereview.chromium.org/23623019/diff/501001/content/common/date_time...
content/common/date_time_suggestion.h:22: // The value to suggest.
On 2013/12/03 14:22:29, bulach wrote:
> nit: these comments are fairly redundant :)

Done. Added more details.

https://codereview.chromium.org/23623019/diff/501001/content/common/date_time...
content/common/date_time_suggestion.h:25: base::string16 localizedValue;
On 2013/12/03 14:22:29, bulach wrote:
> nit: localized_value

Done.

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

https://codereview.chromium.org/23623019/diff/501001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/DateTimeChooserAndroid.java:67:
private static DateTimeSuggestion createDateTimeSuggestion(
On 2013/12/03 14:22:29, bulach wrote:
> as above, have two methods:
> 1) private static DateTimeSuggestion[] createDateTimeSuggestionsArray
> 2) private static void setDateTimeSuggestionAt(DateTimeSuggestion[] array,
> double value, String localizedValue, String label)
> 
> it'll simplify the implementation and remove all env-> and GetClass..

Done.

https://codereview.chromium.org/23623019/diff/501001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/input/DateTimeSuggestion.java
(right):

https://codereview.chromium.org/23623019/diff/501001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/DateTimeSuggestion.java:11:
public class DateTimeSuggestion {
On 2013/12/03 14:22:29, bulach wrote:
> nit: can this be package private?

Done.

https://codereview.chromium.org/23623019/diff/501001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/DateTimeSuggestion.java:12:
public final double value;
On 2013/12/03 14:22:29, bulach wrote:
> nit: I think we normally use private fields and getters for accessors.

Done.

https://codereview.chromium.org/23623019/diff/501001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/input/DateTimeSuggestionListAdapter.java
(right):

https://codereview.chromium.org/23623019/diff/501001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/DateTimeSuggestionListAdapter.java:23:
public class DateTimeSuggestionListAdapter extends
ArrayAdapter<DateTimeSuggestion> {
On 2013/12/03 14:22:29, bulach wrote:
> nit: as above, perhaps package-private?

Done.

https://codereview.chromium.org/23623019/diff/501001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java
(right):

https://codereview.chromium.org/23623019/diff/501001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java:211:
void showPickerDialog(final int dialogType,
On 2013/12/03 14:22:29, bulach wrote:
> nit: wrong indent.

Done.

[jam, 2013-12-03 22:24:49]

https://codereview.chromium.org/23623019/diff/461001/content/public/common/da...
File content/public/common/date_time_suggestion.h (right):

https://codereview.chromium.org/23623019/diff/461001/content/public/common/da...
content/public/common/date_time_suggestion.h:1: // Copyright 2013 The Chromium
Authors. All rights reserved.
On 2013/12/03 03:21:25, keishi1 wrote:
> On 2013/12/02 17:27:22, jam wrote:
> > why is this in content/public? Per
> > http://www.chromium.org/developers/content-module/content-api, stuff should
> only
> > go into content/public if embedders of content need it. If this is only used
> > inside content, it should be in content/common
> 
> I moved this file to content/common but including third_party/WebKit was
> prohibited so I had to remove the constructor using
> blink::WebDateTimeSuggestion.

this is because we can't include blink in content\common (or chrome\common),
since that code is linked into chrome.dll which doesn't include blink code (that
goes in chrome_child.dll). Instead, we put these blink constructors/factory
methods in content/renderer

https://codereview.chromium.org/23623019/diff/521001/content/common/date_time...
File content/common/date_time_suggestion.h (right):

https://codereview.chromium.org/23623019/diff/521001/content/common/date_time...
content/common/date_time_suggestion.h:19: 
nit: blank line

[keishi, 2013-12-04 02:26:15]

https://codereview.chromium.org/23623019/diff/461001/content/public/common/da...
File content/public/common/date_time_suggestion.h (right):

https://codereview.chromium.org/23623019/diff/461001/content/public/common/da...
content/public/common/date_time_suggestion.h:1: // Copyright 2013 The Chromium
Authors. All rights reserved.
On 2013/12/03 22:24:50, jam wrote:
> On 2013/12/03 03:21:25, keishi1 wrote:
> > On 2013/12/02 17:27:22, jam wrote:
> > > why is this in content/public? Per
> > > http://www.chromium.org/developers/content-module/content-api, stuff
should
> > only
> > > go into content/public if embedders of content need it. If this is only
used
> > > inside content, it should be in content/common
> > 
> > I moved this file to content/common but including third_party/WebKit was
> > prohibited so I had to remove the constructor using
> > blink::WebDateTimeSuggestion.
> 
> this is because we can't include blink in content\common (or chrome\common),
> since that code is linked into chrome.dll which doesn't include blink code
(that
> goes in chrome_child.dll). Instead, we put these blink constructors/factory
> methods in content/renderer

Done.

https://codereview.chromium.org/23623019/diff/521001/content/common/date_time...
File content/common/date_time_suggestion.h (right):

https://codereview.chromium.org/23623019/diff/521001/content/common/date_time...
content/common/date_time_suggestion.h:19: 
On 2013/12/03 22:24:51, jam wrote:
> nit: blank line

Done.

[jam, 2013-12-04 06:25:52]

https://codereview.chromium.org/23623019/diff/551001/content/common/date_time...
File content/common/date_time_suggestion.h (right):

https://codereview.chromium.org/23623019/diff/551001/content/common/date_time...
content/common/date_time_suggestion.h:25: explicit DateTimeSuggestion(const
blink::WebDateTimeSuggestion& suggestion);
this shouldn't be declared in content/common and defined in content/renderer as
it won't link (i.e. chrome.dll which includes this wouldn't find the method
definition). as an example, see content/renderer/*_builder.*.

so you can have content\renderer\date_time_suggestion_builder.h/cc

[bulach, 2013-12-04 10:37:53]

lgtm for the android bits % nits, and of course, jam's comments are also
necessary :) if it ends up requiring changes in android, please ping me again.

https://codereview.chromium.org/23623019/diff/551001/content/browser/android/...
File content/browser/android/date_time_chooser_android.cc (right):

https://codereview.chromium.org/23623019/diff/551001/content/browser/android/...
content/browser/android/date_time_chooser_android.cc:34: }
while(sanitized_iterator.Advance());
nit: space after "while"

https://codereview.chromium.org/23623019/diff/551001/content/browser/android/...
content/browser/android/date_time_chooser_android.cc:90:
Java_DateTimeChooserAndroid_createSuggestionsArray(env,
nit: indent + 2

https://codereview.chromium.org/23623019/diff/551001/content/common/date_time...
File content/common/date_time_suggestion.h (right):

https://codereview.chromium.org/23623019/diff/551001/content/common/date_time...
content/common/date_time_suggestion.h:16: }
nit:  // namespace blink

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

https://codereview.chromium.org/23623019/diff/551001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/DateTimeChooserAndroid.java:61:
@CalledByNative
nit: unindent

https://codereview.chromium.org/23623019/diff/551001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/DateTimeChooserAndroid.java:67:
* @param array ColorSuggestion array that should get a new suggestion added.
nit: s/Color/DateTime/ :)

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

https://codereview.chromium.org/23623019/diff/551001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/DateTimePickerDialog.java:66:
mMinTimeMillis = (long) min;
should we instead change these members' types to be double?

https://codereview.chromium.org/23623019/diff/551001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/input/DateTimeSuggestion.java
(right):

https://codereview.chromium.org/23623019/diff/551001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/DateTimeSuggestion.java:22:
public DateTimeSuggestion(double value, String localizedValue, String label) {
nit: I think this can be package private as well..

https://codereview.chromium.org/23623019/diff/551001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java
(right):

https://codereview.chromium.org/23623019/diff/551001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java:226:
(long) min, (long) max);
nit: as above, perhaps best to change this method to use double instead of this
cast?

https://codereview.chromium.org/23623019/diff/551001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java:375:
cal.clear();
nit: unindent

[keishi, 2013-12-04 12:21:23]

https://codereview.chromium.org/23623019/diff/551001/content/browser/android/...
File content/browser/android/date_time_chooser_android.cc (right):

https://codereview.chromium.org/23623019/diff/551001/content/browser/android/...
content/browser/android/date_time_chooser_android.cc:34: }
while(sanitized_iterator.Advance());
On 2013/12/04 10:37:54, bulach wrote:
> nit: space after "while"

Done.

https://codereview.chromium.org/23623019/diff/551001/content/browser/android/...
content/browser/android/date_time_chooser_android.cc:90:
Java_DateTimeChooserAndroid_createSuggestionsArray(env,
On 2013/12/04 10:37:54, bulach wrote:
> nit: indent + 2

Done.

https://codereview.chromium.org/23623019/diff/551001/content/common/date_time...
File content/common/date_time_suggestion.h (right):

https://codereview.chromium.org/23623019/diff/551001/content/common/date_time...
content/common/date_time_suggestion.h:16: }
On 2013/12/04 10:37:54, bulach wrote:
> nit:  // namespace blink

Done.

https://codereview.chromium.org/23623019/diff/551001/content/common/date_time...
content/common/date_time_suggestion.h:25: explicit DateTimeSuggestion(const
blink::WebDateTimeSuggestion& suggestion);
On 2013/12/04 06:25:53, jam wrote:
> this shouldn't be declared in content/common and defined in content/renderer
as
> it won't link (i.e. chrome.dll which includes this wouldn't find the method
> definition). as an example, see content/renderer/*_builder.*.
> 
> so you can have content\renderer\date_time_suggestion_builder.h/cc

Done.

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

https://codereview.chromium.org/23623019/diff/551001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/DateTimeChooserAndroid.java:61:
@CalledByNative
On 2013/12/04 10:37:54, bulach wrote:
> nit: unindent

Done.

https://codereview.chromium.org/23623019/diff/551001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/DateTimeChooserAndroid.java:67:
* @param array ColorSuggestion array that should get a new suggestion added.
On 2013/12/04 10:37:54, bulach wrote:
> nit: s/Color/DateTime/ :)

Done.

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

https://codereview.chromium.org/23623019/diff/551001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/DateTimePickerDialog.java:66:
mMinTimeMillis = (long) min;
On 2013/12/04 10:37:54, bulach wrote:
> should we instead change these members' types to be double?

I think we should keep it as long.

Time and Calendar return number of milliseconds as long.
min/max are double because they may mean different things depending on type,
but mMinTimeMillis/mMaxTimeMillis always represents the number of milliseconds.

https://codereview.chromium.org/23623019/diff/551001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/input/DateTimeSuggestion.java
(right):

https://codereview.chromium.org/23623019/diff/551001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/DateTimeSuggestion.java:22:
public DateTimeSuggestion(double value, String localizedValue, String label) {
On 2013/12/04 10:37:54, bulach wrote:
> nit: I think this can be package private as well..

Done.

https://codereview.chromium.org/23623019/diff/551001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java
(right):

https://codereview.chromium.org/23623019/diff/551001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java:226:
(long) min, (long) max);
On 2013/12/04 10:37:54, bulach wrote:
> nit: as above, perhaps best to change this method to use double instead of
this
> cast?

DateDialogNormalizer will only take min/max as number if milliseconds so I will
keep it a long.
I changed the parameter names of DateDialogNormalizer to minMillis/maxMillis to
emphasize that.

https://codereview.chromium.org/23623019/diff/551001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/InputDialogContainer.java:375:
cal.clear();
On 2013/12/04 10:37:54, bulach wrote:
> nit: unindent

Done.

[kenrb, 2013-12-04 14:55:09]

lgtm

[jam, 2013-12-04 18:06:53]

lgtm

[commit-bot: I haz the power, 2013-12-04 23:19:46]

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

[commit-bot: I haz the power, 2013-12-04 23:41:01]

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-12-05 01:43:35]

primiano@ could you review R.java? Thanks

[keishi, 2013-12-05 14:34:59]

Adding torne@ and benm@ to review R.java, just in case anyone is on vacation.

[benm (inactive), 2013-12-05 17:44:23]

R.java lgtm

[keishi, 2013-12-05 17:49:25]

On 2013/12/05 17:44:23, benm wrote:
> R.java lgtm

Thanks!

[commit-bot: I haz the power, 2013-12-05 17:50:27]

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

[commit-bot: I haz the power, 2013-12-05 18:59:10]

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

[newt (away), 2013-12-05 19:29:49]

AOSP is failing to compile. Looks like you need to update
ui/android/java/resource_map/org/chromium/ui/R.java

[keishi, 2013-12-06 00:51:00]

On 2013/12/05 19:29:49, newt wrote:
> AOSP is failing to compile. Looks like you need to update
> ui/android/java/resource_map/org/chromium/ui/R.java

Thanks!

[commit-bot: I haz the power, 2013-12-06 01:51:43]

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

[commit-bot: I haz the power, 2013-12-06 05:48:23]

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

[commit-bot: I haz the power, 2013-12-06 06:11:42]

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

[commit-bot: I haz the power, 2013-12-06 12:57:58]

Commit queue failed due to new patchset.

[commit-bot: I haz the power, 2013-12-06 13:01:37]

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

[commit-bot: I haz the power, 2013-12-06 14:09:55]

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

[commit-bot: I haz the power, 2013-12-06 14:38:40]

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

[Torne, 2013-12-06 14:46:29]

On 2013/12/06 14:38:40, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/keishi%40chromium.org/23623019/661001

Unfortunately android_aosp trybots are currently in a bad state (I have emailed
troopers and filed an infra bug) and you may not be able to get a green build at
present. If this CL isn't urgent I suggest waiting a while until they have been
repaired; landing this CL without a green android_aosp build wouldn't be ideal
for us :/

[commit-bot: I haz the power, 2013-12-06 14:59:55]

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

[commit-bot: I haz the power, 2013-12-06 23:29:09]

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

[commit-bot: I haz the power, 2013-12-07 02:56:19]

Change committed as 239290