Do not allow GTK File Chooser dialogs to return directories. This is probably...

Do not allow GTK File Chooser dialogs to return directories. This is probably a regression in newer versions of GTK.

BUG=23595
TEST=In Jaunty, attempt to attach /usr/bin in Gmail.

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

[Evan Stade, 2009-10-22 23:01:20]

http://codereview.chromium.org/306060/diff/1/2
File chrome/browser/gtk/dialogs_gtk.cc (right):

http://codereview.chromium.org/306060/diff/1/2#newcode10
Line 10: #include "base/file_util.h"
nit: include file_path as well

http://codereview.chromium.org/306060/diff/1/2#newcode490
Line 490: if (!file_util::GetFileInfo(path, &file_info))
this wouldn't happen to be a file access on the UI thread, would it?

http://codereview.chromium.org/306060/diff/1/2#newcode493
Line 493: goto select_single_file_failed;
gotos are not within chrome style

[Lei Zhang, 2009-10-23 00:48:34]

http://codereview.chromium.org/306060/diff/1/2
File chrome/browser/gtk/dialogs_gtk.cc (right):

http://codereview.chromium.org/306060/diff/1/2#newcode10
Line 10: #include "base/file_util.h"
On 2009/10/22 23:01:21, Evan Stade wrote:
> nit: include file_path as well

file_util includes file_path and I don't think that'll ever get removed.

http://codereview.chromium.org/306060/diff/1/2#newcode490
Line 490: if (!file_util::GetFileInfo(path, &file_info))
On 2009/10/22 23:01:21, Evan Stade wrote:
> this wouldn't happen to be a file access on the UI thread, would it?

yes, but we just accessed the same files in gtk_file_chooser_get_filename().

http://codereview.chromium.org/306060/diff/1/2#newcode493
Line 493: goto select_single_file_failed;
On 2009/10/22 23:01:21, Evan Stade wrote:
> gotos are not within chrome style

gotos be gone.

[Lei Zhang, 2009-10-28 22:06:08]

ping.

[Evan Stade, 2009-10-28 22:16:29]

lgtm

http://codereview.chromium.org/306060/diff/1/2
File chrome/browser/gtk/dialogs_gtk.cc (right):

http://codereview.chromium.org/306060/diff/1/2#newcode10
Line 10: #include "base/file_util.h"
On 2009/10/23 00:48:34, Lei Zhang wrote:
> On 2009/10/22 23:01:21, Evan Stade wrote:
> > nit: include file_path as well
> 
> file_util includes file_path and I don't think that'll ever get removed.

that's true but I don't think it's a reason not to include file_path, most files
seem to include both if they use both

http://codereview.chromium.org/306060/diff/3001/4001#newcode478
Line 478: gint response_id,
wrong indentation

http://codereview.chromium.org/306060/diff/3001/4001#newcode501
Line 501: if (filename)
can you make a note here that we specifically don't care that we're making disk
accesses on the ui thread

[Lei Zhang, 2009-10-28 22:30:00]

http://codereview.chromium.org/306060/diff/3001/4001
File chrome/browser/gtk/dialogs_gtk.cc (right):

http://codereview.chromium.org/306060/diff/3001/4001#newcode478
Line 478: gint response_id,
On 2009/10/28 22:16:29, Evan Stade wrote:
> wrong indentation

Fixed.

http://codereview.chromium.org/306060/diff/3001/4001#newcode501
Line 501: if (file_util::GetFileInfo(path, &file_info) &&
!file_info.is_directory) {
On 2009/10/28 22:16:29, Evan Stade wrote:
> can you make a note here that we specifically don't care that we're making
disk
> accesses on the ui thread

Done.

[Evan Stade, 2009-10-29 00:37:41]

http://codereview.chromium.org/306060/diff/7002/7003
File chrome/browser/gtk/dialogs_gtk.cc (right):

http://codereview.chromium.org/306060/diff/7002/7003#newcode502
Line 502: // the file selection dialog.
nit: the reason it's ok is because it's silly to optimize out a single stat when
the dialog just did like a million stats. I doubt it's true the file selection
dialog *necessarily* just accessed it (consider when the user types the name in
manually and hits enter quickly)

http://codereview.chromium.org/306060/diff/7002/7003#newcode504
Line 504: if (file_util::GetFileInfo(path, &file_info) &&
!file_info.is_directory) {
I just remembered: there's a file_util function that checks if a file is a
directory. You should use that.

[Lei Zhang, 2009-10-29 01:11:01]

On 2009/10/29 00:37:41, Evan Stade wrote:
> http://codereview.chromium.org/306060/diff/7002/7003
> File chrome/browser/gtk/dialogs_gtk.cc (right):
> 
> http://codereview.chromium.org/306060/diff/7002/7003#newcode502
> Line 502: // the file selection dialog.
> nit: the reason it's ok is because it's silly to optimize out a single stat
when
> the dialog just did like a million stats. I doubt it's true the file selection
> dialog *necessarily* just accessed it (consider when the user types the name
in
> manually and hits enter quickly)

The GTK file dialog tries to auto complete what you're typing, so it probably
doing some stats. The one more stat won't hurt argument works too.

> http://codereview.chromium.org/306060/diff/7002/7003#newcode504
> Line 504: if (file_util::GetFileInfo(path, &file_info) &&
> !file_info.is_directory) {
> I just remembered: there's a file_util function that checks if a file is a
> directory. You should use that.

Done.