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

Issue 856603 link

Starred by 1 user

Issue metadata

Status: Started
Owner:
Last visit > 30 days ago
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Replace Notification Database with Service Worker Database

Project Member Reported by yangsharon@google.com, Jun 26 2018

Issue description

Currently, there are two databases, the notification database and the service worker database. There is some degree of redundancy between them, and in combining the databases, we can remove a data storage source, the data that has to be stored and clean up the notification database code.

The notification database stores information about the notification itself and the service worker it is tied to. Since each notification is tied to exactly one service worker, combining the databases will remove duplicate storage. Functions that operate on all the notifications tied to a service worker can also be removed, as all notifications will be grouped by service worker in the service worker database. This will mean we can clean up the existing code.

The present notification database also stores a persistent notification id, which will be moved to the profile as it needs to persist across restarts.

Information about existing functions and what will have to change here: https://docs.google.com/document/d/1mrQEIjOittDZ-sRPKRW_6S5-j112idSSTnQxT9kUpmM/edit?usp=sharing
 
Cc: peter@chromium.org rayankans@chromium.org
Description: Show this description
Description: Show this description
Components: UI>Notifications
Project Member

Comment 5 by bugdroid1@chromium.org, Jul 16

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

commit c6706906e35d96358fa5a8a78931c0452fe098c2
Author: Sharon Yang <yangsharon@google.com>
Date: Mon Jul 16 13:06:19 2018

Move persistent notification id from notification database to profile.

Create a profile field that has the persistent notification ID, where it
can be stored on disk, as this value needs to persist across restarts.

Updating related unit tests.

Bug: 856603
Change-Id: I5c7083c8359c5e7fd7a2b612b861e8968150e90f
Reviewed-on: https://chromium-review.googlesource.com/1114840
Commit-Queue: Sharon Yang <yangsharon@google.com>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#575236}
[modify] https://crrev.com/c6706906e35d96358fa5a8a78931c0452fe098c2/chrome/browser/notifications/platform_notification_service_impl.cc
[modify] https://crrev.com/c6706906e35d96358fa5a8a78931c0452fe098c2/chrome/browser/notifications/platform_notification_service_impl.h
[modify] https://crrev.com/c6706906e35d96358fa5a8a78931c0452fe098c2/chrome/browser/notifications/platform_notification_service_unittest.cc
[modify] https://crrev.com/c6706906e35d96358fa5a8a78931c0452fe098c2/chrome/browser/prefs/browser_prefs.cc
[modify] https://crrev.com/c6706906e35d96358fa5a8a78931c0452fe098c2/chrome/browser/push_messaging/push_messaging_notification_manager.cc
[modify] https://crrev.com/c6706906e35d96358fa5a8a78931c0452fe098c2/chrome/common/pref_names.cc
[modify] https://crrev.com/c6706906e35d96358fa5a8a78931c0452fe098c2/chrome/common/pref_names.h
[modify] https://crrev.com/c6706906e35d96358fa5a8a78931c0452fe098c2/content/browser/notifications/blink_notification_service_impl.cc
[modify] https://crrev.com/c6706906e35d96358fa5a8a78931c0452fe098c2/content/browser/notifications/blink_notification_service_impl.h
[modify] https://crrev.com/c6706906e35d96358fa5a8a78931c0452fe098c2/content/browser/notifications/notification_database.cc
[modify] https://crrev.com/c6706906e35d96358fa5a8a78931c0452fe098c2/content/browser/notifications/notification_database.h
[modify] https://crrev.com/c6706906e35d96358fa5a8a78931c0452fe098c2/content/browser/notifications/notification_database_unittest.cc
[modify] https://crrev.com/c6706906e35d96358fa5a8a78931c0452fe098c2/content/browser/notifications/platform_notification_context_impl.cc
[modify] https://crrev.com/c6706906e35d96358fa5a8a78931c0452fe098c2/content/browser/notifications/platform_notification_context_impl.h
[modify] https://crrev.com/c6706906e35d96358fa5a8a78931c0452fe098c2/content/browser/notifications/platform_notification_context_unittest.cc
[modify] https://crrev.com/c6706906e35d96358fa5a8a78931c0452fe098c2/content/public/browser/platform_notification_context.h
[modify] https://crrev.com/c6706906e35d96358fa5a8a78931c0452fe098c2/content/public/browser/platform_notification_service.h
[modify] https://crrev.com/c6706906e35d96358fa5a8a78931c0452fe098c2/content/test/mock_platform_notification_service.cc
[modify] https://crrev.com/c6706906e35d96358fa5a8a78931c0452fe098c2/content/test/mock_platform_notification_service.h

Project Member

Comment 6 by bugdroid1@chromium.org, Aug 1

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

commit addbe4923a68210baf7e2fe02458391392085a68
Author: Sharon Yang <yangsharon@google.com>
Date: Wed Aug 01 18:16:43 2018

Migrate functionality from notification db to service worker db.

This CL implements the Read and Write functionality for the new
data store. It has not yet replaced the existing notification
database.

Bug: 856603
Change-Id: Ie218078f1a17ef0948279fef4632c24cdc437ff0
Reviewed-on: https://chromium-review.googlesource.com/1140163
Commit-Queue: Sharon Yang <yangsharon@google.com>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Reviewed-by: Rayan Kanso <rayankans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579863}
[modify] https://crrev.com/addbe4923a68210baf7e2fe02458391392085a68/chrome/browser/push_messaging/push_messaging_notification_manager.cc
[modify] https://crrev.com/addbe4923a68210baf7e2fe02458391392085a68/content/browser/BUILD.gn
[modify] https://crrev.com/addbe4923a68210baf7e2fe02458391392085a68/content/browser/notifications/blink_notification_service_impl.cc
[modify] https://crrev.com/addbe4923a68210baf7e2fe02458391392085a68/content/browser/notifications/notification_database.cc
[add] https://crrev.com/addbe4923a68210baf7e2fe02458391392085a68/content/browser/notifications/notification_storage.cc
[add] https://crrev.com/addbe4923a68210baf7e2fe02458391392085a68/content/browser/notifications/notification_storage.h
[add] https://crrev.com/addbe4923a68210baf7e2fe02458391392085a68/content/browser/notifications/notification_storage_unittest.cc
[modify] https://crrev.com/addbe4923a68210baf7e2fe02458391392085a68/content/browser/notifications/platform_notification_context_impl.cc
[modify] https://crrev.com/addbe4923a68210baf7e2fe02458391392085a68/content/browser/notifications/platform_notification_context_impl.h
[modify] https://crrev.com/addbe4923a68210baf7e2fe02458391392085a68/content/browser/notifications/platform_notification_context_unittest.cc
[modify] https://crrev.com/addbe4923a68210baf7e2fe02458391392085a68/content/public/browser/platform_notification_context.h
[modify] https://crrev.com/addbe4923a68210baf7e2fe02458391392085a68/content/test/BUILD.gn

Labels: Hotlist-DesktopUIValid Hotlist-DesktopUIChecked
*** UI Mass Triage***
Seems like WIP and bug is valid, hence tagging with appropriate label.If there is no pending work, please feel free to close the issue.

Sign in to add a comment