Bug 834541 - Remove the public History.hasHistoryEntries synchronous API. r=standard8 draft
authorMarco Bonardo <mbonardo@mozilla.com>
Tue, 08 May 2018 10:26:36 +0200
changeset 793508 c379c145386478eb0cadfefe20c881f8652e6c2d
parent 793505 ce963b7d9e89a3271b55d6e04a41477b66d14dc1
child 793510 4e57d4dc19fa464e86c25f8ff45c897c0d864b8e
push id109408
push usermak77@bonardo.net
push dateThu, 10 May 2018 08:16:57 +0000
reviewersstandard8
bugs834541
milestone62.0a1
Bug 834541 - Remove the public History.hasHistoryEntries synchronous API. r=standard8 MozReview-Commit-ID: KJW9YNwoSZb
browser/components/extensions/test/xpcshell/test_ext_history.js
toolkit/components/places/nsINavHistoryService.idl
toolkit/components/places/nsNavHistory.cpp
toolkit/components/places/nsNavHistory.h
toolkit/components/places/nsNavHistoryResult.cpp
toolkit/components/places/tests/unit/test_browserhistory.js
toolkit/components/places/tests/unit/test_history_clear.js
--- a/browser/components/extensions/test/xpcshell/test_ext_history.js
+++ b/browser/components/extensions/test/xpcshell/test_ext_history.js
@@ -124,17 +124,16 @@ add_task(async function test_delete() {
   equal((await PlacesTestUtils.visitsInDB(visits[0].url)), 0, "0 visits for uri found in history database");
   equal((await PlacesTestUtils.isPageInDB(visits[1].url)), false, "expected uri not found in history database");
   equal((await PlacesTestUtils.visitsInDB(visits[1].url)), 0, "0 visits for uri found in history database");
 
   ok((await PlacesTestUtils.isPageInDB(visits[3].url)), "expected uri found in history database");
 
   extension.sendMessage("delete-all");
   [historyClearedCount, removedUrls] = await extension.awaitMessage("history-cleared");
-  equal(PlacesUtils.history.hasHistoryEntries, false, "history is empty");
   equal(historyClearedCount, 2, "onVisitRemoved called for each clearing of history");
   equal(removedUrls.length, 3, "onVisitRemoved called the expected number of times");
   for (let i = 1; i < 3; ++i) {
     let url = visits[i].url;
     ok(removedUrls.includes(url), `${url} received by onVisitRemoved`);
   }
   await extension.unload();
 });
--- a/toolkit/components/places/nsINavHistoryService.idl
+++ b/toolkit/components/places/nsINavHistoryService.idl
@@ -1268,24 +1268,16 @@ interface nsINavHistoryService : nsISupp
   const unsigned short DATABASE_STATUS_LOCKED = 4;
 
   /**
    * Returns the current database status
    */
   readonly attribute unsigned short databaseStatus;
 
   /**
-   * True if there is any history. This can be used in UI to determine whether
-   * the "clear history" button should be enabled or not. This is much better
-   * than using BrowserHistory.count since that can be very slow if there is
-   * a lot of history (it must enumerate each item). This is pretty fast.
-   */
-  readonly attribute boolean hasHistoryEntries;
-
-  /**
    * This is just like markPageAsTyped (in nsIBrowserHistory, also implemented
    * by the history service), but for bookmarks. It declares that the given URI
    * is being opened as a result of following a bookmark. If this URI is loaded
    * soon after this message has been received, that transition will be marked
    * as following a bookmark.
    */
   void markPageAsFollowedBookmark(in nsIURI aURI);
 
--- a/toolkit/components/places/nsNavHistory.cpp
+++ b/toolkit/components/places/nsNavHistory.cpp
@@ -782,22 +782,20 @@ nsNavHistory::DomainNameFromURI(nsIURI *
   }
 
   // just return the original hostname
   // (it's also possible the host is an IP address)
   aURI->GetAsciiHost(aDomainName);
 }
 
 
