Abstract notification clicks in its own dictionary

Abstract notification clicks in its own dictionary

So it can be reused by both chrome and a future XPC service.

BUG=571056
Committed: https://crrev.com/e3147b73a1a7eb67ac23b326e61512a0188d4ea4
Cr-Commit-Position: refs/heads/master@{#401030}

[Miguel Garcia, 2016-06-14 11:19:37]

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

[Miguel Garcia, 2016-06-14 11:19:37]

Requires https://codereview.chromium.org/2053613002/  (although the diff show
all the combined changes so this CL as is works standalone, it will just become
smaller once the pre-requisite lands)

Full picture CL for reference:
https://chromiumcodereview.appspot.com/2047643003/

[Robert Sesek, 2016-06-14 14:07:22]

On 2016/06/14 11:19:37, Miguel Garcia wrote:
> Requires https://codereview.chromium.org/2053613002/  (although the diff show
> all the combined changes so this CL as is works standalone, it will just
become
> smaller once the pre-requisite lands)

Can you rebase it and set that dependent CL as the upstream branch so that we
only have to review the differences on top of that?

[Miguel Garcia, 2016-06-14 20:17:03]

On 2016/06/14 14:07:22, Robert Sesek wrote:
> On 2016/06/14 11:19:37, Miguel Garcia wrote:
> > Requires https://codereview.chromium.org/2053613002/  (although the diff
show
> > all the combined changes so this CL as is works standalone, it will just
> become
> > smaller once the pre-requisite lands)
> 
> Can you rebase it and set that dependent CL as the upstream branch so that we
> only have to review the differences on top of that?

Done. Thanks for the tip :)

[Peter Beverloo, 2016-06-16 17:02:43]

rs lgtm, please wait for Robert's sign-off before landing.

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

https://codereview.chromium.org/2065943002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_mac.mm:191:
[NotificationResponseBuilder buildDictionary:notification];
Could be pedantic and add a whole lot of DCHECKS to this method, since we
control both sides of the usage.

https://codereview.chromium.org/2065943002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_mac.mm:199: [response
objectForKey:notification_constants::kNotificationOrigin]);
nit: would be good to consistently use (or not use) the `notification` prefix in
the local variable names. Right now it is:

kNotificationButtonIndex -> buttonIndex
kNotificationOperation -> operation
kNotificationOrigin -> *notification*Origin
kNotificationId -> *notification*Id
kNotificationProfileId -> profileId
kNotificationIncognito -> isIncognito

https://codereview.chromium.org/2065943002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_mac.mm:208: }
This method has a mixture of macStyle and unix_style variables (notification_id,
notificationId, persistent_notification_id). Since this is pure Mac code,
shouldn't it always be macStyle?

https://codereview.chromium.org/2065943002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/notifications/notification_builder_mac_unittest.mm
(right):

https://codereview.chromium.org/2065943002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/notifications/notification_builder_mac_unittest.mm:11:
#include "chrome/browser/ui/cocoa/notifications/notification_constants_mac.h"
You use #include for this file here, #import elsewhere. In my understanding, the
only difference is that "#import" ensures that a file is only being included
once. Maybe make that consistent?

https://codereview.chromium.org/2065943002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/notifications/notification_response_builder_mac.mm
(right):

https://codereview.chromium.org/2065943002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/notifications/notification_response_builder_mac.mm:21: }
nits: types start with a capital, blank line above this one, and add a "//
namespace" comment

That also is a really old way to define an enum :-). We can't use an enum class
because you need the integral value, but you could write it like:

enum NotificationOperation {
  NOTIFICATION_CLICK = 0,
  NOTIFICATION_CLOSE = 1,
  NOTIFICATION_SETTINGS = 2
}

https://codereview.chromium.org/2065943002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/notifications/notification_response_builder_mac.mm:92:
kNotificationOperation : [NSNumber numberWithInt:operation],
nit: could we wrap after the colon, avoiding breaking in the middle of the
constant's name?

[Miguel Garcia, 2016-06-17 17:53:08]

Robert can you have a final look? 

I didn't add the DCHECKs yet but will do before submitting.

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

