[Mac] Add XPC alerts to GetDisplayedNotifications

Change the flow on mac so that in addition to the local banners XPC alerts are also considered.

BUG=571056
Review-Url: https://codereview.chromium.org/2709213005
Cr-Commit-Position: refs/heads/master@{#462281}
Committed: https://chromium.googlesource.com/chromium/src/+/76bc01769c89fd75526253472222a9854533100a

[Miguel Garcia, 2017-02-23 18:57:23]

The CQ bit was checked by miguelg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 18:58:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Miguel Garcia, 2017-02-23 19:03:54]

The CQ bit was checked by miguelg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 19:04:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Miguel Garcia, 2017-02-23 19:09:33]

Patchset #1 (id:1) has been deleted

[Miguel Garcia, 2017-02-23 19:16:23]

miguelg@chromium.org changed reviewers:
+ peter@chromium.org, rsesek@chromium.org

[Peter Beverloo, 2017-02-24 16:12:24]

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/message_center_display_service.h (right):

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/message_center_display_service.h:31: void
GetDisplayedAsync(const NotificationCommon::NotificationResultCallback&
Did `git cl format` do this? It looks weird. I would expect:

  void GetDisplayedAsync(
      const NotificationCommon::NotificationResultCallback& callback)
          const override;

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/native_notification_display_service.h (right):

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/native_notification_display_service.h:43: void
GetDisplayedAsync(const NotificationCommon::NotificationResultCallback&
nit: no blank line

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/notification_common.h (right):

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_common.h:20: using
NotificationResultCallback =
Let's define this type on the NotificationDisplayService instead. The
indirection of having it on another class is really uncommon and complicates the
API.

(It's fine to define these types multiple times.)

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/notification_display_service.h (right):

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_display_service.h:44: virtual void
GetDisplayedAsync(
It would be much easier if you would change this around.

First update GetDisplayed() to take a callback and change the callsites to match
this. (Just immediately posting to it.)

Then update the implementation where necessary (Mac) to provide the added
functionality.

Please do drop the "Async" from the method name regardless which path you take.
The fact that it returns void and takes a callback implies that clearly enough.

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/notification_platform_bridge.h (right):

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge.h:48: //
TODO(miguelg): remmove the sync version once all paths have been updated.
micro nit: remmmmmmmmmmmmmmove

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/notification_platform_bridge_android.cc
(right):

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_android.cc:331: false
/* supports sync */));
nit: "sync" is rather ambiguous (also in the other impl) when the method name
includes "async"

There's plenty of space here, what about:

  false /* supports getting displayed notifications */

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/notification_platform_bridge_mac.mm (right):

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_mac.mm:126: //  and
included in |callback|.
nit: weird line break

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_mac.mm:511:
base::MakeUnique<std::set<std::string>>();
nit {}

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_mac.mm:513: for
(NSUserNotification* toast in localNotifications) {
Can it happen that the XPC service does not reply, or replies with a large
latency that causes |localNotifications| to be out of sync? (Is it safe to store
a pointer to that member like this at all?)

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_mac.mm:514: NSString*
toast_profile_id = [toast.userInfo
nit: toastProfileId

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_mac.mm:526:
base::SysNSStringToUTF8(alert));
nit: {}

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/platform_notification_service_impl.h (right):

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_impl.h:101: 
nit: drop the linebreak

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/notifications/notification_delivery.h (right):

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/notifications/notification_delivery.h:26: // Returns the
alerts currently being displayed for |profileId|.
nit: // Will invoke |reply| with the alerts...

https://codereview.chromium.org/2709213005/diff/40001/content/public/browser/...
File content/public/browser/platform_notification_service.h (right):

https://codereview.chromium.org/2709213005/diff/40001/content/public/browser/...
content/public/browser/platform_notification_service.h:93: bool /* support sync
*/)>&) = 0;
Please define the callback's type with a `using` at the top of this class.

support sync -> whatever you end up with per the other comment

[Robert Sesek, 2017-02-24 22:38:05]

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/alert_dispatcher_mac.h (right):

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/alert_dispatcher_mac.h:26: -
(void)getDisplayedAlertIds:(NSString*)profileId
Maybe getDisplayedAlertsForProfileId:withLocalNotifications:callback: instead
(Cocoa naming is more art than science…).

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/message_center_display_service.cc (right):

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/message_center_display_service.cc:48:
std::unique_ptr<std::set<std::string>> displayedNotifications =
nit: under_scores

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/notification_platform_bridge_mac.mm (right):

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_mac.mm:508:
withReply:^(NSArray* alerts) {
This is hard to read due to the amount of indent necessary. You can pull the
block out into a local to make this better:

auto reply = ^(NSArray* alerts) {
};

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_mac.mm:513: for
(NSUserNotification* toast in localNotifications) {
On 2017/02/24 16:12:24, Peter Beverloo wrote:
> Can it happen that the XPC service does not reply, or replies with a large
> latency that causes |localNotifications| to be out of sync? (Is it safe to
store
> a pointer to that member like this at all?)

Yes this is possible.

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/notifications/notification_delivery.h (right):

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/notifications/notification_delivery.h:27: -
(void)getDisplayedAlertIds:(NSString*)profileId
Similarly name: getDisplayedAlertsForProfileId:withReply:

[Miguel Garcia, 2017-03-22 21:58:54]

Description was changed from

==========
Define a flow to retrieve displayed notifications asynchronously

Implement it in the mac port by using XPC replies.
Implement a pass through for all other platforms that support synchronization.

A second CL will hook it up in content, and remove the sync flow.

You can see all the pieces together in
https://codereview.chromium.org/2714743002/

BUG=571056
==========

to

==========
Change the flow on mac so that in addition to the local banners XPC alerts are
also considered.

BUG=571056
==========

[Miguel Garcia, 2017-03-22 22:00:09]

PTAL

Now that the asynchronous flow has landed this just implements the mac pieces,
resulting in a much smaller CL.

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/alert_dispatcher_mac.h (right):

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/alert_dispatcher_mac.h:26: -
(void)getDisplayedAlertIds:(NSString*)profileId
On 2017/02/24 22:38:05, Robert Sesek wrote:
> Maybe getDisplayedAlertsForProfileId:withLocalNotifications:callback: instead
> (Cocoa naming is more art than science…).

Done, but shouldn't it be withCallback or andCallback ? Trying to learn the art
:)

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/message_center_display_service.cc (right):

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/message_center_display_service.cc:48:
std::unique_ptr<std::set<std::string>> displayedNotifications =
On 2017/02/24 22:38:05, Robert Sesek wrote:
> nit: under_scores

Done.

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/message_center_display_service.h (right):

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/message_center_display_service.h:31: void
GetDisplayedAsync(const NotificationCommon::NotificationResultCallback&
Yes, will change it to your version anyway.

On 2017/02/24 16:12:23, Peter Beverloo (OOO - Mar 6th) wrote:
> Did `git cl format` do this? It looks weird. I would expect:
> 
>   void GetDisplayedAsync(
>       const NotificationCommon::NotificationResultCallback& callback)
>           const override;

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/native_notification_display_service.h (right):

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/native_notification_display_service.h:43: void
GetDisplayedAsync(const NotificationCommon::NotificationResultCallback&
On 2017/02/24 16:12:23, Peter Beverloo (OOO - Mar 6th) wrote:
> nit: no blank line

Done.

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/notification_display_service.h (right):

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_display_service.h:44: virtual void
GetDisplayedAsync(
Yeah Async was temporary, I am however if I should add something like
"OnIOThread" to indicate that the callback is posted in said thread.

Re: the order, will send a CL doing that first. I did it this way because I was
more interested in getting feedback for the mac pieces.


On 2017/02/24 16:12:23, Peter Beverloo (OOO - Mar 6th) wrote:
> It would be much easier if you would change this around.
> 
> First update GetDisplayed() to take a callback and change the callsites to
match
> this. (Just immediately posting to it.)
> 
> Then update the implementation where necessary (Mac) to provide the added
> functionality.
> 
> Please do drop the "Async" from the method name regardless which path you
take.
> The fact that it returns void and takes a callback implies that clearly
enough.

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/notification_platform_bridge.h (right):

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge.h:48: //
TODO(miguelg): remmove the sync version once all paths have been updated.
On 2017/02/24 16:12:23, Peter Beverloo (OOO - Mar 6th) wrote:
> micro nit: remmmmmmmmmmmmmmove

Done.

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/notification_platform_bridge_android.cc
(right):

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_android.cc:331: false
/* supports sync */));
On 2017/02/24 16:12:24, Peter Beverloo (OOO - Mar 6th) wrote:
> nit: "sync" is rather ambiguous (also in the other impl) when the method name
> includes "async"
> 
> There's plenty of space here, what about:
> 
>   false /* supports getting displayed notifications */

Done.

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/notification_platform_bridge_mac.mm (right):

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_mac.mm:126: //  and
included in |callback|.
On 2017/02/24 16:12:24, Peter Beverloo (OOO - Mar 6th) wrote:
> nit: weird line break

Done.

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_mac.mm:508:
withReply:^(NSArray* alerts) {
On 2017/02/24 22:38:05, Robert Sesek wrote:
> This is hard to read due to the amount of indent necessary. You can pull the
> block out into a local to make this better:
> 
> auto reply = ^(NSArray* alerts) {
> };

Done.

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_mac.mm:511:
base::MakeUnique<std::set<std::string>>();
On 2017/02/24 16:12:24, Peter Beverloo (OOO - Mar 6th) wrote:
> nit {}

Done.

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_mac.mm:513: for
(NSUserNotification* toast in localNotifications) {
I have changed the flow now so that the xpc alerts are checked first, then the
local ones are collected.


On 2017/02/24 22:38:05, Robert Sesek wrote:
> On 2017/02/24 16:12:24, Peter Beverloo wrote:
> > Can it happen that the XPC service does not reply, or replies with a large
> > latency that causes |localNotifications| to be out of sync? (Is it safe to
> store
> > a pointer to that member like this at all?)
> 
> Yes this is possible.

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_mac.mm:514: NSString*
toast_profile_id = [toast.userInfo
On 2017/02/24 16:12:24, Peter Beverloo (OOO - Mar 6th) wrote:
> nit: toastProfileId

Done.

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_mac.mm:526:
base::SysNSStringToUTF8(alert));
On 2017/02/24 16:12:24, Peter Beverloo (OOO - Mar 6th) wrote:
> nit: {}

Done.

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/platform_notification_service_impl.h (right):

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_impl.h:101: 
On 2017/02/24 16:12:24, Peter Beverloo (OOO - Mar 6th) wrote:
> nit: drop the linebreak

Done.

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/notifications/notification_delivery.h (right):

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/notifications/notification_delivery.h:26: // Returns the
alerts currently being displayed for |profileId|.
On 2017/02/24 16:12:24, Peter Beverloo (OOO - Mar 6th) wrote:
> nit: // Will invoke |reply| with the alerts...

Done.

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/notifications/notification_delivery.h:27: -
(void)getDisplayedAlertIds:(NSString*)profileId
On 2017/02/24 22:38:05, Robert Sesek wrote:
> Similarly name: getDisplayedAlertsForProfileId:withReply:

Done.

[Miguel Garcia, 2017-03-22 22:00:11]

PTAL

Now that the asynchronous flow has landed this just implements the mac pieces,
resulting in a much smaller CL.

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/platform_notification_service_impl.h (right):

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_impl.h:101: 
On 2017/02/24 16:12:24, Peter Beverloo (OOO - Mar 6th) wrote:
> nit: drop the linebreak

Done.

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/notifications/notification_delivery.h (right):

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/notifications/notification_delivery.h:26: // Returns the
alerts currently being displayed for |profileId|.
On 2017/02/24 16:12:24, Peter Beverloo (OOO - Mar 6th) wrote:
> nit: // Will invoke |reply| with the alerts...

Done.

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/notifications/notification_delivery.h:27: -
(void)getDisplayedAlertIds:(NSString*)profileId
On 2017/02/24 22:38:05, Robert Sesek wrote:
> Similarly name: getDisplayedAlertsForProfileId:withReply:

Done.

[Miguel Garcia, 2017-03-22 22:00:17]

The CQ bit was checked by miguelg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 22:00:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-22 22:30:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-22 22:30:01]

Dry run: This issue passed the CQ dry run.

[Miguel Garcia, 2017-04-03 11:48:35]

Now that I am back it would be great if I could get some feedback here since I
think this is the last CL before we are feature complete :)


On 2017/03/22 22:00:11, Miguel Garcia wrote:
> PTAL
> 
> Now that the asynchronous flow has landed this just implements the mac pieces,
> resulting in a much smaller CL.
> 
>
https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
> File chrome/browser/notifications/platform_notification_service_impl.h
(right):
> 
>
https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
> chrome/browser/notifications/platform_notification_service_impl.h:101: 
> On 2017/02/24 16:12:24, Peter Beverloo (OOO - Mar 6th) wrote:
> > nit: drop the linebreak
> 
> Done.
> 
>
https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/ui/cocoa...
> File chrome/browser/ui/cocoa/notifications/notification_delivery.h (right):
> 
>
https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/ui/cocoa...
> chrome/browser/ui/cocoa/notifications/notification_delivery.h:26: // Returns
the
> alerts currently being displayed for |profileId|.
> On 2017/02/24 16:12:24, Peter Beverloo (OOO - Mar 6th) wrote:
> > nit: // Will invoke |reply| with the alerts...
> 
> Done.
> 
>
https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/ui/cocoa...
> chrome/browser/ui/cocoa/notifications/notification_delivery.h:27: -
> (void)getDisplayedAlertIds:(NSString*)profileId
> On 2017/02/24 22:38:05, Robert Sesek wrote:
> > Similarly name: getDisplayedAlertsForProfileId:withReply:
> 
> Done.

[Peter Beverloo, 2017-04-03 12:33:29]

Largely nits, this is very Objective C heavy. Is there any way you can add a
test for this?

https://codereview.chromium.org/2709213005/diff/60001/chrome/browser/notifica...
File chrome/browser/notifications/alert_dispatcher_mac.h (right):

https://codereview.chromium.org/2709213005/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/alert_dispatcher_mac.h:1: 
revert

https://codereview.chromium.org/2709213005/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/alert_dispatcher_mac.h:18: }  // namespace
We shouldn't use anonymous namespaces in header files. Can we inline the
callback type if we can't put it as an interface member?

Alternatively, you can fake-namespace it -

  GetDisplayedMacNotificationAlertsCallback

https://codereview.chromium.org/2709213005/diff/60001/chrome/browser/notifica...
File chrome/browser/notifications/notification_platform_bridge_mac.mm (right):

https://codereview.chromium.org/2709213005/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_mac.mm:140: //  Get
displayed alerts for |profileId| merge them with |localNotifications|
nit: no double space

https://codereview.chromium.org/2709213005/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_mac.mm:141: // and
post |callback| to the IO thread.
IO -> UI?

https://codereview.chromium.org/2709213005/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_mac.mm:307:
withNotificationCenter:notification_center_
Shouldn't this call understand |incognito|?

https://codereview.chromium.org/2709213005/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_mac.mm:545: }
nit: no {}

https://codereview.chromium.org/2709213005/diff/60001/chrome/browser/notifica...
File chrome/browser/notifications/stub_alert_dispatcher_mac.h (right):

https://codereview.chromium.org/2709213005/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/stub_alert_dispatcher_mac.h:20: }  // namespace
dito re: anonymous namespaces in header files

[Robert Sesek, 2017-04-03 16:51:24]

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/alert_dispatcher_mac.h (right):

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/alert_dispatcher_mac.h:26: -
(void)getDisplayedAlertIds:(NSString*)profileId
On 2017/03/22 22:00:06, Miguel Garcia wrote:
> On 2017/02/24 22:38:05, Robert Sesek wrote:
> > Maybe getDisplayedAlertsForProfileId:withLocalNotifications:callback:
instead
> > (Cocoa naming is more art than science…).
> 
> Done, but shouldn't it be withCallback or andCallback ? Trying to learn the
art
> :)

Methods with a lot of parameters get too wordy if every one is described as
"with" so it can be omitted after the first. It should read like a sentence, and
so you can think of the many parameters as being in a comma-separated serial
list (omitting the final conjunction).

https://codereview.chromium.org/2709213005/diff/60001/chrome/browser/notifica...
File chrome/browser/notifications/alert_dispatcher_mac.h (right):

https://codereview.chromium.org/2709213005/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/alert_dispatcher_mac.h:15: using
DisplayedNotificationsCallback =
Why isn't this using NotificationDisplayService::DisplayedNotificationsCallback
?

https://codereview.chromium.org/2709213005/diff/60001/chrome/browser/notifica...
File chrome/browser/notifications/notification_platform_bridge_mac.mm (right):

https://codereview.chromium.org/2709213005/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_mac.mm:140: //  Get
displayed alerts for |profileId| merge them with |localNotifications|
I missed this earlier, but you can omit re-declaring a protocol's methods in the
@interface. The NSObject<AlertDispatcher> tells the reader the methods. The
method in @implementation can have additional documentation that's specific to
it though.

https://codereview.chromium.org/2709213005/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_mac.mm:510: 
Just mark the protocol's methods:

// AlertDispatcher:

https://codereview.chromium.org/2709213005/diff/60001/chrome/browser/notifica...
File chrome/browser/notifications/stub_alert_dispatcher_mac.h (right):

https://codereview.chromium.org/2709213005/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/stub_alert_dispatcher_mac.h:17: using
DisplayedNotificationsCallback =
Why is this re-declared instead of using the one in alert_dispatcher_mac.h?

https://codereview.chromium.org/2709213005/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/stub_alert_dispatcher_mac.h:24: -
(void)dispatchNotification:(NSDictionary*)data;
Similarly, you can just remove the AlertDispatcher methods here, too.

https://codereview.chromium.org/2709213005/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/notifications/notification_delivery.h (right):

https://codereview.chromium.org/2709213005/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/notifications/notification_delivery.h:31: // Will invoke
|reply| with the alerts for |profileId| being displayed.
… alerts "as an array of NSString notification IDs" …

[Miguel Garcia, 2017-04-04 10:29:42]

The CQ bit was checked by miguelg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 10:30:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-04 10:59:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-04 10:59:15]

Dry run: This issue passed the CQ dry run.

[Miguel Garcia, 2017-04-04 12:21:26]

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/alert_dispatcher_mac.h (right):

https://codereview.chromium.org/2709213005/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/alert_dispatcher_mac.h:26: -
(void)getDisplayedAlertIds:(NSString*)profileId
On 2017/04/03 16:51:24, Robert Sesek wrote:
> On 2017/03/22 22:00:06, Miguel Garcia wrote:
> > On 2017/02/24 22:38:05, Robert Sesek wrote:
> > > Maybe getDisplayedAlertsForProfileId:withLocalNotifications:callback:
> instead
> > > (Cocoa naming is more art than science…).
> > 
> > Done, but shouldn't it be withCallback or andCallback ? Trying to learn the
> art
> > :)
> 
> Methods with a lot of parameters get too wordy if every one is described as
> "with" so it can be omitted after the first. It should read like a sentence,
and
> so you can think of the many parameters as being in a comma-separated serial
> list (omitting the final conjunction).

