More installer refactoring in the interest of fixing some bugs and cleaning t...

More installer refactoring in the interest of fixing some bugs and cleaning things up:
- Introduced ProductOperations: an interface implemented for each product that takes care of product-specific functions. Each Product owns an instance and delegates certain operations to it.
- Removed the use of MasterPreferences by BrowserDistribution so that the former isn't needed outside of the installer.
- Replaced PackageProperties with a new BrowserDistribution type (CHROME_BINARIES)
- Plumbed the concept of InstallerState more thoroughly through installer
- Removed ProductPackageMapping and Package
- Moved more registry read ops into ProductState
- Validation of products to be installed is now done in CheckPreInstallConditions
- Ignore --chrome-frame --ready-mode if chrome is also being installed/updated and a SxS GCF is found (chrome is updated).
- Migrates existing single-install Chrome to multi-install where appropriate.
- Fixes update to Chrome's uninstallation arguments when Chrome Frame is uninstalled.
- Removed dead code from install.cc.
- Added code to update products' "ap" values when ready-mode is accepted.
- Skip post-install things such as launching the browser when Chrome was implicitly added to the install/upgrade process by virtue of being part of a multi-install.

BUG=61609
TEST=run the installer, see it work.  existing tests in installer_util_unittests have been updated; new tests are included for ProductState, ChannelInfo, etc.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=72497

[grt (UTC plus 2), 2011-01-20 06:07:51]

Sending this out for early review.  Stylistically, there are a number of things
I'd like to do before committing (better commenting, better factoring of new
code), but the guts of it is here.

Erik: please have a look at the ready-mode changes.
Everyone else: May as well start with ProductOperations and friends,
BrowserDistribution, Instal*State classes, and the logic in
CheckPreInstallConditions.

Cheers.

[erikwright (departed), 2011-01-20 15:16:40]

The ready-mode bits LGTM.

http://codereview.chromium.org/6288009/diff/1/chrome/installer/setup/chrome_f...
File chrome/installer/setup/chrome_frame_ready_mode.cc (right):

http://codereview.chromium.org/6288009/diff/1/chrome/installer/setup/chrome_f...
chrome/installer/setup/chrome_frame_ready_mode.cc:72: // Add the two products
we're going to operate on.
Consider using either reference or pointer for both cf and chrome variables. I
don't often see variables declared as references in Chromium, so probably
pointers is more consistent.

http://codereview.chromium.org/6288009/diff/1/chrome/installer/setup/chrome_f...
chrome/installer/setup/chrome_frame_ready_mode.cc:81: if
(!cf->SetOption(kOptionReadyMode, false))
Braces.

[robertshield, 2011-01-20 22:06:06]

First batch: I've reviewed up to browser_distribution.h .. which doesn't seem
like much. Carry on..

http://codereview.chromium.org/6288009/diff/1/chrome/installer/setup/chrome_f...
File chrome/installer/setup/chrome_frame_ready_mode.cc (right):

http://codereview.chromium.org/6288009/diff/1/chrome/installer/setup/chrome_f...
chrome/installer/setup/chrome_frame_ready_mode.cc:70:
installer_state->ResetProducts();
Why not build a new installer_state here? Modifying the input parameter in this
(undocumented) way feels odd. Could use a const & above then too.

http://codereview.chromium.org/6288009/diff/1/chrome/installer/setup/install.cc
File chrome/installer/setup/install.cc (right):

http://codereview.chromium.org/6288009/diff/1/chrome/installer/setup/install....
chrome/installer/setup/install.cc:353: const MasterPreferences& prefs, const
Version& new_version) {
could we put each of these parameters on a separate line?

http://codereview.chromium.org/6288009/diff/1/chrome/installer/setup/install.h
File chrome/installer/setup/install.h (right):

http://codereview.chromium.org/6288009/diff/1/chrome/installer/setup/install....
chrome/installer/setup/install.h:50: const installer::MasterPreferences& prefs,
const Version& new_version);
For readability, can you move each of these parameters to its own line?

http://codereview.chromium.org/6288009/diff/1/chrome/installer/setup/install_...
File chrome/installer/setup/install_worker.cc (right):

http://codereview.chromium.org/6288009/diff/1/chrome/installer/setup/install_...
chrome/installer/setup/install_worker.cc:681: (installer_state.operation() !=
InstallerState::UNINSTALL);
indent

http://codereview.chromium.org/6288009/diff/1/chrome/installer/setup/setup_ma...
File chrome/installer/setup/setup_main.cc (right):

http://codereview.chromium.org/6288009/diff/1/chrome/installer/setup/setup_ma...
chrome/installer/setup/setup_main.cc:191: //     install.
This comment, while useful, is specific to the current planned deployment
scenario. Could we change the wording from "We will push..." to "The supported
install modes are..."

Also, when saying "add it to the set of products to install", should reference
the installer_state parameter to which the product will be added.

http://codereview.chromium.org/6288009/diff/1/chrome/installer/setup/setup_ma...
chrome/installer/setup/setup_main.cc:234: } else if (chrome_frame != NULL) {
This is fair, but we will need explicit integration testing for this code path.

I wonder if we could simply error out here for now until we intend to deploy the
new CF-install cases.

Specifically, I think we do not intend to deploy "--multi --chromeframe" in the
immediate future.

http://codereview.chromium.org/6288009/diff/1/chrome/installer/setup/setup_ma...
chrome/installer/setup/setup_main.cc:320: // There should be no error dialog.
As a nit: having the launching of the existing system level install occur inside
CheckPreInstallConditions() is ugly and is not at all implied by the documented
contract of the function. Could add a comment to the top of the function
describing this behaviour?

http://codereview.chromium.org/6288009/diff/1/chrome/installer/setup/setup_ma...
chrome/installer/setup/setup_main.cc:346: dont_change_enum);
I feel this COMPILE_ASSERT should be next to the installer result enum. It just
adds visual noise while reading this function.

http://codereview.chromium.org/6288009/diff/1/chrome/installer/setup/setup_ma...
chrome/installer/setup/setup_main.cc:377: type)->GetApplicationName();
indent looks like it should be three more spaces in

http://codereview.chromium.org/6288009/diff/1/chrome/installer/setup/setup_ma...
chrome/installer/setup/setup_main.cc:673: InstallerState* installer_state,
Could installer_state be a const ref here?

http://codereview.chromium.org/6288009/diff/1/chrome/installer/setup/setup_ma...
chrome/installer/setup/setup_main.cc:802:
installer::ChromeFrameReadyModeOptIn(original_state, installer_state));
I think this is the only place you need to pass a pointer to  installer_state
but I believe the OptIn method could be similarly modified to take a const ref.

http://codereview.chromium.org/6288009/diff/1/chrome/installer/setup/setup_ma...
chrome/installer/setup/setup_main.cc:1035: // If --uninstall option is given,
uninstall chrome
This comment looks out of date.

