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

Issue 796991 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocked on:
issue 798742

Blocking:
issue 595685
issue 814316



Sign in to add a comment

Migrate persistent web notifications to mojo

Project Member Reported by awdf@chromium.org, Dec 21 2017

Issue description

Comment 1 by awdf@chromium.org, Dec 21 2017

Summary: Migrate persistent web notifications to mojo (was: Migrate persistent notifications to mojo)
Project Member

Comment 2 by bugdroid1@chromium.org, Jan 17 2018

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

commit 3de15be70a17bce0000e0ab476120dd90a063360
Author: Anita Woodruff <awdf@chromium.org>
Date: Wed Jan 17 12:06:11 2018

[Mojoification] Add notification.mojom with struct traits

- Added NotificationData mojo struct and struct traits to
allow conversion to/from blink::WebNotificationData and
content::PlatformNotificationData.

- The struct traits live in //content/common/ for now because there
is still event dispatching code in //content/renderer/. Once
mojification is complete, they can be moved to //content/browser/.

- Note the mojo pathway is still behind the NotificationsWithMojo
flag while development continues.

Bug:  595685 ,  796990 ,  796991 
Change-Id: I60e195f8c0bf9a2c0e8f9dc78ebaf95349b738c8
Reviewed-on: https://chromium-review.googlesource.com/832396
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Commit-Queue: Anita Woodruff <awdf@chromium.org>
Cr-Commit-Position: refs/heads/master@{#529718}
[modify] https://crrev.com/3de15be70a17bce0000e0ab476120dd90a063360/chrome/browser/notifications/platform_notification_service_interactive_uitest.cc
[modify] https://crrev.com/3de15be70a17bce0000e0ab476120dd90a063360/content/browser/notifications/OWNERS
[modify] https://crrev.com/3de15be70a17bce0000e0ab476120dd90a063360/content/browser/notifications/blink_notification_service_impl.cc
[modify] https://crrev.com/3de15be70a17bce0000e0ab476120dd90a063360/content/browser/notifications/blink_notification_service_impl.h
[modify] https://crrev.com/3de15be70a17bce0000e0ab476120dd90a063360/content/common/BUILD.gn
[add] https://crrev.com/3de15be70a17bce0000e0ab476120dd90a063360/content/common/notifications/DEPS
[add] https://crrev.com/3de15be70a17bce0000e0ab476120dd90a063360/content/common/notifications/OWNERS
[add] https://crrev.com/3de15be70a17bce0000e0ab476120dd90a063360/content/common/notifications/notification_struct_traits.cc
[add] https://crrev.com/3de15be70a17bce0000e0ab476120dd90a063360/content/common/notifications/notification_struct_traits.h
[add] https://crrev.com/3de15be70a17bce0000e0ab476120dd90a063360/content/common/notifications/notification_struct_traits_unittest.cc
[add] https://crrev.com/3de15be70a17bce0000e0ab476120dd90a063360/content/common/notifications/notification_types.typemap
[modify] https://crrev.com/3de15be70a17bce0000e0ab476120dd90a063360/content/common/typemaps.gni
[modify] https://crrev.com/3de15be70a17bce0000e0ab476120dd90a063360/content/test/BUILD.gn
[modify] https://crrev.com/3de15be70a17bce0000e0ab476120dd90a063360/third_party/WebKit/Source/modules/notifications/Notification.cpp
[modify] https://crrev.com/3de15be70a17bce0000e0ab476120dd90a063360/third_party/WebKit/Source/modules/notifications/NotificationManager.cpp
[modify] https://crrev.com/3de15be70a17bce0000e0ab476120dd90a063360/third_party/WebKit/Source/modules/notifications/NotificationManager.h
[modify] https://crrev.com/3de15be70a17bce0000e0ab476120dd90a063360/third_party/WebKit/Source/platform/BUILD.gn
[add] https://crrev.com/3de15be70a17bce0000e0ab476120dd90a063360/third_party/WebKit/Source/platform/mojo/NotificationStructTraits.cpp
[add] https://crrev.com/3de15be70a17bce0000e0ab476120dd90a063360/third_party/WebKit/Source/platform/mojo/NotificationStructTraits.h
[add] https://crrev.com/3de15be70a17bce0000e0ab476120dd90a063360/third_party/WebKit/Source/platform/mojo/NotificationStructTraitsTest.cpp
[modify] https://crrev.com/3de15be70a17bce0000e0ab476120dd90a063360/third_party/WebKit/Source/platform/mojo/blink_typemaps.gni
[modify] https://crrev.com/3de15be70a17bce0000e0ab476120dd90a063360/third_party/WebKit/public/BUILD.gn
[add] https://crrev.com/3de15be70a17bce0000e0ab476120dd90a063360/third_party/WebKit/public/platform/modules/notifications/NotificationTypes.typemap
[modify] https://crrev.com/3de15be70a17bce0000e0ab476120dd90a063360/third_party/WebKit/public/platform/modules/notifications/OWNERS
[add] https://crrev.com/3de15be70a17bce0000e0ab476120dd90a063360/third_party/WebKit/public/platform/modules/notifications/notification.mojom
[modify] https://crrev.com/3de15be70a17bce0000e0ab476120dd90a063360/third_party/WebKit/public/platform/modules/notifications/notification_service.mojom

Project Member

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

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

commit b0e3c370f735edef8128ae377f6fe163e02df2b0
Author: Anita Woodruff <awdf@chromium.org>
Date: Fri Jan 19 19:45:50 2018

[Mojoification] Add notification resources

- Added NotificationResources mojo struct and struct traits to
allow conversion to/from blink::WebNotificationResources and
content::NotificationResources.

- The struct traits live in //content/common/ for now because there
is still event dispatching code in //content/renderer/. Once
mojification is complete, they can be moved to //content/browser/.

- Note the mojo pathway is still behind the NotificationsWithMojo
flag while development continues.

Bug:  595685 ,  796990 ,  796991 
Change-Id: I82955f5d072545a66c31e8a7a502ac36eb8b6008
Reviewed-on: https://chromium-review.googlesource.com/874692
Commit-Queue: Anita Woodruff <awdf@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#530585}
[modify] https://crrev.com/b0e3c370f735edef8128ae377f6fe163e02df2b0/chrome/browser/notifications/platform_notification_service_interactive_uitest.cc
[modify] https://crrev.com/b0e3c370f735edef8128ae377f6fe163e02df2b0/content/browser/notifications/blink_notification_service_impl.cc
[modify] https://crrev.com/b0e3c370f735edef8128ae377f6fe163e02df2b0/content/browser/notifications/blink_notification_service_impl.h
[modify] https://crrev.com/b0e3c370f735edef8128ae377f6fe163e02df2b0/content/common/notifications/notification_struct_traits.cc
[modify] https://crrev.com/b0e3c370f735edef8128ae377f6fe163e02df2b0/content/common/notifications/notification_struct_traits.h
[modify] https://crrev.com/b0e3c370f735edef8128ae377f6fe163e02df2b0/content/common/notifications/notification_struct_traits_unittest.cc
[modify] https://crrev.com/b0e3c370f735edef8128ae377f6fe163e02df2b0/content/common/notifications/notification_types.typemap
[modify] https://crrev.com/b0e3c370f735edef8128ae377f6fe163e02df2b0/third_party/WebKit/Source/modules/notifications/Notification.cpp
[modify] https://crrev.com/b0e3c370f735edef8128ae377f6fe163e02df2b0/third_party/WebKit/Source/modules/notifications/NotificationManager.cpp
[modify] https://crrev.com/b0e3c370f735edef8128ae377f6fe163e02df2b0/third_party/WebKit/Source/modules/notifications/NotificationManager.h
[modify] https://crrev.com/b0e3c370f735edef8128ae377f6fe163e02df2b0/third_party/WebKit/Source/platform/mojo/DEPS
[modify] https://crrev.com/b0e3c370f735edef8128ae377f6fe163e02df2b0/third_party/WebKit/Source/platform/mojo/NotificationStructTraits.cpp
[modify] https://crrev.com/b0e3c370f735edef8128ae377f6fe163e02df2b0/third_party/WebKit/Source/platform/mojo/NotificationStructTraits.h
[modify] https://crrev.com/b0e3c370f735edef8128ae377f6fe163e02df2b0/third_party/WebKit/Source/platform/mojo/NotificationStructTraitsTest.cpp
[modify] https://crrev.com/b0e3c370f735edef8128ae377f6fe163e02df2b0/third_party/WebKit/public/platform/modules/notifications/NotificationTypes.typemap
[modify] https://crrev.com/b0e3c370f735edef8128ae377f6fe163e02df2b0/third_party/WebKit/public/platform/modules/notifications/notification.mojom
[modify] https://crrev.com/b0e3c370f735edef8128ae377f6fe163e02df2b0/third_party/WebKit/public/platform/modules/notifications/notification_service.mojom

Project Member

Comment 4 by bugdroid1@chromium.org, Feb 8 2018

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

commit 51fba7d28474e0a3a2a1b4dbf42ef20c97fa22dd
Author: Anita Woodruff <awdf@chromium.org>
Date: Thu Feb 08 17:34:09 2018

[Mojo] Use NotificationsWithMojo flag for persistent notifications

- No-op instead of using the legacy IPC message handlers when the
NotificationsWithMojo blink feature flag is enabled. This sets things
up for mojoifying persistent notifications incrementally behind the flag.

- Updates test expectations for most notification layout tests to
start failing when the flag is enabled.

Bug:  796991 
Change-Id: I4a722001a0945b4eb6c49059094640ff1c410d68
Reviewed-on: https://chromium-review.googlesource.com/906772
Commit-Queue: Anita Woodruff <awdf@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#535426}
[modify] https://crrev.com/51fba7d28474e0a3a2a1b4dbf42ef20c97fa22dd/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/51fba7d28474e0a3a2a1b4dbf42ef20c97fa22dd/third_party/WebKit/Source/modules/notifications/Notification.cpp
[modify] https://crrev.com/51fba7d28474e0a3a2a1b4dbf42ef20c97fa22dd/third_party/WebKit/Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp

Comment 5 by awdf@chromium.org, Feb 21 2018

Blocking: 814316
Project Member

Comment 6 by bugdroid1@chromium.org, Feb 22 2018

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

commit 6c4108abcf29ce93d07ce5d9514f3b3a34728d7f
Author: Anita Woodruff <awdf@chromium.org>
Date: Thu Feb 22 19:23:25 2018

[Notifications] Implement DisplayPersistentNotification via mojo

Part 1:
- Hook up the Display path from ServiceWorkerRegistrationNotifications
through to the PlatformNotificationService via the mojo service.

- This patch does not yet write data to the notification database or
pass the real service worker scope to the display service - these
changes will follow in a further patch.

- Note this code is all behind a flag, and can be enabled at the
command line for testing with '--enable-features=NotificationsWithMojo'

R=peter@chromium.org

Bug:  796991 
Change-Id: I3e937f267df85996761d34355b20920836592712
Reviewed-on: https://chromium-review.googlesource.com/921582
Reviewed-by: Oliver Chang <ochang@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Commit-Queue: Anita Woodruff <awdf@chromium.org>
Cr-Commit-Position: refs/heads/master@{#538520}
[modify] https://crrev.com/6c4108abcf29ce93d07ce5d9514f3b3a34728d7f/chrome/browser/notifications/platform_notification_service_interactive_uitest.cc
[modify] https://crrev.com/6c4108abcf29ce93d07ce5d9514f3b3a34728d7f/content/browser/notifications/blink_notification_service_impl.cc
[modify] https://crrev.com/6c4108abcf29ce93d07ce5d9514f3b3a34728d7f/content/browser/notifications/blink_notification_service_impl.h
[modify] https://crrev.com/6c4108abcf29ce93d07ce5d9514f3b3a34728d7f/content/browser/notifications/blink_notification_service_impl_unittest.cc
[modify] https://crrev.com/6c4108abcf29ce93d07ce5d9514f3b3a34728d7f/third_party/WebKit/Source/modules/notifications/NotificationManager.cpp
[modify] https://crrev.com/6c4108abcf29ce93d07ce5d9514f3b3a34728d7f/third_party/WebKit/Source/modules/notifications/NotificationManager.h
[modify] https://crrev.com/6c4108abcf29ce93d07ce5d9514f3b3a34728d7f/third_party/WebKit/Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp
[modify] https://crrev.com/6c4108abcf29ce93d07ce5d9514f3b3a34728d7f/third_party/WebKit/public/platform/modules/notifications/notification_service.mojom

Project Member

Comment 7 by bugdroid1@chromium.org, Feb 26 2018

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

commit 980756aa646d97316889d11eea6a7660e0ef8456
Author: Anita Woodruff <awdf@chromium.org>
Date: Mon Feb 26 17:10:49 2018

[Notifications] Write data to database when displaying via mojo

- Display() Part 2: Write notification data to database; use a real
notification ID.

- Note this is still behind the NotificationsWithMojo flag while
development continues.

Bug:  796991 
Change-Id: I61d146b8f675c5e82fc7b3ea5e7782a3229f9a7c
Reviewed-on: https://chromium-review.googlesource.com/925461
Commit-Queue: Anita Woodruff <awdf@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#539171}
[modify] https://crrev.com/980756aa646d97316889d11eea6a7660e0ef8456/content/browser/notifications/blink_notification_service_impl.cc
[modify] https://crrev.com/980756aa646d97316889d11eea6a7660e0ef8456/content/browser/notifications/blink_notification_service_impl.h
[modify] https://crrev.com/980756aa646d97316889d11eea6a7660e0ef8456/content/browser/notifications/blink_notification_service_impl_unittest.cc

Project Member

Comment 8 by bugdroid1@chromium.org, Feb 28 2018

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

commit ade876eb90e644cea0a1d3ca79674b398f141ba3
Author: Anita Woodruff <awdf@chromium.org>
Date: Wed Feb 28 16:13:36 2018

[Notifications] Respect content image kill switch via mojo

- Previously this flag was enforced in ValidateNotificationResources in
NotificationMessageFilter, in case of a compromised renderer which
didn't respect the NotificationContentImage feature flag.

- This patch ensures we continue to enforce this flag for notifications
which take the new mojo code path.

R=peter@chromium.org

Bug:  796990 ,  796991 
Change-Id: Ic048757c8a2f3c5b063e4987da3a00347335927d
Reviewed-on: https://chromium-review.googlesource.com/930965
Commit-Queue: Anita Woodruff <awdf@chromium.org>
Reviewed-by: Oliver Chang <ochang@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#539840}
[modify] https://crrev.com/ade876eb90e644cea0a1d3ca79674b398f141ba3/chrome/browser/notifications/platform_notification_service_interactive_uitest.cc
[modify] https://crrev.com/ade876eb90e644cea0a1d3ca79674b398f141ba3/content/common/notifications/notification_struct_traits.cc
[modify] https://crrev.com/ade876eb90e644cea0a1d3ca79674b398f141ba3/content/common/notifications/notification_struct_traits_unittest.cc

Project Member

Comment 9 by bugdroid1@chromium.org, Mar 13 2018

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

commit 03cec264f1e0fd79a6d4623d8100adebfcb5a80d
Author: Anita Woodruff <awdf@chromium.org>
Date: Tue Mar 13 19:54:47 2018

[Notifications] Use the service worker scope when displaying via mojo

- Display() Part 3: Attribute notification correctly by looking up
service worker scope from the service worker registration ID.

- Note the mojo pathway is guarded behind the NotificationsWithMojo
flag as development continues.

R=peter@chromium.org

Bug:  796991 
Change-Id: I7fb7b9402cb44ee6810b29c8ed472771842fc5ed
Reviewed-on: https://chromium-review.googlesource.com/929082
Commit-Queue: Anita Woodruff <awdf@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#542887}
[modify] https://crrev.com/03cec264f1e0fd79a6d4623d8100adebfcb5a80d/chrome/browser/notifications/platform_notification_service_interactive_uitest.cc
[modify] https://crrev.com/03cec264f1e0fd79a6d4623d8100adebfcb5a80d/content/browser/notifications/blink_notification_service_impl.cc
[modify] https://crrev.com/03cec264f1e0fd79a6d4623d8100adebfcb5a80d/content/browser/notifications/blink_notification_service_impl.h
[modify] https://crrev.com/03cec264f1e0fd79a6d4623d8100adebfcb5a80d/content/browser/notifications/blink_notification_service_impl_unittest.cc
[modify] https://crrev.com/03cec264f1e0fd79a6d4623d8100adebfcb5a80d/content/browser/notifications/platform_notification_context_impl.cc

Comment 10 by awdf@chromium.org, Mar 20 2018

Description: Show this description
Project Member

Comment 11 by bugdroid1@chromium.org, Mar 20 2018

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

commit 5ea5ae1706669fb14df5854f2421c6444483ae42
Author: Anita Woodruff <awdf@chromium.org>
Date: Tue Mar 20 19:39:55 2018

[Notifications] Re-enable some layout tests when mojo flag enabled

- Now that the Display path has been migrated to mojo, the only tests
that should still fail are those relying on getNotifications and
Notification.close, when the NotificationsWithMojo flag is enabled.

R=peter@chromium.org

Bug:  796991 
Change-Id: If26ec3c1f47b1ce83866d0c514a5b15a827f3577
Reviewed-on: https://chromium-review.googlesource.com/971484
Reviewed-by: Peter Beverloo <peter@chromium.org>
Commit-Queue: Anita Woodruff <awdf@chromium.org>
Cr-Commit-Position: refs/heads/master@{#544492}
[modify] https://crrev.com/5ea5ae1706669fb14df5854f2421c6444483ae42/third_party/WebKit/LayoutTests/TestExpectations

Project Member

Comment 12 by bugdroid1@chromium.org, Apr 11 2018

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

commit 9d7e842db8222f28f16a82671da54933ef05709f
Author: Anita Woodruff <awdf@chromium.org>
Date: Wed Apr 11 23:39:43 2018

[Notifications] Use mojo array of *optional* bitmaps for action icons

- While the array length of NotificationResources.action_icons must
match that of NotificationData.actions, the bitmaps it contains may be
null, so add a '?' to allow for that.

- This allows 2 more layout tests to pass when the mojo-notifications
flag is enabled. (They previously failed with 'bad serialization'
errors, when actions were used with no icons).

Bug:  796991 
Change-Id: Ibf8f93ceb9cd2a556597d28ec41fab726257a661
Reviewed-on: https://chromium-review.googlesource.com/1005116
Reviewed-by: Oliver Chang <ochang@chromium.org>
Commit-Queue: Anita Woodruff <awdf@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549971}
[modify] https://crrev.com/9d7e842db8222f28f16a82671da54933ef05709f/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/9d7e842db8222f28f16a82671da54933ef05709f/third_party/blink/public/platform/modules/notifications/notification.mojom

Project Member

Comment 13 by bugdroid1@chromium.org, Apr 12 2018

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

commit 108f6589f28676495ce549448272391bf1bd283a
Author: Anita Woodruff <awdf@chromium.org>
Date: Thu Apr 12 21:44:33 2018

[Notifications] Implement GetNotifications via mojo

- Now when the '--enable-features=NotificationsWithMojo' flag is set,
  ServiceWorkerRegistration.getNotifications() will be routed through
  the notification mojo service, instead of via legacy IPC.

R=peter@chromium.org

Bug:  796991 
Change-Id: Ie3d24ae753238da90c316187a4f2b84b4b52d531
Reviewed-on: https://chromium-review.googlesource.com/976222
Commit-Queue: Anita Woodruff <awdf@chromium.org>
Reviewed-by: Oliver Chang <ochang@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550372}
[modify] https://crrev.com/108f6589f28676495ce549448272391bf1bd283a/chrome/browser/notifications/platform_notification_service_interactive_uitest.cc
[modify] https://crrev.com/108f6589f28676495ce549448272391bf1bd283a/content/browser/notifications/blink_notification_service_impl.cc
[modify] https://crrev.com/108f6589f28676495ce549448272391bf1bd283a/content/browser/notifications/blink_notification_service_impl.h
[modify] https://crrev.com/108f6589f28676495ce549448272391bf1bd283a/content/browser/notifications/blink_notification_service_impl_unittest.cc
[modify] https://crrev.com/108f6589f28676495ce549448272391bf1bd283a/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/108f6589f28676495ce549448272391bf1bd283a/third_party/blink/public/platform/modules/notifications/notification_service.mojom
[modify] https://crrev.com/108f6589f28676495ce549448272391bf1bd283a/third_party/blink/renderer/modules/notifications/notification_manager.cc
[modify] https://crrev.com/108f6589f28676495ce549448272391bf1bd283a/third_party/blink/renderer/modules/notifications/notification_manager.h
[modify] https://crrev.com/108f6589f28676495ce549448272391bf1bd283a/third_party/blink/renderer/modules/notifications/service_worker_registration_notifications.cc

Comment 14 by awdf@chromium.org, Apr 12 2018

Blockedon: 798742
Project Member

Comment 15 by bugdroid1@chromium.org, Apr 16 2018

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

commit 34e1a2c540fb79adae59dddad6bbdcf716b9da13
Author: Anita Woodruff <awdf@chromium.org>
Date: Mon Apr 16 20:17:34 2018

[Notifications] Use ScriptPromiseResolver directly in Mojo codepath

- Previously when displaying and getting notifications the promise
resolver was always converted to callbacks, for the legacy IPC code
which needed to store these callbacks in a map.

- Mojo renders a callback map unnecessary, so this patch refactors
the code to avoid the conversion when the Mojo pathway is taken.

Bug:  796991 
Change-Id: I216730c06fdb42a3c2c9fe21708252cb652e8b6b
Reviewed-on: https://chromium-review.googlesource.com/1011422
Commit-Queue: Anita Woodruff <awdf@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551094}
[modify] https://crrev.com/34e1a2c540fb79adae59dddad6bbdcf716b9da13/third_party/blink/renderer/modules/notifications/notification_manager.cc
[modify] https://crrev.com/34e1a2c540fb79adae59dddad6bbdcf716b9da13/third_party/blink/renderer/modules/notifications/notification_manager.h
[modify] https://crrev.com/34e1a2c540fb79adae59dddad6bbdcf716b9da13/third_party/blink/renderer/modules/notifications/service_worker_registration_notifications.cc
[modify] https://crrev.com/34e1a2c540fb79adae59dddad6bbdcf716b9da13/third_party/blink/renderer/modules/notifications/service_worker_registration_notifications.h