Acknowledged.

https://codereview.chromium.org/2709213005/diff/60001/chrome/browser/notifica...
File chrome/browser/notifications/alert_dispatcher_mac.h (right):

https://codereview.chromium.org/2709213005/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/alert_dispatcher_mac.h:1: 
On 2017/04/03 12:33:28, Peter Beverloo wrote:
> revert

Done.

https://codereview.chromium.org/2709213005/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/alert_dispatcher_mac.h:15: using
DisplayedNotificationsCallback =
On 2017/04/03 16:51:24, Robert Sesek wrote:
> Why isn't this using
NotificationDisplayService::DisplayedNotificationsCallback
> ?
Do you think it's worth including the display_service here just for the
callback? they seem completly unrelated pieces otherwise.

https://codereview.chromium.org/2709213005/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/alert_dispatcher_mac.h:18: }  // namespace
On 2017/04/03 12:33:28, Peter Beverloo wrote:
> We shouldn't use anonymous namespaces in header files. Can we inline the
> callback type if we can't put it as an interface member?
> 
> Alternatively, you can fake-namespace it -
> 
>   GetDisplayedMacNotificationAlertsCallback

I went with inlining (which is a pity since I need to inline in the .mm too for
it to work)

https://codereview.chromium.org/2709213005/diff/60001/chrome/browser/notifica...
File chrome/browser/notifications/notification_platform_bridge_mac.mm (right):

