Group rendering and weblinking in gr-diff.reload()

This will make it easier to move the loading portion into a separate
gr-diff-host component while keeping the rest in gr-diff.

This will delay computing the weblinks until the image diffs are loaded
if there are any. I think that is very minor because creating these
links is very cheap, so it should not really matter.

`finally` is used for loading instead of `then` because that is more
robust in the case where any of the early steps throw an error
(currently they don't, but in future they might).

I had to change two tests a bit because they were testing implementation
details instead of the API - I think they are better now.

Bug: Issue 9623
Change-Id: Ic4f8706e902d333da622580e3231944bd24cdeb0

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

@@ -237,13 +237,21 @@
       this._loading = true;
       this._errorMessage = null;
 
-      const diffRequest = this._getDiff();
+      const diffRequest = this._getDiff()
+          .then(diff => {
+            this._reportDiff(diff);
+            return diff;
+          })
+          .catch(e => {
+            this._handleGetDiffError(e);
+            return null;
+          });
 
       const assetRequest = diffRequest.then(diff => {
         // If the diff is null, then it's failed to load.
         if (!diff) { return null; }
 
-        return this._loadDiffAssets();
+        return this._loadDiffAssets(diff);
       });
 
       return Promise.all([diffRequest, assetRequest]).then(results => {
@@ -251,11 +259,10 @@
         if (!diff) {
           return Promise.resolve();
         }
-        if (this.prefs) {
-          return this._renderDiffTable();
-        }
-        return Promise.resolve();
-      }).then(() => {
+        this.filesWeblinks = this._getFilesWeblinks(diff);
+        this._diff = diff;
+        return this._renderDiffTable();
+      }).finally(() => {
         this._loading = false;
       });
     },
@@ -709,6 +716,7 @@
     },
 
     _renderDiffTable() {
+      if (!this.prefs) { return Promise.resolve(); }
       if (this.prefs.context === -1 &&
           this._diffLength(this._diff) >= LARGE_DIFF_THRESHOLD_LINES &&
           this._safetyBypass === null) {
@@ -758,7 +766,7 @@
     _getDiff() {
       // Wrap the diff request in a new promise so that the error handler
       // rejects the promise, allowing the error to be handled in the .catch.
-      const request = new Promise((resolve, reject) => {
+      return new Promise((resolve, reject) => {
         this.$.restAPI.getDiff(
             this.changeNum,
             this.patchRange.basePatchNum,
@@ -767,18 +775,6 @@
             reject)
             .then(resolve);
       });
-
-      return request
-          .then(diff => {
-            this.filesWeblinks = this._getFilesWeblinks(diff);
-            this._diff = diff;
-            this._reportDiff(diff);
-            return diff;
-          })
-          .catch(e => {
-            this._handleGetDiffError(e);
-            return null;
-          });
     },
 
     _getFilesWeblinks(diff) {
@@ -837,22 +833,28 @@
       return this.$.restAPI.getLoggedIn();
     },
 
-    /** @return {boolean} */
-    _computeIsImageDiff() {
-      if (!this._diff) { return false; }
+    /**
+     * @param {Object} diff
+     * @return {boolean}
+     */
+    _computeIsImageDiff(diff) {
+      if (!diff) { return false; }
 
-      const isA = this._diff.meta_a &&
-          this._diff.meta_a.content_type.startsWith('image/');
-      const isB = this._diff.meta_b &&
-          this._diff.meta_b.content_type.startsWith('image/');
+      const isA = diff.meta_a &&
+          diff.meta_a.content_type.startsWith('image/');
+      const isB = diff.meta_b &&
+          diff.meta_b.content_type.startsWith('image/');
 
-      return !!(this._diff.binary && (isA || isB));
+      return !!(diff.binary && (isA || isB));
     },
 
-    /** @return {!Promise} */
-    _loadDiffAssets() {
-      if (this.isImageDiff) {
-        return this._getImages().then(images => {
+    /**
+     * @param {Object} diff
+     * @return {!Promise}
+     */
+    _loadDiffAssets(diff) {
+      if (this._computeIsImageDiff(diff)) {
+        return this._getImages(diff).then(images => {
           this._baseImage = images.baseImage;
           this._revisionImage = images.revisionImage;
         });
@@ -863,9 +865,12 @@
       }
     },
 
-    /** @return {!Promise} */
-    _getImages() {
-      return this.$.restAPI.getImagesForDiff(this.changeNum, this._diff,
+    /**
+     * @param {Object} diff
+     * @return {!Promise}
+     */
+    _getImages(diff) {
+      return this.$.restAPI.getImagesForDiff(this.changeNum, diff,
           this.patchRange);
     },
 

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

@@ -212,12 +212,14 @@ limitations under the License.
       test('loads files weblinks', () => {
         const weblinksStub = sandbox.stub(Gerrit.Nav, '_generateWeblinks')
             .returns({name: 'stubb', url: '#s'});
-        sandbox.stub(element.$.restAPI, 'getDiff').returns(Promise.resolve({}));
+        sandbox.stub(element.$.restAPI, 'getDiff').returns(Promise.resolve({
+          content: [],
+        }));
         element.projectName = 'test-project';
         element.path = 'test-path';
         element.commitRange = {baseCommit: 'test-base', commit: 'test-commit'};
         element.patchRange = {};
-        return element._getDiff().then(() => {
+        return element.reload().then(() => {
           assert.isTrue(weblinksStub.calledTwice);
           assert.isTrue(weblinksStub.firstCall.calledWith({
             commit: 'test-base',
@@ -778,15 +780,14 @@ limitations under the License.
         element._getDiff().then(done);
       });
 
-      test('_getDiff resolves as null on error', () => {
+      test('reload resolves on error', () => {
         const onErrStub = sandbox.stub(element, '_handleGetDiffError');
         const error = {ok: false, status: 500};
         sandbox.stub(element.$.restAPI, 'getDiff',
             (changeNum, basePatchNum, patchNum, path, onErr) => {
               onErr(error);
             });
-        return element._getDiff().then(diff => {
-          assert.isNull(diff);
+        return element.reload().then(() => {
           assert.isTrue(onErrStub.calledOnce);
         });
       });