Project Member

Comment 16 by bugdroid1@chromium.org, Apr 16 2018

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

commit 7b7146becae09691247fd05c792bc9f84102d464
Author: Anita Woodruff <awdf@chromium.org>
Date: Mon Apr 16 22:26:16 2018

[Notifications] Check max data on the renderer-side

- The legacy IPC pathway was checking the max data
size *before* the IPC call, and rejecting if it was
exceeded.

- This patch reinstates the check for the new mojo
pathway, so that we don't risk crashing the renderer
(thanks to verification in struct traits) if max data
is exceeded.

- This involved moving the max data constant to mojo
so it can be accessed from both blink and content.

R=peter@chromium.org

Bug:  798742 , 796991 
Change-Id: I14ae782c9265411da1374beb7ae7312cb265b07e
Reviewed-on: https://chromium-review.googlesource.com/1011602
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Commit-Queue: Anita Woodruff <awdf@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551146}
[modify] https://crrev.com/7b7146becae09691247fd05c792bc9f84102d464/content/common/notifications/notification_struct_traits.cc
[modify] https://crrev.com/7b7146becae09691247fd05c792bc9f84102d464/content/public/common/platform_notification_data.h
[modify] https://crrev.com/7b7146becae09691247fd05c792bc9f84102d464/content/renderer/notifications/notification_manager.cc
[modify] https://crrev.com/7b7146becae09691247fd05c792bc9f84102d464/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/7b7146becae09691247fd05c792bc9f84102d464/third_party/blink/public/platform/modules/notifications/notification.mojom
[modify] https://crrev.com/7b7146becae09691247fd05c792bc9f84102d464/third_party/blink/renderer/modules/notifications/notification_manager.cc