https://codereview.chromium.org/2709213005/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_mac.mm:140: //  Get
displayed alerts for |profileId| merge them with |localNotifications|
On 2017/04/03 12:33:28, Peter Beverloo wrote:
> nit: no double space

Done.

https://codereview.chromium.org/2709213005/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_mac.mm:140: //  Get
displayed alerts for |profileId| merge them with |localNotifications|
On 2017/04/03 12:33:28, Peter Beverloo wrote:
> nit: no double space

Done.

https://codereview.chromium.org/2709213005/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_mac.mm:141: // and
post |callback| to the IO thread.
On 2017/04/03 12:33:29, Peter Beverloo wrote:
> IO -> UI?

Done.

https://codereview.chromium.org/2709213005/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_mac.mm:307:
withNotificationCenter:notification_center_
On 2017/04/03 12:33:28, Peter Beverloo wrote:
> Shouldn't this call understand |incognito|?

Well I guess but since we don't support notifications in incognito I thought I
should not complicate things further.

https://codereview.chromium.org/2709213005/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_mac.mm:510: 
On 2017/04/03 16:51:24, Robert Sesek wrote:
> Just mark the protocol's methods:
> 
> // AlertDispatcher:

Done.

https://codereview.chromium.org/2709213005/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_mac.mm:545: }
On 2017/04/03 12:33:28, Peter Beverloo wrote:
> nit: no {}

