Bug 1677408 - Part 6 - Remove onItemVisited and the history observer in the bookmarks service. r=Standard8

The result can directly listen for page-visited and recursively update children.
With onItemVisited gone, all the reasons for the bookmarks service to listen for
history notifications are gone, thus it can stop observing history.

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

toolkit/components/places/History.jsm

@@ -73,16 +73,12 @@ var EXPORTED_SYMBOLS = ["History"];
 const { XPCOMUtils } = ChromeUtils.import(
   "resource://gre/modules/XPCOMUtils.jsm"
 );
-ChromeUtils.defineModuleGetter(
-  this,
-  "NetUtil",
-  "resource://gre/modules/NetUtil.jsm"
-);
-ChromeUtils.defineModuleGetter(
-  this,
-  "PlacesUtils",
-  "resource://gre/modules/PlacesUtils.jsm"
-);
+
+XPCOMUtils.defineLazyModuleGetters(this, {
+  AppConstants: "resource://gre/modules/AppConstants.jsm",
+  PlacesUtils: "resource://gre/modules/PlacesUtils.jsm",
+  Services: "resource://gre/modules/Services.jsm",
+});
 
 XPCOMUtils.defineLazyServiceGetter(
   this,
@@ -119,7 +115,11 @@ function notify(observers, notification, args = []) {
   for (let observer of observers) {
     try {
       observer[notification](...args);
-    } catch (ex) {}
+    } catch (ex) {
+      if (AppConstants.DEBUG || Cu.isInAutomation) {
+        Cu.reportError(ex);
+      }
+    }
   }
 }
 
