Introduce a new API to handle native notification clicks

Introduce a new API to handle native notification clicks

BUG=596161, 595487
Committed: https://crrev.com/87e986d94f7bf82930af385d88d7e3b79eac4515
Cr-Commit-Position: refs/heads/master@{#404436}

[Miguel Garcia, 2016-06-28 16:21:03]

Description was changed from

==========
WIP: Introduce a new API to handle native notification clicks

BUG=
==========

to

==========
Introduce a new API to handle native notification clicks

BUG=
==========

[Miguel Garcia, 2016-06-28 16:21:41]

Description was changed from

==========
Introduce a new API to handle native notification clicks

BUG=
==========

to

==========
Introduce a new API to handle native notification clicks

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

[Miguel Garcia, 2016-06-28 17:27:29]

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

[Miguel Garcia, 2016-06-28 17:27:30]

This is the larger CL that allows clicks to be propagated back from native
notifications in the right way. With this patch in both persistent and in-page
notification click and close events are properly propagated.

Only Mac is implemented right now but since history tells we will end up
changing the API I rather send this out now and do android once we agree on the
final shape.

I have also chopped individual pieces in different CLs that would be
prerequisites of this one (which should become smaller once the other land).

Fix a few bugs when closing mac notifications
https://chromiumcodereview.appspot.com/2100663002/

Refactor notification operation functionality out of PNS
https://chromiumcodereview.appspot.com/2095223002/

Verify that the notification response contains sensible data
https://chromiumcodereview.appspot.com/2105863002/

[Peter Beverloo, 2016-06-29 00:23:16]

A few higher-level comments. This increases duplication and complexity *a lot*,
do we really need this for M54?

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

https://codereview.chromium.org/2093953002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/in_page_notification_handler.h:18: class
InPageNotificationHandler : public NotificationHandler {
Please be consistent in your terminology. We have persistent and non-persistent
Web Notifications. I would argue that, at this level, we should even emphasize
on the fact that they're *Web* Notifications.

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

https://codereview.chromium.org/2093953002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_display_service.h:32:
notification_operation_common::NotificationHandlerType notification_type,
In the longer run, taking the services model, this knowledge needs to be removed
from the notification "service" entirely.

Let's not ignore that— could we consider a GCMAppHandler::CanHandler-kind of
construct where identification of owned notifications is the handler's
responsibility? (Is that even sufficient?) It would completely remove the need
for these arguments at least.

Would a service own a token-based registry of sorts?

https://codereview.chromium.org/2093953002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_display_service.h:32:
notification_operation_common::NotificationHandlerType notification_type,
Needing to use the `notification_operation_common` prefix every time a
notification handler type or operation is mentioned is unfortunately long. We
can't use enum classes since it needs to be casted to an int, either...

Could we instead put the enumeration in the global scope w/ values such as
NOTIFICATION_TYPE_NON_PERSISTENT?

https://codereview.chromium.org/2093953002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_display_service.h:53: int
action_index) = 0;
NotificationDisplayService defines the [feature] -> [notification system]
interface. This method is an implementation detail of the [notification system]
that [feature] does not have to know about, so should not be part of the public
API.

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

https://codereview.chromium.org/2093953002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_handler.h:28:
notification_operation_common::NotificationOperation operation,
Could we instead just have multiple methods on the NotificationHandler
interface?

OnClick(Profile* profile, const std::string& origin,
        const std::string& notification_id, int action_index);
OnSettingsClick(Profile* profile, const std::string& origin,
                const std::string& notification_id);
OnClose(Profile* profile, const std::string& origin,
        const std::string& notification_id);

(Do they all need all this information?)

https://codereview.chromium.org/2093953002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_handler.h:35: virtual void
RegisterNotification(
This should be the displayer's responsibility. For example, for persistent
notifications we would now always allocate a Notification object, which gets
destroyed immediately because the handler doesn't need it.

https://codereview.chromium.org/2093953002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_handler.h:40: virtual void
CloseNotification(Profile* profile,
This should be a |by_user| boolean flag in the `OnClose` method, presumably?

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

https://codereview.chromium.org/2093953002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_operation_common.h:19:
NOTIFICATION_HANDLER_TYPE_LAST = INPAGE_NOTIFICATION
Why do you call this _LAST, but the enum eight lines down _MAX?

[Miguel Garcia, 2016-07-05 13:00:26]

Description was changed from

==========
Introduce a new API to handle native notification clicks

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

to

==========
Introduce a new API to handle native notification clicks

BUG=596161,595487
==========

[Miguel Garcia, 2016-07-05 13:53:23]

Patchset #4 (id:60001) has been deleted

[Miguel Garcia, 2016-07-05 13:54:50]

Ok so all the dependent patches have landed and this is now rebased on top. I
also went through all the high level comments. So I think this is ready for
another round.

If the review get longs but you feel it's still in shape to implement android do
let me know.

https://chromiumcodereview.appspot.com/2093953002/diff/40001/chrome/browser/n...
File chrome/browser/notifications/in_page_notification_handler.h (right):

https://chromiumcodereview.appspot.com/2093953002/diff/40001/chrome/browser/n...
chrome/browser/notifications/in_page_notification_handler.h:18: class
InPageNotificationHandler : public NotificationHandler {
On 2016/06/29 00:23:15, Peter Beverloo wrote:
> Please be consistent in your terminology. We have persistent and
non-persistent
> Web Notifications. I would argue that, at this level, we should even emphasize
> on the fact that they're *Web* Notifications.

Renamed to NonPersistentNotificationHandler Adding Web there would make it even
longer so I am shying away from it.

https://chromiumcodereview.appspot.com/2093953002/diff/40001/chrome/browser/n...
File chrome/browser/notifications/notification_display_service.h (right):

https://chromiumcodereview.appspot.com/2093953002/diff/40001/chrome/browser/n...
chrome/browser/notifications/notification_display_service.h:32:
notification_operation_common::NotificationHandlerType notification_type,
On 2016/06/29 00:23:15, Peter Beverloo wrote:
> Needing to use the `notification_operation_common` prefix every time a
> notification handler type or operation is mentioned is unfortunately long. We
> can't use enum classes since it needs to be casted to an int, either...
> 
> Could we instead put the enumeration in the global scope w/ values such as
> NOTIFICATION_TYPE_NON_PERSISTENT?

I went with a class and enum classes.

https://chromiumcodereview.appspot.com/2093953002/diff/40001/chrome/browser/n...
chrome/browser/notifications/notification_display_service.h:32:
notification_operation_common::NotificationHandlerType notification_type,
As discussed offline I am maintaining the type enum for now given that we have a
finite and known set of notification types.

On 2016/06/29 00:23:15, Peter Beverloo wrote:
> In the longer run, taking the services model, this knowledge needs to be
removed
> from the notification "service" entirely.
> 
> Let's not ignore that— could we consider a GCMAppHandler::CanHandler-kind of
> construct where identification of owned notifications is the handler's
> responsibility? (Is that even sufficient?) It would completely remove the need
> for these arguments at least.
> 
> Would a service own a token-based registry of sorts?

https://chromiumcodereview.appspot.com/2093953002/diff/40001/chrome/browser/n...
chrome/browser/notifications/notification_display_service.h:53: int
action_index) = 0;
On 2016/06/29 00:23:15, Peter Beverloo wrote:
> NotificationDisplayService defines the [feature] -> [notification system]
> interface. This method is an implementation detail of the [notification
system]
> that [feature] does not have to know about, so should not be part of the
public
> API.

Indeed, made it a method of the native display service and removed it from the
overall interface.

https://chromiumcodereview.appspot.com/2093953002/diff/40001/chrome/browser/n...
File chrome/browser/notifications/notification_handler.h (right):

https://chromiumcodereview.appspot.com/2093953002/diff/40001/chrome/browser/n...
chrome/browser/notifications/notification_handler.h:28:
notification_operation_common::NotificationOperation operation,
On 2016/06/29 00:23:16, Peter Beverloo wrote:
> Could we instead just have multiple methods on the NotificationHandler
> interface?
> 
> OnClick(Profile* profile, const std::string& origin,
>         const std::string& notification_id, int action_index);
> OnSettingsClick(Profile* profile, const std::string& origin,
>                 const std::string& notification_id);
> OnClose(Profile* profile, const std::string& origin,
>         const std::string& notification_id);
> 
> (Do they all need all this information?)

Done.

https://chromiumcodereview.appspot.com/2093953002/diff/40001/chrome/browser/n...
chrome/browser/notifications/notification_handler.h:35: virtual void
RegisterNotification(
On 2016/06/29 00:23:16, Peter Beverloo wrote:
> This should be the displayer's responsibility. For example, for persistent
> notifications we would now always allocate a Notification object, which gets
> destroyed immediately because the handler doesn't need it.

As agreed offline I moved to only storing the delegates so we don't create
useless notification objects.

https://chromiumcodereview.appspot.com/2093953002/diff/40001/chrome/browser/n...
chrome/browser/notifications/notification_handler.h:40: virtual void
CloseNotification(Profile* profile,
On 2016/06/29 00:23:16, Peter Beverloo wrote:
> This should be a |by_user| boolean flag in the `OnClose` method, presumably?

Done.

https://chromiumcodereview.appspot.com/2093953002/diff/40001/chrome/browser/n...
File chrome/browser/notifications/notification_operation_common.h (right):

https://chromiumcodereview.appspot.com/2093953002/diff/40001/chrome/browser/n...
chrome/browser/notifications/notification_operation_common.h:19:
NOTIFICATION_HANDLER_TYPE_LAST = INPAGE_NOTIFICATION
On 2016/06/29 00:23:16, Peter Beverloo wrote:
> Why do you call this _LAST, but the enum eight lines down _MAX?

Done.

[Peter Beverloo, 2016-07-05 14:25:35]

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
File chrome/browser/notifications/message_center_display_service.cc (right):

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
chrome/browser/notifications/message_center_display_service.cc:22:
ui_manager_->Add(notification, profile_);
Maybe add a TODO saying that the MCDS should stop relying on the
|notification|'s delegate? Not feasible to do for now, though, until we move to
the mojom for |notification|.

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
File chrome/browser/notifications/native_notification_display_service.cc
(right):

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
chrome/browser/notifications/native_notification_display_service.cc:45:
AddNotificationHandler(persistent->NotificationType(), std::move(persistent));
These are the only two calls to the NotificationType() method. May be better to
remove it entirely from the NotificationHandler class, and instead register this
as:

  AddNotificationHandler(NotificationCommon::PERSISTENT,
                         base::MakeUnique<PersistentNotificationHandler>());
  AddNotificationHandler(NotificationCommon::NON_PERSISTENT,
                         base::MakeUnique<NonPersistentNotificationHandler>());

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
chrome/browser/notifications/native_notification_display_service.cc:45:
AddNotificationHandler(persistent->NotificationType(), std::move(persistent));
Brainfart:

What if we add a GetNotificationHandler() method, and use that in
PlatformNotificationServiceImpl::Display() to register non-persistent
notifications? That allows us to kill RegisterNotification() from the
NotificationHandler altogether.

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
chrome/browser/notifications/native_notification_display_service.cc:45:
AddNotificationHandler(persistent->NotificationType(), std::move(persistent));
Do you think it'd make sense to change PlatformNotificationServiceImpl to be
per-profile as well, in which case it can register these handlers rather than
having the knowledge here?

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
chrome/browser/notifications/native_notification_display_service.cc:49:
notification_handlers_.clear();
Why is this necessary? The map's destructor will do this for us, right?

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
chrome/browser/notifications/native_notification_display_service.cc:73: // the
origin.
It seems to me like the GetNotificationHandler() idea could address this too?

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
chrome/browser/notifications/native_notification_display_service.cc:89:
NotificationHandler* handler = GetNotificationHandler(notification_type);
Handle the |!handler| case— CHECK()?

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
chrome/browser/notifications/native_notification_display_service.cc:95:
handler->Close(profile_, origin, notification_id, true /*by user*/);
nit: /* by_user */

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
chrome/browser/notifications/native_notification_display_service.cc:120: <<
notification_type << " NOT REGISTERED";
NOT REGISTERED -> is not registered.

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
chrome/browser/notifications/native_notification_display_service.cc:120: <<
notification_type << " NOT REGISTERED";
Maybe it should just be OK to return nullptrs here?

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
File chrome/browser/notifications/native_notification_display_service.h (right):

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
chrome/browser/notifications/native_notification_display_service.h:46: void
AddNotificationHandler(NotificationCommon::Type notification_type,
Please document public APIs.

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
chrome/browser/notifications/native_notification_display_service.h:50: bool
SupportsNotificationCenter() const override;
Move this to line 38.

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
chrome/browser/notifications/native_notification_display_service.h:54:
NotificationCommon::Type notification_type);
nit: const?

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
chrome/browser/notifications/native_notification_display_service.h:58:
std::map<NotificationCommon::Type, std::unique_ptr<NotificationHandler>>
nit: unordered_map

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
File chrome/browser/notifications/nonpersistent_notification_handler.cc (right):

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
chrome/browser/notifications/nonpersistent_notification_handler.cc:20: }
nit (avoids multiple lookups - elsewhere too):

auto iter = notifications_.find(notification_id);
if (iter != notifications_.end()) {
  iter->Close(by_user);
  notifications_.erase(iter);
}

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
chrome/browser/notifications/nonpersistent_notification_handler.cc:28:
DCHECK(action_index = -1);
This is not doing what you want it to do :)

DCHECK_EQ(action_index, -1);

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
File chrome/browser/notifications/nonpersistent_notification_handler.h (right):

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
chrome/browser/notifications/nonpersistent_notification_handler.h:1: //
Copyright 2016 The Chromium Authors. All rights reserved.
nit: non_persistent_notification_handler.h

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
chrome/browser/notifications/nonpersistent_notification_handler.h:18: //
notifications.
nit: No need to wrap.

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
File chrome/browser/notifications/notification_common.h (right):

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
chrome/browser/notifications/notification_common.h:24: enum Type { PERSISTENT =
0, NONPERSISTENT = 1, TYPE_MAX = NONPERSISTENT };
Please place each type on their own line.

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
chrome/browser/notifications/notification_common.h:24: enum Type { PERSISTENT =
0, NONPERSISTENT = 1, TYPE_MAX = NONPERSISTENT };
nit: NONPERSISTENT -> NON_PERSISTENT

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
File chrome/browser/notifications/notification_display_service.h (right):

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
chrome/browser/notifications/notification_display_service.h:10: #include
"base/macros.h"
nit: blank line above here

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
File chrome/browser/notifications/notification_handler.h (right):

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
chrome/browser/notifications/notification_handler.h:19: // the right kind of
NotificationHandler will deal with the operation.
nit: remove lines 18/19- it's partly implied, partly not the responsibility of
the handler.

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
chrome/browser/notifications/notification_handler.h:26: // This is driven by the
|by_user| flag.
This method observes notifications being closed for the specific type of handler
(so is informed after-the-fact), but this comment makes me think this method
*closes* notifications with the given details.

Please consider naming this OnClose or DidClose to distinguish. Dito re: the
other methods in this class.

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
chrome/browser/notifications/notification_handler.h:39: virtual void
Settings(Profile* profile) = 0;
nit: OpenSettings

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
File chrome/browser/notifications/notification_platform_bridge_mac.mm (right):

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
chrome/browser/notifications/notification_platform_bridge_mac.mm:52: // Callback
to run once the profile has been loaded in order to perform a
micro nit: blank line above here

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
chrome/browser/notifications/notification_platform_bridge_mac.mm:262: LOG(ERROR)
<< "ProfileId not provided";
Bad merge?

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/u...
File chrome/browser/ui/cocoa/notifications/notification_builder_mac_unittest.mm
(right):

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/u...
chrome/browser/ui/cocoa/notifications/notification_builder_mac_unittest.mm:25:
[builder setNotificationType:[NSNumber numberWithInt:1]];
Can we use the enum values here w/ a cast? (For clarity.)

dito elsewhere in this file

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/chrome_br...
File chrome/chrome_browser.gypi (right):

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/chrome_br...
chrome/chrome_browser.gypi:2242:
'browser/notifications/persistent_notification_handler.cc',
micro nit: cc first (alphabetize)

[Miguel Garcia, 2016-07-05 17:12:53]

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
File chrome/browser/notifications/message_center_display_service.cc (right):

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
chrome/browser/notifications/message_center_display_service.cc:22:
ui_manager_->Add(notification, profile_);
On 2016/07/05 14:25:33, Peter Beverloo wrote:
> Maybe add a TODO saying that the MCDS should stop relying on the
> |notification|'s delegate? Not feasible to do for now, though, until we move
to
> the mojom for |notification|.

Done.

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
File chrome/browser/notifications/native_notification_display_service.cc
(right):

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
chrome/browser/notifications/native_notification_display_service.cc:45:
AddNotificationHandler(persistent->NotificationType(), std::move(persistent));
On 2016/07/05 14:25:34, Peter Beverloo wrote:
> Do you think it'd make sense to change PlatformNotificationServiceImpl to be
> per-profile as well, in which case it can register these handlers rather than
> having the knowledge here?

Maybe eventually? I would like to continue removing trimming the functionality
of that class (for example by moving the actual implementation of
On{Click/Close} to the handlers) before we consider that.

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
chrome/browser/notifications/native_notification_display_service.cc:45:
AddNotificationHandler(persistent->NotificationType(), std::move(persistent));
I was struggling with this, I thought the knowledge of what kind of type a
handler is for should remain in the handler itself but if you feel it's ok to
have this class the one that links what a given delegate implementation is for
let's do it.

On 2016/07/05 14:25:34, Peter Beverloo wrote:
> These are the only two calls to the NotificationType() method. May be better
to
> remove it entirely from the NotificationHandler class, and instead register
this
> as:
> 
>   AddNotificationHandler(NotificationCommon::PERSISTENT,
>                          base::MakeUnique<PersistentNotificationHandler>());
>   AddNotificationHandler(NotificationCommon::NON_PERSISTENT,
>                         
base::MakeUnique<NonPersistentNotificationHandler>());

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
chrome/browser/notifications/native_notification_display_service.cc:45:
AddNotificationHandler(persistent->NotificationType(), std::move(persistent));
On 2016/07/05 14:25:33, Peter Beverloo wrote:
> Brainfart:
> 
> What if we add a GetNotificationHandler() method, and use that in
> PlatformNotificationServiceImpl::Display() to register non-persistent
> notifications? That allows us to kill RegisterNotification() from the
> NotificationHandler altogether.

Well extensions will need something similar too.. In general it feels like the
map of delegates kind of belongs to the display machinery, (like in the case of
message center) not the platform notification service.

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
chrome/browser/notifications/native_notification_display_service.cc:49:
notification_handlers_.clear();
On 2016/07/05 14:25:33, Peter Beverloo wrote:
> Why is this necessary? The map's destructor will do this for us, right?
indeed

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
chrome/browser/notifications/native_notification_display_service.cc:89:
NotificationHandler* handler = GetNotificationHandler(notification_type);
On 2016/07/05 14:25:34, Peter Beverloo wrote:
> Handle the |!handler| case— CHECK()?

Added a check, the GetNotificationHandler itself has a DCHECK and it's really
never meant to return an unknown value so I guess this has us covered.

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
chrome/browser/notifications/native_notification_display_service.cc:95:
handler->Close(profile_, origin, notification_id, true /*by user*/);
On 2016/07/05 14:25:34, Peter Beverloo wrote:
> nit: /* by_user */

Done.

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
chrome/browser/notifications/native_notification_display_service.cc:120: <<
notification_type << " NOT REGISTERED";
On 2016/07/05 14:25:34, Peter Beverloo wrote:
> Maybe it should just be OK to return nullptrs here?

In what situation do you think returning a null pointer makes sense? We register
the only two possible values in the constructor.

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
chrome/browser/notifications/native_notification_display_service.cc:120: <<
notification_type << " NOT REGISTERED";
On 2016/07/05 14:25:34, Peter Beverloo wrote:
> NOT REGISTERED -> is not registered.

Done.

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
File chrome/browser/notifications/native_notification_display_service.h (right):

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
chrome/browser/notifications/native_notification_display_service.h:46: void
AddNotificationHandler(NotificationCommon::Type notification_type,
On 2016/07/05 14:25:34, Peter Beverloo wrote:
> Please document public APIs.

Done.

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
chrome/browser/notifications/native_notification_display_service.h:50: bool
SupportsNotificationCenter() const override;
On 2016/07/05 14:25:34, Peter Beverloo wrote:
> Move this to line 38.

Done.

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
chrome/browser/notifications/native_notification_display_service.h:54:
NotificationCommon::Type notification_type);
On 2016/07/05 14:25:34, Peter Beverloo wrote:
> nit: const?

I know if I add it will compile but it feels like const should really mean that
you cannot change the state of the object and here you can (and we do) change
the state of the handler itself (for example when adding/removing delegates).

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
chrome/browser/notifications/native_notification_display_service.h:58:
std::map<NotificationCommon::Type, std::unique_ptr<NotificationHandler>>
On 2016/07/05 14:25:34, Peter Beverloo wrote:
> nit: unordered_map

C++ 11 does not easily allow unordered maps keyed by enums unfortunately.  There
are some work around to this but for a map of two, eventually three, elements is
probably not worth it.

Once we move to C++14 it should be possible.

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
File chrome/browser/notifications/nonpersistent_notification_handler.cc (right):

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
chrome/browser/notifications/nonpersistent_notification_handler.cc:20: }
But unordered_map does not allow lookups by iterator right? 
Do you want me to switch to map?

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
chrome/browser/notifications/nonpersistent_notification_handler.cc:28:
DCHECK(action_index = -1);
On 2016/07/05 14:25:34, Peter Beverloo wrote:
> This is not doing what you want it to do :)
> 
> DCHECK_EQ(action_index, -1);

changed ;)

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
File chrome/browser/notifications/nonpersistent_notification_handler.h (right):

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
chrome/browser/notifications/nonpersistent_notification_handler.h:1: //
Copyright 2016 The Chromium Authors. All rights reserved.
On 2016/07/05 14:25:34, Peter Beverloo wrote:
> nit: non_persistent_notification_handler.h

