Port directory lister so test shell can view file directories.

Port directory lister so test shell can view file directories.


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

[Evan Stade, 2008-11-18 02:22:00]

This doesn't get it working yet, but it's closer.

http://codereview.chromium.org/11437/diff/1/5
File net/base/directory_lister.cc (left):

http://codereview.chromium.org/11437/diff/1/5#oldcode88
Line 88: CloseHandle(thread_);
Should there be some sort of ProcessThread::CloseHandle function which would get
called here?

[Evan Stade, 2008-11-18 03:27:49]

OK I take it back. This does get it working.

[tony, 2008-11-18 04:41:17]

The net_util and url_request changes LGTM.  A couple nits below.

http://codereview.chromium.org/11437/diff/213/218
File net/base/net_util.cc (right):

http://codereview.chromium.org/11437/diff/213/218#newcode810
Line 810: std::wstring path;
I realize this is temporary code, but this should use a FilePath.  A DLOG(INFO)
would be nice too.

http://codereview.chromium.org/11437/diff/213/219
File net/base/net_util.h (right):

http://codereview.chromium.org/11437/diff/213/219#newcode23
Line 23: 
Nit: This blank line and the one two lines below are unnecessary.

[ibrar, 2008-11-18 07:40:45]

Hi Evan,

Actually I have done the same work and planning to upload it. But you did it a
marvelous job here and beat me. Here are my comments.

BTW if you need my changes of directory_lister I can send you. May be it will
helpful to you.

http://codereview.chromium.org/11437/diff/213/222
File base/file_util.h (left):

http://codereview.chromium.org/11437/diff/213/222#oldcode365
Line 365: std::wstring root_path_;
Same we should use FilePath here

http://codereview.chromium.org/11437/diff/213/217
File net/base/directory_lister.cc (right):

http://codereview.chromium.org/11437/diff/213/217#newcode35
Line 35: DirectoryLister::DirectoryLister(const std::wstring& dir,
What about using FilePath instead of wstring to avoid casting to string.

http://codereview.chromium.org/11437/diff/213/220
File net/base/directory_lister.h (right):

http://codereview.chromium.org/11437/diff/213/220#newcode57
Line 57: void ThreadMain();
I don't agreed with the name ThreadMain. Name of thread function should be
logical like PathTraversalTh etc.

http://codereview.chromium.org/11437/diff/213/214
File net/url_request/url_request_file_dir_job.cc (right):

http://codereview.chromium.org/11437/diff/213/214#newcode192
Line 192: NotifyDone(URLRequestStatus(URLRequestStatus::FAILED, 0));  // TODO:
Better error code.
Line size issue here (80 character)

[Evan Stade, 2008-11-18 19:01:09]

thanks for the comments Ibrar.

http://codereview.chromium.org/11437/diff/213/217
File net/base/directory_lister.cc (right):

http://codereview.chromium.org/11437/diff/213/217#newcode35
Line 35: DirectoryLister::DirectoryLister(const std::wstring& dir,
On 2008/11/18 07:40:45, ibrar wrote:
> What about using FilePath instead of wstring to avoid casting to string.

it's true that this should be a FilePath, but there tends to be a domino effect
when you start changing wstrings to FilePaths. I'd rather leave that for another
changelist.

http://codereview.chromium.org/11437/diff/213/220
File net/base/directory_lister.h (right):

http://codereview.chromium.org/11437/diff/213/220#newcode57
Line 57: void ThreadMain();
On 2008/11/18 07:40:45, ibrar wrote:
> I don't agreed with the name ThreadMain. Name of thread function should be
> logical like PathTraversalTh etc. 

I don't really have a choice.

http://codereview.chromium.org/11437/diff/213/218
File net/base/net_util.cc (right):

http://codereview.chromium.org/11437/diff/213/218#newcode810
Line 810: std::wstring path;
On 2008/11/18 04:41:18, tony wrote:
> I realize this is temporary code, but this should use a FilePath.  A
DLOG(INFO)
> would be nice too.

Done.

http://codereview.chromium.org/11437/diff/213/219
File net/base/net_util.h (right):

http://codereview.chromium.org/11437/diff/213/219#newcode23
Line 23: 
On 2008/11/18 04:41:18, tony wrote:
> Nit: This blank line and the one two lines below are unnecessary.

Done.

http://codereview.chromium.org/11437/diff/23/28
File net/base/net_util.cc (right):

http://codereview.chromium.org/11437/diff/23/28#newcode25
Line 25: #include "base/basictypes.h"
#include "base/file_path.h"

[Dean McNamee, 2008-11-19 01:58:52]

just skimmed

http://codereview.chromium.org/11437/diff/50/58
File base/file_util_win.cc (right):

http://codereview.chromium.org/11437/diff/50/58#newcode634
Line 634: memcpy(info, &find_data_, sizeof(WIN32_FIND_DATA));
sizeof(*info) ?

http://codereview.chromium.org/11437/diff/50/54
File net/base/directory_lister.cc (right):

http://codereview.chromium.org/11437/diff/50/54#newcode60
Line 60: if (!PlatformThread::Create(0, this, &thread_)) {
I think it makes more sense to use worker_pool ?

[darin (slow to review), 2008-11-19 02:11:49]

http://codereview.chromium.org/11437/diff/50/59
File base/file_util.h (right):

http://codereview.chromium.org/11437/diff/50/59#newcode333
Line 333: } FileInfo;
hrm... notice that there is another FileInfo structure defined in this file :-)

[Evan Stade, 2008-11-19 04:07:57]

http://codereview.chromium.org/11437/diff/50/59
File base/file_util.h (right):

http://codereview.chromium.org/11437/diff/50/59#newcode333
Line 333: } FileInfo;
On 2008/11/19 02:11:49, darin wrote:
> hrm... notice that there is another FileInfo structure defined in this file
:-)

are you suggesting I rename this one?

http://codereview.chromium.org/11437/diff/50/58
File base/file_util_win.cc (right):

http://codereview.chromium.org/11437/diff/50/58#newcode634
Line 634: memcpy(info, &find_data_, sizeof(WIN32_FIND_DATA));
On 2008/11/19 01:58:52, Dean McNamee wrote:
> sizeof(*info) ?

ok

http://codereview.chromium.org/11437/diff/50/54
File net/base/directory_lister.cc (right):

http://codereview.chromium.org/11437/diff/50/54#newcode60
Line 60: if (!PlatformThread::Create(0, this, &thread_)) {
On 2008/11/19 01:58:52, Dean McNamee wrote:
> I think it makes more sense to use worker_pool ?

do you mean to make DirectoryLister a task and post itself via WorkerPool here?
Because that doesn't work (WorkerPool deletes it instead of calling Release()).

[Evan Stade, 2008-11-19 19:04:53]

FileEnumerator::FileInfo --> FileEnumerator::FindInfo

[Evan Stade, 2008-11-19 22:38:08]

Added LOG(INFO) message.

ping

[Evan Martin, 2008-11-19 23:00:51]

LGTM