Bug 1341097 - part 3: don't dispatch oodles of titlechanged notifications for new history entries, r?mak draft
authorGijs Kruitbosch <gijskruitbosch@gmail.com>
Mon, 20 Feb 2017 18:47:56 +0000
changeset 487101 c9ee653ffe5e6bf20527349aa0393ccecece4b43
parent 487100 cc857c6d96d8827de88ad0d1f1f92274779b90d0
child 487102 91b8c0112353799ce59add072cddbd35fc1a07b3
push id46133
push userbmo:gijskruitbosch+bugs@gmail.com
push dateMon, 20 Feb 2017 18:49:36 +0000
reviewersmak
bugs1341097
milestone54.0a1
Bug 1341097 - part 3: don't dispatch oodles of titlechanged notifications for new history entries, r?mak MozReview-Commit-ID: 7jHOcCQ5ZBb
toolkit/components/downloads/nsDownloadManager.cpp
toolkit/components/places/History.cpp
toolkit/components/places/nsINavHistoryService.idl
toolkit/components/places/nsNavBookmarks.cpp
toolkit/components/places/nsNavHistory.cpp
toolkit/components/places/nsNavHistory.h
toolkit/components/places/nsNavHistoryResult.cpp
toolkit/components/places/nsNavHistoryResult.h
toolkit/components/places/tests/history/test_updatePlaces_sameUri_titleChanged.js
toolkit/components/places/tests/unit/test_async_history_api.js
--- a/toolkit/components/downloads/nsDownloadManager.cpp
+++ b/toolkit/components/downloads/nsDownloadManager.cpp
@@ -2327,17 +2327,18 @@ nsDownloadManager::OnEndUpdateBatch()
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsDownloadManager::OnVisit(nsIURI *aURI, int64_t aVisitID, PRTime aTime,
                            int64_t aSessionID, int64_t aReferringID,
                            uint32_t aTransitionType, const nsACString& aGUID,
-                           bool aHidden, uint32_t aVisitCount, uint32_t aTyped)
+                           bool aHidden, uint32_t aVisitCount, uint32_t aTyped,
+                           const nsAString& aLastKnowntTitle)
 {
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsDownloadManager::OnTitleChanged(nsIURI *aURI,
                                   const nsAString &aPageTitle,
                                   const nsACString &aGUID)
--- a/toolkit/components/places/History.cpp
+++ b/toolkit/components/places/History.cpp
@@ -653,17 +653,18 @@ public:
 
     // Notify the visit.  Note that TRANSITION_EMBED visits are never added
     // to the database, thus cannot be queried and we don't notify them.
     if (mPlace.transitionType != nsINavHistoryService::TRANSITION_EMBED) {
       navHistory->NotifyOnVisit(uri, mPlace.visitId, mPlace.visitTime,
                                 mPlace.referrerVisitId, mPlace.transitionType,
                                 mPlace.guid, mPlace.hidden,
                                 mPlace.visitCount + 1, // Add current visit.
-                                static_cast<uint32_t>(mPlace.typed));
+                                static_cast<uint32_t>(mPlace.typed),
+                                mPlace.title);
     }
 
     nsCOMPtr<nsIObserverService> obsService =
       mozilla::services::GetObserverService();
     if (obsService) {
       DebugOnly<nsresult> rv =
         obsService->NotifyObservers(uri, URI_VISIT_SAVED, nullptr);
       NS_WARNING_ASSERTION(NS_SUCCEEDED(rv), "Could not notify observers");
@@ -1043,17 +1044,17 @@ public:
       // visit/place, so update the count:
       (*successfulUpdatedCount)++;
 
       nsCOMPtr<nsIRunnable> event = new NotifyVisitObservers(place);
       rv = NS_DispatchToMainThread(event);
       NS_ENSURE_SUCCESS(rv, rv);
 
       // Notify about title change if needed.
-      if ((!known && !place.title.IsVoid()) || place.titleChanged) {
+      if (place.titleChanged) {
         event = new NotifyTitleObservers(place.spec, place.title, place.guid);
         rv = NS_DispatchToMainThread(event);
         NS_ENSURE_SUCCESS(rv, rv);
       }
     }
 
     nsresult rv = transaction.Commit();
     NS_ENSURE_SUCCESS(rv, rv);
--- a/toolkit/components/places/nsINavHistoryService.idl
+++ b/toolkit/components/places/nsINavHistoryService.idl
@@ -666,27 +666,30 @@ interface nsINavHistoryObserver : nsISup
    * @param aHidden
    *        Whether the visited page is marked as hidden.
    * @param aVisitCount
    *        Number of visits (included this one) for this URI.
    * @param aTyped
    *        Whether the URI has been typed or not.
    *        TODO (Bug 1271801): This will become a count, rather than a boolean.
    *        For future compatibility, always compare it with "> 0".
+   * @param aLastKnownTitle
+   *        The last known title of the page. Might not be from the current visit.
    */
   void onVisit(in nsIURI aURI,
                in long long aVisitId,
                in PRTime aTime,
                in long long aSessionId,
                in long long aReferrerVisitId,
                in unsigned long aTransitionType,
                in ACString aGuid,
                in boolean aHidden,
                in unsigned long aVisitCount,
-               in unsigned long aTyped);
+               in unsigned long aTyped,
+               in AString aLastKnownTitle);
 
   /**
    * Called whenever either the "real" title or the custom title of the page
    * changed. BOTH TITLES ARE ALWAYS INCLUDED in this notification, even though
    * only one will change at a time. Often, consumers will want to display the
    * user title if it is available, and fall back to the page title (the one
    * specified in the <title> tag of the page).
    *
--- a/toolkit/components/places/nsNavBookmarks.cpp
+++ b/toolkit/components/places/nsNavBookmarks.cpp
@@ -3271,17 +3271,18 @@ nsNavBookmarks::OnEndUpdateBatch()
   return NS_OK;
 }
 
 
 NS_IMETHODIMP
 nsNavBookmarks::OnVisit(nsIURI* aURI, int64_t aVisitId, PRTime aTime,
                         int64_t aSessionID, int64_t aReferringID,
                         uint32_t aTransitionType, const nsACString& aGUID,
-                        bool aHidden, uint32_t aVisitCount, uint32_t aTyped)
+                        bool aHidden, uint32_t aVisitCount, uint32_t aTyped,
+                        const nsAString& aLastKnownTitle)
 {
   NS_ENSURE_ARG(aURI);
 
   // If the page is bookmarked, notify observers for each associated bookmark.
   ItemVisitData visitData;
   nsresult rv = aURI->GetSpec(visitData.bookmark.url);
   NS_ENSURE_SUCCESS(rv, rv);
   visitData.visitId = aVisitId;
--- a/toolkit/components/places/nsNavHistory.cpp
+++ b/toolkit/components/places/nsNavHistory.cpp
@@ -490,40 +490,41 @@ nsNavHistory::LoadPrefs()
   FRECENCY_PREF(mDefaultWeight,            PREF_FREC_DEFAULT_BUCKET_WEIGHT);
 
 #undef FRECENCY_PREF
 }
 
 
 void
 nsNavHistory::NotifyOnVisit(nsIURI* aURI,
-                          int64_t aVisitId,
-                          PRTime aTime,
-                          int64_t aReferrerVisitId,
-                          int32_t aTransitionType,
-                          const nsACString& aGuid,
-                          bool aHidden,
-                          uint32_t aVisitCount,
-                          uint32_t aTyped)
+                            int64_t aVisitId,
+                            PRTime aTime,
+                            int64_t aReferrerVisitId,
+                            int32_t aTransitionType,
+                            const nsACString& aGuid,
+                            bool aHidden,
+                            uint32_t aVisitCount,
+                            uint32_t aTyped,
+                            const nsAString& aLastKnownTitle)
 {
   MOZ_ASSERT(!aGuid.IsEmpty());
   // If there's no history, this visit will surely add a day.  If the visit is
   // added before or after the last cached day, the day count may have changed.
   // Otherwise adding multiple visits in the same day should not invalidate
   // the cache.
   if (mDaysOfHistory == 0) {
     mDaysOfHistory = 1;
   } else if (aTime > mLastCachedEndOfDay || aTime < mLastCachedStartOfDay) {
     mDaysOfHistory = -1;
   }
 
   NOTIFY_OBSERVERS(mCanNotify, mCacheObservers, mObservers,
                    nsINavHistoryObserver,
                    OnVisit(aURI, aVisitId, aTime, 0, aReferrerVisitId,
-                           aTransitionType, aGuid, aHidden, aVisitCount, aTyped));
+                           aTransitionType, aGuid, aHidden, aVisitCount, aTyped, aLastKnownTitle));
 }
 
 void
 nsNavHistory::NotifyTitleChange(nsIURI* aURI,
                                 const nsString& aTitle,
                                 const nsACString& aGUID)
 {
   MOZ_ASSERT(!aGUID.IsEmpty());
--- a/toolkit/components/places/nsNavHistory.h
+++ b/toolkit/components/places/nsNavHistory.h
@@ -438,17 +438,18 @@ public:
   void NotifyOnVisit(nsIURI* aURI,
                      int64_t aVisitId,
                      PRTime aTime,
                      int64_t aReferrerVisitId,
                      int32_t aTransitionType,
                      const nsACString& aGuid,
                      bool aHidden,
                      uint32_t aVisitCount,
-                     uint32_t aTyped);
+                     uint32_t aTyped,
+                     const nsAString& aLastKnownTitle);
 
   /**
    * Fires onTitleChanged event to nsINavHistoryService observers
    */
   void NotifyTitleChange(nsIURI* aURI,
                          const nsString& title,
                          const nsACString& aGUID);
 
