Bug 737836 - Improve frecency handling for multiple redirect sequences, and add a test. r?mak
MozReview-Commit-ID: ImJMF5KDvtz
--- a/browser/app/profile/firefox.js
+++ b/browser/app/profile/firefox.js
@@ -859,18 +859,19 @@ pref("places.frecency.typedVisitBonus",
// the redirect source and the typed ones.
pref("places.frecency.bookmarkVisitBonus", 75);
// The redirect source bonus overwrites any transition bonus.
// 0 would hide these pages, instead we want them low ranked. Thus we use
// linkVisitBonus - bookmarkVisitBonus, so that a bookmarked source is in par
// with a common link.
pref("places.frecency.redirectSourceVisitBonus", 25);
pref("places.frecency.downloadVisitBonus", 0);
-pref("places.frecency.permRedirectVisitBonus", 0);
-pref("places.frecency.tempRedirectVisitBonus", 0);
+// The perm/temp redirects here relate to redirect targets, not sources.
+pref("places.frecency.permRedirectVisitBonus", 50);
+pref("places.frecency.tempRedirectVisitBonus", 40);
pref("places.frecency.reloadVisitBonus", 0);
pref("places.frecency.defaultVisitBonus", 0);
// bonus (in percent) for place types for frecency calculations
pref("places.frecency.unvisitedBookmarkBonus", 140);
pref("places.frecency.unvisitedTypedBonus", 200);
// Controls behavior of the "Add Exception" dialog launched from SSL error pages
--- a/toolkit/components/places/SQLFunctions.cpp
+++ b/toolkit/components/places/SQLFunctions.cpp
@@ -544,17 +544,18 @@ namespace places {
nsCString redirectsTransitionFragment =
nsPrintfCString("%d AND %d ", nsINavHistoryService::TRANSITION_REDIRECT_PERMANENT,
nsINavHistoryService::TRANSITION_REDIRECT_TEMPORARY);
nsCOMPtr<mozIStorageStatement> getVisits = DB->GetStatement(
NS_LITERAL_CSTRING(
"/* do not warn (bug 659740 - SQLite may ignore index if few visits exist) */"
"SELECT "
"ROUND((strftime('%s','now','localtime','utc') - v.visit_date/1000000)/86400), "
- "IFNULL(origin.visit_type, v.visit_type), "
+ "origin.visit_type, "
+ "v.visit_type, "
"target.id NOTNULL "
"FROM moz_historyvisits v "
"LEFT JOIN moz_historyvisits origin ON origin.id = v.from_visit "
"AND v.visit_type BETWEEN "
) + redirectsTransitionFragment + NS_LITERAL_CSTRING(
"LEFT JOIN moz_historyvisits target ON v.id = target.from_visit "
"AND target.visit_type BETWEEN "
) + redirectsTransitionFragment + NS_LITERAL_CSTRING(
@@ -568,28 +569,40 @@ namespace places {
NS_ENSURE_SUCCESS(rv, rv);
// Fetch only a limited number of recent visits.
bool hasResult = false;
for (int32_t maxVisits = history->GetNumVisitsForFrecency();
numSampledVisits < maxVisits &&
NS_SUCCEEDED(getVisits->ExecuteStep(&hasResult)) && hasResult;
numSampledVisits++) {
+
int32_t visitType;
- rv = getVisits->GetInt32(1, &visitType);
+ bool isNull = false;
+ rv = getVisits->GetIsNull(1, &isNull);
NS_ENSURE_SUCCESS(rv, rv);
+ if (isRedirect == eIsRedirect || isNull) {
+ // Use the main visit_type.
+ rv = getVisits->GetInt32(2, &visitType);
+ NS_ENSURE_SUCCESS(rv, rv);
+ } else {
+ // This is a redirect target, so use the origin visit_type.
+ rv = getVisits->GetInt32(1, &visitType);
+ NS_ENSURE_SUCCESS(rv, rv);
+ }
+
RedirectState visitIsRedirect = isRedirect;
// If we don't know if this is a redirect or not, or this is not the
// most recent visit that we're looking at, then we use the redirect
// value from the database.
if (visitIsRedirect == eRedirectUnknown || numSampledVisits >= 1) {
int32_t redirect;
- rv = getVisits->GetInt32(2, &redirect);
+ rv = getVisits->GetInt32(3, &redirect);
NS_ENSURE_SUCCESS(rv, rv);
visitIsRedirect = !!redirect ? eIsRedirect : eIsNotRedirect;
}
bonus = history->GetFrecencyTransitionBonus(visitType, true, visitIsRedirect == eIsRedirect);
// Add the bookmark visit bonus.
if (hasBookmark) {
--- a/toolkit/components/places/tests/browser/browser.ini
+++ b/toolkit/components/places/tests/browser/browser.ini
@@ -14,13 +14,14 @@ support-files =
[browser_colorAnalyzer.js]
[browser_double_redirect.js]
[browser_favicon_privatebrowsing_perwindowpb.js]
[browser_favicon_setAndFetchFaviconForPage.js]
[browser_favicon_setAndFetchFaviconForPage_failures.js]
[browser_history_post.js]
[browser_notfound.js]
[browser_redirect.js]
+[browser_multi_redirect_frecency.js]
[browser_settitle.js]
[browser_visited_notfound.js]
[browser_visituri.js]
[browser_visituri_nohistory.js]
-[browser_visituri_privatebrowsing_perwindowpb.js]
\ No newline at end of file
+[browser_visituri_privatebrowsing_perwindowpb.js]
new file mode 100644
--- /dev/null
+++ b/toolkit/components/places/tests/browser/browser_multi_redirect_frecency.js
@@ -0,0 +1,135 @@
+const REDIRECT_URI = NetUtil.newURI("http://mochi.test:8888/tests/toolkit/components/places/tests/browser/redirect_thrice.sjs");
+const INTERMEDIATE_URI_1 = NetUtil.newURI("http://mochi.test:8888/tests/toolkit/components/places/tests/browser/redirect_twice_perma.sjs");
+const INTERMEDIATE_URI_2 = NetUtil.newURI("http://mochi.test:8888/tests/toolkit/components/places/tests/browser/redirect_once.sjs");
+const TARGET_URI = NetUtil.newURI("http://mochi.test:8888/tests/toolkit/components/places/tests/browser/final.html");
+
+const REDIRECT_SOURCE_VISIT_BONUS =
+ Services.prefs.getIntPref("places.frecency.redirectSourceVisitBonus");
+// const LINK_VISIT_BONUS =
+// Services.prefs.getIntPref("places.frecency.linkVisitBonus");
+// const TYPED_VISIT_BONUS =
+// Services.prefs.getIntPref("places.frecency.typedVisitBonus");
+const PERM_REDIRECT_VISIT_BONUS =
+ Services.prefs.getIntPref("places.frecency.permRedirectVisitBonus");
+
+registerCleanupFunction(function*() {
+ yield PlacesTestUtils.clearHistory();
+});
+
+function promiseVisitedURIObserver(redirectURI, targetURI, expectedTargetFrecency) {
+ // Create and add history observer.
+ return new Promise(resolve => {
+ let historyObserver = {
+ _redirectNotified: false,
+ onVisit(aURI, aVisitID, aTime, aSessionID, aReferringID,
+ aTransitionType) {
+ info("Received onVisit: " + aURI.spec);
+
+ if (aURI.equals(redirectURI)) {
+ this._redirectNotified = true;
+ // Wait for the target page notification.
+ return;
+ }
+
+ if (!aURI.equals(targetURI)) {
+ return;
+ }
+
+ PlacesUtils.history.removeObserver(historyObserver);
+
+ ok(this._redirectNotified, "The redirect should have been notified");
+
+ resolve();
+ },
+ onBeginUpdateBatch() {},
+ onEndUpdateBatch() {},
+ onTitleChanged() {},
+ onDeleteURI() {},
+ onClearHistory() {},
+ onPageChanged() {},
+ onDeleteVisits() {},
+ QueryInterface: XPCOMUtils.generateQI([Ci.nsINavHistoryObserver])
+ };
+ PlacesUtils.history.addObserver(historyObserver, false);
+ });
+}
+
+function* testURIFields(url, expectedFrecency, expectedHidden) {
+ let frecency = yield promiseFieldForUrl(url, "frecency");
+ is(frecency, expectedFrecency,
+ `Frecency of the page is the expected one (${url.spec})`);
+
+ let hidden = yield promiseFieldForUrl(url, "hidden");
+ is(hidden, expectedHidden, `The redirecting page should be hidden (${url.spec})`);
+}
+
+let expectedRedirectSourceFrecency = 0;
+let expectedTargetFrecency = 0;
+
+add_task(function* test_multiple_redirect() {
+ // Used to verify the redirect bonus overrides the typed bonus.
+ PlacesUtils.history.markPageAsTyped(REDIRECT_URI);
+
+ expectedRedirectSourceFrecency += REDIRECT_SOURCE_VISIT_BONUS;
+ // TODO Bug 487813 - This should be TYPED_VISIT_BONUS, however as we don't
+ // currently track redirects across multiple redirects, we fallback to the
+ // PERM_REDIRECT_VISIT_BONUS.
+ expectedTargetFrecency += PERM_REDIRECT_VISIT_BONUS;
+
+ let visitedURIPromise = promiseVisitedURIObserver(REDIRECT_URI, TARGET_URI, expectedTargetFrecency);
+
+ let newTabPromise = BrowserTestUtils.openNewForegroundTab(gBrowser, REDIRECT_URI.spec);
+ yield Promise.all([visitedURIPromise, newTabPromise]);
+
+ yield testURIFields(REDIRECT_URI, expectedRedirectSourceFrecency, 1);
+ yield testURIFields(INTERMEDIATE_URI_1, expectedRedirectSourceFrecency, 1);
+ yield testURIFields(INTERMEDIATE_URI_2, expectedRedirectSourceFrecency, 1);
+ yield testURIFields(TARGET_URI, expectedTargetFrecency, 0);
+
+ gBrowser.removeCurrentTab();
+});
+
+add_task(function* redirect_check_second_typed_visit() {
+ // A second visit with a typed url.
+ PlacesUtils.history.markPageAsTyped(REDIRECT_URI);
+
+ expectedRedirectSourceFrecency += REDIRECT_SOURCE_VISIT_BONUS;
+ // TODO Bug 487813 - This should be TYPED_VISIT_BONUS, however as we don't
+ // currently track redirects across multiple redirects, we fallback to the
+ // PERM_REDIRECT_VISIT_BONUS.
+ expectedTargetFrecency += PERM_REDIRECT_VISIT_BONUS;
+
+ let visitedURIPromise = promiseVisitedURIObserver(REDIRECT_URI, TARGET_URI, expectedTargetFrecency);
+
+ let newTabPromise = BrowserTestUtils.openNewForegroundTab(gBrowser, REDIRECT_URI.spec);
+ yield Promise.all([visitedURIPromise, newTabPromise]);
+
+ yield testURIFields(REDIRECT_URI, expectedRedirectSourceFrecency, 1);
+ yield testURIFields(INTERMEDIATE_URI_1, expectedRedirectSourceFrecency, 1);
+ yield testURIFields(INTERMEDIATE_URI_2, expectedRedirectSourceFrecency, 1);
+ yield testURIFields(TARGET_URI, expectedTargetFrecency, 0);
+
+ gBrowser.removeCurrentTab();
+});
+
+
+add_task(function* redirect_check_subsequent_link_visit() {
+ // Another visit, but this time as a visited url.
+ expectedRedirectSourceFrecency += REDIRECT_SOURCE_VISIT_BONUS;
+ // TODO Bug 487813 - This should be LINK_VISIT_BONUS, however as we don't
+ // currently track redirects across multiple redirects, we fallback to the
+ // PERM_REDIRECT_VISIT_BONUS.
+ expectedTargetFrecency += PERM_REDIRECT_VISIT_BONUS;
+
+ let visitedURIPromise = promiseVisitedURIObserver(REDIRECT_URI, TARGET_URI, expectedTargetFrecency);
+
+ let newTabPromise = BrowserTestUtils.openNewForegroundTab(gBrowser, REDIRECT_URI.spec);
+ yield Promise.all([visitedURIPromise, newTabPromise]);
+
+ yield testURIFields(REDIRECT_URI, expectedRedirectSourceFrecency, 1);
+ yield testURIFields(INTERMEDIATE_URI_1, expectedRedirectSourceFrecency, 1);
+ yield testURIFields(INTERMEDIATE_URI_2, expectedRedirectSourceFrecency, 1);
+ yield testURIFields(TARGET_URI, expectedTargetFrecency, 0);
+
+ gBrowser.removeCurrentTab();
+});
--- a/toolkit/components/places/tests/browser/head.js
+++ b/toolkit/components/places/tests/browser/head.js
@@ -13,19 +13,21 @@ const TRANSITION_REDIRECT_PERMANENT = Pl
const TRANSITION_REDIRECT_TEMPORARY = PlacesUtils.history.TRANSITION_REDIRECT_TEMPORARY;
const TRANSITION_EMBED = PlacesUtils.history.TRANSITION_EMBED;
const TRANSITION_FRAMED_LINK = PlacesUtils.history.TRANSITION_FRAMED_LINK;
const TRANSITION_DOWNLOAD = PlacesUtils.history.TRANSITION_DOWNLOAD;
/**
* Returns a moz_places field value for a url.
*
- * @param aURI
+ * @param {nsIURI|String} aURI
* The URI or spec to get field for.
- * param aCallback
+ * @param {String} aFieldName
+ * The field name to get the value of.
+ * @param {Function} aCallback
* Callback function that will get the property value.
*/
function fieldForUrl(aURI, aFieldName, aCallback) {
let url = aURI instanceof Ci.nsIURI ? aURI.spec : aURI;
let stmt = PlacesUtils.history.QueryInterface(Ci.nsPIPlacesDatabase)
.DBConnection.createAsyncStatement(
`SELECT ${aFieldName} FROM moz_places WHERE url_hash = hash(:page_url) AND url = :page_url`
);
@@ -44,16 +46,35 @@ function fieldForUrl(aURI, aFieldName, a
ok(false, "The statement should properly succeed");
aCallback(this._value);
}
});
stmt.finalize();
}
/**
+ * Promise wrapper for fieldForUrl.
+ *
+ * @param {nsIURI|String} aURI
+ * The URI or spec to get field for.
+ * @param {String} aFieldName
+ * The field name to get the value of.
+ * @return {Promise}
+ * A promise that is resolved with the value of the field.
+ */
+function promiseFieldForUrl(aURI, aFieldName) {
+ return new Promise(resolve => {
+ function callback(result) {
+ resolve(result);
+ }
+ fieldForUrl(aURI, aFieldName, callback);
+ });
+}
+
+/**
* 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() {},
onEndUpdateBatch() {},
new file mode 100644
--- /dev/null
+++ b/toolkit/components/places/tests/browser/redirect_thrice.sjs
@@ -0,0 +1,9 @@
+/**
+ * Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/
+ */
+
+function handleRequest(request, response) {
+ response.setStatusLine("1.1", 302, "Found");
+ response.setHeader("Location", "redirect_twice_perma.sjs", false);
+}
new file mode 100644
--- /dev/null
+++ b/toolkit/components/places/tests/browser/redirect_twice_perma.sjs
@@ -0,0 +1,9 @@
+/**
+ * Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/
+ */
+
+function handleRequest(request, response) {
+ response.setStatusLine("1.1", 301, "Found");
+ response.setHeader("Location", "redirect_once.sjs", false);
+}
--- a/toolkit/components/places/tests/moz.build
+++ b/toolkit/components/places/tests/moz.build
@@ -47,17 +47,19 @@ TEST_HARNESS_FILES.testing.mochitest.tes
'browser/favicon-normal32.png',
'browser/favicon.html',
'browser/final.html',
'browser/history_post.html',
'browser/history_post.sjs',
'browser/redirect-target.html',
'browser/redirect.sjs',
'browser/redirect_once.sjs',
+ 'browser/redirect_thrice.sjs',
'browser/redirect_twice.sjs',
+ 'browser/redirect_twice_perma.sjs',
'browser/title1.html',
'browser/title2.html',
]
TEST_HARNESS_FILES.testing.mochitest.tests.toolkit.components.places.tests.chrome += [
'chrome/bad_links.atom',
'chrome/link-less-items-no-site-uri.rss',
'chrome/link-less-items.rss',