Bug 1474638 - Change users of nsIDownloadHistory::removeAllDownloads() to PlacesUtils.history.removeVisitsByFilter. r=mak
authorMark Banner <standard8@mozilla.com>
Wed, 11 Jul 2018 16:04:25 +0000
changeset 425799 43dc237c33f34532c76b9736860d7cd4c1c66ca2
parent 425798 8de2dc60c55bd48ef8b7b6c6637c98af5de398f3
child 425800 4e729ca38f8c420ed39eec6294e3fcd4b6316526
push id34266
push userrgurzau@mozilla.com
push dateWed, 11 Jul 2018 22:03:10 +0000
treeherdermozilla-central@4e729ca38f8c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak
bugs1474638
milestone63.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1474638 - Change users of nsIDownloadHistory::removeAllDownloads() to PlacesUtils.history.removeVisitsByFilter. r=mak Also change PlacesUtils.history.removeVisitsByFilter to be able to remove by transition type. MozReview-Commit-ID: Bkiv0ScUi07 Differential Revision: https://phabricator.services.mozilla.com/D2056
browser/components/downloads/DownloadsSubview.jsm
browser/components/downloads/content/allDownloadsView.js
browser/components/extensions/parent/ext-history.js
toolkit/components/places/History.jsm
toolkit/components/places/tests/history/test_download_history.js
toolkit/components/places/tests/history/test_removeVisitsByFilter.js
toolkit/components/places/tests/history/xpcshell.ini
toolkit/components/places/tests/unit/test_download_history.js
toolkit/components/places/tests/unit/xpcshell.ini
--- a/browser/components/downloads/DownloadsSubview.jsm
+++ b/browser/components/downloads/DownloadsSubview.jsm
@@ -15,16 +15,18 @@ ChromeUtils.defineModuleGetter(this, "Ap
 ChromeUtils.defineModuleGetter(this, "Downloads",
                                "resource://gre/modules/Downloads.jsm");
 ChromeUtils.defineModuleGetter(this, "DownloadsCommon",
                                "resource:///modules/DownloadsCommon.jsm");
 ChromeUtils.defineModuleGetter(this, "DownloadsViewUI",
                                "resource:///modules/DownloadsViewUI.jsm");
 ChromeUtils.defineModuleGetter(this, "FileUtils",
                                "resource://gre/modules/FileUtils.jsm");
+ChromeUtils.defineModuleGetter(this, "PlacesUtils",
+                               "resource://gre/modules/PlacesUtils.jsm");
 
 let gPanelViewInstances = new WeakMap();
 const kRefreshBatchSize = 10;
 const kMaxWaitForIdleMs = 200;
 XPCOMUtils.defineLazyGetter(this, "kButtonLabels", () => {
   return {
     show: DownloadsCommon.strings[AppConstants.platform == "macosx" ? "showMacLabel" : "showLabel"],
     open: DownloadsCommon.strings.openFileLabel,
@@ -235,19 +237,19 @@ class DownloadsSubview extends Downloads
    *
    * @param {DOMNode} button Button that was clicked to call this method.
    */
   static onClearDownloads(button) {
     let instance = gPanelViewInstances.get(button.closest("panelview"));
     if (!instance)
       return;
     instance._downloadsData.removeFinished();
-    Cc["@mozilla.org/browser/download-history;1"]
-      .getService(Ci.nsIDownloadHistory)
-      .removeAllDownloads();
+    PlacesUtils.history.removeVisitsByFilter({
+      transition: PlacesUtils.history.TRANSITIONS.DOWNLOAD
+    }).catch(Cu.reportError);
   }
 
   /**
    * Just before showing the context menu, anchored to a download item, we need
    * to set the right properties to make sure the right menu-items are visible.
    *
    * @param {DOMNode} button The Button the context menu will be anchored to.
    * @param {DOMNode} menu   The context menu.
--- a/browser/components/downloads/content/allDownloadsView.js
+++ b/browser/components/downloads/content/allDownloadsView.js
@@ -8,17 +8,18 @@ ChromeUtils.import("resource://gre/modul
 XPCOMUtils.defineLazyModuleGetters(this, {
   BrowserWindowTracker: "resource:///modules/BrowserWindowTracker.jsm",
   Downloads: "resource://gre/modules/Downloads.jsm",
   DownloadsCommon: "resource:///modules/DownloadsCommon.jsm",
   DownloadsViewUI: "resource:///modules/DownloadsViewUI.jsm",
   FileUtils: "resource://gre/modules/FileUtils.jsm",
   NetUtil: "resource://gre/modules/NetUtil.jsm",
   OS: "resource://gre/modules/osfile.jsm",
-  Services: "resource://gre/modules/Services.jsm"
+  Services: "resource://gre/modules/Services.jsm",
+  PlacesUtils: "resource://gre/modules/PlacesUtils.jsm",
 });
 
 /**
  * A download element shell is responsible for handling the commands and the
  * displayed data for a single download view element.
  *
  * The shell may contain a session download, a history download, or both.  When
  * both a history and a session download are present, the session download gets
@@ -607,19 +608,19 @@ DownloadsPlacesView.prototype = {
 
   cmd_paste() {
     this._downloadURLFromClipboard();
   },
 
   downloadsCmd_clearDownloads() {
     this._downloadsData.removeFinished();
     if (this._place) {
-      Cc["@mozilla.org/browser/download-history;1"]
-        .getService(Ci.nsIDownloadHistory)
-        .removeAllDownloads();
+      PlacesUtils.history.removeVisitsByFilter({
+        transition: PlacesUtils.history.TRANSITIONS.DOWNLOAD
+      }).catch(Cu.reportError);
     }
     // There may be no selection or focus change as a result
     // of these change, and we want the command updated immediately.
     goUpdateCommand("downloadsCmd_clearDownloads");
   },
 
   onContextMenu(aEvent) {
     let element = this._richlistbox.selectedItem;
--- a/browser/components/extensions/parent/ext-history.js
+++ b/browser/components/extensions/parent/ext-history.js
@@ -114,17 +114,19 @@ const getHistoryObserver = () => {
       }
       onClearHistory() {
         this.emit("visitRemoved", {allHistory: true, urls: []});
       }
       onPageChanged() {}
       onFrecencyChanged() {}
       onManyFrecenciesChanged() {}
       onDeleteVisits(uri, partialRemoval, guid, reason) {
-        this.emit("visitRemoved", {allHistory: false, urls: [uri.spec]});
+        if (!partialRemoval) {
+          this.emit("visitRemoved", {allHistory: false, urls: [uri.spec]});
+        }
       }
     }();
     PlacesUtils.observers.addListener(
       ["page-visited"],
       _observer.handlePlacesEvents.bind(_observer));
     PlacesUtils.history.addObserver(_observer);
   }
   return _observer;
--- a/toolkit/components/places/History.jsm
+++ b/toolkit/components/places/History.jsm
@@ -367,16 +367,18 @@ var History = Object.freeze({
    *      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
+   *          - transition: (Integer)
+   *                The type of the transition (see TRANSITIONS below)
    *      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.
@@ -393,26 +395,31 @@ var History = Object.freeze({
     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;
+    let hasTransition = "transition" in filter;
     if (hasBeginDate) {
       this.ensureDate(filter.beginDate);
     }
     if (hasEndDate) {
       this.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 && !hasURL && !hasLimit) {
+    if (hasTransition &&
+        !this.isValidTransition(filter.transition)) {
+      throw new TypeError("`transition` should be valid");
+    }
+    if (!hasBeginDate && !hasEndDate && !hasURL && !hasLimit && !hasTransition) {
       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`");
     }
 
@@ -568,21 +575,21 @@ var History = Object.freeze({
     return PlacesUtils.withConnectionWrapper("History.jsm: clear",
       clear
     );
   },
 
   /**
    * Is a value a valid transition type?
    *
-   * @param transitionType: (String)
+   * @param transition: (String)
    * @return (Boolean)
    */
-  isValidTransition(transitionType) {
-    return Object.values(History.TRANSITIONS).includes(transitionType);
+  isValidTransition(transition) {
+    return Object.values(History.TRANSITIONS).includes(transition);
   },
 
   /**
    * Throw if an object is not a Date object.
    */
   ensureDate(arg) {
     if (!arg || typeof arg != "object" || arg.constructor.name != "Date") {
       throw new TypeError("Expected a Date, got " + arg);
@@ -895,37 +902,35 @@ var cleanupPages = async function(db, pa
  *      Each object should have the following properties:
  *          - id: (number) The `moz_places` identifier for the place.
  *          - hasVisits: (boolean) If `true`, there remains at least one
  *              visit to this page, so the page should be kept and its
  *              frecency updated.
  *          - hasForeign: (boolean) If `true`, the page has at least
  *              one foreign reference (i.e. a bookmark), so the page should
  *              be kept and its frecency updated.
+ * @param transition: (Number)
+ *      Set to a valid TRANSITIONS value to indicate all transitions of a
+ *      certain type have been removed, otherwise defaults to -1 (unknown value).
  * @return (Promise)
  */
-var notifyCleanup = async function(db, pages) {
+var notifyCleanup = async function(db, pages, transition = -1) {
   let notifiedCount = 0;
   let observers = PlacesUtils.history.getObservers();
 
   let reason = Ci.nsINavHistoryObserver.REASON_DELETED;
 
   for (let page of pages) {
     let uri = NetUtil.newURI(page.url.href);
     let guid = page.guid;
-    if (page.hasVisits) {
-      // For the moment, we do not have the necessary observer API
-      // to notify when we remove a subset of visits, see bug 937560.
-      continue;
-    }
-    if (page.hasForeign) {
+    if (page.hasVisits || page.hasForeign) {
       // We have removed all visits, but the page is still alive, e.g.
       // because of a bookmark.
       notify(observers, "onDeleteVisits",
-        [uri, false, guid, reason, -1]);
+        [uri, page.hasVisits > 0, guid, reason, transition]);
     } else {
       // The page has been entirely removed.
       notify(observers, "onDeleteURI",
         [uri, guid, reason]);
     }
     if (++notifiedCount % NOTIFICATION_CHUNK_SIZE == 0) {
       // Every few notifications, yield time back to the main
       // thread to avoid jank.
@@ -1031,27 +1036,33 @@ var fetch = async function(db, guidOrURL
 
 // Inner implementation of History.removeVisitsByFilter.
 var removeVisitsByFilter = 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 conditions = [];
   let args = {};
+  let transition = -1;
   if ("beginDate" in filter) {
     conditions.push("v.visit_date >= :begin * 1000");
     args.begin = Number(filter.beginDate);
   }
   if ("endDate" in filter) {
     conditions.push("v.visit_date <= :end * 1000");
     args.end = Number(filter.endDate);
   }
   if ("limit" in filter) {
     args.limit = Number(filter.limit);
   }
+  if ("transition" in filter) {
+    conditions.push("v.visit_type = :transition");
+    args.transition = filter.transition;
+    transition = filter.transition;
+  }
 
   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;
@@ -1117,17 +1128,17 @@ var removeVisitsByFilter = async functio
            };
            pages.push(page);
          });
 
       // 4. Clean up and notify
       await cleanupPages(db, pages);
     });
 
-    notifyCleanup(db, pages);
+    notifyCleanup(db, pages, transition);
     notifyOnResult(onResultData, onResult); // don't wait
   } finally {
     // Ensure we cleanup embed visits, even if we bailed out early.
     PlacesUtils.history.clearEmbedVisits();
   }
 
   return visitsToRemove.length != 0;
 };
rename from toolkit/components/places/tests/unit/test_download_history.js
rename to toolkit/components/places/tests/history/test_download_history.js
--- a/toolkit/components/places/tests/unit/test_download_history.js
+++ b/toolkit/components/places/tests/history/test_download_history.js
@@ -14,39 +14,38 @@ const REFERRER_URI = NetUtil.newURI("htt
 const PRIVATE_URI = NetUtil.newURI("http://www.example.net/");
 
 /**
  * Waits for the first visit notification to be received.
  *
  * @param aCallback
  *        Callback function to be called with the same arguments of onVisit.
  */
-function waitForOnVisit(aCallback) {
-  function listener(aEvents) {
+async function waitForOnVisit(aCallback) {
+  await PlacesTestUtils.waitForNotification("page-visited", aEvents => {
     Assert.equal(aEvents.length, 1, "Right number of visits notified");
     Assert.equal(aEvents[0].type, "page-visited");
     let {
       url,
       visitId,
       visitTime,
       referringVisitId,
       transitionType,
       pageGuid,
       hidden,
       visitCount,
       typedCount,
       lastKnownTitle,
     } = aEvents[0];
-    PlacesObservers.removeListener(["page-visited"], listener);
     let uriArg = NetUtil.newURI(url);
     aCallback(uriArg, visitId, visitTime, 0, referringVisitId,
               transitionType, pageGuid, hidden, visitCount,
               typedCount, lastKnownTitle);
-  }
-  PlacesObservers.addListener(["page-visited"], listener);
+    return true;
+  }, "places");
 }
 
 /**
  * Waits for the first onDeleteURI notification to be received.
  *
  * @param aCallback
  *        Callback function to be called with the same arguments of onDeleteURI.
  */
@@ -73,88 +72,110 @@ function waitForOnDeleteVisits(aCallback
     onDeleteVisits: function HO_onDeleteVisits() {
       PlacesUtils.history.removeObserver(this);
       aCallback.apply(null, arguments);
     }
   };
   PlacesUtils.history.addObserver(historyObserver);
 }
 
-add_test(function test_dh_is_from_places() {
+add_task(async function test_dh_is_from_places() {
   // Test that this nsIDownloadHistory is the one places implements.
   Assert.ok(gDownloadHistory instanceof Ci.mozIAsyncHistory);
-
-  run_next_test();
 });
 
-add_test(function test_dh_addRemoveDownload() {
-  waitForOnVisit(function DHAD_onVisit(aURI) {
-    Assert.ok(aURI.equals(DOWNLOAD_URI));
+
+async function checkAddAndRemove(expected) {
+  let visitedPromise = PlacesTestUtils.waitForNotification("page-visited",
+    visits => visits[0].url == DOWNLOAD_URI.spec,
+    "places");
 
-    // Verify that the URI is already available in results at this time.
-    Assert.ok(!!page_in_database(DOWNLOAD_URI));
+  let pageInfo = await PlacesUtils.history.insert({
+    url: DOWNLOAD_URI.spec,
+    visits: [{
+      date: new Date(),
+      transition: PlacesUtils.history.TRANSITIONS.DOWNLOAD,
+    }]
+  });
 
-    waitForOnDeleteURI(function DHRAD_onDeleteURI(aDeletedURI) {
-      Assert.ok(aDeletedURI.equals(DOWNLOAD_URI));
+  await visitedPromise;
+
+  Assert.ok(!!page_in_database(DOWNLOAD_URI),
+    "Should have added the page to the database");
 
-      // Verify that the URI is already available in results at this time.
-      Assert.ok(!page_in_database(DOWNLOAD_URI));
+  let notificationArgs;
+  let removedPromise = PlacesTestUtils.waitForNotification(
+    expected.deleteVisitOnly ? "onDeleteVisits" : "onDeleteURI",
+    (...args) => {
+      notificationArgs = args;
+      return true;
+    },
+    "history");
 
-      run_next_test();
-    });
-    gDownloadHistory.removeAllDownloads();
+  await PlacesUtils.history.removeVisitsByFilter({
+    transition: PlacesUtils.history.TRANSITIONS.DOWNLOAD
   });
 
-  gDownloadHistory.addDownload(DOWNLOAD_URI, null, Date.now() * 1000);
+  await removedPromise;
+
+  Assert.equal(notificationArgs[0].spec, DOWNLOAD_URI.spec,
+    "Should have received the correct URI in the notification");
+
+  if (expected.deleteVisitOnly) {
+    Assert.equal(notificationArgs[1], expected.partialRemoval,
+      "Should have received the expected partialRemoval in the notification");
+    Assert.equal(notificationArgs[2], pageInfo.guid,
+      "Should have received the correct GUID in the notification");
+    Assert.equal(notificationArgs[3], Ci.nsINavHistoryObserver.REASON_DELETED,
+      "Should have received the correct reason in the notification");
+    Assert.equal(notificationArgs[4], PlacesUtils.history.TRANSITIONS.DOWNLOAD,
+      "Should have received the correct transition type in the notification");
+    Assert.ok(!!page_in_database(DOWNLOAD_URI),
+      "Should have kept the page in the database.");
+  } else {
+    Assert.equal(notificationArgs[1], pageInfo.guid,
+      "Should have received the correct GUID in the notification");
+    Assert.equal(notificationArgs[2], Ci.nsINavHistoryObserver.REASON_DELETED,
+      "Should have received the correct reason in the notification");
+    Assert.ok(!page_in_database(DOWNLOAD_URI),
+      "Should have removed the page from the database.");
+  }
+}
+
+add_task(async function test_dh_addRemoveDownload() {
+  await checkAddAndRemove({
+    deleteVisitOnly: false,
+    partialRemoval: false,
+  });
 });
 
-add_test(function test_dh_addMultiRemoveDownload() {
-  PlacesTestUtils.addVisits({
+add_task(async function test_dh_addMultiRemoveDownload() {
+  await PlacesTestUtils.addVisits({
     uri: DOWNLOAD_URI,
     transition: TRANSITION_TYPED
-  }).then(function() {
-    waitForOnVisit(function DHAD_onVisit(aURI) {
-      Assert.ok(aURI.equals(DOWNLOAD_URI));
-      Assert.ok(!!page_in_database(DOWNLOAD_URI));
+  });
 
-      waitForOnDeleteVisits(function DHRAD_onDeleteVisits(aDeletedURI) {
-        Assert.ok(aDeletedURI.equals(DOWNLOAD_URI));
-        Assert.ok(!!page_in_database(DOWNLOAD_URI));
+  await checkAddAndRemove({
+    deleteVisitOnly: true,
+    partialRemoval: true,
+  });
 
-        PlacesUtils.history.clear().then(run_next_test);
-      });
-      gDownloadHistory.removeAllDownloads();
-    });
-
-    gDownloadHistory.addDownload(DOWNLOAD_URI, null, Date.now() * 1000);
-  });
+  await PlacesUtils.history.clear();
 });
 
 add_task(async function test_dh_addBookmarkRemoveDownload() {
   await PlacesUtils.bookmarks.insert({
     parentGuid: PlacesUtils.bookmarks.unfiledGuid,
     url: DOWNLOAD_URI,
     title: "A bookmark"
   });
 
-  await new Promise(resolve => {
-    waitForOnVisit(function DHAD_onVisit(aURI) {
-      Assert.ok(aURI.equals(DOWNLOAD_URI));
-      Assert.ok(!!page_in_database(DOWNLOAD_URI));
-
-      waitForOnDeleteVisits(function DHRAD_onDeleteVisits(aDeletedURI) {
-        Assert.ok(aDeletedURI.equals(DOWNLOAD_URI));
-        Assert.ok(!!page_in_database(DOWNLOAD_URI));
-
-        PlacesUtils.history.clear().then(resolve);
-      });
-      gDownloadHistory.removeAllDownloads();
-    });
-
-    gDownloadHistory.addDownload(DOWNLOAD_URI, null, Date.now() * 1000);
+  await checkAddAndRemove({
+    deleteVisitOnly: true,
+    partialRemoval: false,
   });
 });
 
 add_task(async function test_dh_addDownload_referrer() {
   // Wait for visits notification and get the visit id.
   let visitId;
   let referrerPromise = PlacesTestUtils.waitForNotification("page-visited", visits => {
     visitId = visits[0].visitId;
--- a/toolkit/components/places/tests/history/test_removeVisitsByFilter.js
+++ b/toolkit/components/places/tests/history/test_removeVisitsByFilter.js
@@ -315,16 +315,20 @@ add_task(async function test_error_cases
   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/
   );
+  Assert.throws(
+    () => PlacesUtils.history.removeVisitsByFilter({transition: -1}),
+    /TypeError: `transition` should be valid/
+  );
 });
 
 add_task(async function test_orphans() {
   let uri = NetUtil.newURI("http://moz.org/");
   await PlacesTestUtils.addVisits({ uri });
 
   PlacesUtils.favicons.setAndFetchFaviconForPage(
     uri, SMALLPNG_DATA_URI, true, PlacesUtils.favicons.FAVICON_LOAD_NON_PRIVATE,
--- a/toolkit/components/places/tests/history/xpcshell.ini
+++ b/toolkit/components/places/tests/history/xpcshell.ini
@@ -1,12 +1,13 @@
 [DEFAULT]
 head = head_history.js
 
 [test_async_history_api.js]
+[test_download_history.js]
 [test_fetch.js]
 [test_hasVisits.js]
 [test_insert.js]
 [test_insert_null_title.js]
 [test_insertMany.js]
 [test_remove.js]
 [test_removeMany.js]
 [test_removeVisits.js]
--- a/toolkit/components/places/tests/unit/xpcshell.ini
+++ b/toolkit/components/places/tests/unit/xpcshell.ini
@@ -51,17 +51,16 @@ skip-if = os == "linux" # Bug 821781
 [test_bookmarks_html_escape_entities.js]
 [test_bookmarks_html_import_tags.js]
 [test_bookmarks_html_singleframe.js]
 [test_bookmarks_restore_notification.js]
 [test_broken_folderShortcut_result.js]
 [test_browserhistory.js]
 [test_bug636917_isLivemark.js]
 [test_childlessTags.js]
-[test_download_history.js]
 [test_frecency.js]
 [test_frecency_decay.js]
 [test_frecency_zero_updated.js]
 [test_getChildIndex.js]
 [test_hash.js]
 [test_history.js]
 [test_history_clear.js]
 [test_history_notifications.js]