A database to hold mappings from origin identifiers to unique directory names.

A database to hold mappings from origin identifiers to unique directory names.


BUG=NONE
TEST=included


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

[ericu, 2011-04-28 21:50:29]

Here's the first component of the obfuscated filesystem.

David, could you just check my use of LevelDB?
Michael, would you handle the rest?

Thanks,

    Eric

[michaeln, 2011-04-28 23:13:44]

thnx for the small CL, looks nice modulo some nits

http://codereview.chromium.org/6903118/diff/2001/webkit/fileapi/file_system_o...
File webkit/fileapi/file_system_origin_database.cc (right):

http://codereview.chromium.org/6903118/diff/2001/webkit/fileapi/file_system_o...
webkit/fileapi/file_system_origin_database.cc:15: const char kLastPathKey[] =
"LAST_PATH";
we don't indent for namespaces

http://codereview.chromium.org/6903118/diff/2001/webkit/fileapi/file_system_o...
webkit/fileapi/file_system_origin_database.cc:102: if (directory) {
is there a use case for 'directory' param being NULL? maybe a remnant of
HasOriginPath being implemented in terms of GetPathForOrigin?

http://codereview.chromium.org/6903118/diff/2001/webkit/fileapi/file_system_o...
webkit/fileapi/file_system_origin_database.cc:127: }
style nit: {}'s not needed for the one-liner

