Merge "Revert "Lift gr-syntax-layer into gr-diff-host""

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

@@ -20,6 +20,7 @@ limitations under the License.
 <link rel="import" href="../gr-coverage-layer/gr-coverage-layer.html">
 <link rel="import" href="../gr-diff-processor/gr-diff-processor.html">
 <link rel="import" href="../gr-ranged-comment-layer/gr-ranged-comment-layer.html">
+<link rel="import" href="../gr-syntax-layer/gr-syntax-layer.html">
 
 <dom-module id="gr-diff-builder">
   <template>
@@ -29,6 +30,9 @@ limitations under the License.
     <gr-ranged-comment-layer
         id="rangeLayer"
         comment-ranges="[[commentRanges]]"></gr-ranged-comment-layer>
+    <gr-syntax-layer
+        id="syntaxLayer"
+        diff="[[diff]]"></gr-syntax-layer>
     <gr-coverage-layer
         id="coverageLayerLeft"
         coverage-ranges="[[_leftCoverageRanges]]"
@@ -60,6 +64,13 @@ limitations under the License.
         UNIFIED: 'UNIFIED_DIFF',
       };
 
+      // If any line of the diff is more than the character limit, then disable
+      // syntax highlighting for the entire file.
+      const SYNTAX_MAX_LINE_LENGTH = 500;
+
+      // Disable syntax highlighting if the overall diff is too large.
+      const SYNTAX_MAX_DIFF_LENGTH = 20000;
+
       const TRAILING_WHITESPACE_PATTERN = /\s+$/;
 
       Polymer({
@@ -73,11 +84,18 @@ limitations under the License.
          */
 
         /**
-         * Fired when the diff finishes rendering text content.
+         * Fired when the diff finishes rendering text content and starts
+         * syntax highlighting.
          *
          * @event render-content
          */
 
+        /**
+         * Fired when the diff finishes syntax highlighting.
+         *
+         * @event render-syntax
+         */
+
         properties: {
           diff: Object,
           diffPath: String,
@@ -120,7 +138,7 @@ limitations under the License.
            * @type {?Object}
            */
           _cancelableRenderPromise: Object,
-          layers: {
+          pluginLayers: {
             type: Array,
             value: [],
           },
@@ -153,10 +171,11 @@ limitations under the License.
           // attached before plugins are installed.
           this._setupAnnotationLayers();
 
+          this.$.syntaxLayer.enabled = prefs.syntax_highlighting;
           this._showTabs = !!prefs.show_tabs;
           this._showTrailingWhitespace = !!prefs.show_whitespace_errors;
 
-          // Stop the processor if it's running.
+          // Stop the processor and syntax layer (if they're running).
           this.cancel();
 
           this._builder = this._getDiffBuilder(this.diff, prefs);
@@ -179,6 +198,16 @@ limitations under the License.
                     }
                     this.dispatchEvent(new CustomEvent('render-content',
                         {bubbles: true, composed: true}));
+
+                    if (this._diffTooLargeForSyntax()) {
+                      this.$.syntaxLayer.enabled = false;
+                    }
+
+                    return this.$.syntaxLayer.process();
+                  })
+                  .then(() => {
+                    this.dispatchEvent(new CustomEvent(
+                        'render-syntax', {bubbles: true, composed: true}));
                   }));
           return this._cancelableRenderPromise
               .finally(() => { this._cancelableRenderPromise = null; })
