[Installer] Cleaning up dead code for App Launcher / App Host installs.

[Installer] Cleaning up dead code for App Launcher / App Host installs.

The "shadow" App Launcher registry keys are still managed.  The code that
do this are concentrated to the new files app_launcher_installer.* to
facilitate future updates.

This CL also significantly reduces the size of setup.exe, presumably due
to removal of dependencies.

TEST=Windows only. Install and update still work as before.
- Right after fresh install, ensure reg key at user level exists:
Google\Update\Clients\{FDA71E6F-AC4C-4a00-8B70-9958A68906BF}

BUG=297647
Committed: https://crrev.com/45817557f9fda44543d513a500eb0d74dd415ee0
Cr-Commit-Position: refs/heads/master@{#313626}

[huangs, 2014-12-18 07:16:58]

huangs@chromium.org changed reviewers:
+ grt@chromium.org

[huangs, 2014-12-18 07:16:59]

Hi Greg, here's the long-awaited cleanup!  I verified the results by dumping
old/new/upgrade registry changes at various points of chrome
install/run/uninstall.  Will send out the data tomorrow morning.

[grt (UTC plus 2), 2014-12-18 19:27:37]

Hi Sam,

Thanks for taking this on. I'm really excited to have this legacy code removed.
Here's a partial review. There's more to come, but probably not today.

Cheers

https://codereview.chromium.org/811283002/diff/1/chrome/installer/mini_instal...
File chrome/installer/mini_installer/mini_installer.cc (right):

https://codereview.chromium.org/811283002/diff/1/chrome/installer/mini_instal...
chrome/installer/mini_installer/mini_installer.cc:260: // Make a last-ditch
effort to look in the Chrome state key.
nit: "client state key."

https://codereview.chromium.org/811283002/diff/1/chrome/installer/setup/app_l...
File chrome/installer/setup/app_launcher_installer.cc (right):

https://codereview.chromium.org/811283002/diff/1/chrome/installer/setup/app_l...
chrome/installer/setup/app_launcher_installer.cc:60: const
UpdatingAppRegistrationData
UpdatingAppRegistrationData should never be used for Chromium builds. How is it
that this is only called when #defined (GOOGLE_CHROME_BUILD) here and below?

https://codereview.chromium.org/811283002/diff/1/chrome/installer/setup/app_l...
chrome/installer/setup/app_launcher_installer.cc:76: // We skip deleting the
ancient app_host.exe.
how about adding a WorkItem to delete old app_host.exe (was there ever an
app_launcher.exe?) during install/update?

https://codereview.chromium.org/811283002/diff/1/chrome/installer/setup/app_l...
chrome/installer/setup/app_launcher_installer.cc:90: // Remove
"install-extension" command from App Launcher.
the installer shouldn't reach into the app launcher's GUID when operating on
sxs/canary.

https://codereview.chromium.org/811283002/diff/1/chrome/installer/setup/app_l...
chrome/installer/setup/app_launcher_installer.cc:90: // Remove
"install-extension" command from App Launcher.
install-extension -> install-application

https://codereview.chromium.org/811283002/diff/1/chrome/installer/setup/app_l...
chrome/installer/setup/app_launcher_installer.cc:95: // Remove
"install-application" command from Chrome.
install-application -> install-extension

https://codereview.chromium.org/811283002/diff/1/chrome/installer/setup/app_l...
File chrome/installer/setup/app_launcher_installer.h (right):

https://codereview.chromium.org/811283002/diff/1/chrome/installer/setup/app_l...
chrome/installer/setup/app_launcher_installer.h:25: class AppLauncherInstaller {
don't use a class as a container of static member functions
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Nonmember,_St...).
just put these functions at namespace scope.

https://codereview.chromium.org/811283002/diff/1/chrome/installer/setup/app_l...
chrome/installer/setup/app_launcher_installer.h:42: static void
RemoveLegacyAppCommandsWorkItems(
these functions are normally called AddFooWorkItems. how about Foo =
"ClearLegacyAppCommands" in this case?

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

https://codereview.chromium.org/811283002/diff/1/chrome/installer/setup/insta...
chrome/installer/setup/install_worker.cc:1274: if
(installer_state.FindProduct(BrowserDistribution::CHROME_APP_HOST)) {
this could be a good place for something like
  // Unconditionally remove the legacy app_host.exe.
  install_list->AddDeleteTreeWorkItem(
      target_path.Append(installer::kChromeAppHostExe).value(),
      temp_path.value())->set_ignore_failure(true);

possibly by way of a function call into app_launcher_installer.cc

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

https://codereview.chromium.org/811283002/diff/1/chrome/installer/setup/insta...
chrome/installer/setup/install_worker.cc:295:
AppLauncherInstaller::RemoveLegacyAppCommandsWorkItems(installer_state, list);
this isn't a product-specific item if it isn't done in the loop above. please
move to AddInstallWorkItems.

https://codereview.chromium.org/811283002/diff/1/chrome/installer/setup/insta...
chrome/installer/setup/install_worker.cc:1185: // Checking .is_multi_install()
excludes Chrome Canary and Chromium.
the presence or absence of the app launcher's Clients and ClientState keys
should be orthogonal to multi-install (Chrome should create/use it regardless of
whether it's single- or multi-install). please remove the is_multi_install()
check and do something else to avoid operating on canary or chromium.

https://codereview.chromium.org/811283002/diff/1/chrome/installer/setup/setup...
File chrome/installer/setup/setup_main.cc (right):

https://codereview.chromium.org/811283002/diff/1/chrome/installer/setup/setup...
chrome/installer/setup/setup_main.cc:390: if (binaries) {
if (binaries && products.size() == 1) {

https://codereview.chromium.org/811283002/diff/1/chrome/installer/setup/setup...
chrome/installer/setup/setup_main.cc:401: }
please test things like "mini_installer.exe --multi-install" when nothing is
installed and when multi chrome is already installed.

https://codereview.chromium.org/811283002/diff/1/chrome/installer/setup/unins...
File chrome/installer/setup/uninstall.cc (right):

https://codereview.chromium.org/811283002/diff/1/chrome/installer/setup/unins...
chrome/installer/setup/uninstall.cc:196: bool* remove_archive) {
remove_archive is now effectively unused. please remove it and refactor
accordingly (e.g., bool CheckShouldRemoveSetup(...))

https://codereview.chromium.org/811283002/diff/1/chrome/installer/setup/unins...
chrome/installer/setup/uninstall.cc:1250:
AppLauncherInstaller::OnUninstall(reg_root);
this should be done in the "if (is_chrome)" block that starts on line 1120,
unless this is a chromium build or canary/sxs (don't use multi-install to figure
this out).

https://codereview.chromium.org/811283002/diff/1/chrome/installer/util/channe...
File chrome/installer/util/channel_info.cc (left):

https://codereview.chromium.org/811283002/diff/1/chrome/installer/util/channe...
chrome/installer/util/channel_info.cc:19: const wchar_t kModAppHostDeprecated[]
= L"-apphost";
we're still getting update checks containing -apphost and -applauncher. please
add to the cleanup code that runs on each install/update a call to remove both
values.

[huangs, 2015-01-05 06:01:13]

Happy New Year!  PTAL.

For the ChannelInfo changes, I removed "-apphost" and "-applauncher" by direct
string manipulation.  The code is isolated into
util\app_launcher_installer_util.* (also added tests into ChannelInfo). 
Alternative is to actually parse the parameter, but I think it's more trouble
than its worth.

For the "mini_installer.exe --multi-install" issue, crashes happen but due to 2
part of code (one is due to my change in, other is missing NULL check).  Also
updated the call site.

I also noticed that for non-multi Chrome install, my new code will add the App
Launcher key "{FDA...}" whereas the old installer won't.  Should I preserve the
old behaviour?

https://codereview.chromium.org/811283002/diff/1/chrome/installer/mini_instal...
File chrome/installer/mini_installer/mini_installer.cc (right):

https://codereview.chromium.org/811283002/diff/1/chrome/installer/mini_instal...
chrome/installer/mini_installer/mini_installer.cc:260: // Make a last-ditch
effort to look in the Chrome state key.
On 2014/12/18 19:27:36, grt wrote:
> nit: "client state key."

Done.

https://codereview.chromium.org/811283002/diff/1/chrome/installer/setup/app_l...
File chrome/installer/setup/app_launcher_installer.cc (right):

https://codereview.chromium.org/811283002/diff/1/chrome/installer/setup/app_l...
chrome/installer/setup/app_launcher_installer.cc:60: const
UpdatingAppRegistrationData
Wrapping the whole thing with #if defined(GOOGLE_CHROME_BUILD).

https://codereview.chromium.org/811283002/diff/1/chrome/installer/setup/app_l...
chrome/installer/setup/app_launcher_installer.cc:76: // We skip deleting the
ancient app_host.exe.
On 2014/12/18 19:27:36, grt (no reviews until Jan 6) wrote:
> how about adding a WorkItem to delete old app_host.exe (was there ever an
> app_launcher.exe?) during install/update?

Done.

https://codereview.chromium.org/811283002/diff/1/chrome/installer/setup/app_l...
chrome/installer/setup/app_launcher_installer.cc:90: // Remove
"install-extension" command from App Launcher.
On 2014/12/18 19:27:36, grt wrote:
> install-extension -> install-application

Done.

https://codereview.chromium.org/811283002/diff/1/chrome/installer/setup/app_l...
chrome/installer/setup/app_launcher_installer.cc:90: // Remove
"install-extension" command from App Launcher.
On 2014/12/18 19:27:36, grt (no reviews until Jan 6) wrote:
> the installer shouldn't reach into the app launcher's GUID when operating on
> sxs/canary.

Done.

https://codereview.chromium.org/811283002/diff/1/chrome/installer/setup/app_l...
chrome/installer/setup/app_launcher_installer.cc:95: // Remove
"install-application" command from Chrome.
On 2014/12/18 19:27:36, grt wrote:
> install-application -> install-extension

Done.

https://codereview.chromium.org/811283002/diff/1/chrome/installer/setup/app_l...
File chrome/installer/setup/app_launcher_installer.h (right):

https://codereview.chromium.org/811283002/diff/1/chrome/installer/setup/app_l...
chrome/installer/setup/app_launcher_installer.h:25: class AppLauncherInstaller {
Replaced with namespace app_launcher_installer.

https://codereview.chromium.org/811283002/diff/1/chrome/installer/setup/app_l...
chrome/installer/setup/app_launcher_installer.h:42: static void
RemoveLegacyAppCommandsWorkItems(
Renamed to AddRemoveLegacyAppCommandsWorkItems() and updated comments.

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

https://codereview.chromium.org/811283002/diff/1/chrome/installer/setup/insta...
chrome/installer/setup/install_worker.cc:1274: if
(installer_state.FindProduct(BrowserDistribution::CHROME_APP_HOST)) {
On 2014/12/18 19:27:36, grt (no reviews until Jan 6) wrote:
> this could be a good place for something like
>   // Unconditionally remove the legacy app_host.exe.
>   install_list->AddDeleteTreeWorkItem(
>       target_path.Append(installer::kChromeAppHostExe).value(),
>       temp_path.value())->set_ignore_failure(true);
> 
> possibly by way of a function call into app_launcher_installer.cc

Done.

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

https://codereview.chromium.org/811283002/diff/1/chrome/installer/setup/insta...
chrome/installer/setup/install_worker.cc:295:
AppLauncherInstaller::RemoveLegacyAppCommandsWorkItems(installer_state, list);
Moved.

https://codereview.chromium.org/811283002/diff/1/chrome/installer/setup/insta...
chrome/installer/setup/install_worker.cc:1185: // Checking .is_multi_install()
excludes Chrome Canary and Chromium.
On 2014/12/18 19:27:36, grt (no reviews until Jan 6) wrote:
> the presence or absence of the app launcher's Clients and ClientState keys
> should be orthogonal to multi-install (Chrome should create/use it regardless
of
> whether it's single- or multi-install). please remove the is_multi_install()
> check and do something else to avoid operating on canary or chromium.

Done.

https://codereview.chromium.org/811283002/diff/1/chrome/installer/setup/setup...
File chrome/installer/setup/setup_main.cc (right):

https://codereview.chromium.org/811283002/diff/1/chrome/installer/setup/setup...
chrome/installer/setup/setup_main.cc:390: if (binaries) {
Moot owing to the next comment (need else case for product.size() == 1).

https://codereview.chromium.org/811283002/diff/1/chrome/installer/setup/setup...
chrome/installer/setup/setup_main.cc:401: }
My change caused fresh install "mini_installer.exe --multi-install" with to
crash because:
- InstallerState::Initialize() too eagerly added CHROME_BINARIES. Fixed this by
also requiring Chrome to be specified.
- UninstallBinariesIfUnused() thinks Binaries are available, causing NULL
pointer ref on |binaries_state|. Fixed by adding NULL check.

If Chrome is already installed then this seems to behave same as before.

For this place, added "else" case, also returning "true".

Note: code here were added from:
https://chromiumcodereview.appspot.com/10829298
https://chromiumcodereview.appspot.com/10805043

https://codereview.chromium.org/811283002/diff/1/chrome/installer/setup/unins...
File chrome/installer/setup/uninstall.cc (right):

https://codereview.chromium.org/811283002/diff/1/chrome/installer/setup/unins...
chrome/installer/setup/uninstall.cc:196: bool* remove_archive) {
Done, updated comments and returning via "return".

https://codereview.chromium.org/811283002/diff/1/chrome/installer/setup/unins...
chrome/installer/setup/uninstall.cc:1250:
AppLauncherInstaller::OnUninstall(reg_root);
On 2014/12/18 19:27:36, grt (no reviews until Jan 6) wrote:
> this should be done in the "if (is_chrome)" block that starts on line 1120,
> unless this is a chromium build or canary/sxs (don't use multi-install to
figure
> this out).

Done.

https://codereview.chromium.org/811283002/diff/1/chrome/installer/util/channe...
File chrome/installer/util/channel_info.cc (left):

https://codereview.chromium.org/811283002/diff/1/chrome/installer/util/channe...
chrome/installer/util/channel_info.cc:19: const wchar_t kModAppHostDeprecated[]
= L"-apphost";
Updated, with refactoring to ChannelInfo:
- Replaced all std::wstring usage with base::string16.
- Adding static ChannelInfo::AuditValue() (which contains the call to remove
-apphost and -applauncher), which gets called by Initialize() and set_value().
- Moved set_value() etc. from .h to .cc.
- Updated unit tests.

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

https://codereview.chromium.org/811283002/diff/1/chrome/installer/util/instal...
chrome/installer/util/installer_state.cc:122: Product* p =
AddProductFromPreferences(
This is problematic, since we can't install binaries by itself.

[huangs, 2015-01-08 18:45:27]

Ping grt@. Thanks!

[grt (UTC plus 2), 2015-01-08 18:59:09]

On 2015/01/08 18:45:27, huangs1 wrote:
> Ping grt@. Thanks!

I'm still here (but home sick)! Hoping to look at this again later today or
tomorrow. Apologies for the delay.

[grt (UTC plus 2), 2015-01-08 21:41:49]

a smattering of comments. still working on it.

https://codereview.chromium.org/811283002/diff/1/chrome/installer/setup/setup...
File chrome/installer/setup/setup_main.cc (right):

https://codereview.chromium.org/811283002/diff/1/chrome/installer/setup/setup...
chrome/installer/setup/setup_main.cc:401: }
On 2015/01/05 06:01:12, huangs1 wrote:
> My change caused fresh install "mini_installer.exe --multi-install" with to
> crash because:
> - InstallerState::Initialize() too eagerly added CHROME_BINARIES. Fixed this
by
> also requiring Chrome to be specified.
> - UninstallBinariesIfUnused() thinks Binaries are available, causing NULL
> pointer ref on |binaries_state|. Fixed by adding NULL check.
> 
> If Chrome is already installed then this seems to behave same as before.
> 
> For this place, added "else" case, also returning "true".
> 
> Note: code here were added from:
> https://chromiumcodereview.appspot.com/10829298
> https://chromiumcodereview.appspot.com/10805043

hmm. --multi-install by itself when nothing else is installed should be a
failure of some sort. it's unsupported. special error strings aren't needed.
Maybe just return with *status = installer::UNSUPPORTED_OPTION in that case.

https://codereview.chromium.org/811283002/diff/20001/chrome/installer/setup/a...
File chrome/installer/setup/app_launcher_installer.cc (right):

https://codereview.chromium.org/811283002/diff/20001/chrome/installer/setup/a...
chrome/installer/setup/app_launcher_installer.cc:1: // Copyright (c) 2014 The
Chromium Authors. All rights reserved.
(c)

https://codereview.chromium.org/811283002/diff/20001/chrome/installer/setup/a...
chrome/installer/setup/app_launcher_installer.cc:24: using base::Version;
remove this and use base:: explicitly where needed (only one spot).

https://codereview.chromium.org/811283002/diff/20001/chrome/installer/setup/a...
chrome/installer/setup/app_launcher_installer.cc:42: return
installer::GetLocalizedString(IDS_PRODUCT_APP_LAUNCHER_NAME_BASE);
installer:: not needed here or elsewhere in this file

https://codereview.chromium.org/811283002/diff/20001/chrome/installer/setup/a...
chrome/installer/setup/app_launcher_installer.cc:90: for
(Products::const_iterator it = products.begin(); it < products.end();
use a range-based for loop here (http://chromium-cpp.appspot.com/):
  for (auto *p : installer_state.products()) {
    if (p->is_chrome())
      ...
  }

https://codereview.chromium.org/811283002/diff/20001/chrome/installer/setup/a...
chrome/installer/setup/app_launcher_installer.cc:107: // Remove
"quick-enable-application-host" command from Binaries.
query-eula-acceptance can be removed as well, no?

https://codereview.chromium.org/811283002/diff/20001/chrome/installer/setup/a...
chrome/installer/setup/app_launcher_installer.cc:121: reg_root,
app_launcher_reg_data.GetVersionKey(), WorkItem::kWow64Default);
WorkItem::kWow64Default -> KEY_WOW64_32KEY

https://codereview.chromium.org/811283002/diff/20001/chrome/installer/setup/a...
chrome/installer/setup/app_launcher_installer.cc:127: #endif  //
defined(GOOGLE_CHROME_BUILD)
my primitive codesearch tells me that
#endif  // FOO
is more prevalent in the codebase than
#endif  // defined(FOO)

please use the former style

https://codereview.chromium.org/811283002/diff/20001/chrome/installer/setup/a...
File chrome/installer/setup/app_launcher_installer.h (right):

https://codereview.chromium.org/811283002/diff/20001/chrome/installer/setup/a...
chrome/installer/setup/app_launcher_installer.h:1: // Copyright (c) 2014 The
Chromium Authors. All rights reserved.
nit: no (c) in the latest copyright template
(http://www.chromium.org/developers/coding-style#TOC-File-headers)

https://codereview.chromium.org/811283002/diff/20001/chrome/installer/setup/a...
chrome/installer/setup/app_launcher_installer.h:5: // The App Launcher is an
adjunct product of the Google Chrome Browser, but it
nit: "the Google Chrome Browser" -> "Google Chrome"

https://codereview.chromium.org/811283002/diff/20001/chrome/installer/setup/a...
chrome/installer/setup/app_launcher_installer.h:29: namespace
app_launcher_installer {
nix this extra namespace: the installer namespace is fine for this.

https://codereview.chromium.org/811283002/diff/20001/chrome/installer/setup/a...
chrome/installer/setup/app_launcher_installer.h:52: // Called during uninstall,
to remove "shadow" registry key for App Launcher.
Document what the function does rather than when it's called. Also, please avoid
"shadow" throughout. It's a subjective term that's irrelevant to the
implementation. The important technical detail is that the app launcher has its
own registration with Google Update that is kept in sync with Google Chrome's.

https://codereview.chromium.org/811283002/diff/20001/chrome/installer/setup/a...
chrome/installer/setup/app_launcher_installer.h:54: void RemoveShadowKey(HKEY
reg_root);
RemoveShadowKey -> RemoveAppLauncherVersionKey, and move this so that it
immediately follows the correspoding Add function above.

https://codereview.chromium.org/811283002/diff/20001/chrome/installer/setup/i...
File chrome/installer/setup/install_worker.cc (right):

https://codereview.chromium.org/811283002/diff/20001/chrome/installer/setup/i...
chrome/installer/setup/install_worker.cc:1150: #if defined(GOOGLE_CHROME_BUILD)
these two conditions sprinkled here and there is a bit off-putting. is there a
better way to do this? i'll try to come back to this with a suggestion after
reviewing the rest of the CL.

https://codereview.chromium.org/811283002/diff/20001/chrome/installer/setup/i...
chrome/installer/setup/install_worker.cc:1198: if
(installer_state.FindProduct(BrowserDistribution::CHROME_BROWSER)) {
the is_chrome() block of AddProductSpecificWorkItems seems like a good place for
this, no?

https://codereview.chromium.org/811283002/diff/20001/chrome/installer/setup/i...
chrome/installer/setup/install_worker.cc:1203: #endif
#endif  // GOOGLE_CHROME_BUILD
here and elsewhere

https://codereview.chromium.org/811283002/diff/20001/chrome/installer/setup/s...
File chrome/installer/setup/setup_main.cc (right):

https://codereview.chromium.org/811283002/diff/20001/chrome/installer/setup/s...
chrome/installer/setup/setup_main.cc:682: if (!binaries_state)
remove this.

[grt (UTC plus 2), 2015-01-09 18:57:04]

a few more comments

https://codereview.chromium.org/811283002/diff/20001/chrome/installer/setup/u...
File chrome/installer/setup/uninstall.cc (right):

https://codereview.chromium.org/811283002/diff/20001/chrome/installer/setup/u...
chrome/installer/setup/uninstall.cc:1148: if
(!InstallUtil::IsChromeSxSProcess()) {
nit: no braces

https://codereview.chromium.org/811283002/diff/20001/chrome/installer/setup/u...
chrome/installer/setup/uninstall.cc:1149:
app_launcher_installer::RemoveShadowKey(reg_root);
i think this belongs in the is_chrome block below since the AppLauncher dr
tracking is specific to the browser.

https://codereview.chromium.org/811283002/diff/20001/chrome/installer/util/ap...
File chrome/installer/util/app_launcher_installer_util.h (right):

https://codereview.chromium.org/811283002/diff/20001/chrome/installer/util/ap...
chrome/installer/util/app_launcher_installer_util.h:13: namespace
app_launcher_installer {
nix this namespace, too

https://codereview.chromium.org/811283002/diff/20001/chrome/installer/util/ch...
File chrome/installer/util/channel_info.cc (right):

https://codereview.chromium.org/811283002/diff/20001/chrome/installer/util/ch...
chrome/installer/util/channel_info.cc:177: AuditValue(&value_);
i don't like how this buries magic that's specific to Chrome into a utility
class. this also breaks the ChannelInfo API. please leave ChannelInfo alone
(except for the string16 conversion, which is useful on its own) and rather
change ChromeBrowserOperations::SetChannelFlags and/or
ChromeBinariesOperations::SetChannelFlags to remove the old flags.

you could note in channel_info.h doc comments that some methods are now
deprecated.

[huangs, 2015-01-18 01:18:24]

Updated, with some notes:
- Didn't remove app_launcher_installer yet, will do so if you insist.
- Didn't remove null check in setup_main.cc since without it, "chrome.exe
--multi-install" would crash due to null reference.
- Realized eula_util.* is now unused can be removed, but I want to do this in a
separate "anti-12035043" CL.

PTAL.

https://chromiumcodereview.appspot.com/811283002/diff/20001/chrome/installer/...
File chrome/installer/setup/app_launcher_installer.cc (right):

https://chromiumcodereview.appspot.com/811283002/diff/20001/chrome/installer/...
chrome/installer/setup/app_launcher_installer.cc:1: // Copyright (c) 2014 The
Chromium Authors. All rights reserved.
On 2015/01/08 21:41:48, grt wrote:
> (c)

Done.

https://chromiumcodereview.appspot.com/811283002/diff/20001/chrome/installer/...
chrome/installer/setup/app_launcher_installer.cc:24: using base::Version;
On 2015/01/08 21:41:49, grt wrote:
> remove this and use base:: explicitly where needed (only one spot).

Done.

https://chromiumcodereview.appspot.com/811283002/diff/20001/chrome/installer/...
chrome/installer/setup/app_launcher_installer.cc:42: return
installer::GetLocalizedString(IDS_PRODUCT_APP_LAUNCHER_NAME_BASE);
On 2015/01/08 21:41:49, grt wrote:
> installer:: not needed here or elsewhere in this file

Done.

https://chromiumcodereview.appspot.com/811283002/diff/20001/chrome/installer/...
chrome/installer/setup/app_launcher_installer.cc:90: for
(Products::const_iterator it = products.begin(); it < products.end();
Done, using "const auto* p".

https://chromiumcodereview.appspot.com/811283002/diff/20001/chrome/installer/...
chrome/installer/setup/app_launcher_installer.cc:107: // Remove
"quick-enable-application-host" command from Binaries.
On 2015/01/08 21:41:49, grt wrote:
> query-eula-acceptance can be removed as well, no?

Ah, I forgot that I added this in
https://chromiumcodereview.appspot.com/12035043 .  Removed.

Also, looks like eula_util.* has no callers and can go.  Do this in a different
anti-12035043 CL?

https://chromiumcodereview.appspot.com/811283002/diff/20001/chrome/installer/...
chrome/installer/setup/app_launcher_installer.cc:121: reg_root,
app_launcher_reg_data.GetVersionKey(), WorkItem::kWow64Default);
On 2015/01/08 21:41:49, grt wrote:
> WorkItem::kWow64Default -> KEY_WOW64_32KEY

Done.

https://chromiumcodereview.appspot.com/811283002/diff/20001/chrome/installer/...
chrome/installer/setup/app_launcher_installer.cc:127: #endif  //
defined(GOOGLE_CHROME_BUILD)
On 2015/01/08 21:41:49, grt wrote:
> my primitive codesearch tells me that
> #endif  // FOO
> is more prevalent in the codebase than
> #endif  // defined(FOO)
> 
> please use the former style

Done.

https://chromiumcodereview.appspot.com/811283002/diff/20001/chrome/installer/...
File chrome/installer/setup/app_launcher_installer.h (right):

https://chromiumcodereview.appspot.com/811283002/diff/20001/chrome/installer/...
chrome/installer/setup/app_launcher_installer.h:1: // Copyright (c) 2014 The
Chromium Authors. All rights reserved.
On 2015/01/08 21:41:49, grt wrote:
> nit: no (c) in the latest copyright template
> (http://www.chromium.org/developers/coding-style#TOC-File-headers)

Done.

https://chromiumcodereview.appspot.com/811283002/diff/20001/chrome/installer/...
chrome/installer/setup/app_launcher_installer.h:5: // The App Launcher is an
adjunct product of the Google Chrome Browser, but it
On 2015/01/08 21:41:49, grt wrote:
> nit: "the Google Chrome Browser" -> "Google Chrome"

Done.

https://chromiumcodereview.appspot.com/811283002/diff/20001/chrome/installer/...
chrome/installer/setup/app_launcher_installer.h:29: namespace
app_launcher_installer {
I want to make it very easy to find entry points to this rather hacky feature. 
Will remove the namespace if you insist.

https://chromiumcodereview.appspot.com/811283002/diff/20001/chrome/installer/...
chrome/installer/setup/app_launcher_installer.h:52: // Called during uninstall,
to remove "shadow" registry key for App Launcher.
On 2015/01/08 21:41:49, grt wrote:
> Document what the function does rather than when it's called. Also, please
avoid
> "shadow" throughout. It's a subjective term that's irrelevant to the
> implementation. The important technical detail is that the app launcher has
its
> own registration with Google Update that is kept in sync with Google Chrome's.

Done.

https://chromiumcodereview.appspot.com/811283002/diff/20001/chrome/installer/...
chrome/installer/setup/app_launcher_installer.h:54: void RemoveShadowKey(HKEY
reg_root);
On 2015/01/08 21:41:49, grt wrote:
> RemoveShadowKey -> RemoveAppLauncherVersionKey, and move this so that it
> immediately follows the correspoding Add function above.

Done.

https://chromiumcodereview.appspot.com/811283002/diff/20001/chrome/installer/...
File chrome/installer/setup/install_worker.cc (right):

https://chromiumcodereview.appspot.com/811283002/diff/20001/chrome/installer/...
chrome/installer/setup/install_worker.cc:1150: #if defined(GOOGLE_CHROME_BUILD)
Ack. Though I think it might be useful for these legacy-treatment code to not
"blend into the background".

https://chromiumcodereview.appspot.com/811283002/diff/20001/chrome/installer/...
chrome/installer/setup/install_worker.cc:1198: if
(installer_state.FindProduct(BrowserDistribution::CHROME_BROWSER)) {
I suppose, but this would require |add_language_identifier| to be redefined or
passed into AddProductSpecificWorkItems().  Okay to do this?

https://chromiumcodereview.appspot.com/811283002/diff/20001/chrome/installer/...
chrome/installer/setup/install_worker.cc:1203: #endif
On 2015/01/08 21:41:49, grt wrote:
> #endif  // GOOGLE_CHROME_BUILD
> here and elsewhere

Done.

https://chromiumcodereview.appspot.com/811283002/diff/20001/chrome/installer/...
File chrome/installer/setup/setup_main.cc (right):

https://chromiumcodereview.appspot.com/811283002/diff/20001/chrome/installer/...
chrome/installer/setup/setup_main.cc:682: if (!binaries_state)
Without this, if you run "setup.exe --multi-install" then this routine gets
called, and line 685 ends up with a null-dereference.

https://chromiumcodereview.appspot.com/811283002/diff/20001/chrome/installer/...
File chrome/installer/setup/uninstall.cc (right):

https://chromiumcodereview.appspot.com/811283002/diff/20001/chrome/installer/...
chrome/installer/setup/uninstall.cc:1148: if
(!InstallUtil::IsChromeSxSProcess()) {
On 2015/01/09 18:57:04, grt wrote:
> nit: no braces

Done.

https://chromiumcodereview.appspot.com/811283002/diff/20001/chrome/installer/...
chrome/installer/setup/uninstall.cc:1149:
app_launcher_installer::RemoveShadowKey(reg_root);
Moved, but I don't understand the reason you stated: This is also an |is_chrome|
block?  Meanwhile, also moved the "reg_root" assignment line.

https://chromiumcodereview.appspot.com/811283002/diff/20001/chrome/installer/...
File chrome/installer/util/app_launcher_installer_util.h (right):

https://chromiumcodereview.appspot.com/811283002/diff/20001/chrome/installer/...
chrome/installer/util/app_launcher_installer_util.h:13: namespace
app_launcher_installer {
Will do so once you insist.

https://chromiumcodereview.appspot.com/811283002/diff/20001/chrome/installer/...
File chrome/installer/util/channel_info.cc (right):

https://chromiumcodereview.appspot.com/811283002/diff/20001/chrome/installer/...
chrome/installer/util/channel_info.cc:177: AuditValue(&value_);
Done: Restored channal_info.*, but set SetAppLauncher() to always remove
-applauncher.  Updated ChromeBrowserOperations() to remove the flag, and wiped
out app_launcher_install_util.cc.

[grt (UTC plus 2), 2015-01-20 21:30:23]

Thanks for continuing to plug away at this, Sam!

https://chromiumcodereview.appspot.com/811283002/diff/20001/chrome/installer/...
File chrome/installer/setup/app_launcher_installer.h (right):

https://chromiumcodereview.appspot.com/811283002/diff/20001/chrome/installer/...
chrome/installer/setup/app_launcher_installer.h:29: namespace
app_launcher_installer {
On 2015/01/18 01:18:23, huangs1 wrote:
> I want to make it very easy to find entry points to this rather hacky feature.

> Will remove the namespace if you insist.

Use of namespaces is somewhat discouraged (see
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/OIkJO2KQKTQ/YQzUS...,
for example). Since these functions are grouped in this header and are
well-named, I don't think there's a need to further mark them with a namespace.

https://chromiumcodereview.appspot.com/811283002/diff/20001/chrome/installer/...
File chrome/installer/setup/install_worker.cc (right):

https://chromiumcodereview.appspot.com/811283002/diff/20001/chrome/installer/...
chrome/installer/setup/install_worker.cc:1198: if
(installer_state.FindProduct(BrowserDistribution::CHROME_BROWSER)) {
On 2015/01/18 01:18:24, huangs1 wrote:
> I suppose, but this would require |add_language_identifier| to be redefined or
> passed into AddProductSpecificWorkItems().  Okay to do this?

Sure. While you're at it, please change the comment to something like:
    // Add items to set up the App Launcher's version key if Google Chrome is
    // being installed or updated.

https://chromiumcodereview.appspot.com/811283002/diff/20001/chrome/installer/...
File chrome/installer/setup/setup_main.cc (right):

https://chromiumcodereview.appspot.com/811283002/diff/20001/chrome/installer/...
chrome/installer/setup/setup_main.cc:682: if (!binaries_state)
On 2015/01/18 01:18:24, huangs1 wrote:
> Without this, if you run "setup.exe --multi-install" then this routine gets
> called, and line 685 ends up with a null-dereference.

Is this a supported flag combination? I don't believe we (Google) will ever run
the installer like that except to update an existing multi-install product, in
which case binaries_state will never be null. Do you get this crash when
something is already installed?

https://chromiumcodereview.appspot.com/811283002/diff/20001/chrome/installer/...
File chrome/installer/setup/uninstall.cc (right):

https://chromiumcodereview.appspot.com/811283002/diff/20001/chrome/installer/...
chrome/installer/setup/uninstall.cc:1149:
app_launcher_installer::RemoveShadowKey(reg_root);
On 2015/01/18 01:18:24, huangs1 wrote:
> Moved, but I don't understand the reason you stated: This is also an
|is_chrome|
> block?

Doh! So it is! In any case, I think the block below is a better place for new
additions. I have to say I don't see why there are two is_chrome blocks like
that. Probably just the way things were split up way back when multi was
introduced.

https://chromiumcodereview.appspot.com/811283002/diff/20001/chrome/installer/...
File chrome/installer/util/channel_info.cc (right):

https://chromiumcodereview.appspot.com/811283002/diff/20001/chrome/installer/...
chrome/installer/util/channel_info.cc:177: AuditValue(&value_);
On 2015/01/18 01:18:24, huangs1 wrote:
> Done: Restored channal_info.*, but set SetAppLauncher() to always remove
> -applauncher.  Updated ChromeBrowserOperations() to remove the flag, and wiped
> out app_launcher_install_util.cc.

I don't think this is the right approach since it means that this class can no
longer be used to introspect an installation's channel. I think it's better to
have the "wipe out app host/launcher" higher up rather than have it buried in
the implementation of a utility class like this.

https://chromiumcodereview.appspot.com/811283002/diff/40001/chrome/installer/...
File chrome/installer/setup/app_launcher_installer.cc (right):

https://chromiumcodereview.appspot.com/811283002/diff/40001/chrome/installer/...
chrome/installer/setup/app_launcher_installer.cc:109: // Remove
"query-eula-acceptance"" command from Binaries.
nit: double double quote quote

https://chromiumcodereview.appspot.com/811283002/diff/40001/chrome/installer/...
File chrome/installer/util/channel_info.h (right):

https://chromiumcodereview.appspot.com/811283002/diff/40001/chrome/installer/...
chrome/installer/util/channel_info.h:36: const std::wstring& value() const {
return value_; }
string16 here and below, and remove #include <string>

https://chromiumcodereview.appspot.com/811283002/diff/40001/chrome/installer/...
File chrome/installer/util/util_constants.cc (left):

https://chromiumcodereview.appspot.com/811283002/diff/40001/chrome/installer/...
chrome/installer/util/util_constants.cc:25: const char kChromeAppLauncher[] =
"app-launcher";
I think this (and probably app-host) should be added to the list in
ContainsUnsupportedSwitch (setup_util.cc) so that the installer exits early if
these are on the command line.  Otherwise setup.exe will install/update Chrome
itself, which would be unexpected for anyone who was trying to get the extremely
old host/launcher behavior. Can you think of any reason not to do this?

[huangs, 2015-01-20 23:26:01]

Updated. A bit confused with the ChannelInfo comments and what to do with
ChromeBrowserOperations::SetChannelFlags().  PTAL.

https://chromiumcodereview.appspot.com/811283002/diff/20001/chrome/installer/...
File chrome/installer/setup/app_launcher_installer.h (right):

https://chromiumcodereview.appspot.com/811283002/diff/20001/chrome/installer/...
chrome/installer/setup/app_launcher_installer.h:29: namespace
app_launcher_installer {
Removed.

https://chromiumcodereview.appspot.com/811283002/diff/20001/chrome/installer/...
File chrome/installer/setup/install_worker.cc (right):

https://chromiumcodereview.appspot.com/811283002/diff/20001/chrome/installer/...
chrome/installer/setup/install_worker.cc:1198: if
(installer_state.FindProduct(BrowserDistribution::CHROME_BROWSER)) {
On 2015/01/20 21:30:22, grt wrote:
> On 2015/01/18 01:18:24, huangs1 wrote:
> > I suppose, but this would require |add_language_identifier| to be redefined
or
> > passed into AddProductSpecificWorkItems().  Okay to do this?
> 
> Sure. While you're at it, please change the comment to something like:
>     // Add items to set up the App Launcher's version key if Google Chrome is
>     // being installed or updated.

Done.

https://chromiumcodereview.appspot.com/811283002/diff/20001/chrome/installer/...
File chrome/installer/setup/uninstall.cc (right):

https://chromiumcodereview.appspot.com/811283002/diff/20001/chrome/installer/...
chrome/installer/setup/uninstall.cc:1149:
app_launcher_installer::RemoveShadowKey(reg_root);
Okay. No additional change here.

https://chromiumcodereview.appspot.com/811283002/diff/20001/chrome/installer/...
File chrome/installer/util/channel_info.cc (right):

https://chromiumcodereview.appspot.com/811283002/diff/20001/chrome/installer/...
chrome/installer/util/channel_info.cc:177: AuditValue(&value_);
Okay, put back the filtering for set_value() and the unit test (not restricted
to Chrome since ChannelValue did not restrict App Launcher stuff for Chrome),
keeping all the code in channel_info*.

Not sure what to do with the ChromeBrowserOperations::SetChannelFlags() change.

https://chromiumcodereview.appspot.com/811283002/diff/40001/chrome/installer/...
File chrome/installer/setup/app_launcher_installer.cc (right):

https://chromiumcodereview.appspot.com/811283002/diff/40001/chrome/installer/...
chrome/installer/setup/app_launcher_installer.cc:109: // Remove
"query-eula-acceptance"" command from Binaries.
On 2015/01/20 21:30:23, grt wrote:
> nit: double double quote quote

Done.

https://chromiumcodereview.appspot.com/811283002/diff/40001/chrome/installer/...
File chrome/installer/util/channel_info.h (right):

https://chromiumcodereview.appspot.com/811283002/diff/40001/chrome/installer/...
chrome/installer/util/channel_info.h:36: const std::wstring& value() const {
return value_; }
On 2015/01/20 21:30:23, grt wrote:
> string16 here and below, and remove #include <string>

Done.

https://chromiumcodereview.appspot.com/811283002/diff/40001/chrome/installer/...
File chrome/installer/util/util_constants.cc (left):

https://chromiumcodereview.appspot.com/811283002/diff/40001/chrome/installer/...
chrome/installer/util/util_constants.cc:25: const char kChromeAppLauncher[] =
"app-launcher";
Perhaps to uninstall?  But there are very few people, if any that would have
stand-alone App Launcher.  So added to list.

[grt (UTC plus 2), 2015-01-21 02:18:12]

https://chromiumcodereview.appspot.com/811283002/diff/20001/chrome/installer/...
File chrome/installer/setup/setup_main.cc (right):

https://chromiumcodereview.appspot.com/811283002/diff/20001/chrome/installer/...
chrome/installer/setup/setup_main.cc:682: if (!binaries_state)
On 2015/01/20 21:30:22, grt wrote:
> On 2015/01/18 01:18:24, huangs1 wrote:
> > Without this, if you run "setup.exe --multi-install" then this routine gets
> > called, and line 685 ends up with a null-dereference.
> 
> Is this a supported flag combination? I don't believe we (Google) will ever
run
> the installer like that except to update an existing multi-install product, in
> which case binaries_state will never be null. Do you get this crash when
> something is already installed?

ping

https://chromiumcodereview.appspot.com/811283002/diff/20001/chrome/installer/...
File chrome/installer/util/channel_info.cc (right):

https://chromiumcodereview.appspot.com/811283002/diff/20001/chrome/installer/...
chrome/installer/util/channel_info.cc:177: AuditValue(&value_);
On 2015/01/20 23:26:00, huangs1 wrote:
> Okay, put back the filtering for set_value() and the unit test (not restricted
> to Chrome since ChannelValue did not restrict App Launcher stuff for Chrome),
> keeping all the code in channel_info*.
> 
> Not sure what to do with the ChromeBrowserOperations::SetChannelFlags()
change.

Let's talk in person. I must be doing a bad job of conveying what I mean in
text. The ChannelInfo implementation should not be responsible for clearing out
old values; its consumers should. I don't think ChannelInfo should be modified
much at all beyond the excellent string16 change and "deprecated" doc comments
here and there. Sorry if I'm wasting your time!

https://chromiumcodereview.appspot.com/811283002/diff/40001/chrome/installer/...
File chrome/installer/util/util_constants.cc (left):

https://chromiumcodereview.appspot.com/811283002/diff/40001/chrome/installer/...
chrome/installer/util/util_constants.cc:25: const char kChromeAppLauncher[] =
"app-launcher";
On 2015/01/20 23:26:00, huangs1 wrote:
> Perhaps to uninstall?  But there are very few people, if any that would have
> stand-alone App Launcher.  So added to list.

If I'm not mistaken, uninstall isn't a concern since a machine with the
host/launcher would have to first be updated to a version containing this change
before an uninstall could take place. The update should wipe out any trace of
app-host/app-launcher, so all is good. Do I have that wrong?

https://chromiumcodereview.appspot.com/811283002/diff/60001/chrome/installer/...
File chrome/installer/setup/install_worker.cc (right):

https://chromiumcodereview.appspot.com/811283002/diff/60001/chrome/installer/...
chrome/installer/setup/install_worker.cc:1159: if
(!InstallUtil::IsChromeSxSProcess()) {
nit: no braces here and elsewhere for single-liners like this

[huangs, 2015-01-21 20:33:52]

Updated, PTAL.

https://chromiumcodereview.appspot.com/811283002/diff/20001/chrome/installer/...
File chrome/installer/setup/setup_main.cc (right):

https://chromiumcodereview.appspot.com/811283002/diff/20001/chrome/installer/...
chrome/installer/setup/setup_main.cc:682: if (!binaries_state)
Per discussion, no need to worry about this, but need to test for:
- "setup.exe --multi-install" when Chrome is installed.
- With canary installed, run new "setup.exe --multi-install" to ensure it
updates.

https://chromiumcodereview.appspot.com/811283002/diff/20001/chrome/installer/...
chrome/installer/setup/setup_main.cc:682: if (!binaries_state)
See next comment.

https://chromiumcodereview.appspot.com/811283002/diff/20001/chrome/installer/...
File chrome/installer/util/channel_info.cc (right):

https://chromiumcodereview.appspot.com/811283002/diff/20001/chrome/installer/...
chrome/installer/util/channel_info.cc:177: AuditValue(&value_);
Acknowledged; restoring this per conversation.

https://chromiumcodereview.appspot.com/811283002/diff/40001/chrome/installer/...
File chrome/installer/util/util_constants.cc (left):

https://chromiumcodereview.appspot.com/811283002/diff/40001/chrome/installer/...
chrome/installer/util/util_constants.cc:25: const char kChromeAppLauncher[] =
"app-launcher";
I think all is good, but since you asked I should try to suggest something.

https://chromiumcodereview.appspot.com/811283002/diff/60001/chrome/installer/...
File chrome/installer/setup/install_worker.cc (right):

https://chromiumcodereview.appspot.com/811283002/diff/60001/chrome/installer/...
chrome/installer/setup/install_worker.cc:1159: if
(!InstallUtil::IsChromeSxSProcess()) {
Done.  Moved comment up.

[huangs, 2015-01-23 19:56:37]

Ping grt@.

[grt (UTC plus 2), 2015-01-27 14:51:50]

looks good. please make these final changes, address the TODOs you left for
yourself, and sync to ToT for final review!

https://codereview.chromium.org/811283002/diff/80001/chrome/installer/setup/i...
File chrome/installer/setup/install_worker.cc (right):

https://codereview.chromium.org/811283002/diff/80001/chrome/installer/setup/i...
chrome/installer/setup/install_worker.cc:299: if (p.is_chrome_binaries()) {
nit: remove braces

https://codereview.chromium.org/811283002/diff/80001/chrome/installer/setup/i...
chrome/installer/setup/install_worker.cc:1197: if
(!InstallUtil::IsChromeSxSProcess()) {
nit: remove braces

https://codereview.chromium.org/811283002/diff/80001/chrome/installer/util/ch...
File chrome/installer/util/channel_info.cc (right):

https://codereview.chromium.org/811283002/diff/80001/chrome/installer/util/ch...
chrome/installer/util/channel_info.cc:212: return false;
  return HasModifier(MOD_APP_LAUNCHER, value_);

https://codereview.chromium.org/811283002/diff/80001/chrome/installer/util/eu...
File chrome/installer/util/eula_util.cc (right):

https://codereview.chromium.org/811283002/diff/80001/chrome/installer/util/eu...
chrome/installer/util/eula_util.cc:49: EULAAcceptanceResponse
IsEULAAccepted(bool system_level) {
i think you removed the only caller of this in setup_main.cc. can this file be
removed now?

[huangs, 2015-01-28 17:43:53]

Still need to sync, then update /chrome/installer/util/BUILD.gn.

https://chromiumcodereview.appspot.com/811283002/diff/80001/chrome/installer/...
File chrome/installer/setup/install_worker.cc (right):

https://chromiumcodereview.appspot.com/811283002/diff/80001/chrome/installer/...
chrome/installer/setup/install_worker.cc:299: if (p.is_chrome_binaries()) {
On 2015/01/27 14:51:50, grt wrote:
> nit: remove braces

Done.

https://chromiumcodereview.appspot.com/811283002/diff/80001/chrome/installer/...
chrome/installer/setup/install_worker.cc:1197: if
(!InstallUtil::IsChromeSxSProcess()) {
On 2015/01/27 14:51:50, grt wrote:
> nit: remove braces

Done.

https://chromiumcodereview.appspot.com/811283002/diff/80001/chrome/installer/...
File chrome/installer/util/channel_info.cc (right):

https://chromiumcodereview.appspot.com/811283002/diff/80001/chrome/installer/...
chrome/installer/util/channel_info.cc:212: return false;
On 2015/01/27 14:51:50, grt wrote:
>   return HasModifier(MOD_APP_LAUNCHER, value_);

Done.

https://chromiumcodereview.appspot.com/811283002/diff/80001/chrome/installer/...
File chrome/installer/util/eula_util.cc (right):

https://chromiumcodereview.appspot.com/811283002/diff/80001/chrome/installer/...
chrome/installer/util/eula_util.cc:49: EULAAcceptanceResponse
IsEULAAccepted(bool system_level) {
I mentioned this in my (2015-01-18 01:18:24 UTC) comment on creating a separate
URL to undo
  https://chromiumcodereview.appspot.com/12035043/

Come to think of it it's easier to do it in this CL. Done.

Should also update \chrome\installer\util\BUILD.gn , but I have to sync first.

[grt (UTC plus 2), 2015-01-28 20:34:07]

lgtm with a comment nit

https://codereview.chromium.org/811283002/diff/140001/chrome/installer/util/i...
File chrome/installer/util/installer_state.cc (left):

https://codereview.chromium.org/811283002/diff/140001/chrome/installer/util/i...
chrome/installer/util/installer_state.cc:275: // Initial, over, and un-installs
will take place under one of the product app
please retain this comment (just update the parenthetical remark at the end).

[Will Harris, 2015-01-28 20:43:25]

On 2015/01/28 20:34:07, grt wrote:
> lgtm with a comment nit
> 
>
https://codereview.chromium.org/811283002/diff/140001/chrome/installer/util/i...
> File chrome/installer/util/installer_state.cc (left):
> 
>
https://codereview.chromium.org/811283002/diff/140001/chrome/installer/util/i...
> chrome/installer/util/installer_state.cc:275: // Initial, over, and
un-installs
> will take place under one of the product app
> please retain this comment (just update the parenthetical remark at the end).

Would it be possible to add a TEST= command, even if the test is manual, for a
change like this it would be good to have QA add it to the build qualification.

[huangs, 2015-01-28 21:06:58]

Updated, also added TEST=.

https://chromiumcodereview.appspot.com/811283002/diff/140001/chrome/installer...
File chrome/installer/util/installer_state.cc (left):

https://chromiumcodereview.appspot.com/811283002/diff/140001/chrome/installer...
chrome/installer/util/installer_state.cc:275: // Initial, over, and un-installs
will take place under one of the product app
On 2015/01/28 20:34:07, grt wrote:
> please retain this comment (just update the parenthetical remark at the end).

Done.

[grt (UTC plus 2), 2015-01-28 21:09:06]

On 2015/01/28 21:06:58, huangs1 wrote:
> Updated, also added TEST=.
> 
>
https://chromiumcodereview.appspot.com/811283002/diff/140001/chrome/installer...
> File chrome/installer/util/installer_state.cc (left):
> 
>
https://chromiumcodereview.appspot.com/811283002/diff/140001/chrome/installer...
> chrome/installer/util/installer_state.cc:275: // Initial, over, and
un-installs
> will take place under one of the product app
> On 2015/01/28 20:34:07, grt wrote:
> > please retain this comment (just update the parenthetical remark at the
end).
> 
> Done.

Still LGTM, thanks.

Will: In case you're wondering, Sam is doing pretty thorough manual testing
before landing this. It'd be great to get some extra QA on it as well. We'll
send a shout out to them to give them a heads-up. Thanks for chiming in.

[Will Harris, 2015-01-28 21:37:48]

On 2015/01/28 21:09:06, grt wrote:
> On 2015/01/28 21:06:58, huangs1 wrote:
> > Updated, also added TEST=.
> > 
> >
>
https://chromiumcodereview.appspot.com/811283002/diff/140001/chrome/installer...
> > File chrome/installer/util/installer_state.cc (left):
> > 
> >
>
https://chromiumcodereview.appspot.com/811283002/diff/140001/chrome/installer...
> > chrome/installer/util/installer_state.cc:275: // Initial, over, and
> un-installs
> > will take place under one of the product app
> > On 2015/01/28 20:34:07, grt wrote:
> > > please retain this comment (just update the parenthetical remark at the
> end).
> > 
> > Done.
> 
> Still LGTM, thanks.
> 
> Will: In case you're wondering, Sam is doing pretty thorough manual testing
> before landing this. It'd be great to get some extra QA on it as well. We'll
> send a shout out to them to give them a heads-up. Thanks for chiming in.

Thanks for adding the test line!  QA are really good at grepping the commit log
for these before each release, and will run them if you specify them, so I try
and always add something especially if the test is manual.

[huangs, 2015-01-28 22:57:03]

Think I did enough tests (Chrome installed by command line, using
--do_not_launch_chrome):
1. Do the below for user-level install, the system-level installs. Registries
are manually cleared before each install.
2. Install old, dump Google registries at various points (on install, Chrome
run, App Launcher run, uninstall).
3. Install new, dump Google registries at various points (same).
4. Compare registry dumps and observe expected changes (ignoring noise).
5. Install old, then upgrade to new, dump registries for updates.
6. Same, but before installing new, add "-applauncher" and "-apphost" to "ap"
keys, and ensure they disappear.

Going to submit.

[huangs, 2015-01-28 22:57:17]

The CQ bit was checked by huangs@chromium.org

[commit-bot: I haz the power, 2015-01-28 22:58:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/811283002/160001

[commit-bot: I haz the power, 2015-01-29 00:00:57]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-01-29 00:03:10]

Patchset 9 (id:??) landed as
https://crrev.com/45817557f9fda44543d513a500eb0d74dd415ee0
Cr-Commit-Position: refs/heads/master@{#313626}