Bug 629814 - Async history API should ignore place ids
authorShawn Wilsher <me@shawnwilsher.com>
Thu, 03 Feb 2011 14:09:37 -0800
changeset 61908 1cb679417a8ba3f4f5e13c7bc880a9c70944f3f8
parent 61907 ab87d34e89fa00423266c8177ee3eb1811e08f61
child 61909 aebd2f355859be18ff73b6462d5ce9abf0ab9ce2
push idunknown
push userunknown
push dateunknown
bugs629814
milestone2.0b12pre
Bug 629814 - Async history API should ignore place ids r=mak sr=rstrong a=softblocker
toolkit/components/places/public/mozIAsyncHistory.idl
toolkit/components/places/src/History.cpp
toolkit/components/places/tests/unit/test_async_history_api.js
--- a/toolkit/components/places/public/mozIAsyncHistory.idl
+++ b/toolkit/components/places/public/mozIAsyncHistory.idl
@@ -153,17 +153,17 @@ interface mozIAsyncHistory : nsISupports
    *        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 visit,
    *        only one callback will be given (i.e. title change and add visit).
    *
    * @throws NS_ERROR_INVALID_ARG
    *         - Passing in NULL for aPlaceInfo.
-   *         - Not providing at least one valid guid, placeId, or uri for all
+   *         - Not providing at least one valid guid, or uri for all
    *           mozIPlaceInfo object[s].
    *         - Not providing an array or nothing for the visits property of
    *           mozIPlaceInfo.
    *         - Not providing a visitDate and transitionType for each
    *           mozIVisitInfo.
    *         - Providing an invalid transitionType for a mozIVisitInfo.
    */
   [implicit_jscontext]
