Implement installation of the Chrome App Host.

Implement installation of the Chrome App Host.

The Chrome App Host is a simple exe that delegates to a Chrome Binaries installation at user or system level. If no installation is available, it will trigger an installation.

The Chrome App Host prevents the Chrome Binaries from being uninstalled, except in the case of an upgrade to system-level.

The Chrome App Host is implemented in this uncommitted CL: http://codereview.chromium.org/10559090/ .

BUG=None
TEST=None

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

[erikwright (departed), 2012-07-11 20:44:09]

Hi Greg,

If you can take a first pass on this before going on leave, that would be great.
I'll get Tommi to do a final review.

Tommi, this is probably not worth looking at now - very rough but I want Greg's
feedback before he's on holiday. But you're the only owner left (Robert + Greg
both out next week, me the week after, and I can't wait that long) so I will
likely ask you for a review early next week.

Thanks,

Erik

[tommi (sloooow) - chröme, 2012-07-12 08:11:31]

looks good. ping me when you're ready for the final review.

http://codereview.chromium.org/10665002/diff/6001/chrome/app/chromium_strings...
File chrome/app/chromium_strings.grd (right):

http://codereview.chromium.org/10665002/diff/6001/chrome/app/chromium_strings...
chrome/app/chromium_strings.grd:226: <message name="IDS_PRODUCT_APP_HOST_NAME"
desc="The Chrome App Host application name">
nit: Chromium?

http://codereview.chromium.org/10665002/diff/6001/chrome/app/chromium_strings...
chrome/app/chromium_strings.grd:229: <message name="IDS_PRODUCT_BINARIES_NAME"
desc="The Chrome Binaries application name">
ditto

http://codereview.chromium.org/10665002/diff/6001/chrome/app/google_chrome_st...
File chrome/app/google_chrome_strings.grd (right):

http://codereview.chromium.org/10665002/diff/6001/chrome/app/google_chrome_st...
chrome/app/google_chrome_strings.grd:150: Google App Host
Should this be Google Chrome App Host?

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/mini_insta...
File chrome/installer/mini_installer/chrome_appid.cc (right):

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/mini_insta...
chrome/installer/mini_installer/chrome_appid.cc:11: const wchar_t
kChromeAppHostAppGuid[] = L"{FDA71E6F-AC4C-4a00-8B70-9958A68906BF}";
80

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/mini_insta...
File chrome/installer/mini_installer/mini_installer.cc (right):

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/mini_insta...
chrome/installer/mini_installer/mini_installer.cc:26: #include <wchar.h>
out of curiosity, why is this needed?

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/setup/inst...
File chrome/installer/setup/install_worker.cc (right):

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.cc:143: // TODO(erikwright): What does
this mean? Chrome Frame _is_ in Add/Remove, No?
I think the comment is out of date and was probably written early on during the
multi-install phase.

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.cc:1354: // Is Chrome Binaries already on
the machine?
s/Is/Are the

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/setup/setu...
File chrome/installer/setup/setup_main.cc (right):

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/setup/setu...
chrome/installer/setup/setup_main.cc:447: true /* system level */,
BrowserDistribution::CHROME_BROWSER);
nit: I think we avoid /**/ comments in general. instead you could do:
GetProductState(
    true,  // system_level
    BrowserDistribution::CHROME_BROWSER);