Project Member

Comment 17 by bugdroid1@chromium.org, Apr 17 2018

Labels: merge-merged-testbranch
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/108f6589f28676495ce549448272391bf1bd283a

commit 108f6589f28676495ce549448272391bf1bd283a
Author: Anita Woodruff <awdf@chromium.org>
Date: Thu Apr 12 21:44:33 2018

[Notifications] Implement GetNotifications via mojo

- Now when the '--enable-features=NotificationsWithMojo' flag is set,
  ServiceWorkerRegistration.getNotifications() will be routed through
  the notification mojo service, instead of via legacy IPC.

R=peter@chromium.org

Bug:  796991 
Change-Id: Ie3d24ae753238da90c316187a4f2b84b4b52d531
Reviewed-on: https://chromium-review.googlesource.com/976222
Commit-Queue: Anita Woodruff <awdf@chromium.org>
Reviewed-by: Oliver Chang <ochang@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550372}
[modify] https://crrev.com/108f6589f28676495ce549448272391bf1bd283a/chrome/browser/notifications/platform_notification_service_interactive_uitest.cc
[modify] https://crrev.com/108f6589f28676495ce549448272391bf1bd283a/content/browser/notifications/blink_notification_service_impl.cc
[modify] https://crrev.com/108f6589f28676495ce549448272391bf1bd283a/content/browser/notifications/blink_notification_service_impl.h
[modify] https://crrev.com/108f6589f28676495ce549448272391bf1bd283a/content/browser/notifications/blink_notification_service_impl_unittest.cc
[modify] https://crrev.com/108f6589f28676495ce549448272391bf1bd283a/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/108f6589f28676495ce549448272391bf1bd283a/third_party/blink/public/platform/modules/notifications/notification_service.mojom
[modify] https://crrev.com/108f6589f28676495ce549448272391bf1bd283a/third_party/blink/renderer/modules/notifications/notification_manager.cc
[modify] https://crrev.com/108f6589f28676495ce549448272391bf1bd283a/third_party/blink/renderer/modules/notifications/notification_manager.h
[modify] https://crrev.com/108f6589f28676495ce549448272391bf1bd283a/third_party/blink/renderer/modules/notifications/service_worker_registration_notifications.cc

