Merge "Update gr-comment-api"

2017-11-07 21:23:46 +00:00
parent 5295b645e6 c7f07dca19
commit ed7ebd7a52
12 changed files with 514 additions and 327 deletions
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html
@@ -21,6 +21,7 @@ limitations under the License.
 <link rel="import" href="../../../behaviors/rest-client-behavior/rest-client-behavior.html">
 <link rel="import" href="../../../styles/shared-styles.html">
 <link rel="import" href="../../core/gr-navigation/gr-navigation.html">
+<link rel="import" href="../../diff/gr-comment-api/gr-comment-api.html">
 <link rel="import" href="../../diff/gr-diff-preferences/gr-diff-preferences.html">
 <link rel="import" href="../../edit/gr-edit-constants.html">
 <link rel="import" href="../../plugins/gr-endpoint-decorator/gr-endpoint-decorator.html">
@@ -431,7 +432,7 @@ limitations under the License.
            all-patch-sets="[[_allPatchSets]]"
            change="[[_change]]"
            change-num="[[_changeNum]]"
-            comments="[[_comments]]"
+            comments="[[_changeComments.comments]]"
            commit-info="[[_commitInfo]]"
            change-url="[[_computeChangeUrl(_change)]]"
            edit-loaded="[[_editLoaded]]"
@@ -453,7 +454,7 @@ limitations under the License.
            change="[[_change]]"
            change-num="[[_changeNum]]"
            patch-range="{{_patchRange}}"
-            comments="[[_comments]]"
+            change-comments="[[_changeComments]]"
            drafts="[[_diffDrafts]]"
            revisions="[[_change.revisions]]"
            project-config="[[_projectConfig]]"
@@ -464,7 +465,8 @@ limitations under the License.
            files-expanded="{{_filesExpanded}}"
            file-list-increment="{{_numFilesShown}}"
            on-files-shown-changed="_setShownFiles"
-            on-file-action-tap="_handleFileActionTap"></gr-file-list>
+            on-file-action-tap="_handleFileActionTap"
+            on-reload-comments="_reloadCommentsWithCallback"></gr-file-list>
      </section>
      <gr-endpoint-decorator name="change-view-integration">
      </gr-endpoint-decorator>
@@ -473,7 +475,7 @@ limitations under the License.
          change-num="[[_changeNum]]"
          messages="[[_change.messages]]"
          reviewer-updates="[[_change.reviewer_updates]]"
-          comments="[[_comments]]"
+          comments="[[_changeComments.comments]]"
          project-name="[[_change.project]]"
          show-reply-buttons="[[_loggedIn]]"
          on-reply="_handleMessageReply"></gr-messages-list>
@@ -508,6 +510,7 @@ limitations under the License.
    </gr-overlay>
    <gr-js-api-interface id="jsAPI"></gr-js-api-interface>
    <gr-rest-api-interface id="restAPI"></gr-rest-api-interface>
+    <gr-comment-api id="commentAPI"></gr-comment-api>
  </template>
  <script src="gr-change-view.js"></script>
 </dom-module>
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
@@ -98,6 +98,8 @@
        type: Object,
        value: {},
      },
+      /** @type {?} */
+      _changeComments: Object,
      _canStartReview: {
        type: Boolean,
        computed: '_computeCanStartReview(_loggedIn, _change, _account)',
@@ -1002,12 +1004,6 @@
          });
    },

-    _getComments() {
-      return this.$.restAPI.getDiffComments(this._changeNum).then(comments => {
-        this._comments = comments;
-      });
-    },
-
    _getEdit() {
      return this.$.restAPI.getChangeEdit(this._changeNum, true);
    },
@@ -1047,30 +1043,28 @@
          });
    },

