diff --git a/toolkit/components/places/src/History.cpp b/toolkit/components/places/src/History.cpp index 1eb5ed4963d..284abf967df 100644 --- a/toolkit/components/places/src/History.cpp +++ b/toolkit/components/places/src/History.cpp @@ -88,6 +88,7 @@ struct VisitData { , typed(false) , transitionType(PR_UINT32_MAX) , visitTime(0) + , titleChanged(false) { guid.SetIsVoid(PR_TRUE); title.SetIsVoid(PR_TRUE); @@ -102,6 +103,7 @@ struct VisitData { , typed(false) , transitionType(PR_UINT32_MAX) , visitTime(0) + , titleChanged(false) { (void)aURI->GetSpec(spec); (void)GetReversedHostname(aURI, revHost); @@ -162,6 +164,9 @@ struct VisitData { PRTime visitTime; nsString title; nsCString referrerSpec; + + // TODO bug 626836 hook up hidden and typed change tracking too! + bool titleChanged; }; //////////////////////////////////////////////////////////////////////////////// @@ -222,10 +227,16 @@ GetStringFromJSObject(JSContext* aCtx, { jsval val; JSBool rc = JS_GetProperty(aCtx, aObject, aProperty, &val); - if (!rc || JSVAL_IS_VOID(val) || !JSVAL_IS_STRING(val)) { + if (!rc || JSVAL_IS_VOID(val) || + !(JSVAL_IS_NULL(val) || JSVAL_IS_STRING(val))) { _string.SetIsVoid(PR_TRUE); return; } + // |null| in JS maps to the empty string. + if (JSVAL_IS_NULL(val)) { + _string.Truncate(); + return; + } size_t length; const jschar* chars = JS_GetStringCharsZAndLength(aCtx, JSVAL_TO_STRING(val), &length); @@ -465,6 +476,45 @@ private: VisitData mReferrer; }; +/** + * Notifies observers about a pages title changing. + */ +class NotifyTitleObservers : public nsRunnable +{ +public: + /** + * Notifies observers on the main thread. + * + * @param aSpec + * The spec of the URI to notify about. + * @param aTitle + * The new title to notify about. + */ + NotifyTitleObservers(const nsCString& aSpec, + const nsString& aTitle) + : mSpec(aSpec) + , mTitle(aTitle) + { + } + + NS_IMETHOD Run() + { + NS_PRECONDITION(NS_IsMainThread(), + "This should be called on the main thread"); + + nsNavHistory* navHistory = nsNavHistory::GetHistoryService(); + NS_ENSURE_TRUE(navHistory, NS_ERROR_OUT_OF_MEMORY); + nsCOMPtr uri; + (void)NS_NewURI(getter_AddRefs(uri), mSpec); + navHistory->NotifyTitleChange(uri, mTitle); + + return NS_OK; + } +private: + const nsCString mSpec; + const nsString mTitle; +}; + /** * Notifies a callback object about completion. */ @@ -641,6 +691,13 @@ public: 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); + rv = NS_DispatchToMainThread(event); + NS_ENSURE_SUCCESS(rv, rv); + } + lastPlace = &mPlaces.ElementAt(i); } @@ -1006,48 +1063,6 @@ private: nsRefPtr mHistory; }; -/** - * Notifies observers about a pages title changing. - */ -class NotifyTitleObservers : public nsRunnable -{ -public: - /** - * Notifies observers on the main thread. - * - * @param aSpec - * The spec of the URI to notify about. - * @param aTitle - * The new title to notify about. - */ - NotifyTitleObservers(const nsCString& aSpec, - const nsString& aTitle) - : mSpec(aSpec) - , mTitle(aTitle) - { - NS_PRECONDITION(!NS_IsMainThread(), - "This should not be called on the main thread"); - } - - NS_IMETHOD Run() - { - NS_PRECONDITION(NS_IsMainThread(), - "This should be called on the main thread"); - - nsNavHistory* navHistory = nsNavHistory::GetHistoryService(); - NS_ENSURE_TRUE(navHistory, NS_ERROR_OUT_OF_MEMORY); - nsCOMPtr uri; - (void)NS_NewURI(getter_AddRefs(uri), mSpec); - navHistory->NotifyTitleChange(uri, mTitle); - - return NS_OK; - } -private: - const nsCString mSpec; - const nsString mTitle; -}; - - /** * Sets the page title for a page in moz_places (if necessary). */ @@ -1094,22 +1109,15 @@ public: // First, see if the page exists in the database (we'll need its id later). bool exists = mHistory->FetchPageInfo(mPlace); - if (!exists) { - // We have no record of this page, so there is no need to do any further - // work. + if (!exists || !mPlace.titleChanged) { + // We have no record of this page, or we have no title change, so there + // is no need to do any further work. return NS_OK; } NS_ASSERTION(mPlace.placeId > 0, "We somehow have an invalid place id here!"); - // Also, if we have the same title, there is no reason to do another write - // or notify our observers, so bail early. - if (mTitle.Equals(mPlace.title) || - (mTitle.IsVoid() && mPlace.title.IsVoid())) { - return NS_OK; - } - // Now we can update our database record. nsCOMPtr stmt = mHistory->syncStatements.GetCachedStatement( @@ -1328,7 +1336,8 @@ History::InsertPlace(const VisitData& aPlace) NS_ENSURE_SUCCESS(rv, rv); rv = URIBinder::Bind(stmt, NS_LITERAL_CSTRING("url"), aPlace.spec); NS_ENSURE_SUCCESS(rv, rv); - if (aPlace.title.IsVoid()) { + // Empty strings should have no title, just like nsNavHistory::SetPageTitle. + if (aPlace.title.IsEmpty()) { rv = stmt->BindNullByName(NS_LITERAL_CSTRING("title")); } else { @@ -1372,11 +1381,15 @@ History::UpdatePlace(const VisitData& aPlace) mozStorageStatementScoper scoper(stmt); nsresult rv; - if (!aPlace.title.IsVoid()) { + // Empty strings should clear the title, just like nsNavHistory::SetPageTitle. + if (aPlace.title.IsEmpty()) { + rv = stmt->BindNullByName(NS_LITERAL_CSTRING("title")); + } + else { rv = stmt->BindStringByName(NS_LITERAL_CSTRING("title"), StringHead(aPlace.title, TITLE_LENGTH_MAX)); - NS_ENSURE_SUCCESS(rv, rv); } + NS_ENSURE_SUCCESS(rv, rv); rv = stmt->BindInt32ByName(NS_LITERAL_CSTRING("typed"), aPlace.typed); NS_ENSURE_SUCCESS(rv, rv); rv = stmt->BindInt32ByName(NS_LITERAL_CSTRING("hidden"), aPlace.hidden); @@ -1420,9 +1433,16 @@ History::FetchPageInfo(VisitData& _place) rv = stmt->GetInt64(0, &_place.placeId); NS_ENSURE_SUCCESS(rv, false); + nsAutoString title; + rv = stmt->GetString(1, title); + NS_ENSURE_SUCCESS(rv, true); + + // We track if we change the title, but will add the current title to _place + // if we do not have one set. + _place.titleChanged = !(_place.title.Equals(title) || + (_place.title.IsVoid() && title.IsVoid())); if (_place.title.IsVoid()) { - rv = stmt->GetString(1, _place.title); - NS_ENSURE_SUCCESS(rv, true); + _place.title = title; } if (_place.hidden) { diff --git a/toolkit/components/places/tests/unit/test_async_history_api.js b/toolkit/components/places/tests/unit/test_async_history_api.js index a2a421fd320..d138295e7f9 100644 --- a/toolkit/components/places/tests/unit/test_async_history_api.js +++ b/toolkit/components/places/tests/unit/test_async_history_api.js @@ -38,6 +38,62 @@ function VisitInfo(aTransitionType, this.visitDate = aVisitTime || Date.now() * 1000; } +/** + * 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: function() { }, + onEndUpdateBatch: function() { }, + onVisit: function() { }, + onTitleChanged: function() { }, + onBeforeDeleteURI: function() { }, + onDeleteURI: function() { }, + onClearHistory: function() { }, + onPageChanged: function() { }, + onDeleteVisits: function() { }, + QueryInterface: XPCOMUtils.generateQI([ + Ci.nsINavHistoryObserver, + ]), +}; + +/** + * 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. + * @param aExpectedTitle + * The expected title of the URI we expect a notification for. + * @param aCallback + * The method to call when we have gotten the proper notification about + * the title changing. + */ +function TitleChangedObserver(aURI, + aExpectedTitle, + aCallback) +{ + this.uri = aURI; + this.expectedTitle = aExpectedTitle; + this.callback = aCallback; +} +TitleChangedObserver.prototype = { + __proto__: NavHistoryObserver.prototype, + onTitleChanged: function(aURI, + aTitle) + { + do_log_info("onTitleChanged(" + aURI.spec + ", " + aTitle + ")"); + if (!this.uri.equals(aURI)) { + return; + } + do_check_eq(aTitle, this.expectedTitle); + this.callback(); + }, +}; + /** * Tests that a title was set properly in the database. * @@ -654,6 +710,7 @@ function test_title_change_saved() // First, add a visit for it. let place = { uri: NetUtil.newURI(TEST_DOMAIN + "test_title_change_saved"), + title: "original title", visits: [ new VisitInfo(), ], @@ -663,18 +720,107 @@ function test_title_change_saved() gHistory.updatePlaces(place, function(aResultCode, aPlaceInfo) { do_check_true(Components.isSuccessCode(aResultCode)); - // Then, change the title with visits. - place.title = "title change"; + // Now, make sure the empty string clears the title. + place.title = ""; place.visits = [new VisitInfo()]; gHistory.updatePlaces(place, function(aResultCode, aPlaceInfo) { do_check_true(Components.isSuccessCode(aResultCode)); - do_check_title_for_uri(place.uri, place.title); + do_check_title_for_uri(place.uri, null); + + // Then, change the title with visits. + place.title = "title change"; + place.visits = [new VisitInfo()]; + gHistory.updatePlaces(place, function(aResultCode, aPlaceInfo) { + do_check_true(Components.isSuccessCode(aResultCode)); + do_check_title_for_uri(place.uri, place.title); + + // Lastly, check that the title is cleared if we set it to null. + place.title = null; + place.visits = [new VisitInfo()]; + gHistory.updatePlaces(place, function(aResultCode, aPlaceInfo) { + do_check_true(Components.isSuccessCode(aResultCode)); + do_check_title_for_uri(place.uri, place.title); + + run_next_test(); + }); + }); + }); + }); +} + +function test_no_title_does_not_clear_title() +{ + const TITLE = "test title"; + // First, add a visit for it. + let place = { + uri: NetUtil.newURI(TEST_DOMAIN + "test_no_title_does_not_clear_title"), + title: TITLE, + visits: [ + new VisitInfo(), + ], + }; + do_check_false(gGlobalHistory.isVisited(place.uri)); + + gHistory.updatePlaces(place, function(aResultCode, aPlaceInfo) { + do_check_true(Components.isSuccessCode(aResultCode)); + + // Now, make sure that not specifying a title does not clear it. + delete place.title; + place.visits = [new VisitInfo()]; + gHistory.updatePlaces(place, function(aResultCode, aPlaceInfo) { + do_check_true(Components.isSuccessCode(aResultCode)); + do_check_title_for_uri(place.uri, TITLE); run_next_test(); }); }); } +function test_title_change_notifies() +{ + // There are three cases to test. The first case is to make sure we do not + // get notified if we do not specify a title. + let place = { + uri: NetUtil.newURI(TEST_DOMAIN + "test_title_change_notifies"), + visits: [ + new VisitInfo(), + ], + }; + do_check_false(gGlobalHistory.isVisited(place.uri)); + + let silentObserver = + new TitleChangedObserver(place.uri, "DO NOT WANT", function() { + do_throw("unexpected callback!"); + }); + + PlacesUtils.history.addObserver(silentObserver, false); + gHistory.updatePlaces(place); + + // The second case to test is that we 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"; + let callbackCount = 0; + let observer = new TitleChangedObserver(place.uri, place.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()]; + gHistory.updatePlaces(place); + break; + case 2: + PlacesUtils.history.removeObserver(silentObserver); + PlacesUtils.history.removeObserver(observer); + run_next_test(); + }; + }); + PlacesUtils.history.addObserver(observer, false); + gHistory.updatePlaces(place); +} + //////////////////////////////////////////////////////////////////////////////// //// Test Runner @@ -696,6 +842,8 @@ let gTests = [ test_sessionId_saved, test_guid_change_saved, test_title_change_saved, + test_no_title_does_not_clear_title, + test_title_change_notifies, ]; function run_test()