Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
147443007:
Add support for localized time strings with two units, eg. "2 hours 17 minutes" (Closed)
|
Created:
6 years, 10 months ago by Thiemo Nagel Modified:
6 years, 9 months ago CC:
chromium-reviews, jshin+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdd support for localized time strings with two units, eg. "2 hours 17 minutes"
At that occasion, replace five existing formatting functions for one-unit time strings by a single consistent interface with the aim to make time formatting functions easier to use by highlighting the availability of different formatting options.
Also at that occasion, rename IDS_TIME_DURATION_LONG* to IDS_TIME_LONG* to be more consistent with other IDS_TIME* ids.
BUG=339835
TEST=full unit test coverage
TBR=oshima@chromium.org (for resolution_notification_controller.cc)
TBR=jennyz@chromium.org (for ash/system)
TBR=asanka@chromium.org (for download_item_model.cc)
TBR=tim@chromium.org (for profile_sync_service.cc)
TBR=xiyuan@chromium.org (for imageburner_ui.cc)
TBR=estade@chromium.org (for foreign_session_handler.cc, policy_ui.cc)
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=253830
Patch Set 1 : First working prototype #Patch Set 2 : Implementation finished, cleanup pending #Patch Set 3 : Update grit_whitelist.txt #Patch Set 4 : Rebase #Patch Set 5 : Cleanup and compilation fixes #Patch Set 6 : More cleanup and documentation #
Total comments: 178
Patch Set 7 : Address Bartosz' comments #
Total comments: 20
Patch Set 8 : Address Bartosz' 2nd round of comments #
Total comments: 4
Patch Set 9 : Address Bartosz' 3rd round of comments #
Total comments: 13
Patch Set 10 : Address Tony's comments #Messages
Total messages: 17 (0 generated)
Hi Bartosz,
may I ask you to take a look at this CL? It grew much larger than I initially
had intended (sorry for that) and supersedes CL 139413005 which was LGTM'ed but
not committed.
What would be a good name for the new files formatter.{cc,h}? They are not
intended to be included/used except by time_format.* and time_format_unittest.*.
Thank you and best regards,
Thiemo
On 2014/02/17 16:30:24, Thiemo Nagel wrote:
> Hi Bartosz,
>
> may I ask you to take a look at this CL? It grew much larger than I initially
> had intended (sorry for that) and supersedes CL 139413005 which was LGTM'ed
but
> not committed.
>
> What would be a good name for the new files formatter.{cc,h}? They are not
> intended to be included/used except by time_format.* and
time_format_unittest.*.
>
> Thank you and best regards,
> Thiemo
A quick nit before I start the actual review: TBR= lines should also not exceed
80 lines. You can specify multipe TBR lines with one person in each.
> A quick nit before I start the actual review: TBR= lines should also not exceed > 80 lines. You can specify multipe TBR lines with one person in each. Thanks, done. BTW: There seems to be potential to remove a bit of duplication from power_status.cc, but I only plan to investigate this once the CL at hand has landed.
Another thing: What would you think of moving Type and Length out of TimeFormat to make TimeFormat method invocations less painful, eg. as in ui::TimeFormat::Simple(ui::kElapsed, ui::kLong, ...) instead of ui::TimeFormat::Simple(ui::TimeFormat::kElapsed, ui::TimeFormat::kLong, ...)?
https://codereview.chromium.org/147443007/diff/760002/chrome/browser/download... File chrome/browser/download/download_item_model.cc (right): https://codereview.chromium.org/147443007/diff/760002/chrome/browser/download... chrome/browser/download/download_item_model.cc:340: time_remaining = ui::TimeFormat::Simple(ui::TimeFormat::kRemaining, Nit: The else branch now spans multiple lines. Hence, {} are needed. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.cc File ui/base/l10n/formatter.cc (right): https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.cc:20: int Ids[kNPluralities]; Nit: User hacker_style, not CamelCaps for member names. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.cc:21: const char *FallbackOne; Nit: Put * on the type, not the member. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.cc:213: void Formatter::Format(Unit unit, int value, Nit: Per the style guide, a declaration/definition must either fit entirely on one line or must specify one argument per line. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.cc:215: UErrorCode error = U_ZERO_ERROR; Nit: Why not move down the definition of |error| down to where you actually use it? https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.cc:216: DCHECK(format_[unit].get()) << "This can never happen."; Nit 1: #include "base/logging.h" Nit 2: format_[unit] can be truth-checked directly. No need for get(). Nit 3: "This can never happen" is the very premise of a DCHECK. No need to spell it out explicitly. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.cc:217: formatted_string = format_[unit].get()->format(value, error); Nit: No need for get() before -> https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.cc:222: void Formatter::Format(TwoUnits units, int value1, int value2, Nit: Per the style guide, a declaration/definition must either fit entirely on one line or must specify one argument per line. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.cc:224: UErrorCode error = U_ZERO_ERROR; Nit: Why not move down the definition of |error| down to where you actually use it? https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.cc:225: DCHECK(detailed_format_[units][0].get()) Nit: detailed_format_[unit][0] can be truth-checked directly. No need for get(). https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.cc:227: DCHECK(detailed_format_[units][1].get()) Nit: detailed_format_[unit][1] can be truth-checked directly. No need for get(). https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.cc:229: formatted_string = detailed_format_[units][0].get()->format(value1, error); Nit: No need for get() before -> https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.cc:231: formatted_string += detailed_format_[units][1].get()->format(value2, error); Nit: No need for get() before -> https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.cc:236: icu::PluralFormat* Formatter::CreateFallbackFormat( Nit: Per the style guide, a declaration/definition must either fit entirely on one line or must specify one argument per line. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.cc:239: if (rules.isKeyword(UNICODE_STRING_SIMPLE("one"))) { Nit: No curly braces necessary for single-line if. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.cc:253: for (size_t j = 0; j < kNPluralities; ++j) { Nit: No need for curly braces when the body of a loop is only one line long. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.cc:257: if (format.get() && !FormatterContainer::GetFallbackForTesting()) Nit 1: scoped_ptr supports truth testing out of the box: if (format) works. No need for format.get(). Nit 2: You could wrap the entire construction of |format| in an if (!FormatterContainer::GetFallbackForTesting()) https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.cc:271: FormatterContainer::FormatterContainer() { Nit: Can you not initialize formatter_ in the initializer list? https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.cc:307: for (int type = 0; type < TimeFormat::kNType; ++type) Nit: This for loop needs curly braces because its body is two lines long. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.cc:309: delete formatter_[type][length]; Can you make formatter_ be an array of scoped_ptrs so that it destroys its contents automatically? https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.h File ui/base/l10n/formatter.h (right): https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.h:9: #include "base/macros.h" Nit: We usually include "base/basictypes.h" instead, which includes macros and some other commonly used types. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.h:16: struct Pluralities; Nit: Add a blank line before this. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.h:23: enum { kNUnit = 4 }; This is not how we do enums in Chrome. See comment in time_format.h for the correct way to define an enum. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.h:24: enum Unit { SEC, MIN, HOUR, DAY }; Nit: Put each enum entry on its own line. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.h:25: enum { kNTwoUnits = 3 }; This is not how we do enums in Chrome. See comment in time_format.h for the correct way to define an enum. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.h:26: enum TwoUnits { MIN_SEC, HOUR_MIN, DAY_HOUR }; Nit: Put each enum entry on its own line. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.h:44: void Format(Unit unit, int value, icu::UnicodeString& formatted_string) const; Nit: Forward-declare icu::UnicodeString. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.h:45: void Format(TwoUnits units, int value1, int value2, Nit: Per the style guide, a declaration/definition must either fit entirely on one line or must specify one argument per line. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.h:51: icu::PluralFormat* CreateFallbackFormat( 1) This passes ownership. Return a scoped_ptr instead. 2) Nit: Per the style guide, a declaration/definition must either fit entirely on one line or must specify one argument per line. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.h:54: icu::PluralFormat* InitFormat(const Pluralities& pluralities); This passes ownership. Return a scoped_ptr instead. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.h:63: class UI_BASE_EXPORT FormatterContainer { Nit: #include "ui/base/ui_base_export.h" https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.h:71: // Implemented in header file to avoid code generation in production builds. Nit 1: For trivial getters and setters, implementation in headers is quite common. No need to justify it in a comment. Nit 2: The production method InitFormat() calls GetFallbackForTesting(). The compiler can inline it but it will not remove it from production builds. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.h:78: static bool ForceFallback_; Nit: user hacker_style, not CamelCase for member names. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.h:80: friend struct base::DefaultLazyInstanceTraits<FormatterContainer>; Nit: It is customary to have the friend declaration be the first thing in the private: block, followed by a blank line. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... File ui/base/l10n/time_format.cc (left): https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.cc:10: #include "base/logging.h" Nit: Still used for NOTREACHED(), DCHECK() and friends. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... File ui/base/l10n/time_format.cc (right): https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.cc:7: #include <vector> Nit: No longer used. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.cc:48: // Less than 59.5 seconds gets "X seconds left", anything larger is Nit, here and below: Comments like this one go inside the block, not before it: if (delta < one_minute - half_second) { // Less than 59.5 seconds gets "X seconds left", anything larger is // rounded to minutes. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.cc:56: } else if (delta < one_hour - (cutoff<60 ? half_minute : half_second)) { 1) Please update the comment to explain where the choice between |half_minute| and |half_second| comes from. 2) Nit: Add spaces around < https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.cc:57: if (delta < cutoff * one_minute - half_second) { Nit: Add a comment that explains what these if/else branches are about. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.cc:58: int minutes = (delta + half_second).InMinutes(); Nit: const. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.cc:59: int seconds = static_cast<int>( Nit 1: const. Nit 2: Would it not be simpler to do this instead: const int seconds = (delta + half_second).InSeconds() % 60; https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.cc:64: int minutes = (delta + half_minute).InMinutes(); Nit: const. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.cc:70: } else if (delta < one_day - (cutoff<24 ? half_hour : half_minute)) { 1) Please update the comment to explain where the choice between |half_hour| and |half_minute| comes from. 2) Nit: Add spaces around < https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.cc:71: if (delta < cutoff * one_hour - half_minute) { Nit: Add a comment that explains what these if/else branches are about. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.cc:72: int hours = (delta + half_minute).InHours(); Nit: const. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.cc:73: int minutes = Nit 1: const. Nit 2: Would it not be simpler to do this instead: const int minutes = (delta + half_minute).InMinutes() % 60; https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.cc:78: int hours = (delta + half_hour).InHours(); Nit: const. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.cc:84: if (delta < cutoff * one_day - half_hour) { Nit: Add a comment that explains what these if/else branches are about. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.cc:85: int days = (delta + half_hour).InDays(); Nit: const. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.cc:86: int hours = (delta - TimeDelta::FromDays(days) + half_hour).InHours(); Nit 1: const. Nit 2: Would it not be simpler to do this instead: const int hours = (delta + half_hour).InHours() % 24; https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.cc:90: int days = (delta + half_day).InDays(); Nit: const. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.cc:95: // With the fallback added, this should never fail. Nit: IIUC, this comment referred to the DCHECK() just below it that you removed. Hence, you should remove the comment as well. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.cc:96: int capacity = time_string.length() + 1; Nit: const. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.cc:100: time_string.extract(static_cast<UChar*>(WriteInto(&result, capacity)), Nit: #include "base/strings/string_util.h" https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.cc:110: return Detailed(type, length, 0, delta); Nit: I think it would be clearer if you just called FormatTimeImpl() from here. One less indirection to follow when trying to understand how this works. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_format.h File ui/base/l10n/time_format.h (right): https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.h:23: enum { kNType = 3 }; This is not how we do enums in Chrome: 1. Enum entries should be UPPER_CASE_HACKER_STYLE, no kCamelCaps. 2. If you want to know how many entries there are, define your enum as: enum Type { TYPE_DURATION = 0, TYPE_REMAINING, TYPE_ELAPSED, TYPE_COUNT // Must be last. }; https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.h:25: kDuration, // plain duration, eg. in English: "2 minutes" Nit: Capitalize the first letter of comments. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.h:30: enum { kNLength = 2 }; See above. This is not how we do enums in Chrome. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.h:32: kShort, // short format, eg. in English: sec/min/hour/day Nit 1: Capitalize the first letter of comments. Nit 2: s/eg./e.g.,/ https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.h:37: // single number, eg. in English "2 hours ago". Currently, all combinations Nit: s/eg./e.g.,/ https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.h:38: // of type and length are implemented except (kElapsed,kLong). Nit 1: Add space after comma. Nit 2: Reorder sentence "of type and length except (kElapsed, kLong) are implemented." https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.h:44: // formatted as a single number, eg. in English "2 hours ago", or as two Nit: s/eg./e.g.,/ https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.h:45: // numbers, eg. in English "2 hours and 13 minutes ago". The two-number Nit: s/eg./e.g.,/ https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.h:47: // value less than cutoff. (Applied to the examples above, cutoff=2 would Nit 1: Put pipes around symbol names referenced in a comment, e.g., |cutoff|. Nit 2: Add spaces around = https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.h:48: // yield the first string and cutoff=3 would return the second string.) Your example is correct but it took me a lot of thinking and remembering our prior offline conversation to figure out what cutoff=2 and cutoff=3 do and why. I think this sentence needs some rephrasing or maybe a clearer explanation that 2 respectively 3 would be compared against the number of hours here. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.h:50: // (kDuration,kLong). Nit: Add space after comma. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.h:53: // IDS_TIME_*_2ND) per [plurality x pair of units] which are contatenated Nit: s/contatenated/concatenated/ https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.h:55: // and second unit (presently a blank) is included in IDS_TIME_*_1ST. I think "a blank in the case of English" would be more accurate than "presently a blank". It is not like we are planning to change the separator in English but other languages may choose something different. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.h:80: static base::string16 SimpleForTesting(const FormatterContainer& container, AFAICT, the only difference between these ForTesting methods and their production counterparts is that these ones allow a |container| to be injected that forces you down the fallback code path. Why not provide a method that calls g_container.Get()->SetFallbackForTesting() instead, setting up |g_container| so that there is no need to inject a |container| for testing? https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.h:86: static base::string16 DetailedForTesting(const FormatterContainer& container, Nit: Add blank line before this. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.h:95: static base::string16 FormatTimeImpl(const FormatterContainer& container, Nit: The suffix |Impl| is used in Chrome for classes that implement a corresponding interface (e.g. UserManagerImpl implements UserManager). For method names, |Impl| is not a suffix we tend to use. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.h:101: #ifndef TIME_FORMAT_UNITTEST There is no precedent for this kind of construction in Chrome code. As commented in time_forma_unittest.cc, I do not think that TimeFormatTest should be inheriting from TimeFormat. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... File ui/base/l10n/time_format_unittest.cc (right): https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format_unittest.cc:5: #define TIME_FORMAT_UNITTEST 1 There is no precedent for such a construct in Chrome. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format_unittest.cc:24: class TimeFormatTest : public ::testing::Test, public TimeFormat { I do not think that is the right way of doing things. Yes, this gives you unqalified access to the methods but is having unqualified access important enough to introduce the TIME_FORMAT_UNITTEST hack and have TimeFormatTest inherit from TimeFormat, both things without precedent in the Chrome code base? I think not. Just make the *ForTesting() methods you need public.Presubmit will warn people who try to use them in production code. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format_unittest.cc:27: delta_0s(TimeDelta::FromSeconds(0)), Nit: Why are these member variables? Why not just create the various deltas when you need them? https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format_unittest.cc:74: LoadLocale(ui::ResourceBundle::GetSharedInstance() Nit: Setup and cleanup should be happening in SetUpTestCase() and TearDownTestCase() whenever possible. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format_unittest.cc:78: virtual ~TimeFormatTest() { Nit: Setup and cleanup should be happening in SetUpTestCase() and TearDownTestCase() whenever possible. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format_unittest.cc:83: void TestStrings(const FormatterContainer& container) { Nit: Helper functions like this one are not a good idea in tests. If e.g. the first expectation is not met, you will get an error in line 85. You will not know which invocation of the function this failure occured in. Just copy & paste the code into place instead of using this function. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format_unittest.cc:183: TimeDelta delta_0s, delta_1ms, delta_499ms, delta_500ms, delta_999ms, Nit 1: Members should end with trailing underscores. Nit 2: It is customary to declare one member per line. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format_unittest.cc:201: // Test rounding behaviour (simple). Nit: s/behaviour/behavior/ https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format_unittest.cc:266: #if 0 Nit: Do not commit code guarded by an #if 0. If this code is not exercised, remove it. https://codereview.chromium.org/147443007/diff/760002/ui/base/strings/ui_stri... File ui/base/strings/ui_strings.grd (right): https://codereview.chromium.org/147443007/diff/760002/ui/base/strings/ui_stri... ui/base/strings/ui_strings.grd:293: <message name="IDS_TIME_LONG_SEC_SINGULAR" I think the distinction between SEC/SECS, DAY/DAYS, etc. in the IDs is not a good idea for two reasons: - It reorders things in a weird way in grit_whitelist.txt so that e.g. IDS_TIME_DAY_1ST_SINGULAR ends up being far away from all the other IDS_TIME_DAYS_* constants. - The _SINGULAR string may actually apply to non-singular numbers like 21, 31, 101, 201, depending on language. A plural ending would be more consistent and more correct.
Hi Bartosz, thanks a lot for your many and detailed comments -- the code has benefited considerably from your review, in my opinion. I have addressed almost all of the comments, and replied to them where I had a different opinion. Could you please take another look? Thank you! Thiemo https://codereview.chromium.org/147443007/diff/760002/chrome/browser/download... File chrome/browser/download/download_item_model.cc (right): https://codereview.chromium.org/147443007/diff/760002/chrome/browser/download... chrome/browser/download/download_item_model.cc:340: time_remaining = ui::TimeFormat::Simple(ui::TimeFormat::kRemaining, On 2014/02/18 12:04:04, bartfab wrote: > Nit: The else branch now spans multiple lines. Hence, {} are needed. Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.cc File ui/base/l10n/formatter.cc (right): https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.cc:20: int Ids[kNPluralities]; On 2014/02/18 12:04:04, bartfab wrote: > Nit: User hacker_style, not CamelCaps for member names. Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.cc:21: const char *FallbackOne; On 2014/02/18 12:04:04, bartfab wrote: > Nit: Put * on the type, not the member. Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.cc:213: void Formatter::Format(Unit unit, int value, On 2014/02/18 12:04:04, bartfab wrote: > Nit: Per the style guide, a declaration/definition must either fit entirely on > one line or must specify one argument per line. Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.cc:215: UErrorCode error = U_ZERO_ERROR; On 2014/02/18 12:04:04, bartfab wrote: > Nit: Why not move down the definition of |error| down to where you actually use > it? Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.cc:216: DCHECK(format_[unit].get()) << "This can never happen."; On 2014/02/18 12:04:04, bartfab wrote: > Nit 1: #include "base/logging.h" > Nit 2: format_[unit] can be truth-checked directly. No need for get(). > Nit 3: "This can never happen" is the very premise of a DCHECK. No need to spell > it out explicitly. Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.cc:217: formatted_string = format_[unit].get()->format(value, error); On 2014/02/18 12:04:04, bartfab wrote: > Nit: No need for get() before -> Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.cc:222: void Formatter::Format(TwoUnits units, int value1, int value2, On 2014/02/18 12:04:04, bartfab wrote: > Nit: Per the style guide, a declaration/definition must either fit entirely on > one line or must specify one argument per line. Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.cc:224: UErrorCode error = U_ZERO_ERROR; On 2014/02/18 12:04:04, bartfab wrote: > Nit: Why not move down the definition of |error| down to where you actually use > it? Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.cc:225: DCHECK(detailed_format_[units][0].get()) On 2014/02/18 12:04:04, bartfab wrote: > Nit: detailed_format_[unit][0] can be truth-checked directly. No need for get(). Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.cc:227: DCHECK(detailed_format_[units][1].get()) On 2014/02/18 12:04:04, bartfab wrote: > Nit: detailed_format_[unit][1] can be truth-checked directly. No need for get(). Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.cc:229: formatted_string = detailed_format_[units][0].get()->format(value1, error); On 2014/02/18 12:04:04, bartfab wrote: > Nit: No need for get() before -> Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.cc:231: formatted_string += detailed_format_[units][1].get()->format(value2, error); On 2014/02/18 12:04:04, bartfab wrote: > Nit: No need for get() before -> Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.cc:236: icu::PluralFormat* Formatter::CreateFallbackFormat( On 2014/02/18 12:04:04, bartfab wrote: > Nit: Per the style guide, a declaration/definition must either fit entirely on > one line or must specify one argument per line. Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.cc:239: if (rules.isKeyword(UNICODE_STRING_SIMPLE("one"))) { On 2014/02/18 12:04:04, bartfab wrote: > Nit: No curly braces necessary for single-line if. Not necessary but optional, if I read the style guide correctly. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.cc:253: for (size_t j = 0; j < kNPluralities; ++j) { On 2014/02/18 12:04:04, bartfab wrote: > Nit: No need for curly braces when the body of a loop is only one line long. I can't find a rule for that. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.cc:257: if (format.get() && !FormatterContainer::GetFallbackForTesting()) On 2014/02/18 12:04:04, bartfab wrote: > Nit 1: scoped_ptr supports truth testing out of the box: if (format) works. No > need for format.get(). > > Nit 2: You could wrap the entire construction of |format| in an if > (!FormatterContainer::GetFallbackForTesting()) Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.cc:271: FormatterContainer::FormatterContainer() { On 2014/02/18 12:04:04, bartfab wrote: > Nit: Can you not initialize formatter_ in the initializer list? I'd love to, but that's C++11. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.cc:307: for (int type = 0; type < TimeFormat::kNType; ++type) On 2014/02/18 12:04:04, bartfab wrote: > Nit: This for loop needs curly braces because its body is two lines long. Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.cc:309: delete formatter_[type][length]; On 2014/02/18 12:04:04, bartfab wrote: > Can you make formatter_ be an array of scoped_ptrs so that it destroys its > contents automatically? Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.h File ui/base/l10n/formatter.h (right): https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.h:9: #include "base/macros.h" On 2014/02/18 12:04:04, bartfab wrote: > Nit: We usually include "base/basictypes.h" instead, which includes macros and > some other commonly used types. Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.h:16: struct Pluralities; On 2014/02/18 12:04:04, bartfab wrote: > Nit: Add a blank line before this. Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.h:23: enum { kNUnit = 4 }; On 2014/02/18 12:04:04, bartfab wrote: > This is not how we do enums in Chrome. See comment in time_format.h for the > correct way to define an enum. Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.h:24: enum Unit { SEC, MIN, HOUR, DAY }; On 2014/02/18 12:04:04, bartfab wrote: > Nit: Put each enum entry on its own line. Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.h:25: enum { kNTwoUnits = 3 }; On 2014/02/18 12:04:04, bartfab wrote: > This is not how we do enums in Chrome. See comment in time_format.h for the > correct way to define an enum. Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.h:26: enum TwoUnits { MIN_SEC, HOUR_MIN, DAY_HOUR }; On 2014/02/18 12:04:04, bartfab wrote: > Nit: Put each enum entry on its own line. Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.h:44: void Format(Unit unit, int value, icu::UnicodeString& formatted_string) const; On 2014/02/18 12:04:04, bartfab wrote: > Nit: Forward-declare icu::UnicodeString. I've tried it, it doesn't work. I can't forward declare it as icu::UnicodeString because icu is an alias for icu_46, however if I'd declare it as icu_46::UnicodeString it would break when the ICU version is changed. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.h:45: void Format(TwoUnits units, int value1, int value2, On 2014/02/18 12:04:04, bartfab wrote: > Nit: Per the style guide, a declaration/definition must either fit entirely on > one line or must specify one argument per line. Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.h:51: icu::PluralFormat* CreateFallbackFormat( On 2014/02/18 12:04:04, bartfab wrote: > 1) This passes ownership. Return a scoped_ptr instead. > > 2) Nit: Per the style guide, a declaration/definition must either fit entirely > on one line or must specify one argument per line. Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.h:54: icu::PluralFormat* InitFormat(const Pluralities& pluralities); On 2014/02/18 12:04:04, bartfab wrote: > This passes ownership. Return a scoped_ptr instead. Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.h:63: class UI_BASE_EXPORT FormatterContainer { On 2014/02/18 12:04:04, bartfab wrote: > Nit: #include "ui/base/ui_base_export.h" Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.h:71: // Implemented in header file to avoid code generation in production builds. On 2014/02/18 12:04:04, bartfab wrote: > Nit 1: For trivial getters and setters, implementation in headers is quite > common. No need to justify it in a comment. > > Nit 2: The production method InitFormat() calls GetFallbackForTesting(). The > compiler can inline it but it will not remove it from production builds. Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.h:78: static bool ForceFallback_; On 2014/02/18 12:04:04, bartfab wrote: > Nit: user hacker_style, not CamelCase for member names. Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.h:80: friend struct base::DefaultLazyInstanceTraits<FormatterContainer>; On 2014/02/18 12:04:04, bartfab wrote: > Nit: It is customary to have the friend declaration be the first thing in the > private: block, followed by a blank line. Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... File ui/base/l10n/time_format.cc (left): https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.cc:10: #include "base/logging.h" On 2014/02/18 12:04:04, bartfab wrote: > Nit: Still used for NOTREACHED(), DCHECK() and friends. Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... File ui/base/l10n/time_format.cc (right): https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.cc:7: #include <vector> On 2014/02/18 12:04:04, bartfab wrote: > Nit: No longer used. Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.cc:48: // Less than 59.5 seconds gets "X seconds left", anything larger is On 2014/02/18 12:04:04, bartfab wrote: > Nit, here and below: Comments like this one go inside the block, not before it: > > if (delta < one_minute - half_second) { > // Less than 59.5 seconds gets "X seconds left", anything larger is > // rounded to minutes. Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.cc:56: } else if (delta < one_hour - (cutoff<60 ? half_minute : half_second)) { On 2014/02/18 12:04:04, bartfab wrote: > 1) Please update the comment to explain where the choice between |half_minute| > and |half_second| comes from. > 2) Nit: Add spaces around < Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.cc:57: if (delta < cutoff * one_minute - half_second) { On 2014/02/18 12:04:04, bartfab wrote: > Nit: Add a comment that explains what these if/else branches are about. This (hopefully) is now explained in time_format.h very well, I'd rather let the code speak for itself here. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.cc:58: int minutes = (delta + half_second).InMinutes(); On 2014/02/18 12:04:04, bartfab wrote: > Nit: const. Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.cc:59: int seconds = static_cast<int>( On 2014/02/18 12:04:04, bartfab wrote: > Nit 2: Would it not be simpler to do this instead: > > const int seconds = (delta + half_second).InSeconds() % 60; Would that not imply using exactly the kind of knowledge that TimeDelta wants to abstract away? https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.cc:64: int minutes = (delta + half_minute).InMinutes(); On 2014/02/18 12:04:04, bartfab wrote: > Nit: const. Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.cc:70: } else if (delta < one_day - (cutoff<24 ? half_hour : half_minute)) { On 2014/02/18 12:04:04, bartfab wrote: > 1) Please update the comment to explain where the choice between |half_hour| and > |half_minute| comes from. > 2) Nit: Add spaces around < Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.cc:71: if (delta < cutoff * one_hour - half_minute) { On 2014/02/18 12:04:04, bartfab wrote: > Nit: Add a comment that explains what these if/else branches are about. see above https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.cc:72: int hours = (delta + half_minute).InHours(); On 2014/02/18 12:04:04, bartfab wrote: > Nit: const. Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.cc:73: int minutes = On 2014/02/18 12:04:04, bartfab wrote: > Nit 2: Would it not be simpler to do this instead: > > const int minutes = (delta + half_minute).InMinutes() % 60; see above https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.cc:78: int hours = (delta + half_hour).InHours(); On 2014/02/18 12:04:04, bartfab wrote: > Nit: const. Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.cc:84: if (delta < cutoff * one_day - half_hour) { On 2014/02/18 12:04:04, bartfab wrote: > Nit: Add a comment that explains what these if/else branches are about. see above https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.cc:85: int days = (delta + half_hour).InDays(); On 2014/02/18 12:04:04, bartfab wrote: > Nit: const. Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.cc:86: int hours = (delta - TimeDelta::FromDays(days) + half_hour).InHours(); On 2014/02/18 12:04:04, bartfab wrote: > Nit 2: Would it not be simpler to do this instead: > > const int hours = (delta + half_hour).InHours() % 24; see above https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.cc:90: int days = (delta + half_day).InDays(); On 2014/02/18 12:04:04, bartfab wrote: > Nit: const. Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.cc:95: // With the fallback added, this should never fail. On 2014/02/18 12:04:04, bartfab wrote: > Nit: IIUC, this comment referred to the DCHECK() just below it that you removed. > Hence, you should remove the comment as well. Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.cc:96: int capacity = time_string.length() + 1; On 2014/02/18 12:04:04, bartfab wrote: > Nit: const. Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.cc:100: time_string.extract(static_cast<UChar*>(WriteInto(&result, capacity)), On 2014/02/18 12:04:04, bartfab wrote: > Nit: #include "base/strings/string_util.h" Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.cc:110: return Detailed(type, length, 0, delta); On 2014/02/18 12:04:04, bartfab wrote: > Nit: I think it would be clearer if you just called FormatTimeImpl() from here. > One less indirection to follow when trying to understand how this works. Obsolete now as FormatTimeImpl() has been removed. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_format.h File ui/base/l10n/time_format.h (right): https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.h:23: enum { kNType = 3 }; On 2014/02/18 12:04:04, bartfab wrote: > This is not how we do enums in Chrome: > > 1. Enum entries should be UPPER_CASE_HACKER_STYLE, no kCamelCaps. > 2. If you want to know how many entries there are, define your enum as: > > enum Type { > TYPE_DURATION = 0, > TYPE_REMAINING, > TYPE_ELAPSED, > TYPE_COUNT // Must be last. > }; Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.h:25: kDuration, // plain duration, eg. in English: "2 minutes" On 2014/02/18 12:04:04, bartfab wrote: > Nit: Capitalize the first letter of comments. Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.h:30: enum { kNLength = 2 }; On 2014/02/18 12:04:04, bartfab wrote: > See above. This is not how we do enums in Chrome. Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.h:32: kShort, // short format, eg. in English: sec/min/hour/day On 2014/02/18 12:04:04, bartfab wrote: > Nit 1: Capitalize the first letter of comments. > Nit 2: s/eg./e.g.,/ Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.h:37: // single number, eg. in English "2 hours ago". Currently, all combinations On 2014/02/18 12:04:04, bartfab wrote: > Nit: s/eg./e.g.,/ Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.h:38: // of type and length are implemented except (kElapsed,kLong). On 2014/02/18 12:04:04, bartfab wrote: > Nit 1: Add space after comma. > Nit 2: Reorder sentence "of type and length except (kElapsed, kLong) are > implemented." Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.h:44: // formatted as a single number, eg. in English "2 hours ago", or as two On 2014/02/18 12:04:04, bartfab wrote: > Nit: s/eg./e.g.,/ Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.h:45: // numbers, eg. in English "2 hours and 13 minutes ago". The two-number On 2014/02/18 12:04:04, bartfab wrote: > Nit: s/eg./e.g.,/ Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.h:47: // value less than cutoff. (Applied to the examples above, cutoff=2 would On 2014/02/18 12:04:04, bartfab wrote: > Nit 1: Put pipes around symbol names referenced in a comment, e.g., |cutoff|. > Nit 2: Add spaces around = Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.h:48: // yield the first string and cutoff=3 would return the second string.) On 2014/02/18 12:04:04, bartfab wrote: > I think this sentence needs some rephrasing or maybe a clearer explanation that > 2 respectively 3 would be compared against the number of hours here. I've tried to improve the comment. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.h:50: // (kDuration,kLong). On 2014/02/18 12:04:04, bartfab wrote: > Nit: Add space after comma. Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.h:53: // IDS_TIME_*_2ND) per [plurality x pair of units] which are contatenated On 2014/02/18 12:04:04, bartfab wrote: > Nit: s/contatenated/concatenated/ Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.h:55: // and second unit (presently a blank) is included in IDS_TIME_*_1ST. On 2014/02/18 12:04:04, bartfab wrote: > I think "a blank in the case of English" would be more accurate than "presently > a blank". It is not like we are planning to change the separator in English but > other languages may choose something different. Done. (I wanted to express that someday someone might implement a different format that uses " and " instead of a blank.) https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.h:80: static base::string16 SimpleForTesting(const FormatterContainer& container, On 2014/02/18 12:04:04, bartfab wrote: > AFAICT, the only difference between these ForTesting methods and their > production counterparts is that these ones allow a |container| to be injected > that forces you down the fallback code path. Why not provide a method that calls > g_container.Get()->SetFallbackForTesting() instead, setting up |g_container| so > that there is no need to inject a |container| for testing? Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.h:86: static base::string16 DetailedForTesting(const FormatterContainer& container, On 2014/02/18 12:04:04, bartfab wrote: > Nit: Add blank line before this. Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.h:95: static base::string16 FormatTimeImpl(const FormatterContainer& container, On 2014/02/18 12:04:04, bartfab wrote: > Nit: The suffix |Impl| is used in Chrome for classes that implement a > corresponding interface (e.g. UserManagerImpl implements UserManager). For > method names, |Impl| is not a suffix we tend to use. I've removed the method anyways. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.h:101: #ifndef TIME_FORMAT_UNITTEST On 2014/02/18 12:04:04, bartfab wrote: > There is no precedent for this kind of construction in Chrome code. As commented > in time_forma_unittest.cc, I do not think that TimeFormatTest should be > inheriting from TimeFormat. Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... File ui/base/l10n/time_format_unittest.cc (right): https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format_unittest.cc:5: #define TIME_FORMAT_UNITTEST 1 On 2014/02/18 12:04:04, bartfab wrote: > There is no precedent for such a construct in Chrome. Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format_unittest.cc:24: class TimeFormatTest : public ::testing::Test, public TimeFormat { On 2014/02/18 12:04:04, bartfab wrote: > I do not think that is the right way of doing things. Yes, this gives you > unqalified access to the methods but is having unqualified access important > enough to introduce the TIME_FORMAT_UNITTEST hack and have TimeFormatTest > inherit from TimeFormat, both things without precedent in the Chrome code base? > I think not. Just make the *ForTesting() methods you need public.Presubmit will > warn people who try to use them in production code. Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format_unittest.cc:27: delta_0s(TimeDelta::FromSeconds(0)), On 2014/02/18 12:04:04, bartfab wrote: > Nit: Why are these member variables? Why not just create the various deltas when > you need them? Some are used in multiple places, others are quite complex, generally I think it's less error prone that way. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format_unittest.cc:74: LoadLocale(ui::ResourceBundle::GetSharedInstance() On 2014/02/18 12:04:04, bartfab wrote: > Nit: Setup and cleanup should be happening in SetUpTestCase() and > TearDownTestCase() whenever possible. Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format_unittest.cc:78: virtual ~TimeFormatTest() { On 2014/02/18 12:04:04, bartfab wrote: > Nit: Setup and cleanup should be happening in SetUpTestCase() and > TearDownTestCase() whenever possible. Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format_unittest.cc:83: void TestStrings(const FormatterContainer& container) { On 2014/02/18 12:04:04, bartfab wrote: > Nit: Helper functions like this one are not a good idea in tests. If e.g. the > first expectation is not met, you will get an error in line 85. You will not > know which invocation of the function this failure occured in. Just copy & paste > the code into place instead of using this function. I try to avoid duplicating code as it is burdensome to maintain, especially if its as much as 100 lines. I could turn the function into a macro as a way to alleviate the line number problem, but I'm unsure whether you'd appreciate that. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format_unittest.cc:183: TimeDelta delta_0s, delta_1ms, delta_499ms, delta_500ms, delta_999ms, On 2014/02/18 12:04:04, bartfab wrote: > Nit 1: Members should end with trailing underscores. > Nit 2: It is customary to declare one member per line. Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format_unittest.cc:201: // Test rounding behaviour (simple). On 2014/02/18 12:04:04, bartfab wrote: > Nit: s/behaviour/behavior/ Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format_unittest.cc:266: #if 0 On 2014/02/18 12:04:04, bartfab wrote: > Nit: Do not commit code guarded by an #if 0. If this code is not exercised, > remove it. It is useful for manual tests. https://codereview.chromium.org/147443007/diff/760002/ui/base/strings/ui_stri... File ui/base/strings/ui_strings.grd (right): https://codereview.chromium.org/147443007/diff/760002/ui/base/strings/ui_stri... ui/base/strings/ui_strings.grd:293: <message name="IDS_TIME_LONG_SEC_SINGULAR" On 2014/02/18 12:04:04, bartfab wrote: > I think the distinction between SEC/SECS, DAY/DAYS, etc. in the IDs is not a > good idea for two reasons: > > A plural ending would be more consistent and more correct. I'm just following the existing convention. I'd suggest and would be happy to address this in a separate CL.
https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.cc File ui/base/l10n/formatter.cc (right): https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.cc:239: if (rules.isKeyword(UNICODE_STRING_SIMPLE("one"))) { On 2014/02/19 17:08:44, Thiemo Nagel wrote: > On 2014/02/18 12:04:04, bartfab wrote: > > Nit: No curly braces necessary for single-line if. > > Not necessary but optional, if I read the style guide correctly. Yes, it is a matter of choice. But if you look at Chrome code, you will find that single-line ifs do not use {}, except for rare cases where a multi-line if was shortened over time, reaching one line eventually but nobody remembered to remove the {}. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.cc:253: for (size_t j = 0; j < kNPluralities; ++j) { On 2014/02/19 17:08:44, Thiemo Nagel wrote: > On 2014/02/18 12:04:04, bartfab wrote: > > Nit: No need for curly braces when the body of a loop is only one line long. > > I can't find a rule for that. It's hiding in the rule for conditionals: "In general, curly braces are not required for single-line statements, but they are allowed if you like them; conditional or loop statements with complex conditions or statements may be more readable with curly braces." As you see, this sentence refers to both conditionals and loops. Again, it is a matter of taste. Again, there is a strong preference in Chrome code toward no {} for single-line statements. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.cc:271: FormatterContainer::FormatterContainer() { On 2014/02/19 17:08:44, Thiemo Nagel wrote: > On 2014/02/18 12:04:04, bartfab wrote: > > Nit: Can you not initialize formatter_ in the initializer list? > > I'd love to, but that's C++11. That's too bad... https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.h File ui/base/l10n/formatter.h (right): https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.h:44: void Format(Unit unit, int value, icu::UnicodeString& formatted_string) const; On 2014/02/19 17:08:44, Thiemo Nagel wrote: > On 2014/02/18 12:04:04, bartfab wrote: > > Nit: Forward-declare icu::UnicodeString. > > I've tried it, it doesn't work. I can't forward declare it as > icu::UnicodeString because icu is an alias for icu_46, however if I'd declare it > as icu_46::UnicodeString it would break when the ICU version is changed. Makes sense. With third-party stuff, there is sometimes nothing we can do. You should include "third_party/icu/source/common/unicode/unistr.h" in that case though. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... File ui/base/l10n/time_format.cc (right): https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.cc:59: int seconds = static_cast<int>( On 2014/02/19 17:08:44, Thiemo Nagel wrote: > On 2014/02/18 12:04:04, bartfab wrote: > > Nit 2: Would it not be simpler to do this instead: > > > > const int seconds = (delta + half_second).InSeconds() % 60; > > Would that not imply using exactly the kind of knowledge that TimeDelta wants to > abstract away? I don't think TimeDelta is trying to abstract away the fact that a minute has 60 seconds. What TimeDelta does is to store an arbitrarily long amount of time, allowing you convert between units. Once you know you want seconds, it seems perfectly fine to call InSeconds() % 60. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... File ui/base/l10n/time_format_unittest.cc (right): https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format_unittest.cc:83: void TestStrings(const FormatterContainer& container) { On 2014/02/19 17:08:44, Thiemo Nagel wrote: > On 2014/02/18 12:04:04, bartfab wrote: > > Nit: Helper functions like this one are not a good idea in tests. If e.g. the > > first expectation is not met, you will get an error in line 85. You will not > > know which invocation of the function this failure occured in. Just copy & > paste > > the code into place instead of using this function. > > I try to avoid duplicating code as it is burdensome to maintain, especially if > its as much as 100 lines. I could turn the function into a macro as a way to > alleviate the line number problem, but I'm unsure whether you'd appreciate that. I agree in general but I have been bitten by this in tests and have had reviewers request that I copy & paste instead of calling the same method multiple times from one test. In this case, there is an elegant way around it: Leave the TestStrings() method as-is but split the test into two test cases, each of which calls TestStrings() only once. That way, if there is a failure inside TestStrings(), you will know what invocation failed. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format_unittest.cc:266: #if 0 On 2014/02/19 17:08:44, Thiemo Nagel wrote: > On 2014/02/18 12:04:04, bartfab wrote: > > Nit: Do not commit code guarded by an #if 0. If this code is not exercised, > > remove it. > > It is useful for manual tests. If you want to use this for manual testing, break it out into a separate test case and prefix the test case name with MANUAL_. That way, it will not run on the bots but you can always run it by hand if needed. https://codereview.chromium.org/147443007/diff/1110001/ui/base/l10n/formatter.cc File ui/base/l10n/formatter.cc (right): https://codereview.chromium.org/147443007/diff/1110001/ui/base/l10n/formatter... ui/base/l10n/formatter.cc:19: static const size_t kNPluralities = 6; Nit: Avoid abbreviations like "N" whenever possible. https://codereview.chromium.org/147443007/diff/1110001/ui/base/l10n/formatter.h File ui/base/l10n/formatter.h (right): https://codereview.chromium.org/147443007/diff/1110001/ui/base/l10n/formatter... ui/base/l10n/formatter.h:33: TWOUNITS_MIN_SEC, Nit: s/TWOUNITS/TWO_UNITS/ https://codereview.chromium.org/147443007/diff/1110001/ui/base/l10n/formatter... ui/base/l10n/formatter.h:36: TWOUNITS_COUNT // Enum size counter, not a unit pair. Must be last. Nit: Two spaces before inline comment. https://codereview.chromium.org/147443007/diff/1110001/ui/base/l10n/formatter... ui/base/l10n/formatter.h:58: int value1, Nit: s/value1/value_1/ https://codereview.chromium.org/147443007/diff/1110001/ui/base/l10n/formatter... ui/base/l10n/formatter.h:59: int value2, Nit: s/value2/value_2/ https://codereview.chromium.org/147443007/diff/1110001/ui/base/l10n/formatter... ui/base/l10n/formatter.h:63: friend class FormatterContainer; // allow access to force_fallback_ Why not make |Formatter| an inner class of |FormatterContainer|? That way, |Formatter| would automatically have access to all private members. https://codereview.chromium.org/147443007/diff/1110001/ui/base/l10n/time_form... File ui/base/l10n/time_format.cc (right): https://codereview.chromium.org/147443007/diff/1110001/ui/base/l10n/time_form... ui/base/l10n/time_format.cc:65: // Anything up to 59.5 minutes (resp. 59:59.500 when cutoff permits Nit 1: Here and below: Avoid abbreviations -> s/resp./respectively/ Nit 2: Here and below: s/cutoff/|cutoff|/ https://codereview.chromium.org/147443007/diff/1110001/ui/base/l10n/time_form... ui/base/l10n/time_format.cc:66: // double-value output) is formatted as minutes (resp. minutes and seconds). Nit: Here and below: I think s/double/two/ would be better. "double-valued" sounds too much like a floating point number. https://codereview.chromium.org/147443007/diff/1110001/ui/base/l10n/time_form... File ui/base/l10n/time_format.h (right): https://codereview.chromium.org/147443007/diff/1110001/ui/base/l10n/time_form... ui/base/l10n/time_format.h:58: // relative rounding error is guaranteed to be less than approximately 0.5 / I like the combination of "guaranteed to" and "approximately" :). https://codereview.chromium.org/147443007/diff/1110001/ui/base/l10n/time_form... ui/base/l10n/time_format.h:68: // and second unit (a blank in English) is included in IDS_TIME_*_1ST. Nit: The comment is excellent. It may make sense to add a quick explanation that the two strings are used because of limitations in icu.
Hi Bartosz, thanks again for your comments which I've dutifully worked through. May I ask you to take another look? Thank you, Thiemo https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.cc File ui/base/l10n/formatter.cc (right): https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.cc:239: if (rules.isKeyword(UNICODE_STRING_SIMPLE("one"))) { > Yes, it is a matter of choice. But if you look at Chrome code, you will find > that single-line ifs do not use {}, except for rare cases where a multi-line if > was shortened over time, reaching one line eventually but nobody remembered to > remove the {}. Done, though I'm not certain the code is more readable now. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.cc:253: for (size_t j = 0; j < kNPluralities; ++j) { On 2014/02/20 16:31:22, bartfab wrote: > On 2014/02/19 17:08:44, Thiemo Nagel wrote: > > On 2014/02/18 12:04:04, bartfab wrote: > > > Nit: No need for curly braces when the body of a loop is only one line long. > > > > I can't find a rule for that. > > It's hiding in the rule for conditionals: > > "In general, curly braces are not required for single-line statements, but they > are allowed if you like them; conditional or loop statements with complex > conditions or statements may be more readable with curly braces." > > As you see, this sentence refers to both conditionals and loops. Again, it is a > matter of taste. Again, there is a strong preference in Chrome code toward no {} > for single-line statements. Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.cc:253: for (size_t j = 0; j < kNPluralities; ++j) { > As you see, this sentence refers to both conditionals and loops. Again, it is a > matter of taste. Again, there is a strong preference in Chrome code toward no {} > for single-line statements. Oh well... https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.h File ui/base/l10n/formatter.h (right): https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/formatter.... ui/base/l10n/formatter.h:44: void Format(Unit unit, int value, icu::UnicodeString& formatted_string) const; On 2014/02/20 16:31:22, bartfab wrote: > On 2014/02/19 17:08:44, Thiemo Nagel wrote: > > On 2014/02/18 12:04:04, bartfab wrote: > > > Nit: Forward-declare icu::UnicodeString. > > > > I've tried it, it doesn't work. I can't forward declare it as > > icu::UnicodeString because icu is an alias for icu_46, however if I'd declare > it > > as icu_46::UnicodeString it would break when the ICU version is changed. > > Makes sense. With third-party stuff, there is sometimes nothing we can do. You > should include "third_party/icu/source/common/unicode/unistr.h" in that case > though. Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... File ui/base/l10n/time_format.cc (right): https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format.cc:59: int seconds = static_cast<int>( On 2014/02/20 16:31:22, bartfab wrote: > On 2014/02/19 17:08:44, Thiemo Nagel wrote: > > On 2014/02/18 12:04:04, bartfab wrote: > > > Nit 2: Would it not be simpler to do this instead: > > > > > > const int seconds = (delta + half_second).InSeconds() % 60; > > > > Would that not imply using exactly the kind of knowledge that TimeDelta wants > to > > abstract away? > > I don't think TimeDelta is trying to abstract away the fact that a minute has 60 > seconds. What TimeDelta does is to store an arbitrarily long amount of time, > allowing you convert between units. Once you know you want seconds, it seems > perfectly fine to call InSeconds() % 60. Done. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... File ui/base/l10n/time_format_unittest.cc (right): https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format_unittest.cc:83: void TestStrings(const FormatterContainer& container) { > In this case, there is an elegant way around it: Leave the TestStrings() method > as-is but split the test into two test cases, each of which calls TestStrings() > only once. That way, if there is a failure inside TestStrings(), you will know > what invocation failed. Thanks! I did it that way. https://codereview.chromium.org/147443007/diff/760002/ui/base/l10n/time_forma... ui/base/l10n/time_format_unittest.cc:266: #if 0 > If you want to use this for manual testing, break it out into a separate test > case and prefix the test case name with MANUAL_. That way, it will not run on > the bots but you can always run it by hand if needed. As far as I can see, the MANUAL_ prefix only works in integration tests (and it's a prefix to the test, not the test case). https://codereview.chromium.org/147443007/diff/1110001/ui/base/l10n/formatter.cc File ui/base/l10n/formatter.cc (right): https://codereview.chromium.org/147443007/diff/1110001/ui/base/l10n/formatter... ui/base/l10n/formatter.cc:19: static const size_t kNPluralities = 6; On 2014/02/20 16:31:23, bartfab wrote: > Nit: Avoid abbreviations like "N" whenever possible. Done. https://codereview.chromium.org/147443007/diff/1110001/ui/base/l10n/formatter.h File ui/base/l10n/formatter.h (right): https://codereview.chromium.org/147443007/diff/1110001/ui/base/l10n/formatter... ui/base/l10n/formatter.h:33: TWOUNITS_MIN_SEC, On 2014/02/20 16:31:23, bartfab wrote: > Nit: s/TWOUNITS/TWO_UNITS/ Done. https://codereview.chromium.org/147443007/diff/1110001/ui/base/l10n/formatter... ui/base/l10n/formatter.h:36: TWOUNITS_COUNT // Enum size counter, not a unit pair. Must be last. On 2014/02/20 16:31:23, bartfab wrote: > Nit: Two spaces before inline comment. Done. https://codereview.chromium.org/147443007/diff/1110001/ui/base/l10n/formatter... ui/base/l10n/formatter.h:58: int value1, On 2014/02/20 16:31:23, bartfab wrote: > Nit: s/value1/value_1/ Done. https://codereview.chromium.org/147443007/diff/1110001/ui/base/l10n/formatter... ui/base/l10n/formatter.h:59: int value2, On 2014/02/20 16:31:23, bartfab wrote: > Nit: s/value2/value_2/ Done. https://codereview.chromium.org/147443007/diff/1110001/ui/base/l10n/formatter... ui/base/l10n/formatter.h:63: friend class FormatterContainer; // allow access to force_fallback_ On 2014/02/20 16:31:23, bartfab wrote: > Why not make |Formatter| an inner class of |FormatterContainer|? That way, > |Formatter| would automatically have access to all private members. I moved force_fallback_ back to FormatterContainer -- that way, there's no need for a friend declaration anymore. https://codereview.chromium.org/147443007/diff/1110001/ui/base/l10n/time_form... File ui/base/l10n/time_format.cc (right): https://codereview.chromium.org/147443007/diff/1110001/ui/base/l10n/time_form... ui/base/l10n/time_format.cc:65: // Anything up to 59.5 minutes (resp. 59:59.500 when cutoff permits On 2014/02/20 16:31:23, bartfab wrote: > Nit 1: Here and below: Avoid abbreviations -> s/resp./respectively/ > Nit 2: Here and below: s/cutoff/|cutoff|/ Done. https://codereview.chromium.org/147443007/diff/1110001/ui/base/l10n/time_form... ui/base/l10n/time_format.cc:66: // double-value output) is formatted as minutes (resp. minutes and seconds). On 2014/02/20 16:31:23, bartfab wrote: > Nit: Here and below: I think s/double/two/ would be better. "double-valued" > sounds too much like a floating point number. Done. https://codereview.chromium.org/147443007/diff/1110001/ui/base/l10n/time_form... File ui/base/l10n/time_format.h (right): https://codereview.chromium.org/147443007/diff/1110001/ui/base/l10n/time_form... ui/base/l10n/time_format.h:58: // relative rounding error is guaranteed to be less than approximately 0.5 / On 2014/02/20 16:31:23, bartfab wrote: > I like the combination of "guaranteed to" and "approximately" :). It was tempting to write it that way, but I've change it to a more precise formulation now. https://codereview.chromium.org/147443007/diff/1110001/ui/base/l10n/time_form... ui/base/l10n/time_format.h:68: // and second unit (a blank in English) is included in IDS_TIME_*_1ST. On 2014/02/20 16:31:23, bartfab wrote: > Nit: The comment is excellent. It may make sense to add a quick explanation that > the two strings are used because of limitations in icu. Done.
lgtm https://codereview.chromium.org/147443007/diff/1580001/ui/base/l10n/formatter.h File ui/base/l10n/formatter.h (right): https://codereview.chromium.org/147443007/diff/1580001/ui/base/l10n/formatter... ui/base/l10n/formatter.h:91: static void SetFallbackForTesting(bool val) { force_fallback_ = val; } Nit: Avoid abbreviations: s/val/force_fallback/ https://codereview.chromium.org/147443007/diff/1580001/ui/base/l10n/time_form... File ui/base/l10n/time_format.cc (right): https://codereview.chromium.org/147443007/diff/1580001/ui/base/l10n/time_form... ui/base/l10n/time_format.cc:25: UI_BASE_EXPORT base::LazyInstance<FormatterContainer> g_container = Nit: #include "ui/base/ui_base_export.h"
Hi Tony, this time, a more substantial CL: Could you please take a look at my extension of time_format? Thank you and best regards, Thiemo https://codereview.chromium.org/147443007/diff/1580001/ui/base/l10n/formatter.h File ui/base/l10n/formatter.h (right): https://codereview.chromium.org/147443007/diff/1580001/ui/base/l10n/formatter... ui/base/l10n/formatter.h:91: static void SetFallbackForTesting(bool val) { force_fallback_ = val; } On 2014/02/26 12:45:20, bartfab wrote: > Nit: Avoid abbreviations: s/val/force_fallback/ Done. https://codereview.chromium.org/147443007/diff/1580001/ui/base/l10n/time_form... File ui/base/l10n/time_format.cc (right): https://codereview.chromium.org/147443007/diff/1580001/ui/base/l10n/time_form... ui/base/l10n/time_format.cc:25: UI_BASE_EXPORT base::LazyInstance<FormatterContainer> g_container = On 2014/02/26 12:45:20, bartfab wrote: > Nit: #include "ui/base/ui_base_export.h" Done.
ui/base/l10n LGTM. Some style nits below. https://codereview.chromium.org/147443007/diff/1690001/ui/base/l10n/formatter.cc File ui/base/l10n/formatter.cc (right): https://codereview.chromium.org/147443007/diff/1690001/ui/base/l10n/formatter... ui/base/l10n/formatter.cc:21: int Ids[kNumberPluralities]; Nit: ids https://codereview.chromium.org/147443007/diff/1690001/ui/base/l10n/formatter.h File ui/base/l10n/formatter.h (right): https://codereview.chromium.org/147443007/diff/1690001/ui/base/l10n/formatter... ui/base/l10n/formatter.h:90: static bool GetFallback() { return force_fallback_; } Nit: get_force_fallback() Or you could just move the bool to be a file static in formatter.cc. Then you don't need a getter. Alternately, this method should be private. https://codereview.chromium.org/147443007/diff/1690001/ui/base/l10n/formatter... ui/base/l10n/formatter.h:91: static void SetFallbackForTesting(bool force_fallback) { Nit: set_fallback_for_testing(bool force_fallback) https://codereview.chromium.org/147443007/diff/1690001/ui/base/l10n/formatter... ui/base/l10n/formatter.h:96: Shutdown(); Nit: Move this implementation to the cc file. https://codereview.chromium.org/147443007/diff/1690001/ui/base/l10n/time_form... File ui/base/l10n/time_format_unittest.cc (right): https://codereview.chromium.org/147443007/diff/1690001/ui/base/l10n/time_form... ui/base/l10n/time_format_unittest.cc:347: TEST(TimeFormatManualTest, MANUAL_SimpleFail) { Nit: I would just remove this manual test. Going forward, it should be easy to add single test cases. https://codereview.chromium.org/147443007/diff/1690001/ui/base/strings/ui_str... File ui/base/strings/ui_strings.grd (right): https://codereview.chromium.org/147443007/diff/1690001/ui/base/strings/ui_str... ui/base/strings/ui_strings.grd:572: desc="First part of 'xx minutes yy seconds' time format. This is necessary for every language. It is the default for all the numbers NOT covered by special cases (singular, dual/two, few, many) some languages need. For CJK, Vietnamese, Turkish and Kannada, this is the only string necessary. For languages with singular-plural distinction, this is the generic plural. For Lithuanian, NUMBER_DEFAULT is 11..19."> I would make a note that you need to include the trailing space if the language expects it. Same with the other 1ST_ descriptions.
Hi Tony, thanks a lot for your review! Would you prefer to include the mentioning of the trailing space with every *_1ST_DEFAULT description or with every *_1ST_* description? Best, Thiemo https://codereview.chromium.org/147443007/diff/1690001/ui/base/l10n/formatter.cc File ui/base/l10n/formatter.cc (right): https://codereview.chromium.org/147443007/diff/1690001/ui/base/l10n/formatter... ui/base/l10n/formatter.cc:21: int Ids[kNumberPluralities]; On 2014/02/26 20:29:20, tony wrote: > Nit: ids Done. https://codereview.chromium.org/147443007/diff/1690001/ui/base/l10n/formatter.h File ui/base/l10n/formatter.h (right): https://codereview.chromium.org/147443007/diff/1690001/ui/base/l10n/formatter... ui/base/l10n/formatter.h:90: static bool GetFallback() { return force_fallback_; } On 2014/02/26 20:29:20, tony wrote: > Nit: get_force_fallback() > > Or you could just move the bool to be a file static in formatter.cc. Then you > don't need a getter. Alternately, this method should be private. I can't make it private because it's used by Formatter, not FormatterContainer in formatter.cc -- but to move the bool outside of any class seems like a good idea. https://codereview.chromium.org/147443007/diff/1690001/ui/base/l10n/formatter... ui/base/l10n/formatter.h:91: static void SetFallbackForTesting(bool force_fallback) { On 2014/02/26 20:29:20, tony wrote: > Nit: set_fallback_for_testing(bool force_fallback) Doesn't apply anymore. https://codereview.chromium.org/147443007/diff/1690001/ui/base/l10n/formatter... ui/base/l10n/formatter.h:96: Shutdown(); On 2014/02/26 20:29:20, tony wrote: > Nit: Move this implementation to the cc file. I've renamed it ResetForTesting() and kept it inside the header file to avoid code generation in production builds. https://codereview.chromium.org/147443007/diff/1690001/ui/base/l10n/time_form... File ui/base/l10n/time_format_unittest.cc (right): https://codereview.chromium.org/147443007/diff/1690001/ui/base/l10n/time_form... ui/base/l10n/time_format_unittest.cc:347: TEST(TimeFormatManualTest, MANUAL_SimpleFail) { On 2014/02/26 20:29:20, tony wrote: > Nit: I would just remove this manual test. Going forward, it should be easy to > add single test cases. Done. https://codereview.chromium.org/147443007/diff/1690001/ui/base/strings/ui_str... File ui/base/strings/ui_strings.grd (right): https://codereview.chromium.org/147443007/diff/1690001/ui/base/strings/ui_str... ui/base/strings/ui_strings.grd:572: desc="First part of 'xx minutes yy seconds' time format. This is necessary for every language. It is the default for all the numbers NOT covered by special cases (singular, dual/two, few, many) some languages need. For CJK, Vietnamese, Turkish and Kannada, this is the only string necessary. For languages with singular-plural distinction, this is the generic plural. For Lithuanian, NUMBER_DEFAULT is 11..19."> On 2014/02/26 20:29:20, tony wrote: > I would make a note that you need to include the trailing space if the language > expects it. Same with the other 1ST_ descriptions. Would you prefer to include this with every *_1ST_DEFAULT description or with every *_1ST_* description?
On 2014/02/26 21:13:33, Thiemo Nagel wrote: > Hi Tony, > > thanks a lot for your review! Would you prefer to include the mentioning of the > trailing space with every *_1ST_DEFAULT description or with every *_1ST_* > description? All the strings that end in a space. I don't think the translators get the strings in the same order as in the grd file, so you need to provide the full context for each message.
The CQ bit was checked by tnagel@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tnagel@chromium.org/147443007/1740001
Message was sent while issue was closed.
Change committed as 253830
Message was sent while issue was closed.
https://codereview.chromium.org/147443007/diff/1690001/ui/base/l10n/formatter.h File ui/base/l10n/formatter.h (right): https://codereview.chromium.org/147443007/diff/1690001/ui/base/l10n/formatter... ui/base/l10n/formatter.h:90: static bool GetFallback() { return force_fallback_; } On 2014/02/26 21:13:34, Thiemo Nagel wrote: > On 2014/02/26 20:29:20, tony wrote: > > Nit: get_force_fallback() > > > > Or you could just move the bool to be a file static in formatter.cc. Then you > > don't need a getter. Alternately, this method should be private. > > I can't make it private because it's used by Formatter, not FormatterContainer > in formatter.cc -- but to move the bool outside of any class seems like a good > idea. Note that the style guide gives you a choice getween CamelCaps and hacker_style for simple getters and setters. |
