When app shortcuts are enabled, do a once-off creation of all shortcuts.

When app shortcuts are enabled, do a once-off creation of all shortcuts.

This adds a boolean (kAppShortcutsEnabled) to user prefs. At startup,
ShortcutManager checks the pref and sets the current value. On a
transition from false to true, ShortcutManager iterates over all
apps and creates shortcuts as if the app is being installed.

BUG=249189
TEST=Delete app shortcuts from the OS applications folder.
Start Chrome, the shortcuts should be created.
Delete the shortcuts again.
Restart Chrome, the shortcuts are not created a second time.

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

[benwells, 2013-06-20 08:54:32]

lgtm with nits

https://codereview.chromium.org/17261016/diff/1/apps/DEPS
File apps/DEPS (right):

https://codereview.chromium.org/17261016/diff/1/apps/DEPS#newcode27
apps/DEPS:27: "+components/user_prefs/pref_registry_syncable.h",
nit: move out of temporary list. This one is OK.

https://codereview.chromium.org/17261016/diff/1/apps/shortcut_manager.cc
File apps/shortcut_manager.cc (right):

https://codereview.chromium.org/17261016/diff/1/apps/shortcut_manager.cc#newc...
apps/shortcut_manager.cc:62: registrar_.Add(this,
chrome::NOTIFICATION_PROFILE_ADDED,
I think you should use a different notification like EXTENSIONS_READY.

https://codereview.chromium.org/17261016/diff/1/apps/shortcut_manager.cc#newc...
apps/shortcut_manager.cc:118: bool was_enabled =
prefs_->GetBoolean(apps::prefs::kAppShortcutsEnabled);
The preference name doesn't feel right. Can you make it HaveCreatedAppShortcuts?
I think it would be clearer. Or perhaps, if you prefer, AppShortcutsWereEnabled.

[Matt Giuca, 2013-06-20 10:19:18]

LGTM with nits.

https://codereview.chromium.org/17261016/diff/1/apps/pref_names.cc
File apps/pref_names.cc (right):

https://codereview.chromium.org/17261016/diff/1/apps/pref_names.cc#newcode33
apps/pref_names.cc:33: "apps.app_shortcuts_enabled";
Should fit on one line.

https://codereview.chromium.org/17261016/diff/1/apps/pref_names.cc#newcode33
apps/pref_names.cc:33: "apps.app_shortcuts_enabled";
"enabled" sounds like it's a choice you can turn either on or off. How about
"apps.shortcuts_have_been_created", which gives it a "one time" feel, and
follows the pattern of apps.app_launcher_has_been_enabled.

(Edit: Haha I see Ben made almost exactly the same suggestion later.)

https://codereview.chromium.org/17261016/diff/1/apps/prefs.cc
File apps/prefs.cc (right):

https://codereview.chromium.org/17261016/diff/1/apps/prefs.cc#newcode37
apps/prefs.cc:37: // Indicates whether app shortcuts are enabled.
Can you explain what this is doing? Why are you setting it to false? Is this the
default value?

https://codereview.chromium.org/17261016/diff/1/apps/shortcut_manager.cc
File apps/shortcut_manager.cc (right):

https://codereview.chromium.org/17261016/diff/1/apps/shortcut_manager.cc#newc...
apps/shortcut_manager.cc:41: bool ShouldCreateShortcutFor(const
extensions::Extension* extension) {
Thanks for doing this.

https://codereview.chromium.org/17261016/diff/1/apps/shortcut_manager.cc#newc...
apps/shortcut_manager.cc:60: // because the profile's ExtensionService is not
available when BCKSs are
BCKSs?

https://codereview.chromium.org/17261016/diff/1/apps/shortcut_manager.cc#newc...
apps/shortcut_manager.cc:121: bool is_now_enabled =
Add a comment explaining why Mac is special and that this is a temporary
measure.

https://codereview.chromium.org/17261016/diff/1/apps/shortcut_manager.cc#newc...
apps/shortcut_manager.cc:127: if (was_enabled != is_now_enabled)
I don't think you want to set this to false once it has been set to true (for
some reason). If you just put at the start of this function:

if (prefs_->GetBoolean(...))
  return;

Then you will be able to simplify a lot of this logic. Then it will be all about
turning it on, not possibly turning it on or off.

https://codereview.chromium.org/17261016/diff/1/apps/shortcut_manager.cc#newc...
apps/shortcut_manager.cc:133: ExtensionServiceInterface* extension_service =
// Create an applications menu shortcut for each app in this profile.

[jackhou1, 2013-06-20 11:03:22]

https://codereview.chromium.org/17261016/diff/1/apps/DEPS
File apps/DEPS (right):

https://codereview.chromium.org/17261016/diff/1/apps/DEPS#newcode27
apps/DEPS:27: "+components/user_prefs/pref_registry_syncable.h",
On 2013/06/20 08:54:32, benwells wrote:
> nit: move out of temporary list. This one is OK.

Done.

https://codereview.chromium.org/17261016/diff/1/apps/pref_names.cc
File apps/pref_names.cc (right):

https://codereview.chromium.org/17261016/diff/1/apps/pref_names.cc#newcode33
apps/pref_names.cc:33: "apps.app_shortcuts_enabled";
On 2013/06/20 10:19:18, Matt Giuca wrote:
> Should fit on one line.

Done.

https://codereview.chromium.org/17261016/diff/1/apps/pref_names.cc#newcode33
apps/pref_names.cc:33: "apps.app_shortcuts_enabled";
On 2013/06/20 10:19:18, Matt Giuca wrote:
> "enabled" sounds like it's a choice you can turn either on or off. How about
> "apps.shortcuts_have_been_created", which gives it a "one time" feel, and
> follows the pattern of apps.app_launcher_has_been_enabled.
> 
> (Edit: Haha I see Ben made almost exactly the same suggestion later.)

Done.

https://codereview.chromium.org/17261016/diff/1/apps/prefs.cc
File apps/prefs.cc (right):

https://codereview.chromium.org/17261016/diff/1/apps/prefs.cc#newcode37
apps/prefs.cc:37: // Indicates whether app shortcuts are enabled.
On 2013/06/20 10:19:18, Matt Giuca wrote:
> Can you explain what this is doing? Why are you setting it to false? Is this
the
> default value?

Yup. It needs to be registered before it can be used. Presumably so they're all
read from disk at the same time.

https://codereview.chromium.org/17261016/diff/1/apps/shortcut_manager.cc
File apps/shortcut_manager.cc (right):

https://codereview.chromium.org/17261016/diff/1/apps/shortcut_manager.cc#newc...
apps/shortcut_manager.cc:60: // because the profile's ExtensionService is not
available when BCKSs are
On 2013/06/20 10:19:18, Matt Giuca wrote:
> BCKSs?

BrowserContextKeyedServices. The abbreviation has been used elsewhere. But I've
changed the comment to something that makes more sense with
NOTIFICATION_EXTENSIONS_READY.

https://codereview.chromium.org/17261016/diff/1/apps/shortcut_manager.cc#newc...
apps/shortcut_manager.cc:62: registrar_.Add(this,
chrome::NOTIFICATION_PROFILE_ADDED,
On 2013/06/20 08:54:32, benwells wrote:
> I think you should use a different notification like EXTENSIONS_READY.

Done.

https://codereview.chromium.org/17261016/diff/1/apps/shortcut_manager.cc#newc...
apps/shortcut_manager.cc:118: bool was_enabled =
prefs_->GetBoolean(apps::prefs::kAppShortcutsEnabled);
On 2013/06/20 08:54:32, benwells wrote:
> The preference name doesn't feel right. Can you make it
HaveCreatedAppShortcuts?
> I think it would be clearer. Or perhaps, if you prefer,
AppShortcutsWereEnabled.

Went with Matt's suggestion: ShortcutsHaveBeenCreated.

https://codereview.chromium.org/17261016/diff/1/apps/shortcut_manager.cc#newc...
apps/shortcut_manager.cc:121: bool is_now_enabled =
On 2013/06/20 10:19:18, Matt Giuca wrote:
> Add a comment explaining why Mac is special and that this is a temporary
> measure.

Done.

https://codereview.chromium.org/17261016/diff/1/apps/shortcut_manager.cc#newc...
apps/shortcut_manager.cc:127: if (was_enabled != is_now_enabled)
On 2013/06/20 10:19:18, Matt Giuca wrote:
> I don't think you want to set this to false once it has been set to true (for
> some reason). If you just put at the start of this function:
> 
> if (prefs_->GetBoolean(...))
>   return;
> 
> Then you will be able to simplify a lot of this logic. Then it will be all
about
> turning it on, not possibly turning it on or off.

Yeah that's how I had it initially, but it needs to turn off on Mac. E.g. if the
user enables and later disables app shims, this should turn off so that when we
enable them for good, the shortcuts are created again.

https://codereview.chromium.org/17261016/diff/1/apps/shortcut_manager.cc#newc...
apps/shortcut_manager.cc:133: ExtensionServiceInterface* extension_service =
On 2013/06/20 10:19:18, Matt Giuca wrote:
> // Create an applications menu shortcut for each app in this profile.

Done.

[jackhou1, 2013-06-20 11:15:44]

mnissler, could you please review for OWNERS in chrome/browser/prefs?

[Mattias Nissler (ping if slow), 2013-06-20 16:28:51]

chrome/browser/prefs LGTM

[commit-bot: I haz the power, 2013-06-20 23:15:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jackhou@chromium.org/17261016/8003

[Matt Giuca, 2013-06-20 23:42:20]

https://codereview.chromium.org/17261016/diff/1/apps/prefs.cc
File apps/prefs.cc (right):

https://codereview.chromium.org/17261016/diff/1/apps/prefs.cc#newcode37
apps/prefs.cc:37: // Indicates whether app shortcuts are enabled.
For future reference (don't pull it out of the CQ now), I meant to add that
explanation in a comment.

[commit-bot: I haz the power, 2013-06-21 15:34:46]

Change committed as 207832