Merge "Centralized comment requests"

polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html

@@ -19,6 +19,7 @@ limitations under the License.
 <link rel="import" href="../../../bower_components/polymer/polymer.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="../../shared/gr-button/gr-button.html">
@@ -410,6 +411,7 @@ 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>

polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js

@@ -161,6 +161,9 @@
         });
       }));
 
+      // 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;
@@ -879,6 +882,9 @@
       console.log('Expanding diff', 1 + initialCount - paths.length, 'of',
           initialCount, ':', paths[0]);
       const diffElem = this._findDiffByPath(paths[0], diffElements);
+      diffElem.comments = this.$.commentAPI.getCommentsForPath(paths[0],
+          this.patchRange, this.projectConfig);
+
       const promises = [diffElem.reload()];
       if (this._isLoggedIn) {
         promises.push(this._reviewFile(paths[0]));

polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html

@@ -60,6 +60,12 @@ 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 = fixture('basic');
       element.numFilesShown = 200;
       saveStub = sandbox.stub(element, '_saveReviewedState',
@@ -965,7 +971,7 @@ limitations under the License.
     const setupDiff = function(diff) {
       const mock = document.createElement('mock-diff-response');
       diff._diff = mock.diffResponse;
-      diff._comments = {
+      diff.comments = {
         left: [],
         right: [],
       };
@@ -1013,6 +1019,11 @@ 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 = fixture('basic');
       element.numFilesShown = 75;
       element.selectedIndex = 0;

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

@@ -0,0 +1,26 @@
+<!--
+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="../../../behaviors/gr-patch-set-behavior/gr-patch-set-behavior.html">
+<link rel="import" href="../../shared/gr-rest-api-interface/gr-rest-api-interface.html">
+
+<dom-module id="gr-comment-api">
+  <template>
+    <gr-rest-api-interface id="restAPI"></gr-rest-api-interface>
+  </template>
+  <script src="gr-comment-api.js"></script>
+</dom-module>

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

@@ -0,0 +1,174 @@
+// 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';
+
+  const PARENT = 'PARENT';
+
+  Polymer({
+    is: 'gr-comment-api',
+
+    properties: {
+      _changeNum: Number,
+      _comments: Object,
+      _drafts: Object,
+      _robotComments: Object,
+    },
+
+    behaviors: [
+      Gerrit.PatchSetBehavior,
+    ],
+
+    /**
+     * Load all comments (with drafts and robot comments) for the given change
+     * number. The returned promise resolves when the comments have loaded, but
+     * does not yield the comment data.
+     *
+     * @param {!number} changeNum
+     * @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.getLoggedIn()
+          .then(loggedIn => {
+            if (!loggedIn) { return Promise.resolve({}); }
+            return this.$.restAPI.getDiffDrafts(changeNum);
+          })
+          .then(drafts => { this._drafts = drafts; }));
+
+      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}
+     */
+    _isInPatchRange(comment, range) {
+      return this._isInBaseOfPatchRange(comment, range) ||
+          this._isInRevisionOfPatchRange(comment, range);
+    },
+  });
+})();

polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api_test.html

@@ -0,0 +1,194 @@
+<!DOCTYPE html>
+<!--
+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-comment-api</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"/>
+
+<link rel="import" href="./gr-comment-api.html">
+
+<script>void(0);</script>
+
+<test-fixture id="basic">
+  <template>
+    <gr-comment-api></gr-comment-api>
+  </template>
+</test-fixture>
+
+<script>
+  suite('gr-comment-api tests', () => {
+    const PARENT = 'PARENT';
+
+    let element;
+    let sandbox;
+
+    setup(() => {
+      sandbox = sinon.sandbox.create();
+      element = fixture('basic');
+    });
+
+    teardown(() => { sandbox.restore(); });
+
+    test('loads logged-out', () => {
+      const changeNum = 1234;
+
+      sandbox.stub(element.$.restAPI, 'getLoggedIn')
+          .returns(Promise.resolve(false));
+      sandbox.stub(element.$.restAPI, 'getDiffComments')
+          .returns(Promise.resolve({
+            'foo.c': [{id: '123', message: 'foo bar', in_reply_to: '321'}],
+          }));
+      sandbox.stub(element.$.restAPI, 'getDiffRobotComments')
+          .returns(Promise.resolve({'foo.c': [{id: '321', message: 'done'}]}));
+      sandbox.stub(element.$.restAPI, 'getDiffDrafts');
+
+      return element.loadAll(changeNum).then(() => {
+        assert.isTrue(element.$.restAPI.getDiffComments.calledWithExactly(
+            changeNum));
+        assert.isTrue(element.$.restAPI.getDiffRobotComments.calledWithExactly(
+            changeNum));
+        assert.isFalse(element.$.restAPI.getDiffDrafts.called);
+        assert.isOk(element._comments);
+        assert.isOk(element._robotComments);
+        assert.deepEqual(element._drafts, {});
+      });
+    });
+
+    test('loads logged-in', () => {
+      const changeNum = 1234;
+
+      sandbox.stub(element.$.restAPI, 'getLoggedIn')
+          .returns(Promise.resolve(true));
+      sandbox.stub(element.$.restAPI, 'getDiffComments')
+          .returns(Promise.resolve({
+            'foo.c': [{id: '123', message: 'foo bar', in_reply_to: '321'}],
+          }));
+      sandbox.stub(element.$.restAPI, 'getDiffRobotComments')
+          .returns(Promise.resolve({'foo.c': [{id: '321', message: 'done'}]}));
+      sandbox.stub(element.$.restAPI, 'getDiffDrafts')
+          .returns(Promise.resolve({'foo.c': [{id: '555', message: 'ack'}]}));
+
+      return element.loadAll(changeNum).then(() => {
+        assert.isTrue(element.$.restAPI.getDiffComments.calledWithExactly(
+            changeNum));
+        assert.isTrue(element.$.restAPI.getDiffRobotComments.calledWithExactly(
+            changeNum));
+        assert.isTrue(element.$.restAPI.getDiffDrafts.calledWithExactly(
+            changeNum));
+        assert.isOk(element._comments);
+        assert.isOk(element._robotComments);
+        assert.notDeepEqual(element._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},
+          ],
+        };
+      });
+
+      test('getPaths', () => {
+        const patchRange = {basePatchNum: 1, patchNum: 4};
+        let paths = element.getPaths(patchRange);
+        assert.equal(Object.keys(paths).length, 0);
+
+        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');
+
+        patchRange.patchNum = 2;
+        paths = element.getPaths(patchRange);
+        assert.property(paths, 'file/one');
+        assert.property(paths, 'file/two');
+        assert.property(paths, 'file/three');
+      });
+
+      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);
+
+        path = 'file/two';
+        comments = element.getCommentsForPath(path, patchRange);
+        assert.equal(comments.left.length, 0);
+        assert.equal(comments.right.length, 1);
+
+        patchRange.basePatchNum = 2;
+        comments = element.getCommentsForPath(path, patchRange);
+        assert.equal(comments.left.length, 1);
+        assert.equal(comments.right.length, 1);
+
+        patchRange.basePatchNum = PARENT;
+        path = 'file/three';
+        comments = element.getCommentsForPath(path, patchRange);
+        assert.equal(comments.left.length, 0);
+        assert.equal(comments.right.length, 1);
+      });
+    });
+  });
+</script>

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

