Merge branch 'tz-mr-port-memory-fixes' into 'master'

Porting MR Vue Memory Fixes to current master

See merge request gitlab-org/gitlab-ce!21028

app/assets/javascripts/commons/polyfills.js

@@ -19,3 +19,4 @@ import './polyfills/custom_event';
 import './polyfills/element';
 import './polyfills/event';
 import './polyfills/nodelist';
+import './polyfills/request_idle_callback';

app/assets/javascripts/commons/polyfills/request_idle_callback.js

+window.requestIdleCallback =
+  window.requestIdleCallback ||
+  function requestShim(cb) {
+    const start = Date.now();
+    return setTimeout(() => {
+      cb({
+        didTimeout: false,
+        timeRemaining: () => Math.max(0, 50 - (Date.now() - start)),
+      });
+    }, 1);
+  };
+
+window.cancelIdleCallback =
+  window.cancelIdleCallback ||
+  function cancelShim(id) {
+    clearTimeout(id);
+  };

app/assets/javascripts/diffs/components/diff_line_gutter_content.vue

@@ -71,13 +71,23 @@ export default {
       required: false,
       default: false,
     },
+    isHover: {
+      type: Boolean,
+      required: false,
+      default: false,
+    },
+    discussions: {
+      type: Array,
+      required: false,
+      default: () => [],
+    },
   },
   computed: {
     ...mapState({
       diffViewType: state => state.diffs.diffViewType,
       diffFiles: state => state.diffs.diffFiles,
     }),
-    ...mapGetters(['isLoggedIn', 'discussionsByLineCode']),
+    ...mapGetters(['isLoggedIn']),
     lineHref() {
       return this.lineCode ? `#${this.lineCode}` : '#';
     },
@@ -85,26 +95,22 @@ export default {
       return (
         this.isLoggedIn &&
         this.showCommentButton &&
+        this.isHover &&
         !this.isMatchLine &&
         !this.isContextLine &&
-        !this.hasDiscussions &&
-        !this.isMetaLine
+        !this.isMetaLine &&
+        !this.hasDiscussions
       );
     },
-    discussions() {
-      return this.discussionsByLineCode[this.lineCode] || [];
-    },
     hasDiscussions() {
       return this.discussions.length > 0;
     },
     shouldShowAvatarsOnGutter() {
-      let render = this.hasDiscussions && this.showCommentButton;
-
       if (!this.lineType && this.linePosition === LINE_POSITION_RIGHT) {
-        render = false;
+        return false;
       }
-      return render;
+      return this.showCommentButton && this.hasDiscussions;
     },
   },
   methods: {
@@ -176,7 +182,7 @@ export default {
       v-else
     >
       <button
-        v-show="shouldShowCommentButton"
+        v-if="shouldShowCommentButton"
         type="button"
         class="add-diff-note js-add-diff-note-button"
         title="Add a comment to this line"

app/assets/javascripts/diffs/components/diff_table_cell.vue

@@ -67,6 +67,11 @@ export default {
       required: false,
       default: false,
     },
+    discussions: {
+      type: Array,
+      required: false,
+      default: () => [],
+    },
   },
   computed: {
     ...mapGetters(['isLoggedIn']),
@@ -132,10 +137,12 @@ export default {
       :line-number="lineNumber"
       :meta-data="normalizedLine.metaData"
       :show-comment-button="showCommentButton"
+      :is-hover="isHover"
       :is-bottom="isBottom"
       :is-match-line="isMatchLine"
       :is-context-line="isContentLine"
       :is-meta-line="isMetaLine"
+      :discussions="discussions"
     />
   </td>
 </template>

app/assets/javascripts/diffs/components/inline_diff_comment_row.vue

 <script>
-import { mapState, mapGetters } from 'vuex';
+import { mapState } from 'vuex';
 import diffDiscussions from './diff_discussions.vue';
 import diffLineNoteForm from './diff_line_note_form.vue';
@@ -21,15 +21,16 @@ export default {
       type: Number,
       required: true,
     },
+    discussions: {
+      type: Array,
+      required: false,
+      default: () => [],
+    },
   },
   computed: {
     ...mapState({
       diffLineCommentForms: state => state.diffs.diffLineCommentForms,
     }),
-    ...mapGetters(['discussionsByLineCode']),
-    discussions() {
-      return this.discussionsByLineCode[this.line.lineCode] || [];
-    },
     className() {
       return this.discussions.length ? '' : 'js-temp-notes-holder';
     },

app/assets/javascripts/diffs/components/inline_diff_table_row.vue

@@ -33,6 +33,11 @@ export default {
       required: false,
       default: false,
     },
+    discussions: {
+      type: Array,
+      required: false,
+      default: () => [],
+    },
   },
   data() {
     return {
@@ -89,6 +94,7 @@ export default {
       :is-bottom="isBottom"
       :is-hover="isHover"
       :show-comment-button="true"
+      :discussions="discussions"
       class="diff-line-num old_line"
     />
     <diff-table-cell
@@ -98,6 +104,7 @@ export default {
       :line-type="newLineType"
       :is-bottom="isBottom"
       :is-hover="isHover"
+      :discussions="discussions"
       class="diff-line-num new_line"
     />
     <td

app/assets/javascripts/diffs/components/inline_diff_view.vue

@@ -20,8 +20,11 @@ export default {
     },
   },
   computed: {
-    ...mapGetters('diffs', ['commitId']),
-    ...mapGetters(['discussionsByLineCode']),
+    ...mapGetters('diffs', [
+      'commitId',
+      'shouldRenderInlineCommentRow',
+      'singleDiscussionByLineCode',
+    ]),
     ...mapState({
       diffLineCommentForms: state => state.diffs.diffLineCommentForms,
     }),
@@ -36,15 +39,8 @@ export default {
     },
   },
   methods: {
-    shouldRenderCommentRow(line) {
-      if (this.diffLineCommentForms[line.lineCode]) return true;
-
-      const lineDiscussions = this.discussionsByLineCode[line.lineCode];
-      if (lineDiscussions === undefined) {
-        return false;
-      }
-
-      return lineDiscussions.every(discussion => discussion.expanded);
+    discussionsList(line) {
+      return line.lineCode !== undefined ? this.singleDiscussionByLineCode(line.lineCode) : [];
     },
   },
 };
@@ -65,13 +61,15 @@ export default {
           :line="line"
           :is-bottom="index + 1 === diffLinesLength"
           :key="line.lineCode"
+          :discussions="discussionsList(line)"
         />
         <inline-diff-comment-row
-          v-if="shouldRenderCommentRow(line)"
+          v-if="shouldRenderInlineCommentRow(line)"
           :diff-file-hash="diffFile.fileHash"
           :line="line"
           :line-index="index"
           :key="index"
+          :discussions="discussionsList(line)"
         />
       </template>
     </tbody>

app/assets/javascripts/diffs/components/parallel_diff_comment_row.vue

 <script>
-import { mapState, mapGetters } from 'vuex';
+import { mapState } from 'vuex';
 import diffDiscussions from './diff_discussions.vue';
 import diffLineNoteForm from './diff_line_note_form.vue';
@@ -21,30 +21,34 @@ export default {
       type: Number,
       required: true,
     },
+    leftDiscussions: {
+      type: Array,
+      required: false,
+      default: () => [],
+    },
+    rightDiscussions: {
+      type: Array,
+      required: false,
+      default: () => [],
+    },
   },
   computed: {
     ...mapState({
       diffLineCommentForms: state => state.diffs.diffLineCommentForms,
     }),
-    ...mapGetters(['discussionsByLineCode']),
     leftLineCode() {
       return this.line.left.lineCode;
     },
     rightLineCode() {
       return this.line.right.lineCode;
     },
-    hasDiscussion() {
-      const discussions = this.discussionsByLineCode;
-
-      return discussions[this.leftLineCode] || discussions[this.rightLineCode];
-    },
     hasExpandedDiscussionOnLeft() {
-      const discussions = this.discussionsByLineCode[this.leftLineCode];
+      const discussions = this.leftDiscussions;
       return discussions ? discussions.every(discussion => discussion.expanded) : false;
     },
     hasExpandedDiscussionOnRight() {
-      const discussions = this.discussionsByLineCode[this.rightLineCode];
+      const discussions = this.rightDiscussions;
       return discussions ? discussions.every(discussion => discussion.expanded) : false;
     },
@@ -52,17 +56,18 @@ export default {
       return this.hasExpandedDiscussionOnLeft || this.hasExpandedDiscussionOnRight;
     },
     shouldRenderDiscussionsOnLeft() {
-      return this.discussionsByLineCode[this.leftLineCode] && this.hasExpandedDiscussionOnLeft;
+      return this.leftDiscussions && this.hasExpandedDiscussionOnLeft;
     },
     shouldRenderDiscussionsOnRight() {
-      return (
-        this.discussionsByLineCode[this.rightLineCode] &&
-        this.hasExpandedDiscussionOnRight &&
-        this.line.right.type
-      );
+      return this.rightDiscussions && this.hasExpandedDiscussionOnRight && this.line.right.type;
+    },
+    showRightSideCommentForm() {
+      return this.line.right.type && this.diffLineCommentForms[this.rightLineCode];
     },
     className() {
-      return this.hasDiscussion ? '' : 'js-temp-notes-holder';
+      return this.leftDiscussions.length > 0 || this.rightDiscussions.length > 0
+        ? ''
+        : 'js-temp-notes-holder';
     },
   },
 };
@@ -80,13 +85,12 @@ export default {
         class="content"
       >
         <diff-discussions
-          v-if="discussionsByLineCode[leftLineCode].length"
-          :discussions="discussionsByLineCode[leftLineCode]"
+          v-if="leftDiscussions.length"
+          :discussions="leftDiscussions"
         />
       </div>
       <diff-line-note-form
-        v-if="diffLineCommentForms[leftLineCode] &&
-        diffLineCommentForms[leftLineCode]"
+        v-if="diffLineCommentForms[leftLineCode]"
         :diff-file-hash="diffFileHash"
         :line="line.left"
         :note-target-line="line.left"
@@ -100,13 +104,12 @@ export default {
         class="content"
       >
         <diff-discussions
-          v-if="discussionsByLineCode[rightLineCode].length"
-          :discussions="discussionsByLineCode[rightLineCode]"
+          v-if="rightDiscussions.length"
+          :discussions="rightDiscussions"
         />
       </div>
       <diff-line-note-form
-        v-if="diffLineCommentForms[rightLineCode] &&
-        diffLineCommentForms[rightLineCode] && line.right.type"
+        v-if="showRightSideCommentForm"
         :diff-file-hash="diffFileHash"
         :line="line.right"
         :note-target-line="line.right"

app/assets/javascripts/diffs/components/parallel_diff_table_row.vue

@@ -36,6 +36,16 @@ export default {
       required: false,
       default: false,
     },
+    leftDiscussions: {
+      type: Array,
+      required: false,
+      default: () => [],
+    },
+    rightDiscussions: {
+      type: Array,
+      required: false,
+      default: () => [],
+    },
   },
   data() {
     return {
@@ -116,6 +126,7 @@ export default {
       :is-hover="isLeftHover"
       :show-comment-button="true"
       :diff-view-type="parallelDiffViewType"
+      :discussions="leftDiscussions"
       class="diff-line-num old_line"
     />
     <td
@@ -136,6 +147,7 @@ export default {
       :is-hover="isRightHover"
       :show-comment-button="true"
       :diff-view-type="parallelDiffViewType"
+      :discussions="rightDiscussions"
       class="diff-line-num new_line"
     />
     <td

app/assets/javascripts/diffs/components/parallel_diff_view.vue

@@ -21,8 +21,11 @@ export default {
     },
   },
   computed: {
-    ...mapGetters('diffs', ['commitId']),
-    ...mapGetters(['discussionsByLineCode']),
+    ...mapGetters('diffs', [
+      'commitId',
+      'singleDiscussionByLineCode',
+      'shouldRenderParallelCommentRow',
+    ]),
     ...mapState({
       diffLineCommentForms: state => state.diffs.diffLineCommentForms,
     }),
@@ -53,29 +56,9 @@ export default {
     },
   },
   methods: {
-    shouldRenderCommentRow(line) {
-      const leftLineCode = line.left.lineCode;
-      const rightLineCode = line.right.lineCode;
-      const discussions = this.discussionsByLineCode;
-      const leftDiscussions = discussions[leftLineCode];
-      const rightDiscussions = discussions[rightLineCode];
-      const hasDiscussion = leftDiscussions || rightDiscussions;
-
-      const hasExpandedDiscussionOnLeft = leftDiscussions
-        ? leftDiscussions.every(discussion => discussion.expanded)
-        : false;
-      const hasExpandedDiscussionOnRight = rightDiscussions
-        ? rightDiscussions.every(discussion => discussion.expanded)
-        : false;
-
-      if (hasDiscussion && (hasExpandedDiscussionOnLeft || hasExpandedDiscussionOnRight)) {
-        return true;
-      }
-
-      const hasCommentFormOnLeft = this.diffLineCommentForms[leftLineCode];
-      const hasCommentFormOnRight = this.diffLineCommentForms[rightLineCode];
-
-      return hasCommentFormOnLeft || hasCommentFormOnRight;
+    discussionsByLine(line, leftOrRight) {
+      return line[leftOrRight] && line[leftOrRight].lineCode !== undefined ?
+            this.singleDiscussionByLineCode(line[leftOrRight].lineCode) : [];
     },
   },
 };
@@ -98,13 +81,17 @@ export default {
             :line="line"
             :is-bottom="index + 1 === diffLinesLength"
             :key="index"
+            :left-discussions="discussionsByLine(line, 'left')"
+            :right-discussions="discussionsByLine(line, 'right')"
           />
           <parallel-diff-comment-row
-            v-if="shouldRenderCommentRow(line)"
+            v-if="shouldRenderParallelCommentRow(line)"
             :key="`dcr-${index}`"
             :line="line"
             :diff-file-hash="diffFile.fileHash"
             :line-index="index"
+            :left-discussions="discussionsByLine(line, 'left')"
+            :right-discussions="discussionsByLine(line, 'right')"
           />
         </template>
       </tbody>

app/assets/javascripts/diffs/store/getters.js

@@ -64,6 +64,47 @@ export const getDiffFileDiscussions = (state, getters, rootState, rootGetters) =
       discussion.diff_discussion && _.isEqual(discussion.diff_file.file_hash, diff.fileHash),
   ) || [];
+export const singleDiscussionByLineCode = (state, getters, rootState, rootGetters) => lineCode => {
+  if (!lineCode || lineCode === undefined) return [];
+  const discussions = rootGetters.discussionsByLineCode;
+  return discussions[lineCode] || [];
+};
+
+export const shouldRenderParallelCommentRow = (state, getters) => line => {
+  const leftLineCode = line.left.lineCode;
+  const rightLineCode = line.right.lineCode;
+  const leftDiscussions = getters.singleDiscussionByLineCode(leftLineCode);
+  const rightDiscussions = getters.singleDiscussionByLineCode(rightLineCode);
+  const hasDiscussion = leftDiscussions.length || rightDiscussions.length;
+
+  const hasExpandedDiscussionOnLeft = leftDiscussions.length
+    ? leftDiscussions.every(discussion => discussion.expanded)
+    : false;
+  const hasExpandedDiscussionOnRight = rightDiscussions.length
+    ? rightDiscussions.every(discussion => discussion.expanded)
+    : false;
+
+  if (hasDiscussion && (hasExpandedDiscussionOnLeft || hasExpandedDiscussionOnRight)) {
+    return true;
+  }
+
+  const hasCommentFormOnLeft = state.diffLineCommentForms[leftLineCode];
+  const hasCommentFormOnRight = state.diffLineCommentForms[rightLineCode];
+
+  return hasCommentFormOnLeft || hasCommentFormOnRight;
+};
+
+export const shouldRenderInlineCommentRow = (state, getters) => line => {
+  if (state.diffLineCommentForms[line.lineCode]) return true;
+
+  const lineDiscussions = getters.singleDiscussionByLineCode(line.lineCode);
+  if (lineDiscussions.length === 0) {
+    return false;
+  }
+
+  return lineDiscussions.every(discussion => discussion.expanded);
+};
+
 // prevent babel-plugin-rewire from generating an invalid default during karma∂ tests
 export const getDiffFileByHash = state => fileHash =>
   state.diffFiles.find(file => file.fileHash === fileHash);

app/assets/javascripts/diffs/store/utils.js

@@ -173,3 +173,24 @@ export function trimFirstCharOfLineContent(line = {}) {
   return parsedLine;
 }
+
+export function getDiffRefsByLineCode(diffFiles) {
+  return diffFiles.reduce((acc, diffFile) => {
+    const { baseSha, headSha, startSha } = diffFile.diffRefs;
+    const { newPath, oldPath } = diffFile;
+
+    // We can only use highlightedDiffLines to create the map of diff lines because
+    // highlightedDiffLines will also include every parallel diff line in it.
+    if (diffFile.highlightedDiffLines) {
+      diffFile.highlightedDiffLines.forEach(line => {
+        const { lineCode, oldLine, newLine } = line;
+
+        if (lineCode) {
+          acc[lineCode] = { baseSha, headSha, startSha, newPath, oldPath, oldLine, newLine };
+        }
+      });
+    }
+
+    return acc;
+  }, {});
+}

app/assets/javascripts/lazy_loader.js

@@ -19,11 +19,17 @@ export default class LazyLoader {
     scrollContainer.addEventListener('load', () => this.loadCheck());
   }
   searchLazyImages() {
-    this.lazyImages = [].slice.call(document.querySelectorAll('.lazy'));
-    if (this.lazyImages.length) {
-      this.checkElementsInView();
-    }
+    const that = this;
+    requestIdleCallback(
+      () => {
+        that.lazyImages = [].slice.call(document.querySelectorAll('.lazy'));
+        if (that.lazyImages.length) {
+          that.checkElementsInView();
+        }
+      },
+      { timeout: 500 },
+    );
   }
   startContentObserver() {
     const contentNode = document.querySelector(this.observerNode) || document.querySelector('body');
@@ -56,7 +62,9 @@ export default class LazyLoader {
         const imgBound = imgTop + imgBoundRect.height;
         if (scrollTop < imgBound && visHeight > imgTop) {
-          LazyLoader.loadImage(selectedImage);
+          requestAnimationFrame(() => {
+            LazyLoader.loadImage(selectedImage);
+          });
           return false;
         }

app/assets/javascripts/vue_shared/components/content_viewer/viewers/image_viewer.vue

@@ -42,6 +42,9 @@ export default {
   },
   methods: {
     onImgLoad() {
+      requestIdleCallback(this.calculateImgSize, { timeout: 1000 });
+    },
+    calculateImgSize() {
       const { contentImg } = this.$refs;
       if (contentImg) {

changelogs/unreleased/tz-mr-port-memory-fixes.yml

+---
+title: Improve performance and memory footprint of Changes tab of Merge Requests
+merge_request: 21028
+author:
+type: performance

spec/javascripts/diffs/components/diff_line_gutter_content_spec.js

@@ -48,7 +48,11 @@ describe('DiffLineGutterContent', () => {
       it('should return discussions for the given lineCode', () => {
         const { lineCode } = getDiffFileMock().highlightedDiffLines[1];
-        const component = createComponent({ lineCode, showCommentButton: true });
+        const component = createComponent({
+          lineCode,
+          showCommentButton: true,
+          discussions: getDiscussionsMockData(),
+        });
         setDiscussions(component);

spec/javascripts/diffs/store/getters_spec.js

@@ -184,6 +184,104 @@ describe('Diffs Module Getters', () => {
     });
   });
+  describe('singleDiscussionByLineCode', () => {
+    it('returns found discussion per line Code', () => {
+      const discussionsMock = {};
+      discussionsMock.ABC = discussionMock;
+
+      expect(
+        getters.singleDiscussionByLineCode(localState, {}, null, {
+          discussionsByLineCode: () => discussionsMock,
+        })('DEF'),
+      ).toEqual([]);
+    });
+
+    it('returns empty array when no discussions match', () => {
+      expect(
+        getters.singleDiscussionByLineCode(localState, {}, null, {
+          discussionsByLineCode: () => {},
+        })('DEF'),
+      ).toEqual([]);
+    });
+  });
+
+  describe('shouldRenderParallelCommentRow', () => {
+    let line;
+
+    beforeEach(() => {
+      line = {};
+
+      line.left = {
+        lineCode: 'ABC',
+      };
+
+      line.right = {
+        lineCode: 'DEF',
+      };
+    });
+
+    it('returns true when discussion is expanded', () => {
+      discussionMock.expanded = true;
+
+      expect(
+        getters.shouldRenderParallelCommentRow(localState, {
+          singleDiscussionByLineCode: () => [discussionMock],
+        })(line),
+      ).toEqual(true);
+    });
+
+    it('returns false when no discussion was found', () => {
+      localState.diffLineCommentForms.ABC = false;
+      localState.diffLineCommentForms.DEF = false;
+
+      expect(
+        getters.shouldRenderParallelCommentRow(localState, {
+          singleDiscussionByLineCode: () => [],
+        })(line),
+      ).toEqual(false);
+    });
+
+    it('returns true when discussionForm was found', () => {
+      localState.diffLineCommentForms.ABC = {};
+
+      expect(
+        getters.shouldRenderParallelCommentRow(localState, {
+          singleDiscussionByLineCode: () => [discussionMock],
+        })(line),
+      ).toEqual(true);
+    });
+  });
+
+  describe('shouldRenderInlineCommentRow', () => {
+    it('returns true when diffLineCommentForms has form', () => {
+      localState.diffLineCommentForms.ABC = {};
+
+      expect(
+        getters.shouldRenderInlineCommentRow(localState)({
+          lineCode: 'ABC',
+        }),
+      ).toEqual(true);
+    });
+
+    it('returns false when no line discussions were found', () => {
+      expect(
+        getters.shouldRenderInlineCommentRow(localState, {
+          singleDiscussionByLineCode: () => [],
+        })('DEF'),
+      ).toEqual(false);
+    });
+
+    it('returns true if all found discussions are expanded', () => {
+      discussionMock.expanded = true;
+
+      expect(
+        getters.shouldRenderInlineCommentRow(localState, {
+          singleDiscussionByLineCode: () => [discussionMock],
+        })('ABC'),
+      ).toEqual(true);
+    });
+  });
+
   describe('getDiffFileDiscussions', () => {
     it('returns an array with discussions when fileHash matches and the discussion belongs to a diff', () => {
       discussionMock.diff_file.file_hash = diffFileMock.fileHash;

spec/javascripts/vue_shared/components/diff_viewer/viewers/image_diff_viewer_spec.js

@@ -170,8 +170,6 @@ describe('ImageDiffViewer', () => {
       vm.$el.querySelector('.view-modes-menu li:nth-child(3)').click();
       vm.$nextTick(() => {
-        expect(vm.$el.querySelector('.dragger').style.left).toBe('100px');
-
         dragSlider(vm.$el.querySelector('.dragger'));
         vm.$nextTick(() => {