Use modern two-stage markup cache (PhabricatorMarkupInterface) in Differential

Summary:
See T1963 for discussion of the Facebook-specific hack.

Differential currently uses a one-stage cache (render -> postprocess -> save in cache) rather than the two-stage cache (render -> save in cache -> postprocess) offered by `PhabricatorMarkupInteface`. This breaks Differential comments coming out of cache for the lightbox, and makes various other things suboptimal (status of handles like @mentions and embeds are not displayed accurately).

Instead, use the modern stuff.

Test Plan:
  - Created preview comments and inlines in Differential.
  - Edited a Differential inline.
  - Submitted main and inline Differential comments.
  - Viewed and edited Differential summary and test plan.
  - Created preview comments and inlines in Diffusion.
  - Submitted comments and inlines in Diffusion.
  - Verified Differential now loads and saves to the generalized markup cache (Diffusion is close, but main comments still hold a single-stage cache).
  - Verified old Differential comments work correctly with the lightbox.

Reviewers: vrana, btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1963

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

src/applications/audit/storage/PhabricatorAuditInlineComment.php

@@ -120,4 +120,28 @@ final class PhabricatorAuditInlineComment
     return $this->readField('authorPHID');
   }
 
+/* -(  PhabricatorMarkupInterface Implementation  )-------------------------- */
+
+
+  public function getMarkupFieldKey($field) {
+    return 'AI:'.$this->getID();
+  }
+
+  public function newMarkupEngine($field) {
+    return PhabricatorMarkupEngine::newDifferentialMarkupEngine();
+  }
+
+  public function getMarkupText($field) {
+    return $this->getContent();
+  }
+
+  public function didMarkupText($field, $output, PhutilMarkupEngine $engine) {
+    return $output;
+  }
+
+  public function shouldUseMarkupCache($field) {
+    // Only cache submitted comments.
+    return ($this->getID() && $this->getAuditCommentID());
+  }
+
 }

src/applications/differential/controller/DifferentialChangesetViewController.php

