First step to create application shortcuts on Linux.

First step to create application shortcuts on Linux.

Create a working desktop shortcut. For now it displays no UI, but the backend works.

TEST=none
http://crbug.com/17251

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

[Aaron Boodman, 2009-08-11 00:23:13]

I don't think I'm the right person to review this.

[Paweł Hajdan Jr., 2009-08-11 00:24:57]

-aa, +agl then

[Paweł Hajdan Jr., 2009-08-11 21:10:17]

ping!

[tony, 2009-08-11 23:53:14]

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

http://codereview.chromium.org/164280/diff/1/2#newcode1117
Line 1117: #if defined(OS_WIN) || defined(OS_LINUX)
Can you add a comment saying that this doesn't actually open a dialog on Linux? 
Maybe it's a TODO to actually create a dialog?

http://codereview.chromium.org/164280/diff/1/4
File chrome/browser/shell_integration.h (right):

http://codereview.chromium.org/164280/diff/1/4#newcode10
Line 10: #include "base/basictypes.h"
Move path_service.h and chrome_paths.h to the cc file?  Doesn't seem like you
need it here.  You can also forward declare string16 and not include that
header.  Also, I don't think you need basictypes.h, but will probably need
build/build_config.h.

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

http://codereview.chromium.org/164280/diff/1/5#newcode69
Line 69: file_util::ReplaceIllegalCharacters(&filename, '_');
I think you have to be more careful here.  E.g., .. in the path.  Can you share
some code from the download manager for this?

http://codereview.chromium.org/164280/diff/1/5#newcode159
Line 159: std::string output_buffer;
Can you include a URL to the documentation of this file format in a comment?

http://codereview.chromium.org/164280/diff/1/5#newcode170
Line 170: std::string app_switch(std::string("\"--app=") + url.spec() + "\"");
Nit: Maybe use StringPrintf here?

http://codereview.chromium.org/164280/diff/1/5#newcode177
Line 177: output_buffer += std::string("Name=") + final_title + "\n";
I think we need to be more careful sanitizing here.  E.g., what happens if the
page title has "\nExec=somethingevil" in it?

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

http://codereview.chromium.org/164280/diff/1/6#newcode748
Line 748: ShellIntegration::CreateDesktopShortcut(GetURL(), GetTitle());
I think this should happen on the FILE thread.

http://codereview.chromium.org/164280/diff/1/6#newcode749
Line 749: #else
Can you add an #if defined(OS_MACOSX) with a NOTIMPLEMENTED()?  I imagine they
will want to share with the linux impl.

[Paweł Hajdan Jr., 2009-08-12 16:06:07]

I think I fixed the problems, but please review carefully.

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

http://codereview.chromium.org/164280/diff/1/2#newcode1117
Line 1117: #if defined(OS_WIN) || defined(OS_LINUX)
I added the TODO, but at a different place (where it will actually need to be
fixed).

http://codereview.chromium.org/164280/diff/1/4
File chrome/browser/shell_integration.h (right):

http://codereview.chromium.org/164280/diff/1/4#newcode10
Line 10: #include "base/basictypes.h"
Oh, I just mistakenly put the headers in the .h file. Fixed.

string16 cannot be forward declared because it's a typedef.

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

http://codereview.chromium.org/164280/diff/1/5#newcode69
Line 69: file_util::ReplaceIllegalCharacters(&filename, '_');
Download manager does exactly this. But I added BaseName for safety, because the
path generation happens without user interaction.

http://codereview.chromium.org/164280/diff/1/5#newcode159
Line 159: std::string output_buffer;
Good idea. Done.

http://codereview.chromium.org/164280/diff/1/5#newcode170
Line 170: std::string app_switch(std::string("\"--app=") + url.spec() + "\"");
Done.

http://codereview.chromium.org/164280/diff/1/5#newcode177
Line 177: output_buffer += std::string("Name=") + final_title + "\n";
Added more escaping.

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

http://codereview.chromium.org/164280/diff/1/6#newcode748
Line 748: ShellIntegration::CreateDesktopShortcut(GetURL(), GetTitle());
Indeed, fixed.