http://codereview.chromium.org/6288009/diff/1/chrome/installer/setup/setup_ma...
chrome/installer/setup/setup_main.cc:1045: install_status =
InstallChrome(original_state, cmd_line, prefs,
InstallChrome -> InstallProducts?

http://codereview.chromium.org/6288009/diff/1/chrome/installer/setup/uninstal...
File chrome/installer/setup/uninstall.cc (right):

http://codereview.chromium.org/6288009/diff/1/chrome/installer/setup/uninstal...
chrome/installer/setup/uninstall.cc:57: DCHECK(product_state != NULL);
DCHECK(installer_state.multi_install());

http://codereview.chromium.org/6288009/diff/1/chrome/installer/setup/uninstal...
chrome/installer/setup/uninstall.cc:82:
!product_state->channel().Equals(channel_info)) {
shouldn't product_state->channel().Equals(channel_info) always be true, i.e.
shouldn't we always have modified the channels? If so, a DCHECK / LOG(DFATAL)
seems more appropriate.

http://codereview.chromium.org/6288009/diff/1/chrome/installer/setup/uninstal...
chrome/installer/setup/uninstall.cc:658: suffix, ret);
I know this is old code, but this is silly. DeleteChromeRegistrationKeys sticks
a value in ret that is then ignored and overwritten later on.

[robertshield, 2011-01-20 23:36:34]

Some more comments, getting there...

http://codereview.chromium.org/6288009/diff/1/chrome/installer/util/channel_i...
File chrome/installer/util/channel_info.cc (right):

http://codereview.chromium.org/6288009/diff/1/chrome/installer/util/channel_i...
chrome/installer/util/channel_info.cc:156: bool ChannelInfo::EqualsBaseOf(const
ChannelInfo& other) const {
is there a test added for this?

http://codereview.chromium.org/6288009/diff/1/chrome/installer/util/channel_i...
File chrome/installer/util/channel_info.h (right):

http://codereview.chromium.org/6288009/diff/1/chrome/installer/util/channel_i...
chrome/installer/util/channel_info.h:45: // when all modifiers and suffixes are
omitted).
|this| is somewhat unconventional. Replace with "this object".

http://codereview.chromium.org/6288009/diff/1/chrome/installer/util/chrome_br...
File chrome/installer/util/chrome_browser_operations.h (right):

http://codereview.chromium.org/6288009/diff/1/chrome/installer/util/chrome_br...
chrome/installer/util/chrome_browser_operations.h:19: class
ChromeBrowserOperations : public ProductOperations {
Perhaps a comment referring the reader to ProductOperations for documentation?

http://codereview.chromium.org/6288009/diff/1/chrome/installer/util/chrome_fr...
File chrome/installer/util/chrome_frame_distribution.cc (left):

http://codereview.chromium.org/6288009/diff/1/chrome/installer/util/chrome_fr...
chrome/installer/util/chrome_frame_distribution.cc:61: }
nice deletions!

http://codereview.chromium.org/6288009/diff/1/chrome/installer/util/helper.h
File chrome/installer/util/helper.h (right):

http://codereview.chromium.org/6288009/diff/1/chrome/installer/util/helper.h#...
chrome/installer/util/helper.h:42: // with the location at which it was found.
app_guid doesn't actually get a location though right, it only gets the guid?

http://codereview.chromium.org/6288009/diff/1/chrome/installer/util/product_o...
File chrome/installer/util/product_operations.h (right):

http://codereview.chromium.org/6288009/diff/1/chrome/installer/util/product_o...
chrome/installer/util/product_operations.h:35: // |options|.
Naming the parameter "uninstall_command" conflates function with usage. If this
is only intended to be used with uninstall command lines, it should be called
ReadOptionsFromUninstallCommandLine. Implementations should then also DCHECK
that uninstall_command contains "--uninstall".

I think that's the wrong way to go though, how about renaming the parameter to
simply "command" and leaving the usage of this method generic?

[grt (UTC plus 2), 2011-01-21 05:27:51]

I've backed out the chrome.exe renaming change, as I felt it had the potential
to be risky.  I've also fixed up setup_unittests, which I'd missed the first
time around.  PTAL.

http://codereview.chromium.org/6288009/diff/1/chrome/installer/setup/chrome_f...
File chrome/installer/setup/chrome_frame_ready_mode.cc (right):

http://codereview.chromium.org/6288009/diff/1/chrome/installer/setup/chrome_f...
chrome/installer/setup/chrome_frame_ready_mode.cc:70:
installer_state->ResetProducts();
On 2011/01/20 22:06:06, robertshield wrote:
> Why not build a new installer_state here? Modifying the input parameter in
this
> (undocumented) way feels odd. Could use a const & above then too.

Done.

http://codereview.chromium.org/6288009/diff/1/chrome/installer/setup/chrome_f...
chrome/installer/setup/chrome_frame_ready_mode.cc:72: // Add the two products
we're going to operate on.
On 2011/01/20 15:16:40, erikwright wrote:
> Consider using either reference or pointer for both cf and chrome variables. I
> don't often see variables declared as references in Chromium, so probably
> pointers is more consistent.

Done.

http://codereview.chromium.org/6288009/diff/1/chrome/installer/setup/chrome_f...
chrome/installer/setup/chrome_frame_ready_mode.cc:81: if
(!cf->SetOption(kOptionReadyMode, false))
On 2011/01/20 15:16:40, erikwright wrote:
> Braces.

Done.

http://codereview.chromium.org/6288009/diff/1/chrome/installer/setup/install.cc
File chrome/installer/setup/install.cc (right):

http://codereview.chromium.org/6288009/diff/1/chrome/installer/setup/install....
chrome/installer/setup/install.cc:353: const MasterPreferences& prefs, const
Version& new_version) {
On 2011/01/20 22:06:06, robertshield wrote:
> could we put each of these parameters on a separate line?

Done.

http://codereview.chromium.org/6288009/diff/1/chrome/installer/setup/install.h
File chrome/installer/setup/install.h (right):

http://codereview.chromium.org/6288009/diff/1/chrome/installer/setup/install....
chrome/installer/setup/install.h:50: const installer::MasterPreferences& prefs,
const Version& new_version);
On 2011/01/20 22:06:06, robertshield wrote:
> For readability, can you move each of these parameters to its own line?

Done.

http://codereview.chromium.org/6288009/diff/1/chrome/installer/setup/install_...
File chrome/installer/setup/install_worker.cc (right):

http://codereview.chromium.org/6288009/diff/1/chrome/installer/setup/install_...
chrome/installer/setup/install_worker.cc:681: (installer_state.operation() !=
InstallerState::UNINSTALL);
On 2011/01/20 22:06:06, robertshield wrote:
> indent

Done.

http://codereview.chromium.org/6288009/diff/1/chrome/installer/setup/setup_ma...
File chrome/installer/setup/setup_main.cc (right):

http://codereview.chromium.org/6288009/diff/1/chrome/installer/setup/setup_ma...
chrome/installer/setup/setup_main.cc:191: //     install.
On 2011/01/20 22:06:06, robertshield wrote:
> This comment, while useful, is specific to the current planned deployment
> scenario. Could we change the wording from "We will push..." to "The supported
> install modes are..."
> 
> Also, when saying "add it to the set of products to install", should reference
> the installer_state parameter to which the product will be added.

Done.

