Add database recovery for FileSystemDirectoryDatabase.

Add database recovery for FileSystemDirectoryDatabase.

BUG=103018, 116615
TEST='FileSystemDirectoryDatabaseTest.*'

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=132369

[tzik, 2012-04-02 08:22:04]

Hi.

I implemented fsck for FileSystemDirectoryDatabase and database recovery.
Could you review this CL?

[ericu, 2012-04-03 01:52:11]

Very good start--thanks!

http://codereview.chromium.org/9910005/diff/4002/webkit/fileapi/file_system_d...
File webkit/fileapi/file_system_directory_database.cc (right):

http://codereview.chromium.org/9910005/diff/4002/webkit/fileapi/file_system_d...
webkit/fileapi/file_system_directory_database.cc:118: //  - Each |file_id| has
unique backing file.
No--there are file_ids for directories, which have no backing files.

http://codereview.chromium.org/9910005/diff/4002/webkit/fileapi/file_system_d...
webkit/fileapi/file_system_directory_database.cc:137: bool ScanDatabase();
It's probably worth a comment that these need to be called in a specific order,
even though this is all private.

http://codereview.chromium.org/9910005/diff/4002/webkit/fileapi/file_system_d...
webkit/fileapi/file_system_directory_database.cc:160: files_in_db_(),
Omit the default constructor for files_in_db.

http://codereview.chromium.org/9910005/diff/4002/webkit/fileapi/file_system_d...
webkit/fileapi/file_system_directory_database.cc:219: ++num_directories_in_db_;
Verify that there's no data path in the pickle.

http://codereview.chromium.org/9910005/diff/4002/webkit/fileapi/file_system_d...
webkit/fileapi/file_system_directory_database.cc:221: ++num_files_in_db_;
Verify that the file id is unique.

http://codereview.chromium.org/9910005/diff/4002/webkit/fileapi/file_system_d...
webkit/fileapi/file_system_directory_database.cc:271: dir_path) != kExcludes +
arraysize(kExcludes))
Add a counter to make sure that you find the right number of these files.

http://codereview.chromium.org/9910005/diff/4002/webkit/fileapi/file_system_d...
webkit/fileapi/file_system_directory_database.cc:279:
pending_directories.push(data_path);
You're adding all files to pending_directories, not just the directories.

http://codereview.chromium.org/9910005/diff/4002/webkit/fileapi/file_system_d...
webkit/fileapi/file_system_directory_database.cc:691: if
(!Init(FAIL_ON_CORRUPTION))
You're calling Init(FAIL_ON_CORRUPTION) both here and at line 673; it seems that
one should be enough.  At 673 you could just return true.

http://codereview.chromium.org/9910005/diff/4002/webkit/fileapi/file_system_d...
File webkit/fileapi/file_system_directory_database_unittest.cc (right):

http://codereview.chromium.org/9910005/diff/4002/webkit/fileapi/file_system_d...
webkit/fileapi/file_system_directory_database_unittest.cc:42: // Call
CloseDatabes() to avoid multiple database instance for single
Typo: CloseDatabase.

http://codereview.chromium.org/9910005/diff/4002/webkit/fileapi/file_system_d...
webkit/fileapi/file_system_directory_database_unittest.cc:63: FileId
file_id_alt;
[here and below]

Mostly we tend to do this by just having a local temp FileId, then storing back
to *file_id if it's non-NULL.  It's not a style rule or anything, but it's
standard enough that doing it this way stands out and looks weird.

We usually only do the reference trick if the copy is expensive, which it's not
here.

http://codereview.chromium.org/9910005/diff/4002/webkit/fileapi/file_system_d...
webkit/fileapi/file_system_directory_database_unittest.cc:504: FileId file_id;
file_id can go away.

http://codereview.chromium.org/9910005/diff/4002/webkit/fileapi/file_system_d...
webkit/fileapi/file_system_directory_database_unittest.cc:545: }
Please add a test for a directory loop that includes the root and for one that
does not [which can be as simple as a single directory that is its own parent].

