Bug 1285577 - part 0b: expand History.removeVisitsByFilter functionality, r=mak draft
authorGijs Kruitbosch <gijskruitbosch@gmail.com>
Tue, 20 Dec 2016 22:49:17 +0000
changeset 451735 03e47bcca21b951f5402c7335a91e601cb0cabfd
parent 451734 371688e1112a91b6068bbd4780d33231dbaef7fd
child 451736 88a07f15e920d570576d0220f541f156f6da86f9
push id39276
push usergijskruitbosch@gmail.com
push dateTue, 20 Dec 2016 22:50:39 +0000
reviewersmak
bugs1285577
milestone53.0a1
Bug 1285577 - part 0b: expand History.removeVisitsByFilter functionality, r=mak This patch allows History.removeVisitsByFilter to take `limit` and `url` options restricting which visits are removed. MozReview-Commit-ID: KJpSnMSJnUW
toolkit/components/places/History.jsm
toolkit/components/places/tests/history/test_removeVisitsByFilter.js
--- a/toolkit/components/places/History.jsm
+++ b/toolkit/components/places/History.jsm
@@ -323,16 +323,19 @@ this.History = Object.freeze({
    *
    * @param filter: (object)
    *      The `object` may contain some of the following
    *      properties:
    *          - beginDate: (Date) Remove visits that have
    *                been added since this date (inclusive).
    *          - endDate: (Date) Remove visits that have
    *                been added before this date (inclusive).
+   *          - limit: (Number) Limit the number of visits
+   *                we remove to this number
+   *          - url: (URL) Only remove visits to this URL
    *      If both `beginDate` and `endDate` are specified,
    *      visits between `beginDate` (inclusive) and `end`
    *      (inclusive) are removed.
    *
    * @param onResult: (function(VisitInfo), [optional])
    *     A callback invoked for each visit found and removed.
    *     Note that the referrer property of `VisitInfo`
    *     is NOT populated.
@@ -347,29 +350,43 @@ this.History = Object.freeze({
    */
   removeVisitsByFilter: function(filter, onResult = null) {
     if (!filter || typeof filter != "object") {
       throw new TypeError("Expected a filter");
     }
 
     let hasBeginDate = "beginDate" in filter;
     let hasEndDate = "endDate" in filter;
+    let hasURL = "url" in filter;
+    let hasLimit = "limit" in filter;
     if (hasBeginDate) {
       ensureDate(filter.beginDate);
     }
     if (hasEndDate) {
       ensureDate(filter.endDate);
     }
     if (hasBeginDate && hasEndDate && filter.beginDate > filter.endDate) {
       throw new TypeError("`beginDate` should be at least as old as `endDate`");
     }
-    if (!hasBeginDate && !hasEndDate) {
+    if (!hasBeginDate && !hasEndDate && !hasURL && !hasLimit) {
       throw new TypeError("Expected a non-empty filter");
     }
 
+    if (hasURL && !(filter.url instanceof URL) && typeof filter.url != "string" &&
+        !(filter.url instanceof Ci.nsIURI)) {
+      throw new TypeError("Expected a valid URL for `url`");
+    }
+
+    if (hasLimit &&
+        (typeof filter.limit != "number" ||
+         filter.limit <= 0 ||
+         !Number.isInteger(filter.limit))) {
+      throw new TypeError("Expected a non-zero positive integer as a limit");
+    }
+
     if (onResult && typeof onResult != "function") {
       throw new TypeError("Invalid function: " + onResult);
     }
 
     return PlacesUtils.withConnectionWrapper("History.jsm: removeVisitsByFilter",
       db => removeVisitsByFilter(db, filter, onResult)
     );
   },
@@ -775,37 +792,53 @@ var notifyOnResult = Task.async(function
   }
 });
 
 // Inner implementation of History.removeVisitsByFilter.
 var removeVisitsByFilter = Task.async(function*(db, filter, onResult = null) {
   // 1. Determine visits that took place during the interval.  Note
   // that the database uses microseconds, while JS uses milliseconds,
   // so we need to *1000 one way and /1000 the other way.
-  let dates = {
-    conditions: [],
-    args: {},
-  };
+  let conditions = [];
+  let args = {};
   if ("beginDate" in filter) {
-    dates.conditions.push("visit_date >= :begin * 1000");
-    dates.args.begin = Number(filter.beginDate);
+    conditions.push("v.visit_date >= :begin * 1000");
+    args.begin = Number(filter.beginDate);
   }
   if ("endDate" in filter) {
-    dates.conditions.push("visit_date <= :end * 1000");
-    dates.args.end = Number(filter.endDate);
+    conditions.push("v.visit_date <= :end * 1000");
+    args.end = Number(filter.endDate);
+  }
+  if ("limit" in filter) {
+    args.limit = Number(filter.limit);
   }
 
+  let optionalJoin = "";
+  if ("url" in filter) {
+    let url = filter.url;
+    if (url instanceof Ci.nsIURI) {
+      url = filter.url.spec;
+    } else {
+      url = new URL(url).href;
+    }
+    optionalJoin = `JOIN moz_places h ON h.id = v.place_id`;
+    conditions.push("h.url_hash = hash(:url)", "h.url = :url");
+    args.url = url;
+  }
+
+
   let visitsToRemove = [];
   let pagesToInspect = new Set();
   let onResultData = onResult ? [] : null;
 
   yield db.executeCached(
-    `SELECT id, place_id, visit_date / 1000 AS date, visit_type FROM moz_historyvisits
-     WHERE ${ dates.conditions.join(" AND ") }`,
-     dates.args,
+     `SELECT v.id, place_id, visit_date / 1000 AS date, visit_type FROM moz_historyvisits v
+             ${optionalJoin}
+             WHERE ${ conditions.join(" AND ") }${ args.limit ? " LIMIT :limit" : "" }`,
+     args,
      row => {
        let id = row.getResultByName("id");
        let place_id = row.getResultByName("place_id");
        visitsToRemove.push(id);
        pagesToInspect.add(place_id);
 
        if (onResult) {
          onResultData.push({
--- a/toolkit/components/places/tests/history/test_removeVisitsByFilter.js
+++ b/toolkit/components/places/tests/history/test_removeVisitsByFilter.js
@@ -19,25 +19,35 @@ add_task(function* test_removeVisitsByFi
     let SAMPLE_SIZE = options.sampleSize;
 
     yield PlacesTestUtils.clearHistory();
     yield PlacesUtils.bookmarks.eraseEverything();
 
     // Populate the database.
     // Create `SAMPLE_SIZE` visits, from the oldest to the newest.
 
-    let bookmarks = new Set(options.bookmarks);
+    let bookmarkIndices = new Set(options.bookmarks);
     let visits = [];
-    let visitByURI = new Map();
+    let frecencyChangePromises = new Map();
+    let uriDeletePromises = new Map();
+    let getURL = options.url ?
+      i => "http://mozilla.com/test_browserhistory/test_removeVisitsByFilter/removeme/byurl/" + Math.floor(i / (SAMPLE_SIZE / 5)) + "/" :
+      i => "http://mozilla.com/test_browserhistory/test_removeVisitsByFilter/removeme/" + i + "/" + Math.random();
     for (let i = 0; i < SAMPLE_SIZE; ++i) {
-      let spec = "http://mozilla.com/test_browserhistory/test_removeVisitsByFilter/removeme/" + i + "/" + Math.random();
+      let spec = getURL(i);
       let uri = NetUtil.newURI(spec);
       let jsDate = new Date(Number(referenceDate) + 3600 * 1000 * i);
       let dbDate = jsDate * 1000;
-      let hasBookmark = bookmarks.has(i);
+      let hasBookmark = bookmarkIndices.has(i);
+      let hasOwnBookmark = hasBookmark;
+      if (!hasOwnBookmark && options.url) {
+        // Also mark as bookmarked if one of the earlier bookmarked items has the same URL.
+        hasBookmark =
+          options.bookmarks.filter(n => n < i).some(n => visits[n].uri.spec == spec && visits[n].test.hasBookmark);
+      }
       do_print("Generating " + uri.spec + ", " + dbDate);
       let visit = {
         uri,
         title: "visit " + i,
         visitDate: dbDate,
         test: {
           // `visitDate`, as a Date
           jsDate: jsDate,
@@ -48,18 +58,17 @@ add_task(function* test_removeVisitsByFi
           // `true` if there is a bookmark for this URI, i.e. of the page
           // should not be entirely removed.
           hasBookmark: hasBookmark,
           onFrecencyChanged: null,
           onDeleteURI: null,
         },
       };
       visits.push(visit);
-      visitByURI.set(spec, visit);
-      if (hasBookmark) {
+      if (hasOwnBookmark) {
         do_print("Adding a bookmark to visit " + i);
         yield PlacesUtils.bookmarks.insert({
           url: uri,
           parentGuid: PlacesUtils.bookmarks.unfiledGuid,
           title: "test bookmark"
         });
         do_print("Bookmark added");
       }
@@ -78,24 +87,51 @@ add_task(function* test_removeVisitsByFi
       filter.beginDate = new Date(ms);
       beginIndex = options.begin;
     }
     if ("end" in options) {
       let ms = Number(visits[options.end].test.jsDate) + 1000;
       filter.endDate = new Date(ms);
       endIndex = options.end;
     }
-    for (let i = beginIndex; i <= endIndex; ++i) {
-      let test = visits[i].test;
-      do_print("Marking visit " + i + " as expecting removal");
+    if ("limit" in options) {
+      endIndex = beginIndex + options.limit - 1; // -1 because the start index is inclusive.
+      filter.limit = options.limit;
+    }
+    let removedItems = visits.slice(beginIndex);
+    endIndex -= beginIndex;
+    if (options.url) {
+      let rawURL = "";
+      switch (options.url) {
+        case 1:
+          filter.url = new URL(removedItems[0].uri.spec);
+          rawURL = filter.url.href;
+          break;
+        case 2:
+          filter.url = removedItems[0].uri;
+          rawURL = filter.url.spec;
+          break;
+        case 3:
+          filter.url = removedItems[0].uri.spec;
+          rawURL = filter.url;
+          break;
+      }
+      endIndex = Math.min(endIndex, removedItems.findIndex((v, index) => v.uri.spec != rawURL) - 1);
+    }
+    removedItems.splice(endIndex + 1);
+    let remainingItems = visits.filter(v => !removedItems.includes(v));
+    for (let i = 0; i < removedItems.length; i++) {
+      let test = removedItems[i].test;
+      do_print("Marking visit " + (beginIndex + i) + " as expecting removal");
       test.toRemove = true;
-      if (test.hasBookmark) {
-        test.onFrecencyChanged = PromiseUtils.defer();
-      } else {
-        test.onDeleteURI = PromiseUtils.defer();
+      if (test.hasBookmark ||
+          (options.url && remainingItems.some(v => v.uri.spec == removedItems[i].uri.spec))) {
+        frecencyChangePromises.set(removedItems[i].uri.spec, PromiseUtils.defer());
+      } else if (!options.url || i == 0) {
+        uriDeletePromises.set(removedItems[i].uri.spec, PromiseUtils.defer());
       }
     }
 
     let observer = {
       deferred: PromiseUtils.defer(),
       onBeginUpdateBatch: function() {},
       onEndUpdateBatch: function() {},
       onVisit: function(uri) {
@@ -107,33 +143,31 @@ add_task(function* test_removeVisitsByFi
       onClearHistory: function() {
         this.deferred.reject("Unexpected call to onClearHistory");
       },
       onPageChanged: function(uri) {
         this.deferred.reject(new Error("Unexpected call to onPageChanged " + uri.spec));
       },
       onFrecencyChanged: function(aURI) {
         do_print("onFrecencyChanged " + aURI.spec);
-        let visit = visitByURI.get(aURI.spec);
-        Assert.ok(!!visit.test.onFrecencyChanged, "Observing onFrecencyChanged");
-        visit.test.onFrecencyChanged.resolve();
+        let deferred = frecencyChangePromises.get(aURI.spec);
+        Assert.ok(!!deferred, "Observing onFrecencyChanged");
+        deferred.resolve();
       },
       onManyFrecenciesChanged: function() {
         do_print("Many frecencies changed");
-        for (let visit of visits) {
-          if (visit.onFrecencyChanged) {
-            visit.onFrecencyChanged.resolve();
-          }
+        for (let [, deferred] of frecencyChangePromises) {
+          deferred.resolve();
         }
       },
       onDeleteURI: function(aURI) {
         do_print("onDeleteURI " + aURI.spec);
-        let visit = visitByURI.get(aURI.spec);
-        Assert.ok(!!visit.test.onDeleteURI, "Observing onDeleteURI");
-        visit.test.onDeleteURI.resolve();
+        let deferred = uriDeletePromises.get(aURI.spec);
+        Assert.ok(!!deferred, "Observing onDeleteURI");
+        deferred.resolve();
       },
       onDeleteVisits: function(aURI) {
         // Not sure we can test anything.
       }
     };
     PlacesUtils.history.addObserver(observer, false);
 
     let cbarg;
@@ -161,67 +195,71 @@ add_task(function* test_removeVisitsByFi
 
     Assert.ok(result, "Removal succeeded");
 
     // Make sure that we have eliminated exactly the entries we expected
     // to eliminate.
     for (let i = 0; i < visits.length; ++i) {
       let visit = visits[i];
       do_print("Controlling the results on visit " + i);
+      let remainingVisitsForURI = remainingItems.filter(v => visit.uri.spec == v.uri.spec).length;
       Assert.equal(
-        visits_in_database(visit.uri) == 0,
-        visit.test.toRemove,
+        visits_in_database(visit.uri),
+        remainingVisitsForURI,
         "Visit is still present iff expected");
       if (options.useCallback) {
         Assert.equal(
           visit.test.toRemove,
           visit.test.announcedByOnRow,
           "Visit removal has been announced by onResult iff expected");
       }
-      if (visit.test.hasBookmark || !visit.test.toRemove) {
-        Assert.notEqual(page_in_database(visit.uri), 0, "The page is should still appear in the db");
+      if (visit.test.hasBookmark || remainingVisitsForURI) {
+        Assert.notEqual(page_in_database(visit.uri), 0, "The page should still appear in the db");
       } else {
         Assert.equal(page_in_database(visit.uri), 0, "The page should have been removed from the db");
       }
     }
 
     // Make sure that the observer has been called wherever applicable.
-    for (let visit of visits) {
-      do_print("Making sure that the observer has been called for " + visit.uri.spec);
-      let test = visit.test;
-      do_print("Checking onFrecencyChanged");
-      if (test.onFrecencyChanged) {
-        yield test.onFrecencyChanged.promise;
-      }
-      do_print("Checking onDeleteURI");
-      if (test.onDeleteURI) {
-        yield test.onDeleteURI.promise;
-      }
-    }
+    do_print("Checking URI delete promises.");
+    yield Promise.all(Array.from(uriDeletePromises.values()));
+    do_print("Checking frecency change promises.");
+    yield Promise.all(Array.from(frecencyChangePromises.values()));
     PlacesUtils.history.removeObserver(observer);
   });
 
   let size = 20;
   for (let range of [
     {begin: 0},
     {end: 19},
     {begin: 0, end: 10},
     {begin: 3, end: 4},
+    {begin: 5, end: 8, limit: 2},
+    {begin: 10, end: 18, limit: 5},
   ]) {
     for (let bookmarks of [[], [5, 6]]) {
       let options = {
         sampleSize: size,
         bookmarks: bookmarks,
       };
       if ("begin" in range) {
         options.begin = range.begin;
       }
       if ("end" in range) {
         options.end = range.end;
       }
+      if ("limit" in range) {
+        options.limit = range.limit;
+      }
+      yield remover(options);
+      options.url = 1;
+      yield remover(options);
+      options.url = 2;
+      yield remover(options);
+      options.url = 3;
       yield remover(options);
     }
   }
   yield PlacesTestUtils.clearHistory();
 });
 
 // Test the various error cases
 add_task(function* test_error_cases() {
@@ -248,16 +286,48 @@ add_task(function* test_error_cases() {
   Assert.throws(
     () => PlacesUtils.history.removeVisitsByFilter({beginDate: new Date()}, "obviously, not a callback"),
     /TypeError: Invalid function/
   );
   Assert.throws(
     () => PlacesUtils.history.removeVisitsByFilter({beginDate: new Date(1000), endDate: new Date(0)}),
     /TypeError: `beginDate` should be at least as old/
   );
+  Assert.throws(
+    () => PlacesUtils.history.removeVisitsByFilter({limit: {}}),
+    /Expected a non-zero positive integer as a limit/
+  );
+  Assert.throws(
+    () => PlacesUtils.history.removeVisitsByFilter({limit: -1}),
+    /Expected a non-zero positive integer as a limit/
+  );
+  Assert.throws(
+    () => PlacesUtils.history.removeVisitsByFilter({limit: 0.1}),
+    /Expected a non-zero positive integer as a limit/
+  );
+  Assert.throws(
+    () => PlacesUtils.history.removeVisitsByFilter({limit: Infinity}),
+    /Expected a non-zero positive integer as a limit/
+  );
+  Assert.throws(
+    () => PlacesUtils.history.removeVisitsByFilter({url: {}}),
+    /Expected a valid URL for `url`/
+  );
+  Assert.throws(
+    () => PlacesUtils.history.removeVisitsByFilter({url: 0}),
+    /Expected a valid URL for `url`/
+  );
+  Assert.throws(
+    () => PlacesUtils.history.removeVisitsByFilter({beginDate: new Date(1000), endDate: new Date(0)}),
+    /TypeError: `beginDate` should be at least as old/
+  );
+  Assert.throws(
+    () => PlacesUtils.history.removeVisitsByFilter({beginDate: new Date(1000), endDate: new Date(0)}),
+    /TypeError: `beginDate` should be at least as old/
+  );
 });
 
 add_task(function* test_orphans() {
   let uri = NetUtil.newURI("http://moz.org/");
   yield PlacesTestUtils.addVisits({ uri });
 
   PlacesUtils.favicons.setAndFetchFaviconForPage(
     uri, SMALLPNG_DATA_URI, true,  PlacesUtils.favicons.FAVICON_LOAD_NON_PRIVATE,