Store the actual notification data in the notification database.

Store the actual notification data in the notification database.

This patch introduces the actual functionality for storing notification
data in the NotificationDatabase. There are three separate methods
now, ReadNotificationData() and WriteNotificationData() and
DeleteNotificationData(),, which do exactly what their names imply.

These methods are covered by a variety of unit tests, but still aren't
hooked up to any production code.

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

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

[Peter Beverloo, 2015-03-11 22:09:26]

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

[Peter Beverloo, 2015-03-12 22:55:39]

Please take a look.

This CL depends on the following two:
  https://codereview.chromium.org/1003843002/
  https://codereview.chromium.org/1004493002/

[cmumford, 2015-03-13 17:13:15]

https://codereview.chromium.org/999933002/diff/20001/content/browser/notifica...
File content/browser/notifications/notification_database.cc (right):

https://codereview.chromium.org/999933002/diff/20001/content/browser/notifica...
content/browser/notifications/notification_database.cc:35: const char
kKeySeparator = '\x00';
Not seriously suggesting this, but I've never seen ASCII record/group separator
char values used (ASCII values 0x1C - 0x1F).

https://codereview.chromium.org/999933002/diff/20001/content/browser/notifica...
content/browser/notifications/notification_database.cc:57:
origin.spec().c_str(),
Instead of using the origin directly (which may be totally OK), you may want to
consider using storage::GetIdentifierFromOrigin() as it has some extra logic
used by other systems (IDB, etc.) which have needs similar to this one.

https://codereview.chromium.org/999933002/diff/20001/content/browser/notifica...
content/browser/notifications/notification_database.cc:178:
batch.Put(kNextNotificationIdKey, base::Int64ToString(notification_id + 1));
I'm feeling a little less comfortable with this class not really being under
control of the next notification ID. The contract here (as I understand it) is
the caller must first call GetNextNotificationId(), and then pass that into this
function. If the caller messes this up then we could wind up lowering the next
notification ID, and then start overwriting notification entries. Unless I'm
overlooking something in the implementation I'd rather make
GetNextNotificationId private, and just have an
AddNotificationData/UpdateNotificationData set of functions.

Please let me know if I'm missing something obvious.

[Bernhard Bauer, 2015-03-13 17:25:35]

https://codereview.chromium.org/999933002/diff/20001/content/browser/notifica...
File content/browser/notifications/notification_database.cc (right):

https://codereview.chromium.org/999933002/diff/20001/content/browser/notifica...
content/browser/notifications/notification_database.cc:24: // value: Decimal
string which fits into an int64_t.
Add the new key to the schema?

https://codereview.chromium.org/999933002/diff/20001/content/browser/notifica...
content/browser/notifications/notification_database.cc:140: DCHECK_EQ(state_,
STATE_INITIALIZED);
Put the expected value first :)

[Peter Beverloo, 2015-03-13 20:32:32]

All done, thanks for the comments :)

https://codereview.chromium.org/999933002/diff/20001/content/browser/notifica...
File content/browser/notifications/notification_database.cc (right):

https://codereview.chromium.org/999933002/diff/20001/content/browser/notifica...
content/browser/notifications/notification_database.cc:24: // value: Decimal
string which fits into an int64_t.
On 2015/03/13 17:25:35, Bernhard (OOO until Mar 23) wrote:
> Add the new key to the schema?

Done.

https://codereview.chromium.org/999933002/diff/20001/content/browser/notifica...
content/browser/notifications/notification_database.cc:35: const char
kKeySeparator = '\x00';
On 2015/03/13 17:13:15, cmumford wrote:
> Not seriously suggesting this, but I've never seen ASCII record/group
separator
> char values used (ASCII values 0x1C - 0x1F).

Heh, I didn't even know these existed. I guess \x1D could be appropriate.
LevelDB handles both \x00 and \x1D well, so implementation wise it doesn't
really matter.