@@ -191,6 +220,7 @@ limitations under the License.
         _setupAnnotationLayers() {
           const layers = [
             this._createTrailingWhitespaceLayer(),
+            this.$.syntaxLayer,
             this._createIntralineLayer(),
             this._createTabIndicatorLayer(),
             this.$.rangeLayer,
@@ -198,8 +228,8 @@ limitations under the License.
             this.$.coverageLayerRight,
           ];
 
-          if (this.layers) {
+          if (this.pluginLayers) {
-            layers.push(...this.layers);
+            layers.push(...this.pluginLayers);
           }
           this._layers = layers;
         },
@@ -277,6 +307,7 @@ limitations under the License.
 
         cancel() {
           this.$.processor.cancel();
+          this.$.syntaxLayer.cancel();
           if (this._cancelableRenderPromise) {
             this._cancelableRenderPromise.cancel();
             this._cancelableRenderPromise = null;
@@ -415,10 +446,44 @@ limitations under the License.
           };
         },
 
+        /**
+         * @return {boolean} whether any of the lines in _groups are longer
+         * than SYNTAX_MAX_LINE_LENGTH.
+         */
+        _anyLineTooLong() {
+          return this._groups.reduce((acc, group) => {
+            return acc || group.lines.reduce((acc, line) => {
+              return acc || line.text.length >= SYNTAX_MAX_LINE_LENGTH;
+            }, false);
+          }, false);
+        },
+
+        _diffTooLargeForSyntax() {
+          return this._anyLineTooLong() ||
+              this.getDiffLength() > SYNTAX_MAX_DIFF_LENGTH;
+        },
+
         setBlame(blame) {
           if (!this._builder || !blame) { return; }
           this._builder.setBlame(blame);
         },
+
+        /**
+         * Get the approximate length of the diff as the sum of the maximum
+         * length of the chunks.
+         * @return {number}
+         */
+        getDiffLength() {
+          return this.diff.content.reduce((sum, sec) => {
+            if (sec.hasOwnProperty('ab')) {
+              return sum + sec.ab.length;
+            } else {
+              return sum + Math.max(
+                  sec.hasOwnProperty('a') ? sec.a.length : 0,
+                  sec.hasOwnProperty('b') ? sec.b.length : 0);
+            }
+          }, 0);
+        },
       });
     })();
   </script>

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

@@ -590,38 +590,38 @@ limitations under the License.
       });
     });
 
-    suite('layers', () => {
+    suite('layers from plugins', () => {
       let element;
       let initialLayersCount;
-      let withLayerCount;
+      let withPluginLayerCount;
       setup(() => {
-        const layers = [];
+        const pluginLayers = [];
         element = fixture('basic');
-        element.layers = layers;
+        element.pluginLayers = pluginLayers;
         element._showTrailingWhitespace = true;
         element._setupAnnotationLayers();
         initialLayersCount = element._layers.length;
       });
 
-      test('no layers', () => {
+      test('no plugin layers', () => {
         element._setupAnnotationLayers();
         assert.equal(element._layers.length, initialLayersCount);
       });
 
-      suite('with layers', () => {
+      suite('with plugin layers', () => {
-        const layers = [{}, {}];
+        const pluginLayers = [{}, {}];
         setup(() => {
           element = fixture('basic');
-          element.layers = layers;
+          element.pluginLayers = pluginLayers;
           element._showTrailingWhitespace = true;
           element._setupAnnotationLayers();
-          withLayerCount = element._layers.length;
+          withPluginLayerCount = element._layers.length;
         });
-        test('with layers', () => {
+        test('with plugin layers', () => {
           element._setupAnnotationLayers();
-          assert.equal(element._layers.length, withLayerCount);
+          assert.equal(element._layers.length, withPluginLayerCount);
-          assert.equal(initialLayersCount + layers.length,
+          assert.equal(initialLayersCount + pluginLayers.length,
-              withLayerCount);
+              withPluginLayerCount);
         });
       });
     });
@@ -733,6 +733,7 @@ limitations under the License.
         element.viewMode = 'SIDE_BY_SIDE';
         processStub = sandbox.stub(element.$.processor, 'process')
             .returns(Promise.resolve());
+        sandbox.stub(element, '_anyLineTooLong').returns(true);
         keyLocations = {left: {}, right: {}};
         prefs = {
           line_length: 10,
@@ -861,14 +862,37 @@ limitations under the License.
               .map(c => { return c.args[0].type; });
           assert.include(firedEventTypes, 'render-start');
           assert.include(firedEventTypes, 'render-content');
+          assert.include(firedEventTypes, 'render-syntax');
+          done();
+        });
+      });
+
+      test('rendering normal-sized diff does not disable syntax', () => {
+        assert.isTrue(element.$.syntaxLayer.enabled);
+      });
+
+      test('rendering large diff disables syntax', done => {
+        // Before it renders, set the first diff line to 500 '*' characters.
+        element.diff.content[0].a = [new Array(501).join('*')];
+        const prefs = {
+          line_length: 10,
+          show_tabs: true,
+          tab_size: 4,
+          context: -1,
+          syntax_highlighting: true,
+        };
+        element.render(keyLocations, prefs).then(() => {
+          assert.isFalse(element.$.syntaxLayer.enabled);
           done();
         });
       });
 
       test('cancel', () => {
         const processorCancelStub = sandbox.stub(element.$.processor, 'cancel');
+        const syntaxCancelStub = sandbox.stub(element.$.syntaxLayer, 'cancel');
         element.cancel();
         assert.isTrue(processorCancelStub.called);
+        assert.isTrue(syntaxCancelStub.called);
       });
     });
 
@@ -897,6 +921,10 @@ limitations under the License.
         });
       });
 
