Fix bug in setting directories for tests.

make_canonical_dir_path only worked if the provided directory
did not end with a slash. Remove this function, and call
SkPathJoin instead. Update the documentation to acknowledge
that this is an acceptable use of SkPathJoin, and update its
test.

R=epoger@google.com

Review URL: https://codereview.chromium.org/16098011

git-svn-id: http://skia.googlecode.com/svn/trunk@9458 2bbb7eff-a529-9590-31e7-b0007b416f81

include/core/SkOSFile.h

@@ -138,7 +138,9 @@ class SkOSPath {
 public:
     /**
      * Assembles rootPath and relativePath into a single path, like this:
-     * rootPath/relativePath
+     * rootPath/relativePath.
+     * It is okay to call with a NULL rootPath and/or relativePath. A path
+     * separator will still be inserted.
      *
      * Uses SkPATH_SEPARATOR, to work on all platforms.
      */

tests/OSPathTest.cpp

@@ -59,6 +59,12 @@ static void test_os_path_utils_tests(skiatest::Reporter* reporter) {
     dir.appendUnichar(SkPATH_SEPARATOR);
     test_dir_with_file(reporter, dir, filename);
 
+    // Test using no filename.
+    test_dir_with_file(reporter, dir, SkString());
+
+    // Testing using no directory.
+    test_dir_with_file(reporter, SkString(), filename);
+
     // Test with a sub directory.
     dir.append("subDir");
     test_dir_with_file(reporter, dir, filename);
@@ -71,6 +77,11 @@ static void test_os_path_utils_tests(skiatest::Reporter* reporter) {
     // Basename of NULL is an empty string.
     SkString empty = SkOSPath::SkBasename(NULL);
     REPORTER_ASSERT(reporter, empty.size() == 0);
+
+    // Test that NULL can be used for the directory and filename.
+    SkString emptyPath = SkOSPath::SkPathJoin(NULL, NULL);
+    REPORTER_ASSERT(reporter, emptyPath.size() == 1);
+    REPORTER_ASSERT(reporter, emptyPath.contains(SkPATH_SEPARATOR));
 }
 
 #include "TestClassDef.h"

tests/StreamTest.cpp

@@ -35,8 +35,7 @@ static void test_loop_stream(skiatest::Reporter* reporter, SkStream* stream,
 }
 
 static void test_filestreams(skiatest::Reporter* reporter, const char* tmpDir) {
-    SkString path;
-    path.printf("%s%s", tmpDir, "wstream_test");
+    SkString path = SkOSPath::SkPathJoin(tmpDir, "wstream_test");
 
     const char s[] = "abcdefghijklmnopqrstuvwxyz";
 
@@ -98,8 +97,9 @@ static void TestWStream(skiatest::Reporter* reporter) {
     }
     delete[] dst;
 
-    if (!skiatest::Test::GetTmpDir().isEmpty()) {
-        test_filestreams(reporter, skiatest::Test::GetTmpDir().c_str());
+    SkString tmpDir = skiatest::Test::GetTmpDir();
+    if (!tmpDir.isEmpty()) {
+        test_filestreams(reporter, tmpDir.c_str());
     }
 }
 

tests/Test.h

@@ -75,9 +75,9 @@ namespace skiatest {
         bool passed() const { return fPassed; }
         SkMSec elapsedMs() const { return fElapsed; }
 
-        static const SkString& GetTmpDir();
+        static SkString GetTmpDir();
 
-        static const SkString& GetResourcePath();
+        static SkString GetResourcePath();
 
         virtual bool isThreadsafe() const { return true; }
 

tests/skia_test.cpp

@@ -112,34 +112,6 @@ private:
     bool fAllowThreaded;
 };
 
-static const char* make_canonical_dir_path(const char* path, SkString* storage) {
-    if (path) {
-        // clean it up so it always has a trailing searator
-        size_t len = strlen(path);
-        if (0 == len) {
-            path = NULL;
-        } else if (SkPATH_SEPARATOR != path[len - 1]) {
-            // resize to len + 1, to make room for searator
-            storage->set(path, len + 1);
-            storage->writable_str()[len] = SkPATH_SEPARATOR;
-            path = storage->c_str();
-        }
-    }
-    return path;
-}
-
-static SkString gTmpDir;
-
-const SkString& Test::GetTmpDir() {
-    return gTmpDir;
-}
-
-static SkString gResourcePath;
-
-const SkString& Test::GetResourcePath() {
-    return gResourcePath;
-}
-
 DEFINE_string2(match, m, NULL, "[~][^]substring[$] [...] of test name to run.\n" \
                                "Multiple matches may be separated by spaces.\n" \
                                "~ causes a matching test to always be skipped\n" \
@@ -156,6 +128,16 @@ DEFINE_bool2(verbose, v, false, "enable verbose output.");
 DEFINE_int32(threads, SkThreadPool::kThreadPerCore,
              "Run threadsafe tests on a threadpool with this many threads.");
 
+SkString Test::GetTmpDir() {
+    const char* tmpDir = FLAGS_tmpDir.isEmpty() ? NULL : FLAGS_tmpDir[0];
+    return SkString(tmpDir);
+}
+
+SkString Test::GetResourcePath() {
+    const char* resourcePath = FLAGS_resourcePath.isEmpty() ? NULL : FLAGS_resourcePath[0];
+    return SkString(resourcePath);
+}
+
 // Deletes self when run.
 class SkTestRunnable : public SkRunnable {
 public:
@@ -218,13 +200,6 @@ int tool_main(int argc, char** argv) {
     SkCommandLineFlags::SetUsage("");
     SkCommandLineFlags::Parse(argc, argv);
 
-    if (!FLAGS_tmpDir.isEmpty()) {
-        make_canonical_dir_path(FLAGS_tmpDir[0], &gTmpDir);
-    }
-    if (!FLAGS_resourcePath.isEmpty()) {
-        make_canonical_dir_path(FLAGS_resourcePath[0], &gResourcePath);
-    }
-
 #if SK_ENABLE_INST_COUNT
     gPrintInstCount = true;
 #endif
@@ -239,11 +214,13 @@ int tool_main(int argc, char** argv) {
                 header.appendf(" %s", FLAGS_match[index]);
             }
         }
-        if (!gTmpDir.isEmpty()) {
-            header.appendf(" --tmpDir %s", gTmpDir.c_str());
+        SkString tmpDir = Test::GetTmpDir();
+        if (!tmpDir.isEmpty()) {
+            header.appendf(" --tmpDir %s", tmpDir.c_str());
         }
-        if (!gResourcePath.isEmpty()) {
-            header.appendf(" --resourcePath %s", gResourcePath.c_str());
+        SkString resourcePath = Test::GetResourcePath();
+        if (!resourcePath.isEmpty()) {
+            header.appendf(" --resourcePath %s", resourcePath.c_str());
         }
 #ifdef SK_DEBUG
         header.append(" SK_DEBUG");