Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10823437:
Callback flow to register Chrome and update shortcuts after OS upgrade to Windows 8 (Closed)
|
Created:
8 years, 4 months ago by huangs Modified:
8 years, 3 months ago CC:
chromium-reviews, grt+watch_chromium.org, chrome-win8-eng_google.com Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionCallback flow to register Chrome and update shortcuts after OS upgrade to Windows 8
There are 2 parts to this CL:
1. Added --on-os-upgrade switch to setup.exe, which when invoked, executes ChromeBrowserOnOsUpgrade(), which registers Chrome and updates shortcuts, so that Metro Chrome would work in Windows.
2. During Chrome install, creates the registry key
HK??\Software\Google\Update\Clients\{Chrome GUID}\Commands\on-os-upgrade
containing
CommandLine: ...\setup.exe --on-os-upgrade ...
AutoRunOnOSUpgrade: 1
This is detected by Google Update, so that the command is executed when user logs in for the first time after OS upgrade.
BUG=127544
TEST=
1. In Windows 7 / Vista, install Chrome.
2. (Verify that the registry key exists).
3. Upgrade to Windows 8, while *keeping apps and settings* (this option may be unavailable for some versions of Windows).
4. After update, command line should be called automatically, and Metro Chrome should work.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=154630
Patch Set 1 #
Total comments: 16
Patch Set 2 : Response to review. However, additional major changes will follow. #Patch Set 3 : Reimplementing registry changes using AppCommand; adding checks in InstallationValidator. #
Total comments: 70
Patch Set 4 : Refactoring AppCommand constructor; replacing std::wstring with string16. #Patch Set 5 : Abandoning the use of ON_OS_UPGRADE_SUCCESSFUL installer message. #
Total comments: 87
Patch Set 6 : Refactoring; renamed OnOsUpgrade() to HandleOsUpgradeForBrowser(). #
Total comments: 18
Patch Set 7 : Refactoring; changing return value of --on-os-upgrade flow. #
Total comments: 15
Patch Set 8 : Refactoring and nits. #
Total comments: 12
Patch Set 9 : Supplying mock data for unit tests; refactoring. #
Total comments: 4
Patch Set 10 : Renaming kRegAutoRunOnOSUpgrade to kRegAutoRunOnOSUpgradeField; deleting bad debug message. #Messages
Total messages: 44 (0 generated)
Code-complete, although some testing is needed.
some drive-by comments. looks good overall. https://chromiumcodereview.appspot.com/10823437/diff/1/chrome/installer/setup... File chrome/installer/setup/install.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/1/chrome/installer/setup... chrome/installer/setup/install.cc:555: InstallStatus OnOsUpgrade(const InstallationState& original_state, since this does work specifically for the browser, I suggest putting Chrome in the function name somewhere so that it's clear that it isn't mean to be called for Chrome Frame, the binaries, or the app host. https://chromiumcodereview.appspot.com/10823437/diff/1/chrome/installer/setup... File chrome/installer/setup/install.h (right): https://chromiumcodereview.appspot.com/10823437/diff/1/chrome/installer/setup... chrome/installer/setup/install.h:111: } // namespace installer nit: add a newline before this line https://chromiumcodereview.appspot.com/10823437/diff/1/chrome/installer/setup... File chrome/installer/setup/install_worker.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/1/chrome/installer/setup... chrome/installer/setup/install_worker.cc:483: void AddOsUpgradeWorkItems(const InstallationState& original_state, Since this does work for the browser only, I think it's cleaner to 1) add a clause to AddProductSpecificWorkItems that calls this function, and 2) put Chrome in the function name in some way. This will let you get rid of the installer_state.state_type() call, which doesn't do what you think it does. https://chromiumcodereview.appspot.com/10823437/diff/1/chrome/installer/setup... chrome/installer/setup/install_worker.cc:490: // In Chrome Browser, add registry entries so that whenOS upgrade occurs, "whenOS" -> "when an OS" https://chromiumcodereview.appspot.com/10823437/diff/1/chrome/installer/setup... chrome/installer/setup/install_worker.cc:491: // Google Update would call setup.exe with --on-os-upgrade switch, which "would" -> "will" https://chromiumcodereview.appspot.com/10823437/diff/1/chrome/installer/setup... chrome/installer/setup/install_worker.cc:505: command.AppendSwitch(installer::switches::kOnOsUpgrade); my gut tells me that you want to add kMultiInstall to the command-line when this is a multi-install of Chrome. https://chromiumcodereview.appspot.com/10823437/diff/1/chrome/installer/setup... chrome/installer/setup/install_worker.cc:512: ->set_log_message("creating AppCommand command registry key"); does this use of set_log_message add useful meaning to the logs? i don't think this is standard practice in the installer. should it be? https://chromiumcodereview.appspot.com/10823437/diff/1/chrome/installer/setup... chrome/installer/setup/install_worker.cc:524: install_list->AddDeleteRegKeyWorkItem(root_key, is this necessary? the whole version key is deleted on uninstall, right?
Also, please update the CL description so that it includes a nice description of the change along with a TEST= line (see http://www.chromium.org/developers/contributing-code#TOC-Create-a-change-list...). Since you already have a BUG= line, you can remove the "Fixing Issue 127544..." remark.
On 2012/08/21 20:35:20, grt wrote: > Also, please update the CL description so that it includes a nice description of > the change along with a TEST= line (see > http://www.chromium.org/developers/contributing-code#TOC-Create-a-change-list...). > Since you already have a BUG= line, you can remove the "Fixing Issue 127544..." > remark. Right, you don't need the "fix *" part of the title either. Also the first line of the description is typically the title (the descriptions ends up being the comment on the commit). e.g. http://codereview.chromium.org/10825342/
On 2012/08/21 20:42:33, gab wrote: > On 2012/08/21 20:35:20, grt wrote: > > Also, please update the CL description so that it includes a nice description > of > > the change along with a TEST= line (see > > > http://www.chromium.org/developers/contributing-code#TOC-Create-a-change-list...). > > Since you already have a BUG= line, you can remove the "Fixing Issue > 127544..." > > remark. > > Right, you don't need the "fix *" part of the title either. > > Also the first line of the description is typically the title (the descriptions > ends up being the comment on the commit). > > e.g. http://codereview.chromium.org/10825342/ Also, in general, don't put 3 reviewers simultaneously. The correct way to go is to put a single reviewer (CC other people who may care) and when the CL is in a good stage, get the OWNERS to review the final CL (in this case we are all OWNERS of chrome\installer\* and you are not changing files outside of that so you won't need more reviewers (i.e. one of us is fine, but pick one, not 3 :)).
On 2012/08/21 20:47:20, gab wrote: > Also, in general, don't put 3 reviewers simultaneously. In Sam's defense, I wasn't on the original list (I did a drive-by).
On 2012/08/21 21:05:47, grt wrote: > On 2012/08/21 20:47:20, gab wrote: > > Also, in general, don't put 3 reviewers simultaneously. > > In Sam's defense, I wasn't on the original list (I did a drive-by). Ah ok, my bad :)! I'll continue with the review after you address Greg's initial comments then. Cheers! Gab
On 2012/08/21 21:12:22, gab wrote: > On 2012/08/21 21:05:47, grt wrote: > > On 2012/08/21 20:47:20, gab wrote: > > > Also, in general, don't put 3 reviewers simultaneously. > > > > In Sam's defense, I wasn't on the original list (I did a drive-by). > > Ah ok, my bad :)! > > I'll continue with the review after you address Greg's initial comments then. > > Cheers! > Gab Oh and while you're at fixing the description, you'll want to add a TEST= field below the BUG= field which will basically be a description of how QA/anyone could validate that this CL works (and which you hopefully also tried yourself :)). Cheers! Gab
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a lowly provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
https://chromiumcodereview.appspot.com/10823437/diff/1/chrome/installer/setup... File chrome/installer/setup/install.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/1/chrome/installer/setup... chrome/installer/setup/install.cc:555: InstallStatus OnOsUpgrade(const InstallationState& original_state, On 2012/08/21 20:32:55, grt wrote: > since this does work specifically for the browser, I suggest putting Chrome in > the function name somewhere so that it's clear that it isn't mean to be called > for Chrome Frame, the binaries, or the app host. Renamed to ChromeBrowserOnOsUpgrade() (to match AddChromeBrowserOsUpgradeWorkItems()). https://chromiumcodereview.appspot.com/10823437/diff/1/chrome/installer/setup... File chrome/installer/setup/install.h (right): https://chromiumcodereview.appspot.com/10823437/diff/1/chrome/installer/setup... chrome/installer/setup/install.h:111: } // namespace installer On 2012/08/21 20:32:55, grt wrote: > nit: add a newline before this line Done. https://chromiumcodereview.appspot.com/10823437/diff/1/chrome/installer/setup... File chrome/installer/setup/install_worker.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/1/chrome/installer/setup... chrome/installer/setup/install_worker.cc:483: void AddOsUpgradeWorkItems(const InstallationState& original_state, On 2012/08/21 20:32:55, grt wrote: > Since this does work for the browser only, I think it's cleaner to 1) add a > clause to AddProductSpecificWorkItems that calls this function, and 2) put > Chrome in the function name in some way. This will let you get rid of the > installer_state.state_type() call, which doesn't do what you think it does. Renaming to AddCromeBrowserOsUpgradeWorkItems(). Added "Browser" because "AddChromeOsUpgradeWorkItems()" is confusing, owing to "ChromeOs". I'm also moving routine to the bottom of the file. https://chromiumcodereview.appspot.com/10823437/diff/1/chrome/installer/setup... chrome/installer/setup/install_worker.cc:490: // In Chrome Browser, add registry entries so that whenOS upgrade occurs, On 2012/08/21 20:32:55, grt wrote: > "whenOS" -> "when an OS" Done. https://chromiumcodereview.appspot.com/10823437/diff/1/chrome/installer/setup... chrome/installer/setup/install_worker.cc:491: // Google Update would call setup.exe with --on-os-upgrade switch, which On 2012/08/21 20:32:55, grt wrote: > "would" -> "will" Done. https://chromiumcodereview.appspot.com/10823437/diff/1/chrome/installer/setup... chrome/installer/setup/install_worker.cc:505: command.AppendSwitch(installer::switches::kOnOsUpgrade); On 2012/08/21 20:32:55, grt wrote: > my gut tells me that you want to add kMultiInstall to the command-line when this > is a multi-install of Chrome. Done. https://chromiumcodereview.appspot.com/10823437/diff/1/chrome/installer/setup... chrome/installer/setup/install_worker.cc:512: ->set_log_message("creating AppCommand command registry key"); On 2012/08/21 20:32:55, grt wrote: > does this use of set_log_message add useful meaning to the logs? i don't think > this is standard practice in the installer. should it be? This was modelled after http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/installer/util/app_... which I just realized is out-of-date, so I'll remove redundant comments. https://chromiumcodereview.appspot.com/10823437/diff/1/chrome/installer/setup... chrome/installer/setup/install_worker.cc:524: install_list->AddDeleteRegKeyWorkItem(root_key, On 2012/08/21 20:32:55, grt wrote: > is this necessary? the whole version key is deleted on uninstall, right? I'm following the general style of the file, where add/remove come in pairs (remove is idempotent anyway). Will remove if you still don't like that. :)
Please take another look.
Looking good! Comments inline. Cheers, Gab https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/s... File chrome/installer/setup/install.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/s... chrome/installer/setup/install.cc:557: const CommandLine& cmd_line) { Since all you need is setup_exe here, take setup_exe as an argument instead of cmd_line. https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/s... chrome/installer/setup/install.cc:562: const FilePath& setup_exe = cmd_line.GetProgram(); Chromium style is to initialize non-POD types using: const foo& bar(myFoo); instead of: const foo& bar = myFoo; (this style is for POD types only, i.e. int, char, pointers, etc.) https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/s... chrome/installer/setup/install.cc:563: base::win::Version win_version = base::win::GetVersion(); inline in if condition https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/s... chrome/installer/setup/install.cc:569: DCHECK(win_version == base::win::VERSION_WIN8); Remove this comment/DCHECK, everything will need to be revisited on the next OS upgrade, registration and shortcuts would already be done this way on the next auto-update we are simply forcing it early here. https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/s... chrome/installer/setup/install.cc:580: return installer::ON_OS_UPGRADE_SUCCESSFUL; I don't think this has any value as there is no other return statement possible, having a void method accomplishes the same thing here. https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/s... File chrome/installer/setup/install.h (right): https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/s... chrome/installer/setup/install.h:106: // Code that needs to executed right after OS upgrade, when user logs in. This is not necessarily when the user logs in. For system-level installs this will happen at system boot and will be ran by SYSTEM before any log in. https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/s... chrome/installer/setup/install.h:108: InstallStatus ChromeBrowserOnOsUpgrade(const InstallationState& original_state, Remove "ChromeBrowser" from the name of this function. This is also valid for Chromium and could potentially be useful to non-browser products, since this is invoked by the generic --on-os-upgrade switch, keep the call generic too. https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/s... File chrome/installer/setup/install_worker.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/s... chrome/installer/setup/install_worker.cc:1605: void RenderChromeBrowserOsUpgradeCommand( s/Render/Get https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/s... chrome/installer/setup/install_worker.cc:1605: void RenderChromeBrowserOsUpgradeCommand( Remove "ChromeBrowser" from function name https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/s... chrome/installer/setup/install_worker.cc:1614: cmd_line->AppendSwitch(installer::switches::kChrome); nit: unindent 2 spaces https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/s... chrome/installer/setup/install_worker.cc:1629: std::wstring cmd_key(product.distribution()->GetVersionKey()); std::wstring -> string16 https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/s... chrome/installer/setup/install_worker.cc:1630: cmd_key.append(1, L'\\').append(google_update::kRegCommandsKey) use .push_back(FilePath::kSeparators[0]) instead of .append(1, L'\\') https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/s... File chrome/installer/setup/setup_main.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/s... chrome/installer/setup/setup_main.cc:1229: } else if (cmd_line.HasSwitch(installer::switches::kOnOsUpgrade)) { All the last switches are ChromeFrame related, I feel this belongs higher in this "else if madness", closer to the Chrome switches with similar goal like installer::switches::kRegisterChromeBrowser Conversely, please move all the other functions around to match the new alignment here (as it seems other functions, e.g. in install_worker.cc, do that). Please address comments in one patch set and do a move with no changes in another patch set (easier to see what changed in the diff). https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/u... File chrome/installer/util/app_command.h (right): https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/u... chrome/installer/util/app_command.h:28: AppCommand(const std::wstring& command_line, bool send_pings, Use string16 instead of std::wstring (or presubmit will fail when committing)... this implies you have to include "base/string16.h" and since I don't generally like to include both <string> and string16.h that means I'd like you to change all std::wstring to string16's in this file (I've done most of them in the installer already as part of other CLs, but looks like app_command.h is behind). https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/u... chrome/installer/util/app_command.h:29: bool is_web_accessible, bool is_auto_run_on_os_upgrade); Why do you need anything, but |is_auto_run_on_os_upgrade| here? How are |send_pings| and |is_web_accessible| relevant? https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/u... File chrome/installer/util/installation_validator.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/u... chrome/installer/util/installation_validator.cc:258: expected.push_back(std::make_pair(std::string(switches::kSystemLevel), What about switches::kChrome? https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/u... chrome/installer/util/installation_validator.cc:273: LOG(ERROR) << "On-os-upgrade command can send pings."; s/can/cannot https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/u... chrome/installer/util/installation_validator.cc:278: LOG(ERROR) << "On-os-upgrade command can be web accessible."; s/can/cannot https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/u... File chrome/installer/util/installation_validator.h (right): https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/u... chrome/installer/util/installation_validator.h:203: const ProductRules& rules): See style at http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Class_... 1) Constructor at top 2) Colon on first initializer line https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/u... chrome/installer/util/installation_validator.h:204: machine_state(machine_state), I don't know what the style is here, but I don't like that the variables and parameters have the same name... https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/u... File chrome/installer/util/work_item_list.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/u... chrome/installer/util/work_item_list.cc:203: const std::wstring& value_name, bool value_data) { string16 https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/u... chrome/installer/util/work_item_list.cc:206: (DWORD) 1, true); s/(DWORD) 1/static_cast<DWORD>(1) https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/u... File chrome/installer/util/work_item_list.h (right): https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/u... chrome/installer/util/work_item_list.h:116: // Add an "optional" boolean registry value, specified by value_data. Remove quotes around "optional" https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/u... chrome/installer/util/work_item_list.h:121: virtual WorkItem* AddSetOptionalBoolRegValueWorkItem( I believe all other methods in this file are tested. Add a test for this new method. https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/u... chrome/installer/util/work_item_list.h:122: HKEY predefined_root, const std::wstring& key_path, Use string16
Clarifying one comment. https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/u... File chrome/installer/util/installation_validator.h (right): https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/u... chrome/installer/util/installation_validator.h:204: machine_state(machine_state), On 2012/08/28 16:08:19, gab wrote: > I don't know what the style is here, but I don't like that the variables and > parameters have the same name... Discussed with Erik and Greg, suffix all the parameters with "_in".
lookin' pretty good. comments below. https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/s... File chrome/installer/setup/install.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/s... chrome/installer/setup/install.cc:580: return installer::ON_OS_UPGRADE_SUCCESSFUL; On 2012/08/28 16:08:19, gab wrote: > I don't think this has any value as there is no other return statement possible, > having a void method accomplishes the same thing here. should the CreateOrUpdate... and Register... funcs return their status so that it can be propagated here? https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/s... File chrome/installer/setup/install_worker.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/s... chrome/installer/setup/install_worker.cc:1605: void RenderChromeBrowserOsUpgradeCommand( suggestion: you can make this product-independent by passing in the const Product& product instance and using product.AppendProductFlags(cmd_line) in place of the manual appends of --chrome and --multi-install. in that case, i don't know that there's any good reason not to roll it into AddOsUpgradeWorkItems, so i propose that you do that too. https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/s... File chrome/installer/setup/install_worker.h (right): https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/s... chrome/installer/setup/install_worker.h:212: // For given product, provide command line that will be stored in doc comments for functions should be of the form: "Adds work items to |install_list| to register a command to be run by Google Update upon OS update for |product|." or something of that nature. see http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/s... File chrome/installer/setup/setup_main.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/s... chrome/installer/setup/setup_main.cc:1229: } else if (cmd_line.HasSwitch(installer::switches::kOnOsUpgrade)) { On 2012/08/28 16:08:19, gab wrote: > All the last switches are ChromeFrame related, I feel this belongs higher in > this "else if madness", closer to the Chrome switches with similar goal like > installer::switches::kRegisterChromeBrowser > > Conversely, please move all the other functions around to match the new > alignment here (as it seems other functions, e.g. in install_worker.cc, do > that). i strongly advise against changing the order of the other conditionals here. you'll break at least one feature if you reorder two of the clauses in particular. i'll leave it as an exercise to the reader to figure out which ones. :-) > Please address comments in one patch set and do a move with no changes in > another patch set (easier to see what changed in the diff). https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/s... chrome/installer/setup/setup_main.cc:1231: if (cmd_line.HasSwitch(installer::switches::kChrome)) { one way to make this more OO would be to add a method to the ProductOperations interface (you could call it HandleOSUpgrade, OnOsUpgrade, or somesuch), implement it for ChromeBrowserOperations, and add a Product::HandleOsUpgrade that delegated to the underlying operations implementation. the code here, then, could be totally generic: iterate over all products and call the new Product method. the registration code wouldn't change: register only for the product Chrome, and only Chrome's will be hit. if another product needs it in the future, it'll be clear where to put the code. https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/u... File chrome/installer/util/app_command.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/u... chrome/installer/util/app_command.cc:16: is_web_accessible_(false) { add is_auto_run_on_os_upgrade_(false) to the initializer list https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/u... chrome/installer/util/app_command.cc:58: if (result != ERROR_SUCCESS) { ReadValueDW only modifies its out_value on success, so you can remove each of these checks for the result (here and the two below). https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/u... chrome/installer/util/app_command.cc:91: predefined_root, command_path, google_update::kRegSendsPingsField, can these be wrapped like line 87 and 88 (as they were before)? i think that's more readable in this case on account of the call to set_log_message https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/u... File chrome/installer/util/app_command.h (right): https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/u... chrome/installer/util/app_command.h:28: AppCommand(const std::wstring& command_line, bool send_pings, since the command line is the only mandatory property of an app command, i suggest replacing these two many-arg ctors with one that takes only the command line. callers can then set only those bool attributes they care about. the callsites will be more readable in this case since the reader won't need to know what AppCommand(cmd_line, true, false, false) means. https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/u... File chrome/installer/util/installation_validator.h (right): https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/u... chrome/installer/util/installation_validator.h:204: machine_state(machine_state), On 2012/08/28 16:08:19, gab wrote: > I don't know what the style is here, but I don't like that the variables and > parameters have the same name... +1. https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/u... File chrome/installer/util/util_constants.h (right): https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/u... chrome/installer/util/util_constants.h:79: ON_OS_UPGRADE_SUCCESSFUL, // 46. Successfully updated after OS upgrade. this enum defines values that are returned to Google Update and eventually make it into logs. it doesn't appear that this new value ever makes it out of setup.exe. if this is true, please remove it. https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/u... File chrome/installer/util/work_item_list.h (right): https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/u... chrome/installer/util/work_item_list.h:116: // Add an "optional" boolean registry value, specified by value_data. i think this function is too special-purpose to belong here. i think it's more clear for the three callers to do the work themselves. you could add a private helper function to app_command.cc if you'd like.
comment on grt's comment https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/s... File chrome/installer/setup/setup_main.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/s... chrome/installer/setup/setup_main.cc:1229: } else if (cmd_line.HasSwitch(installer::switches::kOnOsUpgrade)) { On 2012/08/28 19:35:39, grt wrote: > On 2012/08/28 16:08:19, gab wrote: > > All the last switches are ChromeFrame related, I feel this belongs higher in > > this "else if madness", closer to the Chrome switches with similar goal like > > installer::switches::kRegisterChromeBrowser > > > > Conversely, please move all the other functions around to match the new > > alignment here (as it seems other functions, e.g. in install_worker.cc, do > > that). > > i strongly advise against changing the order of the other conditionals here. > you'll break at least one feature if you reorder two of the clauses in > particular. i'll leave it as an exercise to the reader to figure out which > ones. :-) No no no! I'm not suggesting to move other calls! I'm just saying this new one should be above (not here with Chrome Frame specific stuff). What I'm otherwise also saying is that the alignment of the new corresponding function in install_worker.cc and the likes should also be aligned at the same place (i.e. w.r.t other installer:: functions called here -- as those seem to respect the order here from what I looked at). I feel I'm having a hard-time expressing what I mean in words here, we can talk about it tomorrow if that's not clear. > > > Please address comments in one patch set and do a move with no changes in > > another patch set (easier to see what changed in the diff).
Please take another look! Please note that: - work_item_list.* are no longer changed. - In setup_main.cc, I didn't adopt the ProductOperations suggestion. https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/s... File chrome/installer/setup/install.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/s... chrome/installer/setup/install.cc:557: const CommandLine& cmd_line) { On 2012/08/28 16:08:19, gab wrote: > Since all you need is setup_exe here, take setup_exe as an argument instead of > cmd_line. Done. https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/s... chrome/installer/setup/install.cc:562: const FilePath& setup_exe = cmd_line.GetProgram(); On 2012/08/28 16:08:19, gab wrote: > Chromium style is to initialize non-POD types using: > const foo& bar(myFoo); > > instead of: > const foo& bar = myFoo; > (this style is for POD types only, i.e. int, char, pointers, etc.) Done. https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/s... chrome/installer/setup/install.cc:569: DCHECK(win_version == base::win::VERSION_WIN8); On 2012/08/28 16:08:19, gab wrote: > Remove this comment/DCHECK, everything will need to be revisited on the next OS > upgrade, registration and shortcuts would already be done this way on the next > auto-update we are simply forcing it early here. Done. https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/s... chrome/installer/setup/install.cc:580: return installer::ON_OS_UPGRADE_SUCCESSFUL; On 2012/08/28 19:35:39, grt wrote: > On 2012/08/28 16:08:19, gab wrote: > > I don't think this has any value as there is no other return statement > possible, > > having a void method accomplishes the same thing here. > > should the CreateOrUpdate... and Register... funcs return their status so that > it can be propagated here? The caller HandleNonInstallCmdLineOptions() expects some exit_code to be supplied. Since there is no generic "SUCCESS", so I created the extra flag ON_OS_UPGRADE_SUCCESSFUL to be returned here. https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/s... File chrome/installer/setup/install.h (right): https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/s... chrome/installer/setup/install.h:106: // Code that needs to executed right after OS upgrade, when user logs in. On 2012/08/28 16:08:19, gab wrote: > This is not necessarily when the user logs in. For system-level installs this > will happen at system boot and will be ran by SYSTEM before any log in. Done. https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/s... chrome/installer/setup/install.h:108: InstallStatus ChromeBrowserOnOsUpgrade(const InstallationState& original_state, On 2012/08/28 16:08:19, gab wrote: > Remove "ChromeBrowser" from the name of this function. This is also valid for > Chromium and could potentially be useful to non-browser products, since this is > invoked by the generic --on-os-upgrade switch, keep the call generic too. Done. Also removed "(with --chrome)" in comment. https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/s... File chrome/installer/setup/install_worker.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/s... chrome/installer/setup/install_worker.cc:1605: void RenderChromeBrowserOsUpgradeCommand( On 2012/08/28 19:35:39, grt wrote: > suggestion: you can make this product-independent by passing in the const > Product& product instance and using product.AppendProductFlags(cmd_line) in > place of the manual appends of --chrome and --multi-install. in that case, i > don't know that there's any good reason not to roll it into > AddOsUpgradeWorkItems, so i propose that you do that too. Took grt@'s approach. https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/s... chrome/installer/setup/install_worker.cc:1614: cmd_line->AppendSwitch(installer::switches::kChrome); On 2012/08/28 16:08:19, gab wrote: > nit: unindent 2 spaces Done. https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/s... chrome/installer/setup/install_worker.cc:1629: std::wstring cmd_key(product.distribution()->GetVersionKey()); Done. Also replacing everywhere else in file. On 2012/08/28 16:08:19, gab wrote: > std::wstring -> string16 https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/s... chrome/installer/setup/install_worker.cc:1630: cmd_key.append(1, L'\\').append(google_update::kRegCommandsKey) Okay, but .push_back() returns void, and cannot be chained. So I'm using .append(1, FilePath::kSeparators[0]) instead. On 2012/08/28 16:08:19, gab wrote: > use > .push_back(FilePath::kSeparators[0]) > instead of > .append(1, L'\\') https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/s... File chrome/installer/setup/install_worker.h (right): https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/s... chrome/installer/setup/install_worker.h:212: // For given product, provide command line that will be stored in On 2012/08/28 19:35:39, grt wrote: > doc comments for functions should be of the form: > "Adds work items to |install_list| to register a command to be run by Google > Update upon OS update for |product|." > or something of that nature. see > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... Done, and imitated comments from above. Also removed original_state, because it is unused! https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/s... File chrome/installer/setup/setup_main.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/s... chrome/installer/setup/setup_main.cc:1229: } else if (cmd_line.HasSwitch(installer::switches::kOnOsUpgrade)) { On 2012/08/28 19:35:39, grt wrote: > On 2012/08/28 16:08:19, gab wrote: > > All the last switches are ChromeFrame related, I feel this belongs higher in > > this "else if madness", closer to the Chrome switches with similar goal like > > installer::switches::kRegisterChromeBrowser > > > > Conversely, please move all the other functions around to match the new > > alignment here (as it seems other functions, e.g. in install_worker.cc, do > > that). > > i strongly advise against changing the order of the other conditionals here. > you'll break at least one feature if you reorder two of the clauses in > particular. i'll leave it as an exercise to the reader to figure out which > ones. :-) > > > Please address comments in one patch set and do a move with no changes in > > another patch set (easier to see what changed in the diff). Moved (and only moved) my code. https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/s... chrome/installer/setup/setup_main.cc:1231: if (cmd_line.HasSwitch(installer::switches::kChrome)) { Doing this would bring in another 11 files to the CL! Can I do this after, as a refactoring CL? https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/u... File chrome/installer/util/app_command.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/u... chrome/installer/util/app_command.cc:16: is_web_accessible_(false) { On 2012/08/28 19:35:39, grt wrote: > add is_auto_run_on_os_upgrade_(false) to the initializer list Done. https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/u... chrome/installer/util/app_command.cc:58: if (result != ERROR_SUCCESS) { On 2012/08/28 19:35:39, grt wrote: > ReadValueDW only modifies its out_value on success, so you can remove each of > these checks for the result (here and the two below). Done, also added comment. https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/u... chrome/installer/util/app_command.cc:91: predefined_root, command_path, google_update::kRegSendsPingsField, On 2012/08/28 19:35:39, grt wrote: > can these be wrapped like line 87 and 88 (as they were before)? i think that's > more readable in this case on account of the call to set_log_message Done, convenient since AddSetOptionalBoolRegValueWorkItem() now became a local helper! https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/u... File chrome/installer/util/app_command.h (right): https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/u... chrome/installer/util/app_command.h:28: AppCommand(const std::wstring& command_line, bool send_pings, On 2012/08/28 16:08:19, gab wrote: > Use string16 instead of std::wstring (or presubmit will fail when committing)... > this implies you have to include "base/string16.h" and since I don't generally > like to include both <string> and string16.h that means I'd like you to change > all std::wstring to string16's in this file (I've done most of them in the > installer already as part of other CLs, but looks like app_command.h is behind). Done (std::wstring --> string16). https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/u... chrome/installer/util/app_command.h:28: AppCommand(const std::wstring& command_line, bool send_pings, On 2012/08/28 19:35:40, grt wrote: > since the command line is the only mandatory property of an app command, i > suggest replacing these two many-arg ctors with one that takes only the command > line. callers can then set only those bool attributes they care about. the > callsites will be more readable in this case since the reader won't need to know > what AppCommand(cmd_line, true, false, false) means. Done (simplifying constructor; changed callers to use setters after construction). https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/u... chrome/installer/util/app_command.h:29: bool is_web_accessible, bool is_auto_run_on_os_upgrade); On 2012/08/28 16:08:19, gab wrote: > Why do you need anything, but |is_auto_run_on_os_upgrade| here? How are > |send_pings| and |is_web_accessible| relevant? Made changes suggested by grt@. https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/u... File chrome/installer/util/installation_validator.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/u... chrome/installer/util/installation_validator.cc:258: expected.push_back(std::make_pair(std::string(switches::kSystemLevel), On 2012/08/28 16:08:19, gab wrote: > What about switches::kChrome? Done. https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/u... chrome/installer/util/installation_validator.cc:273: LOG(ERROR) << "On-os-upgrade command can send pings."; On 2012/08/28 16:08:19, gab wrote: > s/can/cannot But it should not, so "can" == bad. I'll change the comment though. https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/u... chrome/installer/util/installation_validator.cc:278: LOG(ERROR) << "On-os-upgrade command can be web accessible."; On 2012/08/28 16:08:19, gab wrote: > s/can/cannot Same as before. https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/u... File chrome/installer/util/installation_validator.h (right): https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/u... chrome/installer/util/installation_validator.h:203: const ProductRules& rules): On 2012/08/28 16:08:19, gab wrote: > See style at > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Class_... > > 1) Constructor at top > 2) Colon on first initializer line Done. https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/u... chrome/installer/util/installation_validator.h:204: machine_state(machine_state), On 2012/08/28 16:08:19, gab wrote: > I don't know what the style is here, but I don't like that the variables and > parameters have the same name... Done. https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/u... File chrome/installer/util/util_constants.h (right): https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/u... chrome/installer/util/util_constants.h:79: ON_OS_UPGRADE_SUCCESSFUL, // 46. Successfully updated after OS upgrade. On 2012/08/28 19:35:40, grt wrote: > this enum defines values that are returned to Google Update and eventually make > it into logs. it doesn't appear that this new value ever makes it out of > setup.exe. if this is true, please remove it. Not removing this yet; please see comment in setup_main.cc and we'll see. https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/u... File chrome/installer/util/work_item_list.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/u... chrome/installer/util/work_item_list.cc:203: const std::wstring& value_name, bool value_data) { On 2012/08/28 16:08:19, gab wrote: > string16 No longer need change this file. https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/u... chrome/installer/util/work_item_list.cc:206: (DWORD) 1, true); On 2012/08/28 16:08:19, gab wrote: > s/(DWORD) 1/static_cast<DWORD>(1) Done (in app_command.cc). https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/u... File chrome/installer/util/work_item_list.h (right): https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/u... chrome/installer/util/work_item_list.h:116: // Add an "optional" boolean registry value, specified by value_data. On 2012/08/28 19:35:40, grt wrote: > i think this function is too special-purpose to belong here. i think it's more > clear for the three callers to do the work themselves. you could add a private > helper function to app_command.cc if you'd like. Moved to app_command.cc as local helper. https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/u... chrome/installer/util/work_item_list.h:121: virtual WorkItem* AddSetOptionalBoolRegValueWorkItem( On 2012/08/28 16:08:19, gab wrote: > I believe all other methods in this file are tested. Add a test for this new > method. No longer need to test! https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/u... chrome/installer/util/work_item_list.h:122: HKEY predefined_root, const std::wstring& key_path, On 2012/08/28 16:08:19, gab wrote: > Use string16 Now I no longer need to change this file. Should I make this change anyway?
one quick comment for now. i'll review the rest once this is incorporated. https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/s... File chrome/installer/setup/install.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/s... chrome/installer/setup/install.cc:580: return installer::ON_OS_UPGRADE_SUCCESSFUL; On 2012/08/29 17:02:54, huangs1 wrote: > On 2012/08/28 19:35:39, grt wrote: > > On 2012/08/28 16:08:19, gab wrote: > > > I don't think this has any value as there is no other return statement > > possible, > > > having a void method accomplishes the same thing here. > > > > should the CreateOrUpdate... and Register... funcs return their status so that > > it can be propagated here? > > The caller HandleNonInstallCmdLineOptions() expects some exit_code to be > supplied. Since there is no generic "SUCCESS", so I created the extra flag > ON_OS_UPGRADE_SUCCESSFUL to be returned here. as mentioned in my comment in util_constants.h, InstallerStatus is a special-purpose enum used to convey information to Google Update. this new value you've added isn't passed to Google Update, so it doesn't belong. do you lose anything by having this return bool and using that to set *exit_code in HandleNonInstallCmdLineOptions?
On 2012/08/29 17:39:21, grt wrote: > one quick comment for now. i'll review the rest once this is incorporated. > > https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/s... > File chrome/installer/setup/install.cc (right): > > https://chromiumcodereview.appspot.com/10823437/diff/18001/chrome/installer/s... > chrome/installer/setup/install.cc:580: return > installer::ON_OS_UPGRADE_SUCCESSFUL; > On 2012/08/29 17:02:54, huangs1 wrote: > > On 2012/08/28 19:35:39, grt wrote: > > > On 2012/08/28 16:08:19, gab wrote: > > > > I don't think this has any value as there is no other return statement > > > possible, > > > > having a void method accomplishes the same thing here. > > > > > > should the CreateOrUpdate... and Register... funcs return their status so > that > > > it can be propagated here? > > > > The caller HandleNonInstallCmdLineOptions() expects some exit_code to be > > supplied. Since there is no generic "SUCCESS", so I created the extra flag > > ON_OS_UPGRADE_SUCCESSFUL to be returned here. > > as mentioned in my comment in util_constants.h, InstallerStatus is a > special-purpose enum used to convey information to Google Update. this new > value you've added isn't passed to Google Update, so it doesn't belong. do you > lose anything by having this return bool and using that to set *exit_code in > HandleNonInstallCmdLineOptions? I see. I'm fine with that.
Please review the updated version.
next round https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... File chrome/installer/setup/install.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... chrome/installer/setup/install.cc:560: DCHECK(chrome_install); if (!chrome_install) { NOTREACHED(); return false; } https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... chrome/installer/setup/install.cc:565: VLOG(1) << "Updating and registering shortcut for Windows 8."; "shortcut" -> "shortcuts" and remove "Windows 8" since this will be called for Win11, too. https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... File chrome/installer/setup/install.h (right): https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... chrome/installer/setup/install.h:21: class CommandLine; remove this https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... chrome/installer/setup/install.h:106: // Code that needs to executed right after OS upgrade. chromium style: "Performs installation-related tasks following an OS upgrade. Returns true if all tasks succeed." https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... File chrome/installer/setup/install_worker.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... chrome/installer/setup/install_worker.cc:325: // In Chrome Browser, add registry entries so that when an OS upgrade i think this comment is stating the obvious: the line above makes it clear that this only applies to chrome (so "In Chrome Browser..." is redundant), and "AddOsUpgradeWorkItems" is pretty self-documenting. i think the nugget that Google Update will call setup.exe would be well-placed in the doc comment for AddOsUpgradeWorkItems. https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... chrome/installer/setup/install_worker.cc:327: // which leads to ChromeBrowserOnOsUpgrade() being called. "ChromeBrowserOnOsUpgrade" -> "OnOsUpgrade" https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... chrome/installer/setup/install_worker.cc:469: const string16& brand(chrome_product_state->brand()); please revert this since ProductState::brand still returns a std::wstring. i'd rather see the methods change before the consumers rather than the other way around. https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... chrome/installer/setup/install_worker.cc:1616: .append(1, FilePath::kSeparators[0]).append(kCmdOnOsUpgrade); nit: move this last append to its own line https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... chrome/installer/setup/install_worker.cc:1628: cmd_line.AppendSwitch(installer::switches::kVerboseLogging); i think this should be added unconditionally, at least for now, so that we get useful data from the field when bugs are filed. https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... chrome/installer/setup/install_worker.cc:1635: install_list->set_log_message("Removing OS upgrade command"); i think you want to set the message for the item added on the line below, not on the list itself. https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... File chrome/installer/setup/install_worker.h (right): https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... chrome/installer/setup/install_worker.h:204: // Add work items to add or remove the "on-os-upgrade" command to |product|'s "Add" -> "Adds" https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... File chrome/installer/setup/setup_main.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... chrome/installer/setup/setup_main.cc:1174: handled = false; i think it's simpler to only set |handled| to false for the one case where the switch isn't handled rather than making it false here and true later on. so, remove this. https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... chrome/installer/setup/setup_main.cc:1175: if (cmd_line.HasSwitch(installer::switches::kChrome)) { --chrome won't be on the command-line in all cases. the right thing to do is to get the Product* for chrome via installer_state->FindProduct(BrowserDistribution::CHROME_BROWSER). if that's non-null, then call OnOsUpgrade. this will simplify OnOsUpgrade, as you won't need the FindProduct there (you can pass it in from here). https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... chrome/installer/setup/setup_main.cc:1176: if (installer::OnOsUpgrade(original_state, *installer_state, i think gab, you, and i need to discuss this in person. i don't like that this very browser-specific function has a general name. i maintain that it should have Chrome Browser in the name in some way. it only applies to Google Chrome (not Chromium), and it does things that only have meaning for the browser. if another product needs to do os upgrade work, then that work probably belongs in its own function. https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... chrome/installer/setup/setup_main.cc:1176: if (installer::OnOsUpgrade(original_state, *installer_state, this needs to be: *exit_code = !installer::OnOsUpgrade(....) so that the code is non-zero on failure. https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... chrome/installer/setup/setup_main.cc:1180: handled = true; and remove this https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... chrome/installer/setup/setup_main.cc:1181: } and change this to: } else { handled = false; } https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... File chrome/installer/setup/uninstall.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... chrome/installer/setup/uninstall.cc:132: const installer::InstallationState& machine_state, unused https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... chrome/installer/setup/uninstall.cc:139: LOG(ERROR) << "Failed to update on-os-upgrade command."; should "update" be "remove"? https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/u... File chrome/installer/util/app_command.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/u... chrome/installer/util/app_command.cc:16: // Helper: Add an optional boolean registry value, specified by value_data. The sentence beginning with "In the registry, ..." is documenting Google Update behavior, not Windows registry behavior, which is how it reads. I don't think this adds anything useful. I propose replacing this whole comment with something simple like: "Adds a work item to set |value_name| to DWORD 1 if |value_data| is true; adds a work item to remove |value_name| otherwise." https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/u... chrome/installer/util/app_command.cc:22: WorkItemList* item_list, chromium style: out params go at the end https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/u... chrome/installer/util/app_command.cc:28: return item_list->AddSetRegValueWorkItem(predefined_root, get rid of the cast by adding: const DWORD kOne = 1; before line 28 and replace "static_cast<DWORD>(1)" with "kOne" https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/u... chrome/installer/util/app_command.cc:74: // For the following calls, if result != ERROR_SUCCESS, then the I don't think this comment is needed. If you feel otherwise, please simplify it to something like: // Note: ReadValueDW only modifies its out param on success. https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/u... chrome/installer/util/app_command.cc:76: result = key.ReadValueDW(google_update::kRegSendsPingsField, &sends_pings); remove "result = " here and below https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/u... chrome/installer/util/app_command.cc:93: ->set_log_message("creating AppCommand command registry key"); "AppCommand command" -> "AppCommand" https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/u... File chrome/installer/util/app_command.h (right): https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/u... chrome/installer/util/app_command.h:28: // |command_line| is specified. The bool member variables are default to Suggestion: "Constructs a new command that will execute the given |command_line|. All other properties default to false." https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/u... File chrome/installer/util/installation_validator.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/u... chrome/installer/util/installation_validator.cc:265: expected.push_back(std::make_pair(std::string(switches::kChrome), --chrome is expected to only be present if --multi-install https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/u... chrome/installer/util/installation_validator.cc:797: ProductContext ctx(machine_state, system_level, *product_state, this change appears to make ValidateProduct harder to use rather than easier. please explain.
looks great! more comments below. https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... File chrome/installer/setup/install.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... chrome/installer/setup/install.cc:572: SHChangeNotify(SHCNE_ASSOCCHANGED, SHCNF_IDLIST, NULL, NULL); You should not need to SHChangeNotify, the shortcut update code already takes care of that (I know I suggested doing it here at top-level, as a test to see if something weird was happening, but I recall you saying that wasn't it). https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... File chrome/installer/setup/install_worker.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... chrome/installer/setup/install_worker.cc:469: const string16& brand(chrome_product_state->brand()); On 2012/08/30 04:15:25, grt wrote: > please revert this since ProductState::brand still returns a std::wstring. i'd > rather see the methods change before the consumers rather than the other way > around. Does it matter? I'd rather be consistent on a per-file basis. https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... chrome/installer/setup/install_worker.cc:1613: string16 cmd_key(product.distribution()->GetVersionKey()); Why initialize and then use append? I would prefer: const string16 cmd_key(product.distribution()->GetVersionKey() .append(...) .append(...) ....); https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... chrome/installer/setup/install_worker.cc:1620: .GetInstallerDirectory(*new_version).Append(setup_path->BaseName())); s/setup_path->BaseName()/installer::kSetupExe and then you can remove |setup_path| as an argument. https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... File chrome/installer/setup/install_worker.h (right): https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... chrome/installer/setup/install_worker.h:212: const Version* new_version, Don't pass a pointer just for the sake of being able to pass NULL. In Chromium style, passing a pointer means you need to modify the object. Make this a const& instead and pass in an empty version on uninstall (I'm pretty sure this is what other calls like AddActiveSetupWorkItems() do). https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... File chrome/installer/setup/setup_main.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... chrome/installer/setup/setup_main.cc:1176: if (installer::OnOsUpgrade(original_state, *installer_state, On 2012/08/30 04:15:25, grt wrote: > i think gab, you, and i need to discuss this in person. i don't like that this > very browser-specific function has a general name. i maintain that it should > have Chrome Browser in the name in some way. it only applies to Google Chrome > (not Chromium), and it does things that only have meaning for the browser. if > another product needs to do os upgrade work, then that work probably belongs in > its own function. How does this not apply to Chromium? I agree it only applies to browsers for now though... I don't feel strongly on naming, but we tended towards not putting "Chrome" in the function names before since it applies just as well to Chromium. I also feel that "BrowserOnOsUpgrade" sounds weird... https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... chrome/installer/setup/setup_main.cc:1181: } On 2012/08/30 04:15:25, grt wrote: > and change this to: > } else { > handled = false; > } Hmmmm, |handled| should not be touched by the handlers from what I see here. |handled| == false is this method's way to know that none of the special command-line flags were present. I don't think that us failing to handle the --on-os-upgrade flag when its present means we should keep going with a full regular install instead... https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/u... File chrome/installer/util/app_command.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/u... chrome/installer/util/app_command.cc:28: return item_list->AddSetRegValueWorkItem(predefined_root, On 2012/08/30 04:15:25, grt wrote: > get rid of the cast by adding: > const DWORD kOne = 1; > before line 28 and replace "static_cast<DWORD>(1)" with "kOne" Interesting, I've never it this way in Chromium, but I see (and have been suggested before) static_cast<DWORD>(1) all over the place. Isn't the compiler smart enough to do this at compiler time anyways? Seems more concise then a new variable... https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/u... File chrome/installer/util/installation_validator.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/u... chrome/installer/util/installation_validator.cc:10: #include <set> #include <string> since you (correctly) removed the include from the header, but std::string is still used here. https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/u... chrome/installer/util/installation_validator.cc:256: Remove this line (i.e. declare variable + initialize it belongs together). https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/u... chrome/installer/util/installation_validator.cc:262: const ProductState* product_state = Move this before "SwitchExpectations expected;" so that |expected| can be initialized in one block without interruption. https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/u... chrome/installer/util/installation_validator.cc:797: ProductContext ctx(machine_state, system_level, *product_state, On 2012/08/30 04:15:25, grt wrote: > this change appears to make ValidateProduct harder to use rather than easier. > please explain. I agree, the goal of making a struct to pass in arguments is usually to make it clear what each argument is (i.e. as the caller has to explicitly set them), but now that you made a constructor for that struct it's just as opaque and I don't see the benefit either. https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/u... File chrome/installer/util/installation_validator.h (right): https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/u... chrome/installer/util/installation_validator.h:195: const bool system_install_in, chromium-style: Don't need to make POD type parameter const. https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/u... File chrome/installer/util/util_constants.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/u... chrome/installer/util/util_constants.cc:112: // Perform updates after the OS has been upgraded. I prefer "Notify the installer that the OS has been upgraded." (i.e. whatever we decide to do when notified shouldn't be in this comment).
https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... File chrome/installer/setup/install_worker.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... chrome/installer/setup/install_worker.cc:1620: .GetInstallerDirectory(*new_version).Append(setup_path->BaseName())); On 2012/08/30 14:24:32, gab wrote: > s/setup_path->BaseName()/installer::kSetupExe > > and then you can remove |setup_path| as an argument. Please don't. BaseName() of the exe is what's used to put setup.exe into the installation, so it is the canonical name that should be used. BaseName() is the convention for this pupose. This makes it much easier if the names should ever change in the future. https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... File chrome/installer/setup/setup_main.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... chrome/installer/setup/setup_main.cc:1181: } On 2012/08/30 14:24:32, gab wrote: > On 2012/08/30 04:15:25, grt wrote: > > and change this to: > > } else { > > handled = false; > > } > > Hmmmm, |handled| should not be touched by the handlers from what I see here. > |handled| == false is this method's way to know that none of the special > command-line flags were present. I don't think that us failing to handle the > --on-os-upgrade flag when its present means we should keep going with a full > regular install instead... @gab's right. do what he says. https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/u... File chrome/installer/util/app_command.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/u... chrome/installer/util/app_command.cc:28: return item_list->AddSetRegValueWorkItem(predefined_root, On 2012/08/30 14:24:32, gab wrote: > On 2012/08/30 04:15:25, grt wrote: > > get rid of the cast by adding: > > const DWORD kOne = 1; > > before line 28 and replace "static_cast<DWORD>(1)" with "kOne" > > Interesting, I've never it this way in Chromium, but I see (and have been > suggested before) static_cast<DWORD>(1) all over the place. Isn't the compiler > smart enough to do this at compiler time anyways? Seems more concise then a new > variable... from the compiler's point of view, the two are equivalent. from a code style point of view, casts are the work of the devil and should be avoided when possible. in this case, it's possible to avoid the cast with no cost from the POV of performance/code size/etc, and with a net increase in readability.
Please take another look. https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... File chrome/installer/setup/install.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... chrome/installer/setup/install.cc:560: DCHECK(chrome_install); On 2012/08/30 04:15:25, grt wrote: > if (!chrome_install) { > NOTREACHED(); > return false; > } No longer need this, as we'll pass Product& chrome_install from caller. https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... chrome/installer/setup/install.cc:565: VLOG(1) << "Updating and registering shortcut for Windows 8."; On 2012/08/30 04:15:25, grt wrote: > "shortcut" -> "shortcuts" > and remove "Windows 8" since this will be called for Win11, too. Done. https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... chrome/installer/setup/install.cc:572: SHChangeNotify(SHCNE_ASSOCCHANGED, SHCNF_IDLIST, NULL, NULL); On 2012/08/30 14:24:32, gab wrote: > You should not need to SHChangeNotify, the shortcut update code already takes > care of that (I know I suggested doing it here at top-level, as a test to see if > something weird was happening, but I recall you saying that wasn't it). Done. https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... File chrome/installer/setup/install.h (right): https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... chrome/installer/setup/install.h:21: class CommandLine; On 2012/08/30 04:15:25, grt wrote: > remove this Done. https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... chrome/installer/setup/install.h:106: // Code that needs to executed right after OS upgrade. On 2012/08/30 04:15:25, grt wrote: > chromium style: > "Performs installation-related tasks following an OS upgrade. Returns true if > all tasks succeed." Done. Also removing unused "const InstallationState& original_state" and adding "const Product& chrome_install". https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... File chrome/installer/setup/install_worker.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... chrome/installer/setup/install_worker.cc:325: // In Chrome Browser, add registry entries so that when an OS upgrade On 2012/08/30 04:15:25, grt wrote: > i think this comment is stating the obvious: the line above makes it clear that > this only applies to chrome (so "In Chrome Browser..." is redundant), and > "AddOsUpgradeWorkItems" is pretty self-documenting. i think the nugget that > Google Update will call setup.exe would be well-placed in the doc comment for > AddOsUpgradeWorkItems. Deleted redundancy; moved comment to body of AddOsUpgradeWorkItems(). https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... chrome/installer/setup/install_worker.cc:327: // which leads to ChromeBrowserOnOsUpgrade() being called. On 2012/08/30 04:15:25, grt wrote: > "ChromeBrowserOnOsUpgrade" -> "OnOsUpgrade" Done => new name is HandleOsUpgradeForBrowser(). https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... chrome/installer/setup/install_worker.cc:1613: string16 cmd_key(product.distribution()->GetVersionKey()); On 2012/08/30 14:24:32, gab wrote: > Why initialize and then use append? > I would prefer: > const string16 cmd_key(product.distribution()->GetVersionKey() > .append(...) > .append(...) > ....); Per our discussion, not changing. https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... chrome/installer/setup/install_worker.cc:1616: .append(1, FilePath::kSeparators[0]).append(kCmdOnOsUpgrade); On 2012/08/30 04:15:25, grt wrote: > nit: move this last append to its own line Done. https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... chrome/installer/setup/install_worker.cc:1620: .GetInstallerDirectory(*new_version).Append(setup_path->BaseName())); On 2012/08/30 14:24:32, gab wrote: > s/setup_path->BaseName()/installer::kSetupExe > > and then you can remove |setup_path| as an argument. Done. https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... chrome/installer/setup/install_worker.cc:1628: cmd_line.AppendSwitch(installer::switches::kVerboseLogging); On 2012/08/30 04:15:25, grt wrote: > i think this should be added unconditionally, at least for now, so that we get > useful data from the field when bugs are filed. Done. https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... chrome/installer/setup/install_worker.cc:1635: install_list->set_log_message("Removing OS upgrade command"); On 2012/08/30 04:15:25, grt wrote: > i think you want to set the message for the item added on the line below, not on > the list itself. Done. https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... File chrome/installer/setup/install_worker.h (right): https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... chrome/installer/setup/install_worker.h:204: // Add work items to add or remove the "on-os-upgrade" command to |product|'s On 2012/08/30 04:15:25, grt wrote: > "Add" -> "Adds" Done, also fixed for three other methods. https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... chrome/installer/setup/install_worker.h:212: const Version* new_version, On 2012/08/30 14:24:32, gab wrote: > Don't pass a pointer just for the sake of being able to pass NULL. In Chromium > style, passing a pointer means you need to modify the object. > Make this a const& instead and pass in an empty version on uninstall (I'm pretty > sure this is what other calls like AddActiveSetupWorkItems() do). Not fixing this yet, per discussion. Will file cleanup bug. https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... File chrome/installer/setup/setup_main.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... chrome/installer/setup/setup_main.cc:1174: handled = false; On 2012/08/30 04:15:25, grt wrote: > i think it's simpler to only set |handled| to false for the one case where the > switch isn't handled rather than making it false here and true later on. so, > remove this. Done. https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... chrome/installer/setup/setup_main.cc:1175: if (cmd_line.HasSwitch(installer::switches::kChrome)) { On 2012/08/30 04:15:25, grt wrote: > --chrome won't be on the command-line in all cases. the right thing to do is to > get the Product* for chrome via > installer_state->FindProduct(BrowserDistribution::CHROME_BROWSER). if that's > non-null, then call OnOsUpgrade. this will simplify OnOsUpgrade, as you won't > need the FindProduct there (you can pass it in from here). Done. https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... chrome/installer/setup/setup_main.cc:1176: if (installer::OnOsUpgrade(original_state, *installer_state, On 2012/08/30 04:15:25, grt wrote: > i think gab, you, and i need to discuss this in person. i don't like that this > very browser-specific function has a general name. i maintain that it should > have Chrome Browser in the name in some way. it only applies to Google Chrome > (not Chromium), and it does things that only have meaning for the browser. if > another product needs to do os upgrade work, then that work probably belongs in > its own function. Renamed to HandleOsUpgradeForBrowser(), per discussion. https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... chrome/installer/setup/setup_main.cc:1176: if (installer::OnOsUpgrade(original_state, *installer_state, On 2012/08/30 04:15:25, grt wrote: > this needs to be: > *exit_code = !installer::OnOsUpgrade(....) > so that the code is non-zero on failure. On failure, then exit_code == 1, which will be returned to Windows! I think we should be more explicit, and set it to *exit_code = installer::UNKNOWN_STATUS; on failure. Returning 0 if successful (also hard-coded in InstallUtil::GetInstallReturnCode()). https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... chrome/installer/setup/setup_main.cc:1180: handled = true; On 2012/08/30 04:15:25, grt wrote: > and remove this Done. https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... chrome/installer/setup/setup_main.cc:1181: } On 2012/08/30 04:15:25, grt wrote: > and change this to: > } else { > handled = false; > } Done. https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... chrome/installer/setup/setup_main.cc:1181: } On 2012/08/30 14:24:32, gab wrote: > On 2012/08/30 04:15:25, grt wrote: > > and change this to: > > } else { > > handled = false; > > } > > Hmmmm, |handled| should not be touched by the handlers from what I see here. > |handled| == false is this method's way to know that none of the special > command-line flags were present. I don't think that us failing to handle the > --on-os-upgrade flag when its present means we should keep going with a full > regular install instead... Ah. Replacing this with *exit_code = installer::UNKNOWN_STATUS; like elsewhere in the code. https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... File chrome/installer/setup/uninstall.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... chrome/installer/setup/uninstall.cc:132: const installer::InstallationState& machine_state, On 2012/08/30 04:15:25, grt wrote: > unused Done. https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... chrome/installer/setup/uninstall.cc:139: LOG(ERROR) << "Failed to update on-os-upgrade command."; On 2012/08/30 04:15:25, grt wrote: > should "update" be "remove"? Done. https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/u... File chrome/installer/util/app_command.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/u... chrome/installer/util/app_command.cc:16: // Helper: Add an optional boolean registry value, specified by value_data. On 2012/08/30 04:15:25, grt wrote: > The sentence beginning with "In the registry, ..." is documenting Google Update > behavior, not Windows registry behavior, which is how it reads. I don't think > this adds anything useful. I propose replacing this whole comment with > something simple like: > "Adds a work item to set |value_name| to DWORD 1 if |value_data| is true; adds a > work item to remove |value_name| otherwise." Done. https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/u... chrome/installer/util/app_command.cc:22: WorkItemList* item_list, On 2012/08/30 04:15:25, grt wrote: > chromium style: out params go at the end Done. https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/u... chrome/installer/util/app_command.cc:28: return item_list->AddSetRegValueWorkItem(predefined_root, On 2012/08/30 04:15:25, grt wrote: > get rid of the cast by adding: > const DWORD kOne = 1; > before line 28 and replace "static_cast<DWORD>(1)" with "kOne" Done. https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/u... chrome/installer/util/app_command.cc:28: return item_list->AddSetRegValueWorkItem(predefined_root, On 2012/08/30 14:24:32, gab wrote: > On 2012/08/30 04:15:25, grt wrote: > > get rid of the cast by adding: > > const DWORD kOne = 1; > > before line 28 and replace "static_cast<DWORD>(1)" with "kOne" > > Interesting, I've never it this way in Chromium, but I see (and have been > suggested before) static_cast<DWORD>(1) all over the place. Isn't the compiler > smart enough to do this at compiler time anyways? Seems more concise then a new > variable... Conflicting reviews... Going with my personal preference of with "static_cast<DWORD>(1)", for shorter code. https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/u... chrome/installer/util/app_command.cc:74: // For the following calls, if result != ERROR_SUCCESS, then the On 2012/08/30 04:15:25, grt wrote: > I don't think this comment is needed. If you feel otherwise, please simplify it > to something like: > // Note: ReadValueDW only modifies its out param on success. Done. So refer to functions by "ReadValueDW", not "ReadValueDW()"! https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/u... chrome/installer/util/app_command.cc:76: result = key.ReadValueDW(google_update::kRegSendsPingsField, &sends_pings); On 2012/08/30 04:15:25, grt wrote: > remove "result = " here and below Done (embarrassed). https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/u... chrome/installer/util/app_command.cc:93: ->set_log_message("creating AppCommand command registry key"); On 2012/08/30 04:15:25, grt wrote: > "AppCommand command" -> "AppCommand" Done. https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/u... File chrome/installer/util/app_command.h (right): https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/u... chrome/installer/util/app_command.h:28: // |command_line| is specified. The bool member variables are default to On 2012/08/30 04:15:25, grt wrote: > Suggestion: > > "Constructs a new command that will execute the given |command_line|. All other > properties default to false." Done. https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/u... File chrome/installer/util/installation_validator.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/u... chrome/installer/util/installation_validator.cc:10: #include <set> On 2012/08/30 14:24:32, gab wrote: > #include <string> > since you (correctly) removed the include from the header, but std::string is > still used here. Done. https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/u... chrome/installer/util/installation_validator.cc:256: On 2012/08/30 14:24:32, gab wrote: > Remove this line (i.e. declare variable + initialize it belongs together). Done. https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/u... chrome/installer/util/installation_validator.cc:262: const ProductState* product_state = On 2012/08/30 14:24:32, gab wrote: > Move this before "SwitchExpectations expected;" so that |expected| can be > initialized in one block without interruption. Made unnecessary by new change. https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/u... chrome/installer/util/installation_validator.cc:265: expected.push_back(std::make_pair(std::string(switches::kChrome), On 2012/08/30 04:15:25, grt wrote: > --chrome is expected to only be present if --multi-install Done. And also "if", so "if and only if" => Checking identical conditions. https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/u... chrome/installer/util/installation_validator.cc:797: ProductContext ctx(machine_state, system_level, *product_state, On 2012/08/30 14:24:32, gab wrote: > On 2012/08/30 04:15:25, grt wrote: > > this change appears to make ValidateProduct harder to use rather than easier. > > please explain. > > I agree, the goal of making a struct to pass in arguments is usually to make it > clear what each argument is (i.e. as the caller has to explicitly set them), but > now that you made a constructor for that struct it's just as opaque and I don't > see the benefit either. Done. https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/u... File chrome/installer/util/installation_validator.h (right): https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/u... chrome/installer/util/installation_validator.h:195: const bool system_install_in, On 2012/08/30 14:24:32, gab wrote: > chromium-style: Don't need to make POD type parameter const. Done. https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/u... File chrome/installer/util/util_constants.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/u... chrome/installer/util/util_constants.cc:112: // Perform updates after the OS has been upgraded. On 2012/08/30 14:24:32, gab wrote: > I prefer "Notify the installer that the OS has been upgraded." (i.e. whatever we > decide to do when notified shouldn't be in this comment). Done.
Just expressing opinion, no changes required! https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... File chrome/installer/setup/install_worker.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... chrome/installer/setup/install_worker.cc:1620: .GetInstallerDirectory(*new_version).Append(setup_path->BaseName())); On 2012/08/30 17:06:08, grt wrote: > On 2012/08/30 14:24:32, gab wrote: > > s/setup_path->BaseName()/installer::kSetupExe > > > > and then you can remove |setup_path| as an argument. > > Please don't. BaseName() of the exe is what's used to put setup.exe into the > installation, so it is the canonical name that should be used. BaseName() is > the convention for this pupose. This makes it much easier if the names should > ever change in the future. Changing the name in the future would be the craziest thing on earth imo, but ok :)! https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/u... File chrome/installer/util/app_command.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/u... chrome/installer/util/app_command.cc:28: return item_list->AddSetRegValueWorkItem(predefined_root, On 2012/08/30 17:06:08, grt wrote: > On 2012/08/30 14:24:32, gab wrote: > > On 2012/08/30 04:15:25, grt wrote: > > > get rid of the cast by adding: > > > const DWORD kOne = 1; > > > before line 28 and replace "static_cast<DWORD>(1)" with "kOne" > > > > Interesting, I've never it this way in Chromium, but I see (and have been > > suggested before) static_cast<DWORD>(1) all over the place. Isn't the compiler > > smart enough to do this at compiler time anyways? Seems more concise then a > new > > variable... > > from the compiler's point of view, the two are equivalent. from a code style > point of view, casts are the work of the devil and should be avoided when > possible. in this case, it's possible to avoid the cast with no cost from the > POV of performance/code size/etc, and with a net increase in readability. I disagree that this is more readbable, when one sees "kOne" he has to read up to see what "kOne" is instead of seeing it inline... I don't see what's wrong with a cast, to me, in this case, it's just a way of initializing a variable to particular POD time inline. But let's not make a second round of reviews just for this.
almost there! Cheers! Gab https://chromiumcodereview.appspot.com/10823437/diff/31004/chrome/installer/s... File chrome/installer/setup/install.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/31004/chrome/installer/s... chrome/installer/setup/install.cc:555: bool HandleOsUpgradeForBrowser(const InstallerState& installer_state, I thought I'd mentioned this before (maybe it argued against and I missed it?): You only return true from here so the return value is rather useless. You can either: 1) make the shortcuts function return a bool and then return true if they all passed (they already have failure logs they just don't return their success as it is now). 2) simply return void. I prefer (1). https://chromiumcodereview.appspot.com/10823437/diff/31004/chrome/installer/s... File chrome/installer/setup/install_worker.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/31004/chrome/installer/s... chrome/installer/setup/install_worker.cc:1619: .Append(installer::kSetupExe)); Greg argued against this (preferring your original BaseName). I don't feel strongly either way. https://chromiumcodereview.appspot.com/10823437/diff/31004/chrome/installer/s... File chrome/installer/setup/setup_main.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/31004/chrome/installer/s... chrome/installer/setup/setup_main.cc:1177: if (installer::HandleOsUpgradeForBrowser(*installer_state, Instead of using a double if with the same else block, use if (x && y) {} else {} https://chromiumcodereview.appspot.com/10823437/diff/31004/chrome/installer/u... File chrome/installer/util/shell_util.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/31004/chrome/installer/u... chrome/installer/util/shell_util.cc:1460: return AddRegistryEntries(root, progid_and_appreg_entries) && Do not check this in!!
almost there! https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... File chrome/installer/setup/install_worker.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/s... chrome/installer/setup/install_worker.cc:469: const string16& brand(chrome_product_state->brand()); On 2012/08/30 14:24:32, gab wrote: > On 2012/08/30 04:15:25, grt wrote: > > please revert this since ProductState::brand still returns a std::wstring. > i'd > > rather see the methods change before the consumers rather than the other way > > around. > > Does it matter? I'd rather be consistent on a per-file basis. My preferred way of doing the wstring -> string16 migration is to do it horizontally on a per-API or per-class basis rather than file by file. In the former case, producers and consumers are consistent. In the latter case, you end up with a given "thing" being a std::wstring in one place and a string16 in another, which I feel decreases readability. https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/u... File chrome/installer/util/app_command.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/30001/chrome/installer/u... chrome/installer/util/app_command.cc:28: return item_list->AddSetRegValueWorkItem(predefined_root, On 2012/08/30 17:15:50, gab wrote: > On 2012/08/30 17:06:08, grt wrote: > > On 2012/08/30 14:24:32, gab wrote: > > > On 2012/08/30 04:15:25, grt wrote: > > > > get rid of the cast by adding: > > > > const DWORD kOne = 1; > > > > before line 28 and replace "static_cast<DWORD>(1)" with "kOne" > > > > > > Interesting, I've never it this way in Chromium, but I see (and have been > > > suggested before) static_cast<DWORD>(1) all over the place. Isn't the > compiler > > > smart enough to do this at compiler time anyways? Seems more concise then a > > new > > > variable... > > > > from the compiler's point of view, the two are equivalent. from a code style > > point of view, casts are the work of the devil and should be avoided when > > possible. in this case, it's possible to avoid the cast with no cost from the > > POV of performance/code size/etc, and with a net increase in readability. > > I disagree that this is more readbable, when one sees "kOne" he has to read up > to see what "kOne" is instead of seeing it inline... I don't see what's wrong > with a cast, to me, in this case, it's just a way of initializing a variable to > particular POD time inline. i disagree. in my religious text, good code has no casts. https://chromiumcodereview.appspot.com/10823437/diff/31004/chrome/installer/s... File chrome/installer/setup/install.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/31004/chrome/installer/s... chrome/installer/setup/install.cc:556: const Product& chrome_install, chrome_install -> chrome https://chromiumcodereview.appspot.com/10823437/diff/31004/chrome/installer/s... chrome/installer/setup/install.cc:558: // Upon upgrading to Windows 8, we need to fix Chrome shortcuts and register DCHECK(chrome.is_chrome()); before this line https://chromiumcodereview.appspot.com/10823437/diff/31004/chrome/installer/s... File chrome/installer/setup/install_worker.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/31004/chrome/installer/s... chrome/installer/setup/install_worker.cc:324: if (p.is_chrome()) { remove braces https://chromiumcodereview.appspot.com/10823437/diff/31004/chrome/installer/s... chrome/installer/setup/install_worker.cc:1619: .Append(installer::kSetupExe)); On 2012/08/30 17:55:19, gab wrote: > Greg argued against this (preferring your original BaseName). > > I don't feel strongly either way. I do. :-) If you want setup to run on OS upgrade, then you need to get the name of it right. By using the same name chosen when the file is put into place (see AddInstallerCopyTasks), you have a greater chance of this actually working. Also, I feel strongly that the code should be consistent. The other places in the code that create a command line to setup.exe use BaseName in this way. (all but the one I didn't review -- please fix that one, gab!) https://chromiumcodereview.appspot.com/10823437/diff/31004/chrome/installer/s... File chrome/installer/setup/setup_main.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/31004/chrome/installer/s... chrome/installer/setup/setup_main.cc:1182: *exit_code = installer::UNKNOWN_STATUS; aaah. now i see why you wanted to add a new InstallStatus. if you choose to keep HandleOsUpgradeForBrowser returning bool, please hit me with a wet noodle and re-add the InstallStatus. https://chromiumcodereview.appspot.com/10823437/diff/31004/chrome/installer/u... File chrome/installer/util/shell_util.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/31004/chrome/installer/u... chrome/installer/util/shell_util.cc:1460: return AddRegistryEntries(root, progid_and_appreg_entries) && On 2012/08/30 17:55:19, gab wrote: > Do not check this in!! Good catch! +1 to that.
Please check again! https://chromiumcodereview.appspot.com/10823437/diff/31004/chrome/installer/s... File chrome/installer/setup/install.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/31004/chrome/installer/s... chrome/installer/setup/install.cc:555: bool HandleOsUpgradeForBrowser(const InstallerState& installer_state, On 2012/08/30 17:55:19, gab wrote: > I thought I'd mentioned this before (maybe it argued against and I missed it?): > > You only return true from here so the return value is rather useless. > > You can either: > > 1) make the shortcuts function return a bool and then return true if they all > passed (they already have failure logs they just don't return their success as > it is now). > > 2) simply return void. > > I prefer (1). Done. I chased after error messages. Avoided changing control flow (so store in "is_successful" variable instead of early "return false;"). Also, other callers of the Shortcut routines ignore the return values. https://chromiumcodereview.appspot.com/10823437/diff/31004/chrome/installer/s... chrome/installer/setup/install.cc:556: const Product& chrome_install, On 2012/08/30 19:10:18, grt wrote: > chrome_install -> chrome Done. https://chromiumcodereview.appspot.com/10823437/diff/31004/chrome/installer/s... chrome/installer/setup/install.cc:558: // Upon upgrading to Windows 8, we need to fix Chrome shortcuts and register On 2012/08/30 19:10:18, grt wrote: > DCHECK(chrome.is_chrome()); before this line Done. https://chromiumcodereview.appspot.com/10823437/diff/31004/chrome/installer/s... File chrome/installer/setup/install_worker.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/31004/chrome/installer/s... chrome/installer/setup/install_worker.cc:324: if (p.is_chrome()) { On 2012/08/30 19:10:18, grt wrote: > remove braces No change; we have to pass &setup_path again => wrapping => need braces again. https://chromiumcodereview.appspot.com/10823437/diff/31004/chrome/installer/s... chrome/installer/setup/install_worker.cc:1619: .Append(installer::kSetupExe)); On 2012/08/30 19:10:18, grt wrote: > On 2012/08/30 17:55:19, gab wrote: > > Greg argued against this (preferring your original BaseName). > > > > I don't feel strongly either way. > > I do. :-) If you want setup to run on OS upgrade, then you need to get the > name of it right. By using the same name chosen when the file is put into place > (see AddInstallerCopyTasks), you have a greater chance of this actually working. > Also, I feel strongly that the code should be consistent. The other places in > the code that create a command line to setup.exe use BaseName in this way. (all > but the one I didn't review -- please fix that one, gab!) Okay, no-op. https://chromiumcodereview.appspot.com/10823437/diff/31004/chrome/installer/s... File chrome/installer/setup/setup_main.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/31004/chrome/installer/s... chrome/installer/setup/setup_main.cc:1177: if (installer::HandleOsUpgradeForBrowser(*installer_state, On 2012/08/30 17:55:19, gab wrote: > Instead of using a double if with the same else block, use if (x && y) {} else > {} Done. https://chromiumcodereview.appspot.com/10823437/diff/31004/chrome/installer/s... chrome/installer/setup/setup_main.cc:1182: *exit_code = installer::UNKNOWN_STATUS; On 2012/08/30 19:10:18, grt wrote: > aaah. now i see why you wanted to add a new InstallStatus. if you choose to > keep HandleOsUpgradeForBrowser returning bool, please hit me with a wet noodle > and re-add the InstallStatus. Discussed with gab. Now adding installer::HANDLE_OS_UPGRADE_FAILED flag, and on success, will return installer::INSTALL_REPAIRED (this imitates the behaviour of ChromeFrameReadyModeEndTempOptOut(). https://chromiumcodereview.appspot.com/10823437/diff/31004/chrome/installer/u... File chrome/installer/util/shell_util.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/31004/chrome/installer/u... chrome/installer/util/shell_util.cc:1460: return AddRegistryEntries(root, progid_and_appreg_entries) && On 2012/08/30 17:55:19, gab wrote: > Do not check this in!! Doh! Forgot about this.
Looking great, couple nits, and one thing that just won't work (i.e. the way you handle the exit code now returns non-zero on success...). https://chromiumcodereview.appspot.com/10823437/diff/40004/chrome/installer/s... File chrome/installer/setup/install.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/40004/chrome/installer/s... chrome/installer/setup/install.cc:264: bool is_successful = true; nit: We try to avoid is_* in chromium. Please rename to "bool success" (if you codesearch for one vs the other in quotes and you'll see a lot more instances of "bool success"). https://chromiumcodereview.appspot.com/10823437/diff/40004/chrome/installer/s... chrome/installer/setup/install.cc:351: bool is_successful = true; nit: same here https://chromiumcodereview.appspot.com/10823437/diff/40004/chrome/installer/s... chrome/installer/setup/install.cc:568: bool is_successful = true; nit: and here https://chromiumcodereview.appspot.com/10823437/diff/40004/chrome/installer/s... chrome/installer/setup/install.cc:578: if (!CreateOrUpdateStartMenuAndTaskbarShortcuts( Unify the ifs, i.e. if (!1 || !2 || !3) {} https://chromiumcodereview.appspot.com/10823437/diff/40004/chrome/installer/s... chrome/installer/setup/install.cc:582: RegisterChromeOnMachine(installer_state, chrome, false); Add this call to the list of possible failures too. https://chromiumcodereview.appspot.com/10823437/diff/40004/chrome/installer/s... File chrome/installer/setup/install_worker.h (right): https://chromiumcodereview.appspot.com/10823437/diff/40004/chrome/installer/s... chrome/installer/setup/install_worker.h:210: const FilePath* setup_path, & not * (arg see contradicting comment in uninstall.cc) https://chromiumcodereview.appspot.com/10823437/diff/40004/chrome/installer/s... File chrome/installer/setup/setup_main.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/40004/chrome/installer/s... chrome/installer/setup/setup_main.cc:1180: *exit_code = installer::INSTALL_REPAIRED; // Success. Use the style in this file, i.e. have HandleOsUpgradeForBrowser return an InstallStatus and then use InstallUtil::GetInstallReturnCode() to convert that to the return code. https://chromiumcodereview.appspot.com/10823437/diff/40004/chrome/installer/s... File chrome/installer/setup/uninstall.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/40004/chrome/installer/s... chrome/installer/setup/uninstall.cc:135: AddOsUpgradeWorkItems(installer_state, NULL, NULL, product, Arg... I hate this... ok sure use a pointer... but this needs to get better in your future refactoring CL.
Please check again! https://chromiumcodereview.appspot.com/10823437/diff/40004/chrome/installer/s... File chrome/installer/setup/install.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/40004/chrome/installer/s... chrome/installer/setup/install.cc:264: bool is_successful = true; On 2012/08/30 20:26:27, gab wrote: > nit: We try to avoid is_* in chromium. Please rename to "bool success" (if you > codesearch for one vs the other in quotes and you'll see a lot more instances of > "bool success"). Done. https://chromiumcodereview.appspot.com/10823437/diff/40004/chrome/installer/s... chrome/installer/setup/install.cc:351: bool is_successful = true; On 2012/08/30 20:26:28, gab wrote: > nit: same here Done. https://chromiumcodereview.appspot.com/10823437/diff/40004/chrome/installer/s... chrome/installer/setup/install.cc:568: bool is_successful = true; On 2012/08/30 20:26:28, gab wrote: > nit: and here Done. https://chromiumcodereview.appspot.com/10823437/diff/40004/chrome/installer/s... chrome/installer/setup/install.cc:578: if (!CreateOrUpdateStartMenuAndTaskbarShortcuts( On 2012/08/30 20:26:28, gab wrote: > Unify the ifs, i.e. if (!1 || !2 || !3) {} Discussed: Doing success = fun() && success; instead. https://chromiumcodereview.appspot.com/10823437/diff/40004/chrome/installer/s... File chrome/installer/setup/install_worker.h (right): https://chromiumcodereview.appspot.com/10823437/diff/40004/chrome/installer/s... chrome/installer/setup/install_worker.h:210: const FilePath* setup_path, On 2012/08/30 20:26:28, gab wrote: > & not * (arg see contradicting comment in uninstall.cc) Okay, no-op. https://chromiumcodereview.appspot.com/10823437/diff/40004/chrome/installer/s... File chrome/installer/setup/setup_main.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/40004/chrome/installer/s... chrome/installer/setup/setup_main.cc:1180: *exit_code = installer::INSTALL_REPAIRED; // Success. On 2012/08/30 20:26:28, gab wrote: > Use the style in this file, i.e. have HandleOsUpgradeForBrowser return an > InstallStatus and then use InstallUtil::GetInstallReturnCode() to convert that > to the return code. As discussed, doing some other way. https://chromiumcodereview.appspot.com/10823437/diff/40004/chrome/installer/s... File chrome/installer/setup/uninstall.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/40004/chrome/installer/s... chrome/installer/setup/uninstall.cc:135: AddOsUpgradeWorkItems(installer_state, NULL, NULL, product, On 2012/08/30 20:26:28, gab wrote: > Arg... I hate this... ok sure use a pointer... but this needs to get better in > your future refactoring CL. Acknowledged.
LGTM++ /w nit I know you're gone and this is just a style nit. If greg's comments don't involve logic changes we can probably commit this as is and you can make a follow up style CL when you return. Cheers! Gab https://chromiumcodereview.appspot.com/10823437/diff/38005/chrome/installer/s... File chrome/installer/setup/install.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/38005/chrome/installer/s... chrome/installer/setup/install.cc:580: && success; nit: && should be on the line above and the 2nd condition below alternatively you can do: success = RegisterChromeOnMachine(installer_state, chrome, false) && success; if that fits.
One more comment (which makes this not commitable as is imo :(...) https://chromiumcodereview.appspot.com/10823437/diff/38005/chrome/installer/s... File chrome/installer/setup/install.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/38005/chrome/installer/s... chrome/installer/setup/install.cc:423: return true; A bunch of things can fail above, simply returning true here is not enough...
Doh! So we check the return values of all the ShellUtil:: calls?
On 2012/08/30 21:30:44, huangs1 wrote: > Doh! So we check the return values of all the ShellUtil:: calls? Well in any one situation only one of them happens, but ya, the return value should be the success of the one we tried to make happen.
@gab: please have a look at my comment regarding returning bool from the shortcut funcs and let me know if i'm mistaken about the return code. i think the safe thing to do is to keep them as void for now, and fix them later after your shortcut refactor. https://chromiumcodereview.appspot.com/10823437/diff/38005/chrome/installer/s... File chrome/installer/setup/install.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/38005/chrome/installer/s... chrome/installer/setup/install.cc:307: success = false; UpdateChromeShortcut will return false in certain success cases. i think we need to back this change out. it's safer to go with gab's less-prefered option of making the new os upgrade fn return void for now. perhaps it'll make sense to propagate errors correctly once gab refactors shortcut creation so that it's more sensible.
Ah right, let's go with the void solution for now. Actually, now that I think about it, I wanted the status return mostly for logging (i.e. it's the only purpose since we won't retry anyways on failure)... but the calls themselves have logging in them so we would still get a decent log (i.e. if we get user reports from people who updated but didn't get Metro Chrome we can see that the update failed...). I think we can commit this if I prepare an immediate follow-up CL to remove the return status (I want Sam to get credit for his work from this big/first commit of his so I'd rather not just patch it in and check it in myself). What you say sir? LGTY? https://chromiumcodereview.appspot.com/10823437/diff/38005/chrome/installer/s... File chrome/installer/setup/install.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/38005/chrome/installer/s... chrome/installer/setup/install.cc:307: success = false; On 2012/08/31 00:41:36, grt wrote: > UpdateChromeShortcut will return false in certain success cases. i think we > need to back this change out. it's safer to go with gab's less-prefered option > of making the new os upgrade fn return void for now. perhaps it'll make sense > to propagate errors correctly once gab refactors shortcut creation so that it's > more sensible. Ah yes indeed, UpdateChromeShortcut() currently fails if the shortcut has been deleted by the user and this shouldn't be considered an install/upgrade failure over all.
https://chromiumcodereview.appspot.com/10823437/diff/38005/chrome/installer/s... File chrome/installer/setup/install.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/38005/chrome/installer/s... chrome/installer/setup/install.cc:423: return true; On 2012/08/30 21:12:03, gab wrote: > A bunch of things can fail above, simply returning true here is not enough... We never cared about failure here before. This change is a no-op in practice.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/huangs@chromium.org/10823437/38005
Try job failure for 10823437-38005 (retry) on win_rel for step "installer_util_unittests". It's a second try, previously, step "installer_util_unittests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
trybot failed unit test because I didn't supply new mock data. That should be fixed. Please take another look! https://chromiumcodereview.appspot.com/10823437/diff/38005/chrome/installer/s... File chrome/installer/setup/install.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/38005/chrome/installer/s... chrome/installer/setup/install.cc:307: success = false; On 2012/08/31 01:31:06, gab wrote: > On 2012/08/31 00:41:36, grt wrote: > > UpdateChromeShortcut will return false in certain success cases. i think we > > need to back this change out. it's safer to go with gab's less-prefered > option > > of making the new os upgrade fn return void for now. perhaps it'll make sense > > to propagate errors correctly once gab refactors shortcut creation so that > it's > > more sensible. > > Ah yes indeed, UpdateChromeShortcut() currently fails if the shortcut has been > deleted by the user and this shouldn't be considered an install/upgrade failure > over all. Removing all these, per discussion. https://chromiumcodereview.appspot.com/10823437/diff/38005/chrome/installer/s... chrome/installer/setup/install.cc:423: return true; On 2012/08/31 02:24:17, robertshield wrote: > On 2012/08/30 21:12:03, gab wrote: > > A bunch of things can fail above, simply returning true here is not enough... > > We never cared about failure here before. This change is a no-op in practice. Done. https://chromiumcodereview.appspot.com/10823437/diff/38005/chrome/installer/s... chrome/installer/setup/install.cc:580: && success; On 2012/08/30 20:51:07, gab wrote: > nit: && should be on the line above and the 2nd condition below > alternatively you can do: > success = > RegisterChromeOnMachine(installer_state, chrome, false) && success; > if that fits. Okay, but removing this. https://chromiumcodereview.appspot.com/10823437/diff/38005/chrome/installer/s... File chrome/installer/setup/setup_main.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/38005/chrome/installer/s... chrome/installer/setup/setup_main.cc:1183: *exit_code = InstallUtil::GetInstallReturnCode(status); Per discussion, this will always be successful. https://chromiumcodereview.appspot.com/10823437/diff/38005/chrome/installer/u... File chrome/installer/util/installation_validator_unittest.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/38005/chrome/installer/u... chrome/installer/util/installation_validator_unittest.cc:88: int channel_modifiers); Need to declare void AddOsUpgradeCommand(...); here. https://chromiumcodereview.appspot.com/10823437/diff/38005/chrome/installer/u... chrome/installer/util/installation_validator_unittest.cc:238: } Need to implement void FakeProductState::AddOsUpgradeCommand(...){} here. https://chromiumcodereview.appspot.com/10823437/diff/38005/chrome/installer/u... chrome/installer/util/installation_validator_unittest.cc:436: } Need to call state->AddOsUpgradeCommand(...) here, if CHROME_BROWSER.
lgtm on the condition that you either land this as-is and address these two comments in a followup CL, or make these two changes (and no others) before landing. https://chromiumcodereview.appspot.com/10823437/diff/38024/chrome/installer/s... File chrome/installer/setup/install_worker.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/38024/chrome/installer/s... chrome/installer/setup/install_worker.cc:1631: install_list->set_log_message("Adding OS upgrade command"); delete this line. you should not set a message on the list itself. https://chromiumcodereview.appspot.com/10823437/diff/38024/chrome/installer/u... File chrome/installer/util/google_update_constants.h (right): https://chromiumcodereview.appspot.com/10823437/diff/38024/chrome/installer/u... chrome/installer/util/google_update_constants.h:33: extern const wchar_t kRegAutoRunOnOSUpgrade[]; this should have the "Field" suffix like the other constants here
lgtm, +1 to greg's comment.
Made exactly the changes requested. Going to submit soon! https://chromiumcodereview.appspot.com/10823437/diff/38024/chrome/installer/s... File chrome/installer/setup/install_worker.cc (right): https://chromiumcodereview.appspot.com/10823437/diff/38024/chrome/installer/s... chrome/installer/setup/install_worker.cc:1631: install_list->set_log_message("Adding OS upgrade command"); On 2012/09/01 01:30:24, grt wrote: > delete this line. you should not set a message on the list itself. Done. https://chromiumcodereview.appspot.com/10823437/diff/38024/chrome/installer/u... File chrome/installer/util/google_update_constants.h (right): https://chromiumcodereview.appspot.com/10823437/diff/38024/chrome/installer/u... chrome/installer/util/google_update_constants.h:33: extern const wchar_t kRegAutoRunOnOSUpgrade[]; On 2012/09/01 01:30:24, grt wrote: > this should have the "Field" suffix like the other constants here Done, 4 changes in 3 files.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/huangs@chromium.org/10823437/42006
Change committed as 154630 |
