Fix sending multiple feedback reports within short durations of each other

chrome/browser/resources/feedback/js/event_handler.js

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 /**
  * @type {number}
  * @const
  */
 var FEEDBACK_WIDTH = 500;
 /**
  * @type {number}
  * @const
  */
 var FEEDBACK_HEIGHT = 585;
 
 /**
  * @type {string}
  * @const
  */
 var FEEDBACK_DEFAULT_WINDOW_ID = 'default_window';
 
-/**
- * The feedback info received initially when the feedback UI was first requested
- * to start.
- * @type {Object}
- */
-var initialFeedbackInfo = null;
-
-/**
- * The feedbak info that is ready to be sent later when the system information
- * becomes available.
- * @type {Object}
- */
-var finalFeedbackInfo = null;
-
-/**
- * The system information received from the C++ side.
- * @type {Object}
- */
-var systemInfo = null;
-
-/**
- * True if the system information has been received, false otherwise.
- * @type {boolean}
- */
-var isSystemInfoReady = false;
-
-/**
- * A callback to be invoked when the system information is ready.
- * @type {function(sysInfo)}
- */
-var onSystemInfoReadyCallback = null;
-
 // To generate a hashed extension ID, use a sha-256 hash, all in lower case.
 // Example:
 //   echo -n 'abcdefghijklmnopqrstuvwxyzabcdef' | sha1sum | \
 //       awk '{print toupper($1)}'
 var whitelistedExtensionIds = [
   '12E618C3C6E97495AAECF2AC12DEB082353241C6', // QuickOffice
   '3727DD3E564B6055387425027AD74C58784ACC15', // QuickOffice
   '2FC374607C2DF285634B67C64A2E356C607091C3', // QuickOffice
   '2843C1E82A9B6C6FB49308FDDF4E157B6B44BC2B', // G+ Photos
   '5B5DA6D054D10DB917AF7D9EAE3C56044D1B0B03', // G+ Photos
@@ skipping 40 matching lines @@
   'A8208CCC87F8261AFAEB6B85D5E8D47372DDEA6B', // http://crbug.com/478929
   'B620CF4203315F9F2E046EDED22C7571A935958D', // http://crbug.com/510270
   'B206D8716769728278D2D300349C6CB7D7DE2EF9', // http://crbug.com/510270
   'EFCF5358672FEE04789FD2EC3638A67ADEDB6C8C', // http://crbug.com/514696
   'FAD85BC419FE00995D196312F53448265EFA86F1', // http://crbug.com/516527
   'F33B037DEDA65F226B7409C2ADB0CF3F8565AB03', // http://crbug.com/541769
   '969C788BCBC82FBBE04A17360CA165C23A419257', // http://crbug.com/541769
   '3BC3740BFC58F06088B300274B4CFBEA20136342', // http://crbug.com/541769
 ];
 
