Add a helper class that keeps per-profile information for FileSystem API

Add a helper class that keeps per-profile information for FileSystem API

BUG=32277
TEST=FileSystemHostContextTest

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

[kinuko, 2010-07-29 02:14:21]

A small helper class that is going to be hooked up by a DispatcherHost for
FileSystem API.

I wondered if I should make a subdir, but for now just put under chrome/browser
as we aren't going to have many files for the API.

[Paweł Hajdan Jr., 2010-07-29 16:53:03]

Drive-by with minor test comments.

http://codereview.chromium.org/3028033/diff/1/4
File chrome/browser/filesystem_host_context_unittest.cc (right):

http://codereview.chromium.org/3028033/diff/1/4#newcode79
chrome/browser/filesystem_host_context_unittest.cc:79: for (size_t i = 0; i <
ARRAYSIZE_UNSAFE(kRootPathTestCases); ++i) {
Could you use SCOPED_TRACE to display the test case information? It'd be useful
to have the iteration number, and data from the test case.

http://codereview.chromium.org/3028033/diff/1/4#newcode96
chrome/browser/filesystem_host_context_unittest.cc:96: for (size_t i = 0; i <
ARRAYSIZE_UNSAFE(kCheckValidPathTestCases); ++i) {
Same here.

[michaeln, 2010-07-29 22:48:40]

> I wondered if I should make a subdir, but for now just put under
chrome/browser
> as we aren't going to have many files for the API.

I wonder if we should make a 'src/webkit/file_system' directory and
have this class live there? If we want to build it for testing in
DRT/test_shell, I think that's where this would go.

http://codereview.chromium.org/3028033/diff/1/2
File chrome/browser/filesystem_host_context.cc (right):

http://codereview.chromium.org/3028033/diff/1/2#newcode20
chrome/browser/filesystem_host_context.cc:20: const char
FileSystemHostContext::kTemporaryName[] = "Temporary";
Unfortunate to have two forms of "Persistent" constants here. If all we had was
the ascii char form, could we generate the FilePaths as needed? FilePath has an
AppendASCII() method that can be useful.

http://codereview.chromium.org/3028033/diff/1/2#newcode30
chrome/browser/filesystem_host_context.cc:30: return
base_path_.Append(origin_id).Append(kPersistentDirectory);
Having FilePath::StringType here and std::string origin_id in GetFileSystemName
seems odd, why the differing types?

Honestly, I'm always hesitant when looking at 'origin' parameters in string
form. It's never clear if its formatted as the so called 'storage_identifier' or
if its formatted as a url. I can only tell by looking at the code that you
expect 'storage_identifier' formatting here.

We could use GURL as a representation of origin, and produce
'storage_identifier' formatted strings as needed.

http://codereview.chromium.org/3028033/diff/1/3
File chrome/browser/filesystem_host_context.h (right):

http://codereview.chromium.org/3028033/diff/1/3#newcode19
chrome/browser/filesystem_host_context.h:19: explicit
FileSystemHostContext(Profile* profile);
Since Profile is a chrome class, maybe construct this with a FilePath and bool
incognito param?

[michaeln, 2010-07-29 22:50:05]

http://codereview.chromium.org/3028033/diff/1/4
File chrome/browser/filesystem_host_context_unittest.cc (right):

http://codereview.chromium.org/3028033/diff/1/4#newcode86
chrome/browser/filesystem_host_context_unittest.cc:86:
kRootPathTestCases[i].persistent);
nit: indent two more spaces

http://codereview.chromium.org/3028033/diff/1/4#newcode102
chrome/browser/filesystem_host_context_unittest.cc:102:
kCheckValidPathTestCases[i].origin_id, true);
nit: indent two more spaces

[kinuko, 2010-07-30 16:29:52]

Thanks for reviewing;

On 2010/07/29 22:48:40, michaeln wrote:
> > I wondered if I should make a subdir, but for now just put under
> chrome/browser
> > as we aren't going to have many files for the API.
> 
> I wonder if we should make a 'src/webkit/file_system' directory and
> have this class live there? If we want to build it for testing in
> DRT/test_shell, I think that's where this would go.

Per our offline discussion, I moved them to browser/filesystem.

http://codereview.chromium.org/3028033/diff/1/2
File chrome/browser/filesystem_host_context.cc (right):

http://codereview.chromium.org/3028033/diff/1/2#newcode20
chrome/browser/filesystem_host_context.cc:20: const char
FileSystemHostContext::kTemporaryName[] = "Temporary";
On 2010/07/29 22:48:40, michaeln wrote:
> Unfortunate to have two forms of "Persistent" constants here. If all we had
was
> the ascii char form, could we generate the FilePaths as needed? FilePath has
an
> AppendASCII() method that can be useful.

Done.

