Allow net::DirectoryLister to be used to recursively list the directory, and ...

Allow net::DirectoryLister to be used to recursively list the directory, and add a FULL_PATH sort option.

BUG=41762
TEST=unit test


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

[Evan Stade, 2010-08-19 19:57:19]

http://codereview.chromium.org/3175023/diff/9001/10004
File net/base/directory_lister.h (right):

http://codereview.chromium.org/3175023/diff/9001/10004#newcode21
net/base/directory_lister.h:21: struct DirectoryListerData {
document. And it seems this should be a private data type of DirectoryLister.

http://codereview.chromium.org/3175023/diff/9001/10004#newcode41
net/base/directory_lister.h:41: const FilePath& path) = 0;
document this function please.

http://codereview.chromium.org/3175023/diff/9001/10004#newcode52
net/base/directory_lister.h:52: FULL_PATH
how does FULL_PATH differ from ALPHA?

http://codereview.chromium.org/3175023/diff/9001/10006
File net/url_request/url_request_file_dir_job.cc (right):

http://codereview.chromium.org/3175023/diff/9001/10006#newcode106
net/url_request/url_request_file_dir_job.cc:106: const FilePath& path) {
In which implementation of OnListFile is |path| actually used?

[John Gregg, 2010-08-20 17:33:08]

http://codereview.chromium.org/3175023/diff/9001/10004
File net/base/directory_lister.h (right):

http://codereview.chromium.org/3175023/diff/9001/10004#newcode21
net/base/directory_lister.h:21: struct DirectoryListerData {
On 2010/08/19 19:57:19, Evan Stade wrote:
> document. And it seems this should be a private data type of DirectoryLister.

Done.  I agree about making it private, but it was simpler not to make all the
comparators and the task class private as well.  In the new patch I went ahead
and did that.

http://codereview.chromium.org/3175023/diff/9001/10004#newcode41
net/base/directory_lister.h:41: const FilePath& path) = 0;
On 2010/08/19 19:57:19, Evan Stade wrote:
> document this function please.

Done.

http://codereview.chromium.org/3175023/diff/9001/10004#newcode52
net/base/directory_lister.h:52: FULL_PATH
On 2010/08/19 19:57:19, Evan Stade wrote:
> how does FULL_PATH differ from ALPHA?

ALPHA's not the best name; it's a custom sort that orders all directories before
all files.  I added comments.

http://codereview.chromium.org/3175023/diff/9001/10006
File net/url_request/url_request_file_dir_job.cc (right):

http://codereview.chromium.org/3175023/diff/9001/10006#newcode106
net/url_request/url_request_file_dir_job.cc:106: const FilePath& path) {
On 2010/08/19 19:57:19, Evan Stade wrote:
> In which implementation of OnListFile is |path| actually used?

The one I'm adding in my next patch!  Until then, no one. This is for a
directory upload form input.

[Evan Stade, 2010-08-20 18:54:28]

http://codereview.chromium.org/3175023/diff/9001/10004
File net/base/directory_lister.h (right):

http://codereview.chromium.org/3175023/diff/9001/10004#newcode52
net/base/directory_lister.h:52: FULL_PATH
On 2010/08/20 17:33:08, John Gregg wrote:
> On 2010/08/19 19:57:19, Evan Stade wrote:
> > how does FULL_PATH differ from ALPHA?
> 
> ALPHA's not the best name; it's a custom sort that orders all directories
before
> all files.  I added comments.
> 

I guess then the question becomes "why do you want to sort by type with ALPHA
but not FULL_PATH?" If the sort order is going to be user-facing, you probably
want to show directories first, and you can just use ALPHA. If not, can you
change the names to be more descriptive? That would be even better than
documentation in the header.

http://codereview.chromium.org/3175023/diff/25001/4006#newcode37
net/base/directory_lister.h:37: class DirectoryListerDelegate {
isn't |path| redundant?

[Evan Stade, 2010-08-20 19:07:55]

> If the sort order is going to be user-facing, you probably
> want to show directories first, and you can just use ALPHA.

in retrospect, this might not make sense in a recursive traversal. Anyway, the
rest of the comment applies.

[John Gregg, 2010-08-20 19:47:31]

yes, the main difference is you would use ALPHA for a single directory and
FULL_PATH for a recursive search.  I changed ALPHA to ALPHA_DIRS_FIRST to be
more specific.

http://codereview.chromium.org/3175023/diff/25001/4006
File net/base/directory_lister.h (right):

http://codereview.chromium.org/3175023/diff/25001/4006#newcode37
net/base/directory_lister.h:37: // and |path| contains the full path.
On 2010/08/20 18:54:29, Evan Stade wrote:
> isn't |path| redundant?

path isn't redundant because the FileInfo struct only contains the leaf file
name (along with size, date, various other OS-level stuff)

[Evan Stade, 2010-08-20 20:22:15]

thanks for the explanation(s)

mostly LGTM

http://codereview.chromium.org/3175023/diff/33001/34003
File net/base/directory_lister.cc (right):

http://codereview.chromium.org/3175023/diff/33001/34003#newcode161
net/base/directory_lister.cc:161: while (!canceled_.IsSet() && !((path =
file_enum.Next()).empty())) {
nit: too many () around path

http://codereview.chromium.org/3175023/diff/33001/34003#newcode185
net/base/directory_lister.cc:185: else if (sort_ != NO_SORT)
nit:

else if (sort_ == ALPHA_DIRS_FIRST)
  [...]
else
  DCHECK(sort_ == NO_SORT)

http://codereview.chromium.org/3175023/diff/33001/34004
File net/base/directory_lister.h (right):

http://codereview.chromium.org/3175023/diff/33001/34004#newcode38
net/base/directory_lister.h:38: virtual void OnListFile(
on second thought, maybe you should make this pass a DirectoryListerData struct
(and make that struct public?)

[John Gregg, 2010-08-20 23:41:06]

okay, nits fixed and changed the interface to take the struct.  please take
another look.

[Evan Stade, 2010-08-20 23:55:09]

LGTM with one last question

http://codereview.chromium.org/3175023/diff/50001/51004
File net/base/directory_lister.h (right):

http://codereview.chromium.org/3175023/diff/50001/51004#newcode89
net/base/directory_lister.h:89: class DirectoryDataEvent : public Task {
why did you move this class?

[John Gregg, 2010-08-21 00:17:32]

On 2010/08/20 23:55:09, Evan Stade wrote:
> LGTM with one last question
> 
> http://codereview.chromium.org/3175023/diff/50001/51004
> File net/base/directory_lister.h (right):
> 
> http://codereview.chromium.org/3175023/diff/50001/51004#newcode89
> net/base/directory_lister.h:89: class DirectoryDataEvent : public Task {
> why did you move this class?

I moved that while DirectoryListerData was private (it needs access).  Now I can
& will move it back.

[Evan Stade, 2010-08-21 00:20:04]

thanks, lg