Implementing unified Chrome / App Launcher flow, and migrating old stand-alone App Launcher.

Implementing unified Chrome / App Launcher flow, and migrating old stand-alone App Launcher.

Main doc: https://docs.google.com/a/google.com/document/d/1PVS-rxGSWThdkw94-K_liYFZZzGqYCRVhH2W2EcaN_8/edit#

The artifacts of the old App Launcher flow (in addition to Chrome install) are:
(A1) Add / Remove entry ("Chrome Application Launcher").
(A2) Clients and ClientState registry key {FDA71E6F-AC4C-4a00-8B70-9958A68906BF} at user-level.
(A3) AP value in registry (-applauncher).
(A4) quick-enable-application-host app command (in Chrome Binaries).
(A5) app_host.exe at user-level (alongside chrome.exe)..
(A6) Google Update at user-level (https://chromiumcodereview.appspot.com/10957016/).
(A7) Main shortcuts  to app_host.exe on {desktop, Start Menu\Chrome App Launcher, taskbar pinned}.
(A8) User-created shortcuts to app_host.exe on {desktop, Start Menu (root), taskbar pinned}.

The desired new artifacts of the unified Chrome / App Launcher (in addition to Chrome install) are:

(C1) Client key {FDA71E6F-AC4C-4a00-8B70-9958A68906BF} at same level as Chrome (specifically, for "version"); ClientState key at the user-level (for DidRun key "dr").
(C2) quick-enable-application-host app command (in Chrome Binaries).
(C3) Main shortcuts  to chrome.exe on {desktop, Start Menu\Google Chrome, taskbar pinned}.
(C4) User-created shortcuts to chrome.exe on {desktop, Start Menu\Chrome Apps, taskbar pinned}.

This CL makes the installer create (C1)-(C4).  If an old version of App Launcher is present, then (A1)-(A8) are removed.

Murkier complication to be considered (still need work; may be done) are:
- Coordinating shortcut creation / migration with chrome.exe's own version (done for creation).
- Mixed user-level, system-level flows; self-destruction flows.
- Role of Chromium and Canary.

BUG=174469

[huangs, 2013-04-25 22:44:31]

This is still in progress; in particular, I need to finish
https://chromiumcodereview.appspot.com/14287008/ first.  However, it's probably
best to get some feedback and discussion going, so I'm uploading now.

[gab, 2013-04-29 20:25:41]

Some very preliminary comments/questions. There are a LOT of things happening in
this CL (I'm also not so familiar with the products side of the installer) and I
can't say I nearly understood all of it, could this be broken down in multiple
CLs perhaps?

Cheers!
Gab

https://codereview.chromium.org/14031025/diff/1/chrome/installer/setup/instal...
File chrome/installer/setup/install.cc (left):

https://codereview.chromium.org/14031025/diff/1/chrome/installer/setup/instal...
chrome/installer/setup/install.cc:560: // Install per-user shortcuts on
user-level installs and all-users
This comment belonged with the code that moved.

https://codereview.chromium.org/14031025/diff/1/chrome/installer/setup/instal...
File chrome/installer/setup/install.cc (right):

https://codereview.chromium.org/14031025/diff/1/chrome/installer/setup/instal...
chrome/installer/setup/install.cc:291: }
This doesn't do anything :)

https://codereview.chromium.org/14031025/diff/1/chrome/installer/setup/instal...
chrome/installer/setup/install.cc:544: const base::FilePath chrome_exe(
indent

https://codereview.chromium.org/14031025/diff/1/chrome/installer/setup/instal...
chrome/installer/setup/install.cc:565: install_level,
app_launcher_shortcut_operation);
Wait, are we creating All-users app launcher shortcuts?!

https://codereview.chromium.org/14031025/diff/1/chrome/installer/setup/instal...
File chrome/installer/setup/install_worker.cc (left):

https://codereview.chromium.org/14031025/diff/1/chrome/installer/setup/instal...
chrome/installer/setup/install_worker.cc:365:
cmd_line.AppendSwitch(switches::kEnsureGoogleUpdatePresent);
Why is this being removed?

https://codereview.chromium.org/14031025/diff/1/chrome/installer/setup/instal...
File chrome/installer/setup/install_worker.cc (right):

https://codereview.chromium.org/14031025/diff/1/chrome/installer/setup/instal...
chrome/installer/setup/install_worker.cc:671: L"Google Chrome App Launcher");
Where was this set from before? Does it have to be hardcoded?

https://codereview.chromium.org/14031025/diff/1/chrome/installer/setup/instal...
chrome/installer/setup/install_worker.cc:683: base::FilePath
app_host_exe(chrome_launcher_support::GetAnyAppHostPath());
This will always prefer deleting the system-level path over the user-level path
it seems.

https://codereview.chromium.org/14031025/diff/1/chrome/installer/util/install...
File chrome/installer/util/installer_state.cc (right):

https://codereview.chromium.org/14031025/diff/1/chrome/installer/util/install...
chrome/installer/util/installer_state.cc:123: // App Launcher and Chrome are
unified.
The Chrome product (i.e. the shortcuts) is always installed when the app
launcher is installed?

