Establish annotation pipeline

Apply diff annotations (intraline differences and comment ranges) by
executing the annotation layers in order to each line. The diff builder
maintains an ordered array of annotation layers which are communicated
to GrDiffBuilder subclass instances. The builder also listens to each
layer for notifications that annotations have changed for some line
range and re-renders (i.e. re-applies the pipeline on DIV.contentText
elements) accordingly.

Change-Id: Iea0599d4869cafaadc0974158153a91d927913e8

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

@@ -19,7 +19,7 @@
 
   function GrDiffBuilderImage(diff, comments, prefs, outputEl, baseImage,
       revisionImage) {
-    GrDiffBuilderSideBySide.call(this, diff, comments, prefs, outputEl);
+    GrDiffBuilderSideBySide.call(this, diff, comments, prefs, outputEl, []);
     this._baseImage = baseImage;
     this._revisionImage = revisionImage;
   }

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

@@ -17,8 +17,8 @@
   // Prevent redefinition.
   if (window.GrDiffBuilderSideBySide) { return; }
 
-  function GrDiffBuilderSideBySide(diff, comments, prefs, outputEl) {
+  function GrDiffBuilderSideBySide(diff, comments, prefs, outputEl, layers) {
-    GrDiffBuilder.call(this, diff, comments, prefs, outputEl);
+    GrDiffBuilder.call(this, diff, comments, prefs, outputEl, layers);
   }
   GrDiffBuilderSideBySide.prototype = Object.create(GrDiffBuilder.prototype);
   GrDiffBuilderSideBySide.prototype.constructor = GrDiffBuilderSideBySide;
@@ -57,7 +57,7 @@
     if (action) {
       row.appendChild(action);
     } else {
-      var textEl = this._createTextEl(line);
+      var textEl = this._createTextEl(line, side);
       var threadEl = this._commentThreadForLine(line, side);
       if (threadEl) {
         textEl.appendChild(threadEl);

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

@@ -17,8 +17,8 @@
   // Prevent redefinition.
   if (window.GrDiffBuilderUnified) { return; }
 
-  function GrDiffBuilderUnified(diff, comments, prefs, outputEl) {
+  function GrDiffBuilderUnified(diff, comments, prefs, outputEl, layers) {
-    GrDiffBuilder.call(this, diff, comments, prefs, outputEl);
+    GrDiffBuilder.call(this, diff, comments, prefs, outputEl, layers);
   }
   GrDiffBuilderUnified.prototype = Object.create(GrDiffBuilder.prototype);
   GrDiffBuilderUnified.prototype.constructor = GrDiffBuilderUnified;

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

@@ -15,12 +15,16 @@ limitations under the License.
 -->
 <link rel="import" href="../../../bower_components/polymer/polymer.html">
 <link rel="import" href="../gr-diff-processor/gr-diff-processor.html">
+<link rel="import" href="../gr-ranged-comment-layer/gr-ranged-comment-layer.html">
 
 <dom-module id="gr-diff-builder">
   <template>
     <div class="contentWrapper">
       <content></content>
     </div>
+    <gr-ranged-comment-layer
+        id="rangeLayer"
+        comments="[[comments]]"></gr-ranged-comment-layer>
     <gr-diff-processor
         id="processor"
         groups="{{_groups}}"></gr-diff-processor>
@@ -52,11 +56,13 @@ limitations under the License.
 
         properties: {
           viewMode: String,
+          comments: Object,
           isImageDiff: Boolean,
           baseImage: Object,
           revisionImage: Object,
           _builder: Object,
           _groups: Array,
+          _layers: Array,
         },
 
         get diffElement() {
@@ -67,6 +73,14 @@ limitations under the License.
           '_groupsChanged(_groups.splices)',
         ],
 
+        attached: function() {
+          // Setup annotation layers.
+          this._layers = [
+            this._createIntralineLayer(),
+            this.$.rangeLayer,
+          ];
+        },
+
         render: function(diff, comments, prefs) {
           // Stop the processor (if it's running).
           this.$.processor.cancel();
@@ -216,10 +230,10 @@ limitations under the License.
                 this.diffElement, this.baseImage, this.revisionImage);
           } else if (this.viewMode === DiffViewMode.SIDE_BY_SIDE) {
             return new GrDiffBuilderSideBySide(
-                diff, comments, prefs, this.diffElement);
+                diff, comments, prefs, this.diffElement, this._layers);
           } else if (this.viewMode === DiffViewMode.UNIFIED) {
             return new GrDiffBuilderUnified(
-                diff, comments, prefs, this.diffElement);
+                diff, comments, prefs, this.diffElement, this._layers);
           }
           throw Error('Unsupported diff view mode: ' + this.viewMode);
         },

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

@@ -19,12 +19,18 @@
 
   var REGEX_ASTRAL_SYMBOL = /[\uD800-\uDBFF][\uDC00-\uDFFF]/;
 
-  function GrDiffBuilder(diff, comments, prefs, outputEl) {
+  function GrDiffBuilder(diff, comments, prefs, outputEl, layers) {
     this._diff = diff;
     this._comments = comments;
     this._prefs = prefs;
     this._outputEl = outputEl;
     this.groups = [];
+
+    this.layers = layers || [];
+
+    this.layers.forEach(function(layer) {
+      layer.addListener(this._handleLayerUpdate.bind(this));
+    }.bind(this));
   }
 
   GrDiffBuilder.LESS_THAN_CODE = '<'.charCodeAt(0);
@@ -165,7 +171,8 @@
     for (var i = 0; i < lines.length; i++) {
       line = lines[i];
       el = elements[i];
-      el.parentElement.replaceChild(this._createTextEl(line).firstChild, el);
+      el.parentElement.replaceChild(this._createTextEl(line, side).firstChild,
+          el);
     }
   };
 
@@ -350,7 +357,7 @@
     return td;
   };
 
