Cleanup: Removing FileSystemPathManager

Cleanup: Removing FileSystemPathManager

- Deprecating FileSystemPathManager, which I believe no longer has a lot of reasons to exist
 * Changed GetFileSystemTypeString to a regular function in file_system_util
 * Added GetFileUtil(FileSystemType), GetMountPointProvider(FileSystemType), external_provider() to FileSystemContext
 * Moved all the tests in file_system_path_manager_unittest to file_system_mount_point_provider_unittest

- Also replaced two boolean values in FileSystemContext with a thin FileSystemOptions interface

BUG=none
TEST=no functional changes

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

[kinuko, 2011-12-20 15:05:27]

Sorry for a lot of churn, I want to get rid of FileSystemPathManager as (in my
belief) it now merely adds undesirable complexity to the fileapi code.  This
change looks big but each change is only a few lines.  Could you take a look at?
 Thanks so much.

[ericu, 2011-12-20 18:12:59]

Looks good overall.  Just a few questions.

http://codereview.chromium.org/9004019/diff/7002/chrome/browser/browsing_data...
File chrome/browser/browsing_data_file_system_helper_unittest.cc (right):

http://codereview.chromium.org/9004019/diff/7002/chrome/browser/browsing_data...
chrome/browser/browsing_data_file_system_helper_unittest.cc:9: #include
"base/file_util.h"
Why is this now needed?

http://codereview.chromium.org/9004019/diff/7002/webkit/fileapi/file_system_c...
File webkit/fileapi/file_system_context.h (right):

http://codereview.chromium.org/9004019/diff/7002/webkit/fileapi/file_system_c...
webkit/fileapi/file_system_context.h:43: // The ownership of |options| is
transferred from the caller.
It's a very small class.  Perhaps we should just pass it by value?

http://codereview.chromium.org/9004019/diff/7002/webkit/fileapi/file_system_c...
webkit/fileapi/file_system_context.h:47: quota::SpecialStoragePolicy*
special_storage_policy,
Why are these no longer scoped_refptrs?  Should they not have been all along?

http://codereview.chromium.org/9004019/diff/7002/webkit/fileapi/file_system_m...
File webkit/fileapi/file_system_mount_point_provider_unittest.cc (right):

http://codereview.chromium.org/9004019/diff/7002/webkit/fileapi/file_system_m...
webkit/fileapi/file_system_mount_point_provider_unittest.cc:204: // Mock
FileSystem options for testing incognito mode.
Maybe go ahead and put these in mock_file_system_options.h, just to have them
all in one place?  Also, these are all mock filesystem options; perhaps
MockFileSystemOptions should have a more descriptive name, to distinguish it
from these two?

[tzik, 2011-12-21 02:52:32]

http://codereview.chromium.org/9004019/diff/7004/webkit/fileapi/file_system_m...
File webkit/fileapi/file_system_mount_point_provider.h (right):

http://codereview.chromium.org/9004019/diff/7004/webkit/fileapi/file_system_m...
webkit/fileapi/file_system_mount_point_provider.h:11: #include "base/callback.h"
can be #include "base/callback_forward.h"?

http://codereview.chromium.org/9004019/diff/7004/webkit/fileapi/file_system_m...
File webkit/fileapi/file_system_mount_point_provider_unittest.cc (right):

http://codereview.chromium.org/9004019/diff/7004/webkit/fileapi/file_system_m...
webkit/fileapi/file_system_mount_point_provider_unittest.cc:284: return
FilePath::FromUTF8Unsafe(std::string(fileapi::kExternalDir));
maybe we can omit std::string

http://codereview.chromium.org/9004019/diff/7004/webkit/fileapi/file_system_m...
webkit/fileapi/file_system_mount_point_provider_unittest.cc:287: return
FilePath::FromUTF8Unsafe(std::string("/tmp"));
ditto

http://codereview.chromium.org/9004019/diff/7004/webkit/fileapi/sandbox_mount...
File webkit/fileapi/sandbox_mount_point_provider.cc (right):

