Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2402303002:
Implement the ability to close alerts programatically (Closed)
|
Created:
4 years, 2 months ago by Miguel Garcia Modified:
4 years, 2 months ago CC:
chromium-reviews, mlamouri+watch-notifications_chromium.org, awdf+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement the ability to close alerts programatically.
BUG=571056
Committed: https://crrev.com/470c5e56f7afdfa2552babe34b484c2808bfa1ef
Cr-Commit-Position: refs/heads/master@{#425559}
Patch Set 1 #
Total comments: 7
Patch Set 2 : review #
Total comments: 2
Patch Set 3 : review #Patch Set 4 : break early if the notification to close is found #
Total comments: 4
Patch Set 5 : review #
Depends on Patchset: Messages
Total messages: 27 (7 generated)
Description was changed from ========== Implement programatic closing for alerts. BUG=571056 ========== to ========== Implement the ability to close alerts programatically. BUG=571056 ==========
miguelg@chromium.org changed reviewers: + peter@chromium.org, rsesek@chromium.org
Patchset #1 (id:1) has been deleted
https://codereview.chromium.org/2402303002/diff/20001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_mac.mm (right): https://codereview.chromium.org/2402303002/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac.mm:434: alertsDisplayed_.reset([[NSMutableSet alloc] init]); What is the purpose of this set? It looks like things are only ever added or removed from it, never consulted. https://codereview.chromium.org/2402303002/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac.mm:499: - (NSString*)_createAlertKeyWithNotification:(NSString*)notificationId We don't name private methods with _. https://codereview.chromium.org/2402303002/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/notifications/notification_delivery.h (right): https://codereview.chromium.org/2402303002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/notifications/notification_delivery.h:20: - (void)closeNotification:(NSString*)notificationId I'd name this -closeNotificationWithId:withProfileId: because Notification and Profile both imply objects rather than strings.
https://chromiumcodereview.appspot.com/2402303002/diff/20001/chrome/browser/n... File chrome/browser/notifications/notification_platform_bridge_mac.mm (right): https://chromiumcodereview.appspot.com/2402303002/diff/20001/chrome/browser/n... chrome/browser/notifications/notification_platform_bridge_mac.mm:434: alertsDisplayed_.reset([[NSMutableSet alloc] init]); On 2016/10/10 18:22:47, Robert Sesek wrote: > What is the purpose of this set? It looks like things are only ever added or > removed from it, never consulted. It is consulted through isAlertDisplayed which is used in the ::Close( method to figure out if what we should be closing is an alert or a banner. https://chromiumcodereview.appspot.com/2402303002/diff/20001/chrome/browser/n... chrome/browser/notifications/notification_platform_bridge_mac.mm:499: - (NSString*)_createAlertKeyWithNotification:(NSString*)notificationId On 2016/10/10 18:22:47, Robert Sesek wrote: > We don't name private methods with _. Done. https://chromiumcodereview.appspot.com/2402303002/diff/20001/chrome/browser/u... File chrome/browser/ui/cocoa/notifications/notification_delivery.h (right): https://chromiumcodereview.appspot.com/2402303002/diff/20001/chrome/browser/u... chrome/browser/ui/cocoa/notifications/notification_delivery.h:20: - (void)closeNotification:(NSString*)notificationId On 2016/10/10 18:22:47, Robert Sesek wrote: > I'd name this -closeNotificationWithId:withProfileId: because Notification and > Profile both imply objects rather than strings. Done.
https://codereview.chromium.org/2402303002/diff/40001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_mac.mm (right): https://codereview.chromium.org/2402303002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac.mm:430: base::scoped_nsobject<NSMutableSet> alertsDisplayed_; Instead of caching which alerts we still think are opened, why not try to close a banner first (as you do in NotificationPlatformBridgeMac::Close), and always fall back to the NotificationRemoteDispatcher if that failed? It looks like that would remove the majority of complexity here.
On 2016/10/11 14:37:49, Peter Beverloo wrote: > https://codereview.chromium.org/2402303002/diff/40001/chrome/browser/notifica... > File chrome/browser/notifications/notification_platform_bridge_mac.mm (right): > > https://codereview.chromium.org/2402303002/diff/40001/chrome/browser/notifica... > chrome/browser/notifications/notification_platform_bridge_mac.mm:430: > base::scoped_nsobject<NSMutableSet> alertsDisplayed_; > Instead of caching which alerts we still think are opened, why not try to close > a banner first (as you do in NotificationPlatformBridgeMac::Close), and always > fall back to the NotificationRemoteDispatcher if that failed? > > It looks like that would remove the majority of complexity here. I thought about that but it just does not seems correct. "Complexity" here is keeping a set in sync which does not seem too bad. If you both feel strongly I'll switch to that.
On 2016/10/11 14:41:08, Miguel Garcia wrote: > On 2016/10/11 14:37:49, Peter Beverloo wrote: > > > https://codereview.chromium.org/2402303002/diff/40001/chrome/browser/notifica... > > File chrome/browser/notifications/notification_platform_bridge_mac.mm (right): > > > > > https://codereview.chromium.org/2402303002/diff/40001/chrome/browser/notifica... > > chrome/browser/notifications/notification_platform_bridge_mac.mm:430: > > base::scoped_nsobject<NSMutableSet> alertsDisplayed_; > > Instead of caching which alerts we still think are opened, why not try to > close > > a banner first (as you do in NotificationPlatformBridgeMac::Close), and always > > fall back to the NotificationRemoteDispatcher if that failed? > > > > It looks like that would remove the majority of complexity here. > > I thought about that but it just does not seems correct. "Complexity" here is > keeping a set in sync which does not seem too bad. If you both feel strongly > I'll switch to that. What is not correct about it? The possibility of sending a message to the XPC service for closing a notification that doesn't exist? My concern with the cache is that we don't know whether it'll be completely correct, i.e. whether there are cases where the close event is used (or not used) for the wrong reasons. Additionally, if we ever find a way to continue showing alerts while Chrome isn't running, this is per definition going to be incorrect.
On 2016/10/11 14:46:38, Peter Beverloo wrote: > On 2016/10/11 14:41:08, Miguel Garcia wrote: > > On 2016/10/11 14:37:49, Peter Beverloo wrote: > > > > > > https://codereview.chromium.org/2402303002/diff/40001/chrome/browser/notifica... > > > File chrome/browser/notifications/notification_platform_bridge_mac.mm > (right): > > > > > > > > > https://codereview.chromium.org/2402303002/diff/40001/chrome/browser/notifica... > > > chrome/browser/notifications/notification_platform_bridge_mac.mm:430: > > > base::scoped_nsobject<NSMutableSet> alertsDisplayed_; > > > Instead of caching which alerts we still think are opened, why not try to > > close > > > a banner first (as you do in NotificationPlatformBridgeMac::Close), and > always > > > fall back to the NotificationRemoteDispatcher if that failed? > > > > > > It looks like that would remove the majority of complexity here. > > > > I thought about that but it just does not seems correct. "Complexity" here is > > keeping a set in sync which does not seem too bad. If you both feel strongly > > I'll switch to that. > > What is not correct about it? The possibility of sending a message to the XPC > service for closing a notification that doesn't exist? > > My concern with the cache is that we don't know whether it'll be completely > correct, i.e. whether there are cases where the close event is used (or not > used) for the wrong reasons. > > Additionally, if we ever find a way to continue showing alerts while Chrome > isn't running, this is per definition going to be incorrect. I'm also concerned about this. In the original notification center integration I did, we did end up having bugs around the OS not reliably notifying us when notifications were closed after a period of time, so our cache got stale.
Yes, the fact that we will be issuing close requests to the wrong process every time we need to close an alert. We don't really need it to be completely correct, we just need it to be a super set of reality. I don't think we will ever be able to continue showing alerts while chrome isn't running. Once you lose the XPC service they will not be able to send clicks backs. That is a no go. At most we will be able to continue showing banners while chrome is running. On 2016/10/11 14:46:38, Peter Beverloo wrote: > On 2016/10/11 14:41:08, Miguel Garcia wrote: > > On 2016/10/11 14:37:49, Peter Beverloo wrote: > > > > > > https://codereview.chromium.org/2402303002/diff/40001/chrome/browser/notifica... > > > File chrome/browser/notifications/notification_platform_bridge_mac.mm > (right): > > > > > > > > > https://codereview.chromium.org/2402303002/diff/40001/chrome/browser/notifica... > > > chrome/browser/notifications/notification_platform_bridge_mac.mm:430: > > > base::scoped_nsobject<NSMutableSet> alertsDisplayed_; > > > Instead of caching which alerts we still think are opened, why not try to > > close > > > a banner first (as you do in NotificationPlatformBridgeMac::Close), and > always > > > fall back to the NotificationRemoteDispatcher if that failed? > > > > > > It looks like that would remove the majority of complexity here. > > > > I thought about that but it just does not seems correct. "Complexity" here is > > keeping a set in sync which does not seem too bad. If you both feel strongly > > I'll switch to that. > > What is not correct about it? The possibility of sending a message to the XPC > service for closing a notification that doesn't exist? > > My concern with the cache is that we don't know whether it'll be completely > correct, i.e. whether there are cases where the close event is used (or not > used) for the wrong reasons. > > Additionally, if we ever find a way to continue showing alerts while Chrome > isn't running, this is per definition going to be incorrect.
On 2016/10/11 15:25:41, Robert Sesek wrote: > On 2016/10/11 14:46:38, Peter Beverloo wrote: > > On 2016/10/11 14:41:08, Miguel Garcia wrote: > > > On 2016/10/11 14:37:49, Peter Beverloo wrote: > > > > > > > > > > https://codereview.chromium.org/2402303002/diff/40001/chrome/browser/notifica... > > > > File chrome/browser/notifications/notification_platform_bridge_mac.mm > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2402303002/diff/40001/chrome/browser/notifica... > > > > chrome/browser/notifications/notification_platform_bridge_mac.mm:430: > > > > base::scoped_nsobject<NSMutableSet> alertsDisplayed_; > > > > Instead of caching which alerts we still think are opened, why not try to > > > close > > > > a banner first (as you do in NotificationPlatformBridgeMac::Close), and > > always > > > > fall back to the NotificationRemoteDispatcher if that failed? > > > > > > > > It looks like that would remove the majority of complexity here. > > > > > > I thought about that but it just does not seems correct. "Complexity" here > is > > > keeping a set in sync which does not seem too bad. If you both feel strongly > > > I'll switch to that. > > > > What is not correct about it? The possibility of sending a message to the XPC > > service for closing a notification that doesn't exist? > > > > My concern with the cache is that we don't know whether it'll be completely > > correct, i.e. whether there are cases where the close event is used (or not > > used) for the wrong reasons. > > > > Additionally, if we ever find a way to continue showing alerts while Chrome > > isn't running, this is per definition going to be incorrect. > > I'm also concerned about this. In the original notification center integration I > did, we did end up having bugs around the OS not reliably notifying us when > notifications were closed after a period of time, so our cache got stale. Can you elaborate on this? That is certainly concerning? Why would this cache get stale?
https://codereview.chromium.org/2402303002/diff/20001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_mac.mm (right): https://codereview.chromium.org/2402303002/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac.mm:434: alertsDisplayed_.reset([[NSMutableSet alloc] init]); On 2016/10/11 11:55:41, Miguel Garcia wrote: > On 2016/10/10 18:22:47, Robert Sesek wrote: > > What is the purpose of this set? It looks like things are only ever added or > > removed from it, never consulted. > > It is consulted through isAlertDisplayed which is used in the ::Close( method to > figure out if what we should be closing is an alert or a banner. Sorry, missed that due to a find-in-page bug on Mac.
> That is certainly concerning? That was meant to be a statement (no ?) not a question. On 2016/10/11 15:28:15, Miguel Garcia wrote: > On 2016/10/11 15:25:41, Robert Sesek wrote: > > On 2016/10/11 14:46:38, Peter Beverloo wrote: > > > On 2016/10/11 14:41:08, Miguel Garcia wrote: > > > > On 2016/10/11 14:37:49, Peter Beverloo wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2402303002/diff/40001/chrome/browser/notifica... > > > > > File chrome/browser/notifications/notification_platform_bridge_mac.mm > > > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2402303002/diff/40001/chrome/browser/notifica... > > > > > chrome/browser/notifications/notification_platform_bridge_mac.mm:430: > > > > > base::scoped_nsobject<NSMutableSet> alertsDisplayed_; > > > > > Instead of caching which alerts we still think are opened, why not try > to > > > > close > > > > > a banner first (as you do in NotificationPlatformBridgeMac::Close), and > > > always > > > > > fall back to the NotificationRemoteDispatcher if that failed? > > > > > > > > > > It looks like that would remove the majority of complexity here. > > > > > > > > I thought about that but it just does not seems correct. "Complexity" here > > is > > > > keeping a set in sync which does not seem too bad. If you both feel > strongly > > > > I'll switch to that. > > > > > > What is not correct about it? The possibility of sending a message to the > XPC > > > service for closing a notification that doesn't exist? > > > > > > My concern with the cache is that we don't know whether it'll be completely > > > correct, i.e. whether there are cases where the close event is used (or not > > > used) for the wrong reasons. > > > > > > Additionally, if we ever find a way to continue showing alerts while Chrome > > > isn't running, this is per definition going to be incorrect. > > > > I'm also concerned about this. In the original notification center integration > I > > did, we did end up having bugs around the OS not reliably notifying us when > > notifications were closed after a period of time, so our cache got stale. > > Can you elaborate on this? That is certainly concerning? Why would this cache > get stale?
On 2016/10/11 15:28:15, Miguel Garcia wrote: > On 2016/10/11 15:25:41, Robert Sesek wrote: > > On 2016/10/11 14:46:38, Peter Beverloo wrote: > > > On 2016/10/11 14:41:08, Miguel Garcia wrote: > > > > On 2016/10/11 14:37:49, Peter Beverloo wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2402303002/diff/40001/chrome/browser/notifica... > > > > > File chrome/browser/notifications/notification_platform_bridge_mac.mm > > > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2402303002/diff/40001/chrome/browser/notifica... > > > > > chrome/browser/notifications/notification_platform_bridge_mac.mm:430: > > > > > base::scoped_nsobject<NSMutableSet> alertsDisplayed_; > > > > > Instead of caching which alerts we still think are opened, why not try > to > > > > close > > > > > a banner first (as you do in NotificationPlatformBridgeMac::Close), and > > > always > > > > > fall back to the NotificationRemoteDispatcher if that failed? > > > > > > > > > > It looks like that would remove the majority of complexity here. > > > > > > > > I thought about that but it just does not seems correct. "Complexity" here > > is > > > > keeping a set in sync which does not seem too bad. If you both feel > strongly > > > > I'll switch to that. > > > > > > What is not correct about it? The possibility of sending a message to the > XPC > > > service for closing a notification that doesn't exist? > > > > > > My concern with the cache is that we don't know whether it'll be completely > > > correct, i.e. whether there are cases where the close event is used (or not > > > used) for the wrong reasons. > > > > > > Additionally, if we ever find a way to continue showing alerts while Chrome > > > isn't running, this is per definition going to be incorrect. > > > > I'm also concerned about this. In the original notification center integration > I > > did, we did end up having bugs around the OS not reliably notifying us when > > notifications were closed after a period of time, so our cache got stale. > > Can you elaborate on this? That is certainly concerning? Why would this cache > get stale? I think this was a result of a bunch of notifications coming, the system aging the oldest ones out, but not notifying chrome.
Ok cache removed. PTAL https://codereview.chromium.org/2402303002/diff/40001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_mac.mm (right): https://codereview.chromium.org/2402303002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac.mm:430: base::scoped_nsobject<NSMutableSet> alertsDisplayed_; Given you both had concerns on the cache approach and the fact that your proposal certainly works I went ahead and implemented that instead. The cache is removed now. On 2016/10/11 14:37:49, Peter Beverloo wrote: > Instead of caching which alerts we still think are opened, why not try to close > a banner first (as you do in NotificationPlatformBridgeMac::Close), and always > fall back to the NotificationRemoteDispatcher if that failed? > > It looks like that would remove the majority of complexity here.
lgtm - are tests blocked on the swizzling CL? https://codereview.chromium.org/2402303002/diff/80001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_mac.mm (right): https://codereview.chromium.org/2402303002/diff/80001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac.mm:429: base::scoped_nsobject<NSMutableSet> alertsDisplayed_; delete (line 434 too)
Yes, once that one lands in whatever form we decide I'll increase coverage for clicks/close... On Wed, Oct 12, 2016 at 1:20 PM, <peter@chromium.org> wrote: > lgtm - are tests blocked on the swizzling CL? > > > https://codereview.chromium.org/2402303002/diff/80001/ > chrome/browser/notifications/notification_platform_bridge_mac.mm > File chrome/browser/notifications/notification_platform_bridge_mac.mm > (right): > > https://codereview.chromium.org/2402303002/diff/80001/ > chrome/browser/notifications/notification_platform_bridge_ > mac.mm#newcode429 > chrome/browser/notifications/notification_platform_bridge_mac.mm:429: > base::scoped_nsobject<NSMutableSet> alertsDisplayed_; > delete (line 434 too) > > https://codereview.chromium.org/2402303002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
LGTM w/ Peter's comment addressed too https://codereview.chromium.org/2402303002/diff/80001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_mac.mm (right): https://codereview.chromium.org/2402303002/diff/80001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac.mm:112: nit: remove extra blank line
https://codereview.chromium.org/2402303002/diff/80001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_mac.mm (right): https://codereview.chromium.org/2402303002/diff/80001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac.mm:112: On 2016/10/12 15:37:36, Robert Sesek wrote: > nit: remove extra blank line Done. https://codereview.chromium.org/2402303002/diff/80001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac.mm:429: base::scoped_nsobject<NSMutableSet> alertsDisplayed_; On 2016/10/12 12:20:38, Peter Beverloo wrote: > delete (line 434 too) Done.
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, rsesek@chromium.org Link to the patchset: https://codereview.chromium.org/2402303002/#ps100001 (title: "review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Implement the ability to close alerts programatically. BUG=571056 ========== to ========== Implement the ability to close alerts programatically. BUG=571056 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Implement the ability to close alerts programatically. BUG=571056 ========== to ========== Implement the ability to close alerts programatically. BUG=571056 Committed: https://crrev.com/470c5e56f7afdfa2552babe34b484c2808bfa1ef Cr-Commit-Position: refs/heads/master@{#425559} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/470c5e56f7afdfa2552babe34b484c2808bfa1ef Cr-Commit-Position: refs/heads/master@{#425559} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
