New issue
Advanced search Search tips

Issue 776187 link

Starred by 1 user

Issue metadata

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

Blocking:
issue 578868



Sign in to add a comment

remove return values from message_center::NotificationDelegate

Project Member Reported by est...@chromium.org, Oct 18 2017

Issue description

Since some NotificationDelegate handlers will effectively live in other processes (i.e. in a client process, not ash), NotificationDelegate can't have return values.

bool HasClickedListener() --- can be removed in favor of Notification::clickable()

bool SettingsClick() --- return value can be removed in favor of enum in Notification (see ShouldDisplaySettingsButton)

bool ShouldDisplaySettingsButton() --- can be a three-value enum in Notification --- Yes (show default), Yes (let delegate perform a custom action), and No

bool ShouldDisplayOverFullscreen() --- the logic for this checks the notification's origin against what tab (if any) is currently fullscreen. This can probably also be a bit in Notification since it's only called at show-time. If we enter fullscreen when a notification is already showing we can probably just hide the notification. Supporting an async return value seems like a lot more effort than value.

CreateCustomMessageView() --- this doesn't seem like it belongs in NotificationDelegate. Maybe MessageCenterTray instead? It's not clear to me how arc notifications and their surfaces will work.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Oct 19 2017

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

commit e43ca886e2a7c023f6237c5390a98c7cbc886ed5
Author: Evan Stade <estade@chromium.org>
Date: Thu Oct 19 21:12:45 2017

Remove NotificationDelegate::HasClickedListener().

For all cases where it returned the non-default value (true), replace
with setting the clickable field in Notification.

Bug:  776187 
Change-Id: I24c99a651a0753b8ab474102ed50ee94b72b0d90
Reviewed-on: https://chromium-review.googlesource.com/727468
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#510217}
[modify] https://crrev.com/e43ca886e2a7c023f6237c5390a98c7cbc886ed5/ash/accelerators/accelerator_controller.cc
[modify] https://crrev.com/e43ca886e2a7c023f6237c5390a98c7cbc886ed5/ash/display/resolution_notification_controller.cc
[modify] https://crrev.com/e43ca886e2a7c023f6237c5390a98c7cbc886ed5/ash/message_center/message_center_view.cc
[modify] https://crrev.com/e43ca886e2a7c023f6237c5390a98c7cbc886ed5/ash/message_center/message_center_view.h
[modify] https://crrev.com/e43ca886e2a7c023f6237c5390a98c7cbc886ed5/ash/message_center/message_center_view_unittest.cc
[modify] https://crrev.com/e43ca886e2a7c023f6237c5390a98c7cbc886ed5/ash/message_center/message_list_view_unittest.cc
[modify] https://crrev.com/e43ca886e2a7c023f6237c5390a98c7cbc886ed5/ash/system/locale/locale_notification_controller.cc
[modify] https://crrev.com/e43ca886e2a7c023f6237c5390a98c7cbc886ed5/ash/system/screen_layout_observer.cc
[modify] https://crrev.com/e43ca886e2a7c023f6237c5390a98c7cbc886ed5/chrome/browser/background/background_contents_service.cc
[modify] https://crrev.com/e43ca886e2a7c023f6237c5390a98c7cbc886ed5/chrome/browser/chromeos/printing/cups_print_job_notification.cc
[modify] https://crrev.com/e43ca886e2a7c023f6237c5390a98c7cbc886ed5/chrome/browser/chromeos/status/data_promo_notification.cc
[modify] https://crrev.com/e43ca886e2a7c023f6237c5390a98c7cbc886ed5/chrome/browser/ui/ash/chrome_screenshot_grabber.cc
[modify] https://crrev.com/e43ca886e2a7c023f6237c5390a98c7cbc886ed5/ui/arc/notification/arc_notification_content_view_unittest.cc
[modify] https://crrev.com/e43ca886e2a7c023f6237c5390a98c7cbc886ed5/ui/arc/notification/arc_notification_view_unittest.cc
[modify] https://crrev.com/e43ca886e2a7c023f6237c5390a98c7cbc886ed5/ui/message_center/fake_message_center.cc
[modify] https://crrev.com/e43ca886e2a7c023f6237c5390a98c7cbc886ed5/ui/message_center/fake_message_center.h
[modify] https://crrev.com/e43ca886e2a7c023f6237c5390a98c7cbc886ed5/ui/message_center/message_center.h
[modify] https://crrev.com/e43ca886e2a7c023f6237c5390a98c7cbc886ed5/ui/message_center/message_center_impl.cc
[modify] https://crrev.com/e43ca886e2a7c023f6237c5390a98c7cbc886ed5/ui/message_center/message_center_impl.h
[modify] https://crrev.com/e43ca886e2a7c023f6237c5390a98c7cbc886ed5/ui/message_center/notification.cc
[modify] https://crrev.com/e43ca886e2a7c023f6237c5390a98c7cbc886ed5/ui/message_center/notification.h
[modify] https://crrev.com/e43ca886e2a7c023f6237c5390a98c7cbc886ed5/ui/message_center/notification_delegate.cc
[modify] https://crrev.com/e43ca886e2a7c023f6237c5390a98c7cbc886ed5/ui/message_center/notification_delegate.h
[modify] https://crrev.com/e43ca886e2a7c023f6237c5390a98c7cbc886ed5/ui/message_center/notification_delegate_unittest.cc
[modify] https://crrev.com/e43ca886e2a7c023f6237c5390a98c7cbc886ed5/ui/message_center/views/message_center_controller.h
[modify] https://crrev.com/e43ca886e2a7c023f6237c5390a98c7cbc886ed5/ui/message_center/views/message_popup_collection.cc
[modify] https://crrev.com/e43ca886e2a7c023f6237c5390a98c7cbc886ed5/ui/message_center/views/message_popup_collection.h
[modify] https://crrev.com/e43ca886e2a7c023f6237c5390a98c7cbc886ed5/ui/message_center/views/notification_view.cc
[modify] https://crrev.com/e43ca886e2a7c023f6237c5390a98c7cbc886ed5/ui/message_center/views/notification_view_md.cc
[modify] https://crrev.com/e43ca886e2a7c023f6237c5390a98c7cbc886ed5/ui/message_center/views/notification_view_md_unittest.cc
[modify] https://crrev.com/e43ca886e2a7c023f6237c5390a98c7cbc886ed5/ui/message_center/views/notification_view_unittest.cc
[modify] https://crrev.com/e43ca886e2a7c023f6237c5390a98c7cbc886ed5/ui/message_center/views/toast_contents_view.cc
[modify] https://crrev.com/e43ca886e2a7c023f6237c5390a98c7cbc886ed5/ui/message_center/views/toast_contents_view.h