+      test('getDiffLength', () => {
+        assert.equal(element.getDiffLength(diff), 52);
+      });
+
       test('getContentByLine', () => {
         let actual;
 

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

@@ -23,7 +23,6 @@ limitations under the License.
 <link rel="import" href="../../shared/gr-comment-thread/gr-comment-thread.html">
 <link rel="import" href="../../shared/gr-js-api-interface/gr-js-api-interface.html">
 <link rel="import" href="../gr-diff/gr-diff.html">
-<link rel="import" href="../gr-syntax-layer/gr-syntax-layer.html">
 
 <dom-module id="gr-diff-host">
   <template>
@@ -50,13 +49,8 @@ limitations under the License.
         revision-image=[[_revisionImage]]
         coverage-ranges="[[_coverageRanges]]"
         blame="[[_blame]]"
-        layers="[[_layers]]"
+        plugin-layers="[[pluginLayers]]"
-        diff="[[_diff]]">
+        diff="[[_diff]]"></gr-diff>
-    </gr-diff>
-    <gr-syntax-layer
-        id="syntaxLayer"
-        enabled="[[_syntaxHighlightingEnabled]]"
-        diff="[[_diff]]"></gr-syntax-layer>
     <gr-js-api-interface id="jsAPI"></gr-js-api-interface>
     <gr-rest-api-interface id="restAPI"></gr-rest-api-interface>
     <gr-reporting id="reporting" category="diff"></gr-reporting>

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

@@ -35,13 +35,6 @@
     SYNTAX: 'Diff Syntax Render',
   };
 
-  // Disable syntax highlighting if the overall diff is too large.
-  const SYNTAX_MAX_DIFF_LENGTH = 20000;
-
-  // If any line of the diff is more than the character limit, then disable
-  // syntax highlighting for the entire file.
-  const SYNTAX_MAX_LINE_LENGTH = 500;
-
   const WHITESPACE_IGNORE_NONE = 'IGNORE_NONE';
 
   /**
@@ -212,13 +205,7 @@
         computed: '_computeParentIndex(patchRange.*)',
       },
 
-      _syntaxHighlightingEnabled: {
+      pluginLayers: {
-        type: Boolean,
-        computed:
-          '_isSyntaxHighlightingEnabled(prefs.syntax_highlighting, _diff)',
-      },
-
-      _layers: {
         type: Array,
         value: [],
       },
@@ -243,6 +230,7 @@
 
       'render-start': '_handleRenderStart',
       'render-content': '_handleRenderContent',
+      'render-syntax': '_handleRenderSyntax',
 
       'normalize-range': '_handleNormalizeRange',
     },
@@ -270,13 +258,13 @@
       this._errorMessage = null;
       const whitespaceLevel = this._getIgnoreWhitespace();
 
-      const layers = [this.$.syntaxLayer];
+      const pluginLayers = [];
       // Get layers from plugins (if any).
       for (const pluginLayer of this.$.jsAPI.getDiffLayers(
           this.diffPath, this.changeNum, this.patchNum)) {
-        layers.push(pluginLayer);
+        pluginLayers.push(pluginLayer);
       }
-      this._layers = layers;
+      this.push('pluginLayers', ...pluginLayers);
 
       this._coverageRanges = [];
       const {changeNum, path, patchRange: {basePatchNum, patchNum}} = this;
@@ -863,25 +851,6 @@
           item => item.__draftID === comment.__draftID);
     },
 
-    _isSyntaxHighlightingEnabled(preference, diff) {
-      if (!preference) return false;
-      return !this._anyLineTooLong(diff) &&
-          this.$.diff.getDiffLength(diff) <= SYNTAX_MAX_DIFF_LENGTH;
-    },
-
-    /**
-     * @return {boolean} whether any of the lines in diff are longer
-     * than SYNTAX_MAX_LINE_LENGTH.
-     */
-    _anyLineTooLong(diff) {
-      return diff.content.some(section => {
-        const lines = section.ab ?
-              section.ab :
-              (section.a || []).concat(section.b || []);
-        return lines.some(line => line.length >= SYNTAX_MAX_LINE_LENGTH);
-      });
-    },
-
     _handleRenderStart() {
       this.$.reporting.time(TimingLabel.TOTAL);
       this.$.reporting.time(TimingLabel.CONTENT);
@@ -890,10 +859,11 @@
     _handleRenderContent() {
       this.$.reporting.timeEnd(TimingLabel.CONTENT);
       this.$.reporting.time(TimingLabel.SYNTAX);
-      this.$.syntaxLayer.process().then(() => {
+    },
+
+    _handleRenderSyntax() {
       this.$.reporting.timeEnd(TimingLabel.SYNTAX);
       this.$.reporting.timeEnd(TimingLabel.TOTAL);
-      });
     },
 
     _handleNormalizeRange(event) {

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

@@ -60,7 +60,7 @@ limitations under the License.
 
 
     suite('plugin layers', () => {
-      const pluginLayers = [{annotate: () => {}}, {annotate: () => {}}];
+      const pluginLayers = [{}, {}];
       setup(() => {
         stub('gr-js-api-interface', {
           getDiffLayers() { return pluginLayers; },
@@ -303,7 +303,6 @@ limitations under the License.
       });
 
       test('ends content and starts syntax timer on render-content', done => {
-        element._diff = {content: []};
         element.dispatchEvent(
             new CustomEvent('render-content', {bubbles: true, composed: true}));
         assert.isTrue(element.$.reporting.time.calledWithExactly(
@@ -313,12 +312,9 @@ limitations under the License.
         done();
       });
 
-      test('ends total and syntax timer after syntax layer processing', done => {
+      test('ends total and syntax timer on render-syntax', done => {
-        const processed = Promise.resolve();
-        sandbox.stub(element.$.syntaxLayer, 'process').returns(processed);
         element.dispatchEvent(
-            new CustomEvent('render-content', {bubbles: true, composed: true}));
+            new CustomEvent('render-syntax', {bubbles: true, composed: true}));
-        processed.then(() => {
         assert.isTrue(element.$.reporting.timeEnd.calledWithExactly(
             'Diff Total Render'));
         assert.isTrue(element.$.reporting.timeEnd.calledWithExactly(
@@ -326,7 +322,6 @@ limitations under the License.
         done();
       });
     });
-    });
 
     test('reload() cancels before network resolves', () => {
       const cancelStub = sandbox.stub(element.$.diff, 'cancel');
@@ -1290,50 +1285,5 @@ limitations under the License.
       assert.deepEqual(element._filterThreadElsForLocation(threadEls, line,
           Gerrit.DiffSide.RIGHT), [r]);
     });
-
-    suite('syntax layer', () => {
-      setup(() => {
-        const prefs = {
-          line_length: 10,
-          show_tabs: true,
-          tab_size: 4,
-          context: -1,
-          syntax_highlighting: true,
-        };
-        element.prefs = prefs;
-      });
-
-      test('gr-diff-host provides syntax highlighting layer to gr-diff', () => {
-        element.patchRange = {};
-        element.reload();
-        assert.equal(element.$.diff.layers[0], element.$.syntaxLayer);
-      });
-
-      test('rendering normal-sized diff does not disable syntax', () => {
-        element._diff = {
-          content: [{
-            a: ['foo'],
-          }],
-        };
-        assert.isTrue(element.$.syntaxLayer.enabled);
-      });
-
-      test('rendering large diff disables syntax', () => {
-        // Before it renders, set the first diff line to 500 '*' characters.
-        element._diff = {
-          content: [{
-            a: [new Array(501).join('*')],
-          }],
-        };
-        assert.isFalse(element.$.syntaxLayer.enabled);
-      });
-
-      test('starts syntax layer processing on render-content event', () => {
-        sandbox.stub(element.$.syntaxLayer, 'process').returns(Promise.resolve());
-        element.dispatchEvent(
-            new CustomEvent('render-content', {bubbles: true, composed: true}));
-        assert.isTrue(element.$.syntaxLayer.process.called);
-      });
-    });
   });
 </script>

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

@@ -378,7 +378,7 @@ limitations under the License.
               line-wrapping="[[lineWrapping]]"
               is-image-diff="[[isImageDiff]]"
               base-image="[[baseImage]]"
-              layers="[[layers]]"
+              plugin-layers="[[pluginLayers]]"
               revision-image="[[revisionImage]]">
             <table
                 id="diffTable"

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

@@ -271,7 +271,7 @@
 
       /** Set by Polymer. */
       isAttached: Boolean,
-      layers: Array,
+      pluginLayers: Array,
     },
 
     behaviors: [
@@ -724,7 +724,7 @@
 
     _diffChanged(newValue) {
       if (newValue) {
-        this._diffLength = this.getDiffLength(newValue);
+        this._diffLength = this.$.diffBuilder.getDiffLength();
         this._debounceRenderDiffTable();
       }
     },
@@ -955,23 +955,5 @@
       if (loading || !warning) { return 'newlineWarning hidden'; }
       return 'newlineWarning';
     },
-
-    /**
-     * Get the approximate length of the diff as the sum of the maximum
-     * length of the chunks.
-     * @param {Object} diff object
-     * @return {number}
-     */
-    getDiffLength(diff) {
-      return diff.content.reduce((sum, sec) => {
-        if (sec.hasOwnProperty('ab')) {
-          return sum + sec.ab.length;
-        } else {
-          return sum + Math.max(
-              sec.hasOwnProperty('a') ? sec.a.length : 0,
-              sec.hasOwnProperty('b') ? sec.b.length : 0);
-        }
-      }, 0);
-    },
   });
 })();

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

@@ -767,7 +767,7 @@ limitations under the License.
                   new CustomEvent('render', {bubbles: true, composed: true}));
             });
         const mock = document.createElement('mock-diff-response');