http://codereview.chromium.org/9004019/diff/7004/webkit/fileapi/sandbox_mount...
webkit/fileapi/sandbox_mount_point_provider.cc:41: kOldFileSystemUniqueLength +
arraysize(kOldFileSystemUniqueNamePrefix) - 1;
size_t?

http://codereview.chromium.org/9004019/diff/7004/webkit/fileapi/sandbox_mount...
webkit/fileapi/sandbox_mount_point_provider.cc:60: '/', '\\',
FILE_PATH_LITERAL?

http://codereview.chromium.org/9004019/diff/7004/webkit/fileapi/sandbox_mount...
webkit/fileapi/sandbox_mount_point_provider.cc:434: std::string filename_lower =
StringToLowerASCII(filename.MaybeAsASCII());
MaybeAsASCII will return empty string if filename contains non-ASCII character.
This behaves differ to FilePathStringToASCII in previous implementation.
Does it cause any trouble?
That is, if filename contains both non-ASCII char and restricted char, it will
path the test accidentally.

[kinuko, 2011-12-21 13:00:50]

http://codereview.chromium.org/9004019/diff/7002/chrome/browser/browsing_data...
File chrome/browser/browsing_data_file_system_helper_unittest.cc (right):

http://codereview.chromium.org/9004019/diff/7002/chrome/browser/browsing_data...
chrome/browser/browsing_data_file_system_helper_unittest.cc:9: #include
"base/file_util.h"
On 2011/12/20 18:12:59, ericu wrote:
> Why is this now needed?

This used to be included in file_system_path_manager.h and this test code relies
on some of the file_util methods (e.g. file_util::DirectoryExists).

http://codereview.chromium.org/9004019/diff/7002/webkit/fileapi/file_system_c...
File webkit/fileapi/file_system_context.h (right):

http://codereview.chromium.org/9004019/diff/7002/webkit/fileapi/file_system_c...
webkit/fileapi/file_system_context.h:43: // The ownership of |options| is
transferred from the caller.
On 2011/12/20 18:12:59, ericu wrote:
> It's a very small class.  Perhaps we should just pass it by value?

The point is I used virtual methods for allowing browser code to implement them.
I could also turn this class into a simple struct but I wanted to avoid using
raw boolean values.

http://codereview.chromium.org/9004019/diff/7002/webkit/fileapi/file_system_c...
webkit/fileapi/file_system_context.h:47: quota::SpecialStoragePolicy*
special_storage_policy,
On 2011/12/20 18:12:59, ericu wrote:
> Why are these no longer scoped_refptrs?  Should they not have been all along?

They are all cast'ed to scoped_refptr in the initializers so they should be just
fine.  (There's no strong reason to turn them into raw pointers for now though.
Subtle benefits from this change are the code looks simpler and we can more
easily pass NULL pointers)

http://codereview.chromium.org/9004019/diff/7002/webkit/fileapi/file_system_m...
File webkit/fileapi/file_system_mount_point_provider_unittest.cc (right):

http://codereview.chromium.org/9004019/diff/7002/webkit/fileapi/file_system_m...
webkit/fileapi/file_system_mount_point_provider_unittest.cc:204: // Mock
FileSystem options for testing incognito mode.
On 2011/12/20 18:12:59, ericu wrote:
> Maybe go ahead and put these in mock_file_system_options.h, just to have them
> all in one place?  Also, these are all mock filesystem options; perhaps
> MockFileSystemOptions should have a more descriptive name, to distinguish it
> from these two?

Done.

http://codereview.chromium.org/9004019/diff/7004/webkit/fileapi/file_system_m...
File webkit/fileapi/file_system_mount_point_provider.h (right):

http://codereview.chromium.org/9004019/diff/7004/webkit/fileapi/file_system_m...
webkit/fileapi/file_system_mount_point_provider.h:11: #include "base/callback.h"
On 2011/12/21 02:52:33, tzik wrote:
> can be #include "base/callback_forward.h"?

