Make FilePath accept FilePath as Append parameter.

Make FilePath accept FilePath as Append parameter.

[please use my chromium address, 2008-12-03 17:23:22]

This is a change considered for http://codereview.chromium.org/9639 .

[please use my chromium address, 2008-12-03 17:49:24]

Updated for consistency. Just finished compiling and running unit tests.

[Mark Mentovai, 2008-12-03 17:56:26]

http://codereview.chromium.org/12907/diff/8/10
File base/file_path.cc (right):

http://codereview.chromium.org/12907/diff/8/10#newcode6
Line 6: #include "base/logging.h"
Adding this dependency means that we can't necessarily use FilePath safely from
logging code.  We may need to be careful here if we ever move logging over to
use FilePath.  Just something to keep in mind.

http://codereview.chromium.org/12907/diff/8/10#newcode96
Line 96: DCHECK(!FilePath(component).IsAbsolute());
Sucky to turn a StringType into a FilePath just for this.  Maybe refactor
IsAbsolute to be implemented in terms of a static (to this file) function like
|static bool IsAbsolute(const FilePath::StringType& path)|, which we could also
call directly from here?

http://codereview.chromium.org/12907/diff/8/10#newcode132
Line 132: DCHECK(!component.IsAbsolute());
Now this isn't needed because Append(const StringType&) is going to do this
check anyway.

[please use my chromium address, 2008-12-03 18:28:54]

On 2008/12/03 17:56:26, Mark Mentovai wrote:
> http://codereview.chromium.org/12907/diff/8/10
> File base/file_path.cc (right):
> 
> http://codereview.chromium.org/12907/diff/8/10#newcode6
> Line 6: #include "base/logging.h"
> Adding this dependency means that we can't necessarily use FilePath safely
from
> logging code.  We may need to be careful here if we ever move logging over to
> use FilePath.  Just something to keep in mind.

Last I checked the maintainer of logging code didn't want to switch to FilePath
because it is also used in some other places...

> http://codereview.chromium.org/12907/diff/8/10#newcode96
> Line 96: DCHECK(!FilePath(component).IsAbsolute());
> Sucky to turn a StringType into a FilePath just for this.  Maybe refactor
> IsAbsolute to be implemented in terms of a static (to this file) function like
> |static bool IsAbsolute(const FilePath::StringType& path)|, which we could
also
> call directly from here?

Thought about that, done now.

> http://codereview.chromium.org/12907/diff/8/10#newcode132
> Line 132: DCHECK(!component.IsAbsolute());
> Now this isn't needed because Append(const StringType&) is going to do this
> check anyway.

Done.

[Dean McNamee, 2008-12-03 18:36:06]

Looks ok, but mark on the details.

http://codereview.chromium.org/12907/diff/14/210
File base/file_path.cc (right):

http://codereview.chromium.org/12907/diff/14/210#newcode29
Line 29: FilePath::StringType::size_type FindDriveLetter(const
FilePath::StringType& path) {
80 cols, you can probably just use size_t.

[please use my chromium address, 2008-12-03 18:38:40]

On 2008/12/03 18:36:06, Dean McNamee wrote:
> Looks ok, but mark on the details.
> 
> http://codereview.chromium.org/12907/diff/14/210
> File base/file_path.cc (right):
> 
> http://codereview.chromium.org/12907/diff/14/210#newcode29
> Line 29: FilePath::StringType::size_type FindDriveLetter(const
> FilePath::StringType& path) {
> 80 cols, you can probably just use size_t.

Added a line break. IMHO it's more consistent (and slightly more ugly), but if
you wish I'll just change it to size_t.

[Mark Mentovai, 2008-12-03 18:49:31]

Other than that, LGTM, nice job.

http://codereview.chromium.org/12907/diff/215/216
File base/file_path_unittest.cc (right):

http://codereview.chromium.org/12907/diff/215/216#newcode278
Line 278: EXPECT_EQ(FilePath::StringType(cases[i].expected),
observed_str.value()) <<
Minor nit: do the EXPECT before doing anything with observed_path.

[please use my chromium address, 2008-12-03 18:56:46]

On 2008/12/03 18:49:31, Mark Mentovai wrote:
> Other than that, LGTM, nice job.
> 
> http://codereview.chromium.org/12907/diff/215/216
> File base/file_path_unittest.cc (right):
> 
> http://codereview.chromium.org/12907/diff/215/216#newcode278
> Line 278: EXPECT_EQ(FilePath::StringType(cases[i].expected),
> observed_str.value()) <<
> Minor nit: do the EXPECT before doing anything with observed_path.

Done.

[Mark Mentovai, 2008-12-03 18:59:24]

LGTM, committed r6299.

[please use my chromium address, 2008-12-03 21:26:58]

Updated to fix base_unittests on Win.

[Mark Mentovai, 2008-12-03 22:02:54]

The fix seems right.  Have you tested it on Windows?

[Mark Mentovai, 2008-12-03 22:09:58]

Committed (again) r6315.

[please use my chromium address, 2008-12-03 22:44:47]

Updated. Maruel suggested temporary disabling test on Vista. Filed a bug.

[Mark Mentovai, 2008-12-08 16:49:45]

Committed again, r6511