|
|
Created:
8 years, 9 months ago by tzik Modified:
8 years, 8 months ago CC:
chromium-reviews, kinuko+watch, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd database recovery for FileSystemOriginDatabase
BUG=103018, 116615
TEST=FileSystemOriginDatabase.DatabaseRecoveryTest
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=129631
Patch Set 1 #Patch Set 2 : add tests #
Total comments: 4
Patch Set 3 : bool -> enum #
Total comments: 10
Patch Set 4 : '' #Patch Set 5 : repairing #Patch Set 6 : deletion for directory database #
Total comments: 9
Patch Set 7 : disabling directory database cleanup #
Total comments: 6
Patch Set 8 : repairing test #Patch Set 9 : '' #Patch Set 10 : build fix #
Total comments: 21
Patch Set 11 : build fix & test fix #Patch Set 12 : build fix for windows & kinuko-san comments #Patch Set 13 : '' #
Total comments: 22
Patch Set 14 : address comments #
Total comments: 2
Patch Set 15 : build fix #
Total comments: 2
Patch Set 16 : add comment, remove default label #
Total comments: 24
Patch Set 17 : '' #Patch Set 18 : rebase #Messages
Total messages: 36 (0 generated)
Hi. I'd try to handle corrupted LevelDB issue on File API. Could you take a look to the CL?
Just some quick nits. http://codereview.chromium.org/9663021/diff/2001/webkit/fileapi/file_system_d... File webkit/fileapi/file_system_directory_database.cc (right): http://codereview.chromium.org/9663021/diff/2001/webkit/fileapi/file_system_d... webkit/fileapi/file_system_directory_database.cc:134: if (!Init(true)) The Boolean argument isn't very readable. Use an enum, or const bool recover = true; http://codereview.chromium.org/9663021/diff/2001/webkit/fileapi/file_system_d... webkit/fileapi/file_system_directory_database.cc:446: bool FileSystemDirectoryDatabase::CheckIfDatabaseCorrupted( What is the benefit in factoring this method out?
http://codereview.chromium.org/9663021/diff/2001/webkit/fileapi/file_system_d... File webkit/fileapi/file_system_directory_database.cc (right): http://codereview.chromium.org/9663021/diff/2001/webkit/fileapi/file_system_d... webkit/fileapi/file_system_directory_database.cc:134: if (!Init(true)) On 2012/03/09 23:10:11, jsbell wrote: > The Boolean argument isn't very readable. Use an enum, or const bool recover = > true; Done. http://codereview.chromium.org/9663021/diff/2001/webkit/fileapi/file_system_d... webkit/fileapi/file_system_directory_database.cc:446: bool FileSystemDirectoryDatabase::CheckIfDatabaseCorrupted( On 2012/03/09 23:10:11, jsbell wrote: > What is the benefit in factoring this method out? I'd initially thought, we have many thing to do here to detect the corruption. Omitted.
lgtm with a few more nits. http://codereview.chromium.org/9663021/diff/6001/webkit/fileapi/file_system_d... File webkit/fileapi/file_system_directory_database.cc (right): http://codereview.chromium.org/9663021/diff/6001/webkit/fileapi/file_system_d... webkit/fileapi/file_system_directory_database.cc:441: return Init(LEAVE_ON_CORRUPTION); What will status metrics (when added) look like if this recursive call to Init() fails? Will we be able to distinguish failure-on-first-open from failure-after-recovery? http://codereview.chromium.org/9663021/diff/6001/webkit/fileapi/file_system_d... File webkit/fileapi/file_system_directory_database.h (right): http://codereview.chromium.org/9663021/diff/6001/webkit/fileapi/file_system_d... webkit/fileapi/file_system_directory_database.h:21: class Status; This can be removed from the .h now.
eric probably should take a look at this too http://codereview.chromium.org/9663021/diff/6001/webkit/fileapi/file_system_d... File webkit/fileapi/file_system_directory_database.cc (right): http://codereview.chromium.org/9663021/diff/6001/webkit/fileapi/file_system_d... webkit/fileapi/file_system_directory_database.cc:438: if (leveldb::DestroyDB(path_, leveldb::Options()).ok()) { once we've lost the directory structure, have we orphaned the externally stored files... do we need to delete them too? http://codereview.chromium.org/9663021/diff/6001/webkit/fileapi/file_system_o... File webkit/fileapi/file_system_origin_database.cc (right): http://codereview.chromium.org/9663021/diff/6001/webkit/fileapi/file_system_o... webkit/fileapi/file_system_origin_database.cc:78: if (leveldb::DestroyDB(path_, leveldb::Options()).ok()) { once we've lost this database of all origins using the filesystem, have we orphaned all externally stored files... do we need to them as well?
There's a leveldb::Repair that we should try before we use levelDB::Destroy. At a glance, it doesn't look like it's intended to fix CURRENT corruption, but there are certainly other types of curruption that we might hit. Add a REPAIR_ON_CORRUPTION that falls back to DELETE_ON_CORRUPTION, just as DELETE falls back to FAIL. We should also add tests that succeed in repairing, too. See third_party/leveldatabase/src/db/corruption_test.cc for ideas. http://codereview.chromium.org/9663021/diff/6001/webkit/fileapi/file_system_d... File webkit/fileapi/file_system_directory_database.cc (right): http://codereview.chromium.org/9663021/diff/6001/webkit/fileapi/file_system_d... webkit/fileapi/file_system_directory_database.cc:438: if (leveldb::DestroyDB(path_, leveldb::Options()).ok()) { On 2012/03/11 17:40:10, michaeln wrote: > once we've lost the directory structure, have we orphaned the externally stored > files... do we need to delete them too? Yes. We've lost their paths, and there is no way to recover, so we should delete them all. http://codereview.chromium.org/9663021/diff/6001/webkit/fileapi/file_system_d... File webkit/fileapi/file_system_directory_database.h (right): http://codereview.chromium.org/9663021/diff/6001/webkit/fileapi/file_system_d... webkit/fileapi/file_system_directory_database.h:41: enum RecoveringOption { I'd prefer RecoveryOption to RecoveringOption, and I think that DELETE_ON_CORRUPTION is better than REBUILD_ON_CORRUPTION, since it makes it clear that there is data loss. And how about FAIL_ON_CORRUPTION instead of LEAVE_ON_CORRUPTION? http://codereview.chromium.org/9663021/diff/6001/webkit/fileapi/file_system_o... File webkit/fileapi/file_system_origin_database.cc (right): http://codereview.chromium.org/9663021/diff/6001/webkit/fileapi/file_system_o... webkit/fileapi/file_system_origin_database.cc:78: if (leveldb::DestroyDB(path_, leveldb::Options()).ok()) { On 2012/03/11 17:40:10, michaeln wrote: > once we've lost this database of all origins using the filesystem, have we > orphaned all externally stored files... do we need to them as well? Yes. If we lose the origins database, we have no way of knowing who owns any of the existing filesystems, even though they're still individually valid. We need to nuke everything.
On 2012/03/12 19:46:01, ericu wrote: > There's a leveldb::Repair that we should try before we use levelDB::Destroy. At > a glance, it doesn't look like it's intended to fix CURRENT corruption, but > there are certainly other types of curruption that we might hit. Add a > REPAIR_ON_CORRUPTION that falls back to DELETE_ON_CORRUPTION, just as DELETE > falls back to FAIL. We should also add tests that succeed in repairing, too. > See third_party/leveldatabase/src/db/corruption_test.cc for ideas. http://code.google.com/p/leveldb/issues/detail?id=68 describes what the recovery of CURRENT would look like.
food for thought... we could store enough in the per-origin directory databases to reconstruct this top-level origins data base in the event of a total loss of the origins db? > Yes. If we lose the origins database, we have no way of knowing who owns any of > the existing filesystems, even though they're still individually valid. We need > to nuke everything.
On 2012/03/12 22:05:19, michaeln wrote: > food for thought... we could store enough in the per-origin directory databases > to reconstruct this top-level origins data base in the event of a total loss of > the origins db? We certainly could, although we don't currently.
So, IIUC, we should: - delete entire directory for the origin on directory database corruption, - attempt to repair (not destroy) origin database on origin database corruption. And to do them cleanly, - move some ObfuscatedFileUtil roles (like, GetDirectoryForOriginType) to *Database. Can I do it in separate CL? http://codereview.chromium.org/9663021/diff/6001/webkit/fileapi/file_system_d... File webkit/fileapi/file_system_directory_database.cc (right): http://codereview.chromium.org/9663021/diff/6001/webkit/fileapi/file_system_d... webkit/fileapi/file_system_directory_database.cc:441: return Init(LEAVE_ON_CORRUPTION); On 2012/03/10 00:54:50, jsbell wrote: > What will status metrics (when added) look like if this recursive call to Init() > fails? Will we be able to distinguish failure-on-first-open from > failure-after-recovery? Maybe, we will collect separate metrics with recovering_option. http://codereview.chromium.org/9663021/diff/6001/webkit/fileapi/file_system_d... File webkit/fileapi/file_system_directory_database.h (right): http://codereview.chromium.org/9663021/diff/6001/webkit/fileapi/file_system_d... webkit/fileapi/file_system_directory_database.h:21: class Status; On 2012/03/10 00:54:50, jsbell wrote: > This can be removed from the .h now. Done. http://codereview.chromium.org/9663021/diff/6001/webkit/fileapi/file_system_d... webkit/fileapi/file_system_directory_database.h:41: enum RecoveringOption { On 2012/03/12 19:46:01, ericu wrote: > I'd prefer RecoveryOption to RecoveringOption, and I think that > DELETE_ON_CORRUPTION is better than REBUILD_ON_CORRUPTION, since it makes it > clear that there is data loss. And how about FAIL_ON_CORRUPTION instead of > LEAVE_ON_CORRUPTION? Done.
With regard to deleting leveldb's while leaving orphaned files, i think it'd be good to not put code on trunk that can get the system into that state. Wondering what others would think about that? > Can I do it in separate CL?
On Mon, Mar 12, 2012 at 11:40 PM, <tzik@chromium.org> wrote: > So, IIUC, we should: > Â - delete entire directory for the origin on directory database corruption, > Â - attempt to repair (not destroy) origin database on origin database > corruption. > And to do them cleanly, > Â - move some ObfuscatedFileUtil roles (like, GetDirectoryForOriginType) to > *Database. > > Can I do it in separate CL? I'd really rather not have deletion code in there first, then the repair code that might make deletion unnecessary. If you want to put the repair code in first, and then a CL that does the deletion and cleans up the orphaned files, that would make sense. > http://codereview.chromium.org/9663021/diff/6001/webkit/fileapi/file_system_d... > File webkit/fileapi/file_system_directory_database.cc (right): > > http://codereview.chromium.org/9663021/diff/6001/webkit/fileapi/file_system_d... > webkit/fileapi/file_system_directory_database.cc:441: return > Init(LEAVE_ON_CORRUPTION); > On 2012/03/10 00:54:50, jsbell wrote: >> >> What will status metrics (when added) look like if this recursive call > > to Init() >> >> fails? Will we be able to distinguish failure-on-first-open from >> failure-after-recovery? > > > Maybe, we will collect separate metrics with recovering_option. > > > http://codereview.chromium.org/9663021/diff/6001/webkit/fileapi/file_system_d... > File webkit/fileapi/file_system_directory_database.h (right): > > http://codereview.chromium.org/9663021/diff/6001/webkit/fileapi/file_system_d... > webkit/fileapi/file_system_directory_database.h:21: class Status; > On 2012/03/10 00:54:50, jsbell wrote: >> >> This can be removed from the .h now. > > > Done. > > > http://codereview.chromium.org/9663021/diff/6001/webkit/fileapi/file_system_d... > webkit/fileapi/file_system_directory_database.h:41: enum > RecoveringOption { > On 2012/03/12 19:46:01, ericu wrote: >> >> I'd prefer RecoveryOption to RecoveringOption, and I think that >> DELETE_ON_CORRUPTION is better than REBUILD_ON_CORRUPTION, since it > > makes it >> >> clear that there is data loss. Â And how about FAIL_ON_CORRUPTION > > instead of >> >> LEAVE_ON_CORRUPTION? > > > Done. > > http://codereview.chromium.org/9663021/
On Wed, Mar 14, 2012 at 6:55 AM, Eric Uhrhane <ericu@chromium.org> wrote: > On Mon, Mar 12, 2012 at 11:40 PM, <tzik@chromium.org> wrote: > > So, IIUC, we should: > > - delete entire directory for the origin on directory database > corruption, > > - attempt to repair (not destroy) origin database on origin database > > corruption. > > And to do them cleanly, > > - move some ObfuscatedFileUtil roles (like, GetDirectoryForOriginType) > to > > *Database. > > > > Can I do it in separate CL? > > I'd really rather not have deletion code in there first, then the > repair code that might make deletion unnecessary. If you want to put > the repair code in first, and then a CL that does the deletion and > cleans up the orphaned files, that would make sense. If CURRENT file has been just broken for a user the user must have been unable to open the filesystem since then. I wonder if trying to repair something that hasn't been working for a while is really worth trying. If we could add deletion code quicker than the repair code that may help more users after all. Anyway, adding the massive deletion code in the trunk may make some people uneasy so I'm ok with adding repair code first... as far as we can make it look good in a reasonable period. > > > http://codereview.chromium.org/9663021/diff/6001/webkit/fileapi/file_system_d... > > File webkit/fileapi/file_system_directory_database.cc (right): > > > > > http://codereview.chromium.org/9663021/diff/6001/webkit/fileapi/file_system_d... > > webkit/fileapi/file_system_directory_database.cc:441: return > > Init(LEAVE_ON_CORRUPTION); > > On 2012/03/10 00:54:50, jsbell wrote: > >> > >> What will status metrics (when added) look like if this recursive call > > > > to Init() > >> > >> fails? Will we be able to distinguish failure-on-first-open from > >> failure-after-recovery? > > > > > > Maybe, we will collect separate metrics with recovering_option. > > > > > > > http://codereview.chromium.org/9663021/diff/6001/webkit/fileapi/file_system_d... > > File webkit/fileapi/file_system_directory_database.h (right): > > > > > http://codereview.chromium.org/9663021/diff/6001/webkit/fileapi/file_system_d... > > webkit/fileapi/file_system_directory_database.h:21: class Status; > > On 2012/03/10 00:54:50, jsbell wrote: > >> > >> This can be removed from the .h now. > > > > > > Done. > > > > > > > http://codereview.chromium.org/9663021/diff/6001/webkit/fileapi/file_system_d... > > webkit/fileapi/file_system_directory_database.h:41: enum > > RecoveringOption { > > On 2012/03/12 19:46:01, ericu wrote: > >> > >> I'd prefer RecoveryOption to RecoveringOption, and I think that > >> DELETE_ON_CORRUPTION is better than REBUILD_ON_CORRUPTION, since it > > > > makes it > >> > >> clear that there is data loss. And how about FAIL_ON_CORRUPTION > > > > instead of > >> > >> LEAVE_ON_CORRUPTION? > > > > > > Done. > > > > http://codereview.chromium.org/9663021/ >
On Tue, Mar 13, 2012 at 6:29 PM, Kinuko Yasuda <kinuko@chromium.org> wrote: > On Wed, Mar 14, 2012 at 6:55 AM, Eric Uhrhane <ericu@chromium.org> wrote: >> >> On Mon, Mar 12, 2012 at 11:40 PM, Â <tzik@chromium.org> wrote: >> > So, IIUC, we should: >> > Â - delete entire directory for the origin on directory database >> > corruption, >> > Â - attempt to repair (not destroy) origin database on origin database >> > corruption. >> > And to do them cleanly, >> > Â - move some ObfuscatedFileUtil roles (like, GetDirectoryForOriginType) >> > to >> > *Database. >> > >> > Can I do it in separate CL? >> >> I'd really rather not have deletion code in there first, then the >> repair code that might make deletion unnecessary. Â If you want to put >> the repair code in first, and then a CL that does the deletion and >> cleans up the orphaned files, that would make sense. > > > If CURRENT file has been just broken for a user the user must have been > unable to open the filesystem since then. > I wonder if trying to repair something that hasn't been working for a while > is really worth trying. Â If we could add deletion code quicker than the > repair code that may help more users after all. I think the repair code's just a single call. Since it would mean not putting in the bigger remove-all-the-orphaned-files change in this CL, it would be faster to get in the repair than the deletion. > Anyway, adding the massive deletion code in the trunk may make some people > uneasy so I'm ok with adding repair code first... as far as we can make it > look good in a reasonable period. Sounds good to me. > >> >> > >> > http://codereview.chromium.org/9663021/diff/6001/webkit/fileapi/file_system_d... >> > File webkit/fileapi/file_system_directory_database.cc (right): >> > >> > >> > http://codereview.chromium.org/9663021/diff/6001/webkit/fileapi/file_system_d... >> > webkit/fileapi/file_system_directory_database.cc:441: return >> > Init(LEAVE_ON_CORRUPTION); >> > On 2012/03/10 00:54:50, jsbell wrote: >> >> >> >> What will status metrics (when added) look like if this recursive call >> > >> > to Init() >> >> >> >> fails? Will we be able to distinguish failure-on-first-open from >> >> failure-after-recovery? >> > >> > >> > Maybe, we will collect separate metrics with recovering_option. >> > >> > >> > >> > http://codereview.chromium.org/9663021/diff/6001/webkit/fileapi/file_system_d... >> > File webkit/fileapi/file_system_directory_database.h (right): >> > >> > >> > http://codereview.chromium.org/9663021/diff/6001/webkit/fileapi/file_system_d... >> > webkit/fileapi/file_system_directory_database.h:21: class Status; >> > On 2012/03/10 00:54:50, jsbell wrote: >> >> >> >> This can be removed from the .h now. >> > >> > >> > Done. >> > >> > >> > >> > http://codereview.chromium.org/9663021/diff/6001/webkit/fileapi/file_system_d... >> > webkit/fileapi/file_system_directory_database.h:41: enum >> > RecoveringOption { >> > On 2012/03/12 19:46:01, ericu wrote: >> >> >> >> I'd prefer RecoveryOption to RecoveringOption, and I think that >> >> DELETE_ON_CORRUPTION is better than REBUILD_ON_CORRUPTION, since it >> > >> > makes it >> >> >> >> clear that there is data loss. Â And how about FAIL_ON_CORRUPTION >> > >> > instead of >> >> >> >> LEAVE_ON_CORRUPTION? >> > >> > >> > Done. >> > >> > http://codereview.chromium.org/9663021/ > >
Implemented recovery and deletion for origin database, and deletion for directory database. Could you take a look again?
http://codereview.chromium.org/9663021/diff/19002/webkit/fileapi/file_system_... File webkit/fileapi/file_system_directory_database.h (right): http://codereview.chromium.org/9663021/diff/19002/webkit/fileapi/file_system_... webkit/fileapi/file_system_directory_database.h:92: FAIL_ON_CORRUPTION, We should also attempt to repair here before deleting. http://codereview.chromium.org/9663021/diff/19002/webkit/fileapi/file_system_... File webkit/fileapi/file_system_origin_database.cc (right): http://codereview.chromium.org/9663021/diff/19002/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database.cc:130: for (std::set<FilePath>::iterator dir_itr = directories.begin(); Add comment: "Delete any directories not listed in the origins database." http://codereview.chromium.org/9663021/diff/19002/webkit/fileapi/file_system_... File webkit/fileapi/file_system_origin_database_unittest.cc (right): http://codereview.chromium.org/9663021/diff/19002/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database_unittest.cc:176: TEST(FileSystemOriginDatabaseTest, DatabaseRecoveryTest) { We need a test for the code that removes orphaned directories, and ideally another that corrupts a database, but recovers all the data via repairing it.
http://codereview.chromium.org/9663021/diff/19002/webkit/fileapi/file_system_... File webkit/fileapi/file_system_directory_database.h (right): http://codereview.chromium.org/9663021/diff/19002/webkit/fileapi/file_system_... webkit/fileapi/file_system_directory_database.h:92: FAIL_ON_CORRUPTION, On 2012/03/19 16:50:05, ericu wrote: > We should also attempt to repair here before deleting. To avoid web apps facing inconsistency, we can't simply use RepairDB, like the origins database. IMO, we should repair database, check its consistency, and delete all if it was inconsistent. But, checking consistency needs some more work. How about disabling DELETE_ON_CORRUPTION, and enable with fsck code in next CL? http://codereview.chromium.org/9663021/diff/19002/webkit/fileapi/file_system_... File webkit/fileapi/file_system_origin_database.cc (right): http://codereview.chromium.org/9663021/diff/19002/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database.cc:130: for (std::set<FilePath>::iterator dir_itr = directories.begin(); On 2012/03/19 16:50:05, ericu wrote: > Add comment: "Delete any directories not listed in the origins database." Done. http://codereview.chromium.org/9663021/diff/19002/webkit/fileapi/file_system_... File webkit/fileapi/file_system_origin_database_unittest.cc (right): http://codereview.chromium.org/9663021/diff/19002/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database_unittest.cc:176: TEST(FileSystemOriginDatabaseTest, DatabaseRecoveryTest) { On 2012/03/19 16:50:05, ericu wrote: > We need a test for the code that removes orphaned directories, and ideally > another that corrupts a database, but recovers all the data via repairing it. Done adding test for deletion of orphaned.
http://codereview.chromium.org/9663021/diff/19002/webkit/fileapi/file_system_... File webkit/fileapi/file_system_directory_database.h (right): http://codereview.chromium.org/9663021/diff/19002/webkit/fileapi/file_system_... webkit/fileapi/file_system_directory_database.h:92: FAIL_ON_CORRUPTION, On 2012/03/21 08:59:21, tzik wrote: > On 2012/03/19 16:50:05, ericu wrote: > > We should also attempt to repair here before deleting. > > To avoid web apps facing inconsistency, we can't simply use RepairDB, like the > origins database. > IMO, we should repair database, check its consistency, and delete all if it was > inconsistent. > But, checking consistency needs some more work. > > How about disabling DELETE_ON_CORRUPTION, and enable with fsck code in next CL? Sounds good. http://codereview.chromium.org/9663021/diff/19002/webkit/fileapi/file_system_... File webkit/fileapi/file_system_origin_database_unittest.cc (right): http://codereview.chromium.org/9663021/diff/19002/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database_unittest.cc:176: TEST(FileSystemOriginDatabaseTest, DatabaseRecoveryTest) { On 2012/03/21 08:59:21, tzik wrote: > On 2012/03/19 16:50:05, ericu wrote: > > We need a test for the code that removes orphaned directories, and ideally > > another that corrupts a database, but recovers all the data via repairing it. > > Done adding test for deletion of orphaned. Looks good. Can you do a repair test as well? It would be good to verify that repair is actually being called and doing something. I'm not sure what kinds of corruption it can handle, but third_party/leveldatabase/src/db/corruption_test.cc should tell you. http://codereview.chromium.org/9663021/diff/28001/webkit/fileapi/file_system_... File webkit/fileapi/file_system_origin_database.cc (right): http://codereview.chromium.org/9663021/diff/28001/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database.cc:84: if (recovery_option == REPAIR_ON_CORRUPTION && RepairDB(path)) The changes the semantics of REPAIR_ON_CORRUPTION to fall back to DELETE if it fails. Before it fell back to FAIL. I think that's the right decision, but I just wanted to confirm that that was your intention. http://codereview.chromium.org/9663021/diff/28001/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database.cc:89: DropDatabase(); The DropDatabase shouldn't be needed; nothing should have set db_, and it was NULL at line 57. http://codereview.chromium.org/9663021/diff/28001/webkit/fileapi/file_system_... File webkit/fileapi/file_system_origin_database_unittest.cc (right): http://codereview.chromium.org/9663021/diff/28001/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database_unittest.cc:228: LOG(WARNING) << kGarbageFile.value(); Remove this LOG.
http://codereview.chromium.org/9663021/diff/19002/webkit/fileapi/file_system_... File webkit/fileapi/file_system_origin_database_unittest.cc (right): http://codereview.chromium.org/9663021/diff/19002/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database_unittest.cc:176: TEST(FileSystemOriginDatabaseTest, DatabaseRecoveryTest) { On 2012/03/22 03:54:17, ericu wrote: > On 2012/03/21 08:59:21, tzik wrote: > > On 2012/03/19 16:50:05, ericu wrote: > > > We need a test for the code that removes orphaned directories, and ideally > > > another that corrupts a database, but recovers all the data via repairing > it. > > > > Done adding test for deletion of orphaned. > > Looks good. Can you do a repair test as well? It would be good to verify that > repair is actually being called and doing something. I'm not sure what kinds of > corruption it can handle, but > third_party/leveldatabase/src/db/corruption_test.cc should tell you. Done. Right before repairing, database has: - a deleted database entry, - a database entry without backing directory, - a directory and a file without database entry. The repairing wipes out all of them and keeps all other entries. http://codereview.chromium.org/9663021/diff/28001/webkit/fileapi/file_system_... File webkit/fileapi/file_system_origin_database.cc (right): http://codereview.chromium.org/9663021/diff/28001/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database.cc:84: if (recovery_option == REPAIR_ON_CORRUPTION && RepairDB(path)) On 2012/03/22 03:54:17, ericu wrote: > The changes the semantics of REPAIR_ON_CORRUPTION to fall back to DELETE if it > fails. Before it fell back to FAIL. I think that's the right decision, but I > just wanted to confirm that that was your intention. Yes, it's intentional. Repairing is destructive operation, so after its failure, we need to cleanup. http://codereview.chromium.org/9663021/diff/28001/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database.cc:89: DropDatabase(); On 2012/03/22 03:54:17, ericu wrote: > The DropDatabase shouldn't be needed; nothing should have set db_, and it was > NULL at line 57. Done. http://codereview.chromium.org/9663021/diff/28001/webkit/fileapi/file_system_... File webkit/fileapi/file_system_origin_database_unittest.cc (right): http://codereview.chromium.org/9663021/diff/28001/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database_unittest.cc:228: LOG(WARNING) << kGarbageFile.value(); On 2012/03/22 03:54:17, ericu wrote: > Remove this LOG. Done.
Mostly nits only. http://codereview.chromium.org/9663021/diff/35003/webkit/fileapi/file_system_... File webkit/fileapi/file_system_directory_database.cc (right): http://codereview.chromium.org/9663021/diff/35003/webkit/fileapi/file_system_... webkit/fileapi/file_system_directory_database.cc:425: sandbox_directory_.Append(kDirectoryDatabaseName).value()); Can we use AsUTF8Unsafe() and remove ifdefs? http://codereview.chromium.org/9663021/diff/35003/webkit/fileapi/file_system_... webkit/fileapi/file_system_directory_database.cc:444: DCHECK_EQ(FAIL_ON_CORRUPTION, recovery_option); DELETE_ON_CORRUPTION? http://codereview.chromium.org/9663021/diff/35003/webkit/fileapi/file_system_... File webkit/fileapi/file_system_origin_database.cc (right): http://codereview.chromium.org/9663021/diff/35003/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database.cc:68: #endif Can we use AsUTF8Unsafe and remove ifdefs? (Here and elsewhere) http://codereview.chromium.org/9663021/diff/35003/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database.cc:102: return false; If we think we shouldn't fail here (or somewhere) after the repair maybe we should add a WARNING log before returning false? http://codereview.chromium.org/9663021/diff/35003/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database.cc:103: Can we add a brief comment to state what we're doing here: // See if the repaired entries match with what we have on disk. http://codereview.chromium.org/9663021/diff/35003/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database.cc:110: directories.insert(path_each.BaseName()); Maybe we could skip inserting path here if it is kOriginDaatbaseName? http://codereview.chromium.org/9663021/diff/35003/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database.cc:144: } Please put { } around for body if it spans multiple lines http://codereview.chromium.org/9663021/diff/35003/webkit/fileapi/file_system_... File webkit/fileapi/file_system_origin_database.h (right): http://codereview.chromium.org/9663021/diff/35003/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database.h:62: bool RepairDB(const std::string& db_path); LevelDB's method name is RepairDB but in this class looks like we use 'Database' rather than 'DB'... RepairDatabase might be better for the consistency? http://codereview.chromium.org/9663021/diff/35003/webkit/fileapi/file_system_... File webkit/fileapi/file_system_origin_database_unittest.cc (right): http://codereview.chromium.org/9663021/diff/35003/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database_unittest.cc:30: #endif Ditto for ifdefs
http://codereview.chromium.org/9663021/diff/35003/webkit/fileapi/file_system_... File webkit/fileapi/file_system_directory_database.cc (right): http://codereview.chromium.org/9663021/diff/35003/webkit/fileapi/file_system_... webkit/fileapi/file_system_directory_database.cc:425: sandbox_directory_.Append(kDirectoryDatabaseName).value()); On 2012/03/23 04:01:33, kinuko wrote: > Can we use AsUTF8Unsafe() and remove ifdefs? (We chatted offline) ok this does something slightly different from what we do here (especially for POSIX case), and we use another pair of path conversion methods in leveldatabase/env_chromium.cc, i.e. UTF8ToUTF16 and UTF16ToUTF8. (Since recent windows almost always use UTF16 it should be ok I believe) We should use the same method pair in both places, but for now please feel free to keep it as is (or add some neat common method somewhere else) but with an appropriate TODO.
http://codereview.chromium.org/9663021/diff/35003/webkit/fileapi/file_system_... File webkit/fileapi/file_system_directory_database.cc (right): http://codereview.chromium.org/9663021/diff/35003/webkit/fileapi/file_system_... webkit/fileapi/file_system_directory_database.cc:425: sandbox_directory_.Append(kDirectoryDatabaseName).value()); On 2012/03/23 04:01:33, kinuko wrote: > Can we use AsUTF8Unsafe() and remove ifdefs? On linux, it calls SysNativeMBToWide() and WideToUTF8(), and there is no way to convert back. How about go along with third_party/leveldb/env_chromium.cc? That calls UTF16ToUTF8 in Windows and do nothing in POSIX. http://codereview.chromium.org/9663021/diff/35003/webkit/fileapi/file_system_... webkit/fileapi/file_system_directory_database.cc:444: DCHECK_EQ(FAIL_ON_CORRUPTION, recovery_option); On 2012/03/23 04:01:33, kinuko wrote: > DELETE_ON_CORRUPTION? Done. http://codereview.chromium.org/9663021/diff/35003/webkit/fileapi/file_system_... File webkit/fileapi/file_system_origin_database.cc (right): http://codereview.chromium.org/9663021/diff/35003/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database.cc:68: #endif On 2012/03/23 04:01:33, kinuko wrote: > Can we use AsUTF8Unsafe and remove ifdefs? (Here and elsewhere) ditto http://codereview.chromium.org/9663021/diff/35003/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database.cc:102: return false; On 2012/03/23 04:01:33, kinuko wrote: > If we think we shouldn't fail here (or somewhere) after the repair maybe we > should add a WARNING log before returning false? Done. http://codereview.chromium.org/9663021/diff/35003/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database.cc:103: On 2012/03/23 04:01:33, kinuko wrote: > Can we add a brief comment to state what we're doing here: > // See if the repaired entries match with what we have on disk. Done. http://codereview.chromium.org/9663021/diff/35003/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database.cc:110: directories.insert(path_each.BaseName()); On 2012/03/23 04:01:33, kinuko wrote: > Maybe we could skip inserting path here if it is kOriginDaatbaseName? I'd like to leave it if it won't hurt anything. The DCHECK() below prevents me to "repair" completely wrong directory. http://codereview.chromium.org/9663021/diff/35003/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database.cc:144: } On 2012/03/23 04:01:33, kinuko wrote: > Please put { } around for body if it spans multiple lines Done. http://codereview.chromium.org/9663021/diff/35003/webkit/fileapi/file_system_... File webkit/fileapi/file_system_origin_database.h (right): http://codereview.chromium.org/9663021/diff/35003/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database.h:62: bool RepairDB(const std::string& db_path); On 2012/03/23 04:01:33, kinuko wrote: > LevelDB's method name is RepairDB but in this class looks like we use 'Database' > rather than 'DB'... RepairDatabase might be better for the consistency? Done. http://codereview.chromium.org/9663021/diff/35003/webkit/fileapi/file_system_... File webkit/fileapi/file_system_origin_database_unittest.cc (right): http://codereview.chromium.org/9663021/diff/35003/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database_unittest.cc:30: #endif On 2012/03/23 04:01:33, kinuko wrote: > Ditto for ifdefs ditto
http://codereview.chromium.org/9663021/diff/35003/webkit/fileapi/file_system_... File webkit/fileapi/file_system_origin_database.cc (right): http://codereview.chromium.org/9663021/diff/35003/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database.cc:110: directories.insert(path_each.BaseName()); On 2012/03/23 07:09:00, tzik wrote: > On 2012/03/23 04:01:33, kinuko wrote: > > Maybe we could skip inserting path here if it is kOriginDaatbaseName? > > I'd like to leave it if it won't hurt anything. > The DCHECK() below prevents me to "repair" completely wrong directory. Ok. Might be worth adding a short comment for the DCHECK like: 'Make sure we have the database file in its directory and therefore we are working on the correct path.'? http://codereview.chromium.org/9663021/diff/41004/webkit/fileapi/file_system_... File webkit/fileapi/file_system_directory_database.cc (right): http://codereview.chromium.org/9663021/diff/41004/webkit/fileapi/file_system_... webkit/fileapi/file_system_directory_database.cc:22: std::string FilePathStringToString(const FilePath::StringType& path) { Hmm. Could we move this into file_system_util.h or somewhere common for now? http://codereview.chromium.org/9663021/diff/41004/webkit/fileapi/file_system_... File webkit/fileapi/file_system_directory_database.h (right): http://codereview.chromium.org/9663021/diff/41004/webkit/fileapi/file_system_... webkit/fileapi/file_system_directory_database.h:58: explicit FileSystemDirectoryDatabase(const FilePath& path); path -> sandbox_directory (or whatever we actually call in .cc) http://codereview.chromium.org/9663021/diff/41004/webkit/fileapi/file_system_... webkit/fileapi/file_system_directory_database.h:105: FilePath sandbox_directory_; This sounds slightly unclear to me. (What's 'sandbox' here?) How else could we call it... origin_data_directory_? http://codereview.chromium.org/9663021/diff/41004/webkit/fileapi/file_system_... File webkit/fileapi/file_system_directory_database_unittest.cc (right): http://codereview.chromium.org/9663021/diff/41004/webkit/fileapi/file_system_... webkit/fileapi/file_system_directory_database_unittest.cc:33: db_.reset(new FileSystemDirectoryDatabase(sandbox_path())); Do we need first db_.reset() on line 31? http://codereview.chromium.org/9663021/diff/41004/webkit/fileapi/file_system_... webkit/fileapi/file_system_directory_database_unittest.cc:44: FilePath sandbox_path() const { It's not used anywhere other than in InitDatabase? Then maybe we do not need this method? http://codereview.chromium.org/9663021/diff/41004/webkit/fileapi/file_system_... File webkit/fileapi/file_system_origin_database.cc (right): http://codereview.chromium.org/9663021/diff/41004/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database.cc:110: The lines 93-110 might look easier to follow in switch-case statement. http://codereview.chromium.org/9663021/diff/41004/webkit/fileapi/file_system_... File webkit/fileapi/file_system_origin_database_unittest.cc (right): http://codereview.chromium.org/9663021/diff/41004/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database_unittest.cc:89: int wrote_size = base::WritePlatformFile(file, offset, nit: written_size ? http://codereview.chromium.org/9663021/diff/41004/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database_unittest.cc:255: TEST(FileSystemOriginDatabaseTest, DatabaseRecoveryTest) { Would be worth adding some toplevel comments about what this test actually tests. http://codereview.chromium.org/9663021/diff/41004/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database_unittest.cc:299: CorruptDatabase(kDBDir, leveldb::kDescriptorFile, 0, 100); Could you add some comment about why we specify '0:100' here? http://codereview.chromium.org/9663021/diff/41004/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database_unittest.cc:301: const std::string kOrigin("piyo.example.org"); Should we move this before line 308? http://codereview.chromium.org/9663021/diff/41004/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database_unittest.cc:306: EXPECT_EQ(arraysize(kOrigins) - 2, origins_in_db.size()); Could you add some comment about what this '-2' means?
http://codereview.chromium.org/9663021/diff/35003/webkit/fileapi/file_system_... File webkit/fileapi/file_system_origin_database.cc (right): http://codereview.chromium.org/9663021/diff/35003/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database.cc:110: directories.insert(path_each.BaseName()); On 2012/03/26 05:25:09, kinuko wrote: > On 2012/03/23 07:09:00, tzik wrote: > > On 2012/03/23 04:01:33, kinuko wrote: > > > Maybe we could skip inserting path here if it is kOriginDaatbaseName? > > > > I'd like to leave it if it won't hurt anything. > > The DCHECK() below prevents me to "repair" completely wrong directory. > > Ok. Might be worth adding a short comment for the DCHECK like: 'Make sure we > have the database file in its directory and therefore we are working on the > correct path.'? Done. http://codereview.chromium.org/9663021/diff/41004/webkit/fileapi/file_system_... File webkit/fileapi/file_system_directory_database.cc (right): http://codereview.chromium.org/9663021/diff/41004/webkit/fileapi/file_system_... webkit/fileapi/file_system_directory_database.cc:22: std::string FilePathStringToString(const FilePath::StringType& path) { On 2012/03/26 05:25:09, kinuko wrote: > Hmm. Could we move this into file_system_util.h or somewhere common for now? Done. http://codereview.chromium.org/9663021/diff/41004/webkit/fileapi/file_system_... File webkit/fileapi/file_system_directory_database.h (right): http://codereview.chromium.org/9663021/diff/41004/webkit/fileapi/file_system_... webkit/fileapi/file_system_directory_database.h:58: explicit FileSystemDirectoryDatabase(const FilePath& path); On 2012/03/26 05:25:09, kinuko wrote: > path -> sandbox_directory (or whatever we actually call in .cc) Done. http://codereview.chromium.org/9663021/diff/41004/webkit/fileapi/file_system_... webkit/fileapi/file_system_directory_database.h:105: FilePath sandbox_directory_; On 2012/03/26 05:25:09, kinuko wrote: > This sounds slightly unclear to me. (What's 'sandbox' here?) > How else could we call it... origin_data_directory_? Done. http://codereview.chromium.org/9663021/diff/41004/webkit/fileapi/file_system_... File webkit/fileapi/file_system_directory_database_unittest.cc (right): http://codereview.chromium.org/9663021/diff/41004/webkit/fileapi/file_system_... webkit/fileapi/file_system_directory_database_unittest.cc:33: db_.reset(new FileSystemDirectoryDatabase(sandbox_path())); On 2012/03/26 05:25:09, kinuko wrote: > Do we need first db_.reset() on line 31? Yes, we need. Without it, we make two FileSystemDirectoryDatabase object for the directory at the moment. http://codereview.chromium.org/9663021/diff/41004/webkit/fileapi/file_system_... webkit/fileapi/file_system_directory_database_unittest.cc:44: FilePath sandbox_path() const { On 2012/03/26 05:25:09, kinuko wrote: > It's not used anywhere other than in InitDatabase? Then maybe we do not need > this method? Done. http://codereview.chromium.org/9663021/diff/41004/webkit/fileapi/file_system_... File webkit/fileapi/file_system_origin_database.cc (right): http://codereview.chromium.org/9663021/diff/41004/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database.cc:110: On 2012/03/26 05:25:09, kinuko wrote: > The lines 93-110 might look easier to follow in switch-case statement. Done. http://codereview.chromium.org/9663021/diff/41004/webkit/fileapi/file_system_... File webkit/fileapi/file_system_origin_database_unittest.cc (right): http://codereview.chromium.org/9663021/diff/41004/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database_unittest.cc:89: int wrote_size = base::WritePlatformFile(file, offset, On 2012/03/26 05:25:09, kinuko wrote: > nit: written_size ? Done. http://codereview.chromium.org/9663021/diff/41004/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database_unittest.cc:255: TEST(FileSystemOriginDatabaseTest, DatabaseRecoveryTest) { On 2012/03/26 05:25:09, kinuko wrote: > Would be worth adding some toplevel comments about what this test actually > tests. Done. http://codereview.chromium.org/9663021/diff/41004/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database_unittest.cc:299: CorruptDatabase(kDBDir, leveldb::kDescriptorFile, 0, 100); On 2012/03/26 05:25:09, kinuko wrote: > Could you add some comment about why we specify '0:100' here? Done. Modified area to corrupt and added comment. http://codereview.chromium.org/9663021/diff/41004/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database_unittest.cc:301: const std::string kOrigin("piyo.example.org"); On 2012/03/26 05:25:09, kinuko wrote: > Should we move this before line 308? Done. http://codereview.chromium.org/9663021/diff/41004/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database_unittest.cc:306: EXPECT_EQ(arraysize(kOrigins) - 2, origins_in_db.size()); On 2012/03/26 05:25:09, kinuko wrote: > Could you add some comment about what this '-2' means? Done.
lgtm http://codereview.chromium.org/9663021/diff/47001/webkit/fileapi/file_system_... File webkit/fileapi/file_system_origin_database.cc (right): http://codereview.chromium.org/9663021/diff/47001/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database.cc:90: default: Let's not to have default here so that we get a warning for possible unhandled switch cases. http://codereview.chromium.org/9663021/diff/47003/webkit/fileapi/file_system_... File webkit/fileapi/file_system_util.h (right): http://codereview.chromium.org/9663021/diff/47003/webkit/fileapi/file_system_... webkit/fileapi/file_system_util.h:99: // path.AppendASCII("SubDirectory"); Do we want to mention the corresponding routines in leveldatabase/env_chromium.cc?
http://codereview.chromium.org/9663021/diff/47001/webkit/fileapi/file_system_... File webkit/fileapi/file_system_origin_database.cc (right): http://codereview.chromium.org/9663021/diff/47001/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database.cc:90: default: On 2012/03/26 08:43:45, kinuko wrote: > Let's not to have default here so that we get a warning for possible unhandled > switch cases. Done. http://codereview.chromium.org/9663021/diff/47003/webkit/fileapi/file_system_... File webkit/fileapi/file_system_util.h (right): http://codereview.chromium.org/9663021/diff/47003/webkit/fileapi/file_system_... webkit/fileapi/file_system_util.h:99: // path.AppendASCII("SubDirectory"); On 2012/03/26 08:43:45, kinuko wrote: > Do we want to mention the corresponding routines in > leveldatabase/env_chromium.cc? Done.
LGTM with nits. Thanks for the changes! http://codereview.chromium.org/9663021/diff/46006/webkit/fileapi/file_system_... File webkit/fileapi/file_system_directory_database.cc (right): http://codereview.chromium.org/9663021/diff/46006/webkit/fileapi/file_system_... webkit/fileapi/file_system_directory_database.cc:14: #include "base/utf_string_conversions.h" Is this header needed any more, now that you have FilePathToString? http://codereview.chromium.org/9663021/diff/46006/webkit/fileapi/file_system_... File webkit/fileapi/file_system_directory_database.h (right): http://codereview.chromium.org/9663021/diff/46006/webkit/fileapi/file_system_... webkit/fileapi/file_system_directory_database.h:105: FilePath origin_data_directory_; It's not really the origin data directory, since it's specific to a filesystem type [temporary, persistent]. Sandbox wasn't the best name for it either, I admit. How about filesystem_data_directory? Or do you think that would sound like the top-level FileSystem directory, rather than the directory for a specific filesystem? http://codereview.chromium.org/9663021/diff/46006/webkit/fileapi/file_system_... File webkit/fileapi/file_system_directory_database_unittest.cc (right): http://codereview.chromium.org/9663021/diff/46006/webkit/fileapi/file_system_... webkit/fileapi/file_system_directory_database_unittest.cc:33: db_.reset(new FileSystemDirectoryDatabase(base_.path())); Remove the extra reset() call. http://codereview.chromium.org/9663021/diff/46006/webkit/fileapi/file_system_... File webkit/fileapi/file_system_origin_database.cc (right): http://codereview.chromium.org/9663021/diff/46006/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database.cc:14: #include "base/utf_string_conversions.h" Remove utf_string_conversions.h? http://codereview.chromium.org/9663021/diff/46006/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database.cc:122: } How about a comment here to match that below: "Delete any obsolete entries from the origins database." http://codereview.chromium.org/9663021/diff/46006/webkit/fileapi/file_system_... File webkit/fileapi/file_system_origin_database_unittest.cc (right): http://codereview.chromium.org/9663021/diff/46006/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database_unittest.cc:27: std::string FilePathToString(const FilePath& path) { Use the new one in file_system_util? http://codereview.chromium.org/9663021/diff/46006/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database_unittest.cc:50: uint64 picked_file_number = kuint64max; How about just initializing picked_file_number to -1 here? http://codereview.chromium.org/9663021/diff/46006/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database_unittest.cc:58: (picked_file_number == kuint64max || picked_file_number < number)) { Then you can simplify this to: if (file_type == type && picked_file_number < number) { http://codereview.chromium.org/9663021/diff/46006/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database_unittest.cc:65: EXPECT_NE(kuint64max, picked_file_number); And you can EXPECT_GE(0, picked_file_number); http://codereview.chromium.org/9663021/diff/46006/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database_unittest.cc:99: EXPECT_LE(0, written_size); This is probably more readable as EXPECT_GT(written_size, 0) [you did mean LT, not LE, right? http://codereview.chromium.org/9663021/diff/46006/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database_unittest.cc:268: // entries lost. Very nice! http://codereview.chromium.org/9663021/diff/46006/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database_unittest.cc:325: // should be dropped due to absense of backing directory. Typo: absence.
http://codereview.chromium.org/9663021/diff/46006/webkit/fileapi/file_system_... File webkit/fileapi/file_system_directory_database.cc (right): http://codereview.chromium.org/9663021/diff/46006/webkit/fileapi/file_system_... webkit/fileapi/file_system_directory_database.cc:14: #include "base/utf_string_conversions.h" On 2012/03/26 18:39:18, ericu wrote: > Is this header needed any more, now that you have FilePathToString? Removed. No, it's not needed. Thanks! http://codereview.chromium.org/9663021/diff/46006/webkit/fileapi/file_system_... File webkit/fileapi/file_system_directory_database.h (right): http://codereview.chromium.org/9663021/diff/46006/webkit/fileapi/file_system_... webkit/fileapi/file_system_directory_database.h:105: FilePath origin_data_directory_; On 2012/03/26 18:39:18, ericu wrote: > It's not really the origin data directory, since it's specific to a filesystem > type [temporary, persistent]. Sandbox wasn't the best name for it either, I > admit. How about filesystem_data_directory? Or do you think that would sound > like the top-level FileSystem directory, rather than the directory for a > specific filesystem? Done. It looks a bit confusing, yet semantically correct. http://codereview.chromium.org/9663021/diff/46006/webkit/fileapi/file_system_... File webkit/fileapi/file_system_directory_database_unittest.cc (right): http://codereview.chromium.org/9663021/diff/46006/webkit/fileapi/file_system_... webkit/fileapi/file_system_directory_database_unittest.cc:33: db_.reset(new FileSystemDirectoryDatabase(base_.path())); On 2012/03/26 18:39:18, ericu wrote: > Remove the extra reset() call. First reset() call is to avoid multiple database instance for single directory at once. # comment added Can I leave it as is? http://codereview.chromium.org/9663021/diff/46006/webkit/fileapi/file_system_... File webkit/fileapi/file_system_origin_database.cc (right): http://codereview.chromium.org/9663021/diff/46006/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database.cc:14: #include "base/utf_string_conversions.h" On 2012/03/26 18:39:18, ericu wrote: > Remove utf_string_conversions.h? Done. http://codereview.chromium.org/9663021/diff/46006/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database.cc:122: } On 2012/03/26 18:39:18, ericu wrote: > How about a comment here to match that below: > > "Delete any obsolete entries from the origins database." Done. Sounds good. http://codereview.chromium.org/9663021/diff/46006/webkit/fileapi/file_system_... File webkit/fileapi/file_system_origin_database_unittest.cc (right): http://codereview.chromium.org/9663021/diff/46006/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database_unittest.cc:27: std::string FilePathToString(const FilePath& path) { On 2012/03/26 18:39:18, ericu wrote: > Use the new one in file_system_util? Done. http://codereview.chromium.org/9663021/diff/46006/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database_unittest.cc:50: uint64 picked_file_number = kuint64max; On 2012/03/26 18:39:18, ericu wrote: > How about just initializing picked_file_number to -1 here? I'd like to avoid integer overflow as far as possible. Even if this case works as we expected. http://codereview.chromium.org/9663021/diff/46006/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database_unittest.cc:65: EXPECT_NE(kuint64max, picked_file_number); On 2012/03/26 18:39:18, ericu wrote: > And you can EXPECT_GE(0, picked_file_number); picked_file_number and number are unsigned, so it is always true. http://codereview.chromium.org/9663021/diff/46006/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database_unittest.cc:99: EXPECT_LE(0, written_size); On 2012/03/26 18:39:18, ericu wrote: > This is probably more readable as EXPECT_GT(written_size, 0) [you did mean LT, > not LE, right? Done. http://codereview.chromium.org/9663021/diff/46006/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database_unittest.cc:268: // entries lost. On 2012/03/26 18:39:18, ericu wrote: > Very nice! :)
eric: Does the CL still look good to you? I'll submit it after another win_rel tryjob. On 2012/03/27 02:44:18, tzik wrote: > http://codereview.chromium.org/9663021/diff/46006/webkit/fileapi/file_system_... > File webkit/fileapi/file_system_directory_database.cc (right): > > http://codereview.chromium.org/9663021/diff/46006/webkit/fileapi/file_system_... > webkit/fileapi/file_system_directory_database.cc:14: #include > "base/utf_string_conversions.h" > On 2012/03/26 18:39:18, ericu wrote: > > Is this header needed any more, now that you have FilePathToString? > > Removed. No, it's not needed. Thanks! > > http://codereview.chromium.org/9663021/diff/46006/webkit/fileapi/file_system_... > File webkit/fileapi/file_system_directory_database.h (right): > > http://codereview.chromium.org/9663021/diff/46006/webkit/fileapi/file_system_... > webkit/fileapi/file_system_directory_database.h:105: FilePath > origin_data_directory_; > On 2012/03/26 18:39:18, ericu wrote: > > It's not really the origin data directory, since it's specific to a filesystem > > type [temporary, persistent]. Sandbox wasn't the best name for it either, I > > admit. How about filesystem_data_directory? Or do you think that would sound > > like the top-level FileSystem directory, rather than the directory for a > > specific filesystem? > > Done. It looks a bit confusing, yet semantically correct. > > http://codereview.chromium.org/9663021/diff/46006/webkit/fileapi/file_system_... > File webkit/fileapi/file_system_directory_database_unittest.cc (right): > > http://codereview.chromium.org/9663021/diff/46006/webkit/fileapi/file_system_... > webkit/fileapi/file_system_directory_database_unittest.cc:33: db_.reset(new > FileSystemDirectoryDatabase(base_.path())); > On 2012/03/26 18:39:18, ericu wrote: > > Remove the extra reset() call. > > First reset() call is to avoid multiple database instance for single directory > at once. # comment added > Can I leave it as is? > > http://codereview.chromium.org/9663021/diff/46006/webkit/fileapi/file_system_... > File webkit/fileapi/file_system_origin_database.cc (right): > > http://codereview.chromium.org/9663021/diff/46006/webkit/fileapi/file_system_... > webkit/fileapi/file_system_origin_database.cc:14: #include > "base/utf_string_conversions.h" > On 2012/03/26 18:39:18, ericu wrote: > > Remove utf_string_conversions.h? > > Done. > > http://codereview.chromium.org/9663021/diff/46006/webkit/fileapi/file_system_... > webkit/fileapi/file_system_origin_database.cc:122: } > On 2012/03/26 18:39:18, ericu wrote: > > How about a comment here to match that below: > > > > "Delete any obsolete entries from the origins database." > > Done. Sounds good. > > http://codereview.chromium.org/9663021/diff/46006/webkit/fileapi/file_system_... > File webkit/fileapi/file_system_origin_database_unittest.cc (right): > > http://codereview.chromium.org/9663021/diff/46006/webkit/fileapi/file_system_... > webkit/fileapi/file_system_origin_database_unittest.cc:27: std::string > FilePathToString(const FilePath& path) { > On 2012/03/26 18:39:18, ericu wrote: > > Use the new one in file_system_util? > > Done. > > http://codereview.chromium.org/9663021/diff/46006/webkit/fileapi/file_system_... > webkit/fileapi/file_system_origin_database_unittest.cc:50: uint64 > picked_file_number = kuint64max; > On 2012/03/26 18:39:18, ericu wrote: > > How about just initializing picked_file_number to -1 here? > > I'd like to avoid integer overflow as far as possible. > Even if this case works as we expected. > > http://codereview.chromium.org/9663021/diff/46006/webkit/fileapi/file_system_... > webkit/fileapi/file_system_origin_database_unittest.cc:65: EXPECT_NE(kuint64max, > picked_file_number); > On 2012/03/26 18:39:18, ericu wrote: > > And you can EXPECT_GE(0, picked_file_number); > > picked_file_number and number are unsigned, so it is always true. > > http://codereview.chromium.org/9663021/diff/46006/webkit/fileapi/file_system_... > webkit/fileapi/file_system_origin_database_unittest.cc:99: EXPECT_LE(0, > written_size); > On 2012/03/26 18:39:18, ericu wrote: > > This is probably more readable as EXPECT_GT(written_size, 0) [you did mean LT, > > not LE, right? > > Done. > > http://codereview.chromium.org/9663021/diff/46006/webkit/fileapi/file_system_... > webkit/fileapi/file_system_origin_database_unittest.cc:268: // entries lost. > On 2012/03/26 18:39:18, ericu wrote: > > Very nice! > > :)
Yep; looks fine. http://codereview.chromium.org/9663021/diff/46006/webkit/fileapi/file_system_... File webkit/fileapi/file_system_directory_database_unittest.cc (right): http://codereview.chromium.org/9663021/diff/46006/webkit/fileapi/file_system_... webkit/fileapi/file_system_directory_database_unittest.cc:33: db_.reset(new FileSystemDirectoryDatabase(base_.path())); On 2012/03/27 02:44:18, tzik wrote: > On 2012/03/26 18:39:18, ericu wrote: > > Remove the extra reset() call. > > First reset() call is to avoid multiple database instance for single directory > at once. # comment added > Can I leave it as is? Sure, thanks for the comment. http://codereview.chromium.org/9663021/diff/46006/webkit/fileapi/file_system_... File webkit/fileapi/file_system_origin_database_unittest.cc (right): http://codereview.chromium.org/9663021/diff/46006/webkit/fileapi/file_system_... webkit/fileapi/file_system_origin_database_unittest.cc:65: EXPECT_NE(kuint64max, picked_file_number); On 2012/03/27 02:44:18, tzik wrote: > On 2012/03/26 18:39:18, ericu wrote: > > And you can EXPECT_GE(0, picked_file_number); > > picked_file_number and number are unsigned, so it is always true. Good point; changing that -1 to kuint64max made it clearer.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tzik@chromium.org/9663021/54001
Commit queue rejected this change because the description was changed between the time the change entered the commit queue and the time it was ready to commit. You can safely check the commit box again.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tzik@chromium.org/9663021/54001
Try job failure for 9663021-54001 (retry) on win_rel for step "browser_tests". It's a second try, previously, steps "ui_tests, browser_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tzik@chromium.org/9663021/54001
Change committed as 129631 |