Done.

http://codereview.chromium.org/9004019/diff/7004/webkit/fileapi/file_system_m...
File webkit/fileapi/file_system_mount_point_provider_unittest.cc (right):

http://codereview.chromium.org/9004019/diff/7004/webkit/fileapi/file_system_m...
webkit/fileapi/file_system_mount_point_provider_unittest.cc:284: return
FilePath::FromUTF8Unsafe(std::string(fileapi::kExternalDir));
On 2011/12/21 02:52:33, tzik wrote:
> maybe we can omit std::string

Done.

http://codereview.chromium.org/9004019/diff/7004/webkit/fileapi/file_system_m...
webkit/fileapi/file_system_mount_point_provider_unittest.cc:287: return
FilePath::FromUTF8Unsafe(std::string("/tmp"));
On 2011/12/21 02:52:33, tzik wrote:
> ditto

Done.

http://codereview.chromium.org/9004019/diff/7004/webkit/fileapi/sandbox_mount...
File webkit/fileapi/sandbox_mount_point_provider.cc (right):

http://codereview.chromium.org/9004019/diff/7004/webkit/fileapi/sandbox_mount...
webkit/fileapi/sandbox_mount_point_provider.cc:41: kOldFileSystemUniqueLength +
arraysize(kOldFileSystemUniqueNamePrefix) - 1;
On 2011/12/21 02:52:33, tzik wrote:
> size_t?

Done.

http://codereview.chromium.org/9004019/diff/7004/webkit/fileapi/sandbox_mount...
webkit/fileapi/sandbox_mount_point_provider.cc:60: '/', '\\',
On 2011/12/21 02:52:33, tzik wrote:
> FILE_PATH_LITERAL?

Done.

http://codereview.chromium.org/9004019/diff/7004/webkit/fileapi/sandbox_mount...
webkit/fileapi/sandbox_mount_point_provider.cc:434: std::string filename_lower =
StringToLowerASCII(filename.MaybeAsASCII());
On 2011/12/21 02:52:33, tzik wrote:
> MaybeAsASCII will return empty string if filename contains non-ASCII
character.
> This behaves differ to FilePathStringToASCII in previous implementation.
> Does it cause any trouble?
> That is, if filename contains both non-ASCII char and restricted char, it will
> path the test accidentally.

Good catch.  Changed the code to use FilePath::CharType.

[ericu, 2011-12-21 22:22:48]

http://codereview.chromium.org/9004019/diff/7002/webkit/fileapi/file_system_c...
File webkit/fileapi/file_system_context.h (right):

http://codereview.chromium.org/9004019/diff/7002/webkit/fileapi/file_system_c...
webkit/fileapi/file_system_context.h:43: // The ownership of |options| is
transferred from the caller.
On 2011/12/21 13:00:50, kinuko wrote:
> On 2011/12/20 18:12:59, ericu wrote:
> > It's a very small class.  Perhaps we should just pass it by value?
> 
> The point is I used virtual methods for allowing browser code to implement
them.
> I could also turn this class into a simple struct but I wanted to avoid using
> raw boolean values.

I meant that the class, with its virtual methods, could be passed by value to
avoid the mental overhead of lifetime management of a dynamically-allocated
object.  But if you really prefer it this way, I don't feel very strongly about
it.

http://codereview.chromium.org/9004019/diff/7002/webkit/fileapi/file_system_c...
webkit/fileapi/file_system_context.h:47: quota::SpecialStoragePolicy*
special_storage_policy,
On 2011/12/21 13:00:50, kinuko wrote:
> On 2011/12/20 18:12:59, ericu wrote:
> > Why are these no longer scoped_refptrs?  Should they not have been all
along?
> 
> They are all cast'ed to scoped_refptr in the initializers so they should be
just
> fine.  (There's no strong reason to turn them into raw pointers for now
though.
> Subtle benefits from this change are the code looks simpler and we can more
> easily pass NULL pointers)