http://codereview.chromium.org/6288009/diff/1/chrome/installer/setup/setup_ma...
chrome/installer/setup/setup_main.cc:234: } else if (chrome_frame != NULL) {
On 2011/01/20 22:06:06, robertshield wrote:
> This is fair, but we will need explicit integration testing for this code
path.
> 
> I wonder if we could simply error out here for now until we intend to deploy
the
> new CF-install cases.
> 
> Specifically, I think we do not intend to deploy "--multi --chromeframe" in
the
> immediate future.

It seems fairly straightforward.  Are you comfortable with punting on the
integration testing for now but leaving the code as-is?

http://codereview.chromium.org/6288009/diff/1/chrome/installer/setup/setup_ma...
chrome/installer/setup/setup_main.cc:320: // There should be no error dialog.
On 2011/01/20 22:06:06, robertshield wrote:
> As a nit: having the launching of the existing system level install occur
inside
> CheckPreInstallConditions() is ugly and is not at all implied by the
documented
> contract of the function. Could add a comment to the top of the function
> describing this behaviour?

Done.

http://codereview.chromium.org/6288009/diff/1/chrome/installer/setup/setup_ma...
chrome/installer/setup/setup_main.cc:346: dont_change_enum);
On 2011/01/20 22:06:06, robertshield wrote:
> I feel this COMPILE_ASSERT should be next to the installer result enum. It
just
> adds visual noise while reading this function.

Me too, but it doesn't seem to work there for some reason.

http://codereview.chromium.org/6288009/diff/1/chrome/installer/setup/setup_ma...
chrome/installer/setup/setup_main.cc:377: type)->GetApplicationName();
On 2011/01/20 22:06:06, robertshield wrote:
> indent looks like it should be three more spaces in

Done.

http://codereview.chromium.org/6288009/diff/1/chrome/installer/setup/setup_ma...
chrome/installer/setup/setup_main.cc:673: InstallerState* installer_state,
On 2011/01/20 22:06:06, robertshield wrote:
> Could installer_state be a const ref here?

Done.

http://codereview.chromium.org/6288009/diff/1/chrome/installer/setup/setup_ma...
chrome/installer/setup/setup_main.cc:802:
installer::ChromeFrameReadyModeOptIn(original_state, installer_state));
On 2011/01/20 22:06:06, robertshield wrote:
> I think this is the only place you need to pass a pointer to  installer_state
> but I believe the OptIn method could be similarly modified to take a const
ref.

Done.

http://codereview.chromium.org/6288009/diff/1/chrome/installer/setup/setup_ma...
chrome/installer/setup/setup_main.cc:1035: // If --uninstall option is given,
uninstall chrome
On 2011/01/20 22:06:06, robertshield wrote:
> This comment looks out of date.

Done.

http://codereview.chromium.org/6288009/diff/1/chrome/installer/setup/setup_ma...
chrome/installer/setup/setup_main.cc:1045: install_status =
InstallChrome(original_state, cmd_line, prefs,
On 2011/01/20 22:06:06, robertshield wrote:
> InstallChrome -> InstallProducts?

Done.

http://codereview.chromium.org/6288009/diff/1/chrome/installer/setup/uninstal...
File chrome/installer/setup/uninstall.cc (right):

http://codereview.chromium.org/6288009/diff/1/chrome/installer/setup/uninstal...
chrome/installer/setup/uninstall.cc:57: DCHECK(product_state != NULL);
On 2011/01/20 22:06:06, robertshield wrote:
> DCHECK(installer_state.multi_install());

Done.

http://codereview.chromium.org/6288009/diff/1/chrome/installer/setup/uninstal...
chrome/installer/setup/uninstall.cc:82:
!product_state->channel().Equals(channel_info)) {
On 2011/01/20 22:06:06, robertshield wrote:
> shouldn't product_state->channel().Equals(channel_info) always be true, i.e.
> shouldn't we always have modified the channels? If so, a DCHECK / LOG(DFATAL)
> seems more appropriate.

Actually, we expect that the two will never be equal (if CF is being
uninstalled, Chrome's ap should change from "blah-chromeframe-deblah" to just
"blah-deblah").  If they are equal, then 1) someone must have diddled the
registry, and 2) we don't need to store the new value because the current value
is desired.  I'll add a DFATAL so we can see if this ever happens.

http://codereview.chromium.org/6288009/diff/1/chrome/installer/setup/uninstal...
chrome/installer/setup/uninstall.cc:658: suffix, ret);
On 2011/01/20 22:06:06, robertshield wrote:
> I know this is old code, but this is silly. DeleteChromeRegistrationKeys
sticks
> a value in ret that is then ignored and overwritten later on.

The exit code returned via that parameter is used by setup_main when handling
--remove-chrome-registration; the uninstaller ignores it.

http://codereview.chromium.org/6288009/diff/1/chrome/installer/util/channel_i...
File chrome/installer/util/channel_info.cc (right):

http://codereview.chromium.org/6288009/diff/1/chrome/installer/util/channel_i...
chrome/installer/util/channel_info.cc:156: bool ChannelInfo::EqualsBaseOf(const
ChannelInfo& other) const {
On 2011/01/20 23:36:34, robertshield wrote:
> is there a test added for this?

Done.

http://codereview.chromium.org/6288009/diff/1/chrome/installer/util/channel_i...
File chrome/installer/util/channel_info.h (right):

http://codereview.chromium.org/6288009/diff/1/chrome/installer/util/channel_i...
chrome/installer/util/channel_info.h:45: // when all modifiers and suffixes are
omitted).
On 2011/01/20 23:36:34, robertshield wrote:
> |this| is somewhat unconventional. Replace with "this object".

Done.

http://codereview.chromium.org/6288009/diff/1/chrome/installer/util/chrome_br...
File chrome/installer/util/chrome_browser_operations.h (right):

http://codereview.chromium.org/6288009/diff/1/chrome/installer/util/chrome_br...
chrome/installer/util/chrome_browser_operations.h:19: class
ChromeBrowserOperations : public ProductOperations {
On 2011/01/20 23:36:34, robertshield wrote:
> Perhaps a comment referring the reader to ProductOperations for documentation?

Done.

http://codereview.chromium.org/6288009/diff/1/chrome/installer/util/chrome_fr...
File chrome/installer/util/chrome_frame_distribution.cc (left):

http://codereview.chromium.org/6288009/diff/1/chrome/installer/util/chrome_fr...
chrome/installer/util/chrome_frame_distribution.cc:61: }
On 2011/01/20 23:36:34, robertshield wrote:
> nice deletions!

Thanks.

http://codereview.chromium.org/6288009/diff/1/chrome/installer/util/helper.h
File chrome/installer/util/helper.h (right):

http://codereview.chromium.org/6288009/diff/1/chrome/installer/util/helper.h#...
chrome/installer/util/helper.h:42: // with the location at which it was found.
On 2011/01/20 23:36:34, robertshield wrote:
> app_guid doesn't actually get a location though right, it only gets the guid?

This code has been removed.

http://codereview.chromium.org/6288009/diff/1/chrome/installer/util/product_o...
File chrome/installer/util/product_operations.h (right):

http://codereview.chromium.org/6288009/diff/1/chrome/installer/util/product_o...
chrome/installer/util/product_operations.h:35: // |options|.
On 2011/01/20 23:36:34, robertshield wrote:
> Naming the parameter "uninstall_command" conflates function with usage. If
this
> is only intended to be used with uninstall command lines, it should be called
> ReadOptionsFromUninstallCommandLine. Implementations should then also DCHECK
> that uninstall_command contains "--uninstall".
> 
> I think that's the wrong way to go though, how about renaming the parameter to
> simply "command" and leaving the usage of this method generic?

Done.

[robertshield, 2011-01-21 16:58:56]