Project Member

Comment 18 by bugdroid1@chromium.org, Apr 17 2018

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

commit 34e1a2c540fb79adae59dddad6bbdcf716b9da13
Author: Anita Woodruff <awdf@chromium.org>
Date: Mon Apr 16 20:17:34 2018

[Notifications] Use ScriptPromiseResolver directly in Mojo codepath

- Previously when displaying and getting notifications the promise
resolver was always converted to callbacks, for the legacy IPC code
which needed to store these callbacks in a map.

- Mojo renders a callback map unnecessary, so this patch refactors
the code to avoid the conversion when the Mojo pathway is taken.

Bug:  796991 
Change-Id: I216730c06fdb42a3c2c9fe21708252cb652e8b6b
Reviewed-on: https://chromium-review.googlesource.com/1011422
Commit-Queue: Anita Woodruff <awdf@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551094}
[modify] https://crrev.com/34e1a2c540fb79adae59dddad6bbdcf716b9da13/third_party/blink/renderer/modules/notifications/notification_manager.cc
[modify] https://crrev.com/34e1a2c540fb79adae59dddad6bbdcf716b9da13/third_party/blink/renderer/modules/notifications/notification_manager.h
[modify] https://crrev.com/34e1a2c540fb79adae59dddad6bbdcf716b9da13/third_party/blink/renderer/modules/notifications/service_worker_registration_notifications.cc
[modify] https://crrev.com/34e1a2c540fb79adae59dddad6bbdcf716b9da13/third_party/blink/renderer/modules/notifications/service_worker_registration_notifications.h

