Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2158463003:
Initial client side implementation of the XPC service (Closed)
|
Created:
4 years, 5 months ago by Miguel Garcia Modified:
4 years, 3 months ago CC:
chromium-reviews, Peter Beverloo, mlamouri+watch-notifications_chromium.org, Nico Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionInitial client side implementation of the XPC service
Both Displaying and clicking on notifications is supported.
Closing notifiations and retrieving them is not.
By itself the CL does nothing (so don't enable the flag for daily use
just yet) since there is no XPC service backing up the calls yet.
Since no changes in the BUILD system are required for this piece
to land I am only adding them behind a run time flag not a compile time
one.
If you feel brave The companion XPC service is in
https://chromiumcodereview.appspot.com/2070903002/
BUG=571056
Committed: https://crrev.com/9fd9154690781cf18df34319683ff697e84ba165
Cr-Commit-Position: refs/heads/master@{#418601}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Rebase #
Total comments: 32
Patch Set 3 : address most (not all) review comments #Patch Set 4 : review comments #
Total comments: 10
Patch Set 5 : review #
Total comments: 2
Patch Set 6 : flag change #
Messages
Total messages: 50 (32 generated)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by miguelg@chromium.org to run a CQ dry run
Patchset #1 (id:20001) has been deleted
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
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
notifications rs lgtm, but this *really* needs Robert's review. Consider dropping the second command line flag if you can, that way we can have the existing about:flag for the native notification center, and the compile time flag for use of the XPC service. https://codereview.chromium.org/2158463003/diff/40001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_mac.h (right): https://codereview.chromium.org/2158463003/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac.h:24: // Interface to communicate with the Alert XPC service. This could use a little bit more colour to explain what "Alert" means in this context. Right now it looks like the name of the XPC service? https://codereview.chromium.org/2158463003/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac.h:27: // The connection to the XPC server in charge of delivering alerts Here, and lines 56, 68 and 71: sentences end with periods. https://codereview.chromium.org/2158463003/diff/40001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_mac.mm (right): https://codereview.chromium.org/2158463003/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac.mm:91: ProfileManager* profileManager = g_browser_process->profile_manager(); nit: profile_manager https://codereview.chromium.org/2158463003/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac.mm:129: // But make sure the ones shown by the XPC service are actually Is this a continuation from the TODO on line 128? It should probably be it's own TODO. https://codereview.chromium.org/2158463003/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac.mm:198: if (notification.never_timeout() && nit: // TODO(miguelg): Remove the command line flag requirement when the XPC // service is no longer guarded behind a compile time flag. An alternative to this would be to add a static on NotificationPlatformBridgeMac that comes down to: bool NotificationPlatformBridgeMac::UseRemoteService() { #if BUILDFLAG(YOUR_COMPILE_TIME_FLAG) return true; #else return false; #endif } https://codereview.chromium.org/2158463003/diff/40001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac.mm:256: notificationResponseData)) nit: brackets around the return (this now is a multi-line if statement).
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.
PTAL Rebased & verified it still works. Happy to remove the run time flag once the compile flag is introduced.
https://codereview.chromium.org/2158463003/diff/50001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_mac.h (right): https://codereview.chromium.org/2158463003/diff/50001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac.h:25: @interface NotificationRemoteDispatcher : NSObject You can just @class the forward declaration instead, and not declare this in the header. https://codereview.chromium.org/2158463003/diff/50001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac.h:56: // Processes a notification response response from where? And punctuation. https://codereview.chromium.org/2158463003/diff/50001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_mac.mm (right): https://codereview.chromium.org/2158463003/diff/50001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac.mm:254: NSDictionary* notificationResponseData) { naming: under_scores (and maybe a shorter name to make for less wrapping). https://codereview.chromium.org/2158463003/diff/50001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac.mm:376: @synthesize xpcConnection = _xpcConnection; Use trailing underscores. https://codereview.chromium.org/2158463003/diff/50001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac.mm:376: @synthesize xpcConnection = _xpcConnection; There's no need for this @property. It can just be a scoped_nsobject ivar. https://codereview.chromium.org/2158463003/diff/50001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac.mm:378: - (instancetype)init { This needs to be wrapped in a if ((self = [super init])) { // your initialization here } https://codereview.chromium.org/2158463003/diff/50001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac.mm:381: _xpcConnection = [[NSXPCConnection alloc] This is leaked. https://codereview.chromium.org/2158463003/diff/50001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac.mm:389: NSLog(@"connection interrupted: interruptionHandler: %@", _xpcConnection); Use Chromium logging instead. https://codereview.chromium.org/2158463003/diff/50001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/notifications/notification_delivery.h (right): https://codereview.chromium.org/2158463003/diff/50001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/notifications/notification_delivery.h:13: // Indicate the XPC service that it needs to display an alert. "Protocol for the XPC notification service." https://codereview.chromium.org/2158463003/diff/50001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/notifications/notification_delivery.h:14: // |notificationData| is generated using a NofiticationBuilder object. This should be next to -deliverNotification:. https://codereview.chromium.org/2158463003/diff/50001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/notifications/notification_delivery.h:21: // Used to talk back from the XPC service to chrome upon a notification click. "Response protocol for the XPC notification service to notify Chrome of notification interactions." https://codereview.chromium.org/2158463003/diff/50001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/notifications/notification_delivery.h:22: // |notificationResponseData| is generated through a Move down next to the method.
https://codereview.chromium.org/2158463003/diff/50001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_mac.h (right): https://codereview.chromium.org/2158463003/diff/50001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac.h:27: // The connection to the XPC server in charge of delivering alerts nit: finish the sentence with a period. Elsewhere too. https://codereview.chromium.org/2158463003/diff/50001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_mac.mm (right): https://codereview.chromium.org/2158463003/diff/50001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac.mm:130: // removed. Is this comment separate from the TODO, or is it a continuation? Reformat if the former. https://codereview.chromium.org/2158463003/diff/50001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac.mm:200: switches::kEnableNativeAlerts)) { As discussed, we'll want to switch this to a base::Feature instead.(See //chrome/common/chrome_features.h.) That enables us to get Finch for our part of the XPC roll-out for free. This is orthogonal of the compile-time flag. https://codereview.chromium.org/2158463003/diff/50001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac.mm:254: NSDictionary* notificationResponseData) { On 2016/09/08 21:10:08, Robert Sesek wrote: > naming: under_scores (and maybe a shorter name to make for less wrapping). I *really* dislike the hacker_style vs. camelCase inconsistencies within a single file :-(
https://codereview.chromium.org/2158463003/diff/50001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_mac.mm (right): https://codereview.chromium.org/2158463003/diff/50001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac.mm:254: NSDictionary* notificationResponseData) { On 2016/09/09 14:56:19, Peter Beverloo wrote: > On 2016/09/08 21:10:08, Robert Sesek wrote: > > naming: under_scores (and maybe a shorter name to make for less wrapping). > > I *really* dislike the hacker_style vs. camelCase inconsistencies within a > single file :-( The style rule isn't about the file, though. It's about the context (in C++ use those naming conventions, in ObjC use those).
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 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...
Thanks for the review! PTAL https://chromiumcodereview.appspot.com/2158463003/diff/50001/chrome/browser/n... File chrome/browser/notifications/notification_platform_bridge_mac.h (right): https://chromiumcodereview.appspot.com/2158463003/diff/50001/chrome/browser/n... chrome/browser/notifications/notification_platform_bridge_mac.h:25: @interface NotificationRemoteDispatcher : NSObject On 2016/09/08 21:10:08, Robert Sesek wrote: > You can just @class the forward declaration instead, and not declare this in the > header. Done. https://chromiumcodereview.appspot.com/2158463003/diff/50001/chrome/browser/n... chrome/browser/notifications/notification_platform_bridge_mac.h:27: // The connection to the XPC server in charge of delivering alerts On 2016/09/09 14:56:19, Peter Beverloo wrote: > nit: finish the sentence with a period. Elsewhere too. Done. https://chromiumcodereview.appspot.com/2158463003/diff/50001/chrome/browser/n... chrome/browser/notifications/notification_platform_bridge_mac.h:56: // Processes a notification response On 2016/09/08 21:10:08, Robert Sesek wrote: > response from where? And punctuation. Clarified https://chromiumcodereview.appspot.com/2158463003/diff/50001/chrome/browser/n... File chrome/browser/notifications/notification_platform_bridge_mac.mm (right): https://chromiumcodereview.appspot.com/2158463003/diff/50001/chrome/browser/n... chrome/browser/notifications/notification_platform_bridge_mac.mm:130: // removed. Formatted as 2 TODOs now On 2016/09/09 14:56:19, Peter Beverloo wrote: > Is this comment separate from the TODO, or is it a continuation? Reformat if the > former. https://chromiumcodereview.appspot.com/2158463003/diff/50001/chrome/browser/n... chrome/browser/notifications/notification_platform_bridge_mac.mm:254: NSDictionary* notificationResponseData) { On 2016/09/08 21:10:08, Robert Sesek wrote: > naming: under_scores (and maybe a shorter name to make for less wrapping). Done. https://chromiumcodereview.appspot.com/2158463003/diff/50001/chrome/browser/n... chrome/browser/notifications/notification_platform_bridge_mac.mm:254: NSDictionary* notificationResponseData) { On 2016/09/09 20:43:43, Robert Sesek wrote: > On 2016/09/09 14:56:19, Peter Beverloo wrote: > > On 2016/09/08 21:10:08, Robert Sesek wrote: > > > naming: under_scores (and maybe a shorter name to make for less wrapping). > > > > I *really* dislike the hacker_style vs. camelCase inconsistencies within a > > single file :-( > > The style rule isn't about the file, though. It's about the context (in C++ use > those naming conventions, in ObjC use those). I re-named it to a single word to make both of you happy :) But yeah the rule is clear hacker_style in C++ methods, camelCase in Obj-C methods. I just messed it up https://chromiumcodereview.appspot.com/2158463003/diff/50001/chrome/browser/n... chrome/browser/notifications/notification_platform_bridge_mac.mm:376: @synthesize xpcConnection = _xpcConnection; On 2016/09/08 21:10:08, Robert Sesek wrote: > There's no need for this @property. It can just be a scoped_nsobject ivar. Done. https://chromiumcodereview.appspot.com/2158463003/diff/50001/chrome/browser/n... chrome/browser/notifications/notification_platform_bridge_mac.mm:376: @synthesize xpcConnection = _xpcConnection; On 2016/09/08 21:10:08, Robert Sesek wrote: > Use trailing underscores. Done. https://chromiumcodereview.appspot.com/2158463003/diff/50001/chrome/browser/n... chrome/browser/notifications/notification_platform_bridge_mac.mm:378: - (instancetype)init { On 2016/09/08 21:10:08, Robert Sesek wrote: > This needs to be wrapped in a > > if ((self = [super init])) { > // your initialization here > } Done. https://chromiumcodereview.appspot.com/2158463003/diff/50001/chrome/browser/n... chrome/browser/notifications/notification_platform_bridge_mac.mm:381: _xpcConnection = [[NSXPCConnection alloc] On 2016/09/08 21:10:08, Robert Sesek wrote: > This is leaked. Done. https://chromiumcodereview.appspot.com/2158463003/diff/50001/chrome/browser/n... chrome/browser/notifications/notification_platform_bridge_mac.mm:389: NSLog(@"connection interrupted: interruptionHandler: %@", _xpcConnection); On 2016/09/08 21:10:08, Robert Sesek wrote: > Use Chromium logging instead. Done. https://chromiumcodereview.appspot.com/2158463003/diff/50001/chrome/browser/u... File chrome/browser/ui/cocoa/notifications/notification_delivery.h (right): https://chromiumcodereview.appspot.com/2158463003/diff/50001/chrome/browser/u... chrome/browser/ui/cocoa/notifications/notification_delivery.h:13: // Indicate the XPC service that it needs to display an alert. On 2016/09/08 21:10:08, Robert Sesek wrote: > "Protocol for the XPC notification service." Done. https://chromiumcodereview.appspot.com/2158463003/diff/50001/chrome/browser/u... chrome/browser/ui/cocoa/notifications/notification_delivery.h:14: // |notificationData| is generated using a NofiticationBuilder object. On 2016/09/08 21:10:09, Robert Sesek wrote: > This should be next to -deliverNotification:. Done. https://chromiumcodereview.appspot.com/2158463003/diff/50001/chrome/browser/u... chrome/browser/ui/cocoa/notifications/notification_delivery.h:21: // Used to talk back from the XPC service to chrome upon a notification click. On 2016/09/08 21:10:09, Robert Sesek wrote: > "Response protocol for the XPC notification service to notify Chrome of > notification interactions." Done. https://chromiumcodereview.appspot.com/2158463003/diff/50001/chrome/browser/u... chrome/browser/ui/cocoa/notifications/notification_delivery.h:22: // |notificationResponseData| is generated through a On 2016/09/08 21:10:08, Robert Sesek wrote: > Move down next to the method. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2158463003/diff/90001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_mac.mm (right): https://codereview.chromium.org/2158463003/diff/90001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac.mm:117: - (instancetype)init; You don't have to re-declare init if there's no arguments. https://codereview.chromium.org/2158463003/diff/90001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac.mm:207: if (notification.never_timeout()) { This could use some commentary. https://codereview.chromium.org/2158463003/diff/90001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac.mm:269: NSNumber* buttonIndex = under_score all these names https://codereview.chromium.org/2158463003/diff/90001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac.mm:368: NSDictionary* notification_response = but camelCase here ;-) https://codereview.chromium.org/2158463003/diff/90001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_mac.mm:389: #if BUILDFLAG(ENABLE_XPC_NOTIFICATIONS) Why not move this #if to be around the instantiation instead? That way this code will get compile coverage.
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: + thakis@chromium.org
Thanks for the review +Nico for chrome/common/* OWNERS https://chromiumcodereview.appspot.com/2158463003/diff/90001/chrome/browser/n... File chrome/browser/notifications/notification_platform_bridge_mac.mm (right): https://chromiumcodereview.appspot.com/2158463003/diff/90001/chrome/browser/n... chrome/browser/notifications/notification_platform_bridge_mac.mm:117: - (instancetype)init; On 2016/09/12 15:33:47, Robert Sesek wrote: > You don't have to re-declare init if there's no arguments. Done. https://chromiumcodereview.appspot.com/2158463003/diff/90001/chrome/browser/n... chrome/browser/notifications/notification_platform_bridge_mac.mm:207: if (notification.never_timeout()) { On 2016/09/12 15:33:47, Robert Sesek wrote: > This could use some commentary. Done. https://chromiumcodereview.appspot.com/2158463003/diff/90001/chrome/browser/n... chrome/browser/notifications/notification_platform_bridge_mac.mm:269: NSNumber* buttonIndex = On 2016/09/12 15:33:47, Robert Sesek wrote: > under_score all these names Done. https://chromiumcodereview.appspot.com/2158463003/diff/90001/chrome/browser/n... chrome/browser/notifications/notification_platform_bridge_mac.mm:368: NSDictionary* notification_response = On 2016/09/12 15:33:47, Robert Sesek wrote: > but camelCase here ;-) Done. https://chromiumcodereview.appspot.com/2158463003/diff/90001/chrome/browser/n... chrome/browser/notifications/notification_platform_bridge_mac.mm:389: #if BUILDFLAG(ENABLE_XPC_NOTIFICATIONS) On 2016/09/12 15:33:47, Robert Sesek wrote: > Why not move this #if to be around the instantiation instead? That way this code > will get compile coverage. I shied away from that because the instantiation was in the middle of the argument list in the constructor of the bridge but ok :)
LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
NIco can you have a look at the new build flag?
miguelg@chromium.org changed reviewers: + jochen@chromium.org - thakis@chromium.org
Jochen, mind having a look? I think Nico is OOO
On 2016/09/14 08:22:17, Miguel Garcia wrote: > Jochen, mind having a look? I think Nico is OOO I just need OWNERS for the build flag (files in chrome/common). You are welcome to review the whole Cl of course :).
thakis@chromium.org changed reviewers: + thakis@chromium.org
lgtm with either `= false` or `= is_mac`. https://chromiumcodereview.appspot.com/2158463003/diff/110001/chrome/common/f... File chrome/common/features.gni (right): https://chromiumcodereview.appspot.com/2158463003/diff/110001/chrome/common/f... chrome/common/features.gni:43: enable_xpc_notifications = is_mac && false && false ?
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 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, thakis@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/2158463003/#ps130001 (title: "flag change")
miguelg@chromium.org changed reviewers: - jochen@chromium.org
https://chromiumcodereview.appspot.com/2158463003/diff/110001/chrome/common/f... File chrome/common/features.gni (right): https://chromiumcodereview.appspot.com/2158463003/diff/110001/chrome/common/f... chrome/common/features.gni:43: enable_xpc_notifications = is_mac && false On 2016/09/14 14:34:17, Nico wrote: > && false > > ? Done.
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 #6 (id:130001)
Message was sent while issue was closed.
Description was changed from ========== Initial client side implementation of the XPC service Both Displaying and clicking on notifications is supported. Closing notifiations and retrieving them is not. By itself the CL does nothing (so don't enable the flag for daily use just yet) since there is no XPC service backing up the calls yet. Since no changes in the BUILD system are required for this piece to land I am only adding them behind a run time flag not a compile time one. If you feel brave The companion XPC service is in https://chromiumcodereview.appspot.com/2070903002/ BUG=571056 ========== to ========== Initial client side implementation of the XPC service Both Displaying and clicking on notifications is supported. Closing notifiations and retrieving them is not. By itself the CL does nothing (so don't enable the flag for daily use just yet) since there is no XPC service backing up the calls yet. Since no changes in the BUILD system are required for this piece to land I am only adding them behind a run time flag not a compile time one. If you feel brave The companion XPC service is in https://chromiumcodereview.appspot.com/2070903002/ BUG=571056 Committed: https://crrev.com/9fd9154690781cf18df34319683ff697e84ba165 Cr-Commit-Position: refs/heads/master@{#418601} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/9fd9154690781cf18df34319683ff697e84ba165 Cr-Commit-Position: refs/heads/master@{#418601} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