More nits

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

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/util/insta...
chrome/installer/util/installation_state.cc:16: const std::wstring&
uninstall_arguments) {
This looks non-trivial enough to warrant a small unit test.

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/util/insta...
chrome/installer/util/installation_state.cc:57: BrowserDistribution*
distribution) {
I'd feel better if this had a test too. The test could hand it a mock browser
distribution that lied about the state and version keys.

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/util/insta...
chrome/installer/util/installation_state.cc:76: != ERROR_SUCCESS)
NIT WARNING: I find the following more readable:

if (key.ReadValue(google_update::kRegRenameCmdField,
                  &rename_cmd_) != ERROR_SUCCESS)
  rename_cmd.clear();

Also, https://sites.google.com/a/chromium.org/dev/developers/coding-style states
that lines should not begin with operators.

The same applies to a the other places in this method.

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

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/util/insta...
chrome/installer/util/installation_state.h:58: void set_msi(bool set) { msi_ =
set; }
Remove the public setters for this and other fields if they are only used for
testing.

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

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/util/insta...
chrome/installer/util/installer_state.cc:204: const ProductState& state) {
This method should return NULL when package_type_ is SINGLE_PACKAGE, to avoid
getting the InstallerState instance into an invalid state in Release builds.

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/util/insta...
File chrome/installer/util/installer_state.h (right):

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/util/insta...
chrome/installer/util/installer_state.h:135: void
RemoveOldVersionDirectories(const Version& latest_version) const;
Please add comment.

[robertshield, 2011-01-21 19:37:46]

Yay! Got to the end!

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/util/chrom...
File chrome/installer/util/chrome_frame_operations.cc (right):

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/util/chrom...
chrome/installer/util/chrome_frame_operations.cc:62: std::vector<FilePath>*
key_files) {
DCHECK(key_files);

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/util/chrom...
chrome/installer/util/chrome_frame_operations.cc:71: std::vector<FilePath>*
com_dll_list) {
DCHECK(com_dll_list);

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

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/util/insta...
chrome/installer/util/installer_state.cc:349: return modified;
this is neat, but the following (or a non-broken version thereof) looks simpler:

bool modified = false;
Products::iterator iter(products.begin());
for (; iter != products.end(); ++iter) {
  modified |= iter->SetChannelFlags(set, channel_info);
}
return modified;

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/util/insta...
File chrome/installer/util/installer_state_unittest.cc (right):

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/util/insta...
chrome/installer/util/installer_state_unittest.cc:101:
ASSERT_TRUE(file_util::PathExists(chrome_dll_4));
This setup code above looks like it should be in the InstallerStateTest fixture.
It is also mostly shared with the next test.

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/util/insta...
chrome/installer/util/installer_state_unittest.cc:111: != NULL);
Add expectations that CHROME_FRAME is not present?

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/util/insta...
chrome/installer/util/installer_state_unittest.cc:193: != NULL);
This looks copy/pasted. Stick it in a method?

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/util/produ...
File chrome/installer/util/product_operations.h (right):

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/util/produ...
chrome/installer/util/product_operations.h:63: ChannelInfo* channel_info) const
= 0;
why are these methods const and not the other ones. Given that the header at the
top of this interfaces suggests that implementations should be stateless, I'd
expect that all of the methods could be const.

[amit, 2011-01-21 20:01:49]

lgtm as long as other reviewers are happy, Thanks!

[tommi (sloooow) - chröme, 2011-01-21 21:45:16]

I'm up to installer_state_unittest.cc.  Here's what I've got so far.

http://codereview.chromium.org/6288009/diff/1/chrome/browser/first_run/first_...
File chrome/browser/first_run/first_run_win.cc (right):

http://codereview.chromium.org/6288009/diff/1/chrome/browser/first_run/first_...
chrome/browser/first_run/first_run_win.cc:271: std::wstring rename_cmd =
installer::GetRenameCommand(&app_guid);
general style nit: We prefer std::string foo(Bar()); to std::string foo ==
Bar();

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

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.cc:157: (installer_state.operation() !=
InstallerState::UNINSTALL)) {
nit: parenthesis not necessary

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

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/setup/setu...
chrome/installer/setup/setup_main.cc:257: base::win::RegKey key;
if ready-mode is being installed you need to check if Chrome is also installed
as multi.

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/setup/setu...
chrome/installer/setup/setup_main.cc:278: chrome =
installer_state->AddProductFromState(
is there a missing check for multi first?

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/setup/setu...
chrome/installer/setup/setup_main.cc:284: }
} else {
  Check if ready-mode is being asked for.
  If it is and Chrome is not installed, we must abort.
}

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/setup/setu...
chrome/installer/setup/setup_main.cc:286: }
} else {
  abort if ready-mode is specified?
  abort if more than one product is being installed?
}

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/setup/setu...
chrome/installer/setup/setup_main.cc:408: }
just a general comment on this function.  It's getting so long that it's hard to
wrap one's head around all the different loops and checks.  It would be good to
break it up into more easily digestible bites.

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/util/chrom...
File chrome/installer/util/chrome_browser_operations.h (right):

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/util/chrom...
chrome/installer/util/chrome_browser_operations.h:25: std::set<std::wstring>*
options) OVERRIDE;
I think the "= 0;" approach is more common. Is chrome moving towards using
OVERRIDE?

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

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/util/insta...
chrome/installer/util/installation_state.cc:104: switches::kMultiInstall);
braces

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

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/util/insta...
chrome/installer/util/installer_state.cc:98: if
(!prefs.GetBool(master_preferences::kVerboseLogging, &verbose_logging_))
in order to get rid of this pattern we should add a method, GetBool that accepts
a default value and returns it if the preference isn't set.  Then we could
write:

verbose_logging_ = prefs.GetBool(kVerboseLogging, false);

and lose quite a few if's.

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/util/insta...
chrome/installer/util/installer_state.cc:233: std::find(products_.begin(),
products_.end(), product);
indent

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/util/insta...
chrome/installer/util/installer_state.cc:246: distribution_type));
this feels less readable to me than a simple loop that does the same.  (bind2nd
and mem_fun are not in my (granted, shallow) stl cache, so I had to look them up
to see how they work).

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/util/insta...
chrome/installer/util/installer_state.cc:299: if (version.get() &&
(latest_version.CompareTo(*version) > 0)) {
no need for () around latest_version check.

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/util/insta...
chrome/installer/util/installer_state.cc:349: return modified;
On 2011/01/21 19:37:47, robertshield wrote:
> this is neat, but the following (or a non-broken version thereof) looks
simpler:
> 
> bool modified = false;
> Products::iterator iter(products.begin());
> for (; iter != products.end(); ++iter) {
>   modified |= iter->SetChannelFlags(set, channel_info);
> }
> return modified;

+1

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/util/insta...
File chrome/installer/util/installer_state.h (right):

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/util/insta...
chrome/installer/util/installer_state.h:20: #include <windows.h>
I'm assuming you can't move this to the top due to the OS_WIN check, so I think
you should add the NOLINT tag.

[grt (UTC plus 2), 2011-01-24 16:07:02]

I've added some new unit tests, improved the implementation of InstallerState,
added some new comments, and generally made the CL better.  PTAL.

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

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.cc:157: (installer_state.operation() !=
InstallerState::UNINSTALL)) {
On 2011/01/21 21:45:17, tommi wrote:
> nit: parenthesis not necessary

I thought I'd seen extra parens thrown about in the code for the sake of
readability, I suppose.  I'm more than happy to remove them.

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

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/setup/setu...
chrome/installer/setup/setup_main.cc:257: base::win::RegKey key;
On 2011/01/21 21:45:17, tommi wrote:
> if ready-mode is being installed you need to check if Chrome is also installed
> as multi.

Done.  Thanks for the patch you sent for doing this previously.

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/setup/setu...
chrome/installer/setup/setup_main.cc:278: chrome =
installer_state->AddProductFromState(
On 2011/01/21 21:45:17, tommi wrote:
> is there a missing check for multi first?

In this case, we'll transition an existing single Chrome into a multi Chrome;
processing is the same regardless, so no need to check.  I see, however, that
this current code would blow up during the transition, so I'll fix that up right
quick.

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/setup/setu...
chrome/installer/setup/setup_main.cc:284: }
On 2011/01/21 21:45:17, tommi wrote:
> } else {
>   Check if ready-mode is being asked for.
>   If it is and Chrome is not installed, we must abort.
> }

