Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1509923002:
Implement native web notifications for mac (Closed)
|
Created:
5 years ago by Miguel Garcia Modified:
4 years, 11 months ago CC:
chromium-reviews, Peter Beverloo, mlamouri+watch-notifications_chromium.org, asvitkine+watch_chromium.org, vmpstr+watch_chromium.org, dewittj Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement native web notifications for mac
Implementation behind the enable-native-notifications chrome://flags
Conceptually based on a previous attempt.
(https://codereview.chromium.org/224183002) but now using the same
interface android uses.
System and extensions notifications remain implemented through chrome
toasts.
There are a few things missing that will come in future CLs, most notably
Buttons
Auto dimsissal (i.e all notifications requireInteraction)
Sounds
BUG=571056
Committed: https://crrev.com/3bf899c9d98f84f3e7a297e04c012867cf0ddc11
Cr-Commit-Position: refs/heads/master@{#369255}
Patch Set 1 : #Patch Set 2 : #
Total comments: 39
Patch Set 3 : #
Total comments: 1
Patch Set 4 : rebase #
Total comments: 16
Patch Set 5 : #Patch Set 6 : #
Total comments: 4
Patch Set 7 : rebase #
Total comments: 15
Patch Set 8 : #Patch Set 9 : #Patch Set 10 : #Messages
Total messages: 38 (11 generated)
miguelg@chromium.org changed reviewers: + peter@chromium.org
Patchset #1 (id:1) has been deleted
This is incredibly exciting to see, thank you for the patch! First of all, I'm very (positively) surprised about the size of it. While there are a lot of TODOs left, it already provides basic functionality. There main higher level topic I'd like to touch on is that usage of the NotificationUIManager seems inappropriate. Like Android, Mac will not be able to provide all functionality defined by the interface. More importantly, the implementations differ in their ability to outlive the browser process itself. What if we were to recognize this difference accordingly? Drop the dependency on the //ui/message_center/ code and implement the new code in a component that provides access to the native notification centers per a new interface? Let me start a thread about this. Orthogonally, I've left a number of smaller questions and comments as well. https://codereview.chromium.org/1509923002/diff/40001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1509923002/diff/40001/chrome/app/generated_re... chrome/app/generated_resources.grd:8086: + Enable support for using the native notification toasts and notification center in available platforms. s/in available platforms/on platforms where these are available/ https://codereview.chromium.org/1509923002/diff/40001/chrome/browser/notifica... File chrome/browser/notifications/notification_test_util.cc (right): https://codereview.chromium.org/1509923002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_test_util.cc:141: ; Don't empty statements cause compile errors? https://codereview.chromium.org/1509923002/diff/40001/chrome/browser/notifica... File chrome/browser/notifications/notification_ui_manager_mac.h (right): https://codereview.chromium.org/1509923002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_ui_manager_mac.h:22: // send platform notifications to the Notification Center on 10.8 and above. This would be an excellent place to document *why* it's only available on 10.8+. https://codereview.chromium.org/1509923002/diff/40001/chrome/browser/notifica... File chrome/browser/notifications/notification_ui_manager_mac.mm (right): https://codereview.chromium.org/1509923002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_ui_manager_mac.mm:31: // - Sound names can be implemented by setting soundName in NSUserNotification What would we use sounds for? Getting attention? Can we determine the system default? https://codereview.chromium.org/1509923002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_ui_manager_mac.mm:32: // - notifiation.requireInteraction can be implemented by removing the nit: s/notifiation/notification/ https://codereview.chromium.org/1509923002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_ui_manager_mac.mm:60: @interface NotificationCenterDelegate What happens when a Chrome notification is shown in the notification center, but the process itself isn't running? (I know that we work around this by calling CancelAll() in the destructor, but ideally we'd like this to outlive the browser of course.) https://codereview.chromium.org/1509923002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_ui_manager_mac.mm:96: notification.context_message().empty() nit: This class is only functional for persistent notifications, so context_message() will never be set. Perhaps change the comment on lines 82-84 to a TODO? https://codereview.chromium.org/1509923002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_ui_manager_mac.mm:104: // Icon nit: did you accidentally a https://codereview.chromium.org/1509923002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_ui_manager_mac.mm:248: ProfileManager::GetLastUsedProfile(), This is not a safe TODO on Mac, given that we can have multiple profiles there (as opposed to Android). https://codereview.chromium.org/1509923002/diff/40001/chrome/common/chrome_sw... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/1509923002/diff/40001/chrome/common/chrome_sw... chrome/common/chrome_switches.cc:441: // using the chrome based ones. nit: weird line breaks, and apply some grammar
Description was changed from ========== Implement native web notifications for mac Implementation behind the enable-native-notifications chrome://flags Conceptually based on a previous attempt. (https://codereview.chromium.org/224183002) but now using the same interface android uses. System and extensions notifications remain implemented through chrome toasts. There a few things missing that will come in future CLs, most notably Buttons Auto dimsissal (i.e all notifications requireInteraction) Sounds BUG= ========== to ========== Implement native web notifications for mac Implementation behind the enable-native-notifications chrome://flags Conceptually based on a previous attempt. (https://codereview.chromium.org/224183002) but now using the same interface android uses. System and extensions notifications remain implemented through chrome toasts. There are a few things missing that will come in future CLs, most notably Buttons Auto dimsissal (i.e all notifications requireInteraction) Sounds BUG= ==========
miguelg@chromium.org changed reviewers: + rsesek@chromium.org
Robert can you have an initial look at the Mac specific pieces to see if I am doing something crazy? Peter and I are discussing if we should use a separate interface instead of the existing NotificationUIManager (and clean up some android inconsistencies while at it) but the Mac changes will more or less be the same in any case.
Robert can you have an initial look at the Mac specific pieces to see if I am doing something crazy? Peter and I are discussing if we should use a separate interface instead of the existing NotificationUIManager (and clean up some android inconsistencies while at it) but the Mac changes will more or less be the same in any case.
Looks pretty good! It does seem like only a subset of NotificationUIManager is used, e.g. the NOTREACHED() calls. https://codereview.chromium.org/1509923002/diff/40001/base/mac/sdk_forward_de... File base/mac/sdk_forward_declarations.h (right): https://codereview.chromium.org/1509923002/diff/40001/base/mac/sdk_forward_de... base/mac/sdk_forward_declarations.h:55: @interface NSUserNotification : NSObject Do you actually need these? We now build with the 10.10 SDK (required) so this condition should probably never be triggered. https://codereview.chromium.org/1509923002/diff/40001/chrome/browser/notifica... File chrome/browser/notifications/notification_ui_manager_mac.mm (right): https://codereview.chromium.org/1509923002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_ui_manager_mac.mm:76: NotificationUIManagerMac::~NotificationUIManagerMac() { Set the NSUserNotificationCenter delegate to nil, too? https://codereview.chromium.org/1509923002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_ui_manager_mac.mm:89: NSUserNotification* toast = [[NSUserNotification alloc] init]; This is leaked, so put it in a scoped_nsobject. https://codereview.chromium.org/1509923002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_ui_manager_mac.mm:117: int64_t profile_id = (int64_t)GetProfileID(profile); Use static_cast instead. https://codereview.chromium.org/1509923002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_ui_manager_mac.mm:120: dictionaryWithObjectsAndKeys: You can use a dictionary literal syntax here, which is slightly easier to read: @{ key : value } https://codereview.chromium.org/1509923002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_ui_manager_mac.mm:132: delegate->Display(); This should probably be called in userNotificationCenter:didDeliverNotification: instead. https://codereview.chromium.org/1509923002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_ui_manager_mac.mm:226: CHECK(manager); DCHECK instead? https://codereview.chromium.org/1509923002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_ui_manager_mac.mm:234: std::string notification_tag = base::SysNSStringToUTF8( naming: camelCase https://codereview.chromium.org/1509923002/diff/40001/chrome/browser/notifica... File chrome/browser/notifications/platform_notification_service_impl.h (right): https://codereview.chromium.org/1509923002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/platform_notification_service_impl.h:128: scoped_ptr<NotificationUIManager> native_notification_ui_manager_; Comment?
Thanks both for the review! Getting it ready for landing, module the possible interface change which we might delay until we know how it should look. https://codereview.chromium.org/1509923002/diff/40001/base/mac/sdk_forward_de... File base/mac/sdk_forward_declarations.h (right): https://codereview.chromium.org/1509923002/diff/40001/base/mac/sdk_forward_de... base/mac/sdk_forward_declarations.h:55: @interface NSUserNotification : NSObject On 2015/12/09 16:42:24, Robert Sesek wrote: > Do you actually need these? We now build with the 10.10 SDK (required) so this > condition should probably never be triggered. I think I just need the @class definitions below which I can move to the ui_manager instead. https://codereview.chromium.org/1509923002/diff/40001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1509923002/diff/40001/chrome/app/generated_re... chrome/app/generated_resources.grd:8086: + Enable support for using the native notification toasts and notification center in available platforms. On 2015/12/09 00:12:00, Peter Beverloo wrote: > s/in available platforms/on platforms where these are available/ Done. https://codereview.chromium.org/1509923002/diff/40001/chrome/browser/notifica... File chrome/browser/notifications/notification_test_util.cc (right): https://codereview.chromium.org/1509923002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_test_util.cc:141: ; On 2015/12/09 00:12:00, Peter Beverloo wrote: > Don't empty statements cause compile errors? Actually it didn't, thanks for spotting this one. https://codereview.chromium.org/1509923002/diff/40001/chrome/browser/notifica... File chrome/browser/notifications/notification_ui_manager_mac.h (right): https://codereview.chromium.org/1509923002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_ui_manager_mac.h:22: // send platform notifications to the Notification Center on 10.8 and above. On 2015/12/09 00:12:00, Peter Beverloo wrote: > This would be an excellent place to document *why* it's only available on 10.8+. Done. https://codereview.chromium.org/1509923002/diff/40001/chrome/browser/notifica... File chrome/browser/notifications/notification_ui_manager_mac.mm (right): https://codereview.chromium.org/1509923002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_ui_manager_mac.mm:31: // - Sound names can be implemented by setting soundName in NSUserNotification On 2015/12/09 00:12:00, Peter Beverloo wrote: > What would we use sounds for? Getting attention? Can we determine the system > default? Yes, will change the todo accordingly. https://codereview.chromium.org/1509923002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_ui_manager_mac.mm:32: // - notifiation.requireInteraction can be implemented by removing the On 2015/12/09 00:12:00, Peter Beverloo wrote: > nit: s/notifiation/notification/ Done. https://codereview.chromium.org/1509923002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_ui_manager_mac.mm:60: @interface NotificationCenterDelegate It wakes up the app and once a delegate is up and running it will deliver the event. We can totally make this work once we solve the profile issue mentioned later. On 2015/12/09 00:12:00, Peter Beverloo wrote: > What happens when a Chrome notification is shown in the notification center, but > the process itself isn't running? > > (I know that we work around this by calling CancelAll() in the destructor, but > ideally we'd like this to outlive the browser of course.) https://codereview.chromium.org/1509923002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_ui_manager_mac.mm:76: NotificationUIManagerMac::~NotificationUIManagerMac() { On 2015/12/09 16:42:25, Robert Sesek wrote: > Set the NSUserNotificationCenter delegate to nil, too? Done. https://codereview.chromium.org/1509923002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_ui_manager_mac.mm:89: NSUserNotification* toast = [[NSUserNotification alloc] init]; On 2015/12/09 16:42:25, Robert Sesek wrote: > This is leaked, so put it in a scoped_nsobject. Good catch https://codereview.chromium.org/1509923002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_ui_manager_mac.mm:96: notification.context_message().empty() It actually does get set for extensions using the web notification api On 2015/12/09 00:12:00, Peter Beverloo wrote: > nit: This class is only functional for persistent notifications, so > context_message() will never be set. Perhaps change the comment on lines 82-84 > to a TODO? https://codereview.chromium.org/1509923002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_ui_manager_mac.mm:104: // Icon On 2015/12/09 00:12:00, Peter Beverloo wrote: > nit: did you accidentally a Not sure what you mean here. https://codereview.chromium.org/1509923002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_ui_manager_mac.mm:117: int64_t profile_id = (int64_t)GetProfileID(profile); On 2015/12/09 16:42:25, Robert Sesek wrote: > Use static_cast instead. static_cast does work with void* types but I can use reinterpret_cast. We need to change the whole ProfileID thing anyway https://codereview.chromium.org/1509923002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_ui_manager_mac.mm:120: dictionaryWithObjectsAndKeys: On 2015/12/09 16:42:25, Robert Sesek wrote: > You can use a dictionary literal syntax here, which is slightly easier to read: > > @{ > key : value > } Done. https://codereview.chromium.org/1509923002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_ui_manager_mac.mm:132: delegate->Display(); On 2015/12/09 16:42:25, Robert Sesek wrote: > This should probably be called in userNotificationCenter:didDeliverNotification: > instead. Yes, it's probably more consistent but the problem is that I cannot pass the delegate itself safely since it is not a serializable object. https://codereview.chromium.org/1509923002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_ui_manager_mac.mm:226: CHECK(manager); On 2015/12/09 16:42:25, Robert Sesek wrote: > DCHECK instead? Done. https://codereview.chromium.org/1509923002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_ui_manager_mac.mm:234: std::string notification_tag = base::SysNSStringToUTF8( On 2015/12/09 16:42:25, Robert Sesek wrote: > naming: camelCase Done. https://codereview.chromium.org/1509923002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_ui_manager_mac.mm:248: ProfileManager::GetLastUsedProfile(), On 2015/12/09 00:12:00, Peter Beverloo wrote: > This is not a safe TODO on Mac, given that we can have multiple profiles there > (as opposed to Android). True I will add a scarier message. We are aware this cannot ship until this is fixed and are working on it separatly
https://codereview.chromium.org/1509923002/diff/60001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (left): https://codereview.chromium.org/1509923002/diff/60001/chrome/browser/about_fl... chrome/browser/about_flags.cc:526: const FeatureEntry kFeatureEntries[] = { clank format decided to do all of this, will revert and keep only my changes before landing
PTAL I worked through all the comments and rebased. Will fix the failing tests along the way.
https://codereview.chromium.org/1509923002/diff/40001/chrome/browser/notifica... File chrome/browser/notifications/notification_ui_manager_mac.mm (right): https://codereview.chromium.org/1509923002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_ui_manager_mac.mm:132: delegate->Display(); On 2015/12/10 20:16:30, Miguel Garcia wrote: > On 2015/12/09 16:42:25, Robert Sesek wrote: > > This should probably be called in > userNotificationCenter:didDeliverNotification: > > instead. > > Yes, it's probably more consistent but the problem is that I cannot pass the > delegate itself safely since it is not a serializable object. Can't you look the notification delegate up by tag or ID? https://codereview.chromium.org/1509923002/diff/80001/chrome/browser/notifica... File chrome/browser/notifications/notification_test_util.cc (right): https://codereview.chromium.org/1509923002/diff/80001/chrome/browser/notifica... chrome/browser/notifications/notification_test_util.cc:141: ; nit: remove https://codereview.chromium.org/1509923002/diff/80001/chrome/browser/notifica... File chrome/browser/notifications/notification_ui_manager.h (right): https://codereview.chromium.org/1509923002/diff/80001/chrome/browser/notifica... chrome/browser/notifications/notification_ui_manager.h:49: // own notificationn center. nit: indent +2 https://codereview.chromium.org/1509923002/diff/80001/chrome/browser/notifica... File chrome/browser/notifications/notification_ui_manager_mac.h (right): https://codereview.chromium.org/1509923002/diff/80001/chrome/browser/notifica... chrome/browser/notifications/notification_ui_manager_mac.h:13: #include "base/mac/sdk_forward_declarations.h" Unused? https://codereview.chromium.org/1509923002/diff/80001/chrome/browser/notifica... chrome/browser/notifications/notification_ui_manager_mac.h:18: @class NSUserNotification; These forward-declares don't need to be in the .h. https://codereview.chromium.org/1509923002/diff/80001/chrome/browser/notifica... File chrome/browser/notifications/notification_ui_manager_mac.mm (right): https://codereview.chromium.org/1509923002/diff/80001/chrome/browser/notifica... chrome/browser/notifications/notification_ui_manager_mac.mm:92: toast.get().title = base::SysUTF16ToNSString(notification.title()); Do you need the .get()s everywhere? The operator T() should convert. https://codereview.chromium.org/1509923002/diff/80001/chrome/browser/notifica... chrome/browser/notifications/notification_ui_manager_mac.mm:159: [[NSUserNotificationCenter defaultUserNotificationCenter] Pull the result of -defaultUserNotificationCenter out into a local? It's verbose and would make line 164 easier to read, too. https://codereview.chromium.org/1509923002/diff/80001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/1509923002/diff/80001/chrome/chrome_browser.g... chrome/chrome_browser.gypi:2063: 'chrome_browser_notifications_mac_sources': [ The GYP filename rules should filter this out, so it can probably just go in chrome_browser_notifications_sources and you can remove the extra conditions below.
all sorted I think. PTAL https://codereview.chromium.org/1509923002/diff/40001/chrome/browser/notifica... File chrome/browser/notifications/notification_ui_manager_mac.mm (right): https://codereview.chromium.org/1509923002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_ui_manager_mac.mm:132: delegate->Display(); On 2015/12/11 21:17:53, Robert Sesek wrote: > On 2015/12/10 20:16:30, Miguel Garcia wrote: > > On 2015/12/09 16:42:25, Robert Sesek wrote: > > > This should probably be called in > > userNotificationCenter:didDeliverNotification: > > > instead. > > > > Yes, it's probably more consistent but the problem is that I cannot pass the > > delegate itself safely since it is not a serializable object. > > Can't you look the notification delegate up by tag or ID? Yeah we could maintain state but even better, I realized that the Display method in the delegate is empty :) So I just removed the call https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/not... https://codereview.chromium.org/1509923002/diff/80001/chrome/browser/notifica... File chrome/browser/notifications/notification_test_util.cc (right): https://codereview.chromium.org/1509923002/diff/80001/chrome/browser/notifica... chrome/browser/notifications/notification_test_util.cc:141: ; On 2015/12/11 21:17:53, Robert Sesek wrote: > nit: remove Done. https://codereview.chromium.org/1509923002/diff/80001/chrome/browser/notifica... File chrome/browser/notifications/notification_ui_manager.h (right): https://codereview.chromium.org/1509923002/diff/80001/chrome/browser/notifica... chrome/browser/notifications/notification_ui_manager.h:49: // own notificationn center. On 2015/12/11 21:17:53, Robert Sesek wrote: > nit: indent +2 Done. https://codereview.chromium.org/1509923002/diff/80001/chrome/browser/notifica... File chrome/browser/notifications/notification_ui_manager_mac.h (right): https://codereview.chromium.org/1509923002/diff/80001/chrome/browser/notifica... chrome/browser/notifications/notification_ui_manager_mac.h:13: #include "base/mac/sdk_forward_declarations.h" On 2015/12/11 21:17:53, Robert Sesek wrote: > Unused? Done. https://codereview.chromium.org/1509923002/diff/80001/chrome/browser/notifica... chrome/browser/notifications/notification_ui_manager_mac.h:18: @class NSUserNotification; On 2015/12/11 21:17:53, Robert Sesek wrote: > These forward-declares don't need to be in the .h. Moved them to the .mm https://codereview.chromium.org/1509923002/diff/80001/chrome/browser/notifica... File chrome/browser/notifications/notification_ui_manager_mac.mm (right): https://codereview.chromium.org/1509923002/diff/80001/chrome/browser/notifica... chrome/browser/notifications/notification_ui_manager_mac.mm:92: toast.get().title = base::SysUTF16ToNSString(notification.title()); On 2015/12/11 21:17:53, Robert Sesek wrote: > Do you need the .get()s everywhere? The operator T() should convert. Well if I do for example toast.title = base::SysUTF16ToNSString(notification.title()); I get error: no member named 'title' in 'base::scoped_nsobject<NSUserNotification>' toast.title = base::SysUTF16ToNSString(notification.title()); Apologies if this is super basic. First tie I use a scoped_nsobject. With scoped pointers this does not usually happen. I did a quick search in the codebase and occurrences either do use get() for assignments or do it via [setObject... forKey] assignments. https://codereview.chromium.org/1509923002/diff/80001/chrome/browser/notifica... chrome/browser/notifications/notification_ui_manager_mac.mm:159: [[NSUserNotificationCenter defaultUserNotificationCenter] On 2015/12/11 21:17:53, Robert Sesek wrote: > Pull the result of -defaultUserNotificationCenter out into a local? It's verbose > and would make line 164 easier to read, too. Done. Meta question. How do I figure out if [NSUserNotificationCenter defaultUserNotificationCenter] returns an object that I need to free myself? I assume I don't but the documentation on the matter is very scarce https://developer.apple.com/library/mac/documentation/Foundation/Reference/NS... https://codereview.chromium.org/1509923002/diff/80001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/1509923002/diff/80001/chrome/chrome_browser.g... chrome/chrome_browser.gypi:2063: 'chrome_browser_notifications_mac_sources': [ On 2015/12/11 21:17:53, Robert Sesek wrote: > The GYP filename rules should filter this out, so it can probably just go in > chrome_browser_notifications_sources and you can remove the extra conditions > below. Done. Good idea
https://codereview.chromium.org/1509923002/diff/40001/chrome/browser/notifica... File chrome/browser/notifications/notification_ui_manager_mac.mm (right): https://codereview.chromium.org/1509923002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_ui_manager_mac.mm:132: delegate->Display(); On 2015/12/15 13:37:38, Miguel Garcia wrote: > On 2015/12/11 21:17:53, Robert Sesek wrote: > > On 2015/12/10 20:16:30, Miguel Garcia wrote: > > > On 2015/12/09 16:42:25, Robert Sesek wrote: > > > > This should probably be called in > > > userNotificationCenter:didDeliverNotification: > > > > instead. > > > > > > Yes, it's probably more consistent but the problem is that I cannot pass the > > > delegate itself safely since it is not a serializable object. > > > > Can't you look the notification delegate up by tag or ID? > > Yeah we could maintain state but even better, I realized that the Display method > in the delegate is empty :) So I just removed the call > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/not... That's a little surprising, given that a show event must be fired (http://www.w3.org/TR/notifications/#display-steps), but I guess Blink will just do that after pushing the notification up to content? https://codereview.chromium.org/1509923002/diff/80001/chrome/browser/notifica... File chrome/browser/notifications/notification_ui_manager_mac.mm (right): https://codereview.chromium.org/1509923002/diff/80001/chrome/browser/notifica... chrome/browser/notifications/notification_ui_manager_mac.mm:92: toast.get().title = base::SysUTF16ToNSString(notification.title()); On 2015/12/15 13:37:40, Miguel Garcia wrote: > On 2015/12/11 21:17:53, Robert Sesek wrote: > > Do you need the .get()s everywhere? The operator T() should convert. > > Well if I do for example > toast.title = base::SysUTF16ToNSString(notification.title()); > > I get > > error: no member named 'title' in 'base::scoped_nsobject<NSUserNotification>' > toast.title = base::SysUTF16ToNSString(notification.title()); > > > Apologies if this is super basic. First tie I use a scoped_nsobject. With scoped > pointers this does not usually happen. I did a quick search in the codebase and > occurrences either do use get() for assignments or do it via [setObject... > forKey] assignments. Ah, yes C++ gets confused about that. If you use [toast setTitle:base::SysUTF16ToNSString(notification.title())] you'll be able to drop the get. https://codereview.chromium.org/1509923002/diff/80001/chrome/browser/notifica... chrome/browser/notifications/notification_ui_manager_mac.mm:159: [[NSUserNotificationCenter defaultUserNotificationCenter] On 2015/12/15 13:37:41, Miguel Garcia wrote: > On 2015/12/11 21:17:53, Robert Sesek wrote: > > Pull the result of -defaultUserNotificationCenter out into a local? It's > verbose > > and would make line 164 easier to read, too. > > Done. > > Meta question. How do I figure out if [NSUserNotificationCenter > defaultUserNotificationCenter] returns an object that I need to free myself? I > assume I don't but the documentation on the matter is very scarce > https://developer.apple.com/library/mac/documentation/Foundation/Reference/NS... > Methods by default return weak references to objects, unless the method name begins with alloc, new, or copy. Full details: https://developer.apple.com/library/mac/documentation/Cocoa/Conceptual/Memory...
Description was changed from ========== Implement native web notifications for mac Implementation behind the enable-native-notifications chrome://flags Conceptually based on a previous attempt. (https://codereview.chromium.org/224183002) but now using the same interface android uses. System and extensions notifications remain implemented through chrome toasts. There are a few things missing that will come in future CLs, most notably Buttons Auto dimsissal (i.e all notifications requireInteraction) Sounds BUG= ========== to ========== Implement native web notifications for mac Implementation behind the enable-native-notifications chrome://flags Conceptually based on a previous attempt. (https://codereview.chromium.org/224183002) but now using the same interface android uses. System and extensions notifications remain implemented through chrome toasts. There are a few things missing that will come in future CLs, most notably Buttons Auto dimsissal (i.e all notifications requireInteraction) Sounds BUG=571056 ==========
miguelg@chromium.org changed reviewers: + cpu@chromium.org
+cpu@ for chrome/app/app-Info.plist +mpearson@ for tools/metrics/histograms/histograms.xml
Thanks for the review! > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/not... > > That's a little surprising, given that a show event must be fired > (http://www.w3.org/TR/notifications/#display-steps), but I guess Blink will just > do that after pushing the notification up to content? > Exactly > https://codereview.chromium.org/1509923002/diff/80001/chrome/browser/notifica... > File chrome/browser/notifications/notification_ui_manager_mac.mm (right): > > https://codereview.chromium.org/1509923002/diff/80001/chrome/browser/notifica... > chrome/browser/notifications/notification_ui_manager_mac.mm:92: > toast.get().title = base::SysUTF16ToNSString(notification.title()); > On 2015/12/15 13:37:40, Miguel Garcia wrote: > > On 2015/12/11 21:17:53, Robert Sesek wrote: > > > Do you need the .get()s everywhere? The operator T() should convert. > > > > Well if I do for example > > toast.title = base::SysUTF16ToNSString(notification.title()); > > > > I get > > > > error: no member named 'title' in 'base::scoped_nsobject<NSUserNotification>' > > toast.title = base::SysUTF16ToNSString(notification.title()); > > > > > > Apologies if this is super basic. First tie I use a scoped_nsobject. With > scoped > > pointers this does not usually happen. I did a quick search in the codebase > and > > occurrences either do use get() for assignments or do it via [setObject... > > forKey] assignments. > > Ah, yes C++ gets confused about that. If you use [toast > setTitle:base::SysUTF16ToNSString(notification.title())] you'll be able to drop > the get. Done. > > https://codereview.chromium.org/1509923002/diff/80001/chrome/browser/notifica... > chrome/browser/notifications/notification_ui_manager_mac.mm:159: > [[NSUserNotificationCenter defaultUserNotificationCenter] > On 2015/12/15 13:37:41, Miguel Garcia wrote: > > On 2015/12/11 21:17:53, Robert Sesek wrote: > > > Pull the result of -defaultUserNotificationCenter out into a local? It's > > verbose > > > and would make line 164 easier to read, too. > > > > Done. > > > > Meta question. How do I figure out if [NSUserNotificationCenter > > defaultUserNotificationCenter] returns an object that I need to free myself? I > > assume I don't but the documentation on the matter is very scarce > > > https://developer.apple.com/library/mac/documentation/Foundation/Reference/NS... > > > > Methods by default return weak references to objects, unless the method name > begins with alloc, new, or copy. Full details: > https://developer.apple.com/library/mac/documentation/Cocoa/Conceptual/Memory... Ah thanks!
LGTM https://codereview.chromium.org/1509923002/diff/120001/chrome/browser/notific... File chrome/browser/notifications/notification_ui_manager_mac.h (right): https://codereview.chromium.org/1509923002/diff/120001/chrome/browser/notific... chrome/browser/notifications/notification_ui_manager_mac.h:8: #include <map> Unused?
On 2015/12/21 23:14:25, Robert Sesek wrote: > LGTM > > https://codereview.chromium.org/1509923002/diff/120001/chrome/browser/notific... > File chrome/browser/notifications/notification_ui_manager_mac.h (right): > > https://codereview.chromium.org/1509923002/diff/120001/chrome/browser/notific... > chrome/browser/notifications/notification_ui_manager_mac.h:8: #include <map> > Unused? Friendly ping post vacations.
hrome/app/app-Info.plist lgtm with note https://codereview.chromium.org/1509923002/diff/120001/chrome/app/app-Info.plist File chrome/app/app-Info.plist (right): https://codereview.chromium.org/1509923002/diff/120001/chrome/app/app-Info.pl... chrome/app/app-Info.plist:352: <string>alert</string> weird indentation, btw
miguelg@chromium.org changed reviewers: + mpearson@chromium.org
+mpearson@chromium.org for realz this time.
histograms.xml lgtm
Peter care to have a final look before I land it? The profile issue is unresolved in this cl but as you know we have a solution for it and we are dealing with it as part of crbug.com/437574 so I feel more comfortable landing this. https://codereview.chromium.org/1509923002/diff/120001/chrome/app/app-Info.plist File chrome/app/app-Info.plist (right): https://codereview.chromium.org/1509923002/diff/120001/chrome/app/app-Info.pl... chrome/app/app-Info.plist:352: <string>alert</string> On 2016/01/06 17:30:19, cpu_slow_until_jan4 wrote: > weird indentation, btw This seems to match the indentation in the rest of the file. Is there anything I should be doing here? https://codereview.chromium.org/1509923002/diff/120001/chrome/browser/notific... File chrome/browser/notifications/notification_ui_manager_mac.h (right): https://codereview.chromium.org/1509923002/diff/120001/chrome/browser/notific... chrome/browser/notifications/notification_ui_manager_mac.h:8: #include <map> On 2015/12/21 23:14:25, Robert Sesek wrote: > Unused? Done.
Thank you, and sorry for the delay! A few comments. https://codereview.chromium.org/1509923002/diff/140001/chrome/browser/notific... File chrome/browser/notifications/notification_ui_manager_mac.h (right): https://codereview.chromium.org/1509923002/diff/140001/chrome/browser/notific... chrome/browser/notifications/notification_ui_manager_mac.h:11: #include "base/mac/scoped_nsobject.h" +base/macros.h https://codereview.chromium.org/1509923002/diff/140001/chrome/browser/notific... chrome/browser/notifications/notification_ui_manager_mac.h:22: // notifications are still used there. Please document that delegates MUST be of type PersistentNotificationDelegate now.. https://codereview.chromium.org/1509923002/diff/140001/chrome/browser/notific... File chrome/browser/notifications/notification_ui_manager_mac.mm (right): https://codereview.chromium.org/1509923002/diff/140001/chrome/browser/notific... chrome/browser/notifications/notification_ui_manager_mac.mm:186: // Therefore because when chrome quits we cancel all pending notifications. Please do follow up with the profile changes you recently made to the Android notification manager, so that we can stop using the ProfileID for this purpose. https://codereview.chromium.org/1509923002/diff/140001/chrome/browser/notific... File chrome/browser/notifications/platform_notification_service_impl.cc (right): https://codereview.chromium.org/1509923002/diff/140001/chrome/browser/notific... chrome/browser/notifications/platform_notification_service_impl.cc:102: NotificationUIManager::CreateNativeNotificationManager()); Could you revert the changes to notification_ui_manager.h, and instead create a helper function (in the anonymous namespace above) that does the following? scoped_ptr<NotificationUIManager> GetNativeNotificationUIManager() { #if MAC if (... AcceptNativeNotifications() conditions ...) return new NotificationUIManagerMac(); #else return nullptr; #endif } You can then assign in the initialization list of this constructor. That significantly reduces the impact of this patch on code it doesn't have to touch, and gives us a much cleaner patch towards supporting a Windows prototype or converting the Android manager. https://codereview.chromium.org/1509923002/diff/140001/chrome/browser/notific... chrome/browser/notifications/platform_notification_service_impl.cc:342: base::Int64ToString(persistent_notification_id), I very badly need to clean this up :-(. Per the previous comment, you could make this slightly better by doing something like: #if ANDROID bool cancel_by_persistent_id = true; #else bool cancel_by_persistent_id = !!native_notification_ui_manager_; #endif if (cancel_by_persistent_id) { GetNoti..()->.. } https://codereview.chromium.org/1509923002/diff/140001/chrome/browser/notific... File chrome/browser/notifications/platform_notification_service_impl.h (right): https://codereview.chromium.org/1509923002/diff/140001/chrome/browser/notific... chrome/browser/notifications/platform_notification_service_impl.h:129: scoped_ptr<NotificationUIManager> native_notification_ui_manager_; + docs https://codereview.chromium.org/1509923002/diff/140001/chrome/common/chrome_s... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/1509923002/diff/140001/chrome/common/chrome_s... chrome/common/chrome_switches.cc:440: // using the chrome based ones. weird wrapping chrome -> Chrome
thanks Peter! https://codereview.chromium.org/1509923002/diff/140001/chrome/browser/notific... File chrome/browser/notifications/notification_ui_manager_mac.h (right): https://codereview.chromium.org/1509923002/diff/140001/chrome/browser/notific... chrome/browser/notifications/notification_ui_manager_mac.h:11: #include "base/mac/scoped_nsobject.h" On 2016/01/08 05:14:26, Peter Beverloo wrote: > +base/macros.h Done. https://codereview.chromium.org/1509923002/diff/140001/chrome/browser/notific... chrome/browser/notifications/notification_ui_manager_mac.h:22: // notifications are still used there. On 2016/01/08 05:14:26, Peter Beverloo wrote: > Please document that delegates MUST be of type PersistentNotificationDelegate > now.. Done. https://codereview.chromium.org/1509923002/diff/140001/chrome/browser/notific... File chrome/browser/notifications/notification_ui_manager_mac.mm (right): https://codereview.chromium.org/1509923002/diff/140001/chrome/browser/notific... chrome/browser/notifications/notification_ui_manager_mac.mm:186: // Therefore because when chrome quits we cancel all pending notifications. On 2016/01/08 05:14:26, Peter Beverloo wrote: > Please do follow up with the profile changes you recently made to the Android > notification manager, so that we can stop using the ProfileID for this purpose. Acknowledged. https://codereview.chromium.org/1509923002/diff/140001/chrome/browser/notific... File chrome/browser/notifications/platform_notification_service_impl.cc (right): https://codereview.chromium.org/1509923002/diff/140001/chrome/browser/notific... chrome/browser/notifications/platform_notification_service_impl.cc:102: NotificationUIManager::CreateNativeNotificationManager()); So the problem with doing it the way you suggest is that then this (pure C++) file needs to include notification_ui_manager_mac.h which contains ObjC code. This is sort of ugly but on top of that it does not compile since ObjC can only be included as a seaparate compilation unit that is then merged at link time. I am happy to discuss alternatives but let's do it offline then, not in the code review. Perhaps also after this cl lands? In any case I think that AcceptNativeNotifications() does belong to the notification_ui_manager.h interface. On 2016/01/08 05:14:26, Peter Beverloo wrote: > Could you revert the changes to notification_ui_manager.h, and instead create a > helper function (in the anonymous namespace above) that does the following? > > scoped_ptr<NotificationUIManager> GetNativeNotificationUIManager() { > #if MAC > if (... AcceptNativeNotifications() conditions ...) > return new NotificationUIManagerMac(); > #else > return nullptr; > #endif > } > > You can then assign in the initialization list of this constructor. That > significantly reduces the impact of this patch on code it doesn't have to touch, > and gives us a much cleaner patch towards supporting a Windows prototype or > converting the Android manager. https://codereview.chromium.org/1509923002/diff/140001/chrome/browser/notific... chrome/browser/notifications/platform_notification_service_impl.cc:342: base::Int64ToString(persistent_notification_id), On 2016/01/08 05:14:26, Peter Beverloo wrote: > I very badly need to clean this up :-(. > > Per the previous comment, you could make this slightly better by doing something > like: > > #if ANDROID > bool cancel_by_persistent_id = true; > #else > bool cancel_by_persistent_id = !!native_notification_ui_manager_; > #endif > > if (cancel_by_persistent_id) { > GetNoti..()->.. > } Done. https://codereview.chromium.org/1509923002/diff/140001/chrome/browser/notific... File chrome/browser/notifications/platform_notification_service_impl.h (right): https://codereview.chromium.org/1509923002/diff/140001/chrome/browser/notific... chrome/browser/notifications/platform_notification_service_impl.h:129: scoped_ptr<NotificationUIManager> native_notification_ui_manager_; On 2016/01/08 05:14:26, Peter Beverloo wrote: > + docs Done. https://codereview.chromium.org/1509923002/diff/140001/chrome/common/chrome_s... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/1509923002/diff/140001/chrome/common/chrome_s... chrome/common/chrome_switches.cc:440: // using the chrome based ones. On 2016/01/08 05:14:26, Peter Beverloo wrote: > weird wrapping > > chrome -> Chrome Done.
https://codereview.chromium.org/1509923002/diff/140001/chrome/browser/notific... File chrome/browser/notifications/platform_notification_service_impl.cc (right): https://codereview.chromium.org/1509923002/diff/140001/chrome/browser/notific... chrome/browser/notifications/platform_notification_service_impl.cc:102: NotificationUIManager::CreateNativeNotificationManager()); On 2016/01/08 11:46:55, Miguel Garcia wrote: > So the problem with doing it the way you suggest is that then this (pure C++) > file needs to include notification_ui_manager_mac.h which contains ObjC code. > This is sort of ugly but on top of that it does not compile since ObjC can only > be included as a seaparate compilation unit that is then merged at link time. This is a good point. Looking at various other files, we can include C++ files from ObjC code, but not the other way around. Using an interface class for NotificationCenterDelegate won't help either because scoped_nsobject.h uses ObjC code as well. Instead, let's remove the #ifdefs - define a CreateNativeNotificationManager() for Android and non-Mac desktop (w/ an ifdef...) that always returns a nullptr, and always initialise native_notification_ui_manager_ with its value. In the Mac version (in the .mm file), only return a non-nullptr when the conditions you currently list in AcceptNativeNotifications() pass. > I am happy to discuss alternatives but let's do it offline then, not in the code > review. Perhaps also after this cl lands? > > In any case I think that AcceptNativeNotifications() does belong to the > notification_ui_manager.h interface. We won't use it if AcceptNativeNotifications() returns false, so we've allocated memory for no reason at that point. This is exactly the reason why factory functions exists. > On 2016/01/08 05:14:26, Peter Beverloo wrote: > > Could you revert the changes to notification_ui_manager.h, and instead create > a > > helper function (in the anonymous namespace above) that does the following? > > > > scoped_ptr<NotificationUIManager> GetNativeNotificationUIManager() { > > #if MAC > > if (... AcceptNativeNotifications() conditions ...) > > return new NotificationUIManagerMac(); > > #else > > return nullptr; > > #endif > > } > > > > You can then assign in the initialization list of this constructor. That > > significantly reduces the impact of this patch on code it doesn't have to > touch, > > and gives us a much cleaner patch towards supporting a Windows prototype or > > converting the Android manager. >
PTAL I removed the AcceptNativeNotifications and removed the ifdefs around CreateNativeNotificationManager On 2016/01/08 19:40:20, Peter Beverloo wrote: > https://codereview.chromium.org/1509923002/diff/140001/chrome/browser/notific... > File chrome/browser/notifications/platform_notification_service_impl.cc (right): > > https://codereview.chromium.org/1509923002/diff/140001/chrome/browser/notific... > chrome/browser/notifications/platform_notification_service_impl.cc:102: > NotificationUIManager::CreateNativeNotificationManager()); > On 2016/01/08 11:46:55, Miguel Garcia wrote: > > So the problem with doing it the way you suggest is that then this (pure C++) > > file needs to include notification_ui_manager_mac.h which contains ObjC code. > > This is sort of ugly but on top of that it does not compile since ObjC can > only > > be included as a seaparate compilation unit that is then merged at link time. > > This is a good point. Looking at various other files, we can include C++ files > from ObjC code, but not the other way around. Using an interface class for > NotificationCenterDelegate won't help either because scoped_nsobject.h uses ObjC > code as well. > > Instead, let's remove the #ifdefs - define a CreateNativeNotificationManager() > for Android and non-Mac desktop (w/ an ifdef...) that always returns a nullptr, > and always initialise native_notification_ui_manager_ with its value. In the Mac > version (in the .mm file), only return a non-nullptr when the conditions you > currently list in AcceptNativeNotifications() pass. > > > I am happy to discuss alternatives but let's do it offline then, not in the > code > > review. Perhaps also after this cl lands? > > > > In any case I think that AcceptNativeNotifications() does belong to the > > notification_ui_manager.h interface. > > We won't use it if AcceptNativeNotifications() returns false, so we've allocated > memory for no reason at that point. This is exactly the reason why factory > functions exists. > > > On 2016/01/08 05:14:26, Peter Beverloo wrote: > > > Could you revert the changes to notification_ui_manager.h, and instead > create > > a > > > helper function (in the anonymous namespace above) that does the following? > > > > > > scoped_ptr<NotificationUIManager> GetNativeNotificationUIManager() { > > > #if MAC > > > if (... AcceptNativeNotifications() conditions ...) > > > return new NotificationUIManagerMac(); > > > #else > > > return nullptr; > > > #endif > > > } > > > > > > You can then assign in the initialization list of this constructor. That > > > significantly reduces the impact of this patch on code it doesn't have to > > touch, > > > and gives us a much cleaner patch towards supporting a Windows prototype or > > > converting the Android manager. > >
The CQ bit was checked by peter@chromium.org
lgtm
The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org, cpu@chromium.org, mpearson@chromium.org Link to the patchset: https://codereview.chromium.org/1509923002/#ps200001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1509923002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1509923002/200001
Message was sent while issue was closed.
Description was changed from ========== Implement native web notifications for mac Implementation behind the enable-native-notifications chrome://flags Conceptually based on a previous attempt. (https://codereview.chromium.org/224183002) but now using the same interface android uses. System and extensions notifications remain implemented through chrome toasts. There are a few things missing that will come in future CLs, most notably Buttons Auto dimsissal (i.e all notifications requireInteraction) Sounds BUG=571056 ========== to ========== Implement native web notifications for mac Implementation behind the enable-native-notifications chrome://flags Conceptually based on a previous attempt. (https://codereview.chromium.org/224183002) but now using the same interface android uses. System and extensions notifications remain implemented through chrome toasts. There are a few things missing that will come in future CLs, most notably Buttons Auto dimsissal (i.e all notifications requireInteraction) Sounds BUG=571056 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Implement native web notifications for mac Implementation behind the enable-native-notifications chrome://flags Conceptually based on a previous attempt. (https://codereview.chromium.org/224183002) but now using the same interface android uses. System and extensions notifications remain implemented through chrome toasts. There are a few things missing that will come in future CLs, most notably Buttons Auto dimsissal (i.e all notifications requireInteraction) Sounds BUG=571056 ========== to ========== Implement native web notifications for mac Implementation behind the enable-native-notifications chrome://flags Conceptually based on a previous attempt. (https://codereview.chromium.org/224183002) but now using the same interface android uses. System and extensions notifications remain implemented through chrome toasts. There are a few things missing that will come in future CLs, most notably Buttons Auto dimsissal (i.e all notifications requireInteraction) Sounds BUG=571056 Committed: https://crrev.com/3bf899c9d98f84f3e7a297e04c012867cf0ddc11 Cr-Commit-Position: refs/heads/master@{#369255} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/3bf899c9d98f84f3e7a297e04c012867cf0ddc11 Cr-Commit-Position: refs/heads/master@{#369255} |
