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..4013bd8f85aa413b279fded12722556cc6936212 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,101 @@ 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 {

[xiyuan, 2016/03/14 23:47:28]

Are we allowed to use this ES6 feature? This seems to be the first js file using
it.

[afakhry, 2016/03/15 19:25:38]

I asked sky@ who referred me to estade@ who referred me to dbeam@, and he told
me that yes we are allowed to use this feature on condition that the code
doesn't run on iOS, and it passes both the linter and the closure compiler.

I ran the closure compiler locally and found that it passes successfully. I'm
not sure if it doesn't run on iOS though.

[xiyuan, 2016/03/15 19:54:06]

Thank you for digging into this. I am fine as long as dbeam@ says okay.

+  constructor(requestId, feedbackInfo) {
+    this.id_ = requestId;
+    this.feedbackInfo_ = feedbackInfo;
+    this.onSystemInfoReadyCallback_ = null;
+    this.isSystemInfoReady_ = false;
+  }
+
+  /**
+   * Called when the system information is sent from the C++ side.
+   * @param {Object} sysInfo The received system information.
+   */
+  getSystemInformationCallback(sysInfo) {
+    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;
+    }

[xiyuan, 2016/03/14 23:47:28]

Just to double check with you that changes here on |this.feedbackInfo_| would
also be visible to |appWindow.contentWindow.feedbackInfo|, i.e. we can share js
var instance between here and the inside the app content window.

[afakhry, 2016/03/15 19:25:38]

Yes I made sure they refer to the same thing, and that when this.feedbackInfo_
changes the changes are seen by the sys info page.

+
+    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;

[xiyuan, 2016/03/14 23:47:28]

Think we need to bind here, i.e. this.sendReportNow.bind(this). Otherwise,
this.feedbackInfo_ etc is undefined when called from
getSystemInformationCallback.

[afakhry, 2016/03/15 19:25:38]

It works without this bind, since it will be called from
this.getSystemInformationCallback() which is bound to the same object.

The only bind that I found absolutely necessary is the one in:
chrome.feedbackPrivate.getSystemInformation(boundCallback);

inside getSystemInformation() above. Since this boundCallback will be called
from C++ and needs to be bound to a particular instance of FeedbackRequest.

Please let me know if it's really necessary so that I can change it.

[xiyuan, 2016/03/15 19:54:06]

Interesting. Just tried it and it works as you described. So ignore my comments
and no need to change. There is always something to learn in JS. :)

+      return;
+    }
+
+    this.sendReportNow();
+  }
+
+  /**
+   * Sends the report immediately and removes this object once the report is
+   * sent.
+   */
+  sendReportNow() {
+    /** @const */ var ID = this.id_;
+    chrome.feedbackPrivate.sendFeedback(this.feedbackInfo_,
+        function(result) {
+          console.log('Feedback: Report sent for request with ID ' + ID);
+
+          // The following deletes this FeedbackRequest object.
+          feedbackRequestsMap.delete(ID);
+        });
+  }
+};
+
+/**
+ * Maps each FeedbackRequest object by its unique ID.
+ * @type {Map}
+ */
+var feedbackRequestsMap = new Map();
+
+/**
+ * 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 +208,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 +224,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 +241,55 @@ 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;
+        feedbackRequestsMap.set(ID, new FeedbackRequest(ID, feedbackInfo));

[xiyuan, 2016/03/14 23:47:28]

It seems |feedbackRequestsMap| could be optimized out. I mean, we use |ID| as
part of the closure here, why not use FeedbackRequest instance directly?

[afakhry, 2016/03/15 19:25:38]

Yes, thanks for the suggestion. Done.

However, we still need the |feedbackRequestsMap| as we need to keep the various
FeedbackRequest instances alive long after their corresponding windows are
closed and this closure is removed, until the report is sent.

[xiyuan, 2016/03/15 19:54:06]

I don't think we need to manage this explicitly. If the request needs to be kept
alive, it means we are waiting for chrome.feedbackPrivate.getSystemInformation
to come back. This should be enough to keep the FeedbackRequest instances alive.

[afakhry, 2016/03/16 00:45:34]

Right. I removed the map, and it works. My brain needs to adjust a little to the
JS world.

+        // 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 =
+          feedbackRequestsMap.get(ID).feedbackInfo_;
+
+        // When this instance of the feedback window is closed, the
+        // corresponding FeedbackRequest object must be deleted unless it needs
+        // to be kept alive until the report is sent later.
+        var deferRequestdeleteUntilSent = false;
 
         // 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);
+          var request = feedbackRequestsMap.get(ID);
+          if (request == undefined) {
+            console.log('Feedback Error: Invalid request ID ' + ID);
             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;
+        // Define a function to request sending the feedback report.
+        appWindow.contentWindow.sendFeedbackReport = function(useSystemInfo) {
+          var request = feedbackRequestsMap.get(ID);
+          if (request == undefined) {
+            console.log('Feedback Error: Invalid request ID ' + ID);
             return;
           }
 
-          onSysInfoReadyForSend(systemInfo);
+          // The report will be sent by the request object. Keep it alive until
+          // the report is sent.
+          deferRequestdeleteUntilSent = 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 (!deferRequestdeleteUntilSent)
+            feedbackRequestsMap.delete(appWindow.contentWindow.REQUEST_ID);
+        });
       });
 }