Bug 1354531 - Remove PlacesUtils.asyncHistory to make History internals access less convenient. r=standard8 draft
authorMarco Bonardo <mbonardo@mozilla.com>
Thu, 03 May 2018 15:08:06 +0200
changeset 791106 1f9c19911602
parent 790321 258fa1eb49c9
push id108696
push usermak77@bonardo.net
push dateThu, 03 May 2018 14:21:22 +0000
reviewersstandard8
bugs1354531
milestone61.0a1
Bug 1354531 - Remove PlacesUtils.asyncHistory to make History internals access less convenient. r=standard8 MozReview-Commit-ID: K9oQTJbVAZF
browser/components/extensions/test/xpcshell/test_ext_history.js
services/sync/modules/engines/history.js
services/sync/tests/unit/test_history_engine.js
toolkit/components/downloads/test/unit/test_DownloadList.js
toolkit/components/places/History.jsm
toolkit/components/places/PlacesUtils.jsm
toolkit/components/places/nsNavHistoryResult.cpp
toolkit/components/places/tests/PlacesTestUtils.jsm
toolkit/components/places/tests/browser/head.js
toolkit/components/places/tests/history/test_async_history_api.js
toolkit/components/places/tests/history/test_sameUri_titleChanged.js
toolkit/components/places/tests/history/test_updatePlaces_embed.js
--- a/browser/components/extensions/test/xpcshell/test_ext_history.js
+++ b/browser/components/extensions/test/xpcshell/test_ext_history.js
@@ -408,17 +408,17 @@ add_task(async function test_transition_
   const VISIT_URL_PREFIX = "http://example.com/";
   const TRANSITIONS = [
     ["link", Ci.nsINavHistoryService.TRANSITION_LINK],
     ["typed", Ci.nsINavHistoryService.TRANSITION_TYPED],
     ["auto_bookmark", Ci.nsINavHistoryService.TRANSITION_BOOKMARK],
     // Only session history contains TRANSITION_EMBED visits,
     // So global history query cannot find them.
     // ["auto_subframe", Ci.nsINavHistoryService.TRANSITION_EMBED],
-    // Redirects are not correctly tested here because History::UpdatePlaces
+    // Redirects are not correctly tested here because History
     // will not make redirect entries hidden.
     ["link", Ci.nsINavHistoryService.TRANSITION_REDIRECT_PERMANENT],
     ["link", Ci.nsINavHistoryService.TRANSITION_REDIRECT_TEMPORARY],
     ["link", Ci.nsINavHistoryService.TRANSITION_DOWNLOAD],
     ["manual_subframe", Ci.nsINavHistoryService.TRANSITION_FRAMED_LINK],
     ["reload", Ci.nsINavHistoryService.TRANSITION_RELOAD],
   ];
 
--- a/services/sync/modules/engines/history.js
+++ b/services/sync/modules/engines/history.js
@@ -418,17 +418,17 @@ HistoryStore.prototype = {
       curVisits.add(visitKey);
 
       visit.transition = visit.type;
       k += 1;
     }
     record.visits.length = k; // truncate array
 
     // No update if there aren't any visits to apply.
-    // mozIAsyncHistory::updatePlaces() wants at least one visit.
+    // History wants at least one visit.
     // In any case, the only thing we could change would be the title
     // and that shouldn't change without a visit.
     if (!record.visits.length) {
       this._log.trace("Ignoring record " + record.id + " with URI "
                       + record.uri.spec + ": no visits to add.");
       return false;
     }
 
--- a/services/sync/tests/unit/test_history_engine.js
+++ b/services/sync/tests/unit/test_history_engine.js
@@ -1,30 +1,34 @@
 /* Any copyright is dedicated to the Public Domain.
    http://creativecommons.org/publicdomain/zero/1.0/ */
 
 ChromeUtils.import("resource://services-sync/service.js");
 ChromeUtils.import("resource://services-sync/engines/history.js");
 ChromeUtils.import("resource://services-common/utils.js");
 
+// Use only for rawAddVisit.
+XPCOMUtils.defineLazyServiceGetter(this, "asyncHistory",
+                                   "@mozilla.org/browser/history;1",
+                                   "mozIAsyncHistory");
 async function rawAddVisit(id, uri, visitPRTime, transitionType) {
   return new Promise((resolve, reject) => {
     let results = [];
     let handler = {
       handleResult(result) {
         results.push(result);
       },
       handleError(resultCode, placeInfo) {
         do_throw(`updatePlaces gave error ${resultCode}!`);
       },
       handleCompletion(count) {
         resolve({ results, count });
       }
     };
-    PlacesUtils.asyncHistory.updatePlaces([{
+    asyncHistory.updatePlaces([{
       guid: id,
       uri: typeof uri == "string" ? CommonUtils.makeURI(uri) : uri,
       visits: [{ visitDate: visitPRTime, transitionType }]
     }], handler);
   });
 }
 
 
@@ -142,17 +146,17 @@ add_task(async function test_history_vis
   engine._tracker.start();
 
   let id = "aaaaaaaaaaaa";
   let oneHourMS = 60 * 60 * 1000;
   // Insert a visit with a non-round microsecond timestamp (e.g. it's not evenly
   // divisible by 1000). This will typically be the case for visits that occur
   // during normal navigation.
   let time = (Date.now() - oneHourMS) * 1000 + 555;
-  // We use the low level updatePlaces api since it lets us provide microseconds
+  // We use the low level history api since it lets us provide microseconds
   let {count} = await rawAddVisit(id, "https://www.example.com", time,
                                   PlacesUtils.history.TRANSITIONS.TYPED);
   equal(count, 1);
   // Check that it was inserted and that we didn't round on the insert.
   let visits = await PlacesSyncUtils.history.fetchVisitsForURL("https://www.example.com");
   equal(visits.length, 1);
   equal(visits[0].date, time);
 
--- a/toolkit/components/downloads/test/unit/test_DownloadList.js
+++ b/toolkit/components/downloads/test/unit/test_DownloadList.js
@@ -7,63 +7,48 @@
  * Tests the DownloadList object.
  */
 
 "use strict";
 
 // Globals
 
 /**
- * Returns a PRTime in the past usable to add expirable visits.
+ * Returns a Date in the past usable to add expirable visits.
  *
  * @note Expiration ignores any visit added in the last 7 days, but it's
  *       better be safe against DST issues, by going back one day more.
  */
-function getExpirablePRTime() {
+function getExpirableDate() {
   let dateObj = new Date();
   // Normalize to midnight
   dateObj.setHours(0);
   dateObj.setMinutes(0);
   dateObj.setSeconds(0);
   dateObj.setMilliseconds(0);
-  dateObj = new Date(dateObj.getTime() - 8 * 86400000);
-  return dateObj.getTime() * 1000;
+  return new Date(dateObj.getTime() - 8 * 86400000);
 }
 
 /**
  * Adds an expirable history visit for a download.
  *
  * @param aSourceUrl
  *        String containing the URI for the download source, or null to use
  *        httpUrl("source.txt").
  *
  * @return {Promise}
  * @rejects JavaScript exception.
  */
 function promiseExpirableDownloadVisit(aSourceUrl) {
-  return new Promise((resolve, reject) => {
-    PlacesUtils.asyncHistory.updatePlaces(
-      {
-        uri: NetUtil.newURI(aSourceUrl || httpUrl("source.txt")),
-        visits: [{
-          transitionType: Ci.nsINavHistoryService.TRANSITION_DOWNLOAD,
-          visitDate: getExpirablePRTime(),
-        }]
-      },
-      {
-        handleError: function handleError(aResultCode, aPlaceInfo) {
-          let ex = new Components.Exception("Unexpected error in adding visits.",
-                                            aResultCode);
-          reject(ex);
-        },
-        handleResult() {},
-        handleCompletion: function handleCompletion() {
-          resolve();
-        }
-      });
+  return PlacesUtils.history.insert({
+    url: aSourceUrl || httpUrl("source.txt"),
+    visits: [{
+      transition: PlacesUtils.history.TRANSITIONS.DOWNLOAD,
+      date: getExpirableDate(),
+    }]
   });
 }
 
 // Tests
 
 /**
  * Checks the testing mechanism used to build different download lists.
  */
--- a/toolkit/components/places/History.jsm
+++ b/toolkit/components/places/History.jsm
@@ -68,16 +68,21 @@
 
 var EXPORTED_SYMBOLS = [ "History" ];
 
 ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
 ChromeUtils.defineModuleGetter(this, "NetUtil",
                                "resource://gre/modules/NetUtil.jsm");
 ChromeUtils.defineModuleGetter(this, "PlacesUtils",
                                "resource://gre/modules/PlacesUtils.jsm");
+
+XPCOMUtils.defineLazyServiceGetter(this, "asyncHistory",
+                                   "@mozilla.org/browser/history;1",
+                                   "mozIAsyncHistory");
+
 Cu.importGlobalProperties(["URL"]);
 
 /**
  * Whenever we update or remove numerous pages, it is preferable
  * to yield time to the main thread every so often to avoid janking.
  * These constants determine the maximal number of notifications we
  * may emit before we yield.
  */
@@ -521,17 +526,17 @@ var History = Object.freeze({
    * @throws (Error)
    *      If `guidOrURI` has an unexpected type or if a string provided
    *      is neither not a valid GUID nor a valid URI.
    */
   hasVisits(guidOrURI) {
     // Quick fallback to the cpp version.
     if (guidOrURI instanceof Ci.nsIURI) {
       return new Promise(resolve => {
-        PlacesUtils.asyncHistory.isURIVisited(guidOrURI, (aURI, aIsVisited) => {
+        asyncHistory.isURIVisited(guidOrURI, (aURI, aIsVisited) => {
           resolve(aIsVisited);
         });
       });
     }
 
     guidOrURI = PlacesUtils.normalizeToURLOrGUID(guidOrURI);
     let isGuid = typeof guidOrURI == "string";
     let sqlFragment = isGuid ? "guid = :val"
@@ -1322,17 +1327,17 @@ function mergeUpdateInfoIntoPageInfo(upd
   return pageInfo;
 }
 
 // Inner implementation of History.insert.
 var insert = function(db, pageInfo) {
   let info = convertForUpdatePlaces(pageInfo);
 
   return new Promise((resolve, reject) => {
-    PlacesUtils.asyncHistory.updatePlaces(info, {
+    asyncHistory.updatePlaces(info, {
       handleError: error => {
         reject(error);
       },
       handleResult: result => {
         pageInfo = mergeUpdateInfoIntoPageInfo(result, pageInfo);
       },
       handleCompletion: () => {
         resolve(pageInfo);
@@ -1348,17 +1353,17 @@ var insertMany = function(db, pageInfos,
   let onErrorData = [];
 
   for (let pageInfo of pageInfos) {
     let info = convertForUpdatePlaces(pageInfo);
     infos.push(info);
   }
 
   return new Promise((resolve, reject) => {
-    PlacesUtils.asyncHistory.updatePlaces(infos, {
+    asyncHistory.updatePlaces(infos, {
       handleError: (resultCode, result) => {
         let pageInfo = mergeUpdateInfoIntoPageInfo(result);
         onErrorData.push(pageInfo);
       },
       handleResult: result => {
         let pageInfo = mergeUpdateInfoIntoPageInfo(result);
         onResultData.push(pageInfo);
       },
--- a/toolkit/components/places/PlacesUtils.jsm
+++ b/toolkit/components/places/PlacesUtils.jsm
@@ -1868,19 +1868,23 @@ XPCOMUtils.defineLazyGetter(PlacesUtils,
       if (typeof property == "function") {
         return property.bind(object);
       }
       return property;
     }
   }));
 });
 
-XPCOMUtils.defineLazyServiceGetter(PlacesUtils, "asyncHistory",
-                                   "@mozilla.org/browser/history;1",
-                                   "mozIAsyncHistory");
+if (AppConstants.MOZ_APP_NAME != "firefox") {
+  // TODO (bug 1458865): This is deprecated and should not be used. We'll
+  // remove it once comm-central stops using it.
+  XPCOMUtils.defineLazyServiceGetter(PlacesUtils, "asyncHistory",
+                                    "@mozilla.org/browser/history;1",
+                                    "mozIAsyncHistory");
+}
 
 XPCOMUtils.defineLazyServiceGetter(PlacesUtils, "favicons",
                                    "@mozilla.org/browser/favicon-service;1",
                                    "mozIAsyncFavicons");
 
 XPCOMUtils.defineLazyServiceGetter(this, "bmsvc",
                                    "@mozilla.org/browser/nav-bookmarks-service;1",
                                    "nsINavBookmarksService");
--- a/toolkit/components/places/nsNavHistoryResult.cpp
+++ b/toolkit/components/places/nsNavHistoryResult.cpp
@@ -4703,22 +4703,21 @@ nsNavHistoryResult::OnVisit(nsIURI* aURI
     return NS_OK;
   }
 
   uint32_t added = 0;
 
   ENUMERATE_HISTORY_OBSERVERS(OnVisit(aURI, aVisitId, aTime, aTransitionType,
                                       aHidden, &added));
 
-  // When we add visits through UpdatePlaces, we don't bother telling
-  // the world that the title 'changed' from nothing to the first title
-  // we ever see for a history entry. Our consumers here might still
-  // care, though, so we have to tell them - but only for the first
-  // visit we add. For subsequent changes, updateplaces will dispatch
-  // ontitlechanged notifications as normal.
+  // When we add visits, we don't bother telling the world that the title
+  // 'changed' from nothing to the first title we ever see for a history entry.
+  // Our consumers here might still care, though, so we have to tell them, but
+  // only for the first visit we add. Subsequent changes will get an usual
+  // ontitlechanged notification.
   if (!aLastKnownTitle.IsVoid() && aVisitCount == 1) {
     ENUMERATE_HISTORY_OBSERVERS(OnTitleChanged(aURI, aLastKnownTitle, aGUID));
   }
 
   if (!mRootNode->mExpanded)
     return NS_OK;
 
   // If this visit is accepted by an overlapped container, and not all
--- a/toolkit/components/places/tests/PlacesTestUtils.jsm
+++ b/toolkit/components/places/tests/PlacesTestUtils.jsm
@@ -58,17 +58,17 @@ var PlacesTestUtils = Object.freeze({
         place.referrer = Services.io.newURI(place.referrer);
       } else if (place.referrer && place.referrer instanceof URL) {
         place.referrer = Services.io.newURI(place.referrer.href);
       }
       let visitDate = place.visitDate;
       if (visitDate) {
         if (visitDate.constructor.name != "Date") {
           // visitDate should be in microseconds. It's easy to do the wrong thing
-          // and pass milliseconds to updatePlaces, so we lazily check for that.
+          // and pass milliseconds, so we lazily check for that.
           // While it's not easily distinguishable, since both are integers, we
           // can check if the value is very far in the past, and assume it's
           // probably a mistake.
           if (visitDate <= Date.now()) {
             throw new Error("AddVisits expects a Date object or _micro_seconds!");
           }
           visitDate = PlacesUtils.toDate(visitDate);
         }
--- a/toolkit/components/places/tests/browser/head.js
+++ b/toolkit/components/places/tests/browser/head.js
@@ -84,69 +84,12 @@ NavHistoryObserver.prototype = {
   onClearHistory() {},
   onPageChanged() {},
   onDeleteVisits() {},
   QueryInterface: ChromeUtils.generateQI([
     Ci.nsINavHistoryObserver,
   ])
 };
 
-/**
- * Asynchronously adds visits to a page, invoking a callback function when done.
- *
- * @param aPlaceInfo
- *        Either an nsIURI, in such a case a single LINK visit will be added.
- *        Or can be an object describing the visit to add, or an array
- *        of these objects:
- *          { uri: nsIURI of the page,
- *            transition: one of the TRANSITION_* from nsINavHistoryService,
- *            [optional] title: title of the page,
- *            [optional] visitDate: visit date in microseconds from the epoch
- *            [optional] referrer: nsIURI of the referrer for this visit
- *          }
- * @param [optional] aCallback
- *        Function to be invoked on completion.
- * @param [optional] aStack
- *        The stack frame used to report errors.
- */
-function addVisits(aPlaceInfo, aWindow, aCallback, aStack) {
-  let places = [];
-  if (aPlaceInfo instanceof Ci.nsIURI) {
-    places.push({ uri: aPlaceInfo });
-  } else if (Array.isArray(aPlaceInfo)) {
-    places = places.concat(aPlaceInfo);
-  } else {
-    places.push(aPlaceInfo);
-  }
-
-  // Create mozIVisitInfo for each entry.
-  let now = Date.now();
-  for (let place of places) {
-    if (!place.title) {
-      place.title = "test visit for " + place.uri.spec;
-    }
-    place.visits = [{
-      transitionType: place.transition === undefined ? TRANSITION_LINK
-                                                     : place.transition,
-      visitDate: place.visitDate || (now++) * 1000,
-      referrerURI: place.referrer
-    }];
-  }
-
-  aWindow.PlacesUtils.asyncHistory.updatePlaces(
-    places,
-    {
-      handleError: function AAV_handleError() {
-        throw ("Unexpected error in adding visit.");
-      },
-      handleResult() {},
-      handleCompletion: function UP_handleCompletion() {
-        if (aCallback)
-          aCallback();
-      }
-    }
-  );
-}
-
 function whenNewWindowLoaded(aOptions, aCallback) {
   BrowserTestUtils.waitForNewWindow().then(aCallback);
   OpenBrowserWindow(aOptions);
 }
--- a/toolkit/components/places/tests/history/test_async_history_api.js
+++ b/toolkit/components/places/tests/history/test_async_history_api.js
@@ -1,14 +1,18 @@
 /**
  * This file tests the async history API exposed by mozIAsyncHistory.
  */
 
 // Globals
 
+XPCOMUtils.defineLazyServiceGetter(this, "asyncHistory",
+                                   "@mozilla.org/browser/history;1",
+                                   "mozIAsyncHistory");
+
 const TEST_DOMAIN = "http://mozilla.org/";
 const URI_VISIT_SAVED = "uri-visit-saved";
 const RECENT_EVENT_THRESHOLD = 15 * 60 * 1000000;
 
 // Helpers
 /**
  * Object that represents a mozIVisitInfo object.
  *
@@ -22,17 +26,17 @@ function VisitInfo(aTransitionType,
                    aVisitTime) {
   this.transitionType =
     aTransitionType === undefined ? TRANSITION_LINK : aTransitionType;
   this.visitDate = aVisitTime || Date.now() * 1000;
 }
 
 function promiseUpdatePlaces(aPlaces, aOptions, aBatchFrecencyNotifications) {
   return new Promise((resolve, reject) => {
-    PlacesUtils.asyncHistory.updatePlaces(aPlaces, Object.assign({
+    asyncHistory.updatePlaces(aPlaces, Object.assign({
       _errors: [],
       _results: [],
       handleError(aResultCode, aPlace) {
         this._errors.push({ resultCode: aResultCode, info: aPlace});
       },
       handleResult(aPlace) {
         this._results.push(aPlace);
       },
@@ -179,17 +183,17 @@ add_task(async function test_invalid_uri
       Assert.equal(e.result, Cr.NS_ERROR_INVALID_ARG);
     }
   }
 });
 
 add_task(async function test_invalid_places_throws() {
   // First, test passing in nothing.
   try {
-    PlacesUtils.asyncHistory.updatePlaces();
+    asyncHistory.updatePlaces();
     do_throw("Should have thrown!");
   } catch (e) {
     Assert.equal(e.result, Cr.NS_ERROR_XPC_NOT_ENOUGH_ARGS);
   }
 
   // Now, test other bogus things.
   const TEST_VALUES = [
     null,
@@ -986,25 +990,25 @@ add_task(async function test_title_chang
         Assert.equal(visits.length, 1, "Should only get notified for one visit.");
         let {uri} = visits[0];
         Assert.equal(uri.spec, place.uri.spec, "Should get notified for visiting the new URI.");
         PlacesUtils.history.removeObserver(this);
         resolve();
       }
     });
   });
-  PlacesUtils.asyncHistory.updatePlaces(place);
+  asyncHistory.updatePlaces(place);
   await visitPromise;
 
   // The third case to test is to make sure we get a notification when
   // we change an existing place.
   expectedNotification = true;
   titleChangeObserver.expectedTitle = place.title = "title 2";
   place.visits = [new VisitInfo()];
-  PlacesUtils.asyncHistory.updatePlaces(place);
+  asyncHistory.updatePlaces(place);
 
   await titleChangePromise;
   await PlacesTestUtils.promiseAsyncUpdates();
 });
 
 add_task(async function test_visit_notifies() {
   // There are two observers we need to see for each visit.  One is an
   // nsINavHistoryObserver and the other is the uri-visit-saved observer topic.
@@ -1040,17 +1044,17 @@ add_task(async function test_visit_notif
         info("observe(" + aSubject + ", " + aTopic + ", " + aData + ")");
         Assert.ok(aSubject instanceof Ci.nsIURI);
         Assert.ok(aSubject.equals(place.uri));
 
         Services.obs.removeObserver(observer, URI_VISIT_SAVED);
         finisher();
       };
       Services.obs.addObserver(observer, URI_VISIT_SAVED);
-      PlacesUtils.asyncHistory.updatePlaces(place);
+      asyncHistory.updatePlaces(place);
     });
   }
 
   await promiseVisitObserver(place);
   await PlacesTestUtils.promiseAsyncUpdates();
 });
 
 // test with empty mozIVisitInfoCallback object
@@ -1076,17 +1080,17 @@ add_task(async function test_callbacks_n
       }
       // NetUtil.newURI() can throw if e.g. our app knows about imap://
       // but the account is not set up and so the URL is invalid for us.
       // Note this in the log but ignore as it's not the subject of this test.
       info("Could not construct URI for '" + url + "'; ignoring");
     }
   });
 
-  PlacesUtils.asyncHistory.updatePlaces(places, {});
+  asyncHistory.updatePlaces(places, {});
   await PlacesTestUtils.promiseAsyncUpdates();
 });
 
 // Test that we don't wrongly overwrite typed and hidden when adding new visits.
 add_task(async function test_typed_hidden_not_overwritten() {
   await PlacesUtils.history.clear();
   let places = [
     { uri: NetUtil.newURI("http://mozilla.org/"),
--- a/toolkit/components/places/tests/history/test_sameUri_titleChanged.js
+++ b/toolkit/components/places/tests/history/test_sameUri_titleChanged.js
@@ -1,9 +1,9 @@
-// Test that repeated additions of the same URI through updatePlaces, properly
+// Test that repeated additions of the same URI to history, properly
 // update from_visit and notify titleChanged.
 
 add_task(async function test() {
   let uri = "http://test.com/";
 
   let promiseTitleChangedNotifications = new Promise(resolve => {
     let historyObserver = {
       _count: 0,
--- a/toolkit/components/places/tests/history/test_updatePlaces_embed.js
+++ b/toolkit/components/places/tests/history/test_updatePlaces_embed.js
@@ -1,27 +1,32 @@
 /* Any copyright is dedicated to the Public Domain.
    http://creativecommons.org/publicdomain/zero/1.0/ */
 
 // Tests that updatePlaces properly handled callbacks for embed visits.
 
 "use strict";
 
+
+XPCOMUtils.defineLazyServiceGetter(this, "asyncHistory",
+                                   "@mozilla.org/browser/history;1",
+                                   "mozIAsyncHistory");
+
 add_task(async function test_embed_visit() {
   let place = {
     uri: NetUtil.newURI("http://places.test/"),
     visits: [
       { transitionType: PlacesUtils.history.TRANSITIONS.EMBED,
         visitDate: PlacesUtils.toPRTime(new Date()) }
     ],
   };
   let errors = 0;
   let results = 0;
   let updated = await new Promise(resolve => {
-    PlacesUtils.asyncHistory.updatePlaces(place, {
+    asyncHistory.updatePlaces(place, {
       ignoreErrors: true,
       ignoreResults: true,
       handleError(aResultCode, aPlace) {
         errors++;
       },
       handleResult(aPlace) {
         results++;
       },
@@ -43,17 +48,17 @@ add_task(async function test_misc_visits
         visitDate: PlacesUtils.toPRTime(new Date()) },
       { transitionType: PlacesUtils.history.TRANSITIONS.LINK,
         visitDate: PlacesUtils.toPRTime(new Date()) }
     ],
   };
   let errors = 0;
   let results = 0;
   let updated = await new Promise(resolve => {
-    PlacesUtils.asyncHistory.updatePlaces(place, {
+    asyncHistory.updatePlaces(place, {
       ignoreErrors: true,
       ignoreResults: true,
       handleError(aResultCode, aPlace) {
         errors++;
       },
       handleResult(aPlace) {
         results++;
       },