Derive keyLocations from thread elements

This cuts one more dependency on Gerrit's comment model.

Change-Id: Ia0da0162f5bf97f350914cebda9be3b0a19a7680

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

@@ -136,6 +136,21 @@
        /** @type ?Defs.LineOfInterest */
       lineOfInterest: Object,
 
+      /**
+       * The key locations based on the comments and line of interests,
+       * where lines should not be collapsed.
+       *
+       * @type {{left: Object<(string|number), number>,
+       *     right: Object<(string|number), number>}}
+       */
+      _keyLocations: {
+        type: Object,
+        value: () => ({
+          left: {},
+          right: {},
+        }),
+      },
+
       loading: {
         type: Boolean,
         value: false,
@@ -230,7 +245,7 @@
     },
 
     attached() {
-      this._updateRangesWhenNodesChange();
+      this._observeNodes();
     },
 
     detached() {
@@ -238,25 +253,38 @@
       this._unobserveNodes();
     },
 
-    _updateRangesWhenNodesChange() {
+    _observeNodes() {
+      this._nodeObserver = Polymer.dom(this).observeNodes(info => {
+        const addedThreadEls = info.addedNodes.filter(isThreadEl);
+        // In principal we should also handle removed nodes, but I have not
+        // figured out how to do that yet without also catching all the removals
+        // caused by further redistribution. Right now, comments are never
+        // removed by no longer slotting them in, so I decided to not handle
+        // this situation until it occurs.
+        this._updateRanges(addedThreadEls);
+        this._updateKeyLocations(addedThreadEls);
+      });
+    },
+
+    _updateRanges(addedThreadEls) {
       function commentRangeFromThreadEl(threadEl) {
         const side = threadEl.getAttribute('comment-side');
         const range = JSON.parse(threadEl.getAttribute('range'));
         return {side, range, hovering: false};
       }
 
-      this._nodeObserver = Polymer.dom(this).observeNodes(info => {
+      const addedCommentRanges = addedThreadEls
-        const addedThreadEls = info.addedNodes.filter(isThreadEl);
+          .map(commentRangeFromThreadEl)
-        const addedCommentRanges = addedThreadEls
+          .filter(({range}) => range);
-            .map(commentRangeFromThreadEl)
+      this.push('_commentRanges', ...addedCommentRanges);
-            .filter(({range}) => range);
+    },
-        this.push('_commentRanges', ...addedCommentRanges);
+
-        // In principal we should also handle removed nodes, but I have not
+    _updateKeyLocations(addedThreadEls) {
-        // figured out how to do that yet without also catching all the removals
+      for (const threadEl of addedThreadEls) {
-        // caused by further redistribution. Right now, comments are never
+        const commentSide = threadEl.getAttribute('comment-side');
-        // removed by no longer slotting them in, so I decided to not handle
+        const lineNum = threadEl.getAttribute('line-num') || GrDiffLine.FILE;
-        // this situation until it occurs.
+        this._keyLocations[commentSide][lineNum] = true;
-      });
+      }
     },
 
     /** Cancel any remaining diff builder rendering work. */
@@ -645,9 +673,12 @@
       }
 
       this._showWarning = false;
-      const keyLocations = this._getKeyLocations(this.comments,
+
-          this.lineOfInterest);
+      if (this.lineOfInterest) {
-      this.$.diffBuilder.render(keyLocations, this._getBypassPrefs());
+        const side = this.lineOfInterest.leftSide ? 'left' : 'right';
+        this._keyLocations[side][this.lineOfInterest.number] = true;
+      }
+      this.$.diffBuilder.render(this._keyLocations, this._getBypassPrefs());
     },
 
     _handleRenderContent() {
@@ -683,39 +714,6 @@
       }
     },
 
-    /**
-     * Returns the key locations based on the comments and line of interests,
-     * where lines should not be collapsed.
-     *
-     * @param {!Object} comments
-     * @param {Defs.LineOfInterest|null} lineOfInterest
-     *
-     * @return {{left: Object<(string|number), boolean>,
-     *     right: Object<(string|number), boolean>}}
-     */
-    _getKeyLocations(comments, lineOfInterest) {
-      const result = {
-        left: {},
-        right: {},
-      };
-      for (const side in comments) {
-        if (side !== GrDiffBuilder.Side.LEFT &&
-            side !== GrDiffBuilder.Side.RIGHT) {
-          continue;
-        }
-        for (const c of comments[side]) {
-          result[side][c.line || GrDiffLine.FILE] = true;
-        }
-      }
-
-      if (lineOfInterest) {
-        const side = lineOfInterest.leftSide ? 'left' : 'right';
-        result[side][lineOfInterest.number] = true;
-      }
-
-      return result;
-    },
-
     /**
      * Get the preferences object including the safety bypass context (if any).
      */

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

@@ -1128,31 +1128,55 @@ limitations under the License.
       });
     });
 
-    test('_getKeyLocations', () => {
+    suite('key locations', () => {
-      assert.deepEqual(element._getKeyLocations({left: [], right: []}, null),
+      let renderStub;
-          {left: {}, right: {}});
+
-      const comments = {
+      setup(() => {
-        left: [{line: 123}, {}],
+        element = fixture('basic');
-        right: [{line: 456}],
+        element.prefs = {};
-      };
+        renderStub = sandbox.stub(element.$.diffBuilder, 'render');
-      assert.deepEqual(element._getKeyLocations(comments, null), {
-        left: {FILE: true, 123: true},
-        right: {456: true},
       });
 
-      const lineOfInterest = {number: 789, leftSide: true};
+      test('lineOfInterest is a key location', () => {
-      assert.deepEqual(
+        element.lineOfInterest = {number: 789, leftSide: true};
-          element._getKeyLocations(comments, lineOfInterest), {
+        element._renderDiffTable();
-            left: {FILE: true, 123: true, 789: true},
+        assert.isTrue(renderStub.called);
-            right: {456: true},
+        assert.deepEqual(renderStub.lastCall.args[0], {
-          });
+          left: {789: true},
+          right: {},
+        });
+      });
 
-      delete lineOfInterest.leftSide;
+      test('line comments are key locations', () => {
-      assert.deepEqual(
+        const threadEl = document.createElement('div');
-          element._getKeyLocations(comments, lineOfInterest), {
+        threadEl.className = 'comment-thread';
-            left: {FILE: true, 123: true},
+        threadEl.setAttribute('comment-side', 'right');
-            right: {456: true, 789: true},
+        threadEl.setAttribute('line-num', 3);
-          });
+        Polymer.dom(element).appendChild(threadEl);
+        Polymer.dom.flush();
+
+        element._renderDiffTable();
+        assert.isTrue(renderStub.called);
+        assert.deepEqual(renderStub.lastCall.args[0], {
+          left: {},
+          right: {3: true},
+        });
+      });
+
+      test('file comments are key locations', () => {
+        const threadEl = document.createElement('div');
+        threadEl.className = 'comment-thread';
+        threadEl.setAttribute('comment-side', 'left');
+        Polymer.dom(element).appendChild(threadEl);
+        Polymer.dom.flush();
+
+        element._renderDiffTable();
+        assert.isTrue(renderStub.called);
+        assert.deepEqual(renderStub.lastCall.args[0], {
+          left: {FILE: true},
+          right: {},
+        });
+      });
     });
   });