Fix sending multiple feedback reports within short durations of each other

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

Index: chrome/browser/resources/feedback/js/event_handler.js
diff --git a/chrome/browser/resources/feedback/js/event_handler.js b/chrome/browser/resources/feedback/js/event_handler.js
index 4ca6d128807197b2b7c69c726da565aa3a5e513c..65101e09ad3f2697feca87c3964c23df042c87f6 100644
--- a/chrome/browser/resources/feedback/js/event_handler.js
+++ b/chrome/browser/resources/feedback/js/event_handler.js
@@ -19,38 +19,6 @@ var FEEDBACK_HEIGHT = 585;
  */
 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 | \
@@ -111,6 +79,100 @@ var whitelistedExtensionIds = [
   '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
@@ -145,13 +207,10 @@ function senderWhitelisted(id, startFeedbackCallback, feedbackInfo) {
  * @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.
@@ -164,38 +223,6 @@ function requestFeedbackHandler(request, sender, sendResponse) {
 }
 
 /**
- * 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.
  */
@@ -213,46 +240,41 @@ function startFeedbackUI(feedbackInfo) {
       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;
+        });
       });
 }