Don't show binary image content & fix image label

Previously there were times when binary data from an image diff would display. This change adds a check in the diff processor for image diffs and does not display anything other than the file line in that case. This change also addresses an issue where the label is calculated too soon. The image size (if it exists) is supposed to be included as part of the label, but often this was calculated before the image was done rendering, so it didn't display. Bug: Issue 5887 Change-Id: I9cd1ad0c3f2603492d7d84892147bd6852bbae29
2017-03-29 17:29:17 -07:00
parent 29e1ce5e84
commit 7fe6a3b4cc
6 changed files with 285 additions and 100 deletions
--- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-image.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-image.js
@@ -41,44 +41,89 @@
    var tr = this._createElement('tr');

    tr.appendChild(this._createElement('td'));
-    tr.appendChild(this._createImageCell(this._baseImage, 'left'));
+    tr.appendChild(this._createImageCell(this._baseImage, 'left', section));

    tr.appendChild(this._createElement('td'));
-    tr.appendChild(this._createImageCell(this._revisionImage, 'right'));
+    tr.appendChild(this._createImageCell(
+          this._revisionImage, 'right', section));

    section.appendChild(tr);
  };

-  GrDiffBuilderImage.prototype._createImageCell = function(image, className) {
+  GrDiffBuilderImage.prototype._createImageCell =
+      function(image, className, section) {
    var td = this._createElement('td', className);
    if (image) {
      var imageEl = this._createElement('img');
+      imageEl.onload = function() {
+        image._height = imageEl.naturalHeight;
+        image._width = imageEl.naturalWidth;
+        this._updateImageLabel(section, className, image);
+      }.bind(this);
      imageEl.src = 'data:' + image.type + ';base64, ' + image.body;
-      image._height = imageEl.naturalHeight;
-      image._width = imageEl.naturalWidth;
      imageEl.addEventListener('error', function(e) {
        imageEl.remove();
        td.textContent = '[Image failed to load]';
      });
      td.appendChild(imageEl);
-    }
    return td;
+    }
+  };
+
+  GrDiffBuilderImage.prototype._updateImageLabel =
+      function(section, className, image) {
+    var label = Polymer.dom(section)
+        .querySelector('.' + className + ' span.label');
+    this._setLabelText(label, image);
+  };
+
+  GrDiffBuilderImage.prototype._setLabelText = function(label, image) {
+    label.textContent = this._getImageLabel(image);
  };

  GrDiffBuilderImage.prototype._emitImageLabels = function(section) {
    var tr = this._createElement('tr');

+    var addNamesInLabel = false;
+
+    if (this._baseImage._name !== this._revisionImage._name) {
+      addNamesInLabel = true;
+    }
+
    tr.appendChild(this._createElement('td'));
    var td = this._createElement('td', 'left');
    var label = this._createElement('label');
-    label.textContent = this._getImageLabel(this._baseImage);
+    var nameSpan;
+    var labelSpan = this._createElement('span', 'label');
+
+    if (addNamesInLabel) {
+      nameSpan = this._createElement('span', 'name');
+      nameSpan.textContent = this._baseImage._name;
+      label.appendChild(nameSpan);
+      label.appendChild(this._createElement('br'));
+    }
+
+    this._setLabelText(labelSpan, this._baseImage, addNamesInLabel);
+
+    label.appendChild(labelSpan);
    td.appendChild(label);
    tr.appendChild(td);

    tr.appendChild(this._createElement('td'));
    td = this._createElement('td', 'right');
    label = this._createElement('label');
-    label.textContent = this._getImageLabel(this._revisionImage);
+    labelSpan = this._createElement('span', 'label');
+
+    if (addNamesInLabel) {
+      nameSpan = this._createElement('span', 'name');
+      nameSpan.textContent = this._revisionImage._name;
+      label.appendChild(nameSpan);
+      label.appendChild(this._createElement('br'));
+    }
+
+    this._setLabelText(labelSpan, this._revisionImage, addNamesInLabel);
+
+    label.appendChild(labelSpan);
    td.appendChild(label);
    tr.appendChild(td);

--- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html
@@ -145,7 +145,8 @@ limitations under the License.
          reporting.time(TimingLabel.TOTAL);
          reporting.time(TimingLabel.CONTENT);
          this.dispatchEvent(new CustomEvent('render-start', {bubbles: true}));
-          return this.$.processor.process(this.diff.content).then(function() {
+          return this.$.processor.process(this.diff.content, this.isImageDiff)
+              .then(function() {
            if (this.isImageDiff) {
              this._builder.renderDiffImages();
            }
--- a/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor.js
@@ -103,11 +103,14 @@
     * @return {Promise} A promise that resolves when the diff is completely
     *     processed.
     */
-    process: function(content) {
-      return new Promise(function(resolve) {
-        this.groups = [];
-        this.push('groups', this._makeFileComments());
+    process: function(content, isImageDiff) {
+      this.groups = [];
+      this.push('groups', this._makeFileComments());

+      // If image diff, only render the file lines.
+      if (isImageDiff) { return Promise.resolve(); }
+
+      return new Promise(function(resolve) {
        var state = {
          lineNums: {left: 0, right: 0},
          sectionIndex: 0,
@@ -117,7 +120,6 @@

        var currentBatch = 0;
        var nextStep = function() {
-
          if (this._isScrolling) {
            this.async(nextStep, 100);
            return;
--- a/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor_test.html
@@ -459,6 +459,23 @@ limitations under the License.
        assert.equal(element.groups.length, 33);
      });

+      test('image diffs', function() {
+        var contentRow = {
+          ab: [
+            '<!DOCTYPE html>',
+            '<meta charset="utf-8">',
+          ]
+        };
+        var content = _.times(200, _.constant(contentRow));
+        sandbox.stub(element, 'async');
+        element.process(content, true);
+        assert.equal(element.groups.length, 1);
+
+        // Image diffs don't process content, just the 'FILE' line.
+        assert.equal(element.groups[0].lines.length, 1);
+      });
+
+
      suite('gr-diff-processor helpers', function() {
        var rows;

--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html
@@ -286,101 +286,219 @@ limitations under the License.
        assert.equal(threadLength, 2);
      });

-      test('renders image diffs', function(done) {
-        var mockDiff = {
-          meta_a: {name: 'carrot.jpg', content_type: 'image/jpeg', lines: 66},
-          meta_b: {name: 'carrot.jpg', content_type: 'image/jpeg', lines: 560},
-          intraline_status: 'OK',
-          change_type: 'MODIFIED',
-          diff_header: [
-            'diff --git a/carrot.jpg b/carrot.jpg',
-            'index 2adc47d..f9c2f2c 100644',
-            '--- a/carrot.jpg',
-            '+++ b/carrot.jpg',
-            'Binary files differ',
-          ],
-          content: [{skip: 66}],
-          binary: true,
-        };
-        var mockFile1 = {
-          body: 'Qk06AAAAAAAAADYAAAAoAAAAAQAAAP////8BACAAAAAAAAAAAAATCwAAEwsA' +
-              'AAAAAAAAAAAAAAAA/w==',
-          type: 'image/bmp',
-        };
-        var mockFile2 = {
-          body: 'Qk06AAAAAAAAADYAAAAoAAAAAQAAAP////8BACAAAAAAAAAAAAATCwAAEwsA' +
-              'AAAAAAAAAAAA/////w==',
-          type: 'image/bmp'
-        };
-        var mockCommit = {
-          commit: '9a1a1d10baece5efbba10bc4ccf808a67a50ac0a',
-          parents: [{
-            commit: '7338aa9adfe57909f1fdaf88975cdea467d3382f',
-            subject: 'Added a carrot',
-          }],
-          author: {
-            name: 'Wyatt Allen',
-            email: 'wyatta@google.com',
-            date: '2016-05-23 21:44:51.000000000',
-            tz: -420,
-          },
-          committer: {
-            name: 'Wyatt Allen',
-            email: 'wyatta@google.com',
-            date: '2016-05-25 00:25:41.000000000',
-            tz: -420,
-          },
-          subject: 'Updated the carrot',
-          message: 'Updated the carrot\n\nChange-Id: Iabcd123\n',
-        };
-        var mockComments = {baseComments: [], comments: []};
-
+      suite('image diffs', function() {
+        var mockFile1;
+        var mockFile2;
        var stubs = [];
-        stubs.push(sandbox.stub(element, '_getDiff',
-            function() { return Promise.resolve(mockDiff); }));
-        stubs.push(sandbox.stub(element.$.restAPI, 'getCommitInfo',
-            function() { return Promise.resolve(mockCommit); }));
-        stubs.push(sandbox.stub(element.$.restAPI,
-            'getCommitFileContents',
-            function() { return Promise.resolve(mockFile1); }));
-        stubs.push(sandbox.stub(element.$.restAPI,
-            'getChangeFileContents',
-            function() { return Promise.resolve(mockFile2); }));
-        stubs.push(sandbox.stub(element.$.restAPI, '_getDiffComments',
-            function() { return Promise.resolve(mockComments); }));
-        stubs.push(sandbox.stub(element.$.restAPI, 'getDiffDrafts',
-            function() { return Promise.resolve(mockComments); }));
+        setup(function() {
+          mockFile1 = {
+            body: 'Qk06AAAAAAAAADYAAAAoAAAAAQAAAP////8BACAAAAAAAAAAAAATCwAAE' +
+            'wsAAAAAAAAAAAAAAAAA/w==',
+            type: 'image/bmp',
+          };
+          mockFile2 = {
+            body: 'Qk06AAAAAAAAADYAAAAoAAAAAQAAAP////8BACAAAAAAAAAAAAATCwAAE' +
+            'wsAAAAAAAAAAAAA/////w==',
+            type: 'image/bmp'
+          };
+          var mockCommit = {
+            commit: '9a1a1d10baece5efbba10bc4ccf808a67a50ac0a',
+            parents: [{
+              commit: '7338aa9adfe57909f1fdaf88975cdea467d3382f',
+              subject: 'Added a carrot',
+            }],
+            author: {
+              name: 'Wyatt Allen',
+              email: 'wyatta@google.com',
+              date: '2016-05-23 21:44:51.000000000',
+              tz: -420,
+            },
+            committer: {
+              name: 'Wyatt Allen',
+              email: 'wyatta@google.com',
+              date: '2016-05-25 00:25:41.000000000',
+              tz: -420,
+            },
+            subject: 'Updated the carrot',
+            message: 'Updated the carrot\n\nChange-Id: Iabcd123\n',
+          };
+          var mockComments = {baseComments: [], comments: []};

-        element.patchRange = {basePatchNum: 'PARENT', patchNum: 1};
+          stubs.push(sandbox.stub(element.$.restAPI, 'getCommitInfo',
+              function() { return Promise.resolve(mockCommit); }));
+          stubs.push(sandbox.stub(element.$.restAPI,
+              'getCommitFileContents',
+              function() { return Promise.resolve(mockFile1); }));
+          stubs.push(sandbox.stub(element.$.restAPI,
+              'getChangeFileContents',
+              function() { return Promise.resolve(mockFile2); }));
+          stubs.push(sandbox.stub(element.$.restAPI, '_getDiffComments',
+              function() { return Promise.resolve(mockComments); }));
+          stubs.push(sandbox.stub(element.$.restAPI, 'getDiffDrafts',
+              function() { return Promise.resolve(mockComments); }));

-        var rendered = function() {
-          // Recognizes that it should be an image diff.
-          assert.isTrue(element.isImageDiff);
-          assert.instanceOf(element.$.diffBuilder._builder, GrDiffBuilderImage);
+          element.patchRange = {basePatchNum: 'PARENT', patchNum: 1};

-          // Left image rendered with the parent commit's version of the file.
-          var leftInmage = element.$.diffTable.querySelector('td.left img');
-          assert.isOk(leftInmage);
-          assert.equal(leftInmage.getAttribute('src'),
-              'data:image/bmp;base64, ' + mockFile1.body);
+        });

-          // Right image rendered with this change's revision of the image.
-          var rightInmage = element.$.diffTable.querySelector('td.right img');
-          assert.isOk(rightInmage);
-          assert.equal(rightInmage.getAttribute('src'),
-              'data:image/bmp;base64, ' + mockFile2.body);
+        test('renders image diffs with same file name', function(done) {
+          var mockDiff = {
+            meta_a: {name: 'carrot.jpg', content_type: 'image/jpeg', lines: 66},
+            meta_b: {name: 'carrot.jpg', content_type: 'image/jpeg',
+                lines: 560},
+            intraline_status: 'OK',
+            change_type: 'MODIFIED',
+            diff_header: [
+              'diff --git a/carrot.jpg b/carrot.jpg',
+              'index 2adc47d..f9c2f2c 100644',
+              '--- a/carrot.jpg',
+              '+++ b/carrot.jpg',
+              'Binary files differ',
+            ],
+            content: [{skip: 66}],
+            binary: true,
+          };
+          stubs.push(sandbox.stub(element, '_getDiff',
+              function() { return Promise.resolve(mockDiff); }));

-          // Cleanup.
-          element.removeEventListener('render', rendered);
+          var rendered = function() {
+            // Recognizes that it should be an image diff.
+            assert.isTrue(element.isImageDiff);
+            assert.instanceOf(
+                  element.$.diffBuilder._builder, GrDiffBuilderImage);

-          done();
-        };
+            // Left image rendered with the parent commit's version of the file.
+            var leftImage = element.$.diffTable.querySelector('td.left img');
+            var leftLabel = element.$.diffTable.querySelector('td.left label');
+            var leftLabelContent = leftLabel.querySelector('.label');
+            var leftLabelName = leftLabel.querySelector('.name');

-        element.addEventListener('render', rendered);
+            var rightImage = element.$.diffTable.querySelector('td.right img');
+            var rightLabel = element.$.diffTable.querySelector(
+                  'td.right label');
+            var rightLabelContent = rightLabel.querySelector('.label');
+            var rightLabelName = rightLabel.querySelector('.name');

-        element.$.restAPI.getDiffPreferences().then(function(prefs) {
-          element.prefs = prefs;
-          element.reload();
+            assert.isNotOk(rightLabelName);
+            assert.isNotOk(leftLabelName);
+
+            var leftLoaded = false;
+            var rightLoaded = false;
+
+            leftImage.addEventListener('load', function() {
+              assert.isOk(leftImage);
+              assert.equal(leftImage.getAttribute('src'),
+                  'data:image/bmp;base64, ' + mockFile1.body);
+              assert.equal(leftLabelContent.textContent, '1⨉1 image/bmp');
+              leftLoaded = true;
+              if (rightLoaded) {
+                element.removeEventListener('render', rendered);
+                done();
+              }
+            });
+
+            rightImage.addEventListener('load', function() {
+              assert.isOk(rightImage);
+              assert.equal(rightImage.getAttribute('src'),
+                  'data:image/bmp;base64, ' + mockFile2.body);
+              assert.equal(rightLabelContent.textContent, '1⨉1 image/bmp');
+
+              rightLoaded = true;
+              if (leftLoaded) {
+                element.removeEventListener('render', rendered);
+                done();
+              }
+            });
+
+          };
+
+          element.addEventListener('render', rendered);
+
+          element.$.restAPI.getDiffPreferences().then(function(prefs) {
+            element.prefs = prefs;
+            element.reload();
+          });
+        });
+
+        test('renders image diffs with a different file name', function(done) {
+          var mockDiff = {
+            meta_a: {name: 'carrot.jpg', content_type: 'image/jpeg', lines: 66},
+            meta_b: {name: 'carrot2.jpg', content_type: 'image/jpeg',
+                lines: 560},
+            intraline_status: 'OK',
+            change_type: 'MODIFIED',
+            diff_header: [
+              'diff --git a/carrot.jpg b/carrot2.jpg',
+              'index 2adc47d..f9c2f2c 100644',
+              '--- a/carrot.jpg',
+              '+++ b/carrot2.jpg',
+              'Binary files differ',
+            ],
+            content: [{skip: 66}],
+            binary: true,
+          };
+          stubs.push(sandbox.stub(element, '_getDiff',
+              function() { return Promise.resolve(mockDiff); }));
+
+          var rendered = function() {
+            // Recognizes that it should be an image diff.
+            assert.isTrue(element.isImageDiff);
+            assert.instanceOf(
+                  element.$.diffBuilder._builder, GrDiffBuilderImage);
+
+            // Left image rendered with the parent commit's version of the file.
+            var leftImage = element.$.diffTable.querySelector('td.left img');
+            var leftLabel = element.$.diffTable.querySelector('td.left label');
+            var leftLabelContent = leftLabel.querySelector('.label');
+            var leftLabelName = leftLabel.querySelector('.name');
+
+            var rightImage = element.$.diffTable.querySelector('td.right img');
+            var rightLabel = element.$.diffTable.querySelector(
+                  'td.right label');
+            var rightLabelContent = rightLabel.querySelector('.label');
+            var rightLabelName = rightLabel.querySelector('.name');
+
+            assert.isOk(rightLabelName);
+            assert.isOk(leftLabelName);
+            assert.equal(leftLabelName.textContent, mockDiff.meta_a.name);
+            assert.equal(rightLabelName.textContent, mockDiff.meta_b.name);
+
+            var leftLoaded = false;
+            var rightLoaded = false;
+
+            leftImage.addEventListener('load', function() {
+              assert.isOk(leftImage);
+              assert.equal(leftImage.getAttribute('src'),
+                  'data:image/bmp;base64, ' + mockFile1.body);
+              assert.equal(leftLabelContent.textContent, '1⨉1 image/bmp');
+              leftLoaded = true;
+              if (rightLoaded) {
+                element.removeEventListener('render', rendered);
+                done();
+              }
+            });
+
+            rightImage.addEventListener('load', function() {
+              assert.isOk(rightImage);
+              assert.equal(rightImage.getAttribute('src'),
+                  'data:image/bmp;base64, ' + mockFile2.body);
+              assert.equal(rightLabelContent.textContent, '1⨉1 image/bmp');
+
+              rightLoaded = true;
+              if (leftLoaded) {
+                element.removeEventListener('render', rendered);
+                done();
+              }
+            });
+
+          };
+
+          element.addEventListener('render', rendered);
+
+          element.$.restAPI.getDiffPreferences().then(function(prefs) {
+            element.prefs = prefs;
+            element.reload();
+          });
        });
      });

--- 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
@@ -971,9 +971,11 @@
          // Sometimes the server doesn't send back the content type.
          if (baseImage) {
            baseImage._expectedType = diff.meta_a.content_type;
+            baseImage._name = diff.meta_a.name;
          }
          if (revisionImage) {
            revisionImage._expectedType = diff.meta_b.content_type;
+            revisionImage._name = diff.meta_b.name;
          }

          return {baseImage: baseImage, revisionImage: revisionImage};