Add ScopedTempDir - a class that manages the lifetime of a temporary director...

Add ScopedTempDir - a class that manages the lifetime of a temporary directory.

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

[Erik does not do reviews, 2009-01-28 01:11:58]

I didn't add this to Linux/Mac build files yet.  I'm curious on your feedback as
to whether I should push this down into base instead.

[Matt Perry, 2009-01-28 01:30:24]

http://codereview.chromium.org/19411/diff/1/3
File chrome/common/scoped_temp_dir.h (right):

http://codereview.chromium.org/19411/diff/1/3#newcode18
Line 18: ScopedTempDir(const FilePath& path);
I find the two constructors a bit confusing.  I think the difference between
them is too subtle to notice at a call site.

What about a single constructor that takes a FilePath and a bool that indicates
whether the path will be created under TempPath?

[Erik does not do reviews, 2009-01-28 01:38:03]

I'm also curious about your thoughts to my original question about base vs.
chrome/common.

http://codereview.chromium.org/19411/diff/1/3
File chrome/common/scoped_temp_dir.h (right):

http://codereview.chromium.org/19411/diff/1/3#newcode18
Line 18: ScopedTempDir(const FilePath& path);
On 2009/01/28 01:30:24, Matt Perry wrote:
> I find the two constructors a bit confusing.  I think the difference between
> them is too subtle to notice at a call site.
> 
> What about a single constructor that takes a FilePath and a bool that
indicates
> whether the path will be created under TempPath?

Agree that it's a bit confusing, however the point of the second one isn't to
create under tempdir, but to use the built-in functionality of the underlying
CreateNewTempDirectory which ensures that a unique name is created.  Would this
be more clear if I changed it to be a constructor that took no arguments and not
let you specify the magic prefix?

[Matt Perry, 2009-01-28 01:48:52]

I think it's fair to put it into base, since all its dependencies are there.

http://codereview.chromium.org/19411/diff/1/3
File chrome/common/scoped_temp_dir.h (right):

http://codereview.chromium.org/19411/diff/1/3#newcode18
Line 18: ScopedTempDir(const FilePath& path);
On 2009/01/28 01:38:03, Erik Kay wrote:
> On 2009/01/28 01:30:24, Matt Perry wrote:
> > I find the two constructors a bit confusing.  I think the difference between
> > them is too subtle to notice at a call site.
> > 
> > What about a single constructor that takes a FilePath and a bool that
> indicates
> > whether the path will be created under TempPath?
> 
> Agree that it's a bit confusing, however the point of the second one isn't to
> create under tempdir, but to use the built-in functionality of the underlying
> CreateNewTempDirectory which ensures that a unique name is created.  Would
this
> be more clear if I changed it to be a constructor that took no arguments and
not
> let you specify the magic prefix?

Hmm... I think a no-arg constructor might be clearer, but obviously less
flexible as well.  And I guess it doesn't make sense to use a FilePath for a
prefix.  I don't really have a better suggestion, so what you have is fine.

[Matt Perry, 2009-01-28 01:49:03]

also, lgtm

[darin (slow to review), 2009-01-28 07:14:32]

On 2009/01/28 01:48:52, Matt Perry wrote:
> I think it's fair to put it into base, since all its dependencies are there.

That's true.  Also, this class seems to have generic enough appeal that it
definitely seems like a reasonable thing to put in base.  However, our guiding
principle has always been to only put things in base/ that are used by more than
just the chrome/ module with the idea being that we could always move things
from chrome/common/ into base/ if it proves useful to do so.  I think that
principle is a bit squishy :)

[tommi (sloooow) - chröme, 2009-01-28 16:12:16]

drive-by-review

http://codereview.chromium.org/19411/diff/1/5
File chrome/common/scoped_temp_dir.cc (right):

http://codereview.chromium.org/19411/diff/1/5#newcode11
Line 11: ScopedTempDir::ScopedTempDir(const FilePath& path) : path_(path) {
see comments in .h file for all non-trivial constructors

http://codereview.chromium.org/19411/diff/1/5#newcode32
Line 32: }
shouldn't you also delete the directory?
assuming you'll add that, also suggest to CHECK() that the deletion of the
directory succeeds.

http://codereview.chromium.org/19411/diff/1/5#newcode40
Line 40: #endif
perhaps add a catch-all?
#else
#error "blabla"
#endif

http://codereview.chromium.org/19411/diff/1/5#newcode45
Line 45: if (!file_util::DirectoryExists(path_)) {
since this can easily fail, it makes sense to return an error value.

http://codereview.chromium.org/19411/diff/1/3
File chrome/common/scoped_temp_dir.h (right):

http://codereview.chromium.org/19411/diff/1/3#newcode16
Line 16: public:
fix indent

http://codereview.chromium.org/19411/diff/1/3#newcode18
Line 18: ScopedTempDir(const FilePath& path);
since this can easily fail for various reasons (e.g. what will you do if the
directory already exists?) I don't think this belongs in a constructor.
Instead suggest a method that does what this constructor is meant to do that
returns an error value and has a descriptive name to avoid confusion

http://codereview.chromium.org/19411/diff/1/3#newcode21
Line 21: ScopedTempDir(const std::string& ascii_prefix);
this can also fail.

http://codereview.chromium.org/19411/diff/1/3#newcode23
Line 23: virtual ~ScopedTempDir();
why add a vtable?

http://codereview.chromium.org/19411/diff/1/3#newcode25
Line 25: const FilePath& path() { return path_; }
const?

http://codereview.chromium.org/19411/diff/1/3#newcode28
Line 28: bool IsValid();
const?

http://codereview.chromium.org/19411/diff/1/3#newcode32
Line 32: void Release();
does it make sense to return the FilePath that's being released? (like other
Scoped classes do)

