Merge changes from topic "Issue 6969"

* changes:
  Proactively collapse diffs while change view params are updated
  Deep clone ETag cache hits
  Revert "Ensure undefined patch ranges are always populated"

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

@@ -468,10 +468,6 @@
         return;
       }
 
-      // If the patch changed, and was not set to undefined/undefined, we need
-      // not reload all resources -- only the commit info and the file list.
-      // If the patch range was set to undefined/undefined, the user is looking
-      // to refresh the whole view.
       const patchChanged = this._patchRange &&
           (value.patchNum !== undefined && value.basePatchNum !== undefined) &&
           (this._patchRange.patchNum !== value.patchNum ||
@@ -481,24 +477,28 @@
         this._initialLoadComplete = false;
       }
 
-      const patchNum = value.patchNum ||
-          this.computeLatestPatchNum(this._allPatchSets);
-
-      const basePatchNum = value.basePatchNum || 'PARENT';
-
-      this._patchRange = {patchNum, basePatchNum};
+      const patchRange = {
+        patchNum: value.patchNum,
+        basePatchNum: value.basePatchNum || 'PARENT',
+      };
+      this.$.fileList.collapseAllDiffs();
 
       if (this._initialLoadComplete && patchChanged) {
+        if (patchRange.patchNum == null) {
+          patchRange.patchNum = this.computeLatestPatchNum(this._allPatchSets);
+        }
+        this._patchRange = patchRange;
         this._reloadPatchNumDependentResources().then(() => {
           this.$.jsAPI.handleEvent(this.$.jsAPI.EventType.SHOW_CHANGE, {
             change: this._change,
-            patchNum,
+            patchNum: patchRange.patchNum,
           });
         });
         return;
       }
 
       this._changeNum = value.changeNum;
