Bug 1341097 - part 2: allow turning off notifications for individual inserted results when calling updatePlaces, r?mak draft
authorGijs Kruitbosch <gijskruitbosch@gmail.com>
Tue, 21 Feb 2017 14:20:21 +0000
changeset 488746 25de1c9dc5860032ae21e61caaec8a84ab291c5f
parent 488745 ad5bbce40eaa802a6a88bc0c9a9c7026fd056598
child 488747 56ad3bab924582a19430e47c708e20eab878c184
child 490133 b4416a2024e280ceba38dbb4dcae8357c653a6a6
child 490136 c38ad4696dabc9f675dec9fd1b6f1a7ee0c569e1
push id46616
push usergijskruitbosch@gmail.com
push dateThu, 23 Feb 2017 17:26:43 +0000
reviewersmak
bugs1341097
milestone54.0a1
Bug 1341097 - part 2: allow turning off notifications for individual inserted results when calling updatePlaces, r?mak This sets a property on the callback object rather than passing an argument to handleCompletion, to avoid accidentally breaking consumers who don't expect arguments. The downside is that this required more changes to C++ consumers, but we control all of those so that seemed an acceptable trade-off. We should probably actually report errors for all the migrators, but I didn't want to add risk (what if in some edge-case there are lots?) so I didn't. I'll file a followup to update them to Cu.reportError() any errors. MozReview-Commit-ID: Hue9Ci3hyVz
browser/components/migration/ChromeProfileMigrator.js
browser/components/migration/EdgeProfileMigrator.js
browser/components/migration/IEProfileMigrator.js
browser/components/migration/SafariProfileMigrator.js
toolkit/components/places/History.cpp
toolkit/components/places/mozIAsyncHistory.idl
toolkit/components/places/tests/unit/test_async_history_api.js
--- a/browser/components/migration/ChromeProfileMigrator.js
+++ b/browser/components/migration/ChromeProfileMigrator.js
@@ -357,24 +357,20 @@ function GetHistoryResource(aProfileFold
           } catch (e) {
             Cu.reportError(e);
           }
         }
 
         if (places.length > 0) {
           yield new Promise((resolve, reject) => {
             MigrationUtils.insertVisitsWrapper(places, {
-              _success: false,
-              handleResult() {
-                // Importing any entry is considered a successful import.
-                this._success = true;
-              },
-              handleError() {},
-              handleCompletion() {
-                if (this._success) {
+              ignoreErrors: true,
+              ignoreResults: true,
+              handleCompletion(updatedCount) {
+                if (updatedCount > 0) {
                   resolve();
                 } else {
                   reject(new Error("Couldn't add visits"));
                 }
               }
             });
           });
         }
--- a/browser/components/migration/EdgeProfileMigrator.js
+++ b/browser/components/migration/EdgeProfileMigrator.js
@@ -134,24 +134,20 @@ EdgeTypedURLMigrator.prototype = {
     }
 
     if (places.length == 0) {
       aCallback(typedURLs.size == 0);
       return;
     }
 
     MigrationUtils.insertVisitsWrapper(places, {
-      _success: false,
-      handleResult() {
-        // Importing any entry is considered a successful import.
-        this._success = true;
-      },
-      handleError() {},
-      handleCompletion() {
-        aCallback(this._success);
+      ignoreErrors: true,
+      ignoreResults: true,
+      handleCompletion(updatedCount) {
+        aCallback(updatedCount > 0);
       }
     });
   },
 };
 
 function EdgeReadingListMigrator() {
 }
 
--- a/browser/components/migration/IEProfileMigrator.js
+++ b/browser/components/migration/IEProfileMigrator.js
@@ -86,24 +86,20 @@ History.prototype = {
 
     // Check whether there is any history to import.
     if (places.length == 0) {
       aCallback(true);
       return;
     }
 
     MigrationUtils.insertVisitsWrapper(places, {
-      _success: false,
-      handleResult() {
-        // Importing any entry is considered a successful import.
-        this._success = true;
-      },
-      handleError() {},
-      handleCompletion() {
-        aCallback(this._success);
+      ignoreErrors: true,
+      ignoreResults: true,
+      handleCompletion(updatedCount) {
+        aCallback(updatedCount > 0);
       }
     });
   }
 };
 
 // IE form password migrator supporting windows from XP until 7 and IE from 7 until 11
 function IE7FormPasswords() {
   // used to distinguish between this migrator and other passwords migrators in tests.
--- a/browser/components/migration/SafariProfileMigrator.js
+++ b/browser/components/migration/SafariProfileMigrator.js
@@ -222,24 +222,20 @@ History.prototype = {
               // Safari's History file may contain malformed URIs which
               // will be ignored.
               Cu.reportError(ex);
             }
           }
         }
         if (places.length > 0) {
           MigrationUtils.insertVisitsWrapper(places, {
-            _success: false,
-            handleResult() {
-              // Importing any entry is considered a successful import.
-              this._success = true;
-            },
-            handleError() {},
-            handleCompletion() {
-              aCallback(this._success);
+            ignoreErrors: true,
+            ignoreResults: true,
+            handleCompletion(updatedCount) {
+              aCallback(updatedCount > 0);
             }
           });
         } else {
           aCallback(false);
         }
       } catch (ex) {
         Cu.reportError(ex);
         aCallback(false);
--- a/toolkit/components/places/History.cpp
+++ b/toolkit/components/places/History.cpp
@@ -798,35 +798,38 @@ private:
 };
 
 /**
  * Notifies a callback object when the operation is complete.
  */
 class NotifyCompletion : public Runnable
 {
 public:
-  explicit NotifyCompletion(const nsMainThreadPtrHandle<mozIVisitInfoCallback>& aCallback)
+  explicit NotifyCompletion(const nsMainThreadPtrHandle<mozIVisitInfoCallback>& aCallback,
+                            uint32_t aUpdatedCount = 0)
   : mCallback(aCallback)
+  , mUpdatedCount(aUpdatedCount)
   {
     MOZ_ASSERT(aCallback, "Must pass a non-null callback!");
   }
 
   NS_IMETHOD Run() override
   {
     if (NS_IsMainThread()) {
-      (void)mCallback->HandleCompletion();
+      (void)mCallback->HandleCompletion(mUpdatedCount);
     }
     else {
       (void)NS_DispatchToMainThread(this);
     }
     return NS_OK;
   }
 
 private:
   nsMainThreadPtrHandle<mozIVisitInfoCallback> mCallback;
+  uint32_t mUpdatedCount;
 };
 
 /**
  * Checks to see if we can add aURI to history, and dispatches an error to
  * aCallback (if provided) if we cannot.
  *
  * @param aURI
  *        The URI to check.
@@ -912,32 +915,55 @@ public:
     nsNavHistory* navHistory = nsNavHistory::GetHistoryService();
     MOZ_ASSERT(navHistory, "Could not get nsNavHistory?!");
     if (!navHistory) {
       return NS_ERROR_FAILURE;
     }
 
     nsMainThreadPtrHandle<mozIVisitInfoCallback>
       callback(new nsMainThreadPtrHolder<mozIVisitInfoCallback>(aCallback));
+    bool ignoreErrors = false, ignoreResults = false;
+    if (aCallback) {
+      // We ignore errors from either of these methods in case old JS consumers
+      // don't implement them (in which case they will get error/result
+      // notifications as normal).
+      Unused << aCallback->GetIgnoreErrors(&ignoreErrors);
+      Unused << aCallback->GetIgnoreResults(&ignoreResults);
+    }
     RefPtr<InsertVisitedURIs> event =
-      new InsertVisitedURIs(aConnection, aPlaces, callback, aGroupNotifications);
+      new InsertVisitedURIs(aConnection, aPlaces, callback, aGroupNotifications,
+                            ignoreErrors, ignoreResults);
 
     // Get the target thread, and then start the work!
     nsCOMPtr<nsIEventTarget> target = do_GetInterface(aConnection);
     NS_ENSURE_TRUE(target, NS_ERROR_UNEXPECTED);
     nsresult rv = target->Dispatch(event, NS_DISPATCH_NORMAL);
     NS_ENSURE_SUCCESS(rv, rv);
 
     return NS_OK;
   }
 
   NS_IMETHOD Run() override
   {
     MOZ_ASSERT(!NS_IsMainThread(), "This should not be called on the main thread");
 
+    // The inner run method may bail out at any point, so we ensure we do
+    // whatever we can and then notify the main thread we're done.
+    nsresult rv = InnerRun();
+
+    if (mSuccessfulUpdatedCount > 0 && mGroupNotifications) {
+      NS_DispatchToMainThread(new NotifyManyFrecenciesChanged());
+    }
+    if (!!mCallback) {
+      NS_DispatchToMainThread(new NotifyCompletion(mCallback, mSuccessfulUpdatedCount));
+    }
+    return rv;
+  }
+
+  nsresult InnerRun() {
     // Prevent the main thread from shutting down while this is running.
     MutexAutoLock lockedScope(mHistory->GetShutdownMutex());
     if (mHistory->IsShuttingDown()) {
       // If we were already shutting down, we cannot insert the URIs.
       return NS_OK;
     }
 
     mozStorageTransaction transaction(mDBConn, false,
@@ -953,17 +979,17 @@ public:
       bool hidden = place.hidden;
 
       // We can avoid a database lookup if it's the same place as the last
       // visit we added.
       bool known = lastFetchedPlace && lastFetchedPlace->spec.Equals(place.spec);
       if (!known) {
         nsresult rv = mHistory->FetchPageInfo(place, &known);
         if (NS_FAILED(rv)) {
-          if (!!mCallback) {
+          if (!!mCallback && !mIgnoreErrors) {
             nsCOMPtr<nsIRunnable> event =
               new NotifyPlaceInfoCallback(mCallback, place, true, rv);
             return NS_DispatchToMainThread(event);
           }
           return NS_OK;
         }
         lastFetchedPlace = &mPlaces.ElementAt(i);
       } else {
@@ -993,55 +1019,62 @@ public:
       if (!known || !lastFetchedPlace->hidden) {
         place.shouldUpdateHidden = false;
       }
 
       FetchReferrerInfo(place);
 
       nsresult rv = DoDatabaseInserts(known, place);
       if (!!mCallback) {
-        nsCOMPtr<nsIRunnable> event =
-          new NotifyPlaceInfoCallback(mCallback, place, true, rv);
-        nsresult rv2 = NS_DispatchToMainThread(event);
-        NS_ENSURE_SUCCESS(rv2, rv2);
+        // Check if consumers wanted to be notified about success/failure,
+        // depending on whether this action succeeded or not.
+        if ((NS_SUCCEEDED(rv) && !mIgnoreResults) ||
+            (NS_FAILED(rv) && !mIgnoreErrors)) {
+          nsCOMPtr<nsIRunnable> event =
+            new NotifyPlaceInfoCallback(mCallback, place, true, rv);
+          nsresult rv2 = NS_DispatchToMainThread(event);
+          NS_ENSURE_SUCCESS(rv2, rv2);
+        }
       }
       NS_ENSURE_SUCCESS(rv, rv);
 
+      // If we get here, we must have been successful adding/updating this
+      // visit/place, so update the count:
+      mSuccessfulUpdatedCount++;
+
       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) {
         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);
 
-    if (mGroupNotifications) {
-      nsCOMPtr<nsIRunnable> event =
-        new NotifyManyFrecenciesChanged();
-      rv = NS_DispatchToMainThread(event);
-      NS_ENSURE_SUCCESS(rv, rv);
-    }
-
     return NS_OK;
   }
 private:
   InsertVisitedURIs(mozIStorageConnection* aConnection,
                     nsTArray<VisitData>& aPlaces,
                     const nsMainThreadPtrHandle<mozIVisitInfoCallback>& aCallback,
-                    bool aGroupNotifications)
+                    bool aGroupNotifications,
+                    bool aIgnoreErrors,
+                    bool aIgnoreResults)
   : mDBConn(aConnection)
   , mCallback(aCallback)
   , mGroupNotifications(aGroupNotifications)
+  , mIgnoreErrors(aIgnoreErrors)
+  , mIgnoreResults(aIgnoreResults)
+  , mSuccessfulUpdatedCount(0)
   , mHistory(History::GetService())
   {
     MOZ_ASSERT(NS_IsMainThread(), "This should be called on the main thread");
 
     mPlaces.SwapElements(aPlaces);
 
 #ifdef DEBUG
     for (nsTArray<VisitData>::size_type i = 0; i < mPlaces.Length(); i++) {
@@ -1251,16 +1284,22 @@ private:
   mozIStorageConnection* mDBConn;
 
   nsTArray<VisitData> mPlaces;
 
   nsMainThreadPtrHandle<mozIVisitInfoCallback> mCallback;
 
   bool mGroupNotifications;
 
+  bool mIgnoreErrors;
+
+  bool mIgnoreResults;
+
+  uint32_t mSuccessfulUpdatedCount;
+
   /**
    * Strong reference to the History object because we do not want it to
    * disappear out from under us.
    */
   RefPtr<History> mHistory;
 };
 
 class GetPlaceInfo final : public Runnable {
@@ -1440,16 +1479,28 @@ public:
   explicit SetDownloadAnnotations(nsIURI* aDestination)
   : mDestination(aDestination)
   , mHistory(History::GetService())
   {
     MOZ_ASSERT(mDestination);
     MOZ_ASSERT(NS_IsMainThread());
   }
 
+  NS_IMETHOD GetIgnoreResults(bool *aIgnoreResults) override
+  {
+    *aIgnoreResults = false;
+    return NS_OK;
+  }
+
+  NS_IMETHOD GetIgnoreErrors(bool *aIgnoreErrors) override
+  {
+    *aIgnoreErrors = false;
+    return NS_OK;
+  }
+
   NS_IMETHOD HandleError(nsresult aResultCode, mozIPlaceInfo *aPlaceInfo) override
   {
     // Just don't add the annotations in case the visit isn't added.
     return NS_OK;
   }
 
   NS_IMETHOD HandleResult(mozIPlaceInfo *aPlaceInfo) override
   {
@@ -1508,17 +1559,17 @@ public:
     if (title.IsEmpty()) {
       rv = mHistory->SetURITitle(source, destinationFileName);
       NS_ENSURE_SUCCESS(rv, rv);
     }
 
     return NS_OK;
   }
 
-  NS_IMETHOD HandleCompletion() override
+  NS_IMETHOD HandleCompletion(uint32_t aUpdatedCount) override
   {
     return NS_OK;
   }
 
 private:
   ~SetDownloadAnnotations() {}
 
   nsCOMPtr<nsIURI> mDestination;
@@ -2949,23 +3000,25 @@ History::UpdatePlaces(JS::Handle<JS::Val
 
   // It is possible that all of the visits we were passed were dissallowed by
   // CanAddURI, which isn't an error.  If we have no visits to add, however,
   // we should not call InsertVisitedURIs::Start.
   if (visitData.Length()) {
     nsresult rv = InsertVisitedURIs::Start(dbConn, visitData,
                                            callback, aGroupNotifications);
     NS_ENSURE_SUCCESS(rv, rv);
-  }
-
-  // Be sure to notify that all of our operations are complete.  This
-  // is dispatched to the background thread first and redirected to the
-  // main thread from there to make sure that all database notifications
-  // and all embed or canAddURI notifications have finished.
-  if (aCallback) {
+  } else if (aCallback) {
+    // Be sure to notify that all of our operations are complete.  This
+    // is dispatched to the background thread first and redirected to the
+    // main thread from there to make sure that all database notifications
+    // and all embed or canAddURI notifications have finished.
+
+    // Note: if we're inserting anything, it's the responsibility of
+    // InsertVisitedURIs to call the completion callback, as here we won't
+    // know how yet many items we will successfully insert/update.
     nsCOMPtr<nsIEventTarget> backgroundThread = do_GetInterface(dbConn);
     NS_ENSURE_TRUE(backgroundThread, NS_ERROR_UNEXPECTED);
     nsCOMPtr<nsIRunnable> event = new NotifyCompletion(callback);
     return backgroundThread->Dispatch(event, NS_DISPATCH_NORMAL);
   }
 
   return NS_OK;
 }
--- a/toolkit/components/places/mozIAsyncHistory.idl
+++ b/toolkit/components/places/mozIAsyncHistory.idl
@@ -92,19 +92,28 @@ interface mozIVisitInfoCallback : nsISup
    *
    * @param aPlaceInfo
    *        The current info stored for the place.
    */
   void handleResult(in mozIPlaceInfo aPlaceInfo);
 
   /**
    * Called when all records were processed.
+   * @param aUpdatedItems
+   *        How many items were successfully updated.
    */
-  void handleCompletion();
+  void handleCompletion(in unsigned long aUpdatedItems);
 
+  /**
+   * These two attributes govern whether we attempt to call
+   * handleResult and handleError, respectively, if/once
+   * results/errors occur.
+   */
+  readonly attribute bool ignoreResults;
+  readonly attribute bool ignoreErrors;
 };
 
 [scriptable, function, uuid(994092bf-936f-449b-8dd6-0941e024360d)]
 interface mozIVisitedStatusCallback : nsISupports
 {
   /**
    * Notifies whether a certain URI has been visited.
    *
--- a/toolkit/components/places/tests/unit/test_async_history_api.js
+++ b/toolkit/components/places/tests/unit/test_async_history_api.js
@@ -20,31 +20,31 @@ const RECENT_EVENT_THRESHOLD = 15 * 60 *
  */
 function VisitInfo(aTransitionType,
                    aVisitTime) {
   this.transitionType =
     aTransitionType === undefined ? TRANSITION_LINK : aTransitionType;
   this.visitDate = aVisitTime || Date.now() * 1000;
 }
 
-function promiseUpdatePlaces(aPlaces, aBatchFrecencyNotifications) {
+function promiseUpdatePlaces(aPlaces, aOptions, aBatchFrecencyNotifications) {
   return new Promise((resolve, reject) => {
-    PlacesUtils.asyncHistory.updatePlaces(aPlaces, {
+    PlacesUtils.asyncHistory.updatePlaces(aPlaces, Object.assign({
       _errors: [],
       _results: [],
       handleError(aResultCode, aPlace) {
         this._errors.push({ resultCode: aResultCode, info: aPlace});
       },
       handleResult(aPlace) {
         this._results.push(aPlace);
       },
-      handleCompletion() {
-        resolve({ errors: this._errors, results: this._results });
+      handleCompletion(resultCount) {
+        resolve({ errors: this._errors, results: this._results, resultCount});
       }
-    }, aBatchFrecencyNotifications);
+    }, aOptions), aBatchFrecencyNotifications);
   });
 }
 
 /**
  * Listens for a title change notification, and calls aCallback when it gets it.
  *
  * @param aURI
  *        The URI of the page we expect a notification for.
@@ -1120,11 +1120,118 @@ add_task(function* test_omit_frecency_no
       onManyFrecenciesChanged() {
         ok(true, "Should fire many frecencies changed notification instead.");
         PlacesUtils.history.removeObserver(frecencyObserverCheck);
         resolve();
       },
     };
     PlacesUtils.history.addObserver(frecencyObserverCheck, false);
   });
-  yield promiseUpdatePlaces(places, true);
+  yield promiseUpdatePlaces(places, {}, true);
   yield promiseFrecenciesChanged;
 });
+
+add_task(function* test_ignore_errors() {
+  yield PlacesTestUtils.clearHistory();
+  // This test ensures that trying to add a visit, with a guid already found in
+  // another visit, fails - but doesn't report if we told it not to.
+  let place = {
+    uri: NetUtil.newURI(TEST_DOMAIN + "test_duplicate_guid_fails_first"),
+    visits: [
+      new VisitInfo(),
+    ],
+  };
+
+  do_check_false(yield promiseIsURIVisited(place.uri));
+  let placesResult = yield promiseUpdatePlaces(place);
+  if (placesResult.errors.length > 0) {
+    do_throw("Unexpected error.");
+  }
+  let placeInfo = placesResult.results[0];
+  do_check_true(yield promiseIsURIVisited(placeInfo.uri));
+
+  let badPlace = {
+    uri: NetUtil.newURI(TEST_DOMAIN + "test_duplicate_guid_fails_second"),
+    visits: [
+      new VisitInfo(),
+    ],
+    guid: placeInfo.guid,
+  };
+
+  do_check_false(yield promiseIsURIVisited(badPlace.uri));
+  placesResult = yield promiseUpdatePlaces(badPlace, {ignoreErrors: true});
+  if (placesResult.results.length > 0) {
+    do_throw("Unexpected success.");
+  }
+  Assert.equal(placesResult.errors.length, 0,
+               "Should have seen 0 errors because we disabled reporting.");
+  Assert.equal(placesResult.results.length, 0,
+               "Should have seen 0 results because there were none.");
+  Assert.equal(placesResult.resultCount, 0,
+               "Should know that we updated 0 items from the completion callback.");
+  yield PlacesTestUtils.promiseAsyncUpdates();
+});
+
+add_task(function* test_ignore_results() {
+  yield PlacesTestUtils.clearHistory();
+  let place = {
+    uri: NetUtil.newURI("http://mozilla.org/"),
+    title: "test",
+    visits: [
+      new VisitInfo()
+    ]
+  };
+  let placesResult = yield promiseUpdatePlaces(place, {ignoreResults: true});
+  Assert.equal(placesResult.results.length, 0,
+               "Should have seen 0 results because we disabled reporting.");
+  Assert.equal(placesResult.errors.length, 0,
+               "Should have seen 0 errors because there were none.");
+  Assert.equal(placesResult.resultCount, 1,
+               "Should know that we updated 1 item from the completion callback.");
+  yield PlacesTestUtils.promiseAsyncUpdates();
+});
+
+add_task(function* test_ignore_results_and_errors() {
+  yield PlacesTestUtils.clearHistory();
+  // This test ensures that trying to add a visit, with a guid already found in
+  // another visit, fails - but doesn't report if we told it not to.
+  let place = {
+    uri: NetUtil.newURI(TEST_DOMAIN + "test_duplicate_guid_fails_first"),
+    visits: [
+      new VisitInfo(),
+    ],
+  };
+
+  do_check_false(yield promiseIsURIVisited(place.uri));
+  let placesResult = yield promiseUpdatePlaces(place);
+  if (placesResult.errors.length > 0) {
+    do_throw("Unexpected error.");
+  }
+  let placeInfo = placesResult.results[0];
+  do_check_true(yield promiseIsURIVisited(placeInfo.uri));
+
+  let badPlace = {
+    uri: NetUtil.newURI(TEST_DOMAIN + "test_duplicate_guid_fails_second"),
+    visits: [
+      new VisitInfo(),
+    ],
+    guid: placeInfo.guid,
+  };
+  let allPlaces = [
+    {
+      uri: NetUtil.newURI(TEST_DOMAIN + "test_other_successful_item"),
+      visits: [
+        new VisitInfo(),
+      ],
+    },
+    badPlace,
+  ];
+
+  do_check_false(yield promiseIsURIVisited(badPlace.uri));
+  placesResult = yield promiseUpdatePlaces(allPlaces, {ignoreErrors: true, ignoreResults: true});
+  Assert.equal(placesResult.errors.length, 0,
+               "Should have seen 0 errors because we disabled reporting.");
+  Assert.equal(placesResult.results.length, 0,
+               "Should have seen 0 results because we disabled reporting.");
+  Assert.equal(placesResult.resultCount, 1,
+               "Should know that we updated 1 item from the completion callback.");
+  yield PlacesTestUtils.promiseAsyncUpdates();
+});