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

Issue 796990 link

Starred by 2 users

Issue metadata

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

Blocking:
issue 595685



Sign in to add a comment

Migrate non-persistent web notifications to mojo

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

Issue description

Project Member

Comment 1 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 2 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 3 by bugdroid1@chromium.org, Jan 25 2018

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

commit f5ae28eea17a45f895a581ecb887aba6f2e43b1d
Author: Anita Woodruff <awdf@chromium.org>
Date: Thu Jan 25 18:22:38 2018

[Mojoification] Non-persistent notification events

- All non-persistent notification events (onshow, onclick, and
onclose) now go through Mojo when the NotificationsWithMojo
blink feature flag is enabled.

- Layout tests with mojo enabled that rely on
 non-persistent notification events now pass and are re-enabled.
(Except for those relying on the Notification.close() path which
still needs to be migrated to mojo).

- Interactive ui tests with mojo enabled are simplified to wait
 for the show event rather than NotificationDisplayService
 mutation.

BUG= 796990 

Change-Id: Id99cd7b936b2ef18fd4be270aa76e4732e137db9
Reviewed-on: https://chromium-review.googlesource.com/878744
Reviewed-by: Peter Beverloo <peter@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Commit-Queue: Anita Woodruff <awdf@chromium.org>
Cr-Commit-Position: refs/heads/master@{#531953}
[modify] https://crrev.com/f5ae28eea17a45f895a581ecb887aba6f2e43b1d/chrome/browser/notifications/platform_notification_service_interactive_uitest.cc
[modify] https://crrev.com/f5ae28eea17a45f895a581ecb887aba6f2e43b1d/chrome/test/data/notifications/platform_notification_service.html
[modify] https://crrev.com/f5ae28eea17a45f895a581ecb887aba6f2e43b1d/content/browser/notifications/blink_notification_service_impl.cc
[modify] https://crrev.com/f5ae28eea17a45f895a581ecb887aba6f2e43b1d/content/browser/notifications/blink_notification_service_impl.h
[modify] https://crrev.com/f5ae28eea17a45f895a581ecb887aba6f2e43b1d/content/browser/notifications/notification_event_dispatcher_impl.cc
[modify] https://crrev.com/f5ae28eea17a45f895a581ecb887aba6f2e43b1d/content/browser/notifications/notification_event_dispatcher_impl.h
[add] https://crrev.com/f5ae28eea17a45f895a581ecb887aba6f2e43b1d/content/browser/notifications/notification_event_dispatcher_impl_unittest.cc
[modify] https://crrev.com/f5ae28eea17a45f895a581ecb887aba6f2e43b1d/content/test/BUILD.gn
[modify] https://crrev.com/f5ae28eea17a45f895a581ecb887aba6f2e43b1d/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/f5ae28eea17a45f895a581ecb887aba6f2e43b1d/third_party/WebKit/Source/modules/notifications/DEPS
[modify] https://crrev.com/f5ae28eea17a45f895a581ecb887aba6f2e43b1d/third_party/WebKit/Source/modules/notifications/Notification.cpp
[modify] https://crrev.com/f5ae28eea17a45f895a581ecb887aba6f2e43b1d/third_party/WebKit/Source/modules/notifications/Notification.h
[modify] https://crrev.com/f5ae28eea17a45f895a581ecb887aba6f2e43b1d/third_party/WebKit/Source/modules/notifications/NotificationManager.cpp
[modify] https://crrev.com/f5ae28eea17a45f895a581ecb887aba6f2e43b1d/third_party/WebKit/Source/modules/notifications/NotificationManager.h
[modify] https://crrev.com/f5ae28eea17a45f895a581ecb887aba6f2e43b1d/third_party/WebKit/public/platform/modules/notifications/notification_service.mojom

Project Member

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

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

commit 34eb0318e337790e49f0fa4f920308746668555c
Author: Anita Woodruff <awdf@chromium.org>
Date: Wed Feb 07 15:48:43 2018

[Mojoification] Migrate Notification.close() to Mojo

- The CloseNonPersistentNotification call is now routed through mojo
from Notification.cpp to blink_notification_service_impl.cc, so that
the Notification.close() API now works when the NotificationsWithMojo
flag is enabled.

- We avoid passing the Notification ID back to the renderer process to
ensure it is always recomputed using the trusted origin in the browser
process as a security measure (so origins can't spoof each other's
notification IDs).

- This completes mojoification of all non-persistent notification code.
Note, the mojoification code is still behind a NotificationsWithMojo
runtime flag; a further patch will follow removing the legacy IPC
codepath and flag.

Bug:  796990 
Change-Id: I29a7f621b9074369c2470e59408eb01672caacb9
Reviewed-on: https://chromium-review.googlesource.com/893188
Commit-Queue: Anita Woodruff <awdf@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#535010}
[modify] https://crrev.com/34eb0318e337790e49f0fa4f920308746668555c/chrome/browser/notifications/platform_notification_service_interactive_uitest.cc
[modify] https://crrev.com/34eb0318e337790e49f0fa4f920308746668555c/content/browser/notifications/blink_notification_service_impl.cc
[modify] https://crrev.com/34eb0318e337790e49f0fa4f920308746668555c/content/browser/notifications/blink_notification_service_impl.h
[modify] https://crrev.com/34eb0318e337790e49f0fa4f920308746668555c/content/browser/notifications/notification_id_generator.cc
[modify] https://crrev.com/34eb0318e337790e49f0fa4f920308746668555c/content/browser/notifications/notification_id_generator.h
[modify] https://crrev.com/34eb0318e337790e49f0fa4f920308746668555c/content/browser/notifications/notification_id_generator_unittest.cc
[modify] https://crrev.com/34eb0318e337790e49f0fa4f920308746668555c/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/34eb0318e337790e49f0fa4f920308746668555c/third_party/WebKit/Source/modules/notifications/DEPS
[modify] https://crrev.com/34eb0318e337790e49f0fa4f920308746668555c/third_party/WebKit/Source/modules/notifications/Notification.cpp
[modify] https://crrev.com/34eb0318e337790e49f0fa4f920308746668555c/third_party/WebKit/Source/modules/notifications/Notification.h
[modify] https://crrev.com/34eb0318e337790e49f0fa4f920308746668555c/third_party/WebKit/Source/modules/notifications/NotificationManager.cpp
[modify] https://crrev.com/34eb0318e337790e49f0fa4f920308746668555c/third_party/WebKit/Source/modules/notifications/NotificationManager.h
[modify] https://crrev.com/34eb0318e337790e49f0fa4f920308746668555c/third_party/WebKit/public/platform/modules/notifications/notification_service.mojom

Project Member

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

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

commit 608feabbe2f35c159a1dd8178774bc68da209d9f
Author: Anita Woodruff <awdf@chromium.org>
Date: Wed Feb 07 17:33:37 2018

[Notifications] Check permission when displaying via mojo

- This behaviour now matches the permission checks in the
NotificationMessageFilter (original codepath), paving the way to enable
the mojo pathway for non-persistent notifications by default.

R=peter@chromium.org

Bug:  796990 
Change-Id: I624299944c702a2c8b18236f55db5504d7e97aa1
Reviewed-on: https://chromium-review.googlesource.com/906710
Commit-Queue: Anita Woodruff <awdf@chromium.org>
Commit-Queue: Peter Beverloo <peter@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#535041}
[modify] https://crrev.com/608feabbe2f35c159a1dd8178774bc68da209d9f/content/browser/notifications/blink_notification_service_impl.cc
[modify] https://crrev.com/608feabbe2f35c159a1dd8178774bc68da209d9f/content/browser/notifications/blink_notification_service_impl.h

Project Member

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

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

commit cc6b5355b62502d87267583f9441494e9dc1b821
Author: Anita Woodruff <awdf@chromium.org>
Date: Wed Feb 07 20:03:44 2018

[Notifications] Update state_ when closing via mojo

 - We ought to be doing the same thing as the legacy codepath here.

R=peter@chromium.org

Bug:  796990 
Change-Id: I6524b51560e9d5e6b212996ee4c7676dd20353ec
Reviewed-on: https://chromium-review.googlesource.com/907268
Commit-Queue: Anita Woodruff <awdf@chromium.org>
Commit-Queue: Peter Beverloo <peter@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#535114}
[modify] https://crrev.com/cc6b5355b62502d87267583f9441494e9dc1b821/third_party/WebKit/Source/modules/notifications/Notification.cpp

Project Member

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

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

commit ef76ee3e68ec5171506894cb52a565356ee221ec
Author: Anita Woodruff <awdf@chromium.org>
Date: Mon Feb 12 14:19:41 2018

[Notifications] Non-persistent notifications now use Mojo path

- Switched non-persistent notifications over to the mojo path and
removed the legacy IPC message handlers and surrounding code.

R=peter@chromium.org

Bug:  796990 
Change-Id: I49baccc562ffacb5f76f4cc3067d60824b269daf
Reviewed-on: https://chromium-review.googlesource.com/908448
Reviewed-by: Emily Stark <estark@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Commit-Queue: Anita Woodruff <awdf@chromium.org>
Cr-Commit-Position: refs/heads/master@{#536071}
[modify] https://crrev.com/ef76ee3e68ec5171506894cb52a565356ee221ec/chrome/browser/notifications/platform_notification_service_interactive_uitest.cc
[modify] https://crrev.com/ef76ee3e68ec5171506894cb52a565356ee221ec/content/browser/notifications/blink_notification_service_impl.cc
[modify] https://crrev.com/ef76ee3e68ec5171506894cb52a565356ee221ec/content/browser/notifications/notification_event_dispatcher_impl.cc
[modify] https://crrev.com/ef76ee3e68ec5171506894cb52a565356ee221ec/content/browser/notifications/notification_event_dispatcher_impl.h
[modify] https://crrev.com/ef76ee3e68ec5171506894cb52a565356ee221ec/content/browser/notifications/notification_id_generator.cc
[modify] https://crrev.com/ef76ee3e68ec5171506894cb52a565356ee221ec/content/browser/notifications/notification_id_generator.h
[modify] https://crrev.com/ef76ee3e68ec5171506894cb52a565356ee221ec/content/browser/notifications/notification_id_generator_unittest.cc
[modify] https://crrev.com/ef76ee3e68ec5171506894cb52a565356ee221ec/content/browser/notifications/notification_message_filter.cc
[modify] https://crrev.com/ef76ee3e68ec5171506894cb52a565356ee221ec/content/browser/notifications/notification_message_filter.h
[modify] https://crrev.com/ef76ee3e68ec5171506894cb52a565356ee221ec/content/common/platform_notification_messages.h
[modify] https://crrev.com/ef76ee3e68ec5171506894cb52a565356ee221ec/content/renderer/notifications/notification_manager.cc
[modify] https://crrev.com/ef76ee3e68ec5171506894cb52a565356ee221ec/content/renderer/notifications/notification_manager.h
[modify] https://crrev.com/ef76ee3e68ec5171506894cb52a565356ee221ec/third_party/WebKit/Source/modules/notifications/Notification.cpp
[modify] https://crrev.com/ef76ee3e68ec5171506894cb52a565356ee221ec/third_party/WebKit/Source/modules/notifications/Notification.h
[modify] https://crrev.com/ef76ee3e68ec5171506894cb52a565356ee221ec/third_party/WebKit/public/BUILD.gn
[delete] https://crrev.com/8da4113eb6c89c825f8a4575c60fc6ceb91118a9/third_party/WebKit/public/platform/modules/notifications/WebNotificationDelegate.h
[modify] https://crrev.com/ef76ee3e68ec5171506894cb52a565356ee221ec/third_party/WebKit/public/platform/modules/notifications/WebNotificationManager.h

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

Comment 9 by awdf@chromium.org, Mar 2 2018

Status: Fixed (was: Started)

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

As of M66 all non-persistent notifications use the mojo pathway.

Sign in to add a comment