This eliminates the need to copy the generated images from a temporary directory to the directory that is served by the rebaseline_server.

BUG=skia:2815, skia:2818
R=epoger@google.com

Author: stephana@google.com

Review URL: https://codereview.chromium.org/457203003
diff --git a/gm/rebaseline_server/imagediffdb.py b/gm/rebaseline_server/imagediffdb.py
index 167134a..3c5f45f 100644
--- a/gm/rebaseline_server/imagediffdb.py
+++ b/gm/rebaseline_server/imagediffdb.py
@@ -125,8 +125,10 @@
     try:
       skpdiff_summary_file = os.path.join(skpdiff_output_dir,
                                           'skpdiff-output.json')
-      skpdiff_rgbdiff_dir = os.path.join(skpdiff_output_dir, 'rgbDiff')
-      skpdiff_whitediff_dir = os.path.join(skpdiff_output_dir, 'whiteDiff')
+      skpdiff_rgbdiff_dir = os.path.join(storage_root, RGBDIFFS_SUBDIR)
+      skpdiff_whitediff_dir = os.path.join(storage_root, WHITEDIFFS_SUBDIR)
+      _mkdir_unless_exists(skpdiff_rgbdiff_dir)
+      _mkdir_unless_exists(skpdiff_rgbdiff_dir)
 
       # TODO(epoger): Consider calling skpdiff ONCE for all image pairs,
       # instead of calling it separately for each image pair.
@@ -134,9 +136,13 @@
       # spinning up the skpdiff binary, etc.
       # Con: we would have to wait until all image pairs were loaded before
       # generating any of the diffs?
