Fix a few bugs when closing mac notifications

Fix a few bugs when closing mac notifications

The ID and profile comparisons in the native notification bridge should
use isEqualToString instead of checking the pointers.

PlatformNotificationServiceImpls#ClosePersistentNotifiction should invoke
the right close method depending on wether it's a persistent or an
in-page notification

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

[Miguel Garcia, 2016-06-25 20:33:22]

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

[Peter Beverloo, 2016-06-26 20:46:26]

https://chromiumcodereview.appspot.com/2100663002/diff/1/chrome/browser/notif...
File chrome/browser/notifications/notification_platform_bridge_mac.mm (right):

https://chromiumcodereview.appspot.com/2100663002/diff/1/chrome/browser/notif...
chrome/browser/notifications/notification_platform_bridge_mac.mm:157:
[persistent_profile_id isEqualToString:current_profile_id]) {
This seems like the kind of bug that would easily be catchable by unit tests. Do
you still plan to add some?

https://chromiumcodereview.appspot.com/2100663002/diff/1/chrome/browser/notif...
File chrome/browser/notifications/platform_notification_service_impl.cc (right):

https://chromiumcodereview.appspot.com/2100663002/diff/1/chrome/browser/notif...
chrome/browser/notifications/platform_notification_service_impl.cc:415: return;
This will break closing persistent notifications shown during a previous session
of the browser. (I.e. before a restart.)

https://chromiumcodereview.appspot.com/2100663002/diff/1/chrome/browser/notif...
chrome/browser/notifications/platform_notification_service_impl.cc:422:
GetNotificationDisplayService(profile)->Close(iter->second);
Is invoking Close() twice with different values a problem for Mac?

[Miguel Garcia, 2016-06-28 08:17:20]

https://chromiumcodereview.appspot.com/2100663002/diff/1/chrome/browser/notif...
File chrome/browser/notifications/notification_platform_bridge_mac.mm (right):

https://chromiumcodereview.appspot.com/2100663002/diff/1/chrome/browser/notif...
chrome/browser/notifications/notification_platform_bridge_mac.mm:157:
[persistent_profile_id isEqualToString:current_profile_id]) {
On 2016/06/26 20:46:26, Peter Beverloo wrote:
> This seems like the kind of bug that would easily be catchable by unit tests.
Do
> you still plan to add some?

You'd thing but in the tests all strings would be static so they'd share the
same pointer and it would not be caught :(

In any case yes, as you've seen in previews reviews as I refactor all the paths
(send, click..) to provide support for XPC I am adding unit tests. Close has not
been through that process yet.

https://chromiumcodereview.appspot.com/2100663002/diff/1/chrome/browser/notif...
File chrome/browser/notifications/platform_notification_service_impl.cc (right):

https://chromiumcodereview.appspot.com/2100663002/diff/1/chrome/browser/notif...
chrome/browser/notifications/platform_notification_service_impl.cc:415: return;
On 2016/06/26 20:46:26, Peter Beverloo wrote:
> This will break closing persistent notifications shown during a previous
session
> of the browser. (I.e. before a restart.)

True, changed it a bit to reflect that case, if instead of deleting the element
through the iterator we delete it through the key it becomes easier .

https://chromiumcodereview.appspot.com/2100663002/diff/1/chrome/browser/notif...
chrome/browser/notifications/platform_notification_service_impl.cc:422:
GetNotificationDisplayService(profile)->Close(iter->second);
On 2016/06/26 20:46:26, Peter Beverloo wrote:
> Is invoking Close() twice with different values a problem for Mac?

It isn't but in a world with an XPC service it will have to go try to find the
notification id both in the local notification center and the XPC one which
seemed like a waste.

[Peter Beverloo, 2016-06-28 23:05:36]

lgtm, but why do you remove the whitespace :(

[Miguel Garcia, 2016-06-29 08:08:19]

The CQ bit was checked by miguelg@chromium.org

[Miguel Garcia, 2016-06-29 08:08:20]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org
Link to the patchset: https://codereview.chromium.org/2100663002/#ps40001
(title: "add balnk lines")

[commit-bot: I haz the power, 2016-06-29 08:08:38]

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-06-29 08:40:54]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-06-29 08:42:08]

Description was changed from

==========
Fix a few bugs when closing mac notifications

The ID and profile comparisons in the native notification bridge should
use isEqualToString instead of checking the pointers.

PlatformNotificationServiceImpls#ClosePersistentNotifiction should invoke
the right close method depending on wether it's a persistent or an
in-page notification


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

to

==========
Fix a few bugs when closing mac notifications

The ID and profile comparisons in the native notification bridge should
use isEqualToString instead of checking the pointers.

PlatformNotificationServiceImpls#ClosePersistentNotifiction should invoke
the right close method depending on wether it's a persistent or an
in-page notification

BUG=571056

Committed: https://crrev.com/6bda39f7d226fdc39d4fc12d47a2c6de82e042f3
Cr-Commit-Position: refs/heads/master@{#402755}
==========

[commit-bot: I haz the power, 2016-06-29 08:42:09]

Patchset 3 (id:??) landed as
https://crrev.com/6bda39f7d226fdc39d4fc12d47a2c6de82e042f3
Cr-Commit-Position: refs/heads/master@{#402755}