Add path utils, plus a test for it.

Add path utils, plus a test for it.

SkOSFile:
Added class SkOSPath with functions for
modifying strings representing path names.

OSPathTest.cpp:
Test of the new utilities.

factory.cpp:
Use SkPathJoin.

gmmain and gm_expectations:
Use SkOSPath::SkPathJoin instead of a local version.

skimage_main.cpp:
Use the new location of SkPathJoin and SkBasename.

R=epoger@google.com

Committed: https://code.google.com/p/skia/source/detail?r=9277

[scroggo, 2013-05-22 16:00:17]

On 2013/05/22 15:52:13, scroggo wrote:

I meant to mention, I decided on SkOSPathUtils instead of SkOSFile because I
wasn't sure this belonged in core/, and it is just for string modification,
rather than interacting with the OS. That said, I suppose you'd only use it for
interacting with OS, so maybe SkOSFile is the right place to put it after all.
Let me know what you think.

[epoger, 2013-05-22 17:33:58]

Generally looks good... thanks for doing this!

https://codereview.chromium.org/15747004/diff/1001/gm/image.cpp
File gm/image.cpp (left):

https://codereview.chromium.org/15747004/diff/1001/gm/image.cpp#oldcode34
gm/image.cpp:34: SkAutoDataUnref
data(fileToData("/Users/mike/Downloads/skia.google.jpeg"));
What the .. ???  Why wasn't this causing failures before?  Is it not exercised
anywhere?

https://codereview.chromium.org/15747004/diff/1001/include/utils/SkOSPathUtils.h
File include/utils/SkOSPathUtils.h (right):

https://codereview.chromium.org/15747004/diff/1001/include/utils/SkOSPathUtil...
include/utils/SkOSPathUtils.h:2: * Copyright 2013 Google Inc.
You might want to get Mike's blessing before adding this file to include/ .  He
may want to keep it private, I dunno.

And if we make it public, is there some SK_API macro we need to use in the
definitions?

https://codereview.chromium.org/15747004/diff/1001/tests/OSPathUtilsTest.cpp
File tests/OSPathUtilsTest.cpp (right):

