Add support for a directory file chooser command, which creates a file list b...

Add support for a directory file chooser command, which creates a file list by enumerating all the files in the selected directory.

BUG=47162
TEST=layout test input-file-directory-upload.html, or www/~johnnyg/dir_upload/ demo page


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

[Andrew T Wilson (Slow), 2010-07-16 20:24:34]

LGTM, assuming that compile error is due to a pending webkit roll.

[Andrew T Wilson (Slow), 2010-07-16 20:24:47]

http://codereview.chromium.org/2825051/diff/1/4
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/2825051/diff/1/4#newcode2860
chrome/browser/tab_contents/tab_contents.cc:2860: // TODO(johnnyg): This
probably should be done async on a different thread.
Consider logging a bug and putting a bug number here.

http://codereview.chromium.org/2825051/diff/1/4#newcode2864
chrome/browser/tab_contents/tab_contents.cc:2864:
file_util::FileEnumerator::FILES));
In other places (like extensions/user_script_master.cc) they don't use the
static_cast, so perhaps you can omit it here as well since you're not or-ing two
values.

[brettw, 2010-07-18 23:49:26]

The good news is that your TODOs are exactly correct, we don't want to do IO on
the UI thread. The bad news is that you shouldn't check this in until those
flaws are fixed.

This isn't the type of thing we want to have a bug on file for, it's the type of
thing we need to do correctly every time. Avoiding any file operations on the UI
thread (including enumeration like you're doing in the RVH) is one of the main
reasons Chrome is so responsive.

[John Gregg, 2010-07-19 22:27:11]

On 2010/07/18 23:49:26, brettw wrote:
> The good news is that your TODOs are exactly correct, we don't want to do IO
on
> the UI thread. The bad news is that you shouldn't check this in until those
> flaws are fixed.
> 
> This isn't the type of thing we want to have a bug on file for, it's the type
of
> thing we need to do correctly every time. Avoiding any file operations on the
UI
> thread (including enumeration like you're doing in the RVH) is one of the main
> reasons Chrome is so responsive.

Point taken.  As I said in the TODO, that would have been the next step anyway
but I'm happy to do it now; new CL posted.

[Andrew T Wilson (Slow), 2010-07-20 01:21:32]

LGTM, but clearly it'd be good to let brett take a look also.

http://codereview.chromium.org/2825051/diff/26001/27002
File chrome/browser/file_enumerator_task.h (right):

http://codereview.chromium.org/2825051/diff/26001/27002#newcode49
chrome/browser/file_enumerator_task.h:49: // Non owned pointer; only accessed on
the UI thread thread.
nit: thread thread => thread

[brettw, 2010-07-22 18:08:16]

http://codereview.chromium.org/2825051/diff/26001/27003
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/2825051/diff/26001/27003#newcode2891
chrome/browser/tab_contents/tab_contents.cc:2891: file_util::GetFileInfo(path,
&info);
GetFileInfo also counts as file I/O, so that will have to be moved to a
background thread also.

It seems like changing this will make this even less natural, and I'm not super
enthusiastic about adding this logic to TabContents anyway. It adds an extra
interface to the already overwhelming list, and this file enumeration seems
unnatural here.

It seems to me like this should be a feature of the file selector. At least on
Windows, this already runs on a background thread with a callback. Ideally it
would have a mode where it would return a vector with the file(s) you picked and
would include the contents of a directory in that.

Does that make sense? I haven't thought through the implementation. If you need
help, please let me know. Sorry for the extra work.

[John Gregg, 2010-07-22 18:21:10]

> Does that make sense? I haven't thought through the implementation. If you
need
> help, please let me know. Sorry for the extra work.

On 2010/07/22 18:08:16, brettw wrote:
> http://codereview.chromium.org/2825051/diff/26001/27003
> File chrome/browser/tab_contents/tab_contents.cc (right):
> 
> http://codereview.chromium.org/2825051/diff/26001/27003#newcode2891
> chrome/browser/tab_contents/tab_contents.cc:2891: file_util::GetFileInfo(path,
> &info);
> GetFileInfo also counts as file I/O, so that will have to be moved to a
> background thread also.
> 
> It seems like changing this will make this even less natural, and I'm not
super
> enthusiastic about adding this logic to TabContents anyway. It adds an extra
> interface to the already overwhelming list, and this file enumeration seems
> unnatural here.

I agree that moving the GetFileInfo to the other thread would make things more
awkward.  However I'm sure I can find a non-file I/O way of remembering if it's
a folder selection or file selection, which would eliminate the problem.

> 
> It seems to me like this should be a feature of the file selector. At least on
> Windows, this already runs on a background thread with a callback. Ideally it
> would have a mode where it would return a vector with the file(s) you picked
and
> would include the contents of a directory in that.
> 

I think that's reasonable, as long as it is a separate mode.  There are some
situations where the low-level file selector might want to choose a folder just
for the name of the folder (like saving a file) rather than for the complete
directory contents (uploading the directory).

But the downside to this alternative is that I was thinking that as a further
improvement, we could provide more feedback while the enumeration is happening,
such as the number of files found and a cancel button, perhaps in an InfoBar. 
That suggests TabContents being the observer for the file enumeration in some
form, although I suppose it's independent of whether we use the file-selector
layer or a utility task.

[brettw, 2010-07-22 18:24:14]

> I think that's reasonable, as long as it is a separate mode.  There are some
> situations where the low-level file selector might want to choose a folder
> just
> for the name of the folder (like saving a file) rather than for the complete
> directory contents (uploading the directory).

Right, exactly. I think we would want some flag on the file chooser
specifying this behavior.


> But the downside to this alternative is that I was thinking that as a
> further
> improvement, we could provide more feedback while the enumeration is
> happening,
> such as the number of files found and a cancel button, perhaps in an
> InfoBar.
> That suggests TabContents being the observer for the file enumeration in
> some
> form, although I suppose it's independent of whether we use the
> file-selector
> layer or a utility task.

I'm guessing that we won't need that, but if we do, it could be a
dialog totally internal to the file picker, or we could have a file
picker delegate (which TabContents already implements) report status
if we wanted an infobar. So I don't think this limits future
enhancements. Does that sound right?

Brett

[John Gregg, 2010-07-22 18:31:38]

On 2010/07/22 18:24:14, brettw wrote:
> > I think that's reasonable, as long as it is a separate mode.  There are
some
> > situations where the low-level file selector might want to choose a folder
> > just
> > for the name of the folder (like saving a file) rather than for the complete
> > directory contents (uploading the directory).
> 
> Right, exactly. I think we would want some flag on the file chooser
> specifying this behavior.
> 
> 
> > But the downside to this alternative is that I was thinking that as a
> > further
> > improvement, we could provide more feedback while the enumeration is
> > happening,
> > such as the number of files found and a cancel button, perhaps in an
> > InfoBar.
> > That suggests TabContents being the observer for the file enumeration in
> > some
> > form, although I suppose it's independent of whether we use the
> > file-selector
> > layer or a utility task.
> 
> I'm guessing that we won't need that, but if we do, it could be a
> dialog totally internal to the file picker, or we could have a file
> picker delegate (which TabContents already implements) report status
> if we wanted an infobar. So I don't think this limits future
> enhancements. Does that sound right?
> 

Okay fair enough, I'll try this approach.

[brettw, 2010-07-22 18:55:44]

Thanks a lot!

[darin (slow to review), 2010-07-23 06:23:17]

http://codereview.chromium.org/2825051/diff/26001/27001
File chrome/browser/file_enumerator_task.cc (right):

http://codereview.chromium.org/2825051/diff/26001/27001#newcode34
chrome/browser/file_enumerator_task.cc:34: while (!(file =
enumerator.Next()).empty())
since this could be a really large directory that we are enumerating, what is
happening back on the UI thread while we are busy on this background thread?

if my disk is very busy, we could spend several seconds performing this
directory enumeration.

http://codereview.chromium.org/2825051/diff/26001/27001#newcode45
chrome/browser/file_enumerator_task.cc:45:
observer_->EnumerationFinished(files_);
what happens if the TabContents is destroyed between the time that Start is
called and Done is called?  won't we crash?

http://codereview.chromium.org/2825051/diff/26001/27002
File chrome/browser/file_enumerator_task.h (right):

http://codereview.chromium.org/2825051/diff/26001/27002#newcode15
chrome/browser/file_enumerator_task.h:15: class FileEnumeratorTask
you could have also used net::DirectoryLister here.  although it is designed for
more interactive uses as it generates a lot of PostTask traffic back to the
originating thread.  if you wanted to render progress UI (enumerating a
directory can be extremely slow!), it might be helpful.

alternatively, i think it would be worth it to generalize this class a little
bit.  perhaps we could create a ChromeThreadTask class that can be used whenever
we want to do some work on a specific ChromeThread and return to the originating
thread with some results.  i bet we have a lot of code in chrome/browser like
that.  we shouldn't have to write all of this thread proxying code everytime we
need to do one simple thing on a background thread!  someone needs to make
things cleaner for everyone :-)

http://codereview.chromium.org/2825051/diff/26001/27004
File chrome/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/2825051/diff/26001/27004#newcode107
chrome/browser/tab_contents/tab_contents.h:107: public
FileEnumeratorTask::Observer,
can you use Callback (from base/callback.h) to avoid this Observer interface? 
(also, Delegate or Client would be a better description of this interface.)

http://codereview.chromium.org/2825051/diff/26001/27007
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/2825051/diff/26001/27007#newcode1927
chrome/renderer/render_view.cc:1927: ipc_params.mode = params.directory ?
nested ?: operators hurts readability.  how about some if/else love?

[John Gregg, 2010-07-23 17:11:08]

Okay, let me try to reconcile the various pieces of advice.  

Now that I know about net::DirectoryLister, I should definitely scrap my custom
task class in favor of that. 

BrettW and I had settled on putting this work inside the system file picker
logic and extend that interface to support progress.  Having looked into that
I'm a little concerned that it's designed with blocking logic and not going to
handle the async problems (i.e., there's no Cancel support, so we will still
need to deal with the potential disappearance of the TabContents).

On 2010/07/23 06:23:17, darin wrote:
> http://codereview.chromium.org/2825051/diff/26001/27001
> File chrome/browser/file_enumerator_task.cc (right):
> 
> http://codereview.chromium.org/2825051/diff/26001/27001#newcode34
> chrome/browser/file_enumerator_task.cc:34: while (!(file =
> enumerator.Next()).empty())
> since this could be a really large directory that we are enumerating, what is
> happening back on the UI thread while we are busy on this background thread?
> 
> if my disk is very busy, we could spend several seconds performing this
> directory enumeration.

The UI thread will be free to do other things; the renderer queues up file
chooser requests already so the same renderer won't send over another until this
one returns, but otherwise things are normal. 

> 
> http://codereview.chromium.org/2825051/diff/26001/27001#newcode45
> chrome/browser/file_enumerator_task.cc:45:
> observer_->EnumerationFinished(files_);
> what happens if the TabContents is destroyed between the time that Start is
> called and Done is called?  won't we crash?

Yes, that is a problem.  I was planning to add a Cancel to the task, fortunately
the DirectoryLister already has it.

> 
> http://codereview.chromium.org/2825051/diff/26001/27002
> File chrome/browser/file_enumerator_task.h (right):
> 
> http://codereview.chromium.org/2825051/diff/26001/27002#newcode15
> chrome/browser/file_enumerator_task.h:15: class FileEnumeratorTask
> you could have also used net::DirectoryLister here.  although it is designed
for
> more interactive uses as it generates a lot of PostTask traffic back to the
> originating thread.  if you wanted to render progress UI (enumerating a
> directory can be extremely slow!), it might be helpful.
> 
> alternatively, i think it would be worth it to generalize this class a little
> bit.  perhaps we could create a ChromeThreadTask class that can be used
whenever
> we want to do some work on a specific ChromeThread and return to the
originating
> thread with some results.  i bet we have a lot of code in chrome/browser like
> that.  we shouldn't have to write all of this thread proxying code everytime
we
> need to do one simple thing on a background thread!  someone needs to make
> things cleaner for everyone :-)
> 
> http://codereview.chromium.org/2825051/diff/26001/27004
> File chrome/browser/tab_contents/tab_contents.h (right):
> 
> http://codereview.chromium.org/2825051/diff/26001/27004#newcode107
> chrome/browser/tab_contents/tab_contents.h:107: public
> FileEnumeratorTask::Observer,
> can you use Callback (from base/callback.h) to avoid this Observer interface? 
> (also, Delegate or Client would be a better description of this interface.)

