Migrate app_host.exe shortcuts to chrome.exe shortcuts

Migrate app_host.exe shortcuts to chrome.exe shortcuts

ShellIntegration::MigrateChromiumShortcuts() now looks for app_host.exe
shortcuts and converts them to chrome.exe.

While I was there, I also added the "User Pinned/ImplicitAppShortcuts"
directory to the migration search paths and made the function generate
the correct app ids when given shortcuts with the --profile-directory flag
that had previously been messing up groupings on the taskbar.

Tests were added.

BUG=233434

[koz (OOO until 15th September), 2013-04-18 02:06:08]

lgtm

Nice work!

https://chromiumcodereview.appspot.com/14027008/diff/2001/chrome/browser/shel...
File chrome/browser/shell_integration_win.cc (right):

https://chromiumcodereview.appspot.com/14027008/diff/2001/chrome/browser/shel...
chrome/browser/shell_integration_win.cc:95: if
(command_line.HasSwitch(switches::kUserDataDir)) {
I think this reads better if you use some intermediate variables here

base::FilePath user_data_dir;
chrome::GetDefaultUserDataDirectory(&user_data_dir);
if (command_line.HasSwitch(switches::kUserDataDir))
  user_data_dir = command_line.GetSwitchValuePath(switches::kUserDataDir);

base::FilePath profile_subdir = chrome::kInitialProfile;
if (command_line.HasSwitch(switches::kProfileDirectory)
  profile_subdir = command_line.GetSwitchValuePath(switches::kProfileDirectory);

base::FilePath profile_path = user_data_dir.AppendASCII(profile_subdir);

https://chromiumcodereview.appspot.com/14027008/diff/2001/chrome/browser/shel...
File chrome/browser/shell_integration_win_unittest.cc (right):

https://chromiumcodereview.appspot.com/14027008/diff/2001/chrome/browser/shel...
chrome/browser/shell_integration_win_unittest.cc:105: // Shortcut 7 tests that a
non-default profile directory gets a non-default
nit: the above comments describe the shortcut, as opposed to what it's testing,
so maybe rephrase as:

// Shortcut 7 has a non-default profile directory and so should get a
non-default app id.

https://chromiumcodereview.appspot.com/14027008/diff/2001/chrome/browser/shel...
chrome/browser/shell_integration_win_unittest.cc:115: // Shortcut 8 tests that
app_host.exe shortcuts are migrated to chrome.exe
// Shortcut 8 points to app_host.exe and should be retargeted to chrome.exe.

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

https://codereview.chromium.org/14027008/diff/2001/chrome/browser/shell_integ...
File chrome/browser/shell_integration.h (right):

https://codereview.chromium.org/14027008/diff/2001/chrome/browser/shell_integ...
chrome/browser/shell_integration.h:180: // This method should not be called
prior to Windows 7.
I just noticed this comment. The app launcher works prior to Win7 so we may need
to split out this functionality into its own MigrateAppHostShortcuts()...

[calamity, 2013-04-18 08:24:34]

+ben for OWNERS

Could you please review all these files.

The main review target is shell_integration_win.cc.

Thanks.

https://chromiumcodereview.appspot.com/14027008/diff/2001/chrome/browser/shel...
File chrome/browser/shell_integration.h (right):

https://chromiumcodereview.appspot.com/14027008/diff/2001/chrome/browser/shel...
chrome/browser/shell_integration.h:180: // This method should not be called
prior to Windows 7.
On 2013/04/18 02:16:33, koz wrote:
> I just noticed this comment. The app launcher works prior to Win7 so we may
need
> to split out this functionality into its own MigrateAppHostShortcuts()...

Done by koz@.

https://chromiumcodereview.appspot.com/14027008/diff/2001/chrome/browser/shel...
File chrome/browser/shell_integration_win.cc (right):

https://chromiumcodereview.appspot.com/14027008/diff/2001/chrome/browser/shel...
chrome/browser/shell_integration_win.cc:95: if
(command_line.HasSwitch(switches::kUserDataDir)) {
On 2013/04/18 02:06:08, koz wrote:
> I think this reads better if you use some intermediate variables here
> 
> base::FilePath user_data_dir;
> chrome::GetDefaultUserDataDirectory(&user_data_dir);
> if (command_line.HasSwitch(switches::kUserDataDir))
>   user_data_dir = command_line.GetSwitchValuePath(switches::kUserDataDir);
> 
> base::FilePath profile_subdir = chrome::kInitialProfile;
> if (command_line.HasSwitch(switches::kProfileDirectory)
>   profile_subdir =
command_line.GetSwitchValuePath(switches::kProfileDirectory);
> 
> base::FilePath profile_path = user_data_dir.AppendASCII(profile_subdir);

Done.

https://chromiumcodereview.appspot.com/14027008/diff/2001/chrome/browser/shel...
File chrome/browser/shell_integration_win_unittest.cc (right):

https://chromiumcodereview.appspot.com/14027008/diff/2001/chrome/browser/shel...
chrome/browser/shell_integration_win_unittest.cc:105: // Shortcut 7 tests that a
non-default profile directory gets a non-default
On 2013/04/18 02:06:08, koz wrote:
> nit: the above comments describe the shortcut, as opposed to what it's
testing,
> so maybe rephrase as:
> 
> // Shortcut 7 has a non-default profile directory and so should get a
> non-default app id.

Done.

https://chromiumcodereview.appspot.com/14027008/diff/2001/chrome/browser/shel...
chrome/browser/shell_integration_win_unittest.cc:115: // Shortcut 8 tests that
app_host.exe shortcuts are migrated to chrome.exe
On 2013/04/18 02:06:08, koz wrote:
> // Shortcut 8 points to app_host.exe and should be retargeted to chrome.exe.

Done.

[Ben Goodger (Google), 2013-04-18 18:16:50]

You need a base owner for the base bits. browser lgtm.

[huangs, 2013-04-19 15:21:28]

Adding gab@ for shortcut details.

Please update BUG=.

My main concern is, why is shortcut migration done when Chrome runs, rather than
in the installer?

https://chromiumcodereview.appspot.com/14027008/diff/17001/chrome/browser/she...
File chrome/browser/shell_integration_win.cc (right):

https://chromiumcodereview.appspot.com/14027008/diff/17001/chrome/browser/she...
chrome/browser/shell_integration_win.cc:48: const int64
kMigrateAppHostShortcutsDelaySeconds = 15;
This appears unused.

https://chromiumcodereview.appspot.com/14027008/diff/17001/chrome/browser/she...
chrome/browser/shell_integration_win.cc:167: base::FilePath app_host_exe =
GetAppHostPathForInstallationLevel(
Does this code account for the case:
1. Chrome is installed in system-level,
2. (The old) App Launcher is installed at user-level?

[gab, 2013-04-19 18:34:50]

High-level comments, will review further once addressed:

As Sam said, the app_host.exe -> chrome.exe migration should be done by the
installer, not Chrome.

Also, please split that change from the appid change, they're unrelated imo.

Thanks,
Gab

PS: Feel free to ping me for further insights on shortcuts, etc.

https://chromiumcodereview.appspot.com/14027008/diff/17001/chrome/browser/she...
File chrome/browser/shell_integration.h (right):

https://chromiumcodereview.appspot.com/14027008/diff/17001/chrome/browser/she...
chrome/browser/shell_integration.h:192: static void MigrateAppHostShortcuts();
This shouldn't be done in Chrome, instead it should be done in the installer;
where the shortcuts are created. The installer already has all the functionality
to update existing shortcut properties.

Furthermore Chrome (the product not the binaries) might not be installed even if
the App Launcher is installed.

https://chromiumcodereview.appspot.com/14027008/diff/17001/chrome/browser/she...
File chrome/browser/shell_integration_win.cc (right):

https://chromiumcodereview.appspot.com/14027008/diff/17001/chrome/browser/she...
chrome/browser/shell_integration_win.cc:420: BrowserThread::PostTask(
No need for a second task here given this is already called from a callback on
the file thread in your new implementation.

https://chromiumcodereview.appspot.com/14027008/diff/17001/chrome/browser/she...
chrome/browser/shell_integration_win.cc:571: // it is fixed to work with
FilePaths with spaces.
PS: This has been fixed, you can now use
InstallUtil::ProgramCompare::EvaluatePath() to compare paths (much better than
string comparison).

Thanks :)