--- a/toolkit/components/places/nsNavHistoryResult.cpp
+++ b/toolkit/components/places/nsNavHistoryResult.cpp
@@ -4650,31 +4650,42 @@ nsNavHistoryResult::OnItemMoved(int64_t 
   return NS_OK;
 }
 
 
 NS_IMETHODIMP
 nsNavHistoryResult::OnVisit(nsIURI* aURI, int64_t aVisitId, PRTime aTime,
                             int64_t aSessionId, int64_t aReferringId,
                             uint32_t aTransitionType, const nsACString& aGUID,
-                            bool aHidden, uint32_t aVisitCount, uint32_t aTyped)
+                            bool aHidden, uint32_t aVisitCount, uint32_t aTyped,
+                            const nsAString& aLastKnownTitle)
 {
   NS_ENSURE_ARG(aURI);
 
   // Embed visits are never shown in our views.
   if (aTransitionType == nsINavHistoryService::TRANSITION_EMBED) {
     return NS_OK;
   }
 
   uint32_t added = 0;
 
   ENUMERATE_HISTORY_OBSERVERS(OnVisit(aURI, aVisitId, aTime, aSessionId,
                                       aReferringId, aTransitionType, aGUID,
                                       aHidden, &added));
 
+  // When we add visits through UpdatePlaces, we don't bother telling
+  // the world that the title 'changed' from nothing to the first title
+  // we ever see for a history entry. Our consumers here might still
+  // care, though, so we have to tell them - but only for the first
+  // visit we add. For subsequent changes, updateplaces will dispatch
+  // ontitlechanged notifications as normal.
+  if (!aLastKnownTitle.IsVoid() && aVisitCount <= 1) {
+    ENUMERATE_HISTORY_OBSERVERS(OnTitleChanged(aURI, aLastKnownTitle, aGUID));
+  }
+
   if (!mRootNode->mExpanded)
     return NS_OK;
 
   // If this visit is accepted by an overlapped container, and not all
   // overlapped containers are visible, we should still call Refresh if the
   // visit falls into any of them.
   bool todayIsMissing = false;
   uint32_t resultType = mRootNode->mOptions->ResultType();
--- a/toolkit/components/places/nsNavHistoryResult.h
+++ b/toolkit/components/places/nsNavHistoryResult.h
@@ -89,17 +89,19 @@ private:
                      bool aHidden, uint32_t* aAdded);
 
 // The external version is used by results.
 #define NS_DECL_BOOKMARK_HISTORY_OBSERVER_EXTERNAL(...)                 \
   NS_DECL_BOOKMARK_HISTORY_OBSERVER_BASE(__VA_ARGS__)                   \
   NS_IMETHOD OnVisit(nsIURI* aURI, int64_t aVisitId, PRTime aTime,      \
                      int64_t aSessionId, int64_t aReferringId,          \
                      uint32_t aTransitionType, const nsACString& aGUID, \
-                     bool aHidden, uint32_t aVisitCount, uint32_t aTyped) __VA_ARGS__;
+                     bool aHidden, uint32_t aVisitCount,                \
+                     uint32_t aTyped, const nsAString& aLastKnownTitle) \
+                     __VA_ARGS__;
 
 // nsNavHistoryResult
 //
 //    nsNavHistory creates this object and fills in mChildren (by getting
 //    it through GetTopLevel()). Then FilledAllResults() is called to finish
 //    object initialization.
 
 #define NS_NAVHISTORYRESULT_IID \