Project Member

Comment 2 by bugdroid1@chromium.org, Oct 25 2017

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

commit 146f3f5b9d81bbe575244c0375066fd08da396c1
Author: Evan Stade <estade@chromium.org>
Date: Wed Oct 25 20:27:08 2017

Remove NotificationDelegate::CreateCustomMessageView().

Replace it with the ability to set a factory function for creating custom
message views. This factory function is specific to Views (and is only
used by and for ARC notifications).

NotificationDelegate is not specific to Views and shouldn't contain
references to Views-specific classes.

TBR: derat@chromium.org
Bug:  776187 
Change-Id: I8cc1eacdbc5030426d1aec62ee598fca043f715b
Reviewed-on: https://chromium-review.googlesource.com/730651
Reviewed-by: Evan Stade <estade@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>
Reviewed-by: Yoshiki Iguchi <yoshiki@chromium.org>
Commit-Queue: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#511564}
[modify] https://crrev.com/146f3f5b9d81bbe575244c0375066fd08da396c1/chrome/browser/chromeos/login/quick_unlock/quick_unlock_notification_controller.h
[modify] https://crrev.com/146f3f5b9d81bbe575244c0375066fd08da396c1/ui/arc/notification/arc_notification_content_view_unittest.cc
[modify] https://crrev.com/146f3f5b9d81bbe575244c0375066fd08da396c1/ui/arc/notification/arc_notification_delegate.h
[modify] https://crrev.com/146f3f5b9d81bbe575244c0375066fd08da396c1/ui/arc/notification/arc_notification_manager.cc
[modify] https://crrev.com/146f3f5b9d81bbe575244c0375066fd08da396c1/ui/arc/notification/arc_notification_manager.h
[modify] https://crrev.com/146f3f5b9d81bbe575244c0375066fd08da396c1/ui/arc/notification/arc_notification_view_unittest.cc
[modify] https://crrev.com/146f3f5b9d81bbe575244c0375066fd08da396c1/ui/message_center/BUILD.gn
[modify] https://crrev.com/146f3f5b9d81bbe575244c0375066fd08da396c1/ui/message_center/notification_delegate.h
[delete] https://crrev.com/79438ded3cfafb2a3ae274ee8b5fe3861ead2ed3/ui/message_center/notification_delegate_views.cc
[modify] https://crrev.com/146f3f5b9d81bbe575244c0375066fd08da396c1/ui/message_center/views/message_view_factory.cc
[modify] https://crrev.com/146f3f5b9d81bbe575244c0375066fd08da396c1/ui/message_center/views/message_view_factory.h

