Merge changes Ia0cb78aa,I96f714c7,Ia85bb230

* changes:
  Gr-diff retrofit: diff section state.
  Gr-diff retrofit: store comment state
  Gr-diff retrofit: implement re-render.

polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.js

@@ -61,10 +61,8 @@
       }
 
       var draft = this._newDraft(opt_lineNum);
+      draft.__editing = true;
       this.push('comments', draft);
-      this.async(function() {
-        this._commentElWithDraftID(draft.__draftID).editing = true;
-      }.bind(this), 1);
     },
 
     _getLoggedIn: function() {
@@ -121,13 +119,8 @@
             function(line) { return ' > ' + line; }).join('\n') + '\n\n';
       }
       var reply = this._newReply(comment.id, comment.line, quoteStr);
+      reply.__editing = true;
       this.push('comments', reply);
-
-      // Allow the reply to render in the dom-repeat.
-      this.async(function() {
-        var commentEl = this._commentElWithDraftID(reply.__draftID);
-        commentEl.editing = true;
-      }.bind(this), 1);
     },
 
     _handleCommentDone: function(e) {

polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.js

@@ -15,6 +15,7 @@
   'use strict';
 
   var STORAGE_DEBOUNCE_INTERVAL = 400;
+  var UPDATE_DEBOUNCE_INTERVAL = 500;
 
   Polymer({
     is: 'gr-diff-comment',
@@ -43,11 +44,18 @@
      * @event comment-save
      */
 
+    /**
+     * Fired when this comment is updated.
+     *
+     * @event comment-update
+     */
+
     properties: {
       changeNum: String,
       comment: {
         type: Object,
         notify: true,
+        observer: '_commentChanged',
       },
       disabled: {
         type: Boolean,
@@ -81,6 +89,10 @@
       '_loadLocalDraft(changeNum, patchNum, comment)',
     ],
 
+    detached: function() {
+      this.flushDebouncer('fire-update');
+    },
+
     save: function() {
       this.comment.message = this._messageText;
       this.disabled = true;
@@ -106,8 +118,7 @@
           }
           this.comment = comment;
           this.editing = false;
-          this.fire('comment-save');
+          this.fire('comment-save', {comment: this.comment});
-
           return obj;
         }.bind(this));
       }.bind(this)).catch(function(err) {
@@ -116,6 +127,16 @@
       }.bind(this));
     },
 
+    _commentChanged: function(comment) {
+      this.editing = !!comment.__editing;
+    },
+
+    _fireUpdate: function() {
+      this.debounce('fire-update', function() {
+        this.fire('comment-update', {comment: this.comment});
+      }, UPDATE_DEBOUNCE_INTERVAL);
+    },
+
     _draftChanged: function(draft) {
       this.$.container.classList.toggle('draft', draft);
     },
@@ -134,6 +155,10 @@
       if (this.comment && this.comment.id) {
         this.$$('.cancel').hidden = !editing;
       }
+      if (this.comment) {
+        this.comment.__editing = this.editing;
+      }
+      this._fireUpdate();
     },
 
     _computeLinkToComment: function(comment) {
@@ -174,6 +199,7 @@
         } else {
           this.$.storage.setDraftComment(commentLocation, message);
         }
+        this._fireUpdate();
       }, STORAGE_DEBOUNCE_INTERVAL);
     },
 