--- a/toolkit/components/places/tests/history/test_updatePlaces_sameUri_titleChanged.js
+++ b/toolkit/components/places/tests/history/test_updatePlaces_sameUri_titleChanged.js
@@ -5,17 +5,17 @@ add_task(function* test() {
   let uri = "http://test.com/";
 
   let promiseTitleChangedNotifications = new Promise(resolve => {
     let historyObserver = {
       _count: 0,
       __proto__: NavHistoryObserver.prototype,
       onTitleChanged(aURI, aTitle, aGUID) {
         Assert.equal(aURI.spec, uri, "Should notify the proper url");
-        if (++this._count == 2) {
+        if (++this._count == 1) {
           PlacesUtils.history.removeObserver(historyObserver);
           resolve();
         }
       }
     };
     PlacesUtils.history.addObserver(historyObserver, false);
   });
 
--- a/toolkit/components/places/tests/unit/test_async_history_api.js
+++ b/toolkit/components/places/tests/unit/test_async_history_api.js
@@ -946,47 +946,55 @@ add_task(function* test_title_change_not
     });
 
   PlacesUtils.history.addObserver(silentObserver, false);
   let placesResult = yield promiseUpdatePlaces(place);
   if (placesResult.errors.length > 0) {
     do_throw("Unexpected error.");
   }
 
-  // The second case to test is that we get the notification when we add
+  // The second case to test is that we don't get the notification when we add
   // it for the first time.  The first case will fail before our callback if it
   // is busted, so we can do this now.
   place.uri = NetUtil.newURI(place.uri.spec + "/new-visit-with-title");
   place.title = "title 1";
-  function promiseTitleChangedObserver(aPlace) {
-    return new Promise((resolve, reject) => {
-      let callbackCount = 0;
-      let observer = new TitleChangedObserver(aPlace.uri, aPlace.title, function() {
-        switch (++callbackCount) {
-          case 1:
-            // The third case to test is to make sure we get a notification when
-            // we change an existing place.
-            observer.expectedTitle = place.title = "title 2";
-            place.visits = [new VisitInfo()];
-            PlacesUtils.asyncHistory.updatePlaces(place);
-            break;
-          case 2:
-            PlacesUtils.history.removeObserver(silentObserver);
-            PlacesUtils.history.removeObserver(observer);
-            resolve();
-            break;
-        }
-      });
+  let expectedNotification = false;
+  let titleChangeObserver;
+  let titleChangePromise = new Promise((resolve, reject) => {
+    titleChangeObserver = new TitleChangedObserver(place.uri, place.title, function() {
+      Assert.ok(expectedNotification, "Should not get notified for " + place.uri.spec + " with title " + place.title);
+      if (expectedNotification) {
+        PlacesUtils.history.removeObserver(silentObserver);
+        PlacesUtils.history.removeObserver(titleChangeObserver);
+        resolve();
+      }
+    });
+    PlacesUtils.history.addObserver(titleChangeObserver, false);
+  });
 
-      PlacesUtils.history.addObserver(observer, false);
-      PlacesUtils.asyncHistory.updatePlaces(aPlace);
-    });
-  }
+  let visitPromise = new Promise(resolve => {
+    PlacesUtils.history.addObserver({
+      onVisit(uri) {
+        Assert.equal(uri.spec, place.uri.spec, "Should get notified for visiting the new URI.");
+        PlacesUtils.history.removeObserver(this);
+        resolve();
+      }
+    }, false);
+  });
+  PlacesUtils.asyncHistory.updatePlaces(place);
+  yield visitPromise;
 
-  yield promiseTitleChangedObserver(place);
+  // The third case to test is to make sure we get a notification when
+  // we change an existing place.
+  expectedNotification = true;
+  titleChangeObserver.expectedTitle = place.title = "title 2";
+  place.visits = [new VisitInfo()];
+  PlacesUtils.asyncHistory.updatePlaces(place);
+
+  yield titleChangePromise;
   yield PlacesTestUtils.promiseAsyncUpdates();
 });
 
 add_task(function* test_visit_notifies() {
   // There are two observers we need to see for each visit.  One is an
   // nsINavHistoryObserver and the other is the uri-visit-saved observer topic.
   let place = {
     guid: "abcdefghijkl",