Project Member

Comment 3 by bugdroid1@chromium.org, Oct 31 2017

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

commit 1b8fb65c17069cc192cac7cb91005fe05cc164aa
Author: Evan Stade <estade@chromium.org>
Date: Tue Oct 31 16:30:43 2017

Remove feature flags and histograms for fullscreen notification features

Both the extension (app) one and the web notification one have been
enabled by default for a while. This change makes it easier to get rid
of synchronous methods in NotificationDelegate.

Bug:  776187 
Change-Id: Ie14b348293891807fa1973961506ade7b6914bea
Reviewed-on: https://chromium-review.googlesource.com/744426
Commit-Queue: Evan Stade <estade@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Justin DeWitt <dewittj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#512849}
[modify] https://crrev.com/1b8fb65c17069cc192cac7cb91005fe05cc164aa/chrome/browser/extensions/api/notifications/extension_notification_handler.cc
[modify] https://crrev.com/1b8fb65c17069cc192cac7cb91005fe05cc164aa/chrome/browser/extensions/api/notifications/extension_notification_handler.h
[modify] https://crrev.com/1b8fb65c17069cc192cac7cb91005fe05cc164aa/chrome/browser/extensions/api/notifications/notifications_api.cc
[modify] https://crrev.com/1b8fb65c17069cc192cac7cb91005fe05cc164aa/chrome/browser/extensions/api/notifications/notifications_api.h
[modify] https://crrev.com/1b8fb65c17069cc192cac7cb91005fe05cc164aa/chrome/browser/extensions/api/notifications/notifications_apitest.cc
[modify] https://crrev.com/1b8fb65c17069cc192cac7cb91005fe05cc164aa/chrome/browser/notifications/notification_common.cc
[modify] https://crrev.com/1b8fb65c17069cc192cac7cb91005fe05cc164aa/chrome/browser/notifications/notification_common.h
[modify] https://crrev.com/1b8fb65c17069cc192cac7cb91005fe05cc164aa/chrome/browser/notifications/notification_interactive_uitest.cc
[modify] https://crrev.com/1b8fb65c17069cc192cac7cb91005fe05cc164aa/chrome/browser/notifications/notification_interactive_uitest_support.cc
[modify] https://crrev.com/1b8fb65c17069cc192cac7cb91005fe05cc164aa/chrome/browser/notifications/notification_interactive_uitest_support.h
[modify] https://crrev.com/1b8fb65c17069cc192cac7cb91005fe05cc164aa/chrome/browser/notifications/platform_notification_service_interactive_uitest.cc
[modify] https://crrev.com/1b8fb65c17069cc192cac7cb91005fe05cc164aa/tools/metrics/histograms/histograms.xml

Project Member

Comment 4 by bugdroid1@chromium.org, Nov 1 2017

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

commit d0e4089e99429ceea6c0f54a67c4964dcd236f42
Author: Evan Stade <estade@chromium.org>
Date: Wed Nov 01 16:53:14 2017

Remove two synchronous methods from NotificationDelegate.

ShouldShowSettingsButton() and the return value for SettingsClick() are
replaced by an enum in Notification (by way of RichNotificationData).