I used the interface rather than a callback to allow extensibility later (such
as passing back additional progress to the Observer/Client), which I also think
is going to be desired.

> http://codereview.chromium.org/2825051/diff/26001/27007
> File chrome/renderer/render_view.cc (right):
> 
> http://codereview.chromium.org/2825051/diff/26001/27007#newcode1927
> chrome/renderer/render_view.cc:1927: ipc_params.mode = params.directory ?
> nested ?: operators hurts readability.  how about some if/else love?

Okay done.

[brettw, 2010-07-23 21:08:37]

On 2010/07/23 17:11:08, John Gregg wrote:
> Okay, let me try to reconcile the various pieces of advice.  
> 
> Now that I know about net::DirectoryLister, I should definitely scrap my
custom
> task class in favor of that. 
> 
> BrettW and I had settled on putting this work inside the system file picker
> logic and extend that interface to support progress.  Having looked into that
> I'm a little concerned that it's designed with blocking logic and not going to
> handle the async problems (i.e., there's no Cancel support, so we will still
> need to deal with the potential disappearance of the TabContents).

I see your concern about blocking. Perhaps the file chooser should return a flag
if a file or a directory was picked so then you can fire off a non-blocking
directory enumeration task.

[John Gregg, 2010-08-23 05:18:44]