@@ -59,6 +59,7 @@ limitations under the License.
       // Register the diff with the cursor.
       cursorElement.push('diffs', diffElement);
 
+      diffElement.comments = {left: [], right: []};
       diffElement.$.restAPI.getDiffPreferences().then(prefs => {
         diffElement.prefs = prefs;
       });
@@ -67,19 +68,8 @@ limitations under the License.
         return Promise.resolve(mockDiffResponse.diffResponse);
       });
 
-      sandbox.stub(diffElement, '_getDiffComments', () => {
-        return Promise.resolve({baseComments: [], comments: []});
-      });
-
-      sandbox.stub(diffElement, '_getDiffDrafts', () => {
-        return Promise.resolve({baseComments: [], comments: []});
-      });
-
-      sandbox.stub(diffElement, '_getDiffRobotComments', () => {
-        return Promise.resolve({baseComments: [], comments: []});
-      });
-
       const setupDone = () => {
+        cursorElement._updateStops();
         cursorElement.moveToFirstChunk();
         diffElement.removeEventListener('render', setupDone);
         done();
@@ -218,12 +208,13 @@ limitations under the License.
       const moveToNumStub = sandbox.stub(cursorElement, 'moveToLineNumber');
       const moveToChunkStub = sandbox.stub(cursorElement, 'moveToFirstChunk');
 
-      diffElement.addEventListener('render', () => {
+      function renderHandler() {
+        diffElement.removeEventListener('render', renderHandler);
         assert.isFalse(moveToNumStub.called);
         assert.isTrue(moveToChunkStub.called);
         done();
-      });
+      }
-
+      diffElement.addEventListener('render', renderHandler);
       diffElement.reload();
     });
 
