New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 797558 link

Starred by 3 users
Status: Fixed
Owner:
est...@chromium.org
Closed: Nov 8
Cc:
█ yoshiki@chromium.org
jamescook@chromium.org
est...@chromium.org
peter@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug



Sign in to add a comment

Move Notification::CreateSystemNotification to ash/

Project Member Reported by tetsui@chromium.org, Dec 25 2017 
message_center::Notification::CreateSystemNotification() is a function to create Chrome OS system notification. By default, the notification created by the function is given a default |display_source| "Chrome OS system". The function is only used on Chrome OS.

ui/message_center is shared among all platforms, so it is not the right place to put this function. Probably ash/public/cpp would be the best place. Also, MessageCenter::GetProductOSName() (which returns "Chrome OS" or "Chromium OS") should be removed.

There are two problems:
* CreateSystemNotification is used in many places under chrome/browser e.g. chrome/browser/chromeos.
* ash/ does not have google_chrome_strings.grd/chromium_strings.grd equivalent.
  * We would need some shared strings file that gets compiled into both executables.

Comment 1 by tetsui@chromium.org, Dec 25 2017 

Some related discussion here: https://crrev.com/c/830984

Comment 2 by est...@chromium.org, Jan 12 2018

Owner: est...@chromium.org
Status: Started (was: Assigned)
I started looking at this today (to remove the dependency from Notification to MessageCenter) and I think our current setup is mostly ok:

- users of the new style notifications (hopefully one day that includes Linux and Windows) have to tell the message center the name of the system. In the case of Chrome OS, this is "Chrome OS system", although why we have a string that expands to "[...] system system" is beyond me.
- notifications that are missing an app name, i.e. a display name describing the notifier, i.e. something that should probably actually be in NotifierId and not Notification, will be assigned this name by the view.
- CreateSystemNotification could/should hypothetically be used for a few non-Chrome OS notifications, like the webusb notification. Thus it can't live in ash/.

Our current setup almost accomplishes this; all we have to do is change the exact string the MessageCenter holds onto and where it's applied.
Project Member

Comment 3 by bugdroid1@chromium.org, Jan 18 2018 

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

commit 491ec4cd9d4fe1971c16c10ccfbe27e4a85f9f49
Author: Evan Stade <estade@chromium.org>
Date: Thu Jan 18 01:09:26 2018

Move default display source for system notifications

Apply it in NotificationViewMd rather than Notification. This removes
the dependency from Notification to MessageCenter.

Also introduce a new string instead of trying to compose two strings at
runtime.

Bug:  797558 
Change-Id: I71d794d24d1030f11b675f96e80b0e5562ead142
Reviewed-on: https://chromium-review.googlesource.com/862923
Commit-Queue: Evan Stade <estade@chromium.org>
Reviewed-by: Tetsui Ohkubo <tetsui@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#529992}
[modify] https://crrev.com/491ec4cd9d4fe1971c16c10ccfbe27e4a85f9f49/ash/system/network/sms_observer.cc
[modify] https://crrev.com/491ec4cd9d4fe1971c16c10ccfbe27e4a85f9f49/chrome/browser/chromeos/chrome_browser_main_chromeos.cc
[modify] https://crrev.com/491ec4cd9d4fe1971c16c10ccfbe27e4a85f9f49/ui/message_center/fake_message_center.cc
[modify] https://crrev.com/491ec4cd9d4fe1971c16c10ccfbe27e4a85f9f49/ui/message_center/fake_message_center.h
[modify] https://crrev.com/491ec4cd9d4fe1971c16c10ccfbe27e4a85f9f49/ui/message_center/message_center.h
[modify] https://crrev.com/491ec4cd9d4fe1971c16c10ccfbe27e4a85f9f49/ui/message_center/message_center_impl.cc
[modify] https://crrev.com/491ec4cd9d4fe1971c16c10ccfbe27e4a85f9f49/ui/message_center/message_center_impl.h
[modify] https://crrev.com/491ec4cd9d4fe1971c16c10ccfbe27e4a85f9f49/ui/message_center/notification.cc
[modify] https://crrev.com/491ec4cd9d4fe1971c16c10ccfbe27e4a85f9f49/ui/message_center/views/notification_view_md.cc
Project Member

