Move Android Date/Time parsing to the renderer (C++ and ICU) instead of the current parsing that ha…

Move Android Date/Time parsing to the renderer (C++ and ICU) instead of the current parsing that happens in the browser (on java using java libraries)


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

[Miguel Garcia, 2013-02-04 19:17:45]

During the review of https://chromiumcodereview.appspot.com/11783088 Chris
requested that we would not send strings via IPC but instead send some bounded
format of the date/time representation. A bit delayed but the change is finally
here, all the parsing now happens on the renderer and everything in the browser
(Java and native) operates with the safely parsed values.

The linux bot is failing because it cannot link the parser int the unittest I
did. I don't understand why this happens (it should work and it does in fact
link fine locally) but I would appreciate some feedback while I look int it.

[palmer, 2013-02-04 22:14:08]

https://codereview.chromium.org/12191005/diff/12001/content/renderer/date_tim...
File content/renderer/date_time_formatter.cc (right):

https://codereview.chromium.org/12191005/diff/12001/content/renderer/date_tim...
content/renderer/date_time_formatter.cc:82: ui::TextInputType
DateTimeFormatter::GetType() {
Efficiency: This can be const too, right?

https://codereview.chromium.org/12191005/diff/12001/content/renderer/date_tim...
content/renderer/date_time_formatter.cc:86: std::string
DateTimeFormatter::GetFormattedValue() {
Could also be const? Does it have to return a copy?

https://codereview.chromium.org/12191005/diff/12001/content/renderer/date_tim...
content/renderer/date_time_formatter.cc:94: return "";
But what if I want to actually represent New Year's Day of year 0 as a string?
:) Rare, probably, but... E.g. libc asctime returns "Sun Jan  0 00:00:00 1900".

https://codereview.chromium.org/12191005/diff/12001/content/renderer/date_tim...
content/renderer/date_time_formatter.cc:182: void DateTimeFormatter::ClearAll()
{
Yeah, it seems we have a semipredicate problem here. You're using all zeroes
means "no value", but it could also be a legitimate date-time.

https://codereview.chromium.org/12191005/diff/12001/content/renderer/date_tim...
File content/renderer/date_time_formatter.h (right):

https://codereview.chromium.org/12191005/diff/12001/content/renderer/date_tim...
content/renderer/date_time_formatter.h:50: std::map<ui::TextInputType,
icu::UnicodeString> pattern_map_;
Efficiency: This could be a static array instead of a dynamic map, since
ui::TextInputType is a simple enum.

[jam, 2013-02-05 00:08:26]

can you point me to what i'm supposed to review? if just gypi, lgtm

[Miguel Garcia, 2013-02-05 11:48:41]

On 2013/02/05 00:08:26, jam wrote:
> can you point me to what i'm supposed to review? if just gypi, lgtm

Thanks jam@ do you mind rubber stamping the small change in web_contents_impl as
well?

[bulach, 2013-02-05 11:54:55]

thanks miguel! some drive by comments and style nits, the android side looks
fine once chris and jam are happy with the rest of the implementation.

https://codereview.chromium.org/12191005/diff/12001/content/browser/web_conte...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/12191005/diff/12001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.h:35: struct
ViewHostMsg_DateTimeValue_Params;
nit: sort order

https://codereview.chromium.org/12191005/diff/12001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.h:561: void
OnOpenDateTimeDialog(ViewHostMsg_DateTimeValue_Params value);
nit: const &

https://codereview.chromium.org/12191005/diff/12001/content/common/view_messa...
File content/common/view_messages.h (right):

https://codereview.chromium.org/12191005/diff/12001/content/common/view_messa...
content/common/view_messages.h:361:
IPC_STRUCT_BEGIN(ViewHostMsg_DateTimeValue_Params)
nit: maybe DateTimeDialogValue

https://codereview.chromium.org/12191005/diff/12001/content/common/view_messa...
content/common/view_messages.h:368: IPC_STRUCT_MEMBER(int, dialog_type)
nit: maybe start with dialog_type then date? ..if we ever need to add
milliseconds..

https://codereview.chromium.org/12191005/diff/12001/content/renderer/date_tim...
File content/renderer/date_time_formatter.cc (right):

https://codereview.chromium.org/12191005/diff/12001/content/renderer/date_tim...
content/renderer/date_time_formatter.cc:24:
pattern_map_[ui::TEXT_INPUT_TYPE_NONE] = "";
nit: unindent this block, but chris suggestion is probably better..

https://codereview.chromium.org/12191005/diff/12001/content/renderer/date_tim...
content/renderer/date_time_formatter.cc:29: const
WebKit::WebDateTimeChooserParams& source):
nit: ":" goes on the next line

https://codereview.chromium.org/12191005/diff/12001/content/renderer/date_tim...
content/renderer/date_time_formatter.cc:42: int year, int month, int day, int
hour, int minute, int second) :
nit: ":" goes into the next line

