Remove --show-app-list-shortcut flag and implement new app launcher enable logic.

Remove --show-app-list-shortcut flag and implement new app launcher enable logic.

The app launcher now adds itself to the start menu on the first run of chrome
past this patch.

The webstore enable of the app launcher will create shortcuts on the desktop
and pin an icon to the taskbar.

The app launcher should never be disabled after it is enabled.

BUG=233434
TBR=benwells@chromium.org

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

[calamity, 2013-04-16 08:28:03]

The honor of first review is yours.

[koz (OOO until 15th September), 2013-04-16 22:32:46]

https://chromiumcodereview.appspot.com/13940006/diff/1/apps/app_launcher.cc
File apps/app_launcher.cc (right):

https://chromiumcodereview.appspot.com/13940006/diff/1/apps/app_launcher.cc#n...
apps/app_launcher.cc:65: }
Add a NOTREACHED() if the callback isn't going to get fired. Could you add a
TODO to make this synchronous as well?

https://chromiumcodereview.appspot.com/13940006/diff/1/chrome/browser/ui/view...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

https://chromiumcodereview.appspot.com/13940006/diff/1/chrome/browser/ui/view...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:130: // upgrading.
The desktop and taskbar shortcuts are created the first time the
Seems like it would be cleaner to treat all shortcuts equally and only create
them when the user opts into the app launcher.

https://chromiumcodereview.appspot.com/13940006/diff/1/chrome/browser/ui/view...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:179: LOG(ERROR) <<
shortcut_file.value();
Remove LOG

https://chromiumcodereview.appspot.com/13940006/diff/1/chrome/browser/ui/view...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:185: success =
base::win::CreateOrUpdateShortcutLink(
success &&= x;

instead of

success = x && success;

https://chromiumcodereview.appspot.com/13940006/diff/1/chrome/browser/ui/view...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:948: LOG(ERROR) <<
l10n_util::GetStringUTF16(IDS_PRODUCT_NAME);
Remove LOG

https://chromiumcodereview.appspot.com/13940006/diff/1/chrome/browser/web_app...
File chrome/browser/web_applications/web_app_win.cc (right):

https://chromiumcodereview.appspot.com/13940006/diff/1/chrome/browser/web_app...
chrome/browser/web_applications/web_app_win.cc:336: // For Win7,
create_in_quick_launch_bar means pinning to taskbar. Use
s/create_in_quick_launch_bar/in_quick_launch_bar/

[calamity, 2013-04-18 01:33:56]

+sky for OWNERS

Can you please review chrome/browser/shell_integration.h?

Thanks

https://chromiumcodereview.appspot.com/13940006/diff/1/apps/app_launcher.cc
File apps/app_launcher.cc (right):

https://chromiumcodereview.appspot.com/13940006/diff/1/apps/app_launcher.cc#n...
apps/app_launcher.cc:65: }
On 2013/04/16 22:32:46, koz wrote:
> Add a NOTREACHED() if the callback isn't going to get fired. Could you add a
> TODO to make this synchronous as well?

Done. Comment changed in app_launcher.h.

https://chromiumcodereview.appspot.com/13940006/diff/1/chrome/browser/ui/view...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

https://chromiumcodereview.appspot.com/13940006/diff/1/chrome/browser/ui/view...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:130: // upgrading.
The desktop and taskbar shortcuts are created the first time the
On 2013/04/16 22:32:46, koz wrote:
> Seems like it would be cleaner to treat all shortcuts equally and only create
> them when the user opts into the app launcher.

As discussed, since the design for the start menu shortcut is to create on a
fresh install, creating at startup time may be justifiable on the grounds of
consistency.

Waiting for a decision from saroop@ on this.

https://chromiumcodereview.appspot.com/13940006/diff/1/chrome/browser/ui/view...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:179: LOG(ERROR) <<
shortcut_file.value();
On 2013/04/16 22:32:46, koz wrote:
> Remove LOG

Done.