Done.

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
chrome/browser/notifications/nonpersistent_notification_handler.h:18: //
notifications.
On 2016/07/05 14:25:34, Peter Beverloo wrote:
> nit: No need to wrap.

Done.

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
File chrome/browser/notifications/notification_common.h (right):

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
chrome/browser/notifications/notification_common.h:24: enum Type { PERSISTENT =
0, NONPERSISTENT = 1, TYPE_MAX = NONPERSISTENT };
On 2016/07/05 14:25:34, Peter Beverloo wrote:
> Please place each type on their own line.

This is super annoying. clank format decides to change it for me every time.

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
File chrome/browser/notifications/notification_display_service.h (right):

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
chrome/browser/notifications/notification_display_service.h:10: #include
"base/macros.h"
On 2016/07/05 14:25:34, Peter Beverloo wrote:
> nit: blank line above here

Done.

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
File chrome/browser/notifications/notification_handler.h (right):

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
chrome/browser/notifications/notification_handler.h:19: // the right kind of
NotificationHandler will deal with the operation.
On 2016/07/05 14:25:34, Peter Beverloo wrote:
> nit: remove lines 18/19- it's partly implied, partly not the responsibility of
> the handler.

Done.

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
chrome/browser/notifications/notification_handler.h:26: // This is driven by the
|by_user| flag.
On 2016/07/05 14:25:34, Peter Beverloo wrote:
> This method observes notifications being closed for the specific type of
handler
> (so is informed after-the-fact), but this comment makes me think this method
> *closes* notifications with the given details.
> 
> Please consider naming this OnClose or DidClose to distinguish. Dito re: the
> other methods in this class.
Added the On* prefix

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
chrome/browser/notifications/notification_handler.h:39: virtual void
Settings(Profile* profile) = 0;
On 2016/07/05 14:25:34, Peter Beverloo wrote:
> nit: OpenSettings

