Temp dir cleanup:...

Temp dir cleanup:
- use ScopedTempDir in work items that need backup space for rollback
- all work items that need backup space take a parent dir in which they create temp dirs
- use ScopedTempDir in a few other places
- renamed some parameters in certain functions so that the same name is used everywhere

While I was at it, I couldn't help but replace Append(UTF8ToWide(version.GetString())) with the more pleasing AppendASCII(version.GetString())

BUG=70368
TEST=existing tests in installer_util_unittests cover the changes


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=75392

[grt (UTC plus 2), 2011-02-17 18:36:15]

This change doesn't yet fix the temp-on-another-volume bug, but it lays some of
the groundwork.

[tommi (sloooow) - chröme, 2011-02-17 20:00:49]

lgtm

http://codereview.chromium.org/6538025/diff/1/chrome/installer/setup/chrome_f...
File chrome/installer/setup/chrome_frame_quick_enable.cc (right):

http://codereview.chromium.org/6538025/diff/1/chrome/installer/setup/chrome_f...
chrome/installer/setup/chrome_frame_quick_enable.cc:85: LOG(ERROR) << "Failed to
create Temp directory";
I think PLOG would be useful here.  Things that can fail are GetTempPath and
CreateDirectory.

http://codereview.chromium.org/6538025/diff/1/chrome/installer/setup/setup_ma...
File chrome/installer/setup/setup_main.cc (right):

http://codereview.chromium.org/6538025/diff/1/chrome/installer/setup/setup_ma...
chrome/installer/setup/setup_main.cc:153: LOG(ERROR) << "Failed to create Temp
directory";
PLOG

http://codereview.chromium.org/6538025/diff/1/chrome/installer/util/copy_tree...
File chrome/installer/util/copy_tree_work_item.cc (right):

http://codereview.chromium.org/6538025/diff/1/chrome/installer/util/copy_tree...
chrome/installer/util/copy_tree_work_item.cc:75: LOG(ERROR) << "Failed to get
backup path in folder " << temp_dir_.value();
PLOG

http://codereview.chromium.org/6538025/diff/1/chrome/installer/util/copy_tree...
chrome/installer/util/copy_tree_work_item.cc:85: LOG(ERROR) << "failed moving "
<< dest_path_.value()
I think PLOG would also be useful here (MoveFileEx et al)

http://codereview.chromium.org/6538025/diff/1/chrome/installer/util/copy_tree...
chrome/installer/util/copy_tree_work_item.cc:115: FilePath backup =
backup_path_.path().Append(dest_path_.BaseName());
FilePath backup(...);

http://codereview.chromium.org/6538025/diff/1/chrome/installer/util/delete_tr...
File chrome/installer/util/delete_tree_work_item.cc (right):

http://codereview.chromium.org/6538025/diff/1/chrome/installer/util/delete_tr...
chrome/installer/util/delete_tree_work_item.cc:58: FilePath& key_file =
key_paths_[i];
key_file(key_paths[i])

http://codereview.chromium.org/6538025/diff/1/chrome/installer/util/delete_tr...
chrome/installer/util/delete_tree_work_item.cc:59: ScopedTempDir& backup =
key_backup_paths_[i];
ditto

http://codereview.chromium.org/6538025/diff/1/chrome/installer/util/delete_tr...
chrome/installer/util/delete_tree_work_item.cc:133: FilePath& key_file =
key_paths_[i];
dittos

[grt (UTC plus 2), 2011-02-17 20:26:41]

Thanks.  Changes made.

http://codereview.chromium.org/6538025/diff/1/chrome/installer/setup/chrome_f...
File chrome/installer/setup/chrome_frame_quick_enable.cc (right):

http://codereview.chromium.org/6538025/diff/1/chrome/installer/setup/chrome_f...
chrome/installer/setup/chrome_frame_quick_enable.cc:85: LOG(ERROR) << "Failed to
create Temp directory";
On 2011/02/17 20:00:49, tommi wrote:
> I think PLOG would be useful here.  Things that can fail are GetTempPath and
> CreateDirectory.

Great suggestion.  Done here and for all other CreateUnique* calls.

http://codereview.chromium.org/6538025/diff/1/chrome/installer/setup/setup_ma...
File chrome/installer/setup/setup_main.cc (right):

http://codereview.chromium.org/6538025/diff/1/chrome/installer/setup/setup_ma...
chrome/installer/setup/setup_main.cc:153: LOG(ERROR) << "Failed to create Temp
directory";
On 2011/02/17 20:00:49, tommi wrote:
> PLOG

Done.

http://codereview.chromium.org/6538025/diff/1/chrome/installer/util/copy_tree...
File chrome/installer/util/copy_tree_work_item.cc (right):

http://codereview.chromium.org/6538025/diff/1/chrome/installer/util/copy_tree...
chrome/installer/util/copy_tree_work_item.cc:75: LOG(ERROR) << "Failed to get
backup path in folder " << temp_dir_.value();
On 2011/02/17 20:00:49, tommi wrote:
> PLOG

Done.

http://codereview.chromium.org/6538025/diff/1/chrome/installer/util/copy_tree...
chrome/installer/util/copy_tree_work_item.cc:85: LOG(ERROR) << "failed moving "
<< dest_path_.value()
On 2011/02/17 20:00:49, tommi wrote:
> I think PLOG would also be useful here (MoveFileEx et al)