Done.

https://codereview.chromium.org/2709213005/diff/60001/chrome/browser/notifica...
File chrome/browser/notifications/stub_alert_dispatcher_mac.h (right):

https://codereview.chromium.org/2709213005/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/stub_alert_dispatcher_mac.h:17: using
DisplayedNotificationsCallback =
On 2017/04/03 16:51:24, Robert Sesek wrote:
> Why is this re-declared instead of using the one in alert_dispatcher_mac.h?
Moved to inlining as per Peter's comment about anonymous namespaces in .h files
(but yes, in the original patch it could just use that).

https://codereview.chromium.org/2709213005/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/stub_alert_dispatcher_mac.h:20: }  // namespace
On 2017/04/03 12:33:29, Peter Beverloo wrote:
> dito re: anonymous namespaces in header files

Acknowledged.

https://codereview.chromium.org/2709213005/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/notifications/notification_delivery.h (right):

https://codereview.chromium.org/2709213005/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/notifications/notification_delivery.h:31: // Will invoke
|reply| with the alerts for |profileId| being displayed.
On 2017/04/03 16:51:24, Robert Sesek wrote:
> … alerts "as an array of NSString notification IDs" …

Done.

[Peter Beverloo, 2017-04-04 13:16:25]

lg % Incognito

https://codereview.chromium.org/2709213005/diff/60001/chrome/browser/notifica...
File chrome/browser/notifications/notification_platform_bridge_mac.mm (right):

