Move Contains() method to file_utils, stop relying on in extensions_protocol

Move Contains() method to file_utils, stop relying on in extensions_protocol

[Matt Perry, 2009-01-09 20:55:12]

http://codereview.chromium.org/16805/diff/1/7
File base/file_util_unittest.cc (right):

http://codereview.chromium.org/16805/diff/1/7#newcode987
Line 987: file_util::Delete(data_dir, true);
Most of these file operations can fail. I'd wrap them in ASSERT_TRUEs so that if
they fail, we know what went wrong with the test.

http://codereview.chromium.org/16805/diff/1/8
File chrome/browser/extensions/extension_protocol.cc (right):

http://codereview.chromium.org/16805/diff/1/8#newcode48
Line 48: ret_val.value().c_str()[extension_path.value().length()] !=
It's slower, but I think it'd be clearer to do:
(ret_val.value().find(extension_path.value() + FilePath::kSeparators[0]) != 0)

http://codereview.chromium.org/16805/diff/1/9
File chrome/browser/extensions/extension_protocol_unittest.cc (right):

http://codereview.chromium.org/16805/diff/1/9#newcode20
Line 20: EXPECT_EQ(std::wstring(L"C:\\myextension\\\\foo.gif"),
I don't understand the double path separators. What are you testing exactly?

[Aaron Boodman, 2009-01-09 21:25:50]

Ready for another look.

http://codereview.chromium.org/16805/diff/1/7
File base/file_util_unittest.cc (right):

http://codereview.chromium.org/16805/diff/1/7#newcode987
Line 987: file_util::Delete(data_dir, true);
On 2009/01/09 20:55:12, Matt Perry wrote:
> Most of these file operations can fail. I'd wrap them in ASSERT_TRUEs so that
if
> they fail, we know what went wrong with the test.

Done.

http://codereview.chromium.org/16805/diff/1/8
File chrome/browser/extensions/extension_protocol.cc (right):

http://codereview.chromium.org/16805/diff/1/8#newcode48
Line 48: ret_val.value().c_str()[extension_path.value().length()] !=
On 2009/01/09 20:55:12, Matt Perry wrote:
> It's slower, but I think it'd be clearer to do:
> (ret_val.value().find(extension_path.value() + FilePath::kSeparators[0]) != 0)

Done.

http://codereview.chromium.org/16805/diff/1/9
File chrome/browser/extensions/extension_protocol_unittest.cc (right):

http://codereview.chromium.org/16805/diff/1/9#newcode20
Line 20: EXPECT_EQ(std::wstring(L"C:\\myextension\\\\foo.gif"),
On 2009/01/09 20:55:12, Matt Perry wrote:
> I don't understand the double path separators. What are you testing exactly?

Good point, removed those tests.

[Matt Perry, 2009-01-09 21:41:54]

LGTM

[Erik does not do reviews, 2009-01-12 17:14:02]

LGTM

http://codereview.chromium.org/16805/diff/404/405
File base/file_path.cc (left):

http://codereview.chromium.org/16805/diff/404/405#oldcode6
Line 6: #include "base/file_util.h"
maybe it's worth adding a comment here so that others in the future don't ever
include file_util or do any operations that touch the filesystem.

http://codereview.chromium.org/16805/diff/404/408
File base/file_util.cc (right):

http://codereview.chromium.org/16805/diff/404/408#newcode291
Line 291: #else
#else if defined(OS_POSIX)

http://codereview.chromium.org/16805/diff/404/408#newcode292
Line 292: if (!StartsWithASCII(abs_child.value(), abs_parent.value(), true))
Put a comment / TODO / bug in here to reference the MacOS case sensitivity
issue.  Since this is in file_util, it seems like we could fall back to a test
that actually touches the filesystem in some way when StartsWithASCII returns
false.

http://codereview.chromium.org/16805/diff/404/409
File base/file_util.h (right):

http://codereview.chromium.org/16805/diff/404/409#newcode107
Line 107: // absolute paths before doing the comparison.
If we're not going to handle MacOSX case-sensitivity issues, then there should
be a comment to that effect here.

http://codereview.chromium.org/16805/diff/404/410
File base/file_util_unittest.cc (right):

http://codereview.chromium.org/16805/diff/404/410#newcode1011
Line 1011: // Platform-specific concerns
missing indent