Teach the PlatformNotificationContext how to delete notifications.

Teach the PlatformNotificationContext how to delete notifications.

This follows the very same pattern as the read and write methods, but does
so for deleting notifications instead. Note that the notification database
still isn't hooked up to production code, but is covered by a broad
series of unit tests.

Design document:
  http://goo.gl/TciXVp

BUG=447628
Committed: https://crrev.com/18fd3fcfe4bf9d695d582c0b55a16039d57023fa
Cr-Commit-Position: refs/heads/master@{#320914}

[Peter Beverloo, 2015-03-16 12:24:06]

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

[Peter Beverloo, 2015-03-16 12:24:06]

+johnme

Easy one to start with :)

This CL depends on:
  https://codereview.chromium.org/1006493005/

[johnme, 2015-03-16 16:55:18]

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

https://codereview.chromium.org/1008003003/diff/1/content/browser/notificatio...
content/browser/notifications/platform_notification_context.cc:137: const bool
success = status == NotificationDatabase::STATUS_OK;
It doesn't seem ideal to treat STATUS_ERROR_NOT_FOUND the same as other
failures, since for many use cases that's likely to be considered a success
(whereas STATUS_ERROR_FAILED may mean the key is still stored in the DB).

https://codereview.chromium.org/1008003003/diff/1/content/browser/notificatio...
content/browser/notifications/platform_notification_context.cc:137: const bool
success = status == NotificationDatabase::STATUS_OK;
It doesn't seem ideal to treat STATUS_ERROR_NOT_FOUND the same as other
failures, since for many use cases that's likely to be considered a success
(whereas STATUS_ERROR_FAILED may mean the key is still stored in the DB).

https://codereview.chromium.org/1008003003/diff/1/content/browser/notificatio...
File content/browser/notifications/platform_notification_context.h (right):

https://codereview.chromium.org/1008003003/diff/1/content/browser/notificatio...
content/browser/notifications/platform_notification_context.h:90: // for
invoking |callback| on when the operation has completed.
Nit: While you're re-wording this, why not just "Will invoke |callback| on the
IO thread when"?

[Peter Beverloo, 2015-03-16 18:16:51]

Thank you for the review!

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

https://codereview.chromium.org/1008003003/diff/1/content/browser/notificatio...
content/browser/notifications/platform_notification_context.cc:137: const bool
success = status == NotificationDatabase::STATUS_OK;
On 2015/03/16 16:55:18, johnme wrote:
> It doesn't seem ideal to treat STATUS_ERROR_NOT_FOUND the same as other
> failures, since for many use cases that's likely to be considered a success
> (whereas STATUS_ERROR_FAILED may mean the key is still stored in the DB).

This is not the case anymore per new semantics in the NotificationDatabase class
-- deleting a non-existent notification is not considered to be a failure
anymore.

I've updated the tests to reflect this.

https://codereview.chromium.org/1008003003/diff/1/content/browser/notificatio...
File content/browser/notifications/platform_notification_context.h (right):

https://codereview.chromium.org/1008003003/diff/1/content/browser/notificatio...
content/browser/notifications/platform_notification_context.h:90: // for
invoking |callback| on when the operation has completed.
On 2015/03/16 16:55:18, johnme wrote:
> Nit: While you're re-wording this, why not just "Will invoke |callback| on the
> IO thread when"?

Because it won't invoke, it'll post a task to invoke. The former sounds to me
like it may be synchronous?

[johnme, 2015-03-16 19:34:46]

lgtm (with optional nit) - thanks :)

https://codereview.chromium.org/1008003003/diff/1/content/browser/notificatio...
File content/browser/notifications/platform_notification_context.h (right):

https://codereview.chromium.org/1008003003/diff/1/content/browser/notificatio...
content/browser/notifications/platform_notification_context.h:90: // for
invoking |callback| on when the operation has completed.
On 2015/03/16 18:16:51, Peter Beverloo wrote:
> On 2015/03/16 16:55:18, johnme wrote:
> > Nit: While you're re-wording this, why not just "Will invoke |callback| on
the
> > IO thread when"?
> 
> Because it won't invoke, it'll post a task to invoke. The former sounds to me
> like it may be synchronous?

Fair. Maybe say "|callback| will be invoked on the IO threads when" then, like
you do above.

[Peter Beverloo, 2015-03-17 13:44:57]

Done. Thanks for the review :-).

[Peter Beverloo, 2015-03-17 13:45:02]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2015-03-17 13:45:03]

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/1008003003/#ps40001
(title: " ")

[commit-bot: I haz the power, 2015-03-17 13:45:17]

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

[commit-bot: I haz the power, 2015-03-17 15:08:52]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-03-17 15:09:17]

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