https://codereview.chromium.org/2065943002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_mac.mm:199: [response
objectForKey:notification_constants::kNotificationOrigin]);
so I am using notificationId because id is a semi reserved keyword in
objective-C
for notificationOrigin I just did it so that I can call the final variable
origin (the GURL in 216)


On 2016/06/16 17:02:43, Peter Beverloo - OOO June 8-16 wrote:
> nit: would be good to consistently use (or not use) the `notification` prefix
in
> the local variable names. Right now it is:
> 
> kNotificationButtonIndex -> buttonIndex
> kNotificationOperation -> operation
> kNotificationOrigin -> *notification*Origin
> kNotificationId -> *notification*Id
> kNotificationProfileId -> profileId
> kNotificationIncognito -> isIncognito

https://codereview.chromium.org/2065943002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_mac.mm:208: }
On 2016/06/16 17:02:42, Peter Beverloo - OOO June 8-16 wrote:
> This method has a mixture of macStyle and unix_style variables
(notification_id,
> notificationId, persistent_notification_id). Since this is pure Mac code,
> shouldn't it always be macStyle?

Yeah good point.

https://codereview.chromium.org/2065943002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/notifications/notification_builder_mac_unittest.mm
(right):

https://codereview.chromium.org/2065943002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/notifications/notification_builder_mac_unittest.mm:11:
#include "chrome/browser/ui/cocoa/notifications/notification_constants_mac.h"
On 2016/06/16 17:02:43, Peter Beverloo - OOO June 8-16 wrote:
> You use #include for this file here, #import elsewhere. In my understanding,
the
> only difference is that "#import" ensures that a file is only being included
> once. Maybe make that consistent?

Should be include everywhere since it's a c++ file.

https://codereview.chromium.org/2065943002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/notifications/notification_response_builder_mac.mm
(right):

https://codereview.chromium.org/2065943002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/notifications/notification_response_builder_mac.mm:21: }
Yeah I looked it up to see how people defined enums in Obj-C and this what it
came out (presumably because most of the code out there is Obj-C not Obj-C++).
Looking in chrome though it seems regular C++ enums are fine so changing this.

On 2016/06/16 17:02:43, Peter Beverloo - OOO June 8-16 wrote:
> nits: types start with a capital, blank line above this one, and add a "//
> namespace" comment
> 
> That also is a really old way to define an enum :-). We can't use an enum
class
> because you need the integral value, but you could write it like:
> 
> enum NotificationOperation {
>   NOTIFICATION_CLICK = 0,
>   NOTIFICATION_CLOSE = 1,
>   NOTIFICATION_SETTINGS = 2
> }

https://codereview.chromium.org/2065943002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/notifications/notification_response_builder_mac.mm:92:
kNotificationOperation : [NSNumber numberWithInt:operation],
On 2016/06/16 17:02:43, Peter Beverloo - OOO June 8-16 wrote:
> nit: could we wrap after the colon, avoiding breaking in the middle of the
> constant's name?

Done.

[Robert Sesek, 2016-06-17 18:48:25]

LGTM

[Miguel Garcia, 2016-06-21 16:33:47]

The CQ bit was checked by miguelg@chromium.org

[Miguel Garcia, 2016-06-21 16:33:48]

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/2065943002/#ps80001
(title: "rebase")

[commit-bot: I haz the power, 2016-06-21 16:34:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2065943002/80001

[commit-bot: I haz the power, 2016-06-21 17:39:25]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-06-21 17:49:54]

Description was changed from

==========
Abstract notification clicks in its own dictionary

So it can be reused by both chrome and a future XPC service.

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

to

==========
Abstract notification clicks in its own dictionary

So it can be reused by both chrome and a future XPC service.

BUG=571056

Committed: https://crrev.com/e3147b73a1a7eb67ac23b326e61512a0188d4ea4
Cr-Commit-Position: refs/heads/master@{#401030}
==========

[commit-bot: I haz the power, 2016-06-21 17:49:55]

Patchset 5 (id:??) landed as
https://crrev.com/e3147b73a1a7eb67ac23b326e61512a0188d4ea4
Cr-Commit-Position: refs/heads/master@{#401030}