Done.

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/setup/setu...
chrome/installer/setup/setup_main.cc:286: }
On 2011/01/21 21:45:17, tommi wrote:
> } else {
>   abort if ready-mode is specified?
>   abort if more than one product is being installed?
> }

For the former, setup.exe currently silently ignores command line options that
don't apply for a given operation.  I've just made this the case for ready-mode
(see chrome_frame_operations.cc).  Do you think it's necessary to abort if
--ready-mode is present for a single-install Chrome Frame rather than simply
ignore it?

For the latter, I've added a DCHECK for this since InstallerState should already
prohibit it from ever happening.

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/setup/setu...
chrome/installer/setup/setup_main.cc:408: }
On 2011/01/21 21:45:17, tommi wrote:
> just a general comment on this function.  It's getting so long that it's hard
to
> wrap one's head around all the different loops and checks.  It would be good
to
> break it up into more easily digestible bites.

Totally.  For now, I've pulled out two funcs.

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/util/chrom...
File chrome/installer/util/chrome_browser_operations.h (right):

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/util/chrom...
chrome/installer/util/chrome_browser_operations.h:25: std::set<std::wstring>*
options) OVERRIDE;
On 2011/01/21 21:45:17, tommi wrote:
> I think the "= 0;" approach is more common. Is chrome moving towards using
> OVERRIDE?

"= 0;" makes a method pure-virtual.  "OVERRIDE" is a macro that, where
available, expands to compiler-specific juju that throws an error if a method in
a subclass isn't actually overriding anything (e.g., if someone tweaks the base
interface but forgets to change everyone who inherits from it).  The macro was
discussed on chromium-dev a few weeks ago.  I'm a fan of it since it lets MSVC
catch a fairly common error.  Looks like it does something meaningful for clang,
too.

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/util/chrom...
File chrome/installer/util/chrome_frame_operations.cc (right):

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/util/chrom...
chrome/installer/util/chrome_frame_operations.cc:62: std::vector<FilePath>*
key_files) {
On 2011/01/21 19:37:47, robertshield wrote:
> DCHECK(key_files);

Done.

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/util/chrom...
chrome/installer/util/chrome_frame_operations.cc:71: std::vector<FilePath>*
com_dll_list) {
On 2011/01/21 19:37:47, robertshield wrote:
> DCHECK(com_dll_list);

Done.

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

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/util/insta...
chrome/installer/util/installation_state.cc:16: const std::wstring&
uninstall_arguments) {
On 2011/01/21 16:58:56, robertshield wrote:
> This looks non-trivial enough to warrant a small unit test.

Done.

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/util/insta...
chrome/installer/util/installation_state.cc:57: BrowserDistribution*
distribution) {
On 2011/01/21 16:58:56, robertshield wrote:
> I'd feel better if this had a test too. The test could hand it a mock browser
> distribution that lied about the state and version keys.

Done; see product_state_unittest.cc.

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/util/insta...
chrome/installer/util/installation_state.cc:76: != ERROR_SUCCESS)
On 2011/01/21 16:58:56, robertshield wrote:
> NIT WARNING: I find the following more readable:
> 
> if (key.ReadValue(google_update::kRegRenameCmdField,
>                   &rename_cmd_) != ERROR_SUCCESS)
>   rename_cmd.clear();
> 
> Also, https://sites.google.com/a/chromium.org/dev/developers/coding-style
states
> that lines should not begin with operators.
> 
> The same applies to a the other places in this method.

I like that better, too.  Thanks.

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/util/insta...
chrome/installer/util/installation_state.cc:104: switches::kMultiInstall);
On 2011/01/21 21:45:17, tommi wrote:
> braces

Done.

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

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/util/insta...
chrome/installer/util/installation_state.h:58: void set_msi(bool set) { msi_ =
set; }
On 2011/01/21 16:58:56, robertshield wrote:
> Remove the public setters for this and other fields if they are only used for
> testing.

Done.

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

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/util/insta...
chrome/installer/util/installer_state.cc:98: if
(!prefs.GetBool(master_preferences::kVerboseLogging, &verbose_logging_))
On 2011/01/21 21:45:17, tommi wrote:
> in order to get rid of this pattern we should add a method, GetBool that
accepts
> a default value and returns it if the preference isn't set.  Then we could
> write:
> 
> verbose_logging_ = prefs.GetBool(kVerboseLogging, false);
> 
> and lose quite a few if's.

<nods head>

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/util/insta...
chrome/installer/util/installer_state.cc:204: const ProductState& state) {
On 2011/01/21 16:58:56, robertshield wrote:
> This method should return NULL when package_type_ is SINGLE_PACKAGE, to avoid
> getting the InstallerState instance into an invalid state in Release builds.

Done.  Also if the target_path would conflict.

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/util/insta...
chrome/installer/util/installer_state.cc:233: std::find(products_.begin(),
products_.end(), product);
On 2011/01/21 21:45:17, tommi wrote:
> indent

Done.

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/util/insta...
chrome/installer/util/installer_state.cc:246: distribution_type));
On 2011/01/21 21:45:17, tommi wrote:
> this feels less readable to me than a simple loop that does the same. 
(bind2nd
> and mem_fun are not in my (granted, shallow) stl cache, so I had to look them
up
> to see how they work).

Okay.

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/util/insta...
chrome/installer/util/installer_state.cc:299: if (version.get() &&
(latest_version.CompareTo(*version) > 0)) {
On 2011/01/21 21:45:17, tommi wrote:
> no need for () around latest_version check.

Done.

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/util/insta...
chrome/installer/util/installer_state.cc:349: return modified;
On 2011/01/21 21:45:17, tommi wrote:
> On 2011/01/21 19:37:47, robertshield wrote:
> > this is neat, but the following (or a non-broken version thereof) looks
> simpler:
> > 
> > bool modified = false;
> > Products::iterator iter(products.begin());
> > for (; iter != products.end(); ++iter) {
> >   modified |= iter->SetChannelFlags(set, channel_info);
> > }
> > return modified;
> 
> +1

Okay.  I can't wait for lambdas...

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/util/insta...
File chrome/installer/util/installer_state.h (right):

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/util/insta...
chrome/installer/util/installer_state.h:20: #include <windows.h>
On 2011/01/21 21:45:17, tommi wrote:
> I'm assuming you can't move this to the top due to the OS_WIN check, so I
think
> you should add the NOLINT tag.

Right.  My understanding is that platform-specific stuff comes after everything
else.  Thanks for the NOLINT tip; done.

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/util/insta...
chrome/installer/util/installer_state.h:135: void
RemoveOldVersionDirectories(const Version& latest_version) const;
On 2011/01/21 16:58:56, robertshield wrote:
> Please add comment.

Done.

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/util/insta...
File chrome/installer/util/installer_state_unittest.cc (right):

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/util/insta...
chrome/installer/util/installer_state_unittest.cc:101:
ASSERT_TRUE(file_util::PathExists(chrome_dll_4));
On 2011/01/21 19:37:47, robertshield wrote:
> This setup code above looks like it should be in the InstallerStateTest
fixture.
> It is also mostly shared with the next test.

Adding TODO to clean this up since it pre-dates this change.

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/util/insta...
chrome/installer/util/installer_state_unittest.cc:111: != NULL);
On 2011/01/21 19:37:47, robertshield wrote:
> Add expectations that CHROME_FRAME is not present?

