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

Issue 596161 link

Starred by 7 users

Issue metadata

Status: Archived
Owner:
Closed: Sep 25
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Feature

Blocked on:
issue 720345
issue 439950

Blocking:
issue 571056



Sign in to add a comment

Re-design NotificationUIManager to account for native notifications

Project Member Reported by miguelg@chromium.org, Mar 18 2016

Issue description

Web (and to some extent extension) notifications need a much smaller api surface than the one provided by the existing NotificationUIManager. This is especially true if we move some platforms to use their native notification centers.

This class also has some important problems like the ProfileID workaround and the fact that we now have to support 3 types of notification IDs (notification_id, persistent_notification_id and delegate_id).

We should be able to create a new interface (NotificationDisplayManager) that can be used by PlatformNotificationServiceImpl as well as the extension code.

 
Labels: -Type-Bug Type-Feature
Blocking: 571056
Project Member

Comment 3 by bugdroid1@chromium.org, Apr 21 2016

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

commit 23cd2dd719c8e76e17e764100a6eff131c34a418
Author: miguelg <miguelg@chromium.org>
Date: Thu Apr 21 15:24:03 2016

Nuke NotificationUIManager from PlatformNotificationServiceImpl

A few highlights
- Always use one kind of profile id (the stable one)
- Always use a single notification id (the persistent one when possible)
- The mac implementation injects the NSUSerNotificationService to allow unittests

Note that the mac and android files are not renamed. Ideally I would land this and then land a straight forward rename cl.

BUG= 571056 , 596161 

Review URL: https://codereview.chromium.org/1814923002