Okay, please take another look at this patch.  I had to make some changes to
net::DirectoryLister to allow it to fit here (it didn't actually support
recursive listing in its original form) but now those have landed and this code
is ready to use it.

[brettw, 2010-08-23 22:07:30]

Thanks, this is looking better. I think we can make this, along with the
existing code, better with a little refactoring, which I think is very
worthwhile considering the current state of TabContents. Sorry for the extra
work, but I think it will be worthwhile.

I'm still ale uncomfortable adding extra code to TabContents (though it's not
that much), an extra base class to the existing 11 (!), and an extra header file
dependency to tab_contents.h (which is included in almost 300 other files in the
project).

I'd feel better about moving this to a helper class, along with the existing
file chooser code. That way the helper class can implement all the callback
interfaces 

This will also be closer to the design we'll likely end up with when the
TabContents refactor is complete, so it will be less work for the folks doing
that cleanup.

If you use composition and a scoped_ptr in tab_contents.h, we can actually
remove the file chooser dependencies from tab_contents.h which will help
compilation.

To pipe the commands to the helper class, I'd move the commands you need to a
different sub-class in RenderViewHostDelegate, and then add a getter there that
TabContents can use to (1) lazily create the helper class, and (2) return a
pointer to it. You can see an example, minus the lazy creation, in
GetRenderManagementDelegate.

