Bug 737836 - Improve frecency handling for multiple redirect sequences, and add a test. r=mak
authorMark Banner <standard8@mozilla.com>
Wed, 18 Jan 2017 14:12:58 +0000
changeset 377601 05848bf39248f1199d6877efd49c1fcbb0da4585
parent 377600 9c0c8b1a1250b7c0314368f8cba9d8380ba4fc82
child 377602 ddad29b58dc109550461240152afc473056e3c2d
push id1419
push userjlund@mozilla.com
push dateMon, 10 Apr 2017 20:44:07 +0000
treeherdermozilla-release@5e6801b73ef6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak
bugs737836
milestone53.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 737836 - Improve frecency handling for multiple redirect sequences, and add a test. r=mak MozReview-Commit-ID: ImJMF5KDvtz
browser/app/profile/firefox.js
toolkit/components/places/SQLFunctions.cpp
toolkit/components/places/tests/browser/browser.ini
toolkit/components/places/tests/browser/browser_multi_redirect_frecency.js
toolkit/components/places/tests/browser/head.js
toolkit/components/places/tests/browser/redirect_thrice.sjs
toolkit/components/places/tests/browser/redirect_twice_perma.sjs
toolkit/components/places/tests/moz.build
--- 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',