finished testing functions and added events to bookmark tests.

added infrastructure to better support events and cleaned up callback testing.

Review URL: http://codereview.chromium.org/192028

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@25623 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 221 insertions, 83 deletions

diff --git a/chrome/browser/extensions/extension_bookmarks_apitest.cc b/chrome/browser/extensions/extension_bookmarks_apitest.cc
index 6eba8b1..1ca6198 100644
--- a/chrome/browser/extensions/extension_bookmarks_apitest.cc
+++ b/chrome/browser/extensions/extension_bookmarks_apitest.cc
@@ -6,6 +6,5 @@
 
 // Flaky, http://crbug.com/19866. Please consult phajdan.jr before re-enabling.
 IN_PROC_BROWSER_TEST_F(ExtensionApiTest, DISABLED_Bookmarks) {
-  // TODO(erikkay) no initial state for this test.
   ASSERT_TRUE(RunExtensionTest("bookmarks")) << message_;
 }
diff --git a/chrome/browser/extensions/extension_bookmarks_module.cc b/chrome/browser/extensions/extension_bookmarks_module.cc
index 910684f..f06a19e 100644
--- a/chrome/browser/extensions/extension_bookmarks_module.cc
+++ b/chrome/browser/extensions/extension_bookmarks_module.cc
@@ -197,9 +197,6 @@ void ExtensionBookmarkEventRouter::BookmarkNodeAdded(BookmarkModel* model,
   const BookmarkNode* node = parent->GetChild(index);
   args.Append(new StringValue(Int64ToString(node->id())));
   DictionaryValue* obj = ExtensionBookmarks::GetNodeDictionary(node, false);
-
-  // Remove id since it's already being passed as the first argument.
-  obj->Remove(keys::kIdKey, NULL);
   args.Append(obj);
 
   std::string json_args;
diff --git a/chrome/common/extensions/api/extension_api.json b/chrome/common/extensions/api/extension_api.json
index 28da984..91ceeba4 100755
--- a/chrome/common/extensions/api/extension_api.json
+++ b/chrome/common/extensions/api/extension_api.json
@@ -916,7 +916,7 @@
       {
         "name": "onRemoved",
         "type": "function",
-        "description": "Fired when a bookmark or folder is removed.",
+        "description": "Fired when a bookmark or folder is removed.  When a folder is removed recursively, a single notification is fired for the folder, and none for its contents.",
         "parameters": [
           {"type": "string", "name": "id"},
           {
@@ -965,7 +965,7 @@
       {
         "name": "onChildrenReordered",
         "type": "function",
-        "description": "Fired when the children of a folder have changed their order.",
+        "description": "Fired when the children of a folder have changed their order due to the order being sorted in the UI.  This is not called as a result of a move().",
         "parameters": [
           {"type": "string", "name": "id"},
           {
diff --git a/chrome/renderer/renderer_resources.grd b/chrome/renderer/renderer_resources.grd
index 0fa1f13..81f924f 100644
--- a/chrome/renderer/renderer_resources.grd
+++ b/chrome/renderer/renderer_resources.grd
@@ -1,6 +1,6 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <!-- This comment is only here because changes to resources are not picked up
-without changes to the corresponding grd file. as1 -->
+without changes to the corresponding grd file. ek1 -->
 <grit latest_public_release="0" current_release="1">
   <outputs>
     <output filename="grit/renderer_resources.h" type="rc_header">
diff --git a/chrome/renderer/resources/extension_apitest.js b/chrome/renderer/resources/extension_apitest.js
index 13150fb..9e4ed5e 100644
--- a/chrome/renderer/resources/extension_apitest.js
+++ b/chrome/renderer/resources/extension_apitest.js
@@ -8,7 +8,8 @@ var chrome = chrome || {};
   chrome.test.tests = chrome.test.tests || [];
 
   var completed = false;
-  var currentTest;
+  var currentTest = null;
+  var lastTest = null;
 
   function complete() {
     completed = true;
@@ -48,7 +49,28 @@ var chrome = chrome || {};
     complete();
   }
 
+  var pendingCallbacks = 0;
+  var totalCallbacks = 0;
+  
+  function callbackAdded() {
+    pendingCallbacks++;
+    chrome.test.assertEq(totalCallbacks, 0);
+  };
+  
+  function callbackCompleted() {
+    if (!totalCallbacks)
+      totalCallbacks = pendingCallbacks;
+    pendingCallbacks--;
+    if (pendingCallbacks == 0) {
+      //chrome.test.log("  " + totalCallbacks + " callbacks ran");
+      totalCallbacks = 0;
+      chrome.test.succeed();
+    }
+  };
+
   chrome.test.runNextTest = function() {
+    chrome.test.assertEq(pendingCallbacks, 0);
+    lastTest = currentTest;
     currentTest = chrome.test.tests.shift();
     if (!currentTest) {
       allTestsSucceeded();
@@ -104,36 +126,68 @@ var chrome = chrome || {};
                        chrome.extension.lastError.message);
     }
   };
+  
+  function safeFunctionApply(func, arguments) {
+    try {
+      if (func) {
+        func.apply(null, arguments);
+      }
+    } catch (e) {
+      var stack = null;
+      if (typeof(e.stack) != "undefined") {
+        stack = e.stack.toString();
+      }
+      var msg = "Exception during execution of callback in " +
+                currentTest.name;
+      if (stack) {
+        msg += "\n" + stack;
+      } else {
+        msg += "\n(no stack available)";
+      }
+      chrome.test.fail(msg);
+    }
+  };
 
   // Wrapper for generating test functions, that takes care of calling
   // assertNoLastError() and (optionally) succeed() for you.
-  chrome.test.testCallback = function(succeedWhenDone, func) {
+  chrome.test.callback = function(func, expectedError) {
+    chrome.test.assertEq(typeof(func), 'function');
+    callbackAdded();
     return function() {
-      chrome.test.assertNoLastError();
-      try {
-        if (func) {
-          func.apply(null, arguments);
-        }
-      } catch (e) {
-        var stack = null;
-        if (typeof(e.stack) != "undefined") {
-          stack = e.stack.toString();
-        }
-        var msg = "Exception during execution of testCallback in " +
-                  currentTest.name;
-        if (stack) {
-          msg += "\n" + stack;
-        } else {
-          msg += "\n(no stack available)";
-        }
-        chrome.test.fail(msg);
-      }
-      if (succeedWhenDone) {
-        chrome.test.succeed();
+      if (expectedError == null) {
+        chrome.test.assertNoLastError();
+      } else {
+        chrome.test.assertEq(typeof(expectedError), 'string');
+        chrome.test.assertEq(expectedError, chrome.extension.lastError.message);
       }
+      safeFunctionApply(func, arguments);
+      callbackCompleted();
+    };
+  };
+  
+  chrome.test.listenOnce = function(event, func) {
+    callbackAdded();
+    var listener = function() {
+      event.removeListener(listener);
+      safeFunctionApply(func, arguments);      
+      callbackCompleted();
     };
+    event.addListener(listener);
+  };
+  
+  chrome.test.callbackPass = function(func) {
+    return chrome.test.callback(func);
+  };
+
+  chrome.test.callbackFail = function(func, expectedError) {
+    return chrome.test.callback(func, expectedError);
   };
 
+  // TODO(erikkay) This is deprecated and should be removed.
+  chrome.test.testCallback = function(succeedWhenDone, func) {
+    return chrome.test.callback(func);
+  };
+  
   chrome.test.runTests = function(tests) {
     chrome.test.tests = tests;
     chrome.test.runNextTest();
diff --git a/chrome/test/data/extensions/api_test/bookmarks/test.js b/chrome/test/data/extensions/api_test/bookmarks/test.js
index 83d5745..6c1fa1b 100644
--- a/chrome/test/data/extensions/api_test/bookmarks/test.js
+++ b/chrome/test/data/extensions/api_test/bookmarks/test.js
@@ -18,11 +18,13 @@ var node2 = {parentId:"1", title:"foo quux",
 var node3 = {parentId:"1", title:"bar baz",
              url:"http://www.google.com/hello/quux"};
 
-var testCallback = chrome.test.testCallback;
+var pass = chrome.test.callbackPass;
+var fail = chrome.test.callbackFail;
 
 function compareNode(left, right) {
-  //chrome.test.log(JSON.stringify(left));
-  //chrome.test.log(JSON.stringify(right));
+  //chrome.test.log("compareNode()");
+  //chrome.test.log(JSON.stringify(left, null, 2));
+  //chrome.test.log(JSON.stringify(right, null, 2));
   // TODO(erikkay): do some comparison of dateAdded
   if (left.id != right.id)
     return "id mismatch: " + left.id + " != " + right.id;
@@ -30,7 +32,7 @@ function compareNode(left, right) {
     // TODO(erikkay): This resource dependency still isn't working reliably.
     // See bug 19866.
     // return "title mismatch: " + left.title + " != " + right.title;
-    console.log("title mismatch: " + left.title + " != " + right.title);
+    chrome.test.log("title mismatch: " + left.title + " != " + right.title);
   }
   if (left.url != right.url)
     return "url mismatch: " + left.url + " != " + right.url;
@@ -43,7 +45,6 @@ function compareTrees(left, right) {
   //chrome.test.log(JSON.stringify(left) || "<null>");
   //chrome.test.log(JSON.stringify(right) || "<null>");
   if (left == null && right == null) {
-    console.log("both left and right are NULL");
     return true;
   }
   if (left == null || right == null)
@@ -53,8 +54,9 @@ function compareTrees(left, right) {
   for (var i = 0; i < left.length; i++) {
     var result = compareNode(left[i], right[i]);
     if (result !== true) {
-      console.log(JSON.stringify(left));
-      console.log(JSON.stringify(right));
+      chrome.test.log(result);
+      chrome.test.log(JSON.stringify(left, null, 2));
+      chrome.test.log(JSON.stringify(right, null, 2));
       return result;
     }
     result = compareTrees(left[i].children, right[i].children);
@@ -66,7 +68,7 @@ function compareTrees(left, right) {
 
 chrome.test.runTests([
   function getTree() {
-    chrome.bookmarks.getTree(testCallback(true, function(results) {
+    chrome.bookmarks.getTree(pass(function(results) {
       chrome.test.assertTrue(compareTrees(results, expected),
                              "getTree() result != expected");
       expected = results;
@@ -74,13 +76,14 @@ chrome.test.runTests([
   },
 
   function get() {
-    chrome.bookmarks.get("1", testCallback(true, function(results) {
+    chrome.bookmarks.get("1", pass(function(results) {
       chrome.test.assertTrue(compareNode(results[0], expected[0].children[0]));
     }));
+    chrome.bookmarks.get("42", fail(function(){}, "Can't find bookmark for id."));
   },
 
   function getArray() {
-    chrome.bookmarks.get(["1", "2"], testCallback(true, function(results) {
+    chrome.bookmarks.get(["1", "2"], pass(function(results) {
       chrome.test.assertTrue(compareNode(results[0], expected[0].children[0]),
                              "get() result != expected");
       chrome.test.assertTrue(compareNode(results[1], expected[0].children[1]),
@@ -89,7 +92,7 @@ chrome.test.runTests([
   },
 
   function getChildren() {
-    chrome.bookmarks.getChildren("0", testCallback(true, function(results) {
+    chrome.bookmarks.getChildren("0", pass(function(results) {
       chrome.test.assertTrue(compareNode(results[0], expected[0].children[0]),
                              "getChildren() result != expected");
       chrome.test.assertTrue(compareNode(results[1], expected[0].children[1]),
@@ -99,7 +102,13 @@ chrome.test.runTests([
 
   function create() {
     var node = {parentId:"1", title:"google", url:"http://www.google.com/"};
-    chrome.bookmarks.create(node, testCallback(true, function(results) {
+    chrome.test.listenOnce(chrome.bookmarks.onCreated, function(id, created) {
+      node.id = created.id;
+      node.index = 0;
+      chrome.test.assertEq(id, node.id);
+      chrome.test.assertTrue(compareNode(node, created));
+    });
+    chrome.bookmarks.create(node, pass(function(results) {
       node.id = results.id;  // since we couldn't know this going in
       node.index = 0;
       chrome.test.assertTrue(compareNode(node, results),
@@ -107,10 +116,16 @@ chrome.test.runTests([
       expected[0].children[0].children.push(node);
     }));
   },
-  
+
   function createFolder() {
     var node = {parentId:"1", title:"foo bar"};  // folder
-    chrome.bookmarks.create(node, testCallback(true, function(results) {
+    chrome.test.listenOnce(chrome.bookmarks.onCreated, function(id, created) {
+      node.id = created.id;
+      node.index = 1;
+      node.children = [];
+      chrome.test.assertTrue(compareNode(node, created));
+    });
+    chrome.bookmarks.create(node, pass(function(results) {
       node.id = results.id;  // since we couldn't know this going in
       node.index = 1;
       node.children = [];
@@ -119,94 +134,168 @@ chrome.test.runTests([
       expected[0].children[0].children.push(node);
     }));
   },
-  
+
   function move_setup() {
-    chrome.bookmarks.create(node1, testCallback(false, function(results) {
+    var bookmarks_bar = expected[0].children[0];
+    chrome.bookmarks.create(node1, pass(function(results) {
       node1.id = results.id;
       node1.index = results.index;
-      expected[0].children[0].children.push(node1);
+      bookmarks_bar.children.push(node1);
     }));
-    chrome.bookmarks.create(node2, testCallback(false, function(results) {
+    chrome.bookmarks.create(node2, pass(function(results) {
       node2.id = results.id;
       node2.index = results.index;
-      expected[0].children[0].children.push(node2);
+      bookmarks_bar.children.push(node2);
     }));
-    chrome.bookmarks.create(node3, testCallback(false, function(results) {
+    chrome.bookmarks.create(node3, pass(function(results) {
       node3.id = results.id;
       node3.index = results.index;
-      expected[0].children[0].children.push(node3);
+      bookmarks_bar.children.push(node3);
     }));
-    chrome.bookmarks.getTree(testCallback(true, function(results) {
+    chrome.bookmarks.getTree(pass(function(results) {
       chrome.test.assertTrue(compareTrees(expected, results),
                              "getTree() result != expected");
       expected = results;
     }));
   },
-  
+
   function move() {
     // Move node1, node2, and node3 from their current location (the bookmark
-    // bar) to be under the "other bookmarks" folder instead.
-    var other = expected[0].children[1];
-    //var folder = expected[0].children[0].children[1];
-    //console.log(JSON.stringify(node1));
-    chrome.bookmarks.move(node1.id, {parentId:other.id},
-                          testCallback(false, function(results) {
-      chrome.test.assertEq(results.parentId, other.id);
+    // bar) to be under the "foo bar" folder (created in createFolder()).
+    // Then move that folder to be under the "other bookmarks" folder.
+    var folder = expected[0].children[0].children[1];
+    var old_node1 = expected[0].children[0].children[2];
+    chrome.test.listenOnce(chrome.bookmarks.onMoved, function(id, moveInfo) {
+      chrome.test.assertEq(node1.id, id);
+      chrome.test.assertEq(moveInfo.parentId, folder.id);
+      chrome.test.assertEq(moveInfo.index, 0);
+      chrome.test.assertEq(moveInfo.oldParentId, old_node1.parentId);
+      chrome.test.assertEq(moveInfo.oldIndex, old_node1.index);
+    });
+    chrome.bookmarks.move(node1.id, {parentId:folder.id},
+                          pass(function(results) {
+      chrome.test.assertEq(results.parentId, folder.id);
       node1.parentId = results.parentId;
+      node1.index = 0;
     }));
-    chrome.bookmarks.move(node2.id, {parentId:other.id},
-                          testCallback(false, function(results) {
-      chrome.test.assertEq(results.parentId, other.id);
-      node3.parentId = results.parentId;
+    chrome.bookmarks.move(node2.id, {parentId:folder.id},
+                          pass(function(results) {
+      chrome.test.assertEq(results.parentId, folder.id);
+      node2.parentId = results.parentId;
+      node2.index = 1;
     }));
     // Insert node3 at index 1 rather than at the end.  This should result in
     // an order of node1, node3, node2.
-    chrome.bookmarks.move(node3.id, {parentId:other.id, index:1},
-                          testCallback(true, function(results) {
-      chrome.test.assertEq(results.parentId, other.id);
+    chrome.bookmarks.move(node3.id, {parentId:folder.id, index:1},
+                          pass(function(results) {
+      chrome.test.assertEq(results.parentId, folder.id);
       chrome.test.assertEq(results.index, 1);
       node3.parentId = results.parentId;
       node3.index = 1;
+      node2.index = 2;
+    }));
+
+    chrome.bookmarks.getTree(pass(function(results) {
+      expected[0].children[0].children.pop();
+      expected[0].children[0].children.pop();
+      expected[0].children[0].children.pop();
+      folder.children.push(node1);
+      folder.children.push(node3);
+      folder.children.push(node2);
+      chrome.test.assertTrue(compareTrees(expected, results),
+                             "getTree() result != expected");
+      expected = results;
+    }));
+
+    // Move folder (and its children) to be a child of Other Bookmarks.
+    var other = expected[0].children[1];
+    chrome.bookmarks.move(folder.id, {parentId:other.id},
+                          pass(function(results) {
+      chrome.test.assertEq(results.parentId, other.id);
+      folder.parentId = results.parentId;
+    }));
+
+    chrome.bookmarks.getTree(pass(function(results) {
+      var folder = expected[0].children[0].children.pop();
+      folder.parentId = other.parentId;
+      folder.index = 0;
+      expected[0].children[1].children.push(folder);
+      chrome.test.assertTrue(compareTrees(expected, results),
+                             "getTree() result != expected");
+      expected = results;
     }));
   },
-  
+
   function search() {
-    chrome.bookmarks.search("baz bar", testCallback(false, function(results) {
+    chrome.bookmarks.search("baz bar", pass(function(results) {
       // matches node1 & node3
-      console.log(JSON.stringify(results));
       chrome.test.assertEq(2, results.length);
     }));
-    chrome.bookmarks.search("www hello", testCallback(false, function(results) {
+    chrome.bookmarks.search("www hello", pass(function(results) {
       // matches node1 & node3
       chrome.test.assertEq(2, results.length);
     }));
     chrome.bookmarks.search("bar example",
-                            testCallback(false, function(results) {
+                            pass(function(results) {
       // matches node2
       chrome.test.assertEq(1, results.length);
     }));
-    chrome.bookmarks.search("foo bar", testCallback(false, function(results) {
+    chrome.bookmarks.search("foo bar", pass(function(results) {
       // matches node1
       chrome.test.assertEq(1, results.length);
     }));
-    chrome.bookmarks.search("quux", testCallback(true, function(results) {
+    chrome.bookmarks.search("quux", pass(function(results) {
       // matches node2 & node3
       chrome.test.assertEq(2, results.length);
     }));
   },
-  
+
   function update() {
-    chrome.bookmarks.update(node1.id, {"title": "hello world"},
-                            testCallback(true, function(results) {
-      chrome.test.assertEq("hello world", results.title);
+    var title = "hello world";
+    chrome.test.listenOnce(chrome.bookmarks.onChanged, function(id, changes) {
+      chrome.test.assertEq(title, changes.title);
+    });
+    chrome.bookmarks.update(node1.id, {"title": title}, pass(function(results) {
+      chrome.test.assertEq(title, results.title);
     }));
   },
-  
+
   function remove() {
-    chrome.test.succeed();
+    var parentId = node1.parentId;
+    chrome.test.listenOnce(chrome.bookmarks.onRemoved,
+                           function(id, removeInfo) {
+      chrome.test.assertEq(id, node1.id);
+      chrome.test.assertEq(removeInfo.parentId, parentId);
+      chrome.test.assertEq(removeInfo.index, node1.index);
+    });
+    chrome.bookmarks.remove(node1.id, pass(function() {}));
+    chrome.bookmarks.getTree(pass(function(results) {
+      // We removed node1, which means that the index of the other two nodes
+      // changes as well.
+      expected[0].children[1].children[0].children.shift();
+      expected[0].children[1].children[0].children[0].index = 0;
+      expected[0].children[1].children[0].children[1].index = 1;
+      chrome.test.assertTrue(compareTrees(expected, results),
+                             "getTree() result != expected");
+      expected = results;
+    }));
   },
   
   function removeTree() {
-    chrome.test.succeed();
+    var parentId = node2.parentId;
+    var folder = expected[0].children[1].children[0];
+    chrome.test.listenOnce(chrome.bookmarks.onRemoved,
+                           function(id, removeInfo) {
+      chrome.test.assertEq(id, folder.id);
+      chrome.test.assertEq(removeInfo.parentId, folder.parentId);
+      chrome.test.assertEq(removeInfo.index, folder.index);
+    });
+    chrome.bookmarks.removeTree(parentId, pass(function(){}));
+    chrome.bookmarks.getTree(pass(function(results) {
+      expected[0].children[1].children.shift();
+      chrome.test.assertTrue(compareTrees(expected, results),
+                             "getTree() result != expected");
+      expected = results;
+    }));
   },
 ]);
diff --git a/chrome/test/data/extensions/api_test/override1/test.js b/chrome/test/data/extensions/api_test/override1/test.js
index 4f94e08..67f73a8 100644
--- a/chrome/test/data/extensions/api_test/override1/test.js
+++ b/chrome/test/data/extensions/api_test/override1/test.js
@@ -1,8 +1,7 @@
 chrome.test.runTests([
   function newtab() {
     chrome.tabs.create({"url": "chrome://newtab/"}, 
-                       chrome.test.testCallback(function(response) {
-    }));
+                       chrome.test.callbackPass(function(response) {}));
   }
 ]);