https://codereview.chromium.org/2709213005/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_mac.mm:307:
withNotificationCenter:notification_center_
On 2017/04/04 12:21:26, Miguel Garcia wrote:
> On 2017/04/03 12:33:28, Peter Beverloo wrote:
> > Shouldn't this call understand |incognito|?
> 
> Well I guess but since we don't support notifications in incognito I thought I
> should not complicate things further.

We absolutely do. Not Web Notifications, but extension notifications and those
of other features do.

https://codereview.chromium.org/2709213005/diff/80001/chrome/browser/notifica...
File chrome/browser/notifications/stub_alert_dispatcher_mac.h (right):

https://codereview.chromium.org/2709213005/diff/80001/chrome/browser/notifica...
chrome/browser/notifications/stub_alert_dispatcher_mac.h:30: bool /*
supports_synchronization */)>)callback;
(Including a header file that has the callback's definition feels nicer to me
too. Up to you.)

[Robert Sesek, 2017-04-04 15:31:48]

https://codereview.chromium.org/2709213005/diff/60001/chrome/browser/notifica...
File chrome/browser/notifications/alert_dispatcher_mac.h (right):

https://codereview.chromium.org/2709213005/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/alert_dispatcher_mac.h:15: using
DisplayedNotificationsCallback =
On 2017/04/04 12:21:26, Miguel Garcia wrote:
> On 2017/04/03 16:51:24, Robert Sesek wrote:
> > Why isn't this using
> NotificationDisplayService::DisplayedNotificationsCallback
> > ?
> Do you think it's worth including the display_service here just for the
> callback? they seem completly unrelated pieces otherwise.