http://codereview.chromium.org/3028033/diff/1/2#newcode30
chrome/browser/filesystem_host_context.cc:30: return
base_path_.Append(origin_id).Append(kPersistentDirectory);
On 2010/07/29 22:48:40, michaeln wrote:
> Having FilePath::StringType here and std::string origin_id in
GetFileSystemName
> seems odd, why the differing types?
> 
> Honestly, I'm always hesitant when looking at 'origin' parameters in string
> form. It's never clear if its formatted as the so called 'storage_identifier'
or
> if its formatted as a url. I can only tell by looking at the code that you
> expect 'storage_identifier' formatting here.
> 
> We could use GURL as a representation of origin, and produce
> 'storage_identifier' formatted strings as needed.

I changed the code to use GURL and added a method that generates an identifier
string from the given URL.  (I used WebKit::WebSecurityOrigin class to do so but
not sure if it's the right solution...)

http://codereview.chromium.org/3028033/diff/1/3
File chrome/browser/filesystem_host_context.h (right):

http://codereview.chromium.org/3028033/diff/1/3#newcode19
chrome/browser/filesystem_host_context.h:19: explicit
FileSystemHostContext(Profile* profile);
On 2010/07/29 22:48:40, michaeln wrote:
> Since Profile is a chrome class, maybe construct this with a FilePath and bool
> incognito param?

Done.

http://codereview.chromium.org/3028033/diff/1/4
File chrome/browser/filesystem_host_context_unittest.cc (right):

http://codereview.chromium.org/3028033/diff/1/4#newcode79
chrome/browser/filesystem_host_context_unittest.cc:79: for (size_t i = 0; i <
ARRAYSIZE_UNSAFE(kRootPathTestCases); ++i) {
On 2010/07/29 16:53:04, Paweł Hajdan Jr. wrote:
> Could you use SCOPED_TRACE to display the test case information? It'd be
useful
> to have the iteration number, and data from the test case.

Done.

http://codereview.chromium.org/3028033/diff/1/4#newcode86
chrome/browser/filesystem_host_context_unittest.cc:86:
kRootPathTestCases[i].persistent);
On 2010/07/29 22:50:06, michaeln wrote:
> nit: indent two more spaces

Done.

http://codereview.chromium.org/3028033/diff/1/4#newcode96
chrome/browser/filesystem_host_context_unittest.cc:96: for (size_t i = 0; i <
ARRAYSIZE_UNSAFE(kCheckValidPathTestCases); ++i) {
On 2010/07/29 16:53:04, Paweł Hajdan Jr. wrote:
> Same here.

Done.

http://codereview.chromium.org/3028033/diff/1/4#newcode102
chrome/browser/filesystem_host_context_unittest.cc:102:
kCheckValidPathTestCases[i].origin_id, true);
On 2010/07/29 22:50:06, michaeln wrote:
> nit: indent two more spaces

Done.

[kinuko, 2010-07-31 02:10:47]

Updated CheckValidFileSystemPath to handle paths with parent references ('..').

[Paweł Hajdan Jr., 2010-08-02 17:27:03]

+mark to maybe suggest something better than #define PS for path separators.

Code I commented in the drive-by LGTM.

[michaeln, 2010-08-11 18:43:12]

I think the addition of the /filesystem directory is basically good, but i
wonder if the name is too generic? Depending on the name we decide for the new
WebKitAPI class, maybe we can less genericize this directory name too.

Some ideas as mentioned here...
https://bugs.webkit.org/show_bug.cgi?id=43151#c10
/protected_filesystem
/sandboxed_filesystem
/virtual_filesystem

Personally I think 'protected file system' may be a nice fit, but it all starts
with the WebKitAPI. Lets see what we do there.

http://codereview.chromium.org/3028033/diff/34001/35001
File chrome/browser/filesystem/filesystem_host_context.cc (right):

http://codereview.chromium.org/3028033/diff/34001/35001#newcode43
chrome/browser/filesystem/filesystem_host_context.cc:43: bool
FileSystemHostContext::CheckValidFileSystemPath(
I'd like to understand what the 'path' values presented to the webkitAPI layer
look like more. As mentioned in the webkit api patch, do we want to use full os
file system paths in the webkitAPI or do we use take measure to not expose the
os username to renderer processes?

https://bugs.webkit.org/show_bug.cgi?id=43151#c10

From a security/privacy point of view, i think not requiring the use of full os
file system patsh in the webkitAPI would be better.

http://codereview.chromium.org/3028033/diff/34001/35002
File chrome/browser/filesystem/filesystem_host_context.h (right):

http://codereview.chromium.org/3028033/diff/34001/35002#newcode18
chrome/browser/filesystem/filesystem_host_context.h:18: explicit
FileSystemHostContext(const FilePath& data_path, bool is_incognito);
no need for explicit here

http://codereview.chromium.org/3028033/diff/34001/35002#newcode19
chrome/browser/filesystem/filesystem_host_context.h:19: ~FileSystemHostContext()
{ }
maybe remove the empty dtor?

http://codereview.chromium.org/3028033/diff/34001/35002#newcode29
chrome/browser/filesystem/filesystem_host_context.h:29: // and |persistent|.
How is GetFileSystemName() used? I see, its used to populate the .name attribute
of the fileSystem object accessible via script.

http://codereview.chromium.org/3028033/diff/34001/35006
File chrome/browser/profile_impl.cc (right):

http://codereview.chromium.org/3028033/diff/34001/35006#newcode941
chrome/browser/profile_impl.cc:941: DCHECK(filesystem_host_context_.get());
can probably do without this dcheck

[Mark Mentovai, 2010-08-11 18:48:44]

http://codereview.chromium.org/3028033/diff/34001/35003
File chrome/browser/filesystem/filesystem_host_context_unittest.cc (right):

http://codereview.chromium.org/3028033/diff/34001/35003#newcode32
chrome/browser/filesystem/filesystem_host_context_unittest.cc:32: "FileSystem"
PS "http_host_1" PS "Temporary" },
You can use FilePath::kSeparators[0], but not inside a character literal
constant.

I think that PS is fine as used here - test code, isolated to a .cc.

[Paweł Hajdan Jr., 2010-08-11 19:41:14]

Code I commented in the drive-by LGTM.

[kinuko, 2010-08-26 17:06:15]

Reviving this patch... can you again take a look at this one?
- removed path canonicalization part (the API just pass absolute path and this
simply reject any paths that have "../")
- changed boolean persistent to enum type (per spec change)
- added the the calling part in dispatcher_host

[Eric U., 2010-08-26 22:13:47]

http://codereview.chromium.org/3028033/diff/49001/50001
File chrome/browser/file_system/file_system_dispatcher_host.cc (right):

http://codereview.chromium.org/3028033/diff/49001/50001#newcode22
chrome/browser/file_system/file_system_dispatcher_host.cc:22:
FileSystemHostContext* file_system_host_context,
You're not using this parameter; looks like it's supposed to go into context_.

http://codereview.chromium.org/3028033/diff/49001/50003
File chrome/browser/file_system/file_system_host_context.cc (right):

http://codereview.chromium.org/3028033/diff/49001/50003#newcode32
chrome/browser/file_system/file_system_host_context.cc:32: return
origin_path.AppendASCII(kTemporaryName);
This isn't sufficient protection for incognito mode; it's going to mix its data
with non-incognito temporary storage, which will probably persist indefinitely
on systems without storage pressure.  It will take a bit of work to support it
properly.  Until we have time to do it, we should probably just reject FS access
in incognito mode.

http://codereview.chromium.org/3028033/diff/49001/50004
File chrome/browser/file_system/file_system_host_context.h (right):

http://codereview.chromium.org/3028033/diff/49001/50004#newcode24
chrome/browser/file_system/file_system_host_context.h:24: // and |persistent|.
Since we're using an enum for type now, the comment seems dated [throughout the
file].

