New issue
Advanced search Search tips

Issue 840957 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 23
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug



Sign in to add a comment

CreateSystemNotification no longer needs icon parameter

Project Member Reported by est...@chromium.org, May 8 2018

Issue description

I believe all callers pass gfx::Image() for |icon| now.
 
Cc: est...@chromium.org
Owner: sgabr...@chromium.org
It looks like there are a couple notifications that still use icon in NetworkStateNotifier. They are shown for cellular activation, both success and failure. One example attached.

https://cs.chromium.org/chromium/src/chrome/app/theme/default_200_percent/cros/notification_3g.png
https://cs.chromium.org/chromium/src/chrome/app/theme/default_200_percent/cros/notification_lte.png
https://cs.chromium.org/chromium/src/chrome/app/theme/default_200_percent/cros/status_cellular_failed.png

Should these also be updated to some new template?
lte.png
13.6 KB View Download

Comment 2 by est...@chromium.org, Jun 15 2018

ping Sebastien
Cc: -est...@chromium.org sgabr...@chromium.org
Owner: est...@chromium.org
We should remove the icon to only keep the badges. 
Use "ic_notification_data" for the 3G and LTE "ic_notification_data_off" for cellular failed. I'm not sure we need distinction in the type of network and if we do, it should be in the notification copy.