@@ -231,13 +222,15 @@ limitations under the License.
       const moveToNumStub = sandbox.stub(cursorElement, 'moveToLineNumber');
       const moveToChunkStub = sandbox.stub(cursorElement, 'moveToFirstChunk');
 
-      diffElement.addEventListener('render', () => {
+      function renderHandler() {
+        diffElement.removeEventListener('render', renderHandler);
         assert.isFalse(moveToChunkStub.called);
         assert.isTrue(moveToNumStub.called);
         assert.equal(moveToNumStub.lastCall.args[0], 10);
         assert.equal(moveToNumStub.lastCall.args[1], 'right');
         done();
-      });
+      }
+      diffElement.addEventListener('render', renderHandler);
 
       cursorElement.initialLineNumber = 10;
       cursorElement.side = 'right';

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

@@ -24,6 +24,7 @@ limitations under the License.
 <link rel="import" href="../../shared/gr-fixed-panel/gr-fixed-panel.html">
 <link rel="import" href="../../shared/gr-rest-api-interface/gr-rest-api-interface.html">
 <link rel="import" href="../../shared/gr-select/gr-select.html">
+<link rel="import" href="../gr-comment-api/gr-comment-api.html">
 <link rel="import" href="../gr-diff-cursor/gr-diff-cursor.html">
 <link rel="import" href="../gr-diff-preferences/gr-diff-preferences.html">
 <link rel="import" href="../gr-diff/gr-diff.html">
@@ -337,6 +338,7 @@ limitations under the License.
     <gr-rest-api-interface id="restAPI"></gr-rest-api-interface>
     <gr-storage id="storage"></gr-storage>
     <gr-diff-cursor id="cursor"></gr-diff-cursor>
+    <gr-comment-api id="commentAPI"></gr-comment-api>
   </template>
   <script src="gr-diff-view.js"></script>
 </dom-module>

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

@@ -21,6 +21,8 @@
 
   const COMMENT_SAVE = 'Try again when all comments have saved.';
 