https://codereview.chromium.org/999933002/diff/20001/content/browser/notifica...
content/browser/notifications/notification_database.cc:57:
origin.spec().c_str(),
On 2015/03/13 17:13:15, cmumford wrote:
> Instead of using the origin directly (which may be totally OK), you may want
to
> consider using storage::GetIdentifierFromOrigin() as it has some extra logic
> used by other systems (IDB, etc.) which have needs similar to this one.

I'm all for re-using logic :). Done

https://codereview.chromium.org/999933002/diff/20001/content/browser/notifica...
content/browser/notifications/notification_database.cc:140: DCHECK_EQ(state_,
STATE_INITIALIZED);
On 2015/03/13 17:25:35, Bernhard (OOO until Mar 23) wrote:
> Put the expected value first :)

Done.

https://codereview.chromium.org/999933002/diff/20001/content/browser/notifica...
content/browser/notifications/notification_database.cc:178:
batch.Put(kNextNotificationIdKey, base::Int64ToString(notification_id + 1));
On 2015/03/13 17:13:15, cmumford wrote:
> I'm feeling a little less comfortable with this class not really being under
> control of the next notification ID. The contract here (as I understand it) is
> the caller must first call GetNextNotificationId(), and then pass that into
this
> function. If the caller messes this up then we could wind up lowering the next
> notification ID, and then start overwriting notification entries. Unless I'm
> overlooking something in the implementation I'd rather make
> GetNextNotificationId private, and just have an
> AddNotificationData/UpdateNotificationData set of functions.
> 
> Please let me know if I'm missing something obvious.

Yes, this makes sense. I've changed the API.

[Bernhard Bauer, 2015-03-13 20:57:35]

lgtm

[cmumford, 2015-03-13 21:03:40]

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

https://codereview.chromium.org/999933002/diff/40001/content/browser/notifica...
content/browser/notifications/notification_database.cc:156: Status status =
GetAndIncrementNextNotificationId(&batch, notification_id);
What about just getting the next notification ID once, when the db is opened,
and keeping in a member variable. And then when WriteNotificationData is called,
you'd increment and write it out in the batch. Basically what you do now, but
w/o a db.get on every write?

[Peter Beverloo, 2015-03-16 11:56:45]

Thank you for your comment!!

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

https://codereview.chromium.org/999933002/diff/40001/content/browser/notifica...
content/browser/notifications/notification_database.cc:156: Status status =
GetAndIncrementNextNotificationId(&batch, notification_id);
On 2015/03/13 21:03:40, cmumford wrote:
> What about just getting the next notification ID once, when the db is opened,
> and keeping in a member variable. And then when WriteNotificationData is
called,
> you'd increment and write it out in the batch. Basically what you do now, but
> w/o a db.get on every write?

Awesome, even better :) Thanks for the suggestion - done!

[cmumford, 2015-03-16 17:14:07]

Mostly nits.

https://codereview.chromium.org/999933002/diff/60001/content/browser/notifica...
File content/browser/notifications/notification_database.cc (right):

https://codereview.chromium.org/999933002/diff/60001/content/browser/notifica...
content/browser/notifications/notification_database.cc:72:
next_notification_id_(0),
Could initialize this (and state_) in the header if you want. Not required I
believe.

https://codereview.chromium.org/999933002/diff/60001/content/browser/notifica...
content/browser/notifications/notification_database.cc:111: return
GetNextNotificationId(&next_notification_id_);
Now that GetNextNotificationId is private and is always called to set
next_notification_id_ you could simplify it to accept no params, and set
next_notification_id_ within.

https://codereview.chromium.org/999933002/diff/60001/content/browser/notifica...
content/browser/notifications/notification_database.cc:120:
DCHECK_GE(notification_id, kFirstNotificationId);
Could you also add DCHECK_LT(notification_id, next_notification_id_), or do you
want to allow attempts to read non-existent notifications?

https://codereview.chromium.org/999933002/diff/60001/content/browser/notifica...
content/browser/notifications/notification_database.cc:182: // TODO(peter):
Instead of validating the existence of the data by reading the
Regarding validation. I would suggest making this function idempotent and simply
not error out if the key doesn't exist. leveldb will still read data from the db
- even if the value doesn't exist (which you can improve by adding a Bloom
filter). See issue 409520 for more info on the cost of "empty get's".

