Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2942143002:
Add progress notification support to new-style notification. (Closed)
DescriptionAdd progress notification support to new-style notification.
CompactTitleMessageView is added to follow Android's behavior showing
notification title and message in single line when it has progress bar.
Also, added constructor option allow_round_corner to views::ProgressBar
so that progress bar style follows Material Design mock.
BUG=726245
TEST=manually tested by progress notification with
* short title, short message
* short title, long message
* long title, long message
Review-Url: https://codereview.chromium.org/2942143002
Cr-Commit-Position: refs/heads/master@{#481439}
Committed: https://chromium.googlesource.com/chromium/src/+/1046503e7e4f933250371d4af0742a463e6b4f77
Patch Set 1 #Patch Set 2 : Not to round progress bar corner. #Patch Set 3 : Implement appropriate eliding. #
Total comments: 7
Patch Set 4 : Resolve review comments. #
Total comments: 2
Patch Set 5 : Resolve review comments. #
Messages
Total messages: 36 (22 generated)
Description was changed from ========== WIP: Add progress notification support to new-style notification. BUG=726245 ========== to ========== Add progress notification support to new-style notification. CompactTitleMessageView is added to follow Android's behavior showing notification title and message in single line when it has progress bar. Also, added constructor option allow_round_corner to views::ProgressBar so that progress bar style follows Material Design mock. BUG=726245 TEST=manually tested by progress notification with * short title, short message * short title, long message * long title, long message ==========
Description was changed from ========== Add progress notification support to new-style notification. CompactTitleMessageView is added to follow Android's behavior showing notification title and message in single line when it has progress bar. Also, added constructor option allow_round_corner to views::ProgressBar so that progress bar style follows Material Design mock. BUG=726245 TEST=manually tested by progress notification with * short title, short message * short title, long message * long title, long message ========== to ========== Add progress notification support to new-style notification. CompactTitleMessageView is added to follow Android's behavior showing notification title and message in single line when it has progress bar. Also, added constructor option allow_round_corner to views::ProgressBar so that progress bar style follows Material Design mock. BUG=726245 TEST=manually tested by progress notification with * short title, short message * short title, long message * long title, long message ==========
tetsui@chromium.org changed reviewers: + fukino@chromium.org, yoshiki@chromium.org
PTAL. 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: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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.
https://codereview.chromium.org/2942143002/diff/40001/ui/message_center/views... File ui/message_center/views/notification_view_md.cc (right): https://codereview.chromium.org/2942143002/diff/40001/ui/message_center/views... ui/message_center/views/notification_view_md.cc:184: // Elides title and message. The behavior is based on Android's one. I'm not where where we should put message. In the mock for DOWNLOAD notification, there are "4 secs left" on right side of the title and "6.9/21.2 MB from url.abomariname.com" is below the progress bar. I assumed that "6.9/21...." is the message and "4 secs left" is new to CrOS, but I have no idea about the consistency with the Android one. Could you confirm it with Sebastien?
On 2017/06/19 04:44:52, fukino wrote: > https://codereview.chromium.org/2942143002/diff/40001/ui/message_center/views... > File ui/message_center/views/notification_view_md.cc (right): > > https://codereview.chromium.org/2942143002/diff/40001/ui/message_center/views... > ui/message_center/views/notification_view_md.cc:184: // Elides title and > message. The behavior is based on Android's one. > I'm not where where we should put message. > In the mock for DOWNLOAD notification, there are "4 secs left" on right side of > the title and "6.9/21.2 MB from url.abomariname.com" is below the progress bar. > > I assumed that "6.9/21...." is the message and "4 secs left" is new to CrOS, but > I have no idea about the consistency with the Android one. > > Could you confirm it with Sebastien? I confirmed it with Sebastien. It seems he didn't expect either of them being message, but considers this implementation OK as it's mirroring Android. Let's discuss further on the mail I CC'd.
lgtm wih nit https://codereview.chromium.org/2942143002/diff/40001/ui/message_center/views... File ui/message_center/views/notification_view_md.cc (right): https://codereview.chromium.org/2942143002/diff/40001/ui/message_center/views... ui/message_center/views/notification_view_md.cc:426: delete title_view_; nit: left_content_->RemoveChildView(title_view_); should be better, since it ensures |title_view_| was a child of |left_content|. https://codereview.chromium.org/2942143002/diff/40001/ui/message_center/views... ui/message_center/views/notification_view_md.cc:481: delete compact_title_message_view_; ditto. https://codereview.chromium.org/2942143002/diff/40001/ui/message_center/views... ui/message_center/views/notification_view_md.cc:499: delete progress_bar_view_; ditto
Thanks! I got an advice from Sebastien that the implementation is correct in general but "% of progress in header" is missing, so I would do this in another CL. https://codereview.chromium.org/2942143002/diff/40001/ui/message_center/views... File ui/message_center/views/notification_view_md.cc (right): https://codereview.chromium.org/2942143002/diff/40001/ui/message_center/views... ui/message_center/views/notification_view_md.cc:426: delete title_view_; On 2017/06/21 00:39:58, yoshiki wrote: > nit: left_content_->RemoveChildView(title_view_); should be better, since it > ensures |title_view_| was a child of |left_content|. Done. https://codereview.chromium.org/2942143002/diff/40001/ui/message_center/views... ui/message_center/views/notification_view_md.cc:481: delete compact_title_message_view_; On 2017/06/21 00:39:58, yoshiki wrote: > ditto. Done. https://codereview.chromium.org/2942143002/diff/40001/ui/message_center/views... ui/message_center/views/notification_view_md.cc:499: delete progress_bar_view_; On 2017/06/21 00:39:59, yoshiki wrote: > ditto Done.
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...
tetsui@chromium.org changed reviewers: + sadrul@chromium.org
sadrul@: PTAL for OWNERS review ui/views/controls. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
btw, did you check the layout in RTL languages?
lgtm
I'm sorry for a late response. lgtm with a nit. https://codereview.chromium.org/2942143002/diff/60001/ui/message_center/views... File ui/message_center/views/notification_view_md.cc (right): https://codereview.chromium.org/2942143002/diff/60001/ui/message_center/views... ui/message_center/views/notification_view_md.cc:247: right_content_->SetVisible(notification.type() != I think you can check the notification type in CreateOrUpdateIconView() and determine if the notification icon should be added or not. (I assume we don't need to change the visibility of right_content_ when notification icon is not added.)
yoshiki@: Thanks! I tested it with Hebrew and it worked properly. sadrul@: Thanks! fukino@: Thanks! https://codereview.chromium.org/2942143002/diff/60001/ui/message_center/views... File ui/message_center/views/notification_view_md.cc (right): https://codereview.chromium.org/2942143002/diff/60001/ui/message_center/views... ui/message_center/views/notification_view_md.cc:247: right_content_->SetVisible(notification.type() != On 2017/06/22 00:27:10, fukino wrote: > I think you can check the notification type in CreateOrUpdateIconView() and > determine if the notification icon should be added or not. (I assume we don't > need to change the visibility of right_content_ when notification icon is not > added.) Done.
The CQ bit was checked by tetsui@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yoshiki@chromium.org, sadrul@chromium.org, fukino@chromium.org Link to the patchset: https://codereview.chromium.org/2942143002/#ps80001 (title: "Resolve review comments.")
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: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by tetsui@chromium.org
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": 80001, "attempt_start_ts": 1498101906675810,
"parent_rev": "3c56c268f1477a1ab6be286a50e062592ba69d94", "commit_rev":
"1046503e7e4f933250371d4af0742a463e6b4f77"}
Message was sent while issue was closed.
Description was changed from ========== Add progress notification support to new-style notification. CompactTitleMessageView is added to follow Android's behavior showing notification title and message in single line when it has progress bar. Also, added constructor option allow_round_corner to views::ProgressBar so that progress bar style follows Material Design mock. BUG=726245 TEST=manually tested by progress notification with * short title, short message * short title, long message * long title, long message ========== to ========== Add progress notification support to new-style notification. CompactTitleMessageView is added to follow Android's behavior showing notification title and message in single line when it has progress bar. Also, added constructor option allow_round_corner to views::ProgressBar so that progress bar style follows Material Design mock. BUG=726245 TEST=manually tested by progress notification with * short title, short message * short title, long message * long title, long message Review-Url: https://codereview.chromium.org/2942143002 Cr-Commit-Position: refs/heads/master@{#481439} Committed: https://chromium.googlesource.com/chromium/src/+/1046503e7e4f933250371d4af074... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/1046503e7e4f933250371d4af074... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