-        sandbox.stub(element, 'getDiffLength').returns(10000);
+        sandbox.stub(element.$.diffBuilder, 'getDiffLength').returns(10000);
         element.diff = mock.diffResponse;
         element.noRenderOnPrefsChange = true;
       });
@@ -1101,11 +1101,6 @@ limitations under the License.
           ));
       });
     });
-
-    test('getDiffLength', () => {
-      const diff = document.createElement('mock-diff-response').diffResponse;
-      assert.equal(element.getDiffLength(diff), 52);
-    });
   });
 
   a11ySuite('basic');

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

@@ -225,7 +225,7 @@
     process() {
       // Cancel any still running process() calls, because they append to the
       // same _baseRanges and _revisionRanges fields.
-      this._cancel();
+      this.cancel();
 
       // Discard existing ranges.
       this._baseRanges = [];
@@ -295,7 +295,7 @@
     /**
      * Cancel any asynchronous syntax processing jobs.
      */
-    _cancel() {
+    cancel() {
       if (this._processHandle != null) {
         this.cancelAsync(this._processHandle);
         this._processHandle = null;
@@ -306,7 +306,7 @@
     },
 
     _diffChanged() {
-      this._cancel();
+      this.cancel();
       this._baseRanges = [];
       this._revisionRanges = [];
     },