Use WebCore:DateTimeChooser for date/time form types instead of considering them text fields.

Use WebCore:DateTimeChooser for date/time form types instead of considering them text fields.


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

[Miguel Garcia, 2012-12-03 19:44:41]

This cl is built on top of https://codereview.chromium.org/11421173/ but I
thought I'd send an early review before that cl is in.

[bulach, 2012-12-03 20:23:02]

thanks! just some drive-by comments:

https://codereview.chromium.org/11418295/diff/1/content/renderer/render_widge...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/11418295/diff/1/content/renderer/render_widge...
content/renderer/render_widget.cc:1726: static bool
IsDialogType(ui::TextInputType type) {
is this going to hold for all platforms?
I mean, "DialogType" is an implementation detail...
perhaps: "HasTextInputState" would be clearer? also, does it need to have a
OS_ANDROID guard?

https://codereview.chromium.org/11418295/diff/1/content/renderer/render_widge...
content/renderer/render_widget.cc:1742: return; // not handled as text anymore
nit: remove the "anymore", won't make much sense in the future ;)

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

https://codereview.chromium.org/11418295/diff/1/content/renderer/renderer_dat...
content/renderer/renderer_date_time_picker_impl.cc:57: if (chooser_completion_
!= NULL) {
nit: like 51, s/!= NULL//

also, both here and above, remove { } for one line blocks..

https://codereview.chromium.org/11418295/diff/1/content/renderer/renderer_dat...
content/renderer/renderer_date_time_picker_impl.cc:70: static const WebString
date = "date";
nargh! :) can't have non-POD statics, they're nasty..

https://codereview.chromium.org/11418295/diff/1/content/renderer/renderer_dat...
content/renderer/renderer_date_time_picker_impl.cc:88: return
ui::TEXT_INPUT_TYPE_WEEK;
nit: wrong indentation on all "return" clauses

https://codereview.chromium.org/11418295/diff/1/content/renderer/renderer_dat...
File content/renderer/renderer_date_time_picker_impl.h (right):

https://codereview.chromium.org/11418295/diff/1/content/renderer/renderer_dat...
content/renderer/renderer_date_time_picker_impl.h:15: }
nit:  // namespace WebKit

https://codereview.chromium.org/11418295/diff/1/content/renderer/renderer_dat...
content/renderer/renderer_date_time_picker_impl.h:31: 
nit: remove extra \n

https://codereview.chromium.org/11418295/diff/1/content/renderer/renderer_dat...
content/renderer/renderer_date_time_picker_impl.h:32: void
OnReplaceDateTime(string16 new_date);
nit: const string16& ?

https://codereview.chromium.org/11418295/diff/1/content/renderer/renderer_dat...
content/renderer/renderer_date_time_picker_impl.h:42:
WebKit::WebDateTimeChooserCompletion* chooser_completion_;  // Not owned by us
nit: \n before DISALLOW_...

https://codereview.chromium.org/11418295/diff/1/content/renderer/renderer_dat...
content/renderer/renderer_date_time_picker_impl.h:46: }  // namespace content
nit: \n after this

[Miguel Garcia, 2012-12-04 10:40:43]

Thanks for the review!

https://codereview.chromium.org/11418295/diff/1/content/renderer/render_widge...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/11418295/diff/1/content/renderer/render_widge...
content/renderer/render_widget.cc:1726: static bool
IsDialogType(ui::TextInputType type) {
I renamed it to IsDateTimeInput. We should really split the date/time releated
stuff from ime but not in this CL. I added a TODO when initializing IME about
it.

About the guard.I am just following the same pattern as with
UpdateTextInputState I can move both inside an android guard if you prefer.


On 2012/12/03 20:23:03, bulach wrote:
> is this going to hold for all platforms?
> I mean, "DialogType" is an implementation detail...
> perhaps: "HasTextInputState" would be clearer? also, does it need to have a
> OS_ANDROID guard?

https://codereview.chromium.org/11418295/diff/1/content/renderer/render_widge...
content/renderer/render_widget.cc:1742: return; // not handled as text anymore
On 2012/12/03 20:23:03, bulach wrote:
> nit: remove the "anymore", won't make much sense in the future ;)

Done.

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

https://codereview.chromium.org/11418295/diff/1/content/renderer/renderer_dat...
content/renderer/renderer_date_time_picker_impl.cc:57: if (chooser_completion_
!= NULL) {
On 2012/12/03 20:23:03, bulach wrote:
> nit: like 51, s/!= NULL//
> 
> also, both here and above, remove { } for one line blocks..

Done.

https://codereview.chromium.org/11418295/diff/1/content/renderer/renderer_dat...
content/renderer/renderer_date_time_picker_impl.cc:70: static const WebString
date = "date";
On 2012/12/03 20:23:03, bulach wrote:
> nargh! :) can't have non-POD statics, they're nasty..

Argh sorry about that. I'll replace this 100% before submitting once the webkit
changes makes it through, that's why I did it in the first place :)

https://codereview.chromium.org/11418295/diff/1/content/renderer/renderer_dat...
content/renderer/renderer_date_time_picker_impl.cc:88: return
ui::TEXT_INPUT_TYPE_WEEK;
On 2012/12/03 20:23:03, bulach wrote:
> nit: wrong indentation on all "return" clauses

Done.

https://codereview.chromium.org/11418295/diff/1/content/renderer/renderer_dat...
File content/renderer/renderer_date_time_picker_impl.h (right):

https://codereview.chromium.org/11418295/diff/1/content/renderer/renderer_dat...
content/renderer/renderer_date_time_picker_impl.h:15: }
On 2012/12/03 20:23:03, bulach wrote:
> nit:  // namespace WebKit