+/**
+ * A FeedbackRequest object represents a unique feedback report, requested by an
+ * instance of the feedback window. It contains the system information specific
+ * to this report, the full feedbackInfo, and callbacks to send the report upon
+ * request.
+ */
+class FeedbackRequest {
+  constructor(requestId, feedbackInfo) {
+    this.id_ = requestId;

[xiyuan, 2016/03/16 16:39:28]

This is no longer needed, right? Can we get rid of this and |lastUsedId| etc?

[afakhry, 2016/03/16 17:51:22]

As discussed offline, we use it for logging that the report was sent for Request
with this particular ID.

+    this.feedbackInfo_ = feedbackInfo;
+    this.onSystemInfoReadyCallback_ = null;
+    this.isSystemInfoReady_ = false;
+    this.isRequestCanceled_ = false;
+  }
+
+  /**
+   * Called when the system information is sent from the C++ side.
+   * @param {Object} sysInfo The received system information.
+   */
+  getSystemInformationCallback(sysInfo) {
+    if (this.isRequestCanceled_) {
+      // If the window had been closed before the system information was
+      // received, we skip the rest of the operations and return immediately.
+      return;
+    }
+
+    this.isSystemInfoReady_ = true;
+
+    // Combine the newly received system information with whatever system
+    // information we have in the feedback info (if any).
+    if (this.feedbackInfo_.systemInformation) {
+      this.feedbackInfo_.systemInformation =
+          this.feedbackInfo_.systemInformation.concat(sysInfo);
+    } else {
+      this.feedbackInfo_.systemInformation = sysInfo;
+    }
+
+    if (this.onSystemInfoReadyCallback_ != null) {
+      this.onSystemInfoReadyCallback_();
+      this.onSystemInfoReadyCallback_ = null;
+    }
+  }
+
+  /**
+   * Retrieves the system information for this request object.
+   * @param {function()} callback Invoked to notify the listener that the system
+   * information has been received.
+   */
+  getSystemInformation(callback) {
+    if (this.isSystemInfoReady_) {
+      callback();
+      return;
+    }
+
+    this.onSystemInfoReadyCallback_ = callback;
+    // The C++ side must reply to the callback specific to this object.
+    var boundCallback = this.getSystemInformationCallback.bind(this);
+    chrome.feedbackPrivate.getSystemInformation(boundCallback);
+  }
+
+  /**
+   * Sends the feedback report represented by the object, either now if system
+   * information is ready, or later once it is.
+   * @param {boolean} useSystemInfo True if the user would like the system
+   * information to be sent with the report.
+   */
+  sendReport(useSystemInfo) {
+    if (useSystemInfo && !this.isSystemInfoReady_) {
+      this.onSystemInfoReadyCallback_ = this.sendReportNow;
+      return;
+    }
+
+    this.sendReportNow();
+  }
+
+  /**
+   * Sends the report immediately and removes this object once the report is
+   * sent.
+   */
+  sendReportNow() {
+    /** @const */ var ID = this.id_;
+    var that = this;

[xiyuan, 2016/03/16 16:39:28]

unused?

[afakhry, 2016/03/16 17:51:22]

Oops, left overs from testing.

+    chrome.feedbackPrivate.sendFeedback(this.feedbackInfo_,
+        function(result) {
+          console.log('Feedback: Report sent for request with ID ' + ID);
+        });
+  }
+};
+
+/**
+ * Used to generate unique IDs for FeedbackRequest objects.
+ * @type {number}
+ */
+var lastUsedId = 0;
 
 /**
  * Function to determine whether or not a given extension id is whitelisted to
  * invoke the feedback UI. If the extension is whitelisted, the callback to
  * start the Feedback UI will be called.
  * @param {string} id the id of the sender extension.
  * @param {Function} startFeedbackCallback The callback function that will
  *     will start the feedback UI.
  * @param {Object} feedbackInfo The feedback info object to pass to the
  *     start feedback UI callback.
@@ skipping 14 matching lines @@
 }
 
 /**
  * Callback which gets notified once our feedback UI has loaded and is ready to
  * receive its initial feedback info object.
  * @param {Object} request The message request object.
  * @param {Object} sender The sender of the message.
  * @param {function(Object)} sendResponse Callback for sending a response.
  */
 function feedbackReadyHandler(request, sender, sendResponse) {
-  if (request.ready) {
-    chrome.runtime.sendMessage(
-        {sentFromEventPage: true, data: initialFeedbackInfo});
-  }
+  if (request.ready)
+    chrome.runtime.sendMessage({sentFromEventPage: true});
 }
 