+  const PARENT = 'PARENT';
+
   const DiffSides = {
     LEFT: 'left',
     RIGHT: 'right',
@@ -99,6 +101,8 @@
        */
       _commentMap: Object,
 
+      _commentsForDiff: Object,
+
       /**
        * Object to contain the path of the next and previous file in the current
        * change and patch range that has comments.
@@ -464,7 +468,7 @@
       this._changeNum = value.changeNum;
       this._patchRange = {
         patchNum: value.patchNum,
-        basePatchNum: value.basePatchNum || 'PARENT',
+        basePatchNum: value.basePatchNum || PARENT,
       };
       this._path = value.path;
 
@@ -493,14 +497,13 @@
 
       promises.push(this._getChangeDetail(this._changeNum));
 
+      promises.push(this._loadComments());
+
       Promise.all(promises).then(() => {
         this._loading = false;
+        this.$.diff.comments = this._commentsForDiff;
         this.$.diff.reload();
       });
-
-      this._loadCommentMap().then(commentMap => {
-        this._commentMap = commentMap;
-      });
     },
 
     _changeViewStateChanged(changeViewState) {
@@ -557,7 +560,7 @@
     _patchRangeStr(patchRange) {
       let patchStr = patchRange.patchNum;
       if (patchRange.basePatchNum != null &&
-          patchRange.basePatchNum != 'PARENT') {
+          patchRange.basePatchNum != PARENT) {
         patchStr = patchRange.basePatchNum + '..' + patchRange.patchNum;
       }
       return patchStr;
@@ -584,7 +587,7 @@
       for (const rev of Object.values(revisions || {})) {
         latestPatchNum = Math.max(latestPatchNum, rev._number);
       }
-      if (patchRange.basePatchNum !== 'PARENT' ||
+      if (patchRange.basePatchNum !== PARENT ||
           parseInt(patchRange.patchNum, 10) !== latestPatchNum) {
         patchNum = patchRange.patchNum;
         basePatchNum = patchRange.basePatchNum;
@@ -717,33 +720,12 @@
       return url;
     },
 
-    /**
+    _loadComments() {
-     * Request all comments (and drafts and robot comments) for the current
+      return this.$.commentAPI.loadAll(this._changeNum).then(() => {
-     * change and construct the map of file paths that have comments for the
+        this._commentMap = this.$.commentAPI.getPaths(this._patchRange);
-     * current patch range.
-     * @return {Promise} A promise that yields a comment map object.
-     */
-    _loadCommentMap() {
-      const filterByRange = comment =>
-          this.patchNumEquals(comment.patch_set, this._patchRange.patchNum) ||
-            this.patchNumEquals(comment.patch_set,
-                this._patchRange.basePatchNum);
 
-      return Promise.all([
+        this._commentsForDiff = this.$.commentAPI.getCommentsForPath(this._path,
-        this.$.restAPI.getDiffComments(this._changeNum),
+            this._patchRange, this._projectConfig);
-        this._getDiffDrafts(),
-        this.$.restAPI.getDiffRobotComments(this._changeNum),
-      ]).then(results => {
-        const commentMap = {};
-        for (const response of results) {
-          for (const path in response) {
-            if (response.hasOwnProperty(path) &&
-                response[path].filter(filterByRange).length) {
-              commentMap[path] = true;
-            }
-          }
-        }
-        return commentMap;
       });
     },
 

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

@@ -56,7 +56,7 @@ limitations under the License.
         getDiffChangeDetail() { return Promise.resolve(null); },
         getChangeFiles() { return Promise.resolve({}); },
         saveFileReviewed() { return Promise.resolve(); },
-        getDiffRobotComments() { return Promise.resolve(); },
+        getDiffRobotComments() { return Promise.resolve({}); },
         getDiffDrafts() { return Promise.resolve(); },
       });
       element = fixture('basic');
@@ -639,57 +639,41 @@ limitations under the License.
       assert.equal(element._getDiffViewMode(), 'SIDE_BY_SIDE');
     });
 