Done.

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
File chrome/browser/notifications/notification_platform_bridge_mac.mm (right):

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
chrome/browser/notifications/notification_platform_bridge_mac.mm:52: // Callback
to run once the profile has been loaded in order to perform a
On 2016/07/05 14:25:35, Peter Beverloo wrote:
> micro nit: blank line above here

Done.

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/n...
chrome/browser/notifications/notification_platform_bridge_mac.mm:262: LOG(ERROR)
<< "ProfileId not provided";
On 2016/07/05 14:25:35, Peter Beverloo wrote:
> Bad merge?

Yeah sorry.

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/u...
File chrome/browser/ui/cocoa/notifications/notification_builder_mac_unittest.mm
(right):

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/browser/u...
chrome/browser/ui/cocoa/notifications/notification_builder_mac_unittest.mm:25:
[builder setNotificationType:[NSNumber numberWithInt:1]];
On 2016/07/05 14:25:35, Peter Beverloo wrote:
> Can we use the enum values here w/ a cast? (For clarity.)
> 
> dito elsewhere in this file

Ok.. In theory the builder should not really know about the enum (since it lives
in a different compilation unit) but I guess for the test it's fine.

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/chrome_br...
File chrome/chrome_browser.gypi (right):

https://chromiumcodereview.appspot.com/2093953002/diff/80001/chrome/chrome_br...
chrome/chrome_browser.gypi:2242:
'browser/notifications/persistent_notification_handler.cc',
On 2016/07/05 14:25:35, Peter Beverloo wrote:
> micro nit: cc first (alphabetize)