http://codereview.chromium.org/2825051/diff/45001/46001
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/2825051/diff/45001/46001#newcode3015
chrome/browser/tab_contents/tab_contents.cc:3015: if
(file_util::FileEnumerator::IsDirectory(data.info))
Can you add a comment above this call that IsDirectory merely checks the flags
on the existing file finding results rather than actually checking the disk? If
you're ust casually looking at this code it looks like this is reading disk
which would be a big no-no from TabContents.

http://codereview.chromium.org/2825051/diff/45001/46002
File chrome/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/2825051/diff/45001/46002#newcode978
chrome/browser/tab_contents/tab_contents.h:978: //
net::DirectoryLister::DirectoryListerDelegate -----------------------------
Blank line below here would match the rest of the file.

[John Gregg, 2010-08-23 22:18:55]

> To pipe the commands to the helper class, I'd move the commands you need to a
> different sub-class in RenderViewHostDelegate, and then add a getter there
that
> TabContents can use to (1) lazily create the helper class, and (2) return a
> pointer to it. You can see an example, minus the lazy creation, in
> GetRenderManagementDelegate.

There's only one method on RVHD that is relevant (RunFileChooser), do you still
think I should create a secondary interface for that?

[brettw, 2010-08-23 22:41:11]

On Mon, Aug 23, 2010 at 3:18 PM,  <johnnyg@chromium.org> wrote:
>> To pipe the commands to the helper class, I'd move the commands you need
>> to a
>> different sub-class in RenderViewHostDelegate, and then add a getter there
>
> that
>>
>> TabContents can use to (1) lazily create the helper class, and (2) return
>> a
>> pointer to it. You can see an example, minus the lazy creation, in
>> GetRenderManagementDelegate.
>
> There's only one method on RVHD that is relevant (RunFileChooser), do you
> still
> think I should create a secondary interface for that?

I guess not. I guess I thought there was another one.

Brett

[John Gregg, 2010-08-24 00:00:13]

