Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10957016:
Ensuring Google Update at user-level is installed alongside App Host, for the quick-enable App Host… (Closed)
|
Created:
8 years, 3 months ago by huangs Modified:
8 years, 2 months ago CC:
chromium-reviews, grt+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionEnsuring Google Update at user-level is installed alongside App Host, for the quick-enable App Host flow
1. Added new switch to setup.exe: --ensure-google-update-installed.
2. When invoked, if user-level Google Update is not installed, then it executes:
GoogleUpdateSetup.exe /install "runtime=true&needsadmin=false" /silent
to install user-level Google Update, using GoogleUpdateSetup.exe from system-level install.
3. The --ensure-google-update-installed switch is added to the quick-enable-application-host command.
Caveats:
- We are not validating Google Setup install from setup.exe, since it is launched as a separate process.
BUG=138320
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=158962
Patch Set 1 #
Total comments: 42
Patch Set 2 : Fixes; moved switch checks; moved UninstallGoogleUpdate() and refactored with EnsureUserLevelGoogle… #
Total comments: 56
Patch Set 3 : Removing time-out, and wait for Google Update to install; moving flag to InstallerState; adding to … #
Total comments: 18
Patch Set 4 : Factoring Google Update execution code, since base::TimeDelta::FromMilliseconds(INFINITE) can be us… #
Total comments: 2
Patch Set 5 : Fixing unit tests. #
Total comments: 4
Patch Set 6 : Fixing nits. #
Total comments: 2
Messages
Total messages: 28 (0 generated)
Please review my preliminary changes. https://codereview.chromium.org/10957016/diff/1/chrome/installer/util/google_... File chrome/installer/util/google_update_util.cc (right): https://codereview.chromium.org/10957016/diff/1/chrome/installer/util/google_... chrome/installer/util/google_update_util.cc:30: &path_str) == ERROR_SUCCESS) && path_str will fit in the previous line, but right now I think it's better to maintain visual symmetry!
looks good. i have mostly some nits for you. https://codereview.chromium.org/10957016/diff/1/chrome/installer/setup/instal... File chrome/installer/setup/install_worker.cc (right): https://codereview.chromium.org/10957016/diff/1/chrome/installer/setup/instal... chrome/installer/setup/install_worker.cc:1550: cmd_line.AppendSwitch(switches::kEnsureGoogleUpdateInstalled); should the validator not check for this? https://codereview.chromium.org/10957016/diff/1/chrome/installer/setup/setup_... File chrome/installer/setup/setup_main.cc (right): https://codereview.chromium.org/10957016/diff/1/chrome/installer/setup/setup_... chrome/installer/setup/setup_main.cc:742: // TODO: Get a more appropriate message for other subsets. TODO(who?) https://codereview.chromium.org/10957016/diff/1/chrome/installer/setup/setup_... chrome/installer/setup/setup_main.cc:859: // Install Google Update, if it is not installed already. no comma https://codereview.chromium.org/10957016/diff/1/chrome/installer/setup/setup_... chrome/installer/setup/setup_main.cc:864: if (!google_update::EnsureUserLevelGoogleUpdateInstalled()) { no braces https://codereview.chromium.org/10957016/diff/1/chrome/installer/setup/setup_... chrome/installer/setup/setup_main.cc:868: LOG(ERROR) << "--" << installer::switches::kEnsureGoogleUpdateInstalled this belongs in CheckAppHostPreconditions, no? https://codereview.chromium.org/10957016/diff/1/chrome/installer/util/google_... File chrome/installer/util/google_update_constants.h (right): https://codereview.chromium.org/10957016/diff/1/chrome/installer/util/google_... chrome/installer/util/google_update_constants.h:17: extern const wchar_t kGoogleUpdateSetupExecutable[]; i think kGoogleUpdateSetupExe is more obvious. https://codereview.chromium.org/10957016/diff/1/chrome/installer/util/google_... File chrome/installer/util/google_update_util.cc (right): https://codereview.chromium.org/10957016/diff/1/chrome/installer/util/google_... chrome/installer/util/google_update_util.cc:44: // Constants are found in googleclient/omaha/base/const_cmd_line.h. please refer to omaha code by way of code.google.com/p/omaha here and everywhere. https://codereview.chromium.org/10957016/diff/1/chrome/installer/util/google_... File chrome/installer/util/google_update_util.h (right): https://codereview.chromium.org/10957016/diff/1/chrome/installer/util/google_... chrome/installer/util/google_update_util.h:12: namespace google_update { google_update -> installer https://codereview.chromium.org/10957016/diff/1/chrome/installer/util/google_... chrome/installer/util/google_update_util.h:18: // Appends "/install runtime=true&needsadmin=false /silent" to |cmd_line| why is this a public function? who would call it?
https://codereview.chromium.org/10957016/diff/1/chrome/installer/setup/setup_... File chrome/installer/setup/setup_main.cc (right): https://codereview.chromium.org/10957016/diff/1/chrome/installer/setup/setup_... chrome/installer/setup/setup_main.cc:743: message_id = IDS_INSTALL_HIGHER_VERSION_APP_HOST_BASE; I think that this message is actually generic (doesn't reference the App Host in the string). If so, just change the ID to not reference the app host. It doesn't make sense to get into every single possible permutation of this stuff. https://codereview.chromium.org/10957016/diff/1/chrome/installer/setup/setup_... chrome/installer/setup/setup_main.cc:860: // This is for quick-enable App Host install via system-level Google Update. via ... -> from a system-level Chrome Binaries installation. https://codereview.chromium.org/10957016/diff/1/chrome/installer/setup/setup_... chrome/installer/setup/setup_main.cc:862: // Currently this feature is only needed by, and implemented for user-level. Remove this comment and edit line 859 to say something like: "Install Google Update at user-level (if not present) using the installer from an existing system-level installation." https://codereview.chromium.org/10957016/diff/1/chrome/installer/util/google_... File chrome/installer/util/google_update_util.cc (right): https://codereview.chromium.org/10957016/diff/1/chrome/installer/util/google_... chrome/installer/util/google_update_util.cc:21: FilePath GetGoogleUpdateSetupExe(bool system_install) { doesn't match name in header file, but it's probably not needed outside this CC so maybe just remove from header and put in anonymous namespace. https://codereview.chromium.org/10957016/diff/1/chrome/installer/util/google_... chrome/installer/util/google_update_util.cc:37: return google_update_setup; if this doesn't exist, it's very unusual. LOG an ERROR. https://codereview.chromium.org/10957016/diff/1/chrome/installer/util/google_... chrome/installer/util/google_update_util.cc:54: if (GetGoogleUpdateSetupExe(false).empty()) { I have mixed feelings about using the presence of the GoogleUpdateSetup.exe to determine if it's already at user-level. Probably the right thing is to just check in the Clients key. https://codereview.chromium.org/10957016/diff/1/chrome/installer/util/google_... chrome/installer/util/google_update_util.cc:60: return base::LaunchProcess(cmd_line, base::LaunchOptions(), NULL); Probably appropriate to wait up to some timeout for the process to exit, and return its exit code. https://codereview.chromium.org/10957016/diff/1/chrome/installer/util/google_... File chrome/installer/util/google_update_util.h (right): https://codereview.chromium.org/10957016/diff/1/chrome/installer/util/google_... chrome/installer/util/google_update_util.h:8: #include "base/command_line.h" forward decl https://codereview.chromium.org/10957016/diff/1/chrome/installer/util/google_... chrome/installer/util/google_update_util.h:9: #include "base/file_path.h" forward decl https://codereview.chromium.org/10957016/diff/1/chrome/installer/util/google_... chrome/installer/util/google_update_util.h:10: #include "base/string16.h" not required. https://codereview.chromium.org/10957016/diff/1/chrome/installer/util/google_... chrome/installer/util/google_update_util.h:16: FilePath GetGoogleUpdateSetupExePathString(bool system_level); PathString -> Path https://codereview.chromium.org/10957016/diff/1/chrome/installer/util/google_... chrome/installer/util/google_update_util.h:24: bool EnsureUserLevelGoogleUpdateInstalled(); "Returns true if already installed or installed successfully. False if the installation fails or times-out." Update the implementation accordingly. See code in setup_main.cc that waits for a process to complete.
Please take another look. I did more refactoring than expected, so gonna try jobs, too! https://codereview.chromium.org/10957016/diff/1/chrome/installer/setup/instal... File chrome/installer/setup/install_worker.cc (right): https://codereview.chromium.org/10957016/diff/1/chrome/installer/setup/instal... chrome/installer/setup/install_worker.cc:1550: cmd_line.AppendSwitch(switches::kEnsureGoogleUpdateInstalled); On 2012/09/20 17:58:51, grt wrote: > should the validator not check for this? Done, changed in installation_validator.cc. https://codereview.chromium.org/10957016/diff/1/chrome/installer/setup/setup_... File chrome/installer/setup/setup_main.cc (right): https://codereview.chromium.org/10957016/diff/1/chrome/installer/setup/setup_... chrome/installer/setup/setup_main.cc:743: message_id = IDS_INSTALL_HIGHER_VERSION_APP_HOST_BASE; On 2012/09/20 18:00:01, erikwright wrote: > I think that this message is actually generic (doesn't reference the App Host in > the string). > > If so, just change the ID to not reference the app host. It doesn't make sense > to get into every single possible permutation of this stuff. Reverting and no-op, per discussion. https://codereview.chromium.org/10957016/diff/1/chrome/installer/setup/setup_... chrome/installer/setup/setup_main.cc:859: // Install Google Update, if it is not installed already. On 2012/09/20 17:58:51, grt wrote: > no comma Done. https://codereview.chromium.org/10957016/diff/1/chrome/installer/setup/setup_... chrome/installer/setup/setup_main.cc:860: // This is for quick-enable App Host install via system-level Google Update. On 2012/09/20 18:00:01, erikwright wrote: > via ... -> from a system-level Chrome Binaries installation. Done. https://codereview.chromium.org/10957016/diff/1/chrome/installer/setup/setup_... chrome/installer/setup/setup_main.cc:862: // Currently this feature is only needed by, and implemented for user-level. On 2012/09/20 18:00:01, erikwright wrote: > Remove this comment and edit line 859 to say something like: > > "Install Google Update at user-level (if not present) using the installer from > an existing system-level installation." Done, merged with earlier comment, too. https://codereview.chromium.org/10957016/diff/1/chrome/installer/setup/setup_... chrome/installer/setup/setup_main.cc:864: if (!google_update::EnsureUserLevelGoogleUpdateInstalled()) { On 2012/09/20 17:58:51, grt wrote: > no braces Done. https://codereview.chromium.org/10957016/diff/1/chrome/installer/setup/setup_... chrome/installer/setup/setup_main.cc:868: LOG(ERROR) << "--" << installer::switches::kEnsureGoogleUpdateInstalled On 2012/09/20 17:58:51, grt wrote: > this belongs in CheckAppHostPreconditions, no? Talked to erikwright@: Moving the check for user-level to CheckAppHostPreconditions(), but keeping the install here. https://codereview.chromium.org/10957016/diff/1/chrome/installer/util/google_... File chrome/installer/util/google_update_constants.h (right): https://codereview.chromium.org/10957016/diff/1/chrome/installer/util/google_... chrome/installer/util/google_update_constants.h:17: extern const wchar_t kGoogleUpdateSetupExecutable[]; On 2012/09/20 17:58:51, grt wrote: > i think kGoogleUpdateSetupExe is more obvious. Done. https://codereview.chromium.org/10957016/diff/1/chrome/installer/util/google_... File chrome/installer/util/google_update_util.cc (right): https://codereview.chromium.org/10957016/diff/1/chrome/installer/util/google_... chrome/installer/util/google_update_util.cc:21: FilePath GetGoogleUpdateSetupExe(bool system_install) { On 2012/09/20 18:00:01, erikwright wrote: > doesn't match name in header file, but it's probably not needed outside this CC > so maybe just remove from header and put in anonymous namespace. Done. https://codereview.chromium.org/10957016/diff/1/chrome/installer/util/google_... chrome/installer/util/google_update_util.cc:37: return google_update_setup; On 2012/09/20 18:00:01, erikwright wrote: > if this doesn't exist, it's very unusual. LOG an ERROR. Done. https://codereview.chromium.org/10957016/diff/1/chrome/installer/util/google_... chrome/installer/util/google_update_util.cc:44: // Constants are found in googleclient/omaha/base/const_cmd_line.h. On 2012/09/20 17:58:51, grt wrote: > please refer to omaha code by way of code.google.com/p/omaha here and > everywhere. Done. Also, I had a typo; it's in omaha/common. https://codereview.chromium.org/10957016/diff/1/chrome/installer/util/google_... chrome/installer/util/google_update_util.cc:54: if (GetGoogleUpdateSetupExe(false).empty()) { On 2012/09/20 18:00:01, erikwright wrote: > I have mixed feelings about using the presence of the GoogleUpdateSetup.exe to > determine if it's already at user-level. > > Probably the right thing is to just check in the Clients key. Done. Added simple local routine IsGoogleUpdateInstalled() for now; will move it upon request (e.g. to GoogleUpdateSettings?). https://codereview.chromium.org/10957016/diff/1/chrome/installer/util/google_... chrome/installer/util/google_update_util.cc:60: return base::LaunchProcess(cmd_line, base::LaunchOptions(), NULL); On 2012/09/20 18:00:01, erikwright wrote: > Probably appropriate to wait up to some timeout for the process to exit, and > return its exit code. Wrote the time-out code, but can I output the return code in PLOG(ERROR) and return bool still instead? https://codereview.chromium.org/10957016/diff/1/chrome/installer/util/google_... File chrome/installer/util/google_update_util.h (right): https://codereview.chromium.org/10957016/diff/1/chrome/installer/util/google_... chrome/installer/util/google_update_util.h:8: #include "base/command_line.h" On 2012/09/20 18:00:01, erikwright wrote: > forward decl Done (deleted since it's no longer needed). https://codereview.chromium.org/10957016/diff/1/chrome/installer/util/google_... chrome/installer/util/google_update_util.h:9: #include "base/file_path.h" On 2012/09/20 18:00:01, erikwright wrote: > forward decl Done (deleted since it's no longer needed). https://codereview.chromium.org/10957016/diff/1/chrome/installer/util/google_... chrome/installer/util/google_update_util.h:10: #include "base/string16.h" On 2012/09/20 18:00:01, erikwright wrote: > not required. Done. https://codereview.chromium.org/10957016/diff/1/chrome/installer/util/google_... chrome/installer/util/google_update_util.h:12: namespace google_update { On 2012/09/20 17:58:51, grt wrote: > google_update -> installer No-op: I'm following google_update_constants.cc, which uses the "google_udpate" namespace. Will change if you insist. https://codereview.chromium.org/10957016/diff/1/chrome/installer/util/google_... chrome/installer/util/google_update_util.h:16: FilePath GetGoogleUpdateSetupExePathString(bool system_level); On 2012/09/20 18:00:01, erikwright wrote: > PathString -> Path Done. Also making the routine local, until it is needed outside. https://codereview.chromium.org/10957016/diff/1/chrome/installer/util/google_... chrome/installer/util/google_update_util.h:18: // Appends "/install runtime=true&needsadmin=false /silent" to |cmd_line| On 2012/09/20 17:58:51, grt wrote: > why is this a public function? who would call it? I was considering writing a unit test, since I'm disturbed by the fact that "&" does not quotation, and there is NO mechanism to quote it in CommandLine if I want. But I'll make it local. https://codereview.chromium.org/10957016/diff/1/chrome/installer/util/google_... chrome/installer/util/google_update_util.h:24: bool EnsureUserLevelGoogleUpdateInstalled(); On 2012/09/20 18:00:01, erikwright wrote: > "Returns true if already installed or installed successfully. False if the > installation fails or times-out." > > Update the implementation accordingly. See code in setup_main.cc that waits for > a process to complete. Done.
Question for Greg. https://codereview.chromium.org/10957016/diff/4002/chrome/installer/setup/set... File chrome/installer/setup/setup_main.cc (right): https://codereview.chromium.org/10957016/diff/4002/chrome/installer/setup/set... chrome/installer/setup/setup_main.cc:481: const CommandLine& cmd_line, Rather than introducing the cmd_line argument here and elsewhere, I suggest adding a property to InstallerState (::ensure_google_update_present()) and initializing it in InstallerState::Initialize. Then you can just check installer_state everywhere you were previously accessing cmd_line. I don't think it makes sense to also put it in MasterPreferences. grt, WDYT?
https://codereview.chromium.org/10957016/diff/4002/chrome/installer/setup/set... File chrome/installer/setup/setup_main.cc (right): https://codereview.chromium.org/10957016/diff/4002/chrome/installer/setup/set... chrome/installer/setup/setup_main.cc:510: << " is enabled for user-level only."; enabled -> supported. https://codereview.chromium.org/10957016/diff/4002/chrome/installer/setup/set... chrome/installer/setup/setup_main.cc:766: Put the Google Update installation here and set proceed_with_installation to false if it fails. Add a new error code for this in util_constants.h https://codereview.chromium.org/10957016/diff/4002/chrome/installer/util/goog... File chrome/installer/util/google_update_util.cc (right): https://codereview.chromium.org/10957016/diff/4002/chrome/installer/util/goog... chrome/installer/util/google_update_util.cc:52: LOG(ERROR) << "Unexpectedly missing " << kGoogleUpdateSetupExe; log google_update_setup instead (so full path is included). https://codereview.chromium.org/10957016/diff/4002/chrome/installer/util/goog... chrome/installer/util/google_update_util.cc:61: // This is not in GoogleUpdateSettings because we build the command line, Remove "This is not ..." https://codereview.chromium.org/10957016/diff/4002/chrome/installer/util/goog... chrome/installer/util/google_update_util.cc:86: LOG(INFO) << "Launching " << cmd_name << ": " << cmd_string; just log cmd_string instead of cmd_name here. https://codereview.chromium.org/10957016/diff/4002/chrome/installer/util/goog... chrome/installer/util/google_update_util.cc:90: PLOG(ERROR) << "Failed to launch " << cmd_name << " (" << cmd_string no need to include cmd_name. https://codereview.chromium.org/10957016/diff/4002/chrome/installer/util/goog... chrome/installer/util/google_update_util.cc:99: const base::win::ScopedHandle& process) { Define |process| as a plain-old HANDLE. There is an implicit conversion available. https://codereview.chromium.org/10957016/diff/4002/chrome/installer/util/goog... chrome/installer/util/google_update_util.cc:109: PLOG(ERROR) << cmd_name << " (" << cmd_string << ") exited with code " no need for cmd_name here. https://codereview.chromium.org/10957016/diff/4002/chrome/installer/util/goog... chrome/installer/util/google_update_util.cc:114: LOG(ERROR) << cmd_name << " (" << cmd_string no need for cmd_name here. https://codereview.chromium.org/10957016/diff/4002/chrome/installer/util/goog... chrome/installer/util/google_update_util.cc:130: string16 cmd_name(L"Google Update installer"); There's no need to pass around cmd_name. For the purposes of logs, the actual command-line is sufficient.
https://codereview.chromium.org/10957016/diff/4002/chrome/installer/setup/set... File chrome/installer/setup/setup_main.cc (right): https://codereview.chromium.org/10957016/diff/4002/chrome/installer/setup/set... chrome/installer/setup/setup_main.cc:481: const CommandLine& cmd_line, On 2012/09/21 13:11:11, erikwright wrote: > Rather than introducing the cmd_line argument here and elsewhere, I suggest > adding a property to InstallerState (::ensure_google_update_present()) and > initializing it in InstallerState::Initialize. > > Then you can just check installer_state everywhere you were previously accessing > cmd_line. > > I don't think it makes sense to also put it in MasterPreferences. > > grt, WDYT? sgtm. MasterPreferences would only be required if this option needs to be set in an installerdata block for distro deals, which doesn't appear to be the case. https://codereview.chromium.org/10957016/diff/4002/chrome/installer/setup/set... chrome/installer/setup/setup_main.cc:505: // The flag is generic, but it's primary use case involves App Host. it's -> its https://codereview.chromium.org/10957016/diff/4002/chrome/installer/setup/set... chrome/installer/setup/setup_main.cc:505: // The flag is generic, but it's primary use case involves App Host. if it's generic, then do this check in CheckPreInstallConditions. https://codereview.chromium.org/10957016/diff/4002/chrome/installer/setup/set... chrome/installer/setup/setup_main.cc:508: if (installer_state->system_install()) { note that you'll never get here thanks to the block above. https://codereview.chromium.org/10957016/diff/4002/chrome/installer/setup/set... chrome/installer/setup/setup_main.cc:875: if (cmd_line.HasSwitch(installer::switches::kEnsureGoogleUpdateInstalled)) { if handling of this flag is general (not specific to the app host), i think it's good to treat it as a best-effort thing in case someone adds this switch to a normal "install Chrome" command line (it wouldn't be nice to blow up in that case). as such, what do you think of making the logic: if (theNewSwitchIsPresent && isUserLevel) { if (!ensure...) LOG(ERROR) ...; } apologies for not suggesting this from the get-go. https://codereview.chromium.org/10957016/diff/4002/chrome/installer/setup/set... chrome/installer/setup/setup_main.cc:998: // Ignoring the return value: Success or failure of Google Update Ignoring -> Ignore, Success -> success https://codereview.chromium.org/10957016/diff/4002/chrome/installer/util/goog... File chrome/installer/util/google_update_util.cc (right): https://codereview.chromium.org/10957016/diff/4002/chrome/installer/util/goog... chrome/installer/util/google_update_util.cc:49: if (file_util::PathExists(google_update_setup)) { i suggest you don't bother with this check. it's racy. just return the path here and let CreateProcess fail if it fails. https://codereview.chromium.org/10957016/diff/4002/chrome/installer/util/goog... chrome/installer/util/google_update_util.cc:79: bool ExecuteGoogleUpdateCommand(const string16& cmd_string, please add a doc comment https://codereview.chromium.org/10957016/diff/4002/chrome/installer/util/goog... chrome/installer/util/google_update_util.cc:83: if (cmd_string.empty()) { // Command not rendered. remove this whole block and let an empty cmd_string percolate down through LaunchProcess https://codereview.chromium.org/10957016/diff/4002/chrome/installer/util/goog... chrome/installer/util/google_update_util.cc:91: << ")."; remove the period at the end here since PLOG will add extra stuff to the log message. https://codereview.chromium.org/10957016/diff/4002/chrome/installer/util/goog... chrome/installer/util/google_update_util.cc:103: process, &exit_code, please move these to the previous line to conserve vertical space. https://codereview.chromium.org/10957016/diff/4002/chrome/installer/util/goog... chrome/installer/util/google_update_util.cc:106: LOG(INFO) << " normal exit."; i should never have put this there, please remove it. https://codereview.chromium.org/10957016/diff/4002/chrome/installer/util/goog... chrome/installer/util/google_update_util.cc:109: PLOG(ERROR) << cmd_name << " (" << cmd_string << ") exited with code " PLOG -> LOG https://codereview.chromium.org/10957016/diff/4002/chrome/installer/util/goog... chrome/installer/util/google_update_util.cc:114: LOG(ERROR) << cmd_name << " (" << cmd_string it's not clear to me that the process taking longer than you expect it to should be treated as an error. certainly in the case of UninstallGoogleUpdate it's not an error. since you don't terminate the process, it may complete 1ns after your timeout. what then? https://codereview.chromium.org/10957016/diff/4002/chrome/installer/util/goog... chrome/installer/util/google_update_util.cc:127: LOG(INFO) << "Google Update is already installed."; i don't think this is needed. the log files get pretty big, so i think it's good to take care not to put in information that's easily derived. in this case, the next log message after "Ensuring..." will make it clear whether it was already installed or not. https://codereview.chromium.org/10957016/diff/4002/chrome/installer/util/goog... chrome/installer/util/google_update_util.cc:133: base::win::ScopedHandle process; i think this should check that the command string isn't empty before calling Execute.... Execute itself doesn't need anything special to detect an empty string since it'll eventually fail with an error. https://codereview.chromium.org/10957016/diff/4002/chrome/installer/util/goog... chrome/installer/util/google_update_util.cc:134: return ExecuteGoogleUpdateCommand(install_cmd, cmd_name, &process) && please combine these two into one function that does the launch and wait. i don't see any reason to make callers have three lines of code instead of one. https://codereview.chromium.org/10957016/diff/4002/chrome/installer/util/goog... File chrome/installer/util/google_update_util.h (right): https://codereview.chromium.org/10957016/diff/4002/chrome/installer/util/goog... chrome/installer/util/google_update_util.h:19: // Return true if and only if this executes successfully. "Returns false if Google Update could not be executed." since successful execution isn't well-defined. does it mean that Google Update was uninstalled, or just that it was run?
Please check again. Main Caveat: - Now EnsureUserLevelGoogleUpdatePresent() waits indefinitely. - Should not use WaitForExitCodeWithTimeout(): param TimeDelta cannot be Time::Max() == max int64. - So using WaitForExitCode() - But WaitForExitCode() frees handle, so we have to use ProcessHandle instead of ScopedHandle. - Decided to undo the refactoring, and have semi-duplicated code. https://codereview.chromium.org/10957016/diff/4002/chrome/installer/setup/set... File chrome/installer/setup/setup_main.cc (right): https://codereview.chromium.org/10957016/diff/4002/chrome/installer/setup/set... chrome/installer/setup/setup_main.cc:481: const CommandLine& cmd_line, On 2012/09/21 13:11:11, erikwright wrote: > Rather than introducing the cmd_line argument here and elsewhere, I suggest > adding a property to InstallerState (::ensure_google_update_present()) and > initializing it in InstallerState::Initialize. > > Then you can just check installer_state everywhere you were previously accessing > cmd_line. > > I don't think it makes sense to also put it in MasterPreferences. > > grt, WDYT? Done. "Present" sounds better than "installed", so changing that globally as well. https://codereview.chromium.org/10957016/diff/4002/chrome/installer/setup/set... chrome/installer/setup/setup_main.cc:505: // The flag is generic, but it's primary use case involves App Host. On 2012/09/21 14:20:33, grt wrote: > it's -> its How did that happen?! Done. https://codereview.chromium.org/10957016/diff/4002/chrome/installer/setup/set... chrome/installer/setup/setup_main.cc:505: // The flag is generic, but it's primary use case involves App Host. On 2012/09/21 14:20:33, grt wrote: > if it's generic, then do this check in CheckPreInstallConditions. Done. https://codereview.chromium.org/10957016/diff/4002/chrome/installer/setup/set... chrome/installer/setup/setup_main.cc:508: if (installer_state->system_install()) { On 2012/09/21 14:20:33, grt wrote: > note that you'll never get here thanks to the block above. --ensure-google-update-installed can be specified in a stand-alone fashion for testing (but let me know if I'm missing something). https://codereview.chromium.org/10957016/diff/4002/chrome/installer/setup/set... chrome/installer/setup/setup_main.cc:510: << " is enabled for user-level only."; On 2012/09/21 14:08:49, erikwright wrote: > enabled -> supported. Done. https://codereview.chromium.org/10957016/diff/4002/chrome/installer/setup/set... chrome/installer/setup/setup_main.cc:766: On 2012/09/21 14:08:49, erikwright wrote: > Put the Google Update installation here and set proceed_with_installation to > false if it fails. > > Add a new error code for this in util_constants.h Done. Calling the flag INSTALL_OF_GOOGLE_UPDATE_FAILED (note that GOOGLE_UPDATE_INSTALL_FAILED is ambiguous). https://codereview.chromium.org/10957016/diff/4002/chrome/installer/setup/set... chrome/installer/setup/setup_main.cc:875: if (cmd_line.HasSwitch(installer::switches::kEnsureGoogleUpdateInstalled)) { On 2012/09/21 14:20:33, grt wrote: > if handling of this flag is general (not specific to the app host), i think it's > good to treat it as a best-effort thing in case someone adds this switch to a > normal "install Chrome" command line (it wouldn't be nice to blow up in that > case). as such, what do you think of making the logic: > if (theNewSwitchIsPresent && isUserLevel) { > if (!ensure...) > LOG(ERROR) ...; > } > apologies for not suggesting this from the get-go. Done. https://codereview.chromium.org/10957016/diff/4002/chrome/installer/setup/set... chrome/installer/setup/setup_main.cc:998: // Ignoring the return value: Success or failure of Google Update On 2012/09/21 14:20:33, grt wrote: > Ignoring -> Ignore, Success -> success Done. https://codereview.chromium.org/10957016/diff/4002/chrome/installer/util/goog... File chrome/installer/util/google_update_util.cc (right): https://codereview.chromium.org/10957016/diff/4002/chrome/installer/util/goog... chrome/installer/util/google_update_util.cc:49: if (file_util::PathExists(google_update_setup)) { On 2012/09/21 14:20:33, grt wrote: > i suggest you don't bother with this check. it's racy. just return the path > here and let CreateProcess fail if it fails. Done. https://codereview.chromium.org/10957016/diff/4002/chrome/installer/util/goog... chrome/installer/util/google_update_util.cc:52: LOG(ERROR) << "Unexpectedly missing " << kGoogleUpdateSetupExe; On 2012/09/21 14:08:49, erikwright wrote: > log google_update_setup instead (so full path is included). Moot, as check is removed. https://codereview.chromium.org/10957016/diff/4002/chrome/installer/util/goog... chrome/installer/util/google_update_util.cc:61: // This is not in GoogleUpdateSettings because we build the command line, On 2012/09/21 14:08:49, erikwright wrote: > Remove "This is not ..." Done. https://codereview.chromium.org/10957016/diff/4002/chrome/installer/util/goog... chrome/installer/util/google_update_util.cc:79: bool ExecuteGoogleUpdateCommand(const string16& cmd_string, On 2012/09/21 14:20:33, grt wrote: > please add a doc comment Moot: Routine inlined. https://codereview.chromium.org/10957016/diff/4002/chrome/installer/util/goog... chrome/installer/util/google_update_util.cc:83: if (cmd_string.empty()) { // Command not rendered. On 2012/09/21 14:20:33, grt wrote: > remove this whole block and let an empty cmd_string percolate down through > LaunchProcess Moot: Routine inlined. https://codereview.chromium.org/10957016/diff/4002/chrome/installer/util/goog... chrome/installer/util/google_update_util.cc:86: LOG(INFO) << "Launching " << cmd_name << ": " << cmd_string; On 2012/09/21 14:08:49, erikwright wrote: > just log cmd_string instead of cmd_name here. Done. https://codereview.chromium.org/10957016/diff/4002/chrome/installer/util/goog... chrome/installer/util/google_update_util.cc:91: << ")."; On 2012/09/21 14:20:33, grt wrote: > remove the period at the end here since PLOG will add extra stuff to the log > message. Done. https://codereview.chromium.org/10957016/diff/4002/chrome/installer/util/goog... chrome/installer/util/google_update_util.cc:99: const base::win::ScopedHandle& process) { On 2012/09/21 14:08:49, erikwright wrote: > Define |process| as a plain-old HANDLE. There is an implicit conversion > available. Moot: Function inlined. Note extra complexity with ProcessHandle. https://codereview.chromium.org/10957016/diff/4002/chrome/installer/util/goog... chrome/installer/util/google_update_util.cc:103: process, &exit_code, On 2012/09/21 14:20:33, grt wrote: > please move these to the previous line to conserve vertical space. Done. https://codereview.chromium.org/10957016/diff/4002/chrome/installer/util/goog... chrome/installer/util/google_update_util.cc:106: LOG(INFO) << " normal exit."; On 2012/09/21 14:20:33, grt wrote: > i should never have put this there, please remove it. Done. https://codereview.chromium.org/10957016/diff/4002/chrome/installer/util/goog... chrome/installer/util/google_update_util.cc:109: PLOG(ERROR) << cmd_name << " (" << cmd_string << ") exited with code " On 2012/09/21 14:20:33, grt wrote: > PLOG -> LOG Done. https://codereview.chromium.org/10957016/diff/4002/chrome/installer/util/goog... chrome/installer/util/google_update_util.cc:109: PLOG(ERROR) << cmd_name << " (" << cmd_string << ") exited with code " On 2012/09/21 14:08:49, erikwright wrote: > no need for cmd_name here. Done. https://codereview.chromium.org/10957016/diff/4002/chrome/installer/util/goog... chrome/installer/util/google_update_util.cc:114: LOG(ERROR) << cmd_name << " (" << cmd_string On 2012/09/21 14:08:49, erikwright wrote: > no need for cmd_name here. Done. https://codereview.chromium.org/10957016/diff/4002/chrome/installer/util/goog... chrome/installer/util/google_update_util.cc:114: LOG(ERROR) << cmd_name << " (" << cmd_string On 2012/09/21 14:20:33, grt wrote: > it's not clear to me that the process taking longer than you expect it to should > be treated as an error. certainly in the case of UninstallGoogleUpdate it's not > an error. since you don't terminate the process, it may complete 1ns after your > timeout. what then? As discussed, will wait indefinitely. However, this involves different flow, so I undid the refactoring, and have semi-duplicated code. https://codereview.chromium.org/10957016/diff/4002/chrome/installer/util/goog... chrome/installer/util/google_update_util.cc:127: LOG(INFO) << "Google Update is already installed."; On 2012/09/21 14:20:33, grt wrote: > i don't think this is needed. the log files get pretty big, so i think it's > good to take care not to put in information that's easily derived. in this > case, the next log message after "Ensuring..." will make it clear whether it was > already installed or not. Done. https://codereview.chromium.org/10957016/diff/4002/chrome/installer/util/goog... chrome/installer/util/google_update_util.cc:130: string16 cmd_name(L"Google Update installer"); On 2012/09/21 14:08:49, erikwright wrote: > There's no need to pass around cmd_name. For the purposes of logs, the actual > command-line is sufficient. Done. https://codereview.chromium.org/10957016/diff/4002/chrome/installer/util/goog... chrome/installer/util/google_update_util.cc:133: base::win::ScopedHandle process; On 2012/09/21 14:20:33, grt wrote: > i think this should check that the command string isn't empty before calling > Execute.... Execute itself doesn't need anything special to detect an empty > string since it'll eventually fail with an error. Added check for empty string upstream. https://codereview.chromium.org/10957016/diff/4002/chrome/installer/util/goog... chrome/installer/util/google_update_util.cc:134: return ExecuteGoogleUpdateCommand(install_cmd, cmd_name, &process) && On 2012/09/21 14:20:33, grt wrote: > please combine these two into one function that does the launch and wait. i > don't see any reason to make callers have three lines of code instead of one. Refactored another way! https://codereview.chromium.org/10957016/diff/4002/chrome/installer/util/goog... File chrome/installer/util/google_update_util.h (right): https://codereview.chromium.org/10957016/diff/4002/chrome/installer/util/goog... chrome/installer/util/google_update_util.h:19: // Return true if and only if this executes successfully. On 2012/09/21 14:20:33, grt wrote: > "Returns false if Google Update could not be executed." since successful > execution isn't well-defined. does it mean that Google Update was uninstalled, > or just that it was run? Done.
https://codereview.chromium.org/10957016/diff/12017/chrome/installer/setup/se... File chrome/installer/setup/setup_main.cc (right): https://codereview.chromium.org/10957016/diff/12017/chrome/installer/setup/se... chrome/installer/setup/setup_main.cc:767: if (installer_state.ensure_google_update_present()) { Skip this if system level. i.e., add the following to the condition on 767 && !system_level https://codereview.chromium.org/10957016/diff/12017/chrome/installer/util/goo... File chrome/installer/util/google_update_util.cc (right): https://codereview.chromium.org/10957016/diff/12017/chrome/installer/util/goo... chrome/installer/util/google_update_util.cc:29: // Returns true if and only if Google Update is present at the given level. if and only if -> iff https://codereview.chromium.org/10957016/diff/12017/chrome/installer/util/goo... chrome/installer/util/google_update_util.cc:35: // Returns GoogleUpdateSetup.exe's executable path at |system_level|, |system_level| -> the specified level https://codereview.chromium.org/10957016/diff/12017/chrome/installer/util/goo... chrome/installer/util/google_update_util.cc:55: // Assign |cmd_string| as command line to install Google Update at user-level, "If Google Update is present at system-level, sets |cmd_string| to the command line to install Google Update at user-level and returns true. Otherwise, clears |cmd_string| and returns false." https://codereview.chromium.org/10957016/diff/12017/chrome/installer/util/goo... chrome/installer/util/google_update_util.cc:62: if (google_update_setup.empty()) { if (!google_update_setup.empty()) { ... } return !cmd_string.empty(); https://codereview.chromium.org/10957016/diff/12017/chrome/installer/util/goo... chrome/installer/util/google_update_util.cc:65: cmd.SetProgram(google_update_setup); Declare cmd here, and use the constructor that takes a FilePath to the program. https://codereview.chromium.org/10957016/diff/12017/chrome/installer/util/goo... chrome/installer/util/google_update_util.cc:91: // WaitForExitCode() will releases handle for us, so using ProcessHandle. Use a ScopedHandle and call Take() when passing it to WaitForExitCode. https://codereview.chromium.org/10957016/diff/12017/chrome/installer/util/goo... chrome/installer/util/google_update_util.cc:99: // The process didn't finish in time, or GetExitCodeProcess failed. Remove "The process didn't finish in time, or " Fix the log message to be accurate. https://codereview.chromium.org/10957016/diff/12017/chrome/installer/util/goo... chrome/installer/util/google_update_util.cc:120: // WaitForExitCodeWithTimeout() will not releases handle for us. Remove this comment.
Turns out WaitForExitCode() calls:
bool success = WaitForExitCodeWithTimeout(
handle, exit_code, base::TimeDelta::FromMilliseconds(INFINITE));
and then frees the handle. I'll imitate this, and factor the code!
Please take another look. https://codereview.chromium.org/10957016/diff/12017/chrome/installer/setup/se... File chrome/installer/setup/setup_main.cc (right): https://codereview.chromium.org/10957016/diff/12017/chrome/installer/setup/se... chrome/installer/setup/setup_main.cc:767: if (installer_state.ensure_google_update_present()) { On 2012/09/21 19:10:06, erikwright wrote: > Skip this if system level. i.e., add the following to the condition on 767 > > && !system_level Done. https://codereview.chromium.org/10957016/diff/12017/chrome/installer/util/goo... File chrome/installer/util/google_update_util.cc (right): https://codereview.chromium.org/10957016/diff/12017/chrome/installer/util/goo... chrome/installer/util/google_update_util.cc:29: // Returns true if and only if Google Update is present at the given level. On 2012/09/21 19:10:06, erikwright wrote: > if and only if -> iff Done. https://codereview.chromium.org/10957016/diff/12017/chrome/installer/util/goo... chrome/installer/util/google_update_util.cc:35: // Returns GoogleUpdateSetup.exe's executable path at |system_level|, On 2012/09/21 19:10:06, erikwright wrote: > |system_level| -> the specified level Done. https://codereview.chromium.org/10957016/diff/12017/chrome/installer/util/goo... chrome/installer/util/google_update_util.cc:55: // Assign |cmd_string| as command line to install Google Update at user-level, On 2012/09/21 19:10:06, erikwright wrote: > "If Google Update is present at system-level, sets |cmd_string| to the command > line to install Google Update at user-level and returns true. Otherwise, clears > |cmd_string| and returns false." Done. https://codereview.chromium.org/10957016/diff/12017/chrome/installer/util/goo... chrome/installer/util/google_update_util.cc:62: if (google_update_setup.empty()) { On 2012/09/21 19:10:06, erikwright wrote: > if (!google_update_setup.empty()) { > ... > } > > return !cmd_string.empty(); Done. https://codereview.chromium.org/10957016/diff/12017/chrome/installer/util/goo... chrome/installer/util/google_update_util.cc:65: cmd.SetProgram(google_update_setup); On 2012/09/21 19:10:06, erikwright wrote: > Declare cmd here, and use the constructor that takes a FilePath to the program. Done. https://codereview.chromium.org/10957016/diff/12017/chrome/installer/util/goo... chrome/installer/util/google_update_util.cc:91: // WaitForExitCode() will releases handle for us, so using ProcessHandle. On 2012/09/21 19:10:06, erikwright wrote: > Use a ScopedHandle and call Take() when passing it to WaitForExitCode. Done. Now we can extract into common code! https://codereview.chromium.org/10957016/diff/12017/chrome/installer/util/goo... chrome/installer/util/google_update_util.cc:99: // The process didn't finish in time, or GetExitCodeProcess failed. On 2012/09/21 19:10:06, erikwright wrote: > Remove "The process didn't finish in time, or " > Fix the log message to be accurate. Moot, as this is moved to common code. https://codereview.chromium.org/10957016/diff/12017/chrome/installer/util/goo... chrome/installer/util/google_update_util.cc:120: // WaitForExitCodeWithTimeout() will not releases handle for us. On 2012/09/21 19:10:06, erikwright wrote: > Remove this comment. Done.
LGTM with nit. https://codereview.chromium.org/10957016/diff/1013/chrome/installer/util/goog... File chrome/installer/util/google_update_util.cc (right): https://codereview.chromium.org/10957016/diff/1013/chrome/installer/util/goog... chrome/installer/util/google_update_util.cc:89: // The GetExitCodeProcess failed. "The wait failed or timed-out."
OWNER review request to ben@, for chrome/chrome_installer_util.gypi Thanks!
Please check again. A broken unit test was fixed. https://codereview.chromium.org/10957016/diff/1013/chrome/installer/util/goog... File chrome/installer/util/google_update_util.cc (right): https://codereview.chromium.org/10957016/diff/1013/chrome/installer/util/goog... chrome/installer/util/google_update_util.cc:89: // The GetExitCodeProcess failed. On 2012/09/21 19:56:29, erikwright wrote: > "The wait failed or timed-out." Done.
Still LGTM.
Pinging ben@ for OWNER review.
lgtm w/ two nits which i leave up to you. https://codereview.chromium.org/10957016/diff/4017/chrome/installer/util/goog... File chrome/installer/util/google_update_util.cc (right): https://codereview.chromium.org/10957016/diff/4017/chrome/installer/util/goog... chrome/installer/util/google_update_util.cc:28: // Returns true iff Google Update is present at the given level. imo, "iff" doesn't make sense here. just use "if". https://codereview.chromium.org/10957016/diff/4017/chrome/installer/util/goog... File chrome/installer/util/google_update_util.h (right): https://codereview.chromium.org/10957016/diff/4017/chrome/installer/util/goog... chrome/installer/util/google_update_util.h:19: // Returns true if this executes successfully. you can remove this line since the next one is clear enough.
Done! https://codereview.chromium.org/10957016/diff/4017/chrome/installer/util/goog... File chrome/installer/util/google_update_util.cc (right): https://codereview.chromium.org/10957016/diff/4017/chrome/installer/util/goog... chrome/installer/util/google_update_util.cc:28: // Returns true iff Google Update is present at the given level. On 2012/09/24 15:23:39, grt wrote: > imo, "iff" doesn't make sense here. just use "if". Done. https://codereview.chromium.org/10957016/diff/4017/chrome/installer/util/goog... File chrome/installer/util/google_update_util.h (right): https://codereview.chromium.org/10957016/diff/4017/chrome/installer/util/goog... chrome/installer/util/google_update_util.h:19: // Returns true if this executes successfully. On 2012/09/24 15:23:39, grt wrote: > you can remove this line since the next one is clear enough. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/huangs@chromium.org/10957016/10006
Presubmit check for 10957016-10006 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
chrome
Presubmit checks took 1.6s to calculate.
OWNER review request to brettw@; looks like ben@ is busy. Thanks!
lgtm rubberstamp https://codereview.chromium.org/10957016/diff/10006/chrome/installer/util/goo... File chrome/installer/util/google_update_util.h (right): https://codereview.chromium.org/10957016/diff/10006/chrome/installer/util/goo... chrome/installer/util/google_update_util.h:14: bool EnsureUserLevelGoogleUpdatePresent(); I'm not very familiar with the installer code. Is it worth noting that theses are slow blocking operations? Or do we assume that all the installer ops are slow blocking I/O not bother calling it out? Feel free to use your judgement here.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/huangs@chromium.org/10957016/10006
https://codereview.chromium.org/10957016/diff/10006/chrome/installer/util/goo... File chrome/installer/util/google_update_util.h (right): https://codereview.chromium.org/10957016/diff/10006/chrome/installer/util/goo... chrome/installer/util/google_update_util.h:14: bool EnsureUserLevelGoogleUpdatePresent(); On 2012/09/26 18:17:32, brettw wrote: > I'm not very familiar with the installer code. Is it worth noting that theses > are slow blocking operations? Or do we assume that all the installer ops are > slow blocking I/O not bother calling it out? Feel free to use your judgement > here. The installer is single-threaded and there are many potentially blocking operations. In these cases, the comments do state that one function "waits until [GoogleUpdateSetup.exe] finishes" and the other mentions a timeout.
Retried try job too often for step(s) browser_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/huangs@chromium.org/10957016/10006
Change committed as 158962
lgtm |