Done.

[Peter Beverloo, 2016-07-05 23:44:18]

Current state sgtm for iteration, % android of course :)

https://chromiumcodereview.appspot.com/2093953002/diff/100001/chrome/browser/...
File chrome/browser/notifications/native_notification_display_service.h (right):

https://chromiumcodereview.appspot.com/2093953002/diff/100001/chrome/browser/...
chrome/browser/notifications/native_notification_display_service.h:38: 
micro nit: no new line (the comment at 31 starts a block of "overridden
methods", this is common throughout the project)

https://chromiumcodereview.appspot.com/2093953002/diff/100001/chrome/browser/...
chrome/browser/notifications/native_notification_display_service.h:53: //
Removes an implementation added via |AddNotificationHandler|
nit: finish the comments with periods on lines 41, 49 and 53...

https://chromiumcodereview.appspot.com/2093953002/diff/100001/chrome/browser/...
File chrome/browser/notifications/notification_common.h (right):

https://chromiumcodereview.appspot.com/2093953002/diff/100001/chrome/browser/...
chrome/browser/notifications/notification_common.h:26: NONPERSISTENT = 1,
Carry-over nit: NONPERSISTENT -> NON_PERSISTENT

https://chromiumcodereview.appspot.com/2093953002/diff/100001/chrome/browser/...
File chrome/browser/notifications/notification_handler.h (right):

https://chromiumcodereview.appspot.com/2093953002/diff/100001/chrome/browser/...
chrome/browser/notifications/notification_handler.h:15: // to process operations
coming from the user.
Odd line break again...

https://chromiumcodereview.appspot.com/2093953002/diff/100001/chrome/browser/...
chrome/browser/notifications/notification_handler.h:36: // Registers a
|Notification| object with this handler.
There is no |Notification|.

https://chromiumcodereview.appspot.com/2093953002/diff/100001/chrome/browser/...
File chrome/browser/ui/cocoa/notifications/notification_builder_mac_unittest.mm
(right):

https://chromiumcodereview.appspot.com/2093953002/diff/100001/chrome/browser/...
chrome/browser/ui/cocoa/notifications/notification_builder_mac_unittest.mm:29:
NotificationCommon::Type>(1)]];
wait what? why not:

