Eagerly delete replaced notifications from the database

Eagerly delete replaced notifications from the database

Don't rely on the close() event to happen, instead, delete them as soon
as we know one's being replaced.

BUG=635326
Committed: https://crrev.com/3ce3c468088599dc190fc596d0780c319ed82d3d
Cr-Commit-Position: refs/heads/master@{#410697}

[Peter Beverloo, 2016-08-08 14:45:59]

peter@chromium.org changed reviewers:
+ isherman@chromium.org, johnme@chromium.org

[Peter Beverloo, 2016-08-08 14:46:00]

+johnme for notifications
+isherman for UMA addition

[Peter Beverloo, 2016-08-08 14:48:42]

Description was changed from

==========
Eagerly delete replaced notifications from the database

Don't rely on the close() event to happen, instead, delete them as soon
as we know one's beign replaced.

BUG=635326
==========

to

==========
Eagerly delete replaced notifications from the database

Don't rely on the close() event to happen, instead, delete them as soon
as we know one's being replaced.

BUG=635326
==========

[johnme, 2016-08-08 15:14:30]

*/notifications/ lgtm with nits

https://codereview.chromium.org/2223943002/diff/1/content/browser/notificatio...
File content/browser/notifications/notification_database_unittest.cc (right):

https://codereview.chromium.org/2223943002/diff/1/content/browser/notificatio...
content/browser/notifications/notification_database_unittest.cc:578:
EXPECT_EQ(1u, deleted_notification_set.size());
Nit: s/1u/notifications_with_tag/ to match the ASSERT_GT(notifications_with_tag,
0u) above.

https://codereview.chromium.org/2223943002/diff/1/content/browser/notificatio...
File content/browser/notifications/platform_notification_context_unittest.cc
(right):

https://codereview.chromium.org/2223943002/diff/1/content/browser/notificatio...
content/browser/notifications/platform_notification_context_unittest.cc:211:
base::RunLoop().RunUntilIdle();
RunUntilIdle might allow the first notification to be deleted from the DB as a
consequence of the notification close event? It seems it would be more reliable
here to pass a RunLoop QuitClosure to DidWriteNotificationData.

[johnme, 2016-08-08 15:19:39]

Presumably there will also be a spike in not found UMA statuses, since you
haven't removed the old codepath for removing replaced notifications, so that
will trigger additional deletions. But I guess that's ok, since the other
codepath is presumably still required.

[Peter Beverloo, 2016-08-08 16:29:53]

Thanks John!

On 2016/08/08 15:14:30, johnme wrote:
> Presumably there will also be a spike in not found UMA statuses, since
> you haven't removed the old codepath for removing replaced
> notifications, so that will trigger additional deletions. But I guess
> that's ok, since the other codepath is presumably still required.

There shouldn't be, since the bug was caused by that code path not executing
like we expected it to.

https://codereview.chromium.org/2223943002/diff/1/content/browser/notificatio...
File content/browser/notifications/notification_database_unittest.cc (right):

https://codereview.chromium.org/2223943002/diff/1/content/browser/notificatio...
content/browser/notifications/notification_database_unittest.cc:578:
EXPECT_EQ(1u, deleted_notification_set.size());
On 2016/08/08 15:14:30, johnme wrote:
> Nit: s/1u/notifications_with_tag/ to match the
ASSERT_GT(notifications_with_tag,
> 0u) above.

Done.

https://codereview.chromium.org/2223943002/diff/1/content/browser/notificatio...
File content/browser/notifications/platform_notification_context_unittest.cc
(right):

https://codereview.chromium.org/2223943002/diff/1/content/browser/notificatio...
content/browser/notifications/platform_notification_context_unittest.cc:211:
base::RunLoop().RunUntilIdle();
On 2016/08/08 15:14:30, johnme wrote:
> RunUntilIdle might allow the first notification to be deleted from the DB as a
> consequence of the notification close event? It seems it would be more
reliable
> here to pass a RunLoop QuitClosure to DidWriteNotificationData.

This is a unit test focused on the PlatformNotificationContext, the message
filter (nor other code required for handling close events) doesn't exist, so
we're good here.

(It would be met with, at least, flakiness if this ever changes.)

[Ilya Sherman, 2016-08-08 20:02:05]

metrics lgtm

[Peter Beverloo, 2016-08-09 14:00:57]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2016-08-09 14:00:58]

The patchset sent to the CQ was uploaded after l-g-t-m from johnme@chromium.org
Link to the patchset: https://codereview.chromium.org/2223943002/#ps20001
(title: "Eagerly delete replaced notifications from the database")

[commit-bot: I haz the power, 2016-08-09 14:01:18]

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-08-09 16:25:02]

Description was changed from

==========
Eagerly delete replaced notifications from the database

Don't rely on the close() event to happen, instead, delete them as soon
as we know one's being replaced.

BUG=635326
==========

to

==========
Eagerly delete replaced notifications from the database

Don't rely on the close() event to happen, instead, delete them as soon
as we know one's being replaced.

BUG=635326
==========

[commit-bot: I haz the power, 2016-08-09 16:25:04]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-08-09 16:38:53]

Description was changed from

==========
Eagerly delete replaced notifications from the database

Don't rely on the close() event to happen, instead, delete them as soon
as we know one's being replaced.

BUG=635326
==========

to

==========
Eagerly delete replaced notifications from the database

Don't rely on the close() event to happen, instead, delete them as soon
as we know one's being replaced.

BUG=635326

Committed: https://crrev.com/3ce3c468088599dc190fc596d0780c319ed82d3d
Cr-Commit-Position: refs/heads/master@{#410697}
==========

[commit-bot: I haz the power, 2016-08-09 16:38:54]

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