Done.

https://codereview.chromium.org/11418295/diff/1/content/renderer/renderer_dat...
content/renderer/renderer_date_time_picker_impl.h:31: 
On 2012/12/03 20:23:03, bulach wrote:
> nit: remove extra \n

Done.

https://codereview.chromium.org/11418295/diff/1/content/renderer/renderer_dat...
content/renderer/renderer_date_time_picker_impl.h:32: void
OnReplaceDateTime(string16 new_date);
On 2012/12/03 20:23:03, bulach wrote:
> nit: const string16& ?

Good point

https://codereview.chromium.org/11418295/diff/1/content/renderer/renderer_dat...
content/renderer/renderer_date_time_picker_impl.h:42:
WebKit::WebDateTimeChooserCompletion* chooser_completion_;  // Not owned by us
On 2012/12/03 20:23:03, bulach wrote:
> nit: \n before DISALLOW_...

Done.

https://codereview.chromium.org/11418295/diff/1/content/renderer/renderer_dat...
content/renderer/renderer_date_time_picker_impl.h:46: }  // namespace content
On 2012/12/03 20:23:03, bulach wrote:
> nit: \n after this

Done.

[Miguel Garcia, 2012-12-04 11:20:13]

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

https://codereview.chromium.org/11418295/diff/1/content/renderer/renderer_dat...
content/renderer/renderer_date_time_picker_impl.cc:70: static const WebString
date = "date";
yay! The WebKit change landed so I am using an enum value now.


On 2012/12/04 10:40:43, Miguel Garcia wrote:
> On 2012/12/03 20:23:03, bulach wrote:
> > nargh! :) can't have non-POD statics, they're nasty..
> 
> Argh sorry about that. I'll replace this 100% before submitting once the
webkit
> changes makes it through, that's why I did it in the first place :)

[Peter Beverloo, 2012-12-05 11:57:03]

Hi Miguel, thanks for the patch! I've left comments inline.

https://codereview.chromium.org/11418295/diff/7001/content/browser/renderer_h...
File content/browser/renderer_host/ime_adapter_android.cc (right):

https://codereview.chromium.org/11418295/diff/7001/content/browser/renderer_h...
content/browser/renderer_host/ime_adapter_android.cc:68: // possibilities. As
the entry point is now different we can actually
Phrasing this as "note that WebKit is removing the date and time related input
elements from the enum, since they are no longer considered to be textual input
fields" would be clearer.

https://codereview.chromium.org/11418295/diff/7001/content/browser/renderer_h...
content/browser/renderer_host/ime_adapter_android.cc:69: // tell the IME adapter
if it's a dialog type or not without having to
nit: s/or not//

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

https://codereview.chromium.org/11418295/diff/7001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/DateTimePickerDialog.java:129:
// The default behavior of dialogs changed after JellyBean. In JB
nit: s/after/in/

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

https://codereview.chromium.org/11418295/diff/7001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/InputDialogContainer.java:73:
private boolean mShouldSendCancelation;
nit: \n after this as the coment doesn't apply to mDialog and
mInputActionDelegate.

https://codereview.chromium.org/11418295/diff/7001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/InputDialogContainer.java:143:
@Override
nit: This is indented with 12 spaces for no apparent reason, maybe fix it as
you're touching the code?

https://codereview.chromium.org/11418295/diff/7001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/InputDialogContainer.java:262:
// no more than one callback is sent to the native side.
nit: Maybe phrase as "Prevents more than one callback being sent to the native
side when the dialog triggers multiple events."?

https://codereview.chromium.org/11418295/diff/7001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/MonthPickerDialog.java
(right):

https://codereview.chromium.org/11418295/diff/7001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/MonthPickerDialog.java:102:
// The default behavior of dialogs changed after JellyBean. In JB
nit: s/after/in/.

https://codereview.chromium.org/11418295/diff/7001/content/renderer/render_vi...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/11418295/diff/7001/content/renderer/render_vi...
content/renderer/render_view_impl.h:502: // MULTIPLE FIELDS
nit: s/android/Android/. I'd clarify what MULTIPLE FIELDS means, maybe "Only
used on Android. All other platforms implement date and time input fields using
multiple fields."?

