Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2734883003:
base: Make TimeDurationFormat* report failures. (Closed)
|
Created:
3 years, 9 months ago by Daniel Erat Modified:
3 years, 9 months ago CC:
chromium-reviews, kalyank, sadrul, oshima+watch_chromium.org, vmpstr+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionbase: 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}
Committed: https://chromium.googlesource.com/chromium/src/+/efcd30ca164e0e1bbcfe402840d8c23aa9d40fd9
Patch Set 1 #
Total comments: 10
Patch Set 2 : add WARN_UNUSED_RESULT #
Total comments: 4
Patch Set 3 : add/remove logging #
Total comments: 2
Patch Set 4 : avoid msvc compilation error, hopefully #
Total comments: 2
Patch Set 5 : include utypes.h #
Total comments: 2
Patch Set 6 : log locale #
Messages
Total messages: 49 (26 generated)
The CQ bit was checked by derat@chromium.org to run a CQ dry run
derat@chromium.org changed reviewers: + bruthig@chromium.org, glevin@chromium.org, jshin@chromium.org
https://codereview.chromium.org/2734883003/diff/1/base/i18n/time_formatting.cc File base/i18n/time_formatting.cc (right): https://codereview.chromium.org/2734883003/diff/1/base/i18n/time_formatting.c... base/i18n/time_formatting.cc:191: icu::Measure(minutes, icu::MeasureUnit::createMinute(status), status)}; it seems to be undocumented, but from my reading of the ICU source, ICU methods and constructors generally (and hopefully always) bail out without overwriting the passed-in UErrorCode if it already contains an error.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2734883003/diff/1/base/i18n/time_formatting.h File base/i18n/time_formatting.h (right): https://codereview.chromium.org/2734883003/diff/1/base/i18n/time_formatting.h... base/i18n/time_formatting.h:110: BASE_I18N_EXPORT bool TimeDurationFormat(const TimeDelta time, Consider using the WARN_UNUSED_RESULT macro here.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
https://codereview.chromium.org/2734883003/diff/1/base/i18n/time_formatting.h File base/i18n/time_formatting.h (right): https://codereview.chromium.org/2734883003/diff/1/base/i18n/time_formatting.h... base/i18n/time_formatting.h:110: BASE_I18N_EXPORT bool TimeDurationFormat(const TimeDelta time, On 2017/03/06 21:12:38, bruthig wrote: > Consider using the WARN_UNUSED_RESULT macro here. sure, sounds good.
derat@chromium.org changed reviewers: + afakhry@chromium.org, nick@chromium.org
ahmed or nick, mind doing an owner review for chrome/browser/ui/task_manager/task_manager_table_model.cc?
The CQ bit was checked by derat@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2734883003/diff/1/base/i18n/time_formatting.cc File base/i18n/time_formatting.cc (right): https://codereview.chromium.org/2734883003/diff/1/base/i18n/time_formatting.c... base/i18n/time_formatting.cc:191: icu::Measure(minutes, icu::MeasureUnit::createMinute(status), status)}; On 2017/03/06 20:14:58, Daniel Erat wrote: > it seems to be undocumented, but from my reading of the ICU source, ICU > methods and constructors generally (and hopefully always) bail out without > overwriting the passed-in UErrorCode if it already contains an error. jshin@ would know the ICU code better, but that's certainly how it looks. Insofar as this CL is for debugging unexpected ICU errors like crbug.com/677043, it might be helpful to add one (or three?) debugging logs here as well, to preserve and pinpoint the ICU error code, e.g., if (U_FAILURE(status)) LOG(ERROR) << "Failed to construct measures[], status = " << status; https://codereview.chromium.org/2734883003/diff/1/base/i18n/time_formatting.c... base/i18n/time_formatting.cc:192: icu::MeasureFormat measure_format(icu::Locale::getDefault(), u_width, status); if (U_FAILURE(status)) LOG(ERROR) << "Failed to construct measure_format[], status = " << status; https://codereview.chromium.org/2734883003/diff/1/base/i18n/time_formatting.c... base/i18n/time_formatting.cc:195: measure_format.formatMeasures(measures, 2, formatted, ignore, status); if (U_FAILURE(status)) LOG(ERROR) << "formatMeasures() failed, status = " << status; Or, if you think three LOGs is too aggressive, then how about just putting if (U_FAILURE(status)) LOG(ERROR) << "ICU error in TimeDurationFormat() failed, status = " << status; here? (Normally I'd say 3 LOGs was overkill, but since this function currently seems to be the source of unexpected, mysterious errors, maybe it's warranted for now?) https://codereview.chromium.org/2734883003/diff/1/chrome/browser/ui/task_mana... File chrome/browser/ui/task_manager/task_manager_table_model.cc (right): https://codereview.chromium.org/2734883003/diff/1/chrome/browser/ui/task_mana... chrome/browser/ui/task_manager/task_manager_table_model.cc:144: base::string16 str; nit: Maybe something more descriptive, like |duration| or |time_string|?
https://codereview.chromium.org/2734883003/diff/20001/chrome/browser/ui/task_... File chrome/browser/ui/task_manager/task_manager_table_model.cc (right): https://codereview.chromium.org/2734883003/diff/20001/chrome/browser/ui/task_... chrome/browser/ui/task_manager/task_manager_table_model.cc:147: LOG(ERROR) << "Failed to format duration " << cpu_time.ToInternalValue(); Since we have a n/a string here, I think we gracefully handle the error already (I don't know what else we could do to handle such an error). I'd probably omit the LOG(ERROR) altogether.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
https://codereview.chromium.org/2734883003/diff/20001/chrome/browser/ui/task_... File chrome/browser/ui/task_manager/task_manager_table_model.cc (right): https://codereview.chromium.org/2734883003/diff/20001/chrome/browser/ui/task_... chrome/browser/ui/task_manager/task_manager_table_model.cc:147: LOG(ERROR) << "Failed to format duration " << cpu_time.ToInternalValue(); On 2017/03/06 21:52:37, ncarter wrote: > Since we have a n/a string here, I think we gracefully handle the error already > (I don't know what else we could do to handle such an error). I'd probably omit > the LOG(ERROR) altogether. i don't think these failures should ever occur, so this may be useful in tracking down the cause if you start getting reports of unexpected n/a strings (see the linked bug about bad battery-time-to-empty strings). i'll remove the logging if you'd prefer that, though.
https://codereview.chromium.org/2734883003/diff/20001/chrome/browser/ui/task_... File chrome/browser/ui/task_manager/task_manager_table_model.cc (right): https://codereview.chromium.org/2734883003/diff/20001/chrome/browser/ui/task_... chrome/browser/ui/task_manager/task_manager_table_model.cc:147: LOG(ERROR) << "Failed to format duration " << cpu_time.ToInternalValue(); On 2017/03/06 22:47:10, Daniel Erat wrote: > On 2017/03/06 21:52:37, ncarter wrote: > > Since we have a n/a string here, I think we gracefully handle the error > already > > (I don't know what else we could do to handle such an error). I'd probably > omit > > the LOG(ERROR) altogether. > > i don't think these failures should ever occur, so this may be useful in > tracking down the cause if you start getting reports of unexpected n/a strings > (see the linked bug about bad battery-time-to-empty strings). i'll remove the > logging if you'd prefer that, though. Let's omit the LOG(ERROR); I don't think it's worth the binary-size cost [those get compiled into release right?].
task_manager lgtm assuming you remove the LOG()
https://codereview.chromium.org/2734883003/diff/1/base/i18n/time_formatting.cc File base/i18n/time_formatting.cc (right): https://codereview.chromium.org/2734883003/diff/1/base/i18n/time_formatting.c... base/i18n/time_formatting.cc:192: icu::MeasureFormat measure_format(icu::Locale::getDefault(), u_width, status); On 2017/03/06 21:40:06, Greg Levin wrote: > if (U_FAILURE(status)) > LOG(ERROR) << "Failed to construct measure_format[], status = " << status; Done. https://codereview.chromium.org/2734883003/diff/1/base/i18n/time_formatting.c... base/i18n/time_formatting.cc:195: measure_format.formatMeasures(measures, 2, formatted, ignore, status); On 2017/03/06 21:40:06, Greg Levin wrote: > if (U_FAILURE(status)) > LOG(ERROR) << "formatMeasures() failed, status = " << status; > > Or, if you think three LOGs is too aggressive, then how about just putting > > if (U_FAILURE(status)) > LOG(ERROR) << "ICU error in TimeDurationFormat() failed, status = " << > status; > > here? (Normally I'd say 3 LOGs was overkill, but since this function currently > seems to be the source of unexpected, mysterious errors, maybe it's warranted > for now?) sure, added temporary logging throughout. https://codereview.chromium.org/2734883003/diff/1/chrome/browser/ui/task_mana... File chrome/browser/ui/task_manager/task_manager_table_model.cc (right): https://codereview.chromium.org/2734883003/diff/1/chrome/browser/ui/task_mana... chrome/browser/ui/task_manager/task_manager_table_model.cc:144: base::string16 str; On 2017/03/06 21:40:07, Greg Levin wrote: > nit: Maybe something more descriptive, like |duration| or |time_string|? Done. https://codereview.chromium.org/2734883003/diff/20001/chrome/browser/ui/task_... File chrome/browser/ui/task_manager/task_manager_table_model.cc (right): https://codereview.chromium.org/2734883003/diff/20001/chrome/browser/ui/task_... chrome/browser/ui/task_manager/task_manager_table_model.cc:147: LOG(ERROR) << "Failed to format duration " << cpu_time.ToInternalValue(); On 2017/03/06 23:04:52, ncarter wrote: > On 2017/03/06 22:47:10, Daniel Erat wrote: > > On 2017/03/06 21:52:37, ncarter wrote: > > > Since we have a n/a string here, I think we gracefully handle the error > > already > > > (I don't know what else we could do to handle such an error). I'd probably > > omit > > > the LOG(ERROR) altogether. > > > > i don't think these failures should ever occur, so this may be useful in > > tracking down the cause if you start getting reports of unexpected n/a strings > > (see the linked bug about bad battery-time-to-empty strings). i'll remove the > > logging if you'd prefer that, though. > > Let's omit the LOG(ERROR); I don't think it's worth the binary-size cost [those > get compiled into release right?]. done (although i've more than offset that binary-size reduction by adding more temporary logging to TimeDurationFormat, where i think we're currently seeing a problem) :-P
The CQ bit was checked by derat@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM, except for comment https://codereview.chromium.org/2734883003/diff/40001/base/i18n/time_formatti... File base/i18n/time_formatting.cc (right): https://codereview.chromium.org/2734883003/diff/40001/base/i18n/time_formatti... base/i18n/time_formatting.cc:238: return U_SUCCESS(status); I think this UBool -> bool conversion (well, the previous one, technically) was the source of the win_* compile failures in Patch #2. e:\b\c\b\win\src\base\i18n\time_formatting.cc(197): error C2220: warning treated as error - no 'object' file generated e:\b\c\b\win\src\base\i18n\time_formatting.cc(197): warning C4800: 'UBool': forcing value to bool 'true' or 'false' (performance warning) e:\b\c\b\win\src\base\i18n\time_formatting.cc(220): warning C4800: 'UBool': forcing value to bool 'true' or 'false' (performance warning) I'm guessing this'll need to be fixed...?
https://codereview.chromium.org/2734883003/diff/40001/base/i18n/time_formatti... File base/i18n/time_formatting.cc (right): https://codereview.chromium.org/2734883003/diff/40001/base/i18n/time_formatti... base/i18n/time_formatting.cc:238: return U_SUCCESS(status); On 2017/03/06 23:43:21, Greg Levin wrote: > I think this UBool -> bool conversion (well, the previous one, technically) was > the source of the win_* compile failures in Patch #2. > > e:\b\c\b\win\src\base\i18n\time_formatting.cc(197): error C2220: warning treated > as error - no 'object' file generated > e:\b\c\b\win\src\base\i18n\time_formatting.cc(197): warning C4800: 'UBool': > forcing value to bool 'true' or 'false' (performance warning) > e:\b\c\b\win\src\base\i18n\time_formatting.cc(220): warning C4800: 'UBool': > forcing value to bool 'true' or 'false' (performance warning) > > I'm guessing this'll need to be fixed...? ah, thanks; i didn't look at that failure closely enough, apparently. sigh, ICU...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by derat@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/03/06 23:50:56, Daniel Erat wrote: > https://codereview.chromium.org/2734883003/diff/40001/base/i18n/time_formatti... > File base/i18n/time_formatting.cc (right): > > https://codereview.chromium.org/2734883003/diff/40001/base/i18n/time_formatti... > base/i18n/time_formatting.cc:238: return U_SUCCESS(status); > On 2017/03/06 23:43:21, Greg Levin wrote: > > I think this UBool -> bool conversion (well, the previous one, technically) > was > > the source of the win_* compile failures in Patch #2. > > > > e:\b\c\b\win\src\base\i18n\time_formatting.cc(197): error C2220: warning > treated > > as error - no 'object' file generated > > e:\b\c\b\win\src\base\i18n\time_formatting.cc(197): warning C4800: 'UBool': > > forcing value to bool 'true' or 'false' (performance warning) > > e:\b\c\b\win\src\base\i18n\time_formatting.cc(220): warning C4800: 'UBool': > > forcing value to bool 'true' or 'false' (performance warning) > > > > I'm guessing this'll need to be fixed...? > > ah, thanks; i didn't look at that failure closely enough, apparently. sigh, > ICU... Does !! work better? There's a talk in ICU using real bool.... Anyway, lgtm
On 2017/03/06 23:50:56, Daniel Erat wrote: > https://codereview.chromium.org/2734883003/diff/40001/base/i18n/time_formatti... > File base/i18n/time_formatting.cc (right): > > https://codereview.chromium.org/2734883003/diff/40001/base/i18n/time_formatti... > base/i18n/time_formatting.cc:238: return U_SUCCESS(status); > On 2017/03/06 23:43:21, Greg Levin wrote: > > I think this UBool -> bool conversion (well, the previous one, technically) > was > > the source of the win_* compile failures in Patch #2. > > > > e:\b\c\b\win\src\base\i18n\time_formatting.cc(197): error C2220: warning > treated > > as error - no 'object' file generated > > e:\b\c\b\win\src\base\i18n\time_formatting.cc(197): warning C4800: 'UBool': > > forcing value to bool 'true' or 'false' (performance warning) > > e:\b\c\b\win\src\base\i18n\time_formatting.cc(220): warning C4800: 'UBool': > > forcing value to bool 'true' or 'false' (performance warning) > > > > I'm guessing this'll need to be fixed...? > > ah, thanks; i didn't look at that failure closely enough, apparently. sigh, > ICU... Does !! work better? There's a talk in ICU using real bool.... Anyway, lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
https://codereview.chromium.org/2734883003/diff/60001/base/i18n/time_formatti... File base/i18n/time_formatting.cc (right): https://codereview.chromium.org/2734883003/diff/60001/base/i18n/time_formatti... base/i18n/time_formatting.cc:19: #include "third_party/icu/source/i18n/unicode/smpdtfmt.h" Nit: care to explicitly include an ICU header for u_error_name?
thanks! https://codereview.chromium.org/2734883003/diff/60001/base/i18n/time_formatti... File base/i18n/time_formatting.cc (right): https://codereview.chromium.org/2734883003/diff/60001/base/i18n/time_formatti... base/i18n/time_formatting.cc:19: #include "third_party/icu/source/i18n/unicode/smpdtfmt.h" On 2017/03/07 04:48:54, jungshik at Google wrote: > Nit: care to explicitly include an ICU header for u_error_name? Done.
The CQ bit was checked by derat@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bruthig@chromium.org, nick@chromium.org, glevin@chromium.org, jshin@chromium.org Link to the patchset: https://codereview.chromium.org/2734883003/#ps80001 (title: "include utypes.h")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
https://codereview.chromium.org/2734883003/diff/80001/base/i18n/time_formatti... File base/i18n/time_formatting.cc (right): https://codereview.chromium.org/2734883003/diff/80001/base/i18n/time_formatti... base/i18n/time_formatting.cc:203: LOG(ERROR) << "Creating MeasureFormat failed: " << u_errorName(status); I guess we can get the UI language from elsewhere in the log. If that's not the case, I guess recording the default locale name here by calling getName() method over icu::Locale would be handy.
The CQ bit was checked by derat@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2734883003/diff/80001/base/i18n/time_formatti... File base/i18n/time_formatting.cc (right): https://codereview.chromium.org/2734883003/diff/80001/base/i18n/time_formatti... base/i18n/time_formatting.cc:203: LOG(ERROR) << "Creating MeasureFormat failed: " << u_errorName(status); On 2017/03/07 08:44:46, jungshik at Google wrote: > I guess we can get the UI language from elsewhere in the log. If that's not the > case, I guess recording the default locale name here by calling getName() > method over icu::Locale would be handy. Done.
The CQ bit was checked by derat@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jshin@chromium.org, nick@chromium.org, glevin@chromium.org, bruthig@chromium.org Link to the patchset: https://codereview.chromium.org/2734883003/#ps100001 (title: "log locale")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1488902019211790,
"parent_rev": "0dc2a6bdf85013def2fcbb1b157eee49b63a1d5a", "commit_rev":
"efcd30ca164e0e1bbcfe402840d8c23aa9d40fd9"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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} Committed: https://chromium.googlesource.com/chromium/src/+/efcd30ca164e0e1bbcfe402840d8... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/efcd30ca164e0e1bbcfe402840d8... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
