Change {Obfuscated|Local}FileSystemFileUtil non-Singleton to take an underlying *FileUtil.

Change {Obfuscated|Local}FileSystemFileUtil non-Singleton to take an underlying *FileUtil.

This change :
* reduces dependency between FileUtils, and
* enables more flexible stacking of FileSystemFileUtil, such as {Obfuscated|Local}FSFU without any quota operation.

BUG=none
TEST=*FileUtilTest.*,FileSystemPathManagerTest.*,SandboxMountPointProvider.*,FileSystem*OperationTest.*,FileWriterDelegateTest.*,FileSystemQuotaTest.*

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

[Dai Mikurube (NOT FULLTIME), 2011-06-17 06:17:59]

eric, kinuko,

I'm trying this change to change some *FileUtils to take an underlying
*FileUtils.  For example, LocalFileSystemFileUtil can choose QuotaFileUtil or
FileSystemFileUtil as its underlying FileUtil.

The benefits of this change are :
* they increase quality of unit tests.  (Less tests depend on unnecessary
QuotaFileUtil, for example.)
* they can disable unnecessary QuotaFileUtil, for example, maybe for a part of
type=external file systems.


zel,

Could you take a look at the change in extension_file_browser_private_api.cc?

[ericu, 2011-06-17 20:32:07]

I like the general idea, but have a few tweaks to the implementation to suggest.

http://codereview.chromium.org/7174002/diff/8001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_file_browser_private_api.cc (right):

http://codereview.chromium.org/7174002/diff/8001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_file_browser_private_api.cc:657: if
(!local_file_util_.get())
Why use lazy initialization?  Can we just set this up at construction time?

Better yet, this should just use the same one as I proposed that the mount point
provider would hold.  We really only need one for the whole system, so creating
and deleting them on every operation is unnecessary.

http://codereview.chromium.org/7174002/diff/8001/webkit/fileapi/file_system_p...
File webkit/fileapi/file_system_path_manager.h (right):

http://codereview.chromium.org/7174002/diff/8001/webkit/fileapi/file_system_p...
webkit/fileapi/file_system_path_manager.h:99:
scoped_ptr<LocalFileSystemFileUtil> local_file_util_;
This seems odd.  Why should a FileSystemPathManager own a FSFU subclass
directly?  Shouldn't it be the external_provider_ that takes care of this, just
as the SandboxMountPointProvider supplies the one for the sandbox?

http://codereview.chromium.org/7174002/diff/8001/webkit/fileapi/file_system_q...
File webkit/fileapi/file_system_quota_unittest.cc (right):

http://codereview.chromium.org/7174002/diff/8001/webkit/fileapi/file_system_q...
webkit/fileapi/file_system_quota_unittest.cc:203: local_file_util_.reset();
Clearing member scoped_ptrs shouldn't be necessary in destructors unless there's
some sort of a timing requirement based on variable declaration order.  It's
nicely symmetric to have the creation in SetUp and the deletion in TearDown, but
perhaps instead you could create it in the test constructor and not bother to
destroy it manually?

[same in other tests]

http://codereview.chromium.org/7174002/diff/8001/webkit/fileapi/local_file_sy...
File webkit/fileapi/local_file_system_file_util.h (right):

http://codereview.chromium.org/7174002/diff/8001/webkit/fileapi/local_file_sy...
webkit/fileapi/local_file_system_file_util.h:16: #include
"base/memory/singleton.h"
Singleton include is no longer needed.

http://codereview.chromium.org/7174002/diff/8001/webkit/fileapi/local_file_sy...
webkit/fileapi/local_file_system_file_util.h:121: friend struct
DefaultSingletonTraits<LocalFileSystemFileUtil>;
friend declaration is no longer needed.

http://codereview.chromium.org/7174002/diff/8001/webkit/fileapi/obfuscated_fi...
File webkit/fileapi/obfuscated_file_system_file_util.h (right):

