On Linux, automatically create app shortcuts on install or update.

On Linux, automatically create app shortcuts on install or update.

If app shortcuts already exist when an app is updated, it recreates the
shortcuts in whichever places they already exist.

If no app shortcut exists, a shortcut is created with NoDisplay=true. This means
that the app is not shown in system menus and search systems, but will have the
correct icon in Unity, instead of just using the Chrome logo.

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

[Matt Giuca, 2013-03-04 08:45:17]

This is to finally fix the app icon issue in Unity (at least on Gtk, Aura can
come later). (And Gnome Shell can come later too.)

Some notes:

This CL is a diff against Issue 12386077. So imagine that that has already been
committed. (This is because otherwise it won't be able to find .desktop files in
~/.local/share/applications.)

Note that if there is a .desktop file on the Desktop, but not one in the
applications menu, I will *not* create one in the applications menu. This means
that if the user later decides to delete the shortcut from the Desktop, the app
icon will stop displaying properly. The alternative to this is to explicitly
create a NoDisplay version of the .desktop file in the applications directory
regardless of whether there is one on the desktop (which is a bit more code). I
can do it if a reviewer wants me to.

[Elliot Glaysher, 2013-03-04 22:14:04]

lgtm

I don't have enough experience in this area to comment on the delete .desktop
file from the desktop edge case.

[Matt Giuca, 2013-03-05 03:08:39]

OK. Well note that this problem only shows up if you have a Desktop shortcut for
an app _before_ installing it, and then you delete the Desktop shortcut _after_
installing it. I'd say that the ideal behaviour is to create the NoDisplay=true
.desktop file when installing an app, even if there is already a shortcut on the
Desktop, but this requires a bit of refactoring (since it would involve changing
the ShortcutLocations struct to include more information).

I'd say it's a really obscure case and it shouldn't block this CL from
proceeding (since it is not a regression). So I would propose to submit this CL
and then file a new, low-priority bug about that edge case. Is that okay with
you?

[Matt Giuca, 2013-03-05 03:12:45]

Adding sky for OWNERS again.

sky: I apologize for adding you to a lot of CLs lately. It's a bit of a problem
with shell_integration being so high up the tree, there are only a handful of
OWNERS there, and you have at least some state on what I'm working on. But if
you'd prefer, I can randomly allocate other reviewers instead.

[sky, 2013-03-05 04:13:30]

I'm rubber stamping. How about one of you guys create file specific owners for
these files?

[sky, 2013-03-05 04:13:50]

LGTM

[benwells, 2013-03-05 04:20:18]

On 2013/03/05 04:13:30, sky wrote:
> I'm rubber stamping. How about one of you guys create file specific owners for
> these files?

sg. Matt, please wait for my review before landing. Sorry for being so slow.

[benwells, 2013-03-05 04:32:38]

lgtm with a few nits

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

https://codereview.chromium.org/12208085/diff/7002/chrome/browser/shell_integ...
chrome/browser/shell_integration_linux.cc:746: bool no_display =
(!creation_locations.on_desktop &&
Note for later: this feels a bit strange. It would be better to have a special
location for this.

Having a platform specific ShortcutLocation type would let us do this cleanly.

https://codereview.chromium.org/12208085/diff/7002/chrome/browser/shell_integ...
chrome/browser/shell_integration_linux.cc:790: const std::string& extension_id)
{
Nit: one parameter per line.

https://codereview.chromium.org/12208085/diff/7002/chrome/browser/shell_integ...
chrome/browser/shell_integration_linux.cc:800: const std::string& extension_id,
const base::FilePath& desktop_path) {
Nit: one parameter per line.

https://codereview.chromium.org/12208085/diff/7002/chrome/browser/shell_integ...
File chrome/browser/shell_integration_linux.h (right):

https://codereview.chromium.org/12208085/diff/7002/chrome/browser/shell_integ...
chrome/browser/shell_integration_linux.h:81: base::Environment* env, const
base::FilePath& profile_path,
Nit: One parameter per line.

https://codereview.chromium.org/12208085/diff/7002/chrome/browser/shell_integ...
chrome/browser/shell_integration_linux.h:89: const std::string& extension_id,
const base::FilePath& desktop_path);
Nit: One parameter per line.

[Matt Giuca, 2013-03-05 05:25:44]

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

