Merge "Harden gr-formatted-text agains slow/failed project configs"

2017-08-02 20:38:32 +00:00
parent 299ae16f0c f0eb4bbf53
commit 43cfea89e0
17 changed files with 71 additions and 34 deletions
--- 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
@@ -338,6 +338,7 @@ limitations under the License.
              patch-range="[[patchRange]]"
              path="[[file.__path]]"
              prefs="[[diffPrefs]]"
+              project-name="[[change.project]]"
              project-config="[[projectConfig]]"
              on-line-selected="_onLineSelected"
              no-render-on-prefs-change
--- 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
@@ -19,9 +19,10 @@

  const IMAGE_MIME_PATTERN = /^image\/(bmp|gif|jpeg|jpg|png|tiff|webp)$/;

-  function GrDiffBuilderImage(diff, comments, prefs, outputEl, baseImage,
-      revisionImage) {
-    GrDiffBuilderSideBySide.call(this, diff, comments, prefs, outputEl, []);
+  function GrDiffBuilderImage(
+      diff, comments, prefs, projectName, outputEl, baseImage, revisionImage) {
+    GrDiffBuilderSideBySide.call(
+        this, diff, comments, prefs, projectName, outputEl, []);
    this._baseImage = baseImage;
    this._revisionImage = revisionImage;
  }
--- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-side-by-side.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-side-by-side.js
@@ -17,8 +17,10 @@
  // Prevent redefinition.
  if (window.GrDiffBuilderSideBySide) { return; }

-  function GrDiffBuilderSideBySide(diff, comments, prefs, outputEl, layers) {
-    GrDiffBuilder.call(this, diff, comments, prefs, outputEl, layers);
+  function GrDiffBuilderSideBySide(
+      diff, comments, prefs, projectName, outputEl, layers) {
+    GrDiffBuilder.call(
+        this, diff, comments, prefs, projectName, outputEl, layers);
  }
  GrDiffBuilderSideBySide.prototype = Object.create(GrDiffBuilder.prototype);
  GrDiffBuilderSideBySide.prototype.constructor = GrDiffBuilderSideBySide;
--- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-unified.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-unified.js
@@ -17,8 +17,10 @@
  // Prevent redefinition.
  if (window.GrDiffBuilderUnified) { return; }

-  function GrDiffBuilderUnified(diff, comments, prefs, outputEl, layers) {
-    GrDiffBuilder.call(this, diff, comments, prefs, outputEl, layers);
+  function GrDiffBuilderUnified(
+      diff, comments, prefs, projectName, outputEl, layers) {
+    GrDiffBuilder.call(
+        this, diff, comments, prefs, projectName, outputEl, layers);
  }
  GrDiffBuilderUnified.prototype = Object.create(GrDiffBuilder.prototype);
  GrDiffBuilderUnified.prototype.constructor = GrDiffBuilderUnified;
--- 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
@@ -94,6 +94,7 @@ limitations under the License.
          isImageDiff: Boolean,
          baseImage: Object,
          revisionImage: Object,
+          projectName: String,
          _builder: Object,
          _groups: Array,
          _layers: Array,
@@ -219,9 +220,9 @@ limitations under the License.
        },

        createCommentThreadGroup(changeNum, patchNum, path,
-            isOnParent, commentSide, projectConfig) {
+            isOnParent, commentSide) {
          return this._builder.createCommentThreadGroup(changeNum, patchNum,
-              path, isOnParent, commentSide, projectConfig);
+              path, isOnParent, commentSide);
        },

        emitGroup(group, sectionEl) {
@@ -252,13 +253,14 @@ limitations under the License.
        _getDiffBuilder(diff, comments, prefs) {
          if (this.isImageDiff) {
            return new GrDiffBuilderImage(diff, comments, prefs,
-            this.diffElement, this.baseImage, this.revisionImage);
+                this.projectName, this.diffElement, this.baseImage,
+                this.revisionImage);
          } else if (this.viewMode === DiffViewMode.SIDE_BY_SIDE) {
-            return new GrDiffBuilderSideBySide(
-            diff, comments, prefs, this.diffElement, this._layers);
+            return new GrDiffBuilderSideBySide(diff, comments, prefs,
+                this.projectName, this.diffElement, this._layers);
          } else if (this.viewMode === DiffViewMode.UNIFIED) {
-            return new GrDiffBuilderUnified(
-            diff, comments, prefs, this.diffElement, this._layers);
+            return new GrDiffBuilderUnified(diff, comments, prefs,
+                this.projectName, this.diffElement, this._layers);
          }
          throw Error('Unsupported diff view mode: ' + this.viewMode);
        },
--- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js
@@ -30,10 +30,11 @@

  const REGEX_ASTRAL_SYMBOL = /[\uD800-\uDBFF][\uDC00-\uDFFF]/;

-  function GrDiffBuilder(diff, comments, prefs, outputEl, layers) {
+  function GrDiffBuilder(diff, comments, prefs, projectName, outputEl, layers) {
    this._diff = diff;
    this._comments = comments;
    this._prefs = prefs;
+    this._projectName = projectName;
    this._outputEl = outputEl;
    this.groups = [];

@@ -336,14 +337,14 @@
  };

  GrDiffBuilder.prototype.createCommentThreadGroup = function(changeNum,
-      patchNum, path, isOnParent, projectConfig, range) {
+      patchNum, path, isOnParent, range) {
    const threadGroupEl =
        document.createElement('gr-diff-comment-thread-group');
    threadGroupEl.changeNum = changeNum;
    threadGroupEl.patchForNewThreads = patchNum;
    threadGroupEl.path = path;
    threadGroupEl.isOnParent = isOnParent;
-    threadGroupEl.projectConfig = projectConfig;
+    threadGroupEl.projectName = this._projectName;
    threadGroupEl.range = range;
    return threadGroupEl;
  };
@@ -370,8 +371,7 @@
        this._comments.meta.changeNum,
        patchNum,
        this._comments.meta.path,
-        isOnParent,
-        this._comments.meta.projectConfig);
+        isOnParent);
    threadGroupEl.comments = comments;
    if (opt_side) {
      threadGroupEl.setAttribute('data-side', opt_side);
--- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html
@@ -62,13 +62,16 @@ limitations under the License.
    setup(() => {
      stub('gr-rest-api-interface', {
        getLoggedIn() { return Promise.resolve(false); },
+        getProjectConfig() { return Promise.resolve({}); },
      });
      const prefs = {
        line_length: 10,
        show_tabs: true,
        tab_size: 4,
      };
-      builder = new GrDiffBuilder({content: []}, {left: [], right: []}, prefs);
+      const projectName = 'my-project';
+      builder = new GrDiffBuilder(
+          {content: []}, {left: [], right: []}, prefs, projectName);
    });

    test('context control buttons', () => {
@@ -265,7 +268,7 @@ limitations under the License.
        assert.equal(threadGroupEl.patchForNewThreads, patchNum);
        assert.equal(threadGroupEl.path, '/path/to/foo');
        assert.equal(threadGroupEl.isOnParent, isOnParent);
-        assert.deepEqual(threadGroupEl.projectConfig, {foo: 'bar'});
+        assert.deepEqual(threadGroupEl.projectName, 'my-project');
        assert.deepEqual(threadGroupEl.comments, comments);
      }

@@ -761,7 +764,7 @@ limitations under the License.
        outputEl = element.queryEffectiveChildren('#diffTable');
        sandbox.stub(element, '_getDiffBuilder', () => {
          const builder = new GrDiffBuilder(
-              {content}, {left: [], right: []}, prefs, outputEl);
+              {content}, {left: [], right: []}, prefs, 'my-project', outputEl);
          sandbox.stub(builder, 'addColumns');
          builder.buildSectionElement = function(group) {
            const section = document.createElement('stub');
--- a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.html
@@ -38,7 +38,7 @@ limitations under the License.
          location-range="[[thread.locationRange]]"
          patch-num="[[thread.patchNum]]"
          path="[[path]]"
-          project-config="[[projectConfig]]"></gr-diff-comment-thread>
+          project-name="[[projectName]]"></gr-diff-comment-thread>
    </template>
  </template>
  <script src="gr-diff-comment-thread-group.js"></script>
--- a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.js
@@ -23,8 +23,8 @@
        type: Array,
        value() { return []; },
      },
+      projectName: String,
      patchForNewThreads: String,
-      projectConfig: Object,
      range: Object,
      isOnParent: {
        type: Boolean,
--- a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.html
@@ -61,7 +61,7 @@ limitations under the License.
            show-actions="[[_showActions]]"
            comment-side="[[comment.__commentSide]]"
            side="[[comment.side]]"
-            project-config="[[projectConfig]]"
+            project-config="[[_projectConfig]]"
            on-create-fix-comment="_handleCommentFix"
            on-comment-discard="_handleCommentDiscard"></gr-diff-comment>
      </template>
--- a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.js
@@ -40,7 +40,10 @@
      commentSide: String,
      patchNum: String,
      path: String,
-      projectConfig: Object,
+      projectName: {
+        type: String,
+        observer: '_projectNameChanged',
+      },
      isOnParent: {
        type: Boolean,
        value: false,
@@ -53,6 +56,7 @@
        type: Boolean,
        notify: true,
      },
+      _projectConfig: Object,
    },

    behaviors: [
@@ -352,5 +356,16 @@
    _computeHostClass(unresolved) {
      return unresolved ? 'unresolved' : '';
    },
+
+    /**
+     * Load the project config when a project name has been provided.
+     * @param {string} name The project name.
+     */
+    _projectNameChanged(name) {
+      if (!name) { return; }
+      this.$.restAPI.getProjectConfig(name).then(config => {
+        this._projectConfig = config;
+      });
+    },
  });
 })();