[builder setNotificationType:[NSNumber
numberWithInt:NotificationCommon::PERSISTENT]];

(dito in notification_response_builder_mac_unittest.mm)

https://chromiumcodereview.appspot.com/2093953002/diff/100001/chrome/browser/...
File chrome/browser/ui/cocoa/notifications/notification_response_builder_mac.mm
(right):

https://chromiumcodereview.appspot.com/2093953002/diff/100001/chrome/browser/...
chrome/browser/ui/cocoa/notifications/notification_response_builder_mac.mm:95:
kNotificationOperation : [NSNumber numberWithInt:operation],
indent += 4?

[Miguel Garcia, 2016-07-06 15:37:29]

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

[Miguel Garcia, 2016-07-06 15:37:30]

+Robert for the mac pieces while I finish Android.

[Robert Sesek, 2016-07-06 16:50:39]

LGTM w/ a nit

https://chromiumcodereview.appspot.com/2093953002/diff/100001/chrome/browser/...
File chrome/browser/ui/cocoa/notifications/notification_builder_mac.mm (right):

https://chromiumcodereview.appspot.com/2093953002/diff/100001/chrome/browser/...
chrome/browser/ui/cocoa/notifications/notification_builder_mac.mm:222: 
nit: extra blank line

[Miguel Garcia, 2016-07-07 11:02:29]

