Simplify directory path accounting.

Simplify directory path accounting.

BUG=none
TEST=existing test_shell_tests plus a new OFSFU test.

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

[ericu, 2011-08-11 21:53:49]

This is now ready for review, and I've just kicked off some try jobs.

Dai, this started with a revert of your directory cost CL--if I took out too
much, or forgot to take out something that's no longer needed, please let me
know.

[Dai Mikurube (NOT FULLTIME), 2011-08-12 05:17:54]

Thank you for working on it, Eric.

The basic logic looks good to me though I think some tests should be kept.

http://codereview.chromium.org/7608011/diff/11033/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_file_util.cc (right):

http://codereview.chromium.org/7608011/diff/11033/webkit/fileapi/file_system_...
webkit/fileapi/file_system_file_util.cc:269: }
I guess passing a correct context is better even if not used currently. (e.g.
virtual_path in the dest_context for same_file_system==true is not correct.)

http://codereview.chromium.org/7608011/diff/11033/webkit/fileapi/file_system_...
webkit/fileapi/file_system_file_util.cc:384: dest_file_path_each, false, false);
ditto.  The function names (CreateInherited..., Import...) might be to be fixed,
but a correct context is better.  This dest_context's virtual_path doesn't work
for CreateDirectory though virtual_path is not used currently.

http://codereview.chromium.org/7608011/diff/11033/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_file_util_unittest.cc (right):

http://codereview.chromium.org/7608011/diff/11033/webkit/fileapi/file_system_...
webkit/fileapi/file_system_file_util_unittest.cc:67:
context->set_allowed_bytes_growth(1024 * 1024);  // For paths.
Sorry, I don't understand the reason why this is required...  This is a test for
FSFU, not related to quota?

http://codereview.chromium.org/7608011/diff/11033/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_quota_unittest.cc (right):

http://codereview.chromium.org/7608011/diff/11033/webkit/fileapi/file_system_...
webkit/fileapi/file_system_quota_unittest.cc:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
I think this test should be kept.

FileSystemObfuscatedQuotaTest.TestRevokeUsageCache tests usage re-computation
with path name accounting which is done in
SandboxMountPointProvider::GetOriginUsageOnFileThread.

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

http://codereview.chromium.org/7608011/diff/11033/webkit/fileapi/obfuscated_f...
webkit/fileapi/obfuscated_file_system_file_util_unittest.cc:422:
TEST_F(ObfuscatedFileSystemFileUtilTest, TestCreateAndDeleteFile) {
We should have a quota testing code for every operation with path name
accounting to make sure that path names are accounted correctly.

[kinuko, 2011-08-15 03:42:34]

(Style nits only)

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

http://codereview.chromium.org/7608011/diff/11033/webkit/fileapi/obfuscated_f...
webkit/fileapi/obfuscated_file_system_file_util.cc:57: int
growthInNumberOfPaths,
growth_in_number_of_paths?

You seem to be following webkit's naming style :)

http://codereview.chromium.org/7608011/diff/11033/webkit/fileapi/obfuscated_f...
webkit/fileapi/obfuscated_file_system_file_util.cc:86:
context->file_system_context()->quota_manager_proxy();
nit: weird indent

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

http://codereview.chromium.org/7608011/diff/11033/webkit/fileapi/obfuscated_f...
webkit/fileapi/obfuscated_file_system_file_util_unittest.cc:769: iter !=
components.end(); ++iter) {
nit: indent

[ericu, 2011-08-15 23:47:10]

I've rolled in most of the feedback, as commented inline.  I still need to add
more tests; I'm working on that now.  I also need to check on what's causing
that assert on Windows.

http://codereview.chromium.org/7608011/diff/11033/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_file_util.cc (right):

http://codereview.chromium.org/7608011/diff/11033/webkit/fileapi/file_system_...
webkit/fileapi/file_system_file_util.cc:384: dest_file_path_each, false, false);
On 2011/08/12 05:17:54, Dai Mikurube wrote:
> ditto.  The function names (CreateInherited..., Import...) might be to be
fixed,
> but a correct context is better.  This dest_context's virtual_path doesn't
work
> for CreateDirectory though virtual_path is not used currently.

I agree with you about putting CreateInheritedContextForDest back; the code's
already in two places in this file, so we should consolidate it into one.  I'm
less sure about CreateInheritedContextWithNewVirtualPaths.  It looks like we're
doing a lot of bookkeeping and allocations just to keep the FSOC members, which
aren't used, in sync with the parameters, which carry the same information here.
 Perhaps there's a way to get rid of those FSOC members entirely?