https://codereview.chromium.org/12208085/diff/7002/chrome/browser/shell_integ...
chrome/browser/shell_integration_linux.cc:746: bool no_display =
(!creation_locations.on_desktop &&
On 2013/03/05 04:32:38, benwells wrote:
> Note for later: this feels a bit strange. It would be better to have a special
> location for this.
> 
> Having a platform specific ShortcutLocation type would let us do this cleanly.

Well it wouldn't necessarily need to be platform-specific. We could add a new
Boolean called "hidden" which is a platform-neutral way of saying "create a
shortcut such that the system knows about it, but it is not visible to the user
in menus or search results." That would also solve the "delete Desktop shortcut"
bug identified earlier.

Now I'm starting to like this idea. I think I'll just do it in this CL.

[Matt Giuca, 2013-03-05 05:39:50]

> Now I'm starting to like this idea. I think I'll just do it in this CL.

OK scratch that; Ben says he wants a platform-specific ShortcutLocations and not
to implement any new Booleans in the general one. Leaving as-is.

[benwells, 2013-03-05 10:07:06]

On 2013/03/05 05:39:50, Matt Giuca wrote:
> > Now I'm starting to like this idea. I think I'll just do it in this CL.
> 
> OK scratch that; Ben says he wants a platform-specific ShortcutLocations and
not
> to implement any new Booleans in the general one. Leaving as-is.

I miscommunicated in my hurry to leave today. I think your suggestion is good
and an improvement over this CL as it stands worth doing now.

In future having the shortcut location possibilities defined in a platform
specific way makes sense to me too, I think that will be a nice improvement down
the track.

[Matt Giuca, 2013-03-06 06:29:51]

OK I have refactored it so that there is an explicit 'hidden' Boolean. It will
now only create the NoDisplay shortcut if hidden is True.

This fixes the "delete Desktop shortcuts" issue.

Ben, can you please take another look?

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

https://codereview.chromium.org/12208085/diff/7002/chrome/browser/shell_integ...
chrome/browser/shell_integration_linux.cc:790: const std::string& extension_id)
{
On 2013/03/05 04:32:38, benwells wrote:
> Nit: one parameter per line.

Done.

https://codereview.chromium.org/12208085/diff/7002/chrome/browser/shell_integ...
chrome/browser/shell_integration_linux.cc:800: const std::string& extension_id,
const base::FilePath& desktop_path) {
On 2013/03/05 04:32:38, benwells wrote:
> Nit: one parameter per line.

Done.

https://codereview.chromium.org/12208085/diff/7002/chrome/browser/shell_integ...
File chrome/browser/shell_integration_linux.h (right):

https://codereview.chromium.org/12208085/diff/7002/chrome/browser/shell_integ...
chrome/browser/shell_integration_linux.h:81: base::Environment* env, const
base::FilePath& profile_path,
On 2013/03/05 04:32:38, benwells wrote:
> Nit: One parameter per line.

Done.

https://codereview.chromium.org/12208085/diff/7002/chrome/browser/shell_integ...
chrome/browser/shell_integration_linux.h:89: const std::string& extension_id,
const base::FilePath& desktop_path);
On 2013/03/05 04:32:38, benwells wrote:
> Nit: One parameter per line.

Done.

[benwells, 2013-03-06 06:36:38]

https://codereview.chromium.org/12208085/diff/24001/chrome/browser/shell_inte...
File chrome/browser/shell_integration_linux.cc (right):

https://codereview.chromium.org/12208085/diff/24001/chrome/browser/shell_inte...
chrome/browser/shell_integration_linux.cc:835: locations.hidden = true;
I don't understand this: I thought this function was finding where the shortcuts
already exist.

https://codereview.chromium.org/12208085/diff/24001/chrome/browser/shell_inte...
File chrome/browser/shell_integration_linux.h (right):

https://codereview.chromium.org/12208085/diff/24001/chrome/browser/shell_inte...
chrome/browser/shell_integration_linux.h:80: ShellIntegration::ShortcutLocations
DesktopShortcutLocations(
Can we rename this to GetExistingShortcutLocations?

[Matt Giuca, 2013-03-06 07:23:11]

https://codereview.chromium.org/12208085/diff/24001/chrome/browser/shell_inte...
File chrome/browser/shell_integration_linux.cc (right):

https://codereview.chromium.org/12208085/diff/24001/chrome/browser/shell_inte...
chrome/browser/shell_integration_linux.cc:835: locations.hidden = true;
On 2013/03/06 06:36:39, benwells wrote:
> I don't understand this: I thought this function was finding where the
shortcuts
> already exist.

Yeah now that you mention it, this does not belong here. Moved to
UpdatePlatformShortcuts.

https://codereview.chromium.org/12208085/diff/24001/chrome/browser/shell_inte...
File chrome/browser/shell_integration_linux.h (right):

https://codereview.chromium.org/12208085/diff/24001/chrome/browser/shell_inte...
chrome/browser/shell_integration_linux.h:80: ShellIntegration::ShortcutLocations
DesktopShortcutLocations(
On 2013/03/06 06:36:39, benwells wrote:
> Can we rename this to GetExistingShortcutLocations?

Done. Also moved it up near GetExistingDesktopShortcutContents, and renamed that
to GetExistingShortcutContents for consistency. (Note that that function does
not actually search the Desktop, just the applications directory, so having
"Desktop" in the name is misleading.)

[benwells, 2013-03-06 07:44:25]

https://codereview.chromium.org/12208085/diff/24003/chrome/browser/shell_inte...
File chrome/browser/shell_integration_linux.cc (right):

https://codereview.chromium.org/12208085/diff/24003/chrome/browser/shell_inte...
chrome/browser/shell_integration_linux.cc:516: // have NoDisplay=true.
If there is a shortcut that does have NoDisplay=true, should location.hidden be
set true?

This probably doesn't matter for this patch but will for subsequent changes.

[Matt Giuca, 2013-03-06 08:11:54]

https://codereview.chromium.org/12208085/diff/24003/chrome/browser/shell_inte...
File chrome/browser/shell_integration_linux.cc (right):

https://codereview.chromium.org/12208085/diff/24003/chrome/browser/shell_inte...
chrome/browser/shell_integration_linux.cc:516: // have NoDisplay=true.
Right again. Done.

[benwells, 2013-03-06 08:24:57]

lgtm

[commit-bot: I haz the power, 2013-03-06 23:59:04]

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

[commit-bot: I haz the power, 2013-03-07 01:08:39]

Retried try job too often on linux_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-03-07 06:46:50]

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

[commit-bot: I haz the power, 2013-03-07 08:49:08]

Change committed as 186655