Make saving files from flash less annoying.

Make saving files from flash less annoying.

Resolves missing or relative suggested paths relative to the last opened directory, and when the path is a directory itself, browse files under it rather than itself under its parent.  Determining the latter takes a file system interrogation because the syntax isn't sufficient.

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

[Tom Sepez, 2012-06-27 21:07:06]

Evan, please review or suggest an appropriate reviewer.
This change works well using the example picmonkey.com application under pepper
flash as shown in the dependent bug. I worry that it may have unintended
consequences.

[Evan Stade, 2012-06-28 03:52:18]

http://codereview.chromium.org/10697010/diff/1/chrome/browser/ui/gtk/select_f...
File chrome/browser/ui/gtk/select_file_dialog_impl_gtk.cc (right):

http://codereview.chromium.org/10697010/diff/1/chrome/browser/ui/gtk/select_f...
chrome/browser/ui/gtk/select_file_dialog_impl_gtk.cc:407: if
(CallDirectoryExistsOnUIThread(default_path)) {
there has to be a way to do this without consulting the disk. You said the
caller provided an empty file name, can't we remember or encode that somehow?

[viettrungluu, 2012-06-28 03:59:52]

http://codereview.chromium.org/10697010/diff/1/chrome/browser/ui/gtk/select_f...
File chrome/browser/ui/gtk/select_file_dialog_impl_gtk.cc (right):

http://codereview.chromium.org/10697010/diff/1/chrome/browser/ui/gtk/select_f...
chrome/browser/ui/gtk/select_file_dialog_impl_gtk.cc:407: if
(CallDirectoryExistsOnUIThread(default_path)) {
On 2012/06/28 03:52:18, Evan Stade wrote:
> there has to be a way to do this without consulting the disk. You said the
> caller provided an empty file name, can't we remember or encode that somehow?

I agree that this is really awful (especially since this is an operation that
can take quite a long time, e.g., if the path involves a network drive).

We really should refactor the method/class (on all platforms) to have both
default_path and default_filename arguments.

[Tom Sepez, 2012-06-28 18:07:47]

The diaglog box as provided by your window system is about to stat() a bunch of
files anyways.  I think your thread is unavoidably janked here(as the kids would
say).

Even so, I don't think you can get the optimal behaviour without it.  Given args
like ("/foo", "bar") I think the behaviour changes if /foo/bar is a directory
(show an empty file suggestion under /foo/bar) vs. /foo/bar is a regular file or
doesn't exist (show bar as the suggested file under directory /foo).

I'm cobbling together the change to pass two arguments, but it ripples further
than I'd like.

[Tom Sepez, 2012-06-29 17:06:57]

Not liking the idea of making the callers responsible for this; they may get
input from other places besides this dialog which they lump together into a
string.  As I said above, if that contains any path components, we've just
passed the same path ambiguity problem onto them.

Most of the platform impls call stat() or equivalent on the current thread;
that's bad and we should get rid of it.

Would you be OK with bouncing to the File thread for the directory check and
then bouncing back to the UI thread to do the box?

[Evan Stade, 2012-06-29 18:50:36]

On 2012/06/29 17:06:57, Tom Sepez wrote:
> Not liking the idea of making the callers responsible for this; they may get
> input from other places besides this dialog which they lump together into a
> string.  As I said above, if that contains any path components, we've just
> passed the same path ambiguity problem onto them.
> 
> Most of the platform impls call stat() or equivalent on the current thread;
> that's bad and we should get rid of it.
> 
> Would you be OK with bouncing to the File thread for the directory check and
> then bouncing back to the UI thread to do the box?

well, that would be OK, but it would be kind of silly to take such a hard line
in this case since CallDirectoryExistsOnUIThread already exists and is used in
similar circumstances. Thanks for investigating this, I suppose the change is
not as easy in reality as it is in theory. I'm suppose I'm OK with the original
patch.

[Tom Sepez, 2012-06-29 21:59:08]

Looks like the windows version of this will be doing the disk operation on its
own thread anyways.  I've not run the win code yet.

[Evan Stade, 2012-07-02 18:59:41]

lgtm, assuming it works on windows

http://codereview.chromium.org/10697010/diff/10005/chrome/browser/ui/gtk/sele...
File chrome/browser/ui/gtk/select_file_dialog_impl_gtk.cc (right):

http://codereview.chromium.org/10697010/diff/10005/chrome/browser/ui/gtk/sele...
chrome/browser/ui/gtk/select_file_dialog_impl_gtk.cc:410:
gtk_file_chooser_set_current_name(GTK_FILE_CHOOSER(dialog), "");
is this line necessary?

[commit-bot: I haz the power, 2012-08-14 22:07:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tsepez@chromium.org/10697010/31001

[commit-bot: I haz the power, 2012-08-14 22:07:13]

Presubmit check for 10697010-31001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome
    ui

Presubmit checks took 4.0s to calculate.

[Tom Sepez, 2012-08-14 22:10:30]

+ben who is listed in both OWNERS files.

[Ben Goodger (Google), 2012-08-14 23:06:54]

lgtm

http://codereview.chromium.org/10697010/diff/31001/ui/base/dialogs/select_fil...
File ui/base/dialogs/select_file_dialog_win.cc (right):

http://codereview.chromium.org/10697010/diff/31001/ui/base/dialogs/select_fil...
ui/base/dialogs/select_file_dialog_win.cc:82: file_info.is_directory :
file_util::EndsWithSeparator(path);
4-space indent

[Tom Sepez, 2012-08-14 23:10:43]

http://codereview.chromium.org/10697010/diff/31001/ui/base/dialogs/select_fil...
File ui/base/dialogs/select_file_dialog_win.cc (right):

http://codereview.chromium.org/10697010/diff/31001/ui/base/dialogs/select_fil...
ui/base/dialogs/select_file_dialog_win.cc:82: file_info.is_directory :
file_util::EndsWithSeparator(path);
On 2012/08/14 23:06:54, Ben Goodger (Google) wrote:
> 4-space indent

Done.

[commit-bot: I haz the power, 2012-08-14 23:11:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tsepez@chromium.org/10697010/42001

[commit-bot: I haz the power, 2012-08-15 03:41:21]

Try job failure for 10697010-42001 (retry) on linux_chromeos for step
"browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-08-15 17:08:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tsepez@chromium.org/10697010/42001

[commit-bot: I haz the power, 2012-08-15 20:44:03]

Change committed as 151738