https://codereview.chromium.org/15747004/diff/1001/tests/OSPathUtilsTest.cpp#...
tests/OSPathUtilsTest.cpp:13: static void test_dir_with_file(skiatest::Reporter*
reporter, SkString dir,
Please add documentation stating what this method does, including a note that
SkPATH_SEPARATOR should not be present in filename.

https://codereview.chromium.org/15747004/diff/1001/tests/OSPathUtilsTest.cpp#...
tests/OSPathUtilsTest.cpp:15: #ifdef SK_DEBUG
Why the #ifdef here?  Seems like this code can always be included...

https://codereview.chromium.org/15747004/diff/1001/tests/OSPathUtilsTest.cpp#...
tests/OSPathUtilsTest.cpp:19:
SkASSERT(!filename.contains(pathSeparator.c_str()));
Why not just 

SkASSERT(!filename.contains(SkPATH_SEPARATOR))

https://codereview.chromium.org/15747004/diff/1001/tests/OSPathUtilsTest.cpp#...
tests/OSPathUtilsTest.cpp:22: // fullName should be "dir<SkPATH_SEPARATOR>file"
I think this code would be easier to read with some extra newlines, like this:

// Tests for SkPathJoin and SkBasename

// fullName should be "dir<SkPATH_SEPARATOR>file"
SkString fullName = SkPathJoin(dir.c_str(), filename.c_str());

// basename should be the same as filename
REPORTER_ASSERT(reporter, 0 == strcmp(basename, filename.c_str()));

// Now take the basename of filename, which should be the same as filename
basename = SkBasename(filename.c_str());
REPORTER_ASSERT(reporter, 0 == strcmp(basename, filename.c_str()));

https://codereview.chromium.org/15747004/diff/1001/tests/OSPathUtilsTest.cpp#...
tests/OSPathUtilsTest.cpp:26: REPORTER_ASSERT(reporter, fullName.size() ==
dir.size() + filename.size()
Maybe this tighter test instead?

if (dir.endsWith(SkPATH_SEPARATOR)) {
  expectedSize = dir.size() + filename.size();
} else {
  expectedSize = dir.size() + filename.size() + 1;
}
REPORTER_ASSERT(reporter, expectedSize == fullName.size());

https://codereview.chromium.org/15747004/diff/1001/tests/OSPathUtilsTest.cpp#...
tests/OSPathUtilsTest.cpp:28: const char* basename =
SkBasename(fullName.c_str());
I think the code would be easier to follow if you always used SkStrings, or
always used char *, instead of a mix.

[scroggo, 2013-05-23 15:29:33]

Adding Mike.

Mike, ok to make this public, or should the header be in src/? On the other
hand, should this be a part of SkOSFile?

https://codereview.chromium.org/15747004/diff/1001/gm/image.cpp
File gm/image.cpp (left):

https://codereview.chromium.org/15747004/diff/1001/gm/image.cpp#oldcode34
gm/image.cpp:34: SkAutoDataUnref
data(fileToData("/Users/mike/Downloads/skia.google.jpeg"));
On 2013/05/22 17:33:58, epoger wrote:
> What the .. ???  Why wasn't this causing failures before?  Is it not exercised
> anywhere?

It fails silently - NewEncodedData will return a NULL image if data is empty.
The baseline for this test was never created on Mike's machine, so the expected
image doesn't have this picture.

https://codereview.chromium.org/15747004/diff/1001/include/utils/SkOSPathUtils.h
File include/utils/SkOSPathUtils.h (right):

https://codereview.chromium.org/15747004/diff/1001/include/utils/SkOSPathUtil...
include/utils/SkOSPathUtils.h:2: * Copyright 2013 Google Inc.
On 2013/05/22 17:33:58, epoger wrote:
> You might want to get Mike's blessing before adding this file to include/ . 
He
> may want to keep it private, I dunno.
> 
> And if we make it public, is there some SK_API macro we need to use in the
> definitions?

I believe we only need to add SK_API if code outside of Skia is using it.

https://codereview.chromium.org/15747004/diff/1001/tests/OSPathUtilsTest.cpp
File tests/OSPathUtilsTest.cpp (right):

https://codereview.chromium.org/15747004/diff/1001/tests/OSPathUtilsTest.cpp#...
tests/OSPathUtilsTest.cpp:13: static void test_dir_with_file(skiatest::Reporter*
reporter, SkString dir,
On 2013/05/22 17:33:58, epoger wrote:
> Please add documentation stating what this method does, including a note that
> SkPATH_SEPARATOR should not be present in filename.

Done.

https://codereview.chromium.org/15747004/diff/1001/tests/OSPathUtilsTest.cpp#...
tests/OSPathUtilsTest.cpp:15: #ifdef SK_DEBUG
On 2013/05/22 17:33:58, epoger wrote:
> Why the #ifdef here?  Seems like this code can always be included...

Because it's only here for the SkASSERT. In release mode, we won't run the
assert statement, so there is no need for the prior code.

That said, the setup code has been removed, so the entire SkASSERT is
self-contained.

https://codereview.chromium.org/15747004/diff/1001/tests/OSPathUtilsTest.cpp#...
tests/OSPathUtilsTest.cpp:19:
SkASSERT(!filename.contains(pathSeparator.c_str()));
On 2013/05/22 17:33:58, epoger wrote:
> Why not just 
> 
> SkASSERT(!filename.contains(SkPATH_SEPARATOR))

Done.

https://codereview.chromium.org/15747004/diff/1001/tests/OSPathUtilsTest.cpp#...
tests/OSPathUtilsTest.cpp:22: // fullName should be "dir<SkPATH_SEPARATOR>file"
On 2013/05/22 17:33:58, epoger wrote:
> I think this code would be easier to read with some extra newlines, like this:
> 
> // Tests for SkPathJoin and SkBasename
> 
> // fullName should be "dir<SkPATH_SEPARATOR>file"
> SkString fullName = SkPathJoin(dir.c_str(), filename.c_str());
> 
> // basename should be the same as filename
> REPORTER_ASSERT(reporter, 0 == strcmp(basename, filename.c_str()));
> 
> // Now take the basename of filename, which should be the same as filename
> basename = SkBasename(filename.c_str());
> REPORTER_ASSERT(reporter, 0 == strcmp(basename, filename.c_str()));

Done.

https://codereview.chromium.org/15747004/diff/1001/tests/OSPathUtilsTest.cpp#...
tests/OSPathUtilsTest.cpp:26: REPORTER_ASSERT(reporter, fullName.size() ==
dir.size() + filename.size()
On 2013/05/22 17:33:58, epoger wrote:
> Maybe this tighter test instead?
> 
> if (dir.endsWith(SkPATH_SEPARATOR)) {
>   expectedSize = dir.size() + filename.size();
> } else {
>   expectedSize = dir.size() + filename.size() + 1;
> }
> REPORTER_ASSERT(reporter, expectedSize == fullName.size());

Done.

https://codereview.chromium.org/15747004/diff/1001/tests/OSPathUtilsTest.cpp#...
tests/OSPathUtilsTest.cpp:28: const char* basename =
SkBasename(fullName.c_str());
On 2013/05/22 17:33:58, epoger wrote:
> I think the code would be easier to follow if you always used SkStrings, or
> always used char *, instead of a mix.

Done.

[reed1, 2013-05-23 15:44:07]

I think the names are too generic, and need some sort of scoping or prefixing to
indicate that they are about filenames and directories.

Off hand, I'd be fine with these living in SkOSFile.h

[epoger, 2013-05-23 15:51:12]

Leon- the particular changes in patchset 3 look good to me.  Let me know if
you'd like me to look again after you address Mike's toplevel comments.

https://codereview.chromium.org/15747004/diff/7001/tools/skimage_main.cpp
File tools/skimage_main.cpp (right):

https://codereview.chromium.org/15747004/diff/7001/tools/skimage_main.cpp#new...
tools/skimage_main.cpp:323: const char* filename = basename.c_str();
I think you could just do this:

const char* filename = SkBasename(srcPath).c_str();

[scroggo, 2013-05-24 16:23:37]

> I think the names are too generic, and need some sort of 
> scoping or prefixing to indicate that they are about 
> filenames and directories.

> Off hand, I'd be fine with these living in SkOSFile.h

Moved to SkOSFile.h, and put inside a namespace for scoping.

There seems to be a strange splitting of SkOSFile:

SkOSFile.h is in core.
SkOSFile.cpp is in utils.
SkOSFile_stdio.cpp is in ports.
SkOSFile_iOS.mm is in src/utils/ios, and only built by SampleApp (which fails to
build on iOS).

My new functions are not platform specific, so I put their implementation into
SkOSFile.cpp, though I wonder if that file should be built by core, along with
its header.

https://codereview.chromium.org/15747004/diff/7001/tools/skimage_main.cpp
File tools/skimage_main.cpp (right):

https://codereview.chromium.org/15747004/diff/7001/tools/skimage_main.cpp#new...
tools/skimage_main.cpp:323: const char* filename = basename.c_str();
On 2013/05/23 15:51:13, epoger wrote:
> I think you could just do this:
> 
> const char* filename = SkBasename(srcPath).c_str();

I don't think so. The returned result from c_str() depends on the SkString
sticking around.

https://codereview.chromium.org/15747004/diff/18001/gm/image.cpp
File gm/image.cpp (right):

https://codereview.chromium.org/15747004/diff/18001/gm/image.cpp#newcode128
gm/image.cpp:128: INHERITED::gResourcePath.c_str(), "CMYK.jpg");
Changing this file will require a rebaseline (assuming we have a checked in
expectation for this test). Should I go ahead and make the change and
rebaseline, or leave this change out?

[epoger, 2013-05-24 16:42:55]

LGTM

https://codereview.chromium.org/15747004/diff/7001/tools/skimage_main.cpp
File tools/skimage_main.cpp (right):

https://codereview.chromium.org/15747004/diff/7001/tools/skimage_main.cpp#new...
tools/skimage_main.cpp:323: const char* filename = basename.c_str();
On 2013/05/24 16:23:37, scroggo wrote:
> On 2013/05/23 15:51:13, epoger wrote:
> > I think you could just do this:
> > 
> > const char* filename = SkBasename(srcPath).c_str();
> 
> I don't think so. The returned result from c_str() depends on the SkString
> sticking around.

Ah, I think you're right.  In the code I suggested, SkBasename(srcPath) would be
assigned to an anonymous variable, which would have "expression" scope.  Thus,
the "filename" pointer would be pointing into no-longer-reserved memory at the
end of this C++ expression.  Not good.

See http://www.learncpp.com/cpp-tutorial/814-anonymous-variables-and-objects/

https://codereview.chromium.org/15747004/diff/18001/gm/image.cpp
File gm/image.cpp (right):

https://codereview.chromium.org/15747004/diff/18001/gm/image.cpp#newcode128
gm/image.cpp:128: INHERITED::gResourcePath.c_str(), "CMYK.jpg");
On 2013/05/24 16:23:37, scroggo wrote:
> Changing this file will require a rebaseline (assuming we have a checked in
> expectation for this test). Should I go ahead and make the change and
> rebaseline, or leave this change out?

I think your best bet is to leave this change out (add a TODO instead), since
it's separate from the main reason for your change.  I would hate for you to
have to revert this CL because of problems with this one test, etc.

https://codereview.chromium.org/15747004/diff/18001/include/core/SkOSFile.h
File include/core/SkOSFile.h (right):

https://codereview.chromium.org/15747004/diff/18001/include/core/SkOSFile.h#n...
include/core/SkOSFile.h:118: SkString SkPathJoin(const char *rootPath, const
char *relativePath);
Rather than creating the new SkOSPathUtils namespace, maybe make these static
methods in a new SkOSPath class?  That seems more consistent with the SkOSFile
class defined above.

But I don't feel strongly about it.

https://codereview.chromium.org/15747004/diff/18001/src/utils/SkOSFile.cpp
File src/utils/SkOSFile.cpp (right):

https://codereview.chromium.org/15747004/diff/18001/src/utils/SkOSFile.cpp#ne...
src/utils/SkOSFile.cpp:229: namespace SkOSPathUtils {
I agree that SkOSFile is declared/defined in a strange mix of locations.

Luckily, I don't think that's your problem to solve in this CL.  So I think
where you put stuff is fine.

[scroggo, 2013-05-24 17:36:05]

https://codereview.chromium.org/15747004/diff/18001/gm/image.cpp
File gm/image.cpp (right):

https://codereview.chromium.org/15747004/diff/18001/gm/image.cpp#newcode128
gm/image.cpp:128: INHERITED::gResourcePath.c_str(), "CMYK.jpg");
On 2013/05/24 16:42:55, epoger wrote:
> On 2013/05/24 16:23:37, scroggo wrote:
> > Changing this file will require a rebaseline (assuming we have a checked in
> > expectation for this test). Should I go ahead and make the change and
> > rebaseline, or leave this change out?
> 
> I think your best bet is to leave this change out (add a TODO instead), since
> it's separate from the main reason for your change.  I would hate for you to
> have to revert this CL because of problems with this one test, etc.

Done.

https://codereview.chromium.org/15747004/diff/18001/include/core/SkOSFile.h
File include/core/SkOSFile.h (right):

https://codereview.chromium.org/15747004/diff/18001/include/core/SkOSFile.h#n...
include/core/SkOSFile.h:118: SkString SkPathJoin(const char *rootPath, const
char *relativePath);
On 2013/05/24 16:42:55, epoger wrote:
> Rather than creating the new SkOSPathUtils namespace, maybe make these static
> methods in a new SkOSPath class?  That seems more consistent with the SkOSFile
> class defined above.
> 
> But I don't feel strongly about it.

I like that better. Done.

[epoger, 2013-05-24 17:53:17]

LGTM

[scroggo, 2013-05-24 18:12:28]

Committed patchset #7 manually as r9277 (presubmit successful).