-  GrDiffBuilder.prototype._createTextEl = function(line) {
+  GrDiffBuilder.prototype._createTextEl = function(line, opt_side) {
     var td = this._createElement('td');
     if (line.type !== GrDiffLine.Type.BLANK) {
       td.classList.add('content');
@@ -366,6 +373,9 @@
     }
 
     var contentText = this._createElement('div', 'contentText');
+    if (opt_side) {
+      contentText.setAttribute('data-side', opt_side);
+    }
 
     // If the html is equivalent to the text then it didn't get highlighted
     // or escaped. Use textContent which is faster than innerHTML.
@@ -377,9 +387,10 @@
 
     td.classList.add(line.highlights.length > 0 ?
         'lightHighlight' : 'darkHighlight');
-    if (line.highlights.length > 0) {
+
-      this._addIntralineHighlights(contentText, line);
+    this.layers.forEach(function(layer) {
-    }
+      layer.annotate(contentText, line, GrAnnotation);
+    });
 
     td.appendChild(contentText);
 
@@ -488,33 +499,6 @@
     return result;
   };
 
-  /**
-   * Take a DIV.contentText element and a line object with intraline differences
-   * to highlight and apply them to the element as annotations.
-   * @param {HTMLDivElement} el
-   * @param {[type]} line
-   */
-  GrDiffBuilder.prototype._addIntralineHighlights = function(el, line) {
-    var HL_CLASS = 'style-scope gr-diff';
-
-    line.highlights.forEach(function(highlight) {
-      // The start and end indices could be the same if a highlight is meant
-      // to start at the end of a line and continue onto the next one.
-      // Ignore it.
-      if (highlight.startIndex === highlight.endIndex) { return; }
-
-      // If endIndex isn't present, continue to the end of the line.
-      var endIndex = highlight.endIndex === undefined ?
-          line.text.length : highlight.endIndex;
-
-      GrAnnotation.annotateElement(
-          el,
-          highlight.startIndex,
-          endIndex - highlight.startIndex,
-          HL_CLASS);
-    });
-  };
-
   GrDiffBuilder.prototype._getTabWrapper = function(tabSize, showTabs) {
     // Force this to be a number to prevent arbitrary injection.
     tabSize = +tabSize;
@@ -549,5 +533,9 @@
     return el;
   };
 
+  GrDiffBuilder.prototype._handleLayerUpdate = function(start, end, side) {
+    this._renderContentByRange(start, end, side);
+  };
+
   window.GrDiffBuilder = GrDiffBuilder;
 })(window, GrDiffGroup, GrDiffLine);

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

@@ -20,10 +20,11 @@ limitations under the License.
 
 <script src="../../../bower_components/webcomponentsjs/webcomponents.min.js"></script>
 <script src="../../../bower_components/web-component-tester/browser.js"></script>
+<script src="../../../scripts/util.js"></script>
 <script src="../gr-diff/gr-diff-line.js"></script>
 <script src="../gr-diff/gr-diff-group.js"></script>
+<script src="../gr-diff-highlight/gr-annotation.js"></script>
 <script src="gr-diff-builder.js"></script>
-<script src="../../../scripts/util.js"></script>
 
 <link rel="import" href="../../shared/gr-rest-api-interface/mock-diff-response_test.html">
 <link rel="import" href="gr-diff-builder.html">
@@ -255,6 +256,7 @@ limitations under the License.
       var el;
       var str;
       var annotateElementSpy;
+      var layer;
 
       function slice(str, start, end) {
         return Array.from(str).slice(start, end).join('');
@@ -264,19 +266,21 @@ limitations under the License.
         el = fixture('div-with-text');
         str = el.textContent;
         annotateElementSpy = sinon.spy(GrAnnotation, 'annotateElement');
+        layer = document.createElement('gr-diff-builder')
+            ._createIntralineLayer();
       });
 
       teardown(function() {
         annotateElementSpy.restore();
       });
 