Ok Android done. As I mentioned live there are a few too many conversions from
int_64 to string and back since that avoided me to touch the java layer and make
the CL even larger. I will clean it up in a follow up CL.

https://chromiumcodereview.appspot.com/2093953002/diff/100001/chrome/browser/...
File chrome/browser/notifications/native_notification_display_service.h (right):

https://chromiumcodereview.appspot.com/2093953002/diff/100001/chrome/browser/...
chrome/browser/notifications/native_notification_display_service.h:38: 
On 2016/07/05 23:44:17, Peter Beverloo wrote:
> micro nit: no new line (the comment at 31 starts a block of "overridden
> methods", this is common throughout the project)

Done.

https://chromiumcodereview.appspot.com/2093953002/diff/100001/chrome/browser/...
chrome/browser/notifications/native_notification_display_service.h:53: //
Removes an implementation added via |AddNotificationHandler|
On 2016/07/05 23:44:17, Peter Beverloo wrote:
> nit: finish the comments with periods on lines 41, 49 and 53...

Done.

https://chromiumcodereview.appspot.com/2093953002/diff/100001/chrome/browser/...
File chrome/browser/notifications/notification_common.h (right):

https://chromiumcodereview.appspot.com/2093953002/diff/100001/chrome/browser/...
chrome/browser/notifications/notification_common.h:26: NONPERSISTENT = 1,
On 2016/07/05 23:44:17, Peter Beverloo wrote:
> Carry-over nit: NONPERSISTENT -> NON_PERSISTENT

Done.

https://chromiumcodereview.appspot.com/2093953002/diff/100001/chrome/browser/...
File chrome/browser/notifications/notification_handler.h (right):

https://chromiumcodereview.appspot.com/2093953002/diff/100001/chrome/browser/...
chrome/browser/notifications/notification_handler.h:15: // to process operations
coming from the user.
On 2016/07/05 23:44:17, Peter Beverloo wrote:
> Odd line break again...

Done.

https://chromiumcodereview.appspot.com/2093953002/diff/100001/chrome/browser/...
chrome/browser/notifications/notification_handler.h:36: // Registers a
|Notification| object with this handler.
On 2016/07/05 23:44:18, Peter Beverloo wrote:
> There is no |Notification|.

Done.

https://chromiumcodereview.appspot.com/2093953002/diff/100001/chrome/browser/...
File chrome/browser/ui/cocoa/notifications/notification_builder_mac.mm (right):

https://chromiumcodereview.appspot.com/2093953002/diff/100001/chrome/browser/...
chrome/browser/ui/cocoa/notifications/notification_builder_mac.mm:222: 
On 2016/07/06 16:50:39, Robert Sesek wrote:
> nit: extra blank line

Done.

https://chromiumcodereview.appspot.com/2093953002/diff/100001/chrome/browser/...
File chrome/browser/ui/cocoa/notifications/notification_builder_mac_unittest.mm
(right):

https://chromiumcodereview.appspot.com/2093953002/diff/100001/chrome/browser/...
chrome/browser/ui/cocoa/notifications/notification_builder_mac_unittest.mm:29:
NotificationCommon::Type>(1)]];
On 2016/07/05 23:44:18, Peter Beverloo wrote:
> wait what? why not:
> 
> [builder setNotificationType:[NSNumber
> numberWithInt:NotificationCommon::PERSISTENT]];
> 
> (dito in notification_response_builder_mac_unittest.mm)