https://codereview.chromium.org/11418295/diff/7001/content/renderer/render_vi...
content/renderer/render_view_impl.h:1447: // A date/time picker object for input
type=date and similar.
s/for input type=date and similar/for date and time related input elements/.

https://codereview.chromium.org/11418295/diff/7001/content/renderer/render_vi...
content/renderer/render_view_impl.h:1449: #endif
No indenting for the #endif.

https://codereview.chromium.org/11418295/diff/7001/content/renderer/render_wi...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/11418295/diff/7001/content/renderer/render_wi...
content/renderer/render_widget.cc:1743: return; // not handled as text in WebKit
"not considered as a text input field in WebKit."?

I'm a bit wary about having this early return here. We shouldn't be calling
UpdateTextInputState at all for non-text input fields.

https://codereview.chromium.org/11418295/diff/7001/content/renderer/renderer_...
File content/renderer/renderer_date_time_picker_impl.cc (right):

https://codereview.chromium.org/11418295/diff/7001/content/renderer/renderer_...
content/renderer/renderer_date_time_picker_impl.cc:61: ui::TextInputType
RendererDateTimePickerImpl::ExtractType(
This isn't actually defined as static in the class definition. Generally we put
helper methods such as this in an anonymous namespace at the top of the file.

https://codereview.chromium.org/11418295/diff/7001/content/renderer/renderer_...
File content/renderer/renderer_date_time_picker_impl.h (right):

https://codereview.chromium.org/11418295/diff/7001/content/renderer/renderer_...
content/renderer/renderer_date_time_picker_impl.h:20: class
RendererDateTimePickerImpl : public RenderViewObserver {
I don't think this should be *Impl, as it doesn't actually implement an abstract
class called RendererDateTimePicker.
Since this class talks directly to WebKit, maybe
RendererDateTimePickerDispatcher would be more appropriate? Most classes which
work like this actually implement a Client from the WebKit namespace, but there
is none..

[Miguel Garcia, 2012-12-05 16:23:16]

https://codereview.chromium.org/11418295/diff/7001/content/browser/renderer_h...
File content/browser/renderer_host/ime_adapter_android.cc (right):

https://codereview.chromium.org/11418295/diff/7001/content/browser/renderer_h...
content/browser/renderer_host/ime_adapter_android.cc:68: // possibilities. As
the entry point is now different we can actually
On 2012/12/05 11:57:03, Peter Beverloo wrote:
> Phrasing this as "note that WebKit is removing the date and time related input
> elements from the enum, since they are no longer considered to be textual
input
> fields" would be clearer.

Done.

https://codereview.chromium.org/11418295/diff/7001/content/browser/renderer_h...
content/browser/renderer_host/ime_adapter_android.cc:69: // tell the IME adapter
if it's a dialog type or not without having to
On 2012/12/05 11:57:03, Peter Beverloo wrote:
> nit: s/or not//

Done.

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

https://codereview.chromium.org/11418295/diff/7001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/DateTimePickerDialog.java:129:
// The default behavior of dialogs changed after JellyBean. In JB
On 2012/12/05 11:57:03, Peter Beverloo wrote:
> nit: s/after/in/

Done.

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

https://codereview.chromium.org/11418295/diff/7001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/InputDialogContainer.java:73:
private boolean mShouldSendCancelation;
On 2012/12/05 11:57:03, Peter Beverloo wrote:
> nit: \n after this as the coment doesn't apply to mDialog and
> mInputActionDelegate.

Done.

https://codereview.chromium.org/11418295/diff/7001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/InputDialogContainer.java:143:
@Override
On 2012/12/05 11:57:03, Peter Beverloo wrote:
> nit: This is indented with 12 spaces for no apparent reason, maybe fix it as
> you're touching the code?

Done.

https://codereview.chromium.org/11418295/diff/7001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/InputDialogContainer.java:262:
// no more than one callback is sent to the native side.
On 2012/12/05 11:57:03, Peter Beverloo wrote:
> nit: Maybe phrase as "Prevents more than one callback being sent to the native
> side when the dialog triggers multiple events."?

Done.

https://codereview.chromium.org/11418295/diff/7001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/MonthPickerDialog.java
(right):

https://codereview.chromium.org/11418295/diff/7001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/MonthPickerDialog.java:102:
// The default behavior of dialogs changed after JellyBean. In JB
On 2012/12/05 11:57:03, Peter Beverloo wrote:
> nit: s/after/in/.

Done.

https://codereview.chromium.org/11418295/diff/7001/content/renderer/render_vi...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/11418295/diff/7001/content/renderer/render_vi...
content/renderer/render_view_impl.h:502: // MULTIPLE FIELDS
On 2012/12/05 11:57:03, Peter Beverloo wrote:
> nit: s/android/Android/. I'd clarify what MULTIPLE FIELDS means, maybe "Only
> used on Android. All other platforms implement date and time input fields
using
> multiple fields."?

Done.

https://codereview.chromium.org/11418295/diff/7001/content/renderer/render_vi...
content/renderer/render_view_impl.h:1447: // A date/time picker object for input
type=date and similar.
On 2012/12/05 11:57:03, Peter Beverloo wrote:
> s/for input type=date and similar/for date and time related input elements/.

Done.

https://codereview.chromium.org/11418295/diff/7001/content/renderer/render_vi...
content/renderer/render_view_impl.h:1449: #endif
On 2012/12/05 11:57:03, Peter Beverloo wrote:
> No indenting for the #endif.

Done.

https://codereview.chromium.org/11418295/diff/7001/content/renderer/render_wi...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/11418295/diff/7001/content/renderer/render_wi...
content/renderer/render_widget.cc:1743: return; // not handled as text in WebKit
This is called directly from WebKit (WebViewImpl.cpp) I think it's worth keeping
it for now until we fully split dialogs and text based inputs. 

On 2012/12/05 11:57:03, Peter Beverloo wrote:
> "not considered as a text input field in WebKit."?
> 
> I'm a bit wary about having this early return here. We shouldn't be calling
> UpdateTextInputState at all for non-text input fields.

https://codereview.chromium.org/11418295/diff/7001/content/renderer/renderer_...
File content/renderer/renderer_date_time_picker_impl.cc (right):

https://codereview.chromium.org/11418295/diff/7001/content/renderer/renderer_...
content/renderer/renderer_date_time_picker_impl.cc:61: ui::TextInputType
RendererDateTimePickerImpl::ExtractType(
On 2012/12/05 11:57:03, Peter Beverloo wrote:
> This isn't actually defined as static in the class definition. Generally we
put
> helper methods such as this in an anonymous namespace at the top of the file.

Done.

https://codereview.chromium.org/11418295/diff/7001/content/renderer/renderer_...
File content/renderer/renderer_date_time_picker_impl.h (right):

https://codereview.chromium.org/11418295/diff/7001/content/renderer/renderer_...
content/renderer/renderer_date_time_picker_impl.h:20: class
RendererDateTimePickerImpl : public RenderViewObserver {
I wanted to ask Kent about that, this forces us to keep the client on the
renderer, the color picker and the file picker implement a client on WebKit and
I am not sure why this is any different.

I am renaming the file/class in the meantime to renderer_date_time_picker.{cc|h}
and RendererDateTimePicker (i.e. removing the Impl suffix). I think this hides
implementation details that would not be so hidden if I add the Dispatcher
suffix

On 2012/12/05 11:57:03, Peter Beverloo wrote:
> I don't think this should be *Impl, as it doesn't actually implement an
abstract
> class called RendererDateTimePicker.
> Since this class talks directly to WebKit, maybe
> RendererDateTimePickerDispatcher would be more appropriate? Most classes which
> work like this actually implement a Client from the WebKit namespace, but
there
> is none..

https://codereview.chromium.org/11418295/diff/7001/content/renderer/renderer_...
content/renderer/renderer_date_time_picker_impl.h:35: ui::TextInputType
ExtractType(const WebKit::WebDateTimeChooserParams&);
this can actually be implemented as as static function inside the cpp file so I
am removing it from here.

[Miguel Garcia, 2012-12-05 16:29:03]

It did not feel right to reuse the old IPCs for this so I added a new one and
renamed another in the end.

That caused a few more dependencies so adding some new OWNERS

avi@ for content
jschuh@ for security review of the new messages
darin@ for a question about the interaction with WebKit

Kent, Darin I noticed that most of the WebKit dialogs (the color picker, the
file picker...) implement some sort of client that is owned on the WebKit side.
This picker however does not do that (just provides a callback object) which
means that we need to keep a similar client on the chromium renderer side. I was
wondering if it'd be ok to make this consistent on WebKit (I could do that on a
separate cl on the WebKit side and then redo this bit)

[Avi (use Gerrit), 2012-12-05 16:37:00]

While this looks plausible, I'm not an Android guy and have no idea how this
stuff is supposed to be wired. I'll be happy to owner stamp, but I'll need to
see an Android expert give this his/her OK.

https://codereview.chromium.org/11418295/diff/17001/content/renderer/render_v...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/11418295/diff/17001/content/renderer/render_v...
content/renderer/render_view_impl.h:503: // MULTIPLE FIELDS
Is "MULTIPLE FIELDS" a code thing that you're capitalizing it like that?
All-caps emphasis looks weird.

[Miguel Garcia, 2012-12-05 17:02:42]

Thanks Avi, Marcus will be approving all the android specific pieces (he is
owner on all the android directories) your stamp is only needed for
content_renderer.gypi

https://codereview.chromium.org/11418295/diff/17001/content/renderer/render_v...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/11418295/diff/17001/content/renderer/render_v...
content/renderer/render_view_impl.h:503: // MULTIPLE FIELDS
Yes, that's how the WebKit feature is called.

On 2012/12/05 16:37:01, Avi wrote:
> Is "MULTIPLE FIELDS" a code thing that you're capitalizing it like that?
> All-caps emphasis looks weird.

[Avi (use Gerrit), 2012-12-05 17:52:20]

content_renderer.gypi lgtm

[jschuh, 2012-12-05 22:54:34]

ipc lgtm

[tkent, 2012-12-06 07:26:43]

On 2012/12/05 16:29:03, Miguel Garcia wrote:
> Kent, Darin I noticed that most of the WebKit dialogs (the color picker, the
> file picker...) implement some sort of client that is owned on the WebKit
side.
> This picker however does not do that (just provides a callback object) which
> means that we need to keep a similar client on the chromium renderer side. I
was
> wondering if it'd be ok to make this consistent on WebKit (I could do that on
a
> separate cl on the WebKit side and then redo this bit)

WebViewClient::openDateTimeChooser has the same design as
WebViewClient::runFileChooser.

Anyway, if you have an idea to make it better, it's ok to change
WebViewClient::openDateChooser.  openDateTimeChooser is used only in Android
port, and please feel free to change it for Android.

[Miguel Garcia, 2012-12-06 14:01:04]

Thanks Kent, I will have a look at that post M25 probably.

On 2012/12/06 07:26:43, Kent Tamura wrote:
> On 2012/12/05 16:29:03, Miguel Garcia wrote:
> > Kent, Darin I noticed that most of the WebKit dialogs (the color picker, the
> > file picker...) implement some sort of client that is owned on the WebKit
> side.
> > This picker however does not do that (just provides a callback object) which
> > means that we need to keep a similar client on the chromium renderer side. I
> was
> > wondering if it'd be ok to make this consistent on WebKit (I could do that
on
> a
> > separate cl on the WebKit side and then redo this bit)
> 
> WebViewClient::openDateTimeChooser has the same design as
> WebViewClient::runFileChooser.
> 
> Anyway, if you have an idea to make it better, it's ok to change
> WebViewClient::openDateChooser.  openDateTimeChooser is used only in Android
> port, and please feel free to change it for Android.

[bulach, 2012-12-07 18:10:13]

not an owner everywhere, but a few comments and suggestions below. sorry about
the delay!

https://codereview.chromium.org/11418295/diff/25001/content/browser/renderer_...
File content/browser/renderer_host/ime_adapter_android.cc (right):

https://codereview.chromium.org/11418295/diff/25001/content/browser/renderer_...
content/browser/renderer_host/ime_adapter_android.cc:66: // TODO(miguelg) note
that WebKit is removing the date time related
nits: TODO(miguelg):

also: "is removing" will make very little sense for those reading it.. :)
perhaps a reference to a bug number, and something more direct, like:

