Adding size and last modified time support to FileEnumerator and FileUtilProxy::Entry.

Adding FileUtilProxy::Entry::{size,last_modified_time},
 and FileEnumerator::{GetFilesize,GetLastModifiedTime}.

BUG=89134
TEST=None


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

[tzik, 2011-07-13 05:34:39]

Hi.
I'm adding size and modified time to FileUtilProxy::Entry for
http://codereview.chromium.org/7353012/ (Fill size and modified time entry on
FileSystem API directory view.)

Could you review this patch?

[kinuko, 2011-07-13 05:58:47]

We could do this (http://crbug.com/89134) without the file_util::FileEnumerator
changes (having those methods itself looks useful to me, leaving the decision to
brett), but if we really change the part maybe we should also add unittest?

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

http://codereview.chromium.org/7355013/diff/5011/webkit/fileapi/obfuscated_fi...
webkit/fileapi/obfuscated_file_system_file_util.cc:231: if
(!db->GetFileInfo(*iter, &file_info)) {
seems like now we could (and maybe should) simply call GetFileInfo() in this for
loop as it correctly dispatches the call to its lower layer?

[tzik, 2011-07-13 07:38:41]

revised subject and description

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

http://codereview.chromium.org/7355013/diff/5011/webkit/fileapi/obfuscated_fi...
webkit/fileapi/obfuscated_file_system_file_util.cc:231: if
(!db->GetFileInfo(*iter, &file_info)) {
On 2011/07/13 05:58:47, kinuko wrote:
> seems like now we could (and maybe should) simply call GetFileInfo() in this
for
> loop as it correctly dispatches the call to its lower layer?

Done.

[kinuko, 2011-07-13 07:56:51]

comments for changes under fileapi/

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

http://codereview.chromium.org/7355013/diff/6008/webkit/fileapi/obfuscated_fi...
webkit/fileapi/obfuscated_file_system_file_util.cc:177: if
(!db->GetFileInfo(file_id, &local_info)) {
Maybe this part could also be included into the factored-out method?

http://codereview.chromium.org/7355013/diff/6008/webkit/fileapi/obfuscated_fi...
webkit/fileapi/obfuscated_file_system_file_util.cc:724: PlatformFileError
ObfuscatedFileSystemFileUtil::GetPlatformFileInfo(
This naming may be confusing as it could be returning some virtualized info
(though the structure name we're filling is indeed PlatformFileInfo). 
GetFileInfoInternal() or something might be better?

http://codereview.chromium.org/7355013/diff/6008/webkit/fileapi/obfuscated_fi...
webkit/fileapi/obfuscated_file_system_file_util.cc:730:
FileSystemDirectoryDatabase* db = GetDirectoryDatabase(
Can we pass this pointer as a parameter?  (Actually in this code path do we need
to do this?)

[tzik, 2011-07-13 08:30:44]

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

http://codereview.chromium.org/7355013/diff/6008/webkit/fileapi/obfuscated_fi...
webkit/fileapi/obfuscated_file_system_file_util.cc:177: if
(!db->GetFileInfo(file_id, &local_info)) {
On 2011/07/13 07:56:51, kinuko wrote:
> Maybe this part could also be included into the factored-out method?

Done.

http://codereview.chromium.org/7355013/diff/6008/webkit/fileapi/obfuscated_fi...
webkit/fileapi/obfuscated_file_system_file_util.cc:724: PlatformFileError
ObfuscatedFileSystemFileUtil::GetPlatformFileInfo(
On 2011/07/13 07:56:51, kinuko wrote:
> This naming may be confusing as it could be returning some virtualized info
> (though the structure name we're filling is indeed PlatformFileInfo). 
> GetFileInfoInternal() or something might be better?

Done.

http://codereview.chromium.org/7355013/diff/6008/webkit/fileapi/obfuscated_fi...
webkit/fileapi/obfuscated_file_system_file_util.cc:730:
FileSystemDirectoryDatabase* db = GetDirectoryDatabase(
On 2011/07/13 07:56:51, kinuko wrote:
> Can we pass this pointer as a parameter?  (Actually in this code path do we
need
> to do this?)

Done.

[brettw, 2011-07-13 15:51:41]

base LGTM

http://codereview.chromium.org/7355013/diff/3003/base/file_util_win.cc
File base/file_util_win.cc (right):

http://codereview.chromium.org/7355013/diff/3003/base/file_util_win.cc#newcode13
base/file_util_win.cc:13: #include <limits>
Can you add a blank line here between the C & C++ includes?

http://codereview.chromium.org/7355013/diff/3003/base/file_util_win.cc#newcod...
base/file_util_win.cc:902: DCHECK(size.QuadPart <=
std::numeric_limits<int64>::max());
I probably wouldn't bother with this check, it's not clear to me what it helps.

[kinuko, 2011-07-14 04:18:28]

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

http://codereview.chromium.org/7355013/diff/8001/webkit/fileapi/obfuscated_fi...
webkit/fileapi/obfuscated_file_system_file_util.cc:215: if
(!db->GetFileInfo(*iter, &file_info)) {
this part is not needed?

[tzik, 2011-07-14 07:05:42]

http://codereview.chromium.org/7355013/diff/3003/base/file_util_win.cc
File base/file_util_win.cc (right):

http://codereview.chromium.org/7355013/diff/3003/base/file_util_win.cc#newcode13
base/file_util_win.cc:13: #include <limits>
On 2011/07/13 15:51:41, brettw wrote:
> Can you add a blank line here between the C & C++ includes?

Done.

http://codereview.chromium.org/7355013/diff/3003/base/file_util_win.cc#newcod...
base/file_util_win.cc:902: DCHECK(size.QuadPart <=
std::numeric_limits<int64>::max());
On 2011/07/13 15:51:41, brettw wrote:
> I probably wouldn't bother with this check, it's not clear to me what it
helps.

Out of my cowardice,  QuadPart can be greater than 2^63 in thousands years
after.

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

http://codereview.chromium.org/7355013/diff/8001/webkit/fileapi/obfuscated_fi...
webkit/fileapi/obfuscated_file_system_file_util.cc:215: if
(!db->GetFileInfo(*iter, &file_info)) {
On 2011/07/14 04:18:28, kinuko wrote:
> this part is not needed?

Done.

[kinuko, 2011-07-14 07:18:16]

fileapi/ changes lgtm.

http://codereview.chromium.org/7355013/diff/3003/base/file_util_win.cc
File base/file_util_win.cc (right):

http://codereview.chromium.org/7355013/diff/3003/base/file_util_win.cc#newcod...
base/file_util_win.cc:902: DCHECK(size.QuadPart <=
std::numeric_limits<int64>::max());
On 2011/07/14 07:05:42, tzik wrote:
> On 2011/07/13 15:51:41, brettw wrote:
> > I probably wouldn't bother with this check, it's not clear to me what it
> helps.

Long live chromium...

style-nit: it'd be better to use DCHECK_LE.
> 
> Out of my cowardice,  QuadPart can be greater than 2^63 in thousands years
> after.

[tzik, 2011-07-14 10:42:44]

http://codereview.chromium.org/7355013/diff/3003/base/file_util_win.cc
File base/file_util_win.cc (right):

http://codereview.chromium.org/7355013/diff/3003/base/file_util_win.cc#newcod...
base/file_util_win.cc:902: DCHECK(size.QuadPart <=
std::numeric_limits<int64>::max());
On 2011/07/14 07:18:17, kinuko wrote:
> On 2011/07/14 07:05:42, tzik wrote:
> > On 2011/07/13 15:51:41, brettw wrote:
> > > I probably wouldn't bother with this check, it's not clear to me what it
> > helps.
> 
> Long live chromium...
> 
> style-nit: it'd be better to use DCHECK_LE.
> > 
> > Out of my cowardice,  QuadPart can be greater than 2^63 in thousands years
> > after.
> 

Done.

[michaeln, 2011-07-14 20:43:27]

if it lg to brett and kinuko... then that works for me

[commit-bot: I haz the power, 2011-07-19 05:03:05]

Change committed as 92975