Can you make this the responsibility of the caller?

https://codereview.chromium.org/999933002/diff/60001/content/browser/notifica...
content/browser/notifications/notification_database.cc:229: int64_t
next_notification_id;
No need for local next_notification_id variable, but regarding other comment I
recommend using next_notification_id_ directly.

[Peter Beverloo, 2015-03-16 17:48:23]

All done!

Thank you for your thorough reviews :)

https://codereview.chromium.org/999933002/diff/60001/content/browser/notifica...
File content/browser/notifications/notification_database.cc (right):

https://codereview.chromium.org/999933002/diff/60001/content/browser/notifica...
content/browser/notifications/notification_database.cc:72:
next_notification_id_(0),
On 2015/03/16 17:14:07, cmumford wrote:
> Could initialize this (and state_) in the header if you want. Not required I
> believe.

Cool, I didn't know we allowed that :). Changed!

https://codereview.chromium.org/999933002/diff/60001/content/browser/notifica...
content/browser/notifications/notification_database.cc:111: return
GetNextNotificationId(&next_notification_id_);
On 2015/03/16 17:14:07, cmumford wrote:
> Now that GetNextNotificationId is private and is always called to set
> next_notification_id_ you could simplify it to accept no params, and set
> next_notification_id_ within.

Done.

https://codereview.chromium.org/999933002/diff/60001/content/browser/notifica...
content/browser/notifications/notification_database.cc:120:
DCHECK_GE(notification_id, kFirstNotificationId);
On 2015/03/16 17:14:07, cmumford wrote:
> Could you also add DCHECK_LT(notification_id, next_notification_id_), or do
you
> want to allow attempts to read non-existent notifications?

It would just return STATUS_NOT_FOUND, which in itself is fine.

The reason I didn't add that DCHECK is that keeping the database synchronized
with what's being shown on Android is going to be tricky (FYI - the Android
framework doesn't tell us which of Chrome's notifications are still being
shown). There will be cases which we initially miss, and crashing the entire
browser for that is harsh, even if it's in debug builds only.

https://codereview.chromium.org/999933002/diff/60001/content/browser/notifica...
content/browser/notifications/notification_database.cc:182: // TODO(peter):
Instead of validating the existence of the data by reading the
On 2015/03/16 17:14:07, cmumford wrote:
> Regarding validation. I would suggest making this function idempotent and
simply
> not error out if the key doesn't exist. leveldb will still read data from the
db
> - even if the value doesn't exist (which you can improve by adding a Bloom
> filter). See issue 409520 for more info on the cost of "empty get's".
> 
> Can you make this the responsibility of the caller?

Issue 409520 provides good context. I have been doubting about whether this is
necessary or not - if someone calls DeleteNotificationData then their intended
goal will be for the data to be gone. If it didn't exist in the first place,
their goal is still satisfied.

Per your arguments I've changed this to return STATUS_OK for non-existing
notifications as well.

https://codereview.chromium.org/999933002/diff/60001/content/browser/notifica...
content/browser/notifications/notification_database.cc:229: int64_t
next_notification_id;
On 2015/03/16 17:14:07, cmumford wrote:
> No need for local next_notification_id variable, but regarding other comment I
> recommend using next_notification_id_ directly.

Done.

[cmumford, 2015-03-16 17:51:20]

lgtm

[Peter Beverloo, 2015-03-16 18:20:13]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2015-03-16 18:20:13]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/999933002/#ps80001 (title:
"comments")

[commit-bot: I haz the power, 2015-03-16 18:21:00]

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

[commit-bot: I haz the power, 2015-03-16 18:55:13]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-03-16 18:55:54]

Patchset 5 (id:??) landed as
https://crrev.com/513eb55786639861ac1f367520290493b5b14b61
Cr-Commit-Position: refs/heads/master@{#320761}