TODO(miguelg): remove date time related enums after bugs.webkit.org/foobar.

https://codereview.chromium.org/11418295/diff/25001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/11418295/diff/25001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_android.cc:277: //
TODO(miguelg) this currently dispatches messages for text inputs
nit: add a ":"

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

https://codereview.chromium.org/11418295/diff/25001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/DateTimePickerDialog.java:129:
// The default behavior of dialogs changed after JellyBean. After JB
nit: isn't it "in" rather than "after" ?

https://codereview.chromium.org/11418295/diff/25001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ImeAdapter.java
(right):

https://codereview.chromium.org/11418295/diff/25001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ImeAdapter.java:166:
nativeReplaceDateTime(mNativeImeAdapterAndroid, text);
q: the native could potentially call this back up and change the value, right?
probably best to call the embedder first to commit in the java side, then call
native?

https://codereview.chromium.org/11418295/diff/25001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/MonthPickerDialog.java
(right):

https://codereview.chromium.org/11418295/diff/25001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/MonthPickerDialog.java:102:
// The default behavior of dialogs changed after JellyBean. After JB
ditto

https://codereview.chromium.org/11418295/diff/25001/content/renderer/render_w...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/11418295/diff/25001/content/renderer/render_w...
content/renderer/render_widget.cc:1729: || type == ui::TEXT_INPUT_TYPE_DATE_TIME
nit: || should be at EOL