[tzik, 2012-04-04 06:50:39]

http://codereview.chromium.org/9910005/diff/4002/webkit/fileapi/file_system_d...
File webkit/fileapi/file_system_directory_database.cc (right):

http://codereview.chromium.org/9910005/diff/4002/webkit/fileapi/file_system_d...
webkit/fileapi/file_system_directory_database.cc:118: //  - Each |file_id| has
unique backing file.
On 2012/04/03 01:52:11, ericu wrote:
> No--there are file_ids for directories, which have no backing files.

Done.

http://codereview.chromium.org/9910005/diff/4002/webkit/fileapi/file_system_d...
webkit/fileapi/file_system_directory_database.cc:137: bool ScanDatabase();
On 2012/04/03 01:52:11, ericu wrote:
> It's probably worth a comment that these need to be called in a specific
order,
> even though this is all private.

Done.

http://codereview.chromium.org/9910005/diff/4002/webkit/fileapi/file_system_d...
webkit/fileapi/file_system_directory_database.cc:160: files_in_db_(),
On 2012/04/03 01:52:11, ericu wrote:
> Omit the default constructor for files_in_db.

Done.

http://codereview.chromium.org/9910005/diff/4002/webkit/fileapi/file_system_d...
webkit/fileapi/file_system_directory_database.cc:219: ++num_directories_in_db_;
On 2012/04/03 01:52:11, ericu wrote:
> Verify that there's no data path in the pickle.

Done.
Implementation of FileInfo::is_directory() is data_path.empty(), currently.
So IMO, DCHECK() is enough strict.

http://codereview.chromium.org/9910005/diff/4002/webkit/fileapi/file_system_d...
webkit/fileapi/file_system_directory_database.cc:221: ++num_files_in_db_;
On 2012/04/03 01:52:11, ericu wrote:
> Verify that the file id is unique.

Done.

http://codereview.chromium.org/9910005/diff/4002/webkit/fileapi/file_system_d...
webkit/fileapi/file_system_directory_database.cc:271: dir_path) != kExcludes +
arraysize(kExcludes))
On 2012/04/03 01:52:11, ericu wrote:
> Add a counter to make sure that you find the right number of these files.

IMO, it is not needed for now.
After patchset 5, this path is relative to filesystem_data_directory. So, it can
not exceed # of excluded path.
And since the database works properly at this point, we can assume the Paths
directory exists. Absence of .usage file makes no problem, so we can ignore it
exists or not.

http://codereview.chromium.org/9910005/diff/4002/webkit/fileapi/file_system_d...
webkit/fileapi/file_system_directory_database.cc:279:
pending_directories.push(data_path);
On 2012/04/03 01:52:11, ericu wrote:
> You're adding all files to pending_directories, not just the directories.

Done.
This part of CL was broken..

http://codereview.chromium.org/9910005/diff/4002/webkit/fileapi/file_system_d...
webkit/fileapi/file_system_directory_database.cc:691: if
(!Init(FAIL_ON_CORRUPTION))
On 2012/04/03 01:52:11, ericu wrote:
> You're calling Init(FAIL_ON_CORRUPTION) both here and at line 673; it seems
that
> one should be enough.  At 673 you could just return true.

Done.

http://codereview.chromium.org/9910005/diff/4002/webkit/fileapi/file_system_d...
File webkit/fileapi/file_system_directory_database_unittest.cc (right):

http://codereview.chromium.org/9910005/diff/4002/webkit/fileapi/file_system_d...
webkit/fileapi/file_system_directory_database_unittest.cc:42: // Call
CloseDatabes() to avoid multiple database instance for single
On 2012/04/03 01:52:11, ericu wrote:
> Typo: CloseDatabase.

Done.

