Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2972493002:
Implement time stamp in new-style notification. (Closed)
DescriptionImplement time stamp in new-style notification.
New-style notification should show the notification timestamp relative
to now on the header.
The format should be consistent with Android's one, so we have to
implement our own version of formatter.
BUG=737003
TEST=manual
Review-Url: https://codereview.chromium.org/2972493002
Cr-Commit-Position: refs/heads/master@{#484220}
Committed: https://chromium.googlesource.com/chromium/src/+/0563d2235011452f270d01a7557e50d06e292f5b
Patch Set 1 #Patch Set 2 : Rebased. #Patch Set 3 : Modify comment. #
Total comments: 4
Patch Set 4 : Resolve review comments. #
Total comments: 2
Patch Set 5 : Resolve review comments. #Patch Set 6 : Fix UI strings. #
Messages
Total messages: 32 (21 generated)
The CQ bit was checked by tetsui@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tetsui@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
tetsui@chromium.org changed reviewers: + fukino@chromium.org, yoshiki@chromium.org
Please take a look. Thanks!
https://codereview.chromium.org/2972493002/diff/40001/ui/message_center/views... File ui/message_center/views/notification_header_view.cc (right): https://codereview.chromium.org/2972493002/diff/40001/ui/message_center/views... ui/message_center/views/notification_header_view.cc:48: constexpr int64_t kMinuteInMillis = 60L * 1000L; Use the LL or ULL to create 64-bit constants. https://google.github.io/styleguide/cppguide.html#64-bit_Portability ditto for other constants. https://codereview.chromium.org/2972493002/diff/40001/ui/message_center/views... ui/message_center/views/notification_header_view.cc:162: base::string16 divider_text = Instead of constructing the string here, how about having following string in unnamed namespace? constexpr wchar_t kNotificationHeaderDivider[] = L" \u2022 "; (and then use base::WideToUTF16(kNotificationHaderDivider) for the Labels)
The CQ bit was checked by tetsui@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/2972493002/diff/40001/ui/message_center/views... File ui/message_center/views/notification_header_view.cc (right): https://codereview.chromium.org/2972493002/diff/40001/ui/message_center/views... ui/message_center/views/notification_header_view.cc:48: constexpr int64_t kMinuteInMillis = 60L * 1000L; On 2017/07/05 06:42:59, fukino wrote: > Use the LL or ULL to create 64-bit constants. > https://google.github.io/styleguide/cppguide.html#64-bit_Portability > > ditto for other constants. Done. https://codereview.chromium.org/2972493002/diff/40001/ui/message_center/views... ui/message_center/views/notification_header_view.cc:162: base::string16 divider_text = On 2017/07/05 06:42:59, fukino wrote: > Instead of constructing the string here, how about having following string in > unnamed namespace? > > constexpr wchar_t kNotificationHeaderDivider[] = L" \u2022 "; > > (and then use base::WideToUTF16(kNotificationHaderDivider) for the Labels) Done.
lgtm
fukino@: Thanks! yoshiki@: PTAL for OWNERS review.
https://codereview.chromium.org/2972493002/diff/60001/ui/message_center/views... File ui/message_center/views/notification_header_view.cc (right): https://codereview.chromium.org/2972493002/diff/60001/ui/message_center/views... ui/message_center/views/notification_header_view.cc:51: constexpr int64_t kYearInMillis = 365LL * kDayInMillis; BTW, in Android, YEAR_IN_MILLIS is 364 days. https://developer.android.com/reference/android/text/format/DateUtils.html#YE... I'm not sure why and I think 365 days here is OK. It's up to you to choose 364/365. JFYI.
The CQ bit was checked by tetsui@chromium.org to run a CQ dry run
https://codereview.chromium.org/2972493002/diff/60001/ui/message_center/views... File ui/message_center/views/notification_header_view.cc (right): https://codereview.chromium.org/2972493002/diff/60001/ui/message_center/views... ui/message_center/views/notification_header_view.cc:51: constexpr int64_t kYearInMillis = 365LL * kDayInMillis; On 2017/07/05 07:08:27, fukino wrote: > BTW, in Android, YEAR_IN_MILLIS is 364 days. > https://developer.android.com/reference/android/text/format/DateUtils.html#YE... > > I'm not sure why and I think 365 days here is OK. > It's up to you to choose 364/365. JFYI. Done. It seems Android did YEAR_IN_MILLIS = WEEK_IN_MILLIS * 52, and it's kept for compatibility.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
Thanks!
The CQ bit was checked by tetsui@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tetsui@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fukino@chromium.org, yoshiki@chromium.org Link to the patchset: https://codereview.chromium.org/2972493002/#ps100001 (title: "Fix UI strings.")
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": 1499244635770850,
"parent_rev": "12cad72cd90108551c79ebde78ed564832478707", "commit_rev":
"0563d2235011452f270d01a7557e50d06e292f5b"}
Message was sent while issue was closed.
Description was changed from ========== Implement time stamp in new-style notification. New-style notification should show the notification timestamp relative to now on the header. The format should be consistent with Android's one, so we have to implement our own version of formatter. BUG=737003 TEST=manual ========== to ========== Implement time stamp in new-style notification. New-style notification should show the notification timestamp relative to now on the header. The format should be consistent with Android's one, so we have to implement our own version of formatter. BUG=737003 TEST=manual Review-Url: https://codereview.chromium.org/2972493002 Cr-Commit-Position: refs/heads/master@{#484220} Committed: https://chromium.googlesource.com/chromium/src/+/0563d2235011452f270d01a7557e... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/0563d2235011452f270d01a7557e... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