https://codereview.chromium.org/11418295/diff/25001/content/renderer/render_w...
content/renderer/render_widget.cc:1743: return; // not considered as a text
input field in WebKit
nit: in chromium

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

https://codereview.chromium.org/11418295/diff/25001/content/renderer/renderer...
content/renderer/renderer_date_time_picker.cc:53: Send(new
ViewHostMsg_TextInputStateChanged(routing_id(), p));
since it's changing this anyways, why not go ahead and introduce a new message
for this?

https://codereview.chromium.org/11418295/diff/25001/content/renderer/renderer...
File content/renderer/renderer_date_time_picker.h (right):

https://codereview.chromium.org/11418295/diff/25001/content/renderer/renderer...
content/renderer/renderer_date_time_picker.h:5: #ifndef
CONTENT_RENDERER_RENDERER_DATE_TIME_PICKER_IMPL_H_
nit: s/_IMPL//

https://codereview.chromium.org/11418295/diff/25001/content/renderer/renderer...
content/renderer/renderer_date_time_picker.h:24: const
WebKit::WebDateTimeChooserParams&,
nit: in webkit params are unnamed, in chromium they normally have a name..

https://codereview.chromium.org/11418295/diff/25001/content/renderer/renderer...
content/renderer/renderer_date_time_picker.h:37: const
WebKit::WebDateTimeChooserParams& chooser_params_;
is this expensive to copy? we either store pointers (scoped, if owned) or
values, rarely references..