The assets for the notification badges should be already available but I re-attached them here.
ic_notification_data_off.svg
356 bytes Download
ic_notification_data.svg
252 bytes Download
to be clear, by "badge" you mean the icon in the top left, correct? (currently red chromium icon)
Yes, I believe this is the nomenclature used in the dev doc. "Icon" being the big one on the right (LTE in #!)
Project Member

Comment 6 by bugdroid1@chromium.org, Aug 8

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/35e870cd3e1da9027c25865fa825557d12281db8

commit 35e870cd3e1da9027c25865fa825557d12281db8
Author: Evan Stade <estade@chromium.org>
Date: Wed Aug 08 19:04:09 2018

Chrome OS: Remove gfx::Image parameter from CreateSystemNotification

TBR=sky@chromium.org

Bug:  840957 
Change-Id: I72b9675431e8d5ecb8992884869c93485b6b1742
Reviewed-on: https://chromium-review.googlesource.com/1166198
Reviewed-by: Evan Stade <estade@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Commit-Queue: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581647}
[modify] https://crrev.com/35e870cd3e1da9027c25865fa825557d12281db8/ash/accelerators/accelerator_controller.cc
[modify] https://crrev.com/35e870cd3e1da9027c25865fa825557d12281db8/ash/accessibility/accessibility_controller.cc
[modify] https://crrev.com/35e870cd3e1da9027c25865fa825557d12281db8/ash/assistant/assistant_notification_controller.cc
[modify] https://crrev.com/35e870cd3e1da9027c25865fa825557d12281db8/ash/detachable_base/detachable_base_notification_controller.cc
[modify] https://crrev.com/35e870cd3e1da9027c25865fa825557d12281db8/ash/display/display_util.cc
[modify] https://crrev.com/35e870cd3e1da9027c25865fa825557d12281db8/ash/display/resolution_notification_controller.cc
[modify] https://crrev.com/35e870cd3e1da9027c25865fa825557d12281db8/ash/multi_device_setup/multi_device_notification_presenter.cc
[modify] https://crrev.com/35e870cd3e1da9027c25865fa825557d12281db8/ash/system/bluetooth/bluetooth_notification_controller.cc
[modify] https://crrev.com/35e870cd3e1da9027c25865fa825557d12281db8/ash/system/caps_lock_notification_controller.cc
[modify] https://crrev.com/35e870cd3e1da9027c25865fa825557d12281db8/ash/system/cast/cast_notification_controller.cc
[modify] https://crrev.com/35e870cd3e1da9027c25865fa825557d12281db8/ash/system/locale/locale_notification_controller.cc
[modify] https://crrev.com/35e870cd3e1da9027c25865fa825557d12281db8/ash/system/network/auto_connect_notifier.cc
[modify] https://crrev.com/35e870cd3e1da9027c25865fa825557d12281db8/ash/system/network/sms_observer.cc
[modify] https://crrev.com/35e870cd3e1da9027c25865fa825557d12281db8/ash/system/power/battery_notification.cc
[modify] https://crrev.com/35e870cd3e1da9027c25865fa825557d12281db8/ash/system/power/dual_role_notification.cc
[modify] https://crrev.com/35e870cd3e1da9027c25865fa825557d12281db8/ash/system/power/peripheral_battery_notifier.cc
[modify] https://crrev.com/35e870cd3e1da9027c25865fa825557d12281db8/ash/system/power/power_notification_controller.cc
[modify] https://crrev.com/35e870cd3e1da9027c25865fa825557d12281db8/ash/system/screen_layout_observer.cc
[modify] https://crrev.com/35e870cd3e1da9027c25865fa825557d12281db8/ash/system/screen_security/screen_security_notification_controller.cc
[modify] https://crrev.com/35e870cd3e1da9027c25865fa825557d12281db8/ash/system/session/session_limit_notification_controller.cc
[modify] https://crrev.com/35e870cd3e1da9027c25865fa825557d12281db8/ash/system/supervised/supervised_notification_controller.cc
[modify] https://crrev.com/35e870cd3e1da9027c25865fa825557d12281db8/ash/system/tracing_notification_controller.cc
[modify] https://crrev.com/35e870cd3e1da9027c25865fa825557d12281db8/ash/system/update/update_notification_controller.cc
[delete] https://crrev.com/2e418ffe72bff200c66094c64143d8c8bcaf9959/chrome/app/theme/default_100_percent/cros/notification_3g.png
[delete] https://crrev.com/2e418ffe72bff200c66094c64143d8c8bcaf9959/chrome/app/theme/default_100_percent/cros/notification_lte.png
[delete] https://crrev.com/2e418ffe72bff200c66094c64143d8c8bcaf9959/chrome/app/theme/default_100_percent/cros/status_cellular_failed.png
[modify] https://crrev.com/35e870cd3e1da9027c25865fa825557d12281db8/chrome/app/theme/theme_resources.grd
[modify] https://crrev.com/35e870cd3e1da9027c25865fa825557d12281db8/chrome/browser/chromeos/arc/arc_migration_guide_notification.cc
[modify] https://crrev.com/35e870cd3e1da9027c25865fa825557d12281db8/chrome/browser/chromeos/arc/notification/arc_boot_error_notification.cc
[modify] https://crrev.com/35e870cd3e1da9027c25865fa825557d12281db8/chrome/browser/chromeos/arc/notification/arc_supervision_transition_notification.cc
[modify] https://crrev.com/35e870cd3e1da9027c25865fa825557d12281db8/chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc
[modify] https://crrev.com/35e870cd3e1da9027c25865fa825557d12281db8/chrome/browser/chromeos/child_accounts/screen_time_controller.cc
[modify] https://crrev.com/35e870cd3e1da9027c25865fa825557d12281db8/chrome/browser/chromeos/eol_notification.cc
[modify] https://crrev.com/35e870cd3e1da9027c25865fa825557d12281db8/chrome/browser/chromeos/hats/hats_notification_controller.cc
[modify] https://crrev.com/35e870cd3e1da9027c25865fa825557d12281db8/chrome/browser/chromeos/ui/low_disk_notification.cc
[modify] https://crrev.com/35e870cd3e1da9027c25865fa825557d12281db8/chrome/browser/signin/signin_error_notifier_ash.cc
[modify] https://crrev.com/35e870cd3e1da9027c25865fa825557d12281db8/chrome/browser/sync/sync_error_notifier_ash.cc
[modify] https://crrev.com/35e870cd3e1da9027c25865fa825557d12281db8/chrome/browser/ui/ash/chrome_screenshot_grabber.cc
[modify] https://crrev.com/35e870cd3e1da9027c25865fa825557d12281db8/chrome/browser/ui/ash/network/data_promo_notification.cc
[modify] https://crrev.com/35e870cd3e1da9027c25865fa825557d12281db8/chrome/browser/ui/ash/network/network_portal_notification_controller.cc
[modify] https://crrev.com/35e870cd3e1da9027c25865fa825557d12281db8/chrome/browser/ui/ash/network/network_state_notifier.cc
[modify] https://crrev.com/35e870cd3e1da9027c25865fa825557d12281db8/chrome/browser/ui/ash/network/tether_notification_presenter.cc
[modify] https://crrev.com/35e870cd3e1da9027c25865fa825557d12281db8/chrome/browser/ui/extensions/extension_installed_notification.cc
[modify] https://crrev.com/35e870cd3e1da9027c25865fa825557d12281db8/ui/message_center/public/cpp/notification.cc
[modify] https://crrev.com/35e870cd3e1da9027c25865fa825557d12281db8/ui/message_center/public/cpp/notification.h

Status: Fixed (was: Assigned)

Sign in to add a comment