+      this._patchRange = patchRange;
       this.$.relatedChanges.clear();
 
       this._reload().then(() => {

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

@@ -672,6 +672,7 @@ limitations under the License.
           '_reloadPatchNumDependentResources',
           () => { return Promise.resolve(); });
       const relatedClearSpy = sandbox.spy(element.$.relatedChanges, 'clear');
+      const collapseStub = sandbox.stub(element.$.fileList, 'collapseAllDiffs');
 
       const value = {
         view: Gerrit.Nav.View.CHANGE,
@@ -689,26 +690,22 @@ limitations under the License.
       assert.isFalse(reloadStub.calledTwice);
       assert.isTrue(reloadPatchDependentStub.calledOnce);
       assert.isTrue(relatedClearSpy.calledOnce);
+      assert.isTrue(collapseStub.calledTwice);
     });
 
     test('reload entire page when patchRange doesnt change', () => {
-      const mockPatchRange = {patchNum: '1337', basePatchNum: 'PARENT'};
       const reloadStub = sandbox.stub(element, '_reload',
           () => { return Promise.resolve(); });
-      element._patchRange = {};
-      sandbox.stub(element, 'computeLatestPatchNum').returns('1337');
+      const collapseStub = sandbox.stub(element.$.fileList, 'collapseAllDiffs');
       const value = {
         view: Gerrit.Nav.View.CHANGE,
       };
       element._paramsChanged(value);
       assert.isTrue(reloadStub.calledOnce);
-      assert.deepEqual(element._patchRange, mockPatchRange);
-
       element._initialLoadComplete = true;
-      element._patchRange = {};
       element._paramsChanged(value);
       assert.isTrue(reloadStub.calledTwice);
-      assert.deepEqual(element._patchRange, mockPatchRange);
+      assert.isTrue(collapseStub.calledTwice);
     });
 
     test('include base patch when not parent', () => {

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

@@ -208,7 +208,7 @@ limitations under the License.
             if="[[_fileListActionsVisible(_shownFiles.*, _maxFilesForBulkActions)]]">
           <gr-button link on-tap="_expandAllDiffs">Show diffs</gr-button>
           <span class="separator">/</span>
-          <gr-button link on-tap="_collapseAllDiffs">Hide diffs</gr-button>
+          <gr-button link on-tap="collapseAllDiffs">Hide diffs</gr-button>
         </template>
         <template is="dom-if"
             if="[[!_fileListActionsVisible(_shownFiles.*, _maxFilesForBulkActions)]]">

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

@@ -162,7 +162,7 @@
 
       this._loading = true;
 
-      this._collapseAllDiffs();
+      this.collapseAllDiffs();
       const promises = [];
 
       promises.push(this._getFiles().then(files => {
@@ -304,7 +304,7 @@
       this.splice(...['_expandedFilePaths', 0, 0].concat(newPaths));
     },
 
-    _collapseAllDiffs() {
+    collapseAllDiffs() {
       this._showInlineDiffs = false;
       this._expandedFilePaths = [];
       this.$.diffCursor.handleDiffUpdate();
@@ -640,7 +640,7 @@
 
     _toggleInlineDiffs() {
       if (this._showInlineDiffs) {
-        this._collapseAllDiffs();
+        this.collapseAllDiffs();
       } else {
         this._expandAllDiffs();
       }

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

@@ -859,6 +859,24 @@ limitations under the License.
       assert.notInclude(element._expandedFilePaths, path);
     });
 
+    test('collapseAllDiffs', () => {
+      sandbox.stub(element, '_renderInOrder')
+          .returns(Promise.resolve());
+      const cursorUpdateStub = sandbox.stub(element.$.diffCursor,
+          'handleDiffUpdate');
+
+      const path = 'path/to/my/file.txt';
+      element.files = [{__path: path}];
+      element._expandedFilePaths = [path];
+      element._showInlineDiffs = true;
+
+      element.collapseAllDiffs();
+      flushAsynchronousOperations();
+      assert.equal(element._expandedFilePaths.length, 0);
+      assert.isFalse(element._showInlineDiffs);
+      assert.isTrue(cursorUpdateStub.calledOnce);
+    });
+
     test('_expandedPathsChanged', done => {
       sandbox.stub(element, '_reviewFile');
       const path = 'path/to/my/file.txt';

polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-etag-decorator.js

@@ -55,7 +55,16 @@
   };
 
   GrEtagDecorator.prototype.getCachedPayload = function(url) {
-    return this._payloadCache.get(url);
+    let payload = this._payloadCache.get(url);
+
+    if (typeof payload === 'object') {
+      // Note: For the sake of cache transparency, deep clone the response
+      // object so that cache hits are not equal object references. Some code
+      // expects every network response to deserialize to a fresh object.
+      payload = JSON.parse(JSON.stringify(payload));
+    }
+
+    return payload;
   };
 
   GrEtagDecorator.prototype._truncateCache = function() {

polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-etag-decorator_test.html

@@ -84,7 +84,7 @@ limitations under the License.
     });
 
     test('getCachedPayload', () => {
-      const payload = {};
+      const payload = 'payload';
       etag.collect('/foo', fakeRequest('bar'), payload);
       assert.strictEqual(etag.getCachedPayload('/foo'), payload);
       etag.collect('/foo', fakeRequest('bar', 304), 'garbage');
@@ -92,5 +92,25 @@ limitations under the License.
       etag.collect('/foo', fakeRequest('bar', 200), 'new payload');
       assert.strictEqual(etag.getCachedPayload('/foo'), 'new payload');
     });
+
+    test('getCachedPayload does not preserve object equality', () => {
+      const payload = {foo: 'bar'};
+      etag.collect('/foo', fakeRequest('bar'), payload);
+      assert.deepEqual(etag.getCachedPayload('/foo'), payload);
+      assert.notStrictEqual(etag.getCachedPayload('/foo'), payload);
+      etag.collect('/foo', fakeRequest('bar', 304), {foo: 'baz'});
+      assert.deepEqual(etag.getCachedPayload('/foo'), payload);
+      assert.notStrictEqual(etag.getCachedPayload('/foo'), payload);
+      etag.collect('/foo', fakeRequest('bar', 200), {foo: 'bar baz'});
+      assert.deepEqual(etag.getCachedPayload('/foo'), {foo: 'bar baz'});
+      assert.notStrictEqual(etag.getCachedPayload('/foo'), {foo: 'bar baz'});
+    });
+
+    test('getCachedPayload clones the response deeply', () => {
+      const payload = {foo: {bar: 'baz'}};
+      etag.collect('/foo', fakeRequest('bar'), payload);
+      assert.deepEqual(etag.getCachedPayload('/foo'), payload);
+      assert.notStrictEqual(etag.getCachedPayload('/foo').foo, payload.foo);
+    });
   });
 </script>