New issue
Advanced search Search tips

Issue 768081 link

Starred by 2 users

Issue metadata

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



Sign in to add a comment

Getting rid of ash/system/system_notifier.h access in chrome

Project Member Reported by est...@chromium.org, Sep 22 2017

Issue description

Chromeos code in chrome/ accesses this header in a lot of places. James says (and I concur):

"I think it's because ash uses the ID constants in that file to decide whether or not to show notifications at the lock/login screen. It seems like that behavior should be a property of the notification itself, rather than communicated through shared IDs."

It's also used to determine whether to show notifications from a different user (one that's logged in but not active). We could move these concepts into the Notification or the NotifierId, but then there are other very closely related concepts like whether a notification should be shown over fullscreen, which is instead handled via NotificationDelegate::ShouldDisplayOverFullscreen() (and this can't be set statically as a boolean member as it depends on browser state). Perhaps matching that pattern is the right choice.

On the other hand, both the login/lock screen and the multiuser scenarios are ash-specific and it seems like any ash system notification ought to be generated in ash/. I wonder how many of the references to ash system notifiers can just be moved to ash/.
 
Components: UI>Shell>Notifications
Labels: -Proj-Mustash Proj-Mustash-Mash
It looks like there are 10-ish notifiers from chrome.

https://cs.chromium.org/search/?q=system_notifier::kNotifier+f:src/chrome&sq=package:chromium&type=cs

I'm not sure how hard or easy they might be to migrate to ash.

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

Cc: steve...@chromium.org
Project Member

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

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

commit 2395de65c82b1d877b9e6530768dd5a33ba1177c
Author: Evan Stade <estade@chromium.org>
Date: Mon Jan 15 21:40:10 2018

Remove ash::system_notifier.

This file was used as a centralized list of ash "system" notifiers
which allowed various blockers to control when notifications were
visible (on the lock screen, across user sessions, etc.). There's no
particular reason this list needs to be centralized, and making it the
responsibility of the code generating the notification better
encapsulates logic for individual notifications.

The word "system" in the context of notifications has many meanings.
- There is a SYSTEM_COMPONENT type, which is used by most things that
  are not web pages, extensions, or ARC.
- There is a SYSTEM_PRIORITY priority, which is used for very
  notifications that should not ever time out, but is not used
  consistently across SYSTEM_COMPONENT notifications.
- There are two functions both called CreateSystemNotification, only
  one of which sets SYSTEM_PRIORITY.
- This patch deletes a function called IsAshSystemNotifier which
  actually only returned true for a subset of notifications, and was
  used specifically to determine if a notification should appear
  regardless of the active user.

IsAshSystemNotifier is replaced by checking if there's an associated
user. If there is no associated user, it is assumed the notification
should be shown regardless of the active user.

SYSTEM_COMPONENT notifications that were in the special list for
showing on top of fullsreen, lock screen, and login screen, are now
set to SYSTEM_PRIORITY. The appropriate notification blockers check
priority instead of the centralized list.

Mostly there should be no behavioral changes, but some notifications
were already set to SYSTEM_PRIORITY yet did not show over fullscreen,
etc., and those will now enjoy the added prestige that was formerly
reserved for notifications in the kAlwaysShownSystemNotifierIds list.
Also, some other aspects of SYSTEM_PRIORITY will now be granted to
the notifications that were in that last. Hopefully this makes
behavior more consistent and predictable across system notifications.

Bug:  768081 
Change-Id: I0ee9cd727036df4bdc6fbff293e527b737d756cc
Reviewed-on: https://chromium-review.googlesource.com/860223
Commit-Queue: Evan Stade <estade@chromium.org>
Reviewed-by: Tetsui Ohkubo <tetsui@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#529330}
[modify] https://crrev.com/2395de65c82b1d877b9e6530768dd5a33ba1177c/ash/BUILD.gn
[modify] https://crrev.com/2395de65c82b1d877b9e6530768dd5a33ba1177c/ash/accelerators/accelerator_controller.cc
[modify] https://crrev.com/2395de65c82b1d877b9e6530768dd5a33ba1177c/ash/display/display_util.cc
[modify] https://crrev.com/2395de65c82b1d877b9e6530768dd5a33ba1177c/ash/display/resolution_notification_controller.cc
[modify] https://crrev.com/2395de65c82b1d877b9e6530768dd5a33ba1177c/ash/system/bluetooth/bluetooth_notification_controller.cc
[modify] https://crrev.com/2395de65c82b1d877b9e6530768dd5a33ba1177c/ash/system/locale/locale_notification_controller.cc
[modify] https://crrev.com/2395de65c82b1d877b9e6530768dd5a33ba1177c/ash/system/network/sms_observer.cc
[modify] https://crrev.com/2395de65c82b1d877b9e6530768dd5a33ba1177c/ash/system/network/tray_network.cc
[modify] https://crrev.com/2395de65c82b1d877b9e6530768dd5a33ba1177c/ash/system/power/battery_notification.cc
[modify] https://crrev.com/2395de65c82b1d877b9e6530768dd5a33ba1177c/ash/system/power/dual_role_notification.cc
[modify] https://crrev.com/2395de65c82b1d877b9e6530768dd5a33ba1177c/ash/system/power/peripheral_battery_notifier.cc
[modify] https://crrev.com/2395de65c82b1d877b9e6530768dd5a33ba1177c/ash/system/power/tray_power.cc
[modify] https://crrev.com/2395de65c82b1d877b9e6530768dd5a33ba1177c/ash/system/screen_layout_observer.cc
[modify] https://crrev.com/2395de65c82b1d877b9e6530768dd5a33ba1177c/ash/system/screen_security/screen_capture_tray_item.cc
[modify] https://crrev.com/2395de65c82b1d877b9e6530768dd5a33ba1177c/ash/system/screen_security/screen_share_tray_item.cc
[modify] https://crrev.com/2395de65c82b1d877b9e6530768dd5a33ba1177c/ash/system/session/tray_session_length_limit.cc
[modify] https://crrev.com/2395de65c82b1d877b9e6530768dd5a33ba1177c/ash/system/supervised/tray_supervised_user.cc
[delete] https://crrev.com/27081bd2fc73630f2d9a45df9f4edbc9d5ed39ab/ash/system/system_notifier.cc
[delete] https://crrev.com/27081bd2fc73630f2d9a45df9f4edbc9d5ed39ab/ash/system/system_notifier.h
[modify] https://crrev.com/2395de65c82b1d877b9e6530768dd5a33ba1177c/ash/system/tray_accessibility.cc
[modify] https://crrev.com/2395de65c82b1d877b9e6530768dd5a33ba1177c/ash/system/tray_caps_lock.cc
[modify] https://crrev.com/2395de65c82b1d877b9e6530768dd5a33ba1177c/ash/system/web_notification/fullscreen_notification_blocker.cc
[modify] https://crrev.com/2395de65c82b1d877b9e6530768dd5a33ba1177c/ash/system/web_notification/inactive_user_notification_blocker.cc
[modify] https://crrev.com/2395de65c82b1d877b9e6530768dd5a33ba1177c/ash/system/web_notification/inactive_user_notification_blocker_unittest.cc
[modify] https://crrev.com/2395de65c82b1d877b9e6530768dd5a33ba1177c/ash/system/web_notification/session_state_notification_blocker.cc
[modify] https://crrev.com/2395de65c82b1d877b9e6530768dd5a33ba1177c/ash/system/web_notification/session_state_notification_blocker.h
[modify] https://crrev.com/2395de65c82b1d877b9e6530768dd5a33ba1177c/ash/system/web_notification/session_state_notification_blocker_unittest.cc
[modify] https://crrev.com/2395de65c82b1d877b9e6530768dd5a33ba1177c/chrome/browser/chromeos/login/quick_unlock/quick_unlock_notification_controller.cc
[modify] https://crrev.com/2395de65c82b1d877b9e6530768dd5a33ba1177c/chrome/browser/chromeos/net/DEPS
[modify] https://crrev.com/2395de65c82b1d877b9e6530768dd5a33ba1177c/chrome/browser/chromeos/net/network_portal_notification_controller.cc
[modify] https://crrev.com/2395de65c82b1d877b9e6530768dd5a33ba1177c/chrome/browser/chromeos/ui/low_disk_notification.cc
[modify] https://crrev.com/2395de65c82b1d877b9e6530768dd5a33ba1177c/chrome/browser/notifications/chrome_ash_message_center_client_unittest.cc
[rename] https://crrev.com/2395de65c82b1d877b9e6530768dd5a33ba1177c/chrome/browser/notifications/session_state_notification_blocker_chromeos_browsertest.cc
[modify] https://crrev.com/2395de65c82b1d877b9e6530768dd5a33ba1177c/chrome/browser/ui/ash/auto_connect_notifier.cc
[modify] https://crrev.com/2395de65c82b1d877b9e6530768dd5a33ba1177c/chrome/browser/ui/ash/chrome_screenshot_grabber.cc
[modify] https://crrev.com/2395de65c82b1d877b9e6530768dd5a33ba1177c/chrome/browser/ui/ash/network/data_promo_notification.cc
[modify] https://crrev.com/2395de65c82b1d877b9e6530768dd5a33ba1177c/chrome/browser/ui/ash/network/network_state_notifier.cc
[modify] https://crrev.com/2395de65c82b1d877b9e6530768dd5a33ba1177c/chrome/browser/ui/ash/network/tether_notification_presenter.cc
[modify] https://crrev.com/2395de65c82b1d877b9e6530768dd5a33ba1177c/chrome/browser/usb/DEPS
[modify] https://crrev.com/2395de65c82b1d877b9e6530768dd5a33ba1177c/chrome/browser/usb/web_usb_detector.cc
[modify] https://crrev.com/2395de65c82b1d877b9e6530768dd5a33ba1177c/chrome/test/BUILD.gn

Comment 4 by e...@chromium.org, Mar 9 2018

Cc: -e...@chromium.org
Un-cc-ing me from all bugs on my final day.
Components: Internals>Services>Ash
Labels: -Proj-Mustash-Mash

Comment 6 by est...@chromium.org, May 11 2018

Status: Fixed (was: Assigned)

Sign in to add a comment