Handle inconsistency between DB and Files in ObfuscatedFileSystemFileUtil

Handle inconsistency between DB and Files in ObfuscatedFileSystemFileUtil


BUG=91328
TEST='ObfuscatedFileSystemFileUtilTest.*'

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=95973

[tzik, 2011-08-02 07:36:02]

As I wrote in the BUG, errors in deletion of a origin may make a stray file
without database entry.

I'm trying to handle it with deleting such a file.
Could you review this patch?

[ericu, 2011-08-02 23:45:10]

LGTM.

http://codereview.chromium.org/7540022/diff/9002/webkit/fileapi/obfuscated_fi...
File webkit/fileapi/obfuscated_file_system_file_util.cc (right):

http://codereview.chromium.org/7540022/diff/9002/webkit/fileapi/obfuscated_fi...
webkit/fileapi/obfuscated_file_system_file_util.cc:82: // The file doesn't exist
on database.
typo: "on"->"in the"

http://codereview.chromium.org/7540022/diff/9002/webkit/fileapi/obfuscated_fi...
webkit/fileapi/obfuscated_file_system_file_util.cc:97: file_flags |=
base::PLATFORM_FILE_CREATE_ALWAYS;
Perhaps we should also clear out PLATFORM_FILE_OPEN and
PLATFORM_FILE_OPEN_TRUNCATED, just to be sure?

http://codereview.chromium.org/7540022/diff/9002/webkit/fileapi/obfuscated_fi...
File webkit/fileapi/obfuscated_file_system_file_util_unittest.cc (right):

http://codereview.chromium.org/7540022/diff/9002/webkit/fileapi/obfuscated_fi...
webkit/fileapi/obfuscated_file_system_file_util_unittest.cc:1017:
ofsfu()->Truncate(context.get(), kPath1, 10));
Please validate [using GetFileInfo] that the file is 10 bytes long now.

[tzik, 2011-08-03 06:26:31]

Thanks for your reviewing.

http://codereview.chromium.org/7540022/diff/9002/webkit/fileapi/obfuscated_fi...
File webkit/fileapi/obfuscated_file_system_file_util.cc (right):

http://codereview.chromium.org/7540022/diff/9002/webkit/fileapi/obfuscated_fi...
webkit/fileapi/obfuscated_file_system_file_util.cc:82: // The file doesn't exist
on database.
On 2011/08/02 23:45:10, ericu wrote:
> typo: "on"->"in the"

Done.

http://codereview.chromium.org/7540022/diff/9002/webkit/fileapi/obfuscated_fi...
webkit/fileapi/obfuscated_file_system_file_util.cc:97: file_flags |=
base::PLATFORM_FILE_CREATE_ALWAYS;
On 2011/08/02 23:45:10, ericu wrote:
> Perhaps we should also clear out PLATFORM_FILE_OPEN and
> PLATFORM_FILE_OPEN_TRUNCATED, just to be sure?

Done.

http://codereview.chromium.org/7540022/diff/9002/webkit/fileapi/obfuscated_fi...
File webkit/fileapi/obfuscated_file_system_file_util_unittest.cc (right):

http://codereview.chromium.org/7540022/diff/9002/webkit/fileapi/obfuscated_fi...
webkit/fileapi/obfuscated_file_system_file_util_unittest.cc:1017:
ofsfu()->Truncate(context.get(), kPath1, 10));
On 2011/08/02 23:45:10, ericu wrote:
> Please validate [using GetFileInfo] that the file is 10 bytes long now.

Done.

[kinuko, 2011-08-03 08:09:03]

lgtm

http://codereview.chromium.org/7540022/diff/12001/webkit/fileapi/obfuscated_f...
File webkit/fileapi/obfuscated_file_system_file_util.cc (right):

http://codereview.chromium.org/7540022/diff/12001/webkit/fileapi/obfuscated_f...
webkit/fileapi/obfuscated_file_system_file_util.cc:810: // If the file was
already exists, we found a stray file out of
"If the file already exists" or "has existed"?

http://codereview.chromium.org/7540022/diff/12001/webkit/fileapi/obfuscated_f...
File webkit/fileapi/obfuscated_file_system_file_util_unittest.cc (right):

