Make ScopedTempDir::Delete return a bool so that its functionality can be te...

Make ScopedTempDir::Delete return a bool so that its functionality can be tested.

TEST=Run the new unit test: ScopedTempDir.LockedTempDir
BUG=none


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

[tommi (sloooow) - chröme, 2010-11-26 20:41:19]

it's -> its

On Fri, Nov 26, 2010 at 3:40 PM, <tommi@chromium.org> wrote:

> Reviewers: Paweł Hajdan Jr.,
>
> Description:
> Make ScopedTempDir::Delete return a bool so that it's functionality can be
> tested.
>
> TEST=Run the new unit test: ScopedTempDir.LockedTempDir
> BUG=none
>
>
> Please review this at http://codereview.chromium.org/5340004/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src/
>
> Affected files:
>  M     base/scoped_temp_dir.h
>  M     base/scoped_temp_dir.cc
>  M     base/scoped_temp_dir_unittest.cc
>
>
> Index: base/scoped_temp_dir.cc
> ===================================================================
> --- base/scoped_temp_dir.cc     (revision 67430)
> +++ base/scoped_temp_dir.cc     (working copy)
> @@ -57,10 +57,18 @@
>   return true;
>  }
>
> -void ScopedTempDir::Delete() {
> -  if (!path_.empty() && !file_util::Delete(path_, true))
> -    LOG(ERROR) << "ScopedTempDir unable to delete " << path_.value();
> -  path_.clear();
> +bool ScopedTempDir::Delete() {
> +  bool ret = false;
> +  if (!path_.empty()) {
> +    ret = file_util::Delete(path_, true);
> +    if (!ret) {
> +      // Note that we don't clear the path if we can't delete the
> directory.
> +      LOG(ERROR) << "ScopedTempDir unable to delete " << path_.value();
> +    } else {
> +      path_.clear();
> +    }
> +  }
> +  return ret;
>  }
>
>  FilePath ScopedTempDir::Take() {
> Index: base/scoped_temp_dir.h
> ===================================================================
> --- base/scoped_temp_dir.h      (revision 67430)
> +++ base/scoped_temp_dir.h      (working copy)
> @@ -38,7 +38,10 @@
>   bool Set(const FilePath& path);
>
>   // Deletes the temporary directory wrapped by this object.
> -  void Delete();
> +  // Returns true if the directory was successfully deleted.
> +  // If the current path is empty or if the folder can't be deleted
> +  // for some reason, the return value is false.
> +  bool Delete();
>
>   // Caller takes ownership of the temporary directory so it won't be
> destroyed
>   // when this object goes out of scope.
> Index: base/scoped_temp_dir_unittest.cc
> ===================================================================
> --- base/scoped_temp_dir_unittest.cc    (revision 67430)
> +++ base/scoped_temp_dir_unittest.cc    (working copy)
> @@ -3,6 +3,7 @@
>  // found in the LICENSE file.
>
>  #include "base/file_util.h"
> +#include "base/platform_file.h"
>  #include "base/scoped_temp_dir.h"
>  #include "testing/gtest/include/gtest/gtest.h"
>
> @@ -87,3 +88,20 @@
>   EXPECT_FALSE(dir.CreateUniqueTempDir());
>   EXPECT_FALSE(other_dir.CreateUniqueTempDir());
>  }
> +
> +TEST(ScopedTempDir, LockedTempDir) {
> +  ScopedTempDir dir;
> +  EXPECT_TRUE(dir.CreateUniqueTempDir());
> +  int file_flags = base::PLATFORM_FILE_CREATE_ALWAYS |
> +                   base::PLATFORM_FILE_WRITE;
> +  base::PlatformFileError error_code = base::PLATFORM_FILE_OK;
> +  base::PlatformFile file = base::CreatePlatformFile(
> +      dir.path().Append(FILE_PATH_LITERAL("temp")), file_flags, NULL,
> +      &error_code);
> +  EXPECT_NE(base::kInvalidPlatformFileValue, file);
> +  EXPECT_EQ(base::PLATFORM_FILE_OK, error_code);
> +  EXPECT_FALSE(dir.Delete());  // We should not be able to delete.
> +  EXPECT_FALSE(dir.path().empty());  // We should still have a valid path.
> +  EXPECT_TRUE(base::ClosePlatformFile(file));
> +  EXPECT_TRUE(dir.Delete());  // Now, we should be able to delete.
> +}
>
>
>

[Paweł Hajdan Jr., 2010-11-26 21:01:20]

http://codereview.chromium.org/5340004/diff/1/base/scoped_temp_dir.cc
File base/scoped_temp_dir.cc (right):

http://codereview.chromium.org/5340004/diff/1/base/scoped_temp_dir.cc#newcode62
base/scoped_temp_dir.cc:62: if (!path_.empty()) {
nit: Rather:

if (path.empty())
  return false;

http://codereview.chromium.org/5340004/diff/1/base/scoped_temp_dir.cc#newcode64
base/scoped_temp_dir.cc:64: if (!ret) {
nit: Put success case first to avoid double-negation for "else".

http://codereview.chromium.org/5340004/diff/1/base/scoped_temp_dir.h
File base/scoped_temp_dir.h (right):

http://codereview.chromium.org/5340004/diff/1/base/scoped_temp_dir.h#newcode42
base/scoped_temp_dir.h:42: // If the current path is empty or if the folder
can't be deleted
nit: Don't mention all failure cases. I guess even now the list isn't
exhaustive.

http://codereview.chromium.org/5340004/diff/1/base/scoped_temp_dir.h#newcode44
base/scoped_temp_dir.h:44: bool Delete();
nit: Add WARN_UNUSED_RESULT from base/compiler_specific.

http://codereview.chromium.org/5340004/diff/1/base/scoped_temp_dir_unittest.cc
File base/scoped_temp_dir_unittest.cc (right):

http://codereview.chromium.org/5340004/diff/1/base/scoped_temp_dir_unittest.c...
base/scoped_temp_dir_unittest.cc:103: EXPECT_FALSE(dir.Delete());  // We should
not be able to delete.
That's only true on Windows, right?

[tommi (sloooow) - chröme, 2010-11-26 21:04:44]

http://codereview.chromium.org/5340004/diff/1/base/scoped_temp_dir.cc
File base/scoped_temp_dir.cc (right):

http://codereview.chromium.org/5340004/diff/1/base/scoped_temp_dir.cc#newcode62
base/scoped_temp_dir.cc:62: if (!path_.empty()) {
On 2010/11/26 21:01:20, Paweł Hajdan Jr. wrote:
> nit: Rather:
> 
> if (path.empty())
>   return false;

Done.

http://codereview.chromium.org/5340004/diff/1/base/scoped_temp_dir.cc#newcode64
base/scoped_temp_dir.cc:64: if (!ret) {
On 2010/11/26 21:01:20, Paweł Hajdan Jr. wrote:
> nit: Put success case first to avoid double-negation for "else".

Done.

http://codereview.chromium.org/5340004/diff/1/base/scoped_temp_dir_unittest.cc
File base/scoped_temp_dir_unittest.cc (right):

http://codereview.chromium.org/5340004/diff/1/base/scoped_temp_dir_unittest.c...
base/scoped_temp_dir_unittest.cc:103: EXPECT_FALSE(dir.Delete());  // We should
not be able to delete.
On 2010/11/26 21:01:20, Paweł Hajdan Jr. wrote:
> That's only true on Windows, right?

Apparently so :)
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...

Should I make this test OS_WIN only?

[tommi (sloooow) - chröme, 2010-11-27 00:02:08]

FYI - I made the test Windows only.

[Paweł Hajdan Jr., 2010-11-27 14:57:07]

http://codereview.chromium.org/5340004/diff/12001/base/scoped_temp_dir.h
File base/scoped_temp_dir.h (right):

http://codereview.chromium.org/5340004/diff/12001/base/scoped_temp_dir.h#newc...
base/scoped_temp_dir.h:41: // Returns true if the directory was successfully
deleted.
Please see my earlier comments about this code.

http://codereview.chromium.org/5340004/diff/12001/base/scoped_temp_dir_unitte...
File base/scoped_temp_dir_unittest.cc (right):

http://codereview.chromium.org/5340004/diff/12001/base/scoped_temp_dir_unitte...
base/scoped_temp_dir_unittest.cc:93: 
nit: Remove empty line.

http://codereview.chromium.org/5340004/diff/12001/base/scoped_temp_dir_unitte...
base/scoped_temp_dir_unittest.cc:111: 
nit: Remove empty line.

http://codereview.chromium.org/5340004/diff/12001/base/scoped_temp_dir_unitte...
base/scoped_temp_dir_unittest.cc:112: #endif
nit:  // defined(OS_WIN)

[tommi (sloooow) - chröme, 2010-11-28 02:37:03]

http://codereview.chromium.org/5340004/diff/12001/base/scoped_temp_dir.h
File base/scoped_temp_dir.h (right):

http://codereview.chromium.org/5340004/diff/12001/base/scoped_temp_dir.h#newc...
base/scoped_temp_dir.h:41: // Returns true if the directory was successfully
deleted.
On 2010/11/27 14:57:07, Paweł Hajdan Jr. wrote:
> Please see my earlier comments about this code.

removed my addition to the comment.

http://codereview.chromium.org/5340004/diff/12001/base/scoped_temp_dir_unitte...
File base/scoped_temp_dir_unittest.cc (right):

http://codereview.chromium.org/5340004/diff/12001/base/scoped_temp_dir_unitte...
base/scoped_temp_dir_unittest.cc:93: 
On 2010/11/27 14:57:07, Paweł Hajdan Jr. wrote:
> nit: Remove empty line.

Done.

http://codereview.chromium.org/5340004/diff/12001/base/scoped_temp_dir_unitte...
base/scoped_temp_dir_unittest.cc:111: 
On 2010/11/27 14:57:07, Paweł Hajdan Jr. wrote:
> nit: Remove empty line.

Done.

http://codereview.chromium.org/5340004/diff/12001/base/scoped_temp_dir_unitte...
base/scoped_temp_dir_unittest.cc:112: #endif
On 2010/11/27 14:57:07, Paweł Hajdan Jr. wrote:
> nit:  // defined(OS_WIN)

Done.

[Paweł Hajdan Jr., 2010-11-29 09:32:17]

LGTM with a nit.

http://codereview.chromium.org/5340004/diff/9004/base/scoped_temp_dir.h
File base/scoped_temp_dir.h (right):

http://codereview.chromium.org/5340004/diff/9004/base/scoped_temp_dir.h#newco...
base/scoped_temp_dir.h:41: bool Delete();
nit: Add WARN_UNUSED_RESULT from base/compiler_specific.h.

[tommi (sloooow) - chröme, 2010-11-29 14:50:16]

http://codereview.chromium.org/5340004/diff/9004/base/scoped_temp_dir.h
File base/scoped_temp_dir.h (right):

http://codereview.chromium.org/5340004/diff/9004/base/scoped_temp_dir.h#newco...
base/scoped_temp_dir.h:41: bool Delete();
On 2010/11/29 09:32:17, Paweł Hajdan Jr. wrote:
> nit: Add WARN_UNUSED_RESULT from base/compiler_specific.h.

Done.

[tommi (sloooow) - chröme, 2010-11-29 15:05:15]

Hey Pawel - I'm going to revert the WARN_UNUSED_RESULT change since it will
break the linux build.  I don't think it's necessary in all cases to check the
return value.  Only for tests that require it as part of the test itself.
Let me know if you still prefer to have that in and I'll prepare a second change
that does it.

[Paweł Hajdan Jr., 2010-11-30 09:29:32]

Yes I'd like to have it - wasn't that the point of the change? Thanks!

On Mon, Nov 29, 2010 at 16:05, <tommi@chromium.org> wrote:

> Hey Pawel - I'm going to revert the WARN_UNUSED_RESULT change since it will
> break the linux build.  I don't think it's necessary in all cases to check
> the
> return value.  Only for tests that require it as part of the test itself.
> Let me know if you still prefer to have that in and I'll prepare a second
> change
> that does it.
>
>
> http://codereview.chromium.org/5340004/
>

[tommi (sloooow) - chröme, 2010-11-30 15:26:40]

Nope, that wasn't the point.  The point was to allow tests to check the
return value of Delete when that is a part of the test.
The class and the Delete function are currently used outside of such tests
and checking the return value there isn't critical.

I'll prepare a patch and send you the review.

On Tue, Nov 30, 2010 at 4:29 AM, Paweł Hajdan, Jr.
<phajdan.jr@chromium.org>wrote:

> Yes I'd like to have it - wasn't that the point of the change? Thanks!
>
>
> On Mon, Nov 29, 2010 at 16:05, <tommi@chromium.org> wrote:
>
>> Hey Pawel - I'm going to revert the WARN_UNUSED_RESULT change since it
>> will
>> break the linux build.  I don't think it's necessary in all cases to check
>> the
>> return value.  Only for tests that require it as part of the test itself.
>> Let me know if you still prefer to have that in and I'll prepare a second
>> change
>> that does it.
>>
>>
>> http://codereview.chromium.org/5340004/
>>
>
>