Magic-Mirror/gecko-dev
1
0
Fork 0
mirror of https://github.com/mozilla/gecko-dev.git synced 2024-11-28 23:31:56 +00:00
Code Issues Actions Packages Projects Releases Wiki Activity

Bug 1341097 - part 3: don't dispatch oodles of titlechanged notifications for new history entries, r=mak

Browse Source 
MozReview-Commit-ID: 7jHOcCQ5ZBb

--HG--
extra : rebase_source : 6dff335e78857e7c860ebe7ec6ea415fe0f9e3f6
This commit is contained in:
Gijs Kruitbosch 2017-02-27 18:26:21 +00:00
parent 288e9a57a7
commit b0f2da2a20
18 changed files with 257 additions and 55 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

14
addon-sdk/source/lib/sdk/places/events.js
View File

@ -115,6 +115,20 @@ function formatValue (type, data) {
}
var historyObserver = createObserverInstance(HISTORY_EVENTS, HISTORY_ARGS);
// Hack alert: we sometimes need to send extra title-changed notifications
// ourselves for backwards compat. Replace the original onVisit callback to
// add on that logic:
historyObserver.realOnVisit = historyObserver.onVisit;
historyObserver.onVisit = function(url, visitId, time, sessionId,
referringId, transitionType, guid, hidden,
visitCount, typed, lastKnownTitle) {
// If this is the first visit we're adding, fire title-changed
// in case anyone cares.
if (visitCount == 1) {
historyObserver.onTitleChanged(url, lastKnownTitle);
}
this.realOnVisit(url, visitId, time, sessionId, referringId, transitionType);
};
historyService.addObserver(historyObserver, false);
var bookmarkObserver = createObserverInstance(BOOKMARK_EVENTS, BOOKMARK_ARGS);

35
browser/components/newtab/PlacesProvider.jsm
View File

