Make the client authoritative for "Save" actions

Summary:
Ref T13559. When you click "Save" on an inline comment and it's empty, we may actually delete the comment.

Currently, the client and server both make decisions about whether the comment should be deleted. These decisions may not agree, causing the client state to fall out of sync.

Make the client authoritative about whether it wants to handle the user clicking the "Save" button as an intent to save or an intent to delete.

Test Plan: Saved empty and nonempty inlines. See followup changes.

Maniphest Tasks: T13559

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

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' => '442567d7',
+    'differential.pkg.js' => '7747755e',
     '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' => 'fdebbba6',
+    'rsrc/js/application/diff/DiffInline.js' => '34ccdeda',
     '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' => 'fdebbba6',
+    'phabricator-diff-inline' => '34ccdeda',
     'phabricator-diff-inline-content-state' => '68e6339d',
     'phabricator-diff-path-view' => '8207abf9',
     'phabricator-diff-tree-view' => '5d83623b',
@@ -1220,6 +1220,10 @@ 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',
@@ -2231,10 +2235,6 @@ return array(
     'fdc13e4e' => array(
       'javelin-install',
     ),
-    'fdebbba6' => array(
-      'javelin-dom',
-      'phabricator-diff-inline-content-state',
-    ),
     'ff688a7a' => array(
       'owners-path-editor',
       'javelin-behavior',

src/infrastructure/diff/PhabricatorInlineCommentController.php

@@ -186,23 +186,15 @@ abstract class PhabricatorInlineCommentController
         if ($op === 'save') {
           $this->updateCommentContentState($inline);
 
-          $inline->setIsEditing(false);
+          $inline
+            ->setIsEditing(false)
+            ->setIsDeleted(0);
 
-          if (!$inline->isVoidComment($viewer)) {
+          $this->saveComment($inline);
-            $inline->setIsDeleted(0);
 
-            $this->saveComment($inline);
+          return $this->buildRenderedCommentResponse(
-
+            $inline,
-            return $this->buildRenderedCommentResponse(
+            $this->getIsOnRight());
-              $inline,
-              $this->getIsOnRight());
-          } else {
-            $inline->setIsDeleted(1);
-
-            $this->saveComment($inline);
-
-            return $this->buildEmptyResponse();
-          }
         } else {
           // NOTE: At time of writing, the "editing" state of inlines is
           // preserved by simulating a click on "Edit" when the inline loads.

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

@@ -45,6 +45,7 @@ JX.install('DiffInline', {
     _undoRow: null,
     _undoType: null,
     _undoState: null,
+    _preventUndo: false,
 
     _draftRequest: null,
     _skipFocus: false,
@@ -161,6 +162,14 @@ JX.install('DiffInline', {
       return this._endOffset;
     },
 
+    _setPreventUndo: function(prevent_undo) {
+      this._preventUndo = prevent_undo;
+    },
+
+    _getPreventUndo: function() {
+      return this._preventUndo;
+    },
+
     setIsSelected: function(is_selected) {
       this._isSelected = is_selected;
 
@@ -617,7 +626,9 @@ JX.install('DiffInline', {
         this._editRow = null;
       }
 
-      this._drawUndeleteRows(state);
+      if (!this._getPreventUndo()) {
+        this._drawUndeleteRows(state);
+      }
 
       this.setLoading(false);
       this.setDeleted(true);
@@ -815,17 +826,53 @@ JX.install('DiffInline', {
     },
 
     save: function() {
+      if (this._shouldDeleteOnSave()) {
+        this._setPreventUndo(true);
+        this._applyDelete();
+      } else {
+        this._applySave();
+      }
+    },
+
+    _shouldDeleteOnSave: function() {
       var state = this._getActiveContentState();
-      var handler = JX.bind(this, this._onsubmitresponse);
+
+      // TODO: This is greatly simplified because we don't track all the
+      // state we need yet.
+
+      return !state.getText().length;
+    },
+
+    _applySave: function() {
+      var handler = JX.bind(this, this._onsaveresponse);
+
+      var state = this._getActiveContentState();
+      var data = this._newRequestData('save', state.getWireFormat());
+
+      this._applyCall(handler, data);
+    },
+
+    _applyDelete: function() {
+      var handler = JX.bind(this, this._ondeleteresponse);
+
+      var data = this._newRequestData('delete');
+
+      this._applyCall(handler, data);
+    },
+
+    _applyCall: function(handler, data) {
+      var uri = this._getInlineURI();
+
+      var callback = JX.bind(this, function() {
+        this.setLoading(false);
+        handler.apply(null, arguments);
+      });
 
       this.setLoading(true);
 
-      var uri = this._getInlineURI();
+      new JX.Workflow(uri, data)
-      var data = this._newRequestData('save', state.getWireFormat());
+        .setHandler(callback)
-
+        .start();
-      new JX.Request(uri, handler)
-        .setData(data)
-        .send();
     },
 
     undo: function() {
@@ -917,7 +964,7 @@ JX.install('DiffInline', {
       }
     },
 
-    _onsubmitresponse: function(response) {
+    _onsaveresponse: function(response) {
       if (this._editRow) {
         JX.DOM.remove(this._editRow);
         this._editRow = null;
@@ -927,30 +974,9 @@ JX.install('DiffInline', {
       this.setInvisible(false);
       this.setEditing(false);
 
-      this._onupdate(response);
+      var new_row = this._drawContentRows(JX.$H(response.view).getNode());
-    },
+      JX.DOM.remove(this._row);
-
+      this.bindToRow(new_row);
-    _onupdate: function(response) {
-      var new_row;
-      if (response.view) {
-        new_row = this._drawContentRows(JX.$H(response.view).getNode());
-      }
-
-      // TODO: Save the old row so the action it's undo-able if it was a
-      // delete.
-      var remove_old = true;
-      if (remove_old) {
-        JX.DOM.remove(this._row);
-      }
-
-      // If you delete the content on a comment and save it, it acts like a
-      // delete: the server does not return a new row.
-      if (new_row) {
-        this.bindToRow(new_row);
-      } else {
-        this.setDeleted(true);
-        this._row = null;
-      }
 
       this._didUpdate();
     },