Adding Size() to AbstractFileEnumerator

Adding Size() to AbstractFileEnumerator

BUG=94060
TEST='ObfuscatedFileSystemFileUtil.*'


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

[tzik, 2011-08-24 09:33:24]

Hi, kinuko-san.

I'm adding AbstractFileEnumerator::Size() and changing related code to use it.
Could you review this patch?

[kinuko, 2011-08-25 09:28:48]

This looks ok to me... eric can take a look at the change in Obfuscated layer? 
Thanks,

http://codereview.chromium.org/7717020/diff/1/webkit/fileapi/obfuscated_file_...
File webkit/fileapi/obfuscated_file_system_file_util.cc (right):

http://codereview.chromium.org/7717020/diff/1/webkit/fileapi/obfuscated_file_...
webkit/fileapi/obfuscated_file_system_file_util.cc:675:
underlying_file_util_(underlying_file_util) {
DCHECK() for pointers? (db, context, underlying_fu)

http://codereview.chromium.org/7717020/diff/1/webkit/fileapi/sandbox_mount_po...
File webkit/fileapi/sandbox_mount_point_provider.cc (right):

http://codereview.chromium.org/7717020/diff/1/webkit/fileapi/sandbox_mount_po...
webkit/fileapi/sandbox_mount_point_provider.cc:553: // easily add it on-demand,
so as not to waste time when it's not needed.
Can we remove this TODO now?!

[ericu, 2011-08-25 18:29:30]

The general approach looks fine; thanks for taking care of this.

http://codereview.chromium.org/7717020/diff/1/webkit/fileapi/local_file_syste...
File webkit/fileapi/local_file_system_file_util.cc (right):

http://codereview.chromium.org/7717020/diff/1/webkit/fileapi/local_file_syste...
webkit/fileapi/local_file_system_file_util.cc:265:
file_util::FileEnumerator::FindInfo file_util_info;
Is there any reason that we shouldn't cache the return from GetFindInfo as you
do in FileSystemFileUtil?

http://codereview.chromium.org/7717020/diff/1/webkit/fileapi/obfuscated_file_...
File webkit/fileapi/obfuscated_file_system_file_util.cc (right):

http://codereview.chromium.org/7717020/diff/1/webkit/fileapi/obfuscated_file_...
webkit/fileapi/obfuscated_file_system_file_util.cc:669:
FileSystemOperationContext* context,
You're assuming that this context will remain valid throughout the lifetime of
the enumerator.  That's not guaranteed anywhere.

I think it's probably true in all cases currently; you should add a comment to
the block above CreateFileEnumerator in file_system_file_util.h about the
usage...it won't /guarantee/ that nobody deletes the context, but it may make
debugging easier if they do.

http://codereview.chromium.org/7717020/diff/1/webkit/fileapi/obfuscated_file_...
webkit/fileapi/obfuscated_file_system_file_util.cc:705:
FileSystemOperationContext context(*context_);
There's no need to copy the context here; just pass in context_.

http://codereview.chromium.org/7717020/diff/1/webkit/fileapi/sandbox_mount_po...
File webkit/fileapi/sandbox_mount_point_provider.cc (right):

http://codereview.chromium.org/7717020/diff/1/webkit/fileapi/sandbox_mount_po...
webkit/fileapi/sandbox_mount_point_provider.cc:553: // easily add it on-demand,
so as not to waste time when it's not needed.
On 2011/08/25 09:28:49, kinuko wrote:
> Can we remove this TODO now?!

Yes.  [It was a little inaccurate anyway--we've already looked up the /path/ of
the underlying file, which saves time, but we don't cache the size.]

[tzik, 2011-08-26 02:41:53]

Thank you for reviewing.

http://codereview.chromium.org/7717020/diff/1/webkit/fileapi/local_file_syste...
File webkit/fileapi/local_file_system_file_util.cc (right):

http://codereview.chromium.org/7717020/diff/1/webkit/fileapi/local_file_syste...
webkit/fileapi/local_file_system_file_util.cc:265:
file_util::FileEnumerator::FindInfo file_util_info;
On 2011/08/25 18:29:30, ericu wrote:
> Is there any reason that we shouldn't cache the return from GetFindInfo as you
> do in FileSystemFileUtil?

Done.  No reason other than making the patch smaller.

http://codereview.chromium.org/7717020/diff/1/webkit/fileapi/obfuscated_file_...
File webkit/fileapi/obfuscated_file_system_file_util.cc (right):

http://codereview.chromium.org/7717020/diff/1/webkit/fileapi/obfuscated_file_...
webkit/fileapi/obfuscated_file_system_file_util.cc:669:
FileSystemOperationContext* context,
On 2011/08/25 18:29:30, ericu wrote:
> You're assuming that this context will remain valid throughout the lifetime of
> the enumerator.  That's not guaranteed anywhere.
> 
> I think it's probably true in all cases currently; you should add a comment to
> the block above CreateFileEnumerator in file_system_file_util.h about the
> usage...it won't /guarantee/ that nobody deletes the context, but it may make
> debugging easier if they do.

Make sense. I added a comment there.

http://codereview.chromium.org/7717020/diff/1/webkit/fileapi/obfuscated_file_...
webkit/fileapi/obfuscated_file_system_file_util.cc:675:
underlying_file_util_(underlying_file_util) {
On 2011/08/25 09:28:49, kinuko wrote:
> DCHECK() for pointers? (db, context, underlying_fu)

Done.

http://codereview.chromium.org/7717020/diff/1/webkit/fileapi/obfuscated_file_...
webkit/fileapi/obfuscated_file_system_file_util.cc:705:
FileSystemOperationContext context(*context_);
On 2011/08/25 18:29:30, ericu wrote:
> There's no need to copy the context here; just pass in context_.

I see. Done.

http://codereview.chromium.org/7717020/diff/1/webkit/fileapi/sandbox_mount_po...
File webkit/fileapi/sandbox_mount_point_provider.cc (right):

http://codereview.chromium.org/7717020/diff/1/webkit/fileapi/sandbox_mount_po...
webkit/fileapi/sandbox_mount_point_provider.cc:553: // easily add it on-demand,
so as not to waste time when it's not needed.
On 2011/08/25 18:29:30, ericu wrote:
> On 2011/08/25 09:28:49, kinuko wrote:
> > Can we remove this TODO now?!
> 
> Yes.  [It was a little inaccurate anyway--we've already looked up the /path/
of
> the underlying file, which saves time, but we don't cache the size.]

Done.

[ericu, 2011-08-26 16:38:24]

LGTM.

http://codereview.chromium.org/7717020/diff/7001/webkit/fileapi/file_system_f...
File webkit/fileapi/file_system_file_util.h (right):

http://codereview.chromium.org/7717020/diff/7001/webkit/fileapi/file_system_f...
webkit/fileapi/file_system_file_util.h:244: // instance.
How about just "The supplied context must remain valid for at least the lifetime
of the enumerator instance." ?

http://codereview.chromium.org/7717020/diff/7001/webkit/fileapi/obfuscated_fi...
File webkit/fileapi/obfuscated_file_system_file_util.cc (right):

http://codereview.chromium.org/7717020/diff/7001/webkit/fileapi/obfuscated_fi...
webkit/fileapi/obfuscated_file_system_file_util.cc:679:
DCHECK(underlying_file_util);
It's a tiny nit, but I prefer to DCHECK on the members [db_] rather than the
parameters [db], because it ensures that you've remembered to copy them in.  I
don't always remember to do that, of course, so you'll see both in my code ;'>.

[tzik, 2011-08-29 02:33:32]

http://codereview.chromium.org/7717020/diff/7001/webkit/fileapi/file_system_f...
File webkit/fileapi/file_system_file_util.h (right):

http://codereview.chromium.org/7717020/diff/7001/webkit/fileapi/file_system_f...
webkit/fileapi/file_system_file_util.h:244: // instance.
On 2011/08/26 16:38:24, ericu wrote:
> How about just "The supplied context must remain valid for at least the
lifetime
> of the enumerator instance." ?

Done.

http://codereview.chromium.org/7717020/diff/7001/webkit/fileapi/obfuscated_fi...
File webkit/fileapi/obfuscated_file_system_file_util.cc (right):

http://codereview.chromium.org/7717020/diff/7001/webkit/fileapi/obfuscated_fi...
webkit/fileapi/obfuscated_file_system_file_util.cc:679:
DCHECK(underlying_file_util);
On 2011/08/26 16:38:24, ericu wrote:
> It's a tiny nit, but I prefer to DCHECK on the members [db_] rather than the
> parameters [db], because it ensures that you've remembered to copy them in.  I
> don't always remember to do that, of course, so you'll see both in my code
;'>.

Done.
But, if we leave db_ (a raw pointer) uninitialized, it will probabily have
random non-zero value and pass the DCHECK test.

[commit-bot: I haz the power, 2011-08-29 14:27:28]

Try job failure for 7717020-10001 (retry) (retry) on linux for step
"base_unittests".
It's a second try, previously, step "base_unittests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux&numb...

[commit-bot: I haz the power, 2011-08-30 03:36:52]

Change committed as 98769