https://chromiumcodereview.appspot.com/13940006/diff/1/chrome/browser/ui/view...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:185: success =
base::win::CreateOrUpdateShortcutLink(
On 2013/04/16 22:32:46, koz wrote:
> success &&= x;
> 
> instead of
> 
> success = x && success;

Done

https://chromiumcodereview.appspot.com/13940006/diff/1/chrome/browser/ui/view...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:948: LOG(ERROR) <<
l10n_util::GetStringUTF16(IDS_PRODUCT_NAME);
On 2013/04/16 22:32:46, koz wrote:
> Remove LOG

Done.

https://chromiumcodereview.appspot.com/13940006/diff/1/chrome/browser/web_app...
File chrome/browser/web_applications/web_app_win.cc (right):

https://chromiumcodereview.appspot.com/13940006/diff/1/chrome/browser/web_app...
chrome/browser/web_applications/web_app_win.cc:336: // For Win7,
create_in_quick_launch_bar means pinning to taskbar. Use
On 2013/04/16 22:32:46, koz wrote:
> s/create_in_quick_launch_bar/in_quick_launch_bar/

Done.

[calamity, 2013-04-18 02:00:38]

-sky because he's OOO
+ben for OWNERS instead

Can you please review chrome/browser/shell_integration.h?

Thanks

[Ben Goodger (Google), 2013-04-18 18:15:48]

lgtm

[koz (OOO until 15th September), 2013-04-19 01:39:35]

https://chromiumcodereview.appspot.com/13940006/diff/19001/apps/pref_names.cc
File apps/pref_names.cc (right):

https://chromiumcodereview.appspot.com/13940006/diff/19001/apps/pref_names.cc...
apps/pref_names.cc:31: const char kAppLauncherStartMenuShortcutCreated[] =
Is this still necessary?

https://chromiumcodereview.appspot.com/13940006/diff/19001/chrome/browser/ui/...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

https://chromiumcodereview.appspot.com/13940006/diff/19001/chrome/browser/ui/...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:926: // Check if the
start menu shortcut has been created before. It should only
I think all this new code needs to go.

[calamity, 2013-04-19 02:06:03]

https://chromiumcodereview.appspot.com/13940006/diff/19001/apps/pref_names.cc
File apps/pref_names.cc (right):

https://chromiumcodereview.appspot.com/13940006/diff/19001/apps/pref_names.cc...
apps/pref_names.cc:31: const char kAppLauncherStartMenuShortcutCreated[] =
On 2013/04/19 01:39:35, koz wrote:
> Is this still necessary?

Done.

https://chromiumcodereview.appspot.com/13940006/diff/19001/chrome/browser/ui/...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

https://chromiumcodereview.appspot.com/13940006/diff/19001/chrome/browser/ui/...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:926: // Check if the
start menu shortcut has been created before. It should only
On 2013/04/19 01:39:35, koz wrote:
> I think all this new code needs to go.

Done.

[koz (OOO until 15th September), 2013-04-19 03:27:16]

Awesome!

lgtm

[commit-bot: I haz the power, 2013-04-19 03:40:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/calamity@chromium.org/13940006/25001

[commit-bot: I haz the power, 2013-04-19 13:18:37]

Change committed as 195158

[gab, 2013-04-23 17:59:57]

Just saw this CL, did an overview of the shortcut changes. Please try to include
me in future shortcut CLs.

Thanks,
Gab

https://codereview.chromium.org/13940006/diff/25001/chrome/browser/ui/views/a...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

https://codereview.chromium.org/13940006/diff/25001/chrome/browser/ui/views/a...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:111: return
ShellIntegration::GetAppListAppModelIdForProfile(initial_profile_path);
FYI, the app_id will need to be suffixed with the unique user suffix used by the
Chrome shortcuts now that you will support user-level and system-level AppList
shortcuts like Chrome (I said that when this code was initially written (by
benwells@ or huangs@ I think), but was told it was only for user-level for now
and would be assessed if needed later; well later is now looks like :)!).

See ShellUtil::GetUserSpecificRegistrySuffix().

https://codereview.chromium.org/13940006/diff/25001/chrome/browser/ui/views/a...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:182: return;
Since this is a one time call, it should probably make its best-effort to create
all shortcuts (i.e. it should "continue" here instead of "return").

https://codereview.chromium.org/13940006/diff/25001/chrome/browser/ui/views/a...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:189: if (success &&
pin_to_taskbar) {
Once again, this should be a "best-effort" basis and pin to taskbar as long as
the user_data_dir shortcut itself was successfully created (could probably group
all the pin_to_taskbar related code together).

https://codereview.chromium.org/13940006/diff/25001/chrome/browser/ui/views/a...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:192: success =
base::win::TaskbarPinShortcutLink(
Why keep track of |success| all the way to the last call, but return void?

https://codereview.chromium.org/13940006/diff/25001/chrome/browser/ui/views/a...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:962:
l10n_util::GetStringUTF16(IDS_PRODUCT_NAME);
This should use BrowserDistribution::GetAppShortCutName() to reflect where the
installer puts the Chrome shortcut.

See comments above though, I'm not a fan of the GetShortcutPaths() method as a
whole, consider using ShellUtil::GetShortcutPath().

https://codereview.chromium.org/13940006/diff/25001/chrome/browser/web_applic...
File chrome/browser/web_applications/web_app_win.cc (right):

https://codereview.chromium.org/13940006/diff/25001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_win.cc:315: std::vector<base::FilePath>
GetShortcutPaths(
These locations are wrong if this is a system-level install (unless you want the
user himself to install shortcuts at user-level, but I feel this can get messy).

You probably want to use ShellUtil::GetShortcutPath() instead of re-implementing
all of this logic here.

https://codereview.chromium.org/13940006/diff/25001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_win.cc:335:
creation_locations.in_quick_launch_bar,
Why not simply make this
creation_locations.in_quick_launch_bar && base::win::GetVersion() <
base::win::VERSION_WIN7
instead of special casing it below and skipping it later...?

In fact, it's even fine to not special case at all (i.e. we do create the quick
launch Chrome shortcut on Win7+, there is simply no user facing UI for it in
Windows itself).

https://codereview.chromium.org/13940006/diff/25001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_win.cc:339: base::PATH_START :
base::DIR_APP_DATA,
There is base::DIR_USER_QUICK_LAUNCH, no need for hardcoded subdir below.

https://codereview.chromium.org/13940006/diff/25001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_win.cc:350: if (locations[i].location_id
== base::PATH_START)
Remove this (see comment above).

https://codereview.chromium.org/13940006/diff/25001/chrome/browser/web_applic...
chrome/browser/web_applications/web_app_win.cc:354: continue;
+
NOTREACHED();

[calamity, 2013-04-26 04:38:49]

Patch is here: https://codereview.chromium.org/13864015

https://chromiumcodereview.appspot.com/13940006/diff/25001/chrome/browser/ui/...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

https://chromiumcodereview.appspot.com/13940006/diff/25001/chrome/browser/ui/...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:111: return
ShellIntegration::GetAppListAppModelIdForProfile(initial_profile_path);
On 2013/04/23 17:59:57, gab wrote:
> FYI, the app_id will need to be suffixed with the unique user suffix used by
the
> Chrome shortcuts now that you will support user-level and system-level AppList
> shortcuts like Chrome (I said that when this code was initially written (by
> benwells@ or huangs@ I think), but was told it was only for user-level for now
> and would be assessed if needed later; well later is now looks like :)!).
> 
> See ShellUtil::GetUserSpecificRegistrySuffix().

