Merge changes I58da4785,I6facd64e,Ifdb5da86

* changes:
  Replace gr-diff-comment-thread-group with div
  Move gr-diff-comment-thread factory to diff-host
  Move the actual comment element creation to host

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

@@ -20,10 +20,10 @@
   // Prevent redefinition.
   if (window.GrDiffBuilderBinary) { return; }
 
-  function GrDiffBuilderBinary(diff, comments, parentIndex, changeNum, path,
+  function GrDiffBuilderBinary(diff, patchRange, commentThreadEls, prefs,
-      projectName, prefs, outputEl) {
+      outputEl) {
-    GrDiffBuilder.call(this, diff, comments, parentIndex, changeNum, path,
+    GrDiffBuilder.call(this, diff, patchRange, commentThreadEls, prefs,
-        projectName, prefs, outputEl);
+        outputEl);
   }
 
   GrDiffBuilderBinary.prototype = Object.create(GrDiffBuilder.prototype);

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

@@ -22,10 +22,10 @@
 
   const IMAGE_MIME_PATTERN = /^image\/(bmp|gif|jpeg|jpg|png|tiff|webp)$/;
 
-  function GrDiffBuilderImage(diff, comments, parentIndex, changeNum, path,
+  function GrDiffBuilderImage(diff, patchRange, commentThreadEls, prefs,
-      projectName, prefs, outputEl, baseImage, revisionImage) {
+      outputEl, baseImage, revisionImage) {
-    GrDiffBuilderSideBySide.call(this, diff, comments, parentIndex, changeNum,
+    GrDiffBuilderSideBySide.call(this, diff, patchRange, commentThreadEls,
-        path, projectName, prefs, outputEl, []);
+        prefs, outputEl, []);
     this._baseImage = baseImage;
     this._revisionImage = revisionImage;
   }

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

@@ -20,10 +20,10 @@
   // Prevent redefinition.
   if (window.GrDiffBuilderSideBySide) { return; }
 
-  function GrDiffBuilderSideBySide(diff, comments, parentIndex, changeNum, path,
+  function GrDiffBuilderSideBySide(diff, patchRange, commentThreadEls,
-      projectName, prefs, outputEl, layers) {
+      prefs, outputEl, layers) {
-    GrDiffBuilder.call(this, diff, comments, parentIndex, changeNum, path,
+    GrDiffBuilder.call(this, diff, patchRange, commentThreadEls, prefs,
-        projectName, prefs, outputEl, layers);
+        outputEl, layers);
   }
   GrDiffBuilderSideBySide.prototype = Object.create(GrDiffBuilder.prototype);
   GrDiffBuilderSideBySide.prototype.constructor = GrDiffBuilderSideBySide;

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

@@ -20,10 +20,10 @@
   // Prevent redefinition.
   if (window.GrDiffBuilderUnified) { return; }
 
-  function GrDiffBuilderUnified(diff, comments, parentIndex, changeNum, path,
+  function GrDiffBuilderUnified(diff, patchRange, commentThreadEls, prefs,
-      projectName, prefs, outputEl, layers) {
+      outputEl, layers) {
-    GrDiffBuilder.call(this, diff, comments, parentIndex, changeNum, path,
+    GrDiffBuilder.call(this, diff, patchRange, commentThreadEls, prefs,
-        projectName, prefs, outputEl, layers);
+        outputEl, layers);
   }
   GrDiffBuilderUnified.prototype = Object.create(GrDiffBuilder.prototype);
   GrDiffBuilderUnified.prototype.constructor = GrDiffBuilderUnified;

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

@@ -16,9 +16,10 @@ limitations under the License.
 -->
 <link rel="import" href="../../../bower_components/polymer/polymer.html">
 <link rel="import" href="../../core/gr-reporting/gr-reporting.html">
+<link rel="import" href="../../shared/gr-js-api-interface/gr-js-api-interface.html">
+<link rel="import" href="../gr-diff-processor/gr-diff-processor.html">
 <link rel="import" href="../gr-diff-comment-thread/gr-diff-comment-thread.html">
 <link rel="import" href="../gr-diff-comment-thread-group/gr-diff-comment-thread-group.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">
 <link rel="import" href="../gr-syntax-layer/gr-syntax-layer.html">
 
@@ -131,6 +132,10 @@ limitations under the License.
           return this.queryEffectiveChildren('#diffTable');
         },
 
+        get _commentThreadElements() {
+          return this.queryAllEffectiveChildren('.comment-thread');
+        },
+
         observers: [
           '_groupsChanged(_groups.splices)',
         ],
@@ -166,7 +171,8 @@ limitations under the License.
           // Stop the processor and syntax layer (if they're running).
           this.cancel();
 
-          this._builder = this._getDiffBuilder(this.diff, comments, prefs);
+          this._builder = this._getDiffBuilder(
+              this.diff, comments.meta.patchRange, prefs);
 
           this.$.processor.context = prefs.context;
           this.$.processor.keyLocations = this._getKeyLocations(comments,
@@ -290,7 +296,7 @@ limitations under the License.
           throw Error(`Invalid preference value: ${pref}`);
         },
 
-        _getDiffBuilder(diff, comments, prefs) {
+        _getDiffBuilder(diff, patchRange, prefs) {
           if (isNaN(prefs.tab_size) || prefs.tab_size <= 0) {
             this._handlePreferenceError('tab size');
             return;
@@ -303,22 +309,21 @@ limitations under the License.
 
           let builder = null;
           if (this.isImageDiff) {
-            builder = new GrDiffBuilderImage(diff, comments, this.parentIndex,
+            builder = new GrDiffBuilderImage(diff, patchRange,
-              this.changeNum, this.path, this.projectName, prefs,
+              this._commentThreadElements, prefs, this.diffElement,
-              this.diffElement, this.baseImage, this.revisionImage);
+              this.baseImage, this.revisionImage);
           } else if (diff.binary) {
             // If the diff is binary, but not an image.
-            return new GrDiffBuilderBinary(diff, comments, this.parentIndex,
+            return new GrDiffBuilderBinary(diff, patchRange,
-                this.changeNum, this.path, this.projectName, prefs,
+                this._commentThreadElements, prefs, this.diffElement);
-                this.diffElement);
           } else if (this.viewMode === DiffViewMode.SIDE_BY_SIDE) {
-            builder = new GrDiffBuilderSideBySide(diff, comments,
+            builder = new GrDiffBuilderSideBySide(diff, patchRange,
-                this.parentIndex, this.changeNum, this.path, this.projectName,
+                this._commentThreadElements, prefs, this.diffElement,
-                prefs, this.diffElement, this._layers);
+                this._layers);
           } else if (this.viewMode === DiffViewMode.UNIFIED) {
-            builder = new GrDiffBuilderUnified(diff, comments, this.parentIndex,
+            builder = new GrDiffBuilderUnified(diff, patchRange,
-                this.changeNum, this.path, this.projectName, prefs,
+                this._commentThreadElements, prefs, this.diffElement,
-                this.diffElement, this._layers);
+                this._layers);
           }
           if (!builder) {
             throw Error('Unsupported diff view mode: ' + this.viewMode);

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

@@ -96,14 +96,10 @@
    */
   const REGEX_TAB_OR_SURROGATE_PAIR = /\t|[\uD800-\uDBFF][\uDC00-\uDFFF]/;
 
-  function GrDiffBuilder(diff, comments, parentIndex, changeNum, path,
+  function GrDiffBuilder(diff, patchRange, commentThreadEls, prefs,
-      projectName, prefs, outputEl, layers) {
+      outputEl, layers) {
     this._diff = diff;
-    this._comments = comments;
+    this._patchRange = patchRange;
-    this._parentIndex = parentIndex;
-    this._changeNum = changeNum;
-    this._path = path;
-    this._projectName = projectName;
     this._prefs = prefs;
     this._outputEl = outputEl;
     this.groups = [];
@@ -125,25 +121,7 @@
       }
     }
 
-    if (!this._comments) {
+    this._threadEls = commentThreadEls;
-      this._threadEls = [];
-      return;
-    }
-
-    const allComments = [];
-    for (const side of [GrDiffBuilder.Side.LEFT, GrDiffBuilder.Side.RIGHT]) {
-      // This is needed by the threading.
-      for (const comment of this._comments[side]) {
-        comment.__commentSide = side;
-      }
-      allComments.push(...this._comments[side]);
-    }
-    const threads = this._createThreads(allComments);
-    this._threadEls = threads.map(thread => {
-      return Gerrit.createThreadElement(
-          thread, this._parentIndex, this._changeNum,
-          this._path, this._projectName);
-    });
   }
 
   GrDiffBuilder.GroupType = {
@@ -396,88 +374,27 @@
     return button;
   };
 
-  /**
-   * @param {Array<Object>} comments
-   */
-  GrDiffBuilder.prototype._createThreads = function(comments) {
-    const sortedComments = comments.slice(0).sort((a, b) => {
-      if (b.__draft && !a.__draft ) { return 0; }
-      if (a.__draft && !b.__draft ) { return 1; }
-      return util.parseDate(a.updated) - util.parseDate(b.updated);
-    });
-
-    const threads = [];
-    for (const comment of sortedComments) {
-      // If the comment is in reply to another comment, find that comment's
-      // thread and append to it.
-      if (comment.in_reply_to) {
-        const thread = threads.find(thread =>
-            thread.comments.some(c => c.id === comment.in_reply_to));
-        if (thread) {
-          thread.comments.push(comment);
-          continue;
-        }
-      }
-
-      // Otherwise, this comment starts its own thread.
-      const newThread = {
-        start_datetime: comment.updated,
-        comments: [comment],
-        commentSide: comment.__commentSide,
-        patchNum: comment.patch_set,
-        rootId: comment.id || comment.__draftID,
-        lineNum: comment.line,
-        isOnParent: comment.side === 'PARENT',
-      };
-      if (comment.range) {
-        newThread.range = Object.assign({}, comment.range);
-      }
-      threads.push(newThread);
-    }
-    return threads;
-  };
-
-  /**
-   * Returns the patch number that new comment threads should be attached to.
-   *
-   * @param {GrDiffLine} line The line new thread will be attached to.
-   * @param {string=} opt_side Set to LEFT to force adding it to the LEFT side -
-   *     will be ignored if the left is a parent or a merge parent
-   * @return {number} Patch set to attach the new thread to
-   */
-  GrDiffBuilder.prototype._determinePatchNumForNewThreads = function(
-      patchRange, line, opt_side) {
-    if ((line.type === GrDiffLine.Type.REMOVE ||
-         opt_side === GrDiffBuilder.Side.LEFT) &&
-        patchRange.basePatchNum !== 'PARENT' &&
-        !Gerrit.PatchSetBehavior.isMergeParent(patchRange.basePatchNum)) {
-      return patchRange.basePatchNum;
-    } else {
-      return patchRange.patchNum;
-    }
-  };
-
   /**
    * @param {!GrDiffLine} line
-   * @param {!GrDiffBuilder.Side=} side The side (LEFT, RIGHT, BOTH) for which to return
+   * @param {!GrDiffBuilder.Side=} side The side (LEFT, RIGHT, BOTH) for which
-   *     the thread group (default: BOTH).
+   *     to return the thread group (default: BOTH).
    * @return {!Object}
    */
   GrDiffBuilder.prototype._commentThreadGroupForLine = function(
-      line, side = GrDiffBuilder.Side.BOTH) {
+      line, commentSide = GrDiffBuilder.Side.BOTH) {
     const threadElsForGroup =
-        Gerrit.filterThreadElsForLocation(this._threadEls, line, side);
+        Gerrit.filterThreadElsForLocation(this._threadEls, line, commentSide);
     if (!threadElsForGroup || threadElsForGroup.length === 0) {
       return null;
     }
 
-    const threadGroupEl =
+    const threadGroupEl = document.createElement('div');
-        document.createElement('gr-diff-comment-thread-group');
+    threadGroupEl.className = 'thread-group';
     for (const threadEl of threadElsForGroup) {
       Polymer.dom(threadGroupEl).appendChild(threadEl);
     }
-    if (side) {
+    if (commentSide) {
-      threadGroupEl.setAttribute('data-side', side);
+      threadGroupEl.setAttribute('data-side', commentSide);
     }
     return threadGroupEl;
   };

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

@@ -62,10 +62,6 @@ limitations under the License.
     let builder;
     let sandbox;
     const LINE_FEED_HTML = '<span class="style-scope gr-diff br"></span>';
-    const parentIndex = 3;
-    const changeNum = 4;
-    const path = 'some/path.ext';
-    const projectName = 'Awesome Project';
 
     setup(() => {
       sandbox = sinon.sandbox.create();
@@ -80,8 +76,7 @@ limitations under the License.
         tab_size: 4,
       };
       builder = new GrDiffBuilder(
-          {content: []}, {left: [], right: []}, parentIndex, changeNum, path,
+          {content: []}, {left: [], right: []}, [], prefs);
-          projectName, prefs);
     });
 
     teardown(() => { sandbox.restore(); });
@@ -145,153 +140,6 @@ limitations under the License.
           Gerrit.DiffSide.RIGHT), [r]);
     });
 
-    test('_createThreads', () => {
-      const comments = [
-        {
-          id: 'sallys_confession',
-          message: 'i like you, jack',
-          updated: '2015-12-23 15:00:20.396000000',
-          line: 1,
-          __commentSide: 'left',
-        }, {
-          id: 'jacks_reply',
-          message: 'i like you, too',
-          updated: '2015-12-24 15:01:20.396000000',
-          __commentSide: 'left',
-          line: 1,
-          in_reply_to: 'sallys_confession',
-        },
-        {
-          id: 'new_draft',
-          message: 'i do not like either of you',
-          __commentSide: 'left',
-          __draft: true,
-          updated: '2015-12-20 15:01:20.396000000',
-        },
-      ];
-
-      const actualThreads = builder._createThreads(comments);
-
-      assert.equal(actualThreads.length, 2);
-
-      assert.equal(
-          actualThreads[0].start_datetime, '2015-12-23 15:00:20.396000000');
-      assert.equal(actualThreads[0].commentSide, 'left');
-      assert.equal(actualThreads[0].comments.length, 2);
-      assert.deepEqual(actualThreads[0].comments[0], comments[0]);
-      assert.deepEqual(actualThreads[0].comments[1], comments[1]);
-      assert.equal(actualThreads[0].patchNum, undefined);
-      assert.equal(actualThreads[0].rootId, 'sallys_confession');
-      assert.equal(actualThreads[0].lineNum, 1);
-
-      assert.equal(
-          actualThreads[1].start_datetime, '2015-12-20 15:01:20.396000000');
-      assert.equal(actualThreads[1].commentSide, 'left');
-      assert.equal(actualThreads[1].comments.length, 1);
-      assert.deepEqual(actualThreads[1].comments[0], comments[2]);
-      assert.equal(actualThreads[1].patchNum, undefined);
-      assert.equal(actualThreads[1].rootId, 'new_draft');
-      assert.equal(actualThreads[1].lineNum, undefined);
-    });
-
-    test('_createThreads inherits patchNum and range', () => {
-      const comments = [{
-        id: 'betsys_confession',
-        message: 'i like you, jack',
-        updated: '2015-12-24 15:00:10.396000000',
-        range: {
-          start_line: 1,
-          start_character: 1,
-          end_line: 1,
-          end_character: 2,
-        },
-        patch_set: 5,
-        __commentSide: 'left',
-        line: 1,
-      }];
-
-      expectedThreads = [
-        {
-          start_datetime: '2015-12-24 15:00:10.396000000',
-          commentSide: 'left',
-          comments: [{
-            id: 'betsys_confession',
-            message: 'i like you, jack',
-            updated: '2015-12-24 15:00:10.396000000',
-            range: {
-              start_line: 1,
-              start_character: 1,
-              end_line: 1,
-              end_character: 2,
-            },
-            patch_set: 5,
-            __commentSide: 'left',
-            line: 1,
-          }],
-          patchNum: 5,
-          rootId: 'betsys_confession',
-          range: {
-            start_line: 1,
-            start_character: 1,
-            end_line: 1,
-            end_character: 2,
-          },
-          lineNum: 1,
-          isOnParent: false,
-        },
-      ];
-
-      assert.deepEqual(
-          builder._createThreads(comments),
-          expectedThreads);
-    });
-
-    test('_createThreads does not thread unrelated comments at same location',
-        () => {
-          const comments = [
-            {
-              id: 'sallys_confession',
-              message: 'i like you, jack',
-              updated: '2015-12-23 15:00:20.396000000',
-              __commentSide: 'left',
-            }, {
-              id: 'jacks_reply',
-              message: 'i like you, too',
-              updated: '2015-12-24 15:01:20.396000000',
-              __commentSide: 'left',
-            },
-          ];
-          assert.equal(builder._createThreads(comments).length, 2);
-        });
-
-    test('_createThreads derives isOnParent using  side from first comment',
-        () => {
-          const comments = [
-            {
-              id: 'sallys_confession',
-              message: 'i like you, jack',
-              updated: '2015-12-23 15:00:20.396000000',
-              // line: 1,
-              // __commentSide: 'left',
-            }, {
-              id: 'jacks_reply',
-              message: 'i like you, too',
-              updated: '2015-12-24 15:01:20.396000000',
-              // __commentSide: 'left',
-              // line: 1,
-              in_reply_to: 'sallys_confession',
-            },
-          ];
-
-          assert.equal(builder._createThreads(comments)[0].isOnParent, false);
-
-          comments[0].side = 'REVISION';
-          assert.equal(builder._createThreads(comments)[0].isOnParent, false);
-
-          comments[0].side = 'PARENT';
-          assert.equal(builder._createThreads(comments)[0].isOnParent, true);
-        });
-
     test('_createElement classStr applies all classes', () => {
       const node = builder._createElement('div', 'test classes');
       assert.isTrue(node.classList.contains('gr-diff'));
@@ -466,42 +314,32 @@ limitations under the License.
     });
 
     test('comment thread group creation', () => {
-      const l3 = {id: 'l3', line: 3, updated: '2016-08-09 00:42:32.000000000',
+      const l3 = document.createElement('div');
-        __commentSide: 'left', side: 'PARENT'};
+      l3.className = 'comment-thread';
-      const l5 = {id: 'l5', line: 5, updated: '2016-08-09 00:42:32.000000000',
+      l3.setAttribute('comment-side', 'left');
-        __commentSide: 'left', patch_set: '2', side: 'REVISION'};
+      l3.setAttribute('line-num', 3);
-      const r5 = {id: 'r5', line: 5, updated: '2016-08-09 00:42:32.000000000',
+
-        __commentSide: 'right', patch_set: '3'};
+      const l5 = document.createElement('div');
+      l5.className = 'comment-thread';
+      l5.setAttribute('comment-side', 'left');
+      l5.setAttribute('line-num', 5);
+
+      const r5 = document.createElement('div');
+      r5.className = 'comment-thread';
+      r5.setAttribute('comment-side', 'right');
+      r5.setAttribute('line-num', 5);
 
       builder = new GrDiffBuilder(
-          {content: []}, {
+          {content: []}, {basePatchNum: 'PARENT', patchNum: '3'}, [l3, l5, r5],
-            meta: {
+          prefs);
-              changeNum: '42',
-              patchRange: {
-                basePatchNum: 'PARENT',
-                patchNum: '3',
-              },
-              path: '/path/to/foo',
-              projectConfig: {foo: 'bar'},
-            },
-            left: [l3, l5],
-            right: [r5],
-          }, parentIndex, changeNum, path, projectName, prefs);
 
-      function checkThreadGroupProps(threadGroupEl, patchNum,
+      function checkThreadGroupProps(threadGroupEl,
-          comments) {
+          expectedThreadEls) {
         const threadEls = Polymer.dom(threadGroupEl).queryDistributedElements(
-            'gr-diff-comment-thread');
+            '.comment-thread');
-        assert.equal(threadEls.length, comments.length);
+        assert.equal(threadEls.length, expectedThreadEls.length);
-        for (let i=0; i < comments.length; i++) {
+        for (let i=0; i<expectedThreadEls.length; i++) {
-          const threadEl = threadEls[i];
+          assert.equal(threadEls[i], expectedThreadEls[i]);
-
-          const comment = comments[i];
-          assert.equal(threadEl.patchNum, comment.patch_set);
-          assert.equal(threadEl.commentSide, comment.__commentSide);
-          assert.equal(threadEl.comments.length, 1);
-          assert.equal(threadEl.comments[0], comment);
-          assert.equal(threadEl.rootId, comment.id);
         }
       }
 
@@ -509,42 +347,42 @@ limitations under the License.
       line.beforeNumber = 5;
       line.afterNumber = 5;
       let threadGroupEl = builder._commentThreadGroupForLine(line);
-      checkThreadGroupProps(threadGroupEl, '3', [l5, r5]);
+      checkThreadGroupProps(threadGroupEl, [l5, r5]);
 
       threadGroupEl =
           builder._commentThreadGroupForLine(line, GrDiffBuilder.Side.RIGHT);
-      checkThreadGroupProps(threadGroupEl, '3', [r5]);
+      checkThreadGroupProps(threadGroupEl, [r5]);
 
       threadGroupEl =
           builder._commentThreadGroupForLine(line, GrDiffBuilder.Side.LEFT);
-      checkThreadGroupProps(threadGroupEl, '3', [l5]);
+      checkThreadGroupProps(threadGroupEl, [l5]);
 
-      builder._comments.meta.patchRange.basePatchNum = '1';
+      builder._patchRange.basePatchNum = '1';
 
       threadGroupEl = builder._commentThreadGroupForLine(line);
-      checkThreadGroupProps(threadGroupEl, '3', [l5, r5]);
+      checkThreadGroupProps(threadGroupEl, [l5, r5]);
 
       threadEl =
           builder._commentThreadGroupForLine(line, GrDiffBuilder.Side.LEFT);
-      checkThreadGroupProps(threadEl, '1', [l5]);
+      checkThreadGroupProps(threadEl, [l5]);
 
       threadGroupEl =
           builder._commentThreadGroupForLine(line, GrDiffBuilder.Side.RIGHT);
-      checkThreadGroupProps(threadGroupEl, '3', [r5]);
+      checkThreadGroupProps(threadGroupEl, [r5]);
 
-      builder._comments.meta.patchRange.basePatchNum = 'PARENT';
+      builder._patchRange.basePatchNum = 'PARENT';
 
       line = new GrDiffLine(GrDiffLine.Type.REMOVE);
       line.beforeNumber = 5;
       line.afterNumber = 5;
       threadGroupEl = builder._commentThreadGroupForLine(line);
-      checkThreadGroupProps(threadGroupEl, '3', [l5, r5]);
+      checkThreadGroupProps(threadGroupEl, [l5, r5]);
 
       line = new GrDiffLine(GrDiffLine.Type.ADD);
       line.beforeNumber = 3;
       line.afterNumber = 5;
       threadGroupEl = builder._commentThreadGroupForLine(line);
-      checkThreadGroupProps(threadGroupEl, '3', [l3, r5]);
+      checkThreadGroupProps(threadGroupEl, [l3, r5]);
     });
 
 
@@ -578,16 +416,18 @@ limitations under the License.
       });
 
       const lineOfInterest = {number: 789, leftSide: true};