Making the code look simpler than it is may cause confusion, and these values
won't often be NULL.  When they are, it's probably worth the explicit
scoped_refptr<base::MessageLoopProxy>() to make it clear.  I'd prefer not to
make this change.

[kinuko, 2011-12-22 05:28:39]

http://codereview.chromium.org/9004019/diff/7002/webkit/fileapi/file_system_c...
File webkit/fileapi/file_system_context.h (right):

http://codereview.chromium.org/9004019/diff/7002/webkit/fileapi/file_system_c...
webkit/fileapi/file_system_context.h:43: // The ownership of |options| is
transferred from the caller.
On 2011/12/21 22:22:48, ericu wrote:
> On 2011/12/21 13:00:50, kinuko wrote:
> > On 2011/12/20 18:12:59, ericu wrote:
> > > It's a very small class.  Perhaps we should just pass it by value?
> > 
> > The point is I used virtual methods for allowing browser code to implement
> them.
> > I could also turn this class into a simple struct but I wanted to avoid
using
> > raw boolean values.
> 
> I meant that the class, with its virtual methods, could be passed by value to
> avoid the mental overhead of lifetime management of a dynamically-allocated
> object.  But if you really prefer it this way, I don't feel very strongly
about
> it.

Sorry I might be missing something but I cannot imagine what type we should
receive for the passed value.  (The BrowserFileSystemOption is defined in
content but FileSystemContext doesn't know the subclass(es) so that it cannot
tell how much storage it needs for the value.)

http://codereview.chromium.org/9004019/diff/7002/webkit/fileapi/file_system_c...
webkit/fileapi/file_system_context.h:47: quota::SpecialStoragePolicy*
special_storage_policy,
On 2011/12/21 22:22:48, ericu wrote:
> On 2011/12/21 13:00:50, kinuko wrote:
> > On 2011/12/20 18:12:59, ericu wrote:
> > > Why are these no longer scoped_refptrs?  Should they not have been all
> along?
> > 
> > They are all cast'ed to scoped_refptr in the initializers so they should be
> just
> > fine.  (There's no strong reason to turn them into raw pointers for now
> though.
> > Subtle benefits from this change are the code looks simpler and we can more
> > easily pass NULL pointers)
> 
> Making the code look simpler than it is may cause confusion, and these values
> won't often be NULL.  When they are, it's probably worth the explicit
> scoped_refptr<base::MessageLoopProxy>() to make it clear.  I'd prefer not to
> make this change.

Another subtle benefit is we can save refcount churn at arg passing (though it's
not the only way to do so).  Recently I saw similar changes elsewhere so I
thought this change'd be preferable.  For example SpecialStoragePolicy is almost
always passed as a raw pointer)  I can revert this (personally I have no strong
opinion on this) but I feel changing this to pointer might make the code more
consistent with other parts.

[kinuko, 2011-12-27 17:33:26]

http://codereview.chromium.org/9004019/diff/7002/webkit/fileapi/file_system_c...
File webkit/fileapi/file_system_context.h (right):

http://codereview.chromium.org/9004019/diff/7002/webkit/fileapi/file_system_c...
webkit/fileapi/file_system_context.h:43: // The ownership of |options| is
transferred from the caller.
On 2011/12/22 05:28:39, kinuko wrote:
> On 2011/12/21 22:22:48, ericu wrote:
> > On 2011/12/21 13:00:50, kinuko wrote:
> > > On 2011/12/20 18:12:59, ericu wrote:
> > > > It's a very small class.  Perhaps we should just pass it by value?
> > > 
> > > The point is I used virtual methods for allowing browser code to implement
> > them.
> > > I could also turn this class into a simple struct but I wanted to avoid
> using
> > > raw boolean values.
> > 
> > I meant that the class, with its virtual methods, could be passed by value
to
> > avoid the mental overhead of lifetime management of a dynamically-allocated
> > object.  But if you really prefer it this way, I don't feel very strongly
> about
> > it.
> 
> Sorry I might be missing something but I cannot imagine what type we should
> receive for the passed value.  (The BrowserFileSystemOption is defined in
> content but FileSystemContext doesn't know the subclass(es) so that it cannot
> tell how much storage it needs for the value.)
> 