-    suite('_loadCommentMap', () => {
+    suite('_loadComments', () => {
       test('empty', done => {
-        stub('gr-rest-api-interface', {
+        stub('gr-comment-api', {
-          getDiffComments() { return Promise.resolve({}); },
+          loadAll() { return Promise.resolve(); },
+          getPaths() { return {}; },
+          getCommentsForPath() { return {meta: {}}; },
         });
-        element._loadCommentMap().then(map => {
+        element._loadComments().then(() => {
-          assert.equal(Object.keys(map).length, 0);
+          assert.equal(Object.keys(element._commentMap).length, 0);
           done();
         });
       });
 
-      test('paths in patch range', done => {
+      test('has paths', done => {
-        stub('gr-rest-api-interface', {
+        stub('gr-comment-api', {
-          getDiffComments() {
+          loadAll() { return Promise.resolve(); },
-            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: {}}; },
         });
         element._changeNum = '42';
         element._patchRange = {
           basePatchNum: '3',
           patchNum: '5',
         };
-        element._loadCommentMap().then(map => {
+        element._loadComments().then(() => {
-          assert.deepEqual(Object.keys(map),
+          assert.deepEqual(Object.keys(element._commentMap),
               ['path/to/file/one.cpp', 'path-to/file/two.py']);
           done();
         });
       });
-
-      test('empty for paths outside patch range', done => {
-        stub('gr-rest-api-interface', {
-          getDiffComments() {
-            return Promise.resolve({
-              'path/to/file/one.cpp': [{patch_set: PARENT, message: 'lorem'}],
-              'path-to/file/two.py': [{patch_set: 2, message: 'ipsum'}],
-            });
-          },
-        });
-        element._changeNum = '42';
-        element._patchRange = {
-          basePatchNum: '3',
-          patchNum: '5',
-        };
-        element._loadCommentMap().then(map => {
-          assert.equal(Object.keys(map).length, 0);
-          done();
-        });
-      });
     });
 
     suite('_computeCommentSkips', () => {

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

@@ -225,10 +225,10 @@ limitations under the License.
         <gr-diff-highlight
             id="highlights"
             logged-in="[[_loggedIn]]"
-            comments="{{_comments}}">
+            comments="{{comments}}">
           <gr-diff-builder
               id="diffBuilder"
-              comments="[[_comments]]"
+              comments="[[comments]]"
               diff="[[_diff]]"
               diff-path="[[path]]"
               view-mode="[[viewMode]]"

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

@@ -77,6 +77,7 @@
         reflectToAttribute: true,
       },
       noRenderOnPrefsChange: Boolean,
+      comments: Object,
       _loggedIn: {
         type: Boolean,
         value: false,
@@ -101,7 +102,6 @@
         type: String,
         value: '',
       },
-      _comments: Object,
       _baseImage: Object,
       _revisionImage: Object,
 
@@ -152,10 +152,6 @@
         return this._loadDiffAssets();
       }));
 
-      promises.push(this._getDiffCommentsAndDrafts().then(comments => {
-        this._comments = comments;
-      }));
-
       return Promise.all(promises).then(() => {
         if (this.prefs) {
           return this._renderDiffTable();
@@ -373,7 +369,7 @@
       const comment = e.detail.comment;
       const side = e.detail.comment.__commentSide;
       const idx = this._findDraftIndex(comment, side);
-      this.set(['_comments', side, idx], comment);
+      this.set(['comments', side, idx], comment);
     },
 
     _handleCommentUpdate(e) {
@@ -384,9 +380,9 @@
         idx = this._findDraftIndex(comment, side);
       }
       if (idx !== -1) { // Update draft or comment.
-        this.set(['_comments', side, idx], comment);
+        this.set(['comments', side, idx], comment);
       } else { // Create new draft.
-        this.push(['_comments', side], comment);
+        this.push(['comments', side], comment);
       }
     },
 
@@ -396,24 +392,24 @@
         idx = this._findDraftIndex(comment, side);
       }
       if (idx !== -1) {
-        this.splice('_comments.' + side, idx, 1);
+        this.splice('comments.' + side, idx, 1);
       }
     },
 
     _findCommentIndex(comment, side) {
-      if (!comment.id || !this._comments[side]) {
+      if (!comment.id || !this.comments[side]) {
         return -1;
       }
-      return this._comments[side].findIndex(item => {
+      return this.comments[side].findIndex(item => {
         return item.id === comment.id;
       });
     },
 
     _findDraftIndex(comment, side) {
-      if (!comment.__draftID || !this._comments[side]) {
+      if (!comment.__draftID || !this.comments[side]) {
         return -1;
       }
-      return this._comments[side].findIndex(item => {
+      return this.comments[side].findIndex(item => {
         return item.__draftID === comment.__draftID;
       });
     },
@@ -463,7 +459,7 @@
 
       this.updateStyles(stylesToUpdate);
 
-      if (this._diff && this._comments && !this.noRenderOnPrefsChange) {
+      if (this._diff && this.comments && !this.noRenderOnPrefsChange) {
         this._renderDiffTable();
       }
     },
@@ -477,7 +473,7 @@
       }
 
       this._showWarning = false;
-      return this.$.diffBuilder.render(this._comments, this._getBypassPrefs());
+      return this.$.diffBuilder.render(this.comments, this._getBypassPrefs());
     },
 
     /**
@@ -519,73 +515,6 @@
           });
     },
 
-    _getDiffComments() {
-      return this.$.restAPI.getDiffComments(
-          this.changeNum,
-          this.patchRange.basePatchNum,
-          this.patchRange.patchNum,
-          this.path);
-    },
-
-    _getDiffDrafts() {
-      return this._getLoggedIn().then(loggedIn => {
-        if (!loggedIn) {
-          return Promise.resolve({baseComments: [], comments: []});
-        }
-        return this.$.restAPI.getDiffDrafts(
-            this.changeNum,
-            this.patchRange.basePatchNum,
-            this.patchRange.patchNum,
-            this.path);
-      });
-    },
-
-    _getDiffRobotComments() {
-      return this.$.restAPI.getDiffRobotComments(
-          this.changeNum,
-          this.patchRange.basePatchNum,
-          this.patchRange.patchNum,
-          this.path);
-    },
-
-    _getDiffCommentsAndDrafts() {
-      const promises = [];
-      promises.push(this._getDiffComments());
-      promises.push(this._getDiffDrafts());
-      promises.push(this._getDiffRobotComments());
-      return Promise.all(promises).then(results => {
-        return Promise.resolve({
-          comments: results[0],
-          drafts: results[1],
-          robotComments: results[2],
-        });
-      }).then(this._normalizeDiffCommentsAndDrafts.bind(this));
-    },
-
-    _normalizeDiffCommentsAndDrafts(results) {
-      function markAsDraft(d) {
-        d.__draft = true;
-        return d;
-      }
-      const baseDrafts = results.drafts.baseComments.map(markAsDraft);
-      const drafts = results.drafts.comments.map(markAsDraft);
-
-      const baseRobotComments = results.robotComments.baseComments;
-      const robotComments = results.robotComments.comments;
-      return Promise.resolve({
-        meta: {
-          path: this.path,
-          changeNum: this.changeNum,
-          patchRange: this.patchRange,
-          projectConfig: this.projectConfig,
-        },
-        left: results.comments.baseComments.concat(baseDrafts)
-            .concat(baseRobotComments),
-        right: results.comments.comments.concat(drafts)
-            .concat(robotComments),
-      });
-    },
-
     _getLoggedIn() {
       return this.$.restAPI.getLoggedIn();
     },

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

@@ -53,8 +53,6 @@ limitations under the License.
 
       // Stub the network calls into requests that never resolve.
       sandbox.stub(element, '_getDiff', () => new Promise(() => {}));
-      sandbox.stub(element, '_getDiffCommentsAndDrafts',
-          () => new Promise(() => {}));
 
       element.reload();
       assert.isTrue(cancelStub.called);
@@ -105,29 +103,6 @@ limitations under the License.
             element.$$('.diffContainer').classList.contains('displayLine'));
       });
 
-      test('get drafts', done => {
-        element.patchRange = {basePatchNum: 0, patchNum: 0};
-
-        const getDraftsStub = sandbox.stub(element.$.restAPI, 'getDiffDrafts');
-        element._getDiffDrafts().then(result => {
-          assert.deepEqual(result, {baseComments: [], comments: []});
-          sinon.assert.notCalled(getDraftsStub);
-          done();
-        });
-      });
-
-      test('get robot comments', done => {
-        element.patchRange = {basePatchNum: 0, patchNum: 0};
-
-        const getDraftsStub = sandbox.stub(element.$.restAPI,
-            'getDiffRobotComments');
-        element._getDiffDrafts().then(result => {
-          assert.deepEqual(result, {baseComments: [], comments: []});
-          sinon.assert.notCalled(getDraftsStub);
-          done();
-        });
-      });
-
       test('loads files weblinks', done => {
         sandbox.stub(element.$.restAPI, 'getDiff').returns(
             Promise.resolve({
@@ -149,7 +124,7 @@ limitations under the License.
       });
 
       test('remove comment', () => {
-        element._comments = {
+        element.comments = {
           meta: {
             changeNum: '42',
             patchRange: {
@@ -176,7 +151,7 @@ limitations under the License.
         element._removeComment({});
         // Using JSON.stringify because Safari 9.1 (11601.5.17.1) doesn’t seem
         // to believe that one object deepEquals another even when they do :-/.
-        assert.equal(JSON.stringify(element._comments), JSON.stringify({
+        assert.equal(JSON.stringify(element.comments), JSON.stringify({
           meta: {
             changeNum: '42',
             patchRange: {
@@ -202,7 +177,7 @@ limitations under the License.
 
         element._removeComment({id: 'bc2', side: 'PARENT',
           __commentSide: 'left'});
-        assert.deepEqual(element._comments, {
+        assert.deepEqual(element.comments, {
           meta: {
             changeNum: '42',
             patchRange: {
@@ -226,7 +201,7 @@ limitations under the License.
         });
 
         element._removeComment({id: 'd2', __commentSide: 'right'});
-        assert.deepEqual(element._comments, {
+        assert.deepEqual(element.comments, {
           meta: {
             changeNum: '42',
             patchRange: {
@@ -354,6 +329,7 @@ limitations under the License.
               () => Promise.resolve(mockComments)));
 
           element.patchRange = {basePatchNum: 'PARENT', patchNum: 1};
+          element.comments = {left: [], right: []};
         });
 
         test('renders image diffs with same file name', done => {
@@ -691,7 +667,7 @@ limitations under the License.
         const setupDiff = function() {
           const mock = document.createElement('mock-diff-response');
           element._diff = mock.diffResponse;
-          element._comments = {
+          element.comments = {
             left: [],
             right: [],
           };
@@ -747,102 +723,6 @@ limitations under the License.
         element = fixture('basic');
       });
 
-      test('get drafts', done => {
-        element.patchRange = {basePatchNum: 0, patchNum: 0};
-        const draftsResponse = {
-          baseComments: [{id: 'foo'}],
-          comments: [{id: 'bar'}],
-        };
-        sandbox.stub(element.$.restAPI, 'getDiffDrafts',
-            () => Promise.resolve(draftsResponse));
-        element._getDiffDrafts().then(result => {
-          assert.deepEqual(result, draftsResponse);
-          done();
-        });
-      });
-
-      test('get comments and drafts', done => {
-        const comments = {
-          baseComments: [
-            {id: 'bc1', __commentSide: 'left'},
-            {id: 'bc2', __commentSide: 'left'},
-          ],
-          comments: [
-            {id: 'c1', __commentSide: 'right'},
-            {id: 'c2', __commentSide: 'right'},
-          ],
-        };
-        sandbox.stub(element, '_getDiffComments',
-            () => Promise.resolve(comments));
-
-        const drafts = {
-          baseComments: [
-            {id: 'bd1', __commentSide: 'left'},
-            {id: 'bd2', __commentSide: 'left'},
-          ],
-          comments: [
-            {id: 'd1', __commentSide: 'right'},
-            {id: 'd2', __commentSide: 'right'},
-          ],
-        };
-
-        sandbox.stub(element, '_getDiffDrafts', () => Promise.resolve(drafts));
-
-        const robotComments = {
-          baseComments: [
-            {id: 'br1', __commentSide: 'left'},
-            {id: 'br2', __commentSide: 'left'},
-          ],
-          comments: [
-            {id: 'r1', __commentSide: 'right'},
-            {id: 'r2', __commentSide: 'right'},
-          ],
-        };
-
-        sandbox.stub(element,
-            '_getDiffRobotComments', () => Promise.resolve(robotComments));
-
-        element.changeNum = '42';
-        element.patchRange = {
-          basePatchNum: 'PARENT',
-          patchNum: 3,
-        };
-        element.path = '/path/to/foo';
-        element.projectConfig = {foo: 'bar'};
-
-        element._getDiffCommentsAndDrafts().then(result => {
-          assert.deepEqual(result, {
-            meta: {
-              changeNum: '42',
-              patchRange: {
-                basePatchNum: 'PARENT',
-                patchNum: 3,
-              },
-              path: '/path/to/foo',
-              projectConfig: {foo: 'bar'},
-            },
-            left: [
-              {id: 'bc1', __commentSide: 'left'},
-              {id: 'bc2', __commentSide: 'left'},
-              {id: 'bd1', __draft: true, __commentSide: 'left'},
-              {id: 'bd2', __draft: true, __commentSide: 'left'},
-              {id: 'br1', __commentSide: 'left'},
-              {id: 'br2', __commentSide: 'left'},
-            ],
-            right: [
-              {id: 'c1', __commentSide: 'right'},
-              {id: 'c2', __commentSide: 'right'},
-              {id: 'd1', __draft: true, __commentSide: 'right'},
-              {id: 'd2', __draft: true, __commentSide: 'right'},
-              {id: 'r1', __commentSide: 'right'},
-              {id: 'r2', __commentSide: 'right'},
-            ],
-          });
-
-          done();
-        });
-      });
-
       test('addDraftAtLine', done => {
         const fakeLineEl = {getAttribute: sandbox.stub().returns(42)};
         sandbox.stub(element, '_selectLine');
@@ -868,7 +748,7 @@ limitations under the License.
             change_type: 'MODIFIED',
             content: [{skip: 66}],
           };
-          element._comments = {
+          element.comments = {
             meta: {
               changeNum: '42',
               patchRange: {
@@ -912,7 +792,7 @@ limitations under the License.
 
       suite('handle comment-update', () => {
         setup(() => {
-          element._comments = {
+          element.comments = {
             meta: {
               changeNum: '42',
               patchRange: {
@@ -941,7 +821,7 @@ limitations under the License.
           const comment = {__draft: true, __draftID: 'tempID', side: 'PARENT',
             __commentSide: 'left'};
           element.fire('comment-update', {comment});
-          assert.include(element._comments.left, comment);
+          assert.include(element.comments.left, comment);
         });
 
         test('saving a draft', () => {
@@ -953,10 +833,10 @@ limitations under the License.
             side: 'PARENT',
             __commentSide: 'left',
           };
-          element._comments.left.push(comment);
+          element.comments.left.push(comment);
           comment.id = id;
           element.fire('comment-update', {comment});
-          const drafts = element._comments.left.filter(item => {
+          const drafts = element.comments.left.filter(item => {
             return item.__draftID === draftID;
           });
           assert.equal(drafts.length, 1);
@@ -1007,7 +887,7 @@ limitations under the License.
             () => Promise.resolve());
         const mock = document.createElement('mock-diff-response');
         element._diff = mock.diffResponse;
-        element._comments = {left: [], right: []};
+        element.comments = {left: [], right: []};
         element.noRenderOnPrefsChange = true;
       });
 

polygerrit-ui/app/test/index.html

@@ -71,6 +71,7 @@ limitations under the License.
     'core/gr-router/gr-router_test.html',
     'core/gr-reporting/gr-reporting_test.html',
     'core/gr-search-bar/gr-search-bar_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',