Move file enumeration to filepaths.

Move file enumeration to filepaths.


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

[Avi (use Gerrit), 2008-12-09 23:31:50]

Looks like I missed a few places; put this on hold.

[Evan Stade, 2008-12-10 01:25:47]

kudos!

[Evan Stade, 2008-12-10 01:26:37]

just some nits, for when you get around to taking this up again

http://codereview.chromium.org/13315/diff/1/8
File base/file_path.h (right):

http://codereview.chromium.org/13315/diff/1/8#newcode184
Line 184: FilePath StripTrailingSeparators() const;
can you add a note in file_util.h that TrimTrailingSeparator(std::wstring* file)
is now deprecated, and move the implementation in file_util.cc to the bottom.

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

http://codereview.chromium.org/13315/diff/1/5#newcode395
Line 395: FilePath pattern_;  // Empty when we want to find everything.
It might be more convenient to make |pattern| a FilePath::StringType.

http://codereview.chromium.org/13315/diff/1/4
File base/file_util_win.cc (right):

http://codereview.chromium.org/13315/diff/1/4#newcode374
Line 374: FileEnumerator files(FilePath::FromWStringHack(dir_path),
nit: in windows specific code you don't need to use the Hack methods

[brettw, 2008-12-10 02:46:10]

LGTM with one change (in addition to the other changes suggested by estade).

http://codereview.chromium.org/13315/diff/1/8
File base/file_path.h (right):

http://codereview.chromium.org/13315/diff/1/8#newcode72
Line 72: #ifndef NDEBUG
We don't generally put ifdefs for logging, I would just remove the ifdefs.

[Avi (use Gerrit), 2008-12-10 17:13:19]

http://codereview.chromium.org/13315/diff/1/8
File base/file_path.h (right):

http://codereview.chromium.org/13315/diff/1/8#newcode72
Line 72: #ifndef NDEBUG
On 2008/12/10 02:46:10, brettw wrote:
> We don't generally put ifdefs for logging, I would just remove the ifdefs.

The operator << was pulled entirely. Including logging where it wasn't before
just means tons of LOG redefinition headaches.

http://codereview.chromium.org/13315/diff/1/8#newcode184
Line 184: FilePath StripTrailingSeparators() const;
On 2008/12/10 01:26:37, Evan Stade wrote:
> can you add a note in file_util.h that TrimTrailingSeparator(std::wstring*
file)
> is now deprecated, and move the implementation in file_util.cc to the bottom.

Thus noted in the header, though I didn't see other deprecated implementations
move.

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

http://codereview.chromium.org/13315/diff/1/5#newcode395
Line 395: FilePath pattern_;  // Empty when we want to find everything.
On 2008/12/10 01:26:37, Evan Stade wrote:
> It might be more convenient to make |pattern| a FilePath::StringType.

This is deliberate; the Posix code uses this field as a path.

http://codereview.chromium.org/13315/diff/1/4
File base/file_util_win.cc (right):

http://codereview.chromium.org/13315/diff/1/4#newcode374
Line 374: FileEnumerator files(FilePath::FromWStringHack(dir_path),
On 2008/12/10 01:26:37, Evan Stade wrote:
> nit: in windows specific code you don't need to use the Hack methods

Fixed.

[Mark Mentovai, 2008-12-10 17:27:06]

I only looked carefully at the file_path changes for now.  Everything else just
looks like changes to adapt to the new API, and mostly involves moving
ToWStringHacks around, so I glossed over that stuff.

http://codereview.chromium.org/13315/diff/207/214
File base/file_path.h (right):

http://codereview.chromium.org/13315/diff/207/214#newcode176
Line 176: // Remove trailing separators from this object.  If the path is
absolute, it
This comment needs to be revised now, because this function no longer removes
trailing separators from "this" object.  You should move this comment down to
the private method, and write some new material to quickly describe this one.

http://codereview.chromium.org/13315/diff/207/214#newcode181
Line 181: FilePath StripTrailingSeparators() const;
Rather than doing this, maybe we should just guarantee that a FilePath never has
trailing separators.  We could do that by modifying the FilePath(const
StringType&) constructor to call StripTrailingSeparators().  Then we wouldn't
need to expose this method, which feels a little bit hackish.

[Avi (use Gerrit), 2008-12-10 18:50:40]

On 2008/12/10 17:27:06, Mark Mentovai wrote:
> Rather than doing this, maybe we should just guarantee that a FilePath never
has
> trailing separators.

This breaks all kinds of assumptions, including those inside FilePath already
(see the many places where it copies the current path and immediately strips the
trailing separators). However, it feels like FilePath is of conflicted mind
about that; after all, while you can create a path with a trailing separator,
it'll get stripped during the next append and you can never add one on again
(you'll choke during the DCHECK that starts Append()).

Mark, you're the designer. Should we guarantee no trailing slash? What's the
meaning of EndsWithSeparator(FilePath&) in file_util if we do so?

[darin (slow to review), 2008-12-10 18:55:44]

http://codereview.chromium.org/13315/diff/207/214
File base/file_path.h (right):

http://codereview.chromium.org/13315/diff/207/214#newcode181
Line 181: FilePath StripTrailingSeparators() const;
hmm... in some cases, it is nice to have the trailing slash as a cheap indicator
that the path corresponds to a directory.  this can help avoid having to stat
the filesystem to find out.

[Erik does not do reviews, 2008-12-10 18:59:41]

LGTM - just style nits

http://codereview.chromium.org/13315/diff/207/215
File base/file_util_unittest.cc (right):

http://codereview.chromium.org/13315/diff/207/215#newcode926
Line 926: // (we don't care what).
whitespace - comments don't line up

http://codereview.chromium.org/13315/diff/207/218
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/13315/diff/207/218#newcode79
Line 79: FilePath manifest_path =
child_path.Append(Extension::kManifestFilename);
bad indent on this line

http://codereview.chromium.org/13315/diff/207/221
File chrome/browser/importer/ie_importer.cc (right):

http://codereview.chromium.org/13315/diff/207/221#newcode472
Line 472: !cancelled())
Since you split the conditional across multiple lines, could you wrap the
contents of the file loop in {}.  It's hard to read like this.

[Evan Stade, 2008-12-10 19:20:54]

> However, it feels like FilePath is of conflicted mind
> about that; after all, while you can create a path with a trailing separator,
> it'll get stripped during the next append and you can never add one on again
> (you'll choke during the DCHECK that starts Append()).

wait, what? that DCHECK looks like it only looks for leading separators. But I
agree there are tons of places in the code that rely on trailing separators for
their logic.

[Avi (use Gerrit), 2008-12-10 22:02:41]

OK, so basically everything is addressed. For now I'm leaving FilePath alone,
and if someone in the future needs to make a call to drop a separator at the end
they can do so.