Bug:  776187 
Change-Id: I7f3a04b855f3c6a9871f8eb71639add2722f1088
Reviewed-on: https://chromium-review.googlesource.com/729331
Commit-Queue: Evan Stade <estade@chromium.org>
Reviewed-by: Yoshiki Iguchi <yoshiki@chromium.org>
Cr-Commit-Position: refs/heads/master@{#513157}
[modify] https://crrev.com/d0e4089e99429ceea6c0f54a67c4964dcd236f42/ash/message_center/message_center_view.cc
[modify] https://crrev.com/d0e4089e99429ceea6c0f54a67c4964dcd236f42/chrome/browser/notifications/platform_notification_service_impl.cc
[modify] https://crrev.com/d0e4089e99429ceea6c0f54a67c4964dcd236f42/chrome/browser/notifications/platform_notification_service_interactive_uitest.cc
[modify] https://crrev.com/d0e4089e99429ceea6c0f54a67c4964dcd236f42/chrome/browser/notifications/web_notification_delegate.cc
[modify] https://crrev.com/d0e4089e99429ceea6c0f54a67c4964dcd236f42/chrome/browser/notifications/web_notification_delegate.h
[modify] https://crrev.com/d0e4089e99429ceea6c0f54a67c4964dcd236f42/ui/arc/notification/arc_notification_delegate.cc
[modify] https://crrev.com/d0e4089e99429ceea6c0f54a67c4964dcd236f42/ui/arc/notification/arc_notification_delegate.h
[modify] https://crrev.com/d0e4089e99429ceea6c0f54a67c4964dcd236f42/ui/arc/notification/arc_notification_item_impl.cc
[modify] https://crrev.com/d0e4089e99429ceea6c0f54a67c4964dcd236f42/ui/arc/notification/arc_notification_view.cc
[modify] https://crrev.com/d0e4089e99429ceea6c0f54a67c4964dcd236f42/ui/message_center/cocoa/notification_controller.mm
[modify] https://crrev.com/d0e4089e99429ceea6c0f54a67c4964dcd236f42/ui/message_center/cocoa/notification_controller_unittest.mm
[modify] https://crrev.com/d0e4089e99429ceea6c0f54a67c4964dcd236f42/ui/message_center/message_center_impl.cc
[modify] https://crrev.com/d0e4089e99429ceea6c0f54a67c4964dcd236f42/ui/message_center/notification.cc
[modify] https://crrev.com/d0e4089e99429ceea6c0f54a67c4964dcd236f42/ui/message_center/notification.h
[modify] https://crrev.com/d0e4089e99429ceea6c0f54a67c4964dcd236f42/ui/message_center/notification_delegate.cc
[modify] https://crrev.com/d0e4089e99429ceea6c0f54a67c4964dcd236f42/ui/message_center/notification_delegate.h
[modify] https://crrev.com/d0e4089e99429ceea6c0f54a67c4964dcd236f42/ui/message_center/views/message_popup_collection.cc
[modify] https://crrev.com/d0e4089e99429ceea6c0f54a67c4964dcd236f42/ui/message_center/views/notification_view.cc
[modify] https://crrev.com/d0e4089e99429ceea6c0f54a67c4964dcd236f42/ui/message_center/views/notification_view_md.cc
[modify] https://crrev.com/d0e4089e99429ceea6c0f54a67c4964dcd236f42/ui/message_center/views/notification_view_unittest.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Nov 3 2017

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

commit cff869d755256e09b531ff99586797e13ea31b9e
Author: Evan Stade <estade@chromium.org>
Date: Fri Nov 03 17:33:07 2017

Remove NotificationDelegate::ShouldDisplayOverFullscreen
(as well as NotificationHandler::ShouldDisplayOnFullScreen) in favor of
an enum in RichNotificationData.

Behaviorally, there's a slight change: switching into fullscreen will
hide a notification even if that notification came from the same
app/website. I think it's more important to support showing
notifications on top of existing fullscreen windows (as appropriate),
so this change doesn't seem overly worrisome.

In the future this enum will also let us get rid of the centralized list
of Ash system notifiers and instead let each Ash system notification
control its own visibility for fullscreen/login screen/etc.

Bug:  776187 
Change-Id: I295cd9b18a157b8ba6bbfa27a89f97c9fc6ece85
Reviewed-on: https://chromium-review.googlesource.com/745040
Commit-Queue: Evan Stade <estade@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Justin DeWitt <dewittj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#513823}
[modify] https://crrev.com/cff869d755256e09b531ff99586797e13ea31b9e/ash/system/web_notification/fullscreen_notification_blocker.cc
[modify] https://crrev.com/cff869d755256e09b531ff99586797e13ea31b9e/chrome/browser/BUILD.gn
[modify] https://crrev.com/cff869d755256e09b531ff99586797e13ea31b9e/chrome/browser/extensions/api/notifications/extension_notification_handler.cc
[modify] https://crrev.com/cff869d755256e09b531ff99586797e13ea31b9e/chrome/browser/extensions/api/notifications/extension_notification_handler.h
[modify] https://crrev.com/cff869d755256e09b531ff99586797e13ea31b9e/chrome/browser/extensions/api/notifications/notifications_api.cc
[modify] https://crrev.com/cff869d755256e09b531ff99586797e13ea31b9e/chrome/browser/extensions/api/notifications/notifications_apitest.cc
[modify] https://crrev.com/cff869d755256e09b531ff99586797e13ea31b9e/chrome/browser/notifications/fullscreen_notification_blocker.cc
[modify] https://crrev.com/cff869d755256e09b531ff99586797e13ea31b9e/chrome/browser/notifications/fullscreen_notification_blocker.h
[modify] https://crrev.com/cff869d755256e09b531ff99586797e13ea31b9e/chrome/browser/notifications/non_persistent_notification_handler.cc
[modify] https://crrev.com/cff869d755256e09b531ff99586797e13ea31b9e/chrome/browser/notifications/non_persistent_notification_handler.h
[modify] https://crrev.com/cff869d755256e09b531ff99586797e13ea31b9e/chrome/browser/notifications/notification_common.cc
[modify] https://crrev.com/cff869d755256e09b531ff99586797e13ea31b9e/chrome/browser/notifications/notification_common.h
[modify] https://crrev.com/cff869d755256e09b531ff99586797e13ea31b9e/chrome/browser/notifications/notification_display_service.cc
[modify] https://crrev.com/cff869d755256e09b531ff99586797e13ea31b9e/chrome/browser/notifications/notification_display_service.h
[delete] https://crrev.com/82254284dfda9ac263bbca945c6ed72250e92e80/chrome/browser/notifications/notification_handler.cc
[modify] https://crrev.com/cff869d755256e09b531ff99586797e13ea31b9e/chrome/browser/notifications/notification_handler.h
[modify] https://crrev.com/cff869d755256e09b531ff99586797e13ea31b9e/chrome/browser/notifications/notification_interactive_uitest.cc
[modify] https://crrev.com/cff869d755256e09b531ff99586797e13ea31b9e/chrome/browser/notifications/persistent_notification_handler.cc
[modify] https://crrev.com/cff869d755256e09b531ff99586797e13ea31b9e/chrome/browser/notifications/persistent_notification_handler.h
[modify] https://crrev.com/cff869d755256e09b531ff99586797e13ea31b9e/chrome/browser/notifications/platform_notification_service_impl.cc
[modify] https://crrev.com/cff869d755256e09b531ff99586797e13ea31b9e/chrome/browser/notifications/platform_notification_service_interactive_uitest.cc
[modify] https://crrev.com/cff869d755256e09b531ff99586797e13ea31b9e/chrome/browser/notifications/web_notification_delegate.cc
[modify] https://crrev.com/cff869d755256e09b531ff99586797e13ea31b9e/chrome/browser/notifications/web_notification_delegate.h
[add] https://crrev.com/cff869d755256e09b531ff99586797e13ea31b9e/chrome/test/data/extensions/api_test/notifications/api/notification_on_blur/index.html
[add] https://crrev.com/cff869d755256e09b531ff99586797e13ea31b9e/chrome/test/data/extensions/api_test/notifications/api/notification_on_blur/index.js
[add] https://crrev.com/cff869d755256e09b531ff99586797e13ea31b9e/chrome/test/data/extensions/api_test/notifications/api/notification_on_blur/main.js
[add] https://crrev.com/cff869d755256e09b531ff99586797e13ea31b9e/chrome/test/data/extensions/api_test/notifications/api/notification_on_blur/manifest.json
[modify] https://crrev.com/cff869d755256e09b531ff99586797e13ea31b9e/ui/message_center/notification.cc
[modify] https://crrev.com/cff869d755256e09b531ff99586797e13ea31b9e/ui/message_center/notification.h
[modify] https://crrev.com/cff869d755256e09b531ff99586797e13ea31b9e/ui/message_center/notification_delegate.cc
[modify] https://crrev.com/cff869d755256e09b531ff99586797e13ea31b9e/ui/message_center/notification_delegate.h
[modify] https://crrev.com/cff869d755256e09b531ff99586797e13ea31b9e/ui/message_center/notification_list.cc

Status: Fixed (was: Started)
this is done. Hopefully no one adds new ones. I'm not sure how to guarantee that until we get to the point where we are fully mojo-ified and have tests in place.

Comment 7 by dchan@chromium.org, Jan 22 2018

Status: archived (was: Fixed)

Comment 8 by dchan@chromium.org, Jan 23 2018

Status: Fixed (was: Archived)

Sign in to add a comment