https://codereview.chromium.org/11418295/diff/25001/content/renderer/renderer...
content/renderer/renderer_date_time_picker.h:38:
WebKit::WebDateTimeChooserCompletion* chooser_completion_;  // Not owned by us
if it was, it'd probably be a scoped_ptr :) no need for the comment.

[Miguel Garcia, 2012-12-07 19:04:34]

Thanks a lot for the review Marcus?
I think between you and Avi, everything is covered OWNERS wise, let me know if
you rather add someone else to have another look.

https://codereview.chromium.org/11418295/diff/25001/content/browser/renderer_...
File content/browser/renderer_host/ime_adapter_android.cc (right):

https://codereview.chromium.org/11418295/diff/25001/content/browser/renderer_...
content/browser/renderer_host/ime_adapter_android.cc:66: // TODO(miguelg) note
that WebKit is removing the date time related
On 2012/12/07 18:10:13, bulach wrote:
> nits: TODO(miguelg):
> 
> also: "is removing" will make very little sense for those reading it.. :)
> perhaps a reference to a bug number, and something more direct, like:
> 
> TODO(miguelg): remove date time related enums after bugs.webkit.org/foobar.

Done.

https://codereview.chromium.org/11418295/diff/25001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/11418295/diff/25001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_android.cc:277: //
TODO(miguelg) this currently dispatches messages for text inputs
On 2012/12/07 18:10:13, bulach wrote:
> nit: add a ":"

Done.

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

https://codereview.chromium.org/11418295/diff/25001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/DateTimePickerDialog.java:129:
// The default behavior of dialogs changed after JellyBean. After JB
On 2012/12/07 18:10:13, bulach wrote:
> nit: isn't it "in" rather than "after" ?

Well that's what I had initially, then Peter asked me to change it in Patch 2 :)
It's actually "In JB and onwards.." so I am going to go with that :)

https://codereview.chromium.org/11418295/diff/25001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ImeAdapter.java
(right):

https://codereview.chromium.org/11418295/diff/25001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ImeAdapter.java:166:
nativeReplaceDateTime(mNativeImeAdapterAndroid, text);
Makes sense, I kept the original order but this way is more resistent.

On 2012/12/07 18:10:13, bulach wrote:
> q: the native could potentially call this back up and change the value, right?
> probably best to call the embedder first to commit in the java side, then call
> native?

https://codereview.chromium.org/11418295/diff/25001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/MonthPickerDialog.java
(right):

https://codereview.chromium.org/11418295/diff/25001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/MonthPickerDialog.java:102:
// The default behavior of dialogs changed after JellyBean. After JB
On 2012/12/07 18:10:13, bulach wrote:
> ditto

Done.

https://codereview.chromium.org/11418295/diff/25001/content/renderer/render_w...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/11418295/diff/25001/content/renderer/render_w...
content/renderer/render_widget.cc:1729: || type == ui::TEXT_INPUT_TYPE_DATE_TIME
On 2012/12/07 18:10:13, bulach wrote:
> nit: || should be at EOL

Done.

https://codereview.chromium.org/11418295/diff/25001/content/renderer/render_w...
content/renderer/render_widget.cc:1743: return; // not considered as a text
input field in WebKit
well it's actually in both... I was meaning to say that this is a new
restriction imposed by WebKit. Will 
// not considered as a text input field in WebKit/chromium

work?

On 2012/12/07 18:10:13, bulach wrote:
> nit: in chromium

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

https://codereview.chromium.org/11418295/diff/25001/content/renderer/renderer...
content/renderer/renderer_date_time_picker.cc:53: Send(new
ViewHostMsg_TextInputStateChanged(routing_id(), p));
I thought about it but in order to make it work right now I would have to add
yet another handler to render_widget_host_impl.cc  which seems to have a ton
already.

I'd rather do this as as part of splitting dialogs and text from the ime_adapter
and build a new date_time_picker class on the browser side that sends the
message (this is what the color picker does for example). It'd be much more
convenient to do that change on a second cl since it's also going to be fairly
big and I rather not complicate this one more.


On 2012/12/07 18:10:13, bulach wrote:
> since it's changing this anyways, why not go ahead and introduce a new message
> for this?

https://codereview.chromium.org/11418295/diff/25001/content/renderer/renderer...
File content/renderer/renderer_date_time_picker.h (right):

https://codereview.chromium.org/11418295/diff/25001/content/renderer/renderer...
content/renderer/renderer_date_time_picker.h:5: #ifndef
CONTENT_RENDERER_RENDERER_DATE_TIME_PICKER_IMPL_H_
On 2012/12/07 18:10:13, bulach wrote:
> nit: s/_IMPL//

