diff --git a/docshell/base/IHistory.h b/docshell/base/IHistory.h index b1f5bb983166..47ff2f00278c 100644 --- a/docshell/base/IHistory.h +++ b/docshell/base/IHistory.h @@ -71,22 +71,28 @@ public: */ TOP_LEVEL = 1 << 0, /** - * Indicates whether the URI was loaded as part of a permanent redirect. + * Indicates whether the URI is the target of a permanent redirect. */ REDIRECT_PERMANENT = 1 << 1, /** - * Indicates whether the URI was loaded as part of a temporary redirect. + * Indicates whether the URI is the target of a temporary redirect. */ REDIRECT_TEMPORARY = 1 << 2, /** - * Indicates the URI is redirecting (Response code 3xx). + * Indicates the URI will redirect (Response code 3xx). */ REDIRECT_SOURCE = 1 << 3, /** * Indicates the URI caused an error that is unlikely fixable by a * retry, like a not found or unfetchable page. */ - UNRECOVERABLE_ERROR = 1 << 4 + UNRECOVERABLE_ERROR = 1 << 4, + /** + * If REDIRECT_SOURCE is set, this indicates that the redirect is permanent. + * Note this differs from REDIRECT_PERMANENT because that one refers to how + * we reached the URI, while this is used when the URI itself redirects. + */ + REDIRECT_SOURCE_PERMANENT = 1 << 5 }; /** diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp index d23de893a8ae..02541b70ca0b 100644 --- a/docshell/base/nsDocShell.cpp +++ b/docshell/base/nsDocShell.cpp @@ -12610,6 +12610,9 @@ nsDocShell::AddURIVisit(nsIURI* aURI, if (aResponseStatus >= 300 && aResponseStatus < 400) { visitURIFlags |= IHistory::REDIRECT_SOURCE; + if (aResponseStatus == 301 || aResponseStatus == 308) { + visitURIFlags |= IHistory::REDIRECT_SOURCE_PERMANENT; + } } // Errors 400-501 and 505 are considered unrecoverable, in the sense a // simple retry attempt by the user is unlikely to solve them. diff --git a/toolkit/components/places/History.cpp b/toolkit/components/places/History.cpp index 380994d0b3a2..c20e1f208ae2 100644 --- a/toolkit/components/places/History.cpp +++ b/toolkit/components/places/History.cpp @@ -83,7 +83,7 @@ struct VisitData { , referrerVisitId(0) , titleChanged(false) , shouldUpdateFrecency(true) - , redirect(false) + , useFrecencyRedirectBonus(false) { guid.SetIsVoid(true); title.SetIsVoid(true); @@ -105,7 +105,7 @@ struct VisitData { , referrerVisitId(0) , titleChanged(false) , shouldUpdateFrecency(true) - , redirect(false) + , useFrecencyRedirectBonus(false) { MOZ_ASSERT(aURI); if (aURI) { @@ -164,8 +164,9 @@ struct VisitData { // Indicates whether frecency should be updated for this visit. bool shouldUpdateFrecency; - // Whether this is a redirect source. - bool redirect; + // Whether to override the visit type bonus with a redirect bonus when + // calculating frecency on the most recent visit. + bool useFrecencyRedirectBonus; }; //////////////////////////////////////////////////////////////////////////////// @@ -1489,8 +1490,7 @@ private: rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("page_id"), aPlace.placeId); NS_ENSURE_SUCCESS(rv, rv); - - rv = stmt->BindInt32ByName(NS_LITERAL_CSTRING("redirect"), aPlace.redirect); + rv = stmt->BindInt32ByName(NS_LITERAL_CSTRING("redirect"), aPlace.useFrecencyRedirectBonus); NS_ENSURE_SUCCESS(rv, rv); rv = stmt->Execute(); @@ -2840,8 +2840,13 @@ History::VisitURI(nsIURI* aURI, } place.SetTransitionType(transitionType); - place.redirect = aFlags & IHistory::REDIRECT_SOURCE; - place.hidden = GetHiddenState(place.redirect, place.transitionType); + bool isRedirect = aFlags & IHistory::REDIRECT_SOURCE; + if (isRedirect) { + place.useFrecencyRedirectBonus = + (aFlags & IHistory::REDIRECT_SOURCE_PERMANENT) || + transitionType != nsINavHistoryService::TRANSITION_TYPED; + } + place.hidden = GetHiddenState(isRedirect, place.transitionType); // Error pages should never be autocompleted. if (aFlags & IHistory::UNRECOVERABLE_ERROR) { diff --git a/toolkit/components/places/SQLFunctions.cpp b/toolkit/components/places/SQLFunctions.cpp index f302fdaa39e4..5125bb1c59c4 100644 --- a/toolkit/components/places/SQLFunctions.cpp +++ b/toolkit/components/places/SQLFunctions.cpp @@ -676,16 +676,16 @@ namespace places { return NS_OK; } - enum RedirectState { - eRedirectUnknown, - eIsRedirect, - eIsNotRedirect + enum RedirectBonus { + eUnknown, + eRedirect, + eNormal }; - RedirectState isRedirect = eRedirectUnknown; + RedirectBonus mostRecentVisitBonus = eUnknown; if (numEntries > 1) { - isRedirect = aArguments->AsInt32(1) ? eIsRedirect : eIsNotRedirect; + mostRecentVisitBonus = aArguments->AsInt32(1) ? eRedirect : eNormal; } int32_t typed = 0; @@ -735,8 +735,9 @@ namespace places { if (visitCount > 0) { // Get a sample of the last visits to the page, to calculate its weight. - // In case of a temporary or permanent redirect, calculate the frecency + // In case the visit is a redirect target, calculate the frecency // as if the original page was visited. + // If it's a redirect source, we may want to use a lower bonus. nsCString redirectsTransitionFragment = nsPrintfCString("%d AND %d ", nsINavHistoryService::TRANSITION_REDIRECT_PERMANENT, nsINavHistoryService::TRANSITION_REDIRECT_TEMPORARY); @@ -744,10 +745,9 @@ namespace places { 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), " - "origin.visit_type, " - "v.visit_type, " - "target.id NOTNULL " + "IFNULL(origin.visit_type, v.visit_type) AS visit_type, " + "target.visit_type AS target_visit_type, " + "ROUND((strftime('%s','now','localtime','utc') - v.visit_date/1000000)/86400) AS age_in_days " "FROM moz_historyvisits v " "LEFT JOIN moz_historyvisits origin ON origin.id = v.from_visit " "AND v.visit_type BETWEEN " @@ -770,35 +770,23 @@ namespace places { numSampledVisits < maxVisits && NS_SUCCEEDED(getVisits->ExecuteStep(&hasResult)) && hasResult; numSampledVisits++) { + // If this is a redirect target, we'll use the visitType of the source, + // otherwise the actual visitType. + int32_t visitType = getVisits->AsInt32(0); - int32_t 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); + // When adding a new visit, we should haved passed-in whether we should + // use the redirect bonus. We can't fetch this information from the + // database, because we only store redirect targets. + // For older visits we extract the value from the database. + bool useRedirectBonus = mostRecentVisitBonus == eRedirect; + if (mostRecentVisitBonus == eUnknown || numSampledVisits > 0) { + int32_t targetVisitType = getVisits->AsInt32(1); + useRedirectBonus = targetVisitType == nsINavHistoryService::TRANSITION_REDIRECT_PERMANENT || + (targetVisitType == nsINavHistoryService::TRANSITION_REDIRECT_TEMPORARY && + visitType != nsINavHistoryService::TRANSITION_TYPED); } - 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(3, &redirect); - NS_ENSURE_SUCCESS(rv, rv); - visitIsRedirect = !!redirect ? eIsRedirect : eIsNotRedirect; - } - - bonus = history->GetFrecencyTransitionBonus(visitType, true, visitIsRedirect == eIsRedirect); + bonus = history->GetFrecencyTransitionBonus(visitType, true, useRedirectBonus); // Add the bookmark visit bonus. if (hasBookmark) { @@ -807,7 +795,7 @@ namespace places { // If bonus was zero, we can skip the work to determine the weight. if (bonus) { - int32_t ageInDays = getVisits->AsInt32(0); + int32_t ageInDays = getVisits->AsInt32(2); int32_t weight = history->GetFrecencyAgedWeight(ageInDays); pointsForSampledVisits += (float)(weight * (bonus / 100.0)); } diff --git a/toolkit/components/places/SQLFunctions.h b/toolkit/components/places/SQLFunctions.h index 71a67b0f7f04..ef1012511609 100644 --- a/toolkit/components/places/SQLFunctions.h +++ b/toolkit/components/places/SQLFunctions.h @@ -198,8 +198,9 @@ private: * * @param {int64_t} pageId * The id of the page. Pass -1 if the page is being added right now. - * @param [optional] {int32_t} redirect - * Whether the page visit is a redirect. Default is false. + * @param [optional] {int32_t} useRedirectBonus + * Whether we should use the lower redirect bonus for the most recent + * page visit. If not passed in, it will use a database guess. */ class CalculateFrecencyFunction final : public mozIStorageFunction { diff --git a/toolkit/components/places/tests/browser/browser_multi_redirect_frecency.js b/toolkit/components/places/tests/browser/browser_multi_redirect_frecency.js index 4bb298471d32..ee06c7843899 100644 --- a/toolkit/components/places/tests/browser/browser_multi_redirect_frecency.js +++ b/toolkit/components/places/tests/browser/browser_multi_redirect_frecency.js @@ -11,6 +11,8 @@ const REDIRECT_SOURCE_VISIT_BONUS = Services.prefs.getIntPref("places.frecency.redirectSourceVisitBonus"); const PERM_REDIRECT_VISIT_BONUS = Services.prefs.getIntPref("places.frecency.permRedirectVisitBonus"); +const TYPED_VISIT_BONUS = + Services.prefs.getIntPref("places.frecency.typedVisitBonus"); // Ensure that decay frecency doesn't kick in during tests (as a result // of idle-daily). @@ -28,21 +30,9 @@ async function check_uri(uri, frecency, hidden) { "Hidden value of the page is the expected one"); } -let redirectSourceFrecency = 0; -let redirectTargetFrecency = 0; - -add_task(async function test_multiple_redirect() { - // Used to verify the redirect bonus overrides the typed bonus. - PlacesUtils.history.markPageAsTyped(REDIRECT_URI); - - redirectSourceFrecency += 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. - redirectTargetFrecency += PERM_REDIRECT_VISIT_BONUS; +async function waitVisitedNotifications() { let redirectNotified = false; - - let visitedPromise = PlacesTestUtils.waitForNotification("onVisits", visits => { + await PlacesTestUtils.waitForNotification("onVisits", visits => { is(visits.length, 1, "Was notified for the right number of visits."); let {uri} = visits[0]; info("Received onVisits: " + uri.spec); @@ -51,83 +41,107 @@ add_task(async function test_multiple_redirect() { } return uri.equals(TARGET_URI); }, "history"); + return redirectNotified; +} - let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, REDIRECT_URI.spec); - info("Waiting for onVisits"); - await visitedPromise; - ok(redirectNotified, "The redirect should have been notified"); +let firstRedirectBonus = 0; +let nextRedirectBonus = 0; +let targetBonus = 0; - await check_uri(REDIRECT_URI, redirectSourceFrecency, 1); - await check_uri(INTERMEDIATE_URI_1, redirectSourceFrecency, 1); - await check_uri(INTERMEDIATE_URI_2, redirectSourceFrecency, 1); - await check_uri(TARGET_URI, redirectTargetFrecency, 0); +add_task(async function test_multiple_redirect() { + // The redirect source bonus overrides the link bonus. + let visitedPromise = waitVisitedNotifications(); + await BrowserTestUtils.withNewTab({ + gBrowser, + url: REDIRECT_URI.spec, + }, async function() { + info("Waiting for onVisits"); + let redirectNotified = await visitedPromise; + ok(redirectNotified, "The redirect should have been notified"); - BrowserTestUtils.removeTab(tab); + firstRedirectBonus += REDIRECT_SOURCE_VISIT_BONUS; + await check_uri(REDIRECT_URI, firstRedirectBonus, 1); + nextRedirectBonus += REDIRECT_SOURCE_VISIT_BONUS; + await check_uri(INTERMEDIATE_URI_1, nextRedirectBonus, 1); + await check_uri(INTERMEDIATE_URI_2, nextRedirectBonus, 1); + // 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. + targetBonus += PERM_REDIRECT_VISIT_BONUS; + await check_uri(TARGET_URI, targetBonus, 0); + }); }); -add_task(async function redirect_check_second_typed_visit() { - // A second visit with a typed url. +add_task(async function test_multiple_redirect_typed() { + // The typed bonus wins because the redirect is permanent. PlacesUtils.history.markPageAsTyped(REDIRECT_URI); + let visitedPromise = waitVisitedNotifications(); + await BrowserTestUtils.withNewTab({ + gBrowser, + url: REDIRECT_URI.spec, + }, async function() { + info("Waiting for onVisits"); + let redirectNotified = await visitedPromise; + ok(redirectNotified, "The redirect should have been notified"); - redirectSourceFrecency += 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. - redirectTargetFrecency += PERM_REDIRECT_VISIT_BONUS; - let redirectNotified = false; - - let visitedPromise = PlacesTestUtils.waitForNotification("onVisits", visits => { - Assert.equal(visits.length, 1, "Was notified for the right number of visits."); - let {uri} = visits[0]; - info("Received onVisits: " + uri.spec); - if (uri.equals(REDIRECT_URI)) { - redirectNotified = true; - } - return uri.equals(TARGET_URI); - }, "history"); - - let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, REDIRECT_URI.spec); - info("Waiting for onVisits"); - await visitedPromise; - ok(redirectNotified, "The redirect should have been notified"); - - await check_uri(REDIRECT_URI, redirectSourceFrecency, 1); - await check_uri(INTERMEDIATE_URI_1, redirectSourceFrecency, 1); - await check_uri(INTERMEDIATE_URI_2, redirectSourceFrecency, 1); - await check_uri(TARGET_URI, redirectTargetFrecency, 0); - - BrowserTestUtils.removeTab(tab); + firstRedirectBonus += TYPED_VISIT_BONUS; + await check_uri(REDIRECT_URI, firstRedirectBonus, 1); + nextRedirectBonus += REDIRECT_SOURCE_VISIT_BONUS; + await check_uri(INTERMEDIATE_URI_1, nextRedirectBonus, 1); + await check_uri(INTERMEDIATE_URI_2, nextRedirectBonus, 1); + // 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. + targetBonus += PERM_REDIRECT_VISIT_BONUS; + await check_uri(TARGET_URI, targetBonus, 0); + }); }); +add_task(async function test_second_typed_visit() { + // The typed bonus wins because the redirect is permanent. + PlacesUtils.history.markPageAsTyped(REDIRECT_URI); + let visitedPromise = waitVisitedNotifications(); + await BrowserTestUtils.withNewTab({ + gBrowser, + url: REDIRECT_URI.spec, + }, async function() { + info("Waiting for onVisits"); + let redirectNotified = await visitedPromise; + ok(redirectNotified, "The redirect should have been notified"); -add_task(async function redirect_check_subsequent_link_visit() { - // Another visit, but this time as a visited url. - redirectSourceFrecency += 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. - redirectTargetFrecency += PERM_REDIRECT_VISIT_BONUS; - let redirectNotified = false; - - let visitedPromise = PlacesTestUtils.waitForNotification("onVisits", visits => { - Assert.equal(visits.length, 1, "Was notified for the right number of visits."); - let {uri} = visits[0]; - info("Received onVisits: " + uri.spec); - if (uri.equals(REDIRECT_URI)) { - redirectNotified = true; - } - return uri.equals(TARGET_URI); - }, "history"); - - let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, REDIRECT_URI.spec); - info("Waiting for onVisits"); - await visitedPromise; - ok(redirectNotified, "The redirect should have been notified"); - - await check_uri(REDIRECT_URI, redirectSourceFrecency, 1); - await check_uri(INTERMEDIATE_URI_1, redirectSourceFrecency, 1); - await check_uri(INTERMEDIATE_URI_2, redirectSourceFrecency, 1); - await check_uri(TARGET_URI, redirectTargetFrecency, 0); - - BrowserTestUtils.removeTab(tab); + firstRedirectBonus += TYPED_VISIT_BONUS; + await check_uri(REDIRECT_URI, firstRedirectBonus, 1); + nextRedirectBonus += REDIRECT_SOURCE_VISIT_BONUS; + await check_uri(INTERMEDIATE_URI_1, nextRedirectBonus, 1); + await check_uri(INTERMEDIATE_URI_2, nextRedirectBonus, 1); + // 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. + targetBonus += PERM_REDIRECT_VISIT_BONUS; + await check_uri(TARGET_URI, targetBonus, 0); + }); +}); + +add_task(async function test_subsequent_link_visit() { + // Another non typed visit. + let visitedPromise = waitVisitedNotifications(); + await BrowserTestUtils.withNewTab({ + gBrowser, + url: REDIRECT_URI.spec, + }, async function() { + info("Waiting for onVisits"); + let redirectNotified = await visitedPromise; + ok(redirectNotified, "The redirect should have been notified"); + + firstRedirectBonus += REDIRECT_SOURCE_VISIT_BONUS; + await check_uri(REDIRECT_URI, firstRedirectBonus, 1); + nextRedirectBonus += REDIRECT_SOURCE_VISIT_BONUS; + await check_uri(INTERMEDIATE_URI_1, nextRedirectBonus, 1); + await check_uri(INTERMEDIATE_URI_2, nextRedirectBonus, 1); + // 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. + targetBonus += PERM_REDIRECT_VISIT_BONUS; + await check_uri(TARGET_URI, targetBonus, 0); + }); });