Bug 1350377 - Remove `getPlacesInfo` and change associated files and tests, r=mak

Files which make use of `getPlacesInfo` have been replaced with `History.fetch`.
The code for `GetPlacesInfo` has been deleted from the cpp and idl files.
The test for `getPlacesInfo` has been suitably rewritten and moved alongside the
other History.jsm tests.

There were 2 places where the fact that `getPlacesInfo` takes an array as opposed
to a single uri mattered, in `test_getPlacesInfo.js` and `test_refresh_firefox.py`.

MozReview-Commit-ID: KQSMHCvvlrQ

--HG--
extra : rebase_source : 78a4381587e040bec8ba5e51d1c5b5a67e70897e

browser/base/content/browser-places.js

@@ -482,7 +482,7 @@ var PlacesCommandHook = {
 
       try {
         info.title = docInfo.isErrorPage ?
-          (await PlacesUtils.promisePlaceInfo(aBrowser.currentURI)).title :
+          (await PlacesUtils.history.fetch(aBrowser.currentURI)).title :
           aBrowser.contentTitle;
         info.title = info.title || url.href;
         description = docInfo.description;

browser/components/migration/tests/marionette/test_refresh_firefox.py

@@ -170,35 +170,22 @@ class TestFirefoxRefresh(MarionetteTestCase):
         self.assertEqual(titleInBookmarks, self._bookmarkText)
 
     def checkHistory(self):
-        historyResults = self.runAsyncCode("""
-          let placeInfos = [];
-          PlacesUtils.asyncHistory.getPlacesInfo(makeURI(arguments[0]), {
-            handleError(resultCode, place) {
-              placeInfos = null;
-              marionetteScriptFinished("Unexpected error in fetching visit: " + resultCode);
-            },
-            handleResult(placeInfo) {
-              placeInfos.push(placeInfo);
-            },
-            handleCompletion() {
-              if (placeInfos) {
-                if (!placeInfos.length) {
-                  marionetteScriptFinished("No visits found");
-                } else {
-                  marionetteScriptFinished(placeInfos);
-                }
-              }
-            },
+        historyResult = self.runAsyncCode("""
+          PlacesUtils.history.fetch(arguments[0]).then(pageInfo => {
+            if (!pageInfo) {
+              marionetteScriptFinished("No visits found");
+            } else {
+              marionetteScriptFinished(pageInfo);
+            }
+          }).catch(e => {
+            marionetteScriptFinished("Unexpected error in fetching page: " + e);
           });
         """, script_args=[self._historyURL])
-        if type(historyResults) == str:
-            self.fail(historyResults)
+        if type(historyResult) == str:
+            self.fail(historyResult)
             return
 
-        historyCount = len(historyResults)
-        self.assertEqual(historyCount, 1, "Should have exactly 1 entry for URI, got %d" % historyCount)
-        if historyCount == 1:
-            self.assertEqual(historyResults[0]['title'], self._historyTitle)
+        self.assertEqual(historyResult['title'], self._historyTitle)
 
     def checkFormHistory(self):
         formFieldResults = self.runAsyncCode("""

browser/components/places/content/bookmarkProperties.js

@@ -660,7 +660,7 @@ var BookmarkPropertiesPanel = {
     } else if (this._itemType == BOOKMARK_FOLDER) {
       itemGuid = await PlacesTransactions.NewFolder(info).transact();
       for (let uri of this._URIs) {
-        let placeInfo = await PlacesUtils.promisePlaceInfo(uri);
+        let placeInfo = await PlacesUtils.history.fetch(uri);
         let title = placeInfo ? placeInfo.title : "";
         await PlacesTransactions.transact({ parentGuid: itemGuid, uri, title });
       }

toolkit/components/places/History.cpp

@@ -1305,63 +1305,6 @@ private:
   RefPtr<History> mHistory;
 };
 
-class GetPlaceInfo final : public Runnable {
-public:
-  /**
-   * Get the place info for a given place (by GUID or URI)  asynchronously.
-   */
-  static nsresult Start(mozIStorageConnection* aConnection,
-                        VisitData& aPlace,
-                        mozIVisitInfoCallback* aCallback) {
-    MOZ_ASSERT(NS_IsMainThread(), "This should be called on the main thread");
-
-    nsMainThreadPtrHandle<mozIVisitInfoCallback>
-      callback(new nsMainThreadPtrHolder<mozIVisitInfoCallback>(aCallback));
-    RefPtr<GetPlaceInfo> event = new GetPlaceInfo(aPlace, callback);
-
-    // Get the target thread, and then start the work!
-    nsCOMPtr<nsIEventTarget> target = do_GetInterface(aConnection);
-    NS_ENSURE_TRUE(target, NS_ERROR_UNEXPECTED);
-    nsresult rv = target->Dispatch(event, NS_DISPATCH_NORMAL);
-    NS_ENSURE_SUCCESS(rv, rv);
-
-    return NS_OK;
-  }
-
-  NS_IMETHOD Run() override
-  {
-    MOZ_ASSERT(!NS_IsMainThread(), "This should not be called on the main thread");
-
-    bool exists;
-    nsresult rv = mHistory->FetchPageInfo(mPlace, &exists);
-    NS_ENSURE_SUCCESS(rv, rv);
-
-    if (!exists)
-      rv = NS_ERROR_NOT_AVAILABLE;
-
-    nsCOMPtr<nsIRunnable> event =
-      new NotifyPlaceInfoCallback(mCallback, mPlace, false, rv);
-
-    rv = NS_DispatchToMainThread(event);
-    NS_ENSURE_SUCCESS(rv, rv);
-
-    return NS_OK;
-  }
-private:
-  GetPlaceInfo(VisitData& aPlace,
-               const nsMainThreadPtrHandle<mozIVisitInfoCallback>& aCallback)
-  : mPlace(aPlace)
-  , mCallback(aCallback)
-  , mHistory(History::GetService())
-  {
-    MOZ_ASSERT(NS_IsMainThread(), "This should be called on the main thread");
-  }
-
-  VisitData mPlace;
-  nsMainThreadPtrHandle<mozIVisitInfoCallback> mCallback;
-  RefPtr<History> mHistory;
-};
-
 /**
  * Sets the page title for a page in moz_places (if necessary).
  */
@@ -2808,75 +2751,6 @@ History::RemoveAllDownloads()
 ////////////////////////////////////////////////////////////////////////////////
 //// mozIAsyncHistory
 
-NS_IMETHODIMP
-History::GetPlacesInfo(JS::Handle<JS::Value> aPlaceIdentifiers,
-                       mozIVisitInfoCallback* aCallback,
-                       JSContext* aCtx)
-{
-  // Make sure nsNavHistory service is up before proceeding:
-  nsNavHistory* navHistory = nsNavHistory::GetHistoryService();
-  MOZ_ASSERT(navHistory, "Could not get nsNavHistory?!");
-  if (!navHistory) {
-    return NS_ERROR_FAILURE;
-  }
-
-  uint32_t placesIndentifiersLength;
-  JS::Rooted<JSObject*> placesIndentifiers(aCtx);
-  nsresult rv = GetJSArrayFromJSValue(aPlaceIdentifiers, aCtx,
-                                      &placesIndentifiers,
-                                      &placesIndentifiersLength);
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  nsTArray<VisitData> placesInfo;
-  placesInfo.SetCapacity(placesIndentifiersLength);
-  for (uint32_t i = 0; i < placesIndentifiersLength; i++) {
-    JS::Rooted<JS::Value> placeIdentifier(aCtx);
-    bool rc = JS_GetElement(aCtx, placesIndentifiers, i, &placeIdentifier);
-    NS_ENSURE_TRUE(rc, NS_ERROR_UNEXPECTED);
-
-    // GUID
-    nsAutoString fatGUID;
-    GetJSValueAsString(aCtx, placeIdentifier, fatGUID);
-    if (!fatGUID.IsVoid()) {
-      NS_ConvertUTF16toUTF8 guid(fatGUID);
-      if (!IsValidGUID(guid))
-        return NS_ERROR_INVALID_ARG;
-
-      VisitData& placeInfo = *placesInfo.AppendElement(VisitData());
-      placeInfo.guid = guid;
-    }
-    else {
-      nsCOMPtr<nsIURI> uri = GetJSValueAsURI(aCtx, placeIdentifier);
-      if (!uri)
-        return NS_ERROR_INVALID_ARG; // neither a guid, nor a uri.
-      placesInfo.AppendElement(VisitData(uri));
-    }
-  }
-
-  mozIStorageConnection* dbConn = GetDBConn();
-  NS_ENSURE_STATE(dbConn);
-
-  for (nsTArray<VisitData>::size_type i = 0; i < placesInfo.Length(); i++) {
-    nsresult rv = GetPlaceInfo::Start(dbConn, placesInfo.ElementAt(i), aCallback);
-    NS_ENSURE_SUCCESS(rv, rv);
-  }
-
-  // Be sure to notify that all of our operations are complete.  This
-  // is dispatched to the background thread first and redirected to the
-  // main thread from there to make sure that all database notifications
-  // and all embed or canAddURI notifications have finished.
-  if (aCallback) {
-    nsMainThreadPtrHandle<mozIVisitInfoCallback>
-      callback(new nsMainThreadPtrHolder<mozIVisitInfoCallback>(aCallback));
-    nsCOMPtr<nsIEventTarget> backgroundThread = do_GetInterface(dbConn);
-    NS_ENSURE_TRUE(backgroundThread, NS_ERROR_UNEXPECTED);
-    nsCOMPtr<nsIRunnable> event = new NotifyCompletion(callback);
-    return backgroundThread->Dispatch(event, NS_DISPATCH_NORMAL);
-  }
-
-  return NS_OK;
-}
-
 NS_IMETHODIMP
 History::UpdatePlaces(JS::Handle<JS::Value> aPlaceInfos,
                       mozIVisitInfoCallback* aCallback,

toolkit/components/places/mozIAsyncHistory.idl

@@ -126,37 +126,20 @@ interface mozIVisitedStatusCallback : nsISupports
                  in boolean aVisitedStatus);
 };
 
+/**
+ * This interface contains APIs for cpp consumers.
+ * Javascript consumers should look at History.jsm instead,
+ * that is exposed through PlacesUtils.history.
+ *
+ * If you're evaluating adding a new history API, it should
+ * usually go to History.jsm, unless it needs to do long and
+ * expensive work in a batch, then it could be worth doing
+ * that in History.cpp.
+ */
+
 [scriptable, uuid(1643EFD2-A329-4733-A39D-17069C8D3B2D)]
 interface mozIAsyncHistory : nsISupports
 {
-  /**
-   * Gets the available information for the given array of places, each
-   * identified by either nsIURI or places GUID (string).
-   *
-   * The retrieved places info objects DO NOT include the visits data (the
-   * |visits| attribute is set to null).
-   *
-   * If a given place does not exist in the database, aCallback.handleError is
-   * called for it with NS_ERROR_NOT_AVAILABLE result code.
-   *
-   * @param aPlaceIdentifiers
-   *        The place[s] for which to retrieve information, identified by either
-   *        a single place GUID, a single URI, or a JS array of URIs and/or GUIDs.
-   * @param aCallback
-   *        A mozIVisitInfoCallback object which consists of callbacks to be
-   *        notified for successful or failed retrievals.
-   *        If there's no information available for a given place, aCallback
-   *        is called with a stub place info object, containing just the provided
-   *        data (GUID or URI).
-   *
-   * @throws NS_ERROR_INVALID_ARG
-   *         - Passing in NULL for aPlaceIdentifiers or aCallback.
-   *         - Not providing at least one valid GUID or URI. 
-   */
-  [implicit_jscontext]
-  void getPlacesInfo(in jsval aPlaceIdentifiers,
-                     in mozIVisitInfoCallback aCallback);
-
   /**
    * Adds a set of visits for one or more mozIPlaceInfo objects, and updates
    * each mozIPlaceInfo's title or guid.

toolkit/components/places/nsINavHistoryService.idl

@@ -1316,7 +1316,7 @@ interface nsINavHistoryService : nsISupports
 
   /**
    * Gets the original title of the page.
-   * @deprecated use mozIAsyncHistory.getPlacesInfo instead.
+   * @deprecated use PlacesUtils.history.fetch instead.
    */
   AString getPageTitle(in nsIURI aURI);
 

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

@@ -0,0 +1,110 @@
+/* Any copyright is dedicated to the Public Domain. http://creativecommons.org/publicdomain/zero/1.0/ */
+"use strict";
+
+add_task(async function test_fetch_existent() {
+  await PlacesTestUtils.clearHistory();
+  await PlacesUtils.bookmarks.eraseEverything();
+
+  // Populate places and historyvisits.
+  let uriString = `http://mozilla.com/test_browserhistory/test_fetch`;
+  let uri = NetUtil.newURI(uriString);
+  let title = `Test Visit ${Math.random()}`;
+  let dates = [];
+  let visits = [];
+  let transitions = [ PlacesUtils.history.TRANSITION_LINK,
+                      PlacesUtils.history.TRANSITION_TYPED,
+                      PlacesUtils.history.TRANSITION_BOOKMARK,
+                      PlacesUtils.history.TRANSITION_REDIRECT_TEMPORARY,
+                      PlacesUtils.history.TRANSITION_REDIRECT_PERMANENT,
+                      PlacesUtils.history.TRANSITION_DOWNLOAD,
+                      PlacesUtils.history.TRANSITION_FRAMED_LINK,
+                      PlacesUtils.history.TRANSITION_RELOAD ];
+  let guid = "";
+  for (let i = 0; i != transitions.length; i++) {
+    dates.push(new Date(Date.now() - (i * 10000000)));
+    visits.push({
+      uri,
+      title,
+      transition: transitions[i],
+      visitDate: dates[i]
+    });
+  }
+  await PlacesTestUtils.addVisits(visits);
+  Assert.ok((await PlacesTestUtils.isPageInDB(uri)));
+  Assert.equal(await PlacesTestUtils.visitsInDB(uri), visits.length);
+
+  // Store guid for further use in testing.
+  guid = await PlacesTestUtils.fieldInDB(uri, "guid");
+  Assert.ok(guid, guid);
+
+  // Initialize the objects to compare against.
+  let idealPageInfo = {
+    url: new URL(uriString), guid, title
+  };
+  let idealVisits = visits.map(v => {
+    return {
+      date: v.visitDate,
+      transition: v.transition
+    };
+  });
+
+  // We should check these 4 cases:
+  // 1, 2: visits not included, by URL and guid (same result expected).
+  // 3, 4: visits included, by URL and guid (same result expected).
+  for (let includeVisits of [true, false]) {
+    for (let guidOrURL of [uri, guid]) {
+      let pageInfo = await PlacesUtils.history.fetch(guidOrURL, {includeVisits});
+      if (includeVisits) {
+        idealPageInfo.visits = idealVisits;
+      } else {
+        // We need to explicitly delete this property since deepEqual looks at
+        // the list of properties as well (`visits in pageInfo` is true here).
+        delete idealPageInfo.visits;
+      }
+
+      // Since idealPageInfo doesn't contain a frecency, check it and delete.
+      Assert.ok(typeof pageInfo.frecency === "number");
+      delete pageInfo.frecency;
+
+      // Visits should be from newer to older.
+      if (includeVisits) {
+        for (let i = 0; i !== pageInfo.visits.length - 1; i++) {
+          Assert.lessOrEqual(pageInfo.visits[i + 1].date.getTime(), pageInfo.visits[i].date.getTime());
+        }
+      }
+      Assert.deepEqual(idealPageInfo, pageInfo);
+    }
+  }
+});
+
+add_task(async function test_fetch_nonexistent() {
+  await PlacesTestUtils.clearHistory();
+  await PlacesUtils.bookmarks.eraseEverything();
+
+  let uri = NetUtil.newURI("http://doesntexist.in.db");
+  let pageInfo = await PlacesUtils.history.fetch(uri);
+  Assert.equal(pageInfo, null);
+});
+
+add_task(async function test_error_cases() {
+  Assert.throws(
+    () => PlacesUtils.history.fetch("3"),
+      /TypeError: 3 is not a valid /
+  );
+  Assert.throws(
+    () => PlacesUtils.history.fetch({not: "a valid string or guid"}),
+    /TypeError: Invalid url or guid/
+  );
+  Assert.throws(
+    () => PlacesUtils.history.fetch("http://valid.uri.com", "not an object"),
+      /TypeError: options should be/
+  );
+  Assert.throws(
+    () => PlacesUtils.history.fetch("http://valid.uri.com", null),
+      /TypeError: options should be/
+  );
+  Assert.throws(
+    () => PlacesUtils.history.fetch("http://valid.uri.come", { includeVisits: "not a boolean"}),
+      /TypeError: includeVisits should be a/
+  );
+});

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

@@ -2,6 +2,7 @@
 head = head_history.js
 
 [test_async_history_api.js]
+[test_fetch.js]
 [test_insert.js]
 [test_insertMany.js]
 [test_remove.js]

toolkit/components/places/tests/unit/test_getPlacesInfo.js

@@ -1,110 +0,0 @@
-/* Any copyright is dedicated to the Public Domain.
-   http://creativecommons.org/publicdomain/zero/1.0/ */
-
-function promiseGetPlacesInfo(aPlacesIdentifiers) {
-  return new Promise(resolve => {
-    PlacesUtils.asyncHistory.getPlacesInfo(aPlacesIdentifiers, {
-      _results: [],
-      _errors: [],
-
-      handleResult: function handleResult(aPlaceInfo) {
-        this._results.push(aPlaceInfo);
-      },
-      handleError: function handleError(aResultCode, aPlaceInfo) {
-        this._errors.push({ resultCode: aResultCode, info: aPlaceInfo });
-      },
-      handleCompletion: function handleCompletion() {
-        resolve({ errors: this._errors, results: this._results });
-      }
-    });
-
-  });
-}
-
-function ensurePlacesInfoObjectsAreEqual(a, b) {
-  do_check_true(a.uri.equals(b.uri));
-  do_check_eq(a.title, b.title);
-  do_check_eq(a.guid, b.guid);
-  do_check_eq(a.placeId, b.placeId);
-}
-
-async function test_getPlacesInfoExistentPlace() {
-  let testURI = NetUtil.newURI("http://www.example.tld");
-  await PlacesTestUtils.addVisits(testURI);
-
-  let getPlacesInfoResult = await promiseGetPlacesInfo([testURI]);
-  do_check_eq(getPlacesInfoResult.results.length, 1);
-  do_check_eq(getPlacesInfoResult.errors.length, 0);
-
-  let placeInfo = getPlacesInfoResult.results[0];
-  do_check_true(placeInfo instanceof Ci.mozIPlaceInfo);
-
-  do_check_true(placeInfo.uri.equals(testURI));
-  do_check_eq(placeInfo.title, "test visit for " + testURI.spec);
-  do_check_true(placeInfo.guid.length > 0);
-  do_check_eq(placeInfo.visits, null);
-}
-add_task(test_getPlacesInfoExistentPlace);
-
-async function test_getPlacesInfoNonExistentPlace() {
-  let testURI = NetUtil.newURI("http://www.example_non_existent.tld");
-  let getPlacesInfoResult = await promiseGetPlacesInfo(testURI);
-  do_check_eq(getPlacesInfoResult.results.length, 0);
-  do_check_eq(getPlacesInfoResult.errors.length, 1);
-}
-add_task(test_getPlacesInfoNonExistentPlace);
-
-async function test_promisedHelper() {
-  let uri = NetUtil.newURI("http://www.helper_existent_example.tld");
-  await PlacesTestUtils.addVisits(uri);
-  let placeInfo = await PlacesUtils.promisePlaceInfo(uri);
-  do_check_true(placeInfo instanceof Ci.mozIPlaceInfo);
-
-  uri = NetUtil.newURI("http://www.helper_non_existent_example.tld");
-  try {
-    await PlacesUtils.promisePlaceInfo(uri);
-    do_throw("PlacesUtils.promisePlaceInfo should have rejected the promise");
-  } catch (ex) { }
-}
-add_task(test_promisedHelper);
-
-async function test_infoByGUID() {
-  let testURI = NetUtil.newURI("http://www.guid_example.tld");
-  await PlacesTestUtils.addVisits(testURI);
-
-  let placeInfoByURI = await PlacesUtils.promisePlaceInfo(testURI);
-  let placeInfoByGUID = await PlacesUtils.promisePlaceInfo(placeInfoByURI.guid);
-  ensurePlacesInfoObjectsAreEqual(placeInfoByURI, placeInfoByGUID);
-}
-add_task(test_infoByGUID);
-
-async function test_invalid_guid() {
-  try {
-    await PlacesUtils.promisePlaceInfo("###");
-    do_throw("getPlacesInfo should fail for invalid guids")
-  } catch (ex) { }
-}
-add_task(test_invalid_guid);
-
-async function test_mixed_selection() {
-  let placeInfo1, placeInfo2;
-  let uri = NetUtil.newURI("http://www.mixed_selection_test_1.tld");
-  await PlacesTestUtils.addVisits(uri);
-  placeInfo1 = await PlacesUtils.promisePlaceInfo(uri);
-
-  uri = NetUtil.newURI("http://www.mixed_selection_test_2.tld");
-  await PlacesTestUtils.addVisits(uri);
-  placeInfo2 = await PlacesUtils.promisePlaceInfo(uri);
-
-  let getPlacesInfoResult = await promiseGetPlacesInfo([placeInfo1.uri, placeInfo2.guid]);
-  do_check_eq(getPlacesInfoResult.results.length, 2);
-  do_check_eq(getPlacesInfoResult.errors.length, 0);
-
-  do_check_eq(getPlacesInfoResult.results[0].uri.spec, placeInfo1.uri.spec);
-  do_check_eq(getPlacesInfoResult.results[1].guid, placeInfo2.guid);
-}
-add_task(test_mixed_selection);
-
-function run_test() {
-  run_next_test();
-}

toolkit/components/places/tests/unit/test_history.js

@@ -148,7 +148,7 @@ add_task(async function test_execute() {
 
   // test getPageTitle
   await PlacesTestUtils.addVisits({ uri: uri("http://example.com"), title: "title" });
-  let placeInfo = await PlacesUtils.promisePlaceInfo(uri("http://example.com"));
+  let placeInfo = await PlacesUtils.history.fetch("http://example.com");
   do_check_eq(placeInfo.title, "title");
 
   // query for the visit

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

@@ -81,7 +81,6 @@ skip-if = (os == "win" && os_version == "5.1") # Bug 1158887
 [test_frecency_decay.js]
 [test_frecency_zero_updated.js]
 [test_getChildIndex.js]
-[test_getPlacesInfo.js]
 [test_history.js]
 [test_history_autocomplete_tags.js]
 [test_history_catobs.js]