Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2479143003:
Add tests for the XPC client. (Closed)
|
Created:
4 years, 1 month ago by Miguel Garcia Modified:
4 years, 1 month 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 tests for the XPC client.
Required a small refactoring of the XPC client in order to make it
stub-able.
BUG=571056
Committed: https://crrev.com/5c55e9251f0448c522d6386da4ff95bb0708a6e3
Cr-Commit-Position: refs/heads/master@{#433835}
Patch Set 1 #
Total comments: 13
Patch Set 2 : review #
Total comments: 2
Patch Set 3 : review #
Total comments: 4
Patch Set 4 : review #
Total comments: 6
Patch Set 5 : review #Patch Set 6 : rename alert_dispatcher.h to alert_dispatcher_mac.h and add it to the BUILD.gn file #Messages
Total messages: 40 (25 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...
miguelg@chromium.org changed reviewers: + peter@chromium.org, rsesek@chromium.org
https://codereview.chromium.org/2479143003/diff/1/chrome/browser/notification... File chrome/browser/notifications/notification_platform_bridge_mac.h (right): https://codereview.chromium.org/2479143003/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_platform_bridge_mac.h:65: base::scoped_nsobject<id> alert_dispatcher_; I have not found a better way to define that the what's inside the ns object should be a protocol of type AlertDispatcher.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2479143003/diff/1/chrome/browser/notification... File chrome/browser/notifications/alert_dispatcher.h (right): https://codereview.chromium.org/2479143003/diff/1/chrome/browser/notification... chrome/browser/notifications/alert_dispatcher.h:9: @protocol AlertDispatcher<NSObject> Need to #import Foundation.h for this protocol. https://codereview.chromium.org/2479143003/diff/1/chrome/browser/notification... File chrome/browser/notifications/notification_platform_bridge_mac.h (right): https://codereview.chromium.org/2479143003/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_platform_bridge_mac.h:30: id<AlertDispatcher> alert_dispatcher); What's the ownership of the alert_dispatcher here? Comment for weak or use a scoper and std::move. https://codereview.chromium.org/2479143003/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_platform_bridge_mac.h:65: base::scoped_nsobject<id> alert_dispatcher_; On 2016/11/07 14:24:14, Miguel Garcia wrote: > I have not found a better way to define that the what's inside the ns object > should be a protocol of type AlertDispatcher. scoped_nsprotocol<id<AlertDispatcher>> ? https://codereview.chromium.org/2479143003/diff/1/chrome/browser/notification... File chrome/browser/notifications/stub_alert_dispatcher_mac.h (right): https://codereview.chromium.org/2479143003/diff/1/chrome/browser/notification... chrome/browser/notifications/stub_alert_dispatcher_mac.h:21: // stub specific methods nit: Comments should have proper capitalization and punctuation.
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/2479143003/diff/1/chrome/browser/notification... File chrome/browser/notifications/alert_dispatcher.h (right): https://codereview.chromium.org/2479143003/diff/1/chrome/browser/notification... chrome/browser/notifications/alert_dispatcher.h:9: @protocol AlertDispatcher<NSObject> On 2016/11/07 17:51:18, Robert Sesek wrote: > Need to #import Foundation.h for this protocol. Done. https://codereview.chromium.org/2479143003/diff/1/chrome/browser/notification... File chrome/browser/notifications/notification_platform_bridge_mac.h (right): https://codereview.chromium.org/2479143003/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_platform_bridge_mac.h:30: id<AlertDispatcher> alert_dispatcher); On 2016/11/07 17:51:18, Robert Sesek wrote: > What's the ownership of the alert_dispatcher here? Comment for weak or use a > scoper and std::move. So it is owned by the NotificationPlatformBridgeMac object so I guess it's strong then? Weak would be if it wasn't owned by the class where it is injected right? https://codereview.chromium.org/2479143003/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_platform_bridge_mac.h:65: base::scoped_nsobject<id> alert_dispatcher_; On 2016/11/07 17:51:18, Robert Sesek wrote: > On 2016/11/07 14:24:14, Miguel Garcia wrote: > > I have not found a better way to define that the what's inside the ns object > > should be a protocol of type AlertDispatcher. > > scoped_nsprotocol<id<AlertDispatcher>> ? That seems to expect an object of type id<AlertDispatcher>* I code searched a bit and the very few instances in chrome seem to go with what I have https://cs.chromium.org/search/?q=%22scoped_nsobject%3Cid%22+file:chrome/+-fi... https://codereview.chromium.org/2479143003/diff/1/chrome/browser/notification... File chrome/browser/notifications/stub_alert_dispatcher_mac.h (right): https://codereview.chromium.org/2479143003/diff/1/chrome/browser/notification... chrome/browser/notifications/stub_alert_dispatcher_mac.h:21: // stub specific methods On 2016/11/07 17:51:18, Robert Sesek wrote: > nit: Comments should have proper capitalization and punctuation. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2479143003/diff/1/chrome/browser/notification... File chrome/browser/notifications/notification_platform_bridge_mac.h (right): https://codereview.chromium.org/2479143003/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_platform_bridge_mac.h:30: id<AlertDispatcher> alert_dispatcher); On 2016/11/10 12:46:52, Miguel Garcia wrote: > On 2016/11/07 17:51:18, Robert Sesek wrote: > > What's the ownership of the alert_dispatcher here? Comment for weak or use a > > scoper and std::move. > > So it is owned by the NotificationPlatformBridgeMac object so I guess it's > strong then? Weak would be if it wasn't owned by the class where it is injected > right? Right. https://codereview.chromium.org/2479143003/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_platform_bridge_mac.h:65: base::scoped_nsobject<id> alert_dispatcher_; On 2016/11/10 12:46:52, Miguel Garcia wrote: > On 2016/11/07 17:51:18, Robert Sesek wrote: > > On 2016/11/07 14:24:14, Miguel Garcia wrote: > > > I have not found a better way to define that the what's inside the ns object > > > should be a protocol of type AlertDispatcher. > > > > scoped_nsprotocol<id<AlertDispatcher>> ? > > That seems to expect an object of type id<AlertDispatcher>* > I code searched a bit and the very few instances in chrome seem to go with what > I have > https://cs.chromium.org/search/?q=%22scoped_nsobject%3Cid%22+file:chrome/+-fi... > > Did you try my suggestion? It should work: https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/constrained_wind... https://codereview.chromium.org/2479143003/diff/20001/chrome/browser/notifica... File chrome/browser/notifications/stub_alert_dispatcher_mac.h (right): https://codereview.chromium.org/2479143003/diff/20001/chrome/browser/notifica... chrome/browser/notifications/stub_alert_dispatcher_mac.h:22: - (NSArray*)getAlerts; Getters don't have a "get" prefix, so just "alerts" here.
https://codereview.chromium.org/2479143003/diff/1/chrome/browser/notification... File chrome/browser/notifications/notification_platform_bridge_mac.h (right): https://codereview.chromium.org/2479143003/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_platform_bridge_mac.h:30: id<AlertDispatcher> alert_dispatcher); On 2016/11/10 23:52:41, Robert Sesek wrote: > On 2016/11/10 12:46:52, Miguel Garcia wrote: > > On 2016/11/07 17:51:18, Robert Sesek wrote: > > > What's the ownership of the alert_dispatcher here? Comment for weak or use a > > > scoper and std::move. > > > > So it is owned by the NotificationPlatformBridgeMac object so I guess it's > > strong then? Weak would be if it wasn't owned by the class where it is > injected > > right? > > Right. Done. https://codereview.chromium.org/2479143003/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_platform_bridge_mac.h:65: base::scoped_nsobject<id> alert_dispatcher_; I had misread your suggestion (scoped_nsobject vs scoped_nsprotocol) apologies for the confusion. Done now. On 2016/11/10 23:52:41, Robert Sesek wrote: > On 2016/11/10 12:46:52, Miguel Garcia wrote: > > On 2016/11/07 17:51:18, Robert Sesek wrote: > > > On 2016/11/07 14:24:14, Miguel Garcia wrote: > > > > I have not found a better way to define that the what's inside the ns > object > > > > should be a protocol of type AlertDispatcher. > > > > > > scoped_nsprotocol<id<AlertDispatcher>> ? > > > > That seems to expect an object of type id<AlertDispatcher>* > > I code searched a bit and the very few instances in chrome seem to go with > what > > I have > > > https://cs.chromium.org/search/?q=%22scoped_nsobject%3Cid%22+file:chrome/+-fi... > > > > > > Did you try my suggestion? It should work: > > https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/constrained_wind... https://codereview.chromium.org/2479143003/diff/20001/chrome/browser/notifica... File chrome/browser/notifications/stub_alert_dispatcher_mac.h (right): https://codereview.chromium.org/2479143003/diff/20001/chrome/browser/notifica... chrome/browser/notifications/stub_alert_dispatcher_mac.h:22: - (NSArray*)getAlerts; On 2016/11/10 23:52:41, Robert Sesek wrote: > Getters don't have a "get" prefix, so just "alerts" here. Done.
https://codereview.chromium.org/2479143003/diff/40001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_mac.h (right): https://codereview.chromium.org/2479143003/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac.h:65: // id<AlertDispatcher> Can remove now. https://codereview.chromium.org/2479143003/diff/40001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm (right): https://codereview.chromium.org/2479143003/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:302: StubAlertDispatcher* alert_dispatcher = [[StubAlertDispatcher alloc] init]; The ownership here is a bit odd. I'd expect this to be in a scoped_nsobject if it were local. But it's going to be owned now by the Bridge, so I'd expect it to also be in a scoped_nsobject and std::move'd to the Bridge. But it still needs to be referenced below. I think to make this a little more conventional, place the alert_dispatcher in a scoped_nsobject, have Bridge take just a pointer to it, and then the Bridge can -retain it and place it in a scoped_nsprotocol. OR Make NotificationPlatformBridgeMac() take in a scoped_nsprotocol and keep a weak pointer to it here.
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/2479143003/diff/40001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_mac.h (right): https://codereview.chromium.org/2479143003/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac.h:65: // id<AlertDispatcher> On 2016/11/11 19:21:16, Robert Sesek wrote: > Can remove now. Done. https://codereview.chromium.org/2479143003/diff/40001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm (right): https://codereview.chromium.org/2479143003/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:302: StubAlertDispatcher* alert_dispatcher = [[StubAlertDispatcher alloc] init]; Ok so after our chat I went with retaining both the notification center and the alert dispatcher in a scoped_ns{object,protocol}. On 2016/11/11 19:21:16, Robert Sesek wrote: > The ownership here is a bit odd. I'd expect this to be in a scoped_nsobject if > it were local. But it's going to be owned now by the Bridge, so I'd expect it to > also be in a scoped_nsobject and std::move'd to the Bridge. But it still needs > to be referenced below. > > I think to make this a little more conventional, place the alert_dispatcher in a > scoped_nsobject, have Bridge take just a pointer to it, and then the Bridge can > -retain it and place it in a scoped_nsprotocol. > > OR > > Make NotificationPlatformBridgeMac() take in a scoped_nsprotocol and keep a weak > pointer to it here.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2479143003/diff/60001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_mac.mm (right): https://codereview.chromium.org/2479143003/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac.mm:114: - (instancetype)init; No need to redeclare this. https://codereview.chromium.org/2479143003/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac.mm:153: [[AlertDispatcherImpl alloc] init]); This is leaked here since the Bridge now takes its own reference. This should be in a local scoped_nsobject. https://codereview.chromium.org/2479143003/diff/60001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm (right): https://codereview.chromium.org/2479143003/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:192: new NotificationPlatformBridgeMac(notification_center(), nullptr)); Use nil instead of nullptr for NSObjects.
The CQ bit was checked by miguelg@chromium.org to run a CQ dry run
https://codereview.chromium.org/2479143003/diff/60001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_mac.mm (right): https://codereview.chromium.org/2479143003/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac.mm:114: - (instancetype)init; On 2016/11/21 16:28:17, Robert Sesek wrote: > No need to redeclare this. Done. https://codereview.chromium.org/2479143003/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac.mm:153: [[AlertDispatcherImpl alloc] init]); On 2016/11/21 16:28:17, Robert Sesek wrote: > This is leaked here since the Bridge now takes its own reference. This should be > in a local scoped_nsobject. Of course yes, thanks for pointing it out. https://codereview.chromium.org/2479143003/diff/60001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm (right): https://codereview.chromium.org/2479143003/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm:192: new NotificationPlatformBridgeMac(notification_center(), nullptr)); On 2016/11/21 16:28:17, Robert Sesek wrote: > Use nil instead of nullptr for NSObjects. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by miguelg@chromium.org
The CQ bit was checked by miguelg@chromium.org
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
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 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 Link to the patchset: https://codereview.chromium.org/2479143003/#ps100001 (title: "rename alert_dispatcher.h to alert_dispatcher_mac.h and add it to the BUILD.gn file")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1479810040112150,
"parent_rev": "3ddbb51e44cc90e52a1faaa4d5ae181a868d0ff9", "commit_rev":
"a394f4a9cc5c93c85b1c37b696cc7760146ca837"}
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Add tests for the XPC client. Required a small refactoring of the XPC client in order to make it stub-able. BUG=571056 ========== to ========== Add tests for the XPC client. Required a small refactoring of the XPC client in order to make it stub-able. BUG=571056 Committed: https://crrev.com/5c55e9251f0448c522d6386da4ff95bb0708a6e3 Cr-Commit-Position: refs/heads/master@{#433835} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/5c55e9251f0448c522d6386da4ff95bb0708a6e3 Cr-Commit-Position: refs/heads/master@{#433835} |
