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

content/renderer/date_time_formatter.cc

Index: content/renderer/date_time_formatter.cc
diff --git a/content/renderer/date_time_formatter.cc b/content/renderer/date_time_formatter.cc
new file mode 100644
index 0000000000000000000000000000000000000000..a98111062ecfb345f05917e45a7fcd6b4a318fc3
--- /dev/null
+++ b/content/renderer/date_time_formatter.cc
@@ -0,0 +1,191 @@
+// Copyright (c) 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "content/renderer/date_time_formatter.h"
+
+#include "base/string_util.h"
+#include "third_party/WebKit/Source/Platform/chromium/public/WebCString.h"
+#include "third_party/WebKit/Source/WebKit/chromium/public/WebDateTimeChooserParams.h"
+#include "third_party/icu/public/i18n/unicode/smpdtfmt.h"
+
+
+namespace content {
+
+void DateTimeFormatter::CreatePatternMap() {
+    pattern_map_[ui::TEXT_INPUT_TYPE_DATE] = "yyyy-MM-dd";
+    pattern_map_[ui::TEXT_INPUT_TYPE_DATE_TIME] =  "yyyy-MM-dd'T'HH:mm'Z'";
+    pattern_map_[ui::TEXT_INPUT_TYPE_DATE_TIME_LOCAL] =  "yyyy-MM-dd'T'HH:mm";
+    pattern_map_[ui::TEXT_INPUT_TYPE_MONTH] = "yyyy-MM";
+    pattern_map_[ui::TEXT_INPUT_TYPE_TIME] = "HH:mm";
+
+    // Not implemented
+    pattern_map_[ui::TEXT_INPUT_TYPE_WEEK] = "";
+    pattern_map_[ui::TEXT_INPUT_TYPE_NONE] = "";

[bulach, 2013/02/05 11:54:56]

nit: unindent this block, but chris suggestion is probably better..

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

Yeah I went ahead with that.

On 2013/02/05 11:54:56, bulach wrote:

+}
+
+
+DateTimeFormatter::DateTimeFormatter(
+    const WebKit::WebDateTimeChooserParams& source):

[bulach, 2013/02/05 11:54:56]

nit: ":" goes on the next line

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

Done.

+      formatted_string_(source.currentValue.utf8()) {
+  CreatePatternMap();
+  ExtractType(source);
+  if (!ParseValues()) {
+    type_ = ui::TEXT_INPUT_TYPE_NONE;
+    ClearAll();
+    LOG(WARNING) << "Problems parsing input <" << formatted_string_ << ">";
+  }
+}
+
+DateTimeFormatter::DateTimeFormatter(
+    ui::TextInputType type,
+    int year, int month, int day, int hour, int minute, int second) :

[bulach, 2013/02/05 11:54:56]

nit: ":" goes into the next line

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

Done.

+        type_(type),
+        year_(year),
+        month_(month),
+        day_(day),
+        hour_(hour),
+        minute_(minute),
+        second_(second) {
+  CreatePatternMap();
+  pattern_ = &pattern_map_[(ui::TextInputType) type_];

[bulach, 2013/02/05 11:54:56]

nit: static_cast rather than c-style cast

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

Done.

+  formatted_string_ = FormatString();
+}
+
+DateTimeFormatter::~DateTimeFormatter() {
+}
+
+int DateTimeFormatter::GetYear() const {
+  return year_;
+}
+
+int DateTimeFormatter::GetMonth() const {
+  return month_;
+}
+
+int DateTimeFormatter::GetDay() const {
+  return day_;
+}
+
+int DateTimeFormatter::GetHour() const {
+  return hour_;
+}
+
+int DateTimeFormatter::GetMinute() const {
+  return minute_;
+}
+
+int DateTimeFormatter::GetSecond() const {
+  return second_;
+}
+
+ui::TextInputType DateTimeFormatter::GetType() {

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

Efficiency: This can be const too, right?

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

right

+  return type_;
+}
+
+std::string DateTimeFormatter::GetFormattedValue() {

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

Could also be const? Does it have to return a copy?

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

Done.

+  return formatted_string_;
+}
+
+const std::string DateTimeFormatter::FormatString() const {
+  UErrorCode success = U_ZERO_ERROR;
+  if (year_ == 0 && month_ == 0 && day_ == 0 &&
+      hour_ == 0 && minute_ == 0 && second_ == 0) {
+    return "";

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

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".

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

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:

+  } else {

[bulach, 2013/02/05 11:54:56]

nit: no need for else, could remove and unindent this block

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

Done.

+    std::string result;
+    const icu::GregorianCalendar calendar(
+        year_, month_, day_, hour_, minute_, second_, success);
+    if (success <= U_ZERO_ERROR) {
+      UDate time = calendar.getTime(success);
+      icu::SimpleDateFormat formatter(*pattern_, success);
+      icu::UnicodeString formattedTime;
+      formatter.format(time, formattedTime, success);
+      formattedTime.toUTF8String(result);
+      if (success <= U_ZERO_ERROR) {

[bulach, 2013/02/05 11:54:56]

nit: remove {} here and 108 below

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

Done.

+        return result;
+      }
+    } else {
+      LOG(WARNING) << "Calendar not created: error " <<  success;
+    }
+
+    return "";
+  }
+}
+
+void DateTimeFormatter::ExtractType(
+    const WebKit::WebDateTimeChooserParams& source) {
+  switch (source.type) {
+    case WebKit::WebDateTimeInputTypeDate:
+      type_ = ui::TEXT_INPUT_TYPE_DATE;
+      break;
+    case WebKit::WebDateTimeInputTypeDateTime:
+      type_ = ui::TEXT_INPUT_TYPE_DATE_TIME;
+      break;
+    case WebKit::WebDateTimeInputTypeDateTimeLocal:
+      type_ = ui::TEXT_INPUT_TYPE_DATE_TIME_LOCAL;
+      break;
+    case WebKit::WebDateTimeInputTypeMonth:
+      type_ = ui::TEXT_INPUT_TYPE_MONTH;
+      break;
+    case WebKit::WebDateTimeInputTypeTime:
+      type_ = ui::TEXT_INPUT_TYPE_TIME;
+      break;
+    case WebKit::WebDateTimeInputTypeWeek: // Not implemented
+    case WebKit::WebDateTimeInputTypeNone:
+    default:
+      type_ = ui::TEXT_INPUT_TYPE_NONE;
+  }
+}
+
+// Not all fields are defined in all configurations and ICU might store
+// garbage if success <= U_ZERO_ERROR so the output is sanitized here.
+int DateTimeFormatter::ExtractValue(
+    const icu::Calendar* calendar, UCalendarDateFields value) const {
+  UErrorCode success = U_ZERO_ERROR;
+  int result = calendar->get(value, success);
+  return (success <= U_ZERO_ERROR) ? result : 0;
+}
+
+bool DateTimeFormatter::ParseValues() {
+  if (type_ == ui::TEXT_INPUT_TYPE_NONE) {
+    ClearAll();
+    return false;
+  }
+
+  if (formatted_string_.empty()) {
+    ClearAll();
+    return true;
+  }
+
+  UErrorCode success = U_ZERO_ERROR;
+  icu::UnicodeString icu_value =
+      icu::UnicodeString::fromUTF8(formatted_string_);
+  if (pattern_map_.count(type_) > 0) {

[bulach, 2013/02/05 11:54:56]

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;

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

Since I switched to an array I now just checked the boundaries.

On 2013/02/05 11:54:56, bulach wrote:

+    const icu::UnicodeString pattern = pattern_map_[type_];
+    icu::SimpleDateFormat formatter(pattern, success);
+    formatter.parse(icu_value, success);
+    if (success <= U_ZERO_ERROR) {
+      const icu::Calendar* cal = formatter.getCalendar();
+      year_ = ExtractValue(cal, UCAL_YEAR);
+      month_ = ExtractValue(cal, UCAL_MONTH);
+      day_ = ExtractValue(cal, UCAL_DATE);
+      hour_ = ExtractValue(cal, UCAL_HOUR_OF_DAY);  // 24h format
+      minute_ = ExtractValue(cal, UCAL_MINUTE);
+      second_ = ExtractValue(cal, UCAL_SECOND);
+    }
+  }
+
+  return (success <= U_ZERO_ERROR);
+}
+
+void DateTimeFormatter::ClearAll() {

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

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.

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

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:
value", but it could also be a legitimate date-time.

+  year_ = 0;
+  month_ = 0;
+  day_ = 0;
+  hour_ = 0;
+  minute_ = 0;
+  second_ = 0;
+}
+
+}  // namespace content