-      test('_addIntralineHighlights no highlights', function() {
+      test('annotate no highlights', function() {
         var line = {
           text: str,
           highlights: [],
         };
 
-        GrDiffBuilder.prototype._addIntralineHighlights(el, line);
+        layer.annotate(el, line, GrAnnotation);
 
         // The content is unchanged.
         assert.isFalse(annotateElementSpy.called);
@@ -285,7 +289,7 @@ limitations under the License.
         assert.equal(str, el.childNodes[0].textContent);
       });
 
-      test('_addIntralineHighlights with highlights', function() {
+      test('annotate with highlights', function() {
         var line = {
           text: str,
           highlights: [
@@ -299,7 +303,7 @@ limitations under the License.
         var str3 = slice(str, 18, 22);
         var str4 = slice(str, 22);
 
-        GrDiffBuilder.prototype._addIntralineHighlights(el, line);
+        layer.annotate(el, line, GrAnnotation);
 
         assert.isTrue(annotateElementSpy.called);
         assert.equal(el.childNodes.length, 5);
@@ -320,7 +324,7 @@ limitations under the License.
         assert.equal(el.childNodes[4].textContent, str4);
       });
 
-      test('_addIntralineHighlights without endIndex', function() {
+      test('annotate without endIndex', function() {
         var line = {
           text: str,
           highlights: [
@@ -331,7 +335,7 @@ limitations under the License.
         var str0 = slice(str, 0, 28);
         var str1 = slice(str, 28);
 
-        GrDiffBuilder.prototype._addIntralineHighlights(el, line);
+        layer.annotate(el, line, GrAnnotation);
 
         assert.isTrue(annotateElementSpy.called);
         assert.equal(el.childNodes.length, 2);
@@ -343,7 +347,7 @@ limitations under the License.
         assert.equal(el.childNodes[1].textContent, str1);
       });
 
-      test('_addIntralineHighlights ignores empty highlights', function() {
+      test('annotate ignores empty highlights', function() {
         var line = {
           text: str,
           highlights: [
@@ -351,17 +355,16 @@ limitations under the License.
           ],
         };
 
-        GrDiffBuilder.prototype._addIntralineHighlights(el, line);
+        layer.annotate(el, line, GrAnnotation);
 
         assert.isFalse(annotateElementSpy.called);
         assert.equal(el.childNodes.length, 1);
       });
 
-      test('_addIntralineHighlights handles unicode', function() {
+      test('annotate handles unicode', function() {
         // Put some unicode into the string:
         str = str.replace(/\s/g, '💢');
         el.textContent = str;
-
         var line = {
           text: str,
           highlights: [
@@ -373,7 +376,7 @@ limitations under the License.
         var str1 = slice(str, 6, 12);
         var str2 = slice(str, 12);
 
-        GrDiffBuilder.prototype._addIntralineHighlights(el, line);
+        layer.annotate(el, line, GrAnnotation);
 
         assert.isTrue(annotateElementSpy.called);
         assert.equal(el.childNodes.length, 3);
@@ -388,7 +391,7 @@ limitations under the License.
         assert.equal(el.childNodes[2].textContent, str2);
       });
 
-      test('_addIntralineHighlights handles unicode w/o endIndex', function() {
+      test('annotate handles unicode w/o endIndex', function() {
         // Put some unicode into the string:
         str = str.replace(/\s/g, '💢');
         el.textContent = str;
@@ -403,7 +406,7 @@ limitations under the License.
         var str0 = slice(str, 0, 6);
         var str1 = slice(str, 6);
 
-        GrDiffBuilder.prototype._addIntralineHighlights(el, line);
+        layer.annotate(el, line, GrAnnotation);
 
         assert.isTrue(annotateElementSpy.called);
         assert.equal(el.childNodes.length, 2);

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

@@ -15,7 +15,6 @@
   'use strict';
 
   var STORAGE_DEBOUNCE_INTERVAL = 400;
-  var UPDATE_DEBOUNCE_INTERVAL = 500;
 
   Polymer({
     is: 'gr-diff-comment',
@@ -160,7 +159,7 @@
     _fireUpdate: function() {
       this.debounce('fire-update', function() {
         this.fire('comment-update', this._getEventPayload());
-      }, UPDATE_DEBOUNCE_INTERVAL);
+      });
     },
 
     _draftChanged: function(draft) {

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

@@ -236,21 +236,19 @@ limitations under the License.
       element._xhrPromise.then(function(draft) {
         assert(fireStub.calledWith('comment-save'),
                'comment-save should be sent');
-        assert.deepEqual(fireStub.lastCall.args, [
+        assert.deepEqual(fireStub.lastCall.args[1], {
-          'comment-save', {
+          comment: {
-            comment: {
+            __draft: true,
-              __draft: true,
+            __draftID: 'temp_draft_id',
-              __draftID: 'temp_draft_id',
+            __editing: false,
-              __editing: false,
+            id: 'baf0414d_40572e03',
-              id: 'baf0414d_40572e03',
+            line: 5,
-              line: 5,
+            message: 'saved!',
-              message: 'saved!',
+            path: '/path/to/file',
-              path: '/path/to/file',
+            updated: '2015-12-08 21:52:36.177000000',
-              updated: '2015-12-08 21:52:36.177000000',
-            },
-            patchNum: 1,
           },
-        ]);
+          patchNum: 1,
+        });
         assert.isFalse(element.disabled,
                        'Element should be enabled when done creating draft.');
         assert.equal(draft.message, 'saved!');

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

@@ -24,11 +24,11 @@ limitations under the License.
         position: relative;
       }
       .contentWrapper ::content .range {
-        background-color: #ffd500 !important;
+        background-color: rgba(255,213,0,0.5);
         display: inline;
       }
       .contentWrapper ::content .rangeHighlight {
-        background-color: #ff0 !important;
+        background-color: rgba(255,255,0,0.5);
         display: inline;
       }
     </style>

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

@@ -14,9 +14,6 @@
 (function() {
   'use strict';
 
-  var RANGE_HIGHLIGHT = 'range';
-  var HOVER_HIGHLIGHT = 'rangeHighlight';
-
   Polymer({
     is: 'gr-diff-highlight',
 
@@ -27,13 +24,9 @@
     },
 
     listeners: {
-      'comment-discard': '_handleCommentDiscard',
       'comment-mouse-out': '_handleCommentMouseOut',
       'comment-mouse-over': '_handleCommentMouseOver',
       'create-comment': '_createComment',
-      'render': '_handleRender',
-      'show-context': '_handleShowContext',
-      'thread-discard': '_handleThreadDiscard',
     },
 
     get diffBuilder() {
@@ -56,21 +49,6 @@
       return !!this.$$('gr-selection-action-box');
     },
 
-    _handleThreadDiscard: function(e) {
-      var comment = e.detail.lastComment;
-      // Comment Element was removed from DOM already.
-      if (comment.range) {
-        this._renderCommentRange(comment, e.target);
-      }
-    },
-
-    _handleCommentDiscard: function(e) {
-      var comment = e.detail.comment;
-      if (comment.range) {
-        this._renderCommentRange(comment, e.target);
-      }
-    },
-
     _handleSelectionChange: function() {
       // Can't use up or down events to handle selection started and/or ended in
       // in comment threads or outside of diff.
@@ -79,45 +57,36 @@
       this.debounce('selectionChange', this._handleSelection, 200);
     },
 
-    _handleRender: function() {
-      this._applyAllHighlights();
-    },
-
-    _handleShowContext: function() {
-      // TODO (viktard): Re-render expanded sections only.
-      this._applyAllHighlights();
-    },
-
     _handleCommentMouseOver: function(e) {
       var comment = e.detail.comment;
-      var range = comment.range;
+      if (!comment.range) { return; }
-      if (!range) {
-        return;
-      }
       var lineEl = this.diffBuilder.getLineElByChild(e.target);
       var side = this.diffBuilder.getSideByLineEl(lineEl);
-      this._applyRangedHighlight(
+      var index = this._indexOfComment(side, comment);
-          HOVER_HIGHLIGHT, range.start_line, range.start_character,
+      if (index !== undefined) {
-          range.end_line, range.end_character, side);
+        this.set(['comments', side, index, '__hovering'], true);
+      }
     },
 
     _handleCommentMouseOut: function(e) {
       var comment = e.detail.comment;
-      var range = comment.range;
+      if (!comment.range) { return; }
-      if (!range) {
-        return;
-      }
       var lineEl = this.diffBuilder.getLineElByChild(e.target);
       var side = this.diffBuilder.getSideByLineEl(lineEl);
-      var contentEls = this.diffBuilder.getContentsByLineRange(
+      var index = this._indexOfComment(side, comment);
-          range.start_line, range.end_line, side);
+      if (index !== undefined) {
-      contentEls.forEach(function(content) {
+        this.set(['comments', side, index, '__hovering'], false);
-        Polymer.dom(content).querySelectorAll('.' + HOVER_HIGHLIGHT).forEach(
+      }
-            function(el) {
+    },
-              el.classList.remove(HOVER_HIGHLIGHT);
+
-              el.classList.add(RANGE_HIGHLIGHT);
+    _indexOfComment: function(side, comment) {
-            });
+      var idProp = comment.id ? 'id' : '__draftID';
-      }, this);
+      for (var i = 0; i < this.comments[side].length; i++) {
+        if (comment[idProp] &&
+            this.comments[side][i][idProp] === comment[idProp]) {
+          return i;
+        }
+      }
     },
 
     /**
@@ -226,26 +195,8 @@
       }
     },
 
-    _renderCommentRange: function(comment, el) {
-      var lineEl = this.diffBuilder.getLineElByChild(el);
-      if (!lineEl) {
-        return;
-      }
-      var side = this.diffBuilder.getSideByLineEl(lineEl);
-      this._rerenderByLines(
-          comment.range.start_line, comment.range.end_line, side);
-    },
-
     _createComment: function(e) {
       this._removeActionBox();
-      var side = e.detail.side;
-      var range = e.detail.range;
-      if (!range) {
-        return;
-      }
-      this._applyRangedHighlight(
-          RANGE_HIGHLIGHT, range.startLine, range.startChar,
-          range.endLine, range.endChar, side);
     },
 
     _removeActionBoxDebounced: function() {
@@ -314,226 +265,5 @@
         return GrAnnotation.getLength(node);
       }
     },
-
-    /**
-     * Creates hl tag with cssClass for starting side of range highlight.
-     *
-     * @param {!Element} startContent Range start diff content
-     *     aka div.contentText.
-     * @param {!Element} endContent Range end diff content
-     *     aka div.contentText.
-     * @param {number} startOffset Range start within start content.
-     * @param {number} endOffset Range end within end content.
-     * @param {string} cssClass
-     * @return {!Element} Range start node.
-     */
-    _normalizeStart: function(
-        startContent, endContent, startOffset, endOffset, cssClass) {
-      var isOneLine = startContent === endContent;
-      var startNode = startContent.firstChild;
-      var length = endOffset - startOffset;
-
-      if (!startNode) {
-        return startNode;
-      }
-
-      // Skip nodes before startOffset.
-      var nodeLength = this._getLength(startNode);
-      while (startNode && (nodeLength <= startOffset || nodeLength === 0)) {
-        startOffset -= nodeLength;
-        startNode = startNode.nextSibling;
-        nodeLength = startNode && this._getLength(startNode);
-      }
-      if (!startNode) { return null; }
-
-      // Split Text node.
-      if (startNode instanceof Text) {
-        startNode = GrAnnotation.splitAndWrapInHighlight(
-            startNode, startOffset, cssClass);
-        // Edge case: single line, text node wraps the highlight.
-        if (isOneLine && this._getLength(startNode) > length) {
-          var extra = GrAnnotation.splitTextNode(startNode.firstChild, length);
-          startContent.insertBefore(extra, startNode.nextSibling);
-          startContent.normalize();
-        }
-      } else if (startNode.tagName == 'HL') {
-        if (!startNode.classList.contains(cssClass)) {
-          // Edge case: single line, <hl> wraps the highlight.
-          // Should leave wrapping HL's content after the highlight.
-          if (isOneLine && startOffset + length < this._getLength(startNode)) {
-            GrAnnotation.splitNode(startNode, startOffset + length);
-          }
-          startNode = GrAnnotation.splitAndWrapInHighlight(
-              startNode, startOffset, cssClass);
-        }
-      } else if (startNode.tagName == 'SPAN') {
-        startNode = GrAnnotation.splitAndWrapInHighlight(
-            startNode, startOffset, cssClass);
-      } else {
-        startNode = null;
-      }
-      return startNode;
-    },
-
-    /**
-     * Creates hl tag with cssClass for ending side of range highlight.
-     *
-     * @param {!Element} startContent Range start diff content
-     *     aka div.contentText.
-     * @param {!Element} endContent Range end diff content
-     *     aka div.contentText.
-     * @param {number} startOffset Range start within start content.
-     * @param {number} endOffset Range end within end content.
-     * @param {string} cssClass
-     * @return {!Element} Range start node.
-     */
-    _normalizeEnd: function(
-        startContent, endContent, startOffset, endOffset, cssClass) {
-      var endNode = endContent.firstChild;
-
-      if (!endNode) {
-        return endNode;
-      }
-
-      // Find the node where endOffset points at.
-      var nodeLength = this._getLength(endNode);
-      while (endNode && (nodeLength < endOffset || nodeLength === 0)) {
-        endOffset -= nodeLength;
-        endNode = endNode.nextSibling;
-        nodeLength = endNode && this._getLength(endNode);
-      }
-      if (!endNode) { return null; }
-
-      if (endNode instanceof Text) {
-        endNode = GrAnnotation.splitAndWrapInHighlight(
-            endNode, endOffset, cssClass, true);
-      } else if (endNode.tagName == 'HL') {
-        if (!endNode.classList.contains(cssClass)) {
-          // Split text inside HL.
-          var hl = endNode;
-          endNode = GrAnnotation.splitAndWrapInHighlight(
-              endNode, endOffset, cssClass, true);
-          if (hl.textContent.length === 0) {
-            hl.remove();
-          }
-        }
-      } else {
-        endNode = null;
-      }
-      return endNode;
-    },
-
-    /**
-     * Applies highlight to first and last lines in range.
-     *
-     * @param {!Element} startContent Range start diff content
-     *     aka div.contentText.
-     * @param {!Element} endContent Range end diff content
-     *     aka div.contentText.
-     * @param {number} startOffset Range start within start content.
-     * @param {number} endOffset Range end within end content.
-     * @param {string} cssClass
-     */
-    _highlightSides: function(
-        startContent, endContent, startOffset, endOffset, cssClass) {
-      var isOneLine = startContent === endContent;
-      var startNode = this._normalizeStart(
-          startContent, endContent, startOffset, endOffset, cssClass);
-      var endNode = this._normalizeEnd(
-          startContent, endContent, startOffset, endOffset, cssClass);
-
-      // Grow starting highlight until endNode or end of line.
-      if (startNode && startNode != endNode) {
-        var growStartHl = function(node) {
-          if (node instanceof Text || node.tagName === 'SPAN') {
-            startNode.appendChild(node);
-          } else if (node.tagName === 'HL') {
-            this._traverseContentSiblings(node.firstChild, growStartHl);
-            node.remove();
-          }
-          return node == endNode;
-        }.bind(this);
-        this._traverseContentSiblings(startNode.nextSibling, growStartHl);
-        startNode.normalize();
-      }
-
-      if (!isOneLine && endNode) {
-        var growEndHl = function(node) {
-          if (node instanceof Text || node.tagName === 'SPAN') {
-            endNode.insertBefore(node, endNode.firstChild);
-          } else if (node.tagName === 'HL') {
-            this._traverseContentSiblings(node.firstChild, growEndHl);
-            node.remove();
-          }
-        }.bind(this);
-        // Prepend text up to line start to the ending highlight.
-        this._traverseContentSiblings(
-          endNode.previousSibling, growEndHl, {left: true});
-        endNode.normalize();
-      }
-    },
-
-    /**
-     * @param {string} cssClass
-     * @param {number} startLine Range start code line number.
-     * @param {number} startCol Range start column number.
-     * @param {number} endLine Range end line number.
-     * @param {number} endCol Range end column number.
-     * @param {string=} opt_side Side selector (right or left).
-     */
-    _applyRangedHighlight: function(
-        cssClass, startLine, startCol, endLine, endCol, opt_side) {
-      var startEl = this.diffBuilder.getContentByLine(startLine, opt_side);
-      var endEl = this.diffBuilder.getContentByLine(endLine, opt_side);
-      this._highlightSides(startEl, endEl, startCol, endCol, cssClass);
-      if (endLine - startLine > 1) {
-        // There is at least one line in between.
-        var contents = this.diffBuilder.getContentsByLineRange(
-            startLine + 1, endLine - 1, opt_side);
-        contents.forEach(function(content) {
-          if (!content.firstChild) {
-            return;
-          }
-          // Wrap contents in highlight.
-          var hl = GrAnnotation.wrapInHighlight(content.firstChild, cssClass);
-          var wrapInHl = function(node) {
-            if (node instanceof Text || node.tagName === 'SPAN') {
-              hl.appendChild(node);
-            } else if (node.tagName === 'HL') {
-              this._traverseContentSiblings(node.firstChild, wrapInHl);
-              node.remove();
-            }
-            return node === content.lastChild;
-          }.bind(this);
-          this._traverseContentSiblings(hl.nextSibling, wrapInHl);
-          hl.normalize();
-        }, this);
-      }
-    },
-
-    _applyAllHighlights: function() {
-      var rangedLeft =
-          this.comments.left.filter(function(item) { return !!item.range; });
-      var rangedRight =
-          this.comments.right.filter(function(item) { return !!item.range; });
-      rangedLeft.forEach(function(item) {
-        var range = item.range;
-        this._applyRangedHighlight(
-            RANGE_HIGHLIGHT, range.start_line, range.start_character,
-            range.end_line, range.end_character, 'left');
-      }, this);
-      rangedRight.forEach(function(item) {
-        var range = item.range;
-        this._applyRangedHighlight(
-            RANGE_HIGHLIGHT, range.start_line, range.start_character,
-            range.end_line, range.end_character, 'right');
-      }, this);
-    },
-
-    _rerenderByLines: function(startLine, endLine, opt_side) {
-      this.async(function() {
-        this.diffBuilder.renderLineRange(startLine, endLine, opt_side);
-      }, 1);
-    },
   });
 })();

polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight_test.html

@@ -167,42 +167,17 @@ limitations under the License.
         });
       });
 
-      test('renders lines in comment range on thread discard', function(done) {
-        element.fire('thread-discard', {
-          lastComment: {
-            range: {
-              start_line: 10,
-              end_line: 24,
-            },
-          },
-        });
-        flush(function() {
-          assert.isTrue(
-              builder.renderLineRange.calledWithExactly(10, 24, 'other-side'));
-          done();
-        });
-      });
-
-      test('renders lines in comment range on comment discard', function(done) {
-        element.fire('comment-discard', {
-          comment: {
-            range: {
-              start_line: 10,
-              end_line: 24,
-            },
-          },
-        });
-        flush(function() {
-          assert.isTrue(
-              builder.renderLineRange.calledWithExactly(10, 24, 'other-side'));
-          done();
-        });
-      });
-
       test('comment-mouse-over from line comments is ignored', function() {
-        sandbox.stub(element, '_applyRangedHighlight');
+        sandbox.stub(element, 'set');
         element.fire('comment-mouse-over', {comment: {}});
-        assert.isFalse(element._applyRangedHighlight.called);
+        assert.isFalse(element.set.called);
+      });
+
+      test('comment-mouse-over from ranged comment causes set', function() {
+        sandbox.stub(element, 'set');
+        sandbox.stub(element, '_indexOfComment').returns(0);
+        element.fire('comment-mouse-over', {comment: {range:{}}});
+        assert.isTrue(element.set.called);
       });
 
       test('comment-mouse-out from line comments is ignored', function() {
@@ -210,56 +185,7 @@ limitations under the License.
         assert.isFalse(builder.getContentsByLineRange.called);
       });
 
-      test('on comment-mouse-out highlight classes are removed', function() {
-        var testEl = fixture('highlighted');
-        builder.getContentsByLineRange.returns([testEl]);
-        element.fire('comment-mouse-out', {
-          comment: {
-            range: {
-              start_line: 3,
-              start_character: 14,
-              end_line: 10,
-              end_character: 24,
-            }
-          }});
-        assert.isTrue(builder.getContentsByLineRange.calledWithExactly(
-            3, 10, 'other-side'));
-        assert.equal(0, testEl.querySelectorAll('.rangeHighlight').length);
-        assert.equal(2, testEl.querySelectorAll('.range').length);
-      });
-
-      test('on comment-mouse-over range is highlighted', function() {
-        sandbox.stub(element, '_applyRangedHighlight');
-        element.fire('comment-mouse-over', {
-          comment: {
-            range: {
-              start_line: 3,
-              start_character: 14,
-              end_line: 10,
-              end_character: 24,
-            },
-          }});
-        assert.isTrue(element._applyRangedHighlight.calledWithExactly(
-            'rangeHighlight', 3, 14, 10, 24, 'other-side'));
-      });
-
-      test('on create-comment range is highlighted', function() {
-        sandbox.stub(element, '_applyRangedHighlight');
-        element.fire('create-comment', {
-          range: {
-            startLine: 3,
-            startChar: 14,
-            endLine: 10,
-            endChar: 24,
-          },
-          side: 'some-side',
-        });
-        assert.isTrue(element._applyRangedHighlight.calledWithExactly(
-            'range', 3, 14, 10, 24, 'some-side'));
-      });
-
       test('on create-comment action box is removed', function() {
-        sandbox.stub(element, '_applyRangedHighlight');
         sandbox.stub(element, '_removeActionBox');
         element.fire('create-comment', {
           comment: {
@@ -270,290 +196,6 @@ limitations under the License.
       });
     });
 
-    test('apply multiline highlight', function() {
-      var diff = element.querySelector('#diffTable');
-      var startContent = diff.querySelector(
-          '.left.lineNum[data-value="138"] ~ .content .contentText');
-      var betweenContent = diff.querySelector(
-          '.left.lineNum[data-value="140"] ~ .content .contentText');
-      var endContent = diff.querySelector(
-          '.left.lineNum[data-value="141"] ~ .content .contentText');
-      var builder = {
-        getContentByLine: sandbox.stub().returns({}),
-        getContentsByLineRange: sandbox.stub().returns([betweenContent]),
-        getLineElByChild: sandbox.stub().returns(
-            {getAttribute: sandbox.stub()}),
-      };
-      element._cachedDiffBuilder = builder;
-      builder.getContentByLine.withArgs(138, 'left').returns(
-          startContent);
-      builder.getContentByLine.withArgs(141, 'left').returns(
-          endContent);
-      element._applyRangedHighlight('some', 138, 4, 141, 28, 'left');
-      assert.instanceOf(startContent.childNodes[0], Text);
-      assert.equal(startContent.childNodes[0].textContent, '[14]');
-      assert.instanceOf(startContent.childNodes[1], Element);
-      assert.equal(startContent.childNodes[1].textContent,
-          ' Nam cum ad me in Cumanum salutandi causa uterque venisset,');
-      assert.equal(startContent.childNodes[1].tagName, 'HL');
-      assert.equal(startContent.childNodes[1].className, 'some');
-
-      assert.instanceOf(betweenContent.firstChild, Element);
-      assert.equal(betweenContent.firstChild.tagName, 'HL');
-      assert.equal(betweenContent.firstChild.className, 'some');
-      assert.equal(betweenContent.childNodes.length, 1);
-      assert.equal(betweenContent.firstChild.childNodes.length, 5);
-      assert.equal(betweenContent.firstChild.textContent,
-          'na💢ti te, inquit, sumus aliquando otiosum, certe a udiam, ' +
-          'quid sit, quod Epicurum');
-
-      assert.isNull(diff.querySelector('.right + .content .some'),
-          'Highlight should be applied only to the left side content.');
-
-      assert.instanceOf(endContent.childNodes[0], Element);
-      assert.equal(endContent.childNodes[0].textContent,
-          'nam et\tcomplectitur\tverbis, ');
-      assert.equal(endContent.childNodes[0].tagName, 'HL');
-      assert.equal(endContent.childNodes[0].className, 'some');
-      assert.instanceOf(endContent.childNodes[1], Text);
-      assert.equal(endContent.childNodes[1].textContent,
-          'quod vult, et dicit plane, quod intellegam;');
-      var endHl = endContent.querySelector('hl.some');
-      assert.equal(endHl.childNodes.length, 5);
-      var tabs = endHl.querySelectorAll('span.tab');
-      assert.equal(tabs.length, 2);
-      assert.equal(tabs[0].previousSibling.textContent, 'nam et');
-      assert.equal(tabs[1].previousSibling.textContent, 'complectitur');
-      assert.equal(tabs[1].nextSibling.textContent, 'verbis, ');
-    });
-
-    test('multiline highlight w/ start at end of 1st line', function() {
-      var diff = element.querySelector('#diffTable');
-      var startContent =
-          diff.querySelector('.left.lineNum[data-value="138"] ~ .content');
-      var betweenContent =
-          diff.querySelector('.left.lineNum[data-value="140"] ~ .content');
-      var endContent =
-          diff.querySelector('.left.lineNum[data-value="141"] ~ .content');
-      var commentThread =
-          diff.querySelector('gr-diff-comment-thread');
-      var builder = {
-        getCommentThreadByContentEl: sandbox.stub().returns(commentThread),
-        getContentByLine: sandbox.stub().returns({}),
-        getContentsByLineRange: sandbox.stub().returns([betweenContent]),
-        getLineElByChild: sandbox.stub().returns(
-            {getAttribute: sandbox.stub()}),
-      };
-      element._cachedDiffBuilder = builder;
-      builder.getContentByLine.withArgs(138, 'left').returns(
-          startContent);
-      builder.getContentByLine.withArgs(141, 'left').returns(
-          endContent);
-
-      var expectedStartContentNodes = startContent.childNodes.length;
-
-      // The following should not cause an error.
-      element._applyRangedHighlight(
-          'some', 138, startContent.textContent.length, 141, 28, 'left');
-
-      assert.equal(startContent.childNodes.length, expectedStartContentNodes,
-          'Should not add a highlight to the start content');
-    });
-
-    suite('single line ranges', function() {
-      var diff;
-      var contentText;
-      var commentThread;
-      var builder;
-
-      setup(function() {
-        diff = element.querySelector('#diffTable');
-        var contentTd = diff.querySelector(
-            '.left.lineNum[data-value="140"] ~ .content');
-        contentText = contentTd.querySelector('.contentText');
-        commentThread = diff.querySelector('gr-diff-comment-thread');
-        builder = {
-          getCommentThreadByContentEl: sandbox.stub().returns(commentThread),
-          getContentByLine: sandbox.stub().returns(contentText),
-          getContentsByLineRange: sandbox.stub().returns([]),
-          getLineElByChild: sandbox.stub().returns(
-              {getAttribute: sandbox.stub()}),
-        };
-        element._cachedDiffBuilder = builder;
-      });
-
-      test('whole line range', function() {
-        element._applyRangedHighlight('some', 140, 0, 140, 81, 'left');
-        assert.equal(contentText.childNodes.length, 1);
-        assert.instanceOf(contentText.firstChild, Element);
-        assert.equal(contentText.firstChild.tagName, 'HL');
-        assert.equal(contentText.firstChild.className, 'some');
-        assert.equal(contentText.firstChild.childNodes.length, 5);
-        assert.equal(contentText.firstChild.textContent,
-            'na💢ti te, inquit, sumus aliquando otiosum, certe a udiam, ' +
-            'quid sit, quod Epicurum');
-        var tabs = contentText.querySelectorAll('span.tab');
-        assert.equal(tabs.length, 2);
-        assert.strictEqual(tabs[1].previousSibling, tabs[0].nextSibling);
-        assert.equal(tabs[0].previousSibling.textContent,
-            'na💢ti te, inquit, sumus aliquando otiosum, certe a ');
-        assert.equal(tabs[1].previousSibling.textContent,
-            'udiam, quid sit, ');
-        assert.equal(tabs[1].nextSibling.textContent, 'quod Epicurum');
-      });
-
-      test('merging multiple other hls', function() {
-        element._applyRangedHighlight('some', 140, 1, 140, 80, 'left');
-        assert.instanceOf(contentText.firstChild, Text);
-        assert.equal(contentText.childNodes.length, 3);
-        var hl = contentText.querySelector('hl.some');
-        assert.strictEqual(contentText.firstChild, hl.previousSibling);
-        assert.equal(hl.childNodes.length, 5);
-        assert.equal(contentText.querySelectorAll('span.tab').length, 2);
-        assert.equal(hl.textContent,
-            'a💢ti te, inquit, sumus aliquando otiosum, certe a udiam, ' +
-            'quid sit, quod Epicuru');
-      });
-
-      test('hl inside Text node', function() {
-        // Before: na💢ti
-        //  After: n<hl class="some">a💢t</hl>i
-        element._applyRangedHighlight('some', 140, 1, 140, 4, 'left');
-        var hl = contentText.querySelector('hl.some');
-        assert.equal(hl.outerHTML, '<hl class="some">a💢t</hl>');
-      });
-
-      test('hl ending over different hl', function() {
-        // Before: na💢ti <hl>te, inquit</hl>,
-        //  After: na💢<hl class="some">ti te</hl><hl class="foo">, inquit</hl>,
-        element._applyRangedHighlight('some', 140, 3, 140, 8, 'left');
-        var hl = contentText.querySelector('hl.some');
-        assert.equal(hl.outerHTML, '<hl class="some">ti te</hl>');
-        assert.equal(hl.nextSibling.outerHTML,
-            '<hl class="foo">, inquit</hl>');
-      });
-
-      test('hl starting inside different hl', function() {
-        // Before: na💢ti <hl>te, inquit</hl>, sumus
-        //  After: na💢ti <hl class="foo">te, in</hl><hl class="some">quit, ...
-        element._applyRangedHighlight('some', 140, 12, 140, 21, 'left');
-        var hl = contentText.querySelector('hl.some');
-        assert.equal(hl.textContent, 'quit, sum');
-        assert.equal(
-            hl.previousSibling.outerHTML, '<hl class="foo">te, in</hl>');
-      });
-
-      test('hl inside different hl', function() {
-        // Before: na💢ti <hl class="foo">te, inquit</hl>, sumus
-        //  After: <hl class="foo">t</hl><hl="some">e, i</hl><hl class="foo">n..
-        element._applyRangedHighlight('some', 140, 7, 140, 12, 'left');
-        var hl = contentText.querySelector('hl.some');
-        assert.equal(hl.textContent, 'e, in');
-        assert.equal(hl.previousSibling.outerHTML, '<hl class="foo">t</hl>');
-        assert.equal(hl.nextSibling.outerHTML, '<hl class="foo">quit</hl>');
-      });
-
-      test('hl starts and ends in different hls', function() {
-        element._applyRangedHighlight('some', 140, 8, 140, 27, 'left');
-        var hl = contentText.querySelector('hl.some');
-        assert.equal(hl.textContent, ', inquit, sumus ali');
-        assert.equal(hl.previousSibling.outerHTML, '<hl class="foo">te</hl>');
-        assert.equal(hl.nextSibling.outerHTML, '<hl class="bar">quando</hl>');
-      });
-
-      test('hl over different hl', function() {
-        element._applyRangedHighlight('some', 140, 2, 140, 21, 'left');
-        var hl = contentText.querySelector('hl.some');
-        assert.equal(hl.outerHTML, '<hl class="some">💢ti te, inquit, sum</hl>');
-        assert.notOk(contentText.querySelector('.foo'));
-      });
-
-      test('hl starting and ending in boundaries', function() {
-        element._applyRangedHighlight('some', 140, 6, 140, 33, 'left');
-        var hl = contentText.querySelector('hl.some');
-        assert.equal(hl.textContent, 'te, inquit, sumus aliquando');
-        assert.notOk(contentText.querySelector('.bar'));
-      });
-
-      test('overlapping hls', function() {
-        element._applyRangedHighlight('some', 140, 1, 140, 3, 'left');
-        element._applyRangedHighlight('some', 140, 2, 140, 4, 'left');
-        assert.equal(contentText.querySelectorAll('hl.some').length, 1);
-        var hl = contentText.querySelector('hl.some');
-        assert.equal(hl.outerHTML, '<hl class="some">a💢t</hl>');
-      });
-
-      test('growing hl right including another hl', function() {
-        element._applyRangedHighlight('some', 140, 1, 140, 4, 'left');
-        element._applyRangedHighlight('some', 140, 3, 140, 10, 'left');
-        assert.equal(contentText.querySelectorAll('hl.some').length, 1);
-        var hl = contentText.querySelector('hl.some');
-        assert.equal(hl.outerHTML, '<hl class="some">a💢ti te, </hl>');
-        assert.equal(hl.nextSibling.outerHTML, '<hl class="foo">inquit</hl>');
-      });
-
-      test('growing hl left to start of line', function() {
-        element._applyRangedHighlight('some', 140, 2, 140, 5, 'left');
-        element._applyRangedHighlight('some', 140, 0, 140, 3, 'left');
-        assert.equal(contentText.querySelectorAll('hl.some').length, 1);
-        var hl = contentText.querySelector('hl.some');
-        assert.equal(hl.outerHTML, '<hl class="some">na💢ti</hl>');
-        assert.strictEqual(contentText.firstChild, hl);
-      });
-
-      test('splitting hl containing a tab', function() {
-        element._applyRangedHighlight('some', 140, 63, 140, 72, 'left');
-        assert.equal(contentText.querySelector('hl.some').textContent,
-            'sit, quod');
-        element._applyRangedHighlight('another', 140, 66, 140, 81, 'left');
-        assert.equal(contentText.querySelector('hl.another').textContent,
-            ', quod Epicurum');
-      });
-    });
-
-    test('_applyAllHighlights', function() {
-      element.comments = {
-        left: [
-          {
-            range: {
-              start_line: 3,
-              start_character: 14,
-              end_line: 10,
-              end_character: 24,
-            },
-          },
-        ],
-        right: [
-          {
-            range: {
-              start_line: 320,
-              start_character: 200,
-              end_line: 1024,
-              end_character: 768,
-            },
-          },
-        ],
-      };
-      sandbox.stub(element, '_applyRangedHighlight');
-      element._applyAllHighlights();
-      sinon.assert.calledWith(element._applyRangedHighlight,
-          'range', 3, 14, 10, 24, 'left');
-      sinon.assert.calledWith(element._applyRangedHighlight,
-          'range', 320, 200, 1024, 768, 'right');
-    });
-
-    test('apply comment ranges on render', function() {
-      sandbox.stub(element, '_applyAllHighlights');
-      element.fire('render');
-      assert.isTrue(element._applyAllHighlights.called);
-    });
-
-    test('apply comment ranges on context expand', function() {
-      sandbox.stub(element, '_applyAllHighlights');
-      element.fire('show-context');
-      assert.isTrue(element._applyAllHighlights.called);
-    });
-
     suite('selection', function() {
       var diff;
       var builder;

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

@@ -147,9 +147,10 @@ limitations under the License.
         <gr-diff-highlight
             id="highlights"
             logged-in="[[_loggedIn]]"
-            comments="[[_comments]]">
+            comments="{{_comments}}">
           <gr-diff-builder
               id="diffBuilder"
+              comments="[[_comments]]"
               view-mode="[[viewMode]]"
               is-image-diff="[[isImageDiff]]"
               base-image="[[_baseImage]]"

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

@@ -110,6 +110,7 @@
         var index = match[2];
         var comment = this.comments[side][index];
         if (comment && comment.range) {
+          this._commentMap[side] = this._computeCommentMap(this.comments[side]);
           this._notifyUpdateRange(
               comment.range.start_line, comment.range.end_line, side);
         }
@@ -168,13 +169,18 @@
     _getRangesForLine: function(line, side) {
       var lineNum = side === 'left' ? line.beforeNumber : line.afterNumber;
       var ranges = this.get(['_commentMap', side, lineNum]) || [];
-      return ranges.map(function(range) {
+      return ranges
-        return {
+          .map(function(range) {
-          start: range.start,
+            return {
-          end: range.end === -1 ? line.text.length : range.end,
+              start: range.start,
-          hovering: range.comment.__hovering,
+              end: range.end === -1 ? line.text.length : range.end,
-        };
+              hovering: !!range.comment.__hovering,
-      });
+            };
+          })
+          .sort(function(a, b) {
+            // Sort the ranges so that hovering highlights are on top.
+            return a.hovering && !b.hovering ? 1 : 0;
+          });
     },
   });
 })();

polygerrit-ui/app/elements/diff/gr-ranged-comment-layer/gr-ranged-comment-layer_test.html

@@ -216,7 +216,7 @@ limitations under the License.
       element.set(['comments', 'right', 0, '__hovering'], true);
 
       assert.isTrue(handlerSpy.called);
-      assert.isFalse(mapSpy.called);
+      assert.isTrue(mapSpy.called);
 
       assert.isTrue(notifyStub.called);
       var lastCall = notifyStub.lastCall;