Removed the static_cast. The indenting is decided by git cl format .

https://chromiumcodereview.appspot.com/2093953002/diff/100001/chrome/browser/...
File chrome/browser/ui/cocoa/notifications/notification_response_builder_mac.mm
(right):

https://chromiumcodereview.appspot.com/2093953002/diff/100001/chrome/browser/...
chrome/browser/ui/cocoa/notifications/notification_response_builder_mac.mm:95:
kNotificationOperation : [NSNumber numberWithInt:operation],
On 2016/07/05 23:44:18, Peter Beverloo wrote:
> indent += 4?

Done.

[Peter Beverloo, 2016-07-08 16:06:31]

lgtm

https://chromiumcodereview.appspot.com/2093953002/diff/160001/chrome/browser/...
File chrome/browser/notifications/message_center_display_service.cc (right):

https://chromiumcodereview.appspot.com/2093953002/diff/160001/chrome/browser/...
chrome/browser/notifications/message_center_display_service.cc:24: // once the
Notification object becomes a mojom type.
nit: weird line wrapping

https://chromiumcodereview.appspot.com/2093953002/diff/160001/chrome/browser/...
File chrome/browser/notifications/notification_platform_bridge_android.cc
(right):

https://chromiumcodereview.appspot.com/2093953002/diff/160001/chrome/browser/...
chrome/browser/notifications/notification_platform_bridge_android.cc:66: //
TODO(miguelg) move it to notification_common?
What's your thought?

