Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2703213004:
Migrate extension notifications to the new NotificationDisplayService (Closed)
|
Created:
3 years, 10 months ago by Peter Beverloo Modified:
3 years, 8 months ago CC:
chromium-reviews, Peter Beverloo, chromium-apps-reviews_chromium.org, mlamouri+watch-notifications_chromium.org, awdf+watch_chromium.org, extensions-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMigrate 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}
Committed: https://chromium.googlesource.com/chromium/src/+/28d8b7398617c37736dd2e04afd656795e6cac28
Patch Set 1 #Patch Set 2 : Checkpoint: Extensions code updated #Patch Set 3 : Finish functionality #
Total comments: 22
Patch Set 4 : Miguel's comments #
Total comments: 7
Patch Set 5 : incognito + rebase #
Total comments: 4
Patch Set 6 : full rebase #Patch Set 7 : another rebase #
Total comments: 22
Patch Set 8 : Migrate extension notifications to the new NotificationDisplayService #
Total comments: 2
Patch Set 9 : Migrate extension notifications to the new NotificationDisplayService #Messages
Total messages: 60 (35 generated)
The CQ bit was checked by peter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by peter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
miguelg@chromium.org changed reviewers: + miguelg@chromium.org
Thanks for starting on this! I have mostly questions more than comments at this stage but certainly going in the right direction! https://codereview.chromium.org/2703213004/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/notifications/extension_notification_display_helper.cc (right): https://codereview.chromium.org/2703213004/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/extension_notification_display_helper.cc:22: ExtensionNotificationDisplayHelper::~ExtensionNotificationDisplayHelper() {} Shouldn't this close all displayed notifications? https://codereview.chromium.org/2703213004/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/extension_notification_display_helper.cc:53: } Shouldn't we be duplicating the logic we have in context to sync with the display service? https://codereview.chromium.org/2703213004/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/notifications/extension_notification_display_helper.h (right): https://codereview.chromium.org/2703213004/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/extension_notification_display_helper.h:50: const NotificationVector& GetNotificationsForTests() const; It seems that all the tests in in the apitest could use GetNotificationIdsForExtension + GetByNotificationId which would be preferable to have this public method. https://codereview.chromium.org/2703213004/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/notifications/extension_notification_handler.cc (right): https://codereview.chromium.org/2703213004/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/extension_notification_handler.cc:17: // TODO(peter): Call EraseDataForNotificationId from here once this event Why would you not call it though? native NDSes will already be able to use it. https://codereview.chromium.org/2703213004/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/extension_notification_handler.cc:21: void ExtensionNotificationHandler::OnClick( So you are not propagating click/close events back? https://codereview.chromium.org/2703213004/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/notifications/notifications_api.cc (right): https://codereview.chromium.org/2703213004/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/notifications_api.cc:319: ExtensionNotificationDisplayHelperFactory::GetForProfile(profile)->Close( Wouldn't it be safer to post it as a task then? https://codereview.chromium.org/2703213004/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/notifications/notifications_api.h (right): https://codereview.chromium.org/2703213004/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/notifications_api.h:25: class NotificationsApiFunction : public ChromeAsyncExtensionFunction { Can you describe briefly (perhaps offline) the ownership model of this thing? https://codereview.chromium.org/2703213004/diff/40001/chrome/browser/notifica... File chrome/browser/notifications/native_notification_display_service.cc (right): https://codereview.chromium.org/2703213004/diff/40001/chrome/browser/notifica... chrome/browser/notifications/native_notification_display_service.cc:10: #include "chrome/browser/extensions/api/notifications/extension_notification_handler.h" yeah I can see how this include is not great... And even worse now that I see the trybots since we don't link to the api code on android. Shall we talk about it? https://codereview.chromium.org/2703213004/diff/40001/chrome/browser/notifica... File chrome/browser/notifications/notification_common.h (right): https://codereview.chromium.org/2703213004/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_common.h:16: // TODO(peter): Prefix these options with OPERATION_. FWIW we discussed this during the initial review and decided against it since it seemed an overkill. Happy to revisit the decision again though. https://codereview.chromium.org/2703213004/diff/40001/chrome/browser/notifica... File chrome/browser/notifications/platform_notification_service_unittest.cc (right): https://codereview.chromium.org/2703213004/diff/40001/chrome/browser/notifica... chrome/browser/notifications/platform_notification_service_unittest.cc:99: // TODO(peter): These tests should use the NotificationDisplayService, which I'd like to discuss this offline (and will not block the CL) I kind of like the idea of testing the whole PlatformNotificationService -> Bridge since (as you know) I consider the bridge and the display service very close together (they are not the same class because the latter needs to be a singleton while the former is keyed by profile).
PTAL https://codereview.chromium.org/2703213004/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/notifications/extension_notification_display_helper.cc (right): https://codereview.chromium.org/2703213004/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/extension_notification_display_helper.cc:22: ExtensionNotificationDisplayHelper::~ExtensionNotificationDisplayHelper() {} On 2017/02/21 10:56:43, Miguel Garcia wrote: > Shouldn't this close all displayed notifications? Moved to a Shutdown() function. (The destructor actually is too late.) https://codereview.chromium.org/2703213004/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/extension_notification_display_helper.cc:53: } On 2017/02/21 10:56:43, Miguel Garcia wrote: > Shouldn't we be duplicating the logic we have in context to sync with the > display service? No. There is no risk of left-over logic since the data doesn't persist beyond the lifetime of a profile. https://codereview.chromium.org/2703213004/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/notifications/extension_notification_display_helper.h (right): https://codereview.chromium.org/2703213004/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/extension_notification_display_helper.h:50: const NotificationVector& GetNotificationsForTests() const; On 2017/02/21 10:56:43, Miguel Garcia wrote: > It seems that all the tests in in the apitest could use > GetNotificationIdsForExtension + GetByNotificationId which would be preferable > to have this public method. Good idea! Done. https://codereview.chromium.org/2703213004/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/notifications/extension_notification_handler.cc (right): https://codereview.chromium.org/2703213004/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/extension_notification_handler.cc:17: // TODO(peter): Call EraseDataForNotificationId from here once this event On 2017/02/21 10:56:43, Miguel Garcia wrote: > Why would you not call it though? native NDSes will already be able to use it. Because we'd end up clearing the data multiple times for platforms that both enable extensions and use a native notification center. (I.e. Mac.) When we fix notification handlers to not be isolated to native notification centers, which is a bug in either case, we can rely on this callback as a single source of truth. (We may actually have a bug in this area with Web Notifications.) https://codereview.chromium.org/2703213004/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/extension_notification_handler.cc:21: void ExtensionNotificationHandler::OnClick( On 2017/02/21 10:56:43, Miguel Garcia wrote: > So you are not propagating click/close events back? No. Same answer - we can't rely on this functionality, while we can rely on the delegate's methods being called at the appropriate time. https://codereview.chromium.org/2703213004/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/notifications/notifications_api.cc (right): https://codereview.chromium.org/2703213004/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/notifications_api.cc:319: ExtensionNotificationDisplayHelperFactory::GetForProfile(profile)->Close( On 2017/02/21 10:56:43, Miguel Garcia wrote: > Wouldn't it be safer to post it as a task then? The issue there is that Shutdown() will be called when the profile is being shut down. ExtensionNotificationDisplayHelperFactory requires *the profile*, so we are unlikely to be able to access it when the task is ready to execute. However, if we move this logic to the shutdown method of the ExtensionNotificationDisplayHelper we avoid this problem entirely. Done. https://codereview.chromium.org/2703213004/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/notifications/notifications_api.h (right): https://codereview.chromium.org/2703213004/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/notifications_api.h:25: class NotificationsApiFunction : public ChromeAsyncExtensionFunction { On 2017/02/21 10:56:43, Miguel Garcia wrote: > Can you describe briefly (perhaps offline) the ownership model of this thing? The functions will be created on demand by the ExtensionFunctionDispatcher. There their instance remains reference counted until its execution has been completed. (Only Extension API functions implemented in JavaScript can return values, all the other ones take callback parameters so that their execution can be entirely asynchronous.) https://codereview.chromium.org/2703213004/diff/40001/chrome/browser/notifica... File chrome/browser/notifications/native_notification_display_service.cc (right): https://codereview.chromium.org/2703213004/diff/40001/chrome/browser/notifica... chrome/browser/notifications/native_notification_display_service.cc:10: #include "chrome/browser/extensions/api/notifications/extension_notification_handler.h" On 2017/02/21 10:56:44, Miguel Garcia wrote: > yeah I can see how this include is not great... > > And even worse now that I see the trybots since we don't link to the api code on > android. Shall we talk about it? Sure. Fixed for now. https://codereview.chromium.org/2703213004/diff/40001/chrome/browser/notifica... File chrome/browser/notifications/notification_common.h (right): https://codereview.chromium.org/2703213004/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_common.h:16: // TODO(peter): Prefix these options with OPERATION_. On 2017/02/21 10:56:44, Miguel Garcia wrote: > FWIW we discussed this during the initial review and decided against it since it > seemed an overkill. Happy to revisit the decision again though. We didn't use the prefix when we used `enum classes`, which mean callers used: NotificationCommon::Operation::SETTINGS Now that we switched back to regular enums as we use them as ints, callers now use: NotificationCommon::SETTINGS That conveys no information about what "settings" is. It's not a style guide violation, but I do think it's much clearer. WDYT? https://codereview.chromium.org/2703213004/diff/40001/chrome/browser/notifica... File chrome/browser/notifications/platform_notification_service_unittest.cc (right): https://codereview.chromium.org/2703213004/diff/40001/chrome/browser/notifica... chrome/browser/notifications/platform_notification_service_unittest.cc:99: // TODO(peter): These tests should use the NotificationDisplayService, which On 2017/02/21 10:56:44, Miguel Garcia wrote: > I'd like to discuss this offline (and will not block the CL) I kind of like the > idea of testing the whole PlatformNotificationService -> Bridge since (as you > know) I consider the bridge and the display service very close together (they > are not the same class because the latter needs to be a singleton while the > former is keyed by profile). Sure. In my mind, NotificationDisplayService will provide basic functionality (e.g. attaching handlers), with the {Native,MessageCenter}DisplayServices extending it for targeted functionality. The Stub implementation would use the common functionality too, so we don't actually lose any test coverage.
The CQ bit was checked by peter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm I have one pending question there but if you are sure this is ok, so am I :) https://codereview.chromium.org/2703213004/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/notifications/extension_notification_display_helper.cc (right): https://codereview.chromium.org/2703213004/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/extension_notification_display_helper.cc:53: } On 2017/02/21 17:23:50, Peter Beverloo wrote: > On 2017/02/21 10:56:43, Miguel Garcia wrote: > > Shouldn't we be duplicating the logic we have in context to sync with the > > display service? > > No. There is no risk of left-over logic since the data doesn't persist beyond > the lifetime of a profile. I'm still not convinced sorry, if you use extension notifications with mac native and close the notification from the notification center the notifications_ vector will still believe there is a notification around right?
peter@chromium.org changed reviewers: + dewittj@chromium.org
+dewittj, may I ask you to take a look? https://codereview.chromium.org/2703213004/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/notifications/extension_notification_display_helper.cc (right): https://codereview.chromium.org/2703213004/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/extension_notification_display_helper.cc:53: } On 2017/02/22 14:46:18, Miguel Garcia wrote: > On 2017/02/21 17:23:50, Peter Beverloo wrote: > > On 2017/02/21 10:56:43, Miguel Garcia wrote: > > > Shouldn't we be duplicating the logic we have in context to sync with the > > > display service? > > > > No. There is no risk of left-over logic since the data doesn't persist beyond > > the lifetime of a profile. > > I'm still not convinced sorry, if you use extension notifications with mac > native and close the notification from the notification center the > notifications_ vector will still believe there is a notification around right? > Ah, different form of notification sync. Yeah, this is something we should address. I would like to do that in a follow-up early next week since it'll deserve this class to get some unit tests if that's OK?
On 2017/02/28 01:08:59, Peter Beverloo (OOO - Mar 6th) wrote: > +dewittj, may I ask you to take a look? > > https://codereview.chromium.org/2703213004/diff/40001/chrome/browser/extensio... > File > chrome/browser/extensions/api/notifications/extension_notification_display_helper.cc > (right): > > https://codereview.chromium.org/2703213004/diff/40001/chrome/browser/extensio... > chrome/browser/extensions/api/notifications/extension_notification_display_helper.cc:53: > } > On 2017/02/22 14:46:18, Miguel Garcia wrote: > > On 2017/02/21 17:23:50, Peter Beverloo wrote: > > > On 2017/02/21 10:56:43, Miguel Garcia wrote: > > > > Shouldn't we be duplicating the logic we have in context to sync with the > > > > display service? > > > > > > No. There is no risk of left-over logic since the data doesn't persist > beyond > > > the lifetime of a profile. > > > > I'm still not convinced sorry, if you use extension notifications with mac > > native and close the notification from the notification center the > > notifications_ vector will still believe there is a notification around right? > > > > Ah, different form of notification sync. > > Yeah, this is something we should address. I would like to do that in a > follow-up early next week since it'll deserve this class to get some unit tests > if that's OK Can you please helpme
On 2017/03/01 10:34:30, swade0826 wrote: > On 2017/02/28 01:08:59, Peter Beverloo (OOO - Mar 6th) wrote: > > +dewittj, may I ask you to take a look? > > > > > https://codereview.chromium.org/2703213004/diff/40001/chrome/browser/extensio... > > File > > > chrome/browser/extensions/api/notifications/extension_notification_display_helper.cc > > (right): > > > > > https://codereview.chromium.org/2703213004/diff/40001/chrome/browser/extensio... > > > chrome/browser/extensions/api/notifications/extension_notification_display_helper.cc:53: > > } > > On 2017/02/22 14:46:18, Miguel Garcia wrote: > > > On 2017/02/21 17:23:50, Peter Beverloo wrote: > > > > On 2017/02/21 10:56:43, Miguel Garcia wrote: > > > > > Shouldn't we be duplicating the logic we have in context to sync with > the > > > > > display service? > > > > > > > > No. There is no risk of left-over logic since the data doesn't persist > > beyond > > > > the lifetime of a profile. > > > > > > I'm still not convinced sorry, if you use extension notifications with mac > > > native and close the notification from the notification center the > > > notifications_ vector will still believe there is a notification around > right? > > > > > > > Ah, different form of notification sync. > > > > Yeah, this is something we should address. I would like to do that in a > > follow-up early next week since it'll deserve this class to get some unit > tests > > if that's OK > Can you please helpme I'm trying to get mywifess secret data please. She is a bad person
On 2017/03/01 10:35:37, swade0826 wrote: > On 2017/03/01 10:34:30, swade0826 wrote: > > On 2017/02/28 01:08:59, Peter Beverloo (OOO - Mar 6th) wrote: > > > +dewittj, may I ask you to take a look? > > > > > > > > > https://codereview.chromium.org/2703213004/diff/40001/chrome/browser/extensio... > > > File > > > > > > chrome/browser/extensions/api/notifications/extension_notification_display_helper.cc > > > (right): > > > > > > > > > https://codereview.chromium.org/2703213004/diff/40001/chrome/browser/extensio... > > > > > > chrome/browser/extensions/api/notifications/extension_notification_display_helper.cc:53: > > > } > > > On 2017/02/22 14:46:18, Miguel Garcia wrote: > > > > On 2017/02/21 17:23:50, Peter Beverloo wrote: > > > > > On 2017/02/21 10:56:43, Miguel Garcia wrote: > > > > > > Shouldn't we be duplicating the logic we have in context to sync with > > the > > > > > > display service? > > > > > > > > > > No. There is no risk of left-over logic since the data doesn't persist > > > beyond > > > > > the lifetime of a profile. > > > > > > > > I'm still not convinced sorry, if you use extension notifications with mac > > > > native and close the notification from the notification center the > > > > notifications_ vector will still believe there is a notification around > > right? > > > > > > > > > > Ah, different form of notification sync. > > > > > > Yeah, this is something we should address. I would like to do that in a > > > follow-up early next week since it'll deserve this class to get some unit > > tests > > > if that's OK > > Can you please helpme > > I'm trying to get mywifess secret data please. She is a bad person
On 2017/03/01 10:35:37, swade0826 wrote: > On 2017/03/01 10:34:30, swade0826 wrote: > > On 2017/02/28 01:08:59, Peter Beverloo (OOO - Mar 6th) wrote: > > > +dewittj, may I ask you to take a look? > > > > > > > > > https://codereview.chromium.org/2703213004/diff/40001/chrome/browser/extensio... > > > File > > > > > > chrome/browser/extensions/api/notifications/extension_notification_display_helper.cc > > > (right): > > > > > > > > > https://codereview.chromium.org/2703213004/diff/40001/chrome/browser/extensio... > > > > > > chrome/browser/extensions/api/notifications/extension_notification_display_helper.cc:53: > > > } > > > On 2017/02/22 14:46:18, Miguel Garcia wrote: > > > > On 2017/02/21 17:23:50, Peter Beverloo wrote: > > > > > On 2017/02/21 10:56:43, Miguel Garcia wrote: > > > > > > Shouldn't we be duplicating the logic we have in context to sync with > > the > > > > > > display service? > > > > > > > > > > No. There is no risk of left-over logic since the data doesn't persist > > > beyond > > > > > the lifetime of a profile. > > > > > > > > I'm still not convinced sorry, if you use extension notifications with mac > > > > native and close the notification from the notification center the > > > > notifications_ vector will still believe there is a notification around > > right? > > > > > > > > > > Ah, different form of notification sync. > > > > > > Yeah, this is something we should address. I would like to do that in a > > > follow-up early next week since it'll deserve this class to get some unit > > tests > > > if that's OK > > Can you please helpme > > I'm trying to get mywifess secret data please. She is a bad person
> Ah, different form of notification sync. > > Yeah, this is something we should address. I would like to do that in a > follow-up early next week since it'll deserve this class to get some unit tests > if that's OK? Yeah no problem can you please add a TODO?
Any chance you have a doc for the proposed changes? Mostly looks fine barring a few comments. https://codereview.chromium.org/2703213004/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/notifications/extension_notification_display_helper.cc (right): https://codereview.chromium.org/2703213004/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/extension_notification_display_helper.cc:76: return false; This early return is concerning if the notifications_ data structure ever gets out of sync with the NDS. Perhaps it should be a DCHECK instead? https://codereview.chromium.org/2703213004/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/notifications/extension_notification_display_helper_factory.cc (right): https://codereview.chromium.org/2703213004/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/extension_notification_display_helper_factory.cc:23: GetInstance()->GetServiceForBrowserContext(profile, true /* create */)); this returns nullptr for incognito,is that what you want? https://codereview.chromium.org/2703213004/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/notifications/notifications_api.cc (right): https://codereview.chromium.org/2703213004/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/notifications_api.cc:235: DCHECK(profile_); rather than DCHECK(profile_) we should probably DCHECK the result of ExtensionNotificationDisplayHelperFactory::GetForProfile.
Thanks Justin! I missed your reply. The design document lives here (private): https://docs.google.com/document/d/1u5Uqz240WcMTB__D_tMEFsqMaLg2eaQmj_rNFKCsm..., which, admittedly, is rather light on the NDS and below details. The summary is that in the existing situation calls will still end up in the MessageCenterNotificationManager as they do today, but with native notification support enabled they'd end up in the NotificationPlatformBridge for the appropriate platform. (One pending change that I'll make tomorrow morning as I don't have my chromium account here.) https://codereview.chromium.org/2703213004/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/notifications/extension_notification_display_helper.cc (right): https://codereview.chromium.org/2703213004/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/extension_notification_display_helper.cc:76: return false; On 2017/03/01 17:59:08, dewittj wrote: > This early return is concerning if the notifications_ data structure ever gets > out of sync with the NDS. Perhaps it should be a DCHECK instead? We now have a mechanism to synchronise the shown notifications with the NDS, so Miguel will follow up and implement that. https://codereview.chromium.org/2703213004/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/notifications/extension_notification_display_helper_factory.cc (right): https://codereview.chromium.org/2703213004/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/extension_notification_display_helper_factory.cc:23: GetInstance()->GetServiceForBrowserContext(profile, true /* create */)); On 2017/03/01 17:59:08, dewittj wrote: > this returns nullptr for incognito,is that what you want? That's a fantastic point - no, that's not that I want! Thank you! I'll override GetBrowserContextToUse() w/ chrome::GetBrowserContextOwnInstanceInIncognito(). https://codereview.chromium.org/2703213004/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/notifications/notifications_api.cc (right): https://codereview.chromium.org/2703213004/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/notifications_api.cc:235: DCHECK(profile_); On 2017/03/01 17:59:08, dewittj wrote: > rather than DCHECK(profile_) we should probably DCHECK the result of > ExtensionNotificationDisplayHelperFactory::GetForProfile. GetForProfile() will always return a value per the aforementioned change. It's |profile_| that can be NULL here in the situation where Shutdown() was called prior to Close(), which means we'd be trying to dispatch an event while the profile is shutting down.
The CQ bit was checked by peter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by peter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
This should now be ready, PTAL Justin https://codereview.chromium.org/2703213004/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/notifications/extension_notification_display_helper_factory.cc (right): https://codereview.chromium.org/2703213004/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/extension_notification_display_helper_factory.cc:23: GetInstance()->GetServiceForBrowserContext(profile, true /* create */)); On 2017/04/05 23:22:12, Peter Beverloo wrote: > On 2017/03/01 17:59:08, dewittj wrote: > > this returns nullptr for incognito,is that what you want? > > That's a fantastic point - no, that's not that I want! Thank you! > > I'll override GetBrowserContextToUse() w/ > chrome::GetBrowserContextOwnInstanceInIncognito(). Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
peter@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
+devlin, PTAL
lgtm https://codereview.chromium.org/2703213004/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/api/notifications/notifications_apitest.cc (right): https://codereview.chromium.org/2703213004/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/notifications_apitest.cc:230: #if (defined(OS_LINUX) || defined(OS_WIN)) && !defined(NDEBUG) :( https://codereview.chromium.org/2703213004/diff/80001/chrome/browser/notifica... File chrome/browser/notifications/platform_notification_service_unittest.cc (right): https://codereview.chromium.org/2703213004/diff/80001/chrome/browser/notifica... chrome/browser/notifications/platform_notification_service_unittest.cc:185: mutable std::unique_ptr<std::set<std::string>> displayed_notifications_; nit: const/mutable dance here seems like overkill. Feel free to disregard this comment.
The CQ bit was checked by peter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2703213004/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/api/notifications/extension_notification_display_helper.cc (right): https://codereview.chromium.org/2703213004/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/notifications/extension_notification_display_helper.cc:38: for (const std::unique_ptr<Notification>& notification : notifications_) { optional: const auto& https://codereview.chromium.org/2703213004/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/api/notifications/extension_notification_display_helper.h (right): https://codereview.chromium.org/2703213004/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/notifications/extension_notification_display_helper.h:11: #include <unordered_map> needed? https://codereview.chromium.org/2703213004/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/notifications/extension_notification_display_helper.h:15: #include "chrome/browser/notifications/notification.h" needed? If so, remove the forward declaration. If not, move to the .cc. https://codereview.chromium.org/2703213004/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/api/notifications/extension_notification_display_helper_factory.cc (right): https://codereview.chromium.org/2703213004/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/notifications/extension_notification_display_helper_factory.cc:47: } \n https://codereview.chromium.org/2703213004/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/api/notifications/extension_notification_display_helper_factory.h (right): https://codereview.chromium.org/2703213004/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/notifications/extension_notification_display_helper_factory.h:5: #include "base/macros.h" these need to go in the ifndef https://codereview.chromium.org/2703213004/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/notifications/extension_notification_display_helper_factory.h:46: } // namespace extensions this needs to go above the endif https://codereview.chromium.org/2703213004/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/api/notifications/extension_notification_handler.cc (right): https://codereview.chromium.org/2703213004/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/notifications/extension_notification_handler.cc:15: NOTREACHED(); What is this class used for? https://codereview.chromium.org/2703213004/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/api/notifications/notifications_api.cc (right): https://codereview.chromium.org/2703213004/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/notifications/notifications_api.cc:327: Profile* profile_; Instead of profile_, maybe store ExtensionNotificationDisplayHelper*?
Thanks! I'll apply all changes in the morning - mind if I submit after I do, given that they're trivial? https://codereview.chromium.org/2703213004/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/api/notifications/extension_notification_display_helper.h (right): https://codereview.chromium.org/2703213004/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/notifications/extension_notification_display_helper.h:11: #include <unordered_map> On 2017/04/10 21:23:55, Devlin wrote: > needed? Will remove. https://codereview.chromium.org/2703213004/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/notifications/extension_notification_display_helper.h:15: #include "chrome/browser/notifications/notification.h" On 2017/04/10 21:23:55, Devlin wrote: > needed? If so, remove the forward declaration. If not, move to the .cc. Nope, I'm able to forward declare. https://codereview.chromium.org/2703213004/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/api/notifications/extension_notification_display_helper_factory.h (right): https://codereview.chromium.org/2703213004/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/notifications/extension_notification_display_helper_factory.h:5: #include "base/macros.h" On 2017/04/10 21:23:55, Devlin wrote: > these need to go in the ifndef ???? how did I do this? Thanks, will fix this file. https://codereview.chromium.org/2703213004/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/api/notifications/extension_notification_handler.cc (right): https://codereview.chromium.org/2703213004/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/notifications/extension_notification_handler.cc:15: NOTREACHED(); On 2017/04/10 21:23:55, Devlin wrote: > What is this class used for? Notifications still have delegates, but we'd like to get rid of those in the mid-term as they make a hard assumption that the browser process will outlive the notification. NonPersistentNotificationHandler (the parent class) provides the necessary forwarding behaviour.
https://codereview.chromium.org/2703213004/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/api/notifications/notifications_apitest.cc (right): https://codereview.chromium.org/2703213004/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/notifications_apitest.cc:230: #if (defined(OS_LINUX) || defined(OS_WIN)) && !defined(NDEBUG) On 2017/04/10 17:51:23, dewittj wrote: > :( Odd for just that one test to fail. I'll investigate -- updated the bug. https://codereview.chromium.org/2703213004/diff/80001/chrome/browser/notifica... File chrome/browser/notifications/platform_notification_service_unittest.cc (right): https://codereview.chromium.org/2703213004/diff/80001/chrome/browser/notifica... chrome/browser/notifications/platform_notification_service_unittest.cc:185: mutable std::unique_ptr<std::set<std::string>> displayed_notifications_; On 2017/04/10 17:51:23, dewittj wrote: > nit: const/mutable dance here seems like overkill. Feel free to disregard this > comment. No, you're right. Discussed this with Miguel today and we're going to get rid of it.
On 2017/04/10 23:30:06, Peter Beverloo wrote: > Thanks! I'll apply all changes in the morning - mind if I submit after I do, > given that they're trivial? I'd like to wrap up a few things offline first re the timeline for these.
https://codereview.chromium.org/2703213004/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/api/notifications/extension_notification_display_helper.h (right): https://codereview.chromium.org/2703213004/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/notifications/extension_notification_display_helper.h:29: class ExtensionNotificationDisplayHelper : public KeyedService { Forgot one comment - is there a reason this needs to be a KeyedService, rather than being owned by the NotificationsAPI?
The CQ bit was checked by peter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks! This CL makes possible as opposed to enables, so can land while we continue the offline discussion. https://codereview.chromium.org/2703213004/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/api/notifications/extension_notification_display_helper.cc (right): https://codereview.chromium.org/2703213004/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/notifications/extension_notification_display_helper.cc:38: for (const std::unique_ptr<Notification>& notification : notifications_) { On 2017/04/10 21:23:55, Devlin wrote: > optional: const auto& Done. I tend to prefer to include the type when it doesn't cause wrapping, but don't feel strongly. https://codereview.chromium.org/2703213004/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/api/notifications/extension_notification_display_helper.h (right): https://codereview.chromium.org/2703213004/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/notifications/extension_notification_display_helper.h:11: #include <unordered_map> On 2017/04/10 23:30:06, Peter Beverloo wrote: > On 2017/04/10 21:23:55, Devlin wrote: > > needed? > > Will remove. Done. https://codereview.chromium.org/2703213004/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/notifications/extension_notification_display_helper.h:15: #include "chrome/browser/notifications/notification.h" On 2017/04/10 23:30:06, Peter Beverloo wrote: > On 2017/04/10 21:23:55, Devlin wrote: > > needed? If so, remove the forward declaration. If not, move to the .cc. > > Nope, I'm able to forward declare. Done. https://codereview.chromium.org/2703213004/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/notifications/extension_notification_display_helper.h:29: class ExtensionNotificationDisplayHelper : public KeyedService { On 2017/04/11 01:51:56, Devlin wrote: > Forgot one comment - is there a reason this needs to be a KeyedService, rather > than being owned by the NotificationsAPI? It needs a holistic view of the notifications shown for extensions for a given profile and thus be accessible from any of the functions. There's no such common object in the /api/notifications/ code. https://codereview.chromium.org/2703213004/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/api/notifications/extension_notification_display_helper_factory.cc (right): https://codereview.chromium.org/2703213004/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/notifications/extension_notification_display_helper_factory.cc:47: } On 2017/04/10 21:23:55, Devlin wrote: > \n Done. https://codereview.chromium.org/2703213004/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/api/notifications/extension_notification_display_helper_factory.h (right): https://codereview.chromium.org/2703213004/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/notifications/extension_notification_display_helper_factory.h:5: #include "base/macros.h" On 2017/04/10 23:30:06, Peter Beverloo wrote: > On 2017/04/10 21:23:55, Devlin wrote: > > these need to go in the ifndef > > ???? how did I do this? Thanks, will fix this file. Done. https://codereview.chromium.org/2703213004/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/notifications/extension_notification_display_helper_factory.h:46: } // namespace extensions On 2017/04/10 21:23:55, Devlin wrote: > this needs to go above the endif Done. https://codereview.chromium.org/2703213004/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/api/notifications/notifications_api.cc (right): https://codereview.chromium.org/2703213004/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/notifications/notifications_api.cc:327: Profile* profile_; On 2017/04/10 21:23:55, Devlin wrote: > Instead of profile_, maybe store ExtensionNotificationDisplayHelper*? Done.
https://codereview.chromium.org/2703213004/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/api/notifications/extension_notification_display_helper.h (right): https://codereview.chromium.org/2703213004/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/notifications/extension_notification_display_helper.h:29: class ExtensionNotificationDisplayHelper : public KeyedService { On 2017/04/11 14:19:25, Peter Beverloo wrote: > On 2017/04/11 01:51:56, Devlin wrote: > > Forgot one comment - is there a reason this needs to be a KeyedService, rather > > than being owned by the NotificationsAPI? > > It needs a holistic view of the notifications shown for extensions for a given > profile and thus be accessible from any of the functions. There's no such common > object in the /api/notifications/ code. Whoops! Many APIs have a FooAPI object that's already a KeyedService, but it looks like Notifications isn't one of those. This is fine. https://codereview.chromium.org/2703213004/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/notifications/extension_notification_display_helper.h (right): https://codereview.chromium.org/2703213004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/notifications/extension_notification_display_helper.h:29: using NotificationVector = std::vector<std::unique_ptr<Notification>>; nit: can this be private?
On 2017/04/11 14:55:23, Devlin wrote: > https://codereview.chromium.org/2703213004/diff/120001/chrome/browser/extensi... > File > chrome/browser/extensions/api/notifications/extension_notification_display_helper.h > (right): > > https://codereview.chromium.org/2703213004/diff/120001/chrome/browser/extensi... > chrome/browser/extensions/api/notifications/extension_notification_display_helper.h:29: > class ExtensionNotificationDisplayHelper : public KeyedService { > On 2017/04/11 14:19:25, Peter Beverloo wrote: > > On 2017/04/11 01:51:56, Devlin wrote: > > > Forgot one comment - is there a reason this needs to be a KeyedService, > rather > > > than being owned by the NotificationsAPI? > > > > It needs a holistic view of the notifications shown for extensions for a given > > profile and thus be accessible from any of the functions. There's no such > common > > object in the /api/notifications/ code. > > Whoops! Many APIs have a FooAPI object that's already a KeyedService, but it > looks like Notifications isn't one of those. This is fine. > > https://codereview.chromium.org/2703213004/diff/140001/chrome/browser/extensi... > File > chrome/browser/extensions/api/notifications/extension_notification_display_helper.h > (right): > > https://codereview.chromium.org/2703213004/diff/140001/chrome/browser/extensi... > chrome/browser/extensions/api/notifications/extension_notification_display_helper.h:29: > using NotificationVector = std::vector<std::unique_ptr<Notification>>; > nit: can this be private? and lgtm (rietveld + spotty internet == fail)
Thank you! https://codereview.chromium.org/2703213004/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/notifications/extension_notification_display_helper.h (right): https://codereview.chromium.org/2703213004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/notifications/extension_notification_display_helper.h:29: using NotificationVector = std::vector<std::unique_ptr<Notification>>; On 2017/04/11 14:55:23, Devlin wrote: > nit: can this be private? Yes. Done.
The CQ bit was checked by peter@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from miguelg@chromium.org, dewittj@chromium.org, rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2703213004/#ps160001 (title: "Migrate extension notifications to the new NotificationDisplayService")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1491923022534050,
"parent_rev": "c8c8da726195de21346b4436f2a1bb6711911f4a", "commit_rev":
"28d8b7398617c37736dd2e04afd656795e6cac28"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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} Committed: https://chromium.googlesource.com/chromium/src/+/28d8b7398617c37736dd2e04afd6... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/28d8b7398617c37736dd2e04afd6... |
