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
--- 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();
+});