Added support for KDE file dialogs.

Added support for KDE file dialogs.

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

[Evan Martin, 2011-09-13 19:02:27]

Hey Linux dudes,

I've been reviewing dfilimon's patches and this one looks pretty much perfect to
me.  Do you have any thoughts on whether we should put this behind a flag or
just drop it on dev channel?  I vaguely worry that it won't get any testing (due
to few people on the team using KDE) and it might be missing some important
feature that will break our KDE users due to lack of an escape hatch.  On the
other hand, this is Linux, so maybe I just worry too much.

[dfilimon, 2011-09-13 19:05:57]

There is one issue I'd like to point out that I'm unsure how to deal with:
When FileSelected gets called it forwards the selected path to the listener
along with an index that corresponds to the file selection filter that was used
(I think!).

There's really no way to get this index in a straightforward way because what
they're passing in GTK is the index in the list of filters that the file chooser
has (gtk_file_chooser_list_filters(...)).

I currently just give the listener a 1. Not sure what the correct behavior
should be.

On 2011/09/13 19:02:27, Evan Martin wrote:
> Hey Linux dudes,
> 
> I've been reviewing dfilimon's patches and this one looks pretty much perfect
to
> me.  Do you have any thoughts on whether we should put this behind a flag or
> just drop it on dev channel?  I vaguely worry that it won't get any testing
(due
> to few people on the team using KDE) and it might be missing some important
> feature that will break our KDE users due to lack of an escape hatch.  On the
> other hand, this is Linux, so maybe I just worry too much.

[dfilimon, 2011-09-13 22:12:59]

I should also mention that the index doesn't seem to affect the functionality of
the dialogs in any way.

estade wrote this part of the code, so maybe he could help me understand what
that's for?

[dfilimon, 2011-09-14 03:09:04]

Ping? Anyone? :)

[Mike Mammarella, 2011-09-14 06:42:23]

Sorry, was at a conference today. This is neat; KDE folks will appreciate it I'm
sure. I'm surprised there was a handy helper binary that did everything we
needed without requiring us to link against any KDE libraries, right down to
what I assume is support for making a modal dialog relative to an existing
window.

If we're worried about testing we can just email vandebo a file to download now
and then; he uses KDE. :)

Here's a first pass of comments - I haven't looked as thoroughly as I should
yet, but I'm distracted at the moment and will have to look again later.

http://codereview.chromium.org/7885002/diff/3002/chrome/browser/ui/gtk/dialog...
File chrome/browser/ui/gtk/dialogs_kde.cc (right):

http://codereview.chromium.org/7885002/diff/3002/chrome/browser/ui/gtk/dialog...
chrome/browser/ui/gtk/dialogs_kde.cc:173:
base::ThreadRestrictions::ScopedAllowIO allow_io;
It looks like the reason you need this is that GetMimeTypeFilterString is called
from GetKDialogCommandLine which is called before switching to the file thread.
Could you move this work onto the file thread instead, and avoid needing this?

http://codereview.chromium.org/7885002/diff/3002/chrome/browser/ui/gtk/dialog...
chrome/browser/ui/gtk/dialogs_kde.cc:187: e = filter_set.end(); it != e; ++it) {
This loop construct took me a moment to figure out, trying to figure out where e
was actually defined and what it was for. I wonder if it's worth the tiny (if
any) performance difference compared to the additional overhead to understand
the code. It's probably not that big a deal either way though.

http://codereview.chromium.org/7885002/diff/3002/chrome/browser/ui/gtk/dialog...
chrome/browser/ui/gtk/dialogs_kde.cc:227:
command_line->AppendSwitchASCII("--title", title);
What happens if the title is not ASCII?

http://codereview.chromium.org/7885002/diff/3002/chrome/browser/ui/gtk/dialog...
chrome/browser/ui/gtk/dialogs_kde.cc:241: LOG(INFO) << "KDialog command line: "
It seems that a while back we switched to mostly doing VLOG(1) instead of
LOG(INFO). (I think the bots were generating way too much output and it was
annoying and/or slow. IIRC Evan was involved in making things better.)

