Bug 1463132 - New autofill threshold doesn't work well with redirects. r=adw draft
authorMarco Bonardo <mbonardo@mozilla.com>
Wed, 23 May 2018 16:49:06 +0200
changeset 799936 17e4cba78a34a7541d1ccc2345bff31e2d36fab5
parent 797705 dc1868d255be744a7d2d462216be205086cc60af
push id111215
push usermak77@bonardo.net
push dateFri, 25 May 2018 16:26:35 +0000
reviewersadw
bugs1463132
milestone62.0a1
Bug 1463132 - New autofill threshold doesn't work well with redirects. r=adw MozReview-Commit-ID: 9DqCWA2nGnz
docshell/base/IHistory.h
docshell/base/nsDocShell.cpp
toolkit/components/places/History.cpp
toolkit/components/places/SQLFunctions.cpp
toolkit/components/places/SQLFunctions.h
toolkit/components/places/tests/browser/browser_multi_redirect_frecency.js
--- a/docshell/base/IHistory.h
+++ b/docshell/base/IHistory.h
@@ -66,32 +66,38 @@ public:
 
   enum VisitFlags
   {
     /**
      * Indicates whether the URI was loaded in a top-level window.
      */
     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
   };
 
   /**
    * Adds a history visit for the URI.
    *
    * @pre aURI must not be null.
    *
    * @param aURI
--- a/docshell/base/nsDocShell.cpp
+++ b/docshell/base/nsDocShell.cpp
@@ -12621,16 +12621,19 @@ nsDocShell::AddURIVisit(nsIURI* aURI,
     } else {
       MOZ_ASSERT(!aChannelRedirectFlags,
                  "One of REDIRECT_TEMPORARY or REDIRECT_PERMANENT must be set "
                  "if any flags in aChannelRedirectFlags is set.");
     }
 
     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.
     // 408 is special cased, since may actually indicate a temporary
     // connection problem.
     else if (aResponseStatus != 408 &&
              ((aResponseStatus >= 400 && aResponseStatus <= 501) ||
               aResponseStatus == 505)) {
--- a/toolkit/components/places/History.cpp
+++ b/toolkit/components/places/History.cpp
@@ -78,17 +78,17 @@ struct VisitData {
   , visitTime(0)
   , frecency(-1)
   , lastVisitId(0)
   , lastVisitTime(0)
   , visitCount(0)
   , referrerVisitId(0)
   , titleChanged(false)
   , shouldUpdateFrecency(true)
-  , redirect(false)
+  , useFrecencyRedirectBonus(false)
   {
     guid.SetIsVoid(true);
     title.SetIsVoid(true);
   }
 
   explicit VisitData(nsIURI* aURI,
                      nsIURI* aReferrer = nullptr)
   : placeId(0)
@@ -100,17 +100,17 @@ struct VisitData {
   , visitTime(0)
   , frecency(-1)
   , lastVisitId(0)
   , lastVisitTime(0)
   , visitCount(0)
   , referrerVisitId(0)
   , titleChanged(false)
   , shouldUpdateFrecency(true)
-  , redirect(false)
+  , useFrecencyRedirectBonus(false)
   {
     MOZ_ASSERT(aURI);
     if (aURI) {
       (void)aURI->GetSpec(spec);
       (void)GetReversedHostname(aURI, revHost);
     }
     if (aReferrer) {
       (void)aReferrer->GetSpec(referrerSpec);
@@ -159,18 +159,19 @@ struct VisitData {
   int64_t referrerVisitId;
 
   // TODO bug 626836 hook up hidden and typed change tracking too!
   bool titleChanged;
 
   // 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;
 };
 
 ////////////////////////////////////////////////////////////////////////////////
 //// nsVisitData
 
 class nsVisitData : public nsIVisitData
 {
 public:
@@ -1484,18 +1485,17 @@ private:
           "WHERE id = :page_id"
         );
       }
       NS_ENSURE_STATE(stmt);
       mozStorageStatementScoper scoper(stmt);
 
       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();
       NS_ENSURE_SUCCESS(rv, rv);
     }
 
     if (!aPlace.hidden && aPlace.shouldUpdateHidden) {
       // Mark the page as not hidden if the frecency is now nonzero.
@@ -2835,18 +2835,23 @@ History::VisitURI(nsIURI* aURI,
     transitionType = nsINavHistoryService::TRANSITION_BOOKMARK;
   }
   else if (!(aFlags & IHistory::TOP_LEVEL) && isFollowedLink) {
     // User activated a link in a frame.
     transitionType = nsINavHistoryService::TRANSITION_FRAMED_LINK;
   }
 
   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) {
     place.shouldUpdateFrecency = false;
   }
 
   // EMBED visits are session-persistent and should not go through the database.
   // They exist only to keep track of isVisited status during the session.
--- a/toolkit/components/places/SQLFunctions.cpp
+++ b/toolkit/components/places/SQLFunctions.cpp
@@ -671,26 +671,26 @@ namespace places {
 
     int64_t pageId = aArguments->AsInt64(0);
     MOZ_ASSERT(pageId > 0, "Should always pass a valid page id");
     if (pageId <= 0) {
       NS_ADDREF(*_result = new IntegerVariant(0));
       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;
     int32_t visitCount = 0;
     bool hasBookmark = false;
     int32_t isQuery = 0;
     float pointsForSampledVisits = 0.0;
     int32_t numSampledVisits = 0;
@@ -730,29 +730,29 @@ namespace places {
       NS_ENSURE_SUCCESS(rv, rv);
       hasBookmark = foreignCount > 0;
       rv = getPageInfo->GetInt32(3, &isQuery);
       NS_ENSURE_SUCCESS(rv, rv);
     }
 
     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);
       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), "
-            "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 "
             ) + redirectsTransitionFragment + NS_LITERAL_CSTRING(
           "LEFT JOIN moz_historyvisits target ON v.id = target.from_visit "
                                             "AND target.visit_type BETWEEN "
             ) + redirectsTransitionFragment + NS_LITERAL_CSTRING(
           "WHERE v.place_id = :page_id "
@@ -765,54 +765,42 @@ 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;
-        bool isNull = false;
-        rv = getVisits->GetIsNull(1, &isNull);
-        NS_ENSURE_SUCCESS(rv, rv);
+        // If this is a redirect target, we'll use the visitType of the source,
+        // otherwise the actual visitType.
+        int32_t visitType = getVisits->AsInt32(0);
 
-        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) {
           bonus += history->GetFrecencyTransitionBonus(nsINavHistoryService::TRANSITION_BOOKMARK, true);
         }
 
         // 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));
         }
       }
     }
 
     // If we sampled some visits for this page, use the calculated weight.
     if (numSampledVisits) {
--- a/toolkit/components/places/SQLFunctions.h
+++ b/toolkit/components/places/SQLFunctions.h
@@ -193,18 +193,19 @@ private:
  *
  * In SQL, you'd use it in when setting frecency like:
  * SET frecency = CALCULATE_FRECENCY(place_id).
  * Optional parameters must be passed in if the page is not yet in the database,
  * otherwise they will be fetched from it automatically.
  *
  * @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
 {
 public:
   NS_DECL_THREADSAFE_ISUPPORTS
   NS_DECL_MOZISTORAGEFUNCTION
 
   /**
--- a/toolkit/components/places/tests/browser/browser_multi_redirect_frecency.js
+++ b/toolkit/components/places/tests/browser/browser_multi_redirect_frecency.js
@@ -6,16 +6,18 @@ const REDIRECT_URI = Services.io.newURI(
 const INTERMEDIATE_URI_1 = Services.io.newURI(ROOT_URI + "redirect_twice_perma.sjs");
 const INTERMEDIATE_URI_2 = Services.io.newURI(ROOT_URI + "redirect_once.sjs");
 const TARGET_URI = Services.io.newURI(ROOT_URI + "final.html");
 
 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).
 Services.prefs.setCharPref("places.frecency.decayRate", "1.0");
 
 registerCleanupFunction(async function() {
   Services.prefs.clearUserPref("places.frecency.decayRate");
   await PlacesUtils.history.clear();
@@ -23,111 +25,123 @@ registerCleanupFunction(async function()
 
 async function check_uri(uri, frecency, hidden) {
   is(await PlacesTestUtils.fieldInDB(uri, "frecency"), frecency,
     "Frecency of the page is the expected one");
   is(await PlacesTestUtils.fieldInDB(uri, "hidden"), 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);
     if (uri.equals(REDIRECT_URI)) {
       redirectNotified = true;
     }
     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);
-
-  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 = 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");
 
-  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 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;
+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");
 
-  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");
+    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);
+  });
+});
 
-  let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, REDIRECT_URI.spec);
-  info("Waiting for onVisits");
-  await visitedPromise;
-  ok(redirectNotified, "The redirect should have been notified");
+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");
 
-  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 += 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);
+  });
 });