Project Member

Comment 19 by bugdroid1@chromium.org, Apr 17 2018

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

commit 7b7146becae09691247fd05c792bc9f84102d464
Author: Anita Woodruff <awdf@chromium.org>
Date: Mon Apr 16 22:26:16 2018

[Notifications] Check max data on the renderer-side

- The legacy IPC pathway was checking the max data
size *before* the IPC call, and rejecting if it was
exceeded.

- This patch reinstates the check for the new mojo
pathway, so that we don't risk crashing the renderer
(thanks to verification in struct traits) if max data
is exceeded.

- This involved moving the max data constant to mojo
so it can be accessed from both blink and content.

R=peter@chromium.org

Bug:  798742 , 796991 
Change-Id: I14ae782c9265411da1374beb7ae7312cb265b07e
Reviewed-on: https://chromium-review.googlesource.com/1011602
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Commit-Queue: Anita Woodruff <awdf@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551146}
[modify] https://crrev.com/7b7146becae09691247fd05c792bc9f84102d464/content/common/notifications/notification_struct_traits.cc
[modify] https://crrev.com/7b7146becae09691247fd05c792bc9f84102d464/content/public/common/platform_notification_data.h
[modify] https://crrev.com/7b7146becae09691247fd05c792bc9f84102d464/content/renderer/notifications/notification_manager.cc
[modify] https://crrev.com/7b7146becae09691247fd05c792bc9f84102d464/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/7b7146becae09691247fd05c792bc9f84102d464/third_party/blink/public/platform/modules/notifications/notification.mojom
[modify] https://crrev.com/7b7146becae09691247fd05c792bc9f84102d464/third_party/blink/renderer/modules/notifications/notification_manager.cc

