Ensure ephemeral apps have dock icons on mac

Ensure ephemeral apps have dock icons on mac

App shims are now created for ephemeral apps in the user data directory
to ensure that they have dock icons. App shims are not copied to the
Applications folder as ephemeral apps should be hidden from the OS.

BUG=375027
TEST=Launch an app ephemerally and ensure it has a dock icon, but does
not appear in Spotlight search results. Install the app fully and
ensure it now has both a dock icon and appears in Spotlight.

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

[tmdiep, 2014-06-05 06:36:22]

jackhou: for web_app*
benwells: for c/b/a and c/b/e
Thanks in advance.

jack: I was able to test this in chromium; just copied over the *.dylibs to the
shim folder.

According to this article, OSX will not index stuff in the Library folder, so we
should be ok with just omitting the one in the Applications folder.
http://osxdaily.com/2012/07/21/hide-anything-from-spotlight-in-mac-os-x-with-...

[jackhou1, 2014-06-05 06:58:19]

lgtm

https://codereview.chromium.org/311293002/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_ui_util.h (right):

https://codereview.chromium.org/311293002/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_ui_util.h:27: bool
ShouldDisplayInAppLauncherIgnoreEphemeral(
Maybe ShouldDisplayInAppLauncherIncludingEphemeral would be clearer.

[benwells, 2014-06-05 08:59:23]

setn from phonr
On Jun 5, 2014 4:36 PM, <tmdiep@chromium.org> wrote:

> Reviewers: jackhou1, benwells,
>
> Message:
> jackhou: for web_app*
> benwells: for c/b/a and c/b/e
> Thanks in advance.
>
> jack: I was able to test this in chromium; just copied over the *.dylibs
> to the
> shim folder.
>
> According to this article, OSX will not index stuff in the Library folder,
> so we
> should be ok with just omitting the one in the Applications folder.
> http://osxdaily.com/2012/07/21/hide-anything-from-
> spotlight-in-mac-os-x-with-the-library-folder
>
> Description:
> Ensure ephemeral apps have dock icons on mac
>
> App shims are now created for ephemeral apps in the user data directory
> to ensure that they have dock icons. App shims are not copied to the
> Applications folder as ephemeral apps should be hidden from the OS.
>
> BUG=375027
> TEST=Launch an app ephemerally and ensure it has a dock icon, but does
> not appear in Spotlight search results. Install the app fully and
> ensure it now both has a dock icon and appears in Spotlight.
>
> Please review this at https://codereview.chromium.org/311293002/
>
> SVN Base: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+42, -21 lines):
>   M chrome/browser/apps/shortcut_manager.cc
>   M chrome/browser/extensions/extension_ui_util.h
>   M chrome/browser/extensions/extension_ui_util.cc
>   M chrome/browser/web_applications/web_app.h
>   M chrome/browser/web_applications/web_app.cc
>   M chrome/browser/web_applications/web_app_mac.mm
>
>
> Index: chrome/browser/apps/shortcut_manager.cc
> diff --git a/chrome/browser/apps/shortcut_manager.cc
> b/chrome/browser/apps/shortcut_manager.cc
> index 0da0a8d740b8b12fe5445003be986a5119c9dae1..
> 94cc91729068097e55c99fc4814ca96ee8022a7e 100644
> --- a/chrome/browser/apps/shortcut_manager.cc
> +++ b/chrome/browser/apps/shortcut_manager.cc
> @@ -24,6 +24,7 @@
>  #include "content/public/browser/browser_thread.h"
>  #include "extensions/browser/extension_registry.h"
>  #include "extensions/browser/extension_system.h"
> +#include "extensions/browser/extension_util.h"
>  #include "extensions/common/extension_set.h"
>  #include "extensions/common/one_shot_event.h"
>
> @@ -39,14 +40,20 @@ const int kCurrentAppShortcutsVersion = 0;
>  // Delay in seconds before running UpdateShortcutsForAllApps.
>  const int kUpdateShortcutsForAllAppsDelay = 10;
>
> -// Creates a shortcut for an application in the applications menu, if
> there is
> -// not already one present.
> -void CreateShortcutsInApplicationsMenu(Profile* profile,
> -                                       const Extension* app) {
> +void CreateShortcutsForApp(Profile* profile, const Extension* app) {
>    web_app::ShortcutLocations creation_locations;
> -  // Create the shortcut in the Chrome Apps subdir.
> -  creation_locations.applications_menu_location =
> -      web_app::APP_MENU_LOCATION_SUBDIR_CHROMEAPPS;
> +
> +  if (extensions::util::IsEphemeralApp(app->id(), profile)) {
> +    // Ephemeral apps should not have visible shortcuts, but may still
> require
> +    // platform-specific handling.
> +    creation_locations.hidden = true;
> +  } else {
> +    // Creates a shortcut for an app in the Chrome Apps subdir of the
> +    // applications menu, if there is not already one present.
> +    creation_locations.applications_menu_location =
> +        web_app::APP_MENU_LOCATION_SUBDIR_CHROMEAPPS;
> +  }
> +
>    web_app::CreateShortcuts(
>        web_app::SHORTCUT_CREATION_AUTOMATED, creation_locations, profile,
> app);
>  }
> @@ -120,7 +127,7 @@ void AppShortcutManager::OnExtensionWillBeInstalled(
>      web_app::UpdateAllShortcuts(
>          base::UTF8ToUTF16(old_name), profile_, extension);
>    } else {
> -    CreateShortcutsInApplicationsMenu(profile_, extension);
> +    CreateShortcutsForApp(profile_, extension);
>    }
>  }
>
> Index: chrome/browser/extensions/extension_ui_util.cc
> diff --git a/chrome/browser/extensions/extension_ui_util.cc
> b/chrome/browser/extensions/extension_ui_util.cc
> index ab8c5ac0a717f94934042b769f01546b40264cef..
> a72f56a77e34be491501ea67bebb6bcefca3578e 100644
> --- a/chrome/browser/extensions/extension_ui_util.cc
> +++ b/chrome/browser/extensions/extension_ui_util.cc
> @@ -30,9 +30,15 @@ namespace ui_util {
>
>  bool ShouldDisplayInAppLauncher(const Extension* extension,
>                                  content::BrowserContext* context) {
> +  return ShouldDisplayInAppLauncherIgnoreEphemeral(extension, context) &&
> +         !util::IsEphemeralApp(extension->id(), context);
> +}
> +
> +bool ShouldDisplayInAppLauncherIgnoreEphemeral(
> +    const Extension* extension,
> +    content::BrowserContext* context) {
>    return extension->ShouldDisplayInAppLauncher() &&
> -      !IsBlockedByPolicy(extension, context) &&
> -      !util::IsEphemeralApp(extension->id(), context);
> +         !IsBlockedByPolicy(extension, context);
>  }
>
>  bool ShouldDisplayInNewTabPage(const Extension* extension,
> Index: chrome/browser/extensions/extension_ui_util.h
> diff --git a/chrome/browser/extensions/extension_ui_util.h
> b/chrome/browser/extensions/extension_ui_util.h
> index f9add929e6359ced7a8a214a3fb45eede5389f59..
> 49f93a5f8fedfb67ef1781402604c920f24168f0 100644
> --- a/chrome/browser/extensions/extension_ui_util.h
> +++ b/chrome/browser/extensions/extension_ui_util.h
> @@ -21,6 +21,13 @@ namespace ui_util {
>  bool ShouldDisplayInAppLauncher(const Extension* extension,
>                                  content::BrowserContext* context);
>
> +// Returns true if the extension should be displayed in the app launcher.
> +// Checks whether the extension should be hidden due to policy, but
> ignores
> +// whether it is an ephemeral app.
> +bool ShouldDisplayInAppLauncherIgnoreEphemeral(
> +    const Extension* extension,
> +    content::BrowserContext* context);
> +
>  // Returns true if the extension should be displayed in the browser NTP.
>  // Checks whether the extension is an ephemeral app or should be hidden
> due to
>  // policy.
> Index: chrome/browser/web_applications/web_app.cc
> diff --git a/chrome/browser/web_applications/web_app.cc
> b/chrome/browser/web_applications/web_app.cc
> index 5397c1dc0be51039032afca514a3ee3908276404..
> 68d02f2d23c43312c84258dd41d741029b56005a 100644
> --- a/chrome/browser/web_applications/web_app.cc
> +++ b/chrome/browser/web_applications/web_app.cc
> @@ -237,11 +237,8 @@ ShortcutInfo::~ShortcutInfo() {}
>  ShortcutLocations::ShortcutLocations()
>      : on_desktop(false),
>        applications_menu_location(APP_MENU_LOCATION_NONE),
> -      in_quick_launch_bar(false)
> -#if defined(OS_POSIX)
> -      , hidden(false)
> -#endif
> -      {
> +      in_quick_launch_bar(false),
> +      hidden(false) {
>  }
>
>  void GetShortcutInfoForTab(content::WebContents* web_contents,
> @@ -298,8 +295,9 @@ void UpdateShortcutInfoAndIconForApp(const
> extensions::Extension* extension,
>  bool ShouldCreateShortcutFor(Profile* profile,
>                               const extensions::Extension* extension) {
>    return extension->is_platform_app() &&
> -      extension->location() != extensions::Manifest::COMPONENT &&
> -      extensions::ui_util::ShouldDisplayInAppLauncher(extension,
> profile);
> +         extension->location() != extensions::Manifest::COMPONENT &&
> +         extensions::ui_util::ShouldDisplayInAppLauncherIgnoreEphemeral(
> +             extension, profile);
>  }
>
>  base::FilePath GetWebAppDataDirectory(const base::FilePath& profile_path,
> Index: chrome/browser/web_applications/web_app.h
> diff --git a/chrome/browser/web_applications/web_app.h
> b/chrome/browser/web_applications/web_app.h
> index 29e730a017be5f5bcf83590397c4bb87bd4d319c..
> 074d5b7fac9576cbd25248898b2fce20fbd90633 100644
> --- a/chrome/browser/web_applications/web_app.h
> +++ b/chrome/browser/web_applications/web_app.h
> @@ -83,13 +83,12 @@ struct ShortcutLocations {
>    // implemented yet.
>    bool in_quick_launch_bar;
>
> -#if defined(OS_POSIX)
>    // For Linux, this refers to a shortcut which the system knows about
> (for
>    // the purpose of identifying windows and giving them the correct
>    // title/icon), but which does not show up in menus or search results.
>    // Ignored if applications_menu_location is not APP_MENU_LOCATION_NONE.
> +  // For Mac, app shims are not placed in locations indexed by the OS.
>    bool hidden;
> -#endif
>  };
>
>  // This encodes the cause of shortcut creation as the correct behavior in
> each
> Index: chrome/browser/web_applications/web_app_mac.mm
> diff --git a/chrome/browser/web_applications/web_app_mac.mm
> b/chrome/browser/web_applications/web_app_mac.mm
> index 9199441a640622964c01bc8349fee285ddb7e67e..
> f5ed921b1866697a942e00d2044d97d03bb07204 100644
> --- a/chrome/browser/web_applications/web_app_mac.mm
> +++ b/chrome/browser/web_applications/web_app_mac.mm
> @@ -599,8 +599,11 @@ bool WebAppShortcutCreator::CreateShortcuts(
>    } else {
>      paths.push_back(app_data_dir_);
>    }
> -  paths.push_back(applications_dir);
>
> +  if (!creation_locations.hidden)
> +    paths.push_back(applications_dir);
> +
> +  DCHECK(!paths.empty());
>    size_t success_count = CreateShortcutsIn(paths);
>    if (success_count == 0)
>      return false;
> @@ -611,7 +614,8 @@ bool WebAppShortcutCreator::CreateShortcuts(
>    if (success_count != paths.size())
>      return false;
>
> -  if (creation_locations.in_quick_launch_bar && path_to_add_to_dock) {
> +  if (creation_locations.in_quick_launch_bar && path_to_add_to_dock &&
> +      !creation_locations.hidden) {
>      switch (dock::AddIcon(path_to_add_to_dock, nil)) {
>        case dock::IconAddFailure:
>          // If adding the icon failed, instead reveal the Finder window.
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[benwells, 2014-06-05 09:27:42]

https://codereview.chromium.org/311293002/diff/40001/chrome/browser/apps/shor...
File chrome/browser/apps/shortcut_manager.cc (right):

https://codereview.chromium.org/311293002/diff/40001/chrome/browser/apps/shor...
chrome/browser/apps/shortcut_manager.cc:49: creation_locations.hidden = true;
I think this will have some effect on Linux; it might cause the app to show up
in the unity launcher search. I'm not sure about Windows.

Have you tested on those OSs?

https://codereview.chromium.org/311293002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/extension_ui_util.h (right):

https://codereview.chromium.org/311293002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_ui_util.h:27: bool
ShouldDisplayInAppLauncherIncludingEphemeral(
This needs a better name. How about CanDisplayInAppLauncher?

[tmdiep, 2014-06-06 02:49:56]

+mgiuca: Is NoDisplay=true in .desktop files supposed to hide the app from unity
search?

https://codereview.chromium.org/311293002/diff/40001/chrome/browser/apps/shor...
File chrome/browser/apps/shortcut_manager.cc (right):

https://codereview.chromium.org/311293002/diff/40001/chrome/browser/apps/shor...
chrome/browser/apps/shortcut_manager.cc:49: creation_locations.hidden = true;
On 2014/06/05 09:27:43, benwells wrote:
> I think this will have some effect on Linux; it might cause the app to show up
> in the unity launcher search. I'm not sure about Windows.
> 
> Have you tested on those OSs?

I thought the purpose of adding NoDisplay=true in the .desktop files was to hide
it from unity search, and so used the hidden flag. However, it does show up in
unity search.

I found that adding Hidden=true in .desktop does hide it from search:
http://standards.freedesktop.org/desktop-entry-spec/desktop-entry-spec-latest...

ChromeOS was fine. I forgot about Windows. Fixed now.

https://codereview.chromium.org/311293002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/extension_ui_util.h (right):

https://codereview.chromium.org/311293002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_ui_util.h:27: bool
ShouldDisplayInAppLauncherIncludingEphemeral(
On 2014/06/05 09:27:43, benwells wrote:
> This needs a better name. How about CanDisplayInAppLauncher?

Done.

[Matt Giuca, 2014-06-06 05:50:17]

https://codereview.chromium.org/311293002/diff/40001/chrome/browser/apps/shor...
File chrome/browser/apps/shortcut_manager.cc (right):

https://codereview.chromium.org/311293002/diff/40001/chrome/browser/apps/shor...
chrome/browser/apps/shortcut_manager.cc:49: creation_locations.hidden = true;
Yeah, NoDisplay=true is what you want if you want to hide an app from Unity
search / other menus (but still associate it with apps for icon and title
purposes).

I'm not sure why it is showing up in Unity search, except that Unity is pretty
buggy and not very good at realising things have changed. If you had a shortcut
without NoDisplay=true, you may have to log out and back in again (restart
Unity) to see the effect of adding NoDisplay=true.

Are you sure that setting creation_locations.hidden = true actually causes
NoDisplay=true? From my reading of
shell_integration_linux::CreateDesktopShortcut, it triggers if
applications_menu_location is APP_MENU_LOCATION_NONE, not if
creation_locations.hidden is true.

Hidden=true isn't what you want, because that is equivalent to deleting the file
(which means it won't have an icon associated with it).

[tmdiep, 2014-06-06 06:23:20]

On 2014/06/06 05:50:17, Matt Giuca wrote:
>
https://codereview.chromium.org/311293002/diff/40001/chrome/browser/apps/shor...
> chrome/browser/apps/shortcut_manager.cc:49: creation_locations.hidden = true;
> Yeah, NoDisplay=true is what you want if you want to hide an app from Unity
> search / other menus (but still associate it with apps for icon and title
> purposes).
>
> I'm not sure why it is showing up in Unity search, except that Unity is pretty
> buggy and not very good at realising things have changed. If you had a
shortcut
> without NoDisplay=true, you may have to log out and back in again (restart
> Unity) to see the effect of adding NoDisplay=true.

I suspected it was a bug. There have been previous bugs in this area, e.g.
https://bugs.launchpad.net/ubuntu/+source/unity-lens-applications/+bug/916201

The shortcut is created with NoDisplay=true, but still turns up in search.

> Are you sure that setting creation_locations.hidden = true actually causes
> NoDisplay=true? From my reading of
> shell_integration_linux::CreateDesktopShortcut, it triggers if
> applications_menu_location is APP_MENU_LOCATION_NONE, not if
> creation_locations.hidden is true.

For ephemeral apps, both the menu location is APP_MENU_LOCATION_NONE (which is
the default set in the ShortcutLocations constructor) and hidden is true. You
are correct in that APP_MENU_LOCATION_NONE causes NoDisplay=true. But hidden
also needs to be true, otherwise a shortcut would not be created at all:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/she...

This can be a bit confusing. Perhaps there should have been a
APP_MENU_LOCATION_HIDDEN enum value instead, unless there was a good reason for
the separate flag.

> Hidden=true isn't what you want, because that is equivalent to deleting the
file
> (which means it won't have an icon associated with it).

Yeah, I won't use it as a workaround in case there are unintended effects. The
icon does show up even with Hidden=true though (i.e. it isn't the chrome icon).

[Matt Giuca, 2014-06-10 00:58:39]

On 2014/06/06 06:23:20, tmdiep wrote:
> On 2014/06/06 05:50:17, Matt Giuca wrote:
> >
>
https://codereview.chromium.org/311293002/diff/40001/chrome/browser/apps/shor...
> > chrome/browser/apps/shortcut_manager.cc:49: creation_locations.hidden =
true;
> > Yeah, NoDisplay=true is what you want if you want to hide an app from Unity
> > search / other menus (but still associate it with apps for icon and title
> > purposes).
> >
> > I'm not sure why it is showing up in Unity search, except that Unity is
pretty
> > buggy and not very good at realising things have changed. If you had a
> shortcut
> > without NoDisplay=true, you may have to log out and back in again (restart
> > Unity) to see the effect of adding NoDisplay=true.
> 
> I suspected it was a bug. There have been previous bugs in this area, e.g.
> https://bugs.launchpad.net/ubuntu/+source/unity-lens-applications/+bug/916201
> 
> The shortcut is created with NoDisplay=true, but still turns up in search.

Well I guess it can't be helped (and is probably fixed in Trusty).

In general, I think we should aim to send the right signals (even if the
platform treats it wrong), then either aim to fix bugs upstream or work around
them. (Then the ball is in their court, so to speak.)

> > Are you sure that setting creation_locations.hidden = true actually causes
> > NoDisplay=true? From my reading of
> > shell_integration_linux::CreateDesktopShortcut, it triggers if
> > applications_menu_location is APP_MENU_LOCATION_NONE, not if
> > creation_locations.hidden is true.
> 
> For ephemeral apps, both the menu location is APP_MENU_LOCATION_NONE (which is
> the default set in the ShortcutLocations constructor) and hidden is true. You
> are correct in that APP_MENU_LOCATION_NONE causes NoDisplay=true. But hidden
> also needs to be true, otherwise a shortcut would not be created at all:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/she...
> 
> This can be a bit confusing. Perhaps there should have been a
> APP_MENU_LOCATION_HIDDEN enum value instead, unless there was a good reason
for
> the separate flag.

Great idea. I'm reviewing your other CL now; you should base this one on that if
you haven't already and then it will make a lot of sense.

> > Hidden=true isn't what you want, because that is equivalent to deleting the
> file
> > (which means it won't have an icon associated with it).
> 
> Yeah, I won't use it as a workaround in case there are unintended effects. The
> icon does show up even with Hidden=true though (i.e. it isn't the chrome
icon).

Sounds like Unity is ignoring Hidden. We aren't just targeting Unity, though; we
don't want to set Hidden in case another window manager uses it correctly.

[tmdiep, 2014-06-10 01:47:32]

Patch has now been rebased on:
https://codereview.chromium.org/320503004

Uses the new enum value introduced in the patch above. Hidden flag has been
removed.

[Matt Giuca, 2014-06-10 03:01:34]

Windows / Linux LGTM.

[benwells, 2014-06-10 07:55:25]

lgtm

[tmdiep, 2014-06-11 07:42:30]

The CQ bit was checked by tmdiep@chromium.org

[commit-bot: I haz the power, 2014-06-11 07:44:43]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tmdiep@chromium.org/311293002/100001

[commit-bot: I haz the power, 2014-06-11 13:47:41]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-06-11 17:15:01]

Change committed as 276436