Renames the function CreateTemporaryFilename to CreateTemporaryFile and track...

Renames the function CreateTemporaryFilename to CreateTemporaryFile and track down all callers, also removes the
deprecated function that uses std::wstring.

BUG=3078
(http://crbug.com/3078)
TEST=run base_unittests, installer_util_unittests, net_unittests, setup_unittests, and unit_tests.

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

[tfarina (gmail-do not use), 2009-08-14 00:29:42]

http://codereview.chromium.org/164537/diff/1/15
File chrome/installer/setup/setup_util_unittest.cc (right):

http://codereview.chromium.org/164537/diff/1/15#newcode89
Line 89: FilePath program;
Please disconsider this change. It is from my other patch that is under review
yet.
http://codereview.chromium.org/165213/diff/195/216

[Erik does not do reviews, 2009-08-14 14:50:05]

http://codereview.chromium.org/164537/diff/1/7
File base/file_util.h (right):

http://codereview.chromium.org/164537/diff/1/7#newcode299
Line 299: bool CreateTemporaryFileNameInDir(const std::wstring& dir,
this should be renamed then as well

http://codereview.chromium.org/164537/diff/1/8
File base/file_util_posix.cc (right):

http://codereview.chromium.org/164537/diff/1/8#newcode419
Line 419: // Not implemented yet.
Add TODO(port), also note that CreateTemporaryFile can be pulled into common
code once this is implemented.  (see windows implementation)

http://codereview.chromium.org/164537/diff/1/9
File base/file_util_unittest.cc (right):

http://codereview.chromium.org/164537/diff/1/9#newcode780
Line 780: TEST_F(FileUtilTest, CreateTemporaryFileNameTest) {
rename the test

[tfarina (gmail-do not use), 2009-08-14 15:53:19]

http://codereview.chromium.org/164537/diff/1/7
File base/file_util.h (right):

http://codereview.chromium.org/164537/diff/1/7#newcode299
Line 299: bool CreateTemporaryFileNameInDir(const std::wstring& dir,
On 2009/08/14 14:50:05, Erik Kay wrote:
> this should be renamed then as well

Done.

http://codereview.chromium.org/164537/diff/1/8
File base/file_util_posix.cc (right):

http://codereview.chromium.org/164537/diff/1/8#newcode419
Line 419: // Not implemented yet.
On 2009/08/14 14:50:05, Erik Kay wrote:
> Add TODO(port), also note that CreateTemporaryFile can be pulled into common
> code once this is implemented.  (see windows implementation)

Done.

http://codereview.chromium.org/164537/diff/1/9
File base/file_util_unittest.cc (right):

http://codereview.chromium.org/164537/diff/1/9#newcode780
Line 780: TEST_F(FileUtilTest, CreateTemporaryFileNameTest) {
On 2009/08/14 14:50:05, Erik Kay wrote:
> rename the test
> 

Done.

[Erik does not do reviews, 2009-08-14 16:14:10]

http://codereview.chromium.org/164537/diff/40/1008
File base/file_util.h (right):

http://codereview.chromium.org/164537/diff/40/1008#newcode299
Line 299: bool CreateTemporaryFileInDir(const std::wstring& dir,
Sorry, also add a TODO to change this to FilePath or just do it with this CL.

[tfarina (gmail-do not use), 2009-08-14 17:10:49]

http://codereview.chromium.org/164537/diff/40/1008
File base/file_util.h (right):

http://codereview.chromium.org/164537/diff/40/1008#newcode299
Line 299: bool CreateTemporaryFileInDir(const std::wstring& dir,
On 2009/08/14 16:14:10, Erik Kay wrote:
> Sorry, also add a TODO to change this to FilePath or just do it with this CL.
I tried to fix it.

[Erik does not do reviews, 2009-08-14 17:25:33]

I'm surprised the installer unit tests didn't catch this.  Maybe there's a
missing test case here.

Rahul?

http://codereview.chromium.org/164537/diff/1041/1060
File chrome/installer/util/move_tree_work_item.cc (right):

http://codereview.chromium.org/164537/diff/1041/1060#newcode37
Line 37: &FilePath(backup_path_))) {
This is wrong.  The original code winds up assigning to backup_path_ which is
then used below.  This just assigns to a random object that goes away at the end
of the function call.

[kuchhal, 2009-08-14 17:43:32]

This should have been caught by MoveDirectoryDestExists test in
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/installer/util/move_tr...
<http://src.chromium.org/viewvc/chrome/trunk/src/chrome/installer/util/move_tr...
you run installer_util_unittests.exe with your changes?

On Fri, Aug 14, 2009 at 10:25 AM, <erikkay@chromium.org> wrote:

> I'm surprised the installer unit tests didn't catch this.  Maybe there's
> a missing test case here.
>
> Rahul?
>
>
>
> http://codereview.chromium.org/164537/diff/1041/1060
> File chrome/installer/util/move_tree_work_item.cc (right):
>
> http://codereview.chromium.org/164537/diff/1041/1060#newcode37
> Line 37: &FilePath(backup_path_))) {
> This is wrong.  The original code winds up assigning to backup_path_
> which is then used below.  This just assigns to a random object that
> goes away at the end of the function call.
>
>
> http://codereview.chromium.org/164537
>

[tfarina (gmail-do not use), 2009-08-14 17:57:38]

On 2009/08/14 17:43:32, kuchhal wrote:
> This should have been caught by MoveDirectoryDestExists test in
>
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/installer/util/move_tr...
>
<http://src.chromium.org/viewvc/chrome/trunk/src/chrome/installer/util/move_tr...
> you run installer_util_unittests.exe with your changes?
Sorry, I forgot to run this test.
> 
> On Fri, Aug 14, 2009 at 10:25 AM, <mailto:erikkay@chromium.org> wrote:
> 
> > I'm surprised the installer unit tests didn't catch this.  Maybe there's
> > a missing test case here.
> >
> > Rahul?
> >
> >
> >
> > http://codereview.chromium.org/164537/diff/1041/1060
> > File chrome/installer/util/move_tree_work_item.cc (right):
> >
> > http://codereview.chromium.org/164537/diff/1041/1060#newcode37
> > Line 37: &FilePath(backup_path_))) {
> > This is wrong.  The original code winds up assigning to backup_path_
> > which is then used below.  This just assigns to a random object that
> > goes away at the end of the function call.
> >
> >
> > http://codereview.chromium.org/164537
> >
>

[tfarina (gmail-do not use), 2009-08-14 17:58:10]

http://codereview.chromium.org/164537/diff/1041/1060
File chrome/installer/util/move_tree_work_item.cc (right):

http://codereview.chromium.org/164537/diff/1041/1060#newcode37
Line 37: &FilePath(backup_path_))) {
On 2009/08/14 17:25:34, Erik Kay wrote:
> This is wrong.  The original code winds up assigning to backup_path_ which is
> then used below.  This just assigns to a random object that goes away at the
end
> of the function call.
> 

Done.

[kuchhal, 2009-08-14 18:01:00]

Did you run the tests this time?

On Fri, Aug 14, 2009 at 10:58 AM, <thiago.farina@gmail.com> wrote:

>
> http://codereview.chromium.org/164537/diff/1041/1060
> File chrome/installer/util/move_tree_work_item.cc (right):
>
> http://codereview.chromium.org/164537/diff/1041/1060#newcode37
> Line 37: &FilePath(backup_path_))) {
> On 2009/08/14 17:25:34, Erik Kay wrote:
>
>> This is wrong.  The original code winds up assigning to backup_path_
>>
> which is
>
>> then used below.  This just assigns to a random object that goes away
>>
> at the end
>
>> of the function call.
>>
>
>
> Done.
>
>
> http://codereview.chromium.org/164537
>

[Erik does not do reviews, 2009-08-14 18:01:11]

http://codereview.chromium.org/164537/diff/1071/89
File chrome/installer/util/move_tree_work_item.cc (right):

http://codereview.chromium.org/164537/diff/1071/89#newcode33
Line 33: FilePath backup_path = FilePath(backup_path_);
sorry, but this still doesn't do what it used to do.  The code in the method
works, but the cleanup code in the destructor won't work because backup_path_
wasn't actually assigned to.

[tfarina (gmail-do not use), 2009-08-15 00:10:08]

I ran all the tests required.

http://codereview.chromium.org/164537/diff/1071/89
File chrome/installer/util/move_tree_work_item.cc (right):

http://codereview.chromium.org/164537/diff/1071/89#newcode33
Line 33: FilePath backup_path = FilePath(backup_path_);
On 2009/08/14 18:01:11, Erik Kay wrote:
> sorry, but this still doesn't do what it used to do.  The code in the method
> works, but the cleanup code in the destructor won't work because backup_path_
> wasn't actually assigned to.

Done.

[tfarina (gmail-do not use), 2009-08-15 19:47:46]

Hi Erik,

can you see if move_tree_work_item.cc is it right?

[Erik does not do reviews, 2009-08-17 16:17:12]

LGTM

http://codereview.chromium.org/164537/diff/2001/2020
File chrome/installer/util/move_tree_work_item.cc (right):

http://codereview.chromium.org/164537/diff/2001/2020#newcode52
Line 52: << " to " << backup_path.value();
this can just be backup_path_ again

[tfarina (gmail-do not use), 2009-08-17 17:12:33]

http://codereview.chromium.org/164537/diff/2001/2020
File chrome/installer/util/move_tree_work_item.cc (right):

http://codereview.chromium.org/164537/diff/2001/2020#newcode52
Line 52: << " to " << backup_path.value();
On 2009/08/17 16:17:12, Erik Kay wrote:
> this can just be backup_path_ again

Done.

[Erik does not do reviews, 2009-08-17 17:53:53]

Compile failure on Linux and Mac:
http://build.chromium.org/buildbot/try-server/builders/linux/builds/13358/ste...

Erik


On Mon, Aug 17, 2009 at 10:12 AM, <thiago.farina@gmail.com> wrote:
>
> http://codereview.chromium.org/164537/diff/2001/2020
> File chrome/installer/util/move_tree_work_item.cc (right):
>
> http://codereview.chromium.org/164537/diff/2001/2020#newcode52
> Line 52: << " to " << backup_path.value();
> On 2009/08/17 16:17:12, Erik Kay wrote:
>>
>> this can just be backup_path_ again
>
> Done.
>
> http://codereview.chromium.org/164537
>

[tfarina (gmail-do not use), 2009-08-17 18:46:28]

Fixed!

[Erik does not do reviews, 2009-08-17 20:24:27]

Windows failure now:
http://build.chromium.org/buildbot/try-server/builders/win/builds/14342/steps...


On Mon, Aug 17, 2009 at 11:46 AM, <thiago.farina@gmail.com> wrote:
> Fixed!
>
> http://codereview.chromium.org/164537
>

[tfarina (gmail-do not use), 2009-08-17 21:38:01]

Removed CommandLine issue from the CL.