It looks like they're only ever used to pass paths from a method to a later
method [called after looking up quota] in FileSystemOperation.  If that's the
case, we should really just come up with another way to pass that parameter. 
Either the quota callback should become a closure that carries the information,
or perhaps we should just add it as a data member in FileSystemOperation. 
There's precedent; members such as file_writer_delegate_ and exclusive_ are kept
around only for single operations, and thus have no business being in the FSOC. 
Or of course we could just crack the filesystem URL again to get the path out,
but that also seems wasteful.

http://codereview.chromium.org/7608011/diff/11033/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_file_util_unittest.cc (right):

http://codereview.chromium.org/7608011/diff/11033/webkit/fileapi/file_system_...
webkit/fileapi/file_system_file_util_unittest.cc:67:
context->set_allowed_bytes_growth(1024 * 1024);  // For paths.
On 2011/08/12 05:17:54, Dai Mikurube wrote:
> Sorry, I don't understand the reason why this is required...  This is a test
for
> FSFU, not related to quota?

Added a better comment.

http://codereview.chromium.org/7608011/diff/11033/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_quota_unittest.cc (right):

http://codereview.chromium.org/7608011/diff/11033/webkit/fileapi/file_system_...
webkit/fileapi/file_system_quota_unittest.cc:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
On 2011/08/12 05:17:54, Dai Mikurube wrote:
> I think this test should be kept.
> 
> FileSystemObfuscatedQuotaTest.TestRevokeUsageCache tests usage re-computation
> with path name accounting which is done in
> SandboxMountPointProvider::GetOriginUsageOnFileThread.

I agree that we need a test like this, but it doesn't seem to fit in this file,
which uses LocalFileSystemFileUtil, not ObfuscatedFileSystemFileUtil.  I'll add
it to OFSFU's test.

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

http://codereview.chromium.org/7608011/diff/11033/webkit/fileapi/obfuscated_f...
webkit/fileapi/obfuscated_file_system_file_util.cc:57: int
growthInNumberOfPaths,
On 2011/08/15 03:42:34, kinuko wrote:
> growth_in_number_of_paths?
> 
> You seem to be following webkit's naming style :)

Oops!  Thanks--I often do that.  It doesn't help that I use a more webkitty
style at home, either.

http://codereview.chromium.org/7608011/diff/11033/webkit/fileapi/obfuscated_f...
webkit/fileapi/obfuscated_file_system_file_util.cc:86:
context->file_system_context()->quota_manager_proxy();
On 2011/08/15 03:42:34, kinuko wrote:
> nit: weird indent

Done.

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

http://codereview.chromium.org/7608011/diff/11033/webkit/fileapi/obfuscated_f...
webkit/fileapi/obfuscated_file_system_file_util_unittest.cc:769: iter !=
components.end(); ++iter) {
On 2011/08/15 03:42:34, kinuko wrote:
> nit: indent

4 spaces at a line wrap, two spaces for the interior of a for loop, 4 spaces for
the next line wrap.  What would you prefer?

[Dai Mikurube (NOT FULLTIME), 2011-08-16 05:11:00]

Thank you.

http://codereview.chromium.org/7608011/diff/11033/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_file_util.cc (right):

http://codereview.chromium.org/7608011/diff/11033/webkit/fileapi/file_system_...
webkit/fileapi/file_system_file_util.cc:384: dest_file_path_each, false, false);
On 2011/08/15 23:47:10, ericu wrote:
> On 2011/08/12 05:17:54, Dai Mikurube wrote:
> > ditto.  The function names (CreateInherited..., Import...) might be to be
> fixed,
> > but a correct context is better.  This dest_context's virtual_path doesn't
> work
> > for CreateDirectory though virtual_path is not used currently.
> 
> I agree with you about putting CreateInheritedContextForDest back; the code's
> already in two places in this file, so we should consolidate it into one.  I'm
> less sure about CreateInheritedContextWithNewVirtualPaths.  It looks like
we're
> doing a lot of bookkeeping and allocations just to keep the FSOC members,
which
> aren't used, in sync with the parameters, which carry the same information
here.
>  Perhaps there's a way to get rid of those FSOC members entirely?
> 
> It looks like they're only ever used to pass paths from a method to a later
> method [called after looking up quota] in FileSystemOperation.  If that's the
> case, we should really just come up with another way to pass that parameter. 
> Either the quota callback should become a closure that carries the
information,
> or perhaps we should just add it as a data member in FileSystemOperation. 
> There's precedent; members such as file_writer_delegate_ and exclusive_ are
kept
> around only for single operations, and thus have no business being in the
FSOC. 
> Or of course we could just crack the filesystem URL again to get the path out,
> but that also seems wasteful.