[kinuko, 2010-08-27 00:04:14]

Thanks, updated.

http://codereview.chromium.org/3028033/diff/49001/50001
File chrome/browser/file_system/file_system_dispatcher_host.cc (right):

http://codereview.chromium.org/3028033/diff/49001/50001#newcode22
chrome/browser/file_system/file_system_dispatcher_host.cc:22:
FileSystemHostContext* file_system_host_context,
On 2010/08/26 22:13:47, Eric U. wrote:
> You're not using this parameter; looks like it's supposed to go into context_.

Ugh... good catch, gotten dropped while splitting patches.

Done.

http://codereview.chromium.org/3028033/diff/49001/50003
File chrome/browser/file_system/file_system_host_context.cc (right):

http://codereview.chromium.org/3028033/diff/49001/50003#newcode32
chrome/browser/file_system/file_system_host_context.cc:32: return
origin_path.AppendASCII(kTemporaryName);
On 2010/08/26 22:13:47, Eric U. wrote:
> This isn't sufficient protection for incognito mode; it's going to mix its
data
> with non-incognito temporary storage, which will probably persist indefinitely
> on systems without storage pressure.  It will take a bit of work to support it
> properly.  Until we have time to do it, we should probably just reject FS
access
> in incognito mode.

Right... added a TODO comment and changed the method to return true/false to
indicate if it has successfully assigned a file system path.

http://codereview.chromium.org/3028033/diff/49001/50004
File chrome/browser/file_system/file_system_host_context.h (right):

http://codereview.chromium.org/3028033/diff/49001/50004#newcode24
chrome/browser/file_system/file_system_host_context.h:24: // and |persistent|.
On 2010/08/26 22:13:47, Eric U. wrote:
> Since we're using an enum for type now, the comment seems dated [throughout
the
> file].

Done.

[Eric U., 2010-08-27 00:13:02]

LGTM

[michaeln, 2010-08-27 19:12:56]

LGTM!