-    _reloadDiffDrafts() {
-      this._diffDrafts = {};
-      this._getDiffDrafts().then(() => {
-        if (this.$.replyOverlay.opened) {
-          this.async(() => { this.$.replyOverlay.center(); }, 1);
-        }
+    _reloadCommentsWithCallback(e) {
+      return this._reloadComments().then(() => {
+        return e.detail.resolve();
      });
    },

+    _reloadComments() {
+      return this.$.commentAPI.loadAll(this._changeNum)
+          .then(comments => {
+            this._changeComments = this.$.commentAPI._changeComments;
+          });
+    },
+
    _reload() {
      this._loading = true;
      this._relatedChangesCollapsed = true;

-      this._getLoggedIn().then(loggedIn => {
-        if (!loggedIn) { return; }
-
-        this._reloadDiffDrafts();
-      });
-
      const detailCompletes = this._getChangeDetail().then(() => {
        this._loading = false;
        this._getProjectConfig();
      });
-      this._getComments();
+      this._reloadComments();

      if (this._patchRange.patchNum) {
        return Promise.all([
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html
@@ -241,6 +241,15 @@ limitations under the License.
      });
    });

+    test('comments are reloaded when reload-comments fired', () => {
+      const reloadStub = sandbox.stub(element.$.commentAPI, 'loadAll')
+          .returns(Promise.resolve());
+      element.$.fileList.fire('reload-comments', {
+        resolve: () => { return Promise.resolve(); },
+      });
+      assert.isTrue(reloadStub.called);
+    });
+
    test('download tap calls _handleOpenDownloadDialog', () => {
      sandbox.stub(element, '_handleOpenDownloadDialog');
      element.$.actions.fire('download-tap');
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html
@@ -21,7 +21,6 @@ limitations under the License.
 <link rel="import" href="../../../styles/shared-styles.html">
 <link rel="import" href="../../core/gr-navigation/gr-navigation.html">
 <link rel="import" href="../../core/gr-reporting/gr-reporting.html">
-<link rel="import" href="../../diff/gr-comment-api/gr-comment-api.html">
 <link rel="import" href="../../diff/gr-diff/gr-diff.html">
 <link rel="import" href="../../diff/gr-diff-cursor/gr-diff-cursor.html">
 <link rel="import" href="../../edit/gr-edit-file-controls/gr-edit-file-controls.html">
@@ -267,17 +266,17 @@ limitations under the License.
          </span>
          <div class="comments desktop">
            <span class="drafts">
-              [[_computeDraftsString(drafts, patchRange.patchNum, file.__path)]]
+              [[_computeDraftsString(changeComments.drafts, patchRange.patchNum, file.__path)]]
            </span>
-            [[_computeCommentsString(comments, patchRange.patchNum, file.__path)]]
-            [[_computeUnresolvedString(comments, drafts, patchRange.patchNum, file.__path)]]
+            [[_computeCommentsString(changeComments.comments, patchRange.patchNum, file.__path)]]
+            [[_computeUnresolvedString(changeComments.comments, changeComments.drafts, patchRange.patchNum, file.__path)]]
          </div>
          <div class="comments mobile">
            <span class="drafts">
-              [[_computeDraftsStringMobile(drafts, patchRange.patchNum,
+              [[_computeDraftsStringMobile(changeComments.drafts, patchRange.patchNum,
                  file.__path)]]
            </span>
-            [[_computeCommentsStringMobile(comments, patchRange.patchNum,
+            [[_computeCommentsStringMobile(changeComments.comments, patchRange.patchNum,
                file.__path)]]
          </div>
          <div class$="[[_computeClass('stats', file.__path)]]">
@@ -402,7 +401,6 @@ limitations under the License.
        focus-on-move
        cursor-target-class="selected"></gr-cursor-manager>
    <gr-reporting id="reporting"></gr-reporting>
-    <gr-comment-api id="commentAPI"></gr-comment-api>
  </template>
  <script src="gr-file-list.js"></script>
 </dom-module>
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js
@@ -32,12 +32,19 @@
  Polymer({
    is: 'gr-file-list',

+    /**
+     * Fired when a comment refresh should get triggered
+     *
+     * @event reload-comments
+     */
+
    properties: {
      /** @type {?} */
      patchRange: Object,
      patchNum: String,
      changeNum: String,
-      comments: Object,
+      /** @type {?} */
+      changeComments: Object,
      drafts: Object,
      revisions: Array,
      projectConfig: Object,
@@ -191,9 +198,6 @@
        });
      }));

-      // Load all comments for the change.
-      promises.push(this.$.commentAPI.loadAll(this.changeNum));
-
      this._localPrefs = this.$.storage.getPreferences();
      promises.push(this._getDiffPreferences().then(prefs => {
        this.diffPrefs = prefs;
@@ -270,11 +274,7 @@
      const timerName = 'Update ' + this._expandedFilePaths.length +
          ' diffs with new prefs';
      this._renderInOrder(this._expandedFilePaths, this.diffs,
-          this._expandedFilePaths.length)
-          .then(() => {
-            this.$.reporting.timeEnd(timerName);
-            this.$.diffCursor.handleDiffUpdate();
-          });
+          this._expandedFilePaths.length, timerName);
    },

    _forEachDiff(fn) {
@@ -855,11 +855,7 @@
      // Required so that the newly created diff view is included in this.diffs.
      Polymer.dom.flush();

-      this._renderInOrder(newPaths, this.diffs, newPaths.length)
-          .then(() => {
-            this.$.reporting.timeEnd(timerName);
-            this.$.diffCursor.handleDiffUpdate();
-          });
+      this._renderInOrder(newPaths, this.diffs, newPaths.length, timerName);
      this._updateDiffCursor();
      this.$.diffCursor.handleDiffUpdate();
    },
@@ -872,30 +868,35 @@
     * @param  {!NodeList<!Object>} diffElements (GrDiffElement)
     * @param  {number} initialCount The total number of paths in the pass. This
     *   is used to generate log messages.
+     * @param {string} timerName the timer to stop after the render has
+     *   completed
     * @return {!Promise}
     */
-    _renderInOrder(paths, diffElements, initialCount) {
+    _renderInOrder(paths, diffElements, initialCount, timerName) {
      let iter = 0;

-      return this.$.commentAPI.loadAll(this.changeNum)
-          .then(() => {
-            return this.asyncForeach(paths, path => {
-              iter++;
-              console.log('Expanding diff', iter, 'of', initialCount, ':',
-                  path);
-              const diffElem = this._findDiffByPath(path, diffElements);
-              diffElem.comments = this.$.commentAPI.getCommentsForPath(path,
-                  this.patchRange, this.projectConfig);
-              const promises = [diffElem.reload()];
-              if (this._isLoggedIn) {
-                promises.push(this._reviewFile(path));
-              }
-              return Promise.all(promises);
-            });
-          })
-          .then(() => {
-            console.log('Finished expanding', initialCount, 'diff(s)');
-          });
+      return (new Promise(resolve => {
+        this.fire('reload-comments', {resolve});
+      })).then(() => {
+        return this.asyncForeach(paths, path => {
+          iter++;
+          console.log('Expanding diff', iter, 'of', initialCount, ':',
+              path);
+          const diffElem = this._findDiffByPath(path, diffElements);
+          diffElem.comments = this.changeComments.getCommentsForPath(path,
+              this.patchRange, this.projectConfig);
+          const promises = [diffElem.reload()];
+          if (this._isLoggedIn) {
+            promises.push(this._reviewFile(path));
+          }
+          return Promise.all(promises);
+        }).then(() => {
+          this._nextRenderParams = null;
+          console.log('Finished expanding', initialCount, 'diff(s)');
+          this.$.reporting.timeEnd(timerName);
+          this.$.diffCursor.handleDiffUpdate();
+        });
+      });
    },

    /**
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html
@@ -22,6 +22,7 @@ limitations under the License.
 <script src="../../../bower_components/web-component-tester/browser.js"></script>
 <link rel="import" href="../../../test/common-test-setup.html"/>
 <script src="../../../bower_components/page/page.js"></script>
+<link rel="import" href="../../diff/gr-comment-api/gr-comment-api.html">
 <script src="../../../scripts/util.js"></script>

 <link rel="import" href="../../shared/gr-rest-api-interface/mock-diff-response_test.html">
@@ -29,25 +30,39 @@ limitations under the License.

 <script>void(0);</script>

+<dom-module id="comment-api-mock">
+  <template>
+    <gr-file-list id="fileList"
+        change-comments="[[_changeComments]]"
+        on-reload-comments="_reloadCommentsWithCallback"></gr-file-list>
+    <gr-comment-api id="commentAPI"></gr-comment-api>
+  </template>
+  <script src="../../diff/gr-comment-api/gr-comment-api-mock.js"></script>
+</dom-module>
+
 <test-fixture id="basic">
  <template>
-    <gr-file-list></gr-file-list>
+    <comment-api-mock></comment-api-mock>
  </template>
 </test-fixture>

 <script>
  suite('gr-file-list tests', () => {
    let element;
+    let commentApiWrapper;
    let sandbox;
    let saveStub;
-    let loadCommentStub;
+    let loadCommentSpy;

-    setup(() => {
+    setup(done => {
      sandbox = sinon.sandbox.create();
      stub('gr-rest-api-interface', {
        getLoggedIn() { return Promise.resolve(true); },
        getPreferences() { return Promise.resolve({}); },
        fetchJSON() { return Promise.resolve({}); },
+        getDiffComments() { return Promise.resolve({}); },
+        getDiffRobotComments() { return Promise.resolve({}); },
+        getDiffDrafts() { return Promise.resolve({}); },
      });
      stub('gr-date-formatter', {
        _loadTimeFormat() { return Promise.resolve(''); },
@@ -55,17 +70,25 @@ limitations under the License.
      stub('gr-diff', {
        reload() { return Promise.resolve(); },
      });
-      stub('gr-comment-api', {
-        getPaths() { return {}; },
-        getCommentsForPath() { return {meta: {}, left: [], right: []}; },
+
+      // Element must be wrapped in an element with direct access to the
+      // comment API.
+      commentApiWrapper = fixture('basic');
+      element = commentApiWrapper.$.fileList;
+      loadCommentSpy = sandbox.spy(commentApiWrapper.$.commentAPI, 'loadAll');
+
+      // Stub methods on the changeComments object after changeComments has
+      // been initalized.
+      commentApiWrapper.loadComments().then(() => {
+        sandbox.stub(element.changeComments, 'getPaths').returns({});
+        sandbox.stub(element.changeComments, 'getCommentsForPath')
+            .returns({meta: {}, left: [], right: []});
+        done();
      });

-      element = fixture('basic');
      element.numFilesShown = 200;
      saveStub = sandbox.stub(element, '_saveReviewedState',
          () => { return Promise.resolve(); });
-      loadCommentStub = sandbox.stub(element.$.commentAPI, 'loadAll',
-          () => { return Promise.resolve(); });
    });

    teardown(() => {
@@ -917,7 +940,7 @@ limitations under the License.
      element._renderInOrder(['p2', 'p1', 'p0'], diffs, 3)
          .then(() => {
            assert.isFalse(reviewStub.called);
-            assert.isTrue(loadCommentStub.called);
+            assert.isTrue(loadCommentSpy.called);
            done();
          });
    });
@@ -1040,11 +1063,14 @@ limitations under the License.
      return diffs;
    };

-    setup(() => {
+    setup(done => {
      sandbox = sinon.sandbox.create();
      stub('gr-rest-api-interface', {
        getLoggedIn() { return Promise.resolve(true); },
        getPreferences() { return Promise.resolve({}); },
+        getDiffComments() { return Promise.resolve({}); },
+        getDiffRobotComments() { return Promise.resolve({}); },
+        getDiffDrafts() { return Promise.resolve({}); },
      });
      stub('gr-date-formatter', {
        _loadTimeFormat() { return Promise.resolve(''); },
@@ -1052,12 +1078,21 @@ limitations under the License.
      stub('gr-diff', {
        reload() { return Promise.resolve(); },
      });
-      stub('gr-comment-api', {
-        loadAll() { return Promise.resolve(); },
-        getPaths() { return {}; },
-        getCommentsForPath() { return {meta: {}, left: [], right: []}; },
+
+      // Element must be wrapped in an element with direct access to the
+      // comment API.
+      commentApiWrapper = fixture('basic');
+      element = commentApiWrapper.$.fileList;
+      loadCommentSpy = sandbox.spy(commentApiWrapper.$.commentAPI, 'loadAll');
+
+      // Stub methods on the changeComments object after changeComments has
+      // been initalized.
+      commentApiWrapper.loadComments().then(() => {
+        sandbox.stub(element.changeComments, 'getPaths').returns({});
+        sandbox.stub(element.changeComments, 'getCommentsForPath')
+            .returns({meta: {}, left: [], right: []});
+        done();
      });
-      element = fixture('basic');
      element.numFilesShown = 75;
      element.selectedIndex = 0;
      element._files = [
--- a/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api-mock.js
+++ b/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api-mock.js
@@ -0,0 +1,41 @@
+// 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';
+
+  Polymer({
+    is: 'comment-api-mock',
+
+    properties: {
+      _changeComments: Object,
+    },
+
+    loadComments() {
+      return this._reloadComments();
+    },
+
+    _reloadCommentsWithCallback(e) {
+      return this._reloadComments().then(() => {
+        return e.detail.resolve();
+      });
+    },
+
+    _reloadComments() {
+      return this.$.commentAPI.loadAll(this._changeNum)
+          .then(comments => {
+            this._changeComments = this.$.commentAPI._changeComments;
+          });
+    },
+  });
+})();
--- a/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api.js
+++ b/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api.js
@@ -16,18 +16,191 @@

  const PARENT = 'PARENT';

+  /**
+   * Construct a change comments object, which can be data-bound to child
+   * elements of that which uses the gr-comment-api.
+   *
+   * @param {!Object} comments
+   * @param {!Object} robotComments
+   * @param {!Object} drafts
+   * @param {number} changeNum
+   * @constructor
+   */
+  function ChangeComments(comments, robotComments, drafts, changeNum) {
+    this._comments = comments;
+    this._robotComments = robotComments;
+    this._drafts = drafts;
+    this._changeNum = changeNum;
+  }
+
+  ChangeComments.prototype = {
+    get comments() {
+      return this._comments;
+    },
+    get drafts() {
+      return this._drafts;
+    },
+    get robotComments() {
+      return this._robotComments;
+    },
+  };
+
+  ChangeComments.prototype._patchNumEquals =
+      Gerrit.PatchSetBehavior.patchNumEquals;
+
+  /**
+   * Get an object mapping file paths to a boolean representing whether that
+   * path contains diff comments in the given patch set (including drafts and
+   * robot comments).
+   *
+   * Paths with comments are mapped to true, whereas paths without comments
+   * are not mapped.
+   *
+   * @param {Object=} opt_patchRange The patch-range object containing patchNum
+   *     and basePatchNum properties to represent the range.
+   * @return {!Object}
+   */
+  ChangeComments.prototype.getPaths = function(opt_patchRange) {
+    const responses = [this.comments, this.drafts, this.robotComments];
+    const commentMap = {};
+    for (const response of responses) {
+      for (const path in response) {
+        if (response.hasOwnProperty(path) &&
+            response[path].some(c => {
+              // If don't care about patch range, we know that the path exists.
+              if (!opt_patchRange) { return true; }
+              return this._isInPatchRange(c, opt_patchRange);
+            })) {
+          commentMap[path] = true;
+        }
+      }
+    }
+    return commentMap;
+  };
+
+ /**
+  * Gets all the comments and robot comments for the given change.
+  *
+  * @return {!Object}
+  */
+  ChangeComments.prototype.getAllPublishedComments = function() {
+    const paths = this.getPaths();
+    const publishedComments = {};
+    for (const path of Object.keys(paths)) {
+      publishedComments[path] = this.getAllCommentsForPath(path);
+    }
+    return publishedComments;
+  };
+
+ /**
+  * Get the comments (with drafts and robot comments) for a path and
+  * patch-range. Returns an object with left and right properties mapping to
+  * arrays of comments in on either side of the patch range for that path.
+  *
+  * @param {!string} path
+  * @return {!Object}
+  */
+  ChangeComments.prototype.getAllCommentsForPath = function(path) {
+    const comments = this._comments[path] || [];
+    const robotComments = this._robotComments[path] || [];
+    return comments.concat(robotComments);
+  };
+
+  /**
+   * Get the comments (with drafts and robot comments) for a path and
+   * patch-range. Returns an object with left and right properties mapping to
+   * arrays of comments in on either side of the patch range for that path.
+   *
+   * @param {!string} path
+   * @param {!Object} patchRange The patch-range object containing patchNum
+   *     and basePatchNum properties to represent the range.
+   * @param {Object=} opt_projectConfig Optional project config object to
+   *     include in the meta sub-object.
+   * @return {!Object}
+   */
+  ChangeComments.prototype.getCommentsForPath = function(path, patchRange,
+      opt_projectConfig) {
+    const comments = this._comments[path] || [];
+    const drafts = this._drafts[path] || [];
+    const robotComments = this._robotComments[path] || [];
+
+    drafts.forEach(d => { d.__draft = true; });
+
+    const all = comments.concat(drafts).concat(robotComments);
+
+    const baseComments = all.filter(c =>
+        this._isInBaseOfPatchRange(c, patchRange));
+    const revisionComments = all.filter(c =>
+        this._isInRevisionOfPatchRange(c, patchRange));
+
+    return {
+      meta: {
+        changeNum: this._changeNum,
+        path,
+        patchRange,
+        projectConfig: opt_projectConfig,
+      },
+      left: baseComments,
+      right: revisionComments,
+    };
+  };
+
+  /**
+  * Whether the given comment should be included in the base side of the
+  * given patch range.
+  * @param {!Object} comment
+  * @param {!Object} range
+  * @return {boolean}
+  */
+  ChangeComments.prototype._isInBaseOfPatchRange = function(comment, range) {
+    // If the base of the range is the parent of the patch:
+    if (range.basePatchNum === PARENT &&
+        comment.side === PARENT &&
+        this._patchNumEquals(comment.patch_set, range.patchNum)) {
+      return true;
+    }
+    // If the base of the range is not the parent of the patch:
+    if (range.basePatchNum !== PARENT &&
+        comment.side !== PARENT &&
+        this._patchNumEquals(comment.patch_set, range.basePatchNum)) {
+      return true;
+    }
+    return false;
+  };
+
+  /**
+   * Whether the given comment should be included in the revision side of the
+   * given patch range.
+   * @param {!Object} comment
+   * @param {!Object} range
+   * @return {boolean}
+   */
+  ChangeComments.prototype._isInRevisionOfPatchRange =
+      function(comment, range) {
+        return comment.side !== PARENT &&
+            this._patchNumEquals(comment.patch_set, range.patchNum);
+      };
+
+  /**
+   * Whether the given comment should be included in the given patch range.
+   * @param {!Object} comment
+   * @param {!Object} range
+   * @return {boolean|undefined}
+   */
+  ChangeComments.prototype._isInPatchRange = function(comment, range) {
+    return this._isInBaseOfPatchRange(comment, range) ||
+        this._isInRevisionOfPatchRange(comment, range);
+  };
+
  Polymer({
    is: 'gr-comment-api',

    properties: {
-      /** @type {number} */
-      _changeNum: Number,
-      /** @type {!Object|undefined} */
-      _comments: Object,
-      /** @type {!Object|undefined} */
-      _drafts: Object,
-      /** @type {!Object|undefined} */
-      _robotComments: Object,
+      _changeComments: Object,
+    },
+
+    listeners: {
+      'reload-comments': 'loadAll',
    },

    behaviors: [
@@ -43,132 +216,15 @@
     * @return {!Promise}
     */
    loadAll(changeNum) {
-      this._changeNum = changeNum;
-
-      // Reset comment arrays.
-      this._comments = undefined;
-      this._drafts = undefined;
-      this._robotComments = undefined;
-
      const promises = [];
-      promises.push(this.$.restAPI.getDiffComments(changeNum)
-          .then(comments => { this._comments = comments; }));
-      promises.push(this.$.restAPI.getDiffRobotComments(changeNum)
-          .then(robotComments => { this._robotComments = robotComments; }));
-      promises.push(this.$.restAPI.getDiffDrafts(changeNum)
-          .then(drafts => { this._drafts = drafts; }));
+      promises.push(this.$.restAPI.getDiffComments(changeNum));
+      promises.push(this.$.restAPI.getDiffRobotComments(changeNum));
+      promises.push(this.$.restAPI.getDiffDrafts(changeNum));

-      return Promise.all(promises);
-    },
-
-    /**
-     * Get an object mapping file paths to a boolean representing whether that
-     * path contains diff comments in the given patch set (including drafts and
-     * robot comments).
-     *
-     * Paths with comments are mapped to true, whereas paths without comments
-     * are not mapped.
-     *
-     * @param {!Object} patchRange The patch-range object containing patchNum
-     *     and basePatchNum properties to represent the range.
-     * @return {Object}
-     */
-    getPaths(patchRange) {
-      const responses = [this._comments, this._drafts, this._robotComments];
-      const commentMap = {};
-      for (const response of responses) {
-        for (const path in response) {
-          if (response.hasOwnProperty(path) &&
-              response[path].some(c => this._isInPatchRange(c, patchRange))) {
-            commentMap[path] = true;
-          }
-        }
-      }
-      return commentMap;
-    },
-
-    /**
-     * Get the comments (with drafts and robot comments) for a path and
-     * patch-range. Returns an object with left and right properties mapping to
-     * arrays of comments in on either side of the patch range for that path.
-     *
-     * @param {!string} path
-     * @param {!Object} patchRange The patch-range object containing patchNum
-     *     and basePatchNum properties to represent the range.
-     * @param {Object=} opt_projectConfig Optional project config object to
-     *     include in the meta sub-object.
-     * @return {Object}
-     */
-    getCommentsForPath(path, patchRange, opt_projectConfig) {
-      const comments = this._comments[path] || [];
-      const drafts = this._drafts[path] || [];
-      const robotComments = this._robotComments[path] || [];
-
-      drafts.forEach(d => { d.__draft = true; });
-
-      const all = comments.concat(drafts).concat(robotComments);
-
-      const baseComments = all.filter(c =>
-          this._isInBaseOfPatchRange(c, patchRange));
-      const revisionComments = all.filter(c =>
-          this._isInRevisionOfPatchRange(c, patchRange));
-
-      return {
-        meta: {
-          changeNum: this._changeNum,
-          path,
-          patchRange,
-          projectConfig: opt_projectConfig,
-        },
-        left: baseComments,
-        right: revisionComments,
-      };
-    },
-
-    /**
-     * Whether the given comment should be included in the base side of the
-     * given patch range.
-     * @param {!Object} comment
-     * @param {!Object} range
-     * @return {boolean}
-     */
-    _isInBaseOfPatchRange(comment, range) {
-      // If the base of the range is the parent of the patch:
-      if (range.basePatchNum === PARENT &&
-          comment.side === PARENT &&
-          this.patchNumEquals(comment.patch_set, range.patchNum)) {
-        return true;
-      }
-      // If the base of the range is not the parent of the patch:
-      if (range.basePatchNum !== PARENT &&
-          comment.side !== PARENT &&
-          this.patchNumEquals(comment.patch_set, range.basePatchNum)) {
-        return true;
-      }
-      return false;
-    },
-
-    /**
-     * Whether the given comment should be included in the revision side of the
-     * given patch range.
-     * @param {!Object} comment
-     * @param {!Object} range
-     * @return {boolean}
-     */
-    _isInRevisionOfPatchRange(comment, range) {
-      return comment.side !== PARENT &&
-          this.patchNumEquals(comment.patch_set, range.patchNum);
-    },
-
-    /**
-     * Whether the given comment should be included in the given patch range.
-     * @param {!Object} comment
-     * @param {!Object} range
-     * @return {boolean|undefined}
-     */
-    _isInPatchRange(comment, range) {
-      return this._isInBaseOfPatchRange(comment, range) ||
-          this._isInRevisionOfPatchRange(comment, range);
+      return Promise.all(promises).then(([comments, robotComments, drafts]) => {
+        this._changeComments = new ChangeComments(comments,
+          robotComments, drafts, changeNum);
+      });
    },
  });
 })();
--- a/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api_test.html
@@ -67,9 +67,9 @@ limitations under the License.
            changeNum));
        assert.isTrue(element.$.restAPI.getDiffDrafts.calledWithExactly(
            changeNum));
-        assert.isOk(element._comments);
-        assert.isOk(element._robotComments);
-        assert.deepEqual(element._drafts, {});
+        assert.isOk(element._changeComments._comments);
+        assert.isOk(element._changeComments._robotComments);
+        assert.deepEqual(element._changeComments._drafts, {});
      });
    });

@@ -94,102 +94,144 @@ limitations under the License.
            changeNum));
        assert.isTrue(element.$.restAPI.getDiffDrafts.calledWithExactly(
            changeNum));
-        assert.isOk(element._comments);
-        assert.isOk(element._robotComments);
-        assert.notDeepEqual(element._drafts, {});
+        assert.isOk(element._changeComments._comments);
+        assert.isOk(element._changeComments._robotComments);
+        assert.notDeepEqual(element._changeComments._drafts, {});
      });
    });

