C++ reability review for gab.

chrome/installer/setup/install_worker.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 //
 // This file contains the declarations of the installer functions that build
 // the WorkItemList used to install the application.
 
+//Pickup file for readability review.
+
 #ifndef CHROME_INSTALLER_SETUP_INSTALL_WORKER_H_
 #define CHROME_INSTALLER_SETUP_INSTALL_WORKER_H_
 
 #include <windows.h>
 
 #include <vector>
 
 #include "base/memory/scoped_ptr.h"
 
 class BrowserDistribution;
@@ skipping 39 matching lines @@
 // - Handle the case of in-use-update by updating "opv" (old version) key or
 //   deleting it if not required.
 // - Register any new dlls and unregister old dlls.
 // - If this is an MSI install, ensures that the MSI marker is set, and sets
 //   it if not.
 // If these operations are successful, the function returns true, otherwise
 // false.
 bool AppendPostInstallTasks(const InstallerState& installer_state,
                             const FilePath& setup_path,
                             const Version* current_version,
                             const Version& new_version,

[dominich, 2012/09/06 17:26:12]

Please add to the documentation why current_version is a pointer and new_version
is a ref. Ie, can current_version be null? is it an out parameter?

[gab, 2012/09/07 20:56:44]

This has been a TODO on our team in a while (i.e. many parameters in this file
are pointers just for the sake of being able to pass NULL on uninstall (the
other option is to create another function for uninstall, but a lot of code
would be shared so a third helper function would be needed and we felt it was
overkill)): http://crbug.com/146850.

I changed every parameter in this file so that:

1) They are all const& (and pass in empty objects on uninstall where they are
not required).

2) Swap parameters so that |current_version| always comes right before
|new_version| for consistency.

3) WorkItemList* is always the last parameter.

4) Make all |current_version| const* as it can be NULL if no Chrome is installed
(and add a comment to every function that has a |current_version| to state this
fact).

I considered refactoring further so that we would be able to pass an empty
Version() (i.e. such that current_version.IsValid() is false) if no Chrome
exists on the system instead of passing NULL; so that we could also make this
parameter a const&, but this would involve changing things all the way in
installer_state.cc and, like anything touching the installer, this is dangerous
and shouldn't be done as part of a readability CL imo.

                             const FilePath& temp_path,
                             WorkItemList* post_install_task_list);
 
 // Builds the complete WorkItemList used to build the set of installation steps
 // needed to lay down one or more installed products.
 //
 // setup_path: Path to the executable (setup.exe) as it will be copied
 //           to Chrome install folder after install is complete
 // archive_path: Path to the archive (chrome.7z) as it will be copied
 //               to Chrome install folder after install is complete
 // src_path: the path that contains a complete and unpacked Chrome package
 //           to be installed.
 // temp_path: the path of working directory used during installation. This path
 //            does not need to exist.
 void AddInstallWorkItems(const InstallationState& original_state,
                          const InstallerState& installer_state,
                          const FilePath& setup_path,
                          const FilePath& archive_path,
                          const FilePath& src_path,
                          const FilePath& temp_path,
                          const Version& new_version,
                          scoped_ptr<Version>* current_version,

[dominich, 2012/09/06 17:26:12]

Is current_version an out parameter? If so, please document that. If not, you
don't need the *.

[gab, 2012/09/07 20:56:44]

Changed all "scoped_ptr<Version>* current_version" into "const Version*
current_version" in this file (see longer explanation in previous comment).

                          WorkItemList* install_list);
 
 // Appends registration or unregistration work items to |work_item_list| for the
 // COM DLLs whose file names are given in |dll_files| and which reside in the
 // path |dll_folder|.
 // |system_level| specifies whether to call the system or user level DLL
 // registration entry points.
 // |do_register| says whether to register or unregister.
 // |may_fail| states whether this is best effort or not. If |may_fail| is true
 // then |work_item_list| will still succeed if the registration fails and
@@ skipping 108 matching lines @@
 // installation; it is unused (and may therefore be NULL) when uninstalling.
 void AddOsUpgradeWorkItems(const InstallerState& installer_state,
                            const FilePath* setup_path,
                            const Version* new_version,
                            const Product& product,
                            WorkItemList* install_list);
 
 }  // namespace installer
 
 #endif  // CHROME_INSTALLER_SETUP_INSTALL_WORKER_H_