shell_integration_linux: Follow the XDG Base Directory Specification.

shell_integration_linux: Follow the XDG Base Directory Specification.

GetDesktopShortcutTemplate now:

- Searches $XDG_DATA_HOME/applications instead of $XDG_DATA_HOME.
- If $XDG_DATA_HOME is not found, falls back to $HOME/.local/share/applications.
- No longer searches $XDG_DATA_DIRS (without /applications).
- Only searches /usr/local/share/applications and /usr/share/applications if
  $XDG_DATA_DIRS is not found.
- Searches /usr/local/share/applications before /usr/share/applications.

BUG=179751
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=186351

[benwells, 2013-03-04 03:58:26]

https://codereview.chromium.org/12386077/diff/1/chrome/browser/shell_integrat...
File chrome/browser/shell_integration_linux.cc (right):

https://codereview.chromium.org/12386077/diff/1/chrome/browser/shell_integrat...
chrome/browser/shell_integration_linux.cc:466: // Search paths as specified in
the XDG Base Directory Specification.
Could you put a link to the spec?

[benwells, 2013-03-04 04:00:18]

+mdm who can review instead of me.

I'm a noob when it comes to linux, so I have no idea about whether this change
is a good thing or not.

[Matt Giuca, 2013-03-04 04:18:26]

https://codereview.chromium.org/12386077/diff/1/chrome/browser/shell_integrat...
File chrome/browser/shell_integration_linux.cc (right):

https://codereview.chromium.org/12386077/diff/1/chrome/browser/shell_integrat...
chrome/browser/shell_integration_linux.cc:466: // Search paths as specified in
the XDG Base Directory Specification.
On 2013/03/04 03:58:26, benwells wrote:
> Could you put a link to the spec?

Done.

[sky, 2013-03-04 04:27:30]

I'm not really a good one for this. Maybe erg or derat.

[Daniel Erat, 2013-03-04 15:45:48]

LGTM with a comment, but adding Paweł since it looks like he might've written
this code originally.

https://codereview.chromium.org/12386077/diff/6001/chrome/browser/shell_integ...
File chrome/browser/shell_integration_linux.cc (right):

https://codereview.chromium.org/12386077/diff/6001/chrome/browser/shell_integ...
chrome/browser/shell_integration_linux.cc:488:
search_paths.push_back(base::FilePath("/usr/local/share"));
from reading the spec, i think that these should be pushed in the opposite order
so that /usr/local/share is searched first: "If $XDG_DATA_DIRS is either not set
or empty, a value equal to /usr/local/share/:/usr/share/ should be used."

[Mike Mammarella, 2013-03-04 18:35:54]

The main thing I'd worry about here is whether the current behavior might be
intentional and empirically-determined to work on actual distributions, even
though it's not following the standard. Paweł should probably know, as he wrote
this code originally.

Is this change motivated by broken behavior with the current code, or just
cleanup?

[Paweł Hajdan Jr., 2013-03-04 18:38:06]

LGTM with a comment.

https://codereview.chromium.org/12386077/diff/6001/chrome/browser/shell_integ...
File chrome/browser/shell_integration_linux.cc (right):

https://codereview.chromium.org/12386077/diff/6001/chrome/browser/shell_integ...
chrome/browser/shell_integration_linux.cc:488:
search_paths.push_back(base::FilePath("/usr/local/share"));
On 2013/03/04 15:45:49, Daniel Erat wrote:
> from reading the spec, i think that these should be pushed in the opposite
order
> so that /usr/local/share is searched first: "If $XDG_DATA_DIRS is either not
set
> or empty, a value equal to /usr/local/share/:/usr/share/ should be used."

Agreed.

[Matt Giuca, 2013-03-04 23:03:11]

On 2013/03/04 18:35:54, Mike Mammarella wrote:
> The main thing I'd worry about here is whether the current behavior might be
> intentional and empirically-determined to work on actual distributions, even
> though it's not following the standard. Paweł should probably know, as he
wrote
> this code originally.

I'm assuming from Pawel's reply that this was *not* empirically determined to
work on actual distributions?

Reading more carefully through the history, it seems like this was originally
the *only* case:
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/shell_integrat...

Note that the original version of this code does not look in the /applications
subdirectory at all. I don't understand how this code ever worked, unless you
were testing it on some system that had ~/.local/share/chromium-browser.desktop
or /usr/local/share/chromium-browser.desktop. It may have been that you manually
placed it there for testing purposes. Can you remember? If not, I'll assume this
code was just broken originally.

> Is this change motivated by broken behavior with the current code, or just
> cleanup?

I'm trying to re-use this code for more general purposes (finding out whether an
app shortcut already exists, when updating an app). Since app shortcuts are
created in ~/.local/share/applications, this won't work as it stands. Also see
the bug http://crbug.com/179751 for a description of where it is broken for its
original purpose (if the Chromium shortcut in ~/.local/share/applications then
you cannot create application shortcuts).

https://codereview.chromium.org/12386077/diff/6001/chrome/browser/shell_integ...
File chrome/browser/shell_integration_linux.cc (right):

https://codereview.chromium.org/12386077/diff/6001/chrome/browser/shell_integ...
chrome/browser/shell_integration_linux.cc:488:
search_paths.push_back(base::FilePath("/usr/local/share"));
On 2013/03/04 15:45:49, Daniel Erat wrote:
> from reading the spec, i think that these should be pushed in the opposite
order
> so that /usr/local/share is searched first: "If $XDG_DATA_DIRS is either not
set
> or empty, a value equal to /usr/local/share/:/usr/share/ should be used."

Done. (Good catch, absolutely true.)

[Mike Mammarella, 2013-03-04 23:05:06]

OK, LGTM then.

[commit-bot: I haz the power, 2013-03-05 02:08:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mgiuca@chromium.org/12386077/10002

[commit-bot: I haz the power, 2013-03-05 02:08:58]

Presubmit check for 12386077-10002 failed and returned exit status 1.

INFO:root:Found 2 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    chrome/browser/shell_integration_unittest.cc
    chrome/browser/shell_integration_linux.cc

[Matt Giuca, 2013-03-05 02:54:00]

Hey sky, I think I have enough consensus now. Can I have an LGTM from you as an
OWNER? Thanks.

[Matt Giuca, 2013-03-05 23:18:26]

On 2013/03/05 02:54:00, Matt Giuca wrote:
> Hey sky, I think I have enough consensus now. Can I have an LGTM from you as
an
> OWNER? Thanks.

sky: Friendly ping. (You gave an LGTM on the follow-up CL to this, perhaps you
forgot about this one?)

[sky, 2013-03-05 23:48:04]

Rubber stamp LGTM

[commit-bot: I haz the power, 2013-03-05 23:58:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mgiuca@chromium.org/12386077/10002

[commit-bot: I haz the power, 2013-03-06 04:12:37]

Change committed as 186351