Comment 4 by bugdroid1@chromium.org, Nov 8 

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

commit 70e2ed461d515973d629587c547be37acd2d0359
Author: Evan Stade <estade@chromium.org>
Date: Thu Nov 08 06:23:05 2018

Move CreateSystemNotification from MessageCenter to Ash.

Bug:  797558 
Change-Id: I859f90bb23f4a9ff6550d2a2bffcdb71ba73282d
Reviewed-on: https://chromium-review.googlesource.com/c/1324148
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606366}
[modify] https://crrev.com/70e2ed461d515973d629587c547be37acd2d0359/ash/accelerators/accelerator_controller.cc
[modify] https://crrev.com/70e2ed461d515973d629587c547be37acd2d0359/ash/accessibility/accessibility_controller.cc
[modify] https://crrev.com/70e2ed461d515973d629587c547be37acd2d0359/ash/assistant/assistant_notification_controller.cc
[modify] https://crrev.com/70e2ed461d515973d629587c547be37acd2d0359/ash/detachable_base/detachable_base_notification_controller.cc
[modify] https://crrev.com/70e2ed461d515973d629587c547be37acd2d0359/ash/display/display_util.cc
[modify] https://crrev.com/70e2ed461d515973d629587c547be37acd2d0359/ash/display/resolution_notification_controller.cc
[modify] https://crrev.com/70e2ed461d515973d629587c547be37acd2d0359/ash/media/media_notification_controller.cc
[modify] https://crrev.com/70e2ed461d515973d629587c547be37acd2d0359/ash/multi_device_setup/multi_device_notification_presenter.cc
[modify] https://crrev.com/70e2ed461d515973d629587c547be37acd2d0359/ash/public/cpp/BUILD.gn
[add] https://crrev.com/70e2ed461d515973d629587c547be37acd2d0359/ash/public/cpp/notification_utils.cc
[add] https://crrev.com/70e2ed461d515973d629587c547be37acd2d0359/ash/public/cpp/notification_utils.h
[modify] https://crrev.com/70e2ed461d515973d629587c547be37acd2d0359/ash/system/bluetooth/bluetooth_notification_controller.cc
[modify] https://crrev.com/70e2ed461d515973d629587c547be37acd2d0359/ash/system/caps_lock_notification_controller.cc
[modify] https://crrev.com/70e2ed461d515973d629587c547be37acd2d0359/ash/system/cast/cast_notification_controller.cc
[modify] https://crrev.com/70e2ed461d515973d629587c547be37acd2d0359/ash/system/locale/locale_notification_controller.cc
[modify] https://crrev.com/70e2ed461d515973d629587c547be37acd2d0359/ash/system/network/auto_connect_notifier.cc
[modify] https://crrev.com/70e2ed461d515973d629587c547be37acd2d0359/ash/system/network/sms_observer.cc
[modify] https://crrev.com/70e2ed461d515973d629587c547be37acd2d0359/ash/system/power/battery_notification.cc
[modify] https://crrev.com/70e2ed461d515973d629587c547be37acd2d0359/ash/system/power/dual_role_notification.cc
[modify] https://crrev.com/70e2ed461d515973d629587c547be37acd2d0359/ash/system/power/peripheral_battery_notifier.cc
[modify] https://crrev.com/70e2ed461d515973d629587c547be37acd2d0359/ash/system/power/power_notification_controller.cc
[modify] https://crrev.com/70e2ed461d515973d629587c547be37acd2d0359/ash/system/screen_layout_observer.cc
[modify] https://crrev.com/70e2ed461d515973d629587c547be37acd2d0359/ash/system/screen_security/screen_security_notification_controller.cc
[modify] https://crrev.com/70e2ed461d515973d629587c547be37acd2d0359/ash/system/session/session_limit_notification_controller.cc
[modify] https://crrev.com/70e2ed461d515973d629587c547be37acd2d0359/ash/system/supervised/supervised_notification_controller.cc
[modify] https://crrev.com/70e2ed461d515973d629587c547be37acd2d0359/ash/system/tracing_notification_controller.cc
[modify] https://crrev.com/70e2ed461d515973d629587c547be37acd2d0359/ash/system/update/update_notification_controller.cc
[modify] https://crrev.com/70e2ed461d515973d629587c547be37acd2d0359/chrome/browser/chromeos/arc/arc_migration_guide_notification.cc
[modify] https://crrev.com/70e2ed461d515973d629587c547be37acd2d0359/chrome/browser/chromeos/arc/notification/arc_boot_error_notification.cc
[modify] https://crrev.com/70e2ed461d515973d629587c547be37acd2d0359/chrome/browser/chromeos/arc/notification/arc_supervision_transition_notification.cc
[modify] https://crrev.com/70e2ed461d515973d629587c547be37acd2d0359/chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc
[modify] https://crrev.com/70e2ed461d515973d629587c547be37acd2d0359/chrome/browser/chromeos/child_accounts/time_limit_notifier.cc
[modify] https://crrev.com/70e2ed461d515973d629587c547be37acd2d0359/chrome/browser/chromeos/crostini/crostini_package_installer_notification.cc
[modify] https://crrev.com/70e2ed461d515973d629587c547be37acd2d0359/chrome/browser/chromeos/eol_notification.cc
[modify] https://crrev.com/70e2ed461d515973d629587c547be37acd2d0359/chrome/browser/chromeos/hats/hats_notification_controller.cc
[modify] https://crrev.com/70e2ed461d515973d629587c547be37acd2d0359/chrome/browser/chromeos/printing/cups_print_job_notification.cc
[modify] https://crrev.com/70e2ed461d515973d629587c547be37acd2d0359/chrome/browser/chromeos/tpm_firmware_update_notification.cc
[modify] https://crrev.com/70e2ed461d515973d629587c547be37acd2d0359/chrome/browser/chromeos/ui/low_disk_notification.cc
[modify] https://crrev.com/70e2ed461d515973d629587c547be37acd2d0359/chrome/browser/download/notification/download_item_notification.cc
[modify] https://crrev.com/70e2ed461d515973d629587c547be37acd2d0359/chrome/browser/signin/signin_error_notifier_ash.cc
[modify] https://crrev.com/70e2ed461d515973d629587c547be37acd2d0359/chrome/browser/sync/sync_error_notifier_ash.cc
[modify] https://crrev.com/70e2ed461d515973d629587c547be37acd2d0359/chrome/browser/ui/ash/assistant/assistant_setup.cc
[modify] https://crrev.com/70e2ed461d515973d629587c547be37acd2d0359/chrome/browser/ui/ash/chrome_screenshot_grabber.cc
[modify] https://crrev.com/70e2ed461d515973d629587c547be37acd2d0359/chrome/browser/ui/ash/network/data_promo_notification.cc
[modify] https://crrev.com/70e2ed461d515973d629587c547be37acd2d0359/chrome/browser/ui/ash/network/network_portal_notification_controller.cc
[modify] https://crrev.com/70e2ed461d515973d629587c547be37acd2d0359/chrome/browser/ui/ash/network/network_state_notifier.cc
[modify] https://crrev.com/70e2ed461d515973d629587c547be37acd2d0359/chrome/browser/ui/ash/network/tether_notification_presenter.cc
[modify] https://crrev.com/70e2ed461d515973d629587c547be37acd2d0359/chrome/browser/ui/extensions/extension_installed_notification.cc
[modify] https://crrev.com/70e2ed461d515973d629587c547be37acd2d0359/ui/message_center/public/cpp/message_center_constants.h
[modify] https://crrev.com/70e2ed461d515973d629587c547be37acd2d0359/ui/message_center/public/cpp/notification.cc
[modify] https://crrev.com/70e2ed461d515973d629587c547be37acd2d0359/ui/message_center/public/cpp/notification.h

Comment 5 by est...@chromium.org, Nov 8

Status: Fixed (was: Started)
►

Sign in to add a comment