Bug 1220654 - Replace removeVisitsTimeframe with History.removeVisitsByFilter and deprecate it. r=mak

***
yield on removeVisitsByFilter in tests

MozReview-Commit-ID: JGDaJGEmtoD
***
update bookmarked place tests, inline object declarations

MozReview-Commit-ID: Cxec22UHui0
***
updates for code review

MozReview-Commit-ID: AIRqrJ0B6GQ

services/sync/tps/extensions/tps/resource/modules/history.jsm

@@ -189,9 +189,16 @@ var HistoryEntry = {
       PlacesUtils.history.removePagesFromHost(item.host, false);
     }
     else if ("begin" in item && "end" in item) {
-      PlacesUtils.history.removeVisitsByTimeframe(
-          usSinceEpoch + (item.begin * 60 * 60 * 1000 * 1000),
-          usSinceEpoch + (item.end * 60 * 60 * 1000 * 1000));
+      let cb = Async.makeSpinningCallback();
+      let msSinceEpoch = parseInt(usSinceEpoch / 1000);
+      let filter = {
+        beginDate: new Date(msSinceEpoch + (item.begin * 60 * 60 * 1000)),
+        endDate: new Date(msSinceEpoch + (item.end * 60 * 60 * 1000))
+      };
+      PlacesUtils.history.removeVisitsByFilter(filter)
+      .catch(ex => Logger.AssertTrue(false, "An error occurred while deleting history: " + ex))
+      .then(result => {cb(null, result)}, err => {cb(err)});
+      Async.waitForSyncCallback(cb);
     }
     else {
       Logger.AssertTrue(false, "invalid entry in delete history");

toolkit/components/places/nsIBrowserHistory.idl

@@ -79,6 +79,8 @@ interface nsIBrowserHistory : nsISupports
      *        Microseconds from epoch, representing the final time.
      *
      * @note The removal happens in a batch.
+     *
+     * @deprecated Please use PlacesUtils.history.removeVisitsByFilter instead
      */
     void removeVisitsByTimeframe(in PRTime aBeginTime,
                                  in PRTime aEndTime);

toolkit/components/places/nsNavHistory.cpp

@@ -2703,6 +2703,8 @@ nsNavHistory::RemovePagesByTimeframe(PRTime aBeginTime, PRTime aEndTime)
 NS_IMETHODIMP
 nsNavHistory::RemoveVisitsByTimeframe(PRTime aBeginTime, PRTime aEndTime)
 {
+  PLACES_WARN_DEPRECATED();
+
   NS_ASSERTION(NS_IsMainThread(), "This can only be called on the main thread");
 
   nsresult rv;

toolkit/components/places/tests/history/test_removeVisits.js

@@ -1,10 +1,5 @@
-/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */
-/* vim:set ts=2 sw=2 sts=2 et: */
-/* This Source Code Form is subject to the terms of the Mozilla Public
- * License, v. 2.0. If a copy of the MPL was not distributed with this
- * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
-
-const NOW = Date.now() * 1000;
+const JS_NOW = Date.now();
+const DB_NOW = JS_NOW * 1000;
 const TEST_URI = uri("http://example.com/");
 const PLACE_URI = uri("place:queryType=0&sort=8&maxResults=10");
 
@@ -21,12 +16,16 @@ add_task(function* remove_visits_outside_unbookmarked_uri() {
   do_print("Add 10 visits for the URI from way in the past.");
   let visits = [];
   for (let i = 0; i < 10; i++) {
-    visits.push({ uri: TEST_URI, visitDate: NOW - 1000 - i });
+    visits.push({ uri: TEST_URI, visitDate: DB_NOW - 100000 - (i * 1000) });
   }
   yield PlacesTestUtils.addVisits(visits);
 
   do_print("Remove visits using timerange outside the URI's visits.");
-  PlacesUtils.history.removeVisitsByTimeframe(NOW - 10, NOW);
+  let filter = {
+    beginDate: new Date(JS_NOW - 10),
+    endDate: new Date(JS_NOW)
+  };
+  yield PlacesUtils.history.removeVisitsByFilter(filter);
   yield PlacesTestUtils.promiseAsyncUpdates();
 
   do_print("URI should still exist in moz_places.");
@@ -42,7 +41,7 @@ add_task(function* remove_visits_outside_unbookmarked_uri() {
   do_check_eq(root.childCount, 10);
   for (let i = 0; i < root.childCount; i++) {
     let visitTime = root.getChild(i).time;
-    do_check_eq(visitTime, NOW - 1000 - i);
+    do_check_eq(visitTime, DB_NOW - 100000 - (i * 1000));
   }
   root.containerOpen = false;
 
@@ -62,7 +61,7 @@ add_task(function* remove_visits_outside_bookmarked_uri() {
   do_print("Add 10 visits for the URI from way in the past.");
   let visits = [];
   for (let i = 0; i < 10; i++) {
-    visits.push({ uri: TEST_URI, visitDate: NOW - 1000 - i });
+    visits.push({ uri: TEST_URI, visitDate: DB_NOW - 100000 - (i * 1000) });
   }
   yield PlacesTestUtils.addVisits(visits);
   do_print("Bookmark the URI.");
@@ -73,7 +72,11 @@ add_task(function* remove_visits_outside_bookmarked_uri() {
   yield PlacesTestUtils.promiseAsyncUpdates();
 
   do_print("Remove visits using timerange outside the URI's visits.");
-  PlacesUtils.history.removeVisitsByTimeframe(NOW - 10, NOW);
+  let filter = {
+    beginDate: new Date(JS_NOW - 10),
+    endDate: new Date(JS_NOW)
+  };
+  yield PlacesUtils.history.removeVisitsByFilter(filter);
   yield PlacesTestUtils.promiseAsyncUpdates();
 
   do_print("URI should still exist in moz_places.");
@@ -89,7 +92,7 @@ add_task(function* remove_visits_outside_bookmarked_uri() {
   do_check_eq(root.childCount, 10);
   for (let i = 0; i < root.childCount; i++) {
     let visitTime = root.getChild(i).time;
-    do_check_eq(visitTime, NOW - 1000 - i);
+    do_check_eq(visitTime, DB_NOW - 100000 - (i * 1000));
   }
   root.containerOpen = false;
 
@@ -109,12 +112,16 @@ add_task(function* remove_visits_unbookmarked_uri() {
   do_print("Add 10 visits for the URI from now to 9 usecs in the past.");
   let visits = [];
   for (let i = 0; i < 10; i++) {
-    visits.push({ uri: TEST_URI, visitDate: NOW - i });
+    visits.push({ uri: TEST_URI, visitDate: DB_NOW - (i * 1000) });
   }
   yield PlacesTestUtils.addVisits(visits);
 
   do_print("Remove the 5 most recent visits.");
-  PlacesUtils.history.removeVisitsByTimeframe(NOW - 4, NOW);
+  let filter = {
+    beginDate: new Date(JS_NOW - 4),
+    endDate: new Date(JS_NOW)
+  };
+  yield PlacesUtils.history.removeVisitsByFilter(filter);
   yield PlacesTestUtils.promiseAsyncUpdates();
 
   do_print("URI should still exist in moz_places.");
@@ -130,7 +137,7 @@ add_task(function* remove_visits_unbookmarked_uri() {
   do_check_eq(root.childCount, 5);
   for (let i = 0; i < root.childCount; i++) {
     let visitTime = root.getChild(i).time;
-    do_check_eq(visitTime, NOW - i - 5);
+    do_check_eq(visitTime, DB_NOW - (i * 1000) - 5000);
   }
   root.containerOpen = false;
 
@@ -150,7 +157,7 @@ add_task(function* remove_visits_bookmarked_uri() {
   do_print("Add 10 visits for the URI from now to 9 usecs in the past.");
   let visits = [];
   for (let i = 0; i < 10; i++) {
-    visits.push({ uri: TEST_URI, visitDate: NOW - i });
+    visits.push({ uri: TEST_URI, visitDate: DB_NOW - (i * 1000) });
   }
   yield PlacesTestUtils.addVisits(visits);
   do_print("Bookmark the URI.");
@@ -161,7 +168,11 @@ add_task(function* remove_visits_bookmarked_uri() {
   yield PlacesTestUtils.promiseAsyncUpdates();
 
   do_print("Remove the 5 most recent visits.");
-  PlacesUtils.history.removeVisitsByTimeframe(NOW - 4, NOW);
+  let filter = {
+    beginDate: new Date(JS_NOW - 4),
+    endDate: new Date(JS_NOW)
+  };
+  yield PlacesUtils.history.removeVisitsByFilter(filter);
   yield PlacesTestUtils.promiseAsyncUpdates();
 
   do_print("URI should still exist in moz_places.");
@@ -177,7 +188,7 @@ add_task(function* remove_visits_bookmarked_uri() {
   do_check_eq(root.childCount, 5);
   for (let i = 0; i < root.childCount; i++) {
     let visitTime = root.getChild(i).time;
-    do_check_eq(visitTime, NOW - i - 5);
+    do_check_eq(visitTime, DB_NOW - (i * 1000) - 5000);
   }
   root.containerOpen = false;
 
@@ -197,12 +208,16 @@ add_task(function* remove_all_visits_unbookmarked_uri() {
   do_print("Add some visits for the URI.");
   let visits = [];
   for (let i = 0; i < 10; i++) {
-    visits.push({ uri: TEST_URI, visitDate: NOW - i });
+    visits.push({ uri: TEST_URI, visitDate: DB_NOW - (i * 1000) });
   }
   yield PlacesTestUtils.addVisits(visits);
 
   do_print("Remove all visits.");
-  PlacesUtils.history.removeVisitsByTimeframe(NOW - 10, NOW);
+  let filter = {
+    beginDate: new Date(JS_NOW - 10),
+    endDate: new Date(JS_NOW)
+  };
+  yield PlacesUtils.history.removeVisitsByFilter(filter);
   yield PlacesTestUtils.promiseAsyncUpdates();
 
   do_print("URI should no longer exist in moz_places.");
@@ -224,49 +239,13 @@ add_task(function* remove_all_visits_unbookmarked_uri() {
   yield cleanup();
 });
 
-add_task(function* remove_all_visits_unbookmarked_place_uri() {
-  do_print("*** TEST: Remove all visits from an unbookmarked place: URI");
-  do_print("Add some visits for the URI.");
-  let visits = [];
-  for (let i = 0; i < 10; i++) {
-    visits.push({ uri: PLACE_URI, visitDate: NOW - i });
-  }
-  yield PlacesTestUtils.addVisits(visits);
-
-  do_print("Remove all visits.");
-  PlacesUtils.history.removeVisitsByTimeframe(NOW - 10, NOW);
-  yield PlacesTestUtils.promiseAsyncUpdates();
-
-  do_print("URI should still exist in moz_places.");
-  do_check_true(page_in_database(PLACE_URI.spec));
-
-  do_print("Run a history query and check that no visits exist.");
-  let query = PlacesUtils.history.getNewQuery();
-  let opts = PlacesUtils.history.getNewQueryOptions();
-  opts.resultType = opts.RESULTS_AS_VISIT;
-  opts.sortingMode = opts.SORT_BY_DATE_DESCENDING;
-  let root = PlacesUtils.history.executeQuery(query, opts).root;
-  root.containerOpen = true;
-  do_check_eq(root.childCount, 0);
-  root.containerOpen = false;
-
-  do_print("asyncHistory.isURIVisited should return false.");
-  do_check_false(yield promiseIsURIVisited(PLACE_URI));
-  yield PlacesTestUtils.promiseAsyncUpdates();
-
-  do_print("Frecency should be zero.")
-  do_check_eq(frecencyForUrl(PLACE_URI.spec), 0);
-
-  yield cleanup();
-});
-
 add_task(function* remove_all_visits_bookmarked_uri() {
   do_print("*** TEST: Remove all visits from a bookmarked URI");
 
   do_print("Add some visits for the URI.");
   let visits = [];
   for (let i = 0; i < 10; i++) {
-    visits.push({ uri: TEST_URI, visitDate: NOW - i });
+    visits.push({ uri: TEST_URI, visitDate: DB_NOW - (i * 1000) });
   }
   yield PlacesTestUtils.addVisits(visits);
   do_print("Bookmark the URI.");
@@ -275,9 +254,14 @@ add_task(function* remove_all_visits_bookmarked_uri() {
                                        PlacesUtils.bookmarks.DEFAULT_INDEX,
                                        "bookmark title");
   yield PlacesTestUtils.promiseAsyncUpdates();
+  let initialFrecency = frecencyForUrl(TEST_URI);
 
   do_print("Remove all visits.");
-  PlacesUtils.history.removeVisitsByTimeframe(NOW - 10, NOW);
+  let filter = {
+    beginDate: new Date(JS_NOW - 10),
+    endDate: new Date(JS_NOW)
+  };
+  yield PlacesUtils.history.removeVisitsByFilter(filter);
   yield PlacesTestUtils.promiseAsyncUpdates();
 
   do_print("URI should still exist in moz_places.");
@@ -300,8 +284,8 @@ add_task(function* remove_all_visits_bookmarked_uri() {
   do_check_true(PlacesUtils.bookmarks.isBookmarked(TEST_URI));
   yield PlacesTestUtils.promiseAsyncUpdates();
 
-  do_print("Frecency should be negative.")
-  do_check_true(frecencyForUrl(TEST_URI) < 0);
+  do_print("Frecency should be smaller.")
+  do_check_true(frecencyForUrl(TEST_URI) < initialFrecency);
 
   yield cleanup();
 });
@@ -311,12 +295,16 @@ add_task(function* remove_all_visits_bookmarked_uri() {
 
   do_print("Add some visits for the URI.");
   yield PlacesTestUtils.addVisits([
-    { uri: TEST_URI, transition: TRANSITION_FRAMED_LINK, visitDate: (NOW - 86400000000) },
-    { uri: TEST_URI, transition: TRANSITION_FRAMED_LINK, visitDate: NOW }
+    { uri: TEST_URI, transition: TRANSITION_FRAMED_LINK, visitDate: (DB_NOW - 86400000000000) },
+    { uri: TEST_URI, transition: TRANSITION_FRAMED_LINK, visitDate: DB_NOW }
   ]);
 
   do_print("Remove newer visit.");
-  PlacesUtils.history.removeVisitsByTimeframe(NOW - 10, NOW);
+  let filter = {
+    beginDate: new Date(JS_NOW - 10),
+    endDate: new Date(JS_NOW)
+  };
+  yield PlacesUtils.history.removeVisitsByFilter(filter);
   yield PlacesTestUtils.promiseAsyncUpdates();
 
   do_print("URI should still exist in moz_places.");

toolkit/components/places/tests/history/xpcshell.ini

@@ -3,4 +3,5 @@ head = head_history.js
 tail =
 
 [test_remove.js]
+[test_removeVisits.js]
 [test_removeVisitsByFilter.js]

toolkit/components/places/tests/unit/xpcshell.ini

@@ -73,7 +73,7 @@ skip-if = (os == "win" && os_version == "5.1") # Bug 1158887
 [test_bookmark_catobs.js]
 [test_bookmarks_json.js]
 [test_bookmarks_html.js]
-[test_bookmarks_html_corrupt.js]
+[test_bookmarks_html_corrupt.js]
 [test_bookmarks_html_import_tags.js]
 [test_bookmarks_html_singleframe.js]
 [test_bookmarks_restore_notification.js]
@@ -134,7 +134,6 @@ skip-if = os == "android"
 skip-if = os == "android"
 [test_preventive_maintenance_runTasks.js]
 [test_promiseBookmarksTree.js]
-[test_removeVisitsByTimeframe.js]
 # Bug 676989: test hangs consistently on Android
 skip-if = os == "android"
 [test_resolveNullBookmarkTitles.js]