Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2589753002:
Remove multi-install from chrome/installer/setup. (Closed)
|
Created:
4 years ago by grt (UTC plus 2) Modified:
3 years, 11 months ago CC:
chromium-reviews, grt+watch_chromium.org, pennymac+watch_chromium.org, asvitkine+watch_chromium.org, wfh+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove multi-install from chrome/installer/setup.
This change strips InstallerState of all multi-install knowledge and
trickles the change through the rest of the installer. An
InstallerState now has exactly one Product on which it operates. There
is substantial room for further improvement, which will come after
InstallDetails is woven into setup.
Various "delete old stuff" operations are also coalesced into a single
post-install/update step rather than sprinkled around Chrome's install
path.
While at it, this change also removes some old Chrome Frame code that
is no longer used (IE low rights elevation policy, GCF in use, COM
DLLs) and attempts some IWYU fixes.
BUG=577816
Review-Url: https://codereview.chromium.org/2589753002
Cr-Commit-Position: refs/heads/master@{#442077}
Committed: https://chromium.googlesource.com/chromium/src/+/7bef10cf4c05108c03cf055c75695bbe5737bb56
Patch Set 1 #Patch Set 2 : sync to position 439802 #Patch Set 3 : remove is_chrome #Patch Set 4 : revert util/product changes for now #
Total comments: 48
Patch Set 5 : gab comments #Patch Set 6 : lint fixes #
Total comments: 28
Patch Set 7 : huangs comments #Patch Set 8 : sync to position 441108 #
Total comments: 4
Patch Set 9 : jwd comments #
Total comments: 6
Patch Set 10 : robertshield comments #
Total comments: 2
Patch Set 11 : gab and robertshield comments #
Total comments: 24
Patch Set 12 : huangs comments #Patch Set 13 : sync to position 442053 #Messages
Total messages: 83 (62 generated)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by grt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
The CQ bit was checked by grt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
Patchset #1 (id:60001) has been deleted
Description was changed from ========== Remove multi-install from chrome/installer/setup. More cleanups are still possible. BUG=577816 ========== to ========== Remove multi-install from chrome/installer/setup. This change strips InstallerState of all multi-install knowledge and trickles the change through the rest of the installer. An InstallerState now has exactly one Product on which it operates. There is substantial room for further improvement, which will come after InstallDetails is woven into setup. Various "delete old stuff" operations are also coalesced into a single post-install/update step rather than sprinkled around Chrome's install path. While at it, this change also removes some old Chrome Frame code that is no longer used (IE low rights elevation policy, GCF in use, COM DLLs) and attempts some IWYU fixes. BUG=577816 ==========
grt@chromium.org changed reviewers: + gab@chromium.org, robertshield@chromium.org
The CQ bit was checked by grt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Happy holidays!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by grt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by grt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by grt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Woah :-O!! Mostly questions; feel free to ignore requests to split CF/AL to precursor CLs, in retrospect it's fine to bundle (i.e. more code to read in a single CL but doesn't make logic harder to understand). https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... File chrome/installer/setup/app_launcher_installer.h (left): https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... chrome/installer/setup/app_launcher_installer.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. App Launcher cleanup in precursor CL? https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... File chrome/installer/setup/install.cc (left): https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... chrome/installer/setup/install.cc:228: installer::RefreshElevationPolicy(); This is Chrome Frame stuff, separate CL? https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... chrome/installer/setup/install.cc:445: DCHECK(product.is_chrome()); I don't see a change to product.h in this CL? Leave this for a follow-up? Ditto for all DCHECK(chrome.is_chrome()); strips in this CL https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... chrome/installer/setup/install.cc:552: if (chrome_product) { Nice, we had two if (chrome_product) {} blocks in a row :-O! https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... chrome/installer/setup/install.cc:509: const Product& chrome_product = installer_state.product(); I presume InstallerState and Product will eventually (in a follow-up) become the same thing if we have only one Product? https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... File chrome/installer/setup/install_worker.cc (left): https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... chrome/installer/setup/install_worker.cc:92: installer_state.AddComDllList(&com_dll_list); Also CF specific. https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... chrome/installer/setup/install_worker.cc:1172: KEY_WOW64_32KEY); Don't we still need this cleanup step? https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... File chrome/installer/setup/install_worker.cc (right): https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... chrome/installer/setup/install_worker.cc:615: base::string16 version_key = dist->GetVersionKey(); const https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... chrome/installer/setup/install_worker.cc:634: base::CommandLine product_rename_cmd(rename); Add a comment on what this does? It's unclear even from ProductOperations::AppendRenameFlags()'s description. https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... chrome/installer/setup/install_worker.cc:721: AddChromeWorkItems(original_state, installer_state, setup_path, archive_path, Assuming we'll rename/shuffle this as well in a follow-up? https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... File chrome/installer/setup/install_worker_unittest.cc (left): https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... chrome/installer/setup/install_worker_unittest.cc:629: TEST_F(InstallWorkerTest, AddUsageStatsWorkItems) { This is still used (though moved to anonymous), is it still tested? https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... File chrome/installer/setup/install_worker_unittest.cc (right): https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... chrome/installer/setup/install_worker_unittest.cc:255: if (chrome != NULL) { if (chrome) https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... File chrome/installer/setup/installer_state.cc (right): https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... chrome/installer/setup/installer_state.cc:77: // For a single-install, the current browser dist is the operand. Comment no longer relevant given we're always in single-install? https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... chrome/installer/setup/installer_state.cc:78: BrowserDistribution* operand = BrowserDistribution::GetDistribution(); Along similar thoughts, s/operand/dist ? https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... chrome/installer/setup/installer_state.cc:116: base::FilePath InstallerState::GetDefaultProductInstallPath( This is now so simple it should probably be inlined? https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... chrome/installer/setup/installer_state.cc:126: bool InstallerState::CanAddProduct(const Product& product, Now that the only product is chrome, will this go away in a follow-up CL? https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... chrome/installer/setup/installer_state.cc:141: std::unique_ptr<Product>* product) { If |product| can be moved out, the caller can already not re-use it so passing by std::unique_ptr<Product> would make more sense than std::unique_ptr<Product>* (from a quick code search seems this would be work and be cleaner). (follow-up CL?) https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... File chrome/installer/setup/setup_util.cc (right): https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... chrome/installer/setup/setup_util.cc:190: value.find(L"\\Chrome Frame\\") != base::string16::npos) { Single Chrome Frame still exists? Frozen to some old version I guess? Was it migrated off of multi back then (i.e. this is likely the vast majority of CFs left and we're fine killing remaining multi CF?) https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... chrome/installer/setup/setup_util.cc:225: InstallUtil::DeleteRegistryKey(installer_state.root_key(), path, I'm not 100% sure about the destruction order here. Is the RegKey guaranteed to be deleted (and thus closed) before InstallUtil::DeleteRegistryKey() or only before the end of the whole conditional? https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... chrome/installer/setup/setup_util.cc:255: UMA_HISTOGRAM_BOOLEAN("Setup.Install.InstallExtensionCommandRemoved", true); Is UMA live until the very end of setup.exe? (DoLegacyCleanups() is called very late) https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/util/... File chrome/installer/util/util_constants.h (left): https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/util/... chrome/installer/util/util_constants.h:146: UNINSTALLING_BINARIES, // Uninstalling unused binaries. I thought the stages were tracked by Google Update (i.e. ID's meaning for each stage shouldn't change).
The CQ bit was checked by grt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! PTAL. https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... File chrome/installer/setup/install.cc (left): https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... chrome/installer/setup/install.cc:445: DCHECK(product.is_chrome()); On 2016/12/22 19:05:10, gab (HOHOHO) wrote: > I don't see a change to product.h in this CL? Leave this for a follow-up? While the change to product.h is in https://codereview.chromium.org/2594113002/, I thought it made sense to remove this here since there is no codepath that could lead to is_chrome() being false. It seemed to be in the spirit of removing multi-install from setup -- setup only processes Chrome. WDYT? https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... chrome/installer/setup/install.cc:552: if (chrome_product) { On 2016/12/22 19:05:10, gab (HOHOHO) wrote: > Nice, we had two if (chrome_product) {} blocks in a row :-O! Indeed! I laughed at that one, too. https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... chrome/installer/setup/install.cc:509: const Product& chrome_product = installer_state.product(); On 2016/12/22 19:05:10, gab (HOHOHO) wrote: > I presume InstallerState and Product will eventually (in a follow-up) become the > same thing if we have only one Product? Yeah, there will be opportunities for further simplification once all of this lands. I'm not sure exactly what it will look like when the dust settles, but some of the layers will certainly be removable. https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... File chrome/installer/setup/install_worker.cc (left): https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... chrome/installer/setup/install_worker.cc:1172: KEY_WOW64_32KEY); On 2016/12/22 19:05:10, gab (HOHOHO) wrote: > Don't we still need this cleanup step? Yes. I've moved this into DoLegacyCleanups (with UMA!), which is run after every install/update. https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... File chrome/installer/setup/install_worker.cc (right): https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... chrome/installer/setup/install_worker.cc:615: base::string16 version_key = dist->GetVersionKey(); On 2016/12/22 19:05:10, gab (HOHOHO) wrote: > const Done here and on line 652. https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... chrome/installer/setup/install_worker.cc:634: base::CommandLine product_rename_cmd(rename); On 2016/12/22 19:05:10, gab (HOHOHO) wrote: > Add a comment on what this does? It's unclear even from > ProductOperations::AppendRenameFlags()'s description. Done. https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... chrome/installer/setup/install_worker.cc:721: AddChromeWorkItems(original_state, installer_state, setup_path, archive_path, On 2016/12/22 19:05:10, gab (HOHOHO) wrote: > Assuming we'll rename/shuffle this as well in a follow-up? Yeah. https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... File chrome/installer/setup/install_worker_unittest.cc (left): https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... chrome/installer/setup/install_worker_unittest.cc:629: TEST_F(InstallWorkerTest, AddUsageStatsWorkItems) { On 2016/12/22 19:05:10, gab (HOHOHO) wrote: > This is still used (though moved to anonymous), is it still tested? AddUsageStatsWorkItems was responsible for migrating single-to-multi, and has been removed entirely. https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... File chrome/installer/setup/install_worker_unittest.cc (right): https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... chrome/installer/setup/install_worker_unittest.cc:255: if (chrome != NULL) { On 2016/12/22 19:05:10, gab (HOHOHO) wrote: > if (chrome) Done. https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... File chrome/installer/setup/installer_state.cc (right): https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... chrome/installer/setup/installer_state.cc:77: // For a single-install, the current browser dist is the operand. On 2016/12/22 19:05:10, gab (HOHOHO) wrote: > Comment no longer relevant given we're always in single-install? Done. https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... chrome/installer/setup/installer_state.cc:78: BrowserDistribution* operand = BrowserDistribution::GetDistribution(); On 2016/12/22 19:05:10, gab (HOHOHO) wrote: > Along similar thoughts, s/operand/dist ? Good point. Done. https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... chrome/installer/setup/installer_state.cc:116: base::FilePath InstallerState::GetDefaultProductInstallPath( On 2016/12/22 19:05:10, gab (HOHOHO) wrote: > This is now so simple it should probably be inlined? Done. https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... chrome/installer/setup/installer_state.cc:126: bool InstallerState::CanAddProduct(const Product& product, On 2016/12/22 19:05:10, gab (HOHOHO) wrote: > Now that the only product is chrome, will this go away in a follow-up CL? During the cleanups that will be unlocked when this mess is done, yes. For now, there are still multiple paths to adding a Product to an InstallerState so I think it makes sense to keep it. https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... chrome/installer/setup/installer_state.cc:141: std::unique_ptr<Product>* product) { On 2016/12/22 19:05:10, gab (HOHOHO) wrote: > If |product| can be moved out, the caller can already not re-use it so passing > by std::unique_ptr<Product> would make more sense than std::unique_ptr<Product>* > (from a quick code search seems this would be work and be cleaner). Hmm. As-is, the caller retains ownership of the instance in the failure case. It looks like all callsites would be fine if AddProduct* destroyed the Product on failure, but that seems like an odd ownership model to me. https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... File chrome/installer/setup/setup_util.cc (right): https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... chrome/installer/setup/setup_util.cc:190: value.find(L"\\Chrome Frame\\") != base::string16::npos) { On 2016/12/22 19:05:10, gab (HOHOHO) wrote: > Single Chrome Frame still exists? Yes, and still has active users! > Frozen to some old version I guess? 32.0.1700.107, to be exact. > Was it > migrated off of multi back then (i.e. this is likely the vast majority of CFs > left and we're fine killing remaining multi CF?) Yes. Less than 0.4% of GCF installs (0.32% of actives) could be affected. I can realistically count them between now and the end of my workday. :-) https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... chrome/installer/setup/setup_util.cc:225: InstallUtil::DeleteRegistryKey(installer_state.root_key(), path, On 2016/12/22 19:05:10, gab (HOHOHO) wrote: > I'm not 100% sure about the destruction order here. Is the RegKey guaranteed to > be deleted (and thus closed) before InstallUtil::DeleteRegistryKey() or only > before the end of the whole conditional? "If the second expression is evaluated, every value computation and side effect associated with the first expression is sequenced before every value computation and side effect associated with the second expression." I'm pretty sure that's a "yes". https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... chrome/installer/setup/setup_util.cc:255: UMA_HISTOGRAM_BOOLEAN("Setup.Install.InstallExtensionCommandRemoved", true); On 2016/12/22 19:05:10, gab (HOHOHO) wrote: > Is UMA live until the very end of setup.exe? (DoLegacyCleanups() is called very > late) It's alive until the end of wWinMain (torn down in the dtor of the |persistent_histogram_storage| instance on the stack). https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/util/... File chrome/installer/util/util_constants.h (left): https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/util/... chrome/installer/util/util_constants.h:146: UNINSTALLING_BINARIES, // Uninstalling unused binaries. On 2016/12/22 19:05:10, gab (HOHOHO) wrote: > I thought the stages were tracked by Google Update (i.e. ID's meaning for each > stage shouldn't change). I removed that in r414039. :-)
The CQ bit was checked by grt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
grt@chromium.org changed reviewers: + jwd@chromium.org
+jwd for histograms OWNERS review. Thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
huangs@chromium.org changed reviewers: + huangs@chromium.org
My comments. Just realized that it's less work for you for us to review last CL first... https://codereview.chromium.org/2589753002/diff/180001/chrome/installer/setup... File chrome/installer/setup/installer_state.cc (right): https://codereview.chromium.org/2589753002/diff/180001/chrome/installer/setup... chrome/installer/setup/installer_state.cc:14: #include "base/logging.h" logging.h is already included in .h file. https://codereview.chromium.org/2589753002/diff/180001/chrome/installer/setup... chrome/installer/setup/installer_state.cc:87: machine_state.GetProductState(system_install(), dist->GetType()); Just reminder that |state_type_| == dist->GetType(). Your call on whether to replace. https://codereview.chromium.org/2589753002/diff/180001/chrome/installer/setup... chrome/installer/setup/installer_state.cc:212: prod_type = product_->distribution()->GetType(); Can combine these 2 lines, or even 4 lines. https://codereview.chromium.org/2589753002/diff/180001/chrome/installer/setup... chrome/installer/setup/installer_state.cc:290: // Remove all multi-install products from the channel name. Mention that this is handling legacy? Also, should |product_->SetChannelFlags(...)| be called? https://codereview.chromium.org/2589753002/diff/180001/chrome/installer/setup... File chrome/installer/setup/installer_state.h (right): https://codereview.chromium.org/2589753002/diff/180001/chrome/installer/setup... chrome/installer/setup/installer_state.h:18: #include "chrome/installer/util/product.h" Can Product be forward-declared? Not that .cc file also includes product.h. https://codereview.chromium.org/2589753002/diff/180001/chrome/installer/setup... chrome/installer/setup/installer_state.h:192: #if defined(OS_WIN) This #if seems redundant; or if it serves some weird case there should be a comment. https://codereview.chromium.org/2589753002/diff/180001/chrome/installer/setup... File chrome/installer/setup/setup_main.cc (right): https://codereview.chromium.org/2589753002/diff/180001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:886: LOG(DFATAL) << "system_install:" << installer_state->system_install(); installer_state->system_install() would always be false here. https://codereview.chromium.org/2589753002/diff/180001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:1475: DoLegacyCleanups(installer_state, install_status); Would it be more robust to do legacy cleanup before InstallProducts()? https://codereview.chromium.org/2589753002/diff/180001/chrome/installer/setup... File chrome/installer/setup/setup_util.cc (right): https://codereview.chromium.org/2589753002/diff/180001/chrome/installer/setup... chrome/installer/setup/setup_util.cc:158: // There never was a "Chromium Frame". NIT on comment indent? Disregard if done by "git cl lint". Same below. https://codereview.chromium.org/2589753002/diff/180001/chrome/installer/setup... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/2589753002/diff/180001/chrome/installer/setup... chrome/installer/setup/uninstall.cc:29: #include "base/win/scoped_handle.h" scoped_handle.h unused? https://codereview.chromium.org/2589753002/diff/180001/chrome/installer/setup... chrome/installer/setup/uninstall.cc:129: base::FilePath setup_exe_base_name(installer::kSetupExe); This is now unused. https://codereview.chromium.org/2589753002/diff/180001/chrome/installer/setup... chrome/installer/setup/uninstall.cc:532: const char* install_level = This value is now always "user". https://codereview.chromium.org/2589753002/diff/180001/chrome/installer/setup... chrome/installer/setup/uninstall.cc:1004: // Delete media player registry key that exists only in HKLM. We don't NIT: Spacing after ".". https://codereview.chromium.org/2589753002/diff/180001/chrome/installer/setup... chrome/installer/setup/uninstall.cc:1079: Update and put back the VLOG that was in CheckShouldRemoveSetup()? It was: VLOG(1) << "Removing all installer files for a non-multi installation."; https://codereview.chromium.org/2589753002/diff/180001/chrome/installer/util/... File chrome/installer/util/util_constants.cc (left): https://codereview.chromium.org/2589753002/diff/180001/chrome/installer/util/... chrome/installer/util/util_constants.cc:209: const wchar_t kAppLauncherGuid[] = L"{FDA71E6F-AC4C-4a00-8B70-9958A68906BF}"; This GUID also appears in src/chrome/test/mini_installer/variable_expander.py Maybe this should be changed as well?
The CQ bit was checked by grt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! PTAL. https://codereview.chromium.org/2589753002/diff/180001/chrome/installer/setup... File chrome/installer/setup/installer_state.cc (right): https://codereview.chromium.org/2589753002/diff/180001/chrome/installer/setup... chrome/installer/setup/installer_state.cc:14: #include "base/logging.h" On 2017/01/03 07:26:05, huangs wrote: > logging.h is already included in .h file. Done. https://codereview.chromium.org/2589753002/diff/180001/chrome/installer/setup... chrome/installer/setup/installer_state.cc:87: machine_state.GetProductState(system_install(), dist->GetType()); On 2017/01/03 07:26:05, huangs wrote: > Just reminder that |state_type_| == dist->GetType(). Your call on whether to > replace. Done. https://codereview.chromium.org/2589753002/diff/180001/chrome/installer/setup... chrome/installer/setup/installer_state.cc:212: prod_type = product_->distribution()->GetType(); On 2017/01/03 07:26:05, huangs wrote: > Can combine these 2 lines, or even 4 lines. Done. https://codereview.chromium.org/2589753002/diff/180001/chrome/installer/setup... chrome/installer/setup/installer_state.cc:290: // Remove all multi-install products from the channel name. On 2017/01/03 07:26:05, huangs wrote: > Mention that this is handling legacy? Done. > Also, should |product_->SetChannelFlags(...)| be called? Not in this case. The three lines below are intended to do whatever SetChannelFlags (which is going away in https://codereview.chromium.org/2594113002/) might have done. https://codereview.chromium.org/2589753002/diff/180001/chrome/installer/setup... File chrome/installer/setup/setup_main.cc (right): https://codereview.chromium.org/2589753002/diff/180001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:886: LOG(DFATAL) << "system_install:" << installer_state->system_install(); On 2017/01/03 07:26:05, huangs wrote: > installer_state->system_install() would always be false here. Nice! Rejiggered. https://codereview.chromium.org/2589753002/diff/180001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:1475: DoLegacyCleanups(installer_state, install_status); On 2017/01/03 07:26:05, huangs wrote: > Would it be more robust to do legacy cleanup before InstallProducts()? I don't think so, as it should only be done when the install or update succeeds. Otherwise it could break the existing install. (DoLegacyCleanups checks |install_status| to decide whether or not it should do work.) https://codereview.chromium.org/2589753002/diff/180001/chrome/installer/setup... File chrome/installer/setup/setup_util.cc (right): https://codereview.chromium.org/2589753002/diff/180001/chrome/installer/setup... chrome/installer/setup/setup_util.cc:158: // There never was a "Chromium Frame". On 2017/01/03 07:26:05, huangs wrote: > NIT on comment indent? Disregard if done by "git cl lint". Same below. "cl format" does that if the comment precedes a preprocessor directive. I hate it, too, but I'm trying hard to not reverse the formatter's style. https://codereview.chromium.org/2589753002/diff/180001/chrome/installer/setup... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/2589753002/diff/180001/chrome/installer/setup... chrome/installer/setup/uninstall.cc:29: #include "base/win/scoped_handle.h" On 2017/01/03 07:26:06, huangs wrote: > scoped_handle.h unused? Done. https://codereview.chromium.org/2589753002/diff/180001/chrome/installer/setup... chrome/installer/setup/uninstall.cc:129: base::FilePath setup_exe_base_name(installer::kSetupExe); On 2017/01/03 07:26:06, huangs wrote: > This is now unused. Yay! https://codereview.chromium.org/2589753002/diff/180001/chrome/installer/setup... chrome/installer/setup/uninstall.cc:532: const char* install_level = On 2017/01/03 07:26:05, huangs wrote: > This value is now always "user". Done. https://codereview.chromium.org/2589753002/diff/180001/chrome/installer/setup... chrome/installer/setup/uninstall.cc:1004: // Delete media player registry key that exists only in HKLM. We don't On 2017/01/03 07:26:06, huangs wrote: > NIT: Spacing after ".". Done. https://codereview.chromium.org/2589753002/diff/180001/chrome/installer/setup... chrome/installer/setup/uninstall.cc:1079: On 2017/01/03 07:26:06, huangs wrote: > Update and put back the VLOG that was in CheckShouldRemoveSetup()? It was: > > VLOG(1) << "Removing all installer files for a non-multi installation."; Nice idea; done. https://codereview.chromium.org/2589753002/diff/180001/chrome/installer/util/... File chrome/installer/util/util_constants.cc (left): https://codereview.chromium.org/2589753002/diff/180001/chrome/installer/util/... chrome/installer/util/util_constants.cc:209: const wchar_t kAppLauncherGuid[] = L"{FDA71E6F-AC4C-4a00-8B70-9958A68906BF}"; On 2017/01/03 07:26:06, huangs wrote: > This GUID also appears in > src/chrome/test/mini_installer/variable_expander.py > > Maybe this should be changed as well? The value in the test harness is there so that tests can assert that the key doesn't exist. I think it's worth keeping there.
The CQ bit was checked by grt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2589753002/diff/220001/chrome/installer/setup... File chrome/installer/setup/setup_util.cc (right): https://codereview.chromium.org/2589753002/diff/220001/chrome/installer/setup... chrome/installer/setup/setup_util.cc:239: UMA_HISTOGRAM_BOOLEAN("Setup.Install.AppHostRemoved", true); Is there value in capturing a failure to delete? https://codereview.chromium.org/2589753002/diff/220001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2589753002/diff/220001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:60465: +<histogram name="Setup.Install.AppHostRemoved" enum="BooleanHit"> I'd prefer a more meaningful enum for these, maybe use one of the existing delete ones, or add a BooleanRemoved enum type.
The CQ bit was checked by grt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the suggestions; done. https://codereview.chromium.org/2589753002/diff/220001/chrome/installer/setup... File chrome/installer/setup/setup_util.cc (right): https://codereview.chromium.org/2589753002/diff/220001/chrome/installer/setup... chrome/installer/setup/setup_util.cc:239: UMA_HISTOGRAM_BOOLEAN("Setup.Install.AppHostRemoved", true); On 2017/01/03 21:10:33, jwd wrote: > Is there value in capturing a failure to delete? Good point. It would be useful to know if the file exists but couldn't be deleted. https://codereview.chromium.org/2589753002/diff/220001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2589753002/diff/220001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:60465: +<histogram name="Setup.Install.AppHostRemoved" enum="BooleanHit"> On 2017/01/03 21:10:33, jwd wrote: > I'd prefer a more meaningful enum for these, maybe use one of the existing > delete ones, or add a BooleanRemoved enum type. Rejiggered all of 'em.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Sorry I haven't had time to go through this whole thing yet. I just got back and I'm still wrapping my head around it, here are a couple of quick nits from a partial read-through. https://codereview.chromium.org/2589753002/diff/240001/chrome/installer/setup... File chrome/installer/setup/install_worker.cc (right): https://codereview.chromium.org/2589753002/diff/240001/chrome/installer/setup... chrome/installer/setup/install_worker.cc:170: WorkItemList* list) { Do we still need this function at all? Since we're moving to a single-product model, could the code below be inlined where AddProductSpecificWorkItems is now called? https://codereview.chromium.org/2589753002/diff/240001/chrome/installer/setup... chrome/installer/setup/install_worker.cc:788: // TODO(gab): Remove cleanup code for Metro after M53. Can this be removed now?
The CQ bit was checked by grt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2589753002/diff/240001/chrome/installer/setup... File chrome/installer/setup/install_worker.cc (right): https://codereview.chromium.org/2589753002/diff/240001/chrome/installer/setup... chrome/installer/setup/install_worker.cc:170: WorkItemList* list) { On 2017/01/04 14:23:18, robertshield_slow_reviews wrote: > Do we still need this function at all? Since we're moving to a single-product > model, could the code below be inlined where AddProductSpecificWorkItems is now > called? Nope; removed. https://codereview.chromium.org/2589753002/diff/240001/chrome/installer/setup... chrome/installer/setup/install_worker.cc:788: // TODO(gab): Remove cleanup code for Metro after M53. On 2017/01/04 14:23:18, robertshield_slow_reviews wrote: > Can this be removed now? While the cleanup shipped in M49, we still have over 9% of active users on M48 and older. I'm not sure if this number is dropping over time due to clients updating or being retired. If it's still helping users as they get up-to-date, I'm inclined to keep it for now. WDYT?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Another quick pass (this is awesome by the way). https://codereview.chromium.org/2589753002/diff/240001/chrome/installer/setup... File chrome/installer/setup/install_worker.cc (right): https://codereview.chromium.org/2589753002/diff/240001/chrome/installer/setup... chrome/installer/setup/install_worker.cc:788: // TODO(gab): Remove cleanup code for Metro after M53. On 2017/01/05 13:08:29, grt (UTC plus 1) wrote: > On 2017/01/04 14:23:18, robertshield_slow_reviews wrote: > > Can this be removed now? > > While the cleanup shipped in M49, we still have over 9% of active users on M48 > and older. I'm not sure if this number is dropping over time due to clients > updating or being retired. If it's still helping users as they get up-to-date, > I'm inclined to keep it for now. WDYT? SGTM, maybe update the comment? https://codereview.chromium.org/2589753002/diff/260001/chrome/installer/setup... File chrome/installer/setup/setup_main.cc (right): https://codereview.chromium.org/2589753002/diff/260001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:1236: // We never want to launch Chrome in system level install mode. Though it makes sense, do_not_launch_chrome being user-level only isn't very well documented (https://www.chromium.org/developers/design-documents/first-run-customizations). Might be worth updating the docs.
Comments on comments otherwise this first pass lgtm (didn't do another full pass; I checked for correctness in first round and at this point all I think I could find is more things to remove and since this is only the first CL it's not necessary) This is awesome! Cheers, Gab https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... File chrome/installer/setup/install.cc (left): https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... chrome/installer/setup/install.cc:445: DCHECK(product.is_chrome()); On 2017/01/02 10:45:03, grt (UTC plus 1) wrote: > On 2016/12/22 19:05:10, gab (HOHOHO) wrote: > > I don't see a change to product.h in this CL? Leave this for a follow-up? > > While the change to product.h is in https://codereview.chromium.org/2594113002/, > I thought it made sense to remove this here since there is no codepath that > could lead to is_chrome() being false. It seemed to be in the spirit of removing > multi-install from setup -- setup only processes Chrome. WDYT? I think we should leave the DCHECK while Product has logic that make it look like it could be non-chrome (even though it never is in practice). https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... File chrome/installer/setup/installer_state.cc (right): https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... chrome/installer/setup/installer_state.cc:141: std::unique_ptr<Product>* product) { On 2017/01/02 10:45:03, grt (UTC plus 1) wrote: > On 2016/12/22 19:05:10, gab (HOHOHO) wrote: > > If |product| can be moved out, the caller can already not re-use it so passing > > by std::unique_ptr<Product> would make more sense than > std::unique_ptr<Product>* > > (from a quick code search seems this would be work and be cleaner). > > Hmm. As-is, the caller retains ownership of the instance in the failure case. It > looks like all callsites would be fine if AddProduct* destroyed the Product on > failure, but that seems like an odd ownership model to me. It doesn't seem weird to me. It says: "here's the Product, it's yours, you may delete it if you fail to do what you want with it". This is what the current callers do anyways. https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... File chrome/installer/setup/setup_util.cc (right): https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... chrome/installer/setup/setup_util.cc:142: base::string16 path(reg_data.GetVersionKey()); Why was this called the "version key" again? That confused me just now... it's really the "product key" or something, nothing to do with version.. https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... chrome/installer/setup/setup_util.cc:190: value.find(L"\\Chrome Frame\\") != base::string16::npos) { On 2017/01/02 10:45:03, grt (UTC plus 1) wrote: > On 2016/12/22 19:05:10, gab (HOHOHO) wrote: > > Single Chrome Frame still exists? > > Yes, and still has active users! > > > Frozen to some old version I guess? > > 32.0.1700.107, to be exact. > > > Was it > > migrated off of multi back then (i.e. this is likely the vast majority of CFs > > left and we're fine killing remaining multi CF?) > > Yes. Less than 0.4% of GCF installs (0.32% of actives) could be affected. I can > realistically count them between now and the end of my workday. :-) Ok but which code is this moving? I can't seem to find the old code that did this already? https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... chrome/installer/setup/setup_util.cc:225: InstallUtil::DeleteRegistryKey(installer_state.root_key(), path, On 2017/01/02 10:45:03, grt (UTC plus 1) wrote: > On 2016/12/22 19:05:10, gab (HOHOHO) wrote: > > I'm not 100% sure about the destruction order here. Is the RegKey guaranteed > to > > be deleted (and thus closed) before InstallUtil::DeleteRegistryKey() or only > > before the end of the whole conditional? > > "If the second expression is evaluated, every value computation and side effect > associated with the first expression is sequenced before every value computation > and side effect associated with the second expression." I'm pretty sure that's a > "yes". Cool beans :)
The CQ bit was checked by grt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
gab: Thanks! huangs, robertshield, jwd: PTAL https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... File chrome/installer/setup/install.cc (left): https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... chrome/installer/setup/install.cc:445: DCHECK(product.is_chrome()); On 2017/01/05 22:08:01, gab wrote: > On 2017/01/02 10:45:03, grt (UTC plus 1) wrote: > > On 2016/12/22 19:05:10, gab (HOHOHO) wrote: > > > I don't see a change to product.h in this CL? Leave this for a follow-up? > > > > While the change to product.h is in > https://codereview.chromium.org/2594113002/, > > I thought it made sense to remove this here since there is no codepath that > > could lead to is_chrome() being false. It seemed to be in the spirit of > removing > > multi-install from setup -- setup only processes Chrome. WDYT? > > I think we should leave the DCHECK while Product has logic that make it look > like it could be non-chrome (even though it never is in practice). Done. https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... File chrome/installer/setup/installer_state.cc (right): https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... chrome/installer/setup/installer_state.cc:141: std::unique_ptr<Product>* product) { On 2017/01/05 22:08:01, gab wrote: > On 2017/01/02 10:45:03, grt (UTC plus 1) wrote: > > On 2016/12/22 19:05:10, gab (HOHOHO) wrote: > > > If |product| can be moved out, the caller can already not re-use it so > passing > > > by std::unique_ptr<Product> would make more sense than > > std::unique_ptr<Product>* > > > (from a quick code search seems this would be work and be cleaner). > > > > Hmm. As-is, the caller retains ownership of the instance in the failure case. > It > > looks like all callsites would be fine if AddProduct* destroyed the Product on > > failure, but that seems like an odd ownership model to me. > > It doesn't seem weird to me. > > It says: "here's the Product, it's yours, you may delete it if you fail to do > what you want with it". > > This is what the current callers do anyways. Done. https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... File chrome/installer/setup/setup_util.cc (right): https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... chrome/installer/setup/setup_util.cc:142: base::string16 path(reg_data.GetVersionKey()); On 2017/01/05 22:08:01, gab wrote: > Why was this called the "version key" again? That confused me just now... it's > really the "product key" or something, nothing to do with version.. Historical accident, I think. It's been that way for, like, ever. It is the subkey of the Google Update "Clients" key that holds the app's Product Version ("pv") value. I am starting to call it "ClientsKey" in new APIs (I think :-) ), since it's more clear to me that it be named after what it is rather than calling it the "VersionKey" just because it holds the version value. https://codereview.chromium.org/2589753002/diff/140001/chrome/installer/setup... chrome/installer/setup/setup_util.cc:190: value.find(L"\\Chrome Frame\\") != base::string16::npos) { On 2017/01/05 22:08:01, gab wrote: > On 2017/01/02 10:45:03, grt (UTC plus 1) wrote: > > On 2016/12/22 19:05:10, gab (HOHOHO) wrote: > > > Single Chrome Frame still exists? > > > > Yes, and still has active users! > > > > > Frozen to some old version I guess? > > > > 32.0.1700.107, to be exact. > > > > > Was it > > > migrated off of multi back then (i.e. this is likely the vast majority of > CFs > > > left and we're fine killing remaining multi CF?) > > > > Yes. Less than 0.4% of GCF installs (0.32% of actives) could be affected. I > can > > realistically count them between now and the end of my workday. :-) > > Ok but which code is this moving? I can't seem to find the old code that did > this already? UninstallMultiChromeFrameIfPresent in setup_main.cc used to more-or-less run an Uninstall from the currently-running setup.exe. RemoveMultiChromeFrame is a hack-and-slash to remove the most critical bits and allows for all other CF-related code to be removed (BrowserDistribution, ProductOperations, etc). For completeness: The code to migrate multi-install Chrome Frame to single-install was added in https://crrev.com/224600 (31.0.1639.0). The code to uninstall multi-install Chrome Frame rather than migrate it was added in https://crrev.com/238588 (33.0.1729.0). The migration code went away when installation of Chrome Frame was removed in https://crrev.com/242400 (34.0.1759.0). https://codereview.chromium.org/2589753002/diff/240001/chrome/installer/setup... File chrome/installer/setup/install_worker.cc (right): https://codereview.chromium.org/2589753002/diff/240001/chrome/installer/setup... chrome/installer/setup/install_worker.cc:788: // TODO(gab): Remove cleanup code for Metro after M53. On 2017/01/05 14:20:59, robertshield_slow_reviews wrote: > On 2017/01/05 13:08:29, grt (UTC plus 1) wrote: > > On 2017/01/04 14:23:18, robertshield_slow_reviews wrote: > > > Can this be removed now? > > > > While the cleanup shipped in M49, we still have over 9% of active users on M48 > > and older. I'm not sure if this number is dropping over time due to clients > > updating or being retired. If it's still helping users as they get up-to-date, > > I'm inclined to keep it for now. WDYT? > > SGTM, maybe update the comment? What a good idea! Done! :-) https://codereview.chromium.org/2589753002/diff/260001/chrome/installer/setup... File chrome/installer/setup/setup_main.cc (right): https://codereview.chromium.org/2589753002/diff/260001/chrome/installer/setup... chrome/installer/setup/setup_main.cc:1236: // We never want to launch Chrome in system level install mode. On 2017/01/05 14:20:59, robertshield_slow_reviews wrote: > Though it makes sense, do_not_launch_chrome being user-level only isn't very > well documented > (https://www.chromium.org/developers/design-documents/first-run-customizations). > > > Might be worth updating the docs. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
lgtm
Just NITs. LGTM. https://codereview.chromium.org/2589753002/diff/280001/chrome/installer/setup... File chrome/installer/setup/install_worker.cc (right): https://codereview.chromium.org/2589753002/diff/280001/chrome/installer/setup... chrome/installer/setup/install_worker.cc:598: const base::string16 version_key = dist->GetVersionKey(); NIT: Does same thing as line 637 but looks different. https://codereview.chromium.org/2589753002/diff/280001/chrome/installer/setup... chrome/installer/setup/install_worker.cc:792: const char* install_level = |install_level| = "user" always. https://codereview.chromium.org/2589753002/diff/280001/chrome/installer/setup... File chrome/installer/setup/installer_state.cc (right): https://codereview.chromium.org/2589753002/diff/280001/chrome/installer/setup... chrome/installer/setup/installer_state.cc:116: bool InstallerState::CanAddProduct(const Product& product, Note that |product| is no longer used. Would code deletion impact the flow that starts from line 193? Anyway please at least add TODO() to address this later. https://codereview.chromium.org/2589753002/diff/280001/chrome/installer/setup... File chrome/installer/setup/installer_state.h (right): https://codereview.chromium.org/2589753002/diff/280001/chrome/installer/setup... chrome/installer/setup/installer_state.h:74: // with this object. Ownership of the return value is not given to the NIT: Extra space after ".", then can unwrap. https://codereview.chromium.org/2589753002/diff/280001/chrome/installer/setup... File chrome/installer/setup/setup_util.cc (right): https://codereview.chromium.org/2589753002/diff/280001/chrome/installer/setup... chrome/installer/setup/setup_util.cc:146: const bool success = InstallUtil::DeleteRegistryKey( NIT: const usage for |success| (here and below) is inconsistent with lack of const for |has_clients_key| and |has_client_state_key| in RemoveMultiChromeFrame(). https://codereview.chromium.org/2589753002/diff/280001/chrome/installer/setup... chrome/installer/setup/setup_util.cc:163: // ClientState\UninstallString contains a path including "\Chrome NIT: Fix comment spacing re. "\Chrome Frame\". https://codereview.chromium.org/2589753002/diff/280001/chrome/installer/setup... chrome/installer/setup/setup_util.cc:198: client_state_key.Close(); NIT: Should these appear before the big comment? https://codereview.chromium.org/2589753002/diff/280001/chrome/installer/setup... chrome/installer/setup/setup_util.cc:234: #endif #endif // GOOGLE_CHROME_BUILD Same below. https://codereview.chromium.org/2589753002/diff/280001/chrome/installer/setup... File chrome/installer/setup/setup_util_unittest.cc (right): https://codereview.chromium.org/2589753002/diff/280001/chrome/installer/setup... chrome/installer/setup/setup_util_unittest.cc:725: EXPECT_TRUE(temp_dir_.CreateUniqueTempDir()); NIT: Semantically, temp dir is part of test fixture, not the actual test. So perhaps use ASSERT_TRUE() instead of EXPECT_TRUE()? Same with the reg keys created. If compiler complains about ASSERT usage since you can't return a value in constructor, then maybe move the code to void Setup() override { ... } https://codereview.chromium.org/2589753002/diff/280001/chrome/installer/setup... chrome/installer/setup/setup_util_unittest.cc:740: EXPECT_GT(base::WriteFile(GetAppHostExePath(), "cha", 3), 0); NIT: base::WriteFile() returns the number of bytes written, so perhaps: ASSERT_EQ(base::WriteFile(GetAppHostExePath(), "cha", 3), 3); ? https://codereview.chromium.org/2589753002/diff/280001/chrome/installer/setup... chrome/installer/setup/setup_util_unittest.cc:747: #endif #endif // GOOGLE_CHROME_BUILD Same below. https://codereview.chromium.org/2589753002/diff/280001/chrome/installer/setup... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/2589753002/diff/280001/chrome/installer/setup... chrome/installer/setup/uninstall.cc:375: // Try closing any running Chrome processes and deleting files once NIT: Can unwrap?
Thanks. Will send to CQ after syncing to ToT. https://codereview.chromium.org/2589753002/diff/280001/chrome/installer/setup... File chrome/installer/setup/install_worker.cc (right): https://codereview.chromium.org/2589753002/diff/280001/chrome/installer/setup... chrome/installer/setup/install_worker.cc:598: const base::string16 version_key = dist->GetVersionKey(); On 2017/01/06 17:57:17, huangs wrote: > NIT: Does same thing as line 637 but looks different. Done. https://codereview.chromium.org/2589753002/diff/280001/chrome/installer/setup... chrome/installer/setup/install_worker.cc:792: const char* install_level = On 2017/01/06 17:57:17, huangs wrote: > |install_level| = "user" always. Done. https://codereview.chromium.org/2589753002/diff/280001/chrome/installer/setup... File chrome/installer/setup/installer_state.cc (right): https://codereview.chromium.org/2589753002/diff/280001/chrome/installer/setup... chrome/installer/setup/installer_state.cc:116: bool InstallerState::CanAddProduct(const Product& product, On 2017/01/06 17:57:18, huangs wrote: > Note that |product| is no longer used. Would code deletion impact the flow that > starts from line 193? Anyway please at least add TODO() to address this later. Done. https://codereview.chromium.org/2589753002/diff/280001/chrome/installer/setup... File chrome/installer/setup/installer_state.h (right): https://codereview.chromium.org/2589753002/diff/280001/chrome/installer/setup... chrome/installer/setup/installer_state.h:74: // with this object. Ownership of the return value is not given to the On 2017/01/06 17:57:18, huangs wrote: > NIT: Extra space after ".", then can unwrap. I was produced in an era where we were taught to put two spaces after end-of-sentence punctuation. Once I find my reading glasses and my cane I'll fix all of them in this file. ah, there they are Done. ;-) https://codereview.chromium.org/2589753002/diff/280001/chrome/installer/setup... File chrome/installer/setup/setup_util.cc (right): https://codereview.chromium.org/2589753002/diff/280001/chrome/installer/setup... chrome/installer/setup/setup_util.cc:146: const bool success = InstallUtil::DeleteRegistryKey( On 2017/01/06 17:57:18, huangs wrote: > NIT: const usage for |success| (here and below) is inconsistent with lack of > const for |has_clients_key| and |has_client_state_key| in > RemoveMultiChromeFrame(). Done. https://codereview.chromium.org/2589753002/diff/280001/chrome/installer/setup... chrome/installer/setup/setup_util.cc:163: // ClientState\UninstallString contains a path including "\Chrome On 2017/01/06 17:57:18, huangs wrote: > NIT: Fix comment spacing re. "\Chrome Frame\". Would you believe that my editor was written by people who were created in the "two spaces after end-of-sentence punctuation" era, so it refuses to wrap a line at a mid-sentence period since it creates ambiguity? :-) (Done.) https://codereview.chromium.org/2589753002/diff/280001/chrome/installer/setup... chrome/installer/setup/setup_util.cc:198: client_state_key.Close(); On 2017/01/06 17:57:18, huangs wrote: > NIT: Should these appear before the big comment? Done. https://codereview.chromium.org/2589753002/diff/280001/chrome/installer/setup... chrome/installer/setup/setup_util.cc:234: #endif On 2017/01/06 17:57:18, huangs wrote: > #endif // GOOGLE_CHROME_BUILD > > Same below. Done. https://codereview.chromium.org/2589753002/diff/280001/chrome/installer/setup... File chrome/installer/setup/setup_util_unittest.cc (right): https://codereview.chromium.org/2589753002/diff/280001/chrome/installer/setup... chrome/installer/setup/setup_util_unittest.cc:725: EXPECT_TRUE(temp_dir_.CreateUniqueTempDir()); On 2017/01/06 17:57:18, huangs wrote: > NIT: Semantically, temp dir is part of test fixture, not the actual test. So > perhaps use ASSERT_TRUE() instead of EXPECT_TRUE()? Same with the reg keys > created. > > If compiler complains about ASSERT usage since you can't return a value in > constructor, then maybe move the code to > > void Setup() override { > ... > } Done. https://codereview.chromium.org/2589753002/diff/280001/chrome/installer/setup... chrome/installer/setup/setup_util_unittest.cc:740: EXPECT_GT(base::WriteFile(GetAppHostExePath(), "cha", 3), 0); On 2017/01/06 17:57:18, huangs wrote: > NIT: base::WriteFile() returns the number of bytes written, so perhaps: > > ASSERT_EQ(base::WriteFile(GetAppHostExePath(), "cha", 3), 3); > > ? I chose GT 0 since the test only cares that the file is created, not that all three bytes are written. https://codereview.chromium.org/2589753002/diff/280001/chrome/installer/setup... chrome/installer/setup/setup_util_unittest.cc:747: #endif On 2017/01/06 17:57:18, huangs wrote: > #endif // GOOGLE_CHROME_BUILD > > Same below. Done. https://codereview.chromium.org/2589753002/diff/280001/chrome/installer/setup... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/2589753002/diff/280001/chrome/installer/setup... chrome/installer/setup/uninstall.cc:375: // Try closing any running Chrome processes and deleting files once On 2017/01/06 17:57:18, huangs wrote: > NIT: Can unwrap? Done.
The CQ bit was checked by grt@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org, robertshield@chromium.org, jwd@chromium.org, huangs@chromium.org Link to the patchset: https://codereview.chromium.org/2589753002/#ps320001 (title: "sync to position 442053")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by grt@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 320001, "attempt_start_ts": 1483739888654200,
"parent_rev": "3963e231e4c43187a05f3c6420f5df997d734ea0", "commit_rev":
"7bef10cf4c05108c03cf055c75695bbe5737bb56"}
Message was sent while issue was closed.
Description was changed from ========== Remove multi-install from chrome/installer/setup. This change strips InstallerState of all multi-install knowledge and trickles the change through the rest of the installer. An InstallerState now has exactly one Product on which it operates. There is substantial room for further improvement, which will come after InstallDetails is woven into setup. Various "delete old stuff" operations are also coalesced into a single post-install/update step rather than sprinkled around Chrome's install path. While at it, this change also removes some old Chrome Frame code that is no longer used (IE low rights elevation policy, GCF in use, COM DLLs) and attempts some IWYU fixes. BUG=577816 ========== to ========== Remove multi-install from chrome/installer/setup. This change strips InstallerState of all multi-install knowledge and trickles the change through the rest of the installer. An InstallerState now has exactly one Product on which it operates. There is substantial room for further improvement, which will come after InstallDetails is woven into setup. Various "delete old stuff" operations are also coalesced into a single post-install/update step rather than sprinkled around Chrome's install path. While at it, this change also removes some old Chrome Frame code that is no longer used (IE low rights elevation policy, GCF in use, COM DLLs) and attempts some IWYU fixes. BUG=577816 Review-Url: https://codereview.chromium.org/2589753002 Cr-Commit-Position: refs/heads/master@{#442077} Committed: https://chromium.googlesource.com/chromium/src/+/7bef10cf4c05108c03cf055c7569... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:320001) as https://chromium.googlesource.com/chromium/src/+/7bef10cf4c05108c03cf055c7569... |