http://codereview.chromium.org/7174002/diff/8001/webkit/fileapi/obfuscated_fi...
webkit/fileapi/obfuscated_file_system_file_util.h:185: static
ObfuscatedFileSystemFileUtil* GetInstance();
I think this got in here by accident.

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

http://codereview.chromium.org/7174002/diff/8001/webkit/fileapi/obfuscated_fi...
webkit/fileapi/obfuscated_file_system_file_util_unittest.cc:147:
context->set_allowed_bytes_growth(1024 * 1024);
Does this mean we can get rid of the set_allowed_bytes_growth here and perhaps
elsewhere?

[Dai Mikurube (NOT FULLTIME), 2011-06-23 08:13:32]

Thanks, Eric.

Updated the patch.  Could you take a glance at this?

http://codereview.chromium.org/7174002/diff/8001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_file_browser_private_api.cc (right):

http://codereview.chromium.org/7174002/diff/8001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_file_browser_private_api.cc:657: if
(!local_file_util_.get())
On 2011/06/17 20:32:07, ericu wrote:
> Why use lazy initialization?  Can we just set this up at construction time?
> 
> Better yet, this should just use the same one as I proposed that the mount
point
> provider would hold.  We really only need one for the whole system, so
creating
> and deleting them on every operation is unnecessary.

Agreed.  Changed to use external_provider's GetFileSystemFileUtil.

http://codereview.chromium.org/7174002/diff/8001/webkit/fileapi/file_system_p...
File webkit/fileapi/file_system_path_manager.h (right):

http://codereview.chromium.org/7174002/diff/8001/webkit/fileapi/file_system_p...
webkit/fileapi/file_system_path_manager.h:99:
scoped_ptr<LocalFileSystemFileUtil> local_file_util_;
On 2011/06/17 20:32:07, ericu wrote:
> This seems odd.  Why should a FileSystemPathManager own a FSFU subclass
> directly?  Shouldn't it be the external_provider_ that takes care of this,
just
> as the SandboxMountPointProvider supplies the one for the sandbox?

Agreed.  It's better for the FSFU instance in MountPointProvider.  Moved it to
CrosMountPointProvider.

