Bug 606966 - Need an async history visit API exposed to JS

Part 23 - Update API to reality, and tweak tests.
r=mak
sr=rs
a=blocking

toolkit/components/places/public/mozIAsyncHistory.idl

@@ -44,6 +44,9 @@ interface nsIVariant;
 #include "jsapi.h"
 %}
 
+/**
+ * @status EXPERIMENTAL
+ */
 [scriptable, uuid(1a3b1260-4bdb-45d0-a306-dc377dd9baa4)]
 interface mozIVisitInfo : nsISupports
 {
@@ -78,6 +81,9 @@ interface mozIVisitInfo : nsISupports
   readonly attribute long long sessionId;
 };
 
+/**
+ * @status EXPERIMENTAL
+ */
 [scriptable, uuid(ad83e137-c92a-4b7b-b67e-0a318811f91e)]
 interface mozIPlaceInfo : nsISupports
 {
@@ -113,6 +119,9 @@ interface mozIPlaceInfo : nsISupports
   readonly attribute jsval visits;
 };
 
+/**
+ * @status EXPERIMENTAL
+ */
 [scriptable,function, uuid(3b97ca3c-5ea8-424f-b429-797477c52302)]
 interface mozIVisitInfoCallback : nsISupports
 {
@@ -129,20 +138,22 @@ interface mozIVisitInfoCallback : nsISupports
                   in mozIPlaceInfo aPlaceInfo);
 };
 
+/**
+ * @status EXPERIMENTAL
+ */
 [scriptable, uuid(f79ca67c-7e57-4511-a400-ea31001c762f)]
 interface mozIAsyncHistory : nsISupports
 {
   /**
    * Adds a set of visits for one or more mozIPlaceInfo objects, and updates
-   * each mozIPlaceInfo's title or guid.  It is not necessary to add visits to
-   * change a Place's title or guid.
+   * each mozIPlaceInfo's title or guid.
    *
    * @param aPlaceInfo
    *        The mozIPlaceInfo object[s] containing the information to store or
    *        update.  This can be a single object, or an array of objects.
    * @param [optional] aCallback
    *        Callback to be notified for each visit addition, title change, or
-   *        guid change.  If more than one operation is done for a given place,
+   *        guid change.  If more than one operation is done for a given visit,
    *        only one callback will be given (i.e. title change and add visit).
    *
    * @throws NS_ERROR_INVALID_ARG
@@ -150,8 +161,7 @@ interface mozIAsyncHistory : nsISupports
    *         - Not providing at least one valid guid, placeId, or uri for all
    *           mozIPlaceInfo object[s].
    *         - Not providing an array or nothing for the visits property of
-   *           mozIPlaceInfo (the property can be undefined for title or guid
-   *           changes).
+   *           mozIPlaceInfo.
    *         - Not providing a visitDate and transitionType for each
    *           mozIVisitInfo.
    *         - Providing an invalid transitionType for a mozIVisitInfo.

toolkit/components/places/src/History.cpp

@@ -1875,17 +1875,13 @@ History::UpdatePlaces(const jsval& aPlaceInfos,
         NS_ENSURE_ARG(JS_IsArrayObject(aCtx, visits));
       }
     }
+    NS_ENSURE_ARG(visits);
 
     jsuint visitsLength = 0;
     if (visits) {
       (void)JS_GetArrayLength(aCtx, visits, &visitsLength);
     }
-
-    // If we have no id or guid, we must have visits.
-    if (!(placeId > 0 || isValidGUID)) {
-      NS_ENSURE_ARG(visits);
-      NS_ENSURE_ARG(visitsLength > 0);
-    }
+    NS_ENSURE_ARG(visitsLength > 0);
 
     // Check each visit, and build our array of VisitData objects.
     visitData.SetCapacity(visitData.Length() + visitsLength);

toolkit/components/places/tests/unit/test_async_history_api.js

@@ -205,28 +205,49 @@ function test_invalid_guid_throws()
   run_next_test();
 }
 
-function test_no_id_or_guid_no_visits_throws()
+function test_no_visits_throws()
 {
-  // First, test with no visit attribute.
-  let place = {
-    uri: NetUtil.newURI(TEST_DOMAIN + "test_no_id_or_guid_no_visits_throws"),
-  };
-  try {
-    gHistory.updatePlaces(place);
-    do_throw("Should have thrown!");
-  }
-  catch (e) {
-    do_check_eq(e.result, Cr.NS_ERROR_INVALID_ARG);
-  }
+  const TEST_URI =
+    NetUtil.newURI(TEST_DOMAIN + "test_no_id_or_guid_no_visits_throws");
+  const TEST_GUID = "_RANDOMGUID_";
+  const TEST_PLACEID = 2;
 
-  // Now test with an empty array.
-  place.visits = [];
-  try {
-    gHistory.updatePlaces(place);
-    do_throw("Should have thrown!");
-  }
-  catch (e) {
-    do_check_eq(e.result, Cr.NS_ERROR_INVALID_ARG);
+  let log_test_conditions = function(aPlace) {
+    let str = "Testing place with " +
+      (aPlace.uri ? "uri" : "no uri") + ", " +
+      (aPlace.guid ? "guid" : "no guid") + ", " +
+      (aPlace.placeId ? "placeId" : "no placeId") + ", " +
+      (aPlace.visits ? "visits array" : "no visits array");
+    do_log_info(str);
+  };
+
+  // Loop through every possible case.  Note that we don't actually care about
+  // the case where we have no uri, place id, or guid (covered by another test),
+  // but it is easier to just make sure it too throws than to exclude it.
+  let place = { };
+  for (let uri = 1; uri >= 0; uri--) {
+    place.uri = uri ? TEST_URI : undefined;
+
+    for (let guid = 1; guid >= 0; guid--) {
+      place.guid = guid ? TEST_GUID : undefined;
+
+      for (let placeId = 1; placeId >= 0; placeId--) {
+        place.placeId = placeId ? TEST_PLACEID : undefined;
+
+        for (let visits = 1; visits >= 0; visits--) {
+          place.visits = visits ? [] : undefined;
+
+          log_test_conditions(place);
+          try {
+            gHistory.updatePlaces(place);
+            do_throw("Should have thrown!");
+          }
+          catch (e) {
+            do_check_eq(e.result, Cr.NS_ERROR_INVALID_ARG);
+          }
+        }
+      }
+    }
   }
 
   run_next_test();
@@ -729,7 +750,7 @@ let gTests = [
   test_invalid_places_throws,
   test_invalid_id_throws,
   test_invalid_guid_throws,
-  test_no_id_or_guid_no_visits_throws,
+  test_no_visits_throws,
   test_add_visit_no_date_throws,
   test_add_visit_no_transitionType_throws,
   test_add_visit_invalid_transitionType_throws,