http://codereview.chromium.org/9910005/diff/4002/webkit/fileapi/file_system_d...
webkit/fileapi/file_system_directory_database_unittest.cc:63: FileId
file_id_alt;
On 2012/04/03 01:52:11, ericu wrote:
> [here and below]
> 
> Mostly we tend to do this by just having a local temp FileId, then storing
back
> to *file_id if it's non-NULL.  It's not a style rule or anything, but it's
> standard enough that doing it this way stands out and looks weird.
> 
> We usually only do the reference trick if the copy is expensive, which it's
not
> here.

Done.

http://codereview.chromium.org/9910005/diff/4002/webkit/fileapi/file_system_d...
webkit/fileapi/file_system_directory_database_unittest.cc:504: FileId file_id;
On 2012/04/03 01:52:11, ericu wrote:
> file_id can go away.

Done.

http://codereview.chromium.org/9910005/diff/4002/webkit/fileapi/file_system_d...
webkit/fileapi/file_system_directory_database_unittest.cc:545: }
On 2012/04/03 01:52:11, ericu wrote:
> Please add a test for a directory loop that includes the root and for one that
> does not [which can be as simple as a single directory that is its own
parent].

Done.

[ericu, 2012-04-05 00:22:26]

You've addressed all my concerns, and the code looks good as written, but I've
got another thought.  We used to recover from little things like a leftover file
in the data directory by just removing the file.  Now if there's an extra file
in there for any reason we'll delete the whole filesystem.  I feel like we
should soften that a bit.

Perhaps extra files [lost backing files] should be cleaned up, but not cause a
full reset?  It might mean an inconsistent filesystem from the point of vie of
the app, but it might also be the difference between a user losing a file and a
user losing all files.

Should we allow a little slop in either direction [lost backing file, extra
backing file] and just prune the extras?

Kinuko, Michael, I'd like to hear your opinions as well.

http://codereview.chromium.org/9910005/diff/4002/webkit/fileapi/file_system_d...
File webkit/fileapi/file_system_directory_database.cc (right):

http://codereview.chromium.org/9910005/diff/4002/webkit/fileapi/file_system_d...
webkit/fileapi/file_system_directory_database.cc:271: dir_path) != kExcludes +
arraysize(kExcludes))
On 2012/04/04 06:50:39, tzik wrote:
> On 2012/04/03 01:52:11, ericu wrote:
> > Add a counter to make sure that you find the right number of these files.
> 
> IMO, it is not needed for now.
> After patchset 5, this path is relative to filesystem_data_directory. So, it
can
> not exceed # of excluded path.
> And since the database works properly at this point, we can assume the Paths
> directory exists. Absence of .usage file makes no problem, so we can ignore it
> exists or not.

That works.

http://codereview.chromium.org/9910005/diff/7002/webkit/fileapi/file_system_d...
File webkit/fileapi/file_system_directory_database_unittest.cc (right):