https://chromiumcodereview.appspot.com/2093953002/diff/160001/chrome/browser/...
chrome/browser/notifications/notification_platform_bridge_android.cc:134:
ProfileManager* profileManager = g_browser_process->profile_manager();
profile_manager (you left Mac code :-))

[Miguel Garcia, 2016-07-08 16:27:21]

The CQ bit was checked by miguelg@chromium.org

[Miguel Garcia, 2016-07-08 16:27:22]

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://chromiumcodereview.appspot.com/2093953002/#ps180001 (title: "nits")

[Miguel Garcia, 2016-07-08 16:27:27]

https://chromiumcodereview.appspot.com/2093953002/diff/160001/chrome/browser/...
File chrome/browser/notifications/message_center_display_service.cc (right):

https://chromiumcodereview.appspot.com/2093953002/diff/160001/chrome/browser/...
chrome/browser/notifications/message_center_display_service.cc:24: // once the
Notification object becomes a mojom type.
On 2016/07/08 16:06:31, Peter Beverloo wrote:
> nit: weird line wrapping

Done.

https://chromiumcodereview.appspot.com/2093953002/diff/160001/chrome/browser/...
File chrome/browser/notifications/notification_platform_bridge_android.cc
(right):

https://chromiumcodereview.appspot.com/2093953002/diff/160001/chrome/browser/...
chrome/browser/notifications/notification_platform_bridge_android.cc:66: //
TODO(miguelg) move it to notification_common?
On 2016/07/08 16:06:31, Peter Beverloo wrote:
> What's your thought?

I can actually move it (make it a static method of the NotificationCommon class)
after the follow up CL where the notification Id is passed as a string instead
of an int64. I think windows could use it as is as well which will probably make
it worth it. WDYT?

https://chromiumcodereview.appspot.com/2093953002/diff/160001/chrome/browser/...
chrome/browser/notifications/notification_platform_bridge_android.cc:134:
ProfileManager* profileManager = g_browser_process->profile_manager();
On 2016/07/08 16:06:31, Peter Beverloo wrote:
> profile_manager (you left Mac code :-))

Done :)

[commit-bot: I haz the power, 2016-07-08 16:27:59]

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, 2016-07-08 18:05:08]

Description was changed from

==========
Introduce a new API to handle native notification clicks

BUG=596161,595487
==========

to

==========
Introduce a new API to handle native notification clicks

BUG=596161,595487
==========

[commit-bot: I haz the power, 2016-07-08 18:05:09]

Committed patchset #9 (id:180001)

[commit-bot: I haz the power, 2016-07-08 18:05:39]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-08 18:09:42]

Description was changed from

==========
Introduce a new API to handle native notification clicks

BUG=596161,595487
==========

to

==========
Introduce a new API to handle native notification clicks

BUG=596161,595487

Committed: https://crrev.com/87e986d94f7bf82930af385d88d7e3b79eac4515
Cr-Commit-Position: refs/heads/master@{#404436}
==========

[commit-bot: I haz the power, 2016-07-08 18:09:43]

Patchset 9 (id:??) landed as
https://crrev.com/87e986d94f7bf82930af385d88d7e3b79eac4515
Cr-Commit-Position: refs/heads/master@{#404436}