That would be testing something unrelated to what this test is doing, but okay.

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/util/insta...
chrome/installer/util/installer_state_unittest.cc:193: != NULL);
On 2011/01/21 19:37:47, robertshield wrote:
> This looks copy/pasted. Stick it in a method?

Done.

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/util/produ...
File chrome/installer/util/product_operations.h (right):

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/util/produ...
chrome/installer/util/product_operations.h:63: ChannelInfo* channel_info) const
= 0;
On 2011/01/21 19:37:47, robertshield wrote:
> why are these methods const and not the other ones. Given that the header at
the
> top of this interfaces suggests that implementations should be stateless, I'd
> expect that all of the methods could be const.

Done.

[grt (UTC plus 2), 2011-01-24 19:26:05]

I've unlocked two new levels:
- Existing single Chrome installs are now migrated to multi.  This is required
to send ready-mode down to existing installations.
- The various post-install things like launching the browser no longer take
place if the browser wasn't actually asked for (e.g., when multi Chrome is
updated as a result of installing Chrome Frame).

[tommi (sloooow) - chröme, 2011-01-24 19:39:39]

next batch of comments.  excellent work.

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

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.cc:157: (installer_state.operation() !=
InstallerState::UNINSTALL)) {
On 2011/01/24 16:07:02, grt wrote:
> On 2011/01/21 21:45:17, tommi wrote:
> > nit: parenthesis not necessary
> 
> I thought I'd seen extra parens thrown about in the code for the sake of
> readability, I suppose.  I'm more than happy to remove them.

I've seen those too, but where they help with readability (imho) is when we're
mixing boolean and binary operators.  E.g.

if (MY_FLAG & flags || stuff)
vs
if ((flags & MY_FLAG) || stuff)

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

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/setup/setu...
chrome/installer/setup/setup_main.cc:286: }
On 2011/01/24 16:07:02, grt wrote:
> On 2011/01/21 21:45:17, tommi wrote:
> > } else {
> >   abort if ready-mode is specified?
> >   abort if more than one product is being installed?
> > }
> 
> For the former, setup.exe currently silently ignores command line options that
> don't apply for a given operation.  I've just made this the case for
ready-mode
> (see chrome_frame_operations.cc).  Do you think it's necessary to abort if
> --ready-mode is present for a single-install Chrome Frame rather than simply
> ignore it?

Hmm.. maybe we should just log a warning for now.

> 
> For the latter, I've added a DCHECK for this since InstallerState should
already
> prohibit it from ever happening.

Cool.

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/util/chrom...
File chrome/installer/util/chrome_browser_operations.h (right):

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/util/chrom...
chrome/installer/util/chrome_browser_operations.h:25: std::set<std::wstring>*
options) OVERRIDE;
On 2011/01/24 16:07:02, grt wrote:
> On 2011/01/21 21:45:17, tommi wrote:
> > I think the "= 0;" approach is more common. Is chrome moving towards using
> > OVERRIDE?
> 
> "= 0;" makes a method pure-virtual.  "OVERRIDE" is a macro that, where
> available, expands to compiler-specific juju that throws an error if a method
in
> a subclass isn't actually overriding anything (e.g., if someone tweaks the
base
> interface but forgets to change everyone who inherits from it).  The macro was
> discussed on chromium-dev a few weeks ago.  I'm a fan of it since it lets MSVC
> catch a fairly common error.  Looks like it does something meaningful for
clang,
> too.

Thanks for the info!  In that case it does sound valuable.

http://codereview.chromium.org/6288009/diff/110002/chrome/installer/setup/chr...
File chrome/installer/setup/chrome_frame_ready_mode.cc (right):

http://codereview.chromium.org/6288009/diff/110002/chrome/installer/setup/chr...
chrome/installer/setup/chrome_frame_ready_mode.cc:217: std::wstring
chrome_frame_ua_value_name = kChromeFramePrefix;
nit: chrome_frame_ua_value_name(kChromeFramePrefix);

http://codereview.chromium.org/6288009/diff/110002/chrome/installer/setup/ins...
File chrome/installer/setup/install_worker.cc (right):

http://codereview.chromium.org/6288009/diff/110002/chrome/installer/setup/ins...
chrome/installer/setup/install_worker.cc:154: // in case of uninstall since this
means that Chrome Frame is being
chrome frame is being uninstalled?  Wouldn't it mean that chrome is being
uninstalled?  or am I misunderstanding the comment...

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

http://codereview.chromium.org/6288009/diff/110002/chrome/installer/setup/set...
chrome/installer/setup/setup_main.cc:226: "than installing in ready-mode.";
indent off by one

http://codereview.chromium.org/6288009/diff/110002/chrome/installer/setup/set...
chrome/installer/setup/setup_main.cc:228: VLOG(1) << "Skipping upgrade of
single-install Chrome Frame rather "
LOG(INFO)?

http://codereview.chromium.org/6288009/diff/110002/chrome/installer/setup/set...
chrome/installer/setup/setup_main.cc:241: *status =
installer::NON_MULTI_INSTALLATION_EXISTS;
LOG(ERROR)

http://codereview.chromium.org/6288009/diff/110002/chrome/installer/setup/set...
chrome/installer/setup/setup_main.cc:265:
InstallUtil::WriteInstallerResult(system_level,
LOG(ERROR)

http://codereview.chromium.org/6288009/diff/110002/chrome/installer/setup/set...
chrome/installer/setup/setup_main.cc:277: VLOG(1) << "Upgrading existing
multi-install Chrome browser along with "
LOG(INFO)?

http://codereview.chromium.org/6288009/diff/110002/chrome/installer/setup/set...
chrome/installer/setup/setup_main.cc:282: *status =
installer::READY_MODE_REQUIRES_CHROME;
LOG(ERROR)

http://codereview.chromium.org/6288009/diff/110002/chrome/installer/util/chro...
File chrome/installer/util/chrome_frame_operations.cc (right):

http://codereview.chromium.org/6288009/diff/110002/chrome/installer/util/chro...
chrome/installer/util/chrome_frame_operations.cc:22:
std::set<std::wstring>::iterator ready_mode = options->find(kOptionReadyMode);
nit: use constructor syntax instead of =

http://codereview.chromium.org/6288009/diff/110002/chrome/installer/util/inst...
File chrome/installer/util/installation_state.cc (right):

http://codereview.chromium.org/6288009/diff/110002/chrome/installer/util/inst...
chrome/installer/util/installation_state.cc:41: == ERROR_SUCCESS) {
missed this one

http://codereview.chromium.org/6288009/diff/110002/chrome/installer/util/inst...
File chrome/installer/util/installer_state.h (right):

http://codereview.chromium.org/6288009/diff/110002/chrome/installer/util/inst...
chrome/installer/util/installer_state.h:101: bool system_install() const {
return level_ == SYSTEM_LEVEL; }
just a thought...
should we add a DCHECK here that if level_ is not SYSTEM_LEVEL that it is
USER_LEVEL?  What I'm thinking is that if we have a code path that fails to
initialize this value correctly and we somehow assume that system_install() ==
false, equals user install.

[tommi (sloooow) - chröme, 2011-01-24 19:40:49]

which files were changed? (just so that I can be quicker with my feedback)

On Mon, Jan 24, 2011 at 2:26 PM, <grt@chromium.org> wrote:

> I've unlocked two new levels:
> - Existing single Chrome installs are now migrated to multi.  This is
> required
> to send ready-mode down to existing installations.
> - The various post-install things like launching the browser no longer take
> place if the browser wasn't actually asked for (e.g., when multi Chrome is
> updated as a result of installing Chrome Frame).
>
>
> http://codereview.chromium.org/6288009/
>