-    test('_isInBaseOfPatchRange', () => {
-      const comment = {patch_set: 1};
-      const patchRange = {basePatchNum: 1, patchNum: 2};
-      assert.isTrue(element._isInBaseOfPatchRange(comment, patchRange));
-
-      patchRange.basePatchNum = PARENT;
-      assert.isFalse(element._isInBaseOfPatchRange(comment, patchRange));
-
-      comment.side = PARENT;
-      assert.isFalse(element._isInBaseOfPatchRange(comment, patchRange));
-
-      comment.patch_set = 2;
-      assert.isTrue(element._isInBaseOfPatchRange(comment, patchRange));
-    });
-
-    test('_isInRevisionOfPatchRange', () => {
-      const comment = {patch_set: 123};
-      const patchRange = {basePatchNum: 122, patchNum: 124};
-      assert.isFalse(element._isInRevisionOfPatchRange(comment, patchRange));
-
-      patchRange.patchNum = 123;
-      assert.isTrue(element._isInRevisionOfPatchRange(comment, patchRange));
-
-      comment.side = PARENT;
-      assert.isFalse(element._isInRevisionOfPatchRange(comment, patchRange));
-    });
-
-    suite('comment ranges and paths', () => {
-      setup(() => {
-        element._changeNum = 1234;
-        element._drafts = {};
-        element._robotComments = {};
-        element._comments = {
-          'file/one': [
-            {patch_set: 2, side: PARENT},
-            {patch_set: 2},
-          ],
-          'file/two': [
-            {patch_set: 2},
-            {patch_set: 3},
-          ],
-          'file/three': [
-            {patch_set: 2, side: PARENT},
-            {patch_set: 3},
-          ],
-        };
+    suite('_changeComment methods', () => {
+      setup(done => {
+        const changeNum = 1234;
+        element.loadAll(changeNum).then(() => {
+          done();
+        });
      });

-      test('getPaths', () => {
-        const patchRange = {basePatchNum: 1, patchNum: 4};
-        let paths = element.getPaths(patchRange);
-        assert.equal(Object.keys(paths).length, 0);
+      test('_isInBaseOfPatchRange', () => {
+        const comment = {patch_set: 1};
+        const patchRange = {basePatchNum: 1, patchNum: 2};
+        assert.isTrue(element._changeComments._isInBaseOfPatchRange(comment,
+            patchRange));

        patchRange.basePatchNum = PARENT;
-        patchRange.patchNum = 3;
-        paths = element.getPaths(patchRange);
-        assert.notProperty(paths, 'file/one');
-        assert.property(paths, 'file/two');
-        assert.property(paths, 'file/three');
+        assert.isFalse(element._changeComments._isInBaseOfPatchRange(comment,
+            patchRange));

-        patchRange.patchNum = 2;
-        paths = element.getPaths(patchRange);
-        assert.property(paths, 'file/one');
-        assert.property(paths, 'file/two');
-        assert.property(paths, 'file/three');
+        comment.side = PARENT;
+        assert.isFalse(element._changeComments._isInBaseOfPatchRange(comment,
+            patchRange));
+
+        comment.patch_set = 2;
+        assert.isTrue(element._changeComments._isInBaseOfPatchRange(comment,
+            patchRange));
      });

-      test('getCommentsForPath', () => {
-        const patchRange = {basePatchNum: 1, patchNum: 3};
-        let path = 'file/one';
-        let comments = element.getCommentsForPath(path, patchRange);
-        assert.equal(comments.meta.changeNum, 1234);
-        assert.equal(comments.left.length, 0);
-        assert.equal(comments.right.length, 0);
+      test('_isInRevisionOfPatchRange', () => {
+        const comment = {patch_set: 123};
+        const patchRange = {basePatchNum: 122, patchNum: 124};
+        assert.isFalse(element._changeComments._isInRevisionOfPatchRange(
+            comment, patchRange));

-        path = 'file/two';
-        comments = element.getCommentsForPath(path, patchRange);
-        assert.equal(comments.left.length, 0);
-        assert.equal(comments.right.length, 1);
+        patchRange.patchNum = 123;
+        assert.isTrue(element._changeComments._isInRevisionOfPatchRange(
+            comment, patchRange));

-        patchRange.basePatchNum = 2;
-        comments = element.getCommentsForPath(path, patchRange);
-        assert.equal(comments.left.length, 1);
-        assert.equal(comments.right.length, 1);
+        comment.side = PARENT;
+        assert.isFalse(element._changeComments._isInRevisionOfPatchRange(
+            comment, patchRange));
+      });

-        patchRange.basePatchNum = PARENT;
-        path = 'file/three';
-        comments = element.getCommentsForPath(path, patchRange);
-        assert.equal(comments.left.length, 0);
-        assert.equal(comments.right.length, 1);
+      suite('comment ranges and paths', () => {
+        setup(() => {
+          element._changeComments._drafts = {};
+          element._changeComments._robotComments = {
+            'file/one': [
+              {patch_set: 2, side: PARENT},
+              {patch_set: 2},
+            ],
+          };
+          element._changeComments._comments = {
+            'file/one': [
+              {patch_set: 2, side: PARENT},
+              {patch_set: 2},
+            ],
+            'file/two': [
+              {patch_set: 2},
+              {patch_set: 3},
+            ],
+            'file/three': [
+              {patch_set: 2, side: PARENT},
+              {patch_set: 3},
+            ],
+            'file/four': [
+              {patch_set: 5, side: PARENT},
+              {patch_set: 5},
+            ],
+          };
+        });
+
+        test('getPaths', () => {
+          const patchRange = {basePatchNum: 1, patchNum: 4};
+          let paths = element._changeComments.getPaths(patchRange);
+          assert.equal(Object.keys(paths).length, 0);
+
+          patchRange.basePatchNum = PARENT;
+          patchRange.patchNum = 3;
+          paths = element._changeComments.getPaths(patchRange);
+          assert.notProperty(paths, 'file/one');
+          assert.property(paths, 'file/two');
+          assert.property(paths, 'file/three');
+          assert.notProperty(paths, 'file/four');
+
+          patchRange.patchNum = 2;
+          paths = element._changeComments.getPaths(patchRange);
+          assert.property(paths, 'file/one');
+          assert.property(paths, 'file/two');
+          assert.property(paths, 'file/three');
+          assert.notProperty(paths, 'file/four');
+
+          paths = element._changeComments.getPaths();
+          assert.property(paths, 'file/one');
+          assert.property(paths, 'file/two');
+          assert.property(paths, 'file/three');
+          assert.property(paths, 'file/four');
+        });
+
+        test('getCommentsForPath', () => {
+          const patchRange = {basePatchNum: 1, patchNum: 3};
+          let path = 'file/one';
+          let comments = element._changeComments.getCommentsForPath(path,
+              patchRange);
+          assert.equal(comments.meta.changeNum, 1234);
+          assert.equal(comments.left.length, 0);
+          assert.equal(comments.right.length, 0);
+
+          path = 'file/two';
+          comments = element._changeComments.getCommentsForPath(path,
+              patchRange);
+          assert.equal(comments.left.length, 0);
+          assert.equal(comments.right.length, 1);
+
+          patchRange.basePatchNum = 2;
+          comments = element._changeComments.getCommentsForPath(path,
+              patchRange);
+          assert.equal(comments.left.length, 1);
+          assert.equal(comments.right.length, 1);
+
+          patchRange.basePatchNum = PARENT;
+          path = 'file/three';
+          comments = element._changeComments.getCommentsForPath(path,
+              patchRange);
+          assert.equal(comments.left.length, 0);
+          assert.equal(comments.right.length, 1);
+        });
+
+        test('getAllCommentsForPath', () => {
+          const path = 'file/one';
+          const comments = element._changeComments.getAllCommentsForPath(path);
+          assert.deepEqual(comments.length, 4);
+        });
      });
    });
  });
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js
@@ -71,6 +71,7 @@
       * }}
       */
      _change: Object,
+      _changeComments: Object,
      _changeNum: String,
      _diff: Object,
      _fileList: {
@@ -750,13 +751,23 @@

    _loadComments() {
      return this.$.commentAPI.loadAll(this._changeNum).then(() => {
-        this._commentMap = this.$.commentAPI.getPaths(this._patchRange);
+        this._changeComments = this.$.commentAPI._changeComments;
+        this._commentMap = this._getPaths(this._patchRange);

-        this._commentsForDiff = this.$.commentAPI.getCommentsForPath(this._path,
+        this._commentsForDiff = this._getCommentsForPath(this._path,
            this._patchRange, this._projectConfig);
      });
    },

+    _getPaths(patchRange) {
+      return this._changeComments.getPaths(patchRange);
+    },
+
+    _getCommentsForPath(path, patchRange, projectConfig) {
+      return this._changeComments.getCommentsForPath(path, patchRange,
+          projectConfig);
+    },
+
    _getDiffDrafts() {
      return this.$.restAPI.getDiffDrafts(this._changeNum);
    },
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.html
@@ -47,7 +47,7 @@ limitations under the License.

    const PARENT = 'PARENT';

-    setup(() => {
+    setup(done => {
      sandbox = sinon.sandbox.create();

      stub('gr-rest-api-interface', {
@@ -56,10 +56,14 @@ limitations under the License.
        getDiffChangeDetail() { return Promise.resolve(null); },
        getChangeFiles() { return Promise.resolve({}); },
        saveFileReviewed() { return Promise.resolve(); },
+        getDiffComments() { return Promise.resolve({}); },
        getDiffRobotComments() { return Promise.resolve({}); },
-        getDiffDrafts() { return Promise.resolve(); },
+        getDiffDrafts() { return Promise.resolve({}); },
      });
      element = fixture('basic');
+      element._loadComments().then(() => {
+        done();
+      });
    });

    teardown(() => {
@@ -484,9 +488,6 @@ limitations under the License.
    });

    test('file review status', done => {
-      stub('gr-rest-api-interface', {
-        getDiffComments() { return Promise.resolve({}); },
-      });
      const saveReviewedStub = sandbox.stub(element, '_saveReviewedState',
          () => Promise.resolve());
      sandbox.stub(element.$.diff, 'reload');
@@ -529,9 +530,6 @@ limitations under the License.
    });

    test('hash is determined from params', done => {
-      stub('gr-rest-api-interface', {
-        getDiffComments() { return Promise.resolve({}); },
-      });
      sandbox.stub(element.$.diff, 'reload');
      sandbox.stub(element, '_initCursor');

@@ -679,11 +677,6 @@ limitations under the License.

    suite('_loadComments', () => {
      test('empty', done => {
-        stub('gr-comment-api', {
-          loadAll() { return Promise.resolve(); },
-          getPaths() { return {}; },
-          getCommentsForPath() { return {meta: {}}; },
-        });
        element._loadComments().then(() => {
          assert.equal(Object.keys(element._commentMap).length, 0);
          done();
@@ -691,16 +684,11 @@ limitations under the License.
      });

      test('has paths', done => {
-        stub('gr-comment-api', {
-          loadAll() { return Promise.resolve(); },
-          getPaths() {
-            return {
-              'path/to/file/one.cpp': [{patch_set: 3, message: 'lorem'}],
-              'path-to/file/two.py': [{patch_set: 5, message: 'ipsum'}],
-            };
-          },
-          getCommentsForPath() { return {meta: {}}; },
+        sandbox.stub(element, '_getPaths').returns({
+          'path/to/file/one.cpp': [{patch_set: 3, message: 'lorem'}],
+          'path-to/file/two.py': [{patch_set: 5, message: 'ipsum'}],
        });
+        sandbox.stub(element, '_getCommentsForPath').returns({meta: {}});
        element._changeNum = '42';
        element._patchRange = {
          basePatchNum: '3',
--- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js
+++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js
@@ -1443,15 +1443,23 @@
     * @param {number|string=} opt_basePatchNum
     * @param {number|string=} opt_patchNum
     * @param {string=} opt_path
+     * @return {!Promise<!Object>}
     */
    getDiffComments(changeNum, opt_basePatchNum, opt_patchNum, opt_path) {
      return this._getDiffComments(changeNum, '/comments', opt_basePatchNum,
          opt_patchNum, opt_path);
    },

-    getDiffRobotComments(changeNum, basePatchNum, patchNum, opt_path) {
-      return this._getDiffComments(changeNum, '/robotcomments', basePatchNum,
-          patchNum, opt_path);
+    /**
+     * @param {number|string} changeNum
+     * @param {number|string=} opt_basePatchNum
+     * @param {number|string=} opt_patchNum
+     * @param {string=} opt_path
+     * @return {!Promise<!Object>}
+     */
+    getDiffRobotComments(changeNum, opt_basePatchNum, opt_patchNum, opt_path) {
+      return this._getDiffComments(changeNum, '/robotcomments',
+          opt_basePatchNum, opt_patchNum, opt_path);
    },

    /**
@@ -1463,7 +1471,7 @@
     * @param {number|string=} opt_basePatchNum
     * @param {number|string=} opt_patchNum
     * @param {string=} opt_path
-     * @return {!Promise<?Object>}
+     * @return {!Promise<!Object>}
     */
    getDiffDrafts(changeNum, opt_basePatchNum, opt_patchNum, opt_path) {
      return this.getLoggedIn().then(loggedIn => {
@@ -1502,6 +1510,7 @@
     * @param {number|string=} opt_basePatchNum
     * @param {number|string=} opt_patchNum
     * @param {string=} opt_path
+     * @return {!Promise<!Object>}
     */
    _getDiffComments(changeNum, endpoint, opt_basePatchNum,
        opt_patchNum, opt_path) {
@@ -1510,7 +1519,7 @@
       * Helper function to make promises more legible.
       *
       * @param {string|number=} opt_patchNum
-       * @return {!Object} Diff comments response.
+       * @return {!Promise<!Object>} Diff comments response.
       */
      const fetchComments = opt_patchNum => {
        return this._getChangeURLAndFetch(changeNum, endpoint, opt_patchNum);