http://codereview.chromium.org/19411/diff/1/3#newcode34
Line 34: private:
indent

http://codereview.chromium.org/19411/diff/1/3#newcode35
Line 35: void CreateTempDir(const std::string& ascii_prefix);
no return value?

http://codereview.chromium.org/19411/diff/1/3#newcode36
Line 36: void EnsurePath();
return value?

http://codereview.chromium.org/19411/diff/1/3#newcode39
Line 39: 
remove empty line

http://codereview.chromium.org/19411/diff/1/2
File chrome/common/scoped_temp_dir_unittest.cc (right):

http://codereview.chromium.org/19411/diff/1/2#newcode17
Line 17: ScopedTempDir dir(test_path);
so, the temp directory is not meant to be created by ScopedTempDir?  if true,
then suggest a DCHEC() that the directory already exists in the ScopedTempDir
constructor

http://codereview.chromium.org/19411/diff/1/2#newcode23
Line 23: ScopedTempDir dir(test_path);
I agree that this is confusing.  would prefer two separate methods for attach vs
create

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

thanks for the drive-by

http://codereview.chromium.org/19411/diff/1/5
File chrome/common/scoped_temp_dir.cc (right):

http://codereview.chromium.org/19411/diff/1/5#newcode32
Line 32: }
On 2009/01/28 16:12:16, tommi wrote:
> shouldn't you also delete the directory?
> assuming you'll add that, also suggest to CHECK() that the deletion of the
> directory succeeds.

Sorry, I missed something.  That's what the file_util::Delete does.  I can't put
a CHECK() in there since there are real filesystem issues that could cause the
error randomly (A-V software locks the directory).  As stated in the comments in
the header file, deletion is not guaranteed.

http://codereview.chromium.org/19411/diff/1/5#newcode40
Line 40: #endif
On 2009/01/28 16:12:16, tommi wrote:
> perhaps add a catch-all?
> #else
> #error "blabla"
> #endif

The style we've taken in Chrome is to not add the extra #else unless it is
strictly necessary.  I personally don't care enough about it to argue the point,
so I'd suggest a chrome-team email if you think the style is wrong.

http://codereview.chromium.org/19411/diff/1/3
File chrome/common/scoped_temp_dir.h (right):

http://codereview.chromium.org/19411/diff/1/3#newcode18
Line 18: ScopedTempDir(const FilePath& path);
On 2009/01/28 16:12:16, tommi wrote:
> since this can easily fail for various reasons (e.g. what will you do if the
> directory already exists?) I don't think this belongs in a constructor.
> Instead suggest a method that does what this constructor is meant to do that
> returns an error value and has a descriptive name to avoid confusion

Yes.  This had been bugging me as well (although the directory already existing
is actually a feature, not a failure - just a bad comment here).  The reason I
didn't end up here was an effort to keep the API simple.  I couldn't have a
factory method, so that meant having to call the constructor and then an init
method.  I agree that the tradeoff was probably the wrong one to make here. 
I've taken a fresh swing at the API modeled after scoped_handle.h.

http://codereview.chromium.org/19411/diff/1/3#newcode32
Line 32: void Release();
On 2009/01/28 16:12:16, tommi wrote:
> does it make sense to return the FilePath that's being released? (like other
> Scoped classes do)

I had considered this, but it felt a little odd.  I agree that it kind of
follows the style of the other APIs, but those APIs are all about resources that
technically have to be closed, cleaned up (memory pointer, file handles).  In
this case, we're returning a symbolic representation to the underlying resource.
 However, since I'm updating the API to match scoped_handle, I'll make this
change.

http://codereview.chromium.org/19411/diff/1/2
File chrome/common/scoped_temp_dir_unittest.cc (right):

http://codereview.chromium.org/19411/diff/1/2#newcode17
Line 17: ScopedTempDir dir(test_path);
On 2009/01/28 16:12:16, tommi wrote:
> so, the temp directory is not meant to be created by ScopedTempDir?  if true,
> then suggest a DCHEC() that the directory already exists in the ScopedTempDir
> constructor

The idea here is that the object can take ownership of an existing directory, so
it's not an error condition.

http://codereview.chromium.org/19411/diff/1/2#newcode23
Line 23: ScopedTempDir dir(test_path);
On 2009/01/28 16:12:16, tommi wrote:
> I agree that this is confusing.  would prefer two separate methods for attach
vs
> create

Take a look at the fresh take at the API and let me know if you still feel this
way.