Ah, make sense.  I agree with taking *_virtual_path away from FSOC and moving
them into FSO like exclusive_.  They're used only for Delayed... in FSO now.

http://codereview.chromium.org/7608011/diff/11033/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_quota_unittest.cc (right):

http://codereview.chromium.org/7608011/diff/11033/webkit/fileapi/file_system_...
webkit/fileapi/file_system_quota_unittest.cc:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
On 2011/08/15 23:47:10, ericu wrote:
> On 2011/08/12 05:17:54, Dai Mikurube wrote:
> > I think this test should be kept.
> > 
> > FileSystemObfuscatedQuotaTest.TestRevokeUsageCache tests usage
re-computation
> > with path name accounting which is done in
> > SandboxMountPointProvider::GetOriginUsageOnFileThread.
> 
> I agree that we need a test like this, but it doesn't seem to fit in this
file,
> which uses LocalFileSystemFileUtil, not ObfuscatedFileSystemFileUtil.  I'll
add
> it to OFSFU's test.

Ah, make sense.  Actually, I'd like to test 1), 2) and 3) (Line:5-8) with
LocalFSFU as a test of "the entire behavior of FileSystem usage and quota".  So
I put this RevokeUsageCache test in this file_system_quota_unittest.cc.

But it's using OFSFU since GetOriginUsageOnFileThread is in SandboxMPP.  I guess
it can be at OFSFUTest since you say QFU and OFSFU are considered as one layer
though we cannot check 1) the actual size of files on disk with OFSFU.

http://codereview.chromium.org/7608011/diff/20001/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_file_util.cc (right):

http://codereview.chromium.org/7608011/diff/20001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_file_util.cc:308: false /* recursive */)) {
dest_context->allowed_bytes_growth changes in Delete and other functions.

http://codereview.chromium.org/7608011/diff/20001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_file_util.cc:362: dest_file_path, false, false);
ditto.  dest_context->allowed_bytes_growth changes in CreateDirectory and other
functions.

We have to reflect this change back into the original context.  Without
reflecting it, later operations in CopyOrMoveDirectory might exceed quota.

The same for the other calls with dest_context.

http://codereview.chromium.org/7608011/diff/20001/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_file_util_unittest.cc (right):

http://codereview.chromium.org/7608011/diff/20001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_file_util_unittest.cc:69: // charges quota for paths.
Does this test require OFSFU?  Or could it be in OFSFUTest?

[ericu, 2011-08-16 21:16:31]

I've rolled in the remaining feedback and added all the new tests.  However, I
still need to chase down that Windows trybot failure.  I'm setting up a new
windows machine, so if I can't guess the solution via trybot, this may take a
day or two.

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

http://codereview.chromium.org/7608011/diff/11033/webkit/fileapi/obfuscated_f...
webkit/fileapi/obfuscated_file_system_file_util_unittest.cc:422:
TEST_F(ObfuscatedFileSystemFileUtilTest, TestCreateAndDeleteFile) {
On 2011/08/12 05:17:54, Dai Mikurube wrote:
> We should have a quota testing code for every operation with path name
> accounting to make sure that path names are accounted correctly.

Done.

http://codereview.chromium.org/7608011/diff/20001/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_file_util.cc (right):

http://codereview.chromium.org/7608011/diff/20001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_file_util.cc:308: false /* recursive */)) {
On 2011/08/16 05:11:00, Dai Mikurube wrote:
> dest_context->allowed_bytes_growth changes in Delete and other functions.

Done.

http://codereview.chromium.org/7608011/diff/20001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_file_util.cc:362: dest_file_path, false, false);
On 2011/08/16 05:11:00, Dai Mikurube wrote:
> ditto.  dest_context->allowed_bytes_growth changes in CreateDirectory and
other
> functions.
> 
> We have to reflect this change back into the original context.  Without
> reflecting it, later operations in CopyOrMoveDirectory might exceed quota.
> 
> The same for the other calls with dest_context.

Done [only in the success cases--I don't believe we have any operations that
attempt to continue after a failure].

http://codereview.chromium.org/7608011/diff/20001/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_file_util_unittest.cc (right):

http://codereview.chromium.org/7608011/diff/20001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_file_util_unittest.cc:69: // charges quota for paths.
On 2011/08/16 05:11:00, Dai Mikurube wrote:
> Does this test require OFSFU?  Or could it be in OFSFUTest?

The cross-filesystem copy/move is a FSFU method.  It happens to call into OFSFU
in this case.  We could theoretically test it by copying from one LocalFSFU to
another, but given that they'd both really be looking at the same underlying OS
filesystem, I don't think we could trust the test quite as much.  There could be
bugs that would slip through.

[Dai Mikurube (NOT FULLTIME), 2011-08-18 12:00:10]

http://codereview.chromium.org/7608011/diff/25002/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_file_util.cc (right):

http://codereview.chromium.org/7608011/diff/25002/webkit/fileapi/file_system_...
webkit/fileapi/file_system_file_util.cc:379: dest_file_path_each, false, false);
The dest_context should be reflected back in the while loop since
CopyOrMoveFileHelper uses the original context.  Without reflecting back here,
CopyOrMoveFileHelper succeeds even if CreateDirectory uses up the quota.