http://codereview.chromium.org/7540022/diff/12001/webkit/fileapi/obfuscated_f...
webkit/fileapi/obfuscated_file_system_file_util_unittest.cc:1005: const FilePath
kPath2 = FilePath().AppendASCII("fuga");
nit... you seem to be the first one who's bringing hoge/fuga terms for testing.

http://codereview.chromium.org/7540022/diff/12001/webkit/fileapi/obfuscated_f...
webkit/fileapi/obfuscated_file_system_file_util_unittest.cc:1007:
scoped_ptr<FileSystemOperationContext> context(NewContext());
In most cases we seem to be creating a new OperationContext for each operation. 
I don't have a strong opinion but might be better to follow other tests?

http://codereview.chromium.org/7540022/diff/12001/webkit/fileapi/obfuscated_f...
webkit/fileapi/obfuscated_file_system_file_util_unittest.cc:1023: // Destroy
database to make inconsistency between database and filesystem
nit: finish comments with period. (you may need two lines...)

http://codereview.chromium.org/7540022/diff/12001/webkit/fileapi/obfuscated_f...
webkit/fileapi/obfuscated_file_system_file_util_unittest.cc:1035: 
should we also have a test case for CreateOrOpen or can it be subsumed by
EnsureFileExists tests?

[ericu, 2011-08-03 16:47:44]

http://codereview.chromium.org/7540022/diff/12001/webkit/fileapi/obfuscated_f...
File webkit/fileapi/obfuscated_file_system_file_util_unittest.cc (right):

http://codereview.chromium.org/7540022/diff/12001/webkit/fileapi/obfuscated_f...
webkit/fileapi/obfuscated_file_system_file_util_unittest.cc:1005: const FilePath
kPath2 = FilePath().AppendASCII("fuga");
On 2011/08/03 08:09:04, kinuko wrote:
> nit... you seem to be the first one who's bringing hoge/fuga terms for
testing.

I'm glad he did--I hadn't heard of these before!

Other non-Japanese-speakers can read
http://en.wikipedia.org/wiki/Metasyntactic_variable#Japanese.

[tzik, 2011-08-05 03:31:43]

http://codereview.chromium.org/7540022/diff/12001/webkit/fileapi/obfuscated_f...
File webkit/fileapi/obfuscated_file_system_file_util.cc (right):

http://codereview.chromium.org/7540022/diff/12001/webkit/fileapi/obfuscated_f...
webkit/fileapi/obfuscated_file_system_file_util.cc:810: // If the file was
already exists, we found a stray file out of
On 2011/08/03 08:09:04, kinuko wrote:
> "If the file already exists" or "has existed"?

Done.

http://codereview.chromium.org/7540022/diff/12001/webkit/fileapi/obfuscated_f...
File webkit/fileapi/obfuscated_file_system_file_util_unittest.cc (right):

http://codereview.chromium.org/7540022/diff/12001/webkit/fileapi/obfuscated_f...
webkit/fileapi/obfuscated_file_system_file_util_unittest.cc:1007:
scoped_ptr<FileSystemOperationContext> context(NewContext());
On 2011/08/03 08:09:04, kinuko wrote:
> In most cases we seem to be creating a new OperationContext for each
operation. 
> I don't have a strong opinion but might be better to follow other tests?

Done.

http://codereview.chromium.org/7540022/diff/12001/webkit/fileapi/obfuscated_f...
webkit/fileapi/obfuscated_file_system_file_util_unittest.cc:1023: // Destroy
database to make inconsistency between database and filesystem
On 2011/08/03 08:09:04, kinuko wrote:
> nit: finish comments with period. (you may need two lines...)

Done.

http://codereview.chromium.org/7540022/diff/12001/webkit/fileapi/obfuscated_f...
webkit/fileapi/obfuscated_file_system_file_util_unittest.cc:1035: 
On 2011/08/03 08:09:04, kinuko wrote:
> should we also have a test case for CreateOrOpen or can it be subsumed by
> EnsureFileExists tests?

Done.

[commit-bot: I haz the power, 2011-08-08 06:17:40]

Commit queue patch verification failed without an error message.
Something went wrong, probably a crash, a hickup or simply
the monkeys went out for diner.
Ping the relevant dude on a on-needed basis.

[commit-bot: I haz the power, 2011-08-08 14:22:04]

Commit queue patch verification failed without an error message.
Something went wrong, probably a crash, a hickup or simply
the monkeys went out for diner.
Ping the relevant dude on a on-needed basis.