diff --git a/browser/components/extensions/parent/ext-history.js b/browser/components/extensions/parent/ext-history.js index 8f8fffdf1e65..da3b8e260d36 100644 --- a/browser/components/extensions/parent/ext-history.js +++ b/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) => { - fire.sync(data); - }; - const historyClearedListener = events => { + const listener = 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 ); }; }, diff --git a/browser/components/newtab/lib/PlacesFeed.jsm b/browser/components/newtab/lib/PlacesFeed.jsm index 015c9b07521c..b9d58a3af934 100644 --- a/browser/components/newtab/lib/PlacesFeed.jsm +++ b/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; diff --git a/browser/components/newtab/test/unit/lib/PlacesFeed.test.js b/browser/components/newtab/test/unit/lib/PlacesFeed.test.js index 5aaacb7d63ec..022e6fa63434 100644 --- a/browser/components/newtab/test/unit/lib/PlacesFeed.test.js +++ b/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 () => { - await feed.historyObserver.onDeleteURI({ spec: "foo.com" }); - await feed.historyObserver.onDeleteURI({ spec: "foo1.com" }); - await feed.historyObserver.onDeleteURI({ spec: "foo2.com" }); + it("should only dispatch 1 PLACES_LINKS_CHANGED action if any page-removed notifications happened at once", async () => { + await feed.placesObserver.handlePlacesEvent([ + { type: "page-removed", url: "foo.com", isRemovedFromStore: true }, + ]); + 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( diff --git a/services/sync/modules/engines/history.js b/services/sync/modules/engines/history.js index fda4375e5e5c..bff108ae3256 100644 --- a/services/sync/modules/engines/history.js +++ b/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 - // so they don't do lots of work for each incoming record. - let observers = PlacesUtils.history.getObservers(); - const notifyHistoryObservers = notification => { - for (let observer of observers) { + 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 { - observer[notification](); + await this.remove(record); } catch (ex) { - // don't log an error - it's not our code that failed and we don't - // want an error log written just for this. - 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); + if (Async.isShutdownException(ex)) { + throw 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); + this._log.error("Failed to delete a place info", ex); + this._log.trace("The record that failed", record); + failed.push(record.id); } + }); + } + 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([ - "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 - ) - ); - }, + QueryInterface: ChromeUtils.generateQI(["nsISupportsWeakReference"]), handlePlacesEvents(aEvents) { this.asyncObserver.enqueueCall(() => this._handlePlacesEvents(aEvents)); @@ -634,8 +575,4 @@ HistoryTracker.prototype = { } } }, - - onBeginUpdateBatch() {}, - onEndUpdateBatch() {}, - onBeforeDeleteURI() {}, }; diff --git a/services/sync/tests/unit/head_helpers.js b/services/sync/tests/unit/head_helpers.js index b3954f494ff9..45fe283f83fb 100644 --- a/services/sync/tests/unit/head_helpers.js +++ b/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 diff --git a/toolkit/components/downloads/DownloadHistory.jsm b/toolkit/components/downloads/DownloadHistory.jsm index 6ee9a9b2bff3..540e7bf4aed7 100644 --- a/toolkit/components/downloads/DownloadHistory.jsm +++ b/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([ - "nsINavHistoryObserver", - "nsISupportsWeakReference", - ]), + QueryInterface: ChromeUtils.generateQI(["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() {}, }; /** diff --git a/toolkit/components/downloads/DownloadIntegration.jsm b/toolkit/components/downloads/DownloadIntegration.jsm index ad0b7b25d73d..9b01c0158e1f 100644 --- a/toolkit/components/downloads/DownloadIntegration.jsm +++ b/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() {}, }; /** diff --git a/toolkit/components/places/nsINavHistoryService.idl b/toolkit/components/places/nsINavHistoryService.idl index 562d849d9192..5d84325e4631 100644 --- a/toolkit/components/places/nsINavHistoryService.idl +++ b/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); }; diff --git a/toolkit/components/places/nsNavHistoryResult.cpp b/toolkit/components/places/nsNavHistoryResult.cpp index a3f6a1561301..5250531c4eae 100644 --- a/toolkit/components/places/nsNavHistoryResult.cpp +++ b/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 -nsNavHistoryQueryResultNode::OnDeleteURI(nsIURI* aURI, const nsACString& aGUID, - uint16_t aReason) { +nsresult nsNavHistoryQueryResultNode::OnPageRemovedFromStore( + nsIURI* aURI, const nsACString& aGUID, 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 -nsNavHistoryQueryResultNode::OnDeleteVisits(nsIURI* aURI, bool aPartialRemoval, - const nsACString& aGUID, - uint16_t aReason, - uint32_t aTransitionType) { +nsresult nsNavHistoryQueryResultNode::OnPageRemovedVisits( + nsIURI* aURI, bool aPartialRemoval, const nsACString& aGUID, + 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. - nsresult rv = OnDeleteURI(aURI, aGUID, aReason); + // query this is equivalent to a OnPageRemovedFromStore notification. + 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 - // notification. + // filtering on such transition type, this is equivalent to an + // 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( - OnDeleteURI(uri, removeEvent->mPageGuid, removeEvent->mReason)); + ENUMERATE_HISTORY_OBSERVERS(OnPageRemovedFromStore( + uri, removeEvent->mPageGuid, removeEvent->mReason)); } else { ENUMERATE_HISTORY_OBSERVERS( - OnDeleteVisits(uri, removeEvent->mIsPartialVisistsRemoval, - removeEvent->mPageGuid, removeEvent->mReason, - removeEvent->mTransitionType)); + OnPageRemovedVisits(uri, removeEvent->mIsPartialVisistsRemoval, + removeEvent->mPageGuid, removeEvent->mReason, + 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))); diff --git a/toolkit/components/places/nsNavHistoryResult.h b/toolkit/components/places/nsNavHistoryResult.h index bef36ed051c2..2d7fd3b6c3d6 100644 --- a/toolkit/components/places/nsNavHistoryResult.h +++ b/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: diff --git a/toolkit/components/places/tests/browser/head.js b/toolkit/components/places/tests/browser/head.js index c26cbc31de0c..538727015619 100644 --- a/toolkit/components/places/tests/browser/head.js +++ b/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); diff --git a/toolkit/components/places/tests/expiration/test_pref_maxpages.js b/toolkit/components/places/tests/expiration/test_pref_maxpages.js index 17799ab77120..e4583359f19c 100644 --- a/toolkit/components/places/tests/expiration/test_pref_maxpages.js +++ b/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( diff --git a/toolkit/components/places/tests/head_common.js b/toolkit/components/places/tests/head_common.js index e4aaa259d8cf..fdc983fe2a62 100644 --- a/toolkit/components/places/tests/head_common.js +++ b/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 diff --git a/toolkit/components/places/tests/history/test_remove.js b/toolkit/components/places/tests/history/test_remove.js index a363d556e9c5..8f12e3fe1955 100644 --- a/toolkit/components/places/tests/history/test_remove.js +++ b/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", diff --git a/toolkit/components/places/tests/history/test_removeByFilter.js b/toolkit/components/places/tests/history/test_removeByFilter.js index 8a04289002bc..ea2b8dfcca3b 100644 --- a/toolkit/components/places/tests/history/test_removeByFilter.js +++ b/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 }; } diff --git a/toolkit/components/places/tests/history/test_removeMany.js b/toolkit/components/places/tests/history/test_removeMany.js index 02c67dfab635..3cc20c4aa0ba 100644 --- a/toolkit/components/places/tests/history/test_removeMany.js +++ b/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", diff --git a/toolkit/components/places/tests/history/test_removeVisitsByFilter.js b/toolkit/components/places/tests/history/test_removeVisitsByFilter.js index 157162ac734b..eb2edadce068 100644 --- a/toolkit/components/places/tests/history/test_removeVisitsByFilter.js +++ b/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 diff --git a/toolkit/components/places/tests/unit/test_history_observer.js b/toolkit/components/places/tests/unit/test_history_observer.js index 3f1c012949e3..ab47b161e4f9 100644 --- a/toolkit/components/places/tests/unit/test_history_observer.js +++ b/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. diff --git a/toolkit/components/thumbnails/PageThumbs.jsm b/toolkit/components/thumbnails/PageThumbs.jsm index bf039801a97a..6b760449547c 100644 --- a/toolkit/components/thumbnails/PageThumbs.jsm +++ b/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", - ]), -}; diff --git a/toolkit/modules/NewTabUtils.jsm b/toolkit/modules/NewTabUtils.jsm index 71a1c357a7d9..280b4d7198d6 100644 --- a/toolkit/modules/NewTabUtils.jsm +++ b/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", - ]), }; /**