Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2428573002:
Add test for closing 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, mac-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd test for closing native notifications
BUG=571056
Committed: https://crrev.com/9f514f47362c2e82bccd7460c213b0271c66f110
Cr-Commit-Position: refs/heads/master@{#426445}
Patch Set 1 : - #
Total comments: 13
Patch Set 2 : review #Patch Set 3 : fix compile #Messages
Total messages: 26 (14 generated)
Patchset #1 (id:1) has been deleted
miguelg@chromium.org changed reviewers: + peter@chromium.org, rsesek@chromium.org
https://codereview.chromium.org/2428573002/diff/20001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm (right): https://codereview.chromium.org/2428573002/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:239: base::mac::ScopedObjCClassSwizzler swizzler2( This is needed because base::mac::ScopedObjCClassSwizzler does not offer a way to swizzle more than one method at once. It works but it's a bit quirky. Suggestions welcome (including switching back to doing my own swizzling if needed).
lgtm, but consider using this as an opportunity to also test the cases where the
values are different:
deliveredNotifications = @[ BuildNotification() ]
::Close("random_value", "notification_id") // does nothing
::Close("profile_id", "random_value") // does nothing
::Close("profile_id", "notification_id") // closes
We care as much about those as we do about the success case :).
https://codereview.chromium.org/2428573002/diff/20001/chrome/browser/notifica...
File chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm
(right):
https://codereview.chromium.org/2428573002/diff/20001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:239:
base::mac::ScopedObjCClassSwizzler swizzler2(
On 2016/10/17 14:17:43, Miguel Garcia wrote:
> This is needed because base::mac::ScopedObjCClassSwizzler does not offer a
way
> to swizzle more than one method at once. It works but it's a bit quirky.
> Suggestions welcome (including switching back to doing my own swizzling if
> needed).
>
My only comment is that they could have more descriptive names -
delivered_notifications_swizzler
remove_delivered_notification_swizzler
(Neither requires additional wrapping.)
https://codereview.chromium.org/2428573002/diff/20001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:244:
new NotificationPlatformBridgeMac(notification_center));
auto bridge = base::MakeUnique<NotificationPlatformBridgeMac>(
notification_center);
https://codereview.chromium.org/2428573002/diff/20001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:245:
bridge->Close("profileId", "notificationId");
You're using profile_id and notification_id elsewhere in this file.
LGTM https://codereview.chromium.org/2428573002/diff/20001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm (right): https://codereview.chromium.org/2428573002/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:24: NSUserNotification* BuildNotification() { This could be on NotificationPlatformBridgeMacTest as well. https://codereview.chromium.org/2428573002/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:41: numberWithInt:NotificationCommon::PERSISTENT]]; Does @() work ? https://codereview.chromium.org/2428573002/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:239: base::mac::ScopedObjCClassSwizzler swizzler2( On 2016/10/17 14:17:43, Miguel Garcia wrote: > This is needed because base::mac::ScopedObjCClassSwizzler does not offer a way > to swizzle more than one method at once. It works but it's a bit quirky. > Suggestions welcome (including switching back to doing my own swizzling if > needed). > I think this is fine, since hopefully it'll be temporary once OCMock gets rolled.
review
The CQ bit was checked by miguelg@chromium.org to run a CQ dry run
The CQ bit was unchecked by miguelg@chromium.org
The CQ bit was checked by miguelg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org, peter@chromium.org Link to the patchset: https://codereview.chromium.org/2428573002/#ps40001 (title: "review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by miguelg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by miguelg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org, peter@chromium.org Link to the patchset: https://codereview.chromium.org/2428573002/#ps60001 (title: "fix compile")
https://codereview.chromium.org/2428573002/diff/20001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm (right): https://codereview.chromium.org/2428573002/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:24: NSUserNotification* BuildNotification() { On 2016/10/19 18:22:49, Robert Sesek wrote: > This could be on NotificationPlatformBridgeMacTest as well. Acknowledged. https://codereview.chromium.org/2428573002/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:41: numberWithInt:NotificationCommon::PERSISTENT]]; On 2016/10/19 18:22:49, Robert Sesek wrote: > Does @() work ? Done. https://codereview.chromium.org/2428573002/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:239: base::mac::ScopedObjCClassSwizzler swizzler2( renamed On 2016/10/17 14:27:14, Peter Beverloo wrote: > On 2016/10/17 14:17:43, Miguel Garcia wrote: > > This is needed because base::mac::ScopedObjCClassSwizzler does not offer a > way > > to swizzle more than one method at once. It works but it's a bit quirky. > > Suggestions welcome (including switching back to doing my own swizzling if > > needed). > > > > My only comment is that they could have more descriptive names - > > delivered_notifications_swizzler > remove_delivered_notification_swizzler > > (Neither requires additional wrapping.) https://codereview.chromium.org/2428573002/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:239: base::mac::ScopedObjCClassSwizzler swizzler2( On 2016/10/19 18:22:49, Robert Sesek wrote: > On 2016/10/17 14:17:43, Miguel Garcia wrote: > > This is needed because base::mac::ScopedObjCClassSwizzler does not offer a > way > > to swizzle more than one method at once. It works but it's a bit quirky. > > Suggestions welcome (including switching back to doing my own swizzling if > > needed). > > > > I think this is fine, since hopefully it'll be temporary once OCMock gets > rolled. SG going with that then https://codereview.chromium.org/2428573002/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:244: new NotificationPlatformBridgeMac(notification_center)); On 2016/10/17 14:27:14, Peter Beverloo wrote: > auto bridge = base::MakeUnique<NotificationPlatformBridgeMac>( > notification_center); Acknowledged. https://codereview.chromium.org/2428573002/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:245: bridge->Close("profileId", "notificationId"); On 2016/10/17 14:27:14, Peter Beverloo wrote: > You're using profile_id and notification_id elsewhere in this file. Done.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Also note the change in the last diff, when compiling in the bots I got the
following error
error: type arguments cannot be applied to non-parameterized class 'NSArray'
- (NSArray<NSUserNotification*>*)expectationsDeliveredNotification {
I don't get this when compiling locally (and the error itself seems strange
since NSArray is a paremetrized class AFAIK).
I changed the definitions to - (NSArray*)expectationsDeliveredNotification and
all bots seem happy now but I'd like to understand why this was a problem in the
first place.
On 2016/10/20 10:24:45, Miguel Garcia wrote:
>
https://codereview.chromium.org/2428573002/diff/20001/chrome/browser/notifica...
> File chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm
> (right):
>
>
https://codereview.chromium.org/2428573002/diff/20001/chrome/browser/notifica...
> chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:24:
> NSUserNotification* BuildNotification() {
> On 2016/10/19 18:22:49, Robert Sesek wrote:
> > This could be on NotificationPlatformBridgeMacTest as well.
>
> Acknowledged.
>
>
https://codereview.chromium.org/2428573002/diff/20001/chrome/browser/notifica...
> chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:41:
> numberWithInt:NotificationCommon::PERSISTENT]];
> On 2016/10/19 18:22:49, Robert Sesek wrote:
> > Does @() work ?
>
> Done.
>
>
https://codereview.chromium.org/2428573002/diff/20001/chrome/browser/notifica...
> chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:239:
> base::mac::ScopedObjCClassSwizzler swizzler2(
> renamed
>
> On 2016/10/17 14:27:14, Peter Beverloo wrote:
> > On 2016/10/17 14:17:43, Miguel Garcia wrote:
> > > This is needed because base::mac::ScopedObjCClassSwizzler does not offer
a
> > way
> > > to swizzle more than one method at once. It works but it's a bit quirky.
> > > Suggestions welcome (including switching back to doing my own swizzling if
> > > needed).
> > >
> >
> > My only comment is that they could have more descriptive names -
> >
> > delivered_notifications_swizzler
> > remove_delivered_notification_swizzler
> >
> > (Neither requires additional wrapping.)
>
>
https://codereview.chromium.org/2428573002/diff/20001/chrome/browser/notifica...
> chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:239:
> base::mac::ScopedObjCClassSwizzler swizzler2(
> On 2016/10/19 18:22:49, Robert Sesek wrote:
> > On 2016/10/17 14:17:43, Miguel Garcia wrote:
> > > This is needed because base::mac::ScopedObjCClassSwizzler does not offer
a
> > way
> > > to swizzle more than one method at once. It works but it's a bit quirky.
> > > Suggestions welcome (including switching back to doing my own swizzling if
> > > needed).
> > >
> >
> > I think this is fine, since hopefully it'll be temporary once OCMock gets
> > rolled.
>
> SG going with that then
>
>
https://codereview.chromium.org/2428573002/diff/20001/chrome/browser/notifica...
> chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:244:
> new NotificationPlatformBridgeMac(notification_center));
> On 2016/10/17 14:27:14, Peter Beverloo wrote:
> > auto bridge = base::MakeUnique<NotificationPlatformBridgeMac>(
> > notification_center);
>
> Acknowledged.
>
>
https://codereview.chromium.org/2428573002/diff/20001/chrome/browser/notifica...
> chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:245:
> bridge->Close("profileId", "notificationId");
> On 2016/10/17 14:27:14, Peter Beverloo wrote:
> > You're using profile_id and notification_id elsewhere in this file.
>
> Done.
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Add test for closing native notifications BUG=571056 ========== to ========== Add test for closing native notifications BUG=571056 Committed: https://crrev.com/9f514f47362c2e82bccd7460c213b0271c66f110 Cr-Commit-Position: refs/heads/master@{#426445} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/9f514f47362c2e82bccd7460c213b0271c66f110 Cr-Commit-Position: refs/heads/master@{#426445} |
