Reduce thumbnail flickering in comment previews

Summary:
Ref T10262. Currently, we always render a tag like this when you `{F123}` an image in remarkup:

```
<img src="/xform/preview/abcdef/" />
```

This either generates the preview or redirects to an existing preview. This is a good behavior in general, because the preview may take a while to generate and we don't want to wait for it to generate on the server side.

However, this flickers a lot in Safari. We might be able to cache this, but we really shouldn't, since the preview URI isn't a legitimately stable/permanent one.

Instead, do a (cheap) server-side check to see if the preview already exists. If it does, return a direct URI. This gives us a stable thumbnail in Safari.

Test Plan:
  - Dragged a dog picture into comment box.
  - Typed text.
  - Thing didn't flicker like crazy all the time in Safari.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10262

Differential Revision: https://secure.phabricator.com/D15646

Author: epriestley

src/aphront/response/AphrontResponse.php

@@ -192,18 +192,20 @@ protected function addJSONShield($json_response) {
   public function getCacheHeaders() {
     $headers = array();
     if ($this->cacheable) {
+      $cache_control = array();
+      $cache_control[] = sprintf('max-age=%d', $this->cacheable);
+
       if ($this->canCDN) {
-        $headers[] = array(
-          'Cache-Control',
-          'public',
-        );
+        $cache_control[] = 'public';
       } else {
-        $headers[] = array(
-          'Cache-Control',
-          'private',
-        );
+        $cache_control[] = 'private';
       }
 
+      $headers[] = array(
+        'Cache-Control',
+        implode(', ', $cache_control),
+      );
+
       $headers[] = array(
         'Expires',
         $this->formatEpochTimestampForHTTPHeader(time() + $this->cacheable),

src/applications/files/markup/PhabricatorEmbedFileRemarkupRule.php

@@ -16,6 +16,10 @@ protected function loadObjects(array $ids) {
     $objects = id(new PhabricatorFileQuery())
       ->setViewer($viewer)
       ->withIDs($ids)
+      ->needTransforms(
+        array(
+          PhabricatorFileThumbnailTransform::TRANSFORM_PREVIEW,
+        ))
       ->execute();
 
     $phids_key = self::KEY_EMBED_FILE_PHIDS;
@@ -109,7 +113,15 @@ private function renderImageFile(
         default:
           $preview_key = PhabricatorFileThumbnailTransform::TRANSFORM_PREVIEW;
           $xform = PhabricatorFileTransform::getTransformByKey($preview_key);
-          $attrs['src'] = $file->getURIForTransform($xform);
+
+          $existing_xform = $file->getTransform($preview_key);
+          if ($existing_xform) {
+            $xform_uri = $existing_xform->getCDNURI();
+          } else {
+            $xform_uri = $file->getURIForTransform($xform);
+          }
+
+          $attrs['src'] = $xform_uri;
 
           $dimensions = $xform->getTransformedDimensions($file);
           if ($dimensions) {

src/applications/files/query/PhabricatorFileQuery.php

@@ -15,6 +15,7 @@ final class PhabricatorFileQuery
   private $maxLength;
   private $names;
   private $isPartial;
+  private $needTransforms;
 
   public function withIDs(array $ids) {
     $this->ids = $ids;
@@ -117,6 +118,11 @@ public function showOnlyExplicitUploads($explicit_uploads) {
     return $this;
   }
 
+  public function needTransforms(array $transforms) {
+    $this->needTransforms = $transforms;
+    return $this;
+  }
+
   public function newResultObject() {
     return new PhabricatorFile();
   }
@@ -218,6 +224,44 @@ protected function loadPage() {
     return $files;
   }
 
+  protected function didFilterPage(array $files) {
+    $xform_keys = $this->needTransforms;
+    if ($xform_keys !== null) {
+      $xforms = id(new PhabricatorTransformedFile())->loadAllWhere(
+        'originalPHID IN (%Ls) AND transform IN (%Ls)',
+        mpull($files, 'getPHID'),
+        $xform_keys);
+
+      if ($xforms) {
+        $xfiles = id(new PhabricatorFile())->loadAllWhere(
+          'phid IN (%Ls)',
+          mpull($xforms, 'getTransformedPHID'));
+        $xfiles = mpull($xfiles, null, 'getPHID');
+      }
+
+      $xform_map = array();
+      foreach ($xforms as $xform) {
+        $xfile = idx($xfiles, $xform->getTransformedPHID());
+        if (!$xfile) {
+          continue;
+        }
+        $original_phid = $xform->getOriginalPHID();
+        $xform_key = $xform->getTransform();
+        $xform_map[$original_phid][$xform_key] = $xfile;
+      }
+
+      $default_xforms = array_fill_keys($xform_keys, null);
+
+      foreach ($files as $file) {
+        $file_xforms = idx($xform_map, $file->getPHID(), array());
+        $file_xforms += $default_xforms;
+        $file->attachTransforms($file_xforms);
+      }
+    }
+
+    return $files;
+  }
+
   protected function buildJoinClauseParts(AphrontDatabaseConnection $conn) {
     $joins = parent::buildJoinClauseParts($conn);
 

src/applications/files/storage/PhabricatorFile.php

@@ -56,6 +56,7 @@ final class PhabricatorFile extends PhabricatorFileDAO
   private $objects = self::ATTACHABLE;
   private $objectPHIDs = self::ATTACHABLE;
   private $originalFile = self::ATTACHABLE;
+  private $transforms = self::ATTACHABLE;
 
   public static function initializeNewFile() {
     $app = id(new PhabricatorApplicationQuery())
@@ -1208,6 +1209,15 @@ public function getRedirectResponse() {
       ->setURI($uri);
   }
 
+  public function attachTransforms(array $map) {
+    $this->transforms = $map;
+    return $this;
+  }
+
+  public function getTransform($key) {
+    return $this->assertAttachedKey($this->transforms, $key);
+  }
+
 
 /* -(  PhabricatorApplicationTransactionInterface  )------------------------- */