Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1004493002:
Store the next notification id in the notification database. (Closed)
|
Created:
5 years, 9 months ago by Peter Beverloo Modified:
5 years, 9 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, Michael van Ouwerkerk, dewittj Base URL:
https://chromium.googlesource.com/chromium/src.git@n-db-NotificationDatabase Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionStore the next notification id in the notification database.
Persistent notifications will get ever incrementing (well, int64) ids
assigned to them when they're being written to the database. This CL
implements this behavior as GetNextNotificationId().
Note that this patch contains some temporary code intended for testing
its functionality, which will be removed when we start writing actual
notification data to the database.
Design document:
http://goo.gl/TciXVp
BUG=447628
Committed: https://crrev.com/05e061be8d5e07efc42962f9b6d2aa60d61b5daf
Cr-Commit-Position: refs/heads/master@{#320491}
Patch Set 1 #Patch Set 2 : #
Total comments: 16
Patch Set 3 : Bernhard's feedback #
Total comments: 7
Patch Set 4 : #
Total comments: 4
Patch Set 5 : #
Messages
Total messages: 15 (2 generated)
peter@chromium.org changed reviewers: + bauerb@chromium.org, cmumford@chromium.org
Notably, NotificationDatabaseTest::IncrementNextNotificationId will be removed when we start storing notification data. Having some temporary code seems better to me than having both unused and untested code in the tree. This patch depends on the following CL: https://codereview.chromium.org/988163003/
https://codereview.chromium.org/1004493002/diff/20001/content/browser/notific... File content/browser/notifications/notification_database.cc (right): https://codereview.chromium.org/1004493002/diff/20001/content/browser/notific... content/browser/notifications/notification_database.cc:21: // value: <int64 'next_notification_id'> Is "int64" correct here if the value is really stored as a string? https://codereview.chromium.org/1004493002/diff/20001/content/browser/notific... content/browser/notifications/notification_database.cc:100: db_->Get(leveldb::ReadOptions(), kNextNotificationIdKey, &value)); Do we actually need to read this from the DB every time, or could we only do that if the stored value is 0? https://codereview.chromium.org/1004493002/diff/20001/content/browser/notific... content/browser/notifications/notification_database.cc:139: int64_t current_notification_id) { When would this be called with a notification ID that is greater than |next_available_notification_id_|? https://codereview.chromium.org/1004493002/diff/20001/content/browser/notific... File content/browser/notifications/notification_database.h (right): https://codereview.chromium.org/1004493002/diff/20001/content/browser/notific... content/browser/notifications/notification_database.h:87: leveldb::DB* db() { return db_.get(); } Name this GetDBForTesting(), so the presubmit check can catch usage of this in production code? https://codereview.chromium.org/1004493002/diff/20001/content/browser/notific... File content/browser/notifications/notification_database_unittest.cc (right): https://codereview.chromium.org/1004493002/diff/20001/content/browser/notific... content/browser/notifications/notification_database_unittest.cc:45: if (status.ok()) Change the return type to void and ASSERT? You can append the status to the ASSERT stream as well. (You're going to have to change the call site to use ASSERT_NO_FATAL_FAILURE.) https://codereview.chromium.org/1004493002/diff/20001/content/browser/notific... content/browser/notifications/notification_database_unittest.cc:190: // Deliberately write an invalid value as the next notification id to the Nit: "ID" in uppercase.
https://codereview.chromium.org/1004493002/diff/20001/content/browser/notific... File content/browser/notifications/notification_database.cc (right): https://codereview.chromium.org/1004493002/diff/20001/content/browser/notific... content/browser/notifications/notification_database.cc:21: // value: <int64 'next_notification_id'> On 2015/03/11 22:02:38, Bernhard Bauer wrote: > Is "int64" correct here if the value is really stored as a string? Everything in LevelDB is stored as a string. My thinking is that it makes more sense to document the underlying type. https://codereview.chromium.org/1004493002/diff/20001/content/browser/notific... content/browser/notifications/notification_database.cc:100: db_->Get(leveldb::ReadOptions(), kNextNotificationIdKey, &value)); On 2015/03/11 22:02:38, Bernhard Bauer wrote: > Do we actually need to read this from the DB every time, or could we only do > that if the stored value is 0? In production, the NotificationDatabase will be owned by the PlatformNotificationContextImpl, which will cache and increment this value for it in order to minimize file I/O. It'll be read immediately after it initialized the NotificationDatabase. https://codereview.chromium.org/1004493002/diff/20001/content/browser/notific... content/browser/notifications/notification_database.cc:139: int64_t current_notification_id) { On 2015/03/11 22:02:38, Bernhard Bauer wrote: > When would this be called with a notification ID that is greater than > |next_available_notification_id_|? Good question - I don't have an answer. Simplified! :-) https://codereview.chromium.org/1004493002/diff/20001/content/browser/notific... File content/browser/notifications/notification_database.h (right): https://codereview.chromium.org/1004493002/diff/20001/content/browser/notific... content/browser/notifications/notification_database.h:87: leveldb::DB* db() { return db_.get(); } On 2015/03/11 22:02:38, Bernhard Bauer wrote: > Name this GetDBForTesting(), so the presubmit check can catch usage of this in > production code? Done. https://codereview.chromium.org/1004493002/diff/20001/content/browser/notific... File content/browser/notifications/notification_database_unittest.cc (right): https://codereview.chromium.org/1004493002/diff/20001/content/browser/notific... content/browser/notifications/notification_database_unittest.cc:45: if (status.ok()) On 2015/03/11 22:02:38, Bernhard Bauer wrote: > Change the return type to void and ASSERT? You can append the status to the > ASSERT stream as well. > > (You're going to have to change the call site to use ASSERT_NO_FATAL_FAILURE.) ASSERT_NO_FATAL_FAILURE! Awesome! Changed :-). https://codereview.chromium.org/1004493002/diff/20001/content/browser/notific... content/browser/notifications/notification_database_unittest.cc:190: // Deliberately write an invalid value as the next notification id to the On 2015/03/11 22:02:38, Bernhard Bauer wrote: > Nit: "ID" in uppercase. I've got it in lowercase everywhere in NotificationDatabase, as other reviewers (in entirely separate patches) preferred lowercase. Since lowercase is consistent with the rest of the notification code, I'd prefer to keep it this way.
https://codereview.chromium.org/1004493002/diff/40001/content/browser/notific... File content/browser/notifications/notification_database.cc (right): https://codereview.chromium.org/1004493002/diff/40001/content/browser/notific... content/browser/notifications/notification_database.cc:92: NotificationDatabase::Status NotificationDatabase::GetNextNotificationId( This doesn't modify the class so could be a const method right? https://codereview.chromium.org/1004493002/diff/40001/content/browser/notific... File content/browser/notifications/notification_database.h (right): https://codereview.chromium.org/1004493002/diff/40001/content/browser/notific... content/browser/notifications/notification_database.h:57: Status GetNextNotificationId(int64_t* notification_id); Should we be using int64 instead of int64_t? https://codereview.chromium.org/1004493002/diff/40001/content/browser/notific... content/browser/notifications/notification_database.h:85: leveldb::DB* GetDBForTesting() { return db_.get(); } const method?
Thank you for the review! :-) https://codereview.chromium.org/1004493002/diff/40001/content/browser/notific... File content/browser/notifications/notification_database.cc (right): https://codereview.chromium.org/1004493002/diff/40001/content/browser/notific... content/browser/notifications/notification_database.cc:92: NotificationDatabase::Status NotificationDatabase::GetNextNotificationId( On 2015/03/12 02:09:37, cmumford wrote: > This doesn't modify the class so could be a const method right? It writes to |next_notification_id_| on line 110. https://codereview.chromium.org/1004493002/diff/40001/content/browser/notific... File content/browser/notifications/notification_database.h (right): https://codereview.chromium.org/1004493002/diff/40001/content/browser/notific... content/browser/notifications/notification_database.h:57: Status GetNextNotificationId(int64_t* notification_id); On 2015/03/12 02:09:37, cmumford wrote: > Should we be using int64 instead of int64_t? Using just int64 is deprecated, see basictypes.h. The guidance is to use int64_t from stdint.h. https://codereview.chromium.org/1004493002/diff/40001/content/browser/notific... content/browser/notifications/notification_database.h:85: leveldb::DB* GetDBForTesting() { return db_.get(); } On 2015/03/12 02:09:37, cmumford wrote: > const method? I'll apply this in the morning.
https://codereview.chromium.org/1004493002/diff/20001/content/browser/notific... File content/browser/notifications/notification_database.cc (right): https://codereview.chromium.org/1004493002/diff/20001/content/browser/notific... content/browser/notifications/notification_database.cc:21: // value: <int64 'next_notification_id'> On 2015/03/11 22:33:33, Peter Beverloo wrote: > On 2015/03/11 22:02:38, Bernhard Bauer wrote: > > Is "int64" correct here if the value is really stored as a string? > > Everything in LevelDB is stored as a string. My thinking is that it makes more > sense to document the underlying type. Yes, but mentioning int64_t here doesn't say how it is encoded -- I would actually expect a sequence of eight bytes that represent the value in memory. Maybe it would make more sense to write it out in prose: "The next notification id encoded as a decimal string. The value will fit into an int64_t."? https://codereview.chromium.org/1004493002/diff/20001/content/browser/notific... content/browser/notifications/notification_database.cc:100: db_->Get(leveldb::ReadOptions(), kNextNotificationIdKey, &value)); On 2015/03/11 22:33:33, Peter Beverloo wrote: > On 2015/03/11 22:02:38, Bernhard Bauer wrote: > > Do we actually need to read this from the DB every time, or could we only do > > that if the stored value is 0? > > In production, the NotificationDatabase will be owned by the > PlatformNotificationContextImpl, which will cache and increment this value for > it in order to minimize file I/O. It'll be read immediately after it initialized > the NotificationDatabase. Hm, if the owner is going to cache the value anyway, do we need this cache? All it seems to do is save a read operation when incrementing the value, but not when reading it, and it adds the invariant that Get needs to be called at least once before Increment, or we are going to write a wrong value back.
All applied. Ideally I would like NotificationDatabase::WriteNextNotificationId to verify in debug that the written id is larger than the current id, but since this either needs a local variable or a helper function I don't think it's worth the hassle. The behavior will be covered in unit tests for PlatformNotificationContextImpl anyway. https://codereview.chromium.org/1004493002/diff/20001/content/browser/notific... File content/browser/notifications/notification_database.cc (right): https://codereview.chromium.org/1004493002/diff/20001/content/browser/notific... content/browser/notifications/notification_database.cc:21: // value: <int64 'next_notification_id'> On 2015/03/12 09:29:33, Bernhard Bauer wrote: > On 2015/03/11 22:33:33, Peter Beverloo wrote: > > On 2015/03/11 22:02:38, Bernhard Bauer wrote: > > > Is "int64" correct here if the value is really stored as a string? > > > > Everything in LevelDB is stored as a string. My thinking is that it makes more > > sense to document the underlying type. > > Yes, but mentioning int64_t here doesn't say how it is encoded -- I would > actually expect a sequence of eight bytes that represent the value in memory. > Maybe it would make more sense to write it out in prose: "The next notification > id encoded as a decimal string. The value will fit into an int64_t."? That's a really good point, thanks. I settled on "Decimal string with fits into an int64_t, as the name of the key should be clear on the next notification id part. https://codereview.chromium.org/1004493002/diff/20001/content/browser/notific... content/browser/notifications/notification_database.cc:100: db_->Get(leveldb::ReadOptions(), kNextNotificationIdKey, &value)); On 2015/03/12 09:29:33, Bernhard Bauer wrote: > On 2015/03/11 22:33:33, Peter Beverloo wrote: > > On 2015/03/11 22:02:38, Bernhard Bauer wrote: > > > Do we actually need to read this from the DB every time, or could we only do > > > that if the stored value is 0? > > > > In production, the NotificationDatabase will be owned by the > > PlatformNotificationContextImpl, which will cache and increment this value for > > it in order to minimize file I/O. It'll be read immediately after it > initialized > > the NotificationDatabase. > > Hm, if the owner is going to cache the value anyway, do we need this cache? All > it seems to do is save a read operation when incrementing the value, but not > when reading it, and it adds the invariant that Get needs to be called at least > once before Increment, or we are going to write a wrong value back. You are right. I've made GetNextNotificationId() const, renamed IncrementNextNotificationId to WriteNextNotificationId (given an exact number) and made that const as well. https://codereview.chromium.org/1004493002/diff/40001/content/browser/notific... File content/browser/notifications/notification_database.h (right): https://codereview.chromium.org/1004493002/diff/40001/content/browser/notific... content/browser/notifications/notification_database.h:85: leveldb::DB* GetDBForTesting() { return db_.get(); } On 2015/03/12 02:13:56, Peter Beverloo wrote: > On 2015/03/12 02:09:37, cmumford wrote: > > const method? > > I'll apply this in the morning. Done.
https://codereview.chromium.org/1004493002/diff/60001/content/browser/notific... File content/browser/notifications/notification_database.cc (right): https://codereview.chromium.org/1004493002/diff/60001/content/browser/notific... content/browser/notifications/notification_database.cc:111: next_notification_id <= 0) { `next_notification_id < kFirstNotificationId`? https://codereview.chromium.org/1004493002/diff/60001/content/browser/notific... content/browser/notifications/notification_database.cc:140: DCHECK_GT(next_notification_id, 0); DCHECK_GE(next_notification_id, kFirstNotificationId)?
https://codereview.chromium.org/1004493002/diff/60001/content/browser/notific... File content/browser/notifications/notification_database.cc (right): https://codereview.chromium.org/1004493002/diff/60001/content/browser/notific... content/browser/notifications/notification_database.cc:111: next_notification_id <= 0) { On 2015/03/12 16:38:55, Bernhard Bauer wrote: > `next_notification_id < kFirstNotificationId`? Done. https://codereview.chromium.org/1004493002/diff/60001/content/browser/notific... content/browser/notifications/notification_database.cc:140: DCHECK_GT(next_notification_id, 0); On 2015/03/12 16:38:55, Bernhard Bauer wrote: > DCHECK_GE(next_notification_id, kFirstNotificationId)? I like it. Done :)
lgtm
The CQ bit was checked by peter@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1004493002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/05e061be8d5e07efc42962f9b6d2aa60d61b5daf Cr-Commit-Position: refs/heads/master@{#320491} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