@@ -206,7 +206,16 @@ final class DifferentialChangesetViewController extends DifferentialController {
     $handles = $this->loadViewerHandles($phids);
     $parser->setHandles($handles);
 
-    $engine = PhabricatorMarkupEngine::newDifferentialMarkupEngine();
+    $engine = new PhabricatorMarkupEngine();
+    $engine->setViewer($request->getUser());
+
+    foreach ($inlines as $inline) {
+      $engine->addObject(
+        $inline,
+        PhabricatorInlineCommentInterface::MARKUP_FIELD_BODY);
+    }
+
+    $engine->process();
     $parser->setMarkupEngine($engine);
 
     if ($request->isAjax()) {

src/applications/differential/controller/DifferentialCommentPreviewController.php

@@ -33,7 +33,6 @@ final class DifferentialCommentPreviewController
 
     $action = $request->getStr('action');
 
-    $engine = PhabricatorMarkupEngine::newDifferentialMarkupEngine();
 
     $comment = new DifferentialComment();
     $comment->setContent($request->getStr('content'));
@@ -58,6 +57,11 @@ final class DifferentialCommentPreviewController
 
     $handles = $this->loadViewerHandles($handles);
 
+    $engine = new PhabricatorMarkupEngine();
+    $engine->setViewer($request->getUser());
+    $engine->addObject($comment, DifferentialComment::MARKUP_FIELD_BODY);
+    $engine->process();
+
     $view = new DifferentialRevisionCommentView();
     $view->setUser($request->getUser());
     $view->setComment($comment);

src/applications/differential/parser/DifferentialChangesetParser.php

@@ -250,7 +250,7 @@ final class DifferentialChangesetParser {
     return $this;
   }
 
-  public function setMarkupEngine(PhutilMarkupEngine $engine) {
+  public function setMarkupEngine(PhabricatorMarkupEngine $engine) {
     $this->markupEngine = $engine;
     return $this;
   }

src/applications/differential/storage/DifferentialComment.php

@@ -16,13 +16,16 @@
  * limitations under the License.
  */
 
-final class DifferentialComment extends DifferentialDAO {
+final class DifferentialComment extends DifferentialDAO
+  implements PhabricatorMarkupInterface {
 
   const METADATA_ADDED_REVIEWERS   = 'added-reviewers';
   const METADATA_REMOVED_REVIEWERS = 'removed-reviewers';
   const METADATA_ADDED_CCS         = 'added-ccs';
   const METADATA_DIFF_ID           = 'diff-id';
 
+  const MARKUP_FIELD_BODY          = 'markup:body';
+
   protected $authorPHID;
   protected $revisionID;
   protected $action;
@@ -31,6 +34,13 @@ final class DifferentialComment extends DifferentialDAO {
   protected $metadata = array();
   protected $contentSource;
 
+  private $arbitraryDiffForFacebook;
+
+  public function giveFacebookSomeArbitraryDiff(DifferentialDiff $diff) {
+    $this->arbitraryDiffForFacebook = $diff;
+    return $this;
+  }
+
   public function getConfiguration() {
     return array(
       self::CONFIG_SERIALIZATION => array(
@@ -48,4 +58,46 @@ final class DifferentialComment extends DifferentialDAO {
     return PhabricatorContentSource::newFromSerialized($this->contentSource);
   }
 
+
+  public function getMarkupFieldKey($field) {
+    if ($this->getID()) {
+      return 'DC:'.$this->getID();
+    }
+
+    // The summary and test plan render as comments, but do not have IDs.
+    // They are also mutable. Build keys using content hashes.
+    $hash = PhabricatorHash::digest($this->getMarkupText($field));
+    return 'DC:'.$hash;
+  }
+
+  public function newMarkupEngine($field) {
+    return PhabricatorMarkupEngine::newDifferentialMarkupEngine(
+      array(
+        'differential.diff' => $this->arbitraryDiffForFacebook,
+      ));
+  }
+
+  public function getMarkupText($field) {
+    return $this->getContent();
+  }
+
+  public function didMarkupText($field, $output, PhutilMarkupEngine $engine) {
+    return $output;
+  }
+
+  public function shouldUseMarkupCache($field) {
+    if ($this->getID()) {
+      return true;
+    }
+
+    $action = $this->getAction();
+    switch ($action) {
+      case DifferentialAction::ACTION_SUMMARIZE:
+      case DifferentialAction::ACTION_TESTPLAN:
+        return true;
+    }
+
+    return false;
+  }
+
 }

src/applications/differential/storage/DifferentialInlineComment.php

@@ -121,4 +121,29 @@ final class DifferentialInlineComment
     return $this->readField('authorPHID');
   }
 
+
+/* -(  PhabricatorMarkupInterface Implementation  )-------------------------- */
+
+
+  public function getMarkupFieldKey($field) {
+    return 'DI:'.$this->getID();
+  }
+
+  public function newMarkupEngine($field) {
+    return PhabricatorMarkupEngine::newDifferentialMarkupEngine();
+  }
+
+  public function getMarkupText($field) {
+    return $this->getContent();
+  }
+
+  public function didMarkupText($field, $output, PhutilMarkupEngine $engine) {
+    return $output;
+  }
+
+  public function shouldUseMarkupCache($field) {
+    // Only cache submitted comments.
+    return ($this->getID() && $this->getCommentID());
+  }
+
 }

src/applications/differential/view/DifferentialInlineCommentView.php

@@ -48,7 +48,7 @@ final class DifferentialInlineCommentView extends AphrontView {
     return $this;
   }
 
-  public function setMarkupEngine(PhutilMarkupEngine $engine) {
+  public function setMarkupEngine(PhabricatorMarkupEngine $engine) {
     $this->markupEngine = $engine;
     return $this;
   }
@@ -199,19 +199,9 @@ final class DifferentialInlineCommentView extends AphrontView {
       $links = null;
     }
 
-    $cache = $inline->getCache();
+    $content = $this->markupEngine->getOutput(
-    if (strlen($cache)) {
+      $inline,
-      $content = $cache;
+      PhabricatorInlineCommentInterface::MARKUP_FIELD_BODY);
-    } else {
-      $content = $this->markupEngine->markupText($content);
-      if ($inline->getID()) {
-        $inline->setCache($content);
-
-        $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
-        $inline->save();
-        unset($unguarded);
-      }
-    }
 
     if ($this->preview) {
       $anchor = null;

src/applications/differential/view/DifferentialRevisionCommentListView.php

@@ -77,9 +77,23 @@ final class DifferentialRevisionCommentListView extends AphrontView {
 
     require_celerity_resource('differential-revision-comment-list-css');
 
-    $engine = PhabricatorMarkupEngine::newDifferentialMarkupEngine(array(
+    $engine = new PhabricatorMarkupEngine();
-      'differential.diff' => $this->target
+    $engine->setViewer($this->user);
-    ));
+    foreach ($this->comments as $comment) {
+      $comment->giveFacebookSomeArbitraryDiff($this->target);
+
+      $engine->addObject(
+        $comment,
+        DifferentialComment::MARKUP_FIELD_BODY);
+    }
+
+    foreach ($this->inlines as $inline) {
+      $engine->addObject(
+        $inline,
+        PhabricatorInlineCommentInterface::MARKUP_FIELD_BODY);
+    }
+
+    $engine->process();
 
     $inlines = mgroup($this->inlines, 'getCommentID');
 

src/applications/differential/view/DifferentialRevisionCommentView.php

@@ -40,7 +40,7 @@ final class DifferentialRevisionCommentView extends AphrontView {
     return $this;
   }
 
-  public function setMarkupEngine($markup_engine) {
+  public function setMarkupEngine(PhabricatorMarkupEngine $markup_engine) {
     $this->markupEngine = $markup_engine;
     return $this;
   }
@@ -104,19 +104,11 @@ final class DifferentialRevisionCommentView extends AphrontView {
     $hide_comments = true;
     if (strlen(rtrim($content))) {
       $hide_comments = false;
-      $cache = $comment->getCache();
-      if (strlen($cache)) {
-        $content = $cache;
-      } else {
-        $content = $this->markupEngine->markupText($content);
-        if ($comment->getID()) {
-          $comment->setCache($content);
 
-          $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
+      $content = $this->markupEngine->getOutput(
-          $comment->save();
+        $comment,
-          unset($unguarded);
+        PhabricatorInlineCommentInterface::MARKUP_FIELD_BODY);
-        }
+
-      }
       $content =
         '<div class="phabricator-remarkup">'.
           $content.
@@ -292,9 +284,9 @@ final class DifferentialRevisionCommentView extends AphrontView {
           'id'      => $inline->getID(),
           'line'    => $inline->getLineNumber(),
           'length'  => $inline->getLineLength(),
-          'content' => PhabricatorInlineSummaryView::renderCommentContent(
+          'content' => $this->markupEngine->getOutput(
             $inline,
-            $this->markupEngine),
+            DifferentialInlineComment::MARKUP_FIELD_BODY),
         );
 
         if (!$is_visible) {
@@ -314,5 +306,4 @@ final class DifferentialRevisionCommentView extends AphrontView {
     return $view;
   }
 
-
 }

src/applications/diffusion/controller/DiffusionDiffController.php

@@ -64,12 +64,11 @@ final class DiffusionDiffController extends DiffusionController {
       return new Aphront404Response();
     }
 
+
     $parser = new DifferentialChangesetParser();
     $parser->setUser($user);
     $parser->setChangeset($changeset);
     $parser->setRenderingReference($diff_query->getRenderingReference());
-    $parser->setMarkupEngine(
-      PhabricatorMarkupEngine::newDiffusionMarkupEngine());
 
     $pquery = new DiffusionPathIDQuery(array($changeset->getFilename()));
     $ids = $pquery->loadPathIDs();
@@ -98,6 +97,19 @@ final class DiffusionDiffController extends DiffusionController {
       $parser->setHandles($handles);
     }
 
+    $engine = new PhabricatorMarkupEngine();
+    $engine->setViewer($user);
+
+    foreach ($inlines as $inline) {
+      $engine->addObject(
+        $inline,
+        PhabricatorInlineCommentInterface::MARKUP_FIELD_BODY);
+    }
+
+    $engine->process();
+
+    $parser->setMarkupEngine($engine);
+
     $spec = $request->getStr('range');
     list($range_s, $range_e, $mask) =
       DifferentialChangesetParser::parseRangeSpecification($spec);

src/infrastructure/diff/PhabricatorInlineCommentController.php

@@ -222,7 +222,12 @@ abstract class PhabricatorInlineCommentController
     $request = $this->getRequest();
     $user = $request->getUser();
 
-    $engine = PhabricatorMarkupEngine::newDifferentialMarkupEngine();
+    $engine = new PhabricatorMarkupEngine();
+    $engine->setViewer($user);
+    $engine->addObject(
+      $inline,
+      PhabricatorInlineCommentInterface::MARKUP_FIELD_BODY);
+    $engine->process();
 
     $phids = array($user->getPHID());
 

src/infrastructure/diff/PhabricatorInlineCommentPreviewController.php

@@ -27,7 +27,15 @@ abstract class PhabricatorInlineCommentPreviewController
 
     $inlines = $this->loadInlineComments();
     assert_instances_of($inlines, 'PhabricatorInlineCommentInterface');
-    $engine = PhabricatorMarkupEngine::newDifferentialMarkupEngine();
+
+    $engine = new PhabricatorMarkupEngine();
+    $engine->setViewer($user);
+    foreach ($inlines as $inline) {
+      $engine->addObject(
+        $inline,
+        PhabricatorInlineCommentInterface::MARKUP_FIELD_BODY);
+    }
+    $engine->process();
 
     $phids = array($user->getPHID());
     $handles = $this->loadViewerHandles($phids);

src/infrastructure/diff/interface/PhabricatorInlineCommentInterface.php

@@ -19,7 +19,9 @@
 /**
  * Shared interface used by Differential and Diffusion inline comments.
  */
-interface PhabricatorInlineCommentInterface {
+interface PhabricatorInlineCommentInterface extends PhabricatorMarkupInterface {
+
+  const MARKUP_FIELD_BODY = 'markup:body';
 
   public function setChangesetID($id);
   public function getChangesetID();