Use ICU APIs for Assistant timer notification message.

Previously the timer notification message was manually constructed.
Now we rely on ICU APIs for better I18N support.

Tested in en_US, fr_CA, ja.

Bug: b:122475245
Change-Id: Idf8f375adee0aba198ad7091a2aa96bbe283c6df
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1660261
Commit-Queue: David Black <dmblack@google.com>
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#704912}

ash/BUILD.gn

@@ -1661,6 +1661,7 @@ test("ash_unittests") {
     "app_menu/notification_menu_controller_unittest.cc",
     "app_menu/notification_menu_view_unittest.cc",
     "app_menu/notification_overflow_view_unittest.cc",
+    "assistant/assistant_alarm_timer_controller_unittest.cc",
     "assistant/assistant_notification_controller_unittest.cc",
     "assistant/assistant_screen_context_controller_unittest.cc",
     "assistant/assistant_state_controller_unittest.cc",

ash/ash_strings.grd

@@ -1977,8 +1977,8 @@ This file contains the strings for ash.
       <message name="IDS_ASSISTANT_TIMER_NOTIFICATION_TITLE" desc="Title for Assistant timer notification.">
         Time's up
       </message>
-      <message name="IDS_ASSISTANT_TIMER_NOTIFICATION_MESSAGE" desc="Message for Assistant timer notification. Example: -01:23s.">
-        <ph name="SIGN">{0}<ex>-</ex></ph><ph name="MINUTES_REMAINING">{1,number,00}<ex>01</ex></ph>:<ph name="SECONDS_REMAINING">{2,number,00}<ex>02</ex></ph>s
+      <message name="IDS_ASSISTANT_TIMER_NOTIFICATION_MESSAGE_EXPIRED" desc="Message for Assistant timer notification that has expired. Format: -hh:mm:ss.">
+        -<ph name="TIME_SINCE_EXPIRATION">{0}<ex>10:27</ex></ph>
       </message>
       <message name="IDS_ASSISTANT_TIMER_NOTIFICATION_ADD_1_MIN_BUTTON" desc="Label for button to add 1 minute to timer in Assistant timer notification.">
         Add 1 min

ash/assistant/assistant_alarm_timer_controller.cc

@@ -19,6 +19,10 @@
 #include "chromeos/services/assistant/public/mojom/assistant.mojom.h"
 #include "mojo/public/cpp/bindings/pending_receiver.h"
 #include "mojo/public/cpp/bindings/receiver.h"
+#include "third_party/icu/source/common/unicode/utypes.h"
+#include "third_party/icu/source/i18n/unicode/measfmt.h"
+#include "third_party/icu/source/i18n/unicode/measunit.h"
+#include "third_party/icu/source/i18n/unicode/measure.h"
 #include "ui/base/l10n/l10n_util.h"
 
 namespace ash {
@@ -43,15 +47,62 @@ std::string CreateTimerNotificationId(const std::string& alarm_timer_id) {
   return std::string(kTimerNotificationIdPrefix) + alarm_timer_id;
 }
 
+// Creates a notification message for the given |alarm_timer| which has the
+// specified amount of |time_remaining|. Note that if the alarm/timer is expired
+// the amount of time remaining will be negated.
 std::string CreateTimerNotificationMessage(const AlarmTimer& alarm_timer,
                                            base::TimeDelta time_remaining) {
-  const int minutes_remaining = time_remaining.InMinutes();
-  const int seconds_remaining =
-      (time_remaining - base::TimeDelta::FromMinutes(minutes_remaining))
-          .InSeconds();
-  return base::UTF16ToUTF8(base::i18n::MessageFormatter::FormatWithNumberedArgs(
-      l10n_util::GetStringUTF16(IDS_ASSISTANT_TIMER_NOTIFICATION_MESSAGE),
-      alarm_timer.expired() ? "-" : "", minutes_remaining, seconds_remaining));
+  // Method aliases to prevent line-wrapping below.
+  const auto createHour = icu::MeasureUnit::createHour;
+  const auto createMinute = icu::MeasureUnit::createMinute;
+  const auto createSecond = icu::MeasureUnit::createSecond;
+
+  // Calculate hours/minutes/seconds remaining.
+  const int64_t total_seconds = time_remaining.InSeconds();
+  const int64_t hours = total_seconds / 3600;
+  const int64_t minutes = (total_seconds - hours * 3600) / 60;
+  const int64_t seconds = total_seconds % 60;
+
+  // Success of the ICU APIs is tracked by |status|.
+  UErrorCode status = U_ZERO_ERROR;
+
+  // Create our distinct |measures| to be formatted. We only show |hours| if
+  // necessary, otherwise they are omitted.
+  std::vector<icu::Measure> measures;
+  if (hours)
+    measures.push_back(icu::Measure(hours, createHour(status), status));
+  measures.push_back(icu::Measure(minutes, createMinute(status), status));
+  measures.push_back(icu::Measure(seconds, createSecond(status), status));
+
+  // Format our |measures| into a |unicode_message|.
+  icu::UnicodeString unicode_message;
+  icu::FieldPosition field_position = icu::FieldPosition::DONT_CARE;
+  UMeasureFormatWidth width = UMEASFMT_WIDTH_NUMERIC;
+  icu::MeasureFormat measure_format(icu::Locale::getDefault(), width, status);
+  measure_format.formatMeasures(measures.data(), measures.size(),
+                                unicode_message, field_position, status);
+
+  std::string message;
+  if (U_SUCCESS(status)) {
+    // If formatting was successful, convert our |unicode_message| into UTF-8.
+    unicode_message.toUTF8String(message);
+  } else {
+    // If something went wrong, we'll fall back to using "hh:mm:ss" instead.
+    LOG(ERROR) << "Error formatting timer notification message: " << status;
+    message = base::StringPrintf("%02ld:%02ld:%02ld", hours, minutes, seconds);
+  }
+
+  // If time has elapsed since the |alarm_timer| has expired, we'll need to
+  // negate the amount of time remaining.
+  if (total_seconds && alarm_timer.expired()) {
+    const auto format = l10n_util::GetStringUTF16(
+        IDS_ASSISTANT_TIMER_NOTIFICATION_MESSAGE_EXPIRED);
+    return base::UTF16ToUTF8(
+        base::i18n::MessageFormatter::FormatWithNumberedArgs(format, message));
+  }
+
+  // Otherwise, all necessary formatting has been performed.
+  return message;
 }
 
 // TODO(llin): Migrate to use the AlarmManager API to better support multiple

ash/assistant/assistant_alarm_timer_controller_unittest.cc

+// Copyright 2019 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 "ash/assistant/assistant_alarm_timer_controller.h"
+
+#include <memory>
+#include <vector>
+
+#include "ash/assistant/assistant_controller.h"
+#include "ash/assistant/assistant_notification_controller.h"
+#include "ash/assistant/model/assistant_notification_model.h"
+#include "ash/assistant/model/assistant_notification_model_observer.h"
+#include "ash/public/mojom/assistant_controller.mojom.h"
+#include "ash/shell.h"
+#include "ash/strings/grit/ash_strings.h"
+#include "ash/test/ash_test_base.h"
+#include "base/macros.h"
+#include "base/test/icu_test_util.h"
+#include "base/test/task_environment.h"
+#include "ui/base/l10n/l10n_util.h"
+
+namespace ash {
+
+namespace {
+
+// Helpers ---------------------------------------------------------------------
+
+// Creates a timer event with a given |id| and |state|.
+mojom::AssistantAlarmTimerEventPtr CreateTimerEvent(
+    const std::string& id,
+    mojom::AssistantTimerState state) {
+  auto timer_data = mojom::AssistantTimer::New();
+  timer_data->timer_id = id;
+  timer_data->state = state;
+
+  auto alarm_timer_data = mojom::AlarmTimerData::New();
+  alarm_timer_data->set_timer_data(std::move(timer_data));
+
+  auto timer_event = mojom::AssistantAlarmTimerEvent::New();
+  timer_event->type = mojom::AssistantAlarmTimerEventType::kTimer;
+  timer_event->data = std::move(alarm_timer_data);
+
+  return timer_event;
+}
+
+// ScopedNotificationModelObserver ---------------------------------------------
+
+class ScopedNotificationModelObserver
+    : public AssistantNotificationModelObserver {
+ public:
+  using AssistantNotification =
+      chromeos::assistant::mojom::AssistantNotification;
+  using AssistantNotificationPtr =
+      chromeos::assistant::mojom::AssistantNotificationPtr;
+
+  ScopedNotificationModelObserver() {
+    Shell::Get()
+        ->assistant_controller()
+        ->notification_controller()
+        ->AddModelObserver(this);
+  }
+
+  ~ScopedNotificationModelObserver() override {
+    Shell::Get()
+        ->assistant_controller()
+        ->notification_controller()
+        ->RemoveModelObserver(this);
+  }
+
+  // AssistantNotificationModelObserver:
+  void OnNotificationAdded(const AssistantNotification* notification) override {
+    last_notification_ = notification->Clone();
+  }
+
+  void OnNotificationUpdated(
+      const AssistantNotification* notification) override {
+    last_notification_ = notification->Clone();
+  }
+
+  const AssistantNotification* last_notification() const {
+    return last_notification_.get();
+  }
+
+ private:
+  AssistantNotificationPtr last_notification_;
+
+  DISALLOW_COPY_AND_ASSIGN(ScopedNotificationModelObserver);
+};
+
+}  // namespace
+
+// AssistantAlarmTimerControllerTest -------------------------------------------
+
+class AssistantAlarmTimerControllerTest : public AshTestBase {
+ protected:
+  AssistantAlarmTimerControllerTest()
+      : AshTestBase(base::test::TaskEnvironment::TimeSource::MOCK_TIME) {}
+
+  ~AssistantAlarmTimerControllerTest() override = default;
+
+  // AshTestBase:
+  void SetUp() override {
+    AshTestBase::SetUp();
+
+    controller_ =
+        Shell::Get()->assistant_controller()->alarm_timer_controller();
+    DCHECK(controller_);
+  }
+
+  // Advances the clock by |time_delta|, running any sequenced tasks in the
+  // queue. Note that we don't use |TaskEnvironment::FastForwardBy| because that
+  // API will hang when |time_delta| is sufficiently large, ultimately resulting
+  // in unittest timeout.
+  void AdvanceClock(base::TimeDelta time_delta) {
+    task_environment_->AdvanceClock(time_delta);
+    task_environment_->RunUntilIdle();
+  }
+
+  AssistantAlarmTimerController* controller() { return controller_; }
+
+ private:
+  AssistantAlarmTimerController* controller_ = nullptr;
+
+  DISALLOW_COPY_AND_ASSIGN(AssistantAlarmTimerControllerTest);
+};
+
+// Tests that a notification is added when a timer is fired and that the
+// notification is updated appropriately.
+TEST_F(AssistantAlarmTimerControllerTest, AddsAndUpdatesTimerNotification) {
+  // We're going to run our test over a few locales to ensure i18n compliance.
+  typedef struct {
+    std::string locale;
+    std::string expected_message_at_00_00_00;
+    std::string expected_message_at_00_00_01;
+    std::string expected_message_at_00_01_01;
+    std::string expected_message_at_01_01_01;
+  } I18nTestCase;
+
+  std::vector<I18nTestCase> i18n_test_cases;
+
+  // We'll test in English (United States).
+  i18n_test_cases.push_back({
+      /*locale=*/"en_US",
+      /*expected_message_at_00_00_00=*/"0:00",
+      /*expected_message_at_00_00_01=*/"-0:01",
+      /*expected_message_at_00_01_01=*/"-1:01",
+      /*expected_message_at_01_01_01=*/"-1:01:01",
+  });
+
+  // We'll also test in Slovenian (Slovenia).
+  i18n_test_cases.push_back({
+      /*locale=*/"sl_SI",
+      /*expected_message_at_00_00_00=*/"0.00",
+      /*expected_message_at_00_00_01=*/"-0.01",
+      /*expected_message_at_00_01_01=*/"-1.01",
+      /*expected_message_at_01_01_01=*/"-1.01.01",
+  });
+
+  // Run all of our internationalized test cases.
+  for (auto& i18n_test_case : i18n_test_cases) {
+    base::test::ScopedRestoreICUDefaultLocale locale(i18n_test_case.locale);
+
+    // Observe notifications.
+    ScopedNotificationModelObserver notification_model_observer;
+
+    // Fire a timer.
+    controller()->OnAlarmTimerStateChanged(
+        CreateTimerEvent(/*id=*/"1", mojom::AssistantTimerState::kFired));
+
+    // We expect our title to be internationalized.
+    const std::string expected_title =
+        l10n_util::GetStringUTF8(IDS_ASSISTANT_TIMER_NOTIFICATION_TITLE);
+
+    // Make assertions about the newly added notification.
+    auto* last_notification = notification_model_observer.last_notification();
+    EXPECT_EQ("assistant/timer1", last_notification->client_id);
+    EXPECT_EQ(expected_title, last_notification->title);
+    EXPECT_EQ(i18n_test_case.expected_message_at_00_00_00,
+              last_notification->message);
+
+    // Advance clock by 1 second.
+    AdvanceClock(base::TimeDelta::FromSeconds(1));
+
+    // Make assertions about the updated notification.
+    last_notification = notification_model_observer.last_notification();
+    EXPECT_EQ("assistant/timer1", last_notification->client_id);
+    EXPECT_EQ(expected_title, last_notification->title);
+    EXPECT_EQ(i18n_test_case.expected_message_at_00_00_01,
+              last_notification->message);
+
+    // Advance clock by 1 minute.
+    AdvanceClock(base::TimeDelta::FromMinutes(1));
+
+    // Make assertions about the updated notification.
+    last_notification = notification_model_observer.last_notification();
+    EXPECT_EQ("assistant/timer1", last_notification->client_id);
+    EXPECT_EQ(expected_title, last_notification->title);
+    EXPECT_EQ(i18n_test_case.expected_message_at_00_01_01,
+              last_notification->message);
+
+    // Advance clock by 1 hour.
+    AdvanceClock(base::TimeDelta::FromHours(1));
+
+    // Make assertions about the updated notification.
+    last_notification = notification_model_observer.last_notification();
+    EXPECT_EQ("assistant/timer1", last_notification->client_id);
+    EXPECT_EQ(expected_title, last_notification->title);
+    EXPECT_EQ(i18n_test_case.expected_message_at_01_01_01,
+              last_notification->message);
+  }
+}
+
+}  // namespace ash