https://codereview.chromium.org/14031025/diff/1/chrome/installer/util/install...
chrome/installer/util/installer_state.cc:157: if
(FindProduct(BrowserDistribution::CHROME_APP_HOST))
Why does CHROME_APP_HOST even still exist if it is unified with CHROME_BROWSER?

https://codereview.chromium.org/14031025/diff/1/chrome/installer/util/install...
File chrome/installer/util/installer_state.h (right):

https://codereview.chromium.org/14031025/diff/1/chrome/installer/util/install...
chrome/installer/util/installer_state.h:120: // A convenient method returning
the need to create shortcuts for,
for(...?),

[huangs, 2013-04-30 14:01:49]

The main division of task was to separate the shortcut migration stuff. For the
other items, since this migrates people who already have App Launcher installed
in the old flow, it's important to make this a single change -- hence this
"omnibus" CL.

https://codereview.chromium.org/14031025/diff/1/chrome/installer/setup/instal...
File chrome/installer/setup/install.cc (left):

https://codereview.chromium.org/14031025/diff/1/chrome/installer/setup/instal...
chrome/installer/setup/install.cc:560: // Install per-user shortcuts on
user-level installs and all-users
I thought this refers to CreateOrUpdateShortcuts() below??|install_level| is a
generic thing, which is needed by my code, therefore moved.

https://codereview.chromium.org/14031025/diff/1/chrome/installer/setup/instal...
File chrome/installer/setup/install.cc (right):

https://codereview.chromium.org/14031025/diff/1/chrome/installer/setup/instal...
chrome/installer/setup/install.cc:291: }
This is a stub to call code in
https://chromiumcodereview.appspot.com/14287008/

https://codereview.chromium.org/14031025/diff/1/chrome/installer/setup/instal...
chrome/installer/setup/install.cc:544: const base::FilePath chrome_exe(
On 2013/04/29 20:25:41, gab wrote:
> indent

Done.

https://codereview.chromium.org/14031025/diff/1/chrome/installer/setup/instal...
chrome/installer/setup/install.cc:565: install_level,
app_launcher_shortcut_operation);
Chrome and App Launcher are unified, so whatever Chrome shortcut does, App
Launcher shortcut should follow; this includes system-level presence.  For M28
this will need a switch (hence .create_app_launcher_shortcuts()).  For M29 this 
is planned to appear always.

https://codereview.chromium.org/14031025/diff/1/chrome/installer/setup/instal...
File chrome/installer/setup/install_worker.cc (left):

https://codereview.chromium.org/14031025/diff/1/chrome/installer/setup/instal...
chrome/installer/setup/install_worker.cc:365:
cmd_line.AppendSwitch(switches::kEnsureGoogleUpdatePresent);
This existed so an orphaned app_host.exe (i.e., it's in user-level, but depends
on system-level binaries, which got uninstalled) can invoke Google Update at the
user level to reinstall Chrome. This CL kills app_host.exe (but keeps the Omaha
key), so this switch is not needed any more.

https://codereview.chromium.org/14031025/diff/1/chrome/installer/setup/instal...
File chrome/installer/setup/install_worker.cc (right):

https://codereview.chromium.org/14031025/diff/1/chrome/installer/setup/instal...
chrome/installer/setup/install_worker.cc:671: L"Google Chrome App Launcher");
This obsolete key was living in chrome_app_host_distribution.cc, but should be
deleted. I moved it here because this routine does the main migration.

https://codereview.chromium.org/14031025/diff/1/chrome/installer/setup/instal...
chrome/installer/setup/install_worker.cc:683: base::FilePath
app_host_exe(chrome_launcher_support::GetAnyAppHostPath());
Yes, but app_host.exe have always been in user-level anyway, and it's being
nuked.

https://codereview.chromium.org/14031025/diff/1/chrome/installer/util/install...
File chrome/installer/util/installer_state.cc (right):

https://codereview.chromium.org/14031025/diff/1/chrome/installer/util/install...
chrome/installer/util/installer_state.cc:123: // App Launcher and Chrome are
unified.
This is the main premise of the Chrome / App Launcher unification plan; the
stand-alone App Launcher is going away.

https://codereview.chromium.org/14031025/diff/1/chrome/installer/util/install...
chrome/installer/util/installer_state.cc:157: if
(FindProduct(BrowserDistribution::CHROME_APP_HOST))
Because we need to create the Omaha Product / ProductClient key, so 7DA user
tracking will continue to work independently from Chrome.

https://codereview.chromium.org/14031025/diff/1/chrome/installer/util/install...
File chrome/installer/util/installer_state.h (right):

https://codereview.chromium.org/14031025/diff/1/chrome/installer/util/install...
chrome/installer/util/installer_state.h:120: // A convenient method returning
the need to create shortcuts for,
Oops. Done.

[huangs, 2013-05-14 18:56:39]

PTAL.

[gab, 2013-05-15 22:42:20]