--- a/toolkit/components/places/src/History.cpp
+++ b/toolkit/components/places/src/History.cpp
@@ -584,26 +584,23 @@ private:
 /**
  * Checks to see if we can add aURI to history, and dispatches an error to
  * aCallback (if provided) if we cannot.
  *
  * @param aURI
  *        The URI to check.
  * @param [optional] aGUID
  *        The guid of the URI to check.  This is passed back to the callback.
- * @param [optional] aPlaceId
- *        The placeId of the URI to check.  This is passed back to the callback.
  * @param [optional] aCallback
  *        The callback to notify if the URI cannot be added to history.
  * @return true if the URI can be added to history, false otherwise.
  */
 bool
 CanAddURI(nsIURI* aURI,
           const nsCString& aGUID = EmptyCString(),
-          PRInt64 aPlaceId = 0,
           mozIVisitInfoCallback* aCallback = NULL)
 {
   nsNavHistory* navHistory = nsNavHistory::GetHistoryService();
   NS_ENSURE_TRUE(navHistory, false);
 
   PRBool canAdd;
   nsresult rv = navHistory->CanAddURI(aURI, &canAdd);
   if (NS_SUCCEEDED(rv) && canAdd) {
@@ -614,17 +611,16 @@ CanAddURI(nsIURI* aURI,
   if (aCallback) {
     // NotifyCompletion does not hold a strong reference to the callback, so we
     // have to manage it by AddRefing now and then releasing it after the event
     // has run.
     NS_ADDREF(aCallback);
 
     VisitData place(aURI);
     place.guid = aGUID;
-    place.placeId = aPlaceId;
     nsCOMPtr<nsIRunnable> event =
       new NotifyCompletion(aCallback, place, NS_ERROR_INVALID_ARG);
     (void)NS_DispatchToMainThread(event);
 
     // Also dispatch an event to release our reference to the callback after
     // NotifyCompletion has run.
     nsCOMPtr<nsIThread> mainThread = do_GetMainThread();
     (void)NS_ProxyRelease(mainThread, aCallback, PR_TRUE);
@@ -1850,45 +1846,36 @@ History::UpdatePlaces(const jsval& aPlac
 
   nsTArray<VisitData> visitData;
   for (jsuint i = 0; i < infosLength; i++) {
     JSObject* info;
     nsresult rv = GetJSObjectFromArray(aCtx, infos, i, &info);
     NS_ENSURE_SUCCESS(rv, rv);
 
     nsCOMPtr<nsIURI> uri = GetURIFromJSObject(aCtx, info, "uri");
-    PRInt64 placeId;
-    rv = GetIntFromJSObject(aCtx, info, "placeId", &placeId);
-    if (rv == NS_ERROR_INVALID_ARG) {
-      placeId = 0;
-    }
-    else {
-      NS_ENSURE_SUCCESS(rv, rv);
-      NS_ENSURE_ARG(placeId > 0);
-    }
     nsCString guid;
     {
       nsString fatGUID;
       GetStringFromJSObject(aCtx, info, "guid", fatGUID);
       if (fatGUID.IsVoid()) {
         guid.SetIsVoid(PR_TRUE);
       }
       else {
         guid = NS_ConvertUTF16toUTF8(fatGUID);
       }
     }
 
     // Make sure that any uri we are given can be added to history, and if not,
     // skip it (CanAddURI will notify our callback for us).
-    if (uri && !CanAddURI(uri, guid, placeId, aCallback)) {
+    if (uri && !CanAddURI(uri, guid, aCallback)) {
       continue;
     }
 
-    // We must have at least one of uri, valid id, or guid.
-    NS_ENSURE_ARG(uri || placeId > 0 || !guid.IsVoid());
+    // We must have at least one of uri or guid.
+    NS_ENSURE_ARG(uri || !guid.IsVoid());
 
     // If we were given a guid, make sure it is valid.
     bool isValidGUID = IsValidGUID(guid);
     NS_ENSURE_ARG(guid.IsVoid() || isValidGUID);
 
     nsString title;
     GetStringFromJSObject(aCtx, info, "title", title);
 
@@ -1913,17 +1900,16 @@ History::UpdatePlaces(const jsval& aPlac
     // Check each visit, and build our array of VisitData objects.
     visitData.SetCapacity(visitData.Length() + visitsLength);
     for (jsuint j = 0; j < visitsLength; j++) {
       JSObject* visit;
       rv = GetJSObjectFromArray(aCtx, visits, j, &visit);
       NS_ENSURE_SUCCESS(rv, rv);
 
       VisitData& data = *visitData.AppendElement(VisitData(uri));
-      data.placeId = placeId;
       data.title = title;
       data.guid = guid;
 
       // We must have a date and a transaction type!
       rv = GetIntFromJSObject(aCtx, visit, "visitDate", &data.visitTime);
       NS_ENSURE_SUCCESS(rv, rv);
       PRUint32 transitionType;
       rv = GetIntFromJSObject(aCtx, visit, "transitionType", &transitionType);
--- a/toolkit/components/places/tests/unit/test_async_history_api.js
+++ b/toolkit/components/places/tests/unit/test_async_history_api.js
@@ -193,47 +193,16 @@ function test_invalid_places_throws()
     catch (e) {
       do_check_eq(e.result, Cr.NS_ERROR_INVALID_ARG);
     }
   }
 
   run_next_test();
 }
 
-function test_invalid_id_throws()
-{
-  // First check invalid id "0".
-  let place = {
-    placeId: 0,
-    uri: NetUtil.newURI(TEST_DOMAIN + "test_invalid_id_throws"),
-    visits: [
-      new VisitInfo(),
-    ],
-  };
-  try {
-    gHistory.updatePlaces(place);
-    do_throw("Should have thrown!");
-  }
-  catch (e) {
-    do_check_eq(e.result, Cr.NS_ERROR_INVALID_ARG);
-  }
-
-  // Now check negative id.
-  place.placeId = -5;
-  try {
-    gHistory.updatePlaces(place);
-    do_throw("Should have thrown!");
-  }
-  catch (e) {
-    do_check_eq(e.result, Cr.NS_ERROR_INVALID_ARG);
-  }
-
-  run_next_test();
-}
-
 function test_invalid_guid_throws()
 {
   // First check invalid length guid.
   let place = {
     guid: "BAD_GUID",
     uri: NetUtil.newURI(TEST_DOMAIN + "test_invalid_guid_throws"),
     visits: [
       new VisitInfo(),
@@ -267,45 +236,40 @@ function test_no_visits_throws()
     NetUtil.newURI(TEST_DOMAIN + "test_no_id_or_guid_no_visits_throws");
   const TEST_GUID = "_RANDOMGUID_";
   const TEST_PLACEID = 2;
 
   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;
+      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);
-          }
+        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();
 }
 
@@ -889,17 +853,16 @@ function test_title_change_notifies()
 
 ////////////////////////////////////////////////////////////////////////////////
 //// Test Runner
 
 let gTests = [
   test_interface_exists,
   test_invalid_uri_throws,
   test_invalid_places_throws,
-  test_invalid_id_throws,
   test_invalid_guid_throws,
   test_no_visits_throws,
   test_add_visit_no_date_throws,
   test_add_visit_no_transitionType_throws,
   test_add_visit_invalid_transitionType_throws,
   test_non_addable_uri_errors,
   test_observer_topic_dispatched_when_complete,
   test_add_visit,