Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11412015:
Copy setup when quick-enabling app host to user-level from system-level. (Closed)
|
Created:
8 years, 1 month ago by erikwright (departed) Modified:
7 years, 11 months ago CC:
chromium-reviews, grt+watch_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionFix uninstallation of stand-alone user-level app launcher.
setup.exe is required to be able to uninstall the launcher. So we need to copy it in during installation. Needed to extract the setup/archive deletion logic from the Binaries-specific steps too.
BUG=158785, 151676
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=174543
Patch Set 1 #Patch Set 2 : Now with compilation. #
Total comments: 8
Patch Set 3 : Review feedback. #Patch Set 4 : Delete setup when uninstalling app_host and leave setup when not uninstalling app_host. #Patch Set 5 : Comments / move helpers to anonymous namespace. #
Total comments: 31
Patch Set 6 : Review comments. #Patch Set 7 : Remove unused method. #
Total comments: 14
Patch Set 8 : review comments. #
Total comments: 18
Patch Set 9 : sync #
Messages
Total messages: 24 (0 generated)
gab: PTAL.
https://chromiumcodereview.appspot.com/11412015/diff/2002/chrome/installer/se... File chrome/installer/setup/install_worker.cc (right): https://chromiumcodereview.appspot.com/11412015/diff/2002/chrome/installer/se... chrome/installer/setup/install_worker.cc:172: // We don't copy the archive when only App Host is at user-level. Only how are the App Host binary and this setup.exe kept up to date?
Good question. app_host.exe, after delegating to a system-level Chrome, will run the system-level setup.exe (--app-host --multi-install) if there's a version gap. That's not implemented yet, but should probably be very high priority. On Thu, Nov 15, 2012 at 11:38 PM, <grt@chromium.org> wrote: > > https://chromiumcodereview.**appspot.com/11412015/diff/** > 2002/chrome/installer/setup/**install_worker.cc<https://chromiumcodereview.appspot.com/11412015/diff/2002/chrome/installer/setup/install_worker.cc> > File chrome/installer/setup/**install_worker.cc (right): > > https://chromiumcodereview.**appspot.com/11412015/diff/** > 2002/chrome/installer/setup/**install_worker.cc#newcode172<https://chromiumcodereview.appspot.com/11412015/diff/2002/chrome/installer/setup/install_worker.cc#newcode172> > chrome/installer/setup/**install_worker.cc:172: // We don't copy the > > archive when only App Host is at user-level. Only > how are the App Host binary and this setup.exe kept up to date? > > https://chromiumcodereview.**appspot.com/11412015/<https://chromiumcodereview... >
thanks. one more q and a suggestion. https://chromiumcodereview.appspot.com/11412015/diff/2002/chrome/installer/se... File chrome/installer/setup/install_worker.cc (right): https://chromiumcodereview.appspot.com/11412015/diff/2002/chrome/installer/se... chrome/installer/setup/install_worker.cc:165: FilePath archive_dst(installer_dir.Append(archive_path.BaseName())); move this down to line 176 https://chromiumcodereview.appspot.com/11412015/diff/2002/chrome/installer/se... chrome/installer/setup/install_worker.cc:174: if (installer_state.products().size() != 1 || the comment says to only skip this when doing the app host at user level, yet the code doesn't reflect this. which is correct?
https://chromiumcodereview.appspot.com/11412015/diff/2002/chrome/installer/se... File chrome/installer/setup/install_worker.cc (right): https://chromiumcodereview.appspot.com/11412015/diff/2002/chrome/installer/se... chrome/installer/setup/install_worker.cc:174: if (installer_state.products().size() != 1 || On 2012/11/16 13:32:42, grt wrote: > the comment says to only skip this when doing the app host at user level, yet > the code doesn't reflect this. which is correct? The code reflects skipping this if exactly one product is being installed and that product is APP_HOST. This scenario can only occur with a user-level installation of the App Host piggybacking on a system-level Chrome Binaries. How about: // If only the App Host (not even the Chrome Binaries) is being installed, this must be a user-level App Host piggybacking on system-level Chrome Binaries. Only setup.exe is required, and only for uninstall.
lgtm w/ the one code move nit. thanks. https://chromiumcodereview.appspot.com/11412015/diff/2002/chrome/installer/se... File chrome/installer/setup/install_worker.cc (right): https://chromiumcodereview.appspot.com/11412015/diff/2002/chrome/installer/se... chrome/installer/setup/install_worker.cc:174: if (installer_state.products().size() != 1 || On 2012/11/16 13:58:33, erikwright wrote: > On 2012/11/16 13:32:42, grt wrote: > > the comment says to only skip this when doing the app host at user level, yet > > the code doesn't reflect this. which is correct? > > The code reflects skipping this if exactly one product is being installed and > that product is APP_HOST. This scenario can only occur with a user-level > installation of the App Host piggybacking on a system-level Chrome Binaries. > > How about: > > // If only the App Host (not even the Chrome Binaries) is being installed, this > must be a user-level App Host piggybacking on system-level Chrome Binaries. Only > setup.exe is required, and only for uninstall. sgtm
lgtm, +1 to grt's comments
gab/grt: PTAL. I realized that it wasn't enough just to copy the setup.exe over. 1) When uninstalling Chrome (but leaving app host) we need to keep setup.exe and ditch the binaries. (This could happen if a user-level Chrome is replaced by a system-level Chrome). 2) When uninstalling the app host (and Chrome isn't around) we need to delete setup.exe. I separated the deletion of setup stuff from the deletion of Chrome stuff. https://codereview.chromium.org/11412015/diff/2002/chrome/installer/setup/ins... File chrome/installer/setup/install_worker.cc (right): https://codereview.chromium.org/11412015/diff/2002/chrome/installer/setup/ins... chrome/installer/setup/install_worker.cc:165: FilePath archive_dst(installer_dir.Append(archive_path.BaseName())); On 2012/11/16 13:32:42, grt wrote: > move this down to line 176 Done. https://codereview.chromium.org/11412015/diff/2002/chrome/installer/setup/ins... chrome/installer/setup/install_worker.cc:172: // We don't copy the archive when only App Host is at user-level. Only On 2012/11/16 04:38:26, grt wrote: > how are the App Host binary and this setup.exe kept up to date? replied elsewhere. https://codereview.chromium.org/11412015/diff/2002/chrome/installer/setup/ins... chrome/installer/setup/install_worker.cc:174: if (installer_state.products().size() != 1 || On 2012/11/16 14:14:59, grt wrote: > On 2012/11/16 13:58:33, erikwright wrote: > > On 2012/11/16 13:32:42, grt wrote: > > > the comment says to only skip this when doing the app host at user level, > yet > > > the code doesn't reflect this. which is correct? > > > > The code reflects skipping this if exactly one product is being installed and > > that product is APP_HOST. This scenario can only occur with a user-level > > installation of the App Host piggybacking on a system-level Chrome Binaries. > > > > How about: > > > > // If only the App Host (not even the Chrome Binaries) is being installed, > this > > must be a user-level App Host piggybacking on system-level Chrome Binaries. > Only > > setup.exe is required, and only for uninstall. > > sgtm Done.
https://codereview.chromium.org/11412015/diff/4003/chrome/installer/setup/uni... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/11412015/diff/4003/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:244: // |remove_setup| is true. true -> false? https://codereview.chromium.org/11412015/diff/4003/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:579: target_path, true, FileEnumerator::FILES | FileEnumerator::DIRECTORIES); i find this change impenetrable. could you explain the change to make this recursive and the extra logic on line 589? https://codereview.chromium.org/11412015/diff/4003/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:1364: Version uninstall_version( it seems safer to me to get the path to setup.exe with: if (!PathService::Get(base::FILE_EXE, &setup_path)) { NOTREACHED(); return installer::UNINSTALL_FAILED; } and the install_directory with: FilePath install_directory(setup_path.DirName()); since that doesn't rely on "pv" in the registry existing and pointing to the current setup.exe. https://codereview.chromium.org/11412015/diff/4003/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:1390: if (DELETE_SUCCEEDED != DeleteEmptyDir(install_directory)) i think chromium style says to swap these https://codereview.chromium.org/11412015/diff/4003/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:1419: if (installer::DELETE_FAILED == swap here, too
Please update CL description.
https://codereview.chromium.org/11412015/diff/4003/chrome/installer/setup/uni... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/11412015/diff/4003/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:579: target_path, true, FileEnumerator::FILES | FileEnumerator::DIRECTORIES); On 2012/11/21 20:40:26, grt wrote: > i find this change impenetrable. could you explain the change to make this > recursive and the extra logic on line 589? Previously, we went through the top-level entries and deleted them (except app_host.exe). Now we want to leave the Installer directory for later handling. So we will recursively enumerate files and folders. It's breadth-first, so if a directory is deleted we don't review its individual contents. But if the directory is not deleted we will later review its individual contents. So: C:/Users/erikwright/AppData/Local/Google/Chrome Application/ chrome.exe app_host.exe 25.0.1000.0/ locales/ ... chrome.dll Installer/ setup.exe chrome.7z We delete chrome.exe, leave app_host.exe, and leave 25.0.1000.0 (because it's a parent of installer_directory). We delete locales, chrome.dll, and leave Installer (because it's installer_directory). We leave setup.exe and chrome.7z (because they are children of installer_directory). https://codereview.chromium.org/11412015/diff/4003/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:1364: Version uninstall_version( On 2012/11/21 20:40:26, grt wrote: > it seems safer to me to get the path to setup.exe with: > if (!PathService::Get(base::FILE_EXE, &setup_path)) { > NOTREACHED(); > return installer::UNINSTALL_FAILED; > } > and the install_directory with: > FilePath install_directory(setup_path.DirName()); > since that doesn't rely on "pv" in the registry existing and pointing to the > current setup.exe. Done naively, that actually seems more dangerous to me. If setup.exe is executed from some other location we would potentially wreak havoc. Option could be to only bother with this cleanup step if the current .exe is inside target_path. And if it is, then assume that its parent directory is the install directory. This would be mirrored in DeleteChromeFilesAndFolders (the directory to protect would be the parent directory of the current .exe, if it is a child of target_path). SGTY?
This looks to me like the assumption that setup.exe belongs with the Chrome binaries was just broken. It also sounds to me like app_host.exe is not a binary that depends tightly on the Chrome binaries (i.e., unlike the other multi-install products; e.g., Chrome Frame) and thus probably also doesn't belong in the Application directory. What about we pull both of those out of the Application directory? i.e. Move the Installer directory to the same level as the Application directory. And introduce an AppHost directory on the same level as well. Both the App Host and the Chrome binaries would hold a ref to the Installer directory which should be deleted iff they are both gone. Other comments below. Cheers, Gab https://codereview.chromium.org/11412015/diff/4003/chrome/installer/setup/uni... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/11412015/diff/4003/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:202: for (size_t i = 0; i < BrowserDistribution::NUM_TYPES; ++i) { Could add (*remove_setup || *remove_archive) as once they are both false, they are is no point to keep looping. https://codereview.chromium.org/11412015/diff/4003/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:204: static_cast<BrowserDistribution::Type>(i); I'm not a big fan of looping over all the potential products this way; I thought we had a way to get all currently installed products already (I remember seeing variables like vector<Product> products; which I'm guessing was obtained from some method we might be able to re-use here?). https://codereview.chromium.org/11412015/diff/4003/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:217: VLOG(1) << "Keeping setup.exe due to a remaining " Put the log beside *remove_setup = false; to keep logging and action together. This will need to be in an if(*remove_setup) { VLOG(...); *remove_setup = false; } block unless you don't care about logging this multiple times. https://codereview.chromium.org/11412015/diff/4003/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:235: system_level, static_cast<BrowserDistribution::Type>(i)); Same comment about getting installed products, if one doesn't exist, a common method would be nice for this. https://codereview.chromium.org/11412015/diff/4003/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:240: return Version(); How about: Version version; for (...) { ... if (product_state) { version = ... break; } } DCHECK(version.IsValid()); return version; One return and no need for NOTREACHED() which feels weird in, what looks at first sight like, the main return statement for the method. https://codereview.chromium.org/11412015/diff/4003/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:243: // Removes all files from the installer directory, leaving setup.exe iff s/installer/Installer https://codereview.chromium.org/11412015/diff/4003/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:257: if (!remove_setup && to_delete.BaseName().value() == installer::kSetupExe) I prefer comparing FilePaths to string16s, i.e. to_delete.BaseName() == FilePath(installer::kSetupExe); as arguably FilePath should know on Windows to compare case-insensitively (I don't think it does now, but it could/should). https://codereview.chromium.org/11412015/diff/4003/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:263: return false; Is an early exit intended here? We usually operate on a best-effort basis (i.e. we should return false at the end, but still try to delete the other files first). https://codereview.chromium.org/11412015/diff/4003/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:1354: return uninstall_status; Is the goal on UNINSTALL_FAILED to halt the uninstall? I'm not convinced this was the previous behavior, i.e. should we instead make a best effort at deleting the files anyways, but simply make sure to keep |uninstall_status| in its failure state? https://codereview.chromium.org/11412015/diff/4003/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:1399: if (delete_result == DELETE_NOT_EMPTY && else if? This doesn't actually change the flow, but it feels more readable to me. https://codereview.chromium.org/11412015/diff/4003/chrome/installer/setup/uni... File chrome/installer/setup/uninstall.h (right): https://codereview.chromium.org/11412015/diff/4003/chrome/installer/setup/uni... chrome/installer/setup/uninstall.h:80: installer::InstallStatus uninstall_status); What do you think of taking an installer::InstallStatus* and returning void? When reading the callsite in setup_main.cc originally I felt like it was overwriting install_status there...
in the interest of speed, let's discuss in person tomorrow.
gab/grt: PTAL. https://codereview.chromium.org/11412015/diff/4003/chrome/installer/setup/uni... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/11412015/diff/4003/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:202: for (size_t i = 0; i < BrowserDistribution::NUM_TYPES; ++i) { On 2012/11/21 22:55:55, gab wrote: > Could add (*remove_setup || *remove_archive) as once they are both false, they > are is no point to keep looping. Redundant with the return on 215. https://codereview.chromium.org/11412015/diff/4003/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:204: static_cast<BrowserDistribution::Type>(i); On 2012/11/21 22:55:55, gab wrote: > I'm not a big fan of looping over all the potential products this way; I thought > we had a way to get all currently installed products already (I remember seeing > variables like vector<Product> products; which I'm guessing was obtained from > some method we might be able to re-use here?). Product is from InstallerState. See the InstallationState API - it has no such method, though one could be added. https://codereview.chromium.org/11412015/diff/4003/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:217: VLOG(1) << "Keeping setup.exe due to a remaining " On 2012/11/21 22:55:55, gab wrote: > Put the log beside *remove_setup = false; to keep logging and action together. > > This will need to be in an > if(*remove_setup) { > VLOG(...); > *remove_setup = false; > } > block unless you don't care about logging this multiple times. The thing is, it's only true if the condition on 211 is not met. I could put it in an 'else' block to be more explicit, but that's redundant with the return on 215. https://codereview.chromium.org/11412015/diff/4003/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:235: system_level, static_cast<BrowserDistribution::Type>(i)); On 2012/11/21 22:55:55, gab wrote: > Same comment about getting installed products, if one doesn't exist, a common > method would be nice for this. Removed this instance. https://codereview.chromium.org/11412015/diff/4003/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:240: return Version(); On 2012/11/21 22:55:55, gab wrote: > How about: > > Version version; > for (...) { > ... > if (product_state) { > version = ... > break; > } > } > DCHECK(version.IsValid()); > return version; > > > One return and no need for NOTREACHED() which feels weird in, what looks at > first sight like, the main return statement for the method. Removed this. https://codereview.chromium.org/11412015/diff/4003/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:243: // Removes all files from the installer directory, leaving setup.exe iff On 2012/11/21 22:55:55, gab wrote: > s/installer/Installer IMHO this is a generic, not the specific name 'Installer'. The fact that it has that name is not relevant here. https://codereview.chromium.org/11412015/diff/4003/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:244: // |remove_setup| is true. On 2012/11/21 20:40:26, grt wrote: > true -> false? Done. https://codereview.chromium.org/11412015/diff/4003/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:257: if (!remove_setup && to_delete.BaseName().value() == installer::kSetupExe) On 2012/11/21 22:55:55, gab wrote: > I prefer comparing FilePaths to string16s, i.e. > to_delete.BaseName() == FilePath(installer::kSetupExe); > as arguably FilePath should know on Windows to compare case-insensitively (I > don't think it does now, but it could/should). That's awesome. https://codereview.chromium.org/11412015/diff/4003/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:1354: return uninstall_status; On 2012/11/21 22:55:55, gab wrote: > Is the goal on UNINSTALL_FAILED to halt the uninstall? I'm not convinced this > was the previous behavior, i.e. should we instead make a best effort at deleting > the files anyways, but simply make sure to keep |uninstall_status| in its > failure state? Previously, if we failed to delete a file in DeleteChromeFilesAndFolders, we would halt enumeration of other files (including the installer). I could go either way, but one advantage to keeping setup.exe is that the user could theoretically run it again to retry the uninstallation and hopefully succeed next time. https://codereview.chromium.org/11412015/diff/4003/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:1390: if (DELETE_SUCCEEDED != DeleteEmptyDir(install_directory)) On 2012/11/21 20:40:26, grt wrote: > i think chromium style says to swap these Done. https://codereview.chromium.org/11412015/diff/4003/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:1419: if (installer::DELETE_FAILED == On 2012/11/21 20:40:26, grt wrote: > swap here, too Done. https://codereview.chromium.org/11412015/diff/4003/chrome/installer/setup/uni... File chrome/installer/setup/uninstall.h (right): https://codereview.chromium.org/11412015/diff/4003/chrome/installer/setup/uni... chrome/installer/setup/uninstall.h:80: installer::InstallStatus uninstall_status); On 2012/11/21 22:55:55, gab wrote: > What do you think of taking an installer::InstallStatus* and returning void? > > When reading the callsite in setup_main.cc originally I felt like it was > overwriting install_status there... Done.
lg, just a nit, some comment suggestions, and a question or two. https://codereview.chromium.org/11412015/diff/14001/chrome/installer/setup/un... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/11412015/diff/14001/chrome/installer/setup/un... chrome/installer/setup/uninstall.cc:243: if (!remove_setup && to_delete.BaseName() == FilePath(installer::kSetupExe)) nit: create this FilePath instance out of the loop https://codereview.chromium.org/11412015/diff/14001/chrome/installer/setup/un... chrome/installer/setup/uninstall.cc:1353: const FilePath setup_exe = cmd_line.GetProgram(); wdyt about using file_util::AbsolutePath here in case the full path wasn't used? setup.exe could have been run via the command-line with a relative path. https://codereview.chromium.org/11412015/diff/14001/chrome/installer/setup/un... chrome/installer/setup/uninstall.cc:1373: // Remove files from "...\<product>\Application\<version>\Installer" https://codereview.chromium.org/11412015/diff/14001/chrome/installer/setup/un... chrome/installer/setup/uninstall.cc:1383: // Delete "...\<product>\Application\<version>\Installer" https://codereview.chromium.org/11412015/diff/14001/chrome/installer/setup/un... chrome/installer/setup/uninstall.cc:1388: // Delete "...\<product>\Application\<version>" https://codereview.chromium.org/11412015/diff/14001/chrome/installer/setup/un... chrome/installer/setup/uninstall.cc:1390: while (to_delete != target_path) { isn't the loop overkill? it looks like the version dir is the one and only dir that this loop processes.
PTAL. https://codereview.chromium.org/11412015/diff/14001/chrome/installer/setup/un... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/11412015/diff/14001/chrome/installer/setup/un... chrome/installer/setup/uninstall.cc:243: if (!remove_setup && to_delete.BaseName() == FilePath(installer::kSetupExe)) On 2012/11/23 21:00:55, grt wrote: > nit: create this FilePath instance out of the loop Done. https://codereview.chromium.org/11412015/diff/14001/chrome/installer/setup/un... chrome/installer/setup/uninstall.cc:1353: const FilePath setup_exe = cmd_line.GetProgram(); On 2012/11/23 21:00:55, grt wrote: > wdyt about using file_util::AbsolutePath here in case the full path wasn't used? > setup.exe could have been run via the command-line with a relative path. From my reading of GetCommandLine (which is what CommandLine::ForCurrentProcess()->GetProgram() is ultimately based on) the OS will fully-qualify the path: http://msdn.microsoft.com/en-us/library/windows/desktop/ms683156(v=vs.85).aspx But technically, it says "may", and I suppose AbsolutePath doesn't hurt in either case. https://codereview.chromium.org/11412015/diff/14001/chrome/installer/setup/un... chrome/installer/setup/uninstall.cc:1373: On 2012/11/23 21:00:55, grt wrote: > // Remove files from "...\<product>\Application\<version>\Installer" Done. https://codereview.chromium.org/11412015/diff/14001/chrome/installer/setup/un... chrome/installer/setup/uninstall.cc:1383: On 2012/11/23 21:00:55, grt wrote: > // Delete "...\<product>\Application\<version>\Installer" Done. https://codereview.chromium.org/11412015/diff/14001/chrome/installer/setup/un... chrome/installer/setup/uninstall.cc:1388: On 2012/11/23 21:00:55, grt wrote: > // Delete "...\<product>\Application\<version>" Done. https://codereview.chromium.org/11412015/diff/14001/chrome/installer/setup/un... chrome/installer/setup/uninstall.cc:1390: while (to_delete != target_path) { On 2012/11/23 21:00:55, grt wrote: > isn't the loop overkill? it looks like the version dir is the one and only dir > that this loop processes. I guess I was trying not to code that dependency in here. Nothing else in this function would break if that hierarchy were suddenly deeper. But, then again, the world would break if `dirname $SETUP_EXE`/../../chrome.exe didn't work anymore.
lgtm https://codereview.chromium.org/11412015/diff/14001/chrome/installer/setup/un... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/11412015/diff/14001/chrome/installer/setup/un... chrome/installer/setup/uninstall.cc:1353: const FilePath setup_exe = cmd_line.GetProgram(); On 2012/11/29 07:40:36, erikwright wrote: > On 2012/11/23 21:00:55, grt wrote: > > wdyt about using file_util::AbsolutePath here in case the full path wasn't > used? > > setup.exe could have been run via the command-line with a relative path. > > From my reading of GetCommandLine (which is what > CommandLine::ForCurrentProcess()->GetProgram() is ultimately based on) the OS > will fully-qualify the path: > > http://msdn.microsoft.com/en-us/library/windows/desktop/ms683156%28v=vs.85%29... > > But technically, it says "may", and I suppose AbsolutePath doesn't hurt in > either case. I believe gab@ has run into cases where it isn't fully qualified (when he added the developer install switch to setup.exe), so I think this is a teensy bit more safe.
Looks great, final comments below. https://codereview.chromium.org/11412015/diff/4003/chrome/installer/setup/uni... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/11412015/diff/4003/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:217: VLOG(1) << "Keeping setup.exe due to a remaining " On 2012/11/23 18:54:13, erikwright wrote: > On 2012/11/21 22:55:55, gab wrote: > > Put the log beside *remove_setup = false; to keep logging and action together. > > > > This will need to be in an > > if(*remove_setup) { > > VLOG(...); > > *remove_setup = false; > > } > > block unless you don't care about logging this multiple times. > > The thing is, it's only true if the condition on 211 is not met. I could put it > in an 'else' block to be more explicit, but that's redundant with the return on > 215. Ah ok I see, the logic is essentially: if (dist_type != BrowserDistribution::CHROME_APP_HOST) { VLOG(1) << "Keeping all installer files due to a remaining " << "multi-install product."; *remove_setup = false; *remove_archive = false; return; } *remove_setup = false; VLOG(1) << "Keeping setup.exe due to a remaining " but you put *remove_setup = false at the top to avoid the duplication... I think it's slightly harder to read. What about adding a comment like: // Do not remove setup.exe as there is a remaining multi-install product. above *remove_setup = false; https://codereview.chromium.org/11412015/diff/14001/chrome/installer/setup/un... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/11412015/diff/14001/chrome/installer/setup/un... chrome/installer/setup/uninstall.cc:1353: const FilePath setup_exe = cmd_line.GetProgram(); On 2012/11/30 13:40:30, grt wrote: > On 2012/11/29 07:40:36, erikwright wrote: > > On 2012/11/23 21:00:55, grt wrote: > > > wdyt about using file_util::AbsolutePath here in case the full path wasn't > > used? > > > setup.exe could have been run via the command-line with a relative path. > > > > From my reading of GetCommandLine (which is what > > CommandLine::ForCurrentProcess()->GetProgram() is ultimately based on) the OS > > will fully-qualify the path: > > > > > http://msdn.microsoft.com/en-us/library/windows/desktop/ms683156%2528v=vs.85%... > > > > But technically, it says "may", and I suppose AbsolutePath doesn't hurt in > > either case. > > I believe gab@ has run into cases where it isn't fully qualified (when he added > the developer install switch to setup.exe), so I think this is a teensy bit more > safe. Right, I think it's the path setup.exe was invoked with (i.e., if invoked directly from its directory as "setup.exe --foo --bar" then CommandLine.GetProgram() only returns "setup.exe", not the absolute path). https://codereview.chromium.org/11412015/diff/7004/chrome/installer/setup/uni... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/11412015/diff/7004/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:209: !installer_state.FindProduct(dist_type)) { Does !installer_state.FindProduct(dist_type) here mean that this product is not part of the products currently being uninstalled? Maybe a comment would help clarify the above condition. https://codereview.chromium.org/11412015/diff/7004/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:230: bool RemoveInstallerFiles(const FilePath& install_directory, s/install_directory/installer_directory imo, the install directory is the whole thing whereas the installer directory is what this is representing. https://codereview.chromium.org/11412015/diff/7004/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:248: VLOG(1) << "Deleting install path " << to_delete.value(); s/install/installer (same comment as above). https://codereview.chromium.org/11412015/diff/7004/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:551: const FilePath& installer_path) { s/installer_path/setup_exe to respect naming convention around the installer where *_path usually refers to a directory. Or since you actually only need this to determine the installer_directory, might as well take the installer_directory directly here. (see my comment on lines 565 and 1315 as well for context) https://codereview.chromium.org/11412015/diff/7004/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:565: installer_directory = installer_path.DirName(); This feels weird, isn't this always expected to be true? What about: DCHECK(target_path.IsParent(setup_exe)); installer_directory = setup_exe.DirName(); https://codereview.chromium.org/11412015/diff/7004/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:1315: installer_state, cmd_line.GetProgram()); cmd_line.GetProgram() could be relative path here. This should be handled here or in DeleteChromeFilesAndFolders() imo.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erikwright@chromium.org/11412015/15001
Looks like you missed some of my last comments, mostly nits though so I'll let the commit go through. Have a quick look at the "Ping++" though as that one is slightly above a nit (i.e. could actually cause trouble in some cases). Please address them in a separate CL. Thanks, Gab https://codereview.chromium.org/11412015/diff/7004/chrome/installer/setup/uni... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/11412015/diff/7004/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:209: !installer_state.FindProduct(dist_type)) { On 2012/11/30 15:19:45, gab wrote: > Does !installer_state.FindProduct(dist_type) here mean that this product is not > part of the products currently being uninstalled? Maybe a comment would help > clarify the above condition. Ping. https://codereview.chromium.org/11412015/diff/7004/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:230: bool RemoveInstallerFiles(const FilePath& install_directory, On 2012/11/30 15:19:45, gab wrote: > s/install_directory/installer_directory > > imo, the install directory is the whole thing whereas the installer directory is > what this is representing. Ping. https://codereview.chromium.org/11412015/diff/7004/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:248: VLOG(1) << "Deleting install path " << to_delete.value(); On 2012/11/30 15:19:45, gab wrote: > s/install/installer (same comment as above). Ping. https://codereview.chromium.org/11412015/diff/7004/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:551: const FilePath& installer_path) { On 2012/11/30 15:19:45, gab wrote: > s/installer_path/setup_exe to respect naming convention around the installer > where *_path usually refers to a directory. > > Or since you actually only need this to determine the installer_directory, might > as well take the installer_directory directly here. > > (see my comment on lines 565 and 1315 as well for context) Ping. https://codereview.chromium.org/11412015/diff/7004/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:565: installer_directory = installer_path.DirName(); On 2012/11/30 15:19:45, gab wrote: > This feels weird, isn't this always expected to be true? > > What about: > DCHECK(target_path.IsParent(setup_exe)); > installer_directory = setup_exe.DirName(); Ping. https://codereview.chromium.org/11412015/diff/7004/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:1315: installer_state, cmd_line.GetProgram()); On 2012/11/30 15:19:45, gab wrote: > cmd_line.GetProgram() could be relative path here. > > This should be handled here or in DeleteChromeFilesAndFolders() imo. Ping++
Retried try job too often on win_rel for step(s) browser_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erikwright@chromium.org/11412015/15001
Message was sent while issue was closed.
Change committed as 174543
Message was sent while issue was closed.
Nits addressed in https://codereview.chromium.org/11776003/ . https://codereview.chromium.org/11412015/diff/7004/chrome/installer/setup/uni... File chrome/installer/setup/uninstall.cc (right): https://codereview.chromium.org/11412015/diff/7004/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:209: !installer_state.FindProduct(dist_type)) { On 2012/12/21 21:55:53, gab wrote: > On 2012/11/30 15:19:45, gab wrote: > > Does !installer_state.FindProduct(dist_type) here mean that this product is > not > > part of the products currently being uninstalled? Maybe a comment would help > > clarify the above condition. > > Ping. Done. https://codereview.chromium.org/11412015/diff/7004/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:230: bool RemoveInstallerFiles(const FilePath& install_directory, On 2012/12/21 21:55:53, gab wrote: > On 2012/11/30 15:19:45, gab wrote: > > s/install_directory/installer_directory > > > > imo, the install directory is the whole thing whereas the installer directory > is > > what this is representing. > > Ping. Done. https://codereview.chromium.org/11412015/diff/7004/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:248: VLOG(1) << "Deleting install path " << to_delete.value(); On 2012/12/21 21:55:53, gab wrote: > On 2012/11/30 15:19:45, gab wrote: > > s/install/installer (same comment as above). > > Ping. Done. https://codereview.chromium.org/11412015/diff/7004/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:551: const FilePath& installer_path) { On 2012/12/21 21:55:53, gab wrote: > On 2012/11/30 15:19:45, gab wrote: > > s/installer_path/setup_exe to respect naming convention around the installer > > where *_path usually refers to a directory. > > > > Or since you actually only need this to determine the installer_directory, > might > > as well take the installer_directory directly here. > > > > (see my comment on lines 565 and 1315 as well for context) > > Ping. I chose not to change what is passed in, as that would imply a logic change on line 564. https://codereview.chromium.org/11412015/diff/7004/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:565: installer_directory = installer_path.DirName(); On 2012/11/30 15:19:45, gab wrote: > This feels weird, isn't this always expected to be true? > > What about: > DCHECK(target_path.IsParent(setup_exe)); > installer_directory = setup_exe.DirName(); Not necessarily. For example, if you execute the installer from your build output. I think this particular bit might have been the result of a f2f discussion with Greg. https://codereview.chromium.org/11412015/diff/7004/chrome/installer/setup/uni... chrome/installer/setup/uninstall.cc:1315: installer_state, cmd_line.GetProgram()); On 2012/12/21 21:55:53, gab wrote: > On 2012/11/30 15:19:45, gab wrote: > > cmd_line.GetProgram() could be relative path here. > > > > This should be handled here or in DeleteChromeFilesAndFolders() imo. > > Ping++ Done. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
