The notification database should be able to read or delete multiple items.

The notification database should be able to read or delete multiple items.

This adds five methods to the NotificationDatabase class to read or delete
multiple notifications, which share two actual implementations.

They will be used as follows:
- Read all data: browsing data overviews.
- Read all data for an origin: for origin storage overviews, quota
  client integration (storageQuota.queryInfo()).
- Read all data for a SWR id: to get all notifications associated
  with a given SWR (SWR.getNotifications()).
- Delete all data for an origin: clearing browsing data for a
  given origin.
- Delete all data for a SWR id: to remove notifications when a service
  worker registration gets dropped.

Actual integration with these features will be done in separate patches,
this patch just introduces the primitives in the database.

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

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

[Peter Beverloo, 2015-03-18 20:28:56]

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

[Peter Beverloo, 2015-03-18 20:28:57]

+cmumford for review
+johnme for notifications

I'll clean up and rework the tests for this.

[cmumford, 2015-03-18 22:41:21]

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

https://codereview.chromium.org/1019073002/diff/1/content/browser/notificatio...
content/browser/notifications/notification_database.cc:78: return
base::StringPrintf("%s%s",
Can simplify to:

return CreateDataPrefix(origin) + base::Int64ToString(notification_id);

https://codereview.chromium.org/1019073002/diff/1/content/browser/notificatio...
content/browser/notifications/notification_database.cc:85: bool
RemovePrefix(const std::string& key,
Not sure this function really adds much. It does the StartsWithASCII check - but
that's really something you want to do to terminate the database iteration, and
finally the prefix stripping. I think that if you switch to
leveldb::Slice::starts_with() (see another comment), then you can use
leveldb::Slice::remove_prefix() and do away with this function without any added
code complexity.

https://codereview.chromium.org/1019073002/diff/1/content/browser/notificatio...
content/browser/notifications/notification_database.cc:320: Status status =
LevelDBStatusToStatus(iter->status());
I don't think you'll ever get here (status will always be OK) if iter->Valid()
is in the if block. See my comment below.

https://codereview.chromium.org/1019073002/diff/1/content/browser/notificatio...
content/browser/notifications/notification_database.cc:324: if
(!StartsWithASCII(iter->key().ToString(), prefix, true))
This works, but I recommend creating a prefix_slice, and then using
leveldb::Slice::starts_with for better performance.

https://codereview.chromium.org/1019073002/diff/1/content/browser/notificatio...
content/browser/notifications/notification_database.cc:341: return STATUS_OK;
I think that moving LevelDBStatusToStatus(iter->status()) down here will
properly catch iter Seek/Next failures.

https://codereview.chromium.org/1019073002/diff/1/content/browser/notificatio...
content/browser/notifications/notification_database.cc:359: Status status =
LevelDBStatusToStatus(iter->status());
Same comment on status check as above.

https://codereview.chromium.org/1019073002/diff/1/content/browser/notificatio...
content/browser/notifications/notification_database.cc:364: if
(!RemovePrefix(iter->key().ToString(), prefix, &value))
leveldb::Slice::remove_prefix()

https://codereview.chromium.org/1019073002/diff/1/content/browser/notificatio...
content/browser/notifications/notification_database.cc:368: if
(!base::StringToInt64(value, &notification_id))
Nit: notification_id isn't used until line 383 below. You could move this closer
to where it is used.

https://codereview.chromium.org/1019073002/diff/1/content/browser/notificatio...
content/browser/notifications/notification_database.cc:387: return
LevelDBStatusToStatus(db_->Write(leveldb::WriteOptions(), &batch));
Nit: Not sure if would get called much with no matches, but if so you could
check against deleted_notification_set.empty(), and not call db_->Write(...).

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

https://codereview.chromium.org/1019073002/diff/1/content/browser/notificatio...
content/browser/notifications/notification_database.h:152: // Delets all
notification data with the given constraints. |origin| must
s/Delets/Deletes/

[Peter Beverloo, 2015-03-19 14:35:52]

Thank you!

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

https://codereview.chromium.org/1019073002/diff/1/content/browser/notificatio...
content/browser/notifications/notification_database.cc:78: return
base::StringPrintf("%s%s",
On 2015/03/18 22:41:21, cmumford wrote:
> Can simplify to:
> 
> return CreateDataPrefix(origin) + base::Int64ToString(notification_id);

Done.

https://codereview.chromium.org/1019073002/diff/1/content/browser/notificatio...
content/browser/notifications/notification_database.cc:85: bool
RemovePrefix(const std::string& key,
On 2015/03/18 22:41:20, cmumford wrote:
> Not sure this function really adds much. It does the StartsWithASCII check -
but
> that's really something you want to do to terminate the database iteration,
and
> finally the prefix stripping. I think that if you switch to
> leveldb::Slice::starts_with() (see another comment), then you can use
> leveldb::Slice::remove_prefix() and do away with this function without any
added
> code complexity.

All done, that's awesome and much cleaner! This also allows me to keep using
\x00 as the prefix, reducing the chance of something breaking because part of a
compound key uses a colon.

https://codereview.chromium.org/1019073002/diff/1/content/browser/notificatio...
content/browser/notifications/notification_database.cc:320: Status status =
LevelDBStatusToStatus(iter->status());
On 2015/03/18 22:41:21, cmumford wrote:
> I don't think you'll ever get here (status will always be OK) if iter->Valid()
> is in the if block. See my comment below.

Done.

https://codereview.chromium.org/1019073002/diff/1/content/browser/notificatio...
content/browser/notifications/notification_database.cc:324: if
(!StartsWithASCII(iter->key().ToString(), prefix, true))
On 2015/03/18 22:41:20, cmumford wrote:
> This works, but I recommend creating a prefix_slice, and then using
> leveldb::Slice::starts_with for better performance.

Done.

https://codereview.chromium.org/1019073002/diff/1/content/browser/notificatio...
content/browser/notifications/notification_database.cc:341: return STATUS_OK;
On 2015/03/18 22:41:21, cmumford wrote:
> I think that moving LevelDBStatusToStatus(iter->status()) down here will
> properly catch iter Seek/Next failures.

Done.

https://codereview.chromium.org/1019073002/diff/1/content/browser/notificatio...
content/browser/notifications/notification_database.cc:359: Status status =
LevelDBStatusToStatus(iter->status());
On 2015/03/18 22:41:20, cmumford wrote:
> Same comment on status check as above.

Done.

https://codereview.chromium.org/1019073002/diff/1/content/browser/notificatio...
content/browser/notifications/notification_database.cc:364: if
(!RemovePrefix(iter->key().ToString(), prefix, &value))
On 2015/03/18 22:41:21, cmumford wrote:
> leveldb::Slice::remove_prefix()

Done.

https://codereview.chromium.org/1019073002/diff/1/content/browser/notificatio...
content/browser/notifications/notification_database.cc:368: if
(!base::StringToInt64(value, &notification_id))
On 2015/03/18 22:41:21, cmumford wrote:
> Nit: notification_id isn't used until line 383 below. You could move this
closer
> to where it is used.

Done.

https://codereview.chromium.org/1019073002/diff/1/content/browser/notificatio...
content/browser/notifications/notification_database.cc:387: return
LevelDBStatusToStatus(db_->Write(leveldb::WriteOptions(), &batch));
On 2015/03/18 22:41:21, cmumford wrote:
> Nit: Not sure if would get called much with no matches, but if so you could
> check against deleted_notification_set.empty(), and not call db_->Write(...).

Done.

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

https://codereview.chromium.org/1019073002/diff/1/content/browser/notificatio...
content/browser/notifications/notification_database.h:152: // Delets all
notification data with the given constraints. |origin| must
On 2015/03/18 22:41:21, cmumford wrote:
> s/Delets/Deletes/

Done.

[cmumford, 2015-03-19 15:55:26]

lgtm

[johnme, 2015-03-19 16:17:10]

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

[johnme, 2015-03-19 16:17:10]

lgtm with nit (and optional suggestion for alternative impl)

https://codereview.chromium.org/1019073002/diff/40001/content/browser/notific...
File content/browser/notifications/notification_database.cc (right):

https://codereview.chromium.org/1019073002/diff/40001/content/browser/notific...
content/browser/notifications/notification_database.cc:46: const int64_t
kInvalidServiceWorkerRegistrationId = -1;
Nit: Include content/common/service_worker/service_worker_types.h rather than
redefining this.

https://codereview.chromium.org/1019073002/diff/40001/content/browser/notific...
content/browser/notifications/notification_database.cc:62: std::string
CreateDataPrefix(const GURL& origin) {
Optional alternative: Why not include the service_worker_registration_id in the
data prefix (after the origin, but before the notification_id)? Then
DeleteAllNotificationDataInternal and ReadAllNotificationDataInternal would be
simpler, as you wouldn't need to filter the notifications (or even deserialize
them in the case of Delete).

(you'd have to pass the s_w_r_id to ReadNotificationData of course)

[Peter Beverloo, 2015-03-19 16:53:42]

https://codereview.chromium.org/1019073002/diff/40001/content/browser/notific...
File content/browser/notifications/notification_database.cc (right):

https://codereview.chromium.org/1019073002/diff/40001/content/browser/notific...
content/browser/notifications/notification_database.cc:46: const int64_t
kInvalidServiceWorkerRegistrationId = -1;
On 2015/03/19 16:17:10, johnme wrote:
> Nit: Include content/common/service_worker/service_worker_types.h rather than
> redefining this.

Done.

https://codereview.chromium.org/1019073002/diff/40001/content/browser/notific...
content/browser/notifications/notification_database.cc:62: std::string
CreateDataPrefix(const GURL& origin) {
On 2015/03/19 16:17:10, johnme wrote:
> Optional alternative: Why not include the service_worker_registration_id in
the
> data prefix (after the origin, but before the notification_id)? Then
> DeleteAllNotificationDataInternal and ReadAllNotificationDataInternal would be
> simpler, as you wouldn't need to filter the notifications (or even deserialize
> them in the case of Delete).
> 
> (you'd have to pass the s_w_r_id to ReadNotificationData of course)

I considered this, and actually implemented a few variants of this (including
having an index of it), but neither worked out much nicer.

The main complexity is that the Service Worker Registration ID is part of a
notification's data that users of notifications shouldn't have to care about.
They get an integral ID, which is all reference they need. Increasing their
knowledge to two integral IDs would increase complexity elsewhere.

Additionally, technically service_worker_registration_id is an optional field.
It's not been ruled out that we may use the database for persistent
notifications created by the Chrome Apps extension API, in which case there is
no Service Worker Registration ID.

[Peter Beverloo, 2015-03-19 16:53:46]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2015-03-19 16:53:46]

The patchset sent to the CQ was uploaded after l-g-t-m from johnme@chromium.org,
cmumford@chromium.org
Link to the patchset: https://codereview.chromium.org/1019073002/#ps60001
(title: " ")

[commit-bot: I haz the power, 2015-03-19 16:54:02]

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

[commit-bot: I haz the power, 2015-03-19 18:49:35]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-03-19 18:50:09]

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