--- a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread_test.html
@@ -161,6 +161,17 @@ limitations under the License.
      lastComment.__draft = true;
      assert.equal(element._hideActions(showActions, lastComment), true);
    });
+
+    test('setting project name loads the project config', done => {
+      const projectName = 'foo/bar/baz';
+      const getProjectStub = sandbox.stub(element.$.restAPI, 'getProjectConfig')
+          .returns(Promise.resolve({}));
+      element.projectName = projectName;
+      flush(() => {
+        assert.isTrue(getProjectStub.calledWithExactly(projectName));
+        done();
+      });
+    });
  });

  suite('comment action tests', () => {
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html
@@ -328,6 +328,7 @@ limitations under the License.
        path="[[_path]]"
        prefs="[[_prefs]]"
        project-config="[[_projectConfig]]"
+        project-name="[[_change.project]]"
        view-mode="[[_diffMode]]"
        on-line-selected="_onLineSelected">
    </gr-diff>
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
@@ -229,6 +229,7 @@ limitations under the License.
          <gr-diff-builder
              id="diffBuilder"
              comments="[[comments]]"
+              project-name="[[projectName]]"
              diff="[[_diff]]"
              diff-path="[[path]]"
              view-mode="[[viewMode]]"
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
@@ -58,6 +58,7 @@
        type: Object,
        observer: '_projectConfigChanged',
      },
+      projectName: String,
      displayLine: {
        type: Boolean,
        value: false,
@@ -305,8 +306,7 @@
      let threadGroupEl = this._getThreadGroupForLine(contentEl);
      if (!threadGroupEl) {
        threadGroupEl = this.$.diffBuilder.createCommentThreadGroup(
-            this.changeNum, patchNum, this.path, isOnParent,
-            this.projectConfig);
+            this.changeNum, patchNum, this.path, isOnParent);
        contentEl.appendChild(threadGroupEl);
      }

--- a/polygerrit-ui/app/elements/shared/gr-formatted-text/gr-formatted-text.js
+++ b/polygerrit-ui/app/elements/shared/gr-formatted-text/gr-formatted-text.js
@@ -60,7 +60,7 @@
      // request for it still being in flight), set the content anyway to
      // prevent waiting on the config to display the text.
      if (this.config) { return; }
-      this.$.container.textContent = content;
+      this._contentOrConfigChanged(content);
    },

    /**
--- a/polygerrit-ui/app/elements/shared/gr-formatted-text/gr-formatted-text_test.html
+++ b/polygerrit-ui/app/elements/shared/gr-formatted-text/gr-formatted-text_test.html
@@ -363,12 +363,10 @@ limitations under the License.
      assert.equal(result, expected);
    });

-    test('_contentOrConfigChanged not called without config', () => {
-      const contentStub = sandbox.stub(element, '_contentChanged');
-      const contentConfigStub = sandbox.stub(element, '_contentOrConfigChanged');
+    test('_computeNodes called without config', () => {
+      const computeNodesSpy = sandbox.spy(element, '_computeNodes');
      element.content = 'some text';
-      assert.isTrue(contentStub.called);
-      assert.isFalse(contentConfigStub.called);
+      assert.isTrue(computeNodesSpy.called);
    });

    test('_contentOrConfigChanged called with config', () => {