Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2922903002:
Add List notification support to new-style notification. (Closed)
DescriptionAdd List notification support to new-style notification.
BUG=726244
Review-Url: https://codereview.chromium.org/2922903002
Cr-Commit-Position: refs/heads/master@{#479254}
Committed: https://chromium.googlesource.com/chromium/src/+/fe8c4f9dde53e94cdebf2f7792b7dfea203df13e
Patch Set 1 #
Total comments: 8
Patch Set 2 : Resolve review comments. #Patch Set 3 : Implement collapse support. #Patch Set 4 : Fix layout bug. #
Total comments: 8
Patch Set 5 : Resolve review comments. #Patch Set 6 : Revert item to black title and grey message. #
Total comments: 2
Patch Set 7 : Fix nit. #Patch Set 8 : Use InvalidateLayout. #
Messages
Total messages: 37 (21 generated)
tetsui@chromium.org changed reviewers: + fukino@chromium.org, yoshiki@chromium.org
PTAL. Thanks! Screenshot: https://screenshot.googleplex.com/arN7JV59qy7
https://codereview.chromium.org/2922903002/diff/1/ui/message_center/views/not... File ui/message_center/views/notification_view_md.cc (right): https://codereview.chromium.org/2922903002/diff/1/ui/message_center/views/not... ui/message_center/views/notification_view_md.cc:124: PreferredSizeChanged(); I think this is not necessary, since the preferred size is not changed here. https://codereview.chromium.org/2922903002/diff/1/ui/message_center/views/not... ui/message_center/views/notification_view_md.cc:125: SchedulePaint(); I also think this is not necessary, since adding a view calls SchedulePaint() eventually. (see. https://cs.chromium.org/chromium/src/ui/views/view.cc?rcl=683485cf448a4b1e083...) https://codereview.chromium.org/2922903002/diff/1/ui/message_center/views/not... ui/message_center/views/notification_view_md.cc:133: child_at(i)->SetVisible(visible); It looks necessary? I think setting visibility of the parent automatically hides the children. Please write a reason as a comment if it's truly necessary. https://codereview.chromium.org/2922903002/diff/1/ui/message_center/views/not... ui/message_center/views/notification_view_md.cc:431: DCHECK(top_view_); Should we check main_view_ instead of top_view_, since we're adding views to main_view_?
Let me add a comment from UI perspective. If we're aligning to the behavior of Android, I think we should hide message_view_ if list is available. Fukino-san, WDYT?
Let me add a comment from UI perspective. If we're aligning to the behavior of Android, I think we should hide message_view_ if list is available. Fukino-san, WDYT?
On 2017/06/05 04:32:05, yoshiki wrote: > Let me add a comment from UI perspective. If we're aligning to the behavior of > Android, I think we should hide message_view_ if list is available. Fukino-san, > WDYT? Sorry I missed the comment. Yes I agree that we should hide message_view_ in this case. Actually we are hiding it even in Chrome OS legacy notification. https://codesearch.chromium.org/chromium/src/ui/message_center/views/notifica...
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...
Rebased on the ToT with two-leveled notification, and resolved review comments. Also, it hides message_view_ if list is available. Thanks! https://codereview.chromium.org/2922903002/diff/1/ui/message_center/views/not... File ui/message_center/views/notification_view_md.cc (right): https://codereview.chromium.org/2922903002/diff/1/ui/message_center/views/not... ui/message_center/views/notification_view_md.cc:124: PreferredSizeChanged(); On 2017/06/05 04:29:13, yoshiki wrote: > I think this is not necessary, since the preferred size is not changed here. Done. https://codereview.chromium.org/2922903002/diff/1/ui/message_center/views/not... ui/message_center/views/notification_view_md.cc:125: SchedulePaint(); On 2017/06/05 04:29:13, yoshiki wrote: > I also think this is not necessary, since adding a view calls SchedulePaint() > eventually. > > (see. > https://cs.chromium.org/chromium/src/ui/views/view.cc?rcl=683485cf448a4b1e083...) Done. https://codereview.chromium.org/2922903002/diff/1/ui/message_center/views/not... ui/message_center/views/notification_view_md.cc:133: child_at(i)->SetVisible(visible); On 2017/06/05 04:29:13, yoshiki wrote: > It looks necessary? I think setting visibility of the parent automatically hides > the children. Please write a reason as a comment if it's truly necessary. Done. https://codereview.chromium.org/2922903002/diff/1/ui/message_center/views/not... ui/message_center/views/notification_view_md.cc:431: DCHECK(top_view_); On 2017/06/05 04:29:13, yoshiki wrote: > Should we check main_view_ instead of top_view_, since we're adding views to > main_view_? Done.
https://codereview.chromium.org/2922903002/diff/60001/ui/message_center/views... File ui/message_center/views/notification_view_md.cc (right): https://codereview.chromium.org/2922903002/diff/60001/ui/message_center/views... ui/message_center/views/notification_view_md.cc:112: new views::Label(item.title + base::ASCIIToUTF16(" - ") + item.message); Can we have darker color for item.title than item.message, to have similar style with bundled notifications in Android? https://codereview.chromium.org/2922903002/diff/60001/ui/message_center/views... ui/message_center/views/notification_view_md.cc:112: new views::Label(item.title + base::ASCIIToUTF16(" - ") + item.message); We should have separate between title and message only when both title and message are non-empty. https://codereview.chromium.org/2922903002/diff/60001/ui/message_center/views... ui/message_center/views/notification_view_md.cc:534: if (item_views_.size() > 1) { nit: We usually omit curly braces for 1-line if statements. https://codereview.chromium.org/2922903002/diff/60001/ui/message_center/views... File ui/message_center/views/notification_view_md.h (right): https://codereview.chromium.org/2922903002/diff/60001/ui/message_center/views... ui/message_center/views/notification_view_md.h:102: std::vector<views::View*> item_views_; nit: Remove the blank line above, since item_views_ is a part of "Views which are dinamically ...(snip"
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2922903002/diff/60001/ui/message_center/views... File ui/message_center/views/notification_view_md.cc (right): https://codereview.chromium.org/2922903002/diff/60001/ui/message_center/views... ui/message_center/views/notification_view_md.cc:112: new views::Label(item.title + base::ASCIIToUTF16(" - ") + item.message); On 2017/06/13 04:27:09, fukino wrote: > Can we have darker color for item.title than item.message, to have similar style > with bundled notifications in Android? In my understanding, list notification and bundle notification are different, and at this point we have it grey for consistency as Android also has this style for list notification. https://docs.google.com/a/google.com/document/d/1VXM-Zp_aGDKj_v4l1gTfYjCL8E3x... https://codereview.chromium.org/2922903002/diff/60001/ui/message_center/views... ui/message_center/views/notification_view_md.cc:112: new views::Label(item.title + base::ASCIIToUTF16(" - ") + item.message); On 2017/06/13 04:27:09, fukino wrote: > We should have separate between title and message only when both title and > message are non-empty. Done. https://codereview.chromium.org/2922903002/diff/60001/ui/message_center/views... ui/message_center/views/notification_view_md.cc:534: if (item_views_.size() > 1) { On 2017/06/13 04:27:09, fukino wrote: > nit: We usually omit curly braces for 1-line if statements. Done. https://codereview.chromium.org/2922903002/diff/60001/ui/message_center/views... File ui/message_center/views/notification_view_md.h (right): https://codereview.chromium.org/2922903002/diff/60001/ui/message_center/views... ui/message_center/views/notification_view_md.h:102: std::vector<views::View*> item_views_; On 2017/06/13 04:27:09, fukino wrote: > nit: Remove the blank line above, since item_views_ is a part of "Views which > are dinamically ...(snip" 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...
Fixed items that we discussed offline. 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...
lgtm with a nit. But please wait yoshiki@'s owner review. https://codereview.chromium.org/2922903002/diff/100001/ui/message_center/view... File ui/message_center/views/notification_view_md.cc (right): https://codereview.chromium.org/2922903002/diff/100001/ui/message_center/view... ui/message_center/views/notification_view_md.cc:10: #include "base/strings/utf_string_conversions.h" seems unnecessary?
fukino@: Thank you! https://codereview.chromium.org/2922903002/diff/100001/ui/message_center/view... File ui/message_center/views/notification_view_md.cc (right): https://codereview.chromium.org/2922903002/diff/100001/ui/message_center/view... ui/message_center/views/notification_view_md.cc:10: #include "base/strings/utf_string_conversions.h" On 2017/06/13 05:48:24, fukino wrote: > seems unnecessary? 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...
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...
yoshiki@: Thank you for review. Fixed to use InvalidateLayout as discussed offline.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
Thank you!
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 Link to the patchset: https://codereview.chromium.org/2922903002/#ps140001 (title: "Use InvalidateLayout.")
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": 140001, "attempt_start_ts": 1497406072892750,
"parent_rev": "18b83154103fb0b1b6957d158afb41389e4d0b22", "commit_rev":
"fe8c4f9dde53e94cdebf2f7792b7dfea203df13e"}
Message was sent while issue was closed.
Description was changed from ========== Add List notification support to new-style notification. BUG=726244 ========== to ========== Add List notification support to new-style notification. BUG=726244 Review-Url: https://codereview.chromium.org/2922903002 Cr-Commit-Position: refs/heads/master@{#479254} Committed: https://chromium.googlesource.com/chromium/src/+/fe8c4f9dde53e94cdebf2f7792b7... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/fe8c4f9dde53e94cdebf2f7792b7... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
