New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 698802 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Cc:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug



Sign in to add a comment

Time-formatting functions in //base/i18n shouldn't swallow ICU errors

Project Member Reported by derat@chromium.org, Mar 6 2017

Issue description

base/i18n/time_formatting.cc makes a bunch of calls to ICU without checking for errors. It's possible that this is hiding problems like the one discussed in  issue 677043 .

All of these formatting methods should probably return bool success values and take string out-params. Otherwise, we can end up with nonsensical UI.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Mar 7 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/efcd30ca164e0e1bbcfe402840d8c23aa9d40fd9

commit efcd30ca164e0e1bbcfe402840d8c23aa9d40fd9
Author: derat <derat@chromium.org>
Date: Tue Mar 07 18:25:44 2017

base: Make TimeDurationFormat* report failures.

Update base::TimeDurationFormat() and
base::TimeDurationFormatWithSeconds() to return boolean
"success" values and use out-params to return formatted
strings. ICU errors were formerly swallowed, which makes it
impossible for the UI to handle them gracefully or for
developers to investigate their causes.

BUG=698802, 677043 

Review-Url: https://codereview.chromium.org/2734883003
Cr-Commit-Position: refs/heads/master@{#455144}

[modify] https://crrev.com/efcd30ca164e0e1bbcfe402840d8c23aa9d40fd9/ash/common/system/chromeos/power/battery_notification.cc
[modify] https://crrev.com/efcd30ca164e0e1bbcfe402840d8c23aa9d40fd9/ash/common/system/chromeos/power/power_status_view.cc
[modify] https://crrev.com/efcd30ca164e0e1bbcfe402840d8c23aa9d40fd9/base/i18n/time_formatting.cc
[modify] https://crrev.com/efcd30ca164e0e1bbcfe402840d8c23aa9d40fd9/base/i18n/time_formatting.h
[modify] https://crrev.com/efcd30ca164e0e1bbcfe402840d8c23aa9d40fd9/base/i18n/time_formatting_unittest.cc
[modify] https://crrev.com/efcd30ca164e0e1bbcfe402840d8c23aa9d40fd9/chrome/browser/ui/task_manager/task_manager_table_model.cc

Project Member

Comment 2 by bugdroid1@chromium.org, Mar 8 2017

Labels: merge-merged-3029
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/eaf9c6686c55d0b2d3cbe2c69818db7f017187e7

commit eaf9c6686c55d0b2d3cbe2c69818db7f017187e7
Author: Daniel Erat <derat@chromium.org>
Date: Wed Mar 08 23:17:08 2017

base: Make TimeDurationFormat* report failures.

Update base::TimeDurationFormat() and
base::TimeDurationFormatWithSeconds() to return boolean
"success" values and use out-params to return formatted
strings. ICU errors were formerly swallowed, which makes it
impossible for the UI to handle them gracefully or for
developers to investigate their causes.

BUG=698802, 677043 

Review-Url: https://codereview.chromium.org/2734883003
Cr-Commit-Position: refs/heads/master@{#455144}
(cherry picked from commit efcd30ca164e0e1bbcfe402840d8c23aa9d40fd9)

Review-Url: https://codereview.chromium.org/2736363002 .
Cr-Commit-Position: refs/branch-heads/3029@{#73}
Cr-Branched-From: 939b32ee5ba05c396eef3fd992822fcca9a2e262-refs/heads/master@{#454471}

[modify] https://crrev.com/eaf9c6686c55d0b2d3cbe2c69818db7f017187e7/ash/common/system/chromeos/power/battery_notification.cc
[modify] https://crrev.com/eaf9c6686c55d0b2d3cbe2c69818db7f017187e7/ash/common/system/chromeos/power/power_status_view.cc
[modify] https://crrev.com/eaf9c6686c55d0b2d3cbe2c69818db7f017187e7/base/i18n/time_formatting.cc
[modify] https://crrev.com/eaf9c6686c55d0b2d3cbe2c69818db7f017187e7/base/i18n/time_formatting.h
[modify] https://crrev.com/eaf9c6686c55d0b2d3cbe2c69818db7f017187e7/base/i18n/time_formatting_unittest.cc
[modify] https://crrev.com/eaf9c6686c55d0b2d3cbe2c69818db7f017187e7/chrome/browser/ui/task_manager/task_manager_table_model.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Mar 13 2017

Labels: merge-merged-2987
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/01ae29ee6c467754119319d62513e97085eafa96

commit 01ae29ee6c467754119319d62513e97085eafa96
Author: Daniel Erat <derat@chromium.org>
Date: Mon Mar 13 23:20:57 2017

base: Make TimeDurationFormat* report failures.

Update base::TimeDurationFormat() and
base::TimeDurationFormatWithSeconds() to return boolean
"success" values and use out-params to return formatted
strings. ICU errors were formerly swallowed, which makes it
impossible for the UI to handle them gracefully or for
developers to investigate their causes.

BUG=698802, 677043 
TBR=jshin@chromium.org
NOTRY=true
NOPRESUBMIT=true

Review-Url: https://codereview.chromium.org/2734883003
Cr-Commit-Position: refs/heads/master@{#455144}
(cherry picked from commit efcd30ca164e0e1bbcfe402840d8c23aa9d40fd9)

Review-Url: https://codereview.chromium.org/2745233002 .
Cr-Commit-Position: refs/branch-heads/2987@{#818}
Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943}

[modify] https://crrev.com/01ae29ee6c467754119319d62513e97085eafa96/ash/common/system/chromeos/power/battery_notification.cc
[modify] https://crrev.com/01ae29ee6c467754119319d62513e97085eafa96/ash/common/system/chromeos/power/power_status_view.cc
[modify] https://crrev.com/01ae29ee6c467754119319d62513e97085eafa96/base/i18n/time_formatting.cc
[modify] https://crrev.com/01ae29ee6c467754119319d62513e97085eafa96/base/i18n/time_formatting.h
[modify] https://crrev.com/01ae29ee6c467754119319d62513e97085eafa96/base/i18n/time_formatting_unittest.cc
[modify] https://crrev.com/01ae29ee6c467754119319d62513e97085eafa96/chrome/browser/ui/task_manager/task_manager_table_model.cc

Sign in to add a comment