Cr-Commit-Position: refs/heads/master@{#388776}

[modify] https://crrev.com/23cd2dd719c8e76e17e764100a6eff131c34a418/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java
[modify] https://crrev.com/23cd2dd719c8e76e17e764100a6eff131c34a418/chrome/browser/android/chrome_jni_registrar.cc
[modify] https://crrev.com/23cd2dd719c8e76e17e764100a6eff131c34a418/chrome/browser/browser_process.h
[modify] https://crrev.com/23cd2dd719c8e76e17e764100a6eff131c34a418/chrome/browser/browser_process_impl.cc
[modify] https://crrev.com/23cd2dd719c8e76e17e764100a6eff131c34a418/chrome/browser/browser_process_impl.h
[add] https://crrev.com/23cd2dd719c8e76e17e764100a6eff131c34a418/chrome/browser/notifications/message_center_display_service.cc
[add] https://crrev.com/23cd2dd719c8e76e17e764100a6eff131c34a418/chrome/browser/notifications/message_center_display_service.h
[add] https://crrev.com/23cd2dd719c8e76e17e764100a6eff131c34a418/chrome/browser/notifications/native_notification_display_service.cc
[add] https://crrev.com/23cd2dd719c8e76e17e764100a6eff131c34a418/chrome/browser/notifications/native_notification_display_service.h
[add] https://crrev.com/23cd2dd719c8e76e17e764100a6eff131c34a418/chrome/browser/notifications/notification_display_service.h
[add] https://crrev.com/23cd2dd719c8e76e17e764100a6eff131c34a418/chrome/browser/notifications/notification_display_service_factory.cc
[add] https://crrev.com/23cd2dd719c8e76e17e764100a6eff131c34a418/chrome/browser/notifications/notification_display_service_factory.h
[add] https://crrev.com/23cd2dd719c8e76e17e764100a6eff131c34a418/chrome/browser/notifications/notification_platform_bridge.h
[modify] https://crrev.com/23cd2dd719c8e76e17e764100a6eff131c34a418/chrome/browser/notifications/notification_ui_manager_android.cc
[modify] https://crrev.com/23cd2dd719c8e76e17e764100a6eff131c34a418/chrome/browser/notifications/notification_ui_manager_android.h
[modify] https://crrev.com/23cd2dd719c8e76e17e764100a6eff131c34a418/chrome/browser/notifications/notification_ui_manager_mac.h
[modify] https://crrev.com/23cd2dd719c8e76e17e764100a6eff131c34a418/chrome/browser/notifications/notification_ui_manager_mac.mm
[modify] https://crrev.com/23cd2dd719c8e76e17e764100a6eff131c34a418/chrome/browser/notifications/platform_notification_service_browsertest.cc
[modify] https://crrev.com/23cd2dd719c8e76e17e764100a6eff131c34a418/chrome/browser/notifications/platform_notification_service_impl.cc
[modify] https://crrev.com/23cd2dd719c8e76e17e764100a6eff131c34a418/chrome/browser/notifications/platform_notification_service_impl.h
[modify] https://crrev.com/23cd2dd719c8e76e17e764100a6eff131c34a418/chrome/browser/notifications/platform_notification_service_unittest.cc
[modify] https://crrev.com/23cd2dd719c8e76e17e764100a6eff131c34a418/chrome/browser/push_messaging/push_messaging_browsertest.cc
[modify] https://crrev.com/23cd2dd719c8e76e17e764100a6eff131c34a418/chrome/chrome_browser.gypi
[modify] https://crrev.com/23cd2dd719c8e76e17e764100a6eff131c34a418/chrome/test/base/testing_browser_process.cc
[modify] https://crrev.com/23cd2dd719c8e76e17e764100a6eff131c34a418/chrome/test/base/testing_browser_process.h

Project Member

Comment 4 by bugdroid1@chromium.org, Apr 26 2016

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

commit 884b3ecf7101cdf41820b896f5e7ed34cd3de2ff
Author: miguelg <miguelg@chromium.org>
Date: Tue Apr 26 09:51:52 2016

PlatformNotificationService layering cleanup.

Make the display service execute the delegate->Display() call instead of having every bridge do it.

Access NotificationDisplayService from the profile directly
Simplify how PlatformNotificationServiceImpl unittest access the display
service to make it more similar to production.
Make GetNotificationUIManager private.

BUG= 596161 

Review URL: https://codereview.chromium.org/1895473002

Cr-Commit-Position: refs/heads/master@{#389736}

[modify] https://crrev.com/884b3ecf7101cdf41820b896f5e7ed34cd3de2ff/chrome/browser/BUILD.gn
[modify] https://crrev.com/884b3ecf7101cdf41820b896f5e7ed34cd3de2ff/chrome/browser/notifications/native_notification_display_service.cc
[modify] https://crrev.com/884b3ecf7101cdf41820b896f5e7ed34cd3de2ff/chrome/browser/notifications/notification_ui_manager_android.cc
[modify] https://crrev.com/884b3ecf7101cdf41820b896f5e7ed34cd3de2ff/chrome/browser/notifications/notification_ui_manager_mac.mm
[modify] https://crrev.com/884b3ecf7101cdf41820b896f5e7ed34cd3de2ff/chrome/browser/notifications/platform_notification_service_impl.cc
[modify] https://crrev.com/884b3ecf7101cdf41820b896f5e7ed34cd3de2ff/chrome/browser/notifications/platform_notification_service_impl.h
[modify] https://crrev.com/884b3ecf7101cdf41820b896f5e7ed34cd3de2ff/chrome/browser/notifications/platform_notification_service_unittest.cc
[add] https://crrev.com/884b3ecf7101cdf41820b896f5e7ed34cd3de2ff/chrome/browser/notifications/stub_notification_platform_bridge.cc
[add] https://crrev.com/884b3ecf7101cdf41820b896f5e7ed34cd3de2ff/chrome/browser/notifications/stub_notification_platform_bridge.h
[modify] https://crrev.com/884b3ecf7101cdf41820b896f5e7ed34cd3de2ff/chrome/chrome_tests_unit.gypi
[modify] https://crrev.com/884b3ecf7101cdf41820b896f5e7ed34cd3de2ff/chrome/test/base/testing_browser_process.cc
[modify] https://crrev.com/884b3ecf7101cdf41820b896f5e7ed34cd3de2ff/chrome/test/base/testing_browser_process.h

Project Member

Comment 5 by bugdroid1@chromium.org, Apr 26 2016

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

commit d29366e615abf76bb82f531c2846b878557a4318
Author: miguelg <miguelg@chromium.org>
Date: Tue Apr 26 15:18:48 2016

Initialize the notification bridge instead of the UIManager on Android

BUG= 596161 

Review URL: https://codereview.chromium.org/1919223002

Cr-Commit-Position: refs/heads/master@{#389783}

[modify] https://crrev.com/d29366e615abf76bb82f531c2846b878557a4318/chrome/browser/notifications/notification_ui_manager_android.cc

Project Member

Comment 6 by bugdroid1@chromium.org, Apr 27 2016

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

commit dd6ae73011eb6e25c025b4be3f147638e7570b9e
Author: miguelg <miguelg@chromium.org>
Date: Wed Apr 27 14:56:42 2016

Rename NotificationUIManager to NotificationPlatformBridge (1/2)

This covers the Android files only to make it more manageable. Mac files will come in a second CL.

The message center NotificationUIManager will remain untouched

BUG= 596161 

Review URL: https://codereview.chromium.org/1919183003

Cr-Commit-Position: refs/heads/master@{#390075}

[modify] https://crrev.com/dd6ae73011eb6e25c025b4be3f147638e7570b9e/chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java
[modify] https://crrev.com/dd6ae73011eb6e25c025b4be3f147638e7570b9e/chrome/android/java/src/org/chromium/chrome/browser/document/ChromeLauncherActivity.java
[rename] https://crrev.com/dd6ae73011eb6e25c025b4be3f147638e7570b9e/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java
[modify] https://crrev.com/dd6ae73011eb6e25c025b4be3f147638e7570b9e/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationService.java
[modify] https://crrev.com/dd6ae73011eb6e25c025b4be3f147638e7570b9e/chrome/android/java_sources.gni
[rename] https://crrev.com/dd6ae73011eb6e25c025b4be3f147638e7570b9e/chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridgeIntentTest.java
[rename] https://crrev.com/dd6ae73011eb6e25c025b4be3f147638e7570b9e/chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridgeTest.java
[modify] https://crrev.com/dd6ae73011eb6e25c025b4be3f147638e7570b9e/chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationTestBase.java
[add] https://crrev.com/dd6ae73011eb6e25c025b4be3f147638e7570b9e/chrome/android/junit/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridgeTest.java
[delete] https://crrev.com/1a9ce9415e528d0fe526d6cc7f766a497893265e/chrome/android/junit/src/org/chromium/chrome/browser/notifications/NotificationUIManagerUnitTest.java
[modify] https://crrev.com/dd6ae73011eb6e25c025b4be3f147638e7570b9e/chrome/browser/android/chrome_jni_registrar.cc
[rename] https://crrev.com/dd6ae73011eb6e25c025b4be3f147638e7570b9e/chrome/browser/notifications/notification_platform_bridge_android.cc
[rename] https://crrev.com/dd6ae73011eb6e25c025b4be3f147638e7570b9e/chrome/browser/notifications/notification_platform_bridge_android.h
[modify] https://crrev.com/dd6ae73011eb6e25c025b4be3f147638e7570b9e/chrome/chrome_browser.gypi

Project Member

Comment 8 by bugdroid1@chromium.org, Jul 8 2016

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

commit 87e986d94f7bf82930af385d88d7e3b79eac4515
Author: miguelg <miguelg@chromium.org>
Date: Fri Jul 08 18:04:44 2016

Introduce a new API to handle native notification clicks

BUG= 596161 , 595487 

Review-Url: https://codereview.chromium.org/2093953002
Cr-Commit-Position: refs/heads/master@{#404436}

[modify] https://crrev.com/87e986d94f7bf82930af385d88d7e3b79eac4515/chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridgeIntentTest.java
[modify] https://crrev.com/87e986d94f7bf82930af385d88d7e3b79eac4515/chrome/browser/notifications/message_center_display_service.cc
[modify] https://crrev.com/87e986d94f7bf82930af385d88d7e3b79eac4515/chrome/browser/notifications/message_center_display_service.h
[modify] https://crrev.com/87e986d94f7bf82930af385d88d7e3b79eac4515/chrome/browser/notifications/native_notification_display_service.cc
[modify] https://crrev.com/87e986d94f7bf82930af385d88d7e3b79eac4515/chrome/browser/notifications/native_notification_display_service.h
[add] https://crrev.com/87e986d94f7bf82930af385d88d7e3b79eac4515/chrome/browser/notifications/non_persistent_notification_handler.cc
[add] https://crrev.com/87e986d94f7bf82930af385d88d7e3b79eac4515/chrome/browser/notifications/non_persistent_notification_handler.h
[modify] https://crrev.com/87e986d94f7bf82930af385d88d7e3b79eac4515/chrome/browser/notifications/notification_common.h
[modify] https://crrev.com/87e986d94f7bf82930af385d88d7e3b79eac4515/chrome/browser/notifications/notification_display_service.h
[add] https://crrev.com/87e986d94f7bf82930af385d88d7e3b79eac4515/chrome/browser/notifications/notification_handler.h
[modify] https://crrev.com/87e986d94f7bf82930af385d88d7e3b79eac4515/chrome/browser/notifications/notification_platform_bridge.h
[modify] https://crrev.com/87e986d94f7bf82930af385d88d7e3b79eac4515/chrome/browser/notifications/notification_platform_bridge_android.cc
[modify] https://crrev.com/87e986d94f7bf82930af385d88d7e3b79eac4515/chrome/browser/notifications/notification_platform_bridge_android.h
[modify] https://crrev.com/87e986d94f7bf82930af385d88d7e3b79eac4515/chrome/browser/notifications/notification_platform_bridge_mac.h
[modify] https://crrev.com/87e986d94f7bf82930af385d88d7e3b79eac4515/chrome/browser/notifications/notification_platform_bridge_mac.mm
[modify] https://crrev.com/87e986d94f7bf82930af385d88d7e3b79eac4515/chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm
[add] https://crrev.com/87e986d94f7bf82930af385d88d7e3b79eac4515/chrome/browser/notifications/persistent_notification_handler.cc
[add] https://crrev.com/87e986d94f7bf82930af385d88d7e3b79eac4515/chrome/browser/notifications/persistent_notification_handler.h
[modify] https://crrev.com/87e986d94f7bf82930af385d88d7e3b79eac4515/chrome/browser/notifications/platform_notification_service_impl.cc
[modify] https://crrev.com/87e986d94f7bf82930af385d88d7e3b79eac4515/chrome/browser/notifications/platform_notification_service_impl.h
[modify] https://crrev.com/87e986d94f7bf82930af385d88d7e3b79eac4515/chrome/browser/notifications/stub_notification_platform_bridge.cc
[modify] https://crrev.com/87e986d94f7bf82930af385d88d7e3b79eac4515/chrome/browser/notifications/stub_notification_platform_bridge.h
[modify] https://crrev.com/87e986d94f7bf82930af385d88d7e3b79eac4515/chrome/browser/ui/cocoa/notifications/notification_builder_mac.h
[modify] https://crrev.com/87e986d94f7bf82930af385d88d7e3b79eac4515/chrome/browser/ui/cocoa/notifications/notification_builder_mac.mm
[modify] https://crrev.com/87e986d94f7bf82930af385d88d7e3b79eac4515/chrome/browser/ui/cocoa/notifications/notification_builder_mac_unittest.mm
[modify] https://crrev.com/87e986d94f7bf82930af385d88d7e3b79eac4515/chrome/browser/ui/cocoa/notifications/notification_constants_mac.h
[modify] https://crrev.com/87e986d94f7bf82930af385d88d7e3b79eac4515/chrome/browser/ui/cocoa/notifications/notification_constants_mac.mm
[modify] https://crrev.com/87e986d94f7bf82930af385d88d7e3b79eac4515/chrome/browser/ui/cocoa/notifications/notification_response_builder_mac.mm
[modify] https://crrev.com/87e986d94f7bf82930af385d88d7e3b79eac4515/chrome/browser/ui/cocoa/notifications/notification_response_builder_mac_unittest.mm
[modify] https://crrev.com/87e986d94f7bf82930af385d88d7e3b79eac4515/chrome/chrome_browser.gypi

Blockedon: 439950
Cc: -mvanouwe...@chromium.org
Project Member

Comment 12 by bugdroid1@chromium.org, Feb 7 2017

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

commit e50bfa204461eba626ab5891799669f9bf8231be
Author: miguelg <miguelg@chromium.org>
Date: Tue Feb 07 20:34:21 2017

Fix a couple of notification TODOs

Delete obsolete notifiations from the database
Stop silently ignoring notifications without an ID

BUG= 596161 

Review-Url: https://codereview.chromium.org/2671603005
Cr-Commit-Position: refs/heads/master@{#448721}

[modify] https://crrev.com/e50bfa204461eba626ab5891799669f9bf8231be/content/browser/notifications/notification_database.cc
[modify] https://crrev.com/e50bfa204461eba626ab5891799669f9bf8231be/content/browser/notifications/platform_notification_context_impl.cc

Project Member

Comment 13 by bugdroid1@chromium.org, Feb 9 2017

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

commit f3f824cd5032eac3c694bd9b7a5c05a2e45ebc5c
Author: miguelg <miguelg@chromium.org>
Date: Thu Feb 09 13:51:44 2017

Test the platform notification context synchronize operation

- Extract the mock implementation used for layout tests
- Add the ability to synchronize notifications to the mock implementation
- Create a small class with a custom content browser client
- Use all of that in the test

BUG= 596161 

Review-Url: https://codereview.chromium.org/2672313004
Cr-Commit-Position: refs/heads/master@{#449282}

[modify] https://crrev.com/f3f824cd5032eac3c694bd9b7a5c05a2e45ebc5c/content/browser/notifications/platform_notification_context_unittest.cc
[modify] https://crrev.com/f3f824cd5032eac3c694bd9b7a5c05a2e45ebc5c/content/shell/browser/layout_test/layout_test_notification_manager.cc
[modify] https://crrev.com/f3f824cd5032eac3c694bd9b7a5c05a2e45ebc5c/content/shell/browser/layout_test/layout_test_notification_manager.h
[modify] https://crrev.com/f3f824cd5032eac3c694bd9b7a5c05a2e45ebc5c/content/shell/browser/layout_test/layout_test_permission_manager.cc
[modify] https://crrev.com/f3f824cd5032eac3c694bd9b7a5c05a2e45ebc5c/content/test/BUILD.gn
[add] https://crrev.com/f3f824cd5032eac3c694bd9b7a5c05a2e45ebc5c/content/test/mock_platform_notification_service.cc
[add] https://crrev.com/f3f824cd5032eac3c694bd9b7a5c05a2e45ebc5c/content/test/mock_platform_notification_service.h

Project Member

Comment 14 by bugdroid1@chromium.org, Apr 11 2017

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

commit 28d8b7398617c37736dd2e04afd656795e6cac28
Author: peter <peter@chromium.org>
Date: Tue Apr 11 17:36:17 2017

Migrate extension notifications to the new NotificationDisplayService

Today, the implementation of the Notifications Extension API directly
communicates with the NotificationUIManager. In doing so, it makes the
assumption that displayed notifications can be retrieved in whole.

However, since we're moving to native notification centers, which are
powered by the NotificationDisplayService as opposed to the
NotificationUIManager, we need to drop this assumption without breaking
the extension API.

This CL migrates the extension API to use the NDS while maintaining all
existing functionality. (Assuming the Message Center is used to
implement the NDS, moving away from which is a separate discussion in
which extension compatibility concerns are being discussed.)

BUG= 596161 

Review-Url: https://codereview.chromium.org/2703213004
Cr-Commit-Position: refs/heads/master@{#463671}

[modify] https://crrev.com/28d8b7398617c37736dd2e04afd656795e6cac28/chrome/browser/BUILD.gn
[modify] https://crrev.com/28d8b7398617c37736dd2e04afd656795e6cac28/chrome/browser/extensions/BUILD.gn
[add] https://crrev.com/28d8b7398617c37736dd2e04afd656795e6cac28/chrome/browser/extensions/api/notifications/extension_notification_display_helper.cc
[add] https://crrev.com/28d8b7398617c37736dd2e04afd656795e6cac28/chrome/browser/extensions/api/notifications/extension_notification_display_helper.h
[add] https://crrev.com/28d8b7398617c37736dd2e04afd656795e6cac28/chrome/browser/extensions/api/notifications/extension_notification_display_helper_factory.cc
[add] https://crrev.com/28d8b7398617c37736dd2e04afd656795e6cac28/chrome/browser/extensions/api/notifications/extension_notification_display_helper_factory.h
[add] https://crrev.com/28d8b7398617c37736dd2e04afd656795e6cac28/chrome/browser/extensions/api/notifications/extension_notification_handler.cc
[add] https://crrev.com/28d8b7398617c37736dd2e04afd656795e6cac28/chrome/browser/extensions/api/notifications/extension_notification_handler.h
[modify] https://crrev.com/28d8b7398617c37736dd2e04afd656795e6cac28/chrome/browser/extensions/api/notifications/notifications_api.cc
[modify] https://crrev.com/28d8b7398617c37736dd2e04afd656795e6cac28/chrome/browser/extensions/api/notifications/notifications_api.h
[modify] https://crrev.com/28d8b7398617c37736dd2e04afd656795e6cac28/chrome/browser/extensions/api/notifications/notifications_apitest.cc
[modify] https://crrev.com/28d8b7398617c37736dd2e04afd656795e6cac28/chrome/browser/notifications/native_notification_display_service.cc
[modify] https://crrev.com/28d8b7398617c37736dd2e04afd656795e6cac28/chrome/browser/notifications/non_persistent_notification_handler.cc
[modify] https://crrev.com/28d8b7398617c37736dd2e04afd656795e6cac28/chrome/browser/notifications/notification_common.h
[modify] https://crrev.com/28d8b7398617c37736dd2e04afd656795e6cac28/chrome/browser/notifications/platform_notification_service_unittest.cc
[add] https://crrev.com/28d8b7398617c37736dd2e04afd656795e6cac28/chrome/browser/notifications/stub_notification_display_service.cc
[add] https://crrev.com/28d8b7398617c37736dd2e04afd656795e6cac28/chrome/browser/notifications/stub_notification_display_service.h

Blockedon: 720345
Triage: Is there further work pending for this issue?
Status: Archived (was: Assigned)
Archiving old bugs that have only received trivial updates for some time.

If you feel this issue should still be addressed, feel free to reopen it or to file a new issue. Thanks!

Sign in to add a comment