http://codereview.chromium.org/6903118/diff/2001/webkit/fileapi/file_system_o...
webkit/fileapi/file_system_origin_database.cc:131: } else {
since you return on line 130, do u want/need the else clause?

http://codereview.chromium.org/6903118/diff/2001/webkit/fileapi/file_system_o...
webkit/fileapi/file_system_origin_database.cc:137: LOG(ERROR) << "File system
origin database is corrupt!";
yikes, how do we handle this?

http://codereview.chromium.org/6903118/diff/2001/webkit/fileapi/file_system_o...
File webkit/fileapi/file_system_origin_database.h (right):

http://codereview.chromium.org/6903118/diff/2001/webkit/fileapi/file_system_o...
webkit/fileapi/file_system_origin_database.h:12: #include "base/scoped_ptr.h"
alpha ordering

http://codereview.chromium.org/6903118/diff/2001/webkit/fileapi/file_system_o...
webkit/fileapi/file_system_origin_database.h:22: FileSystemOriginDatabase(const
FilePath& path);
Is this class complex enough to warrant a dtor?
http://dev.chromium.org/developers/coding-style/chromium-style-checker-errors

http://codereview.chromium.org/6903118/diff/2001/webkit/fileapi/file_system_o...
webkit/fileapi/file_system_origin_database.h:32: 
There may be privacy motivated usecases to delete entries from this database?
And data integrity check reasons to list entries in here.

http://codereview.chromium.org/6903118/diff/2001/webkit/fileapi/file_system_o...
webkit/fileapi/file_system_origin_database.h:39: scoped_ptr<leveldb::DB> db_;
DISALLOW_COPY_AND_ASSIGN(FileSystemOriginDatabas);

http://codereview.chromium.org/6903118/diff/2001/webkit/fileapi/file_system_o...
File webkit/fileapi/file_system_origin_database_unittest.cc (right):

http://codereview.chromium.org/6903118/diff/2001/webkit/fileapi/file_system_o...
webkit/fileapi/file_system_origin_database_unittest.cc:29:
EXPECT_FALSE(database.GetPathForOrigin(std::string(), NULL));
ah... the test case use-case, not sure that's a strong enough of a use-case to
say 'directory' can be null

http://codereview.chromium.org/6903118/diff/2001/webkit/tools/test_shell/test...
File webkit/tools/test_shell/test_shell.gypi (right):

http://codereview.chromium.org/6903118/diff/2001/webkit/tools/test_shell/test...
webkit/tools/test_shell/test_shell.gypi:349:
'<(DEPTH)/third_party/leveldb/leveldb.gyp:leveldb',
Since the fileapi library has a dependency on this, does it have to be
explicitly listed here? I would have expected this build target to have
inherited that dependency?

[dgrogan, 2011-04-29 01:34:25]

Leveldb stuff LGTM.

http://codereview.chromium.org/6903118/diff/2001/webkit/fileapi/file_system_o...
File webkit/fileapi/file_system_origin_database_unittest.cc (right):

http://codereview.chromium.org/6903118/diff/2001/webkit/fileapi/file_system_o...
webkit/fileapi/file_system_origin_database_unittest.cc:91:
EXPECT_TRUE(database.HasOriginPath(origin));
HasOriginPath's call to Init() reopens the leveldb database, is that right?

[dgrogan, 2011-04-29 01:44:47]

Sorry, 2 more questions.

http://codereview.chromium.org/6903118/diff/2001/webkit/fileapi/file_system_o...
File webkit/fileapi/file_system_origin_database.cc (right):

http://codereview.chromium.org/6903118/diff/2001/webkit/fileapi/file_system_o...
webkit/fileapi/file_system_origin_database.cc:90: path_string =
base::IntToString(last_path_number + 1);
The unique paths are monotonically increasing integers, is that right?

http://codereview.chromium.org/6903118/diff/2001/webkit/fileapi/file_system_o...
webkit/fileapi/file_system_origin_database.cc:111: return false;
You never expect to get here right?  You might want to assert(something) or
LOG(ERROR) << status.ToString().

[ericu, 2011-04-30 00:44:41]

I've addressed all your comments and uploaded a new version.

http://codereview.chromium.org/6903118/diff/2001/webkit/fileapi/file_system_o...
File webkit/fileapi/file_system_origin_database.cc (right):

http://codereview.chromium.org/6903118/diff/2001/webkit/fileapi/file_system_o...
webkit/fileapi/file_system_origin_database.cc:15: const char kLastPathKey[] =
"LAST_PATH";
On 2011/04/28 23:13:44, michaeln wrote:
> we don't indent for namespaces

Done.

http://codereview.chromium.org/6903118/diff/2001/webkit/fileapi/file_system_o...
webkit/fileapi/file_system_origin_database.cc:90: path_string =
base::IntToString(last_path_number + 1);
On 2011/04/29 01:44:47, dgrogan wrote:
> The unique paths are monotonically increasing integers, is that right?

Yup.  But that's just internal knowledge; we only promise that they're unique,
assuming that nobody messes around in the directory.

http://codereview.chromium.org/6903118/diff/2001/webkit/fileapi/file_system_o...
webkit/fileapi/file_system_origin_database.cc:102: if (directory) {
On 2011/04/28 23:13:44, michaeln wrote:
> is there a use case for 'directory' param being NULL? maybe a remnant of
> HasOriginPath being implemented in terms of GetPathForOrigin?

Done.

http://codereview.chromium.org/6903118/diff/2001/webkit/fileapi/file_system_o...
webkit/fileapi/file_system_origin_database.cc:111: return false;
On 2011/04/29 01:44:47, dgrogan wrote:
> You never expect to get here right?  You might want to assert(something) or
> LOG(ERROR) << status.ToString().

Good catch--I added the missing HandleError call.  This could happen due to
circumstances beyond our control [the users mucks around with storage or has
disk space issues], so I think a log message is the right level of error.

http://codereview.chromium.org/6903118/diff/2001/webkit/fileapi/file_system_o...
webkit/fileapi/file_system_origin_database.cc:127: }
On 2011/04/28 23:13:44, michaeln wrote:
> style nit: {}'s not needed for the one-liner

Done.

http://codereview.chromium.org/6903118/diff/2001/webkit/fileapi/file_system_o...
webkit/fileapi/file_system_origin_database.cc:131: } else {
On 2011/04/28 23:13:44, michaeln wrote:
> since you return on line 130, do u want/need the else clause?

Done.

http://codereview.chromium.org/6903118/diff/2001/webkit/fileapi/file_system_o...
webkit/fileapi/file_system_origin_database.cc:137: LOG(ERROR) << "File system
origin database is corrupt!";
On 2011/04/28 23:13:44, michaeln wrote:
> yikes, how do we handle this?

Well, so far, we just don't open that database any more, and you can't use the
filesystem.  This is one of the things that FSCK will have to handle, but
that'll go in quite a bit later.  It will take knowledge that this class doesn't
cover [what directories actually exist] to repair damage to this database.

http://codereview.chromium.org/6903118/diff/2001/webkit/fileapi/file_system_o...
File webkit/fileapi/file_system_origin_database.h (right):

http://codereview.chromium.org/6903118/diff/2001/webkit/fileapi/file_system_o...
webkit/fileapi/file_system_origin_database.h:12: #include "base/scoped_ptr.h"
On 2011/04/28 23:13:44, michaeln wrote:
> alpha ordering

Done.

http://codereview.chromium.org/6903118/diff/2001/webkit/fileapi/file_system_o...
webkit/fileapi/file_system_origin_database.h:22: FileSystemOriginDatabase(const
FilePath& path);
On 2011/04/28 23:13:44, michaeln wrote:
> Is this class complex enough to warrant a dtor?
> http://dev.chromium.org/developers/coding-style/chromium-style-checker-errors

Done.

http://codereview.chromium.org/6903118/diff/2001/webkit/fileapi/file_system_o...
webkit/fileapi/file_system_origin_database.h:32: 
On 2011/04/28 23:13:44, michaeln wrote:
> There may be privacy motivated usecases to delete entries from this database?
> And data integrity check reasons to list entries in here.

Added methods to list and delete entries.

http://codereview.chromium.org/6903118/diff/2001/webkit/fileapi/file_system_o...
webkit/fileapi/file_system_origin_database.h:39: scoped_ptr<leveldb::DB> db_;
On 2011/04/28 23:13:44, michaeln wrote:
> DISALLOW_COPY_AND_ASSIGN(FileSystemOriginDatabas);

Done.

http://codereview.chromium.org/6903118/diff/2001/webkit/fileapi/file_system_o...
File webkit/fileapi/file_system_origin_database_unittest.cc (right):

http://codereview.chromium.org/6903118/diff/2001/webkit/fileapi/file_system_o...
webkit/fileapi/file_system_origin_database_unittest.cc:29:
EXPECT_FALSE(database.GetPathForOrigin(std::string(), NULL));
On 2011/04/28 23:13:44, michaeln wrote:
> ah... the test case use-case, not sure that's a strong enough of a use-case to
> say 'directory' can be null

It was more a leftover from implementation; I just happened to take advantage of
it here.  Removed.

http://codereview.chromium.org/6903118/diff/2001/webkit/fileapi/file_system_o...
webkit/fileapi/file_system_origin_database_unittest.cc:91:
EXPECT_TRUE(database.HasOriginPath(origin));
On 2011/04/29 01:34:25, dgrogan wrote:
> HasOriginPath's call to Init() reopens the leveldb database, is that right?

Right.  I wanted to make sure we could still use it after calling
DropDatabase().

http://codereview.chromium.org/6903118/diff/2001/webkit/tools/test_shell/test...
File webkit/tools/test_shell/test_shell.gypi (right):

http://codereview.chromium.org/6903118/diff/2001/webkit/tools/test_shell/test...
webkit/tools/test_shell/test_shell.gypi:349:
'<(DEPTH)/third_party/leveldb/leveldb.gyp:leveldb',
On 2011/04/28 23:13:44, michaeln wrote:
> Since the fileapi library has a dependency on this, does it have to be
> explicitly listed here? I would have expected this build target to have
> inherited that dependency?

I tried; it doesn't seem to pick up header file paths that way.

[dgrogan, 2011-04-30 00:59:31]

Leveldb stuff LGTM.

[michaeln, 2011-05-02 01:03:35]

LGTM2

[commit-bot: I haz the power, 2011-05-02 17:03:16]

Try job failure for 6903118-5001 on win:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[dgrogan, 2011-05-02 17:07:52]

D'oh, this has something to do with leveldb.

..\..\third_party\leveldb\include\leveldb/db.h(8) : fatal error C1083:
Cannot open include file: 'stdint.h': No such
<http://build.chromium.org/p/tryserver.chromium/builders/win/builds/29330/step...>
file or
directory<http://build.chromium.org/p/tryserver.chromium/builders/win/builds/29330/steps/compile/logs/stdio>

On Mon, May 2, 2011 at 10:03 AM, <commit-bot@chromium.org> wrote:

> Try job failure for 6903118-5001 on win:
>
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
>
>
> http://codereview.chromium.org/6903118/
>

[ericu, 2011-05-02 17:17:06]

According to a quick web search, stdint.h isn't in VS before 2010, as
it's a C99 header.

David, you must have seen levelDB build on windows in indexedDB, so we
must have that header redefined somewhere magical.  How do we invoke
the magic here?

On Mon, May 2, 2011 at 10:07 AM, David Grogan <dgrogan@chromium.org> wrote:
> D'oh, this has something to do with leveldb.
>
> ..\..\third_party\leveldb\include\leveldb/db.h(8) : fatal error C1083:
> Cannot open include file: 'stdint.h': No such
> file or directory
>
> On Mon, May 2, 2011 at 10:03 AM, <commit-bot@chromium.org> wrote:
>>
>> Try job failure for 6903118-5001 on win:
>>
>>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
>>
>> http://codereview.chromium.org/6903118/
>
>

[ericu, 2011-05-02 17:22:58]

Ah, there's a version inside leveldb that's conditionally included,
and my Linux tryjob just failed too [despite my local Linux build
succeeding].  I think I'm missing a dependency somewhere; I'll look
for that.

On Mon, May 2, 2011 at 10:16 AM, Eric Uhrhane <ericu@chromium.org> wrote:
> According to a quick web search, stdint.h isn't in VS before 2010, as
> it's a C99 header.
>
> David, you must have seen levelDB build on windows in indexedDB, so we
> must have that header redefined somewhere magical.  How do we invoke
> the magic here?
>
> On Mon, May 2, 2011 at 10:07 AM, David Grogan <dgrogan@chromium.org> wrote:
>> D'oh, this has something to do with leveldb.
>>
>> ..\..\third_party\leveldb\include\leveldb/db.h(8) : fatal error C1083:
>> Cannot open include file: 'stdint.h': No such
>> file or directory
>>
>> On Mon, May 2, 2011 at 10:03 AM, <commit-bot@chromium.org> wrote:
>>>
>>> Try job failure for 6903118-5001 on win:
>>>
>>>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
>>>
>>> http://codereview.chromium.org/6903118/
>>
>>
>

[commit-bot: I haz the power, 2011-05-04 02:29:19]

Change committed as 84029