[grt (UTC plus 2), 2011-01-24 20:03:07]

setup_main.cc relative to patch set 4, thanks.

On Mon, Jan 24, 2011 at 2:40 PM, Tommi <tommi@chromium.org> wrote:

> which files were changed? (just so that I can be quicker with my feedback)
>
>
> On Mon, Jan 24, 2011 at 2:26 PM, <grt@chromium.org> wrote:
>
>> I've unlocked two new levels:
>> - Existing single Chrome installs are now migrated to multi.  This is
>> required
>> to send ready-mode down to existing installations.
>> - The various post-install things like launching the browser no longer
>> take
>> place if the browser wasn't actually asked for (e.g., when multi Chrome is
>> updated as a result of installing Chrome Frame).
>>
>>
>> http://codereview.chromium.org/6288009/
>>
>
>

[tommi (sloooow) - chröme, 2011-01-24 20:25:44]

setup main looks good

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

http://codereview.chromium.org/6288009/diff/23005/chrome/installer/setup/setu...
chrome/installer/setup/setup_main.cc:280: // Fail if we're installing Chrome
Frame yet a single-install of it is
yet -> if

[robertshield, 2011-01-25 03:03:42]

LGTM. The code quality and coverage looks much improved, this is really great
work.

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

http://codereview.chromium.org/6288009/diff/23005/chrome/installer/setup/setu...
chrome/installer/setup/setup_main.cc:280: // Fail if we're installing Chrome
Frame yet a single-install of it is
On 2011/01/24 20:25:44, tommi wrote:
> yet -> if

if -> when

http://codereview.chromium.org/6288009/diff/23005/chrome/installer/setup/setu...
chrome/installer/setup/setup_main.cc:330: // Checks for compatibility between
the current state of the system and the
indent

http://codereview.chromium.org/6288009/diff/23005/chrome/installer/setup/setu...
chrome/installer/setup/setup_main.cc:564: // part of a multi-install.
close parenthesis

[grt (UTC plus 2), 2011-01-25 03:09:39]

Thanks for the comments.  Updates are hitting the try servers.

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

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/setup/setu...
chrome/installer/setup/setup_main.cc:286: }
On 2011/01/24 19:39:39, tommi wrote:
> On 2011/01/24 16:07:02, grt wrote:
> > On 2011/01/21 21:45:17, tommi wrote:
> > > } else {
> > >   abort if ready-mode is specified?
> > >   abort if more than one product is being installed?
> > > }
> > 
> > For the former, setup.exe currently silently ignores command line options
that
> > don't apply for a given operation.  I've just made this the case for
> ready-mode
> > (see chrome_frame_operations.cc).  Do you think it's necessary to abort if
> > --ready-mode is present for a single-install Chrome Frame rather than simply
> > ignore it?
> 
> Hmm.. maybe we should just log a warning for now.
> 
> > 
> > For the latter, I've added a DCHECK for this since InstallerState should
> already
> > prohibit it from ever happening.
> 
> Cool.
> 

Done.

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/util/chrom...
File chrome/installer/util/chrome_browser_operations.h (right):

http://codereview.chromium.org/6288009/diff/56001/chrome/installer/util/chrom...
chrome/installer/util/chrome_browser_operations.h:25: std::set<std::wstring>*
options) OVERRIDE;
On 2011/01/24 19:39:39, tommi wrote:
> On 2011/01/24 16:07:02, grt wrote:
> > On 2011/01/21 21:45:17, tommi wrote:
> > > I think the "= 0;" approach is more common. Is chrome moving towards using
> > > OVERRIDE?
> > 
> > "= 0;" makes a method pure-virtual.  "OVERRIDE" is a macro that, where
> > available, expands to compiler-specific juju that throws an error if a
method
> in
> > a subclass isn't actually overriding anything (e.g., if someone tweaks the
> base
> > interface but forgets to change everyone who inherits from it).  The macro
was
> > discussed on chromium-dev a few weeks ago.  I'm a fan of it since it lets
MSVC
> > catch a fairly common error.  Looks like it does something meaningful for
> clang,
> > too.
> 
> Thanks for the info!  In that case it does sound valuable.

It's an MS feature originally built for targeting the CLR, but it works for
regular old C++ compilation, too.  See
http://msdn.microsoft.com/en-us/library/4dd87x3d(v=VS.90).aspx for more info.

http://codereview.chromium.org/6288009/diff/110002/chrome/installer/setup/chr...
File chrome/installer/setup/chrome_frame_ready_mode.cc (right):

http://codereview.chromium.org/6288009/diff/110002/chrome/installer/setup/chr...
chrome/installer/setup/chrome_frame_ready_mode.cc:217: std::wstring
chrome_frame_ua_value_name = kChromeFramePrefix;
On 2011/01/24 19:39:39, tommi wrote:
> nit: chrome_frame_ua_value_name(kChromeFramePrefix);

Done.

http://codereview.chromium.org/6288009/diff/110002/chrome/installer/setup/ins...
File chrome/installer/setup/install_worker.cc (right):

http://codereview.chromium.org/6288009/diff/110002/chrome/installer/setup/ins...
chrome/installer/setup/install_worker.cc:154: // in case of uninstall since this
means that Chrome Frame is being
On 2011/01/24 19:39:39, tommi wrote:
> chrome frame is being uninstalled?  Wouldn't it mean that chrome is being
> uninstalled?  or am I misunderstanding the comment...

This function is called for a variety of reasons, one of which is to update
Chrome's uninstall command during uninstallation of Chrome Frame (by way of
AddChromeFrameWorkItems).  Without this new condition, the Chrome Frame args are
added to Chrome's uninstall command when Chrome Frame is uninstalled (doh!).

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

http://codereview.chromium.org/6288009/diff/110002/chrome/installer/setup/set...
chrome/installer/setup/setup_main.cc:226: "than installing in ready-mode.";
On 2011/01/24 19:39:39, tommi wrote:
> indent off by one

Done.

http://codereview.chromium.org/6288009/diff/110002/chrome/installer/setup/set...
chrome/installer/setup/setup_main.cc:228: VLOG(1) << "Skipping upgrade of
single-install Chrome Frame rather "
On 2011/01/24 19:39:39, tommi wrote:
> LOG(INFO)?

