Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10832210:
Delete the installation folder after deleting its contents. (Closed)
|
Created:
8 years, 4 months ago by erikwright (departed) Modified:
8 years, 4 months ago Reviewers:
gab CC:
chromium-reviews, grt+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionDelete the installation folder after deleting its contents.
R=gab
BUG=138615
TEST=setup.exe --multi-install --chrome; setup.exe --multi-install --uninstall --chrome; (check 'Delete user profile' option); verify C:/Users/<you>/AppData/Local/Google/Chrome is gone.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=151354
Patch Set 1 #
Total comments: 9
Patch Set 2 : Review comments. #
Total comments: 4
Patch Set 3 : Review Comments. #
Total comments: 14
Patch Set 4 : Review comments. #Patch Set 5 : Review comments. #
Total comments: 3
Messages
Total messages: 12 (0 generated)
lg, please update description with TEST= section. https://chromiumcodereview.appspot.com/10832210/diff/1/chrome/installer/setup... File chrome/installer/setup/uninstall.cc (right): https://chromiumcodereview.appspot.com/10832210/diff/1/chrome/installer/setup... chrome/installer/setup/uninstall.cc:448: } else if (!target_path.empty() && file_util::IsDirectoryEmpty(target_path)) { I don't think target_path.empty() is possible at this point, remove this check. https://chromiumcodereview.appspot.com/10832210/diff/1/chrome/installer/setup... chrome/installer/setup/uninstall.cc:515: // Delete the Application directory at reboot if empty. This function does not seem to wait for reboot for deletion (i.e. it doesn't specify MOVEFILE_DELAY_UNTIL_REBOOT as per http://msdn.microsoft.com/en-us/library/windows/desktop/aa365240(v=vs.85).aspx). Please reword comment accordingly. https://chromiumcodereview.appspot.com/10832210/diff/1/chrome/installer/setup... chrome/installer/setup/uninstall.cc:517: // If we need a reboot to continue, schedule the parent directories for nit: add empty line between code and comment https://chromiumcodereview.appspot.com/10832210/diff/1/chrome/installer/setup... chrome/installer/setup/uninstall.cc:524: LOG(ERROR) << "Failed to delete folder: " << target_path.value(); Seems this log message is duplicated all over this file, I don't know if the compiler is smart enough to consolidate those in a single data entry in the binary... otherwise that's a lot of data for the same string... Furthermore this logic is duplicated all over this file, does changing DeleteEmptyParentDir() to something that does both make sense to you?
PTAL. https://chromiumcodereview.appspot.com/10832210/diff/1/chrome/installer/setup... File chrome/installer/setup/uninstall.cc (right): https://chromiumcodereview.appspot.com/10832210/diff/1/chrome/installer/setup... chrome/installer/setup/uninstall.cc:448: } else if (!target_path.empty() && file_util::IsDirectoryEmpty(target_path)) { On 2012/08/08 20:32:59, gab wrote: > I don't think target_path.empty() is possible at this point, remove this check. It may be empty if both Chrome and App Host were installed in the same directory but only App Host is uninstalled. https://chromiumcodereview.appspot.com/10832210/diff/1/chrome/installer/setup... chrome/installer/setup/uninstall.cc:515: // Delete the Application directory at reboot if empty. On 2012/08/08 20:32:59, gab wrote: > This function does not seem to wait for reboot for deletion (i.e. it doesn't > specify MOVEFILE_DELAY_UNTIL_REBOOT as per > http://msdn.microsoft.com/en-us/library/windows/desktop/aa365240%28v=vs.85%29...). > > Please reword comment accordingly. This is the implementation of ScheduleFileSystemEntityForDeletion: http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/installer/util/dele... It does specify that flag. I think the comment is then correct? https://chromiumcodereview.appspot.com/10832210/diff/1/chrome/installer/setup... chrome/installer/setup/uninstall.cc:517: // If we need a reboot to continue, schedule the parent directories for On 2012/08/08 20:32:59, gab wrote: > nit: add empty line between code and comment Done. https://chromiumcodereview.appspot.com/10832210/diff/1/chrome/installer/setup... chrome/installer/setup/uninstall.cc:524: LOG(ERROR) << "Failed to delete folder: " << target_path.value(); On 2012/08/08 20:32:59, gab wrote: > Seems this log message is duplicated all over this file, I don't know if the > compiler is smart enough to consolidate those in a single data entry in the > binary... otherwise that's a lot of data for the same string... > > Furthermore this logic is duplicated all over this file, does changing > DeleteEmptyParentDir() to something that does both make sense to you? I thought about it, but it's also used in DeleteLocalState, where only two directories are to be deleted. String literals are folded during compilation. Please see if the new patchset is more appealing.
comments below. http://codereview.chromium.org/10832210/diff/1/chrome/installer/setup/uninsta... File chrome/installer/setup/uninstall.cc (right): http://codereview.chromium.org/10832210/diff/1/chrome/installer/setup/uninsta... chrome/installer/setup/uninstall.cc:515: // Delete the Application directory at reboot if empty. On 2012/08/09 14:52:51, erikwright wrote: > On 2012/08/08 20:32:59, gab wrote: > > This function does not seem to wait for reboot for deletion (i.e. it doesn't > > specify MOVEFILE_DELAY_UNTIL_REBOOT as per > > > http://msdn.microsoft.com/en-us/library/windows/desktop/aa365240%2528v=vs.85%...). > > > > Please reword comment accordingly. > > This is the implementation of ScheduleFileSystemEntityForDeletion: > http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/installer/util/dele... > > It does specify that flag. I think the comment is then correct? Indeed :), guess I missed that line when reading the implemenation yesterday, sorry about that! http://codereview.chromium.org/10832210/diff/5001/chrome/installer/setup/unin... File chrome/installer/setup/uninstall.cc (right): http://codereview.chromium.org/10832210/diff/5001/chrome/installer/setup/unin... chrome/installer/setup/uninstall.cc:323: bool DeleteEmptyDir(const FilePath& path, bool* error) { I'm not a fan of the bool return and bool* error param. Could you remove |error| and make the return an enum? edit: My desire to remove |error| is even stronger as I see its value is not even used in every scenario below. Maybe add DELETE_INVALID or something to the DeleteValue enum? http://codereview.chromium.org/10832210/diff/5001/chrome/installer/setup/unin... chrome/installer/setup/uninstall.cc:329: if (!file_util::IsDirectoryEmpty(path)) Group this condition with path.empty() above since the result (return false) is the same.
PTAL. http://codereview.chromium.org/10832210/diff/5001/chrome/installer/setup/unin... File chrome/installer/setup/uninstall.cc (right): http://codereview.chromium.org/10832210/diff/5001/chrome/installer/setup/unin... chrome/installer/setup/uninstall.cc:323: bool DeleteEmptyDir(const FilePath& path, bool* error) { On 2012/08/09 16:07:22, gab wrote: > I'm not a fan of the bool return and bool* error param. > > Could you remove |error| and make the return an enum? > > edit: My desire to remove |error| is even stronger as I see its value is not > even used in every scenario below. > > Maybe add DELETE_INVALID or something to the DeleteValue enum? Done. http://codereview.chromium.org/10832210/diff/5001/chrome/installer/setup/unin... chrome/installer/setup/uninstall.cc:329: if (!file_util::IsDirectoryEmpty(path)) On 2012/08/09 16:07:22, gab wrote: > Group this condition with path.empty() above since the result (return false) is > the same. N/A.
looking good, see comments below. http://codereview.chromium.org/10832210/diff/8001/chrome/installer/setup/unin... File chrome/installer/setup/uninstall.cc (right): http://codereview.chromium.org/10832210/diff/8001/chrome/installer/setup/unin... chrome/installer/setup/uninstall.cc:398: FilePath user_data_dir = local_state_folders[0].DirName(); nit: As per Chromium style, initialize non-POD types using '()' rather than '='. (and make it 'const') http://codereview.chromium.org/10832210/diff/8001/chrome/installer/setup/unin... chrome/installer/setup/uninstall.cc:399: if (!user_data_dir.empty() && why do you need to first check if it isn't empty? (and even if you do I feel the current conditional is wrong and should be the opposite, i.e. check that it IS empty... no?) http://codereview.chromium.org/10832210/diff/8001/chrome/installer/setup/unin... chrome/installer/setup/uninstall.cc:400: DELETE_SUCCEEDED == DeleteEmptyDir(user_data_dir)) { nit: put constant on RHS of == http://codereview.chromium.org/10832210/diff/8001/chrome/installer/setup/unin... chrome/installer/setup/uninstall.cc:401: FilePath product_dir = user_data_dir.DirName(); nit: initialization style as above optional: I would personally prefer to have this inlined below. http://codereview.chromium.org/10832210/diff/8001/chrome/installer/setup/unin... chrome/installer/setup/uninstall.cc:433: DeleteResult DeleteApplicationProductAndVendorDirectories( Wouldn't it make more sense to simply delete all parents in the hierarchy iteratively until one of them is not empty? Or take a number of parent directories to delete and expect to be able to delete that amount iteratively (or return the number deleted and make that check caller side)? http://codereview.chromium.org/10832210/diff/8001/chrome/installer/setup/unin... chrome/installer/setup/uninstall.cc:439: FilePath product_directory = application_directory.DirName(); nit: initialization style as above http://codereview.chromium.org/10832210/diff/8001/chrome/installer/setup/unin... chrome/installer/setup/uninstall.cc:440: if (!product_directory.empty()) { As above I don't see the need for this check and feel it is backwards if anything...
PTAL. http://codereview.chromium.org/10832210/diff/8001/chrome/installer/setup/unin... File chrome/installer/setup/uninstall.cc (right): http://codereview.chromium.org/10832210/diff/8001/chrome/installer/setup/unin... chrome/installer/setup/uninstall.cc:398: FilePath user_data_dir = local_state_folders[0].DirName(); On 2012/08/13 18:20:15, gab wrote: > nit: As per Chromium style, initialize non-POD types using '()' rather than '='. > > (and make it 'const') Done. http://codereview.chromium.org/10832210/diff/8001/chrome/installer/setup/unin... chrome/installer/setup/uninstall.cc:399: if (!user_data_dir.empty() && On 2012/08/13 18:20:15, gab wrote: > why do you need to first check if it isn't empty? (and even if you do I feel the > current conditional is wrong and should be the opposite, i.e. check that it IS > empty... no?) Note that this is querying the path string and not the filesystem. http://codereview.chromium.org/10832210/diff/8001/chrome/installer/setup/unin... chrome/installer/setup/uninstall.cc:400: DELETE_SUCCEEDED == DeleteEmptyDir(user_data_dir)) { On 2012/08/13 18:20:15, gab wrote: > nit: put constant on RHS of == Done. http://codereview.chromium.org/10832210/diff/8001/chrome/installer/setup/unin... chrome/installer/setup/uninstall.cc:401: FilePath product_dir = user_data_dir.DirName(); On 2012/08/13 18:20:15, gab wrote: > nit: initialization style as above Done. > > optional: I would personally prefer to have this inlined below. I think labeling it has merit. Also, now it's used twice. http://codereview.chromium.org/10832210/diff/8001/chrome/installer/setup/unin... chrome/installer/setup/uninstall.cc:433: DeleteResult DeleteApplicationProductAndVendorDirectories( On 2012/08/13 18:20:15, gab wrote: > Wouldn't it make more sense to simply delete all parents in the hierarchy > iteratively until one of them is not empty? No. It seems wrong to attempt to delete AppData or AppData/Local even if we do first check that they're empty. > > Or take a number of parent directories to delete and expect to be able to delete > that amount iteratively (or return the number deleted and make that check caller > side)? Seems better to me to explicitly state in code what directories we are attempting to delete, rather than having mysterious literals like '2' and '3'. http://codereview.chromium.org/10832210/diff/8001/chrome/installer/setup/unin... chrome/installer/setup/uninstall.cc:439: FilePath product_directory = application_directory.DirName(); On 2012/08/13 18:20:15, gab wrote: > nit: initialization style as above Done. http://codereview.chromium.org/10832210/diff/8001/chrome/installer/setup/unin... chrome/installer/setup/uninstall.cc:440: if (!product_directory.empty()) { On 2012/08/13 18:20:15, gab wrote: > As above I don't see the need for this check and feel it is backwards if > anything... ::empty() is querying the path string, not the filesystem.
LGTM w/ a single nit below. Thanks! Gab http://codereview.chromium.org/10832210/diff/2003/chrome/installer/setup/unin... File chrome/installer/setup/uninstall.cc (right): http://codereview.chromium.org/10832210/diff/2003/chrome/installer/setup/unin... chrome/installer/setup/uninstall.cc:433: DeleteResult result(DeleteEmptyDir(application_directory)); nit: DeleteResult is a POD (i.e. enum), I think the Chromium style calls for '=' initialization in this case.
Thank you! http://codereview.chromium.org/10832210/diff/2003/chrome/installer/setup/unin... File chrome/installer/setup/uninstall.cc (right): http://codereview.chromium.org/10832210/diff/2003/chrome/installer/setup/unin... chrome/installer/setup/uninstall.cc:433: DeleteResult result(DeleteEmptyDir(application_directory)); On 2012/08/13 19:33:54, gab wrote: > nit: DeleteResult is a POD (i.e. enum), I think the Chromium style calls for '=' > initialization in this case. Found no mention of this for PODS or non-PODS. The Google style guide does mention that you may use either style. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Variab... I know grt prefers () at least for non-POD, but I'm pretty sure for POD as well.
http://codereview.chromium.org/10832210/diff/2003/chrome/installer/setup/unin... File chrome/installer/setup/uninstall.cc (right): http://codereview.chromium.org/10832210/diff/2003/chrome/installer/setup/unin... chrome/installer/setup/uninstall.cc:433: DeleteResult result(DeleteEmptyDir(application_directory)); On 2012/08/13 19:37:37, erikwright wrote: > On 2012/08/13 19:33:54, gab wrote: > > nit: DeleteResult is a POD (i.e. enum), I think the Chromium style calls for > '=' > > initialization in this case. > > Found no mention of this for PODS or non-PODS. The Google style guide does > mention that you may use either style. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Variab... > > I know grt prefers () at least for non-POD, but I'm pretty sure for POD as well. Ah interesting, maybe we should define "grt-style" :)... seriously though my thinking behind it is that a POD initialization is really a simple assignment where as a non-POD involves a constructor so that I like to keep it clear it's a function call.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erikwright@chromium.org/10832210/2003
Change committed as 151354 |