I'll need to make more than one pass on this, but here are some comments.

I think we can split this up into separate CLs as follows (perhaps some will
need to be committed back-to-back to ensure no user ends up in an in-between
state, but still easier to review). Ease-of-reviewing is immensely superior to
ease-of-committing imo. (plus, IIRC App launcher isn't even enabled for Canary,
so that you have a full week to squeeze in every CL between 2 dev channels once
they are all approved, easy enough).

1) Deletion of old apphost installer code/replacing with new logic.
2) Migration logic (ideally bunched in as few methods as possible with TODOs
associated to a bug to delete those in M30 -- as the non-migrated folks are only
on dev-channel I understand as the code going to beta/stable in M28 doesn't
create the bad states we are migrating out of).
3) Shortcut stuff (I'd wait on https://codereview.chromium.org/13864015/ which
is putting a lot of the infrastructure in place for you).

I'd even say we could not do (2) at all if it twists the codebase too much,
leaving a few shortcuts and registry entries behind for dev-channel users is OK
(such is the risk of using dev-channel). Windows will tell them that their
shortcut points to nothing and they'll have a leftover add/remove program entry
and that'll be about it... no big deal... (what would invoking that uninstall
command do if add/remove that program command is used?)

If you can break this down even more, that's even better.

Does that sounds reasonable? As it currently is, there are too many different
things happening and it's very hard to see if I'm missing something.

Thanks!
Gab

https://codereview.chromium.org/14031025/diff/30001/chrome/installer/setup/in...
File chrome/installer/setup/install.cc (right):

https://codereview.chromium.org/14031025/diff/30001/chrome/installer/setup/in...
chrome/installer/setup/install.cc:83: case
ShellUtil::SHORTCUT_LOCATION_START_MENU_ROOT:
We will never create Chrome shortcuts at the root, please use paradigm from
https://codereview.chromium.org/13864015/

https://codereview.chromium.org/14031025/diff/30001/chrome/installer/setup/in...
chrome/installer/setup/install.cc:283: return
installer::INSTALL_SHORTCUT_CREATE_ALL;
nit: wrap in {}

