Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2390153005:
Implement support for closing mac native notifications (Closed)
|
Created:
4 years, 2 months ago by Miguel Garcia Modified:
4 years, 2 months ago CC:
chromium-reviews, Peter Beverloo, mlamouri+watch-notifications_chromium.org, awdf+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement support for closing mac native notifications
BUG=571056
Committed: https://crrev.com/fcfa6816d06cc9e4f1a187ab261f45395a81bffa
Cr-Commit-Position: refs/heads/master@{#425336}
Patch Set 1 #Patch Set 2 : set upstream to the right diff #
Total comments: 4
Patch Set 3 : review #
Total comments: 12
Patch Set 4 : review #
Total comments: 4
Patch Set 5 : review+small refactor of the builder #
Total comments: 4
Patch Set 6 : review #
Total comments: 7
Patch Set 7 : review #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 32 (12 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...
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
miguelg@chromium.org changed reviewers: + peter@chromium.org, rsesek@chromium.org
Caveats: Uses a private API Depends on https://chromiumcodereview.appspot.com/2070903002/ The first patch is the full thing including the dependent patch The second patch is just the diffs.
Btw, these CLs seem to get mailed from chromiumcodereview.appspot.com rather than the more canonical codereview.chromium.org. Not sure if that's intentional. https://codereview.chromium.org/2390153005/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/notifications/notification_private_mac.h (right): https://codereview.chromium.org/2390153005/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/notifications/notification_private_mac.h:13: @protocol I don't think you need this file at all (nothing is using this protocol at the moment). I'd just move the comment to the implementation where it's used. https://codereview.chromium.org/2390153005/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/notifications/notification_response_builder_mac_unittest.mm (right): https://codereview.chromium.org/2390153005/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/notifications/notification_response_builder_mac_unittest.mm:36: NSUserNotificationActivationTypeContentsClicked] You should be able to just use @(NSUserNotificationActivationTypeContentsClicked) instead of the more verbose NSNumber API.
https://codereview.chromium.org/2390153005/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/notifications/notification_private_mac.h (right): https://codereview.chromium.org/2390153005/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/notifications/notification_private_mac.h:13: @protocol On 2016/10/05 20:33:32, Robert Sesek wrote: > I don't think you need this file at all (nothing is using this protocol at the > moment). I'd just move the comment to the implementation where it's used. I see, done then https://codereview.chromium.org/2390153005/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/notifications/notification_response_builder_mac_unittest.mm (right): https://codereview.chromium.org/2390153005/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/notifications/notification_response_builder_mac_unittest.mm:36: NSUserNotificationActivationTypeContentsClicked] On 2016/10/05 20:33:32, Robert Sesek wrote: > You should be able to just use > @(NSUserNotificationActivationTypeContentsClicked) instead of the more verbose > NSNumber API. Ah great, changed in all the tests.
>Btw, these CLs seem to get mailed from chromiumcodereview.appspot.com rather >than the more canonical codereview.chromium.org. Not sure if that's intentional. I think it just depends on the url you are in when you click publish. Switched to the more conventional one now. On 2016/10/05 21:58:37, Miguel Garcia wrote: > https://codereview.chromium.org/2390153005/diff/60001/chrome/browser/ui/cocoa... > File chrome/browser/ui/cocoa/notifications/notification_private_mac.h (right): > > https://codereview.chromium.org/2390153005/diff/60001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/notifications/notification_private_mac.h:13: @protocol > On 2016/10/05 20:33:32, Robert Sesek wrote: > > I don't think you need this file at all (nothing is using this protocol at the > > moment). I'd just move the comment to the implementation where it's used. > > I see, done then > > https://codereview.chromium.org/2390153005/diff/60001/chrome/browser/ui/cocoa... > File > chrome/browser/ui/cocoa/notifications/notification_response_builder_mac_unittest.mm > (right): > > https://codereview.chromium.org/2390153005/diff/60001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/notifications/notification_response_builder_mac_unittest.mm:36: > NSUserNotificationActivationTypeContentsClicked] > On 2016/10/05 20:33:32, Robert Sesek wrote: > > You should be able to just use > > @(NSUserNotificationActivationTypeContentsClicked) instead of the more verbose > > NSNumber API. > > Ah great, changed in all the tests.
rs lgtm Is there any way to test a show -> get -> close -> get sequence hitting for a more integration-y level test? https://codereview.chromium.org/2390153005/diff/80001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_mac.mm (right): https://codereview.chromium.org/2390153005/diff/80001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac.mm:382: // Emmitted when a user clicks the "Close" Button in the notification. Emmitted -> Emitted Button -> button https://codereview.chromium.org/2390153005/diff/80001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac.mm:383: // It not is emmitted if the notification is closed from the notification emmitted -> emitted https://codereview.chromium.org/2390153005/diff/80001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac.mm:384: // center or if the app is not running by the time the Close button is by the time -> at the time https://codereview.chromium.org/2390153005/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/notifications/BUILD.gn (right): https://codereview.chromium.org/2390153005/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/notifications/BUILD.gn:52: "notification_private_mac.h", delete https://codereview.chromium.org/2390153005/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/notifications/notification_response_builder_mac.mm (right): https://codereview.chromium.org/2390153005/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/notifications/notification_response_builder_mac.mm:49: // Closed notifications are not activated. are not -> have not been? https://codereview.chromium.org/2390153005/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/notifications/notification_response_builder_mac_unittest.mm (right): https://codereview.chromium.org/2390153005/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/notifications/notification_response_builder_mac_unittest.mm:33: // was clicked. nit: I don't think this comment (and the other ones) add a lot of value. Setting "activation type" to "contents clicked", which you get by reading the code, is obvious enough. Up to you.
thanks for the review! PTAL https://codereview.chromium.org/2390153005/diff/80001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_mac.mm (right): https://codereview.chromium.org/2390153005/diff/80001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac.mm:382: // Emmitted when a user clicks the "Close" Button in the notification. On 2016/10/06 14:35:18, Peter Beverloo wrote: > Emmitted -> Emitted > Button -> button Done. https://codereview.chromium.org/2390153005/diff/80001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac.mm:383: // It not is emmitted if the notification is closed from the notification On 2016/10/06 14:35:18, Peter Beverloo wrote: > emmitted -> emitted Done. https://codereview.chromium.org/2390153005/diff/80001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac.mm:384: // center or if the app is not running by the time the Close button is On 2016/10/06 14:35:18, Peter Beverloo wrote: > by the time -> at the time Done. https://codereview.chromium.org/2390153005/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/notifications/BUILD.gn (right): https://codereview.chromium.org/2390153005/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/notifications/BUILD.gn:52: "notification_private_mac.h", On 2016/10/06 14:35:19, Peter Beverloo wrote: > delete Done. https://codereview.chromium.org/2390153005/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/notifications/notification_response_builder_mac.mm (right): https://codereview.chromium.org/2390153005/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/notifications/notification_response_builder_mac.mm:49: // Closed notifications are not activated. On 2016/10/06 14:35:19, Peter Beverloo wrote: > are not -> have not been? Well they are never activated so I think this is correct. https://codereview.chromium.org/2390153005/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/notifications/notification_response_builder_mac_unittest.mm (right): https://codereview.chromium.org/2390153005/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/notifications/notification_response_builder_mac_unittest.mm:33: // was clicked. On 2016/10/06 14:35:19, Peter Beverloo wrote: > nit: I don't think this comment (and the other ones) add a lot of value. Setting > "activation type" to "contents clicked", which you get by reading the code, is > obvious enough. Up to you. I rather leave it, I totally hate repeating it on every test though. I do think it's important to mention that this is a test artefact and that in real life the OS does it for us.
Friendly ping on this one Robert. On 2016/10/07 13:42:24, Miguel Garcia wrote: > thanks for the review! PTAL > > https://codereview.chromium.org/2390153005/diff/80001/chrome/browser/notifica... > File chrome/browser/notifications/notification_platform_bridge_mac.mm (right): > > https://codereview.chromium.org/2390153005/diff/80001/chrome/browser/notifica... > chrome/browser/notifications/notification_platform_bridge_mac.mm:382: // > Emmitted when a user clicks the "Close" Button in the notification. > On 2016/10/06 14:35:18, Peter Beverloo wrote: > > Emmitted -> Emitted > > Button -> button > > Done. > > https://codereview.chromium.org/2390153005/diff/80001/chrome/browser/notifica... > chrome/browser/notifications/notification_platform_bridge_mac.mm:383: // It not > is emmitted if the notification is closed from the notification > On 2016/10/06 14:35:18, Peter Beverloo wrote: > > emmitted -> emitted > > Done. > > https://codereview.chromium.org/2390153005/diff/80001/chrome/browser/notifica... > chrome/browser/notifications/notification_platform_bridge_mac.mm:384: // center > or if the app is not running by the time the Close button is > On 2016/10/06 14:35:18, Peter Beverloo wrote: > > by the time -> at the time > > Done. > > https://codereview.chromium.org/2390153005/diff/80001/chrome/browser/ui/cocoa... > File chrome/browser/ui/cocoa/notifications/BUILD.gn (right): > > https://codereview.chromium.org/2390153005/diff/80001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/notifications/BUILD.gn:52: "notification_private_mac.h", > On 2016/10/06 14:35:19, Peter Beverloo wrote: > > delete > > Done. > > https://codereview.chromium.org/2390153005/diff/80001/chrome/browser/ui/cocoa... > File chrome/browser/ui/cocoa/notifications/notification_response_builder_mac.mm > (right): > > https://codereview.chromium.org/2390153005/diff/80001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/notifications/notification_response_builder_mac.mm:49: > // Closed notifications are not activated. > On 2016/10/06 14:35:19, Peter Beverloo wrote: > > are not -> have not been? > > Well they are never activated so I think this is correct. > > https://codereview.chromium.org/2390153005/diff/80001/chrome/browser/ui/cocoa... > File > chrome/browser/ui/cocoa/notifications/notification_response_builder_mac_unittest.mm > (right): > > https://codereview.chromium.org/2390153005/diff/80001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/notifications/notification_response_builder_mac_unittest.mm:33: > // was clicked. > On 2016/10/06 14:35:19, Peter Beverloo wrote: > > nit: I don't think this comment (and the other ones) add a lot of value. > Setting > > "activation type" to "contents clicked", which you get by reading the code, is > > obvious enough. Up to you. > > I rather leave it, I totally hate repeating it on every test though. I do think > it's important to mention > that this is a test artefact and that in real life the OS does it for us.
Sorry, missed this one yesterday. https://codereview.chromium.org/2390153005/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/notifications/notification_response_builder_mac_unittest.mm (right): https://codereview.chromium.org/2390153005/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/notifications/notification_response_builder_mac_unittest.mm:208: [builder setIcon:[NSImage imageNamed:@"NSApplicationIcon"]]; Is this non-nil in tests? I don't think unit_tests has an icon (though it may be picking up the generic executable). You could use NSImageNameFolder to be safe. If it is non-nil, this can also be the constant NSImageNameApplicationIcon. https://codereview.chromium.org/2390153005/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/notifications/notification_response_builder_mac_unittest.mm:214: numberWithInt:NotificationCommon::PERSISTENT]]; Does @() work here too?
PTAL I crossed my personal boundary on the number of times I am willing to copy&paste stuff so I refactored the common pieces of the builder in the test into a static method. Hope it's ok. https://codereview.chromium.org/2390153005/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/notifications/notification_response_builder_mac_unittest.mm (right): https://codereview.chromium.org/2390153005/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/notifications/notification_response_builder_mac_unittest.mm:208: [builder setIcon:[NSImage imageNamed:@"NSApplicationIcon"]]; On 2016/10/11 14:19:32, Robert Sesek wrote: > Is this non-nil in tests? I don't think unit_tests has an icon (though it may be > picking up the generic executable). You could use NSImageNameFolder to be safe. > If it is non-nil, this can also be the constant NSImageNameApplicationIcon. Yeah this is non null in tests, I know because the in the swizzle tests where I fail to Swizzle I get a notification with an icon. Changed to the constant instead. https://codereview.chromium.org/2390153005/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/notifications/notification_response_builder_mac_unittest.mm:214: numberWithInt:NotificationCommon::PERSISTENT]]; On 2016/10/11 14:19:32, Robert Sesek wrote: > Does @() work here too? Done.
https://codereview.chromium.org/2390153005/diff/120001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/notifications/notification_response_builder_mac_unittest.mm (right): https://codereview.chromium.org/2390153005/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/notifications/notification_response_builder_mac_unittest.mm:19: + (NotificationBuilder*)newTestBuilder { Splitting this out makes sense, but because ObjC is a flat, global namespace, I probably wouldn't use an ObjC class for this. Either just a plain static function int his file, or add it as a method to NotificationResponseBuilderMacTest and switch to TEST_F (I have a slight preference for the latter in my own code). https://codereview.chromium.org/2390153005/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/notifications/notification_response_builder_mac_unittest.mm:19: + (NotificationBuilder*)newTestBuilder { This can return a scoped_nsobject<> too.
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/2390153005/diff/120001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/notifications/notification_response_builder_mac_unittest.mm (right): https://codereview.chromium.org/2390153005/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/notifications/notification_response_builder_mac_unittest.mm:19: + (NotificationBuilder*)newTestBuilder { On 2016/10/12 15:34:30, Robert Sesek wrote: > Splitting this out makes sense, but because ObjC is a flat, global namespace, I > probably wouldn't use an ObjC class for this. Either just a plain static > function int his file, or add it as a method to > NotificationResponseBuilderMacTest and switch to TEST_F (I have a slight > preference for the latter in my own code). Switched to a protected method in NotificationResponseBuilderMacTest (and TEST_F as a result) https://codereview.chromium.org/2390153005/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/notifications/notification_response_builder_mac_unittest.mm:19: + (NotificationBuilder*)newTestBuilder { On 2016/10/12 15:34:30, Robert Sesek wrote: > This can return a scoped_nsobject<> too. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2390153005/diff/140001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/notifications/notification_response_builder_mac_unittest.mm (right): https://codereview.chromium.org/2390153005/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/notifications/notification_response_builder_mac_unittest.mm:14: @interface TestBuilder : NSObject Remove this? https://codereview.chromium.org/2390153005/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/notifications/notification_response_builder_mac_unittest.mm:39: base::scoped_nsobject<NotificationBuilder> testBuilder(); naming: TestBuilder() (or maybe even NewTestBuilder()). https://codereview.chromium.org/2390153005/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/notifications/notification_response_builder_mac_unittest.mm:43: NotificationResponseBuilderMacTest::testBuilder() { I generally just inline these into the class declaration. https://codereview.chromium.org/2390153005/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/notifications/notification_response_builder_mac_unittest.mm:66: forKey:@"_activationType"]; We may want to pull this out into notification_constants.h (in another CL?)
https://codereview.chromium.org/2390153005/diff/140001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/notifications/notification_response_builder_mac_unittest.mm (right): https://codereview.chromium.org/2390153005/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/notifications/notification_response_builder_mac_unittest.mm:39: base::scoped_nsobject<NotificationBuilder> testBuilder(); On 2016/10/13 16:50:36, Robert Sesek wrote: > naming: TestBuilder() (or maybe even NewTestBuilder()). Done. https://codereview.chromium.org/2390153005/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/notifications/notification_response_builder_mac_unittest.mm:43: NotificationResponseBuilderMacTest::testBuilder() { On 2016/10/13 16:50:37, Robert Sesek wrote: > I generally just inline these into the class declaration. Done. https://codereview.chromium.org/2390153005/diff/140001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/notifications/notification_response_builder_mac_unittest.mm:66: forKey:@"_activationType"]; On 2016/10/13 16:50:36, Robert Sesek wrote: > We may want to pull this out into notification_constants.h (in another CL?) Happy to consider it in another CL but keep in mind that this is only accessed in tests.
LGTM
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 Link to the patchset: https://codereview.chromium.org/2390153005/#ps160001 (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.
Committed patchset #7 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Implement support for closing mac native notifications BUG=571056 ========== to ========== Implement support for closing mac native notifications BUG=571056 Committed: https://crrev.com/fcfa6816d06cc9e4f1a187ab261f45395a81bffa Cr-Commit-Position: refs/heads/master@{#425336} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/fcfa6816d06cc9e4f1a187ab261f45395a81bffa Cr-Commit-Position: refs/heads/master@{#425336}
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:160001) has been created in https://codereview.chromium.org/2418183002/ by miguelg@chromium.org. The reason for reverting is: Broke one of the waterfall builders. http://build.chromium.org/p/chromium/builders/Mac/builds/20166/steps/compile/.... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