nice catch!

https://codereview.chromium.org/11418295/diff/25001/content/renderer/renderer...
content/renderer/renderer_date_time_picker.h:24: const
WebKit::WebDateTimeChooserParams&,
On 2012/12/07 18:10:13, bulach wrote:
> nit: in webkit params are unnamed, in chromium they normally have a name..

Done.

https://codereview.chromium.org/11418295/diff/25001/content/renderer/renderer...
content/renderer/renderer_date_time_picker.h:37: const
WebKit::WebDateTimeChooserParams& chooser_params_;
It's not. It is just a struct with a few integers. I rather use value then.

On 2012/12/07 18:10:13, bulach wrote:
> is this expensive to copy? we either store pointers (scoped, if owned) or
> values, rarely references..

https://codereview.chromium.org/11418295/diff/25001/content/renderer/renderer...
content/renderer/renderer_date_time_picker.h:38:
WebKit::WebDateTimeChooserCompletion* chooser_completion_;  // Not owned by us
On 2012/12/07 18:10:13, bulach wrote:
> if it was, it'd probably be a scoped_ptr :) no need for the comment. 
So if people see just a raw pointer with no comment will they assume
automatically it's not owned by us?

[Miguel Garcia, 2012-12-15 13:47:25]

Jay, do you think you can have a look at this? Marcus is OOO for a few days and
this is blocking a P1 for M25. I am on paternity leave but if you have a look I
am sure I can address any final concerns and submit.