@@ -1034,11 +1034,12 @@ function removeOrphanIcons(db) {
 var notifyCleanup = async function(db, pages, transition = -1) {
   let notifiedCount = 0;
   let observers = PlacesUtils.history.getObservers();
+  let bookmarkObservers = PlacesUtils.bookmarks.getObservers();
 
   let reason = Ci.nsINavHistoryObserver.REASON_DELETED;
 
   for (let page of pages) {
-    let uri = NetUtil.newURI(page.url.href);
+    let uri = Services.io.newURI(page.url.href);
     let guid = page.guid;
     if (page.hasVisits || page.hasForeign) {
       // We have removed all visits, but the page is still alive, e.g.
@@ -1050,6 +1051,34 @@ var notifyCleanup = async function(db, pages, transition = -1) {
         reason,
         transition,
       ]);
+      // Also asynchronously notify bookmarks for this uri if all the visits
+      // have been removed.
+      if (!page.hasVisits) {
+        PlacesUtils.bookmarks
+          .fetch({ url: page.url }, async bookmark => {
+            let itemId = await PlacesUtils.promiseItemId(bookmark.guid);
+            let parentId = await PlacesUtils.promiseItemId(bookmark.parentGuid);
+            notify(
+              bookmarkObservers,
+              "onItemChanged",
+              [
+                itemId,
+                "cleartime",
+                false,
+                "",
+                0,
+                PlacesUtils.bookmarks.TYPE_BOOKMARK,
+                parentId,
+                bookmark.guid,
+                bookmark.parentGuid,
+                "",
+                PlacesUtils.bookmarks.SOURCES.DEFAULT,
+              ],
+              { concurrent: true }
+            );
+          })
+          .catch(Cu.reportError);
+      }
     } else {
       // The page has been entirely removed.
       notify(observers, "onDeleteURI", [uri, guid, reason]);

toolkit/components/places/nsNavBookmarks.cpp

@@ -60,69 +60,6 @@ bool SkipTags(nsCOMPtr<nsINavBookmarkObserver> obs) {
   return skipTags;
 }
 
-template <typename Method, typename DataType>
-class AsyncGetBookmarksForURI : public AsyncStatementCallback {
- public:
-  AsyncGetBookmarksForURI(nsNavBookmarks* aBookmarksSvc, Method aCallback,
-                          const DataType& aData)
-      : mBookmarksSvc(aBookmarksSvc), mCallback(aCallback), mData(aData) {}
-
-  void Init() {
-    RefPtr<Database> DB = Database::GetDatabase();
-    if (DB) {
-      nsCOMPtr<mozIStorageAsyncStatement> stmt = DB->GetAsyncStatement(
-          "/* do not warn (bug 1175249) */ "
-          "SELECT b.id, b.guid, b.parent, b.lastModified, t.guid, t.parent "
-          "FROM moz_bookmarks b "
-          "JOIN moz_bookmarks t on t.id = b.parent "
-          "WHERE b.fk = (SELECT id FROM moz_places WHERE url_hash = "
-          "hash(:page_url) AND url = :page_url) "
-          "ORDER BY b.lastModified DESC, b.id DESC ");
-      if (stmt) {
-        (void)URIBinder::Bind(stmt, "page_url"_ns, mData.bookmark.url);
-        nsCOMPtr<mozIStoragePendingStatement> pendingStmt;
-        (void)stmt->ExecuteAsync(this, getter_AddRefs(pendingStmt));
-      }
-    }
-  }
-
-  NS_IMETHOD HandleResult(mozIStorageResultSet* aResultSet) override {
-    nsCOMPtr<mozIStorageRow> row;
-    while (NS_SUCCEEDED(aResultSet->GetNextRow(getter_AddRefs(row))) && row) {
-      // Skip tags, for the use-cases of this async getter they are useless.
-      int64_t grandParentId = -1, tagsFolderId = -1;
-      nsresult rv = row->GetInt64(5, &grandParentId);
-      NS_ENSURE_SUCCESS(rv, rv);
-      rv = mBookmarksSvc->GetTagsFolder(&tagsFolderId);
-      NS_ENSURE_SUCCESS(rv, rv);
-      if (grandParentId == tagsFolderId) {
-        continue;
-      }
-
-      mData.bookmark.grandParentId = grandParentId;
-      rv = row->GetInt64(0, &mData.bookmark.id);
-      NS_ENSURE_SUCCESS(rv, rv);
-      rv = row->GetUTF8String(1, mData.bookmark.guid);
-      NS_ENSURE_SUCCESS(rv, rv);
-      rv = row->GetInt64(2, &mData.bookmark.parentId);
-      NS_ENSURE_SUCCESS(rv, rv);
-      // lastModified (3) should not be set for the use-cases of this getter.
-      rv = row->GetUTF8String(4, mData.bookmark.parentGuid);
-      NS_ENSURE_SUCCESS(rv, rv);
-
-      if (mCallback) {
-        ((*mBookmarksSvc).*mCallback)(mData);
-      }
-    }
-    return NS_OK;
-  }
-
- private:
-  RefPtr<nsNavBookmarks> mBookmarksSvc;
-  Method mCallback;
-  DataType mData;
-};
-
 // Returns the sync change counter increment for a change source constant.
 inline int64_t DetermineSyncChangeDelta(uint16_t aSource) {
   return aSource == nsINavBookmarksService::SOURCE_SYNC ? 0 : 1;
@@ -159,8 +96,8 @@ nsNavBookmarks::~nsNavBookmarks() {
   if (gBookmarksService == this) gBookmarksService = nullptr;
 }
 
-NS_IMPL_ISUPPORTS(nsNavBookmarks, nsINavBookmarksService, nsINavHistoryObserver,
-                  nsIObserver, nsISupportsWeakReference)
+NS_IMPL_ISUPPORTS(nsNavBookmarks, nsINavBookmarksService, nsIObserver,
+                  nsISupportsWeakReference)
 
 Atomic<int64_t> nsNavBookmarks::sLastInsertedItemId(0);
 
@@ -189,13 +126,6 @@ nsresult nsNavBookmarks::Init() {
 
   mCanNotify = true;
 
-  // Allows us to notify on title changes. MUST BE LAST so it is impossible
-  // to fail after this call, or the history service will have a reference to
-  // us and we won't go away.
-  nsNavHistory* history = nsNavHistory::GetHistoryService();
-  NS_ENSURE_STATE(history);
-  history->AddObserver(this, true);
-
   // DO NOT PUT STUFF HERE that can fail. See observer comment above.
 
   return NS_OK;
@@ -1809,67 +1739,3 @@ nsNavBookmarks::Observe(nsISupports* aSubject, const char* aTopic,
 
   return NS_OK;
 }
-
-////////////////////////////////////////////////////////////////////////////////
-//// nsINavHistoryObserver
-
-NS_IMETHODIMP
-nsNavBookmarks::OnBeginUpdateBatch() { return NS_OK; }
-
-NS_IMETHODIMP
-nsNavBookmarks::OnEndUpdateBatch() { return NS_OK; }
-
-NS_IMETHODIMP
-nsNavBookmarks::OnDeleteURI(nsIURI* aURI, const nsACString& aGUID,
-                            uint16_t aReason) {
-  return NS_OK;
-}
-
-NS_IMETHODIMP
-nsNavBookmarks::OnClearHistory() {
-  // TODO(bryner): we should notify on visited-time change for all URIs
-  return NS_OK;
-}
-
-NS_IMETHODIMP
-nsNavBookmarks::OnTitleChanged(nsIURI* aURI, const nsAString& aPageTitle,
-                               const nsACString& aGUID) {
-  // NOOP. We don't consume page titles from moz_places anymore.
-  // Title-change notifications are sent from SetItemTitle.
-  return NS_OK;
-}
-
-NS_IMETHODIMP
-nsNavBookmarks::OnFrecencyChanged(nsIURI* aURI, int32_t aNewFrecency,
-                                  const nsACString& aGUID, bool aHidden,
-                                  PRTime aLastVisitDate) {
-  return NS_OK;
-}
-
-NS_IMETHODIMP
-nsNavBookmarks::OnManyFrecenciesChanged() { return NS_OK; }
-
-NS_IMETHODIMP
-nsNavBookmarks::OnDeleteVisits(nsIURI* aURI, bool aPartialRemoval,
-                               const nsACString& aGUID, uint16_t aReason,
-                               uint32_t aTransitionType) {
-  NS_ENSURE_ARG(aURI);
-
-  // Notify "cleartime" only if all visits to the page have been removed.
-  if (!aPartialRemoval) {
-    // If the page is bookmarked, notify observers for each associated bookmark.
-    ItemChangeData changeData;
-    nsresult rv = aURI->GetSpec(changeData.bookmark.url);
-    NS_ENSURE_SUCCESS(rv, rv);
-    changeData.property = "cleartime"_ns;
-    changeData.isAnnotation = false;
-    changeData.bookmark.lastModified = 0;
-    changeData.bookmark.type = TYPE_BOOKMARK;
-
-    RefPtr<AsyncGetBookmarksForURI<ItemChangeMethod, ItemChangeData>> notifier =
-        new AsyncGetBookmarksForURI<ItemChangeMethod, ItemChangeData>(
-            this, &nsNavBookmarks::NotifyItemChanged, changeData);
-    notifier->Init();
-  }
-  return NS_OK;
-}

toolkit/components/places/nsNavBookmarks.h

@@ -72,13 +72,11 @@ enum BookmarkDate { LAST_MODIFIED };
 }  // namespace mozilla
 
 class nsNavBookmarks final : public nsINavBookmarksService,
-                             public nsINavHistoryObserver,
                              public nsIObserver,
                              public nsSupportsWeakReference {
  public:
   NS_DECL_ISUPPORTS
   NS_DECL_NSINAVBOOKMARKSSERVICE
-  NS_DECL_NSINAVHISTORYOBSERVER
   NS_DECL_NSIOBSERVER
 
   nsNavBookmarks();

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

@@ -21,14 +21,8 @@ var testServices = [
   ],
   [
     "browser/nav-bookmarks-service;1",
-    ["nsINavBookmarksService", "nsINavHistoryObserver"],
-    [
-      "createFolder",
-      "getObservers",
-      "onFrecencyChanged",
-      "onTitleChanged",
-      "onDeleteURI",
-    ],
+    ["nsINavBookmarksService"],
+    ["createFolder", "getObservers"],
   ],
   ["browser/favicon-service;1", ["nsIFaviconService"], []],
   ["browser/tagging-service;1", ["nsITaggingService"], []],