http://codereview.chromium.org/7885002/diff/3002/chrome/browser/ui/gtk/dialog...
chrome/browser/ui/gtk/dialogs_kde.cc:407: 
Please remove the trailing whitespace lines.

[dfilimon, 2011-09-14 22:06:08]

This includes the patch to fix the SIGSEGV because I can't figure out how to
work with multiple changes involving the same files in SVN.

The MimeType in the FILE thread is coming soon :)

http://codereview.chromium.org/7885002/diff/3002/chrome/browser/ui/gtk/dialog...
File chrome/browser/ui/gtk/dialogs_kde.cc (right):

http://codereview.chromium.org/7885002/diff/3002/chrome/browser/ui/gtk/dialog...
chrome/browser/ui/gtk/dialogs_kde.cc:173:
base::ThreadRestrictions::ScopedAllowIO allow_io;
On 2011/09/14 06:42:24, Mike Mammarella wrote:
> It looks like the reason you need this is that GetMimeTypeFilterString is
called
> from GetKDialogCommandLine which is called before switching to the file
thread.
> Could you move this work onto the file thread instead, and avoid needing this?

Right, I've uploaded a change incorporating your other feedback except for this
because putting this on the FILE thread would need some more refactoring.

Does it seem reasonable to create the command line entirely in that thread (thus
passing in all of arguments to GetKDialogCommandLine?)

http://codereview.chromium.org/7885002/diff/3002/chrome/browser/ui/gtk/dialog...
chrome/browser/ui/gtk/dialogs_kde.cc:187: e = filter_set.end(); it != e; ++it) {
On 2011/09/14 06:42:24, Mike Mammarella wrote:
> This loop construct took me a moment to figure out, trying to figure out where
e
> was actually defined and what it was for. I wonder if it's worth the tiny (if
> any) performance difference compared to the additional overhead to understand
> the code. It's probably not that big a deal either way though.

I removed it for clarity.

http://codereview.chromium.org/7885002/diff/3002/chrome/browser/ui/gtk/dialog...
chrome/browser/ui/gtk/dialogs_kde.cc:227:
command_line->AppendSwitchASCII("--title", title);
On 2011/09/14 06:42:24, Mike Mammarella wrote:
> What happens if the title is not ASCII?

Yeah, you're right! Shame on me, especially being in Internationalization.

I was trying to get it to work and was unsure which function I should use to
append. Fixing.

http://codereview.chromium.org/7885002/diff/3002/chrome/browser/ui/gtk/dialog...
chrome/browser/ui/gtk/dialogs_kde.cc:241: LOG(INFO) << "KDialog command line: "
On 2011/09/14 06:42:24, Mike Mammarella wrote:
> It seems that a while back we switched to mostly doing VLOG(1) instead of
> LOG(INFO). (I think the bots were generating way too much output and it was
> annoying and/or slow. IIRC Evan was involved in making things better.)

Done.

http://codereview.chromium.org/7885002/diff/3002/chrome/browser/ui/gtk/dialog...
chrome/browser/ui/gtk/dialogs_kde.cc:407: 
On 2011/09/14 06:42:24, Mike Mammarella wrote:
> Please remove the trailing whitespace lines.

Done.

[dfilimon, 2011-09-15 01:52:02]