Agreed, but I've found through experimentation that it doesn't work.  If
MoveFileEx fails, the subsequent call to GetFileAttributes wipes out the
last-error code.  Following that, calls to SHFileOperation don't set the LEC at
all.  It would take some plumbing in file_util_win.cc to make LEC meaningful in
all cases.

http://codereview.chromium.org/6538025/diff/1/chrome/installer/util/copy_tree...
chrome/installer/util/copy_tree_work_item.cc:115: FilePath backup =
backup_path_.path().Append(dest_path_.BaseName());
On 2011/02/17 20:00:49, tommi wrote:
> FilePath backup(...);

Done.

http://codereview.chromium.org/6538025/diff/1/chrome/installer/util/delete_tr...
File chrome/installer/util/delete_tree_work_item.cc (right):

http://codereview.chromium.org/6538025/diff/1/chrome/installer/util/delete_tr...
chrome/installer/util/delete_tree_work_item.cc:58: FilePath& key_file =
key_paths_[i];
On 2011/02/17 20:00:49, tommi wrote:
> key_file(key_paths[i])

Disregarding as per our discussion since key_file is a reference.

http://codereview.chromium.org/6538025/diff/1/chrome/installer/util/delete_tr...
chrome/installer/util/delete_tree_work_item.cc:59: ScopedTempDir& backup =
key_backup_paths_[i];
On 2011/02/17 20:00:49, tommi wrote:
> ditto

Disregarding as per our discussion since key_file is a reference.

http://codereview.chromium.org/6538025/diff/1/chrome/installer/util/delete_tr...
chrome/installer/util/delete_tree_work_item.cc:133: FilePath& key_file =
key_paths_[i];
On 2011/02/17 20:00:49, tommi wrote:
> dittos

Disregarding ditto.

[robertshield, 2011-02-18 13:48:18]

LGTM

http://codereview.chromium.org/6538025/diff/4002/chrome/installer/util/delete...
File chrome/installer/util/delete_tree_work_item.cc (right):

http://codereview.chromium.org/6538025/diff/4002/chrome/installer/util/delete...
chrome/installer/util/delete_tree_work_item.cc:17: template<typename L, typename
R>
This is cool, but seems to exist only because num_key_files_ is defined as a
(signed) ptrdiff_t instead of an (unsigned) size_t. Why not just make it a
size_t and avoid the hassle? Is there a practical case where that would cause a
problem? (I have the looming feeling I'm missing something subtle here ;))

[grt (UTC plus 2), 2011-02-18 14:12:31]

Thanks!

http://codereview.chromium.org/6538025/diff/4002/chrome/installer/util/delete...
File chrome/installer/util/delete_tree_work_item.cc (right):

http://codereview.chromium.org/6538025/diff/4002/chrome/installer/util/delete...
chrome/installer/util/delete_tree_work_item.cc:17: template<typename L, typename
R>
On 2011/02/18 13:48:18, robertshield wrote:
> This is cool, but seems to exist only because num_key_files_ is defined as a
> (signed) ptrdiff_t instead of an (unsigned) size_t. Why not just make it a
> size_t and avoid the hassle? Is there a practical case where that would cause
a
> problem? (I have the looming feeling I'm missing something subtle here ;))

Your cool is my annoying.  You're right about the reason it exists.  The
subtlety with using size_t for storing the size is that scoped_array's
subscripting operator takes a ptrdiff_t, so casting needs to be done in one
place or the other.  I thought it was a bit cleaner to do the size_t ->
ptrdiff_t conversion only once early on rather than in the places where items in
the scoped_array are accessed.

[tommi (sloooow) - chröme, 2011-02-18 16:43:54]

lgtm++

I think "Your cool is my annoying - grt" is going to be my new email
signature XD

On Fri, Feb 18, 2011 at 9:12 AM, <grt@chromium.org> wrote:

> Thanks!
>
>
>
>
>
http://codereview.chromium.org/6538025/diff/4002/chrome/installer/util/delete...
> File chrome/installer/util/delete_tree_work_item.cc (right):
>
>
>
http://codereview.chromium.org/6538025/diff/4002/chrome/installer/util/delete...
> chrome/installer/util/delete_tree_work_item.cc:17: template<typename L,
> typename R>
> On 2011/02/18 13:48:18, robertshield wrote:
>
>> This is cool, but seems to exist only because num_key_files_ is
>>
> defined as a
>
>> (signed) ptrdiff_t instead of an (unsigned) size_t. Why not just make
>>
> it a
>
>> size_t and avoid the hassle? Is there a practical case where that
>>
> would cause a
>
>> problem? (I have the looming feeling I'm missing something subtle here
>>
> ;))
>
> Your cool is my annoying.  You're right about the reason it exists.  The
> subtlety with using size_t for storing the size is that scoped_array's
> subscripting operator takes a ptrdiff_t, so casting needs to be done in
> one place or the other.  I thought it was a bit cleaner to do the size_t
> -> ptrdiff_t conversion only once early on rather than in the places
> where items in the scoped_array are accessed.
>
>
> http://codereview.chromium.org/6538025/
>