Bug 1678618: Remove onDeleteURI and onDeleteVisits from nsINavHistoryObserver. r=mak

Depends on D101115

Differential Revision: https://phabricator.services.mozilla.com/D101118

browser/components/extensions/parent/ext-history.js

@@ -94,27 +94,6 @@ const convertNavHistoryContainerResultNode = (container, converter) => {
   return results;
 };
 
-var _observer;
-
-const getHistoryObserver = () => {
-  if (!_observer) {
-    _observer = new (class extends EventEmitter {
-      onDeleteURI(uri, guid, reason) {
-        this.emit("visitRemoved", { allHistory: false, urls: [uri.spec] });
-      }
-      onBeginUpdateBatch() {}
-      onEndUpdateBatch() {}
-      onDeleteVisits(uri, partialRemoval, guid, reason) {
-        if (!partialRemoval) {
-          this.emit("visitRemoved", { allHistory: false, urls: [uri.spec] });
-        }
-      }
-    })();
-    PlacesUtils.history.addObserver(_observer);
-  }
-  return _observer;
-};
-
 this.history = class extends ExtensionAPI {
   getAPI(context) {
     return {
@@ -257,10 +236,7 @@ this.history = class extends ExtensionAPI {
           context,
           name: "history.onVisitRemoved",
           register: fire => {
-            let listener = (event, data) => {
+            const listener = events => {
-              fire.sync(data);
-            };
-            const historyClearedListener = events => {
               const removedURLs = [];
 
               for (const event of events) {
@@ -283,16 +259,14 @@ this.history = class extends ExtensionAPI {
               }
             };
 
-            getHistoryObserver().on("visitRemoved", listener);
             PlacesUtils.observers.addListener(
               ["history-cleared", "page-removed"],
-              historyClearedListener
+              listener
             );
             return () => {
-              getHistoryObserver().off("visitRemoved", listener);
               PlacesUtils.observers.removeListener(
                 ["history-cleared", "page-removed"],
-                historyClearedListener
+                listener
               );
             };
           },

browser/components/newtab/lib/PlacesFeed.jsm

@@ -46,36 +46,6 @@ class Observer {
   }
 }
 
-/**
- * HistoryObserver - observes events from PlacesUtils.history
- */
-class HistoryObserver extends Observer {
-  constructor(dispatch) {
-    super(dispatch, Ci.nsINavHistoryObserver);
-  }
-
-  /**
-   * onDeleteURI - Called when an link is deleted from history.
-   *
-   * @param  {obj} uri        A URI object representing the link's url
-   *         {str} uri.spec   The URI as a string
-   */
-  onDeleteURI(uri) {
-    this.dispatch({ type: at.PLACES_LINKS_CHANGED });
-    this.dispatch({
-      type: at.PLACES_LINK_DELETED,
-      data: { url: uri.spec },
-    });
-  }
-
-  // Empty functions to make xpconnect happy
-  onBeginUpdateBatch() {}
-
-  onEndUpdateBatch() {}
-
-  onDeleteVisits() {}
-}
-
 /**
  * BookmarksObserver - observes events from PlacesUtils.bookmarks
  */
@@ -188,16 +158,12 @@ class PlacesFeed {
   constructor() {
     this.placesChangedTimer = null;
     this.customDispatch = this.customDispatch.bind(this);
-    this.historyObserver = new HistoryObserver(this.customDispatch);
     this.bookmarksObserver = new BookmarksObserver(this.customDispatch);
     this.placesObserver = new PlacesObserver(this.customDispatch);
   }
 
   addObservers() {
     // NB: Directly get services without importing the *BIG* PlacesUtils module
-    Cc["@mozilla.org/browser/nav-history-service;1"]
-      .getService(Ci.nsINavHistoryService)
-      .addObserver(this.historyObserver, true);
     Cc["@mozilla.org/browser/nav-bookmarks-service;1"]
       .getService(Ci.nsINavBookmarksService)
       .addObserver(this.bookmarksObserver, true);
@@ -243,7 +209,6 @@ class PlacesFeed {
       this.placesChangedTimer.cancel();
       this.placesChangedTimer = null;
     }
-    PlacesUtils.history.removeObserver(this.historyObserver);
     PlacesUtils.bookmarks.removeObserver(this.bookmarksObserver);
     PlacesUtils.observers.removeListener(
       ["bookmark-added", "bookmark-removed", "history-cleared", "page-removed"],
@@ -577,7 +542,6 @@ class PlacesFeed {
 this.PlacesFeed = PlacesFeed;
 
 // Exported for testing only
-PlacesFeed.HistoryObserver = HistoryObserver;
 PlacesFeed.BookmarksObserver = BookmarksObserver;
 PlacesFeed.PlacesObserver = PlacesObserver;
 

browser/components/newtab/test/unit/lib/PlacesFeed.test.js

@@ -1,7 +1,7 @@
 import { actionCreators as ac, actionTypes as at } from "common/Actions.jsm";
 import { GlobalOverrider } from "test/unit/utils";
 import { PlacesFeed } from "lib/PlacesFeed.jsm";
-const { HistoryObserver, BookmarksObserver, PlacesObserver } = PlacesFeed;
+const { BookmarksObserver, PlacesObserver } = PlacesFeed;
 
 const FAKE_BOOKMARK = {
   bookmarkGuid: "xi31",
@@ -82,16 +82,6 @@ describe("PlacesFeed", () => {
   });
   afterEach(() => globals.restore());
 
-  it("should have a HistoryObserver that dispatches to the store", () => {
-    assert.instanceOf(feed.historyObserver, HistoryObserver);
-    const action = { type: "FOO" };
-
-    feed.historyObserver.dispatch(action);
-
-    assert.calledOnce(feed.store.dispatch);
-    assert.equal(feed.store.dispatch.firstCall.args[0].type, action.type);
-  });
-
   it("should have a BookmarksObserver that dispatch to the store", () => {
     assert.instanceOf(feed.bookmarksObserver, BookmarksObserver);
     const action = { type: "FOO" };
@@ -115,11 +105,6 @@ describe("PlacesFeed", () => {
     it("should add bookmark, history, places, blocked observers on INIT", () => {
       feed.onAction({ type: at.INIT });
 
-      assert.calledWith(
-        global.PlacesUtils.history.addObserver,
-        feed.historyObserver,
-        true
-      );
       assert.calledWith(
         global.PlacesUtils.bookmarks.addObserver,
         feed.bookmarksObserver,
@@ -144,10 +129,6 @@ describe("PlacesFeed", () => {
       let spy = feed.placesChangedTimer.cancel;
       feed.onAction({ type: at.UNINIT });
 
-      assert.calledWith(
-        global.PlacesUtils.history.removeObserver,
-        feed.historyObserver
-      );
       assert.calledWith(
         global.PlacesUtils.bookmarks.removeObserver,
         feed.bookmarksObserver
@@ -752,25 +733,6 @@ describe("PlacesFeed", () => {
     });
   });
 
-  describe("HistoryObserver", () => {
-    let dispatch;
-    let observer;
-    beforeEach(() => {
-      dispatch = sandbox.spy();
-      observer = new HistoryObserver(dispatch);
-    });
-    it("should have a QueryInterface property", () => {
-      assert.property(observer, "QueryInterface");
-    });
-    describe("Other empty methods (to keep code coverage happy)", () => {
-      it("should have a various empty functions for xpconnect happiness", () => {
-        observer.onBeginUpdateBatch();
-        observer.onEndUpdateBatch();
-        observer.onDeleteVisits();
-      });
-    });
-  });
-
   describe("Custom dispatch", () => {
     it("should only dispatch 1 PLACES_LINKS_CHANGED action if many bookmark-added notifications happened at once", async () => {
       // Yes, onItemAdded has at least 8 arguments. See function definition for docs.
@@ -821,10 +783,16 @@ describe("PlacesFeed", () => {
         )
       );
     });
-    it("should only dispatch 1 PLACES_LINKS_CHANGED action if any onDeleteURI notifications happened at once", async () => {
+    it("should only dispatch 1 PLACES_LINKS_CHANGED action if any page-removed notifications happened at once", async () => {
-      await feed.historyObserver.onDeleteURI({ spec: "foo.com" });
+      await feed.placesObserver.handlePlacesEvent([
-      await feed.historyObserver.onDeleteURI({ spec: "foo1.com" });
+        { type: "page-removed", url: "foo.com", isRemovedFromStore: true },
-      await feed.historyObserver.onDeleteURI({ spec: "foo2.com" });
+      ]);
+      await feed.placesObserver.handlePlacesEvent([
+        { type: "page-removed", url: "foo1.com", isRemovedFromStore: true },
+      ]);
+      await feed.placesObserver.handlePlacesEvent([
+        { type: "page-removed", url: "foo2.com", isRemovedFromStore: true },
+      ]);
 
       assert.calledOnce(
         feed.store.dispatch.withArgs(

services/sync/modules/engines/history.js

@@ -220,60 +220,41 @@ HistoryStore.prototype = {
       }
     });
     if (toAdd.length || toRemove.length) {
-      // We want to notify history observers that a batch operation is underway
+      if (toRemove.length) {
-      // so they don't do lots of work for each incoming record.
+        // PlacesUtils.history.remove takes an array of visits to remove,
-      let observers = PlacesUtils.history.getObservers();
+        // but the error semantics are tricky - a single "bad" entry will cause
-      const notifyHistoryObservers = notification => {
+        // an exception before anything is removed. So we do remove them one at
-        for (let observer of observers) {
+        // a time.
+        await Async.yieldingForEach(toRemove, async record => {
           try {
-            observer[notification]();
+            await this.remove(record);
           } catch (ex) {
-            // don't log an error - it's not our code that failed and we don't
+            if (Async.isShutdownException(ex)) {
-            // want an error log written just for this.
+              throw ex;
-            this._log.info("history observer failed", ex);
-          }
-        }
-      };
-      notifyHistoryObservers("onBeginUpdateBatch");
-      try {
-        if (toRemove.length) {
-          // PlacesUtils.history.remove takes an array of visits to remove,
-          // but the error semantics are tricky - a single "bad" entry will cause
-          // an exception before anything is removed. So we do remove them one at
-          // a time.
-          await Async.yieldingForEach(toRemove, async record => {
-            try {
-              await this.remove(record);
-            } catch (ex) {
-              if (Async.isShutdownException(ex)) {
-                throw ex;
-              }
-              this._log.error("Failed to delete a place info", ex);
-              this._log.trace("The record that failed", record);
-              failed.push(record.id);
             }
-          });
+            this._log.error("Failed to delete a place info", ex);
-        }
+            this._log.trace("The record that failed", record);
-        for (let chunk of this._generateChunks(toAdd)) {
+            failed.push(record.id);
-          // Per bug 1415560, we ignore any exceptions returned by insertMany
-          // as they are likely to be spurious. We do supply an onError handler
-          // and log the exceptions seen there as they are likely to be
-          // informative, but we still never abort the sync based on them.
-          try {
-            await PlacesUtils.history.insertMany(chunk, null, failedVisit => {
-              this._log.info(
-                "Failed to insert a history record",
-                failedVisit.guid
-              );
-              this._log.trace("The record that failed", failedVisit);
-              failed.push(failedVisit.guid);
-            });
-          } catch (ex) {
-            this._log.info("Failed to insert history records", ex);
           }
+        });
+      }
+      for (let chunk of this._generateChunks(toAdd)) {
+        // Per bug 1415560, we ignore any exceptions returned by insertMany
+        // as they are likely to be spurious. We do supply an onError handler
+        // and log the exceptions seen there as they are likely to be
+        // informative, but we still never abort the sync based on them.
+        try {
+          await PlacesUtils.history.insertMany(chunk, null, failedVisit => {
+            this._log.info(
+              "Failed to insert a history record",
+              failedVisit.guid
+            );
+            this._log.trace("The record that failed", failedVisit);
+            failed.push(failedVisit.guid);
+          });
+        } catch (ex) {
+          this._log.info("Failed to insert history records", ex);
         }
-      } finally {
-        notifyHistoryObservers("onEndUpdateBatch");
       }
     }
 
@@ -521,7 +502,6 @@ HistoryTracker.prototype = {
 
   onStart() {
     this._log.info("Adding Places observer.");
-    PlacesUtils.history.addObserver(this, true);
     this._placesObserver = new PlacesWeakCallbackWrapper(
       this.handlePlacesEvents.bind(this)
     );
@@ -533,7 +513,6 @@ HistoryTracker.prototype = {
 
   onStop() {
     this._log.info("Removing Places observer.");
-    PlacesUtils.history.removeObserver(this);
     if (this._placesObserver) {
       PlacesObservers.removeListener(
         ["page-visited", "history-cleared", "page-removed"],
@@ -542,45 +521,7 @@ HistoryTracker.prototype = {
     }
   },
 
-  QueryInterface: ChromeUtils.generateQI([
+  QueryInterface: ChromeUtils.generateQI(["nsISupportsWeakReference"]),
-    "nsINavHistoryObserver",
-    "nsISupportsWeakReference",
-  ]),
-
-  async onDeleteAffectsGUID(uri, guid, reason, source, increment) {
-    if (this.ignoreAll || reason === PlacesVisitRemoved.REASON_EXPIRED) {
-      return;
-    }
-    this._log.trace(source + ": " + uri.spec + ", reason " + reason);
-    const added = await this.addChangedID(guid);
-    if (added) {
-      this.score += increment;
-    }
-  },
-
-  onDeleteVisits(uri, partialRemoval, guid, reason) {
-    this.asyncObserver.enqueueCall(() =>
-      this.onDeleteAffectsGUID(
-        uri,
-        guid,
-        reason,
-        "onDeleteVisits",
-        SCORE_INCREMENT_SMALL
-      )
-    );
-  },
-
-  onDeleteURI(uri, guid, reason) {
-    this.asyncObserver.enqueueCall(() =>
-      this.onDeleteAffectsGUID(
-        uri,
-        guid,
-        reason,
-        "onDeleteURI",
-        SCORE_INCREMENT_XLARGE
-      )
-    );
-  },
 
   handlePlacesEvents(aEvents) {
     this.asyncObserver.enqueueCall(() => this._handlePlacesEvents(aEvents));
@@ -634,8 +575,4 @@ HistoryTracker.prototype = {
       }
     }
   },
-
-  onBeginUpdateBatch() {},
-  onEndUpdateBatch() {},
-  onBeforeDeleteURI() {},
 };

services/sync/tests/unit/head_helpers.js

@@ -617,7 +617,6 @@ async function promiseVisit(expectedType, expectedURI) {
   return new Promise(resolve => {
     function done(type, uri) {
       if (uri == expectedURI.spec && type == expectedType) {
-        PlacesUtils.history.removeObserver(observer);
         PlacesObservers.removeListener(
           ["page-visited", "page-removed"],
           observer.handlePlacesEvents
@@ -636,14 +635,7 @@ async function promiseVisit(expectedType, expectedURI) {
           done("removed", events[0].url);
         }
       },
-      onBeginUpdateBatch() {},
-      onEndUpdateBatch() {},
-      onDeleteURI(uri) {
-        done("removed", uri.spec);
-      },
-      onDeleteVisits() {},
     };
-    PlacesUtils.history.addObserver(observer, false);
     PlacesObservers.addListener(
       ["page-visited", "page-removed"],
       observer.handlePlacesEvents

toolkit/components/downloads/DownloadHistory.jsm

@@ -191,8 +191,6 @@ var DownloadCache = {
       return this._initializePromise;
     }
     this._initializePromise = (async () => {
-      PlacesUtils.history.addObserver(this, true);
-
       const placesObserver = new PlacesWeakCallbackWrapper(
         this.handlePlacesEvents.bind(this)
       );
@@ -326,10 +324,7 @@ var DownloadCache = {
     }
   },
 
-  QueryInterface: ChromeUtils.generateQI([
+  QueryInterface: ChromeUtils.generateQI(["nsISupportsWeakReference"]),
-    "nsINavHistoryObserver",
-    "nsISupportsWeakReference",
-  ]),
 
   handlePlacesEvents(events) {
     for (const event of events) {
@@ -347,14 +342,6 @@ var DownloadCache = {
       }
     }
   },
-
-  // nsINavHistoryObserver
-  onDeleteURI(uri) {
-    this._data.delete(uri.spec);
-  },
-  onBeginUpdateBatch() {},
-  onEndUpdateBatch() {},
-  onDeleteVisits() {},
 };
 
 /**

toolkit/components/downloads/DownloadIntegration.jsm

@@ -58,11 +58,6 @@ ChromeUtils.defineModuleGetter(
   "NetUtil",
   "resource://gre/modules/NetUtil.jsm"
 );
-ChromeUtils.defineModuleGetter(
-  this,
-  "PlacesUtils",
-  "resource://gre/modules/PlacesUtils.jsm"
-);
 ChromeUtils.defineModuleGetter(
   this,
   "Services",
@@ -1228,7 +1223,6 @@ var DownloadObserver = {
  */
 var DownloadHistoryObserver = function(aList) {
   this._list = aList;
-  PlacesUtils.history.addObserver(this);
 
   const placesObserver = new PlacesWeakCallbackWrapper(
     this.handlePlacesEvents.bind(this)
@@ -1245,8 +1239,6 @@ DownloadHistoryObserver.prototype = {
    */
   _list: null,
 
-  QueryInterface: ChromeUtils.generateQI(["nsINavHistoryObserver"]),
-
   handlePlacesEvents(events) {
     for (const event of events) {
       switch (event.type) {
@@ -1265,17 +1257,6 @@ DownloadHistoryObserver.prototype = {
       }
     }
   },
-
-  // nsINavHistoryObserver
-  onDeleteURI: function DL_onDeleteURI(aURI, aGUID) {
-    this._list.removeFinished(download =>
-      aURI.equals(NetUtil.newURI(download.source.url))
-    );
-  },
-
-  onBeginUpdateBatch() {},
-  onEndUpdateBatch() {},
-  onDeleteVisits() {},
 };
 
 /**

toolkit/components/places/nsINavHistoryService.idl

@@ -578,62 +578,6 @@ interface nsINavHistoryObserver : nsISupports
    * ahead and update UI, etc.
    */
   void onEndUpdateBatch();
-
-  /**
-   * Removed by the user.
-   */
-  const unsigned short REASON_DELETED = 0;
-  /**
-   * Removed by automatic expiration.
-   */
-  const unsigned short REASON_EXPIRED = 1;
-
-  /**
-   * This page and all of its visits are being deleted. Note: the page may not
-   * necessarily have actually existed for this function to be called.
-   *
-   * Delete notifications are only 99.99% accurate. Batch delete operations
-   * must be done in two steps, so first come notifications, then a bulk
-   * delete. If there is some error in the middle (for example, out of memory)
-   * then you'll get a notification and it won't get deleted. There's no easy
-   * way around this.
-   *
-   * @param aURI
-   *        The URI that was deleted.
-   * @param aGUID
-   *        The unique ID associated with the page.
-   * @param aReason
-   *        Indicates the reason for the removal.  see REASON_* constants.
-   */
-  void onDeleteURI(in nsIURI aURI,
-                   in ACString aGUID,
-                   in unsigned short aReason);
-
-  /**
-   * Called when some visits of an history entry are expired.
-   *
-   * @param aURI
-   *        The page whose visits have been expired.
-   * @param aPartialRemoval
-   *        Set to true if only some of the visits for the page have been removed.
-   * @param aGUID
-   *        The unique ID associated with the page.
-   *
-   * @note: when all visits for a page are expired and also the full page entry
-   *        is expired, you will only get an onDeleteURI notification.  If a
-   *        page entry is removed, then you can be sure that we don't have
-   *        anymore visits for it.
-   * @param aReason
-   *        Indicates the reason for the removal.  see REASON_* constants.
-   * @param aTransitionType
-   *        If it's a valid TRANSITION_* value, all visits of the specified type
-   *        have been removed.
-   */
-  void onDeleteVisits(in nsIURI aURI,
-                      in boolean aPartialRemoval,
-                      in ACString aGUID,
-                      in unsigned short aReason,
-                      in unsigned long aTransitionType);
 };
 
 

toolkit/components/places/nsNavHistoryResult.cpp

@@ -2262,9 +2262,8 @@ nsresult nsNavHistoryQueryResultNode::OnTitleChanged(
  * Here, we can always live update by just deleting all occurrences of
  * the given URI.
  */
-NS_IMETHODIMP
+nsresult nsNavHistoryQueryResultNode::OnPageRemovedFromStore(
-nsNavHistoryQueryResultNode::OnDeleteURI(nsIURI* aURI, const nsACString& aGUID,
+    nsIURI* aURI, const nsACString& aGUID, uint16_t aReason) {
-                                         uint16_t aReason) {
   nsNavHistoryResult* result = GetResult();
   NS_ENSURE_STATE(result);
   if (result->mBatchInProgress &&
@@ -2328,11 +2327,9 @@ static nsresult setFaviconCallback(nsNavHistoryResultNode* aNode,
   return NS_OK;
 }
 
-NS_IMETHODIMP
+nsresult nsNavHistoryQueryResultNode::OnPageRemovedVisits(
-nsNavHistoryQueryResultNode::OnDeleteVisits(nsIURI* aURI, bool aPartialRemoval,
+    nsIURI* aURI, bool aPartialRemoval, const nsACString& aGUID,
-                                            const nsACString& aGUID,
+    uint16_t aReason, uint32_t aTransitionType) {
-                                            uint16_t aReason,
-                                            uint32_t aTransitionType) {
   MOZ_ASSERT(
       mOptions->QueryType() == nsINavHistoryQueryOptions::QUERY_TYPE_HISTORY,
       "Bookmarks queries should not get a OnDeleteVisits notification");
@@ -2340,16 +2337,16 @@ nsNavHistoryQueryResultNode::OnDeleteVisits(nsIURI* aURI, bool aPartialRemoval,
   if (!aPartialRemoval) {
     // All visits for this uri have been removed, but the uri won't be removed
     // from the databse, most likely because it's a bookmark.  For a history
-    // query this is equivalent to a onDeleteURI notification.
+    // query this is equivalent to a OnPageRemovedFromStore notification.
-    nsresult rv = OnDeleteURI(aURI, aGUID, aReason);
+    nsresult rv = OnPageRemovedFromStore(aURI, aGUID, aReason);
     NS_ENSURE_SUCCESS(rv, rv);
   }
   if (aTransitionType > 0) {
     // All visits for aTransitionType have been removed, if the query is
-    // filtering on such transition type, this is equivalent to an onDeleteURI
+    // filtering on such transition type, this is equivalent to an
-    // notification.
+    // OnPageRemovedFromStore notification.
     if (mTransitions.Length() > 0 && mTransitions.Contains(aTransitionType)) {
-      nsresult rv = OnDeleteURI(aURI, aGUID, aReason);
+      nsresult rv = OnPageRemovedFromStore(aURI, aGUID, aReason);
       NS_ENSURE_SUCCESS(rv, rv);
     }
   }
@@ -3511,8 +3508,8 @@ void nsNavHistoryResult::StopObserving() {
     nsNavHistory* history = nsNavHistory::GetHistoryService();
     if (history) {
       history->RemoveObserver(this);
-      mIsHistoryObserver = false;
     }
+    mIsHistoryObserver = false;
     events.AppendElement(PlacesEventType::History_cleared);
     events.AppendElement(PlacesEventType::Page_removed);
   }
@@ -4219,14 +4216,15 @@ void nsNavHistoryResult::HandlePlacesEvent(const PlacesEventSequence& aEvents) {
         }
 
         if (removeEvent->mIsRemovedFromStore) {
-          ENUMERATE_HISTORY_OBSERVERS(
+          ENUMERATE_HISTORY_OBSERVERS(OnPageRemovedFromStore(
-              OnDeleteURI(uri, removeEvent->mPageGuid, removeEvent->mReason));
+              uri, removeEvent->mPageGuid, removeEvent->mReason));
         } else {
           ENUMERATE_HISTORY_OBSERVERS(
-              OnDeleteVisits(uri, removeEvent->mIsPartialVisistsRemoval,
+              OnPageRemovedVisits(uri, removeEvent->mIsPartialVisistsRemoval,
-                             removeEvent->mPageGuid, removeEvent->mReason,
+                                  removeEvent->mPageGuid, removeEvent->mReason,
-                             removeEvent->mTransitionType));
+                                  removeEvent->mTransitionType));
         }
+
         break;
       }
       default: {
@@ -4237,29 +4235,6 @@ void nsNavHistoryResult::HandlePlacesEvent(const PlacesEventSequence& aEvents) {
   }
 }
 
-NS_IMETHODIMP
-nsNavHistoryResult::OnDeleteURI(nsIURI* aURI, const nsACString& aGUID,
-                                uint16_t aReason) {
-  NS_ENSURE_ARG(aURI);
-
-  ENUMERATE_HISTORY_OBSERVERS(OnDeleteURI(aURI, aGUID, aReason));
-  return NS_OK;
-}
-
-/**
- * Don't do anything when visits expire.
- */
-NS_IMETHODIMP
-nsNavHistoryResult::OnDeleteVisits(nsIURI* aURI, bool aPartialRemoval,
-                                   const nsACString& aGUID, uint16_t aReason,
-                                   uint32_t aTransitionType) {
-  NS_ENSURE_ARG(aURI);
-
-  ENUMERATE_HISTORY_OBSERVERS(
-      OnDeleteVisits(aURI, aPartialRemoval, aGUID, aReason, aTransitionType));
-  return NS_OK;
-}
-
 void nsNavHistoryResult::OnMobilePrefChanged() {
   ENUMERATE_MOBILE_PREF_OBSERVERS(
       OnMobilePrefChanged(Preferences::GetBool(MOBILE_BOOKMARKS_PREF, false)));

toolkit/components/places/nsNavHistoryResult.h

@@ -60,24 +60,6 @@ class nsTrimInt64HashKey : public PLDHashEntryHdr {
   const int64_t mValue;
 };
 
-// Declare methods for implementing nsINavBookmarkObserver
-// and nsINavHistoryObserver (some methods, such as BeginUpdateBatch overlap)
-#define NS_DECL_BOOKMARK_HISTORY_OBSERVER_BASE(...)                    \
-  NS_DECL_NSINAVBOOKMARKOBSERVER                                       \
-  NS_IMETHOD OnDeleteURI(nsIURI* aURI, const nsACString& aGUID,        \
-                         uint16_t aReason) __VA_ARGS__;                \
-  NS_IMETHOD OnDeleteVisits(nsIURI* aURI, bool aPartialRemoval,        \
-                            const nsACString& aGUID, uint16_t aReason, \
-                            uint32_t aTransitionType) __VA_ARGS__;
-
-// The internal version is used by query nodes.
-#define NS_DECL_BOOKMARK_HISTORY_OBSERVER_INTERNAL \
-  NS_DECL_BOOKMARK_HISTORY_OBSERVER_BASE()
-
-// The external version is used by results.
-#define NS_DECL_BOOKMARK_HISTORY_OBSERVER_EXTERNAL(...) \
-  NS_DECL_BOOKMARK_HISTORY_OBSERVER_BASE(__VA_ARGS__)
-
 // nsNavHistoryResult
 //
 //    nsNavHistory creates this object and fills in mChildren (by getting
@@ -104,7 +86,7 @@ class nsNavHistoryResult final
   NS_DECL_NSINAVHISTORYRESULT
   NS_DECL_CYCLE_COLLECTION_CLASS_AMBIGUOUS(nsNavHistoryResult,
                                            nsINavHistoryResult)
-  NS_DECL_BOOKMARK_HISTORY_OBSERVER_EXTERNAL(override)
+  NS_DECL_NSINAVBOOKMARKOBSERVER;
 
   void AddHistoryObserver(nsNavHistoryQueryResultNode* aNode);
   void AddBookmarkFolderObserver(nsNavHistoryFolderResultNode* aNode,
@@ -687,7 +669,7 @@ class nsNavHistoryQueryResultNode final
 
   virtual nsresult OpenContainer() override;
 
-  NS_DECL_BOOKMARK_HISTORY_OBSERVER_INTERNAL
+  NS_DECL_NSINAVBOOKMARKOBSERVER;
 
   nsresult OnItemAdded(int64_t aItemId, int64_t aParentId, int32_t aIndex,
                        uint16_t aItemType, nsIURI* aURI, PRTime aDateAdded,
@@ -706,6 +688,12 @@ class nsNavHistoryQueryResultNode final
   nsresult OnTitleChanged(nsIURI* aURI, const nsAString& aPageTitle,
                           const nsACString& aGUID);
   nsresult OnClearHistory();
+  nsresult OnPageRemovedFromStore(nsIURI* aURI, const nsACString& aGUID,
+                                  uint16_t aReason);
+  nsresult OnPageRemovedVisits(nsIURI* aURI, bool aPartialRemoval,
+                               const nsACString& aGUID, uint16_t aReason,
+                               uint32_t aTransitionType);
+
   virtual void OnRemoving() override;
 
  public:

toolkit/components/places/tests/browser/head.js

@@ -80,20 +80,6 @@ function promiseFieldForUrl(aURI, aFieldName) {
   });
 }
 
-/**
- * Generic nsINavHistoryObserver that doesn't implement anything, but provides
- * dummy methods to prevent errors about an object not having a certain method.
- */
-function NavHistoryObserver() {}
-
-NavHistoryObserver.prototype = {
-  onBeginUpdateBatch() {},
-  onEndUpdateBatch() {},
-  onDeleteURI() {},
-  onDeleteVisits() {},
-  QueryInterface: ChromeUtils.generateQI(["nsINavHistoryObserver"]),
-};
-
 function whenNewWindowLoaded(aOptions, aCallback) {
   BrowserTestUtils.waitForNewWindow().then(aCallback);
   OpenBrowserWindow(aOptions);

toolkit/components/places/tests/expiration/test_pref_maxpages.js

@@ -84,17 +84,6 @@ add_task(async function test_pref_maxpages() {
       await PlacesTestUtils.addVisits({ uri: uri(page), visitDate: now-- });
     }
 
-    // Observe history.
-    let historyObserver = new NavHistoryObserver();
-    historyObserver.onDeleteURI = aURI => {
-      print("onDeleteURI " + aURI.spec);
-      currentTest.receivedNotifications++;
-    };
-    historyObserver.onDeleteVisits = (aURI, aPartialRemoval) => {
-      print("onDeleteVisits " + aURI.spec + " " + aPartialRemoval);
-    };
-    PlacesUtils.history.addObserver(historyObserver);
-
     const listener = events => {
       for (const event of events) {
         print("page-removed " + event.url);
@@ -111,7 +100,6 @@ add_task(async function test_pref_maxpages() {
     // Expire now.
     await promiseForceExpirationStep(-1);
 
-    PlacesUtils.history.removeObserver(historyObserver, false);
     PlacesObservers.removeListener(["page-removed"], listener);
 
     Assert.equal(

toolkit/components/places/tests/head_common.js

@@ -728,20 +728,6 @@ NavBookmarkObserver.prototype = {
   QueryInterface: ChromeUtils.generateQI(["nsINavBookmarkObserver"]),
 };
 
-/**
- * Generic nsINavHistoryObserver that doesn't implement anything, but provides
- * dummy methods to prevent errors about an object not having a certain method.
- */
-function NavHistoryObserver() {}
-
-NavHistoryObserver.prototype = {
-  onBeginUpdateBatch() {},
-  onEndUpdateBatch() {},
-  onDeleteURI() {},
-  onDeleteVisits() {},
-  QueryInterface: ChromeUtils.generateQI(["nsINavHistoryObserver"]),
-};
-
 /**
  * Generic nsINavHistoryResultObserver that doesn't implement anything, but
  * provides dummy methods to prevent errors about an object not having a certain

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

@@ -39,33 +39,8 @@ add_task(async function test_remove_single() {
     }
 
     let shouldRemove = !options.addBookmark;
-    let observer;
     let placesEventListener;
     let promiseObserved = new Promise((resolve, reject) => {
-      observer = {
-        onBeginUpdateBatch() {},
-        onEndUpdateBatch() {},
-        onDeleteURI(aURI) {
-          try {
-            Assert.ok(shouldRemove, "Observing onDeleteURI");
-            Assert.equal(
-              aURI.spec,
-              uri.spec,
-              "Observing effect on the right uri"
-            );
-          } finally {
-            resolve();
-          }
-        },
-        onDeleteVisits(aURI) {
-          Assert.equal(
-            aURI.spec,
-            uri.spec,
-            "Observing onDeleteVisits on the right uri"
-          );
-        },
-      };
-
       placesEventListener = events => {
         for (const event of events) {
           switch (event.type) {
@@ -105,7 +80,6 @@ add_task(async function test_remove_single() {
         }
       };
     });
-    PlacesUtils.history.addObserver(observer);
     PlacesObservers.addListener(
       [
         "page-title-changed",
@@ -138,7 +112,6 @@ add_task(async function test_remove_single() {
     }
 
     await promiseObserved;
-    PlacesUtils.history.removeObserver(observer);
     PlacesObservers.removeListener(
       [
         "page-title-changed",

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

@@ -58,12 +58,9 @@ add_task(async function test_removeByFilter() {
     await checkBeforeRemove();
 
     // Take care of any observers (due to bookmarks)
-    let { observer, placesEventListener, promiseObserved } = getObserverPromise(
+    let { placesEventListener, promiseObserved } = getObserverPromise(
       bookmarkedUri
     );
-    if (observer) {
-      PlacesUtils.history.addObserver(observer, false);
-    }
     if (placesEventListener) {
       PlacesObservers.addListener(
         ["page-title-changed", "history-cleared", "page-removed"],
@@ -91,12 +88,8 @@ add_task(async function test_removeByFilter() {
     }
     await checkAfterRemove();
     await promiseObserved;
-    if (observer) {
-      PlacesUtils.history.removeObserver(observer);
-      // Remove the added bookmarks as they interfere with following tests
-      await PlacesUtils.bookmarks.eraseEverything();
-    }
     if (placesEventListener) {
+      await PlacesUtils.bookmarks.eraseEverything();
       PlacesObservers.removeListener(
         ["page-title-changed", "history-cleared", "page-removed"],
         placesEventListener
@@ -456,43 +449,10 @@ add_task(async function test_chunking() {
 
 function getObserverPromise(bookmarkedUri) {
   if (!bookmarkedUri) {
-    return { observer: null, promiseObserved: Promise.resolve() };
+    return { promiseObserved: Promise.resolve() };
   }
-  let observer;
   let placesEventListener;
   let promiseObserved = new Promise((resolve, reject) => {
-    observer = {
-      onBeginUpdateBatch() {},
-      onEndUpdateBatch() {},
-      onDeleteURI(aURI) {
-        try {
-          Assert.notEqual(
-            aURI.spec,
-            bookmarkedUri,
-            "Bookmarked URI should not be deleted"
-          );
-        } finally {
-          resolve();
-        }
-      },
-      onDeleteVisits(aURI, aPartialRemoval) {
-        try {
-          Assert.equal(
-            aPartialRemoval,
-            false,
-            "Observing onDeleteVisits deletes all visits"
-          );
-          Assert.equal(
-            aURI.spec,
-            bookmarkedUri,
-            "Bookmarked URI should have all visits removed but not the page itself"
-          );
-        } finally {
-          resolve();
-        }
-      },
-    };
-
     placesEventListener = events => {
       for (const event of events) {
         switch (event.type) {
@@ -530,5 +490,5 @@ function getObserverPromise(bookmarkedUri) {
       }
     };
   });
-  return { observer, placesEventListener, promiseObserved };
+  return { placesEventListener, promiseObserved };
 }

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

@@ -38,10 +38,6 @@ add_task(async function test_remove_many() {
       hasBookmark,
       // `true` once `onResult` has been called for this page
       onResultCalled: false,
-      // `true` once `onDeleteVisits` has been called for this page
-      onDeleteVisitsCalled: false,
-      // `true` once `onDeleteURI` has been called for this page
-      onDeleteURICalled: false,
       // `true` once page-removed for store has been fired for this page
       pageRemovedFromStore: false,
       // `true` once page-removed for all visits has been fired for this page
@@ -81,37 +77,6 @@ add_task(async function test_remove_many() {
   }
 
   let onPageRankingChanged = false;
-  let observer = {
-    onBeginUpdateBatch() {},
-    onEndUpdateBatch() {},
-    onVisits(aVisits) {
-      Assert.ok(false, "Unexpected call to onVisits " + aVisits.length);
-    },
-    onDeleteURI(aURI) {
-      let origin = pages.find(x => x.uri.spec == aURI.spec);
-      Assert.ok(origin);
-      Assert.ok(
-        !origin.hasBookmark,
-        "Observing onDeleteURI on a page without a bookmark"
-      );
-      Assert.ok(
-        !origin.onDeleteURICalled,
-        "Observing onDeleteURI for the first time"
-      );
-      origin.onDeleteURICalled = true;
-    },
-    onDeleteVisits(aURI) {
-      let origin = pages.find(x => x.uri.spec == aURI.spec);
-      Assert.ok(origin);
-      Assert.ok(
-        !origin.onDeleteVisitsCalled,
-        "Observing onDeleteVisits for the first time"
-      );
-      origin.onDeleteVisitsCalled = true;
-    },
-  };
-  PlacesUtils.history.addObserver(observer);
-
   const placesEventListener = events => {
     for (const event of events) {
       switch (event.type) {
@@ -181,7 +146,6 @@ add_task(async function test_remove_many() {
 
   Assert.ok(removed, "Something was removed");
 
-  PlacesUtils.history.removeObserver(observer);
   PlacesObservers.removeListener(
     [
       "page-title-changed",

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

@@ -66,7 +66,6 @@ add_task(async function test_removeVisitsByFilter() {
           // `true` if there is a bookmark for this URI, i.e. of the page
           // should not be entirely removed.
           hasBookmark,
-          onDeleteURI: null,
         },
       };
       visits.push(visit);
@@ -142,22 +141,6 @@ add_task(async function test_removeVisitsByFilter() {
       }
     }
 
-    let observer = {
-      deferred: PromiseUtils.defer(),
-      onBeginUpdateBatch() {},
-      onEndUpdateBatch() {},
-      onDeleteURI(aURI) {
-        info("onDeleteURI " + aURI.spec);
-        let deferred = uriDeletePromises.get(aURI.spec);
-        Assert.ok(!!deferred, "Observing onDeleteURI");
-        deferred.resolve();
-      },
-      onDeleteVisits(aURI) {
-        // Not sure we can test anything.
-      },
-    };
-    PlacesUtils.history.addObserver(observer);
-
     const placesEventListener = events => {
       for (const event of events) {
         switch (event.type) {
@@ -262,7 +245,6 @@ add_task(async function test_removeVisitsByFilter() {
     await Promise.all(Array.from(uriDeletePromises.values()));
     info("Checking frecency change promises.");
     await Promise.all(rankingChangePromises);
-    PlacesUtils.history.removeObserver(observer);
     PlacesObservers.removeListener(
       ["page-title-changed", "history-cleared", "pages-rank-changed"],
       placesEventListener

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

@@ -1,36 +1,6 @@
 /* Any copyright is dedicated to the Public Domain.
    http://creativecommons.org/publicdomain/zero/1.0/ */
 
-/**
- * Generic nsINavHistoryObserver that doesn't implement anything, but provides
- * dummy methods to prevent errors about an object not having a certain method.
- */
-function NavHistoryObserver() {}
-NavHistoryObserver.prototype = {
-  onBeginUpdateBatch() {},
-  onEndUpdateBatch() {},
-  onDeleteURI() {},
-  onDeleteVisits() {},
-  QueryInterface: ChromeUtils.generateQI(["nsINavHistoryObserver"]),
-};
-
-/**
- * Registers a one-time history observer for and calls the callback
- * when the specified nsINavHistoryObserver method is called.
- * Returns a promise that is resolved when the callback returns.
- */
-function onNotify(callback) {
-  return new Promise(resolve => {
-    let obs = new NavHistoryObserver();
-    obs[callback.name] = function() {
-      PlacesUtils.history.removeObserver(this);
-      callback.apply(this, arguments);
-      resolve();
-    };
-    PlacesUtils.history.addObserver(obs);
-  });
-}
-
 /**
  * Registers a one-time places observer for 'page-visited',
  * which resolves a promise on being called.

toolkit/components/thumbnails/PageThumbs.jsm

@@ -34,7 +34,6 @@ XPCOMUtils.defineLazyGlobalGetters(this, ["FileReader"]);
 
 XPCOMUtils.defineLazyModuleGetters(this, {
   Services: "resource://gre/modules/Services.jsm",
-  PlacesUtils: "resource://gre/modules/PlacesUtils.jsm",
   AsyncShutdown: "resource://gre/modules/AsyncShutdown.jsm",
   PageThumbUtils: "resource://gre/modules/PageThumbUtils.jsm",
   PrivateBrowsingUtils: "resource://gre/modules/PrivateBrowsingUtils.jsm",
@@ -117,7 +116,6 @@ var PageThumbs = {
   init: function PageThumbs_init() {
     if (!this._initialized) {
       this._initialized = true;
-      PlacesUtils.history.addObserver(PageThumbsHistoryObserver, true);
 
       this._placesObserver = new PlacesWeakCallbackWrapper(
         this.handlePlacesEvents.bind(this)
@@ -886,18 +884,3 @@ var PageThumbsWorker = new BasePromiseWorker(
 // As the PageThumbsWorker performs I/O, we can receive instances of
 // OS.File.Error, so we need to install a decoder.
 PageThumbsWorker.ExceptionHandlers["OS.File.Error"] = OS.File.Error.fromMsg;
-
-var PageThumbsHistoryObserver = {
-  onDeleteURI(aURI, aGUID) {
-    PageThumbsStorage.remove(aURI.spec);
-  },
-
-  onBeginUpdateBatch() {},
-  onEndUpdateBatch() {},
-  onDeleteVisits() {},
-
-  QueryInterface: ChromeUtils.generateQI([
-    "nsINavHistoryObserver",
-    "nsISupportsWeakReference",
-  ]),
-};

toolkit/modules/NewTabUtils.jsm

@@ -586,12 +586,6 @@ var BlockedLinks = {
  * the history to retrieve the most frequently visited sites.
  */
 var PlacesProvider = {
-  /**
-   * A count of how many batch updates are under way (batches may be nested, so
-   * we keep a counter instead of a simple bool).
-   **/
-  _batchProcessingDepth: 0,
-
   /**
    * Set this to change the maximum number of links the provider will provide.
    */
@@ -601,7 +595,6 @@ var PlacesProvider = {
    * Must be called before the provider is used.
    */
   init: function PlacesProvider_init() {
-    PlacesUtils.history.addObserver(this, true);
     this._placesObserver = new PlacesWeakCallbackWrapper(
       this.handlePlacesEvents.bind(this)
     );
@@ -709,22 +702,7 @@ var PlacesProvider = {
 
   _observers: [],
 
-  /**
-   * Called by the history service.
-   */
-  onBeginUpdateBatch() {
-    this._batchProcessingDepth += 1;
-  },
-
-  onEndUpdateBatch() {
-    this._batchProcessingDepth -= 1;
-  },
-
   handlePlacesEvents(aEvents) {
-    if (this._batchProcessingDepth) {
-      return;
-    }
-
     for (let event of aEvents) {
       switch (event.type) {
         case "page-visited": {
@@ -789,11 +767,6 @@ var PlacesProvider = {
       }
     }
   },
-
-  QueryInterface: ChromeUtils.generateQI([
-    "nsINavHistoryObserver",
-    "nsISupportsWeakReference",
-  ]),
 };
 
 /**