Project Member

Comment 20 by bugdroid1@chromium.org, Apr 18 2018

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

commit f645418b7d80a8468ccc055f893a631612fc8a13
Author: Anita Woodruff <awdf@chromium.org>
Date: Wed Apr 18 05:25:59 2018

[Notifications] Migrate ClosePersistentNotification to mojo

- The final piece in the jigsaw of migrating notification methods
over to mojo.

- Note this is still behind the NotificationsWithMojo feature flag.

R=peter@chromium.org

Bug:  796991 
Change-Id: Ic590154af590595b122f33ab200be22a628b1d67
Reviewed-on: https://chromium-review.googlesource.com/1014042
Commit-Queue: Anita Woodruff <awdf@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Reviewed-by: Oliver Chang <ochang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551590}
[modify] https://crrev.com/f645418b7d80a8468ccc055f893a631612fc8a13/chrome/browser/notifications/platform_notification_service_interactive_uitest.cc
[modify] https://crrev.com/f645418b7d80a8468ccc055f893a631612fc8a13/content/browser/notifications/blink_notification_service_impl.cc
[modify] https://crrev.com/f645418b7d80a8468ccc055f893a631612fc8a13/content/browser/notifications/blink_notification_service_impl.h
[modify] https://crrev.com/f645418b7d80a8468ccc055f893a631612fc8a13/content/browser/notifications/blink_notification_service_impl_unittest.cc
[modify] https://crrev.com/f645418b7d80a8468ccc055f893a631612fc8a13/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/f645418b7d80a8468ccc055f893a631612fc8a13/third_party/blink/public/platform/modules/notifications/notification_service.mojom
[modify] https://crrev.com/f645418b7d80a8468ccc055f893a631612fc8a13/third_party/blink/renderer/modules/notifications/notification.cc
[modify] https://crrev.com/f645418b7d80a8468ccc055f893a631612fc8a13/third_party/blink/renderer/modules/notifications/notification_manager.cc
[modify] https://crrev.com/f645418b7d80a8468ccc055f893a631612fc8a13/third_party/blink/renderer/modules/notifications/notification_manager.h