http://codereview.chromium.org/9910005/diff/7002/webkit/fileapi/file_system_d...
webkit/fileapi/file_system_directory_database_unittest.cc:130: void
ReplaceHierarchyLinkName(FileId file_id,
This could just use DeleteHierarchyLink and MakeHierarchyLink.  We don't need
absolute speed here, and it would be simpler code to maintain.

http://codereview.chromium.org/9910005/diff/7002/webkit/fileapi/file_system_d...
webkit/fileapi/file_system_directory_database_unittest.cc:611:
DeleteHierarchyLink(dir2_id); // delete link from |dir1_id| to |dir2_id|
In each of these tests, I think it would be good to add
EXPECT_TRUE(db()->IsFileSystemConsistent()); right before the breaking change. 
That way it's very explicit that we set everything up right, and what caused the
failure.

[tzik, 2012-04-05 05:54:33]

On 2012/04/05 00:22:26, ericu wrote:
> You've addressed all my concerns, and the code looks good as written, but I've
> got another thought.  We used to recover from little things like a leftover
file
> in the data directory by just removing the file.  Now if there's an extra file
> in there for any reason we'll delete the whole filesystem.  I feel like we
> should soften that a bit.
> 
> Perhaps extra files [lost backing files] should be cleaned up, but not cause a
> full reset?  It might mean an inconsistent filesystem from the point of vie of
> the app, but it might also be the difference between a user losing a file and
a
> user losing all files.
> 
> Should we allow a little slop in either direction [lost backing file, extra
> backing file] and just prune the extras?
> 
> Kinuko, Michael, I'd like to hear your opinions as well.
> 

It's a matter.
We should not wipe out file system here, if the extra/lost backing exists due to
a user operation, with no relation to the database.
Otherwise, if the database is so defective as random entry was lost or changed
(I don't know it can happen), should we expose it to Web Apps as is?

http://codereview.chromium.org/9910005/diff/7002/webkit/fileapi/file_system_d...
File webkit/fileapi/file_system_directory_database_unittest.cc (right):

http://codereview.chromium.org/9910005/diff/7002/webkit/fileapi/file_system_d...
webkit/fileapi/file_system_directory_database_unittest.cc:130: void
ReplaceHierarchyLinkName(FileId file_id,
On 2012/04/05 00:22:26, ericu wrote:
> This could just use DeleteHierarchyLink and MakeHierarchyLink.  We don't need
> absolute speed here, and it would be simpler code to maintain.

Done.

http://codereview.chromium.org/9910005/diff/7002/webkit/fileapi/file_system_d...
webkit/fileapi/file_system_directory_database_unittest.cc:611:
DeleteHierarchyLink(dir2_id); // delete link from |dir1_id| to |dir2_id|
On 2012/04/05 00:22:26, ericu wrote:
> In each of these tests, I think it would be good to add
> EXPECT_TRUE(db()->IsFileSystemConsistent()); right before the breaking change.

> That way it's very explicit that we set everything up right, and what caused
the
> failure.

Done.

[kinuko, 2012-04-05 07:04:23]

On 2012/04/05 05:54:33, tzik wrote:
> On 2012/04/05 00:22:26, ericu wrote:
> > You've addressed all my concerns, and the code looks good as written, but
I've
> > got another thought.  We used to recover from little things like a leftover
> file
> > in the data directory by just removing the file.  Now if there's an extra
file
> > in there for any reason we'll delete the whole filesystem.  I feel like we
> > should soften that a bit.
> > 
> > Perhaps extra files [lost backing files] should be cleaned up, but not cause
a
> > full reset?  It might mean an inconsistent filesystem from the point of vie
of
> > the app, but it might also be the difference between a user losing a file
and
> a
> > user losing all files.
> > 
> > Should we allow a little slop in either direction [lost backing file, extra
> > backing file] and just prune the extras?
> > 
> > Kinuko, Michael, I'd like to hear your opinions as well.
> > 
> 
> It's a matter.
> We should not wipe out file system here, if the extra/lost backing exists due
to
> a user operation, with no relation to the database.
> Otherwise, if the database is so defective as random entry was lost or changed
> (I don't know it can happen), should we expose it to Web Apps as is?

In which case we could tell a file is left with an inconsistent change?

Thinking about this in comparison with the real fsck, I feel that we
should/could probably save both lost and extra backing file cases.

In terms of consistency from the view point of apps it's already hard to tell if
we are keeping it even without visible corruption.  I assume that the
implementation of leveldb and the manipulation order we perform in
ObfuscatedFileUtil will ensure we might be rewinding some recent operations
(say, by corruption in a log file) but will not leave totally random
inconsistent state. (Is that correct?)

>
http://codereview.chromium.org/9910005/diff/7002/webkit/fileapi/file_system_d...
> File webkit/fileapi/file_system_directory_database_unittest.cc (right):
> 
>
http://codereview.chromium.org/9910005/diff/7002/webkit/fileapi/file_system_d...
> webkit/fileapi/file_system_directory_database_unittest.cc:130: void
> ReplaceHierarchyLinkName(FileId file_id,
> On 2012/04/05 00:22:26, ericu wrote:
> > This could just use DeleteHierarchyLink and MakeHierarchyLink.  We don't
need
> > absolute speed here, and it would be simpler code to maintain.
> 
> Done.
> 
>
http://codereview.chromium.org/9910005/diff/7002/webkit/fileapi/file_system_d...
> webkit/fileapi/file_system_directory_database_unittest.cc:611:
> DeleteHierarchyLink(dir2_id); // delete link from |dir1_id| to |dir2_id|
> On 2012/04/05 00:22:26, ericu wrote:
> > In each of these tests, I think it would be good to add
> > EXPECT_TRUE(db()->IsFileSystemConsistent()); right before the breaking
change.
> 
> > That way it's very explicit that we set everything up right, and what caused
> the
> > failure.
> 
> Done.

[michaeln, 2012-04-05 19:07:40]

I'm thinking we should recover from missing/extras instead of deleting all. This
new logic runs when leveldb detects something is amiss inside. Seems fairly
likely that some leveldb edits will have gone missing after its recover function
has returned, and if thats the case it seems fairly likely that the 'checker'
class will then discover extra/missing files. And if that right, the likely
result of 'checking' will be to delete all as coded.

Pruning extra files and db entries with missing files would be my vote, less
data loss.

(This cl looks really nice!)

> > > Should we allow a little slop in either direction [lost backing file,
extra
> > > backing file] and just prune the extras?
> > > 
> > > Kinuko, Michael, I'd like to hear your opinions as well.
> > 
> > It's a matter.
> > We should not wipe out file system here, if the extra/lost backing exists
due
> to
> > a user operation, with no relation to the database.
> > Otherwise, if the database is so defective as random entry was lost or
changed
> > (I don't know it can happen), should we expose it to Web Apps as is?
> 
> In which case we could tell a file is left with an inconsistent change?

I doubt we can distinguish between a user having manually deleted a file and a
more serious failure.

> 
> Thinking about this in comparison with the real fsck, I feel that we
> should/could probably save both lost and extra backing file cases.
> 
> In terms of consistency from the view point of apps it's already hard to tell
if
> we are keeping it even without visible corruption.  I assume that the
> implementation of leveldb and the manipulation order we perform in
> ObfuscatedFileUtil will ensure we might be rewinding some recent operations
> (say, by corruption in a log file) but will not leave totally random
> inconsistent state. (Is that correct?)

[tzik, 2012-04-09 06:28:41]

Updated, now extra files in actual filesystem and extra database entries go away
without deleting whole filesystem.

[ericu, 2012-04-09 19:36:15]

On 2012/04/05 07:04:23, kinuko wrote:
> On 2012/04/05 05:54:33, tzik wrote:
> > On 2012/04/05 00:22:26, ericu wrote:
> > > You've addressed all my concerns, and the code looks good as written, but
> I've
> > > got another thought.  We used to recover from little things like a
leftover
> > file
> > > in the data directory by just removing the file.  Now if there's an extra
> file
> > > in there for any reason we'll delete the whole filesystem.  I feel like we
> > > should soften that a bit.
> > > 
> > > Perhaps extra files [lost backing files] should be cleaned up, but not
cause
> a
> > > full reset?  It might mean an inconsistent filesystem from the point of
vie
> of
> > > the app, but it might also be the difference between a user losing a file
> and
> > a
> > > user losing all files.
> > > 
> > > Should we allow a little slop in either direction [lost backing file,
extra
> > > backing file] and just prune the extras?
> > > 
> > > Kinuko, Michael, I'd like to hear your opinions as well.
> > > 
> > 
> > It's a matter.
> > We should not wipe out file system here, if the extra/lost backing exists
due
> to
> > a user operation, with no relation to the database.
> > Otherwise, if the database is so defective as random entry was lost or
changed
> > (I don't know it can happen), should we expose it to Web Apps as is?
> 
> In which case we could tell a file is left with an inconsistent change?
> 
> Thinking about this in comparison with the real fsck, I feel that we
> should/could probably save both lost and extra backing file cases.

The last time we discussed this, we decided that extra backing files are
probably failed deletions, so dropping them would be OK.  Lost backing files we
can't recover at all.  If we add some kind of "lost+found" directory for extra
backing files, we'll have to reserve that namespace in the spec.  I'd rather
not, at least for now.

> In terms of consistency from the view point of apps it's already hard to tell
if
> we are keeping it even without visible corruption.  I assume that the
> implementation of leveldb and the manipulation order we perform in
> ObfuscatedFileUtil will ensure we might be rewinding some recent operations
> (say, by corruption in a log file) but will not leave totally random
> inconsistent state. (Is that correct?)

I'm sure it's not guaranteed, but depends on the type of corruption.  However,
if we get random state, we'll either delete individual entries until it makes
sense, or we'll nuke the whole filesystem and start over, so at least the user
can start again.

>
http://codereview.chromium.org/9910005/diff/7002/webkit/fileapi/file_system_d...
> > File webkit/fileapi/file_system_directory_database_unittest.cc (right):
> > 
> >
>
http://codereview.chromium.org/9910005/diff/7002/webkit/fileapi/file_system_d...
> > webkit/fileapi/file_system_directory_database_unittest.cc:130: void
> > ReplaceHierarchyLinkName(FileId file_id,
> > On 2012/04/05 00:22:26, ericu wrote:
> > > This could just use DeleteHierarchyLink and MakeHierarchyLink.  We don't
> need
> > > absolute speed here, and it would be simpler code to maintain.
> > 
> > Done.
> > 
> >
>
http://codereview.chromium.org/9910005/diff/7002/webkit/fileapi/file_system_d...
> > webkit/fileapi/file_system_directory_database_unittest.cc:611:
> > DeleteHierarchyLink(dir2_id); // delete link from |dir1_id| to |dir2_id|
> > On 2012/04/05 00:22:26, ericu wrote:
> > > In each of these tests, I think it would be good to add
> > > EXPECT_TRUE(db()->IsFileSystemConsistent()); right before the breaking
> change.
> > 
> > > That way it's very explicit that we set everything up right, and what
caused
> > the
> > > failure.
> > 
> > Done.

[ericu, 2012-04-09 19:36:58]

LGTM except for those test failures.

[kinuko, 2012-04-10 05:04:57]

On 2012/04/09 19:36:15, ericu wrote:
> On 2012/04/05 07:04:23, kinuko wrote:
> > On 2012/04/05 05:54:33, tzik wrote:
> > > On 2012/04/05 00:22:26, ericu wrote:
> > > > You've addressed all my concerns, and the code looks good as written,
but
> > I've
> > > > got another thought.  We used to recover from little things like a
> leftover
> > > file
> > > > in the data directory by just removing the file.  Now if there's an
extra
> > file
> > > > in there for any reason we'll delete the whole filesystem.  I feel like
we
> > > > should soften that a bit.
> > > > 
> > > > Perhaps extra files [lost backing files] should be cleaned up, but not
> cause
> > a
> > > > full reset?  It might mean an inconsistent filesystem from the point of
> vie
> > of
> > > > the app, but it might also be the difference between a user losing a
file
> > and
> > > a
> > > > user losing all files.
> > > > 
> > > > Should we allow a little slop in either direction [lost backing file,
> extra
> > > > backing file] and just prune the extras?
> > > > 
> > > > Kinuko, Michael, I'd like to hear your opinions as well.
> > > > 
> > > 
> > > It's a matter.
> > > We should not wipe out file system here, if the extra/lost backing exists
> due
> > to
> > > a user operation, with no relation to the database.
> > > Otherwise, if the database is so defective as random entry was lost or
> changed
> > > (I don't know it can happen), should we expose it to Web Apps as is?
> > 
> > In which case we could tell a file is left with an inconsistent change?
> > 
> > Thinking about this in comparison with the real fsck, I feel that we
> > should/could probably save both lost and extra backing file cases.
> 
> The last time we discussed this, we decided that extra backing files are
> probably failed deletions, so dropping them would be OK.  Lost backing files
we
> can't recover at all.  If we add some kind of "lost+found" directory for extra
> backing files, we'll have to reserve that namespace in the spec.  I'd rather
> not, at least for now.

Agreed, I wanted to say we should not nuke the entire filesystem in either case.
 Unless we have a way to expose such files in a "lost+found" type directory (at
least to DevTools) I don't think we should keep them either.


> > In terms of consistency from the view point of apps it's already hard to
tell
> if
> > we are keeping it even without visible corruption.  I assume that the
> > implementation of leveldb and the manipulation order we perform in
> > ObfuscatedFileUtil will ensure we might be rewinding some recent operations
> > (say, by corruption in a log file) but will not leave totally random
> > inconsistent state. (Is that correct?)
> 
> I'm sure it's not guaranteed, but depends on the type of corruption.  However,
> if we get random state, we'll either delete individual entries until it makes
> sense, or we'll nuke the whole filesystem and start over, so at least the user
> can start again.
> 
> >
>
http://codereview.chromium.org/9910005/diff/7002/webkit/fileapi/file_system_d...
> > > File webkit/fileapi/file_system_directory_database_unittest.cc (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/9910005/diff/7002/webkit/fileapi/file_system_d...
> > > webkit/fileapi/file_system_directory_database_unittest.cc:130: void
> > > ReplaceHierarchyLinkName(FileId file_id,
> > > On 2012/04/05 00:22:26, ericu wrote:
> > > > This could just use DeleteHierarchyLink and MakeHierarchyLink.  We don't
> > need
> > > > absolute speed here, and it would be simpler code to maintain.
> > > 
> > > Done.
> > > 
> > >
> >
>
http://codereview.chromium.org/9910005/diff/7002/webkit/fileapi/file_system_d...
> > > webkit/fileapi/file_system_directory_database_unittest.cc:611:
> > > DeleteHierarchyLink(dir2_id); // delete link from |dir1_id| to |dir2_id|
> > > On 2012/04/05 00:22:26, ericu wrote:
> > > > In each of these tests, I think it would be good to add
> > > > EXPECT_TRUE(db()->IsFileSystemConsistent()); right before the breaking
> > change.
> > > 
> > > > That way it's very explicit that we set everything up right, and what
> caused
> > > the
> > > > failure.
> > > 
> > > Done.

[tzik, 2012-04-11 04:54:17]

On 2012/04/09 19:36:58, ericu wrote:
> LGTM except for those test failures.

Thanks.
Next try will go well.

[kinuko, 2012-04-11 10:58:49]

lgtm2 modulo some nits.

http://codereview.chromium.org/9910005/diff/33001/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_database_test_helper.cc (right):

http://codereview.chromium.org/9910005/diff/33001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_database_test_helper.cc:83: }
ditto.

http://codereview.chromium.org/9910005/diff/33001/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_database_test_helper.h (right):

http://codereview.chromium.org/9910005/diff/33001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_database_test_helper.h:20: }
nit: can you add '// namespace fileapi' comment and add a blank line before line
20?

http://codereview.chromium.org/9910005/diff/33001/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_directory_database.cc (right):

http://codereview.chromium.org/9910005/diff/33001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_directory_database.cc:295: return false;
Should we simply delete this..?

http://codereview.chromium.org/9910005/diff/33001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_directory_database.cc:701: return false;
Maybe we should output some WARNING message for these two failure cases?

http://codereview.chromium.org/9910005/diff/33001/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_directory_database_unittest.cc (right):

http://codereview.chromium.org/9910005/diff/33001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_directory_database_unittest.cc:43: // Call
CloseDatabase() to avoid multiple database instance for single
nit: 'avoid multiple database instance' -> 'avoid having multiple database
instances'

http://codereview.chromium.org/9910005/diff/33001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_directory_database_unittest.cc:92:
base::PlatformFileError error;
nit: let's initialize created and error before calling in.

http://codereview.chromium.org/9910005/diff/33001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_directory_database_unittest.cc:520: int64 next;
nit: initialize next

http://codereview.chromium.org/9910005/diff/33001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_directory_database_unittest.cc:562:
base::PlatformFileError error;
ditto

[tzik, 2012-04-11 13:39:06]

Thanks!

http://codereview.chromium.org/9910005/diff/33001/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_database_test_helper.cc (right):

http://codereview.chromium.org/9910005/diff/33001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_database_test_helper.cc:83: }
On 2012/04/11 10:58:49, kinuko wrote:
> ditto.

Done.

http://codereview.chromium.org/9910005/diff/33001/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_database_test_helper.h (right):

http://codereview.chromium.org/9910005/diff/33001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_database_test_helper.h:20: }
On 2012/04/11 10:58:49, kinuko wrote:
> nit: can you add '// namespace fileapi' comment and add a blank line before
line
> 20?

Done.

http://codereview.chromium.org/9910005/diff/33001/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_directory_database.cc (right):

http://codereview.chromium.org/9910005/diff/33001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_directory_database.cc:295: return false;
On 2012/04/11 10:58:49, kinuko wrote:
> Should we simply delete this..?

Looks able to erase whole line.
If the link have backing entry, previous scan removes the database entry.
So, next 305 always remove it.

http://codereview.chromium.org/9910005/diff/33001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_directory_database.cc:701: return false;
On 2012/04/11 10:58:49, kinuko wrote:
> Maybe we should output some WARNING message for these two failure cases?

Done.

http://codereview.chromium.org/9910005/diff/33001/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_directory_database_unittest.cc (right):

http://codereview.chromium.org/9910005/diff/33001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_directory_database_unittest.cc:43: // Call
CloseDatabase() to avoid multiple database instance for single
On 2012/04/11 10:58:49, kinuko wrote:
> nit: 'avoid multiple database instance' -> 'avoid having multiple database
> instances'

Done.

http://codereview.chromium.org/9910005/diff/33001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_directory_database_unittest.cc:92:
base::PlatformFileError error;
On 2012/04/11 10:58:49, kinuko wrote:
> nit: let's initialize created and error before calling in.

Done.

http://codereview.chromium.org/9910005/diff/33001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_directory_database_unittest.cc:520: int64 next;
On 2012/04/11 10:58:49, kinuko wrote:
> nit: initialize next

Done.

http://codereview.chromium.org/9910005/diff/33001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_directory_database_unittest.cc:562:
base::PlatformFileError error;
On 2012/04/11 10:58:49, kinuko wrote:
> ditto

Done.

[commit-bot: I haz the power, 2012-04-11 14:32:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tzik@chromium.org/9910005/37002

[commit-bot: I haz the power, 2012-04-11 14:32:07]

Presubmit check for 9910005-37002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    webkit

Presubmit checks took 1.3s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[tzik, 2012-04-12 02:31:53]

+darin for webkit/ owner.

Darin: Can I add files to test_shell_tests?

[tzik, 2012-04-16 04:10:11]

On 2012/04/12 02:31:53, tzik wrote:
> +darin for webkit/ owner.
> 
> Darin: Can I add files to test_shell_tests?

Assuming unreviewed part is enough tiny, I'll dcommit it.

[darin (slow to review), 2012-04-16 18:10:07]

LGTM for webkit/tools/