Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2875673002:
Minimize the delegate dependencies for native extension notifications. (Closed)
|
Created:
3 years, 7 months ago by Miguel Garcia Modified:
3 years, 7 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. |
DescriptionMinimize the delegate dependencies for native extension notifications.
Support for events like click, close etc is now fully handled by the
ExtensionNotificationHandler like persistent web notifications.
The delegate is now just a little shim to hold the delegate id since it
is still required by the Notification class.
BUG=720345
Review-Url: https://codereview.chromium.org/2875673002
Cr-Commit-Position: refs/heads/master@{#473427}
Committed: https://chromium.googlesource.com/chromium/src/+/dcc699ff151393a7d2fa29314befd57d194b0f60
Patch Set 1 #Patch Set 2 : fix compile on chromeos #Patch Set 3 : fix a memory management issue #Patch Set 4 : disable non applicable tests #
Total comments: 14
Patch Set 5 : review #
Total comments: 14
Patch Set 6 : review #
Messages
Total messages: 33 (26 generated)
The CQ bit was checked by miguelg@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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by miguelg@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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
miguelg@chromium.org changed reviewers: + peter@chromium.org
I think I'll need to disable the failing tests that depend on full screen since that's a feature we already don't support anymore (whether to show a notification in full screen or not is decided by the OS now).
The CQ bit was checked by miguelg@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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
lgtm % clarifications https://codereview.chromium.org/2875673002/diff/50001/chrome/browser/extensio... File chrome/browser/extensions/api/notifications/extension_notification_handler.cc (right): https://codereview.chromium.org/2875673002/diff/50001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/extension_notification_handler.cc:39: } // namespace nits: (1) blank line after :38 (2) place the anonymous namespace *inside* of the extensions one (thanks for adding it!) https://codereview.chromium.org/2875673002/diff/50001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/extension_notification_handler.cc:99: NotificationDelegate* delegate) {} Is there an opportunity for merging the handler with the helper? https://codereview.chromium.org/2875673002/diff/50001/chrome/browser/extensio... File chrome/browser/extensions/api/notifications/extension_notification_handler.h (left): https://codereview.chromium.org/2875673002/diff/50001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/extension_notification_handler.h:18: // NotificationHandler implementation. Please keep this. Also remove the blank lines on :28, :34, :36 since the comment applies to all these methods. (This is common throughout Chrome.) https://codereview.chromium.org/2875673002/diff/50001/chrome/browser/extensio... File chrome/browser/extensions/api/notifications/extension_notification_handler.h (right): https://codereview.chromium.org/2875673002/diff/50001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/extension_notification_handler.h:11: #include "extensions/browser/event_router.h" nit: no blank line https://codereview.chromium.org/2875673002/diff/50001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/extension_notification_handler.h:52: #endif // CHROME_BROWSER_EXTENSIONS_API_NOTIFICATIONS_EXTENSION_NOTIFICATION_HANDLER_H_ nit: blank line after 51 https://codereview.chromium.org/2875673002/diff/50001/chrome/browser/extensio... File chrome/browser/extensions/api/notifications/notifications_api.cc (right): https://codereview.chromium.org/2875673002/diff/50001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/notifications_api.cc:519: #if BUILDFLAG(ENABLE_NATIVE_NOTIFICATIONS) Here and above: These #if-defs are ugly. Please be more complete about why they are necessary, for how long (you estimate that) they will be necessary and link to a bug about getting rid of them. https://codereview.chromium.org/2875673002/diff/50001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/notifications_api.cc:524: new NotificationApiDelegate(this, GetProfile(), extension_->id(), id); if () { } else { }
Description was changed from ========== Minimize the delegate dependencies for native extension notifications. Support for events like click, close etc is now fully handled by the ExtensionNotificationHandler like persistent web notifications. The delegate is now just a little shim to hold the delegate id since it is still required by the Notification class. BUG=717725 ========== to ========== Minimize the delegate dependencies for native extension notifications. Support for events like click, close etc is now fully handled by the ExtensionNotificationHandler like persistent web notifications. The delegate is now just a little shim to hold the delegate id since it is still required by the Notification class. BUG=720345 ==========
The CQ bit was checked by miguelg@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...
miguelg@chromium.org changed reviewers: + dewittj@chromium.org
Thanks! +dewittj@chromium.org for OWNERS. dewittj we are trying to unify both the native and the message center flows so that there are fewer differences. The bug outlines the first step in the process. https://codereview.chromium.org/2875673002/diff/50001/chrome/browser/extensio... File chrome/browser/extensions/api/notifications/extension_notification_handler.cc (right): https://codereview.chromium.org/2875673002/diff/50001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/extension_notification_handler.cc:39: } // namespace On 2017/05/15 15:56:52, Peter Beverloo wrote: > nits: > (1) blank line after :38 > (2) place the anonymous namespace *inside* of the extensions one (thanks for > adding it!) Done. https://codereview.chromium.org/2875673002/diff/50001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/extension_notification_handler.cc:99: NotificationDelegate* delegate) {} On 2017/05/15 15:56:52, Peter Beverloo wrote: > Is there an opportunity for merging the handler with the helper? Eventually maybe, right now the handler is also used to display notifications and keep state which I rather not add here for now. https://codereview.chromium.org/2875673002/diff/50001/chrome/browser/extensio... File chrome/browser/extensions/api/notifications/extension_notification_handler.h (left): https://codereview.chromium.org/2875673002/diff/50001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/extension_notification_handler.h:18: // NotificationHandler implementation. On 2017/05/15 15:56:52, Peter Beverloo wrote: > Please keep this. Also remove the blank lines on :28, :34, :36 since the comment > applies to all these methods. (This is common throughout Chrome.) Done. https://codereview.chromium.org/2875673002/diff/50001/chrome/browser/extensio... File chrome/browser/extensions/api/notifications/extension_notification_handler.h (right): https://codereview.chromium.org/2875673002/diff/50001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/extension_notification_handler.h:11: #include "extensions/browser/event_router.h" On 2017/05/15 15:56:52, Peter Beverloo wrote: > nit: no blank line Done. https://codereview.chromium.org/2875673002/diff/50001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/extension_notification_handler.h:52: #endif // CHROME_BROWSER_EXTENSIONS_API_NOTIFICATIONS_EXTENSION_NOTIFICATION_HANDLER_H_ On 2017/05/15 15:56:52, Peter Beverloo wrote: > nit: blank line after 51 Done. https://codereview.chromium.org/2875673002/diff/50001/chrome/browser/extensio... File chrome/browser/extensions/api/notifications/notifications_api.cc (right): https://codereview.chromium.org/2875673002/diff/50001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/notifications_api.cc:519: #if BUILDFLAG(ENABLE_NATIVE_NOTIFICATIONS) On 2017/05/15 15:56:52, Peter Beverloo wrote: > Here and above: These #if-defs are ugly. Please be more complete about why they > are necessary, for how long (you estimate that) they will be necessary and link > to a bug about getting rid of them. Good point I provided a high level explanation in https://bugs.chromium.org/p/chromium/issues/detail?id=720345 and linked to it from the code (not here but in #210 where the delegates are defined) https://codereview.chromium.org/2875673002/diff/50001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/notifications_api.cc:524: new NotificationApiDelegate(this, GetProfile(), extension_->id(), id); On 2017/05/15 15:56:52, Peter Beverloo wrote: > if () { > } else { > } Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
in general, this lgtm https://codereview.chromium.org/2875673002/diff/70001/chrome/browser/extensio... File chrome/browser/extensions/api/notifications/extension_notification_handler.cc (right): https://codereview.chromium.org/2875673002/diff/70001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/extension_notification_handler.cc:48: void ExtensionNotificationHandler::OnClose(Profile* profile, Is it possible to unit test this? https://codereview.chromium.org/2875673002/diff/70001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/extension_notification_handler.cc:55: std::string extension_id(GetExtensionId(origin)); need to check for empty string? https://codereview.chromium.org/2875673002/diff/70001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/extension_notification_handler.cc:80: if (action_index > -1) (Not a required change) It seems like if you already need an if for this, perhaps it's better written as if (action_index > -1) { args->AppendInteger(action_index); SendEvent(profile, extension_id, events::NOTIFICATIONS_ON_BUTTON_CLICKED, api::notifications::OnButtonClicked::kEventName, ...); } else { SendEvent(...); } https://codereview.chromium.org/2875673002/diff/70001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/extension_notification_handler.cc:85: const std::string& event = nit: event_name https://codereview.chromium.org/2875673002/diff/70001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/extension_notification_handler.cc:106: const std::string& name, nit: event_name https://codereview.chromium.org/2875673002/diff/70001/chrome/browser/extensio... File chrome/browser/extensions/api/notifications/notifications_apitest.cc (right): https://codereview.chromium.org/2875673002/diff/70001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/notifications_apitest.cc:247: #if !defined(OS_MACOSX) should this be BUILDFLAG rather than OS? https://codereview.chromium.org/2875673002/diff/70001/chrome/browser/notifica... File chrome/browser/notifications/native_notification_display_service.cc (right): https://codereview.chromium.org/2875673002/diff/70001/chrome/browser/notifica... chrome/browser/notifications/native_notification_display_service.cc:65: base::MakeUnique<extensions::ExtensionNotificationHandler>()); Depending on where else this type is referenced, it might make sense to rename to just extensions::NotificationHandler (or similar) which is shorter.
The CQ bit was checked by miguelg@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...
https://codereview.chromium.org/2875673002/diff/70001/chrome/browser/extensio... File chrome/browser/extensions/api/notifications/extension_notification_handler.cc (right): https://codereview.chromium.org/2875673002/diff/70001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/extension_notification_handler.cc:48: void ExtensionNotificationHandler::OnClose(Profile* profile, On 2017/05/16 22:07:17, dewittj wrote: > Is it possible to unit test this? Yes! Done now https://codereview.chromium.org/2875673002/diff/70001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/extension_notification_handler.cc:55: std::string extension_id(GetExtensionId(origin)); On 2017/05/16 22:07:17, dewittj wrote: > need to check for empty string? I have added a DCHECK since it should not be possible at this stage for the id to be empty. https://codereview.chromium.org/2875673002/diff/70001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/extension_notification_handler.cc:80: if (action_index > -1) On 2017/05/16 22:07:17, dewittj wrote: > (Not a required change) It seems like if you already need an if for this, > perhaps it's better written as > > if (action_index > -1) { > args->AppendInteger(action_index); > SendEvent(profile, extension_id, events::NOTIFICATIONS_ON_BUTTON_CLICKED, > api::notifications::OnButtonClicked::kEventName, ...); > } else { > SendEvent(...); > } Acknowledged. https://codereview.chromium.org/2875673002/diff/70001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/extension_notification_handler.cc:85: const std::string& event = On 2017/05/16 22:07:17, dewittj wrote: > nit: event_name Done. https://codereview.chromium.org/2875673002/diff/70001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/extension_notification_handler.cc:106: const std::string& name, On 2017/05/16 22:07:17, dewittj wrote: > nit: event_name Done. https://codereview.chromium.org/2875673002/diff/70001/chrome/browser/extensio... File chrome/browser/extensions/api/notifications/notifications_apitest.cc (right): https://codereview.chromium.org/2875673002/diff/70001/chrome/browser/extensio... chrome/browser/extensions/api/notifications/notifications_apitest.cc:247: #if !defined(OS_MACOSX) On 2017/05/16 22:07:17, dewittj wrote: > should this be BUILDFLAG rather than OS? We have BUILDFLAG(ENABLE_NATIVE_NOTIFICATIONS) but both linux and windows support the flag but are nowhere near shipping native by default. Therefore for those platforms it's probably better to keep running the tests. https://codereview.chromium.org/2875673002/diff/70001/chrome/browser/notifica... File chrome/browser/notifications/native_notification_display_service.cc (right): https://codereview.chromium.org/2875673002/diff/70001/chrome/browser/notifica... chrome/browser/notifications/native_notification_display_service.cc:65: base::MakeUnique<extensions::ExtensionNotificationHandler>()); On 2017/05/16 22:07:17, dewittj wrote: > Depending on where else this type is referenced, it might make sense to rename > to just extensions::NotificationHandler (or similar) which is shorter. I've noticed this pattern with the notification delegate but I honestly have found it harder to follow than giving a unique name to each subclass instead of relying only on namespaces
The CQ bit was unchecked by miguelg@chromium.org
The CQ bit was checked by miguelg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org, dewittj@chromium.org Link to the patchset: https://codereview.chromium.org/2875673002/#ps90001 (title: "review")
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": 90001, "attempt_start_ts": 1495263872734620,
"parent_rev": "a8acbe5b8351591685b0353437651d8bdc2d42df", "commit_rev":
"dcc699ff151393a7d2fa29314befd57d194b0f60"}
Message was sent while issue was closed.
Description was changed from ========== Minimize the delegate dependencies for native extension notifications. Support for events like click, close etc is now fully handled by the ExtensionNotificationHandler like persistent web notifications. The delegate is now just a little shim to hold the delegate id since it is still required by the Notification class. BUG=720345 ========== to ========== Minimize the delegate dependencies for native extension notifications. Support for events like click, close etc is now fully handled by the ExtensionNotificationHandler like persistent web notifications. The delegate is now just a little shim to hold the delegate id since it is still required by the Notification class. BUG=720345 Review-Url: https://codereview.chromium.org/2875673002 Cr-Commit-Position: refs/heads/master@{#473427} Committed: https://chromium.googlesource.com/chromium/src/+/dcc699ff151393a7d2fa29314bef... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:90001) as https://chromium.googlesource.com/chromium/src/+/dcc699ff151393a7d2fa29314bef... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