I guess reflecting back just after operations with copied contexts is safer...

http://codereview.chromium.org/7608011/diff/25002/webkit/fileapi/file_system_...
webkit/fileapi/file_system_file_util.cc:473: PlatformFileError error =
DeleteFile(context, file_path_each);
file_path_each is better to be passed as src_virtual_path in the context.  (Not
required if we remove *_virtual_path from FSOC in this change.)

[ericu, 2011-08-18 22:30:36]

I think this is ready to go.
I've kicked off some more try jobs to make sure.

http://codereview.chromium.org/7608011/diff/25002/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_file_util.cc (right):

http://codereview.chromium.org/7608011/diff/25002/webkit/fileapi/file_system_...
webkit/fileapi/file_system_file_util.cc:379: dest_file_path_each, false, false);
On 2011/08/18 12:00:10, Dai Mikurube wrote:
> The dest_context should be reflected back in the while loop since
> CopyOrMoveFileHelper uses the original context.  Without reflecting back here,
> CopyOrMoveFileHelper succeeds even if CreateDirectory uses up the quota.
> 
> I guess reflecting back just after operations with copied contexts is safer...

Ah, good catch--thanks!

Fixed.

http://codereview.chromium.org/7608011/diff/25002/webkit/fileapi/file_system_...
webkit/fileapi/file_system_file_util.cc:473: PlatformFileError error =
DeleteFile(context, file_path_each);
On 2011/08/18 12:00:10, Dai Mikurube wrote:
> file_path_each is better to be passed as src_virtual_path in the context. 
(Not
> required if we remove *_virtual_path from FSOC in this change.)

*_virtual_path is unused in all Delete/Remove calls, so it's not necessary here.
 Let's just remove it all.  If it's OK with you, though, I'd like to do that in
a separate CL to keep this one from getting too big.

[Dai Mikurube (NOT FULLTIME), 2011-08-19 10:12:29]

On 2011/08/18 22:30:36, ericu wrote:
> I think this is ready to go.
> I've kicked off some more try jobs to make sure.
> 
>
http://codereview.chromium.org/7608011/diff/25002/webkit/fileapi/file_system_...
> File webkit/fileapi/file_system_file_util.cc (right):
> 
>
http://codereview.chromium.org/7608011/diff/25002/webkit/fileapi/file_system_...
> webkit/fileapi/file_system_file_util.cc:379: dest_file_path_each, false,
false);
> On 2011/08/18 12:00:10, Dai Mikurube wrote:
> > The dest_context should be reflected back in the while loop since
> > CopyOrMoveFileHelper uses the original context.  Without reflecting back
here,
> > CopyOrMoveFileHelper succeeds even if CreateDirectory uses up the quota.
> > 
> > I guess reflecting back just after operations with copied contexts is
safer...
> 
> Ah, good catch--thanks!
> 
> Fixed.
> 
>
http://codereview.chromium.org/7608011/diff/25002/webkit/fileapi/file_system_...
> webkit/fileapi/file_system_file_util.cc:473: PlatformFileError error =
> DeleteFile(context, file_path_each);
> On 2011/08/18 12:00:10, Dai Mikurube wrote:
> > file_path_each is better to be passed as src_virtual_path in the context. 
> (Not
> > required if we remove *_virtual_path from FSOC in this change.)
> 
> *_virtual_path is unused in all Delete/Remove calls, so it's not necessary
here.
>  Let's just remove it all.  If it's OK with you, though, I'd like to do that
in
> a separate CL to keep this one from getting too big.

LGTM, though we'll need incrementing/decrementing the dirty count in OFSFU
immediately if we don't count-up/down it in FSO. 
(http://codereview.chromium.org/7671039/)

[commit-bot: I haz the power, 2011-08-19 22:37:20]

Change committed as 97522