@@ -218,7 +244,7 @@
     _handleCancel: function(e) {
       this._preventDefaultAndBlur(e);
       if (this.comment.message == null || this.comment.message.length == 0) {
-        this.fire('comment-discard');
+        this.fire('comment-discard', {comment: this.comment});
         return;
       }
       this._messageText = this.comment.message;
@@ -234,20 +260,20 @@
       this.disabled = true;
       if (!this.comment.id) {
         this.disabled = false;
-        this.fire('comment-discard');
+        this.fire('comment-discard', {comment: this.comment});
         return;
       }
 
-      this._xhrPromise =
+      this._xhrPromise = this._deleteDraft(this.comment).then(
-          this._deleteDraft(this.comment).then(function(response) {
+          function(response) {
-        this.disabled = false;
+            this.disabled = false;
-        if (!response.ok) { return response; }
+            if (!response.ok) { return response; }
 
-        this.fire('comment-discard');
+            this.fire('comment-discard', {comment: this.comment});
-      }.bind(this)).catch(function(err) {
+          }.bind(this)).catch(function(err) {
-        this.disabled = false;
+            this.disabled = false;
-        throw err;
+            throw err;
-      }.bind(this));;
+          }.bind(this));
     },
 
     _preventDefaultAndBlur: function(e) {

polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment_test.html

@@ -204,16 +204,50 @@ limitations under the License.
     });
 
     test('draft saving/editing', function(done) {
+      var fireStub = sinon.stub(element, 'fire');
+
       element.draft = true;
       MockInteractions.tap(element.$$('.edit'));
       element._messageText = 'good news, everyone!';
+      element.flushDebouncer('fire-update');
+      element.flushDebouncer('store');
+      assert(fireStub.calledWith('comment-update'),
+             'comment-update should be sent');
+      assert.deepEqual(fireStub.lastCall.args, [
+        'comment-update', {
+          comment: {
+            __draft: true,
+            __draftID: 'temp_draft_id',
+            __editing: true,
+            line: 5,
+            path: '/path/to/file',
+          },
+        },
+      ]);
       MockInteractions.tap(element.$$('.save'));
+
       assert.isTrue(element.disabled,
           'Element should be disabled when creating draft.');
 
       element._xhrPromise.then(function(draft) {
+        assert(fireStub.calledWith('comment-save'),
+               'comment-save should be sent');
+        assert.deepEqual(fireStub.lastCall.args, [
+          'comment-save', {
+            comment: {
+              __draft: true,
+              __draftID: 'temp_draft_id',
+              __editing: false,
+              id: 'baf0414d_40572e03',
+              line: 5,
+              message: 'saved!',
+              path: '/path/to/file',
+              updated: '2015-12-08 21:52:36.177000000',
+            },
+          },
+        ]);
         assert.isFalse(element.disabled,
-            'Element should be enabled when done creating draft.');
+                       'Element should be enabled when done creating draft.');
         assert.equal(draft.message, 'saved!');
         assert.isFalse(element.editing);
       }).then(function() {

polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor.js

@@ -172,9 +172,9 @@
     },
 
     _rowHasSide: function(row) {
-      var selector = '.content';
+      var selector = (this.side === DiffSides.LEFT ? '.left' : '.right') +
-      selector += this.side === DiffSides.LEFT ? '.left' : '.right';
+          ' + .content';
-      return row.querySelector(selector);
+      return !!row.querySelector(selector);
     },
 
     _isFirstRowOfChunk: function(row) {
@@ -278,7 +278,7 @@
         }
 
         for (i = 0;
-            i < splice.removed && splicee.removed.length;
+            i < splice.removed && splice.removed.length;
             i++) {
           this.unlisten(splice.removed[i], 'render', 'handleDiffUpdate');
         }

polygerrit-ui/app/elements/diff/gr-diff/gr-diff-builder-side-by-side.js

@@ -20,7 +20,7 @@
   GrDiffBuilderSideBySide.prototype = Object.create(GrDiffBuilder.prototype);
   GrDiffBuilderSideBySide.prototype.constructor = GrDiffBuilderSideBySide;
 
-  GrDiffBuilderSideBySide.prototype.emitGroup = function(group,
+  GrDiffBuilderSideBySide.prototype.buildSectionElement = function(group,
       opt_beforeSection) {
     var sectionEl = this._createElement('tbody', 'section');
     sectionEl.classList.add(group.type);
@@ -29,7 +29,7 @@
       sectionEl.appendChild(this._createRow(sectionEl, pairs[i].left,
           pairs[i].right));
     }
-    this._outputEl.insertBefore(sectionEl, opt_beforeSection);
+    return sectionEl;
   };
 
   GrDiffBuilderSideBySide.prototype._createRow = function(section, leftLine,
@@ -48,13 +48,14 @@
 
   GrDiffBuilderSideBySide.prototype._appendPair = function(section, row, line,
       lineNumber, side) {
-    row.appendChild(this._createLineEl(line, lineNumber, line.type, side));
+    var lineEl = this._createLineEl(line, lineNumber, line.type, side);
+    lineEl.classList.add(side);
+    row.appendChild(lineEl);
     var action = this._createContextControl(section, line);
     if (action) {
       row.appendChild(action);
     } else {
       var textEl = this._createTextEl(line);
-      textEl.classList.add(side);
       var threadEl = this._commentThreadForLine(line, side);
       if (threadEl) {
         textEl.appendChild(threadEl);

polygerrit-ui/app/elements/diff/gr-diff/gr-diff-builder-unified.js

@@ -20,23 +20,26 @@
   GrDiffBuilderUnified.prototype = Object.create(GrDiffBuilder.prototype);
   GrDiffBuilderUnified.prototype.constructor = GrDiffBuilderUnified;
 
-  GrDiffBuilderUnified.prototype.emitGroup = function(group,
+  GrDiffBuilderUnified.prototype.buildSectionElement = function(group) {
-      opt_beforeSection) {
     var sectionEl = this._createElement('tbody', 'section');
     sectionEl.classList.add(group.type);
 
     for (var i = 0; i < group.lines.length; ++i) {
       sectionEl.appendChild(this._createRow(sectionEl, group.lines[i]));
     }
-    this._outputEl.insertBefore(sectionEl, opt_beforeSection);
+    return sectionEl;
   };
 
   GrDiffBuilderUnified.prototype._createRow = function(section, line) {
     var row = this._createElement('tr', line.type);
-    row.appendChild(this._createLineEl(line, line.beforeNumber,
+    var lineEl = this._createLineEl(line, line.beforeNumber,
-        GrDiffLine.Type.REMOVE));
+        GrDiffLine.Type.REMOVE);
-    row.appendChild(this._createLineEl(line, line.afterNumber,
+    lineEl.classList.add('left');
-        GrDiffLine.Type.ADD));
+    row.appendChild(lineEl);
+    lineEl = this._createLineEl(line, line.afterNumber,
+        GrDiffLine.Type.ADD);
+    lineEl.classList.add('right');
+    row.appendChild(lineEl);
     row.classList.add('diff-row', 'unified');
 
     var action = this._createContextControl(section, line);

polygerrit-ui/app/elements/diff/gr-diff/gr-diff-builder.js

@@ -57,8 +57,51 @@
     }
   };
 
+  GrDiffBuilder.prototype.buildSectionElement = function(
+      group, opt_beforeSection) {
+    throw Error('Subclasses must implement buildGroupElement');
+  };
+
   GrDiffBuilder.prototype.emitGroup = function(group, opt_beforeSection) {
-    throw Error('Subclasses must implement emitGroup');
+    var element = this.buildSectionElement(group);
+    this._outputEl.insertBefore(element, opt_beforeSection);
+    group.element = element;
+  };
+
+  GrDiffBuilder.prototype.renderSection = function(element) {
+    for (var i = 0; i < this._groups.length; i++) {
+      var group = this._groups[i];
+      if (group.element === element) {
+        var newElement = this.buildSectionElement(group);
+        group.element.parentElement.replaceChild(newElement, group.element);
+        group.element = newElement;
+        break;
+      }
+    }
+  };
+
+  GrDiffBuilder.prototype.getSectionsByLineRange = function(
+      startLine, endLine, opt_side) {
+    var sections = [];
+    for (var i = 0; i < this._groups.length; i++) {
+      var group = this._groups[i];
+      if (group.lines.length === 0) {
+        continue;
+      }
+      var groupStartLine;
+      var groupEndLine;
+      if (opt_side === GrDiffBuilder.Side.LEFT) {
+        groupStartLine = group.lines[0].beforeNumber;
+        groupEndLine = group.lines[group.lines.length - 1].beforeNumber;
+      } else if (opt_side === GrDiffBuilder.Side.RIGHT) {
+        groupStartLine = group.lines[0].afterNumber;
+        groupEndLine = group.lines[group.lines.length - 1].afterNumber;
+      }
+      if (startLine <= groupEndLine && endLine >= groupStartLine) {
+        sections.push(group.element);
+      }
+    }
+    return sections;
   };
 
   GrDiffBuilder.prototype._processContent = function(content, groups, context) {
@@ -183,6 +226,7 @@
           currentChunk.ab.push(chunk[j]);
         }
       }
+      // != instead of !== because we want to cover both undefined and null.
       if (currentChunk.ab != null && currentChunk.ab.length > 0) {
         result.push(currentChunk);
       }
@@ -261,7 +305,8 @@
     }
 
     var ctxLine = new GrDiffLine(GrDiffLine.Type.CONTEXT_CONTROL);
-    ctxLine.contextLines = hiddenLines;
+    ctxLine.contextGroup =
+        new GrDiffGroup(GrDiffGroup.Type.BOTH, hiddenLines);
     groups.push(new GrDiffGroup(GrDiffGroup.Type.CONTEXT_CONTROL,
         [ctxLine]));
 
@@ -311,13 +356,14 @@
   };
 
   GrDiffBuilder.prototype._createContextControl = function(section, line) {
-    if (!line.contextLines.length) {
+    if (!line.contextGroup || !line.contextGroup.lines.length) {
       return null;
     }
+    var contextLines = line.contextGroup.lines;
     var td = this._createElement('td');
     var button = this._createElement('gr-button', 'showContext');
     button.setAttribute('link', true);
-    var commonLines = line.contextLines.length;
+    var commonLines = contextLines.length;
     var text = 'Show ' + commonLines + ' common line';
     if (commonLines > 1) {
       text += 's';
@@ -326,7 +372,7 @@
     button.textContent = text;
     button.addEventListener('tap', function(e) {
       e.detail = {
-        group: new GrDiffGroup(GrDiffGroup.Type.BOTH, line.contextLines),
+        group: line.contextGroup,
         section: section,
       };
       // Let it bubble up the DOM tree.
@@ -383,7 +429,7 @@
     }
 
     var patchNum = this._comments.meta.patchRange.patchNum;
-    var side = 'REVISION';
+    var side = comments[0].side || 'REVISION';
     if (line.type === GrDiffLine.Type.REMOVE ||
         opt_side === GrDiffBuilder.Side.LEFT) {
       if (this._comments.meta.patchRange.basePatchNum === 'PARENT') {
@@ -413,7 +459,7 @@
     } else if (line.type === GrDiffLine.Type.CONTEXT_CONTROL) {
       td.classList.add('contextLineNum');
       td.setAttribute('data-value', '@@');
-    } else if (line.type === GrDiffLine.Type.BOTH || line.type == type) {
+    } else if (line.type === GrDiffLine.Type.BOTH || line.type === type) {
       td.classList.add('lineNum');
       td.setAttribute('data-value', number);
     }
@@ -476,18 +522,18 @@
 
     // Tags don't count as characters
     while (index < html.length &&
-           html.charCodeAt(index) == GrDiffBuilder.LESS_THAN_CODE) {
+           html.charCodeAt(index) === GrDiffBuilder.LESS_THAN_CODE) {
       while (index < html.length &&
-             html.charCodeAt(index) != GrDiffBuilder.GREATER_THAN_CODE) {
+             html.charCodeAt(index) !== GrDiffBuilder.GREATER_THAN_CODE) {
         index++;
       }
       index++;  // skip the ">" itself
     }
     // An HTML entity (e.g., &lt;) counts as one character.
     if (index < html.length &&
-        html.charCodeAt(index) == GrDiffBuilder.AMPERSAND_CODE) {
+        html.charCodeAt(index) === GrDiffBuilder.AMPERSAND_CODE) {
       while (index < html.length &&
-             html.charCodeAt(index) != GrDiffBuilder.SEMICOLON_CODE) {
+             html.charCodeAt(index) !== GrDiffBuilder.SEMICOLON_CODE) {
         index++;
       }
     }

polygerrit-ui/app/elements/diff/gr-diff/gr-diff-builder_test.html

@@ -144,8 +144,9 @@ limitations under the License.
       assert.equal(groups[0].lines[0].afterNumber, GrDiffLine.FILE);
 
       assert.equal(groups[1].type, GrDiffGroup.Type.CONTEXT_CONTROL);
-      assert.equal(groups[1].lines[0].contextLines.length, 90);
+      assert.instanceOf(groups[1].lines[0].contextGroup, GrDiffGroup);
-      groups[1].lines[0].contextLines.forEach(function(l) {
+      assert.equal(groups[1].lines[0].contextGroup.lines.length, 90);
+      groups[1].lines[0].contextGroup.lines.forEach(function(l) {
         assert.equal(l.text, content[0].ab[0]);
       });
 
@@ -179,8 +180,9 @@ limitations under the License.
       });
 
       assert.equal(groups[7].type, GrDiffGroup.Type.CONTEXT_CONTROL);
-      assert.equal(groups[7].lines[0].contextLines.length, 90);
+      assert.instanceOf(groups[7].lines[0].contextGroup, GrDiffGroup);
-      groups[7].lines[0].contextLines.forEach(function(l) {
+      assert.equal(groups[7].lines[0].contextGroup.lines.length, 90);
+      groups[7].lines[0].contextGroup.lines.forEach(function(l) {
         assert.equal(l.text, content[4].ab[0]);
       });
 
@@ -215,8 +217,9 @@ limitations under the License.
       });
 
       assert.equal(groups[3].type, GrDiffGroup.Type.CONTEXT_CONTROL);
-      assert.equal(groups[3].lines[0].contextLines.length, 30);
+      assert.instanceOf(groups[3].lines[0].contextGroup, GrDiffGroup);
-      groups[3].lines[0].contextLines.forEach(function(l) {
+      assert.equal(groups[3].lines[0].contextGroup.lines.length, 30);
+      groups[3].lines[0].contextGroup.lines.forEach(function(l) {
         assert.equal(l.text, content[1].ab[0]);
       });
 
@@ -517,5 +520,54 @@ limitations under the License.
         }
       ]);
     });
+
+    suite('rendering', function() {
+      var content;
+      var outputEl;
+
+      setup(function() {
+        var prefs = {
+          line_length: 10,
+          show_tabs: true,
+          tab_size: 4,
+          context: -1
+        };
+        content = [
+          {ab: []},
+          {a: ['all work and no play make andybons a dull boy']},
+          {ab: []},
+          {b: ['elgoog elgoog elgoog']},
+          {ab: []},
+        ];
+        outputEl = document.createElement('out');
+        builder =
+            new GrDiffBuilder(
+                {content: content}, {left: [], right: []}, prefs, outputEl);
+        builder.buildSectionElement = function(group) {
+          var section = document.createElement('stub');
+          section.textContent = group.lines.reduce(function(acc, line) {
+            return acc + line.text;
+          }, '');
+          return section;
+        };
+        builder.emitDiff();
+      });
+
+      test('renderSection', function() {
+        var section = outputEl.querySelector('stub:nth-of-type(2)');
+        var prevInnerHTML = section.innerHTML;
+        section.innerHTML = 'wiped';
+        builder.renderSection(section);
+        section = outputEl.querySelector('stub:nth-of-type(2)');
+        assert.equal(section.innerHTML, prevInnerHTML);
+      });
+
+      test('getSectionsByLineRange', function() {
+        var section = outputEl.querySelector('stub:nth-of-type(2)');
+        var sections = builder.getSectionsByLineRange(1, 1, 'left');
+        assert.equal(sections.length, 1);
+        assert.strictEqual(sections[0], section);
+      });
+    });
   });
 </script>

polygerrit-ui/app/elements/diff/gr-diff/gr-diff-group.js

@@ -25,6 +25,8 @@
     }
   }
 
+  GrDiffGroup.prototype.element = null;
+
   GrDiffGroup.Type = {
     BOTH: 'both',
     CONTEXT_CONTROL: 'contextControl',

polygerrit-ui/app/elements/diff/gr-diff/gr-diff-line.js

@@ -16,13 +16,14 @@
 
   function GrDiffLine(type) {
     this.type = type;
-    this.contextLines = [];
     this.highlights = [];
   }
 
+  GrDiffLine.prototype.afterNumber = 0;
+
   GrDiffLine.prototype.beforeNumber = 0;
 
-  GrDiffLine.prototype.afterNumber = 0;
+  GrDiffLine.prototype.contextGroup = null;
 
   GrDiffLine.prototype.text = '';
 

polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js

@@ -71,15 +71,17 @@
       '_prefsChanged(prefs.*, viewMode)',
     ],
 
+    listeners: {
+      'thread-discard': '_handleThreadDiscard',
+      'comment-discard': '_handleCommentDiscard',
+      'comment-update': '_handleCommentUpdate',
+      'comment-save': '_handleCommentSave',
+    },
+
     attached: function() {
       this._getLoggedIn().then(function(loggedIn) {
         this._loggedIn = loggedIn;
       }.bind(this));
-
-      this.addEventListener('thread-discard',
-          this._handleThreadDiscard.bind(this));
-      this.addEventListener('comment-discard',
-          this._handleCommentDiscard.bind(this));
     },
 
     reload: function() {
@@ -213,7 +215,7 @@
       } else {
         var patchNum = this.patchRange.patchNum;
         var side = 'REVISION';
-        if (contentEl.classList.contains(DiffSide.LEFT) ||
+        if (lineEl.classList.contains(DiffSide.LEFT) ||
             contentEl.classList.contains('remove')) {
           if (this.patchRange.basePatchNum === 'PARENT') {
             side = 'PARENT';
@@ -234,29 +236,71 @@
     },
 
     _handleCommentDiscard: function(e) {
-      var comment = Polymer.dom(e).rootTarget.comment;
+      var comment = e.detail.comment;
-      this._removeComment(comment);
+      this._removeComment(comment, e.target.patchNum);
     },
 
-    _removeComment: function(comment) {
+    _removeComment: function(comment, opt_patchNum) {
-      if (!comment.id) { return; }
+      var side = this._findCommentSide(comment, opt_patchNum);
-      this._removeCommentFromSide(comment, DiffSide.LEFT) ||
+      this._removeCommentFromSide(comment, side);
-          this._removeCommentFromSide(comment, DiffSide.RIGHT);
+    },
+
+    _findCommentSide: function(comment, opt_patchNum) {
+      if (comment.side === 'PARENT') {
+        return DiffSide.LEFT;
+      } else {
+        return this._comments.meta.patchRange.basePatchNum === opt_patchNum ?
+            DiffSide.LEFT : DiffSide.RIGHT;
+      }
+    },
+
+    _handleCommentSave: function(e) {
+      var comment = e.detail.comment;
+      var side = this._findCommentSide(comment, e.target.patchNum);
+      var idx = this._findDraftIndex(comment, side);
+      this.set(['_comments', side, idx], comment);
+    },
+
+    _handleCommentUpdate: function(e) {
+      var comment = e.detail.comment;
+      var side = this._findCommentSide(comment, e.target.patchNum);
+      var idx = this._findCommentIndex(comment, side);
+      if (idx === -1) {
+        idx = this._findDraftIndex(comment, side);
+      }
+      if (idx !== -1) { // Update draft or comment.
+        this.set(['_comments', side, idx], comment);
+      } else { // Create new draft.
+        this.push(['_comments', side], comment);
+      }
     },
 
     _removeCommentFromSide: function(comment, side) {
-      var idx = -1;
+      var idx = this._findCommentIndex(comment, side);
-      for (var i = 0; i < this._comments[side].length; i++) {
+      if (idx === -1) {
-        if (this._comments[side][i].id === comment.id) {
+        idx = this._findDraftIndex(comment, side);
-          idx = i;
-          break;
-        }
       }
       if (idx !== -1) {
         this.splice('_comments.' + side, idx, 1);
-        return true;
       }
-      return false;
+    },
+
+    _findCommentIndex: function(comment, side) {
+      if (!comment.id || !this._comments[side]) {
+        return -1;
+      }
+      return this._comments[side].findIndex(function(item) {
+        return item.id === comment.id;
+      });
+    },
+
+    _findDraftIndex: function(comment, side) {
+      if (!comment.__draftID || !this._comments[side]) {
+        return -1;
+      }
+      return this._comments[side].findIndex(function(item) {
+        return item.__draftID === comment.__draftID;
+      });
     },
 
     _handleMouseDown: function(e) {
@@ -320,6 +364,13 @@
     },
 
     _showContext: function(group, sectionEl) {
+      var groups = this._builder._groups;
+      // TODO(viktard): Polyfill findIndex for IE10.
+      var contextIndex = groups.findIndex(function(group) {
+        return group.element == sectionEl;
+      });
+      groups[contextIndex] = group;
+
       this._builder.emitGroup(group, sectionEl);
       sectionEl.parentNode.removeChild(sectionEl);
 
@@ -339,14 +390,17 @@
     },
 
     _render: function() {
-      this._clearDiffContent();
+      this._builder =
-      this._builder = this._getDiffBuilder(this._diff, this._comments,
+          this._getDiffBuilder(this._diff, this._comments, this.prefs);
-          this.prefs);
+      this._renderDiff();
-      this._builder.emitDiff(this._diff.content);
+    },
 
+    _renderDiff: function() {
+      this._clearDiffContent();
+      this._builder.emitDiff();
       this.async(function() {
         this.fire('render', null, {bubbles: false});
-      }.bind(this), 1);
+      }, 1);
     },
 
     _clearDiffContent: function() {

polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html

@@ -35,85 +35,29 @@ limitations under the License.
   suite('gr-diff tests', function() {
     var element;
 
-    setup(function() {
+    suite('not logged in', function() {
-      stub('gr-rest-api-interface', {
+
-        getLoggedIn: function() { return Promise.resolve(false); },
+      setup(function() {
+        stub('gr-rest-api-interface', {
+          getLoggedIn: function() { return Promise.resolve(false); },
+        });
+        element = fixture('basic');
       });
-      element = fixture('basic');
-    });
 
-    test('get drafts logged out', function(done) {
+      test('get drafts', function(done) {
-      element.patchRange = {basePatchNum: 0, patchNum: 0};
+        element.patchRange = {basePatchNum: 0, patchNum: 0};
 
-      var getDraftsStub = sinon.stub(element.$.restAPI, 'getDiffDrafts');
+        var getDraftsStub = sinon.stub(element.$.restAPI, 'getDiffDrafts');
-      var loggedInStub = sinon.stub(element, '_getLoggedIn',
+        element._getDiffDrafts().then(function(result) {
-          function() { return Promise.resolve(false); });
+          assert.deepEqual(result, {baseComments: [], comments: []});
-      element._getDiffDrafts().then(function(result) {
+          sinon.assert.notCalled(getDraftsStub);
-        assert.deepEqual(result, {baseComments: [], comments: []});
+          getDraftsStub.restore();
-        sinon.assert.notCalled(getDraftsStub);
+          done();
-        loggedInStub.restore();
+        });
-        getDraftsStub.restore();
-        done();
       });
-    });
 
-    test('get drafts logged in', function(done) {
+      test('remove comment', function() {
-      element.patchRange = {basePatchNum: 0, patchNum: 0};
+        element._comments = {
-      var draftsResponse = {
-        baseComments: [{id: 'foo'}],
-        comments: [{id: 'bar'}],
-      };
-      var getDraftsStub = sinon.stub(element.$.restAPI, 'getDiffDrafts',
-          function() { return Promise.resolve(draftsResponse); });
-      var loggedInStub = sinon.stub(element, '_getLoggedIn',
-          function() { return Promise.resolve(true); });
-      element._getDiffDrafts().then(function(result) {
-        assert.deepEqual(result, draftsResponse);
-        loggedInStub.restore();
-        getDraftsStub.restore();
-        done();
-      });
-    });
-
-    test('get comments and drafts', function(done) {
-      var loggedInStub = sinon.stub(element, '_getLoggedIn',
-          function() { return Promise.resolve(true); });
-      var comments = {
-        baseComments: [
-          {id: 'bc1'},
-          {id: 'bc2'},
-        ],
-        comments: [
-          {id: 'c1'},
-          {id: 'c2'},
-        ],
-      };
-      var diffCommentsStub = sinon.stub(element, '_getDiffComments',
-          function() { return Promise.resolve(comments); });
-
-      var drafts = {
-        baseComments: [
-          {id: 'bd1'},
-          {id: 'bd2'},
-        ],
-        comments: [
-          {id: 'd1'},
-          {id: 'd2'},
-        ],
-      };
-      var diffDraftsStub = sinon.stub(element, '_getDiffDrafts',
-          function() { return Promise.resolve(drafts); });
-
-      element.changeNum = '42';
-      element.patchRange = {
-        basePatchNum: 'PARENT',
-        patchNum: 3,
-      };
-      element.path = '/path/to/foo';
-      element.projectConfig = {foo: 'bar'};
-
-      element._getDiffCommentsAndDrafts().then(function(result) {
-        assert.deepEqual(result, {
           meta: {
             changeNum: '42',
             patchRange: {
@@ -124,10 +68,10 @@ limitations under the License.
             projectConfig: {foo: 'bar'},
           },
           left: [
-            {id: 'bc1'},
+            {id: 'bc1', side: 'PARENT'},
-            {id: 'bc2'},
+            {id: 'bc2', side: 'PARENT'},
-            {id: 'bd1', __draft: true},
+            {id: 'bd1', __draft: true, side: 'PARENT'},
-            {id: 'bd2', __draft: true},
+            {id: 'bd2', __draft: true, side: 'PARENT'},
           ],
           right: [
             {id: 'c1'},
@@ -135,212 +79,347 @@ limitations under the License.
             {id: 'd1', __draft: true},
             {id: 'd2', __draft: true},
           ],
-        });
+        };
 
-        diffCommentsStub.restore();
+        element._removeComment({});
-        diffDraftsStub.restore();
+        // Using JSON.stringify because Safari 9.1 (11601.5.17.1) doesn’t seem
-        loggedInStub.restore();
+        // to believe that one object deepEquals another even when they do :-/.
-        done();
+        assert.equal(JSON.stringify(element._comments), JSON.stringify({
+          meta: {
+            changeNum: '42',
+            patchRange: {
+              basePatchNum: 'PARENT',
+              patchNum: 3,
+            },
+            path: '/path/to/foo',
+            projectConfig: {foo: 'bar'},
+          },
+          left: [
+            {id: 'bc1', side: 'PARENT'},
+            {id: 'bc2', side: 'PARENT'},
+            {id: 'bd1', __draft: true, side: 'PARENT'},
+            {id: 'bd2', __draft: true, side: 'PARENT'},
+          ],
+          right: [
+            {id: 'c1'},
+            {id: 'c2'},
+            {id: 'd1', __draft: true},
+            {id: 'd2', __draft: true},
+          ],
+        }));
+
+        element._removeComment({id: 'bc2', side: 'PARENT'});
+        assert.equal(JSON.stringify(element._comments), JSON.stringify({
+          meta: {
+            changeNum: '42',
+            patchRange: {
+              basePatchNum: 'PARENT',
+              patchNum: 3,
+            },
+            path: '/path/to/foo',
+            projectConfig: {foo: 'bar'},
+          },
+          left: [
+            {id: 'bc1', side: 'PARENT'},
+            {id: 'bd1', __draft: true, side: 'PARENT'},
+            {id: 'bd2', __draft: true, side: 'PARENT'},
+          ],
+          right: [
+            {id: 'c1'},
+            {id: 'c2'},
+            {id: 'd1', __draft: true},
+            {id: 'd2', __draft: true},
+          ],
+        }));
+
+        element._removeComment({id: 'd2'});
+        assert.deepEqual(JSON.stringify(element._comments), JSON.stringify({
+          meta: {
+            changeNum: '42',
+            patchRange: {
+              basePatchNum: 'PARENT',
+              patchNum: 3,
+            },
+            path: '/path/to/foo',
+            projectConfig: {foo: 'bar'},
+          },
+          left: [
+            {id: 'bc1', side: 'PARENT'},
+            {id: 'bd1', __draft: true, side: 'PARENT'},
+            {id: 'bd2', __draft: true, side: 'PARENT'},
+          ],
+          right: [
+            {id: 'c1'},
+            {id: 'c2'},
+            {id: 'd1', __draft: true},
+          ],
+        }));
+      });
+
+      test('renders image diffs', function(done) {
+        var mockDiff = {
+          meta_a: {name: 'carrot.jpg', content_type: 'image/jpeg', lines: 66},
+          meta_b: {name: 'carrot.jpg', content_type: 'image/jpeg', lines: 560},
+          intraline_status: 'OK',
+          change_type: 'MODIFIED',
+          diff_header: [
+            'diff --git a/carrot.jpg b/carrot.jpg',
+            'index 2adc47d..f9c2f2c 100644',
+            '--- a/carrot.jpg',
+            '+++ b/carrot.jpg',
+            'Binary files differ',
+          ],
+          content: [{skip: 66}],
+          binary: true,
+        };
+        var mockFile1 = {
+          body: 'Qk06AAAAAAAAADYAAAAoAAAAAQAAAP////8BACAAAAAAAAAAAAATCwAAEwsA' +
+              'AAAAAAAAAAAAAAAA/w==',
+          type: 'image/bmp',
+        };
+        var mockFile2 = {
+          body: 'Qk06AAAAAAAAADYAAAAoAAAAAQAAAP////8BACAAAAAAAAAAAAATCwAAEwsA' +
+              'AAAAAAAAAAAA/////w==',
+          type: 'image/bmp'
+        };
+        var mockCommit = {
+          commit: '9a1a1d10baece5efbba10bc4ccf808a67a50ac0a',
+          parents: [{
+            commit: '7338aa9adfe57909f1fdaf88975cdea467d3382f',
+            subject: 'Added a carrot',
+          }],
+          author: {
+            name: 'Wyatt Allen',
+            email: 'wyatta@google.com',
+            date: '2016-05-23 21:44:51.000000000',
+            tz: -420,
+          },
+          committer: {
+            name: 'Wyatt Allen',
+            email: 'wyatta@google.com',
+            date: '2016-05-25 00:25:41.000000000',
+            tz: -420,
+          },
+          subject: 'Updated the carrot',
+          message: 'Updated the carrot\n\nChange-Id: Iabcd123\n',
+        };
+        var mockComments = {baseComments: [], comments: []};
+
+        var stubs = [];
+        stubs.push(sinon.stub(element, '_getDiff',
+            function() { return Promise.resolve(mockDiff); }));
+        stubs.push(sinon.stub(element.$.restAPI, 'getCommitInfo',
+            function() { return Promise.resolve(mockCommit); }));
+        stubs.push(sinon.stub(element.$.restAPI,
+            'getCommitFileContents',
+            function() { return Promise.resolve(mockFile1); }));
+        stubs.push(sinon.stub(element.$.restAPI,
+            'getChangeFileContents',
+            function() { return Promise.resolve(mockFile2); }));
+        stubs.push(sinon.stub(element.$.restAPI, '_getDiffComments',
+            function() { return Promise.resolve(mockComments); }));
+        stubs.push(sinon.stub(element.$.restAPI, 'getDiffDrafts',
+            function() { return Promise.resolve(mockComments); }));
+
+        element.patchRange = {basePatchNum: 'PARENT', patchNum: 1};
+
+        var rendered = function() {
+          // Recognizes that it should be an image diff.
+          assert.isTrue(element.isImageDiff);
+          assert.instanceOf(element._getDiffBuilder(element._diff,
+              element._comments, element.prefs), GrDiffBuilderImage);
+
+          // Left image rendered with the parent commit's version of the file.
+          var leftInmage = element.$.diffTable.querySelector('td.left img');
+          assert.isOk(leftInmage);
+          assert.equal(leftInmage.getAttribute('src'),
+              'data:image/bmp;base64, ' + mockFile1.body);
+
+          // Right image rendered with this change's revision of the image.
+          var rightInmage = element.$.diffTable.querySelector('td.right img');
+          assert.isOk(rightInmage);
+          assert.equal(rightInmage.getAttribute('src'),
+              'data:image/bmp;base64, ' + mockFile2.body);
+
+          // Cleanup.
+          element.removeEventListener('render', rendered);
+          stubs.forEach(function(stub) { stub.restore(); });
+
+          done();
+        };
+
+        element.addEventListener('render', rendered);
+
+        element.$.restAPI.getDiffPreferences().then(function(prefs) {
+          element.prefs = prefs;
+          element.reload();
+        });
       });
     });
 
-    test('remove comment', function() {
+    suite('logged in', function() {
-      element._comments = {
-        meta: {
-          changeNum: '42',
-          patchRange: {
-            basePatchNum: 'PARENT',
-            patchNum: 3,
-          },
-          path: '/path/to/foo',
-          projectConfig: {foo: 'bar'},
-        },
-        left: [
-          {id: 'bc1'},
-          {id: 'bc2'},
-          {id: 'bd1', __draft: true},
-          {id: 'bd2', __draft: true},
-        ],
-        right: [
-          {id: 'c1'},
-          {id: 'c2'},
-          {id: 'd1', __draft: true},
-          {id: 'd2', __draft: true},
-        ],
-      };
 
-      element._removeComment({});
+      setup(function() {
-      // Using JSON.stringify because Safari 9.1 (11601.5.17.1) doesn’t seem to
+        stub('gr-rest-api-interface', {
-      // believe that one object deepEquals another even when they do :-/.
+          getLoggedIn: function() { return Promise.resolve(true); },
-      assert.equal(JSON.stringify(element._comments), JSON.stringify({
+        });
-        meta: {
+        element = fixture('basic');
-          changeNum: '42',
+      });
-          patchRange: {
-            basePatchNum: 'PARENT',
-            patchNum: 3,
-          },
-          path: '/path/to/foo',
-          projectConfig: {foo: 'bar'},
-        },
-        left: [
-          {id: 'bc1'},
-          {id: 'bc2'},
-          {id: 'bd1', __draft: true},
-          {id: 'bd2', __draft: true},
-        ],
-        right: [
-          {id: 'c1'},
-          {id: 'c2'},
-          {id: 'd1', __draft: true},
-          {id: 'd2', __draft: true},
-        ],
-      }));
 
-      element._removeComment({id: 'bc2'});
+      test('get drafts', function(done) {
-      assert.equal(JSON.stringify(element._comments), JSON.stringify({
+        element.patchRange = {basePatchNum: 0, patchNum: 0};
-        meta: {
+        var draftsResponse = {
-          changeNum: '42',
+          baseComments: [{id: 'foo'}],
-          patchRange: {
+          comments: [{id: 'bar'}],
-            basePatchNum: 'PARENT',
+        };
-            patchNum: 3,
+        var getDraftsStub = sinon.stub(element.$.restAPI, 'getDiffDrafts',
-          },
+            function() { return Promise.resolve(draftsResponse); });
-          path: '/path/to/foo',
+        element._getDiffDrafts().then(function(result) {
-          projectConfig: {foo: 'bar'},
+          assert.deepEqual(result, draftsResponse);
-        },
+          getDraftsStub.restore();
-        left: [
+          done();
-          {id: 'bc1'},
+        });
-          {id: 'bd1', __draft: true},
+      });
-          {id: 'bd2', __draft: true},
-        ],
-        right: [
-          {id: 'c1'},
-          {id: 'c2'},
-          {id: 'd1', __draft: true},
-          {id: 'd2', __draft: true},
-        ],
-      }));
 
-      element._removeComment({id: 'd2'});
+      test('get comments and drafts', function(done) {
-      assert.deepEqual(JSON.stringify(element._comments), JSON.stringify({
+        var comments = {
-        meta: {
+          baseComments: [
-          changeNum: '42',
+            {id: 'bc1'},
-          patchRange: {
+            {id: 'bc2'},
-            basePatchNum: 'PARENT',
+          ],
-            patchNum: 3,
+          comments: [
-          },
+            {id: 'c1'},
-          path: '/path/to/foo',
+            {id: 'c2'},
-          projectConfig: {foo: 'bar'},
+          ],
-        },
+        };
-        left: [
+        var diffCommentsStub = sinon.stub(element, '_getDiffComments',
-          {id: 'bc1'},
+            function() { return Promise.resolve(comments); });
-          {id: 'bd1', __draft: true},
+
-          {id: 'bd2', __draft: true},
+        var drafts = {
-        ],
+          baseComments: [
-        right: [
+            {id: 'bd1'},
-          {id: 'c1'},
+            {id: 'bd2'},
-          {id: 'c2'},
+          ],
-          {id: 'd1', __draft: true},
+          comments: [
-        ],
+            {id: 'd1'},
-      }));
+            {id: 'd2'},
+          ],
+        };
+        var diffDraftsStub = sinon.stub(element, '_getDiffDrafts',
+            function() { return Promise.resolve(drafts); });
+
+        element.changeNum = '42';
+        element.patchRange = {
+          basePatchNum: 'PARENT',
+          patchNum: 3,
+        };
+        element.path = '/path/to/foo';
+        element.projectConfig = {foo: 'bar'};
+
+        element._getDiffCommentsAndDrafts().then(function(result) {
+          assert.deepEqual(result, {
+            meta: {
+              changeNum: '42',
+              patchRange: {
+                basePatchNum: 'PARENT',
+                patchNum: 3,
+              },
+              path: '/path/to/foo',
+              projectConfig: {foo: 'bar'},
+            },
+            left: [
+              {id: 'bc1'},
+              {id: 'bc2'},
+              {id: 'bd1', __draft: true},
+              {id: 'bd2', __draft: true},
+            ],
+            right: [
+              {id: 'c1'},
+              {id: 'c2'},
+              {id: 'd1', __draft: true},
+              {id: 'd2', __draft: true},
+            ],
+          });
+
+          diffCommentsStub.restore();
+          diffDraftsStub.restore();
+          done();
+        });
+      });
+
+      suite('handle comment-update', function() {
+
+        setup(function() {
+          element._comments = {
+            meta: {
+              changeNum: '42',
+              patchRange: {
+                basePatchNum: 'PARENT',
+                patchNum: 3,
+              },
+              path: '/path/to/foo',
+              projectConfig: {foo: 'bar'},
+            },
+            left: [
+              {id: 'bc1', side: 'PARENT'},
+              {id: 'bc2', side: 'PARENT'},
+              {id: 'bd1', __draft: true, side: 'PARENT'},
+              {id: 'bd2', __draft: true, side: 'PARENT'},
+            ],
+            right: [
+              {id: 'c1'},
+              {id: 'c2'},
+              {id: 'd1', __draft: true},
+              {id: 'd2', __draft: true},
+            ],
+          };
+        });
+
+        test('creating a draft', function() {
+          var comment = {__draft: true, __draftID: 'tempID', side: 'PARENT'};
+          element.fire('comment-update', {comment: comment});
+          assert.include(element._comments.left, comment);
+        });
+
+        test('saving a draft', function() {
+          var draftID = 'tempID';
+          var id = 'savedID';
+          element._comments.left.push(
+              {__draft: true, __draftID: draftID, side: 'PARENT'});
+          element.fire('comment-update', {comment:
+              {id: id, __draft: true, __draftID: draftID, side: 'PARENT'},
+          });
+          var drafts = element._comments.left.filter(function(item) {
+            return item.__draftID === draftID;
+          });
+          assert.equal(drafts.length, 1);
+          assert.equal(drafts[0].id, id);
+        });
+      });
     });
 
-    test('renders image diffs', function(done) {
+    suite('renderDiff', function() {
-      var mockDiff = {
+      setup(function(done) {
-        meta_a: {name: 'carrot.jpg', content_type: 'image/jpeg', lines: 66},
+        sinon.stub(element, 'fire');
-        meta_b: {name: 'carrot.jpg', content_type: 'image/jpeg', lines: 560},
+        element._builder = {
-        intraline_status: 'OK',
+          emitDiff: sinon.stub(),
-        change_type: 'MODIFIED',
+        };
-        diff_header: [
+        element._renderDiff();
-          'diff --git a/carrot.jpg b/carrot.jpg',
+        flush(function() {
-          'index 2adc47d..f9c2f2c 100644',
+          done();
-          '--- a/carrot.jpg',
+        });
-          '+++ b/carrot.jpg',
+      });
-          'Binary files differ',
-        ],
-        content: [{skip: 66}],
-        binary: true,
-      };
-      var mockFile1 = {
-        body: 'Qk06AAAAAAAAADYAAAAoAAAAAQAAAP////8BACAAAAAAAAAAAAATCwAAEwsAAA' +
-            'AAAAAAAAAAAAAA/w==',
-        type: 'image/bmp',
-      };
-      var mockFile2 = {
-        body: 'Qk06AAAAAAAAADYAAAAoAAAAAQAAAP////8BACAAAAAAAAAAAAATCwAAEwsAAA' +
-            'AAAAAAAAAA/////w==',
-        type: 'image/bmp'
-      };
-      var mockCommit = {
-        commit: '9a1a1d10baece5efbba10bc4ccf808a67a50ac0a',
-        parents: [{
-          commit: '7338aa9adfe57909f1fdaf88975cdea467d3382f',
-          subject: 'Added a carrot',
-        }],
-        author: {
-          name: 'Wyatt Allen',
-          email: 'wyatta@google.com',
-          date: '2016-05-23 21:44:51.000000000',
-          tz: -420,
-        },
-        committer: {
-          name: 'Wyatt Allen',
-          email: 'wyatta@google.com',
-          date: '2016-05-25 00:25:41.000000000',
-          tz: -420,
-        },
-        subject: 'Updated the carrot',
-        message: 'Updated the carrot\n\nChange-Id: Iabcd123\n',
-      };
-      var mockComments = {baseComments: [], comments: []};
 
-      var stubs = [];
+      teardown(function() {
-      stubs.push(sinon.stub(element, '_getDiff',
+        element.fire.restore();
-          function() { return Promise.resolve(mockDiff); }));
+      });
-      stubs.push(sinon.stub(element.$.restAPI, 'getCommitInfo',
-          function() { return Promise.resolve(mockCommit); }));
-      stubs.push(sinon.stub(element.$.restAPI,
-          'getCommitFileContents',
-          function() { return Promise.resolve(mockFile1); }));
-      stubs.push(sinon.stub(element.$.restAPI,
-          'getChangeFileContents',
-          function() { return Promise.resolve(mockFile2); }));
-      stubs.push(sinon.stub(element.$.restAPI, '_getDiffComments',
-          function() { return Promise.resolve(mockComments); }));
-      stubs.push(sinon.stub(element.$.restAPI, 'getDiffDrafts',
-          function() { return Promise.resolve(mockComments); }));
 
-      element.patchRange = {basePatchNum: 'PARENT', patchNum: 1};
+      test('fires render', function() {
-
+        assert(element.fire.calledWithExactly(
-      var rendered = function() {
+            'render', null, {bubbles: false}));
-        // Recognizes that it should be an image diff.
+      });
-        assert.isTrue(element.isImageDiff);
+      test('calls emitDiff on builder', function() {
-        assert.instanceOf(element._getDiffBuilder(element._diff,
+        assert(element._builder.emitDiff.calledOnce);
-            element._comments, element.prefs), GrDiffBuilderImage);
-
-        // The left image rendered with the parent commit's version of the file.
-        var leftInmage = element.$.diffTable.querySelector('td.left img');
-        assert.isOk(leftInmage);
-        assert.equal(leftInmage.getAttribute('src'),
-            'data:image/bmp;base64, ' + mockFile1.body);
-
-        // The right image rendered with this change's revision of the image.
-        var rightInmage = element.$.diffTable.querySelector('td.right img');
-        assert.isOk(rightInmage);
-        assert.equal(rightInmage.getAttribute('src'),
-            'data:image/bmp;base64, ' + mockFile2.body);
-
-        // Cleanup.
-        element.removeEventListener('render', rendered);
-        stubs.forEach(function(stub) { stub.restore(); });
-
-        done();
-      };
-
-      element.addEventListener('render', rendered);
-
-      element.$.restAPI.getDiffPreferences().then(function(prefs) {
-        element.prefs = prefs;
-        element.reload();
       });
     });
   });