Project Member

Comment 21 by bugdroid1@chromium.org, May 10 2018

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

commit 5b6c23a4f50a50bd859d96d22679f2e3124783d4
Author: Anita Woodruff <awdf@chromium.org>
Date: Thu May 10 09:57:04 2018

[Notifications] Enable mojo for service worker notifications

- Enables the mojo pathway by default and removes the legacy IPC
code path for persistent notifications.

- Removes the NotificationsWithMojo feature flag which previously
guarded this code-path during incremental development.

R=kinuko@chromium.org

Bug:  796991 
Change-Id: Ia4dac0ab4b98b7a6df342b7df1b424e3af67bf05
Reviewed-on: https://chromium-review.googlesource.com/1027843
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Commit-Queue: Anita Woodruff <awdf@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557486}
[modify] https://crrev.com/5b6c23a4f50a50bd859d96d22679f2e3124783d4/chrome/browser/notifications/platform_notification_service_interactive_uitest.cc
[modify] https://crrev.com/5b6c23a4f50a50bd859d96d22679f2e3124783d4/content/browser/BUILD.gn
[modify] https://crrev.com/5b6c23a4f50a50bd859d96d22679f2e3124783d4/content/browser/notifications/notification_event_dispatcher_impl.cc
[delete] https://crrev.com/47084bae2a1755b4107436d9f7f01aea72557a31/content/browser/notifications/notification_message_filter.cc
[delete] https://crrev.com/47084bae2a1755b4107436d9f7f01aea72557a31/content/browser/notifications/notification_message_filter.h
[modify] https://crrev.com/5b6c23a4f50a50bd859d96d22679f2e3124783d4/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/5b6c23a4f50a50bd859d96d22679f2e3124783d4/content/browser/renderer_host/render_process_host_impl.h
[modify] https://crrev.com/5b6c23a4f50a50bd859d96d22679f2e3124783d4/content/child/runtime_features.cc
[modify] https://crrev.com/5b6c23a4f50a50bd859d96d22679f2e3124783d4/content/common/platform_notification_messages.h
[modify] https://crrev.com/5b6c23a4f50a50bd859d96d22679f2e3124783d4/content/public/common/content_features.cc
[modify] https://crrev.com/5b6c23a4f50a50bd859d96d22679f2e3124783d4/content/public/common/content_features.h
[modify] https://crrev.com/5b6c23a4f50a50bd859d96d22679f2e3124783d4/content/renderer/BUILD.gn
[modify] https://crrev.com/5b6c23a4f50a50bd859d96d22679f2e3124783d4/content/renderer/notifications/notification_data_conversions.cc
[modify] https://crrev.com/5b6c23a4f50a50bd859d96d22679f2e3124783d4/content/renderer/notifications/notification_data_conversions.h
[modify] https://crrev.com/5b6c23a4f50a50bd859d96d22679f2e3124783d4/content/renderer/notifications/notification_data_conversions_unittest.cc
[delete] https://crrev.com/47084bae2a1755b4107436d9f7f01aea72557a31/content/renderer/notifications/notification_dispatcher.cc
[delete] https://crrev.com/47084bae2a1755b4107436d9f7f01aea72557a31/content/renderer/notifications/notification_dispatcher.h
[delete] https://crrev.com/47084bae2a1755b4107436d9f7f01aea72557a31/content/renderer/notifications/notification_manager.cc
[delete] https://crrev.com/47084bae2a1755b4107436d9f7f01aea72557a31/content/renderer/notifications/notification_manager.h
[modify] https://crrev.com/5b6c23a4f50a50bd859d96d22679f2e3124783d4/content/renderer/render_thread_impl.cc
[modify] https://crrev.com/5b6c23a4f50a50bd859d96d22679f2e3124783d4/content/renderer/render_thread_impl.h
[modify] https://crrev.com/5b6c23a4f50a50bd859d96d22679f2e3124783d4/content/renderer/renderer_blink_platform_impl.cc
[modify] https://crrev.com/5b6c23a4f50a50bd859d96d22679f2e3124783d4/content/renderer/renderer_blink_platform_impl.h
[modify] https://crrev.com/5b6c23a4f50a50bd859d96d22679f2e3124783d4/third_party/WebKit/LayoutTests/VirtualTestSuites
[modify] https://crrev.com/5b6c23a4f50a50bd859d96d22679f2e3124783d4/third_party/blink/public/BUILD.gn
[delete] https://crrev.com/47084bae2a1755b4107436d9f7f01aea72557a31/third_party/blink/public/platform/modules/notifications/web_notification_manager.h
[modify] https://crrev.com/5b6c23a4f50a50bd859d96d22679f2e3124783d4/third_party/blink/public/platform/web_runtime_features.h
[modify] https://crrev.com/5b6c23a4f50a50bd859d96d22679f2e3124783d4/third_party/blink/renderer/modules/notifications/notification.cc
[modify] https://crrev.com/5b6c23a4f50a50bd859d96d22679f2e3124783d4/third_party/blink/renderer/modules/notifications/notification_manager.h
[modify] https://crrev.com/5b6c23a4f50a50bd859d96d22679f2e3124783d4/third_party/blink/renderer/modules/notifications/service_worker_registration_notifications.cc
[modify] https://crrev.com/5b6c23a4f50a50bd859d96d22679f2e3124783d4/third_party/blink/renderer/modules/notifications/service_worker_registration_notifications.h
[modify] https://crrev.com/5b6c23a4f50a50bd859d96d22679f2e3124783d4/third_party/blink/renderer/platform/exported/web_runtime_features.cc
[modify] https://crrev.com/5b6c23a4f50a50bd859d96d22679f2e3124783d4/third_party/blink/renderer/platform/runtime_enabled_features.json5

Comment 22 by awdf@chromium.org, May 10 2018

Status: Fixed (was: Assigned)

Sign in to add a comment