Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10905238:
If Chrome Binaries version > App Host version, then update App Host. (Closed)
|
Created:
8 years, 3 months ago by huangs Modified:
8 years, 3 months ago CC:
chromium-reviews, Aaron Boodman, mihaip-chromium-reviews_chromium.org, grt+watch_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionAfter delegating to chrome.exe (i.e. to launch a packaged app) check if app_host is out-of-date and, if so, invoke the Chrome Binaries installer to update it.
R=erikwright,grt
TBR=brettw
BUG=138315
NOTRY=True
TEST=Install Chrome vX at system level. Install App Host vY at user-level, where Y < X. Launch app_host.exe. Inspect "Properties" of app_host.exe and verify that it has been updated to vX.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=156934
Patch Set 1 #
Total comments: 16
Patch Set 2 : Refactoring; adding app_host_upgrade.*; focusing on system-level setup.exe. #
Total comments: 34
Patch Set 3 : Style issues; removing unneeded debug messages. #
Total comments: 63
Patch Set 4 : Renamed app_host_upgrade.* to update.*; changed how App Host version is obtained; moved code to upd… #
Total comments: 38
Patch Set 5 : Including version info in app_hosts.exe; making GetChromePathForInstallationLevel() check Chrome in… #
Total comments: 18
Patch Set 6 : Renaming main update routine to LaunchAppHostUpdate(); fixing parameters of helper routines. #
Total comments: 25
Patch Set 7 : Undo bad renaming; cosmetics. #
Total comments: 7
Patch Set 8 : Small edits. #
Messages
Total messages: 30 (0 generated)
This is a functional draft that need refactoring & comments. Please take a look. Thanks!
I think InstallationState (chrome/installer/util/installation_state.h) should move into a standalone target and we could probably get most of this from there at that point. But I don't know if that can be achieved in M23 and it would be an unlikely proposition for a merge. So continue with this approach for M23 and log an issue for the above refactoring, targeting M24. Other comments in the code. https://chromiumcodereview.appspot.com/10905238/diff/1/chrome/browser/extensi... File chrome/browser/extensions/app_host/app_host_main.cc (right): https://chromiumcodereview.appspot.com/10905238/diff/1/chrome/browser/extensi... chrome/browser/extensions/app_host/app_host_main.cc:54: // Upgrade App Host if its version is less than CHrome Binary's. CHrome -> Chrome https://chromiumcodereview.appspot.com/10905238/diff/1/chrome/browser/extensi... chrome/browser/extensions/app_host/app_host_main.cc:57: if (chrome_launcher_support::GetAppHostVersionString(&app_host_version_str) && It seems to me these methods should do the conversion to base::Version themselves. https://chromiumcodereview.appspot.com/10905238/diff/1/chrome/browser/extensi... chrome/browser/extensions/app_host/app_host_main.cc:58: chrome_launcher_support::GetAnyChromeBinariesVersionString( All of this only applies if ChromeBinaries are at system-level. So how about GetSystemLevelChromeBinariesVersion() . https://chromiumcodereview.appspot.com/10905238/diff/1/chrome/installer/launc... File chrome/installer/launcher_support/chrome_launcher_support.cc (right): https://chromiumcodereview.appspot.com/10905238/diff/1/chrome/installer/launc... chrome/installer/launcher_support/chrome_launcher_support.cc:39: namespace local_switches { It's in an anonymous namespace already. I don't think the local_switches namespace buys you anything. https://chromiumcodereview.appspot.com/10905238/diff/1/chrome/installer/launc... chrome/installer/launcher_support/chrome_launcher_support.cc:105: using base::win::RegKey; this 'using' can go away. https://chromiumcodereview.appspot.com/10905238/diff/1/chrome/installer/launc... chrome/installer/launcher_support/chrome_launcher_support.cc:106: FilePath chrome_exe_path; declaration of chrome_exe_path can go inside "if (!setup_exe_path.empty()) {" (combined with assignment in line 112). Then just move the "if (...) return chrome_exe_path" from line 122 inside the scope ending at line 120. https://chromiumcodereview.appspot.com/10905238/diff/1/chrome/installer/launc... chrome/installer/launcher_support/chrome_launcher_support.cc:128: bool GetAppHostVersionString(string16* version_str) { This should all be de-duped as: GetAppVersionString(const char* app_guid, InstallationLevel level, string16* version_str) { } https://chromiumcodereview.appspot.com/10905238/diff/1/chrome/installer/launc... File chrome/installer/launcher_support/chrome_launcher_support.h (right): https://chromiumcodereview.appspot.com/10905238/diff/1/chrome/installer/launc... chrome/installer/launcher_support/chrome_launcher_support.h:34: // TODO: Comments These new methods should be moved out of launcher_support into app_host itself for now. Create new files (c/b/e/ah/app_host_upgrade.{h,cc}) for them and add them to the app_host target.
Please take another look! https://chromiumcodereview.appspot.com/10905238/diff/1/chrome/browser/extensi... File chrome/browser/extensions/app_host/app_host_main.cc (right): https://chromiumcodereview.appspot.com/10905238/diff/1/chrome/browser/extensi... chrome/browser/extensions/app_host/app_host_main.cc:54: // Upgrade App Host if its version is less than CHrome Binary's. On 2012/09/12 15:53:50, erikwright wrote: > CHrome -> Chrome Done. https://chromiumcodereview.appspot.com/10905238/diff/1/chrome/browser/extensi... chrome/browser/extensions/app_host/app_host_main.cc:57: if (chrome_launcher_support::GetAppHostVersionString(&app_host_version_str) && On 2012/09/12 15:53:50, erikwright wrote: > It seems to me these methods should do the conversion to base::Version > themselves. Done. https://chromiumcodereview.appspot.com/10905238/diff/1/chrome/browser/extensi... chrome/browser/extensions/app_host/app_host_main.cc:58: chrome_launcher_support::GetAnyChromeBinariesVersionString( On 2012/09/12 15:53:50, erikwright wrote: > All of this only applies if ChromeBinaries are at system-level. So how about > GetSystemLevelChromeBinariesVersion() . Done. https://chromiumcodereview.appspot.com/10905238/diff/1/chrome/installer/launc... File chrome/installer/launcher_support/chrome_launcher_support.cc (right): https://chromiumcodereview.appspot.com/10905238/diff/1/chrome/installer/launc... chrome/installer/launcher_support/chrome_launcher_support.cc:39: namespace local_switches { On 2012/09/12 15:53:50, erikwright wrote: > It's in an anonymous namespace already. I don't think the local_switches > namespace buys you anything. Done. https://chromiumcodereview.appspot.com/10905238/diff/1/chrome/installer/launc... chrome/installer/launcher_support/chrome_launcher_support.cc:105: using base::win::RegKey; On 2012/09/12 15:53:50, erikwright wrote: > this 'using' can go away. Done. https://chromiumcodereview.appspot.com/10905238/diff/1/chrome/installer/launc... chrome/installer/launcher_support/chrome_launcher_support.cc:106: FilePath chrome_exe_path; On 2012/09/12 15:53:50, erikwright wrote: > declaration of chrome_exe_path can go inside "if (!setup_exe_path.empty()) {" > (combined with assignment in line 112). > > Then just move the "if (...) return chrome_exe_path" from line 122 inside the > scope ending at line 120. Done. https://chromiumcodereview.appspot.com/10905238/diff/1/chrome/installer/launc... chrome/installer/launcher_support/chrome_launcher_support.cc:128: bool GetAppHostVersionString(string16* version_str) { Done, but updating Version* instead of string16*; also moved to app_host_upgrade.cc. https://chromiumcodereview.appspot.com/10905238/diff/1/chrome/installer/launc... File chrome/installer/launcher_support/chrome_launcher_support.h (right): https://chromiumcodereview.appspot.com/10905238/diff/1/chrome/installer/launc... chrome/installer/launcher_support/chrome_launcher_support.h:34: // TODO: Comments On 2012/09/12 15:53:50, erikwright wrote: > These new methods should be moved out of launcher_support into app_host itself > for now. Create new files (c/b/e/ah/app_host_upgrade.{h,cc}) for them and add > them to the app_host target. Done.
Lots of minor comments. Generally LG. One thing that I remember, after reading through this, is that the app host will also install if Chrome is installed in single-install mode at system level. This implies that, if Chrome binaries is not available at system level, Chrome itself should next be searched for. It's harmless to do the same when looking at user-level, for simplicity, although this should never occur in practice. http://codereview.chromium.org/10905238/diff/1004/chrome/browser/extensions/a... File chrome/browser/extensions/app_host/app_host_main.cc (right): http://codereview.chromium.org/10905238/diff/1004/chrome/browser/extensions/a... chrome/browser/extensions/app_host/app_host_main.cc:10: #include "base/string16.h" do you still need this? http://codereview.chromium.org/10905238/diff/1004/chrome/browser/extensions/a... chrome/browser/extensions/app_host/app_host_main.cc:66: LOG(INFO) << "App Host version: " << app_host_version.GetString(); Remove these two log statements, though it would be helpful to include the versions in the statement at line 70. http://codereview.chromium.org/10905238/diff/1004/chrome/browser/extensions/a... chrome/browser/extensions/app_host/app_host_main.cc:71: if (!UpgradeAppHost(true)) { // Use system-level setup.exe No need for UpgradeAppHost to take a parameter. It should always be system-level. http://codereview.chromium.org/10905238/diff/1004/chrome/browser/extensions/a... File chrome/browser/extensions/app_host/app_host_upgrade.cc (right): http://codereview.chromium.org/10905238/diff/1004/chrome/browser/extensions/a... chrome/browser/extensions/app_host/app_host_upgrade.cc:6: #include "chrome/installer/launcher_support/chrome_launcher_support.h" chrome_launcher_support should be ordered with the other includes below. http://codereview.chromium.org/10905238/diff/1004/chrome/browser/extensions/a... chrome/browser/extensions/app_host/app_host_upgrade.cc:9: #include <tchar.h> what is tchar used for? http://codereview.chromium.org/10905238/diff/1004/chrome/browser/extensions/a... chrome/browser/extensions/app_host/app_host_upgrade.cc:20: const wchar_t kGoogleRegClientsKey[] = anonymous namespace for this and other constants not used outside this file. http://codereview.chromium.org/10905238/diff/1004/chrome/browser/extensions/a... chrome/browser/extensions/app_host/app_host_upgrade.cc:37: // TODO(huangs): Refactor parameter |system_level|. TODO unnecessary. http://codereview.chromium.org/10905238/diff/1004/chrome/browser/extensions/a... chrome/browser/extensions/app_host/app_host_upgrade.cc:41: using base::win::RegKey; This is only used once, right? So 'using' is overkill. http://codereview.chromium.org/10905238/diff/1004/chrome/browser/extensions/a... chrome/browser/extensions/app_host/app_host_upgrade.cc:45: LOG(INFO) << "Trying to get " << client_key; remove log statement. http://codereview.chromium.org/10905238/diff/1004/chrome/browser/extensions/a... chrome/browser/extensions/app_host/app_host_upgrade.cc:49: != ERROR_SUCCESS) || != should go on previous line (prefer wrap after operator) http://codereview.chromium.org/10905238/diff/1004/chrome/browser/extensions/a... chrome/browser/extensions/app_host/app_host_upgrade.cc:57: // TODO(huangs): Refactor parameter |system_level|. remove TODO http://codereview.chromium.org/10905238/diff/1004/chrome/browser/extensions/a... chrome/browser/extensions/app_host/app_host_upgrade.cc:70: if (CommandLine::ForCurrentProcess()->HasSwitch( Don't check the command-line for the current process. For now, always add kVerboseLogging but add a TODO to only add it if it's present in the setup arguments for the system-level chrome binaries. http://codereview.chromium.org/10905238/diff/1004/chrome/browser/extensions/a... chrome/browser/extensions/app_host/app_host_upgrade.cc:74: LOG(INFO) << "Command line: " << cmd_line.GetCommandLineString(); "Upgrading app_host.exe using command-line: ..." http://codereview.chromium.org/10905238/diff/1004/chrome/browser/extensions/a... File chrome/browser/extensions/app_host/app_host_upgrade.h (right): http://codereview.chromium.org/10905238/diff/1004/chrome/browser/extensions/a... chrome/browser/extensions/app_host/app_host_upgrade.h:8: #include "base/version.h" forward declare Version instead. http://codereview.chromium.org/10905238/diff/1004/chrome/installer/launcher_s... File chrome/installer/launcher_support/chrome_launcher_support.cc (right): http://codereview.chromium.org/10905238/diff/1004/chrome/installer/launcher_s... chrome/installer/launcher_support/chrome_launcher_support.cc:67: LOG(INFO) << "Seeking reg: " << kChromeBinariesRegClientStateKey; remove log statement. http://codereview.chromium.org/10905238/diff/1004/chrome/installer/launcher_s... chrome/installer/launcher_support/chrome_launcher_support.cc:70: // Grab the UninstallString (actually, uninstall progra)m from This comment is confusing, and actually no comment should be necessary here. http://codereview.chromium.org/10905238/diff/1004/chrome/installer/launcher_s... chrome/installer/launcher_support/chrome_launcher_support.cc:76: LOG(INFO) << "Seeking file: " << setup_exe_path.value(); Remove log statement.
Please take another look. I implemented everything except removing the parameter in UpgradeAppHost() (will discuss). https://chromiumcodereview.appspot.com/10905238/diff/1004/chrome/browser/exte... File chrome/browser/extensions/app_host/app_host_main.cc (right): https://chromiumcodereview.appspot.com/10905238/diff/1004/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_main.cc:10: #include "base/string16.h" Nope. Deleted. https://chromiumcodereview.appspot.com/10905238/diff/1004/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_main.cc:66: LOG(INFO) << "App Host version: " << app_host_version.GetString(); On 2012/09/13 01:13:44, erikwright wrote: > Remove these two log statements, though it would be helpful to include the > versions in the statement at line 70. Done. https://chromiumcodereview.appspot.com/10905238/diff/1004/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_main.cc:71: if (!UpgradeAppHost(true)) { // Use system-level setup.exe I did this to make the routine generic and dumb, so that the decision to choose system-level is made by the caller. The the comments on "why is this system-level only" would be centralized. Please reconsider. https://chromiumcodereview.appspot.com/10905238/diff/1004/chrome/browser/exte... File chrome/browser/extensions/app_host/app_host_upgrade.cc (right): https://chromiumcodereview.appspot.com/10905238/diff/1004/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_upgrade.cc:6: #include "chrome/installer/launcher_support/chrome_launcher_support.h" On 2012/09/13 01:13:44, erikwright wrote: > chrome_launcher_support should be ordered with the other includes below. Done. https://chromiumcodereview.appspot.com/10905238/diff/1004/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_upgrade.cc:9: #include <tchar.h> I mistakenly thought it was needed for wchar_t. Deleted. https://chromiumcodereview.appspot.com/10905238/diff/1004/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_upgrade.cc:20: const wchar_t kGoogleRegClientsKey[] = On 2012/09/13 01:13:44, erikwright wrote: > anonymous namespace for this and other constants not used outside this file. Done. https://chromiumcodereview.appspot.com/10905238/diff/1004/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_upgrade.cc:37: // TODO(huangs): Refactor parameter |system_level|. On 2012/09/13 01:13:44, erikwright wrote: > TODO unnecessary. Done. https://chromiumcodereview.appspot.com/10905238/diff/1004/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_upgrade.cc:41: using base::win::RegKey; On 2012/09/13 01:13:44, erikwright wrote: > This is only used once, right? So 'using' is overkill. Done. https://chromiumcodereview.appspot.com/10905238/diff/1004/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_upgrade.cc:45: LOG(INFO) << "Trying to get " << client_key; On 2012/09/13 01:13:44, erikwright wrote: > remove log statement. Done. https://chromiumcodereview.appspot.com/10905238/diff/1004/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_upgrade.cc:49: != ERROR_SUCCESS) || On 2012/09/13 01:13:44, erikwright wrote: > != should go on previous line (prefer wrap after operator) Done. https://chromiumcodereview.appspot.com/10905238/diff/1004/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_upgrade.cc:57: // TODO(huangs): Refactor parameter |system_level|. On 2012/09/13 01:13:44, erikwright wrote: > remove TODO Done. https://chromiumcodereview.appspot.com/10905238/diff/1004/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_upgrade.cc:70: if (CommandLine::ForCurrentProcess()->HasSwitch( On 2012/09/13 01:13:44, erikwright wrote: > Don't check the command-line for the current process. For now, always add > kVerboseLogging but add a TODO to only add it if it's present in the setup > arguments for the system-level chrome binaries. Done. https://chromiumcodereview.appspot.com/10905238/diff/1004/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_upgrade.cc:74: LOG(INFO) << "Command line: " << cmd_line.GetCommandLineString(); On 2012/09/13 01:13:44, erikwright wrote: > "Upgrading app_host.exe using command-line: ..." Done. https://chromiumcodereview.appspot.com/10905238/diff/1004/chrome/browser/exte... File chrome/browser/extensions/app_host/app_host_upgrade.h (right): https://chromiumcodereview.appspot.com/10905238/diff/1004/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_upgrade.h:8: #include "base/version.h" On 2012/09/13 01:13:44, erikwright wrote: > forward declare Version instead. Done. https://chromiumcodereview.appspot.com/10905238/diff/1004/chrome/installer/la... File chrome/installer/launcher_support/chrome_launcher_support.cc (right): https://chromiumcodereview.appspot.com/10905238/diff/1004/chrome/installer/la... chrome/installer/launcher_support/chrome_launcher_support.cc:67: LOG(INFO) << "Seeking reg: " << kChromeBinariesRegClientStateKey; On 2012/09/13 01:13:44, erikwright wrote: > remove log statement. Done. https://chromiumcodereview.appspot.com/10905238/diff/1004/chrome/installer/la... chrome/installer/launcher_support/chrome_launcher_support.cc:70: // Grab the UninstallString (actually, uninstall progra)m from On 2012/09/13 01:13:44, erikwright wrote: > This comment is confusing, and actually no comment should be necessary here. Done. https://chromiumcodereview.appspot.com/10905238/diff/1004/chrome/installer/la... chrome/installer/launcher_support/chrome_launcher_support.cc:76: LOG(INFO) << "Seeking file: " << setup_exe_path.value(); On 2012/09/13 01:13:44, erikwright wrote: > Remove log statement. Done.
drive-by https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... File chrome/browser/extensions/app_host/app_host_main.cc (right): https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_main.cc:49: if (launch_result) { no braces for single-line conditionals like this https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_main.cc:55: // If system-level Chrome Binary is installed, and if its version is i think this block of code is big enough and isolated enough that it would fit nicely in its own function. https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_main.cc:56: // newer than App Host's, then upgrade App Host by calling setup.exe. please change "upgrade" in all cases to "update" .the latter implies that the new version is better in some way, which is subjective) also, "update" is consistent with Google Update and as much of the codebase as i've been able to influence. https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_main.cc:62: if (GetAppVersionForInstallationLevel(kChromeAppHostAppGuid, if this is used to get the version of the currently running app host exe, a much more reliable way is to introspect into the version resource of the running process. our use of the registry for this has historically been problematic. search for CreateFileVersionInfoForCurrentModule in client_util.cc for an example of this. https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_main.cc:66: LOG(INFO) << "App Host version (" << app_host_version.GetString() << wrap use of the streaming operator by putting the "<<" down on the next line and align it with the one on this line. https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_main.cc:69: "), so running setup.exe to upgrade"; general comment: make the log message as concise as possible since it contributes to the size of the binary and download. i suggest something simple and clear like: "Updating App Host from foo to bar". https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_main.cc:70: if (!UpgradeAppHost(true)) { // Use system-level setup.exe no braces https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_main.cc:75: LOG(INFO) << "Failed to get App Host version"; why isn't this an ERROR? https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_main.cc:79: return launch_result ? 0 : 1; optional: return !launch_result; is equivalent and tighter (integral promotion dictates that a bool is promoted to 0 or 1). https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... File chrome/browser/extensions/app_host/app_host_upgrade.cc (right): https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_upgrade.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. since this is in the app_host directory, does it need to be called app_host_upgrade? https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_upgrade.cc:22: L"Software\\Google\\Update\\Clients\\"; this fits on one line https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_upgrade.cc:24: // Copied from util_constants.cc period at the end https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_upgrade.cc:29: // Copied from google_update_constants.cc period at the end https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_upgrade.cc:34: // Copied from chrome_appid.cc period at the end also, please file a bug to remove the duplication if you haven't already https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_upgrade.cc:48: if ((reg_key.Open(root_key, client_key.c_str(), KEY_QUERY_VALUE) != this is more readable if wrapped just before KEY_QUERY_VALUE https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_upgrade.cc:58: using namespace chrome_launcher_support; is this really needed to make the code below more readable? https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_upgrade.cc:63: LOG(INFO) << "Failed to find setup.exe"; INFO -> ERROR? https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_upgrade.cc:69: // TODO: Make --verbose-logging depend on setup arguments for the i pose that it's better to unconditionally add --verbose-logging for all of eternity. https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_upgrade.cc:72: LOG(INFO) << "Upgrading App Host using command line: " << suggest: "Launching: " https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... File chrome/browser/extensions/app_host/app_host_upgrade.h (right): https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_upgrade.h:18: bool GetAppVersionForInstallationLevel(const wchar_t* app_guid, Version has value semantics (see the log message for r88143). a new convention is for functions like this to return a Version instance and for the caller to use its IsValid() method to determine if it got a version or not. this often simplifies things since callers no longer need to test two different things (did the fn return true and is the version it gave valid). https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_upgrade.h:18: bool GetAppVersionForInstallationLevel(const wchar_t* app_guid, please remove "ForInstallationLevel". it's superfluous and inconsistent with most/all other function that takes a bool for system_level or user_level. https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_upgrade.h:22: // Calls setup.exe to reinstall App Host, using setup.exe for the "reinstall" -> "update" https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/installer/la... File chrome/installer/launcher_support/chrome_launcher_support.cc (right): https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/installer/la... chrome/installer/launcher_support/chrome_launcher_support.cc:28: // Copied from util_constants.cc period at the end https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/installer/la... chrome/installer/launcher_support/chrome_launcher_support.cc:68: KEY_QUERY_VALUE) == ERROR_SUCCESS) { align with root_key https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/installer/la... chrome/installer/launcher_support/chrome_launcher_support.cc:71: // Trim the command line parameters. remove this comment as the command-line parameters are not part of this string https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/installer/la... chrome/installer/launcher_support/chrome_launcher_support.cc:80: FilePath GetChromePathForInstallationLevel(InstallationLevel level) { this will no longer work for single installs. is this intentional? https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/installer/la... chrome/installer/launcher_support/chrome_launcher_support.cc:92: chrome_exe_path = this fits on one line
Thanks grt@ for your great comments. https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... File chrome/browser/extensions/app_host/app_host_main.cc (right): https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_main.cc:56: // newer than App Host's, then upgrade App Host by calling setup.exe. On 2012/09/13 13:14:42, grt wrote: > please change "upgrade" in all cases to "update" .the latter implies that the > new version is better in some way, which is subjective) also, "update" is > consistent with Google Update and as much of the codebase as i've been able to > influence. By "the latter" grt@ meant "the former". https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_main.cc:66: LOG(INFO) << "App Host version (" << app_host_version.GetString() << On 2012/09/13 13:14:43, grt wrote: > wrap use of the streaming operator by putting the "<<" down on the next line and > align it with the one on this line. Correct. Note this is an exception to my previous comment about preferring to wrap after operators. https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... File chrome/browser/extensions/app_host/app_host_upgrade.cc (right): https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_upgrade.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2012/09/13 13:14:43, grt wrote: > since this is in the app_host directory, does it need to be called > app_host_upgrade? I requested the file-name because of a possibly mistaken belief that two .cc files with the same name, even in different directories, would cause build failures and 'upgrade' seemed too collision-prone. But after discussion with grt I think that must have been only two files in the same target. So this file could be renamed to update.cc (since we are also switching 'upgrade'->'update'). https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_upgrade.cc:34: // Copied from chrome_appid.cc On 2012/09/13 13:14:43, grt wrote: > period at the end > also, please file a bug to remove the duplication if you haven't already I believe we have - perhaps just reference it here. https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... File chrome/browser/extensions/app_host/app_host_upgrade.h (right): https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_upgrade.h:17: // Returns: true if successful. Remove the ':'. "Returns true if successful." Also below. https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/installer/la... File chrome/installer/launcher_support/chrome_launcher_support.cc (right): https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/installer/la... chrome/installer/launcher_support/chrome_launcher_support.cc:80: FilePath GetChromePathForInstallationLevel(InstallationLevel level) { On 2012/09/13 13:14:43, grt wrote: > this will no longer work for single installs. is this intentional? No. I mentioned this also in my last review.
Please take another look. Thanks! https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... File chrome/browser/extensions/app_host/app_host_main.cc (right): https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_main.cc:49: if (launch_result) { On 2012/09/13 13:14:42, grt wrote: > no braces for single-line conditionals like this Done. https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_main.cc:55: // If system-level Chrome Binary is installed, and if its version is Okay, moving into app_host_upgrade.cc. Also hiding constants and function that don't need to be exposed any more (per discussion). https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_main.cc:56: // newer than App Host's, then upgrade App Host by calling setup.exe. Makes sense. Done! https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_main.cc:62: if (GetAppVersionForInstallationLevel(kChromeAppHostAppGuid, On 2012/09/13 13:14:42, grt wrote: > if this is used to get the version of the currently running app host exe, a much > more reliable way is to introspect into the version resource of the running > process. our use of the registry for this has historically been problematic. > search for CreateFileVersionInfoForCurrentModule in client_util.cc for an > example of this. Done. https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_main.cc:66: LOG(INFO) << "App Host version (" << app_host_version.GetString() << On 2012/09/13 13:48:26, erikwright wrote: > On 2012/09/13 13:14:43, grt wrote: > > wrap use of the streaming operator by putting the "<<" down on the next line > and > > align it with the one on this line. > > Correct. Note this is an exception to my previous comment about preferring to > wrap after operators. Done. https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_main.cc:69: "), so running setup.exe to upgrade"; On 2012/09/13 13:14:43, grt wrote: > general comment: make the log message as concise as possible since it > contributes to the size of the binary and download. i suggest something simple > and clear like: "Updating App Host from foo to bar". Done. https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_main.cc:70: if (!UpgradeAppHost(true)) { // Use system-level setup.exe On 2012/09/13 13:14:43, grt wrote: > no braces Done. https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_main.cc:75: LOG(INFO) << "Failed to get App Host version"; On 2012/09/13 13:14:43, grt wrote: > why isn't this an ERROR? Done. https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_main.cc:79: return launch_result ? 0 : 1; On 2012/09/13 13:14:43, grt wrote: > optional: > return !launch_result; is equivalent and tighter (integral promotion dictates > that a bool is promoted to 0 or 1). Done. https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... File chrome/browser/extensions/app_host/app_host_upgrade.cc (right): https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_upgrade.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2012/09/13 13:48:26, erikwright wrote: > On 2012/09/13 13:14:43, grt wrote: > > since this is in the app_host directory, does it need to be called > > app_host_upgrade? > > I requested the file-name because of a possibly mistaken belief that two .cc > files with the same name, even in different directories, would cause build > failures and 'upgrade' seemed too collision-prone. > > But after discussion with grt I think that must have been only two files in the > same target. So this file could be renamed to update.cc (since we are also > switching 'upgrade'->'update'). Done. https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_upgrade.cc:22: L"Software\\Google\\Update\\Clients\\"; On 2012/09/13 13:14:43, grt wrote: > this fits on one line Done. https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_upgrade.cc:24: // Copied from util_constants.cc On 2012/09/13 13:14:43, grt wrote: > period at the end Done. https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_upgrade.cc:29: // Copied from google_update_constants.cc On 2012/09/13 13:14:43, grt wrote: > period at the end Done. https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_upgrade.cc:34: // Copied from chrome_appid.cc On 2012/09/13 13:48:26, erikwright wrote: > On 2012/09/13 13:14:43, grt wrote: > > period at the end > > also, please file a bug to remove the duplication if you haven't already > > I believe we have - perhaps just reference it here. Done. It was filed as http://code.google.com/p/chromium/issues/detail?id=148538 https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_upgrade.cc:48: if ((reg_key.Open(root_key, client_key.c_str(), KEY_QUERY_VALUE) != On 2012/09/13 13:14:43, grt wrote: > this is more readable if wrapped just before KEY_QUERY_VALUE Done. https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_upgrade.cc:57: bool UpgradeAppHost(bool system_level) { Removing redundant parameter, per discussion. https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_upgrade.cc:58: using namespace chrome_launcher_support; On 2012/09/13 13:14:43, grt wrote: > is this really needed to make the code below more readable? Used to be used 3 times, now 2 times. So removing this. https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_upgrade.cc:63: LOG(INFO) << "Failed to find setup.exe"; On 2012/09/13 13:14:43, grt wrote: > INFO -> ERROR? Done. https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_upgrade.cc:69: // TODO: Make --verbose-logging depend on setup arguments for the On 2012/09/13 13:14:43, grt wrote: > i pose that it's better to unconditionally add --verbose-logging for all of > eternity. Removing comment. https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_upgrade.cc:72: LOG(INFO) << "Upgrading App Host using command line: " << On 2012/09/13 13:14:43, grt wrote: > suggest: "Launching: " Done. https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... File chrome/browser/extensions/app_host/app_host_upgrade.h (right): https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_upgrade.h:17: // Returns: true if successful. Done (2 instances). https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_upgrade.h:18: bool GetAppVersionForInstallationLevel(const wchar_t* app_guid, On 2012/09/13 13:14:43, grt wrote: > Version has value semantics (see the log message for r88143). a new convention > is for functions like this to return a Version instance and for the caller to > use its IsValid() method to determine if it got a version or not. this often > simplifies things since callers no longer need to test two different things (did > the fn return true and is the version it gave valid). Done (for Version). https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_upgrade.h:18: bool GetAppVersionForInstallationLevel(const wchar_t* app_guid, On 2012/09/13 13:14:43, grt wrote: > Version has value semantics (see the log message for r88143). a new convention > is for functions like this to return a Version instance and for the caller to > use its IsValid() method to determine if it got a version or not. this often > simplifies things since callers no longer need to test two different things (did > the fn return true and is the version it gave valid). Done for "ForInstallationLevel", but only limited to the functions that take bool. https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_upgrade.h:22: // Calls setup.exe to reinstall App Host, using setup.exe for the On 2012/09/13 13:14:43, grt wrote: > "reinstall" -> "update" Done. https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/installer/la... File chrome/installer/launcher_support/chrome_launcher_support.cc (right): https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/installer/la... chrome/installer/launcher_support/chrome_launcher_support.cc:28: // Copied from util_constants.cc On 2012/09/13 13:14:43, grt wrote: > period at the end Done. https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/installer/la... chrome/installer/launcher_support/chrome_launcher_support.cc:68: KEY_QUERY_VALUE) == ERROR_SUCCESS) { On 2012/09/13 13:14:43, grt wrote: > align with root_key Done. https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/installer/la... chrome/installer/launcher_support/chrome_launcher_support.cc:71: // Trim the command line parameters. On 2012/09/13 13:14:43, grt wrote: > remove this comment as the command-line parameters are not part of this string Done. https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/installer/la... chrome/installer/launcher_support/chrome_launcher_support.cc:80: FilePath GetChromePathForInstallationLevel(InstallationLevel level) { (No-op) https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/installer/la... chrome/installer/launcher_support/chrome_launcher_support.cc:92: chrome_exe_path = On 2012/09/13 13:14:43, grt wrote: > this fits on one line Done.
It turns out app_host.exe does not contain version information. Trying to figure out how do so now.
https://chromiumcodereview.appspot.com/10905238/diff/9003/chrome/browser/exte... File chrome/browser/extensions/app_host/app_host_main.cc (right): https://chromiumcodereview.appspot.com/10905238/diff/9003/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_main.cc:17: using namespace chrome_launcher_support; It seems these are no longer high-value as the two 'using's only relieve three actual namespace uses. Ultimately, it's actually nice to see which functions are from where. https://chromiumcodereview.appspot.com/10905238/diff/9003/chrome/browser/exte... File chrome/browser/extensions/app_host/update.cc (right): https://chromiumcodereview.appspot.com/10905238/diff/9003/chrome/browser/exte... chrome/browser/extensions/app_host/update.cc:40: // This is the preferred method to no need for the "This is the preferred.." bit. https://chromiumcodereview.appspot.com/10905238/diff/9003/chrome/browser/exte... chrome/browser/extensions/app_host/update.cc:46: Version version = Version(WideToASCII(version_info->product_version())); just return directly. Don't assign to a local variable. https://chromiumcodereview.appspot.com/10905238/diff/9003/chrome/browser/exte... chrome/browser/extensions/app_host/update.cc:52: Version GetAppVersion(const wchar_t* app_guid, bool system_level) { remove the system_level parameter. It will always be true now. https://chromiumcodereview.appspot.com/10905238/diff/9003/chrome/browser/exte... chrome/browser/extensions/app_host/update.cc:59: KEY_QUERY_VALUE) != ERROR_SUCCESS) || KEY_QUERY_VALUE should line up with root_key https://chromiumcodereview.appspot.com/10905238/diff/9003/chrome/browser/exte... chrome/browser/extensions/app_host/update.cc:70: chrome_launcher_support::SYSTEM_LEVEL_INSTALLATION)); indent 4 from FilePath. https://chromiumcodereview.appspot.com/10905238/diff/9003/chrome/browser/exte... chrome/browser/extensions/app_host/update.cc:83: } } // namespace https://chromiumcodereview.appspot.com/10905238/diff/9003/chrome/browser/exte... chrome/browser/extensions/app_host/update.cc:88: if (chrome_binaries_version.IsValid()) { if chrome_binaries_version is not valid, next try to get chrome's version (to support single-install at system-level mode). https://chromiumcodereview.appspot.com/10905238/diff/9003/chrome/installer/la... File chrome/installer/launcher_support/chrome_launcher_support.cc (right): https://chromiumcodereview.appspot.com/10905238/diff/9003/chrome/installer/la... chrome/installer/launcher_support/chrome_launcher_support.cc:67: if (reg_key.Open(root_key, kChromeBinariesRegClientStateKey, If we can't find the ChromeBinaries, next try Chrome. https://chromiumcodereview.appspot.com/10905238/diff/9003/chrome/installer/la... File chrome/installer/launcher_support/chrome_launcher_support.h (right): https://chromiumcodereview.appspot.com/10905238/diff/9003/chrome/installer/la... chrome/installer/launcher_support/chrome_launcher_support.h:27: FilePath GetSetupExeForInstallationLevel(InstallationLevel level); Update documentation (and implementation) to say: "Prefers the installer from a multi-install, but may also return that of a single-install of Chrome if no multi-install exists."
https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/installer/la... File chrome/installer/launcher_support/chrome_launcher_support.cc (right): https://chromiumcodereview.appspot.com/10905238/diff/4002/chrome/installer/la... chrome/installer/launcher_support/chrome_launcher_support.cc:80: FilePath GetChromePathForInstallationLevel(InstallationLevel level) { On 2012/09/13 18:46:00, huangs1 wrote: > (No-op) no, please op. https://chromiumcodereview.appspot.com/10905238/diff/9003/chrome/browser/exte... File chrome/browser/extensions/app_host/app_host_main.cc (right): https://chromiumcodereview.appspot.com/10905238/diff/9003/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_main.cc:48: NULL); nit: move this to the previous line https://chromiumcodereview.appspot.com/10905238/diff/9003/chrome/browser/exte... File chrome/browser/extensions/app_host/update.cc (right): https://chromiumcodereview.appspot.com/10905238/diff/9003/chrome/browser/exte... chrome/browser/extensions/app_host/update.cc:24: // http://code.google.com/p/chromium/issues/detail?id=148538 please use http://crbug.com/148538 and move to previous line https://chromiumcodereview.appspot.com/10905238/diff/9003/chrome/browser/exte... chrome/browser/extensions/app_host/update.cc:45: DCHECK(version_info.get() != NULL); remove DCHECK since scoped_ptr<> operator-> asserts https://chromiumcodereview.appspot.com/10905238/diff/9003/chrome/browser/exte... chrome/browser/extensions/app_host/update.cc:58: if ((reg_key.Open(root_key, client_key.c_str(), optional nit: it seems more natural to me to reverse the logic here: if (this && that) return Version(WideToASCII(version_str)); return Version(); https://chromiumcodereview.appspot.com/10905238/diff/9003/chrome/browser/exte... chrome/browser/extensions/app_host/update.cc:70: chrome_launcher_support::SYSTEM_LEVEL_INSTALLATION)); On 2012/09/13 19:27:49, erikwright wrote: > indent 4 from FilePath. hmm, my gut tells me: FilePath setup_exe( chrome_launcher_support::GetSetupExeForInstallationLevel( chrome_launcher_support::SYSTEM_LEVEL_INSTALLATION)); https://chromiumcodereview.appspot.com/10905238/diff/9003/chrome/browser/exte... chrome/browser/extensions/app_host/update.cc:92: LOG(INFO) << "Updating App Host from" space after "from", and condense as much as possible: LOG(INFO) << "Updating App Host from " << app_host_version.GetString() << " to " << chrome_binaries_version.GetString(); https://chromiumcodereview.appspot.com/10905238/diff/9003/chrome/installer/la... File chrome/installer/launcher_support/chrome_launcher_support.cc (right): https://chromiumcodereview.appspot.com/10905238/diff/9003/chrome/installer/la... chrome/installer/launcher_support/chrome_launcher_support.cc:21: // http://code.google.com/p/chromium/issues/detail?id=148538 same comment about shrinking this
Please take another look! It only takes 2 lines to add Version info, and I verified that it's read by the code (hacked generated resource files to make version small). However, I still cannot get msbuild to build successfully. https://chromiumcodereview.appspot.com/10905238/diff/9003/chrome/browser/exte... File chrome/browser/extensions/app_host/app_host_main.cc (right): https://chromiumcodereview.appspot.com/10905238/diff/9003/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_main.cc:10: #include "base/version.h" Oops, this snuck in. Deleting. https://chromiumcodereview.appspot.com/10905238/diff/9003/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_main.cc:17: using namespace chrome_launcher_support; On 2012/09/13 19:27:49, erikwright wrote: > It seems these are no longer high-value as the two 'using's only relieve three > actual namespace uses. Ultimately, it's actually nice to see which functions are > from where. Done. https://chromiumcodereview.appspot.com/10905238/diff/9003/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_main.cc:48: NULL); On 2012/09/13 20:04:12, grt wrote: > nit: move this to the previous line Done. https://chromiumcodereview.appspot.com/10905238/diff/9003/chrome/browser/exte... File chrome/browser/extensions/app_host/update.cc (right): https://chromiumcodereview.appspot.com/10905238/diff/9003/chrome/browser/exte... chrome/browser/extensions/app_host/update.cc:24: // http://code.google.com/p/chromium/issues/detail?id=148538 On 2012/09/13 20:04:12, grt wrote: > please use http://crbug.com/148538 and move to previous line Done. https://chromiumcodereview.appspot.com/10905238/diff/9003/chrome/browser/exte... chrome/browser/extensions/app_host/update.cc:40: // This is the preferred method to On 2012/09/13 19:27:49, erikwright wrote: > no need for the "This is the preferred.." bit. Done. https://chromiumcodereview.appspot.com/10905238/diff/9003/chrome/browser/exte... chrome/browser/extensions/app_host/update.cc:45: DCHECK(version_info.get() != NULL); On 2012/09/13 20:04:12, grt wrote: > remove DCHECK since scoped_ptr<> operator-> asserts Done. https://chromiumcodereview.appspot.com/10905238/diff/9003/chrome/browser/exte... chrome/browser/extensions/app_host/update.cc:46: Version version = Version(WideToASCII(version_info->product_version())); On 2012/09/13 19:27:49, erikwright wrote: > just return directly. Don't assign to a local variable. Done. Vestige of old "delete version_info;" code. https://chromiumcodereview.appspot.com/10905238/diff/9003/chrome/browser/exte... chrome/browser/extensions/app_host/update.cc:52: Version GetAppVersion(const wchar_t* app_guid, bool system_level) { On 2012/09/13 19:27:49, erikwright wrote: > remove the system_level parameter. It will always be true now. Done. https://chromiumcodereview.appspot.com/10905238/diff/9003/chrome/browser/exte... chrome/browser/extensions/app_host/update.cc:58: if ((reg_key.Open(root_key, client_key.c_str(), On 2012/09/13 20:04:12, grt wrote: > optional nit: it seems more natural to me to reverse the logic here: > if (this && that) > return Version(WideToASCII(version_str)); > return Version(); Done. de'Morgan's Law. https://chromiumcodereview.appspot.com/10905238/diff/9003/chrome/browser/exte... chrome/browser/extensions/app_host/update.cc:59: KEY_QUERY_VALUE) != ERROR_SUCCESS) || On 2012/09/13 19:27:49, erikwright wrote: > KEY_QUERY_VALUE should line up with root_key Done. https://chromiumcodereview.appspot.com/10905238/diff/9003/chrome/browser/exte... chrome/browser/extensions/app_host/update.cc:70: chrome_launcher_support::SYSTEM_LEVEL_INSTALLATION)); On 2012/09/13 20:04:12, grt wrote: > On 2012/09/13 19:27:49, erikwright wrote: > > indent 4 from FilePath. > > hmm, my gut tells me: > > FilePath setup_exe( > chrome_launcher_support::GetSetupExeForInstallationLevel( > chrome_launcher_support::SYSTEM_LEVEL_INSTALLATION)); Done. I prefer staircase. https://chromiumcodereview.appspot.com/10905238/diff/9003/chrome/browser/exte... chrome/browser/extensions/app_host/update.cc:83: } On 2012/09/13 19:27:49, erikwright wrote: > } // namespace Done. https://chromiumcodereview.appspot.com/10905238/diff/9003/chrome/browser/exte... chrome/browser/extensions/app_host/update.cc:88: if (chrome_binaries_version.IsValid()) { On 2012/09/13 19:27:49, erikwright wrote: > if chrome_binaries_version is not valid, next try to get chrome's version (to > support single-install at system-level mode). Done, but shuffled things around a lot. https://chromiumcodereview.appspot.com/10905238/diff/9003/chrome/browser/exte... chrome/browser/extensions/app_host/update.cc:92: LOG(INFO) << "Updating App Host from" On 2012/09/13 20:04:12, grt wrote: > space after "from", and condense as much as possible: > LOG(INFO) << "Updating App Host from " << app_host_version.GetString() > << " to " << chrome_binaries_version.GetString(); Done. https://chromiumcodereview.appspot.com/10905238/diff/9003/chrome/installer/la... File chrome/installer/launcher_support/chrome_launcher_support.cc (right): https://chromiumcodereview.appspot.com/10905238/diff/9003/chrome/installer/la... chrome/installer/launcher_support/chrome_launcher_support.cc:21: // http://code.google.com/p/chromium/issues/detail?id=148538 On 2012/09/13 20:04:12, grt wrote: > same comment about shrinking this Done. https://chromiumcodereview.appspot.com/10905238/diff/9003/chrome/installer/la... chrome/installer/launcher_support/chrome_launcher_support.cc:47: FilePath GetAnyChromePath() { Moving this routine to the end, so dependency propagates upward. https://chromiumcodereview.appspot.com/10905238/diff/9003/chrome/installer/la... chrome/installer/launcher_support/chrome_launcher_support.cc:67: if (reg_key.Open(root_key, kChromeBinariesRegClientStateKey, On 2012/09/13 19:27:49, erikwright wrote: > If we can't find the ChromeBinaries, next try Chrome. Sorry for the no-op; misread comments! Done. Extracted common code to GetSetupExeFromRegistry(). Also, prioritizing code symmetry over slight efficiency (e.g. not initializing setup_exe_path by constructor). https://chromiumcodereview.appspot.com/10905238/diff/9003/chrome/installer/la... File chrome/installer/launcher_support/chrome_launcher_support.h (right): https://chromiumcodereview.appspot.com/10905238/diff/9003/chrome/installer/la... chrome/installer/launcher_support/chrome_launcher_support.h:27: FilePath GetSetupExeForInstallationLevel(InstallationLevel level); On 2012/09/13 19:27:49, erikwright wrote: > Update documentation (and implementation) to say: "Prefers the installer from a > multi-install, but may also return that of a single-install of Chrome if no > multi-install exists." Done.
Looking pretty close. http://codereview.chromium.org/10905238/diff/4005/chrome/browser/extensions/a... File chrome/browser/extensions/app_host/update.cc (right): http://codereview.chromium.org/10905238/diff/4005/chrome/browser/extensions/a... chrome/browser/extensions/app_host/update.cc:85: Version new_version; combine declaration and assignment (lines 85,86) http://codereview.chromium.org/10905238/diff/4005/chrome/browser/extensions/a... chrome/browser/extensions/app_host/update.cc:96: if (!UpdateAppHost()) I think this should be renamed to LaunchAppHostUpdate() because it doesn't wait for the update to complete. Right now it's misleading. http://codereview.chromium.org/10905238/diff/4005/chrome/browser/extensions/a... chrome/browser/extensions/app_host/update.cc:97: LOG(ERROR) << "Failed to update App Host"; "Failed to launch App Host update." http://codereview.chromium.org/10905238/diff/4005/chrome/installer/launcher_s... File chrome/installer/launcher_support/chrome_launcher_support.cc (right): http://codereview.chromium.org/10905238/diff/4005/chrome/installer/launcher_s... chrome/installer/launcher_support/chrome_launcher_support.cc:46: // Looks for setup.exe from the registry field UninstallString from nit: registry field -> value "Reads the path to setup.exe from the value "UninstallString" within the specified product's ClientState registry key. Returns an empty FilePath if an error occurs or the product is not installed at the specified level." http://codereview.chromium.org/10905238/diff/4005/chrome/installer/launcher_s... chrome/installer/launcher_support/chrome_launcher_support.cc:66: HKEY root_key = (level == USER_LEVEL_INSTALLATION ? push this down into GetSetupExeFromRegistry (make InstallationLevel the first parameter instead of HKEY). http://codereview.chromium.org/10905238/diff/4005/chrome/installer/launcher_s... chrome/installer/launcher_support/chrome_launcher_support.cc:69: FilePath setup_exe_path; combine declaration and assignment. http://codereview.chromium.org/10905238/diff/4005/chrome/installer/launcher_s... chrome/installer/launcher_support/chrome_launcher_support.cc:72: GetSetupExeFromRegistry(root_key, kChromeBinariesRegClientStateKey); Define constants for the app guids and pass those instead of the full registry path. Concatenate the registry path and app guids in GetSetupExeFromRegistry. http://codereview.chromium.org/10905238/diff/4005/chrome/installer/launcher_s... chrome/installer/launcher_support/chrome_launcher_support.cc:85: FilePath chrome_exe_path; combine declaration and assignment.
Please check again. tl;dr on testing: I also tested the flow by the following steps: - Build mini_installer.exe in ninja with new code. - In VM: install system-level Chrome, install app_host.exe - in VM: Swap out setup.exe by the one in http://master.chrome.corp/official_builds/23.0.1265.0/win/ so we will have the embedded manifest (I still have no luck with msbuild.exe) - Build a "mutated" version of app_host.exe with deflated version number. This is done by the following (assuming everything is built) 1. touch app_host_exe.ver, and delete out\Debug\app_host.exe 2. ninja -C out\Debug -n -v app_host.exe => Generates 3 commands (#2 is huge) 3. cd out\Debug 4. Run command #1 manually. 5. Edit gen\chrome_version\app_host_exe_version.rc, change "ProductVersion" to something earlier. 6. Back in out\Debug, run commands #2 and #3 manually. 7. Now we have mutated app_host.exe, check version to verify. - Copy mutated app_host.exe to the right directory in VM. - In VM: Run app_host.exe. - We see that the file got replaced with normal version, and log says the expected things: [0818/100023:INFO:update.cc(93)] Updating App Host from 23.0.300.0 to 23.0.1263.0 [0818/100023:INFO:update.cc(77)] Launching: "C:\Program Files (x86)\Google\Chrome\Application\23.0.1263.0\Installer\setup.exe" --multi-install --app-host --verbose-logging https://chromiumcodereview.appspot.com/10905238/diff/4005/chrome/browser/exte... File chrome/browser/extensions/app_host/update.cc (right): https://chromiumcodereview.appspot.com/10905238/diff/4005/chrome/browser/exte... chrome/browser/extensions/app_host/update.cc:85: Version new_version; On 2012/09/14 01:21:36, erikwright wrote: > combine declaration and assignment (lines 85,86) Done. https://chromiumcodereview.appspot.com/10905238/diff/4005/chrome/browser/exte... chrome/browser/extensions/app_host/update.cc:96: if (!UpdateAppHost()) On 2012/09/14 01:21:36, erikwright wrote: > I think this should be renamed to LaunchAppHostUpdate() because it doesn't wait > for the update to complete. Right now it's misleading. Done. https://chromiumcodereview.appspot.com/10905238/diff/4005/chrome/browser/exte... chrome/browser/extensions/app_host/update.cc:97: LOG(ERROR) << "Failed to update App Host"; On 2012/09/14 01:21:36, erikwright wrote: > "Failed to launch App Host update." Done. https://chromiumcodereview.appspot.com/10905238/diff/4005/chrome/installer/la... File chrome/installer/launcher_support/chrome_launcher_support.cc (right): https://chromiumcodereview.appspot.com/10905238/diff/4005/chrome/installer/la... chrome/installer/launcher_support/chrome_launcher_support.cc:46: // Looks for setup.exe from the registry field UninstallString from On 2012/09/14 01:21:36, erikwright wrote: > nit: registry field -> value > "Reads the path to setup.exe from the value "UninstallString" within the > specified product's ClientState registry key. Returns an empty FilePath if an > error occurs or the product is not installed at the specified level." Done. Have quotation around "ClientState", too. https://chromiumcodereview.appspot.com/10905238/diff/4005/chrome/installer/la... chrome/installer/launcher_support/chrome_launcher_support.cc:66: HKEY root_key = (level == USER_LEVEL_INSTALLATION ? On 2012/09/14 01:21:36, erikwright wrote: > push this down into GetSetupExeFromRegistry (make InstallationLevel the first > parameter instead of HKEY). Done. Also wrapping "namespace chrome_launch_support" around anonymous namespace to use InstallationLevel. https://chromiumcodereview.appspot.com/10905238/diff/4005/chrome/installer/la... chrome/installer/launcher_support/chrome_launcher_support.cc:69: FilePath setup_exe_path; On 2012/09/14 01:21:36, erikwright wrote: > combine declaration and assignment. Done. https://chromiumcodereview.appspot.com/10905238/diff/4005/chrome/installer/la... chrome/installer/launcher_support/chrome_launcher_support.cc:72: GetSetupExeFromRegistry(root_key, kChromeBinariesRegClientStateKey); On 2012/09/14 01:21:36, erikwright wrote: > Define constants for the app guids and pass those instead of the full registry > path. > > Concatenate the registry path and app guids in GetSetupExeFromRegistry. Done. Duplicating the GUID definitions in update.cc, to avoid introducing externally visible strings that may conflict (we'll refactor this anyway). https://chromiumcodereview.appspot.com/10905238/diff/4005/chrome/installer/la... chrome/installer/launcher_support/chrome_launcher_support.cc:85: FilePath chrome_exe_path; On 2012/09/14 01:21:36, erikwright wrote: > combine declaration and assignment. Done.
LGTM once the naming misunderstanding is corrected. http://codereview.chromium.org/10905238/diff/4005/chrome/browser/extensions/a... File chrome/browser/extensions/app_host/update.cc (right): http://codereview.chromium.org/10905238/diff/4005/chrome/browser/extensions/a... chrome/browser/extensions/app_host/update.cc:96: if (!UpdateAppHost()) On 2012/09/14 04:03:14, huangs1 wrote: > On 2012/09/14 01:21:36, erikwright wrote: > > I think this should be renamed to LaunchAppHostUpdate() because it doesn't > wait > > for the update to complete. Right now it's misleading. > > Done. Sorry, misunderstanding: EnsureAppHostUpToDate was correctly named. UpdateAppHost should be renamed to LaunchAppHostUpdate
getting closer http://codereview.chromium.org/10905238/diff/9003/chrome/browser/extensions/a... File chrome/browser/extensions/app_host/update.cc (right): http://codereview.chromium.org/10905238/diff/9003/chrome/browser/extensions/a... chrome/browser/extensions/app_host/update.cc:24: // http://code.google.com/p/chromium/issues/detail?id=148538 On 2012/09/13 22:25:37, huangs1 wrote: > On 2012/09/13 20:04:12, grt wrote: > > please use http://crbug.com/148538 and move to previous line > > Done. I meant to put the bug number on the same line as the comment to save vertical space. Apologies for the ambiguous request. http://codereview.chromium.org/10905238/diff/9003/chrome/installer/launcher_s... File chrome/installer/launcher_support/chrome_launcher_support.cc (right): http://codereview.chromium.org/10905238/diff/9003/chrome/installer/launcher_s... chrome/installer/launcher_support/chrome_launcher_support.cc:21: // http://code.google.com/p/chromium/issues/detail?id=148538 On 2012/09/13 22:25:37, huangs1 wrote: > On 2012/09/13 20:04:12, grt wrote: > > same comment about shrinking this > > Done. Same comment about putting bug number on same line as comment. http://codereview.chromium.org/10905238/diff/5015/chrome/app/app_host_exe.ver File chrome/app/app_host_exe.ver (right): http://codereview.chromium.org/10905238/diff/5015/chrome/app/app_host_exe.ver... chrome/app/app_host_exe.ver:2: ORIGINAL_FILENAME=app_host.exe PRODUCT_FULLNAME and PRODUCT_SHORTNAME? They default to "Google Chrome" and "Chrome" otherwise, which aren't obviously the right choice here. http://codereview.chromium.org/10905238/diff/5015/chrome/browser/extensions/a... File chrome/browser/extensions/app_host/app_host_main.cc (right): http://codereview.chromium.org/10905238/diff/5015/chrome/browser/extensions/a... chrome/browser/extensions/app_host/app_host_main.cc:43: NULL); ping: move this to the previous line, please. http://codereview.chromium.org/10905238/diff/5015/chrome/browser/extensions/a... File chrome/browser/extensions/app_host/update.cc (right): http://codereview.chromium.org/10905238/diff/5015/chrome/browser/extensions/a... chrome/browser/extensions/app_host/update.cc:36: const wchar_t kChromeBinariesAppGuid[] = please: - remove these constants - export the identical constants from launcher_support target and use those here in app_host target http://codereview.chromium.org/10905238/diff/5015/chrome/browser/extensions/a... chrome/browser/extensions/app_host/update.cc:40: // Fetches the version of the App Host, directly from the process. ", directly from the process." -> " from the image's version resource." http://codereview.chromium.org/10905238/diff/5015/chrome/browser/extensions/a... chrome/browser/extensions/app_host/update.cc:91: DCHECK(app_host_version.IsValid()); Version::Compare DCHECKs its args, so remove this. if you want, you could put a DCHECK inside of GetAppHostVersion since it makes sense there to enforce that the file actually has a version resource. http://codereview.chromium.org/10905238/diff/5015/chrome/browser/extensions/a... chrome/browser/extensions/app_host/update.cc:92: if (app_host_version.IsOlderThan(new_version.GetString())) { if (app_host_version.CompareTo(new_version) < 0) to avoid the extra round-trip to string and back. http://codereview.chromium.org/10905238/diff/5015/chrome/browser/extensions/a... File chrome/browser/extensions/app_host/update.h (right): http://codereview.chromium.org/10905238/diff/5015/chrome/browser/extensions/a... chrome/browser/extensions/app_host/update.h:12: void LaunchAppHostUpdate(); This only launches an update if one is needed, no? I find the name misleading in that case. How about "UpdateAppHostIfNeeded"? Please discuss with erikwright to come up with something mutually pleasing. :-) http://codereview.chromium.org/10905238/diff/5015/chrome/chrome.gyp File chrome/chrome.gyp (right): http://codereview.chromium.org/10905238/diff/5015/chrome/chrome.gyp#newcode880 chrome/chrome.gyp:880: 'app/app_host_exe.ver', sort http://codereview.chromium.org/10905238/diff/5015/chrome/installer/launcher_s... File chrome/installer/launcher_support/chrome_launcher_support.cc (right): http://codereview.chromium.org/10905238/diff/5015/chrome/installer/launcher_s... chrome/installer/launcher_support/chrome_launcher_support.cc:28: const wchar_t kChromeBinariesAppGuid[] = can you remove some line wrapping throughout this file if you make these constants kBinariesAppGuid and kBrowserAppGuid? http://codereview.chromium.org/10905238/diff/5015/chrome/installer/launcher_s... chrome/installer/launcher_support/chrome_launcher_support.cc:30: const wchar_t kChromeAppGuid[] = L"{8A69D345-D564-463c-AFF1-A69D9E530F96}"; this constant is from google_chrome_distribution.cc. http://codereview.chromium.org/10905238/diff/5015/chrome/installer/launcher_s... chrome/installer/launcher_support/chrome_launcher_support.cc:55: HKEY_CURRENT_USER : these need to be indented four more spaces, no? http://codereview.chromium.org/10905238/diff/5015/chrome/installer/launcher_s... chrome/installer/launcher_support/chrome_launcher_support.cc:81: GetSetupExeFromRegistry(level, kChromeAppGuid); move up to previous line and get rid of braces.
http://codereview.chromium.org/10905238/diff/5015/chrome/browser/extensions/a... File chrome/browser/extensions/app_host/update.cc (right): http://codereview.chromium.org/10905238/diff/5015/chrome/browser/extensions/a... chrome/browser/extensions/app_host/update.cc:36: const wchar_t kChromeBinariesAppGuid[] = On 2012/09/14 14:26:52, grt wrote: > please: > - remove these constants > - export the identical constants from launcher_support target and use those here > in app_host target Negotiated with grt that this can be ignored.
Please take another look. We're punting on Product Name change due to additional complexities. Filed https://code.google.com/p/chromium/issues/detail?id=149541 to keep track. https://chromiumcodereview.appspot.com/10905238/diff/9003/chrome/browser/exte... File chrome/browser/extensions/app_host/update.cc (right): https://chromiumcodereview.appspot.com/10905238/diff/9003/chrome/browser/exte... chrome/browser/extensions/app_host/update.cc:24: // http://code.google.com/p/chromium/issues/detail?id=148538 On 2012/09/14 14:26:52, grt wrote: > On 2012/09/13 22:25:37, huangs1 wrote: > > On 2012/09/13 20:04:12, grt wrote: > > > please use http://crbug.com/148538 and move to previous line > > > > Done. > > I meant to put the bug number on the same line as the comment to save vertical > space. Apologies for the ambiguous request. Done. Just realized that comments can appear in older patches... https://chromiumcodereview.appspot.com/10905238/diff/4005/chrome/browser/exte... File chrome/browser/extensions/app_host/update.cc (right): https://chromiumcodereview.appspot.com/10905238/diff/4005/chrome/browser/exte... chrome/browser/extensions/app_host/update.cc:96: if (!UpdateAppHost()) On 2012/09/14 13:21:24, erikwright wrote: > On 2012/09/14 04:03:14, huangs1 wrote: > > On 2012/09/14 01:21:36, erikwright wrote: > > > I think this should be renamed to LaunchAppHostUpdate() because it doesn't > > wait > > > for the update to complete. Right now it's misleading. > > > > Done. > > Sorry, misunderstanding: > > EnsureAppHostUpToDate was correctly named. > UpdateAppHost should be renamed to LaunchAppHostUpdate Done. https://chromiumcodereview.appspot.com/10905238/diff/5015/chrome/app/app_host... File chrome/app/app_host_exe.ver (right): https://chromiumcodereview.appspot.com/10905238/diff/5015/chrome/app/app_host... chrome/app/app_host_exe.ver:2: ORIGINAL_FILENAME=app_host.exe On 2012/09/14 14:26:52, grt wrote: > PRODUCT_FULLNAME and PRODUCT_SHORTNAME? They default to "Google Chrome" and > "Chrome" otherwise, which aren't obviously the right choice here. This involves additional complexity, and should be done as a separate task. Filed as: https://code.google.com/p/chromium/issues/detail?id=14954 https://chromiumcodereview.appspot.com/10905238/diff/5015/chrome/browser/exte... File chrome/browser/extensions/app_host/app_host_main.cc (right): https://chromiumcodereview.appspot.com/10905238/diff/5015/chrome/browser/exte... chrome/browser/extensions/app_host/app_host_main.cc:43: NULL); On 2012/09/14 14:26:52, grt wrote: > ping: move this to the previous line, please. Done. https://chromiumcodereview.appspot.com/10905238/diff/5015/chrome/browser/exte... File chrome/browser/extensions/app_host/update.cc (right): https://chromiumcodereview.appspot.com/10905238/diff/5015/chrome/browser/exte... chrome/browser/extensions/app_host/update.cc:36: const wchar_t kChromeBinariesAppGuid[] = So no-op. https://chromiumcodereview.appspot.com/10905238/diff/5015/chrome/browser/exte... chrome/browser/extensions/app_host/update.cc:40: // Fetches the version of the App Host, directly from the process. On 2012/09/14 14:26:52, grt wrote: > ", directly from the process." -> " from the image's version resource." Done. https://chromiumcodereview.appspot.com/10905238/diff/5015/chrome/browser/exte... chrome/browser/extensions/app_host/update.cc:91: DCHECK(app_host_version.IsValid()); On 2012/09/14 14:26:52, grt wrote: > Version::Compare DCHECKs its args, so remove this. if you want, you could put a > DCHECK inside of GetAppHostVersion since it makes sense there to enforce that > the file actually has a version resource. Done. https://chromiumcodereview.appspot.com/10905238/diff/5015/chrome/browser/exte... chrome/browser/extensions/app_host/update.cc:92: if (app_host_version.IsOlderThan(new_version.GetString())) { On 2012/09/14 14:26:52, grt wrote: > if (app_host_version.CompareTo(new_version) < 0) to avoid the extra round-trip > to string and back. Done. https://chromiumcodereview.appspot.com/10905238/diff/5015/chrome/browser/exte... File chrome/browser/extensions/app_host/update.h (right): https://chromiumcodereview.appspot.com/10905238/diff/5015/chrome/browser/exte... chrome/browser/extensions/app_host/update.h:12: void LaunchAppHostUpdate(); On 2012/09/14 14:26:52, grt wrote: > This only launches an update if one is needed, no? I find the name misleading > in that case. How about "UpdateAppHostIfNeeded"? Please discuss with > erikwright to come up with something mutually pleasing. :-) Reverted to old name EnsureAppHostUpToDate(). https://chromiumcodereview.appspot.com/10905238/diff/5015/chrome/chrome.gyp File chrome/chrome.gyp (right): https://chromiumcodereview.appspot.com/10905238/diff/5015/chrome/chrome.gyp#n... chrome/chrome.gyp:880: 'app/app_host_exe.ver', On 2012/09/14 14:26:52, grt wrote: > sort Done. https://chromiumcodereview.appspot.com/10905238/diff/5015/chrome/installer/la... File chrome/installer/launcher_support/chrome_launcher_support.cc (right): https://chromiumcodereview.appspot.com/10905238/diff/5015/chrome/installer/la... chrome/installer/launcher_support/chrome_launcher_support.cc:28: const wchar_t kChromeBinariesAppGuid[] = On 2012/09/14 14:26:52, grt wrote: > can you remove some line wrapping throughout this file if you make these > constants kBinariesAppGuid and kBrowserAppGuid? Done. Also changed in update.cc. https://chromiumcodereview.appspot.com/10905238/diff/5015/chrome/installer/la... chrome/installer/launcher_support/chrome_launcher_support.cc:30: const wchar_t kChromeAppGuid[] = L"{8A69D345-D564-463c-AFF1-A69D9E530F96}"; On 2012/09/14 14:26:52, grt wrote: > this constant is from google_chrome_distribution.cc. Done. https://chromiumcodereview.appspot.com/10905238/diff/5015/chrome/installer/la... chrome/installer/launcher_support/chrome_launcher_support.cc:55: HKEY_CURRENT_USER : On 2012/09/14 14:26:52, grt wrote: > these need to be indented four more spaces, no? Done. Also shifted brackets. https://chromiumcodereview.appspot.com/10905238/diff/5015/chrome/installer/la... chrome/installer/launcher_support/chrome_launcher_support.cc:81: GetSetupExeFromRegistry(level, kChromeAppGuid); On 2012/09/14 14:26:52, grt wrote: > move up to previous line and get rid of braces. Done.
https://chromiumcodereview.appspot.com/10905238/diff/10019/chrome/installer/l... File chrome/installer/launcher_support/chrome_launcher_support.cc (right): https://chromiumcodereview.appspot.com/10905238/diff/10019/chrome/installer/l... chrome/installer/launcher_support/chrome_launcher_support.cc:32: // Copied from google_chrome_distribution.cc. google_chrome_distribution.cc -> util_constants.cc https://chromiumcodereview.appspot.com/10905238/diff/10019/chrome/installer/l... chrome/installer/launcher_support/chrome_launcher_support.cc:56: HKEY_CURRENT_USER : HKEY_LOCAL_MACHINE; the second line shouldn't be aligned to the open paren of the first. i can't find a definitive reference, but i would choose one of the following: HKEY root_key = (level == USER_LEVEL_INSTALLATION) ? HKEY_CURRENT_USER : HKEY_LOCAL_MACHINE; HKEY root_key = (level == USER_LEVEL_INSTALLATION ? HKEY_CURRENT_USER : HKEY_LOCAL_MACHINE); both are a four-space indent from some reference. the reference point in the second case is the open paren, since the second line is a continuation of that sub-expression. erik, what do you think? https://chromiumcodereview.appspot.com/10905238/diff/10019/chrome/installer/l... chrome/installer/launcher_support/chrome_launcher_support.cc:78: if (setup_exe_path.empty()) { please remove the braces
https://chromiumcodereview.appspot.com/10905238/diff/10019/chrome/installer/l... File chrome/installer/launcher_support/chrome_launcher_support.cc (right): https://chromiumcodereview.appspot.com/10905238/diff/10019/chrome/installer/l... chrome/installer/launcher_support/chrome_launcher_support.cc:56: HKEY_CURRENT_USER : HKEY_LOCAL_MACHINE; On 2012/09/14 17:01:41, grt wrote: > the second line shouldn't be aligned to the open paren of the first. i can't > find a definitive reference, but i would choose one of the following: > > HKEY root_key = (level == USER_LEVEL_INSTALLATION) ? > HKEY_CURRENT_USER : HKEY_LOCAL_MACHINE; > HKEY root_key = (level == USER_LEVEL_INSTALLATION ? > HKEY_CURRENT_USER : HKEY_LOCAL_MACHINE); > > both are a four-space indent from some reference. the reference point in the > second case is the open paren, since the second line is a continuation of that > sub-expression. > > erik, what do you think? grt's first suggestion is correct.
Sorry about the braces. Please check again! https://chromiumcodereview.appspot.com/10905238/diff/10019/chrome/installer/l... File chrome/installer/launcher_support/chrome_launcher_support.cc (right): https://chromiumcodereview.appspot.com/10905238/diff/10019/chrome/installer/l... chrome/installer/launcher_support/chrome_launcher_support.cc:32: // Copied from google_chrome_distribution.cc. On 2012/09/14 17:01:41, grt wrote: > google_chrome_distribution.cc -> util_constants.cc Done. https://chromiumcodereview.appspot.com/10905238/diff/10019/chrome/installer/l... chrome/installer/launcher_support/chrome_launcher_support.cc:56: HKEY_CURRENT_USER : HKEY_LOCAL_MACHINE; On 2012/09/14 17:14:15, erikwright wrote: > On 2012/09/14 17:01:41, grt wrote: > > the second line shouldn't be aligned to the open paren of the first. i can't > > find a definitive reference, but i would choose one of the following: > > > > HKEY root_key = (level == USER_LEVEL_INSTALLATION) ? > > HKEY_CURRENT_USER : HKEY_LOCAL_MACHINE; > > HKEY root_key = (level == USER_LEVEL_INSTALLATION ? > > HKEY_CURRENT_USER : HKEY_LOCAL_MACHINE); > > > > both are a four-space indent from some reference. the reference point in the > > second case is the open paren, since the second line is a continuation of that > > sub-expression. > > > > erik, what do you think? > > grt's first suggestion is correct. Done. https://chromiumcodereview.appspot.com/10905238/diff/10019/chrome/installer/l... chrome/installer/launcher_support/chrome_launcher_support.cc:78: if (setup_exe_path.empty()) { On 2012/09/14 17:01:41, grt wrote: > please remove the braces Done. (sorry, forgot to save file!)
nice work. lgtm.
brettw: PTAL for chrome/*.gyp{i,} and chrome/app/*
Thanks!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/huangs@chromium.org/10905238/6004
gyp changes lgtm rubberstamp
Sorry for I got bad news for ya. Compile failed with a clobber build. Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/huangs@chromium.org/10905238/6004
Change committed as 156934
It looks like this CL broke the commit queue for everyone. (I note that it was landed with NOTRY=true.) https://codereview.chromium.org/10928221/ is a fix attempt.
I already submitted the fix. This CL actually correctly failed the CQ, and I created a separate one that contained the fix you submitted. But that one failed the CQ numerous times for various flaky reasons (though it had passed enough different bots in different CQ runs to see it was good). So I submitted it NOTRY=True. Unfortunately, I accidentally submitted the one that did not contain the fix, rather than the one that did. I submitted the fix separately today at around 11:15 PDT. Thanks for catching this and following up. Sorry for the trouble. -Erik On Sat, Sep 15, 2012 at 6:54 PM, <thakis@chromium.org> wrote: > It looks like this CL broke the commit queue for everyone. (I note that it > was > landed with NOTRY=true.) > > https://codereview.chromium.**org/10928221/<https://codereview.chromium.org/1... a fix attempt. > > https://chromiumcodereview.**appspot.com/10905238/<https://chromiumcodereview... > |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