The supports_synchronization parameter is completely meaningless in this
context, so I'd say you should either remove that from the callback or use the
type from display_service.

https://codereview.chromium.org/2709213005/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/alert_dispatcher_mac.h:18: }  // namespace
On 2017/04/04 12:21:26, Miguel Garcia wrote:
> On 2017/04/03 12:33:28, Peter Beverloo wrote:
> > We shouldn't use anonymous namespaces in header files. Can we inline the
> > callback type if we can't put it as an interface member?
> > 
> > Alternatively, you can fake-namespace it -
> > 
> >   GetDisplayedMacNotificationAlertsCallback
> 
> I went with inlining (which is a pity since I need to inline in the .mm too
for
> it to work)

Why can't you just declare this in the header without a namespace?

[Miguel Garcia, 2017-04-05 17:07:08]

The CQ bit was checked by miguelg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-05 17:07:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Miguel Garcia, 2017-04-05 17:10:26]

Added support for incognito now
Also agreed with Peter on a way to deal with the callback name. It is now
defined in a separate file and used by both the bridge and the alert dispatcher
Finally an initial linux port has landed in between changes (cool!) so I rebased
that as well

[Robert Sesek, 2017-04-05 17:16:56]

LGTM

https://codereview.chromium.org/2709213005/diff/100001/chrome/browser/notific...
File chrome/browser/notifications/stub_alert_dispatcher_mac.h (right):