But actually, GetFileSystemFileUtil() in FileSystemPathManager and
MountPointProvider looks like a little strange to me.  My thought is "Which
-FileUtil to be used" doesn't depend only on the GURL path.  What do you think?
(I think it'll be in another issue even if we decide to change it.)

http://codereview.chromium.org/7174002/diff/8001/webkit/fileapi/file_system_q...
File webkit/fileapi/file_system_quota_unittest.cc (right):

http://codereview.chromium.org/7174002/diff/8001/webkit/fileapi/file_system_q...
webkit/fileapi/file_system_quota_unittest.cc:203: local_file_util_.reset();
On 2011/06/17 20:32:07, ericu wrote:
> Clearing member scoped_ptrs shouldn't be necessary in destructors unless
there's
> some sort of a timing requirement based on variable declaration order.  It's
> nicely symmetric to have the creation in SetUp and the deletion in TearDown,
but
> perhaps instead you could create it in the test constructor and not bother to
> destroy it manually?
> 
> [same in other tests]

Done.  local_file_util_ can be instanciated before SetUp().

http://codereview.chromium.org/7174002/diff/8001/webkit/fileapi/local_file_sy...
File webkit/fileapi/local_file_system_file_util.h (right):

http://codereview.chromium.org/7174002/diff/8001/webkit/fileapi/local_file_sy...
webkit/fileapi/local_file_system_file_util.h:16: #include
"base/memory/singleton.h"
On 2011/06/17 20:32:07, ericu wrote:
> Singleton include is no longer needed.

Done.

http://codereview.chromium.org/7174002/diff/8001/webkit/fileapi/local_file_sy...
webkit/fileapi/local_file_system_file_util.h:121: friend struct
DefaultSingletonTraits<LocalFileSystemFileUtil>;
On 2011/06/17 20:32:07, ericu wrote:
> friend declaration is no longer needed.

Done.

http://codereview.chromium.org/7174002/diff/8001/webkit/fileapi/obfuscated_fi...
File webkit/fileapi/obfuscated_file_system_file_util.h (right):

http://codereview.chromium.org/7174002/diff/8001/webkit/fileapi/obfuscated_fi...
webkit/fileapi/obfuscated_file_system_file_util.h:185: static
ObfuscatedFileSystemFileUtil* GetInstance();
On 2011/06/17 20:32:07, ericu wrote:
> I think this got in here by accident.

Oh, it's not required now.  Removed.

This declaration was required during development.  I kept it as private to catch
all calls of ObfuscatedFileSystemFileUtil::GetInstance().  Without it,
ObfuscatedFSFU::GetInstance() results in calling
FileSystemFileUtil::GetInstance().

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

http://codereview.chromium.org/7174002/diff/8001/webkit/fileapi/obfuscated_fi...
webkit/fileapi/obfuscated_file_system_file_util_unittest.cc:147:
context->set_allowed_bytes_growth(1024 * 1024);
On 2011/06/17 20:32:07, ericu wrote:
> Does this mean we can get rid of the set_allowed_bytes_growth here and perhaps
> elsewhere?

Done.

[kinuko, 2011-06-28 09:12:26]

I might wonder if we really need one instance per operation for this wiring? 
The number of possible/desired wiring sets would be probably a few.  Is it
possible to keep such information somewhere outside the FSFU?  (Haven't had a
concrete design idea yet, but wondering it might be worth to consider)

Also if we start cleaning up these FSFU stacks we might be able to consider
Darin's refactoring idea as well?


On 2011/06/23 08:13:32, Dai Mikurube wrote:
> Thanks, Eric.
> 
> Updated the patch.  Could you take a glance at this?
> 
>
http://codereview.chromium.org/7174002/diff/8001/chrome/browser/extensions/ex...
> File chrome/browser/extensions/extension_file_browser_private_api.cc (right):
> 
>
http://codereview.chromium.org/7174002/diff/8001/chrome/browser/extensions/ex...
> chrome/browser/extensions/extension_file_browser_private_api.cc:657: if
> (!local_file_util_.get())
> On 2011/06/17 20:32:07, ericu wrote:
> > Why use lazy initialization?  Can we just set this up at construction time?
> > 
> > Better yet, this should just use the same one as I proposed that the mount
> point
> > provider would hold.  We really only need one for the whole system, so
> creating
> > and deleting them on every operation is unnecessary.
> 
> Agreed.  Changed to use external_provider's GetFileSystemFileUtil.
> 
>
http://codereview.chromium.org/7174002/diff/8001/webkit/fileapi/file_system_p...
> File webkit/fileapi/file_system_path_manager.h (right):
> 
>
http://codereview.chromium.org/7174002/diff/8001/webkit/fileapi/file_system_p...
> webkit/fileapi/file_system_path_manager.h:99:
> scoped_ptr<LocalFileSystemFileUtil> local_file_util_;
> On 2011/06/17 20:32:07, ericu wrote:
> > This seems odd.  Why should a FileSystemPathManager own a FSFU subclass
> > directly?  Shouldn't it be the external_provider_ that takes care of this,
> just
> > as the SandboxMountPointProvider supplies the one for the sandbox?
> 
> Agreed.  It's better for the FSFU instance in MountPointProvider.  Moved it to
> CrosMountPointProvider.
> 
> But actually, GetFileSystemFileUtil() in FileSystemPathManager and
> MountPointProvider looks like a little strange to me.  My thought is "Which
> -FileUtil to be used" doesn't depend only on the GURL path.  What do you
think?
> (I think it'll be in another issue even if we decide to change it.)
> 
>
http://codereview.chromium.org/7174002/diff/8001/webkit/fileapi/file_system_q...
> File webkit/fileapi/file_system_quota_unittest.cc (right):
> 
>
http://codereview.chromium.org/7174002/diff/8001/webkit/fileapi/file_system_q...
> webkit/fileapi/file_system_quota_unittest.cc:203: local_file_util_.reset();
> On 2011/06/17 20:32:07, ericu wrote:
> > Clearing member scoped_ptrs shouldn't be necessary in destructors unless
> there's
> > some sort of a timing requirement based on variable declaration order.  It's
> > nicely symmetric to have the creation in SetUp and the deletion in TearDown,
> but
> > perhaps instead you could create it in the test constructor and not bother
to
> > destroy it manually?
> > 
> > [same in other tests]
> 
> Done.  local_file_util_ can be instanciated before SetUp().
> 
>
http://codereview.chromium.org/7174002/diff/8001/webkit/fileapi/local_file_sy...
> File webkit/fileapi/local_file_system_file_util.h (right):
> 
>
http://codereview.chromium.org/7174002/diff/8001/webkit/fileapi/local_file_sy...
> webkit/fileapi/local_file_system_file_util.h:16: #include
> "base/memory/singleton.h"
> On 2011/06/17 20:32:07, ericu wrote:
> > Singleton include is no longer needed.
> 
> Done.
> 
>
http://codereview.chromium.org/7174002/diff/8001/webkit/fileapi/local_file_sy...
> webkit/fileapi/local_file_system_file_util.h:121: friend struct
> DefaultSingletonTraits<LocalFileSystemFileUtil>;
> On 2011/06/17 20:32:07, ericu wrote:
> > friend declaration is no longer needed.
> 
> Done.
> 
>
http://codereview.chromium.org/7174002/diff/8001/webkit/fileapi/obfuscated_fi...
> File webkit/fileapi/obfuscated_file_system_file_util.h (right):
> 
>
http://codereview.chromium.org/7174002/diff/8001/webkit/fileapi/obfuscated_fi...
> webkit/fileapi/obfuscated_file_system_file_util.h:185: static
> ObfuscatedFileSystemFileUtil* GetInstance();
> On 2011/06/17 20:32:07, ericu wrote:
> > I think this got in here by accident.
> 
> Oh, it's not required now.  Removed.
> 
> This declaration was required during development.  I kept it as private to
catch
> all calls of ObfuscatedFileSystemFileUtil::GetInstance().  Without it,
> ObfuscatedFSFU::GetInstance() results in calling
> FileSystemFileUtil::GetInstance().
> 
>
http://codereview.chromium.org/7174002/diff/8001/webkit/fileapi/obfuscated_fi...
> File webkit/fileapi/obfuscated_file_system_file_util_unittest.cc (right):
> 
>
http://codereview.chromium.org/7174002/diff/8001/webkit/fileapi/obfuscated_fi...
> webkit/fileapi/obfuscated_file_system_file_util_unittest.cc:147:
> context->set_allowed_bytes_growth(1024 * 1024);
> On 2011/06/17 20:32:07, ericu wrote:
> > Does this mean we can get rid of the set_allowed_bytes_growth here and
perhaps
> > elsewhere?
> 
> Done.

[kinuko, 2011-07-01 07:48:02]

(Please ignore my previous comment, seems like after some iterations this patch
no longer creates FSFU per operation.)

http://codereview.chromium.org/7174002/diff/26001/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_path_manager.cc (right):

http://codereview.chromium.org/7174002/diff/26001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_path_manager.cc:171: return
external_provider_->GetFileSystemFileUtil();
probably we should check external_provider_.get() is non-null here?

http://codereview.chromium.org/7174002/diff/26001/webkit/fileapi/local_file_s...
File webkit/fileapi/local_file_system_file_util.h (right):

http://codereview.chromium.org/7174002/diff/26001/webkit/fileapi/local_file_s...
webkit/fileapi/local_file_system_file_util.h:38: explicit
LocalFileSystemFileUtil(FileSystemFileUtil* underlying_file_util);
I'm a bit concerned about this asymmetry-- if I read the code correctly some of
FSFUs are singletons but some are not right?  It's not very obvious whether a
FSFU pointer owner should call delete or not.  Now that we only create FSFU
instances per mount point, maybe we can turn all of the FSFU non-singleton?

[Dai Mikurube (NOT FULLTIME), 2011-07-04 06:14:52]

Thank you for the comments. :)

http://codereview.chromium.org/7174002/diff/26001/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_path_manager.cc (right):

http://codereview.chromium.org/7174002/diff/26001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_path_manager.cc:171: return
external_provider_->GetFileSystemFileUtil();
On 2011/07/01 07:48:02, kinuko wrote:
> probably we should check external_provider_.get() is non-null here?

Done.

http://codereview.chromium.org/7174002/diff/26001/webkit/fileapi/local_file_s...
File webkit/fileapi/local_file_system_file_util.h (right):

http://codereview.chromium.org/7174002/diff/26001/webkit/fileapi/local_file_s...
webkit/fileapi/local_file_system_file_util.h:38: explicit
LocalFileSystemFileUtil(FileSystemFileUtil* underlying_file_util);
On 2011/07/01 07:48:02, kinuko wrote:
> I'm a bit concerned about this asymmetry-- if I read the code correctly some
of
> FSFUs are singletons but some are not right?  It's not very obvious whether a
> FSFU pointer owner should call delete or not.  Now that we only create FSFU
> instances per mount point, maybe we can turn all of the FSFU non-singleton?

Yeah, I have the same concern.  ObfuscatedFSFU, however, has been already not
singleton -- asymmetric.

I'm planning to make the other FSFUs non-singleton in another change since they
might effect larger part.

[kinuko, 2011-07-05 03:13:02]

Sorry for my late reviews.  I think this lgtm.

http://codereview.chromium.org/7174002/diff/26001/webkit/fileapi/local_file_s...
File webkit/fileapi/local_file_system_file_util.h (right):

http://codereview.chromium.org/7174002/diff/26001/webkit/fileapi/local_file_s...
webkit/fileapi/local_file_system_file_util.h:38: explicit
LocalFileSystemFileUtil(FileSystemFileUtil* underlying_file_util);
On 2011/07/04 06:14:53, Dai Mikurube wrote:
> On 2011/07/01 07:48:02, kinuko wrote:
> > I'm a bit concerned about this asymmetry-- if I read the code correctly some
> of
> > FSFUs are singletons but some are not right?  It's not very obvious whether
a
> > FSFU pointer owner should call delete or not.  Now that we only create FSFU
> > instances per mount point, maybe we can turn all of the FSFU non-singleton?
> 
> Yeah, I have the same concern.  ObfuscatedFSFU, however, has been already not
> singleton -- asymmetric.

Ok.  Let me make sure, after this change only the 'top-level' FSFUs, which are
directly created by and owned by MountProviders will be non-singleton. Correct?

Could you add a comment to this class about who creates and owns an instance of
this class?
Also let's explicitly note that the |underlying_file_util| is *not* owned by
this instance and will need to be a singleton or to be deleted someone else. 
(Same for the constructor of obfuscatedFSFU's parameter)

[Dai Mikurube (NOT FULLTIME), 2011-07-05 05:30:11]

Thanks.  I'll be committing this change when try bots get happy.

http://codereview.chromium.org/7174002/diff/26001/webkit/fileapi/local_file_s...
File webkit/fileapi/local_file_system_file_util.h (right):

http://codereview.chromium.org/7174002/diff/26001/webkit/fileapi/local_file_s...
webkit/fileapi/local_file_system_file_util.h:38: explicit
LocalFileSystemFileUtil(FileSystemFileUtil* underlying_file_util);
On 2011/07/05 03:13:02, kinuko wrote:
> On 2011/07/04 06:14:53, Dai Mikurube wrote:
> > On 2011/07/01 07:48:02, kinuko wrote:
> > > I'm a bit concerned about this asymmetry-- if I read the code correctly
some
> > of
> > > FSFUs are singletons but some are not right?  It's not very obvious
whether
> a
> > > FSFU pointer owner should call delete or not.  Now that we only create
FSFU
> > > instances per mount point, maybe we can turn all of the FSFU
non-singleton?
> > 
> > Yeah, I have the same concern.  ObfuscatedFSFU, however, has been already
not
> > singleton -- asymmetric.
> 
> Ok.  Let me make sure, after this change only the 'top-level' FSFUs, which are
> directly created by and owned by MountProviders will be non-singleton.
Correct?
> 
> Could you add a comment to this class about who creates and owns an instance
of
> this class?
> Also let's explicitly note that the |underlying_file_util| is *not* owned by
> this instance and will need to be a singleton or to be deleted someone else. 
> (Same for the constructor of obfuscatedFSFU's parameter)

Ok, added comments on {Local|Obfuscated}FileSystemFileUtil.  Could you take a
look at this?

[kinuko, 2011-07-05 06:35:23]

lgtm with a few more nits.

http://codereview.chromium.org/7174002/diff/34001/webkit/fileapi/local_file_s...
File webkit/fileapi/local_file_system_file_util.h (right):

http://codereview.chromium.org/7174002/diff/34001/webkit/fileapi/local_file_s...
webkit/fileapi/local_file_system_file_util.h:15: #include
"base/memory/ref_counted.h"
nit: not necessary?

http://codereview.chromium.org/7174002/diff/34001/webkit/fileapi/local_file_s...
webkit/fileapi/local_file_system_file_util.h:17: #include
"base/tracked_objects.h"
nit: not necessary?

http://codereview.chromium.org/7174002/diff/34001/webkit/fileapi/local_file_s...
webkit/fileapi/local_file_system_file_util.h:36: // Now, this class is created
and owned only by *MountPointProvider and tests.
wording nit: 'Now, ' and 'only' is not needed I think?  ('and tests.' at the end
might be dropped as well)
wording nit: this class is -> an instance of this class is

http://codereview.chromium.org/7174002/diff/34001/webkit/fileapi/local_file_s...
webkit/fileapi/local_file_system_file_util.h:123:
DISALLOW_COPY_AND_ASSIGN(LocalFileSystemFileUtil);
nit: why is this placed here?  can we move this into private?

[Dai Mikurube (NOT FULLTIME), 2011-07-05 08:05:40]

Thanks.  Updated and submitted to try bots including the linux_chromeos bot.

http://codereview.chromium.org/7174002/diff/34001/webkit/fileapi/local_file_s...
File webkit/fileapi/local_file_system_file_util.h (right):

http://codereview.chromium.org/7174002/diff/34001/webkit/fileapi/local_file_s...
webkit/fileapi/local_file_system_file_util.h:15: #include
"base/memory/ref_counted.h"
On 2011/07/05 06:35:24, kinuko wrote:
> nit: not necessary?

Done.

http://codereview.chromium.org/7174002/diff/34001/webkit/fileapi/local_file_s...
webkit/fileapi/local_file_system_file_util.h:17: #include
"base/tracked_objects.h"
On 2011/07/05 06:35:24, kinuko wrote:
> nit: not necessary?

Done.

http://codereview.chromium.org/7174002/diff/34001/webkit/fileapi/local_file_s...
webkit/fileapi/local_file_system_file_util.h:36: // Now, this class is created
and owned only by *MountPointProvider and tests.
On 2011/07/05 06:35:24, kinuko wrote:
> wording nit: 'Now, ' and 'only' is not needed I think?  ('and tests.' at the
end
> might be dropped as well)
> wording nit: this class is -> an instance of this class is

Done.

http://codereview.chromium.org/7174002/diff/34001/webkit/fileapi/local_file_s...
webkit/fileapi/local_file_system_file_util.h:123:
DISALLOW_COPY_AND_ASSIGN(LocalFileSystemFileUtil);
On 2011/07/05 06:35:24, kinuko wrote:
> nit: why is this placed here?  can we move this into private?

Done.

[Dai Mikurube (NOT FULLTIME), 2011-07-06 10:03:24]

The try bot error in Windows:pyauto_functional_tests looks not related to this
change.  Checked the "Commit" box.

[commit-bot: I haz the power, 2011-07-06 10:50:52]

Change committed as 91547