Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1958903003:
Show the bell icon when no unread-notification state (Closed)
DescriptionShow the bell icon when no unread-notification state
This patch shows the bell icon instead of the number "0" in the unread button in the status bar, when there are no unread notifications in message center.
BUG=599891
TEST=manually
Committed: https://crrev.com/821655984efbc9151a6182e56854f6bc01856dc5
Cr-Commit-Position: refs/heads/master@{#393215}
Patch Set 1 #Patch Set 2 #
Total comments: 5
Patch Set 3 : Addressed comments #Patch Set 4 : Change the names of asset #Patch Set 5 : Changed asset name (again) #Patch Set 6 : Rebased #
Messages
Total messages: 48 (25 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Description was changed from ========== wip BUG= ========== to ========== Show the ring icon when no unread-notification state This patch shows the bell icon instead of the number "0" when there are no unread notifications in message center. BUG=599891 ==========
yoshiki@chromium.org changed reviewers: + skuhne@chromium.org
Description was changed from ========== Show the ring icon when no unread-notification state This patch shows the bell icon instead of the number "0" when there are no unread notifications in message center. BUG=599891 ========== to ========== Show the ring icon when no unread-notification state This patch shows the bell icon instead of the number "0" when there are no unread notifications in message center. BUG=599891 TEST=manually ==========
skuhne@, PTAL Thanks.
Description was changed from ========== Show the ring icon when no unread-notification state This patch shows the bell icon instead of the number "0" when there are no unread notifications in message center. BUG=599891 TEST=manually ========== to ========== Show the bell icon when no unread-notification state This patch shows the bell icon instead of the number "0" in the unread button in the status bar, when there are no unread notifications in message center. BUG=599891 TEST=manually ==========
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
lgtm
The CQ bit was checked by yoshiki@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1958903003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1958903003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
yoshiki@chromium.org changed reviewers: + oshima@chromium.org
oshima@, PTAL at changes in ash/resources?
https://codereview.chromium.org/1958903003/diff/100001/ash/system/web_notific... File ash/system/web_notification/web_notification_tray.cc (right): https://codereview.chromium.org/1958903003/diff/100001/ash/system/web_notific... ash/system/web_notification/web_notification_tray.cc:127: rb.GetImageNamed(IDR_ASH_SHELF_NOTIFICATION_TRAY_EMPTY).ToImageSkia()); do you really need an image view here? Doesn't empty view work? https://codereview.chromium.org/1958903003/diff/100001/ash/system/web_notific... ash/system/web_notification/web_notification_tray.cc:131: unread_label_.reset(new views::Label()); you can just use view::Label unread_label_;
Oshima-san, PTAL https://codereview.chromium.org/1958903003/diff/100001/ash/system/web_notific... File ash/system/web_notification/web_notification_tray.cc (right): https://codereview.chromium.org/1958903003/diff/100001/ash/system/web_notific... ash/system/web_notification/web_notification_tray.cc:127: rb.GetImageNamed(IDR_ASH_SHELF_NOTIFICATION_TRAY_EMPTY).ToImageSkia()); On 2016/05/11 02:46:30, oshima wrote: > do you really need an image view here? Doesn't empty view work? Yes, the initial state is unread_count == 0, so we want to show a bell icon at first. https://codereview.chromium.org/1958903003/diff/100001/ash/system/web_notific... ash/system/web_notification/web_notification_tray.cc:131: unread_label_.reset(new views::Label()); On 2016/05/11 02:46:30, oshima wrote: > you can just use view::Label unread_label_; Done.
https://codereview.chromium.org/1958903003/diff/100001/ash/system/web_notific... File ash/system/web_notification/web_notification_tray.cc (right): https://codereview.chromium.org/1958903003/diff/100001/ash/system/web_notific... ash/system/web_notification/web_notification_tray.cc:127: rb.GetImageNamed(IDR_ASH_SHELF_NOTIFICATION_TRAY_EMPTY).ToImageSkia()); On 2016/05/11 05:42:51, yoshiki wrote: > On 2016/05/11 02:46:30, oshima wrote: > > do you really need an image view here? Doesn't empty view work? > > Yes, the initial state is unread_count == 0, so we want to show a bell icon at > first. which one is bell icon? Or empty.png is just a place holder?
On 2016/05/11 14:35:10, oshima wrote: > https://codereview.chromium.org/1958903003/diff/100001/ash/system/web_notific... > File ash/system/web_notification/web_notification_tray.cc (right): > > https://codereview.chromium.org/1958903003/diff/100001/ash/system/web_notific... > ash/system/web_notification/web_notification_tray.cc:127: > rb.GetImageNamed(IDR_ASH_SHELF_NOTIFICATION_TRAY_EMPTY).ToImageSkia()); > On 2016/05/11 05:42:51, yoshiki wrote: > > On 2016/05/11 02:46:30, oshima wrote: > > > do you really need an image view here? Doesn't empty view work? > > > > Yes, the initial state is unread_count == 0, so we want to show a bell icon at > > first. > > which one is bell icon? Or empty.png is just a place holder? empty.png has a white bell and transparent background.
On 2016/05/12 00:18:31, yoshiki wrote: > On 2016/05/11 14:35:10, oshima wrote: > > > https://codereview.chromium.org/1958903003/diff/100001/ash/system/web_notific... > > File ash/system/web_notification/web_notification_tray.cc (right): > > > > > https://codereview.chromium.org/1958903003/diff/100001/ash/system/web_notific... > > ash/system/web_notification/web_notification_tray.cc:127: > > rb.GetImageNamed(IDR_ASH_SHELF_NOTIFICATION_TRAY_EMPTY).ToImageSkia()); > > On 2016/05/11 05:42:51, yoshiki wrote: > > > On 2016/05/11 02:46:30, oshima wrote: > > > > do you really need an image view here? Doesn't empty view work? > > > > > > Yes, the initial state is unread_count == 0, so we want to show a bell icon > at > > > first. > > > > which one is bell icon? Or empty.png is just a place holder? > > empty.png has a white bell and transparent background. Ah, i see. Rietveld showed it just empty and that's why I was confused. can you rename to _bell or something more meaningful name? Then lgtm.
On 2016/05/12 00:18:31, yoshiki wrote: > On 2016/05/11 14:35:10, oshima wrote: > > > https://codereview.chromium.org/1958903003/diff/100001/ash/system/web_notific... > > File ash/system/web_notification/web_notification_tray.cc (right): > > > > > https://codereview.chromium.org/1958903003/diff/100001/ash/system/web_notific... > > ash/system/web_notification/web_notification_tray.cc:127: > > rb.GetImageNamed(IDR_ASH_SHELF_NOTIFICATION_TRAY_EMPTY).ToImageSkia()); > > On 2016/05/11 05:42:51, yoshiki wrote: > > > On 2016/05/11 02:46:30, oshima wrote: > > > > do you really need an image view here? Doesn't empty view work? > > > > > > Yes, the initial state is unread_count == 0, so we want to show a bell icon > at > > > first. > > > > which one is bell icon? Or empty.png is just a place holder? > > empty.png has a white bell and transparent background. I changed the name of asset to prevent confusion.
On 2016/05/12 00:23:32, oshima wrote: > On 2016/05/12 00:18:31, yoshiki wrote: > > On 2016/05/11 14:35:10, oshima wrote: > > > > > > https://codereview.chromium.org/1958903003/diff/100001/ash/system/web_notific... > > > File ash/system/web_notification/web_notification_tray.cc (right): > > > > > > > > > https://codereview.chromium.org/1958903003/diff/100001/ash/system/web_notific... > > > ash/system/web_notification/web_notification_tray.cc:127: > > > rb.GetImageNamed(IDR_ASH_SHELF_NOTIFICATION_TRAY_EMPTY).ToImageSkia()); > > > On 2016/05/11 05:42:51, yoshiki wrote: > > > > On 2016/05/11 02:46:30, oshima wrote: > > > > > do you really need an image view here? Doesn't empty view work? > > > > > > > > Yes, the initial state is unread_count == 0, so we want to show a bell > icon > > at > > > > first. > > > > > > which one is bell icon? Or empty.png is just a place holder? > > > > empty.png has a white bell and transparent background. > > Ah, i see. Rietveld showed it just empty and that's why I was confused. > can you rename to _bell or something more meaningful name? Then lgtm. Got it. Once I changed _no_notification, but will change it _bell and then will commit. Thanks.
Patchset #5 (id:160001) has been deleted
The CQ bit was checked by yoshiki@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skuhne@chromium.org, oshima@chromium.org Link to the patchset: https://codereview.chromium.org/1958903003/#ps180001 (title: "Changed asset name (again)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1958903003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1958903003/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
The CQ bit was checked by yoshiki@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1958903003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1958903003/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by yoshiki@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1958903003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1958903003/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
The CQ bit was checked by yoshiki@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1958903003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1958903003/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
The CQ bit was checked by yoshiki@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org, skuhne@chromium.org Link to the patchset: https://codereview.chromium.org/1958903003/#ps200001 (title: "Rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1958903003/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1958903003/200001
Message was sent while issue was closed.
Description was changed from ========== Show the bell icon when no unread-notification state This patch shows the bell icon instead of the number "0" in the unread button in the status bar, when there are no unread notifications in message center. BUG=599891 TEST=manually ========== to ========== Show the bell icon when no unread-notification state This patch shows the bell icon instead of the number "0" in the unread button in the status bar, when there are no unread notifications in message center. BUG=599891 TEST=manually ==========
Message was sent while issue was closed.
Committed patchset #6 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Show the bell icon when no unread-notification state This patch shows the bell icon instead of the number "0" in the unread button in the status bar, when there are no unread notifications in message center. BUG=599891 TEST=manually ========== to ========== Show the bell icon when no unread-notification state This patch shows the bell icon instead of the number "0" in the unread button in the status bar, when there are no unread notifications in message center. BUG=599891 TEST=manually Committed: https://crrev.com/821655984efbc9151a6182e56854f6bc01856dc5 Cr-Commit-Position: refs/heads/master@{#393215} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/821655984efbc9151a6182e56854f6bc01856dc5 Cr-Commit-Position: refs/heads/master@{#393215} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
