Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
988163003:
Introduce the NotificationDatabase class. (Closed)
|
Created:
5 years, 9 months ago by Peter Beverloo Modified:
5 years, 9 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, cmumford, dewittj Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce the NotificationDatabase class.
This is the first part of the Web Notification database implementation,
which is now only capable of opening and creating empty in-memory and
file-based databases, as well as destroying them.
Together with this first bit of functionality come a few unit tests
to ensure that they work correctly. Until the database reaches sufficient
maturity, it won't be used for anything but unit tests.
Design document:
http://goo.gl/TciXVp
BUG=447628
Committed: https://crrev.com/5bb2e1454ad48e8d6da8f5702eb672daba6231e5
Cr-Commit-Position: refs/heads/master@{#320285}
Patch Set 1 #
Total comments: 46
Patch Set 2 : address comments #
Total comments: 4
Patch Set 3 : further comments #Patch Set 4 : #
Total comments: 2
Patch Set 5 : #
Messages
Total messages: 18 (5 generated)
peter@chromium.org changed reviewers: + bauerb@chromium.org, jsbell@chromium.org, mvanouwerkerk@chromium.org
+bauerb and mvanouwerkerk for the notification part. +jsbell for DEPS addition
Thanks. DEPS lgtm. :) +cmumford as FYI: a new leveldb usage
cmumford@chromium.org changed reviewers: + cmumford@chromium.org
https://codereview.chromium.org/988163003/diff/1/content/browser/notification... File content/browser/notifications/notification_database.cc (right): https://codereview.chromium.org/988163003/diff/1/content/browser/notification... content/browser/notifications/notification_database.cc:20: return NotificationDatabase::STATUS_OK; You could call status.IsNotFound() and return STATUS_ERROR_NOT_FOUND too. https://codereview.chromium.org/988163003/diff/1/content/browser/notification... content/browser/notifications/notification_database.cc:52: options.create_if_missing = create_if_missing; I recommend paranoid_checks as they are cheap and will catch some errors. https://codereview.chromium.org/988163003/diff/1/content/browser/notification... content/browser/notifications/notification_database.cc:60: leveldb::DB::Open(options, path_.AsUTF8Unsafe(), &db)); Will UMA stats arrive in a future CL? https://codereview.chromium.org/988163003/diff/1/content/browser/notification... content/browser/notifications/notification_database.cc:62: return status; FYI: leveldb supports the concept of a database repair. You may be able to get away with calling leveldb::RepairDB if leveldb::DB::Open returns an error if you are OK with a portion of your notifications being lost. https://codereview.chromium.org/988163003/diff/1/content/browser/notification... content/browser/notifications/notification_database.cc:84: return LevelDBStatusToStatus( If you destroy an in-memory db then won't you still get here and use path_? Is that what you want? https://codereview.chromium.org/988163003/diff/1/content/browser/notification... content/browser/notifications/notification_database.cc:85: leveldb::DestroyDB(path_.AsUTF8Unsafe(), options)); FYI: DestroyDB does leave behind non leveldb files (and the dir) if they somehow leak into that directory.
https://codereview.chromium.org/988163003/diff/1/content/browser/notification... File content/browser/notifications/notification_database.cc (right): https://codereview.chromium.org/988163003/diff/1/content/browser/notification... content/browser/notifications/notification_database.cc:35: db_.reset(); You don't need to explicitly reset the database here. https://codereview.chromium.org/988163003/diff/1/content/browser/notification... content/browser/notifications/notification_database.cc:41: DCHECK_EQ(state_, STATE_UNINITIALIZED); Put the expected value first for nicer error messages. https://codereview.chromium.org/988163003/diff/1/content/browser/notification... content/browser/notifications/notification_database.cc:58: leveldb::DB* db; Initialize with nullptr? https://codereview.chromium.org/988163003/diff/1/content/browser/notification... File content/browser/notifications/notification_database.h (right): https://codereview.chromium.org/988163003/diff/1/content/browser/notification... content/browser/notifications/notification_database.h:22: // This class must be constructed on the IO thread. All other methods must be There doesn't seem to anything that requires the construction thread to be the IO thread. You could just say that this class can be constructed on any thread. https://codereview.chromium.org/988163003/diff/1/content/browser/notification... content/browser/notifications/notification_database.h:23: // called on a sequenced task runner because they may cause file I/O, which Nit: I would phrase it as "All other methods must be called on a thread or sequenced task runner that allows I/O" (there are default threads that can do I/O, and sequenced task runners that can't). And we should also express somehow that it has to be the same sequenced task runner / thread... https://codereview.chromium.org/988163003/diff/1/content/browser/notification... content/browser/notifications/notification_database.h:26: using InitCallback = base::Callback<void(bool /* success */)>; Is this used somewhere? And in any case, public: should come first in the class. https://codereview.chromium.org/988163003/diff/1/content/browser/notification... content/browser/notifications/notification_database.h:32: enum Status { Enum declarations should come before methods. https://codereview.chromium.org/988163003/diff/1/content/browser/notification... content/browser/notifications/notification_database.h:35: STATUS_ERROR_FAILED Can you explain what these mean? Also, you might want to add a trailing comma, to reduce edit churn when adding more values later. https://codereview.chromium.org/988163003/diff/1/content/browser/notification... content/browser/notifications/notification_database.h:39: // path, the database will be opened in memory instead. |create_if_missing| "If path is non-empty, [...]"? https://codereview.chromium.org/988163003/diff/1/content/browser/notification... content/browser/notifications/notification_database.h:59: enum State { Move the enum to the beginning of the private section (after the friend declaration). You could also make this an enum class, then you could forward-declare it. https://codereview.chromium.org/988163003/diff/1/content/browser/notification... content/browser/notifications/notification_database.h:62: STATE_DISABLED Also add a trailing comma here. https://codereview.chromium.org/988163003/diff/1/content/browser/notification... content/browser/notifications/notification_database.h:67: base::SequenceChecker sequence_checker_; DISALLOW_COPY_AND_ASSIGN? https://codereview.chromium.org/988163003/diff/1/content/browser/notification... File content/browser/notifications/notification_database_unittest.cc (right): https://codereview.chromium.org/988163003/diff/1/content/browser/notification... content/browser/notifications/notification_database_unittest.cc:13: // Creates a new NotificationDatabase instance in memory. Move this into the test fixture, or move the methods from the fixture here? https://codereview.chromium.org/988163003/diff/1/content/browser/notification... content/browser/notifications/notification_database_unittest.cc:30: return database && database->IsOpen(); What is the null check for here? It looks like in almost all cases you dereference the |database| before this anyway.
Thanks for the thorough look! Chris, do you mind if I cc you on future CLs touching LevelDB? The NotificationDatabase layer won't be super complicated, but your feedback is incredibly valuable. https://codereview.chromium.org/988163003/diff/1/content/browser/notification... File content/browser/notifications/notification_database.cc (right): https://codereview.chromium.org/988163003/diff/1/content/browser/notification... content/browser/notifications/notification_database.cc:20: return NotificationDatabase::STATUS_OK; On 2015/03/11 17:13:24, cmumford wrote: > You could call status.IsNotFound() and return STATUS_ERROR_NOT_FOUND too. Ah yes, good point, thanks! I'm ramping up the error codes as I'm able to write tests for them. I'll include some tests for NOT_FOUND when we actually start reading data. https://codereview.chromium.org/988163003/diff/1/content/browser/notification... content/browser/notifications/notification_database.cc:35: db_.reset(); On 2015/03/11 17:15:22, Bernhard Bauer wrote: > You don't need to explicitly reset the database here. I added this to clarify that db_ needs to be reset before env_. C++ guarantees that class members will be destroyed in reverse declaration order, so we're safe, but it's a somewhat hidden dependency on not commonly known language details. Being able to cause crashes by re-arranging class members feels really fragile to me, so unless you mind I would prefer to be explicit about it. https://codereview.chromium.org/988163003/diff/1/content/browser/notification... content/browser/notifications/notification_database.cc:41: DCHECK_EQ(state_, STATE_UNINITIALIZED); On 2015/03/11 17:15:22, Bernhard Bauer wrote: > Put the expected value first for nicer error messages. Done. https://codereview.chromium.org/988163003/diff/1/content/browser/notification... content/browser/notifications/notification_database.cc:52: options.create_if_missing = create_if_missing; On 2015/03/11 17:13:24, cmumford wrote: > I recommend paranoid_checks as they are cheap and will catch some errors. Done, thanks. It won't be a high-frequency database of any kind, so it makes a lot of sense to push for additional correctness. https://codereview.chromium.org/988163003/diff/1/content/browser/notification... content/browser/notifications/notification_database.cc:58: leveldb::DB* db; On 2015/03/11 17:15:22, Bernhard Bauer wrote: > Initialize with nullptr? Done. https://codereview.chromium.org/988163003/diff/1/content/browser/notification... content/browser/notifications/notification_database.cc:60: leveldb::DB::Open(options, path_.AsUTF8Unsafe(), &db)); On 2015/03/11 17:13:24, cmumford wrote: > Will UMA stats arrive in a future CL? Yes. I've clarified this in the header and added it to my task sheet. https://codereview.chromium.org/988163003/diff/1/content/browser/notification... content/browser/notifications/notification_database.cc:62: return status; On 2015/03/11 17:13:24, cmumford wrote: > FYI: leveldb supports the concept of a database repair. You may be able to get > away with calling leveldb::RepairDB if leveldb::DB::Open returns an error if you > are OK with a portion of your notifications being lost. That's a really good question actually - are we OK with a portion of the notifications being lost? My current inclination is to say no, because some platforms will depend on it for keeping sync on which notifications are actually being displayed as well (Android). Since we won't be able to know which notifications we lost track of, they'll end up being no-ops on user interaction, which is a bad UX. https://codereview.chromium.org/988163003/diff/1/content/browser/notification... content/browser/notifications/notification_database.cc:84: return LevelDBStatusToStatus( On 2015/03/11 17:13:24, cmumford wrote: > If you destroy an in-memory db then won't you still get here and use path_? Is > that what you want? The |path_| is a reference, so AsUTF8Unsafe() will just return an empty string. I see that Service Workers uses the database in the same way, and reading how it gets used in the memory environment this seems to be safe? https://codereview.chromium.org/988163003/diff/1/content/browser/notification... content/browser/notifications/notification_database.cc:85: leveldb::DestroyDB(path_.AsUTF8Unsafe(), options)); On 2015/03/11 17:13:24, cmumford wrote: > FYI: DestroyDB does leave behind non leveldb files (and the dir) if they somehow > leak into that directory. How does that happen? Reading the code, it looks like we'll end up calling ChromiumEnv::DeleteDir() on path_. https://codereview.chromium.org/988163003/diff/1/content/browser/notification... File content/browser/notifications/notification_database.h (right): https://codereview.chromium.org/988163003/diff/1/content/browser/notification... content/browser/notifications/notification_database.h:22: // This class must be constructed on the IO thread. All other methods must be On 2015/03/11 17:15:22, Bernhard Bauer wrote: > There doesn't seem to anything that requires the construction thread to be the > IO thread. You could just say that this class can be constructed on any thread. Done. https://codereview.chromium.org/988163003/diff/1/content/browser/notification... content/browser/notifications/notification_database.h:23: // called on a sequenced task runner because they may cause file I/O, which On 2015/03/11 17:15:22, Bernhard Bauer wrote: > Nit: I would phrase it as "All other methods must be called on a thread or > sequenced task runner that allows I/O" (there are default threads that can do > I/O, and sequenced task runners that can't). And we should also express somehow > that it has to be the same sequenced task runner / thread... Done. https://codereview.chromium.org/988163003/diff/1/content/browser/notification... content/browser/notifications/notification_database.h:26: using InitCallback = base::Callback<void(bool /* success */)>; On 2015/03/11 17:15:22, Bernhard Bauer wrote: > Is this used somewhere? And in any case, public: should come first in the class. Removed. https://codereview.chromium.org/988163003/diff/1/content/browser/notification... content/browser/notifications/notification_database.h:32: enum Status { On 2015/03/11 17:15:22, Bernhard Bauer wrote: > Enum declarations should come before methods. Done. https://codereview.chromium.org/988163003/diff/1/content/browser/notification... content/browser/notifications/notification_database.h:35: STATUS_ERROR_FAILED On 2015/03/11 17:15:22, Bernhard Bauer wrote: > Can you explain what these mean? Also, you might want to add a trailing comma, > to reduce edit churn when adding more values later. Done. https://codereview.chromium.org/988163003/diff/1/content/browser/notification... content/browser/notifications/notification_database.h:39: // path, the database will be opened in memory instead. |create_if_missing| On 2015/03/11 17:15:22, Bernhard Bauer wrote: > "If path is non-empty, [...]"? Done, rephrased this comment. https://codereview.chromium.org/988163003/diff/1/content/browser/notification... content/browser/notifications/notification_database.h:59: enum State { On 2015/03/11 17:15:22, Bernhard Bauer wrote: > Move the enum to the beginning of the private section (after the friend > declaration). You could also make this an enum class, then you could > forward-declare it. It seems like we can't use enum class values in DCHECK_EQ without casting: logging.h:503:23: error: invalid operands to binary expression ('basic_ostream<char, std::char_traits<char> >' and 'content::NotificationDatabase::State') ss << names << " (" << v1 << " vs. " << v2 << ")"; I've kept the enum in the header for now, with a TODO to update this when we do support this. A bug already existed. https://codereview.chromium.org/988163003/diff/1/content/browser/notification... content/browser/notifications/notification_database.h:62: STATE_DISABLED On 2015/03/11 17:15:22, Bernhard Bauer wrote: > Also add a trailing comma here. Done. https://codereview.chromium.org/988163003/diff/1/content/browser/notification... content/browser/notifications/notification_database.h:67: base::SequenceChecker sequence_checker_; On 2015/03/11 17:15:22, Bernhard Bauer wrote: > DISALLOW_COPY_AND_ASSIGN? Oops. Done. https://codereview.chromium.org/988163003/diff/1/content/browser/notification... File content/browser/notifications/notification_database_unittest.cc (right): https://codereview.chromium.org/988163003/diff/1/content/browser/notification... content/browser/notifications/notification_database_unittest.cc:13: // Creates a new NotificationDatabase instance in memory. On 2015/03/11 17:15:23, Bernhard Bauer wrote: > Move this into the test fixture, or move the methods from the fixture here? Moved them to the fixture. I'm having the utility methods there to avoid needing a gazillion FRIEND_TEST_ALL_PREFIXESs. https://codereview.chromium.org/988163003/diff/1/content/browser/notification... content/browser/notifications/notification_database_unittest.cc:30: return database && database->IsOpen(); On 2015/03/11 17:15:22, Bernhard Bauer wrote: > What is the null check for here? It looks like in almost all cases you > dereference the |database| before this anyway. Removed it.
https://codereview.chromium.org/988163003/diff/1/content/browser/notification... File content/browser/notifications/notification_database.cc (right): https://codereview.chromium.org/988163003/diff/1/content/browser/notification... content/browser/notifications/notification_database.cc:35: db_.reset(); On 2015/03/11 18:01:26, Peter Beverloo wrote: > On 2015/03/11 17:15:22, Bernhard Bauer wrote: > > You don't need to explicitly reset the database here. > > I added this to clarify that db_ needs to be reset before env_. > > C++ guarantees that class members will be destroyed in reverse declaration > order, so we're safe, but it's a somewhat hidden dependency on not commonly > known language details. Being able to cause crashes by re-arranging class > members feels really fragile to me, so unless you mind I would prefer to be > explicit about it. I'm all for being explicit about it, but I'd rather do that via a comment in the header, where the members are declared. As it is, that particular subtlety wouldn't have been obvious to me. https://codereview.chromium.org/988163003/diff/20001/content/browser/notifica... File content/browser/notifications/notification_database.h (right): https://codereview.chromium.org/988163003/diff/20001/content/browser/notifica... content/browser/notifications/notification_database.h:32: // The key associated with the operation could not be found. This error code is also used if the file is not found on disk, right?
cmumford@chromium.org changed reviewers: + cmumford@chromium.org
Sure, you can put me on leveldb reviews. https://codereview.chromium.org/988163003/diff/1/content/browser/notification... File content/browser/notifications/notification_database.cc (right): https://codereview.chromium.org/988163003/diff/1/content/browser/notification... content/browser/notifications/notification_database.cc:62: return status; On 2015/03/11 18:01:26, Peter Beverloo wrote: > On 2015/03/11 17:13:24, cmumford wrote: > > FYI: leveldb supports the concept of a database repair. You may be able to get > > away with calling leveldb::RepairDB if leveldb::DB::Open returns an error if > you > > are OK with a portion of your notifications being lost. > > That's a really good question actually - are we OK with a portion of the > notifications being lost? > > My current inclination is to say no, because some platforms will depend on it > for keeping sync on which notifications are actually being displayed as well > (Android). Since we won't be able to know which notifications we lost track of, > they'll end up being no-ops on user interaction, which is a bad UX. If leveldb::DB::Open() returns an status.IsCorruption() then your only fallback is to call DeleteDB() - losing everything which will cause no-ops (I think). If you can tolerate some data going away w/o destroying db integrity it might be a good idea. You know best - just wanted to point that out. https://codereview.chromium.org/988163003/diff/1/content/browser/notification... content/browser/notifications/notification_database.cc:85: leveldb::DestroyDB(path_.AsUTF8Unsafe(), options)); On 2015/03/11 18:01:25, Peter Beverloo wrote: > On 2015/03/11 17:13:24, cmumford wrote: > > FYI: DestroyDB does leave behind non leveldb files (and the dir) if they > somehow > > leak into that directory. > > How does that happen? Reading the code, it looks like we'll end up calling > ChromiumEnv::DeleteDir() on path_. ChromiumEnv::DeleteDir calls base::DeleteFile(..., /*recursive=*/false) so if there are any files in the directory they (and the directory) will be left behind. Maybe not a big deal, but you have code that calls PathExists and assumes the notification db is present if true. https://codereview.chromium.org/988163003/diff/20001/content/browser/notifica... File content/browser/notifications/notification_database.h (right): https://codereview.chromium.org/988163003/diff/20001/content/browser/notifica... content/browser/notifications/notification_database.h:36: STATUS_ERROR_FAILED = 2, I would suggest adding STATUS_CORRUPTED as well. leveldb has a concept of this, and the only solution is to either call DeleteDB or RepairDB. You will also have other (more frequent) failures (file busy, insufficient disk space, etc.) and you'll want to handle these differently.
https://codereview.chromium.org/988163003/diff/1/content/browser/notification... File content/browser/notifications/notification_database.cc (right): https://codereview.chromium.org/988163003/diff/1/content/browser/notification... content/browser/notifications/notification_database.cc:35: db_.reset(); On 2015/03/11 18:20:01, Bernhard Bauer wrote: > On 2015/03/11 18:01:26, Peter Beverloo wrote: > > On 2015/03/11 17:15:22, Bernhard Bauer wrote: > > > You don't need to explicitly reset the database here. > > > > I added this to clarify that db_ needs to be reset before env_. > > > > C++ guarantees that class members will be destroyed in reverse declaration > > order, so we're safe, but it's a somewhat hidden dependency on not commonly > > known language details. Being able to cause crashes by re-arranging class > > members feels really fragile to me, so unless you mind I would prefer to be > > explicit about it. > > I'm all for being explicit about it, but I'd rather do that via a comment in the > header, where the members are declared. As it is, that particular subtlety > wouldn't have been obvious to me. Ok. Done. https://codereview.chromium.org/988163003/diff/1/content/browser/notification... content/browser/notifications/notification_database.cc:62: return status; On 2015/03/11 18:24:33, cmumford wrote: > On 2015/03/11 18:01:26, Peter Beverloo wrote: > > On 2015/03/11 17:13:24, cmumford wrote: > > > FYI: leveldb supports the concept of a database repair. You may be able to > get > > > away with calling leveldb::RepairDB if leveldb::DB::Open returns an error if > > you > > > are OK with a portion of your notifications being lost. > > > > That's a really good question actually - are we OK with a portion of the > > notifications being lost? > > > > My current inclination is to say no, because some platforms will depend on it > > for keeping sync on which notifications are actually being displayed as well > > (Android). Since we won't be able to know which notifications we lost track > of, > > they'll end up being no-ops on user interaction, which is a bad UX. > > If leveldb::DB::Open() returns an status.IsCorruption() then your only fallback > is to call DeleteDB() - losing everything which will cause no-ops (I think). If > you can tolerate some data going away w/o destroying db integrity it might be a > good idea. You know best - just wanted to point that out. Right, thanks for the explanation! This is logic that code one layer higher will be dealing with (PlatformNotificationContextImpl), so I'll be sure to include it there (+ tests). https://codereview.chromium.org/988163003/diff/1/content/browser/notification... content/browser/notifications/notification_database.cc:85: leveldb::DestroyDB(path_.AsUTF8Unsafe(), options)); On 2015/03/11 18:24:33, cmumford wrote: > On 2015/03/11 18:01:25, Peter Beverloo wrote: > > On 2015/03/11 17:13:24, cmumford wrote: > > > FYI: DestroyDB does leave behind non leveldb files (and the dir) if they > > somehow > > > leak into that directory. > > > > How does that happen? Reading the code, it looks like we'll end up calling > > ChromiumEnv::DeleteDir() on path_. > > ChromiumEnv::DeleteDir calls base::DeleteFile(..., /*recursive=*/false) > so if there are any files in the directory they (and the directory) will be left > behind. > > Maybe not a big deal, but you have code that calls PathExists and assumes the > notification db is present if true. I see. I think this is OK, since non-database files shouldn't appear there in the first place, and production will pretty much always use the |create_if_missing=true| configuration, where LevelDB will create a new CURRENTS file rather than throw an invalid argument error. https://codereview.chromium.org/988163003/diff/20001/content/browser/notifica... File content/browser/notifications/notification_database.h (right): https://codereview.chromium.org/988163003/diff/20001/content/browser/notifica... content/browser/notifications/notification_database.h:32: // The key associated with the operation could not be found. On 2015/03/11 18:20:01, Bernhard Bauer wrote: > This error code is also used if the file is not found on disk, right? Done. https://codereview.chromium.org/988163003/diff/20001/content/browser/notifica... content/browser/notifications/notification_database.h:36: STATUS_ERROR_FAILED = 2, On 2015/03/11 18:24:33, cmumford wrote: > I would suggest adding STATUS_CORRUPTED as well. leveldb has a concept of this, > and the only solution is to either call DeleteDB or RepairDB. You will also have > other (more frequent) failures (file busy, insufficient disk space, etc.) and > you'll want to handle these differently. Done.
LGTM w/ a small comment nit: https://codereview.chromium.org/988163003/diff/60001/content/browser/notifica... File content/browser/notifications/notification_database.h (right): https://codereview.chromium.org/988163003/diff/60001/content/browser/notifica... content/browser/notifications/notification_database.h:74: // These members depend on the declaration order for destruction. I would actually be explicit about it: |db_| depends on |env_|.
https://codereview.chromium.org/988163003/diff/60001/content/browser/notifica... File content/browser/notifications/notification_database.h (right): https://codereview.chromium.org/988163003/diff/60001/content/browser/notifica... content/browser/notifications/notification_database.h:74: // These members depend on the declaration order for destruction. On 2015/03/11 21:35:11, Bernhard Bauer wrote: > I would actually be explicit about it: |db_| depends on |env_|. Done.
The CQ bit was checked by peter@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jsbell@chromium.org, bauerb@chromium.org Link to the patchset: https://codereview.chromium.org/988163003/#ps80001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/988163003/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/5bb2e1454ad48e8d6da8f5702eb672daba6231e5 Cr-Commit-Position: refs/heads/master@{#320285} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