https://codereview.chromium.org/14031025/diff/30001/chrome/installer/setup/in...
chrome/installer/setup/install.cc:614: install_level,
app_launcher_shortcut_operation);
I would prefer that we keep installing App Launcher shortcut at user-level only
(i.e. one shortcut per location for each user) and use Active Setup instead to
make sure every user gets them (that also takes care of the fact that global
quick launch and taskbar shortcuts don't exist).

Global shortcuts suck, and the only reason they are still used by Chrome (I
tried to get rid of them --
https://docs.google.com/a/google.com/document/d/1FjFbmKVj0BIP7xBXMs4lLgQcrc--...)
is that we can't port old installs that have them (see http://crbug.com/164655
for details).

https://codereview.chromium.org/14031025/diff/30001/chrome/installer/setup/in...
chrome/installer/setup/install.cc:749: bool InstallFromWebstore(const
std::string& app_code) {
I can't find any callers of this in codesearch... why is this in the installer?

https://codereview.chromium.org/14031025/diff/30001/chrome/installer/setup/in...
File chrome/installer/setup/install_worker.cc (left):

https://codereview.chromium.org/14031025/diff/30001/chrome/installer/setup/in...
chrome/installer/setup/install_worker.cc:365:
cmd_line.AppendSwitch(switches::kEnsureGoogleUpdatePresent);
what did this do?

https://codereview.chromium.org/14031025/diff/30001/chrome/installer/setup/in...
File chrome/installer/setup/install_worker.cc (right):

https://codereview.chromium.org/14031025/diff/30001/chrome/installer/setup/in...
chrome/installer/setup/install_worker.cc:333: void
AddQuickEnableApplicationLauncherWorkItems(
Can the app launcher even still be quick enabled? If it's part of Chrome how
does that make sense?

https://codereview.chromium.org/14031025/diff/30001/chrome/installer/setup/in...
chrome/installer/setup/install_worker.cc:664: HKEY reg_root = HKEY_CURRENT_USER;
inline below

https://codereview.chromium.org/14031025/diff/30001/chrome/installer/setup/in...
chrome/installer/setup/install_worker.cc:668: // this case it's up to the
self-destruct flow to call uninstall.
App launcher was never installed at system-level, right?

https://codereview.chromium.org/14031025/diff/30001/chrome/installer/setup/in...
chrome/installer/setup/install_worker.cc:677: base::FilePath app_host_exe(
Did app_host.exe live beside chrome.exe or in the version folder? If it lived in
the version folder than you don't need to do anything.

https://codereview.chromium.org/14031025/diff/30001/chrome/installer/setup/in...
chrome/installer/setup/install_worker.cc:1201: if
(installer_state.need_to_migrate_legacy_app_launcher()) {
Use && rather than 2 ifs

https://codereview.chromium.org/14031025/diff/30001/chrome/installer/setup/se...
File chrome/installer/setup/setup_main.cc (right):

https://codereview.chromium.org/14031025/diff/30001/chrome/installer/setup/se...
chrome/installer/setup/setup_main.cc:494: if
(!installer_state->is_multi_install()) {
&& instead of 2 ifs

https://codereview.chromium.org/14031025/diff/30001/chrome/installer/setup/se...
chrome/installer/setup/setup_main.cc:593: 
nit: remove new empty lines :)

https://codereview.chromium.org/14031025/diff/30001/chrome/installer/setup/un...
File chrome/installer/setup/uninstall.cc (right):

https://codereview.chromium.org/14031025/diff/30001/chrome/installer/setup/un...
chrome/installer/setup/uninstall.cc:360: VLOG(1) << "Deleting Start Menu root
shortcuts.";
Remove this addition as discussed.

https://codereview.chromium.org/14031025/diff/30001/chrome/installer/util/bro...
File chrome/installer/util/browser_distribution.h (right):

https://codereview.chromium.org/14031025/diff/30001/chrome/installer/util/bro...
chrome/installer/util/browser_distribution.h:57: virtual string16
GetAppShortCutFolderName();
See suggestion on https://codereview.chromium.org/13864015/

https://codereview.chromium.org/14031025/diff/30001/chrome/installer/util/chr...
File chrome/installer/util/chrome_app_host_distribution.cc (right):

https://codereview.chromium.org/14031025/diff/30001/chrome/installer/util/chr...
chrome/installer/util/chrome_app_host_distribution.cc:28: const int
kAppLauncherIconIndex = 5;
Coordinate with https://codereview.chromium.org/13864015/

https://codereview.chromium.org/14031025/diff/30001/chrome/installer/util/ins...
File chrome/installer/util/installer_state.cc (right):

https://codereview.chromium.org/14031025/diff/30001/chrome/installer/util/ins...
chrome/installer/util/installer_state.cc:134:
app_launcher_state->version().IsOlderThan("29.0.1506.0");
Make sure this is the current version of trunk the day you commit.

https://codereview.chromium.org/14031025/diff/30001/chrome/installer/util/she...
File chrome/installer/util/shell_util.cc (right):

https://codereview.chromium.org/14031025/diff/30001/chrome/installer/util/she...
chrome/installer/util/shell_util.cc:1256: // Adaptor to
FilteredBatchShortcutAction(), to process all files.
Remove comma.

https://codereview.chromium.org/14031025/diff/30001/chrome/installer/util/she...
chrome/installer/util/shell_util.cc:1397: case
SHORTCUT_LOCATION_START_MENU_ROOT:
Coordinate with https://codereview.chromium.org/13864015/

https://codereview.chromium.org/14031025/diff/30001/chrome/installer/util/she...
chrome/installer/util/shell_util.cc:2021: case
SHORTCUT_LOCATION_START_MENU_ROOT:  // For emphasis; falls through.
See https://codereview.chromium.org/13864015/ (I think that's explicit enough
with adding this extra case).

https://codereview.chromium.org/14031025/diff/30001/chrome/installer/util/she...
File chrome/installer/util/shell_util.h (right):

https://codereview.chromium.org/14031025/diff/30001/chrome/installer/util/she...
chrome/installer/util/shell_util.h:48: SHORTCUT_LOCATION_START_MENU_ROOT,
Synchronize with https://codereview.chromium.org/13864015/

[huangs, 2013-05-16 01:02:05]

I'm strongly against relying on "checking problematic intermediate stage is fine
as long as we do so in quick succession". That's asking for trouble!

Rather, I'm open to checking in things in pieces if they are code that are
refactoring / non-active. There are a couple of opportunities in this case:

- Changes in ShellUtils are largely refactoring or extensions, and can be
extracted as a separate CL (after  https://codereview.chromium.org/13864015/) is
in.

- Migration logic that's active only behind InstallerState's
need_to_migrate_legacy_app_launcher() and create_app_launcher_shortcuts() flags
(we can have them off by default).

  However, activating the migration flow should still be a single CL.

This current CL is valuable in that it allows me to test the integrated flow. If
grt@ agrees with the split, I can start extracting parts of this CL into new
CL's, remove parts of this CL that are implemented, until we're left with the
"activation" code.

Therefore this CL will drag on for a longer period of time. 

All this, of course, assumes that we're focusing on M29.

[grt (UTC plus 2), 2013-05-16 14:55:52]

Hi Sam,

I made an initial pass. I'll hold off on continuing until you have a chance to
respond to some of my questions here. Cheers.

https://codereview.chromium.org/14031025/diff/30001/chrome/installer/setup/in...
File chrome/installer/setup/install.cc (right):

https://codereview.chromium.org/14031025/diff/30001/chrome/installer/setup/in...
chrome/installer/setup/install.cc:608:
MigrateLegacyAppLauncherShortcuts(installer_state);
Are legacy shortcuts only per-user shortcuts?

https://codereview.chromium.org/14031025/diff/30001/chrome/installer/setup/in...
chrome/installer/setup/install.cc:749: bool InstallFromWebstore(const
std::string& app_code) {
On 2013/05/15 22:42:20, gab wrote:
> I can't find any callers of this in codesearch... why is this in the
installer?

It's called by setup_main.cc, which may not be indexed property for codesearch
due to its Win-only nature.

https://codereview.chromium.org/14031025/diff/30001/chrome/installer/util/chr...
File chrome/installer/util/chrome_app_host_distribution.cc (right):

https://codereview.chromium.org/14031025/diff/30001/chrome/installer/util/chr...
chrome/installer/util/chrome_app_host_distribution.cc:44: #if
defined(GOOGLE_CHROME_BUILD)
I think it's safe to replace the body of this function with:
  NOTREACHED();
  return string();
since this function only applies for distros that can be made the default. If
you want to be paranoid, then make it:
  NOTREACHED();
  L"Google Chrome App Launcher";

https://codereview.chromium.org/14031025/diff/30001/chrome/installer/util/chr...
chrome/installer/util/chrome_app_host_distribution.cc:53: #if
defined(GOOGLE_CHROME_BUILD)
Can this whole function become:
  GetSpecificDistribution(CHROME_BROWSER)->GetAppShortCutFolderName();

https://codereview.chromium.org/14031025/diff/30001/chrome/installer/util/chr...
chrome/installer/util/chrome_app_host_distribution.cc:176: return L"Google
Chrome App Launcher";
see comment above

https://codereview.chromium.org/14031025/diff/30001/chrome/installer/util/ins...
File chrome/installer/util/installer_state.cc (right):

https://codereview.chromium.org/14031025/diff/30001/chrome/installer/util/ins...
chrome/installer/util/installer_state.cc:127: // Create App Launcher shortcuts
if --app-launcher is specified.
This comment also appears on line 154. Can one of them be removed (this one?)?

https://codereview.chromium.org/14031025/diff/30001/chrome/installer/util/ins...
chrome/installer/util/installer_state.cc:128: if (prefs.is_multi_install()) {
  if (!is_uninstall && ....)?

https://codereview.chromium.org/14031025/diff/30001/chrome/installer/util/ins...
chrome/installer/util/installer_state.cc:131: system_install(),
BrowserDistribution::CHROME_APP_HOST);
I forget: is the legacy AL only ever installed at user-level? If so, does it
make sense to only perform this evaluation when !system_install()?

https://codereview.chromium.org/14031025/diff/30001/chrome/installer/util/ins...
chrome/installer/util/installer_state.cc:139: if (prefs.install_chrome() ||
prefs.install_chrome_app_launcher() ||
  if (!is_uninstall && ...)?

https://codereview.chromium.org/14031025/diff/30001/chrome/installer/util/ins...
chrome/installer/util/installer_state.cc:140:
need_to_migrate_legacy_app_launcher_) {
Does this mean that a user-level AL installation (from which Chrome had been
uninstalled) will have user-level Chrome magically appear?

https://codereview.chromium.org/14031025/diff/30001/chrome/installer/util/ins...
chrome/installer/util/installer_state.cc:154: // Create App Launcher shortcuts
if --app-launcher is specified.
Who/what will run the installer with --app-launcher?

https://codereview.chromium.org/14031025/diff/30001/chrome/installer/util/ins...
chrome/installer/util/installer_state.cc:165: if (!is_uninstall &&
is_multi_install()) {
This whole block can be compacted:
if (!is_uninstall && is_multi_install() &&
    !FindProduct(BrowserDistribution::CHROME_BINARIES) &&
    (FindProduct(BrowserDistribution::CHROME_BROWSER) ||
     FindProduct(BrowserDistribution::CHROME_FRAME) ||
     FindProduct(BrowserDistribution::CHROME_APP_HOST))) {
  ...
}

https://codereview.chromium.org/14031025/diff/30001/chrome/installer/util/ins...
File chrome/installer/util/installer_state.h (right):

https://codereview.chromium.org/14031025/diff/30001/chrome/installer/util/ins...
chrome/installer/util/installer_state.h:114: // A convenient method returning
the presence of the
convenient -> convenience (The former means the method is convenient to use,
whereas the latter means it is provided as a convenience (which is the intent
here).)

https://codereview.chromium.org/14031025/diff/30001/chrome/installer/util/ins...
chrome/installer/util/installer_state.h:120: // A convenient method returning
the presence of legacy App Launcher (with
I don't think this method is a convenience in the same was as the others above.
It insulates the caller from an implementation detail rather than just providing
the caller with a simpler way to get at a piece of data. As such, please change
the comment to something along the lines of "Returns true if the legacy App
Launcher is installed and must be migrated to the new one."

https://codereview.chromium.org/14031025/diff/30001/chrome/installer/util/ins...
chrome/installer/util/installer_state.h:126: // A convenient method returning
the need to create shortcuts for the
Similar comment as above. How about "Returns true if shortcuts for the App
Launcher must be created."

[gab, 2013-05-16 15:08:19]

On May 15, 2013 9:02 PM, <huangs@chromium.org> wrote:
>
> I'm strongly against relying on "checking problematic intermediate stage
is fine
> as long as we do so in quick succession". That's asking for trouble!

OK, if migration is such a pain than I say we just don't do it. Leaving
broken shortcuts on Dev channel is fine IMO.

I'd rather review something I fully understand than something huge only for
the sake of fixing Dev channel users (while maybe breaking other things...).

Unless it can be made simple and condensed together in the code, I say no
to migration. So far it is it affects multiple interfaces which is far from
"simple".

>
> Rather, I'm open to checking in things in pieces if they are code that are
> refactoring / non-active. There are a couple of opportunities in this
case:
>
> - Changes in ShellUtils are largely refactoring or extensions, and can be
> extracted as a separate CL (after
https://codereview.chromium.org/13864015/) is
> in.
>
> - Migration logic that's active only behind InstallerState's
> need_to_migrate_legacy_app_launcher() and create_app_launcher_shortcuts()
flags
> (we can have them off by default).
>
>   However, activating the migration flow should still be a single CL.
>
> This current CL is valuable in that it allows me to test the integrated
flow. If
> grt@ agrees with the split, I can start extracting parts of this CL into
new
> CL's, remove parts of this CL that are implemented, until we're left with
the
> "activation" code.
>
> Therefore this CL will drag on for a longer period of time.
>
> All this, of course, assumes that we're focusing on M29.
>
> https://chromiumcodereview.appspot.com/14031025/

[huangs, 2013-05-16 16:19:34]

I'm not sure why you're so negative about this -- the second half of my reply
offers a compromise and largely agrees with you.

[huangs, 2013-05-17 20:59:24]

Responding to changes before splitting this CL.

Action items:
- Wait for https://codereview.chromium.org/13864015/
- Write design doc for plan.
- Finalize on what to keep and what to discard(e.g., shortcut changes).

https://chromiumcodereview.appspot.com/14031025/diff/30001/chrome/installer/s...
File chrome/installer/setup/install.cc (right):

https://chromiumcodereview.appspot.com/14031025/diff/30001/chrome/installer/s...
chrome/installer/setup/install.cc:83: case
ShellUtil::SHORTCUT_LOCATION_START_MENU_ROOT:
Removing.

https://chromiumcodereview.appspot.com/14031025/diff/30001/chrome/installer/s...
chrome/installer/setup/install.cc:283: return
installer::INSTALL_SHORTCUT_CREATE_ALL;
On 2013/05/15 22:42:20, gab wrote:
> nit: wrap in {}

Done.

https://chromiumcodereview.appspot.com/14031025/diff/30001/chrome/installer/s...
chrome/installer/setup/install.cc:608:
MigrateLegacyAppLauncherShortcuts(installer_state);
Yes for the installer, I think so, also for user-created ones.  I made a mistake
in changing the code following (see comments).

https://chromiumcodereview.appspot.com/14031025/diff/30001/chrome/installer/s...
chrome/installer/setup/install.cc:614: install_level,
app_launcher_shortcut_operation);
"per-user shortcuts" doesn't mean what I thought it means.

Changing back to CURRENT_USER.

https://chromiumcodereview.appspot.com/14031025/diff/30001/chrome/installer/s...
chrome/installer/setup/install.cc:749: bool InstallFromWebstore(const
std::string& app_code) {
No-op.

https://chromiumcodereview.appspot.com/14031025/diff/30001/chrome/installer/s...
chrome/installer/setup/install.cc:749: bool InstallFromWebstore(const
std::string& app_code) {
No-op.

https://chromiumcodereview.appspot.com/14031025/diff/30001/chrome/installer/s...
File chrome/installer/setup/install_worker.cc (left):

https://chromiumcodereview.appspot.com/14031025/diff/30001/chrome/installer/s...
chrome/installer/setup/install_worker.cc:365:
cmd_line.AppendSwitch(switches::kEnsureGoogleUpdatePresent);
Installs Google Update at user-level (even if Chrome is at system-level)
whenever quick-enable-application-host occurs.

https://chromiumcodereview.appspot.com/14031025/diff/30001/chrome/installer/s...
File chrome/installer/setup/install_worker.cc (right):

https://chromiumcodereview.appspot.com/14031025/diff/30001/chrome/installer/s...
chrome/installer/setup/install_worker.cc:333: void
AddQuickEnableApplicationLauncherWorkItems(
This is kept for compatibility for the "non-Chrome flow".

This can be deleted as an independent task when/if the time comes (would need to
change validator and validator unittests as well).

https://chromiumcodereview.appspot.com/14031025/diff/30001/chrome/installer/s...
chrome/installer/setup/install_worker.cc:664: HKEY reg_root = HKEY_CURRENT_USER;
On 2013/05/15 22:42:20, gab wrote:
> inline below

Done.

https://chromiumcodereview.appspot.com/14031025/diff/30001/chrome/installer/s...
chrome/installer/setup/install_worker.cc:668: // this case it's up to the
self-destruct flow to call uninstall.
This is |installer_state|. The new App Launcher may be in system level.  The
purpose of this check is avoid system-level install meddling with user-level.

https://chromiumcodereview.appspot.com/14031025/diff/30001/chrome/installer/s...
chrome/installer/setup/install_worker.cc:677: base::FilePath app_host_exe(
app_host.exe lives besides chrome.exe (or by itself if user-level Chrome
self-destructed; there was code to explicitly leave app_host.exe alone).

https://chromiumcodereview.appspot.com/14031025/diff/30001/chrome/installer/s...
chrome/installer/setup/install_worker.cc:1201: if
(installer_state.need_to_migrate_legacy_app_launcher()) {
On 2013/05/15 22:42:20, gab wrote:
> Use && rather than 2 ifs

Done.

https://chromiumcodereview.appspot.com/14031025/diff/30001/chrome/installer/s...
File chrome/installer/setup/setup_main.cc (right):

https://chromiumcodereview.appspot.com/14031025/diff/30001/chrome/installer/s...
chrome/installer/setup/setup_main.cc:494: if
(!installer_state->is_multi_install()) {
On 2013/05/15 22:42:20, gab wrote:
> && instead of 2 ifs

Done.

https://chromiumcodereview.appspot.com/14031025/diff/30001/chrome/installer/s...
chrome/installer/setup/setup_main.cc:593: 
On 2013/05/15 22:42:20, gab wrote:
> nit: remove new empty lines :)

Done.

https://chromiumcodereview.appspot.com/14031025/diff/30001/chrome/installer/s...
File chrome/installer/setup/uninstall.cc (right):

https://chromiumcodereview.appspot.com/14031025/diff/30001/chrome/installer/s...
chrome/installer/setup/uninstall.cc:360: VLOG(1) << "Deleting Start Menu root
shortcuts.";
On 2013/05/15 22:42:20, gab wrote:
> Remove this addition as discussed.

Done.

https://chromiumcodereview.appspot.com/14031025/diff/30001/chrome/installer/u...
File chrome/installer/util/browser_distribution.h (right):

https://chromiumcodereview.appspot.com/14031025/diff/30001/chrome/installer/u...
chrome/installer/util/browser_distribution.h:57: virtual string16
GetAppShortCutFolderName();
Will merge into this; but keeping it for now for testing.

https://chromiumcodereview.appspot.com/14031025/diff/30001/chrome/installer/u...
File chrome/installer/util/chrome_app_host_distribution.cc (right):

https://chromiumcodereview.appspot.com/14031025/diff/30001/chrome/installer/u...
chrome/installer/util/chrome_app_host_distribution.cc:28: const int
kAppLauncherIconIndex = 5;
Will do so once I merge.  Keeping this now for testing.

https://chromiumcodereview.appspot.com/14031025/diff/30001/chrome/installer/u...
chrome/installer/util/chrome_app_host_distribution.cc:44: #if
defined(GOOGLE_CHROME_BUILD)
https://codereview.chromium.org/13864015/ will refactor a whole lot of these. 
Until I merge with the cleaned up code, I'm going to keep this for testing.

https://chromiumcodereview.appspot.com/14031025/diff/30001/chrome/installer/u...
chrome/installer/util/chrome_app_host_distribution.cc:53: #if
defined(GOOGLE_CHROME_BUILD)
Discussed with gab@, the plan for (https://codereview.chromium.org/13864015/) is
to have 2 routines, for Chrome / Chromium / Canary:

1. Given enum ({Chrome, App Launcher}), return struct containing shortcut name,
shortcut icon exe, shortcut icon index.
2. Given enum ({Chrome folder, App list folder}), return directory suffix for
Start Menu shortcuts.

However, leaving this the way it is for testing; will wipe out once we merge.

https://chromiumcodereview.appspot.com/14031025/diff/30001/chrome/installer/u...
chrome/installer/util/chrome_app_host_distribution.cc:176: return L"Google
Chrome App Launcher";
Same as above. This will be deleted. If we decided to delete the Start
Menu\Google Chrome App Launcher shortcut, this will has to be done in an ad-hoc
fashion.

https://chromiumcodereview.appspot.com/14031025/diff/30001/chrome/installer/u...
File chrome/installer/util/installer_state.cc (right):

https://chromiumcodereview.appspot.com/14031025/diff/30001/chrome/installer/u...
chrome/installer/util/installer_state.cc:127: // Create App Launcher shortcuts
if --app-launcher is specified.
Done (deleted this one).

https://chromiumcodereview.appspot.com/14031025/diff/30001/chrome/installer/u...
chrome/installer/util/installer_state.cc:128: if (prefs.is_multi_install()) {
On 2013/05/16 14:55:52, grt wrote:
>   if (!is_uninstall && ....)?

Done.

https://chromiumcodereview.appspot.com/14031025/diff/30001/chrome/installer/u...
chrome/installer/util/installer_state.cc:131: system_install(),
BrowserDistribution::CHROME_APP_HOST);
Yes, but I'm relying on app_launcher_state == NULL whenever system_install() ==
true to skip this.  But I'll spell this out explicitly.

https://chromiumcodereview.appspot.com/14031025/diff/30001/chrome/installer/u...
chrome/installer/util/installer_state.cc:134:
app_launcher_state->version().IsOlderThan("29.0.1506.0");
Yes.  In the planned change, we'll have need_to_migrate_legacy_app_launcher_ =
false always, and in the final "capstone" CL, activate the code with the current
version.

https://chromiumcodereview.appspot.com/14031025/diff/30001/chrome/installer/u...
chrome/installer/util/installer_state.cc:139: if (prefs.install_chrome() ||
prefs.install_chrome_app_launcher() ||
I'm "short-circuiting" the --chrome switch and the --app-launcher switch (only
difference being --app-launcher will also set create_app_launcher_shortcuts_). 
I'd rather not introduce more asymmetry to this.

https://chromiumcodereview.appspot.com/14031025/diff/30001/chrome/installer/u...
chrome/installer/util/installer_state.cc:140:
need_to_migrate_legacy_app_launcher_) {
That is correct; it's the product decision.

https://chromiumcodereview.appspot.com/14031025/diff/30001/chrome/installer/u...
chrome/installer/util/installer_state.cc:154: // Create App Launcher shortcuts
if --app-launcher is specified.
App command quick-enable-app-launcher, testing, and old tagged meta-installer
that would specify "--app-launcher".

https://chromiumcodereview.appspot.com/14031025/diff/30001/chrome/installer/u...
chrome/installer/util/installer_state.cc:165: if (!is_uninstall &&
is_multi_install()) {
Done, but I'll have to remember to exclude the CHROME_APP_HOST case until the
final "capstone" CL.

https://chromiumcodereview.appspot.com/14031025/diff/30001/chrome/installer/u...
File chrome/installer/util/installer_state.h (right):

https://chromiumcodereview.appspot.com/14031025/diff/30001/chrome/installer/u...
chrome/installer/util/installer_state.h:114: // A convenient method returning
the presence of the
On 2013/05/16 14:55:52, grt wrote:
> convenient -> convenience (The former means the method is convenient to use,
> whereas the latter means it is provided as a convenience (which is the intent
> here).)

Done.

https://chromiumcodereview.appspot.com/14031025/diff/30001/chrome/installer/u...
chrome/installer/util/installer_state.h:120: // A convenient method returning
the presence of legacy App Launcher (with
Done (minor rephrasing).

https://chromiumcodereview.appspot.com/14031025/diff/30001/chrome/installer/u...
chrome/installer/util/installer_state.h:126: // A convenient method returning
the need to create shortcuts for the
Done (minor rephrasing).

https://chromiumcodereview.appspot.com/14031025/diff/30001/chrome/installer/u...
File chrome/installer/util/shell_util.cc (right):

https://chromiumcodereview.appspot.com/14031025/diff/30001/chrome/installer/u...
chrome/installer/util/shell_util.cc:1256: // Adaptor to
FilteredBatchShortcutAction(), to process all files.
On 2013/05/15 22:42:20, gab wrote:
> Remove comma.

Done.

https://chromiumcodereview.appspot.com/14031025/diff/30001/chrome/installer/u...
chrome/installer/util/shell_util.cc:1397: case
SHORTCUT_LOCATION_START_MENU_ROOT:
Deleted for now.

https://chromiumcodereview.appspot.com/14031025/diff/30001/chrome/installer/u...
chrome/installer/util/shell_util.cc:2021: case
SHORTCUT_LOCATION_START_MENU_ROOT:  // For emphasis; falls through.
Deleted.

https://chromiumcodereview.appspot.com/14031025/diff/30001/chrome/installer/u...
File chrome/installer/util/shell_util.h (right):

https://chromiumcodereview.appspot.com/14031025/diff/30001/chrome/installer/u...
chrome/installer/util/shell_util.h:48: SHORTCUT_LOCATION_START_MENU_ROOT,
Done; deleted for now.

[gab, 2013-05-23 14:36:46]

Thanks for the update, will wait for decisions to be made on the design doc and
for smaller CLs to be extracted until I review further.

Cheers,
Gab

[huangs, 2013-06-04 13:50:30]

Abandoning this CL.

https://chromiumcodereview.appspot.com/14031025/diff/46001/chrome/installer/s...
File chrome/installer/setup/install.cc (right):

https://chromiumcodereview.appspot.com/14031025/diff/46001/chrome/installer/s...
chrome/installer/setup/install.cc:605:
MigrateLegacyAppLauncherShortcuts(installer_state);
Do this only for user-level installs and updates, since we rely on self-destruct
to remove artifacts on system-level Chrome install or update.

https://chromiumcodereview.appspot.com/14031025/diff/46001/chrome/installer/s...
File chrome/installer/setup/setup_main.cc (right):

https://chromiumcodereview.appspot.com/14031025/diff/46001/chrome/installer/s...
chrome/installer/setup/setup_main.cc:583: }
TODO: Add comment to state that google_update::UninstallGoogleUpdate() will be
called as a result.