GTK: improve app mode .desktop file creation....

GTK: improve app mode .desktop file creation.

1) pass --user-data-dir when appropriate
2) don't overwrite old .desktop files that share the same name.

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

[Evan Stade, 2009-09-30 00:20:10]

I realized I could just quote the switches' values rather than the whole
argument (the .desktop docs[1] are a little hard to follow)

[1] http://standards.freedesktop.org/desktop-entry-spec/latest/ar01s06.html

[Evan Martin, 2009-09-30 00:24:34]

I think Pawel is the appropriate reviewer.

In general, I think trying to manually quote command-line arguments is a recipe
for total disaster.  We should build an argv array and do all the escaping in
one place.

http://codereview.chromium.org/255016/diff/1/2
File chrome/browser/shell_integration_linux.cc (right):

http://codereview.chromium.org/255016/diff/1/2#newcode126
Line 126: return;
Should we report an error here?

http://codereview.chromium.org/255016/diff/1/2#newcode345
Line 345: ReplaceSubstringsAfterOffset(&switches, 0, "$", "");
This makes me anxious.  What about arguments like "> ~/.profile"?

We should use proper shell escaping.

http://codereview.chromium.org/255016/diff/1/3
File chrome/common/chrome_plugin_util.cc (right):

http://codereview.chromium.org/255016/diff/1/3#newcode142
Line 142: // Note: Do not change this flag!  Old Gears shortcuts will break if
you do!
What about this warning?

http://codereview.chromium.org/255016/diff/1/3#newcode144
Line 144: arguments_w += std::wstring(L"--") + switches::kApp + L"=\"" + url_w +
L"\"";
What about quoting this URL?

[Evan Stade, 2009-09-30 00:32:19]

http://codereview.chromium.org/255016/diff/1/2
File chrome/browser/shell_integration_linux.cc (right):

http://codereview.chromium.org/255016/diff/1/2#newcode126
Line 126: return;
On 2009/09/30 00:24:34, Evan Martin wrote:
> Should we report an error here?

yes. See TODO above.

http://codereview.chromium.org/255016/diff/1/2#newcode345
Line 345: ReplaceSubstringsAfterOffset(&switches, 0, "$", "");
On 2009/09/30 00:24:34, Evan Martin wrote:
> This makes me anxious.  What about arguments like "> ~/.profile"?
> 
> We should use proper shell escaping.

we can't just have arbitrary arguments at this point; the set of possible
arguments is very limited at this point as we construct them ourselves and don't
allow user input.

http://codereview.chromium.org/255016/diff/1/3
File chrome/common/chrome_plugin_util.cc (right):

http://codereview.chromium.org/255016/diff/1/3#newcode142
Line 142: // Note: Do not change this flag!  Old Gears shortcuts will break if
you do!
On 2009/09/30 00:24:34, Evan Martin wrote:
> What about this warning?

I believe what the warning means is "do not change the value of kApp or old
shortcuts will break".

http://codereview.chromium.org/255016/diff/1/3#newcode144
Line 144: arguments_w += std::wstring(L"--") + switches::kApp + L"=\"" + url_w +
L"\"";
On 2009/09/30 00:24:34, Evan Martin wrote:
> What about quoting this URL?

sorry, don't follow?

[Evan Martin, 2009-09-30 00:33:14]

On 2009/09/30 00:32:19, Evan Stade wrote:
> On 2009/09/30 00:24:34, Evan Martin wrote:
> > What about quoting this URL?
> 
> sorry, don't follow?

What if the URL contains embedded quotes?

[Evan Stade, 2009-09-30 00:36:17]

good catch. It seems that doesn't work on windows either. The quotes are just
dropped.

[Evan Stade, 2009-09-30 01:26:22]

updated.

I have tried to improve argument escaping, but on the whole I think that is
orthogonal to the two improvements this CL attempts to make; that is, respecting
user-data-dir and not overwriting existing shortcuts; also, Windows has made due
without perfect escaping for over a year it seems, so I think that the command
line escaping issue is not particularly urgent and can be left for a later
iteration.

[Paweł Hajdan Jr., 2009-09-30 17:33:06]

Please take a look at failing unit tests on Linux. It seems that it's just the
placement of quotes that changed. Adjusting the test should be safe.

http://codereview.chromium.org/255016/diff/2003/2004
File chrome/browser/shell_integration_linux.cc (right):

http://codereview.chromium.org/255016/diff/2003/2004#newcode32
Line 32: #include "chrome/common/chrome_plugin_util.h"
It seems awkward to use chrome_plugin_util here. Maybe the CPB_ function we call
here should be moved to some other, more generic location, or have the common
logic extraced out of it?

[Evan Stade, 2009-09-30 18:35:25]

On 2009/09/30 17:33:06, Paweł Hajdan Jr. wrote:
> Please take a look at failing unit tests on Linux. It seems that it's just the
> placement of quotes that changed. Adjusting the test should be safe.

done

> 
> http://codereview.chromium.org/255016/diff/2003/2004
> File chrome/browser/shell_integration_linux.cc (right):
> 
> http://codereview.chromium.org/255016/diff/2003/2004#newcode32
> Line 32: #include "chrome/common/chrome_plugin_util.h"
> It seems awkward to use chrome_plugin_util here. Maybe the CPB_ function we
call
> here should be moved to some other, more generic location, or have the common
> logic extraced out of it?

agreed. We will have to refactor that functionality due to the points Evan
raised so I think it might be best to leave the refactor for later.

[Evan Stade, 2009-10-01 00:33:12]

ping

[Paweł Hajdan Jr., 2009-10-01 00:36:42]

LGTM