https://codereview.chromium.org/2709213005/diff/100001/chrome/browser/notific...
chrome/browser/notifications/stub_alert_dispatcher_mac.h:10: #include <set>
Not needed in the .h

[commit-bot: I haz the power, 2017-04-05 18:11:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-05 18:11:05]

Dry run: This issue passed the CQ dry run.

[Peter Beverloo, 2017-04-05 18:23:00]

lgtm

[Miguel Garcia, 2017-04-05 19:14:57]

The CQ bit was checked by miguelg@chromium.org

[Miguel Garcia, 2017-04-05 19:14:57]

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/2709213005/#ps120001
(title: "review")

[commit-bot: I haz the power, 2017-04-05 19:15:24]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Miguel Garcia, 2017-04-05 19:15:30]

https://codereview.chromium.org/2709213005/diff/100001/chrome/browser/notific...
File chrome/browser/notifications/stub_alert_dispatcher_mac.h (right):

https://codereview.chromium.org/2709213005/diff/100001/chrome/browser/notific...
chrome/browser/notifications/stub_alert_dispatcher_mac.h:10: #include <set>
On 2017/04/05 17:16:54, Robert Sesek wrote:
> Not needed in the .h

Done.

[commit-bot: I haz the power, 2017-04-05 19:19:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-05 19:19:54]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Peter Beverloo, 2017-04-05 23:26:54]

On 2017/04/05 19:19:54, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

OSError: [Errno 2] No such file or directory:
'/Library/Developer/CommandLineTools/Platforms/MacOSX.platform/Developer/SDKs'

Looks like that Mac tryserver is busted:

https://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel...

Which is tracked in crbug.com/708657 - let's try again since it looks like some
CLs are slipping through.

[Peter Beverloo, 2017-04-05 23:27:09]

The CQ bit was checked by peter@chromium.org

[commit-bot: I haz the power, 2017-04-05 23:27:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-05 23:58:02]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1491434829522910,
"parent_rev": "971169a30d72887e285e03db2e524a6b3a15d3bb", "commit_rev":
"76bc01769c89fd75526253472222a9854533100a"}

[commit-bot: I haz the power, 2017-04-05 23:58:57]

Description was changed from

==========
Change the flow on mac so that in addition to the local banners XPC alerts are
also considered.

BUG=571056
==========

to

==========
Change the flow on mac so that in addition to the local banners XPC alerts are
also considered.

BUG=571056

Review-Url: https://codereview.chromium.org/2709213005
Cr-Commit-Position: refs/heads/master@{#462281}
Committed:
https://chromium.googlesource.com/chromium/src/+/76bc01769c89fd75526253472222...
==========

[commit-bot: I haz the power, 2017-04-05 23:59:03]

Committed patchset #6 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/76bc01769c89fd75526253472222...