Remove chrome.* Function Instrumentation Inlining and Use a Separate Instrumented Object

chrome/browser/resources/google_now/utility.js

Index: chrome/browser/resources/google_now/utility.js
diff --git a/chrome/browser/resources/google_now/utility.js b/chrome/browser/resources/google_now/utility.js
index 0ab7c4b7c384b80ee3d9cea4387aef77c0df4cba..7c9d673c5b22b85f2283058c05421411865f6054 100644
--- a/chrome/browser/resources/google_now/utility.js
+++ b/chrome/browser/resources/google_now/utility.js
@@ -55,6 +55,9 @@ function buildServerRequest(handlerName, contentType) {
   return request;
 }
 
+// Partial mirror of chrome.* for all instrumented functions.
+var instrumented = {};
+
 /**
  * Builds the object to manage tasks (mutually exclusive chains of events).
  * @param {function(string, string): boolean} areConflicting Function that
@@ -298,18 +301,36 @@ function buildTaskManager(areConflicting) {
   /**
    * Instruments an API function to add error processing to its user
    * code-provided callback.
-   * @param {Object} namespace Namespace of the API function.
-   * @param {string} functionName Name of the API function.
+   * @param {string} functionIdentifier Full identifier of the function without
+   *     the 'chrome.' portion.
    * @param {number} callbackParameter Index of the callback parameter to this
    *     API function.
    */