I'm not sure if the correct behavior is achieved here simply by appending the
suffix. This suffixed app id is treated as one component and the truncation code
in BuildAppModelId truncates the '.' character out of the app id due to length.

The current code treats Chrome.suffix as 1 component. Isn't this suffix really
its own component?

Change to the shell_integration_win_unittest.cc fails for now due to this.

https://chromiumcodereview.appspot.com/13940006/diff/25001/chrome/browser/ui/...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:182: return;
On 2013/04/23 17:59:57, gab wrote:
> Since this is a one time call, it should probably make its best-effort to
create
> all shortcuts (i.e. it should "continue" here instead of "return").

Done.

https://chromiumcodereview.appspot.com/13940006/diff/25001/chrome/browser/ui/...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:189: if (success &&
pin_to_taskbar) {
On 2013/04/23 17:59:57, gab wrote:
> Once again, this should be a "best-effort" basis and pin to taskbar as long as
> the user_data_dir shortcut itself was successfully created (could probably
group
> all the pin_to_taskbar related code together).

Done.

https://chromiumcodereview.appspot.com/13940006/diff/25001/chrome/browser/ui/...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:192: success =
base::win::TaskbarPinShortcutLink(
On 2013/04/23 17:59:57, gab wrote:
> Why keep track of |success| all the way to the last call, but return void?

Done.

https://chromiumcodereview.appspot.com/13940006/diff/25001/chrome/browser/ui/...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:962:
l10n_util::GetStringUTF16(IDS_PRODUCT_NAME);
On 2013/04/23 17:59:57, gab wrote:
> This should use BrowserDistribution::GetAppShortCutName() to reflect where the
> installer puts the Chrome shortcut.
> 
> See comments above though, I'm not a fan of the GetShortcutPaths() method as a
> whole, consider using ShellUtil::GetShortcutPath().

Done.

https://chromiumcodereview.appspot.com/13940006/diff/25001/chrome/browser/web...
File chrome/browser/web_applications/web_app_win.cc (right):

https://chromiumcodereview.appspot.com/13940006/diff/25001/chrome/browser/web...
chrome/browser/web_applications/web_app_win.cc:315: std::vector<base::FilePath>
GetShortcutPaths(
On 2013/04/23 17:59:57, gab wrote:
> These locations are wrong if this is a system-level install (unless you want
the
> user himself to install shortcuts at user-level, but I feel this can get
messy).
> 
> You probably want to use ShellUtil::GetShortcutPath() instead of
re-implementing
> all of this logic here.
Just making sure: if Chrome is a system install, it will be able to create
system shortcuts when it runs right?

Using GetShortcutPath may cause issues for other callers of this code as we
don't always want to create shortcuts in just the "Google Chrome" folder.

I was thinking of adding SHORTCUT_LOCATION_START_MENU_CHROME_DIR which would
automatically append that subdirectory and change it so that
SHORTCUT_LOCATIONS_START_MENU wouldn't. How does that sound?

https://chromiumcodereview.appspot.com/13940006/diff/25001/chrome/browser/web...
chrome/browser/web_applications/web_app_win.cc:335:
creation_locations.in_quick_launch_bar,
On 2013/04/23 17:59:57, gab wrote:
> Why not simply make this
> creation_locations.in_quick_launch_bar && base::win::GetVersion() <
> base::win::VERSION_WIN7
> instead of special casing it below and skipping it later...?
> 
> In fact, it's even fine to not special case at all (i.e. we do create the
quick
> launch Chrome shortcut on Win7+, there is simply no user facing UI for it in
> Windows itself).

Left the special casing in. Seems messy to create unused shortcuts.

https://chromiumcodereview.appspot.com/13940006/diff/25001/chrome/browser/web...
chrome/browser/web_applications/web_app_win.cc:339: base::PATH_START :
base::DIR_APP_DATA,
On 2013/04/23 17:59:57, gab wrote:
> There is base::DIR_USER_QUICK_LAUNCH, no need for hardcoded subdir below.

Done.

https://chromiumcodereview.appspot.com/13940006/diff/25001/chrome/browser/web...
chrome/browser/web_applications/web_app_win.cc:350: if (locations[i].location_id
== base::PATH_START)
On 2013/04/23 17:59:57, gab wrote:
> Remove this (see comment above).

Done.

https://chromiumcodereview.appspot.com/13940006/diff/25001/chrome/browser/web...
chrome/browser/web_applications/web_app_win.cc:354: continue;
On 2013/04/23 17:59:57, gab wrote:
> +
> NOTREACHED();

Done.