+      # Note(stephana): '--longnames' was added to allow for this 
+      # case (multiple files at once) versus specifying output diffs 
+      # directly.
       find_run_binary.run_command(
           [SKPDIFF_BINARY, '-p', expected_image_file, actual_image_file,
            '--jsonp', 'false',
+           '--longnames', 'true',
            '--output', skpdiff_summary_file,
            '--differs', 'perceptual', 'different_pixels',
            '--rgbDiffDir', skpdiff_rgbdiff_dir,
@@ -157,8 +163,6 @@
       # See http://stackoverflow.com/a/626871
       self._max_diff_per_channel = [
           record['maxRedDiff'], record['maxGreenDiff'], record['maxBlueDiff']]
-      rgb_diff_path = record['rgbDiffPath']
-      white_diff_path = record['whiteDiffPath']
       per_differ_stats = record['diffs']
       for stats in per_differ_stats:
         differ_name = stats['differName']
@@ -175,22 +179,6 @@
       if not 0 <= perceptual_similarity <= 1:
         perceptual_similarity = 0
       self._perceptual_difference = 100 - (perceptual_similarity * 100)
-
-      # Store the rgbdiff and whitediff images generated above.
-      diff_image_locator = _get_difference_locator(
-          expected_image_locator=expected_image_locator,
-          actual_image_locator=actual_image_locator)
-      basename = str(diff_image_locator) + image_suffix
-      _mkdir_unless_exists(os.path.join(storage_root, RGBDIFFS_SUBDIR))
-      _mkdir_unless_exists(os.path.join(storage_root, WHITEDIFFS_SUBDIR))
-      # TODO: Modify skpdiff's behavior so we can tell it exactly where to
-      # write the image files into, rather than having to move them around
-      # after skpdiff writes them out.
-      shutil.copyfile(rgb_diff_path,
-                      os.path.join(storage_root, RGBDIFFS_SUBDIR, basename))
-      shutil.copyfile(white_diff_path,
-                      os.path.join(storage_root, WHITEDIFFS_SUBDIR, basename))
-
     finally:
       shutil.rmtree(skpdiff_output_dir)
 
@@ -479,20 +467,3 @@
     return DISALLOWED_FILEPATH_CHAR_REGEX.sub('_', str(locator))
   else:
     return locator
-
-def _get_difference_locator(expected_image_locator, actual_image_locator):
-  """Returns the locator string used to look up the diffs between expected_image
-  and actual_image.
-
-  We must keep this function in sync with getImageDiffRelativeUrl() in
-  static/loader.js
-
-  Args:
-    expected_image_locator: locator string pointing at expected image
-    actual_image_locator: locator string pointing at actual image
-
-  Returns: already-sanitized locator where the diffs between expected and
-      actual images can be found
-  """
-  return "%s-vs-%s" % (_sanitize_locator(expected_image_locator),
-                       _sanitize_locator(actual_image_locator))
diff --git a/gm/rebaseline_server/static/live-loader.js b/gm/rebaseline_server/static/live-loader.js
index 16a19aa..cd4560f 100644
--- a/gm/rebaseline_server/static/live-loader.js
+++ b/gm/rebaseline_server/static/live-loader.js
@@ -666,12 +666,35 @@
      * @param key (string): sort by value associated with this key in subdict
      */
     $scope.sortResultsBy = function(subdict, key) {
-      $scope.sortColumnSubdict = subdict;
-      $scope.sortColumnKey = key;
+      // if we are already sorting by this column then toggle between asc/desc
+      if ((subdict === $scope.sortColumnSubdict) && ($scope.sortColumnKey === key)) {
+        currSortAsc = !currSortAsc;
+      } else {
+        $scope.sortColumnSubdict = subdict;
+        $scope.sortColumnKey = key;
+        currSortAsc = true; 
+      }
       $scope.updateResults();
     }
 
     /**
+     * Returns ASC or DESC (from constants) if currently the data
+     * is sorted by the provided column. 
+     *
+     * @param colName: name of the column for which we need to get the class.
+     */
+
+    $scope.sortedByColumnsCls = function (colName) {
+      if ($scope.sortColumnKey !== colName) {
+        return '';
+      }
+
+      var result = (currSortAsc) ? constants.ASC : constants.DESC;
+      console.log("sort class:", result);
+      return result;
+    };
+
+    /**
      * For a particular ImagePair, return the value of the column we are
      * sorting on (according to $scope.sortColumnSubdict and
      * $scope.sortColumnKey).
@@ -686,7 +709,7 @@
       } else {
         return undefined;
       }
-    }
+    };
 
     /**
      * For a particular ImagePair, return the value we use for the
@@ -701,7 +724,7 @@
     $scope.getSecondOrderSortValue = function(imagePair) {
       return imagePair[constants.KEY__IMAGEPAIRS__IMAGE_A_URL] + "-vs-" +
           imagePair[constants.KEY__IMAGEPAIRS__IMAGE_B_URL];
-    }
+    };
 
     /**
      * Set $scope.columnStringMatch[name] = value, and update results.
@@ -712,7 +735,7 @@
     $scope.setColumnStringMatch = function(name, value) {
       $scope.columnStringMatch[name] = value;
       $scope.updateResults();
-    }
+    };
 
     /**
      * Update $scope.showingColumnValues[columnName] and $scope.columnStringMatch[columnName]
@@ -727,7 +750,7 @@
       $scope.showingColumnValues[columnName] = {};
       $scope.toggleValueInSet(columnValue, $scope.showingColumnValues[columnName]);
       $scope.updateResults();
-    }
+    };
 
     /**
      * Update $scope.showingColumnValues[columnName] and $scope.columnStringMatch[columnName]
@@ -742,7 +765,7 @@
       $scope.toggleValuesInSet($scope.allColumnValues[columnName],
                                $scope.showingColumnValues[columnName]);
       $scope.updateResults();
-    }
+    };
 
 
     //
@@ -852,7 +875,7 @@
             "Please see server-side log for details.");
         $scope.submitPending = false;
       });
-    }
+    };
 
 
     //
@@ -870,7 +893,7 @@
      */
     $scope.setSize = function(set) {
       return Object.keys(set).length;
-    }
+    };
 
     /**
      * Returns true if value "value" is present within set "set".
@@ -881,7 +904,7 @@
      */
     $scope.isValueInSet = function(value, set) {
       return (true == set[value]);
-    }
+    };
 
     /**
      * If value "value" is already in set "set", remove it; otherwise, add it.
@@ -895,7 +918,7 @@
       } else {
         set[value] = true;
       }
-    }
+    };
 
     /**
      * For each value in valueArray, call toggleValueInSet(value, set).
@@ -908,7 +931,7 @@
       for (var i = 0; i < arrayLength; i++) {
         $scope.toggleValueInSet(valueArray[i], set);
       }
-    }
+    };
 
 
     //
@@ -925,7 +948,7 @@
      */
     $scope.isValueInArray = function(value, array) {
       return (-1 != array.indexOf(value));
-    }
+    };
 
     /**
      * If value "value" is already in array "array", remove it; otherwise,
@@ -941,7 +964,7 @@
       } else {
         array.splice(i, 1);
       }
-    }
+    };
 
 
     //
@@ -969,7 +992,7 @@
         slice.push(array[row][column]);
       }
       return slice;
-    }
+    };
 
     /**
      * Returns a human-readable (in local time zone) time string for a
@@ -980,7 +1003,7 @@
     $scope.localTimeString = function(secondsPastEpoch) {
       var d = new Date(secondsPastEpoch * 1000);
       return d.toString();
-    }
+    };
 
     /**
      * Returns a hex color string (such as "#aabbcc") for the given RGB values.
@@ -1003,7 +1026,7 @@
         bString = "0" + bString;
       }
       return '#' + rString + gString + bString;
-    }
+    };
 
     /**
      * Returns a hex color string (such as "#aabbcc") for the given brightness.
@@ -1016,7 +1039,7 @@
     $scope.brightnessStringToHexColor = function(brightnessString) {
       var v = parseInt(brightnessString);
       return $scope.hexColorString(v, v, v);
-    }
+    };
 
     /**
      * Returns the last path component of image diff URL for a given ImagePair.
@@ -1034,7 +1057,7 @@
           imagePair[constants.KEY__IMAGEPAIRS__IMAGE_A_URL] + "-vs-" +
           imagePair[constants.KEY__IMAGEPAIRS__IMAGE_B_URL];
       return before.replace(/[^\w\-]/g, "_") + ".png";
-    }
+    };
 
   }
 );
diff --git a/gm/rebaseline_server/static/live-view.html b/gm/rebaseline_server/static/live-view.html
index cb50378..8593adc 100644
--- a/gm/rebaseline_server/static/live-view.html
+++ b/gm/rebaseline_server/static/live-view.html
@@ -261,7 +261,6 @@
                class="sortable-header">
                   bugs
             </a>
-            bugs
           </th>
           <th width="{{imageSize}}">
             <a ng-class="'sort-' + sortedByColumnsCls(constants.KEY__IMAGEPAIRS__IMAGE_A_URL)"
diff --git a/gyp/tools.gyp b/gyp/tools.gyp
index 46d668b..1766a09 100644
--- a/gyp/tools.gyp
+++ b/gyp/tools.gyp
@@ -170,10 +170,12 @@
       ],
       'include_dirs': [
         '../src/core/', # needed for SkTLList.h
+        '../tools/',    # needed for picture_utils::replace_char
       ],
       'dependencies': [
         'flags.gyp:flags',
         'skia_lib.gyp:skia_lib',
+        'tools.gyp:picture_utils',
       ],
       'cflags': [
         '-O3',
diff --git a/tools/skpdiff/SkDiffContext.cpp b/tools/skpdiff/SkDiffContext.cpp
index ea44c90..42d20de 100644
--- a/tools/skpdiff/SkDiffContext.cpp
+++ b/tools/skpdiff/SkDiffContext.cpp
@@ -14,6 +14,9 @@
 #include "SkTDict.h"
 #include "SkThreadPool.h"
 
+// from the tools directory for replace_char(...)
+#include "picture_utils.h"
+
 #include "SkDiffContext.h"
 #include "SkImageDiffer.h"
 #include "skpdiff_util.h"
@@ -48,6 +51,10 @@
     }
 }
 
+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,16 @@
     }
 }
 
+static SkString get_combined_name(const SkString& a, const SkString& b) {
+    // Note (stephana): We must keep this function in sync with 
+    // getImageDiffRelativeUrl() in static/loader.js (under rebaseline_server).
+    SkString result = a;
+    result.append("-vs-");
+    result.append(b);
+    sk_tools::replace_char(&result, '.', '_');
+    return result;
+}
+
 void SkDiffContext::addDiff(const char* baselinePath, const char* testPath) {
     // Load the images at the paths
     SkBitmap baselineBitmap;
@@ -100,7 +117,13 @@
     // 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;
diff --git a/tools/skpdiff/SkDiffContext.h b/tools/skpdiff/SkDiffContext.h
index 996737f..8f4789f 100644
--- a/tools/skpdiff/SkDiffContext.h
+++ b/tools/skpdiff/SkDiffContext.h
@@ -52,6 +52,25 @@
     void setWhiteDiffDir(const SkString& directory);
 
     /**
+     * Modify the pattern used to generate commonName (= the 
+     * basename of rgb/white diff files).
+     *
+     * - true: basename is a combination of the input file names.
+     * - false: basename is the common prefix of the input file names.
+     *
+     * For example, for:
+     *   baselinePath=/tmp/dir/image-before.png
+     *   testPath=/tmp/dir/image-after.png
+     * 
+     * If setLongNames(true), commonName would be:
+     *    image-before-png-vs-image-after-png.png
+     * 
+     * If setLongNames(false), commonName would be:
+     *   image-.png
+     */
+    void setLongNames(const bool useLongNames);
+
+    /**
      * Sets the differs to be used in each diff. Already started diffs will not retroactively use
      * these.
      * @param differs An array of differs to use. The array is copied, but not the differs
@@ -85,8 +104,9 @@
      *
      * The format of the JSON document is one top level array named "records".
      * Each record in the array is an object with the following values:
-     *    "commonName"     : string containing the common prefix of the baselinePath
-     *                       and testPath filenames
+     *    "commonName"     : string containing the output filename (basename) 
+     *                       depending on the value of 'longNames'. 
+     *                       (see 'setLongNames' for an explanation and example).
      *    "baselinePath"   : string containing the path to the baseline image
      *    "testPath"       : string containing the path to the test image
      *    "differencePath" : (optional) string containing the path to an alpha
@@ -177,6 +197,7 @@
     SkString fAlphaMaskDir;
     SkString fRgbDiffDir;
     SkString fWhiteDiffDir;
+    bool longNames;
 };
 
 #endif
diff --git a/tools/skpdiff/skpdiff_main.cpp b/tools/skpdiff/skpdiff_main.cpp
index 3d1bcda..6c40552 100644
--- a/tools/skpdiff/skpdiff_main.cpp
+++ b/tools/skpdiff/skpdiff_main.cpp
@@ -49,6 +49,7 @@
 DEFINE_bool(jsonp, true, "Output JSON with padding");
 DEFINE_string(csv, "", "Writes the output of these diffs to a csv file: <filepath>");
 DEFINE_int32(threads, -1, "run N threads in parallel [default is derived from CPUs available]");
+DEFINE_bool(longnames, false, "Output image names are a combination of baseline and test names");
 
 #if SK_SUPPORT_OPENCL
 /// A callback for any OpenCL errors
@@ -206,6 +207,7 @@
             return 1;
         }
     }
+
     if (!FLAGS_whiteDiffDir.isEmpty()) {
         if (1 != FLAGS_whiteDiffDir.count()) {
             SkDebugf("whiteDiffDir flag expects one argument: <directory>\n");
@@ -215,6 +217,7 @@
 
     SkDiffContext ctx;
     ctx.setDiffers(chosenDiffers);
+    ctx.setLongNames(FLAGS_longnames);
 
     if (!FLAGS_alphaDir.isEmpty()) {
         ctx.setAlphaMaskDir(SkString(FLAGS_alphaDir[0]));