@ -52,6 +52,24 @@ Links.prototype = {
* All history events are emitted from this object.
*/
historyObserver: {
_batchProcessingDepth: 0,
_batchCalledFrecencyChanged: false,
/**
* Called by the history service.
*/
onBeginUpdateBatch() {
this._batchProcessingDepth += 1;
},
onEndUpdateBatch() {
this._batchProcessingDepth -= 1;
if (this._batchProcessingDepth == 0 && this._batchCalledFrecencyChanged) {
this.onManyFrecenciesChanged();
this._batchCalledFrecencyChanged = false;
}
},
onDeleteURI: function historyObserver_onDeleteURI(aURI) {
// let observers remove sensetive data associated with deleted visit
gLinks.emit("deleteURI", {
@ -65,6 +83,15 @@ Links.prototype = {
onFrecencyChanged: function historyObserver_onFrecencyChanged(aURI,
aNewFrecency, aGUID, aHidden, aLastVisitDate) { // jshint ignore:line
// If something is doing a batch update of history entries we don't want
// to do lots of work for each record. So we just track the fact we need
// to call onManyFrecenciesChanged() once the batch is complete.
if (this._batchProcessingDepth > 0) {
this._batchCalledFrecencyChanged = true;
return;
}
// The implementation of the query in getLinks excludes hidden and
// unvisited pages, so it's important to exclude them here, too.
if (!aHidden && aLastVisitDate &&
@ -84,6 +111,14 @@ Links.prototype = {
gLinks.emit("manyLinksChanged");
},
onVisit(aURI, aVisitId, aTime, aSessionId, aReferrerVisitId, aTransitionType,
aGuid, aHidden, aVisitCount, aTyped, aLastKnownTitle) {
// For new visits, if we're not batch processing, notify for a title update
if (!this._batchProcessingDepth && aVisitCount == 1 && aLastKnownTitle) {
this.onTitleChanged(aURI, aTitle, aGuid);
}
},
onTitleChanged: function historyObserver_onTitleChanged(aURI, aNewTitle) {
if (NewTabUtils.linkChecker.checkLoadURI(aURI.spec)) {
gLinks.emit("linkChanged", {

5
browser/components/newtab/tests/xpcshell/test_PlacesProvider.js
View File

@ -145,15 +145,14 @@ add_task(function* test_Links_onLinkChanged() {
let linkChangedPromise = new Promise(resolve => {
let handler = (_, link) => { // jshint ignore:line
/* There are 3 linkChanged events:
/* There are 2 linkChanged events:
* 1. visit insertion (-1 frecency by default)
* 2. frecency score update (after transition type calculation etc)
* 3. title change
*/
if (link.url === url) {
equal(link.url, url, `expected url on linkChanged event`);
linkChangedMsgCount += 1;
if (linkChangedMsgCount === 3) {
if (linkChangedMsgCount === 2) {
ok(true, `all linkChanged events captured`);
provider.off("linkChanged", this);
resolve();

9
services/sync/tests/unit/test_history_store.js
View File

@ -33,16 +33,17 @@ function queryHistoryVisits(uri) {
return queryPlaces(uri, options);
}
function onNextTitleChanged(callback) {
function onNextVisit(callback) {
PlacesUtils.history.addObserver({
onBeginUpdateBatch: function onBeginUpdateBatch() {},
onEndUpdateBatch: function onEndUpdateBatch() {},
onPageChanged: function onPageChanged() {},
onTitleChanged: function onTitleChanged() {
},
onVisit: function onVisit() {
PlacesUtils.history.removeObserver(this);
Utils.nextTick(callback);
},
onVisit: function onVisit() {},
onDeleteVisits: function onDeleteVisits() {},
onPageExpired: function onPageExpired() {},
onDeleteURI: function onDeleteURI() {},
@ -125,7 +126,7 @@ add_test(function test_store() {
_("Let's modify the record and have the store update the database.");
let secondvisit = {date: TIMESTAMP2,
type: Ci.nsINavHistoryService.TRANSITION_TYPED};
onNextTitleChanged(ensureThrows(function() {
onNextVisit(ensureThrows(function() {
let queryres = queryHistoryVisits(fxuri);
do_check_eq(queryres.length, 2);
do_check_eq(queryres[0].time, TIMESTAMP1);
@ -147,7 +148,7 @@ add_test(function test_store_create() {
_("Create a brand new record through the store.");
tbguid = Utils.makeGUID();
tburi = Utils.makeURI("http://getthunderbird.com");
onNextTitleChanged(ensureThrows(function() {
onNextVisit(ensureThrows(function() {
do_check_attribute_count(store.getAllIDs(), 2);
let queryres = queryHistoryVisits(tburi);
do_check_eq(queryres.length, 1);

3
toolkit/components/downloads/nsDownloadManager.cpp
View File

@ -2332,7 +2332,8 @@ 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;
}

5
toolkit/components/places/History.cpp
View File

@ -658,7 +658,8 @@ public:
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 =
@ -1045,7 +1046,7 @@ public:
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);

6
toolkit/components/places/nsINavHistoryService.idl
View File

@ -671,6 +671,9 @@ interface nsINavHistoryObserver : nsISupports
* 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,
* and might be null if it is not known.
*/
void onVisit(in nsIURI aURI,
in long long aVisitId,
@ -681,7 +684,8 @@ interface nsINavHistoryObserver : nsISupports
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

3
toolkit/components/places/nsNavBookmarks.cpp
View File

@ -3276,7 +3276,8 @@ 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);

20
toolkit/components/places/nsNavHistory.cpp
View File

@ -495,16 +495,18 @@ nsNavHistory::LoadPrefs()
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());
MOZ_ASSERT(aVisitCount, "Should have at least 1 visit when notifying");
// 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
@ -518,7 +520,7 @@ nsNavHistory::NotifyOnVisit(nsIURI* aURI,
NOTIFY_OBSERVERS(mCanNotify, mCacheObservers, mObservers,
nsINavHistoryObserver,
OnVisit(aURI, aVisitId, aTime, 0, aReferrerVisitId,
aTransitionType, aGuid, aHidden, aVisitCount, aTyped));
aTransitionType, aGuid, aHidden, aVisitCount, aTyped, aLastKnownTitle));
}
void

3
toolkit/components/places/nsNavHistory.h
View File

@ -443,7 +443,8 @@ public:
const nsACString& aGuid,
bool aHidden,
uint32_t aVisitCount,
uint32_t aTyped);
uint32_t aTyped,
const nsAString& aLastKnownTitle);
/**
* Fires onTitleChanged event to nsINavHistoryService observers

13
toolkit/components/places/nsNavHistoryResult.cpp
View File

@ -4655,7 +4655,8 @@ 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);
@ -4670,6 +4671,16 @@ nsNavHistoryResult::OnVisit(nsIURI* aURI, int64_t aVisitId, PRTime aTime,
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;

4
toolkit/components/places/nsNavHistoryResult.h
View File

@ -94,7 +94,9 @@ private:
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
//

3
toolkit/components/places/tests/browser/browser.ini
View File

@ -18,6 +18,9 @@ support-files =
[browser_favicon_setAndFetchFaviconForPage_failures.js]
[browser_history_post.js]
[browser_notfound.js]
[browser_onvisit_title_null_for_navigation.js]
support-files =
empty_page.html
[browser_redirect.js]
[browser_multi_redirect_frecency.js]
[browser_settitle.js]

28
toolkit/components/places/tests/browser/browser_onvisit_title_null_for_navigation.js Normal file
View File

@ -0,0 +1,28 @@
const TEST_PATH = getRootDirectory(gTestPath).replace("chrome://mochitests/content", "http://example.com");
add_task(function* checkTitleNotificationForNavigation() {
const EXPECTED_URL = Services.io.newURI(TEST_PATH + "empty_page.html");
let promiseTitleChanged = new Promise(resolve => {
let obs = {
onVisit(aURI, aVisitId, aTime, aSessionId, aReferrerVisitId, aTransitionType,
aGuid, aHidden, aVisitCount, aTyped, aLastKnownTitle) {
info("onVisit: " + aURI.spec);
if (aURI.equals(EXPECTED_URL)) {
Assert.equal(aLastKnownTitle, null, "Should not have a title");
}
},
onTitleChanged(aURI, aTitle, aGuid) {
if (aURI.equals(EXPECTED_URL)) {
is(aTitle, "I am an empty page", "Should have correct title in titlechanged notification");
PlacesUtils.history.removeObserver(obs);
resolve();
}
},
};
PlacesUtils.history.addObserver(obs, false);
});
let tab = yield BrowserTestUtils.openNewForegroundTab(gBrowser, EXPECTED_URL.spec);
yield promiseTitleChanged;
yield BrowserTestUtils.removeTab(tab);
});

8
toolkit/components/places/tests/browser/empty_page.html Normal file
View File

@ -0,0 +1,8 @@
<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8">
<title>I am an empty page</title>
</head>
<body>Empty</body>
</html>

2
toolkit/components/places/tests/history/test_updatePlaces_sameUri_titleChanged.js
View File

@ -10,7 +10,7 @@ add_task(function* test() {
__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();
}

143
toolkit/components/places/tests/unit/test_async_history_api.js
View File

@ -98,14 +98,17 @@ VisitObserver.prototype = {
aSessionId,
aReferringId,
aTransitionType,
aGUID) {
do_print("onVisit(" + aURI.spec + ", " + aVisitId + ", " + aTime +
", " + aSessionId + ", " + aReferringId + ", " +
aTransitionType + ", " + aGUID + ")");
aGUID,
aHidden,
aVisitCount,
aTyped,
aLastKnownTitle) {
let args = [...arguments].slice(1);
do_print("onVisit(" + aURI.spec + args.join(", ") + ")");
if (!this.uri.equals(aURI) || this.guid != aGUID) {
return;
}
this.callback(aTime, aTransitionType);
this.callback(aTime, aTransitionType, aLastKnownTitle);
},
};
@ -951,37 +954,45 @@ add_task(function* test_title_change_notifies() {
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;
}
});
PlacesUtils.history.addObserver(observer, false);
PlacesUtils.asyncHistory.updatePlaces(aPlace);
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);
});
yield promiseTitleChangedObserver(place);
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;
// 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();
});
@ -1235,3 +1246,75 @@ add_task(function* test_ignore_results_and_errors() {
"Should know that we updated 1 item from the completion callback.");
yield PlacesTestUtils.promiseAsyncUpdates();
});
add_task(function* test_title_on_initial_visit() {
let place = {
uri: NetUtil.newURI(TEST_DOMAIN + "test_visit_title"),
title: "My title",
visits: [
new VisitInfo(),
],
guid: "mnopqrstuvwx",
};
let visitPromise = new Promise(resolve => {
let visitObserver = new VisitObserver(place.uri, place.guid,
function(aVisitDate,
aTransitionType,
aLastKnownTitle) {
do_check_eq(place.title, aLastKnownTitle);
PlacesUtils.history.removeObserver(visitObserver);
resolve();
});
PlacesUtils.history.addObserver(visitObserver, false);
});
yield promiseUpdatePlaces(place);
yield visitPromise;
// Now check an empty title doesn't get reported as null
place = {
uri: NetUtil.newURI(TEST_DOMAIN + "test_visit_title"),
title: "",
visits: [
new VisitInfo(),
],
guid: "fghijklmnopq",
};
visitPromise = new Promise(resolve => {
let visitObserver = new VisitObserver(place.uri, place.guid,
function(aVisitDate,
aTransitionType,
aLastKnownTitle) {
do_check_eq(place.title, aLastKnownTitle);
PlacesUtils.history.removeObserver(visitObserver);
resolve();
});
PlacesUtils.history.addObserver(visitObserver, false);
});
yield promiseUpdatePlaces(place);
yield visitPromise;
// and that a missing title correctly gets reported as null.
place = {
uri: NetUtil.newURI(TEST_DOMAIN + "test_visit_title"),
visits: [
new VisitInfo(),
],
guid: "fghijklmnopq",
};
visitPromise = new Promise(resolve => {
let visitObserver = new VisitObserver(place.uri, place.guid,
function(aVisitDate,
aTransitionType,
aLastKnownTitle) {
do_check_eq(null, aLastKnownTitle);
PlacesUtils.history.removeObserver(visitObserver);
resolve();
});
PlacesUtils.history.addObserver(visitObserver, false);
});
yield promiseUpdatePlaces(place);
yield visitPromise;
});

8
toolkit/modules/NewTabUtils.jsm
View File

@ -734,6 +734,14 @@ var PlacesProvider = {
}
},
onVisit(aURI, aVisitId, aTime, aSessionId, aReferrerVisitId, aTransitionType,
aGuid, aHidden, aVisitCount, aTyped, aLastKnownTitle) {
// For new visits, if we're not batch processing, notify for a title // update
if (!this._batchProcessingDepth && aVisitCount == 1 && aLastKnownTitle) {
this.onTitleChanged(aURI, aLastKnownTitle, aGuid);
}
},
onDeleteURI: function PlacesProvider_onDeleteURI(aURI, aGUID, aReason) {
// let observers remove sensetive data associated with deleted visit
this._callObservers("onDeleteURI", {