On 2010/08/23 22:41:11, brettw wrote:
> On Mon, Aug 23, 2010 at 3:18 PM,  <mailto:johnnyg@chromium.org> wrote:
> >> To pipe the commands to the helper class, I'd move the commands you need
> >> to a
> >> different sub-class in RenderViewHostDelegate, and then add a getter there
> >
> > that
> >>
> >> TabContents can use to (1) lazily create the helper class, and (2) return
> >> a
> >> pointer to it. You can see an example, minus the lazy creation, in
> >> GetRenderManagementDelegate.
> >
> > There's only one method on RVHD that is relevant (RunFileChooser), do you
> > still
> > think I should create a secondary interface for that?
> 
> I guess not. I guess I thought there was another one.
> 
> Brett
> 

Actually, though the interface is small it makes the delegation much cleaner, so
I created it anyway.  

Please take another look at the latest patch.  Now I'm removing code from
TabContents! :)

[brettw, 2010-08-24 16:14:20]

This is much nicer! Just some minor comments.

http://codereview.chromium.org/2825051/diff/50002/45005
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/2825051/diff/50002/45005#newcode16
chrome/browser/tab_contents/tab_contents.cc:16: #include "base/file_util.h"
You can remove this now

http://codereview.chromium.org/2825051/diff/50002/45006
File chrome/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/2825051/diff/50002/45006#newcode161
chrome/browser/tab_contents/tab_contents.h:161: TabContentsFileSelectHelper*
GetFileSelectHelper();
This isn't actually used by anybody so I would just delete it and move the impl
to the GetFileSelectDelegate.

http://codereview.chromium.org/2825051/diff/50002/45008
File chrome/browser/tab_contents/tab_contents_file_select_helper.h (right):

http://codereview.chromium.org/2825051/diff/50002/45008#newcode28
chrome/browser/tab_contents/tab_contents_file_select_helper.h:28: //
SelectFileDialog::Listener ------------------------------------------------
I'd skip the --- in this file. TabContents uses them to help separate things
since there are so many sections in the header file. But normally we don't use
them and it's not necessary here.

http://codereview.chromium.org/2825051/diff/50002/45008#newcode44
chrome/browser/tab_contents/tab_contents_file_select_helper.h:44:
TabContentsView* view();
These getters aren't inline. This is fine, but then they shouldn't use the
variable-style lowercase naming since they aren't as cheap to access as a
variable. Use the GetFoo() style, or in some of these cases, it may make sense
to just delete the function and substitute tab_contents_->foo() in the place
that you use it.

[John Gregg, 2010-08-24 16:59:03]

Okay, new patch posted.

http://codereview.chromium.org/2825051/diff/50002/45005
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/2825051/diff/50002/45005#newcode16
chrome/browser/tab_contents/tab_contents.cc:16: #include "base/file_util.h"
On 2010/08/24 16:14:21, brettw wrote:
> You can remove this now

Done.

http://codereview.chromium.org/2825051/diff/50002/45006
File chrome/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/2825051/diff/50002/45006#newcode161
chrome/browser/tab_contents/tab_contents.h:161: TabContentsFileSelectHelper*
GetFileSelectHelper();
On 2010/08/24 16:14:21, brettw wrote:
> This isn't actually used by anybody so I would just delete it and move the
impl
> to the GetFileSelectDelegate.

Done.

http://codereview.chromium.org/2825051/diff/50002/45008
File chrome/browser/tab_contents/tab_contents_file_select_helper.h (right):

http://codereview.chromium.org/2825051/diff/50002/45008#newcode28
chrome/browser/tab_contents/tab_contents_file_select_helper.h:28: //
SelectFileDialog::Listener ------------------------------------------------
On 2010/08/24 16:14:21, brettw wrote:
> I'd skip the --- in this file. TabContents uses them to help separate things
> since there are so many sections in the header file. But normally we don't use
> them and it's not necessary here.

Done.

http://codereview.chromium.org/2825051/diff/50002/45008#newcode44
chrome/browser/tab_contents/tab_contents_file_select_helper.h:44:
TabContentsView* view();
On 2010/08/24 16:14:21, brettw wrote:
> These getters aren't inline. This is fine, but then they shouldn't use the
> variable-style lowercase naming since they aren't as cheap to access as a
> variable. Use the GetFoo() style, or in some of these cases, it may make sense
> to just delete the function and substitute tab_contents_->foo() in the place
> that you use it.

I guess I could have just inlined them; was trying to avoid the includes but no
one includes this .h file anyway.  I followed your suggestion.

[brettw, 2010-08-24 17:51:44]

LGTM!