-      assert.deepEqual(element._getKeyLocations(comments, lineOfInterest), {
+      assert.deepEqual(
-        left: {FILE: true, 123: true, 789: true},
+          element._getKeyLocations(comments, lineOfInterest), {
-        right: {456: true},
+            left: {FILE: true, 123: true, 789: true},
-      });
+            right: {456: true},
+          });
 
       delete lineOfInterest.leftSide;
-      assert.deepEqual(element._getKeyLocations(comments, lineOfInterest), {
+      assert.deepEqual(
-        left: {FILE: true, 123: true},
+          element._getKeyLocations(comments, lineOfInterest), {
-        right: {456: true, 789: true},
+            left: {FILE: true, 123: true},
-      });
+            right: {456: true, 789: true},
+          });
     });
 
     suite('_isTotal', () => {
@@ -1029,7 +869,7 @@ limitations under the License.
         processStub = sandbox.stub(element.$.processor, 'process')
             .returns(Promise.resolve());
         sandbox.stub(element, '_anyLineTooLong').returns(true);
-        comments = {left: [], right: []};
+        comments = {left: [], right: [], meta: {patchRange: undefined}};
         prefs = {
           line_length: 10,
           show_tabs: true,
@@ -1077,6 +917,7 @@ limitations under the License.
     suite('rendering', () => {
       let content;
       let outputEl;
+      let comments;
 
       setup(done => {
         const prefs = {
@@ -1104,10 +945,10 @@ limitations under the License.
         });
         element = fixture('basic');
         outputEl = element.queryEffectiveChildren('#diffTable');
+        comments = {left: [], right: [], meta: {patchRange: undefined}};
         sandbox.stub(element, '_getDiffBuilder', () => {
           const builder = new GrDiffBuilder(
-              {content}, {left: [], right: []}, parentIndex, changeNum, path,
+              {content}, undefined, [], prefs, outputEl);
-              projectName, prefs, outputEl);
           sandbox.stub(builder, 'addColumns');
           builder.buildSectionElement = function(group) {
             const section = document.createElement('stub');
@@ -1119,7 +960,7 @@ limitations under the License.
           return builder;
         });
         element.diff = {content};
-        element.render({left: [], right: []}, prefs).then(done);
+        element.render(comments, prefs).then(done);
       });
 
       test('reporting', done => {
@@ -1144,7 +985,7 @@ limitations under the License.
       });
 
       test('addColumns is called', done => {
-        element.render({left: [], right: []}, {}).then(done);
+        element.render(comments, {}).then(done);
         assert.isTrue(element._builder.addColumns.called);
       });
 
@@ -1168,7 +1009,7 @@ limitations under the License.
 
       test('render-start and render are fired', done => {
         const dispatchEventStub = sandbox.stub(element, 'dispatchEvent');
-        element.render({left: [], right: []}, {}).then(() => {
+        element.render(comments, {}).then(() => {
           const firedEventTypes = dispatchEventStub.getCalls()
               .map(c => { return c.args[0].type; });
           assert.include(firedEventTypes, 'render-start');
@@ -1196,7 +1037,7 @@ limitations under the License.
           context: -1,
           syntax_highlighting: true,
         };
-        element.render({left: [], right: []}, prefs);
+        element.render(comments, prefs);
       });
 
       test('cancel', () => {
@@ -1213,6 +1054,7 @@ limitations under the License.
       let builder;
       let diff;
       let prefs;
+      let comments;
 
       setup(done => {
         element = fixture('mock-diff');
@@ -1224,12 +1066,12 @@ limitations under the License.
           show_tabs: true,
           tab_size: 4,
         };
+        comments = {left: [], right: [], meta: {patchRange: undefined}};
 
-        element.render(
+        element.render(comments, prefs).then(() => {
-            undefined, prefs).then(() => {
+          builder = element._builder;
-              builder = element._builder;
+          done();
-              done();
+        });
-            });
       });
 
       test('getDiffLength', () => {
@@ -1336,7 +1178,7 @@ limitations under the License.
       test('_getNextContentOnSide unified left', done => {
         // Re-render as unified:
         element.viewMode = 'UNIFIED_DIFF';
-        element.render({left: [], right: []}, prefs).then(() => {
+        element.render(comments, prefs).then(() => {
           builder = element._builder;
 
           const startElem = builder.getContentByLine(5, 'left',
@@ -1356,7 +1198,7 @@ limitations under the License.
       test('_getNextContentOnSide unified right', done => {
         // Re-render as unified:
         element.viewMode = 'UNIFIED_DIFF';
-        element.render({left: [], right: []}, prefs).then(() => {
+        element.render(comments, prefs).then(() => {
           builder = element._builder;
 
           const startElem = builder.getContentByLine(5, 'right',

polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.html

@@ -1,37 +0,0 @@
-<!--
-@license
-Copyright (C) 2017 The Android Open Source Project
-
-Licensed under the Apache License, Version 2.0 (the "License");
-you may not use this file except in compliance with the License.
-You may obtain a copy of the License at
-
-http://www.apache.org/licenses/LICENSE-2.0
-
-Unless required by applicable law or agreed to in writing, software
-distributed under the License is distributed on an "AS IS" BASIS,
-WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-See the License for the specific language governing permissions and
-limitations under the License.
--->
-
-<link rel="import" href="../../../bower_components/polymer/polymer.html">
-<link rel="import" href="../gr-diff-comment-thread/gr-diff-comment-thread.html">
-<link rel="import" href="../../../styles/shared-styles.html">
-
-<dom-module id="gr-diff-comment-thread-group">
-  <template>
-    <style include="shared-styles">
-      :host {
-        display: block;
-        max-width: var(--content-width, 80ch);
-        white-space: normal;
-      }
-      gr-diff-comment-thread + gr-diff-comment-thread {
-        margin-top: .2em;
-      }
-    </style>
-    <slot></slot>
-  </template>
-  <script src="gr-diff-comment-thread-group.js"></script>
-</dom-module>

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

@@ -1,118 +0,0 @@
-/**
- * @license
- * Copyright (C) 2017 The Android Open Source Project
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-(function() {
-  'use strict';
-
-  window.Gerrit = window.Gerrit || {};
-
-  // This method will eventually move to gr-diff-host (so that gr-diff and it's
-  // descendants, including gr-diff-comment-thread-group, do not depend on a
-  // specific comment thread implementation, but can instead be used with other
-  // comment widgets). I cannot move it there yet, because it is still called
-  // from this file, and this file cannot depend on gr-diff-host. I decided to
-  // make it a function on the global Gerrit namespace, so that
-  //   1) I can move the call-side in the next change without moving this code,
-  //      and thereby reduce the number of moving parts per commit.
-  //   2) To already now cut the ties to the this object - if it was an element
-  //      method, I would probably want to use isOnParent etc. from `this`, and
-  //      thus be required to change the code when I move it to gr-diff host.
-  //      Making it a free function first requires me to catch any references to
-  //      `this` and instead pass  those in as parameter, which then allows me
-  //      to move it later without any other changes, which makes the diff
-  //      easier to read.
-  /**
-   * @param {Object} thread
-   * @param {number} parentIndex
-   * @param {number} changeNum
-   * @param {string} path
-   * @param {string} projectName
-   */
-  window.Gerrit.createThreadElement = function(
-      thread, parentIndex, changeNum, path, projectName) {
-    const threadEl = document.createElement('gr-diff-comment-thread');
-    threadEl.comments = thread.comments;
-    threadEl.commentSide = thread.commentSide;
-    threadEl.isOnParent = thread.isOnParent;
-    threadEl.parentIndex = parentIndex;
-    threadEl.changeNum = changeNum;
-    threadEl.patchNum = thread.patchNum;
-    threadEl.lineNum = thread.lineNum;
-    const rootIdChangedListener = changeEvent => {
-      thread.rootId = changeEvent.detail.value;
-    };
-    threadEl.addEventListener('root-id-changed', rootIdChangedListener);
-    threadEl.path = path;
-    threadEl.projectName = projectName;
-    threadEl.range = thread.range;
-    const threadDiscardListener = e => {
-      const threadEl = /** @type {!Node} */ (e.currentTarget);
-      const parent = Polymer.dom(threadEl).parentNode;
-      threadEl.removeEventListener('root-id-changed', rootIdChangedListener);
-      threadEl.removeEventListener('thread-discard', threadDiscardListener);
-      Polymer.dom(parent).removeChild(threadEl);
-    };
-    threadEl.addEventListener('thread-discard', threadDiscardListener);
-    return threadEl;
-  };
-
-  Polymer({
-    is: 'gr-diff-comment-thread-group',
-
-    properties: {
-    },
-
-    get threadEls() {
-      return Polymer.dom(this).queryDistributedElements(
-          'gr-diff-comment-thread');
-    },
-
-    /**
-     * Fetch the thread element at the given range, or the range-less thread
-     * element on the line if no range is provided, lineNum, and side.
-     *
-     * @param {string} side
-     * @param {!Object=} opt_range
-     * @return {!Object|undefined}
-     */
-    getThreadEl(side, opt_range) {
-      const threads = [].filter.call(this.threadEls,
-          thread => this._rangesEqual(thread.range, opt_range))
-          .filter(thread => thread.commentSide === side);
-      if (threads.length === 1) {
-        return threads[0];
-      }
-    },
-
-    /**
-     * Compare two ranges. Either argument may be falsy, but will only return
-     * true if both are falsy or if neither are falsy and have the same position
-     * values.
-     *
-     * @param {Object=} a range 1
-     * @param {Object=} b range 2
-     * @return {boolean}
-     */
-    _rangesEqual(a, b) {
-      if (!a && !b) { return true; }
-      if (!a || !b) { return false; }
-      return a.startLine === b.startLine &&
-          a.startChar === b.startChar &&
-          a.endLine === b.endLine &&
-          a.endChar === b.endChar;
-    },
-  });
-})();

polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group_test.html

@@ -1,165 +0,0 @@
-<!DOCTYPE html>
-<!--
-@license
-Copyright (C) 2017 The Android Open Source Project
-
-Licensed under the Apache License, Version 2.0 (the "License");
-you may not use this file except in compliance with the License.
-You may obtain a copy of the License at
-
-http://www.apache.org/licenses/LICENSE-2.0
-
-Unless required by applicable law or agreed to in writing, software
-distributed under the License is distributed on an "AS IS" BASIS,
-WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-See the License for the specific language governing permissions and
-limitations under the License.
--->
-
-<meta name="viewport" content="width=device-width, minimum-scale=1.0, initial-scale=1.0, user-scalable=yes">
-<title>gr-diff-comment-thread-group</title>
-
-<script src="../../../bower_components/webcomponentsjs/webcomponents-lite.min.js"></script>
-<script src="../../../bower_components/web-component-tester/browser.js"></script>
-<link rel="import" href="../../../test/common-test-setup.html"/>
-<script src="../../../scripts/util.js"></script>
-
-<link rel="import" href="gr-diff-comment-thread-group.html">
-
-<script>void(0);</script>
-
-<test-fixture id="basic">
-  <template>
-    <gr-diff-comment-thread-group></gr-diff-comment-thread-group>
-  </template>
-</test-fixture>
-
-<script>
-  suite('gr-diff-comment-thread-group tests', () => {
-    let element;
-    let sandbox;
-
-    setup(() => {
-      sandbox = sinon.sandbox.create();
-      stub('gr-rest-api-interface', {
-        getLoggedIn() { return Promise.resolve(false); },
-      });
-      element = fixture('basic');
-    });
-
-    teardown(() => {
-      sandbox.restore();
-    });
-
-    test('getThreadEl', () => {
-      const range = {
-        start_line: 1,
-        start_character: 1,
-        end_line: 1,
-        end_character: 2,
-      };
-      const threads = [
-        {
-          rootId: 'sallys_confession',
-          commentSide: 'left',
-          comments: [
-            {
-              id: 'sallys_confession',
-              message: 'i like you, jack',
-              updated: '2015-12-23 15:00:20.396000000',
-              __commentSide: 'left',
-            }, {
-              id: 'jacks_reply',
-              message: 'i like you, too',
-              updated: '2015-12-24 15:01:20.396000000',
-              __commentSide: 'left',
-              in_reply_to: 'sallys_confession',
-            }, {
-              id: 'new_draft',
-              message: 'i do not like either of you',
-              __commentSide: 'left',
-              __draft: true,
-              in_reply_to: 'sallys_confession',
-              updated: '2015-12-20 15:01:20.396000000',
-            },
-          ],
-        },
-        {
-          rootId: 'right_side_comment',
-          commentSide: 'right',
-          comments: [
-            {
-              id: 'right_side_comment',
-              message: 'right side comment',
-              __commentSide: 'right',
-              __draft: true,
-              updated: '2015-12-20 15:01:20.396000000',
-            },
-          ],
-        }, {
-          rootId: 'betsys_confession',
-          commentSide: 'left',
-          range,
-          comments: [
-            {
-              id: 'betsys_confession',
-              message: 'i like you more, jack',
-              updated: '2015-12-24 15:00:10.396000000',
-              range,
-              __commentSide: 'left',
-            },
-          ],
-        },
-      ];
-      for (const thread of threads) {
-        Polymer.dom(element).appendChild(Gerrit.createThreadElement(
-            thread, 1, '2', 'some/path', 'some_project'));
-      }
-
-      flushAsynchronousOperations();
-      assert.deepEqual(
-          element.getThreadEl('right').rootId, 'right_side_comment');
-      assert.deepEqual(element.getThreadEl('right').comments.length, 1);
-      assert.deepEqual(element.getThreadEl('left').rootId, 'sallys_confession');
-      assert.deepEqual(element.getThreadEl('left').comments.length, 3);
-      assert.deepEqual(element.getThreadEl('left', range).rootId,
-          'betsys_confession');
-      assert.deepEqual(element.getThreadEl('left', range).comments.length, 1);
-    });
-
-    test('thread-discard handling', () => {
-      const threads = [
-        {comments: [{id: 4711}]},
-        {comments: [{id: 42}]},
-      ];
-      for (const thread of threads) {
-        const threadEl = Gerrit.createThreadElement(
-            thread, 1, 2, 'some/path', 'Some Project');
-        Polymer.dom(element).appendChild(threadEl);
-      }
-
-      element.threadEls[0].dispatchEvent(
-          new CustomEvent('thread-discard', {detail: {rootId: 4711}}));
-      assert.equal(element.threadEls.length, 1);
-      assert.equal(element.threadEls[0].rootId, 42);
-    });
-
-    test('_rangesEqual', () => {
-      const range1 =
-          {startLine: 123, startChar: 345, endLine: 234, endChar: 456};
-      const range2 =
-          {startLine: 1, startChar: 2, endLine: 3, endChar: 4};
-
-      assert.isTrue(element._rangesEqual(null, null));
-      assert.isTrue(element._rangesEqual(null, undefined));
-      assert.isTrue(element._rangesEqual(undefined, null));
-      assert.isTrue(element._rangesEqual(undefined, undefined));
-
-      assert.isFalse(element._rangesEqual(range1, null));
-      assert.isFalse(element._rangesEqual(null, range1));
-      assert.isFalse(element._rangesEqual(range1, range2));
-
-      assert.isTrue(element._rangesEqual(range1, Object.assign({}, range1)));
-    });
-  });
-</script>

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

@@ -60,7 +60,11 @@ limitations under the License.
 
       diffElement.loggedIn = false;
       diffElement.patchRange = {basePatchNum: 1, patchNum: 2};
-      diffElement.comments = {left: [], right: []};
+      diffElement.comments = {
+        left: [],
+        right: [],
+        meta: {patchRange: undefined},
+      };
       const setupDone = () => {
         cursorElement._updateStops();
         cursorElement.moveToFirstChunk();

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

@@ -19,6 +19,7 @@ limitations under the License.
 <link rel="import" href="../../../behaviors/gr-patch-set-behavior/gr-patch-set-behavior.html">
 <link rel="import" href="../../core/gr-reporting/gr-reporting.html">
 <link rel="import" href="../../shared/gr-rest-api-interface/gr-rest-api-interface.html">
+<link rel="import" href="../gr-diff-comment-thread/gr-diff-comment-thread.html">
 <link rel="import" href="../gr-diff/gr-diff.html">
 
 <dom-module id="gr-diff-host">
@@ -46,8 +47,7 @@ limitations under the License.
         base-image="[[_baseImage]]"
         revision-image=[[_revisionImage]]
         blame="[[_blame]]"
-        diff="[[_diff]]"
+        diff="[[_diff]]"></gr-diff>
-        parent-index="[[_parentIndex]]"></gr-diff>
     <gr-rest-api-interface id="restAPI"></gr-rest-api-interface>
     <gr-reporting id="reporting" category="diff"></gr-reporting>
   </template>

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

@@ -45,13 +45,35 @@
     return !!(diff.binary && (isA || isB));
   }
 
+  /** @typedef {{startLine: number, startChar: number,
+   *             endLine: number, endChar: number}} */
+  Gerrit.Range;
+
+
+  /**
+   * Compare two ranges. Either argument may be falsy, but will only return
+   * true if both are falsy or if neither are falsy and have the same position
+   * values.
+   *
+   * @param {Gerrit.Range=} a range 1
+   * @param {Gerrit.Range=} b range 2
+   * @return {boolean}
+   */
+  function rangesEqual(a, b) {
+    if (!a && !b) { return true; }
+    if (!a || !b) { return false; }
+    return a.startLine === b.startLine &&
+        a.startChar === b.startChar &&
+        a.endLine === b.endLine &&
+        a.endChar === b.endChar;
+  }
+
   /**
    * Wrapper around gr-diff.
    *
    * Webcomponent fetching diffs and related data from restAPI and passing them
    * to the presentational gr-diff for rendering.
    */
-  // TODO(oler): Move all calls to restAPI from gr-diff here.
   Polymer({
     is: 'gr-diff-host',
 
@@ -108,7 +130,10 @@
         type: Boolean,
         value: false,
       },
-      comments: Object,
+      comments: {
+        type: Object,
+        observer: '_commentsChanged',
+      },
       lineWrapping: {
         type: Boolean,
         value: false,
@@ -176,6 +201,11 @@
         type: Number,
         computed: '_computeParentIndex(patchRange.*)',
       },
+
+      _threadEls: {
+        type: Array,
+        value: [],
+      },
     },
 
     behaviors: [
@@ -310,7 +340,7 @@
      * @return {!Array<!HTMLElement>}
      */
     getThreadEls() {
-      return this.$.diff.getThreadEls();
+      return this._threadEls;
     },
 
     /** @param {HTMLElement} el */
@@ -437,6 +467,70 @@
       return isImageDiff(diff);
     },
 
+
+    _commentsChanged(newComments) {
+      const allComments = [];
+      for (const side of [GrDiffBuilder.Side.LEFT, GrDiffBuilder.Side.RIGHT]) {
+        // This is needed by the threading.
+        for (const comment of newComments[side]) {
+          comment.__commentSide = side;
+        }
+        allComments.push(...newComments[side]);
+      }
+      // Currently, the only way this is ever changed here is when the initial
+      // comments are loaded, so it's okay performance wise to clear the threads
+      // and recreate them. If this changes in future, we might want to reuse
+      // some DOM nodes here.
+      this._clearThreads();
+      const threads = this._createThreads(allComments);
+      for (const thread of threads) {
+        const threadEl = this._createThreadElement(thread);
+        this._attachThreadElement(threadEl);
+      }
+    },
+
+    /**
+     * @param {!Array<!Object>} comments
+     * @return {!Array<!Object>} Threads for the given comments.
+     */
+    _createThreads(comments) {
+      const sortedComments = comments.slice(0).sort((a, b) => {
+        if (b.__draft && !a.__draft ) { return 0; }
+        if (a.__draft && !b.__draft ) { return 1; }
+        return util.parseDate(a.updated) - util.parseDate(b.updated);
+      });
+
+      const threads = [];
+      for (const comment of sortedComments) {
+        // If the comment is in reply to another comment, find that comment's
+        // thread and append to it.
+        if (comment.in_reply_to) {
+          const thread = threads.find(thread =>
+              thread.comments.some(c => c.id === comment.in_reply_to));
+          if (thread) {
+            thread.comments.push(comment);
+            continue;
+          }
+        }
+
+        // Otherwise, this comment starts its own thread.
+        const newThread = {
+          start_datetime: comment.updated,
+          comments: [comment],
+          commentSide: comment.__commentSide,
+          patchNum: comment.patch_set,
+          rootId: comment.id || comment.__draftID,
+          lineNum: comment.line,
+          isOnParent: comment.side === 'PARENT',
+        };
+        if (comment.range) {
+          newThread.range = Object.assign({}, comment.range);
+        }
+        threads.push(newThread);
+      }
+      return threads;
+    },
+
     /**
      * @param {Object} blame
      * @return {boolean}
@@ -456,44 +550,117 @@
 
     /** @param {CustomEvent} e */
     _handleCreateComment(e) {
-      const {threadGroupEl, lineNum, side, range, isOnParent,
+      const {lineNum, side, patchNum, isOnParent, range} = e.detail;
-          patchForNewThreads} = e.detail;
+      const threadEl = this._getOrCreateThread(patchNum, lineNum, side, range,
-      const threadEl = this._getOrCreateThread(threadGroupEl, side, range,
+          isOnParent);
-          isOnParent, patchForNewThreads);
       threadEl.addOrEditDraft(lineNum, range);
+
       this.$.reporting.recordDraftInteraction();
     },
 
     /**
-     * Gets or creates a comment thread from a specific thread group.
+     * Gets or creates a comment thread at a given location.
-     * May include a range, if the comment is a range comment.
+     * May provide a range, to get/create a range comment.
      *
-     * @param {!Node} threadGroupEl
+     * @param {string} patchNum
+     * @param {?number} lineNum
      * @param {string} commentSide
-     * @param {?Object} range
+     * @param {Gerrit.Range|undefined} range
      * @param {boolean} isOnParent
-     * @param {string} patchForNewThreads
      * @return {!Object}
      */
-    _getOrCreateThread(threadGroupEl, commentSide, range, isOnParent,
+    _getOrCreateThread(patchNum, lineNum, commentSide, range, isOnParent) {
-        patchForNewThreads) {
+      let threadEl = this._getThreadEl(lineNum, commentSide, range);
-      let threadEl = threadGroupEl.getThreadEl(commentSide, range);
-
       if (!threadEl) {
-        threadEl = Gerrit.createThreadElement(
+        threadEl = this._createThreadElement({
-            {
+          comments: [],
-              comments: [],
+          commentSide,
-              commentSide,
+          patchNum,
-              patchNum: patchForNewThreads,
+          lineNum,
-              range,
+          range,
-              isOnParent,
+          isOnParent,
-            }, this._parentIndex, this.changeNum,
+        });
-            this.path, this.projectName);
+        this._attachThreadElement(threadEl);
-        Polymer.dom(threadGroupEl).appendChild(threadEl);
       }
       return threadEl;
     },
 
+    _attachThreadElement(threadEl) {
+      this._threadEls.push(threadEl);
+      Polymer.dom(this.$.diff).appendChild(threadEl);
+    },
+
+    _clearThreads() {
+      for (const threadEl of this._threadEls) {
+        const parent = Polymer.dom(threadEl).parentNode;
+        Polymer.dom(parent).removeChild(threadEl);
+      }
+      this._threadEls = [];
+    },
+
+    _createThreadElement(thread) {
+      const threadEl = document.createElement('gr-diff-comment-thread');
+      threadEl.className = 'comment-thread';
+      threadEl.comments = thread.comments;
+      threadEl.commentSide = thread.commentSide;
+      threadEl.isOnParent = !!thread.isOnParent;
+      threadEl.parentIndex = this._parentIndex;
+      threadEl.changeNum = this.changeNum;
+      threadEl.patchNum = thread.patchNum;
+      threadEl.lineNum = thread.lineNum;
+      const rootIdChangedListener = changeEvent => {
+        thread.rootId = changeEvent.detail.value;
+      };
+      threadEl.addEventListener('root-id-changed', rootIdChangedListener);
+      threadEl.path = this.path;
+      threadEl.projectName = this.projectName;
+      threadEl.range = thread.range;
+      const threadDiscardListener = e => {
+        const threadEl = /** @type {!Node} */ (e.currentTarget);
+
+        const parent = Polymer.dom(threadEl).parentNode;
+        Polymer.dom(parent).removeChild(threadEl);
+
+        const i = this._threadEls.findIndex(
+            threadEl => threadEl.rootId == e.detail.rootId);
+        this._threadEls.splice(i, 1);
+
+        threadEl.removeEventListener('root-id-changed', rootIdChangedListener);
+        threadEl.removeEventListener('thread-discard', threadDiscardListener);
+      };
+      threadEl.addEventListener('thread-discard', threadDiscardListener);
+      return threadEl;
+    },
+
+    /**
+     * Gets a comment thread element at a given location.
+     * May provide a range, to get a range comment.
+     *
+     * @param {?number} lineNum
+     * @param {string} commentSide
+     * @param {!Gerrit.Range=} range
+     * @return {?Node}
+     */
+    _getThreadEl(lineNum, commentSide, range=undefined) {
+      let line;
+      if (commentSide === GrDiffBuilder.Side.LEFT) {
+        line = {beforeNumber: lineNum};
+      } else if (commentSide === GrDiffBuilder.Side.RIGHT) {
+        line = {afterNumber: lineNum};
+      } else {
+        throw new Error(`Unknown side: ${commentSide}`);
+      }
+      function matchesRange(threadEl) {
+        const threadRange = /** @type {!Gerrit.Range} */(
+            JSON.parse(threadEl.getAttribute('range')));
+        return rangesEqual(threadRange, range);
+      }
+
+      const filteredThreadEls = Gerrit.filterThreadElsForLocation(
+          this._threadEls, line, commentSide).filter(matchesRange);
+      return filteredThreadEls.length ? filteredThreadEls[0] : null;
+    },
+
     /**
      * Take a diff that was loaded with a ignore-whitespace other than
      * IGNORE_NONE, and convert delta chunks labeled as common into shared

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

@@ -46,12 +46,41 @@ limitations under the License.
         async getLoggedIn() { return getLoggedIn; },
       });
       element = fixture('basic');
+      // For reasons beyond me, fixture reuses elements, cleans out some
+      // stuff but not that list.
+      element._threadEls = [];
     });
 
     teardown(() => {
       sandbox.restore();
     });
 
+    test('thread-discard handling', () => {
+      const threads = [
+        {comments: [{id: 4711}]},
+        {comments: [{id: 42}]},
+      ];
+      element._parentIndex = 1;
+      element.changeNum = '2';
+      element.path = 'some/path';
+      element.projectName = 'Some project';
+      const threadEls = threads.map(
+          thread => element._createThreadElement(thread));
+      assert.equal(threadEls.length, 2);
+      assert.equal(threadEls[0].rootId, 4711);
+      assert.equal(threadEls[1].rootId, 42);
+      for (const threadEl of threadEls) {
+        Polymer.dom(element).appendChild(threadEl);
+      }
+
+      threadEls[0].dispatchEvent(
+          new CustomEvent('thread-discard', {detail: {rootId: 4711}}));
+      const attachedThreads = element.queryAllEffectiveChildren(
+          'gr-diff-comment-thread');
+      assert.equal(attachedThreads.length, 1);
+      assert.equal(attachedThreads[0].rootId, 42);
+    });
+
     test('reload() cancels before network resolves', () => {
       const cancelStub = sandbox.stub(element.$.diff, 'cancel');
 
@@ -182,7 +211,11 @@ limitations under the License.
               });
 
           element.patchRange = {basePatchNum: 'PARENT', patchNum: 1};
-          element.comments = {left: [], right: []};
+          element.comments = {
+            left: [],
+            right: [],
+            meta: {patchRange: element.patchRange},
+          };
         });
 
         test('renders image diffs with same file name', done => {
@@ -556,13 +589,10 @@ limitations under the License.
       });
     });
 
-    test('delegates getThreadEls()', () => {
+    test('getThreadEls() returns _threadEls', () => {
       const returnValue = [document.createElement('b')];
-      const stub = sandbox.stub(element.$.diff, 'getThreadEls')
+      element._threadEls = returnValue;
-          .returns(returnValue);
       assert.equal(element.getThreadEls(), returnValue);
-      assert.isTrue(stub.calledOnce);
-      assert.equal(stub.lastCall.args.length, 0);
     });
 
     test('delegates addDraftAtLine(el)', () => {
@@ -771,15 +801,160 @@ limitations under the License.
       });
     });
 
+    test('_createThreads', () => {
+      const comments = [
+        {
+          id: 'sallys_confession',
+          message: 'i like you, jack',
+          updated: '2015-12-23 15:00:20.396000000',
+          line: 1,
+          __commentSide: 'left',
+        }, {
+          id: 'jacks_reply',
+          message: 'i like you, too',
+          updated: '2015-12-24 15:01:20.396000000',
+          __commentSide: 'left',
+          line: 1,
+          in_reply_to: 'sallys_confession',
+        },
+        {
+          id: 'new_draft',
+          message: 'i do not like either of you',
+          __commentSide: 'left',
+          __draft: true,
+          updated: '2015-12-20 15:01:20.396000000',
+        },
+      ];
+
+      const actualThreads = element._createThreads(comments);
+
+      assert.equal(actualThreads.length, 2);
+
+      assert.equal(
+          actualThreads[0].start_datetime, '2015-12-23 15:00:20.396000000');
+      assert.equal(actualThreads[0].commentSide, 'left');
+      assert.equal(actualThreads[0].comments.length, 2);
+      assert.deepEqual(actualThreads[0].comments[0], comments[0]);
+      assert.deepEqual(actualThreads[0].comments[1], comments[1]);
+      assert.equal(actualThreads[0].patchNum, undefined);
+      assert.equal(actualThreads[0].rootId, 'sallys_confession');
+      assert.equal(actualThreads[0].lineNum, 1);
+
+      assert.equal(
+          actualThreads[1].start_datetime, '2015-12-20 15:01:20.396000000');
+      assert.equal(actualThreads[1].commentSide, 'left');
+      assert.equal(actualThreads[1].comments.length, 1);
+      assert.deepEqual(actualThreads[1].comments[0], comments[2]);
+      assert.equal(actualThreads[1].patchNum, undefined);
+      assert.equal(actualThreads[1].rootId, 'new_draft');
+      assert.equal(actualThreads[1].lineNum, undefined);
+    });
+
+    test('_createThreads inherits patchNum and range', () => {
+      const comments = [{
+        id: 'betsys_confession',
+        message: 'i like you, jack',
+        updated: '2015-12-24 15:00:10.396000000',
+        range: {
+          start_line: 1,
+          start_character: 1,
+          end_line: 1,
+          end_character: 2,
+        },
+        patch_set: 5,
+        __commentSide: 'left',
+        line: 1,
+      }];
+
+      expectedThreads = [
+        {
+          start_datetime: '2015-12-24 15:00:10.396000000',
+          commentSide: 'left',
+          comments: [{
+            id: 'betsys_confession',
+            message: 'i like you, jack',
+            updated: '2015-12-24 15:00:10.396000000',
+            range: {
+              start_line: 1,
+              start_character: 1,
+              end_line: 1,
+              end_character: 2,
+            },
+            patch_set: 5,
+            __commentSide: 'left',
+            line: 1,
+          }],
+          patchNum: 5,
+          rootId: 'betsys_confession',
+          range: {
+            start_line: 1,
+            start_character: 1,
+            end_line: 1,
+            end_character: 2,
+          },
+          lineNum: 1,
+          isOnParent: false,
+        },
+      ];
+
+      assert.deepEqual(
+          element._createThreads(comments),
+          expectedThreads);
+    });
+
+    test('_createThreads does not thread unrelated comments at same location',
+        () => {
+          const comments = [
+            {
+              id: 'sallys_confession',
+              message: 'i like you, jack',
+              updated: '2015-12-23 15:00:20.396000000',
+              __commentSide: 'left',
+            }, {
+              id: 'jacks_reply',
+              message: 'i like you, too',
+              updated: '2015-12-24 15:01:20.396000000',
+              __commentSide: 'left',
+            },
+          ];
+          assert.equal(element._createThreads(comments).length, 2);
+        });
+
+    test('_createThreads derives isOnParent using  side from first comment',
+        () => {
+          const comments = [
+            {
+              id: 'sallys_confession',
+              message: 'i like you, jack',
+              updated: '2015-12-23 15:00:20.396000000',
+              // line: 1,
+              // __commentSide: 'left',
+            }, {
+              id: 'jacks_reply',
+              message: 'i like you, too',
+              updated: '2015-12-24 15:01:20.396000000',
+              // __commentSide: 'left',
+              // line: 1,
+              in_reply_to: 'sallys_confession',
+            },
+          ];
+
+          assert.equal(element._createThreads(comments)[0].isOnParent, false);
+
+          comments[0].side = 'REVISION';
+          assert.equal(element._createThreads(comments)[0].isOnParent, false);
+
+          comments[0].side = 'PARENT';
+          assert.equal(element._createThreads(comments)[0].isOnParent, true);
+        });
+
     test('_getOrCreateThread', () => {
-      const threadGroupEl =
-          document.createElement('gr-diff-comment-thread-group');
       const commentSide = 'left';
 
-      assert.isOk(element._getOrCreateThread(threadGroupEl,
+      assert.isOk(element._getOrCreateThread('2', 3,
-          commentSide, undefined, false, '2'));
+          commentSide, undefined, false));
 
-      let threads = Polymer.dom(threadGroupEl)
+      let threads = Polymer.dom(element.$.diff)
           .queryDistributedElements('gr-diff-comment-thread');
 
       assert.equal(threads.length, 1);
@@ -798,9 +973,9 @@ limitations under the License.
       };
 
       assert.isOk(element._getOrCreateThread(
-          threadGroupEl, commentSide, range, true, '3'));
+          '3', 1, commentSide, range, true));
 
-      threads = Polymer.dom(threadGroupEl)
+      threads = Polymer.dom(element.$.diff)
           .queryDistributedElements('gr-diff-comment-thread');
 
       assert.equal(threads.length, 2);

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

@@ -332,7 +332,6 @@ limitations under the License.
         patch-range="[[_patchRange]]"
         path="[[_path]]"
         prefs="[[_prefs]]"
-        project-config="[[_projectConfig]]"
         project-name="[[_change.project]]"
         view-mode="[[_diffMode]]"
         is-blame-loaded="{{_isBlameLoaded}}"

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

@@ -20,7 +20,7 @@ limitations under the License.
 <link rel="import" href="../../../styles/shared-styles.html">
 <link rel="import" href="../../shared/gr-button/gr-button.html">
 <link rel="import" href="../gr-diff-builder/gr-diff-builder.html">
-<link rel="import" href="../gr-diff-comment-thread/gr-diff-comment-thread.html">
+<link rel="import" href="../gr-diff-comment-thread-group/gr-diff-comment-thread-group.html">
 <link rel="import" href="../gr-diff-highlight/gr-diff-highlight.html">
 <link rel="import" href="../gr-diff-selection/gr-diff-selection.html">
 <link rel="import" href="../gr-syntax-themes/gr-syntax-theme.html">
@@ -36,6 +36,11 @@ limitations under the License.
       :host(.no-left) .sideBySide ::content .right:not([data-value]) + td {
         display: none;
       }
+      ::slotted(*) .thread-group {
+        display: block;
+        max-width: var(--content-width, 80ch);
+        white-space: normal;
+      }
       .diffContainer {
         display: flex;
         font-family: var(--monospace-font-family);
@@ -290,9 +295,8 @@ limitations under the License.
               is-image-diff="[[isImageDiff]]"
               base-image="[[baseImage]]"
               revision-image="[[revisionImage]]"
-              parentIndex="[[parentIndex]]"
-              path="[[path]]"
               line-of-interest="[[lineOfInterest]]">
+            <slot></slot>
             <table
                 id="diffTable"
                 class$="[[_diffTableClass]]"

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

@@ -180,6 +180,9 @@
       },
 
       _diffLength: Number,
+
+      /** @type {?PolymerDomApi.ObserveHandle} */
+      _nodeObserver: Object,
     },
 
     behaviors: [
@@ -191,6 +194,11 @@
       'comment-update': '_handleCommentUpdate',
       'comment-save': '_handleCommentSave',
       'create-range-comment': '_handleCreateRangeComment',
+      'render-content': '_handleRenderContent',
+    },
+
+    detached() {
+      this._unobserveNodes();
     },
 
     /** Cancel any remaining diff builder rendering work. */
@@ -230,17 +238,6 @@
           {bubbles: true}));
     },
 
-    /** @return {!Array<!HTMLElement>} */
-    getThreadEls() {
-      let threads = [];
-      const threadGroupEls = Polymer.dom(this.root)
-          .querySelectorAll('gr-diff-comment-thread-group');
-      for (const threadGroupEl of threadGroupEls) {
-        threads = threads.concat(threadGroupEl.threadEls);
-      }
-      return threads;
-    },
-
     /** @return {string} */
     _computeContainerClass(loggedIn, viewMode, displayLine) {
       const classes = ['diffContainer'];
@@ -354,42 +351,40 @@
       const patchForNewThreads = this._getPatchNumByLineAndContent(
           lineEl, contentEl);
       const isOnParent =
-        this._getIsParentCommentByLineAndContent(lineEl, contentEl);
+          this._getIsParentCommentByLineAndContent(lineEl, contentEl);
-      const threadGroupEl = this._getOrCreateThreadGroup(contentEl);
       this.dispatchEvent(new CustomEvent('create-comment', {
         bubbles: true,
         detail: {
-          threadGroupEl,
           lineNum,
           side,
-          range,
+          patchNum: patchForNewThreads,
           isOnParent,
-          patchForNewThreads,
+          range,
         },
       }));
     },
 
     _getThreadGroupForLine(contentEl) {
-      return contentEl.querySelector('gr-diff-comment-thread-group');
+      return contentEl.querySelector('.thread-group');
     },
 
     /**
      * Gets or creates a comment thread group for a specific line and side on a
      * diff.
      * @param {!Object} contentEl
-     * @return {!Object}
+     * @return {!Node}
      */
     _getOrCreateThreadGroup(contentEl) {
       // Check if thread group exists.
       let threadGroupEl = this._getThreadGroupForLine(contentEl);
       if (!threadGroupEl) {
-        threadGroupEl = document.createElement('gr-diff-comment-thread-group');
+        threadGroupEl = document.createElement('div');
+        threadGroupEl.className = 'thread-group';
         contentEl.appendChild(threadGroupEl);
       }
       return threadGroupEl;
     },
 
-
     /**
      * The value to be used for the patch number of new comments created at the
      * given line and content elements.
@@ -578,6 +573,7 @@
     },
 
     _renderDiffTable() {
+      this._unobserveNodes();
       if (!this.prefs) {
         this.dispatchEvent(new CustomEvent('render', {bubbles: true}));
         return;
@@ -594,6 +590,34 @@
       this.$.diffBuilder.render(this.comments, this._getBypassPrefs());
     },
 
+    _handleRenderContent() {
+      this._nodeObserver = Polymer.dom(this).observeNodes(info => {
+        const addedThreadEls = info.addedNodes.filter(
+            node => node.nodeType === Node.ELEMENT_NODE);
+        // 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.
+        for (const threadEl of addedThreadEls) {
+          const lineNum = Number(threadEl.getAttribute('line-num'));
+          const commentSide = threadEl.getAttribute('comment-side');
+          const lineEl = this.$.diffBuilder.getLineElByNumber(
+              lineNum, commentSide);
+          const contentText = this.$.diffBuilder.getContentByLineEl(lineEl);
+          const contentEl = contentText.parentElement;
+          const threadGroupEl = this._getOrCreateThreadGroup(contentEl);
+          Polymer.dom(threadGroupEl).appendChild(threadEl);
+        }
+      });
+    },
+
+    _unobserveNodes() {
+      if (this._nodeObserver) {
+        Polymer.dom(this).unobserveNodes(this._nodeObserver);
+      }
+    },
+
     /**
      * Get the preferences object including the safety bypass context (if any).
      */
@@ -605,6 +629,7 @@
     },
 
     clearDiffContent() {
+      this._unobserveNodes();
       this.$.diffTable.innerHTML = null;
     },
 

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

@@ -315,8 +315,7 @@ limitations under the License.
         // The new thread group can be fetched.
         assert.isOk(element._getThreadGroupForLine(contentEl));
 
-        assert.equal(contentEl.querySelectorAll(
+        assert.equal(contentEl.querySelectorAll('.thread-group').length, 1);
-            'gr-diff-comment-thread-group').length, 1);
       });
 
       suite('image diffs', () => {
@@ -335,7 +334,11 @@ limitations under the License.
           };
 
           element.patchRange = {basePatchNum: 'PARENT', patchNum: 1};
-          element.comments = {left: [], right: []};
+          element.comments = {
+            left: [],
+            right: [],
+            meta: {patchRange: undefined},
+          };
           element.isImageDiff = true;
           element.prefs = {
             auto_hide_diff_table_header: true,
@@ -664,6 +667,7 @@ limitations under the License.
           element.comments = {
             left: [],
             right: [],
+            meta: {patchRange: undefined},
           };
           element.prefs = {
             context: 10,

polygerrit-ui/app/test/index.html

@@ -103,7 +103,6 @@ limitations under the License.
     'core/gr-smart-search/gr-smart-search_test.html',
     'diff/gr-comment-api/gr-comment-api_test.html',
     'diff/gr-diff-builder/gr-diff-builder_test.html',
-    'diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group_test.html',
     'diff/gr-diff-comment-thread/gr-diff-comment-thread_test.html',
     'diff/gr-diff-comment/gr-diff-comment_test.html',
     'diff/gr-diff-cursor/gr-diff-cursor_test.html',