Make the client authoritative for "Cancel" actions

Summary:
Ref T13559. When the user clicks the "Cancel" button, we sometimes take it to mean "delete" (when the comment is empty).

Both the client and server make a decision about this, and they may not agree, which causes the client to fall out of sync.

Make the client responsible for deciding whether it wants to interpret a click on the "Cancel" button as a "revert" or a "delete".

Test Plan: Cancelled empty and nonempty comments, etc. See followup changes.

Maniphest Tasks: T13559

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

resources/celerity/map.php

@@ -13,7 +13,7 @@ return array(
     'core.pkg.js' => 'ab3502fe',
     'dark-console.pkg.js' => '187792c2',
     'differential.pkg.css' => 'ffb69e3d',
-    'differential.pkg.js' => '7747755e',
+    'differential.pkg.js' => 'fdec5f60',
     'diffusion.pkg.css' => '42c75c37',
     'diffusion.pkg.js' => '78c9885d',
     'maniphest.pkg.css' => '35995d6d',
@@ -385,7 +385,7 @@ return array(
     'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => '0116d3e8',
     'rsrc/js/application/diff/DiffChangeset.js' => 'd7d3ba75',
     'rsrc/js/application/diff/DiffChangesetList.js' => 'cc2c5de5',
-    'rsrc/js/application/diff/DiffInline.js' => '34ccdeda',
+    'rsrc/js/application/diff/DiffInline.js' => '2a6fac17',
     'rsrc/js/application/diff/DiffInlineContentState.js' => '68e6339d',
     'rsrc/js/application/diff/DiffPathView.js' => '8207abf9',
     'rsrc/js/application/diff/DiffTreeView.js' => '5d83623b',
@@ -788,7 +788,7 @@ return array(
     'phabricator-dashboard-css' => '5a205b9d',
     'phabricator-diff-changeset' => 'd7d3ba75',
     'phabricator-diff-changeset-list' => 'cc2c5de5',
-    'phabricator-diff-inline' => '34ccdeda',
+    'phabricator-diff-inline' => '2a6fac17',
     'phabricator-diff-inline-content-state' => '68e6339d',
     'phabricator-diff-path-view' => '8207abf9',
     'phabricator-diff-tree-view' => '5d83623b',
@@ -1166,6 +1166,10 @@ return array(
       'javelin-stratcom',
       'javelin-behavior',
     ),
+    '2a6fac17' => array(
+      'javelin-dom',
+      'phabricator-diff-inline-content-state',
+    ),
     '2a8b62d9' => array(
       'multirow-row-manager',
       'javelin-install',
@@ -1220,10 +1224,6 @@ return array(
       'javelin-stratcom',
       'javelin-workflow',
     ),
-    '34ccdeda' => array(
-      'javelin-dom',
-      'phabricator-diff-inline-content-state',
-    ),
     '34e2a838' => array(
       'aphront-typeahead-control-css',
       'phui-tag-view-css',

src/infrastructure/diff/PhabricatorInlineCommentController.php

@@ -248,10 +248,6 @@ abstract class PhabricatorInlineCommentController
         // to set the stored state back to "A".
         $this->updateCommentContentState($inline);
 
-        if ($inline->isVoidComment($viewer)) {
-          $inline->setIsDeleted(1);
-        }
-
         $this->saveComment($inline);
 
         return $this->buildEmptyResponse();

webroot/rsrc/js/application/diff/DiffInline.js

@@ -829,9 +829,10 @@ JX.install('DiffInline', {
       if (this._shouldDeleteOnSave()) {
         this._setPreventUndo(true);
         this._applyDelete();
-      } else {
+        return;
-        this._applySave();
       }
+
+      this._applySave();
     },
 
     _shouldDeleteOnSave: function() {
@@ -843,6 +844,29 @@ JX.install('DiffInline', {
       return !state.getText().length;
     },
 
+    _shouldDeleteOnCancel: function() {
+      var state = this._getActiveContentState();
+
+      // TODO: This is greatly simplified, too.
+
+      return !state.getText().length;
+    },
+
+    _shouldUndoOnCancel: function() {
+      var state = this._getActiveContentState().getWireFormat();
+
+      // TODO: This is also simplified.
+
+      var is_empty = this._isVoidContentState(state);
+      var is_same = this._isSameContentState(state, this._originalState);
+
+      if (!is_empty && !is_same) {
+        return true;
+      }
+
+      return false;
+    },
+
     _applySave: function() {
       var handler = JX.bind(this, this._onsaveresponse);
 
@@ -860,6 +884,14 @@ JX.install('DiffInline', {
       this._applyCall(handler, data);
     },
 
+    _applyCancel: function(state) {
+      var handler = JX.bind(this, this._onCancelResponse);
+
+      var data = this._newRequestData('cancel', state);
+
+      this._applyCall(handler, data);
+    },
+
     _applyCall: function(handler, data) {
       var uri = this._getInlineURI();
 
@@ -903,57 +935,34 @@ JX.install('DiffInline', {
     },
 
     cancel: function() {
-      var state = this._getActiveContentState().getWireFormat();
-
       JX.DOM.remove(this._editRow);
       this._editRow = null;
 
-      var is_empty = this._isVoidContentState(state);
+      if (this._shouldDeleteOnCancel()) {
-      var is_same = this._isSameContentState(state, this._originalState);
+        this._setPreventUndo(true);
-      if (!is_empty && !is_same) {
+        this._applyDelete();
-        this._drawUneditRows(state);
+        return;
       }
 
-      // If this was an empty box and we typed some text and then hit cancel,
+      if (this._shouldUndoOnCancel()) {
-      // don't show the empty concrete inline.
+        var state = this._getActiveContentState().getWireFormat();
-      if (this._isVoidContentState(this._originalState)) {
+        this._drawUneditRows(state);
-        this.setInvisible(true);
-      } else {
-        this.setInvisible(false);
       }
 
       // If you "undo" to restore text ("AB") and then "Cancel", we put you
       // back in the original text state ("A"). We also send the original
       // text ("A") to the server as the current persistent state.
 
-      var uri = this._getInlineURI();
+      this.setEditing(false);
-      var data = this._newRequestData('cancel', this._originalState);
+      this.setInvisible(false);
-      var handler = JX.bind(this, this._onCancelResponse);
 
-      this.setLoading(true);
+      this._applyCancel(this._originalState);
-
-      new JX.Request(uri, handler)
-        .setData(data)
-        .send();
 
       this._didUpdate(true);
     },
 
     _onCancelResponse: function(response) {
-      this.setEditing(false);
+      // Nothing to do.
-      this.setLoading(false);
-
-      // If the comment was empty when we started editing it (there's no
-      // original text) and empty when we finished editing it (there's no
-      // undo row), just delete the comment.
-      if (this._isVoidContentState(this._originalState) && !this.isUndo()) {
-        this.setDeleted(true);
-
-        JX.DOM.remove(this._row);
-        this._row = null;
-
-        this._didUpdate();
-      }
     },
 
     _getSuggestionNode: function(row) {
@@ -970,9 +979,8 @@ JX.install('DiffInline', {
         this._editRow = null;
       }
 
-      this.setLoading(false);
-      this.setInvisible(false);
       this.setEditing(false);
+      this.setInvisible(false);
 
       var new_row = this._drawContentRows(JX.$H(response.view).getNode());
       JX.DOM.remove(this._row);