-NS_IMETHODIMP
-nsNavHistory::GetHasHistoryEntries(bool* aHasEntries)
+bool
+nsNavHistory::hasHistoryEntries()
 {
-  NS_ENSURE_ARG_POINTER(aHasEntries);
-  *aHasEntries = GetDaysOfHistory() > 0;
-  return NS_OK;
+  return GetDaysOfHistory() > 0;
 }
 
 
 namespace {
 
 class InvalidateAllFrecenciesCallback : public AsyncStatementCallback
 {
 public:
--- a/toolkit/components/places/nsNavHistory.h
+++ b/toolkit/components/places/nsNavHistory.h
@@ -327,16 +327,22 @@ public:
   /**
    * Returns any recent activity done with a URL.
    * @return Any recent events associated with this URI.  Each bit is set
    *         according to RecentEventFlags enum values.
    */
   uint32_t GetRecentFlags(nsIURI *aURI);
 
   /**
+   * Whether there are visits.
+   * Note: This may cause synchronous I/O.
+   */
+  bool hasHistoryEntries();
+
+  /**
    * Registers a TRANSITION_EMBED visit for the session.
    *
    * @param aURI
    *        URI of the page.
    * @param aTime
    *        Visit time.  Only the last registered visit time is retained.
    */
   void registerEmbedVisit(nsIURI* aURI, int64_t aTime);
--- a/toolkit/components/places/nsNavHistoryResult.cpp
+++ b/toolkit/components/places/nsNavHistoryResult.cpp
@@ -2054,17 +2054,18 @@ nsNavHistoryQueryResultNode::GetHasChild
   }
 
   // For history containers query we must check if we have any history
   if (resultType == nsINavHistoryQueryOptions::RESULTS_AS_DATE_QUERY ||
       resultType == nsINavHistoryQueryOptions::RESULTS_AS_DATE_SITE_QUERY ||
       resultType == nsINavHistoryQueryOptions::RESULTS_AS_SITE_QUERY) {
     nsNavHistory* history = nsNavHistory::GetHistoryService();
     NS_ENSURE_TRUE(history, NS_ERROR_OUT_OF_MEMORY);
-    return history->GetHasHistoryEntries(aHasChildren);
+    *aHasChildren = history->hasHistoryEntries();
+    return NS_OK;
   }
 
   //XXX: For other containers queries we must:
   // 1. If it's open, just check mChildren for containers
   // 2. Else null the view (keep it in a var), open container, check mChildren
   //    for containers, close container, reset the view
 
   if (mContentsValid) {
--- a/toolkit/components/places/tests/unit/test_browserhistory.js
+++ b/toolkit/components/places/tests/unit/test_browserhistory.js
@@ -2,24 +2,30 @@
 /* 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 TEST_URI = NetUtil.newURI("http://mozilla.com/");
 const TEST_SUBDOMAIN_URI = NetUtil.newURI("http://foobar.mozilla.com/");
 
+async function checkEmptyHistory() {
+  let db = await PlacesUtils.promiseDBConnection();
+  let rows = await db.executeCached("SELECT count(*) FROM moz_historyvisits");
+  return !rows[0].getResultByIndex(0);
+}
+
 add_task(async function test_addPage() {
   await PlacesTestUtils.addVisits(TEST_URI);
-  Assert.equal(1, PlacesUtils.history.hasHistoryEntries);
+  Assert.ok(!await checkEmptyHistory(), "History has entries");
 });
 
 add_task(async function test_removePage() {
   await PlacesUtils.history.remove(TEST_URI);
-  Assert.equal(0, PlacesUtils.history.hasHistoryEntries);
+  Assert.ok(await checkEmptyHistory(), "History is empty");
 });
 
 add_task(async function test_removePages() {
   let pages = [];
   for (let i = 0; i < 8; i++) {
     pages.push(NetUtil.newURI(TEST_URI.spec + i));
   }
 
@@ -37,17 +43,17 @@ add_task(async function test_removePages
     url: pages[BOOKMARK_INDEX],
     title: "test bookmark"
   });
   PlacesUtils.annotations.setPageAnnotation(pages[BOOKMARK_INDEX],
                                             ANNO_NAME, ANNO_VALUE, 0,
                                             Ci.nsIAnnotationService.EXPIRE_NEVER);
 
   await PlacesUtils.history.remove(pages);
-  Assert.equal(0, PlacesUtils.history.hasHistoryEntries);
+  Assert.ok(await checkEmptyHistory(), "History is empty");
 
   // Check that the bookmark and its annotation still exist.
   let folder = await PlacesUtils.getFolderContents(PlacesUtils.unfiledBookmarksFolderId);
   Assert.equal(folder.root.childCount, 1);
   Assert.equal(PlacesUtils.annotations.getPageAnnotation(pages[BOOKMARK_INDEX], ANNO_NAME),
                ANNO_VALUE);
 
   // Check the annotation on the non-bookmarked page does not exist anymore.
@@ -85,49 +91,27 @@ add_task(async function test_removePages
                  i > 0 && i < 9);
   }
 
   // Clear remaining items and check that all pages have been removed.
   await PlacesUtils.history.removeByFilter({
     beginDate: PlacesUtils.toDate(startDate),
     endDate: PlacesUtils.toDate(startDate + 9000)
   });
-  Assert.equal(0, PlacesUtils.history.hasHistoryEntries);
+  Assert.ok(await checkEmptyHistory(), "History is empty");
 });
 
 add_task(async function test_removePagesFromHost() {
   await PlacesTestUtils.addVisits(TEST_URI);
   await PlacesUtils.history.removeByFilter({ host: ".mozilla.com" });
-  Assert.equal(0, PlacesUtils.history.hasHistoryEntries);
+  Assert.ok(await checkEmptyHistory(), "History is empty");
 });
 
 add_task(async function test_removePagesFromHost_keepSubdomains() {
   await PlacesTestUtils.addVisits([{ uri: TEST_URI }, { uri: TEST_SUBDOMAIN_URI }]);
   await PlacesUtils.history.removeByFilter({ host: "mozilla.com" });
-  Assert.equal(1, PlacesUtils.history.hasHistoryEntries);
+  Assert.ok(!await checkEmptyHistory(), "History has entries");
 });
 
 add_task(async function test_history_clear() {
   await PlacesUtils.history.clear();
-  Assert.equal(0, PlacesUtils.history.hasHistoryEntries);
+  Assert.ok(await checkEmptyHistory(), "History is empty");
 });
-
-add_task(async function test_getObservers() {
-  // Ensure that getObservers() invalidates the hasHistoryEntries cache.
-  await PlacesTestUtils.addVisits(TEST_URI);
-  Assert.equal(1, PlacesUtils.history.hasHistoryEntries);
-  // This is just for testing purposes, never do it.
-  return new Promise((resolve, reject) => {
-    DBConn().executeSimpleSQLAsync("DELETE FROM moz_historyvisits", {
-      handleError(error) {
-        reject(error);
-      },
-      handleResult(result) {
-      },
-      handleCompletion(result) {
-        // Just invoking getObservers should be enough to invalidate the cache.
-        PlacesUtils.history.getObservers();
-        Assert.equal(0, PlacesUtils.history.hasHistoryEntries);
-        resolve();
-      }
-    });
-  });
-});
--- a/toolkit/components/places/tests/unit/test_history_clear.js
+++ b/toolkit/components/places/tests/unit/test_history_clear.js
@@ -50,22 +50,18 @@ add_task(async function test_history_cle
     { uri: uri("http://frecency.mozilla.org/"),
       transition: TRANSITION_LINK },
   ]);
   await PlacesTestUtils.promiseAsyncUpdates();
 
   // Clear history and wait for the onClearHistory notification.
   let promiseClearHistory =
     PlacesTestUtils.waitForNotification("onClearHistory", () => true, "history");
-  PlacesUtils.history.clear();
+  await PlacesUtils.history.clear();
   await promiseClearHistory;
-
-  // check browserHistory returns no entries
-  Assert.equal(0, PlacesUtils.history.hasHistoryEntries);
-
   await PlacesTestUtils.promiseAsyncUpdates();
 
   // Check that frecency for not cleared items (bookmarks) has been converted
   // to -1.
   let stmt = mDBConn.createStatement(
     "SELECT h.id FROM moz_places h WHERE h.frecency > 0 ");
   Assert.ok(!stmt.executeStep());
   stmt.finalize();