http://codereview.chromium.org/164280/diff/1/6#newcode749
Line 749: #else
I don't think it's much to share. .desktop files are Linux-specific, and UI will
also have to be different (Cocoa vs GTK). They might want to share the
sanitization code though.

[tony, 2009-08-12 17:11:52]

LGTM, but please write some unittests before checking in.

http://codereview.chromium.org/164280/diff/15/20
File chrome/browser/shell_integration_linux.cc (right):

http://codereview.chromium.org/164280/diff/15/20#newcode84
Line 84: CreateDesktopShortcutTask(const GURL& url, const string16& title)
Can you write some unittests for this?  Try to include some malicious filenames
like things involving ".." and newlines.

[Paweł Hajdan Jr., 2009-08-12 20:29:27]

I added the tests. Had fun.

Could you take a look?

[tony, 2009-08-12 20:41:38]

Thanks for the tests, LGTM!

[Evan Martin, 2009-08-14 18:29:22]

This looks good to me.  CC'ing the XDG masters in case they have comments on the
parts where you're traversing env variables.

http://codereview.chromium.org/164280/diff/28/1013
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/164280/diff/28/1013#newcode748
Line 748: // TODO(phajdan.jr): Finish creating shortcuts (UI etc).
maybe stick a NOTIMPLEMENTED() in here so it doesn't get lost

[Lei Zhang, 2009-08-14 20:18:33]

http://codereview.chromium.org/164280/diff/28/1011
File chrome/browser/shell_integration_linux.cc (right):

http://codereview.chromium.org/164280/diff/28/1011#newcode50
Line 50: const char* xdg_data_home = getenv("XDG_DATA_HOME");
This will not work because almost nobody has $XDG_DATA_HOME set. If you can't
get this env variable, you should default to $HOME/.local/share. Same for
XDG_DATA_DIRS below - defaults to /usr/local/share:/usr/share. (per XDG basedir
specs)

http://codereview.chromium.org/164280/diff/28/1011#newcode65
Line 65: FilePath path = FilePath(*i).Append(template_filename);
Are you expecting to find /usr/share/google-chrome.desktop? You should be
looking in path.Append('applications') followed by path.Append('applnk').

Applnk is a legacy directory, but we seem to be putting our
google-chrome.desktop there?! I'll look into that.

[Lei Zhang, 2009-08-14 20:29:08]

On 2009/08/14 20:18:33, Lei Zhang wrote:
> Applnk is a legacy directory, but we seem to be putting our
> google-chrome.desktop there?! I'll look into that.

Ah, it's actually installed to
/usr/local/share/applications/google-chrome.desktop, the one in applnk has
"OnlyShowIn=Old;" set.

[Mike Mammarella, 2009-08-14 20:56:22]

http://codereview.chromium.org/164280/diff/28/1011
File chrome/browser/shell_integration_linux.cc (right):

http://codereview.chromium.org/164280/diff/28/1011#newcode65
Line 65: FilePath path = FilePath(*i).Append(template_filename);
On 2009/08/14 20:18:33, Lei Zhang wrote:
> Are you expecting to find /usr/share/google-chrome.desktop? You should be
> looking in path.Append('applications') followed by path.Append('applnk').
> 
> Applnk is a legacy directory, but we seem to be putting our
> google-chrome.desktop there?! I'll look into that.

I'm not sure we need to check applnk, but definitely we need to append
"applications" before template_filename.

http://codereview.chromium.org/164280/diff/28/1011#newcode207
Line 207: std::string app_switch(StringPrintf("\"--app=%s\"",
This should use switches::kApp.

[Evan Martin, 2009-08-18 14:43:08]

http://codereview.chromium.org/164280/diff/28/1011
File chrome/browser/shell_integration_linux.cc (right):

http://codereview.chromium.org/164280/diff/28/1011#newcode208
Line 208: url.spec().c_str()));
Does this make us vulnerable to some sort of command line escaping attack?  What
if url.spec() includes quotes?