It's my understanding that we should use VLOG(1) rather than LOG(INFO).

http://codereview.chromium.org/6288009/diff/110002/chrome/installer/setup/set...
chrome/installer/setup/setup_main.cc:241: *status =
installer::NON_MULTI_INSTALLATION_EXISTS;
On 2011/01/24 19:39:39, tommi wrote:
> LOG(ERROR)

Done.

http://codereview.chromium.org/6288009/diff/110002/chrome/installer/setup/set...
chrome/installer/setup/setup_main.cc:265:
InstallUtil::WriteInstallerResult(system_level,
On 2011/01/24 19:39:39, tommi wrote:
> LOG(ERROR)

Done.

http://codereview.chromium.org/6288009/diff/110002/chrome/installer/setup/set...
chrome/installer/setup/setup_main.cc:277: VLOG(1) << "Upgrading existing
multi-install Chrome browser along with "
On 2011/01/24 19:39:39, tommi wrote:
> LOG(INFO)?

Not done.

http://codereview.chromium.org/6288009/diff/110002/chrome/installer/setup/set...
chrome/installer/setup/setup_main.cc:282: *status =
installer::READY_MODE_REQUIRES_CHROME;
On 2011/01/24 19:39:39, tommi wrote:
> LOG(ERROR)

Done.

http://codereview.chromium.org/6288009/diff/110002/chrome/installer/util/chro...
File chrome/installer/util/chrome_frame_operations.cc (right):

http://codereview.chromium.org/6288009/diff/110002/chrome/installer/util/chro...
chrome/installer/util/chrome_frame_operations.cc:22:
std::set<std::wstring>::iterator ready_mode = options->find(kOptionReadyMode);
On 2011/01/24 19:39:39, tommi wrote:
> nit: use constructor syntax instead of =

Done.

http://codereview.chromium.org/6288009/diff/110002/chrome/installer/util/inst...
File chrome/installer/util/installation_state.cc (right):

http://codereview.chromium.org/6288009/diff/110002/chrome/installer/util/inst...
chrome/installer/util/installation_state.cc:41: == ERROR_SUCCESS) {
On 2011/01/24 19:39:39, tommi wrote:
> missed this one

Done.

http://codereview.chromium.org/6288009/diff/110002/chrome/installer/util/inst...
File chrome/installer/util/installer_state.h (right):

http://codereview.chromium.org/6288009/diff/110002/chrome/installer/util/inst...
chrome/installer/util/installer_state.h:101: bool system_install() const {
return level_ == SYSTEM_LEVEL; }
On 2011/01/24 19:39:39, tommi wrote:
> just a thought...
> should we add a DCHECK here that if level_ is not SYSTEM_LEVEL that it is
> USER_LEVEL?  What I'm thinking is that if we have a code path that fails to
> initialize this value correctly and we somehow assume that system_install() ==
> false, equals user install.

Done.

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

http://codereview.chromium.org/6288009/diff/23005/chrome/installer/setup/setu...
chrome/installer/setup/setup_main.cc:280: // Fail if we're installing Chrome
Frame yet a single-install of it is
On 2011/01/24 20:25:44, tommi wrote:
> yet -> if

"yet" is what is meant there.  If you don't like "yet," I think "but" is better
than "if."

[grt (UTC plus 2), 2011-01-25 03:21:52]

Thanks for the great feedback and for keeping me honest.  :-)

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

http://codereview.chromium.org/6288009/diff/23005/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.cc:475: 
self: delete this extra line.

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

http://codereview.chromium.org/6288009/diff/23005/chrome/installer/setup/setu...
chrome/installer/setup/setup_main.cc:181: // The supported multi-install
self: fix this crazy line wrap

http://codereview.chromium.org/6288009/diff/23005/chrome/installer/setup/setu...
chrome/installer/setup/setup_main.cc:280: // Fail if we're installing Chrome
Frame yet a single-install of it is
On 2011/01/25 03:09:40, grt wrote:
> On 2011/01/24 20:25:44, tommi wrote:
> > yet -> if
> 
> "yet" is what is meant there.  If you don't like "yet," I think "but" is
better
> than "if."

yet -> but -> if -> when -> ?
(okay, "when" is nice.)

http://codereview.chromium.org/6288009/diff/23005/chrome/installer/setup/setu...
chrome/installer/setup/setup_main.cc:330: // Checks for compatibility between
the current state of the system and the
On 2011/01/25 03:03:42, robertshield wrote:
> indent

Done.

http://codereview.chromium.org/6288009/diff/23005/chrome/installer/setup/setu...
chrome/installer/setup/setup_main.cc:564: // part of a multi-install.
On 2011/01/25 03:03:42, robertshield wrote:
> close parenthesis

Done.

[grt (UTC plus 2), 2011-01-25 04:41:23]

Patch set 6 is green on the trybots.  Patch set 7 contains no code changes (just
comments).  Tommi, I await your final review.  Thanks.

[tommi (sloooow) - chröme, 2011-01-25 14:00:55]

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

http://codereview.chromium.org/6288009/diff/23005/chrome/installer/setup/setu...
chrome/installer/setup/setup_main.cc:280: // Fail if we're installing Chrome
Frame yet a single-install of it is
I see.  I read it as being a comment for the 'if' and not what's inside the if. 
Suggest using all of the other suggestions just so that there'll be
misunderstanding ;)

On 2011/01/25 03:09:40, grt wrote:
> On 2011/01/24 20:25:44, tommi wrote:
> > yet -> if
> 
> "yet" is what is meant there.  If you don't like "yet," I think "but" is
better
> than "if."

[tommi (sloooow) - chröme, 2011-01-25 14:24:35]

LGTM.  great!

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

http://codereview.chromium.org/6288009/diff/110002/chrome/installer/setup/set...
chrome/installer/setup/setup_main.cc:228: VLOG(1) << "Skipping upgrade of
single-install Chrome Frame rather "
OK. The way I understand it, VLOG(1) is one noise level "above" (as in _more_
noise) INFO.

On 2011/01/25 03:09:40, grt wrote:
> On 2011/01/24 19:39:39, tommi wrote:
> > LOG(INFO)?
> 
> It's my understanding that we should use VLOG(1) rather than LOG(INFO).

http://codereview.chromium.org/6288009/diff/107007/chrome/installer/util/inst...
File chrome/installer/util/installer_state.h (right):

http://codereview.chromium.org/6288009/diff/107007/chrome/installer/util/inst...
chrome/installer/util/installer_state.h:101: bool system_install() const {
return level_ == SYSTEM_LEVEL; }
I saw you added a DCHECK for is_multi_install() to check for valid
post-initialization values - can you do the same for system_install()?

[grt (UTC plus 2), 2011-01-25 16:39:45]

Thanks.  I've sync'd to r72487 and am making a final pass through the trybots
before committing.

http://codereview.chromium.org/6288009/diff/107007/chrome/installer/util/inst...
File chrome/installer/util/installer_state.h (right):

http://codereview.chromium.org/6288009/diff/107007/chrome/installer/util/inst...
chrome/installer/util/installer_state.h:101: bool system_install() const {
return level_ == SYSTEM_LEVEL; }
On 2011/01/25 14:24:35, tommi wrote:
> I saw you added a DCHECK for is_multi_install() to check for valid
> post-initialization values - can you do the same for system_install()?

Done.