Optimize the table creation in the Sys Info page of the feedback app

chrome/browser/resources/feedback/js/sys_info.js

Index: chrome/browser/resources/feedback/js/sys_info.js
diff --git a/chrome/browser/resources/feedback/js/sys_info.js b/chrome/browser/resources/feedback/js/sys_info.js
index fcbd37c9ed0cedc554bf657f7278c7cc7e5b824a..cdd3aaa5da8f8b7f668e62193374a9816ee1d492 100644
--- a/chrome/browser/resources/feedback/js/sys_info.js
+++ b/chrome/browser/resources/feedback/js/sys_info.js
@@ -8,6 +8,18 @@
  */
 var loadTimeData = null;
 
+/**
+ * A queue of a sequence of closures that will incrementally build the sys info
+ * html table.
+ */
+var tableCreationClosuresQueue = [];
+
+/**
+ * The time used to post delayed tasks in MS. Currently set to be enough for two
+ * frames.
+ */
+var STANDARD_DELAY_MS = 32;
+
 function getValueDivForButton(button) {
   return $(button.id.substr(0, button.id.length - 4));
 }
@@ -17,17 +29,47 @@ function getButtonForValueDiv(valueDiv) {
 }
 
 /**
+ * Expands the multiline table cell that contains the given valueDiv.
+ * @param button The expand button.
+ * @param valueDiv The div that contains the multiline logs.
+ * @param delayFactor A value used for increasing the delay after which the
+ *     cell will be expanded. Useful for expandAll() since it expands the
+ *     multiline cells one after another with each expension done slightly after
+ *     the previous one.
+ */
+function expand(button, valueDiv, delayFactor) {
+  button.textContent = loadTimeData.getString('sysinfoPageCollapseBtn');
+  // Show the spinner container.
+  var valueCell = valueDiv.parentNode;
+  valueCell.firstChild.hidden = false;
+  // Expanding huge logs can take a very long time, so we do it after a delay
+  // to have a chance to render the spinner.
+  setTimeout(function() {
+    valueCell.className = 'number-expanded';
+    // Hide the spinner container.
+    valueCell.firstChild.hidden = true;
+  }, STANDARD_DELAY_MS * delayFactor);
+}
+
+/**
+ * Collapses the multiline table cell that contains the given valueDiv.
+ * @param button The expand button.
+ * @param valueDiv The div that contains the multiline logs.
+ */
+function collapse(button, valueDiv) {
+  button.textContent = loadTimeData.getString('sysinfoPageExpandBtn');
+  valueDiv.parentNode.className = 'number-collapsed';
+}
+
+/**
  * Toggles whether an item is collapsed or expanded.
  */
 function changeCollapsedStatus() {
   var valueDiv = getValueDivForButton(this);
-  if (valueDiv.parentNode.className == 'number-collapsed') {
-    valueDiv.parentNode.className = 'number-expanded';
-    this.textContent = loadTimeData.getString('sysinfoPageCollapseBtn');
-  } else {
-    valueDiv.parentNode.className = 'number-collapsed';
-    this.textContent = loadTimeData.getString('sysinfoPageExpandBtn');
-  }
+  if (valueDiv.parentNode.className == 'number-collapsed')
+    expand(this, valueDiv, 1);
+  else
+    collapse(this, valueDiv);
 }
 
 /**
@@ -35,12 +77,12 @@ function changeCollapsedStatus() {
  */
 function collapseAll() {
   var valueDivs = document.getElementsByClassName('stat-value');
-  for (var i = 0; i < valueDivs.length; i++) {
+  for (var i = 0; i < valueDivs.length; ++i) {
+    if (valueDivs[i].parentNode.className != 'number-expanded')
+      continue;
     var button = getButtonForValueDiv(valueDivs[i]);
-    if (button && button.className != 'button-hidden') {
-      button.textContent = loadTimeData.getString('sysinfoPageExpandBtn');
-      valueDivs[i].parentNode.className = 'number-collapsed';
-    }
+    if (button)
+      collapse(button, valueDivs[i]);
   }
 }
 
@@ -49,32 +91,12 @@ function collapseAll() {
  */
 function expandAll() {
   var valueDivs = document.getElementsByClassName('stat-value');
-  for (var i = 0; i < valueDivs.length; i++) {
-    var button = getButtonForValueDiv(valueDivs[i]);
-    if (button && button.className != 'button-hidden') {
-      button.textContent = loadTimeData.getString('sysinfoPageCollapseBtn');
-      valueDivs[i].parentNode.className = 'number-expanded';
-    }
-  }
-}
-
-/**
- * Collapse only those log items with multi-line values.
- */
-function collapseMultiLineStrings() {
-  var valueDivs = document.getElementsByClassName('stat-value');
-  var nameDivs = document.getElementsByClassName('stat-name');
-  for (var i = 0; i < valueDivs.length; i++) {
+  for (var i = 0; i < valueDivs.length; ++i) {
+    if (valueDivs[i].parentNode.className != 'number-collapsed')
+      continue;
     var button = getButtonForValueDiv(valueDivs[i]);
-    button.onclick = changeCollapsedStatus;
-    if (valueDivs[i].scrollHeight > (nameDivs[i].scrollHeight * 2)) {
-      button.className = '';
-      button.textContent = loadTimeData.getString('sysinfoPageExpandBtn');
-      valueDivs[i].parentNode.className = 'number-collapsed';
-    } else {
-      button.className = 'button-hidden';
-      valueDivs[i].parentNode.className = 'number';
-    }
+    if (button)
+      expand(button, valueDivs[i], i + 1);
   }
 }
 
@@ -88,62 +110,102 @@ function createNameCell(key) {
   return nameCell;
 }
 
-function createButtonCell(key) {
+function createButtonCell(key, isMultiLine) {
   var buttonCell = document.createElement('td');
   buttonCell.setAttribute('class', 'button-cell');
-  var button = document.createElement('button');
-  button.setAttribute('id', '' + key + '-value-btn');
-  buttonCell.appendChild(button);
+
+  if (isMultiLine) {
+    var button = document.createElement('button');
+    button.setAttribute('id', '' + key + '-value-btn');
+    button.onclick = changeCollapsedStatus;
+    button.textContent = loadTimeData.getString('sysinfoPageExpandBtn');
+    buttonCell.appendChild(button);
+  }
+
   return buttonCell;
 }
 
-function createValueCell(key, value) {
+function createValueCell(key, value, isMultiLine) {
   var valueCell = document.createElement('td');
   var valueDiv = document.createElement('div');
   valueDiv.setAttribute('class', 'stat-value');
   valueDiv.setAttribute('id', '' + key + '-value');
   valueDiv.appendChild(document.createTextNode(value));
+
+  if (isMultiLine) {
+    valueCell.className = 'number-collapsed';
+    var loadingContainer = $('spinner-container').cloneNode(true);
+    loadingContainer.setAttribute('id', '' + key + '-value-loading');
+    loadingContainer.hidden = true;
+    valueCell.appendChild(loadingContainer);
+  } else {
+    valueCell.className = 'number';
+  }
+
   valueCell.appendChild(valueDiv);
   return valueCell;
 }
 
 function createTableRow(key, value) {
   var row = document.createElement('tr');
+
+  // Avoid using element.scrollHeight as it's very slow. crbug.com/653968.
+  var isMultiLine = value.split('\n').length > 2 || value.length > 1000;
+
   row.appendChild(createNameCell(key));
-  row.appendChild(createButtonCell(key));
-  row.appendChild(createValueCell(key, value));
-  return row;
+  row.appendChild(createButtonCell(key, isMultiLine));
+  row.appendChild(createValueCell(key, value, isMultiLine));
+
+  $('detailsTable').appendChild(row);

[xiyuan, 2016/10/10 16:57:35]

nit: move appendChild into the wrapper function where createTableRow is called. 

[afakhry, 2016/10/12 16:12:07]

Done.

 }
 
 /**
- * Builds the system information table row by row.
- * @param {Element} table The DOM table element to which the newly created rows
- * will be appended.
- * @param {Object} systemInfo The system information that will be used to fill
- * the table.
+ * Finalize the page after the content has been loaded.
  */
-function createTable(table, systemInfo) {
-  for (var key in systemInfo) {
-    var item = systemInfo[key];
-    table.appendChild(createTableRow(item['key'], item['value']));
-  }
+function finishPageLoading() {
+  $('collapseAllBtn').onclick = collapseAll;
+  $('expandAllBtn').onclick = expandAll;
+
+  $('spinner-container').hidden = true;
 }
 
 /**
- * The callback which will be invoked by the parent window, when the system
- * information is ready.
+ * Pops a closure from the front of the queue and executes it.
+ */
+function processQueue() {
+  var closure = tableCreationClosuresQueue.shift();
+  if (closure)
+    closure();
+}
+
+/**
+ * Creates a closure that creates a table row for the given key and value.
+ */
+function createTableRowWrapper(key, value) {
+  return function() {
+    createTableRow(key, value);
+
+    // Post a task to process the next item in the queue.
+    setTimeout(processQueue, STANDARD_DELAY_MS);

[xiyuan, 2016/10/10 16:57:35]

nit: move this into processQueue to keep scheduling related code together.

[afakhry, 2016/10/12 16:12:07]

Done.

+  };
+}
+
+/**
+ * Creates closures to build the system information table row by row
+ * incrementally.
  * @param {Object} systemInfo The system information that will be used to fill
  * the table.
  */
-function onSysInfoReady(systemInfo) {
-  createTable($('detailsTable'), systemInfo);
-
-  $('collapseAllBtn').onclick = collapseAll;
-  $('expandAllBtn').onclick = expandAll;
+function createTable(systemInfo) {
+  for (var key in systemInfo) {
+    var item = systemInfo[key];
+    tableCreationClosuresQueue.push(
+        createTableRowWrapper(item['key'], item['value']));
+  }
 
-  collapseMultiLineStrings();
+  tableCreationClosuresQueue.push(finishPageLoading);
 
-  $('status').textContent = '';
+  processQueue();
 }
 
 /**
@@ -152,5 +214,5 @@ function onSysInfoReady(systemInfo) {
 window.onload = function() {
   loadTimeData = getLoadTimeData();
   i18nTemplate.process(document, loadTimeData);
-  getFullSystemInfo(onSysInfoReady);
+  getFullSystemInfo(createTable);
 };