-
 /**
  * Callback which gets notified if another extension is requesting feedback.
  * @param {Object} request The message request object.
  * @param {Object} sender The sender of the message.
  * @param {function(Object)} sendResponse Callback for sending a response.
  */
 function requestFeedbackHandler(request, sender, sendResponse) {
   if (request.requestFeedback)
     senderWhitelisted(sender.id, startFeedbackUI, request.feedbackInfo);
 }
 
 /**
- * Called when the system information is sent from the C++ side.
- * @param {Object} sysInfo The received system information.
- */
-
-function getSystemInformationCallback(sysInfo) {
-  systemInfo = sysInfo;
-  isSystemInfoReady = true;
-  if (onSystemInfoReadyCallback != null)
-    onSystemInfoReadyCallback(sysInfo);
-}
-
-/**
- * If the user requested to send the report before the system information was
- * received, this callback will be invoked once the system information is ready
- * to send the report then.
- * @param {Object} sysInfo The received system information.
- */
-
-function onSysInfoReadyForSend(sysInfo) {
-  // Combine the newly received system information with whatever system
-  // information we have in the final feedback info (if any).
-  if (finalFeedbackInfo.systemInformation) {
-    finalFeedbackInfo.systemInformation =
-        finalFeedbackInfo.systemInformation.concat(sysInfo);
-  } else {
-    finalFeedbackInfo.systemInformation = sysInfo;
-  }
-
-  chrome.feedbackPrivate.sendFeedback(finalFeedbackInfo, function(result) {});
-}
-
-/**
  * Callback which starts up the feedback UI.
  * @param {Object} feedbackInfo Object containing any initial feedback info.
  */
 function startFeedbackUI(feedbackInfo) {
   var win = chrome.app.window.get(FEEDBACK_DEFAULT_WINDOW_ID);
   if (win) {
     win.show();
     return;
   }
   chrome.app.window.create('html/default.html', {
       frame: 'none',
       id: FEEDBACK_DEFAULT_WINDOW_ID,
       width: FEEDBACK_WIDTH,
       height: FEEDBACK_HEIGHT,
       hidden: true,
       resizable: false },
       function(appWindow) {
-        // Initialize the state of the app only once upon the creation of the
-        // feedback UI window.
-        initialFeedbackInfo = feedbackInfo;
-        finalFeedbackInfo = null;
-        systemInfo = null;
-        isSystemInfoReady = false;
-        onSystemInfoReadyCallback = null;
+        // Generate a unique feedback request ID for this feedback window.
+        // A FeedbackRequest object is used to represent this instance of the
+        // feedback window.
+        /** @const */ var ID = ++lastUsedId;
+        var request = new FeedbackRequest(ID, feedbackInfo);
+
+        // The feedbackInfo member of the new window should refer to the one in
+        // its corresponding FeedbackRequest object to avoid copying and
+        // duplicatations.
+        appWindow.contentWindow.feedbackInfo = request.feedbackInfo_;
+
+        // We mark the FeedbackRequest instance as canceled if the window was
+        // closed before the report was requested to be sent.
+        var reportIsBeingSent = false;

[xiyuan, 2016/03/16 16:39:28]

nit: This can be part of FeedbackRequest.

[afakhry, 2016/03/16 17:51:22]

Done.

 
         // Define some functions for the new window so that it can call back
         // into here.
 
         // Define a function for the new window to get the system information.
         appWindow.contentWindow.getSystemInformation = function(callback) {
-          if (!isSystemInfoReady) {
-            onSystemInfoReadyCallback = callback;
-            chrome.feedbackPrivate.getSystemInformation(
-                getSystemInformationCallback);
-            return;
-          }
-
-          callback(systemInfo);
+          request.getSystemInformation(callback);
         };
 
-        // Define a function to be called by the new window when the report is
-        // not ready yet, and has to be sent later when the system information
-        // is received.
-        appWindow.contentWindow.sendReportLater = function(feedbackInfo) {
-          finalFeedbackInfo = feedbackInfo;
-          if (!isSystemInfoReady) {
-            onSystemInfoReadyCallback = onSysInfoReadyForSend;
-            return;
-          }
-
-          onSysInfoReadyForSend(systemInfo);
+        // Define a function to request sending the feedback report.
+        appWindow.contentWindow.sendFeedbackReport = function(useSystemInfo) {
+          // The report will be sent by the request object.
+          reportIsBeingSent = true;
+          request.sendReport(useSystemInfo);
         };
 
-        // Returns whether the system information has been received or not.
-        appWindow.contentWindow.isSystemInfoReady = function() {
-          return isSystemInfoReady;
-        };
+        // Observe when the window is closed.
+        appWindow.onClosed.addListener(function() {
+          if (!reportIsBeingSent)
+            request.isRequestCanceled_ = true;
+        });
       });
 }
 
 chrome.runtime.onMessage.addListener(feedbackReadyHandler);
 chrome.runtime.onMessageExternal.addListener(requestFeedbackHandler);
 chrome.feedbackPrivate.onFeedbackRequested.addListener(startFeedbackUI);