[Matt Perry, 2009-01-28 18:30:40]

I like the new changes.

http://codereview.chromium.org/19411/diff/21/220
File chrome/common/scoped_temp_dir.cc (right):

http://codereview.chromium.org/19411/diff/21/220#newcode26
Line 26: path_ = path;
would it make more sense to do this assignment before the "return true", so we
don't have to reset it on failure?

http://codereview.chromium.org/19411/diff/21/220#newcode38
Line 38: return path_;
oops, you're returning an empty path here

http://codereview.chromium.org/19411/diff/21/217
File chrome/common/scoped_temp_dir_unittest.cc (right):

http://codereview.chromium.org/19411/diff/21/217#newcode18
Line 18: dir.Set(test_path);
now that we have a return value, maybe this should look like:
EXPECT_TRUE(!dir.Set(test_path) || dir.IsValid())

...might not be worth testing for, but it clarifies the API.

[tommi (sloooow) - chröme, 2009-01-28 18:38:21]

lgtm with Matt's suggestions.
thanks for making the changes.  below are mostly nits, so if you don't agree, my
lgtm still stands.

http://codereview.chromium.org/19411/diff/21/220
File chrome/common/scoped_temp_dir.cc (right):

http://codereview.chromium.org/19411/diff/21/220#newcode26
Line 26: path_ = path;
I concur
On 2009/01/28 18:30:40, Matt Perry wrote:
> would it make more sense to do this assignment before the "return true", so we
> don't have to reset it on failure?

http://codereview.chromium.org/19411/diff/21/220#newcode45
Line 45: ScopedTempDir::~ScopedTempDir() {
move destructor up to where the constructor is

http://codereview.chromium.org/19411/diff/21/220#newcode47
Line 47: if (!file_util::Delete(path_, true)) {
could simply use && and have a single if instead of two scopes

http://codereview.chromium.org/19411/diff/21/218
File chrome/common/scoped_temp_dir.h (right):

http://codereview.chromium.org/19411/diff/21/218#newcode34
Line 34: ~ScopedTempDir();
maybe move the destructor decl up to where the constructor is

http://codereview.chromium.org/19411/diff/21/218#newcode40
Line 40: 
remove extra vertical whitespace

[Mark Mentovai, 2009-01-28 18:47:11]

I mostly just stopped by to say that I think it'd be cool if this lived in base,
but you can move it whenever you want if needed.  Couple tiny nits and a
question, too.

http://codereview.chromium.org/19411/diff/21/220
File chrome/common/scoped_temp_dir.cc (right):

http://codereview.chromium.org/19411/diff/21/220#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
'09 again.  Unit test too.

http://codereview.chromium.org/19411/diff/21/220#newcode17
Line 17: file_util::CreateNewTempDirectory(FILE_PATH_LITERAL("scoped_dir"),
&path_);
Why not check the return value here instead of making an extra call to
DirectoryExists?

http://codereview.chromium.org/19411/diff/21/218
File chrome/common/scoped_temp_dir.h (right):

http://codereview.chromium.org/19411/diff/21/218#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
2009 on new files.

http://codereview.chromium.org/19411/diff/21/218#newcode45
Line 45: DISALLOW_EVIL_CONSTRUCTORS(ScopedTempDir);
Please use the new politically-correct name for this, DISALLOW_COPY_AND_ASSIGN.

[Erik does not do reviews, 2009-01-28 20:26:15]

I moved the files to base.  Sorry that this means that the diffs don't match,
but since it's mostly nits at this point, it should be easy to see.

http://codereview.chromium.org/19411/diff/21/220
File chrome/common/scoped_temp_dir.cc (right):

http://codereview.chromium.org/19411/diff/21/220#newcode17
Line 17: file_util::CreateNewTempDirectory(FILE_PATH_LITERAL("scoped_dir"),
&path_);
On 2009/01/28 18:47:11, Mark Mentovai wrote:
> Why not check the return value here instead of making an extra call to
> DirectoryExists?

yep

http://codereview.chromium.org/19411/diff/21/220#newcode26
Line 26: path_ = path;
On 2009/01/28 18:30:40, Matt Perry wrote:
> would it make more sense to do this assignment before the "return true", so we
> don't have to reset it on failure?

I was thinking about what happens when path_ is already non-null (thinking that
a failed Set should reset it), but in retrospect, I realize that that opens up
another question (in that case, should the existing path be deleted?).  Instead,
I'll add a DCHECK to enforce that Set isn't being called when there's already a
valid path.

http://codereview.chromium.org/19411/diff/21/220#newcode38
Line 38: return path_;
On 2009/01/28 18:30:40, Matt Perry wrote:
> oops, you're returning an empty path here

oops.

[Matt Perry, 2009-01-28 20:29:42]

lgtm, one final nit

http://codereview.chromium.org/19411/diff/238/243
File base/scoped_temp_dir.h (right):

http://codereview.chromium.org/19411/diff/238/243#newcode27
Line 27: bool CreateUniqueTempDir(FilePath* path);
The path argument seems like an unnecessary complication of the API, since a
caller could just use the path() accessor.