As a side note, maybe it would be good for readability to change all these 'bool
system_level' variables to an enum.

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/setup/setu...
chrome/installer/setup/setup_main.cc:943: // TODO(erikwright): if product 0 is
not chrome, assert chrome not in products
do you intend to do that before checkin?

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/setup/unin...
File chrome/installer/setup/uninstall.cc (right):

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/setup/unin...
chrome/installer/setup/uninstall.cc:56: //
ScheduleParentAndGrandparentForDeletion?
if so, file a bug for tracking?

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/util/chrom...
File chrome/installer/util/chrome_app_host_operations.h (right):

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/util/chrom...
chrome/installer/util/chrome_app_host_operations.h:15: #include
"base/compiler_specific.h"
fix include order

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/util/chrom...
File chrome/installer/util/chromium_binaries_distribution.h (right):

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/util/chrom...
chrome/installer/util/chromium_binaries_distribution.h:50: virtual bool
GetDelegateExecuteHandlerData(string16* handler_class_uuid,
would it make sense to group these into a struct?

[grt (UTC plus 2), 2012-07-12 18:37:10]

looks really good.  here are my comments so far.

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/app/google_c...
File chrome/app/google_chrome_strings.grd (right):

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/app/google_c...
chrome/app/google_chrome_strings.grd:383: This computer already has a more
recent version of the Chrome components. Please use a more recent installer.
Chrome -> Google Chrome?

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/chrome_insta...
File chrome/chrome_installer.gypi (right):

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/chrome_insta...
chrome/chrome_installer.gypi:265: '../chrome/chrome.gyp:app_host',
in the past, mini_installer (in chrome/installer/mini_installer.gyp) was the
thing that depended on chrome.  is it not suitable to put the dependency on
app_host there rather than here?  i think it's desirable to be able to build
setup.exe without building chrome and the app host.

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/chrome_insta...
File chrome/chrome_installer_util.gypi (right):

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/chrome_insta...
chrome/chrome_installer_util.gypi:1: # Copyright (c) 2011 The Chromium Authors.
All rights reserved.
2012

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/installer/mi...
File chrome/installer/mini_installer/appid.h (right):

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/installer/mi...
chrome/installer/mini_installer/appid.h:1: // Copyright (c) 2010 The Chromium
Authors. All rights reserved.
2012

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/installer/mi...
chrome/installer/mini_installer/appid.h:14: extern const wchar_t
kChromeAppHostAppGuid[];
move up one line

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/installer/mi...
File chrome/installer/mini_installer/chrome.release (right):

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/installer/mi...
chrome/installer/mini_installer/chrome.release:7: app_host.exe: %(ChromeDir)s\
move up one line

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/installer/mi...
File chrome/installer/mini_installer/chrome_appid.cc (right):

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/installer/mi...
chrome/installer/mini_installer/chrome_appid.cc:1: // Copyright (c) 2010 The
Chromium Authors. All rights reserved.
2012

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/installer/mi...
chrome/installer/mini_installer/chrome_appid.cc:11: const wchar_t
kChromeAppHostAppGuid[] = L"{FDA71E6F-AC4C-4a00-8B70-9958A68906BF}";
please sort this whole collection

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/installer/mi...
File chrome/installer/mini_installer/configuration.cc (right):

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/installer/mi...
chrome/installer/mini_installer/configuration.cc:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
2012

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/installer/mi...
chrome/installer/mini_installer/configuration.cc:75: has_chrome_ =
!(has_chrome_frame_ || has_app_host_);
is more needed here?  what happens if --app-host is given (possibly with
--chrome-frame) without --multi-install?  i haven't reviewed the rest of the CL
yet, but assuming that --multi-install is always required for --app-host, i
think it makes sense to put "has_app_host_ = false;" within this
!is_multi_install_ block.

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/installer/mi...
File chrome/installer/mini_installer/configuration.h (right):

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/installer/mi...
chrome/installer/mini_installer/configuration.h:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
2012

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/installer/mi...
File chrome/installer/mini_installer/mini_installer.cc (right):

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/installer/mi...
chrome/installer/mini_installer/mini_installer.cc:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
2012

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/installer/mi...
chrome/installer/mini_installer/mini_installer.cc:234: const int kMaxGuids = 10;
why not 4?

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/installer/mi...
chrome/installer/mini_installer/mini_installer.cc:235: const wchar_t*
app_guids[kMaxGuids] = {0};
{0} -> {}

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/installer/mi...
chrome/installer/mini_installer/mini_installer.cc:238: // If this is a multi
install, first try looking in the binaries for the path
"path" -> "path."

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/installer/mi...
chrome/installer/mini_installer/mini_installer.cc:241: // Failing that, look in
Chrome Frame's client state key --chrome-frame was
"state key" -> "state key if"

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/installer/se...
File chrome/installer/setup/install_worker.cc (right):

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/installer/se...
chrome/installer/setup/install_worker.cc:143: // TODO(erikwright): What does
this mean? Chrome Frame _is_ in Add/Remove, No?
On 2012/07/12 08:11:31, tommi wrote:
> I think the comment is out of date and was probably written early on during
the
> multi-install phase.

I think it refers to ready-mode, where Chrome Frame is installed yet not present
in Add/Remove.

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/installer/se...
File chrome/installer/setup/setup_main.cc (right):

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/installer/se...
chrome/installer/setup/setup_main.cc:403: } else if (chrome_frame &&
chrome_frame->HasOption(installer::kOptionReadyMode)) {
80 cols

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/installer/se...
chrome/installer/setup/setup_main.cc:438: bool CheckAppHostPreconditions(const
InstallationState& original_state,
please document the failure modes and side effects of this function (e.g., that
the binaries are added to |installer_state| if neither single-install chrome nor
any multi-install product is present on the machine).

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/installer/se...
chrome/installer/setup/setup_main.cc:444: return false;
VLOG(1) the reason for failure

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/installer/se...
chrome/installer/setup/setup_main.cc:445: 
if (instaler_state->package_type() != InstallerState::MULTI_PACKAGE) {
  VLOG(1) << ??;
  return false;
}

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/installer/se...
chrome/installer/setup/setup_main.cc:447: true /* system level */,
BrowserDistribution::CHROME_BROWSER);
On 2012/07/12 08:11:31, tommi wrote:
> As a side note, maybe it would be good for readability to change all these
'bool
> system_level' variables to an enum.

InstallerState::Level could be moved up into the "installer" namespace and given
a less collision-prone name.

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/installer/se...
chrome/installer/setup/setup_main.cc:454: false /* system level */,
BrowserDistribution::CHROME_BINARIES);
should the comment be "!system level"?  i find it confusing for the comment to
be the same for calls with opposing values.

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/installer/se...
chrome/installer/setup/setup_main.cc:465: if (!binaries_state && !
installer_state->is_multi_install()) {
remove this block as discussed

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/installer/se...
chrome/installer/setup/setup_main.cc:694: case kGCFBit & kBrowserBit:
i think you mean "kGCFBit | kBrowserBit" here.

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/installer/ut...
File chrome/installer/util/browser_distribution.cc (right):

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/installer/ut...
chrome/installer/util/browser_distribution.cc:22: #include
"chrome/installer/util/chrome_app_host_distribution.h"
ordering

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/installer/ut...
chrome/installer/util/browser_distribution.cc:61: if
(MasterPreferences::ForCurrentProcess().install_chrome_frame() ||
did you intentionally change this so that it no longer caches the value?  i
don't think we want to do this computation on each call.

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/installer/ut...
chrome/installer/util/browser_distribution.cc:73: const
BrowserDistribution::Type BrowserDistribution::kProductTypes[] = {
please check to see if it now makes sense for kProductTypes to go away and for
all of its users to be converted to simply iterating over the
BrowserDistribution::Type values from 0 to (but not including)
BrowserDistribution::NUM_TYPES.  i think it does.  AddExistingMultiInstalls, for
example, looks like it should now include the binaries so that they are added to
the set of products being operated on.

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/installer/ut...
File chrome/installer/util/chrome_app_host_operations.cc (right):

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/installer/ut...
chrome/installer/util/chrome_app_host_operations.cc:62: // TODO(erikwright): I
guess this isn't needed?
if it's the case now that the binaries' product is always in the
InstallerState's collection, then it seems reasonable for it to be the
responsibility of the binaries to put chrome.dll in the key files list.

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/installer/ut...
File chrome/installer/util/chrome_binaries_operations.cc (right):

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/installer/ut...
chrome/installer/util/chrome_binaries_operations.cc:24: 
is it valid for there to be non-multi-install binaries?  should this be enforced
in this file?

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/installer/ut...
File chrome/installer/util/chrome_browser_operations.cc (right):

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/installer/ut...
chrome/installer/util/chrome_browser_operations.cc:44: // TODO(erikwright): If
multi, can we omit this?
see my comment in the app host's operations file

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/installer/ut...
File chrome/installer/util/chromium_binaries_distribution.cc (right):

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/installer/ut...
chrome/installer/util/chromium_binaries_distribution.cc:80: return L"Uninstall
Chromium Binaries";
why?

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/installer/ut...
chrome/installer/util/chromium_binaries_distribution.cc:84: return
L"Software\\Microsoft\\Windows\\CurrentVersion\\Uninstall\\"
the binaries show up in add/remove?

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/installer/ut...
chrome/installer/util/chromium_binaries_distribution.cc:85: L"Google Chromium
Binaries";
this shouldn't have "Google" in the name, although i wonder why we need a value
here at all.

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/installer/ut...
File chrome/installer/util/installation_state.cc (right):

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/installer/ut...
chrome/installer/util/installation_state.cc:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
2012

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/installer/ut...
File chrome/installer/util/installation_state.h (right):

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/installer/ut...
chrome/installer/util/installation_state.h:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
2012

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/installer/ut...
File chrome/installer/util/installer_state.cc (right):

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/installer/ut...
chrome/installer/util/installer_state.cc:114: 
suggestion: add this here
  const wchar_t* const operation = (is_uninstall ? "Uninstall" : "Install");
and then use |operation| in the log messages below

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/installer/ut...
chrome/installer/util/installer_state.cc:188: // product app guids.  Chrome
Frame's will be used if only Chrome Frame
is this comment out of date now?  is there such thing as an initial install or
over install driven by Google Update of the app host alone (i think the answer
is yes)?

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/installer/ut...
File chrome/installer/util/prebuild/create_string_rc.py (right):

https://chromiumcodereview.appspot.com/10665002/diff/6001/chrome/installer/ut...
chrome/installer/util/prebuild/create_string_rc.py:49:
'IDS_PRODUCT_APP_HOST_NAME',
please move these up two lines so they're in order

[Jói, 2012-07-13 14:07:41]

Quick drive-by:

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/util/chrom...
File chrome/installer/util/chromium_binaries_distribution.cc (right):

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/util/chrom...
chrome/installer/util/chromium_binaries_distribution.cc:80: return L"Uninstall
Chromium Binaries";
No localization on this or other user-visible messages here?

[erikwright (departed), 2012-07-16 20:13:10]

Hi Tommi,

Please take a look. I'm under a bit of time pressure as I leave this Friday for
a week, so let me know if you can't get to it shortly.

While you are looking at this I'll look at which unit tests need to be
expanded/written.

Thanks,

Erik

http://codereview.chromium.org/10665002/diff/6001/chrome/app/chromium_strings...
File chrome/app/chromium_strings.grd (right):

http://codereview.chromium.org/10665002/diff/6001/chrome/app/chromium_strings...
chrome/app/chromium_strings.grd:226: <message name="IDS_PRODUCT_APP_HOST_NAME"
desc="The Chrome App Host application name">
On 2012/07/12 08:11:31, tommi wrote:
> nit: Chromium?

Just being consistent with what's above. I don't know if it's important that the
descriptions match those in the google_chrome_strings.grd or not. Since
elsewhere in the file Chromium is being used, I guess probably not, but most are
Chrome so I'll use that.

http://codereview.chromium.org/10665002/diff/6001/chrome/app/chromium_strings...
chrome/app/chromium_strings.grd:229: <message name="IDS_PRODUCT_BINARIES_NAME"
desc="The Chrome Binaries application name">
On 2012/07/12 08:11:31, tommi wrote:
> ditto

See previous.

http://codereview.chromium.org/10665002/diff/6001/chrome/app/google_chrome_st...
File chrome/app/google_chrome_strings.grd (right):

http://codereview.chromium.org/10665002/diff/6001/chrome/app/google_chrome_st...
chrome/app/google_chrome_strings.grd:150: Google App Host
On 2012/07/12 08:11:31, tommi wrote:
> Should this be Google Chrome App Host?

Done.

http://codereview.chromium.org/10665002/diff/6001/chrome/app/google_chrome_st...
chrome/app/google_chrome_strings.grd:383: This computer already has a more
recent version of the Chrome components. Please use a more recent installer.
On 2012/07/12 18:37:10, grt wrote:
> Chrome -> Google Chrome?

Done.

http://codereview.chromium.org/10665002/diff/6001/chrome/chrome_installer.gypi
File chrome/chrome_installer.gypi (right):

http://codereview.chromium.org/10665002/diff/6001/chrome/chrome_installer.gyp...
chrome/chrome_installer.gypi:265: '../chrome/chrome.gyp:app_host',
On 2012/07/12 18:37:10, grt wrote:
> in the past, mini_installer (in chrome/installer/mini_installer.gyp) was the
> thing that depended on chrome.  is it not suitable to put the dependency on
> app_host there rather than here?  i think it's desirable to be able to build
> setup.exe without building chrome and the app host.

Hm. I guess so. Ultimately it seems pretty useless to build setup without
building the archive, but these lines don't address that, so I can take them
out.

http://codereview.chromium.org/10665002/diff/6001/chrome/chrome_installer_uti...
File chrome/chrome_installer_util.gypi (right):

http://codereview.chromium.org/10665002/diff/6001/chrome/chrome_installer_uti...
chrome/chrome_installer_util.gypi:1: # Copyright (c) 2011 The Chromium Authors.
All rights reserved.
On 2012/07/12 18:37:10, grt wrote:
> 2012

Done.

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/mini_insta...
File chrome/installer/mini_installer/appid.h (right):

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/mini_insta...
chrome/installer/mini_installer/appid.h:1: // Copyright (c) 2010 The Chromium
Authors. All rights reserved.
On 2012/07/12 18:37:10, grt wrote:
> 2012

Done.

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/mini_insta...
chrome/installer/mini_installer/appid.h:14: extern const wchar_t
kChromeAppHostAppGuid[];
On 2012/07/12 18:37:10, grt wrote:
> move up one line

Done.

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/mini_insta...
File chrome/installer/mini_installer/chrome.release (right):

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/mini_insta...
chrome/installer/mini_installer/chrome.release:7: app_host.exe: %(ChromeDir)s\
On 2012/07/12 18:37:10, grt wrote:
> move up one line

Done.

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/mini_insta...
File chrome/installer/mini_installer/chrome_appid.cc (right):

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/mini_insta...
chrome/installer/mini_installer/chrome_appid.cc:1: // Copyright (c) 2010 The
Chromium Authors. All rights reserved.
On 2012/07/12 18:37:10, grt wrote:
> 2012

Done.

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/mini_insta...
chrome/installer/mini_installer/chrome_appid.cc:11: const wchar_t
kChromeAppHostAppGuid[] = L"{FDA71E6F-AC4C-4a00-8B70-9958A68906BF}";
On 2012/07/12 08:11:31, tommi wrote:
> 80

Done.

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/mini_insta...
chrome/installer/mini_installer/chrome_appid.cc:11: const wchar_t
kChromeAppHostAppGuid[] = L"{FDA71E6F-AC4C-4a00-8B70-9958A68906BF}";
On 2012/07/12 18:37:10, grt wrote:
> please sort this whole collection

Done.

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/mini_insta...
File chrome/installer/mini_installer/configuration.cc (right):

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/mini_insta...
chrome/installer/mini_installer/configuration.cc:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
On 2012/07/12 18:37:10, grt wrote:
> 2012

Done.

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/mini_insta...
chrome/installer/mini_installer/configuration.cc:75: has_chrome_ =
!(has_chrome_frame_ || has_app_host_);
On 2012/07/12 18:37:10, grt wrote:
> is more needed here?  what happens if --app-host is given (possibly with
> --chrome-frame) without --multi-install?  i haven't reviewed the rest of the
CL
> yet, but assuming that --multi-install is always required for --app-host, i
> think it makes sense to put "has_app_host_ = false;" within this
> !is_multi_install_ block.

I think I'd rather leave this class responsible for simply interpreting the
command line and then in setup main do the verification that in single mode
exactly one product is specified and that app-host is only used in multi mode.

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/mini_insta...
File chrome/installer/mini_installer/configuration.h (right):

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/mini_insta...
chrome/installer/mini_installer/configuration.h:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
On 2012/07/12 18:37:10, grt wrote:
> 2012

Done.

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/mini_insta...
File chrome/installer/mini_installer/mini_installer.cc (right):

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/mini_insta...
chrome/installer/mini_installer/mini_installer.cc:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
On 2012/07/12 18:37:10, grt wrote:
> 2012

Done.

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/mini_insta...
chrome/installer/mini_installer/mini_installer.cc:26: #include <wchar.h>
On 2012/07/12 08:11:31, tommi wrote:
> out of curiosity, why is this needed?

Extreme IWYU. Obviously too extreme.

Done.

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/mini_insta...
chrome/installer/mini_installer/mini_installer.cc:234: const int kMaxGuids = 10;
On 2012/07/12 18:37:10, grt wrote:
> why not 4?

Done.

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/mini_insta...
chrome/installer/mini_installer/mini_installer.cc:235: const wchar_t*
app_guids[kMaxGuids] = {0};
On 2012/07/12 18:37:10, grt wrote:
> {0} -> {}

Done.

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/mini_insta...
chrome/installer/mini_installer/mini_installer.cc:238: // If this is a multi
install, first try looking in the binaries for the path
On 2012/07/12 18:37:10, grt wrote:
> "path" -> "path."

Done.

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/mini_insta...
chrome/installer/mini_installer/mini_installer.cc:241: // Failing that, look in
Chrome Frame's client state key --chrome-frame was
On 2012/07/12 18:37:10, grt wrote:
> "state key" -> "state key if"

Done.

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/setup/inst...
File chrome/installer/setup/install_worker.cc (right):

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.cc:143: // TODO(erikwright): What does
this mean? Chrome Frame _is_ in Add/Remove, No?
On 2012/07/12 18:37:10, grt wrote:
> On 2012/07/12 08:11:31, tommi wrote:
> > I think the comment is out of date and was probably written early on during
> the
> > multi-install phase.
> 
> I think it refers to ready-mode, where Chrome Frame is installed yet not
present
> in Add/Remove.

Done.

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.cc:1354: // Is Chrome Binaries already on
the machine?
On 2012/07/12 08:11:31, tommi wrote:
> s/Is/Are the

Done.

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/setup/setu...
File chrome/installer/setup/setup_main.cc (right):

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/setup/setu...
chrome/installer/setup/setup_main.cc:403: } else if (chrome_frame &&
chrome_frame->HasOption(installer::kOptionReadyMode)) {
On 2012/07/12 18:37:10, grt wrote:
> 80 cols

Done.

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/setup/setu...
chrome/installer/setup/setup_main.cc:438: bool CheckAppHostPreconditions(const
InstallationState& original_state,
On 2012/07/12 18:37:10, grt wrote:
> please document the failure modes and side effects of this function (e.g.,
that
> the binaries are added to |installer_state| if neither single-install chrome
nor
> any multi-install product is present on the machine).

The logic you refer to moved to InstallerState.

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/setup/setu...
chrome/installer/setup/setup_main.cc:444: return false;
On 2012/07/12 18:37:10, grt wrote:
> VLOG(1) the reason for failure

Done.

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/setup/setu...
chrome/installer/setup/setup_main.cc:445: 
On 2012/07/12 18:37:10, grt wrote:
> if (instaler_state->package_type() != InstallerState::MULTI_PACKAGE) {
>   VLOG(1) << ??;
>   return false;
> }

Done.

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/setup/setu...
chrome/installer/setup/setup_main.cc:447: true /* system level */,
BrowserDistribution::CHROME_BROWSER);
On 2012/07/12 08:11:31, tommi wrote:
> nit: I think we avoid /**/ comments in general. instead you could do:
> GetProductState(
>     true,  // system_level
>     BrowserDistribution::CHROME_BROWSER);
> 
> As a side note, maybe it would be good for readability to change all these
'bool
> system_level' variables to an enum.
>     

Done.

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/setup/setu...
chrome/installer/setup/setup_main.cc:447: true /* system level */,
BrowserDistribution::CHROME_BROWSER);
On 2012/07/12 18:37:10, grt wrote:
> On 2012/07/12 08:11:31, tommi wrote:
> > As a side note, maybe it would be good for readability to change all these
> 'bool
> > system_level' variables to an enum.
> 
> InstallerState::Level could be moved up into the "installer" namespace and
given
> a less collision-prone name.

Not done for this CL.

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/setup/setu...
chrome/installer/setup/setup_main.cc:454: false /* system level */,
BrowserDistribution::CHROME_BINARIES);
On 2012/07/12 18:37:10, grt wrote:
> should the comment be "!system level"?  i find it confusing for the comment to
> be the same for calls with opposing values.

This particular instance is gone.

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/setup/setu...
chrome/installer/setup/setup_main.cc:465: if (!binaries_state && !
installer_state->is_multi_install()) {
On 2012/07/12 18:37:10, grt wrote:
> remove this block as discussed

Done.

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/setup/setu...
chrome/installer/setup/setup_main.cc:694: case kGCFBit & kBrowserBit:
On 2012/07/12 18:37:10, grt wrote:
> i think you mean "kGCFBit | kBrowserBit" here.

Done.

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/setup/setu...
chrome/installer/setup/setup_main.cc:943: // TODO(erikwright): if product 0 is
not chrome, assert chrome not in products
On 2012/07/12 08:11:31, tommi wrote:
> do you intend to do that before checkin?

Done.

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/setup/unin...
File chrome/installer/setup/uninstall.cc (right):

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/setup/unin...
chrome/installer/setup/uninstall.cc:56: //
ScheduleParentAndGrandparentForDeletion?
On 2012/07/12 08:11:31, tommi wrote:
> if so, file a bug for tracking?

I don't know the answer. I'll ask Greg OOB.

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/util/brows...
File chrome/installer/util/browser_distribution.cc (right):

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/util/brows...
chrome/installer/util/browser_distribution.cc:22: #include
"chrome/installer/util/chrome_app_host_distribution.h"
On 2012/07/12 18:37:10, grt wrote:
> ordering

Done.

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/util/brows...
chrome/installer/util/browser_distribution.cc:61: if
(MasterPreferences::ForCurrentProcess().install_chrome_frame() ||
On 2012/07/12 18:37:10, grt wrote:
> did you intentionally change this so that it no longer caches the value?  i
> don't think we want to do this computation on each call.

No. Fixed.

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/util/brows...
chrome/installer/util/browser_distribution.cc:73: const
BrowserDistribution::Type BrowserDistribution::kProductTypes[] = {
On 2012/07/12 18:37:10, grt wrote:
> please check to see if it now makes sense for kProductTypes to go away and for
> all of its users to be converted to simply iterating over the
> BrowserDistribution::Type values from 0 to (but not including)
> BrowserDistribution::NUM_TYPES.  i think it does.  AddExistingMultiInstalls,
for
> example, looks like it should now include the binaries so that they are added
to
> the set of products being operated on.

Done.

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/util/chrom...
File chrome/installer/util/chrome_app_host_operations.cc (right):

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/util/chrom...
chrome/installer/util/chrome_app_host_operations.cc:62: // TODO(erikwright): I
guess this isn't needed?
On 2012/07/12 18:37:10, grt wrote:
> if it's the case now that the binaries' product is always in the
> InstallerState's collection, then it seems reasonable for it to be the
> responsibility of the binaries to put chrome.dll in the key files list.

Done.

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/util/chrom...
File chrome/installer/util/chrome_app_host_operations.h (right):

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/util/chrom...
chrome/installer/util/chrome_app_host_operations.h:15: #include
"base/compiler_specific.h"
On 2012/07/12 08:11:31, tommi wrote:
> fix include order

Done.

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/util/chrom...
File chrome/installer/util/chrome_binaries_operations.cc (right):

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/util/chrom...
chrome/installer/util/chrome_binaries_operations.cc:24: 
On 2012/07/12 18:37:10, grt wrote:
> is it valid for there to be non-multi-install binaries?  should this be
enforced
> in this file?

I've kept that kind of stuff in InstallerState and setup_main. This file doesn't
really have any way to veto the current set of options and I don't think it
should silently change them if something is not as expected.

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/util/chrom...
File chrome/installer/util/chrome_browser_operations.cc (right):

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/util/chrom...
chrome/installer/util/chrome_browser_operations.cc:44: // TODO(erikwright): If
multi, can we omit this?
On 2012/07/12 18:37:10, grt wrote:
> see my comment in the app host's operations file

Decided just to leave it for now.

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/util/chrom...
File chrome/installer/util/chromium_binaries_distribution.cc (right):

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/util/chrom...
chrome/installer/util/chromium_binaries_distribution.cc:80: return L"Uninstall
Chromium Binaries";
On 2012/07/12 18:37:10, grt wrote:
> why?

Some of these NOTREACHED were being hit even though the values are never
actually used.

In the end I fixed it up so no one ever calls the methods.

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/util/chrom...
chrome/installer/util/chromium_binaries_distribution.cc:80: return L"Uninstall
Chromium Binaries";
On 2012/07/13 14:07:41, Jói wrote:
> No localization on this or other user-visible messages here?

They were not actually user-visible. In any case, removed.

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/util/chrom...
chrome/installer/util/chromium_binaries_distribution.cc:84: return
L"Software\\Microsoft\\Windows\\CurrentVersion\\Uninstall\\"
On 2012/07/12 18:37:10, grt wrote:
> the binaries show up in add/remove?

Done.

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/util/chrom...
chrome/installer/util/chromium_binaries_distribution.cc:85: L"Google Chromium
Binaries";
On 2012/07/12 18:37:10, grt wrote:
> this shouldn't have "Google" in the name, although i wonder why we need a
value
> here at all.

Done.

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/util/chrom...
File chrome/installer/util/chromium_binaries_distribution.h (right):

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/util/chrom...
chrome/installer/util/chromium_binaries_distribution.h:50: virtual bool
GetDelegateExecuteHandlerData(string16* handler_class_uuid,
On 2012/07/12 08:11:31, tommi wrote:
> would it make sense to group these into a struct?

Possibly. grt@, when you get back, WDYT?

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/util/insta...
File chrome/installer/util/installation_state.cc (right):

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/util/insta...
chrome/installer/util/installation_state.cc:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
On 2012/07/12 18:37:10, grt wrote:
> 2012

Done.

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/util/insta...
File chrome/installer/util/installation_state.h (right):

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/util/insta...
chrome/installer/util/installation_state.h:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
On 2012/07/12 18:37:10, grt wrote:
> 2012

Done.

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/util/insta...
File chrome/installer/util/installer_state.cc (right):

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/util/insta...
chrome/installer/util/installer_state.cc:188: // product app guids.  Chrome
Frame's will be used if only Chrome Frame
On 2012/07/12 18:37:10, grt wrote:
> is this comment out of date now?  is there such thing as an initial install or
> over install driven by Google Update of the app host alone (i think the answer
> is yes)?

Not driven by Google Update per se, but the App Host at user-level can be
installed via an application command.

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/util/prebu...
File chrome/installer/util/prebuild/create_string_rc.py (right):

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/util/prebu...
chrome/installer/util/prebuild/create_string_rc.py:49:
'IDS_PRODUCT_APP_HOST_NAME',
On 2012/07/12 18:37:10, grt wrote:
> please move these up two lines so they're in order

Done.

[tommi (sloooow) - chröme, 2012-07-18 12:55:40]

lgtm

http://codereview.chromium.org/10665002/diff/28001/chrome/installer/util/chro...
File chrome/installer/util/chrome_app_host_operations.h (right):

http://codereview.chromium.org/10665002/diff/28001/chrome/installer/util/chro...
chrome/installer/util/chrome_app_host_operations.h:14: #include
"base/compiler_specific.h"
nit: is this header actually needed?

http://codereview.chromium.org/10665002/diff/28001/chrome/installer/util/inst...
File chrome/installer/util/installation_validator.cc (right):

http://codereview.chromium.org/10665002/diff/28001/chrome/installer/util/inst...
chrome/installer/util/installation_validator.cc:283: DCHECK(is_valid);
nit: dcheck not needed.  we'll crash anyway.

[erikwright (departed), 2012-07-18 13:27:49]

Is blocked by http://codereview.chromium.org/10559090/ . Will commit after that
one's in.

http://codereview.chromium.org/10665002/diff/28001/chrome/installer/util/chro...
File chrome/installer/util/chrome_app_host_operations.h (right):

http://codereview.chromium.org/10665002/diff/28001/chrome/installer/util/chro...
chrome/installer/util/chrome_app_host_operations.h:14: #include
"base/compiler_specific.h"
On 2012/07/18 12:55:40, tommi wrote:
> nit: is this header actually needed?

OVERRIDE.

http://codereview.chromium.org/10665002/diff/28001/chrome/installer/util/inst...
File chrome/installer/util/installation_validator.cc (right):

http://codereview.chromium.org/10665002/diff/28001/chrome/installer/util/inst...
chrome/installer/util/installation_validator.cc:283: DCHECK(is_valid);
On 2012/07/18 12:55:40, tommi wrote:
> nit: dcheck not needed.  we'll crash anyway.

Will keep for consistency with the rest of this file.

[erikwright (departed), 2012-07-19 05:51:16]

Tommi, just FYI. After your LGTM further testing revealed some needed changes.

No action required from you.

[tommi (sloooow) - chröme, 2012-07-19 05:52:56]

On 2012/07/19 05:51:16, erikwright wrote:
> Tommi, just FYI. After your LGTM further testing revealed some needed changes.
> 
> No action required from you.

Thanks for the heads up. Lgtm still stands.

[grt (UTC plus 2), 2012-07-23 11:31:12]

http://codereview.chromium.org/10665002/diff/6001/chrome/chrome_installer.gypi
File chrome/chrome_installer.gypi (right):

http://codereview.chromium.org/10665002/diff/6001/chrome/chrome_installer.gyp...
chrome/chrome_installer.gypi:265: '../chrome/chrome.gyp:app_host',
On 2012/07/16 20:13:11, erikwright wrote:
> On 2012/07/12 18:37:10, grt wrote:
> > in the past, mini_installer (in chrome/installer/mini_installer.gyp) was the
> > thing that depended on chrome.  is it not suitable to put the dependency on
> > app_host there rather than here?  i think it's desirable to be able to build
> > setup.exe without building chrome and the app host.
> 
> Hm. I guess so. Ultimately it seems pretty useless to build setup without
> building the archive, but these lines don't address that, so I can take them
> out.

FWIW, setup.exe has a growing number of responsibilities that don't require the
archive.

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/mini_insta...
File chrome/installer/mini_installer/mini_installer.cc (right):

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/mini_insta...
chrome/installer/mini_installer/mini_installer.cc:26: #include <wchar.h>
On 2012/07/16 20:13:11, erikwright wrote:
> On 2012/07/12 08:11:31, tommi wrote:
> > out of curiosity, why is this needed?
> 
> Extreme IWYU. Obviously too extreme.

If you added it because the type wchar_t is used in this file, wchar_t is a
keyword in C++-03 so no includes are needed to use it.

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/util/chrom...
File chrome/installer/util/chromium_binaries_distribution.h (right):

http://codereview.chromium.org/10665002/diff/6001/chrome/installer/util/chrom...
chrome/installer/util/chromium_binaries_distribution.h:50: virtual bool
GetDelegateExecuteHandlerData(string16* handler_class_uuid,
On 2012/07/16 20:13:11, erikwright wrote:
> On 2012/07/12 08:11:31, tommi wrote:
> > would it make sense to group these into a struct?
> 
> Possibly. grt@, when you get back, WDYT?

I think it's fine as-is.  Callers can pass NULL for any subset of the parameters
if they only care about a single value.  Using a struct would be more work for
the callers.  I think the interface UUID is actually constant across all
implementations, so it could be removed from the interface.

http://codereview.chromium.org/10665002/diff/32005/chrome/installer/setup/set...
File chrome/installer/setup/setup_main.cc (right):

http://codereview.chromium.org/10665002/diff/32005/chrome/installer/setup/set...
chrome/installer/setup/setup_main.cc:447: << "multi-install.";
nit: i prefer the previous code here since there's only one call to the
streaming operator (and i can't think of a good reason to not let the compiler
concatenate the string constants).

http://codereview.chromium.org/10665002/diff/32005/chrome/installer/setup/uni...
File chrome/installer/setup/uninstall.cc (right):

http://codereview.chromium.org/10665002/diff/32005/chrome/installer/setup/uni...
chrome/installer/setup/uninstall.cc:56: //
ScheduleParentAndGrandparentForDeletion?
i don't understand the question; could you elaborate?