https://codereview.chromium.org/12191005/diff/12001/content/renderer/date_tim...
content/renderer/date_time_formatter.cc:51: pattern_ =
&pattern_map_[(ui::TextInputType) type_];
nit: static_cast rather than c-style cast

https://codereview.chromium.org/12191005/diff/12001/content/renderer/date_tim...
content/renderer/date_time_formatter.cc:95: } else {
nit: no need for else, could remove and unindent this block

https://codereview.chromium.org/12191005/diff/12001/content/renderer/date_tim...
content/renderer/date_time_formatter.cc:105: if (success <= U_ZERO_ERROR) {
nit: remove {} here and 108 below

https://codereview.chromium.org/12191005/diff/12001/content/renderer/date_tim...
content/renderer/date_time_formatter.cc:164: if (pattern_map_.count(type_) > 0)
{
nit: pattern_map_.find(type) != pattern_map_.end()
but shouldn't it return false if we don't know the type?
if so, maybe shortcut:

if (pattern_map_.find(type) == pattern_map_.end())
  return false;

https://codereview.chromium.org/12191005/diff/12001/content/renderer/date_tim...
File content/renderer/date_time_formatter.h (right):

https://codereview.chromium.org/12191005/diff/12001/content/renderer/date_tim...
content/renderer/date_time_formatter.h:24: class DateTimeFormatter {
maybe TextInputDateTimeFormatter would be more appropriate? this is not a
generic DateTimeFormatter.

https://codereview.chromium.org/12191005/diff/12001/content/renderer/date_tim...
content/renderer/date_time_formatter.h:26: DateTimeFormatter(const
WebKit::WebDateTimeChooserParams& source);
nit: explicit

https://codereview.chromium.org/12191005/diff/12001/content/renderer/date_tim...
content/renderer/date_time_formatter.h:27: DateTimeFormatter(ui::TextInputType
type,
nit: first param on the next line

https://codereview.chromium.org/12191005/diff/12001/content/renderer/date_tim...
File content/renderer/date_time_formatter_unittest.cc (right):

https://codereview.chromium.org/12191005/diff/12001/content/renderer/date_tim...
content/renderer/date_time_formatter_unittest.cc:16: class
RendererDateTimePickerTest : public testing::Test {
I think it can be just TEST(RendererDateTimePickerTest, ...) and no need for
TEST_F with this harness..

https://codereview.chromium.org/12191005/diff/12001/content/renderer/date_tim...
content/renderer/date_time_formatter_unittest.cc:24: ASSERT_EQ(2010,
sut.GetYear());
I think EXPECT_EQ is preferred instead of ASSERT_EQ (which throws a fatal
failure)

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

https://codereview.chromium.org/12191005/diff/12001/content/renderer/renderer...
content/renderer/renderer_date_time_picker.cc:63: DateTimeFormatter
formatter((ui::TextInputType) value.dialog_type,
nit: static_cast, also move the first param to the next line indented +4, along
with all other params

[jam, 2013-02-05 15:52:02]

lgtm

[Miguel Garcia, 2013-02-05 17:44:40]

Thanks you all for the quick review.

https://codereview.chromium.org/12191005/diff/12001/content/browser/web_conte...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/12191005/diff/12001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.h:35: struct
ViewHostMsg_DateTimeValue_Params;
On 2013/02/05 11:54:56, bulach wrote:
> nit: sort order

Done.

https://codereview.chromium.org/12191005/diff/12001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.h:561: void
OnOpenDateTimeDialog(ViewHostMsg_DateTimeValue_Params value);
On 2013/02/05 11:54:56, bulach wrote:
> nit: const &

Done.

https://codereview.chromium.org/12191005/diff/12001/content/common/view_messa...
File content/common/view_messages.h (right):

https://codereview.chromium.org/12191005/diff/12001/content/common/view_messa...
content/common/view_messages.h:361:
IPC_STRUCT_BEGIN(ViewHostMsg_DateTimeValue_Params)
On 2013/02/05 11:54:56, bulach wrote:
> nit: maybe DateTimeDialogValue

Done.

https://codereview.chromium.org/12191005/diff/12001/content/common/view_messa...
content/common/view_messages.h:368: IPC_STRUCT_MEMBER(int, dialog_type)
On 2013/02/05 11:54:56, bulach wrote:
> nit: maybe start with dialog_type then date? ..if we ever need to add
> milliseconds..

Done.

https://codereview.chromium.org/12191005/diff/12001/content/renderer/date_tim...
File content/renderer/date_time_formatter.cc (right):

https://codereview.chromium.org/12191005/diff/12001/content/renderer/date_tim...
content/renderer/date_time_formatter.cc:24:
pattern_map_[ui::TEXT_INPUT_TYPE_NONE] = "";
Yeah I went ahead with that.

On 2013/02/05 11:54:56, bulach wrote:
> nit: unindent this block, but chris suggestion is probably better..

https://codereview.chromium.org/12191005/diff/12001/content/renderer/date_tim...
content/renderer/date_time_formatter.cc:29: const
WebKit::WebDateTimeChooserParams& source):
On 2013/02/05 11:54:56, bulach wrote:
> nit: ":" goes on the next line

Done.

https://codereview.chromium.org/12191005/diff/12001/content/renderer/date_tim...
content/renderer/date_time_formatter.cc:42: int year, int month, int day, int
hour, int minute, int second) :
On 2013/02/05 11:54:56, bulach wrote:
> nit: ":" goes into the next line

Done.

https://codereview.chromium.org/12191005/diff/12001/content/renderer/date_tim...
content/renderer/date_time_formatter.cc:51: pattern_ =
&pattern_map_[(ui::TextInputType) type_];
On 2013/02/05 11:54:56, bulach wrote:
> nit: static_cast rather than c-style cast

Done.

https://codereview.chromium.org/12191005/diff/12001/content/renderer/date_tim...
content/renderer/date_time_formatter.cc:82: ui::TextInputType
DateTimeFormatter::GetType() {
On 2013/02/04 22:14:08, Chris P. wrote:
> Efficiency: This can be const too, right?

right

https://codereview.chromium.org/12191005/diff/12001/content/renderer/date_tim...
content/renderer/date_time_formatter.cc:86: std::string
DateTimeFormatter::GetFormattedValue() {
On 2013/02/04 22:14:08, Chris P. wrote:
> Could also be const? Does it have to return a copy?

Done.

https://codereview.chromium.org/12191005/diff/12001/content/renderer/date_tim...
content/renderer/date_time_formatter.cc:94: return "";
The spec prevents 0 based values so I think there is no confusion here. The
desktop version also prevents this case from happening. I will add a test to
make sure this is not sneaked in the future.

http://www.w3.org/TR/html-markup/input.date.html#input.date.attrs.type

"A valid full-date as defined in [RFC 3339], with the additional qualification
that the year component is four or more digits representing a number greater
than 0."


You can try the desktop version here
http://www.w3schools.com/html/tryit.asp?filename=tryhtml5_input_type_date


On 2013/02/04 22:14:08, Chris P. wrote:
> But what if I want to actually represent New Year's Day of year 0 as a string?
> :) Rare, probably, but... E.g. libc asctime returns "Sun Jan  0 00:00:00
1900".

https://codereview.chromium.org/12191005/diff/12001/content/renderer/date_tim...
content/renderer/date_time_formatter.cc:95: } else {
On 2013/02/05 11:54:56, bulach wrote:
> nit: no need for else, could remove and unindent this block

Done.

https://codereview.chromium.org/12191005/diff/12001/content/renderer/date_tim...
content/renderer/date_time_formatter.cc:105: if (success <= U_ZERO_ERROR) {
On 2013/02/05 11:54:56, bulach wrote:
> nit: remove {} here and 108 below

Done.

https://codereview.chromium.org/12191005/diff/12001/content/renderer/date_tim...
content/renderer/date_time_formatter.cc:164: if (pattern_map_.count(type_) > 0)
{
Since I switched to an array I now just checked the boundaries.

On 2013/02/05 11:54:56, bulach wrote:
> nit: pattern_map_.find(type) != pattern_map_.end()
> but shouldn't it return false if we don't know the type?
> if so, maybe shortcut:
> 
> if (pattern_map_.find(type) == pattern_map_.end())
>   return false;

https://codereview.chromium.org/12191005/diff/12001/content/renderer/date_tim...
content/renderer/date_time_formatter.cc:182: void DateTimeFormatter::ClearAll()
{
See above, I am open to suggestions if you still feel that 0 based values could
be a problem

On 2013/02/04 22:14:08, Chris P. wrote:
> Yeah, it seems we have a semipredicate problem here. You're using all zeroes
> means "no 

value", but it could also be a legitimate date-time.

https://codereview.chromium.org/12191005/diff/12001/content/renderer/date_tim...
File content/renderer/date_time_formatter.h (right):

https://codereview.chromium.org/12191005/diff/12001/content/renderer/date_tim...
content/renderer/date_time_formatter.h:24: class DateTimeFormatter {
I'll hold the renaming until the review is complete if that's ok? That way we
can preserve all comments in the same file.

On 2013/02/05 11:54:56, bulach wrote:
> maybe TextInputDateTimeFormatter would be more appropriate? this is not a
> generic DateTimeFormatter.

https://codereview.chromium.org/12191005/diff/12001/content/renderer/date_tim...
content/renderer/date_time_formatter.h:26: DateTimeFormatter(const
WebKit::WebDateTimeChooserParams& source);
On 2013/02/05 11:54:56, bulach wrote:
> nit: explicit

Done.

https://codereview.chromium.org/12191005/diff/12001/content/renderer/date_tim...
content/renderer/date_time_formatter.h:27: DateTimeFormatter(ui::TextInputType
type,
On 2013/02/05 11:54:56, bulach wrote:
> nit: first param on the next line

Done.

https://codereview.chromium.org/12191005/diff/12001/content/renderer/date_tim...
content/renderer/date_time_formatter.h:50: std::map<ui::TextInputType,
icu::UnicodeString> pattern_map_;
This is a pretty good point. I changed it to an array now, I also added some
checks for the size since with the dynamic map it'd be ok to access weird values
of the enum. Also added some extra tests to that respect.

On 2013/02/04 22:14:08, Chris P. wrote:
> Efficiency: This could be a static array instead of a dynamic map, since
> ui::TextInputType is a simple enum.

https://codereview.chromium.org/12191005/diff/12001/content/renderer/date_tim...
File content/renderer/date_time_formatter_unittest.cc (right):

https://codereview.chromium.org/12191005/diff/12001/content/renderer/date_tim...
content/renderer/date_time_formatter_unittest.cc:16: class
RendererDateTimePickerTest : public testing::Test {
Indeed

On 2013/02/05 11:54:56, bulach wrote:
> I think it can be just TEST(RendererDateTimePickerTest, ...) and no need for
> TEST_F with this harness..

https://codereview.chromium.org/12191005/diff/12001/content/renderer/date_tim...
content/renderer/date_time_formatter_unittest.cc:24: ASSERT_EQ(2010,
sut.GetYear());
On 2013/02/05 11:54:56, bulach wrote:
> I think EXPECT_EQ is preferred instead of ASSERT_EQ (which throws a fatal
> failure)

Done.

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

https://codereview.chromium.org/12191005/diff/12001/content/renderer/renderer...
content/renderer/renderer_date_time_picker.cc:63: DateTimeFormatter
formatter((ui::TextInputType) value.dialog_type,
On 2013/02/05 11:54:56, bulach wrote:
> nit: static_cast, also move the first param to the next line indented +4,
along
> with all other params

Done.

[palmer, 2013-02-05 19:57:51]

Starting to L pretty G. When you hammer down the last few nails, I should
approve it very soon. If I don't, nag me on chat.

https://codereview.chromium.org/12191005/diff/22002/content/renderer/date_tim...
File content/renderer/date_time_formatter.cc (right):

https://codereview.chromium.org/12191005/diff/22002/content/renderer/date_tim...
content/renderer/date_time_formatter.cc:16: 
Nit: Get rid of this blank line.

https://codereview.chromium.org/12191005/diff/22002/content/renderer/date_tim...
content/renderer/date_time_formatter.cc:29: 
Nit: Get rid of this blank line.

https://codereview.chromium.org/12191005/diff/22002/content/renderer/date_tim...
content/renderer/date_time_formatter.cc:54: pattern_ =
&patterns_[static_cast<ui::TextInputType>(type_)];
Why do you cast it to the type that it already is?

Also, it's not safe. If someone passes 10000 for type, we will actually offset
patterns_ by that silly number. You should assert at runtime that type_ is
within the range of valid TextInputTypes before doing the array index.

https://codereview.chromium.org/12191005/diff/22002/content/renderer/date_tim...
content/renderer/date_time_formatter.cc:166: if (type_ > 0 && type_ <=
ui::TEXT_INPUT_TYPE_MAX) {
Yes, exactly — do this check above in DateTimeFormatter::DateTimeFormatter, too.

https://codereview.chromium.org/12191005/diff/22002/content/renderer/date_tim...
File content/renderer/date_time_formatter.h (right):

https://codereview.chromium.org/12191005/diff/22002/content/renderer/date_tim...
content/renderer/date_time_formatter.h:8: #include <map>
You no longer need to include this.

https://codereview.chromium.org/12191005/diff/22002/content/renderer/date_tim...
content/renderer/date_time_formatter.h:22: // a set of
year/month/day/hour/minute/second and viceversa.
Nit: "vice versa" is two words. :)

[Miguel Garcia, 2013-02-06 11:39:44]

https://codereview.chromium.org/12191005/diff/22002/content/renderer/date_tim...
File content/renderer/date_time_formatter.cc (right):

https://codereview.chromium.org/12191005/diff/22002/content/renderer/date_tim...
content/renderer/date_time_formatter.cc:16: 
On 2013/02/05 19:57:51, Chris P. wrote:
> Nit: Get rid of this blank line.

Done.

https://codereview.chromium.org/12191005/diff/22002/content/renderer/date_tim...
content/renderer/date_time_formatter.cc:29: 
On 2013/02/05 19:57:51, Chris P. wrote:
> Nit: Get rid of this blank line.

Done.

https://codereview.chromium.org/12191005/diff/22002/content/renderer/date_tim...
content/renderer/date_time_formatter.cc:54: pattern_ =
&patterns_[static_cast<ui::TextInputType>(type_)];
On 2013/02/05 19:57:51, Chris P. wrote:
> Why do you cast it to the type that it already is?
> 
> Also, it's not safe. If someone passes 10000 for type, we will actually offset
> patterns_ by that silly number. You should assert at runtime that type_ is
> within the range of valid TextInputTypes before doing the array index.

Done, added a test as well to check this case.

https://codereview.chromium.org/12191005/diff/22002/content/renderer/date_tim...
content/renderer/date_time_formatter.cc:166: if (type_ > 0 && type_ <=
ui::TEXT_INPUT_TYPE_MAX) {
On 2013/02/05 19:57:51, Chris P. wrote:
> Yes, exactly — do this check above in DateTimeFormatter::DateTimeFormatter,
too.

Done.

https://codereview.chromium.org/12191005/diff/22002/content/renderer/date_tim...
File content/renderer/date_time_formatter.h (right):

https://codereview.chromium.org/12191005/diff/22002/content/renderer/date_tim...
content/renderer/date_time_formatter.h:8: #include <map>
On 2013/02/05 19:57:51, Chris P. wrote:
> You no longer need to include this.

Done.

https://codereview.chromium.org/12191005/diff/22002/content/renderer/date_tim...
content/renderer/date_time_formatter.h:22: // a set of
year/month/day/hour/minute/second and viceversa.
On 2013/02/05 19:57:51, Chris P. wrote:
> Nit: "vice versa" is two words. :)

Done.

[Miguel Garcia, 2013-02-06 15:17:00]

Bots failing because LKGR is behind, please disregard

On 2013/02/06 11:39:44, Miguel Garcia wrote:
>
https://codereview.chromium.org/12191005/diff/22002/content/renderer/date_tim...
> File content/renderer/date_time_formatter.cc (right):
> 
>
https://codereview.chromium.org/12191005/diff/22002/content/renderer/date_tim...
> content/renderer/date_time_formatter.cc:16: 
> On 2013/02/05 19:57:51, Chris P. wrote:
> > Nit: Get rid of this blank line.
> 
> Done.
> 
>
https://codereview.chromium.org/12191005/diff/22002/content/renderer/date_tim...
> content/renderer/date_time_formatter.cc:29: 
> On 2013/02/05 19:57:51, Chris P. wrote:
> > Nit: Get rid of this blank line.
> 
> Done.
> 
>
https://codereview.chromium.org/12191005/diff/22002/content/renderer/date_tim...
> content/renderer/date_time_formatter.cc:54: pattern_ =
> &patterns_[static_cast<ui::TextInputType>(type_)];
> On 2013/02/05 19:57:51, Chris P. wrote:
> > Why do you cast it to the type that it already is?
> > 
> > Also, it's not safe. If someone passes 10000 for type, we will actually
offset
> > patterns_ by that silly number. You should assert at runtime that type_ is
> > within the range of valid TextInputTypes before doing the array index.
> 
> Done, added a test as well to check this case.
> 
>
https://codereview.chromium.org/12191005/diff/22002/content/renderer/date_tim...
> content/renderer/date_time_formatter.cc:166: if (type_ > 0 && type_ <=
> ui::TEXT_INPUT_TYPE_MAX) {
> On 2013/02/05 19:57:51, Chris P. wrote:
> > Yes, exactly — do this check above in DateTimeFormatter::DateTimeFormatter,
> too.
> 
> Done.
> 
>
https://codereview.chromium.org/12191005/diff/22002/content/renderer/date_tim...
> File content/renderer/date_time_formatter.h (right):
> 
>
https://codereview.chromium.org/12191005/diff/22002/content/renderer/date_tim...
> content/renderer/date_time_formatter.h:8: #include <map>
> On 2013/02/05 19:57:51, Chris P. wrote:
> > You no longer need to include this.
> 
> Done.
> 
>
https://codereview.chromium.org/12191005/diff/22002/content/renderer/date_tim...
> content/renderer/date_time_formatter.h:22: // a set of
> year/month/day/hour/minute/second and viceversa.
> On 2013/02/05 19:57:51, Chris P. wrote:
> > Nit: "vice versa" is two words. :)
> 
> Done.

[Miguel Garcia, 2013-02-07 10:55:32]

https://codereview.chromium.org/12191005/diff/12001/content/renderer/date_tim...
File content/renderer/date_time_formatter.h (right):

https://codereview.chromium.org/12191005/diff/12001/content/renderer/date_tim...
content/renderer/date_time_formatter.h:24: class DateTimeFormatter {
Actually Date inputs are not text anymore. I can go with InputDateTimeFormatter
if you prefer but honestly speaking it seems I could just leave it as is...

On 2013/02/05 17:44:40, Miguel Garcia wrote:
> I'll hold the renaming until the review is complete if that's ok? That way we
> can preserve all comments in the same file.
> 
> On 2013/02/05 11:54:56, bulach wrote:
> > maybe TextInputDateTimeFormatter would be more appropriate? this is not a
> > generic DateTimeFormatter.
>

[Miguel Garcia, 2013-02-07 14:36:27]

Friendly ping

On 2013/02/07 10:55:32, Miguel Garcia wrote:
>
https://codereview.chromium.org/12191005/diff/12001/content/renderer/date_tim...
> File content/renderer/date_time_formatter.h (right):
> 
>
https://codereview.chromium.org/12191005/diff/12001/content/renderer/date_tim...
> content/renderer/date_time_formatter.h:24: class DateTimeFormatter {
> Actually Date inputs are not text anymore. I can go with
InputDateTimeFormatter
> if you prefer but honestly speaking it seems I could just leave it as is...
> 
> On 2013/02/05 17:44:40, Miguel Garcia wrote:
> > I'll hold the renaming until the review is complete if that's ok? That way
we
> > can preserve all comments in the same file.
> > 
> > On 2013/02/05 11:54:56, bulach wrote:
> > > maybe TextInputDateTimeFormatter would be more appropriate? this is not a
> > > generic DateTimeFormatter.
> >

[bulach, 2013-02-07 14:44:26]

android lgtm

yeah, I can see your argument here.. it's still not a generic DateTimeFormatter
as it's tightly coupled with the webkit types, but then again, this is under
renderer/... if chris is happy, keep as is.

[palmer, 2013-02-07 19:10:58]

LGTM. Thanks!

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

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

[commit-bot: I haz the power, 2013-02-07 21:14:09]

Change committed as 181364

[joth, 2013-02-07 21:49:42]

https://chromiumcodereview.appspot.com/12191005/diff/28001/content/renderer/d...
File content/renderer/date_time_formatter.h (right):

https://chromiumcodereview.appspot.com/12191005/diff/28001/content/renderer/d...
content/renderer/date_time_formatter.h:38: const std::string&
GetFormattedValue() const;
this needs to #include <string>