On 2012/12/07 19:04:34, Miguel Garcia wrote:
> Thanks a lot for the review Marcus?
> I think between you and Avi, everything is covered OWNERS wise, let me know if
> you rather add someone else to have another look.
> 
>
https://codereview.chromium.org/11418295/diff/25001/content/browser/renderer_...
> File content/browser/renderer_host/ime_adapter_android.cc (right):
> 
>
https://codereview.chromium.org/11418295/diff/25001/content/browser/renderer_...
> content/browser/renderer_host/ime_adapter_android.cc:66: // TODO(miguelg) note
> that WebKit is removing the date time related
> On 2012/12/07 18:10:13, bulach wrote:
> > nits: TODO(miguelg):
> > 
> > also: "is removing" will make very little sense for those reading it.. :)
> > perhaps a reference to a bug number, and something more direct, like:
> > 
> > TODO(miguelg): remove date time related enums after bugs.webkit.org/foobar.
> 
> Done.
> 
>
https://codereview.chromium.org/11418295/diff/25001/content/browser/renderer_...
> File content/browser/renderer_host/render_widget_host_view_android.cc (right):
> 
>
https://codereview.chromium.org/11418295/diff/25001/content/browser/renderer_...
> content/browser/renderer_host/render_widget_host_view_android.cc:277: //
> TODO(miguelg) this currently dispatches messages for text inputs
> On 2012/12/07 18:10:13, bulach wrote:
> > nit: add a ":"
> 
> Done.
> 
>
https://codereview.chromium.org/11418295/diff/25001/content/public/android/ja...
> File
>
content/public/android/java/src/org/chromium/content/browser/DateTimePickerDialog.java
> (right):
> 
>
https://codereview.chromium.org/11418295/diff/25001/content/public/android/ja...
>
content/public/android/java/src/org/chromium/content/browser/DateTimePickerDialog.java:129:
> // The default behavior of dialogs changed after JellyBean. After JB
> On 2012/12/07 18:10:13, bulach wrote:
> > nit: isn't it "in" rather than "after" ?
> 
> Well that's what I had initially, then Peter asked me to change it in Patch 2
:)
> It's actually "In JB and onwards.." so I am going to go with that :)
> 
>
https://codereview.chromium.org/11418295/diff/25001/content/public/android/ja...
> File
> content/public/android/java/src/org/chromium/content/browser/ImeAdapter.java
> (right):
> 
>
https://codereview.chromium.org/11418295/diff/25001/content/public/android/ja...
>
content/public/android/java/src/org/chromium/content/browser/ImeAdapter.java:166:
> nativeReplaceDateTime(mNativeImeAdapterAndroid, text);
> Makes sense, I kept the original order but this way is more resistent.
> 
> On 2012/12/07 18:10:13, bulach wrote:
> > q: the native could potentially call this back up and change the value,
right?
> > probably best to call the embedder first to commit in the java side, then
call
> > native?
> 
>
https://codereview.chromium.org/11418295/diff/25001/content/public/android/ja...
> File
>
content/public/android/java/src/org/chromium/content/browser/MonthPickerDialog.java
> (right):
> 
>
https://codereview.chromium.org/11418295/diff/25001/content/public/android/ja...
>
content/public/android/java/src/org/chromium/content/browser/MonthPickerDialog.java:102:
> // The default behavior of dialogs changed after JellyBean. After JB
> On 2012/12/07 18:10:13, bulach wrote:
> > ditto
> 
> Done.
> 
>
https://codereview.chromium.org/11418295/diff/25001/content/renderer/render_w...
> File content/renderer/render_widget.cc (right):
> 
>
https://codereview.chromium.org/11418295/diff/25001/content/renderer/render_w...
> content/renderer/render_widget.cc:1729: || type ==
ui::TEXT_INPUT_TYPE_DATE_TIME
> On 2012/12/07 18:10:13, bulach wrote:
> > nit: || should be at EOL
> 
> Done.
> 
>
https://codereview.chromium.org/11418295/diff/25001/content/renderer/render_w...
> content/renderer/render_widget.cc:1743: return; // not considered as a text
> input field in WebKit
> well it's actually in both... I was meaning to say that this is a new
> restriction imposed by WebKit. Will 
> // not considered as a text input field in WebKit/chromium
> 
> work?
> 
> On 2012/12/07 18:10:13, bulach wrote:
> > nit: in chromium
> 
>
https://codereview.chromium.org/11418295/diff/25001/content/renderer/renderer...
> File content/renderer/renderer_date_time_picker.cc (right):
> 
>
https://codereview.chromium.org/11418295/diff/25001/content/renderer/renderer...
> content/renderer/renderer_date_time_picker.cc:53: Send(new
> ViewHostMsg_TextInputStateChanged(routing_id(), p));
> I thought about it but in order to make it work right now I would have to add
> yet another handler to render_widget_host_impl.cc  which seems to have a ton
> already.
> 
> I'd rather do this as as part of splitting dialogs and text from the
ime_adapter
> and build a new date_time_picker class on the browser side that sends the
> message (this is what the color picker does for example). It'd be much more
> convenient to do that change on a second cl since it's also going to be fairly
> big and I rather not complicate this one more.
> 
> 
> On 2012/12/07 18:10:13, bulach wrote:
> > since it's changing this anyways, why not go ahead and introduce a new
message
> > for this?
> 
>
https://codereview.chromium.org/11418295/diff/25001/content/renderer/renderer...
> File content/renderer/renderer_date_time_picker.h (right):
> 
>
https://codereview.chromium.org/11418295/diff/25001/content/renderer/renderer...
> content/renderer/renderer_date_time_picker.h:5: #ifndef
> CONTENT_RENDERER_RENDERER_DATE_TIME_PICKER_IMPL_H_
> On 2012/12/07 18:10:13, bulach wrote:
> > nit: s/_IMPL//
> 
> nice catch!
> 
>
https://codereview.chromium.org/11418295/diff/25001/content/renderer/renderer...
> content/renderer/renderer_date_time_picker.h:24: const
> WebKit::WebDateTimeChooserParams&,
> On 2012/12/07 18:10:13, bulach wrote:
> > nit: in webkit params are unnamed, in chromium they normally have a name..
> 
> Done.
> 
>
https://codereview.chromium.org/11418295/diff/25001/content/renderer/renderer...
> content/renderer/renderer_date_time_picker.h:37: const
> WebKit::WebDateTimeChooserParams& chooser_params_;
> It's not. It is just a struct with a few integers. I rather use value then.
> 
> On 2012/12/07 18:10:13, bulach wrote:
> > is this expensive to copy? we either store pointers (scoped, if owned) or
> > values, rarely references..
> 
>
https://codereview.chromium.org/11418295/diff/25001/content/renderer/renderer...
> content/renderer/renderer_date_time_picker.h:38:
> WebKit::WebDateTimeChooserCompletion* chooser_completion_;  // Not owned by us
> On 2012/12/07 18:10:13, bulach wrote:
> > if it was, it'd probably be a scoped_ptr :) no need for the comment. 
> So if people see just a raw pointer with no comment will they assume
> automatically it's not owned by us?

[Jay Civelli, 2012-12-17 17:01:07]

lgtm

https://codereview.chromium.org/11418295/diff/37003/content/renderer/render_w...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/11418295/diff/37003/content/renderer/render_w...
content/renderer/render_widget.cc:1743: return; // not considered as a text
input field in WebKit/Chromium.
Nit: 2 spaces before // and comment should start with a capital letter.

[Miguel Garcia, 2012-12-17 22:13:55]

https://codereview.chromium.org/11418295/diff/37003/content/renderer/render_w...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/11418295/diff/37003/content/renderer/render_w...
content/renderer/render_widget.cc:1743: return; // not considered as a text
input field in WebKit/Chromium.
On 2012/12/17 17:01:07, Jay Civelli wrote:
> Nit: 2 spaces before // and comment should start with a capital letter.

Done.

[commit-bot: I haz the power, 2012-12-17 22:24:05]

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

[commit-bot: I haz the power, 2012-12-18 00:58:50]

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

[commit-bot: I haz the power, 2012-12-18 01:50:02]

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

[commit-bot: I haz the power, 2012-12-18 06:55:29]

Change committed as 173655