Bug 737836 - Improve frecency handling for multiple redirect sequences, and add a test. r?mak draft
authorMark Banner <standard8@mozilla.com>
Wed, 18 Jan 2017 14:12:58 +0000
changeset 464174 ec4e5fe8c7893db92c2a6a47555a519b88d3fe77
parent 464114 21652cedf9ef0c5fea80fcf379a99cc07a27ee94
child 542883 8774ebd22686b55fb703ceb1690b165be58ba93b
push id42304
push userbmo:standard8@mozilla.com
push dateFri, 20 Jan 2017 17:22:43 +0000
reviewersmak
bugs737836
milestone53.0a1
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',