-  function instrumentApiFunction(namespace, functionName, callbackParameter) {
-    var originalFunction = namespace[functionName];
+  function instrumentChromeApiFunction(functionIdentifier, callbackParameter) {
+    var functionIdentifierParts = functionIdentifier.split('.');
+    var functionName = functionIdentifierParts.pop();
+    var functioncontainer = chrome;

[vadimt, 2013/08/08 01:15:10]

chromeContainer?

[robliao, 2013/08/08 17:43:31]

Done.

+    var instrumentedcontainer = instrumented;

[vadimt, 2013/08/08 01:15:10]

camelCased

[robliao, 2013/08/08 17:43:31]

Done.

+    functionIdentifierParts.map(function(fragment) {
+      functioncontainer = functioncontainer[fragment];
+      if (!(fragment in instrumentedcontainer))
+        instrumentedcontainer[fragment] = {};
+
+      instrumentedcontainer = instrumentedcontainer[fragment];
+    });
 
-    if (!originalFunction)
+    var targetFunction = functioncontainer[functionName];
+
+    if (!targetFunction)
       debugAlert('Cannot instrument ' + functionName);
 
-    namespace[functionName] = function() {
+    // After we've verified that the target is indeed a function, throw
+    // it away so that we can't ever use it again.
+    // Chrome can rebind the chrome.* APIs, which may invalidate
+    // any cached function we have.
+    targetFunction = null;

[vadimt, 2013/08/08 01:15:10]

It could be better to move the above piece of code into a separate function
where targetFunction would be a local variable. This would also solve the
problem of accidentally using it in the callback and the problem of the growing
side of instrumentChromeApiFunction.

[robliao, 2013/08/08 17:43:31]

Done.

+
+    instrumentedcontainer[functionName] = function() {
       // This is the wrapper for the API function. Pass the wrapped callback to
       // the original function.
       var callback = arguments[callbackParameter];
@@ -319,14 +340,21 @@ function buildTaskManager(areConflicting) {
       }
       arguments[callbackParameter] = wrapCallback(
           callback, functionName == 'addListener');
-      return originalFunction.apply(namespace, arguments);
+
+      var targetFunctionContainer = chrome;
+      functionIdentifierParts.map(function(fragment) {
+        targetFunctionContainer = targetFunctionContainer[fragment];
+      });
+      return targetFunctionContainer[functionName].
+          apply(targetFunctionContainer, arguments);
     };
   }
 
-  instrumentApiFunction(chrome.alarms, 'get', 1);
-  instrumentApiFunction(chrome.alarms.onAlarm, 'addListener', 0);
-  instrumentApiFunction(chrome.identity, 'getAuthToken', 1);
-  instrumentApiFunction(chrome.runtime.onSuspend, 'addListener', 0);
+  instrumentChromeApiFunction('alarms.get', 1);
+  instrumentChromeApiFunction('alarms.onAlarm.addListener', 0);
+  instrumentChromeApiFunction('identity.getAuthToken', 1);
+  instrumentChromeApiFunction('identity.removeCachedAuthToken', 1);
+  instrumentChromeApiFunction('runtime.onSuspend.addListener', 0);
 
   chrome.runtime.onSuspend.addListener(function() {
     var stringifiedPendingCallbacks = JSON.stringify(pendingCallbacks);
@@ -340,13 +368,11 @@ function buildTaskManager(areConflicting) {
   return {
     add: add,
     debugSetStepName: function() {},  // TODO(vadimt): remove
-    instrumentApiFunction: instrumentApiFunction,
+    instrumentChromeApiFunction: instrumentChromeApiFunction,
     wrapCallback: wrapCallback
   };
 }
 
-var storage = chrome.storage.local;

[vadimt, 2013/08/08 01:15:10]

Why not  var storage = instrumented.storage.local;
?

[robliao, 2013/08/08 17:43:31]

instrumented.storage.local doesn't exist until after buildTaskManager.

We'd have to instantiate buildTaskManager here for this to work.
On 2013/08/08 01:15:10, vadimt wrote:

[vadimt, 2013/08/08 18:19:25]

I's always a good idea to instrument stuff before the code that uses it. Treat
this as 'using' directive.
Let's instrument storage before the task manager, and define 'storage' variable
in the next line.

[robliao, 2013/08/08 21:35:35]

Per discussion, I am going to leave things as is. Having a direct
chrome.storage.local.set/get allows for the bifurcation of storage to
instrumented and noninstrumented parts as well as keeping the calls simple. 

On 2013/08/08 18:19:25, vadimt wrote:

-
 /**
  * Builds an object to manage retrying activities with exponential backoff.
  * @param {string} name Name of this attempt manager.
@@ -380,7 +406,7 @@ function buildAttemptManager(
    *     true if the attempt manager has started, false otherwise.
    */
   function isRunning(callback) {
-    chrome.alarms.get(alarmName, function(alarmInfo) {
+    instrumented.alarms.get(alarmName, function(alarmInfo) {
       callback(!!alarmInfo);
     });
   }
@@ -401,7 +427,7 @@ function buildAttemptManager(
 
     var items = {};
     items[currentDelayStorageKey] = newRetryDelaySeconds;
-    storage.set(items);
+    chrome.storage.local.set(items);
   }
 
   /**
@@ -413,7 +439,7 @@ function buildAttemptManager(
   function start(opt_firstDelaySeconds) {
     if (opt_firstDelaySeconds) {
       createAlarm(opt_firstDelaySeconds);
-      storage.remove(currentDelayStorageKey);
+      chrome.storage.local.remove(currentDelayStorageKey);
     } else {
       scheduleNextAttempt();
     }
@@ -424,7 +450,7 @@ function buildAttemptManager(
    */
   function stop() {
     chrome.alarms.clear(alarmName);
-    storage.remove(currentDelayStorageKey);
+    chrome.storage.local.remove(currentDelayStorageKey);
   }
 
   /**
@@ -433,14 +459,14 @@ function buildAttemptManager(
    *     the planning is done.
    */
   function planForNext(callback) {
-    storage.get(currentDelayStorageKey, function(items) {
+    instrumented.storage.local.get(currentDelayStorageKey, function(items) {
       console.log('planForNext-get-storage ' + JSON.stringify(items));
       scheduleNextAttempt(items[currentDelayStorageKey]);
       callback();
     });
   }
 
-  chrome.alarms.onAlarm.addListener(function(alarm) {
+  instrumented.alarms.onAlarm.addListener(function(alarm) {
     if (alarm.name == alarmName)
       isRunning(function(running) {
         if (running)
@@ -474,7 +500,7 @@ function buildAuthenticationManager() {
    *     If the user is signed in, the string contains the token.
    */
   function isSignedIn(callback) {
-    chrome.identity.getAuthToken({interactive: false}, function(token) {
+    instrumented.identity.getAuthToken({interactive: false}, function(token) {
       token = chrome.runtime.lastError ? undefined : token;
       callback(token);
       checkAndNotifyListeners(!!token);
@@ -487,7 +513,7 @@ function buildAuthenticationManager() {
    * @param {function} onSuccess Called on completion.
    */
   function removeToken(token, onSuccess) {
-    chrome.identity.removeCachedAuthToken({token: token}, function() {
+    instrumented.identity.removeCachedAuthToken({token: token}, function() {
       // Removing the token from the cache will change the sign in state.
       // Repoll now to check the state and notify listeners.
       // This also lets Chrome now about a possible problem with the token.
@@ -522,7 +548,7 @@ function buildAuthenticationManager() {
     lastReturnedSignedInState = currentSignedInState;
   }
 
-  chrome.alarms.onAlarm.addListener(function(alarm) {
+  instrumented.alarms.onAlarm.addListener(function(alarm) {
     if (alarm.name == alarmName)
       isSignedIn(function() {});
   });