Ok I stopped using virtual methods and converted them to use simple pass-by
values.

http://codereview.chromium.org/9004019/diff/7002/webkit/fileapi/file_system_c...
webkit/fileapi/file_system_context.h:47: quota::SpecialStoragePolicy*
special_storage_policy,
On 2011/12/22 05:28:39, kinuko wrote:
> On 2011/12/21 22:22:48, ericu wrote:
> > On 2011/12/21 13:00:50, kinuko wrote:
> > > On 2011/12/20 18:12:59, ericu wrote:
> > > > Why are these no longer scoped_refptrs?  Should they not have been all
> > along?
> > > 
> > > They are all cast'ed to scoped_refptr in the initializers so they should
be
> > just
> > > fine.  (There's no strong reason to turn them into raw pointers for now
> > though.
> > > Subtle benefits from this change are the code looks simpler and we can
more
> > > easily pass NULL pointers)
> > 
> > Making the code look simpler than it is may cause confusion, and these
values
> > won't often be NULL.  When they are, it's probably worth the explicit
> > scoped_refptr<base::MessageLoopProxy>() to make it clear.  I'd prefer not to
> > make this change.
> 
> Another subtle benefit is we can save refcount churn at arg passing (though
it's
> not the only way to do so).  Recently I saw similar changes elsewhere so I
> thought this change'd be preferable.  For example SpecialStoragePolicy is
almost
> always passed as a raw pointer)  I can revert this (personally I have no
strong
> opinion on this) but I feel changing this to pointer might make the code more
> consistent with other parts.

I reverted these changes for now.  We can change them later if we really want
to.

[ericu, 2012-01-03 22:34:24]

LGTM

http://codereview.chromium.org/9004019/diff/23007/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_context.cc (right):

http://codereview.chromium.org/9004019/diff/23007/webkit/fileapi/file_system_...
webkit/fileapi/file_system_context.cc:105: if (!mount_point_provider)
You removed a DCHECK(mount_point_provider).  Should it be able to be NULL here? 
If not, please bring it back.

http://codereview.chromium.org/9004019/diff/23007/webkit/support/webkit_suppo...
File webkit/support/webkit_support.gypi (right):

http://codereview.chromium.org/9004019/diff/23007/webkit/support/webkit_suppo...
webkit/support/webkit_support.gypi:63:
'<(DEPTH)/webkit/fileapi/mock_file_system_options.h',
Do these need to be here, given that they're in webkit_support_common below?

[kinuko, 2012-01-05 09:40:18]

Thanks, will be submitting.

http://codereview.chromium.org/9004019/diff/23007/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_context.cc (right):

http://codereview.chromium.org/9004019/diff/23007/webkit/fileapi/file_system_...
webkit/fileapi/file_system_context.cc:105: if (!mount_point_provider)
On 2012/01/03 22:34:24, ericu wrote:
> You removed a DCHECK(mount_point_provider).  Should it be able to be NULL
here? 
> If not, please bring it back.

I found this could be NULL in a pepper testing code.

http://codereview.chromium.org/9004019/diff/23007/webkit/support/webkit_suppo...
File webkit/support/webkit_support.gypi (right):

http://codereview.chromium.org/9004019/diff/23007/webkit/support/webkit_suppo...
webkit/support/webkit_support.gypi:63:
'<(DEPTH)/webkit/fileapi/mock_file_system_options.h',
On 2012/01/03 22:34:24, ericu wrote:
> Do these need to be here, given that they're in webkit_support_common below?

You're right, we don't need them here.