This version (#9) has the refactoring Mike suggested, making sure that the mime
type string is obtained on the FILE thread.

[dfilimon, 2011-09-15 19:54:42]

Sorry for becoming annoying, but... Ping?

[Evan Martin, 2011-09-15 20:53:37]

LGTM, feel free to fix these suggestions and land it

I worry that the dialog will block the file thread, but I think we can fix that
later.

http://codereview.chromium.org/7885002/diff/6015/chrome/browser/ui/gtk/dialog...
File chrome/browser/ui/gtk/dialogs_common.h (right):

http://codereview.chromium.org/7885002/diff/6015/chrome/browser/ui/gtk/dialog...
chrome/browser/ui/gtk/dialogs_common.h:15: #include "base/memory/ref_counted.h"
Why is this needed now?

http://codereview.chromium.org/7885002/diff/6015/chrome/browser/ui/gtk/dialog...
File chrome/browser/ui/gtk/dialogs_kde.cc (right):

http://codereview.chromium.org/7885002/diff/6015/chrome/browser/ui/gtk/dialog...
chrome/browser/ui/gtk/dialogs_kde.cc:207: output.erase(output.size() - 1, 1);
output.resize(output.size() - 1) might be clearer

[dfilimon, 2011-09-15 21:58:14]

Okay, I've fixed the issues and am committing.

http://codereview.chromium.org/7885002/diff/6015/chrome/browser/ui/gtk/dialog...
File chrome/browser/ui/gtk/dialogs_common.h (right):

http://codereview.chromium.org/7885002/diff/6015/chrome/browser/ui/gtk/dialog...
chrome/browser/ui/gtk/dialogs_common.h:15: #include "base/memory/ref_counted.h"
On 2011/09/15 20:53:38, Evan Martin wrote:
> Why is this needed now?

Done.

http://codereview.chromium.org/7885002/diff/6015/chrome/browser/ui/gtk/dialog...
File chrome/browser/ui/gtk/dialogs_kde.cc (right):

http://codereview.chromium.org/7885002/diff/6015/chrome/browser/ui/gtk/dialog...
chrome/browser/ui/gtk/dialogs_kde.cc:207: output.erase(output.size() - 1, 1);
On 2011/09/15 20:53:38, Evan Martin wrote:
> output.resize(output.size() - 1) might be clearer

Done.

[Mike Mammarella, 2011-09-15 23:24:46]

LGTM, sorry for delay, was travelling after the conference.

[commit-bot: I haz the power, 2011-09-16 04:41:52]

Change committed as 101453

[Mike Mammarella, 2011-09-16 23:13:51]

So I just gave this a try and noticed that at least for me, KDialog seems to
choose files (for e.g. a file upload box) as soon as I click them. Usually you
can click once to select files in these sorts of boxes, and double click (or
click a button) to actually choose them. Do you know if there's a way to make
KDialog do that? The documentation for KDialog seems sparse.

[dfilimon, 2011-09-16 23:15:40]

On 2011/09/16 23:13:51, Mike Mammarella wrote:
> So I just gave this a try and noticed that at least for me, KDialog seems to
> choose files (for e.g. a file upload box) as soon as I click them. Usually you
> can click once to select files in these sorts of boxes, and double click (or
> click a button) to actually choose them. Do you know if there's a way to make
> KDialog do that? The documentation for KDialog seems sparse.

I think this is a KDE preference
(http://www.howtogeek.com/howto/ubuntu/disable-single-click-opening-of-files-i...)
That is for KDE3 but it's the same for KDE4. I disabled that and it behaves as
expected.

[Mike Mammarella, 2011-09-17 01:28:54]

On 2011/09/16 23:15:40, dfilimon wrote:
> On 2011/09/16 23:13:51, Mike Mammarella wrote:
> > So I just gave this a try and noticed that at least for me, KDialog seems to
> > choose files (for e.g. a file upload box) as soon as I click them. Usually
you
> > can click once to select files in these sorts of boxes, and double click (or
> > click a button) to actually choose them. Do you know if there's a way to
make
> > KDialog do that? The documentation for KDialog seems sparse.
> 
> I think this is a KDE preference
>
(http://www.howtogeek.com/howto/ubuntu/disable-single-click-opening-of-files-i...)
> That is for KDE3 but it's the same for KDE4. I disabled that and it behaves as
> expected.

Ah, great, so this means we'll actually be better integrated with the native
settings. I just tried changing that preference in my KDE session and it did
change KDialog's behavior as well. Cool!

[TheKro, 2012-01-11 10:41:20]

Since this update, there is no longer the option to save a website with the
"save as full webpage" option.  See
http://code.google.com/p/chromium/issues/detail?id=40060#c46