CreateDirectory() should check if an existing path is actually a directory...

CreateDirectory() should check if an existing path is actually a directory before skipping it. Also update a couple instances and comments to reflect current behaviour (see also http://codereview.chromium.org/1681).

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

[Erik does not do reviews, 2008-09-10 21:33:09]

http://codereview.chromium.org/1709/diff/214/11
File base/file_util_unittest.cc (right):

http://codereview.chromium.org/1709/diff/214/11#newcode711
Line 711: TEST_F(FileUtilTest, CreateDirectoryTest) {
Since you changed CreateDirectory to return true for existing directories,
please extend this test to exercise that code path.

http://codereview.chromium.org/1709/diff/214/11#newcode734
Line 734: EXPECT_EQ(file_util::PathExists(test_root), true);
add a DirectoryExists test here too

http://codereview.chromium.org/1709/diff/214/11#newcode745
Line 745: // Check a directory
With the addition of the DirectoryExists test above, I don't think these tests
exercise any new code paths.

http://codereview.chromium.org/1709/diff/214/8
File base/file_util_win.cc (right):

http://codereview.chromium.org/1709/diff/214/8#newcode183
Line 183: }
remove unneeded {}

[Michael Moss, 2008-09-10 22:04:48]

http://codereview.chromium.org/1709/diff/214/11
File base/file_util_unittest.cc (right):

http://codereview.chromium.org/1709/diff/214/11#newcode711
Line 711: TEST_F(FileUtilTest, CreateDirectoryTest) {
On 2008/09/10 21:33:09, Erik Kay wrote:
> Since you changed CreateDirectory to return true for existing directories,
> please extend this test to exercise that code path.

Done. Also added test to make sure CreateDirectory fails when given an existing
file (i.e. non-dir), which incorrectly passed before.

http://codereview.chromium.org/1709/diff/214/11#newcode734
Line 734: EXPECT_EQ(file_util::PathExists(test_root), true);
On 2008/09/10 21:33:09, Erik Kay wrote:
> add a DirectoryExists test here too

Done.

http://codereview.chromium.org/1709/diff/214/11#newcode745
Line 745: // Check a directory
On 2008/09/10 21:33:09, Erik Kay wrote:
> With the addition of the DirectoryExists test above, I don't think these tests
> exercise any new code paths.

Done.

http://codereview.chromium.org/1709/diff/214/8
File base/file_util_win.cc (right):

http://codereview.chromium.org/1709/diff/214/8#newcode183
Line 183: }
On 2008/09/10 21:33:09, Erik Kay wrote:
> remove unneeded {}

Done.

[Erik does not do reviews, 2008-09-10 22:18:27]

one minor nit, otherwise LGTM

http://codereview.chromium.org/1709/diff/23/219
File base/file_util_unittest.cc (right):

http://codereview.chromium.org/1709/diff/23/219#newcode724
Line 724: // Works to "create" it again.
Change comment for clarity:
"CreateDirectory returns true if the DirectoryExists returns true"

[Michael Moss, 2008-09-10 22:23:15]

http://codereview.chromium.org/1709/diff/23/219
File base/file_util_unittest.cc (right):

http://codereview.chromium.org/1709/diff/23/219#newcode724
Line 724: // Works to "create" it again.
On 2008/09/10 22:18:27, Erik Kay wrote:
> Change comment for clarity:
> "CreateDirectory returns true if the DirectoryExists returns true"

Done.

[M-A Ruel, 2008-09-10 23:49:04]

lgtm

http://codereview.chromium.org/1709/diff/233/237
File base/file_util_unittest.cc (right):

http://codereview.chromium.org/1709/diff/233/237#newcode732
Line 732: EXPECT_EQ(file_util::CreateDirectory(test_path), false);
I wonder why it's not EXPECT_TRUE and EXPECT_FALSE. Furthermore the expected
argument is the first one, not the second. But I digress.

[Michael Moss, 2008-09-11 15:55:49]

http://codereview.chromium.org/1709/diff/233/237
File base/file_util_unittest.cc (right):

http://codereview.chromium.org/1709/diff/233/237#newcode732
Line 732: EXPECT_EQ(file_util::CreateDirectory(test_path), false);
On 2008/09/10 23:49:05, M-A wrote:
> I wonder why it's not EXPECT_TRUE and EXPECT_FALSE. Furthermore the expected
> argument is the first one, not the second. But I digress.

Good point. I went ahead and updated all those instances to to use
EXPECT_TRUE/FALSE.

[M-A Ruel, 2008-09-11 15:59:54]

Thanks.