Modify skpdiff to write diffs directly to provided directories

tools/skpdiff/SkDiffContext.cpp

Index: tools/skpdiff/SkDiffContext.cpp
diff --git a/tools/skpdiff/SkDiffContext.cpp b/tools/skpdiff/SkDiffContext.cpp
index ea44c90da1e2efe22077bbc42c6b1d73a1353d34..8e193419a05c1c614bfab24c5d69db5bd46cf5f2 100644
--- a/tools/skpdiff/SkDiffContext.cpp
+++ b/tools/skpdiff/SkDiffContext.cpp
@@ -18,6 +18,9 @@
 #include "SkImageDiffer.h"
 #include "skpdiff_util.h"
 
+#include <string>
+#include <algorithm>
+
 SkDiffContext::SkDiffContext() {
     fDiffers = NULL;
     fDifferCount = 0;
@@ -48,6 +51,10 @@ void SkDiffContext::setWhiteDiffDir(const SkString& path) {
     }
 }
 
+void SkDiffContext::setLongNames(const bool useLongNames) {
+    longNames = useLongNames;
+}
+
 void SkDiffContext::setDiffers(const SkTDArray<SkImageDiffer*>& differs) {
     // Delete whatever the last array of differs was
     if (NULL != fDiffers) {
@@ -79,6 +86,20 @@ static SkString get_common_prefix(const SkString& a, const SkString& b) {
     }
 }
 
+static SkString sanitize_name(const SkString& str) {
+    std::string result = std::string(str.c_str());
+    std::replace(result.begin(), result.end(), '.', '_');
+
+    return SkString(result.c_str());
+}
+
+static SkString get_combined_name(const SkString& a, const SkString& b) {
+    SkString result = a;
+    result.append("-vs-");

[epoger, 2014/08/11 19:33:28]

Looking at _get_difference_locator() in imagediffdb.py, I see:

"We must keep this function in sync with getImageDiffRelativeUrl() in
static/loader.js"

I suspect that would be the case here too, right?

I worry that this change makes our code yet more fragile... now this naming
convention must be kept in sync between loader.js and this native skpdiff code
(which is even further away from loader.js than imagediffdb.py is)

Could we instead continue to construct the filename in imagediffdb.py, and pass
the fully constructed filename into skpdiff?  I think that would be more
robust...

[stephana, 2014/08/11 20:03:57]

Yes, I just realized that it breaks the front-end code. 
Ok, I'll construct the filename in imagediffdb.py and pass it to skpdiff. 

But I would also advocate to pass the relative URL of the diffs from the backend
to the frontend, instead of generating the name on the frontend. Getting rid of
that fragility. 

On 2014/08/11 19:33:28, epoger wrote:

+    result.append(b);
+    return sanitize_name(result);
+}

[stephana, 2014/08/11 14:58:31]

This uses the <string> and <algorithm> headers included above. I didn't see a
lot of use of the C++ standard library in the Skia source - is that something to
be avoided ? 

[epoger, 2014/08/11 19:33:28]

A fine question... in my experience, within the Skia code we have always favored
our homebrew library (SkString and such) rather than bringing in the C++
standard library.

In a tool like skpdiff (as opposed to the core API), it probably doesn't matter
either way... but I guess for the core API we don't want to depend on the C++
std library.

I recommend you ask Mike Reed for guidance on this.

In this particular case, it seems like SkString should offer a replace() method.
 Searching through our code (git grep replace | grep cpp), I find several
implementations:

src/utils/SkRTConf.cpp : static inline void str_replace(char *s, char search,
char replace)
tools/picture_utils.cpp : void replace_char(SkString* str, const char oldChar,
const char newChar)
tools/skimage_main.cpp : static void replace_char(SkString* str, const char
oldChar, const char newChar)

[stephana, 2014/08/11 20:03:57]

Make sense, I'll consult with Mike about this. 

On 2014/08/11 19:33:28, epoger wrote:

+
 void SkDiffContext::addDiff(const char* baselinePath, const char* testPath) {
     // Load the images at the paths
     SkBitmap baselineBitmap;
@@ -100,7 +121,13 @@ void SkDiffContext::addDiff(const char* baselinePath, const char* testPath) {
     // compute the common name
     SkString baseName = SkOSPath::Basename(baselinePath);
     SkString testName = SkOSPath::Basename(testPath);
-    newRecord->fCommonName = get_common_prefix(baseName, testName);
+
+    if (longNames) {
+        newRecord->fCommonName = get_combined_name(baseName, testName);
+    } else {
+        newRecord->fCommonName = get_common_prefix(baseName, testName);
+    }
+    newRecord->fCommonName.append(".png");
 
     newRecord->fBaselinePath = baselinePath;
     newRecord->fTestPath = testPath;