bug 892454 - Use a less fragile flag than zero frecency to skip the frecency update on visit.
authorMarco Bonardo <mbonardo@mozilla.com>
Tue, 16 Jul 2013 15:36:08 +0200
changeset 138674 4912bbcb549426a73a46ce60a76e3066e45f894f
parent 138673 9db461b687fdbdf946829150ec3313ebdce6baec
child 138675 d80c320f522ab756241660665af0f8211e439fe5
push id24964
push userryanvm@gmail.com
push dateTue, 16 Jul 2013 20:04:09 +0000
treeherderautoland@fd10ead17ace [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs892454
milestone25.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 892454 - Use a less fragile flag than zero frecency to skip the frecency update on visit. r=Mano
toolkit/components/places/History.cpp
toolkit/components/places/tests/browser/Makefile.in
toolkit/components/places/tests/browser/browser_visited_notfound.js
toolkit/components/places/tests/unit/test_frecency_zero_updated.js
toolkit/components/places/tests/unit/xpcshell.ini
--- a/toolkit/components/places/History.cpp
+++ b/toolkit/components/places/History.cpp
@@ -71,31 +71,33 @@ struct VisitData {
   : placeId(0)
   , visitId(0)
   , hidden(true)
   , typed(false)
   , transitionType(UINT32_MAX)
   , visitTime(0)
   , frecency(-1)
   , titleChanged(false)
+  , shouldUpdateFrecency(true)
   {
     guid.SetIsVoid(true);
     title.SetIsVoid(true);
   }
 
   VisitData(nsIURI* aURI,
             nsIURI* aReferrer = NULL)
   : placeId(0)
   , visitId(0)
   , hidden(true)
   , typed(false)
   , transitionType(UINT32_MAX)
   , visitTime(0)
   , frecency(-1)
   , titleChanged(false)
+  , shouldUpdateFrecency(true)
   {
     (void)aURI->GetSpec(spec);
     (void)GetReversedHostname(aURI, revHost);
     if (aReferrer) {
       (void)aReferrer->GetSpec(referrerSpec);
     }
     guid.SetIsVoid(true);
     title.SetIsVoid(true);
@@ -152,16 +154,19 @@ struct VisitData {
    * should remain false.
    */
   nsString title;
 
   nsCString referrerSpec;
 
   // TODO bug 626836 hook up hidden and typed change tracking too!
   bool titleChanged;
+
+  // Indicates whether frecency should be updated for this visit.
+  bool shouldUpdateFrecency;
 };
 
 ////////////////////////////////////////////////////////////////////////////////
 //// RemoveVisitsFilter
 
 /**
  * Used to store visit filters for RemoveVisits.
  */
@@ -992,18 +997,22 @@ private:
       }
     }
 
     rv = AddVisit(aPlace, aReferrer);
     NS_ENSURE_SUCCESS(rv, rv);
 
     // TODO (bug 623969) we shouldn't update this after each visit, but
     // rather only for each unique place to save disk I/O.
-    rv = UpdateFrecency(aPlace);
-    NS_ENSURE_SUCCESS(rv, rv);
+
+    // Don't update frecency if the page should not appear in autocomplete.
+    if (aPlace.shouldUpdateFrecency) {
+      rv = UpdateFrecency(aPlace);
+      NS_ENSURE_SUCCESS(rv, rv);
+    }
 
     return NS_OK;
   }
 
   /**
    * Loads visit information about the page into _place.
    *
    * @param _place
@@ -1163,20 +1172,17 @@ private:
   /**
    * Updates the frecency, and possibly the hidden-ness of aPlace.
    *
    * @param aPlace
    *        The VisitData for the place we want to update.
    */
   nsresult UpdateFrecency(const VisitData& aPlace)
   {
-    // Don't update frecency if the page should not appear in autocomplete.
-    if (aPlace.frecency == 0) {
-      return NS_OK;
-    }
+    MOZ_ASSERT(aPlace.shouldUpdateFrecency);
 
     nsresult rv;
     { // First, set our frecency to the proper value.
       nsCOMPtr<mozIStorageStatement> stmt;
       if (aPlace.placeId) {
         stmt = mHistory->GetStatement(
           "UPDATE moz_places "
           "SET frecency = CALCULATE_FRECENCY(:page_id) "
@@ -2059,17 +2065,20 @@ History::InsertPlace(const VisitData& aP
   }
   else {
     rv = stmt->BindStringByName(NS_LITERAL_CSTRING("title"),
                                 StringHead(aPlace.title, TITLE_LENGTH_MAX));
   }
   NS_ENSURE_SUCCESS(rv, rv);
   rv = stmt->BindInt32ByName(NS_LITERAL_CSTRING("typed"), aPlace.typed);
   NS_ENSURE_SUCCESS(rv, rv);
-  rv = stmt->BindInt32ByName(NS_LITERAL_CSTRING("frecency"), aPlace.frecency);
+  // When inserting a page for a first visit that should not appear in
+  // autocomplete, for example an error page, use a zero frecency.
+  int32_t frecency = aPlace.shouldUpdateFrecency ? aPlace.frecency : 0;
+  rv = stmt->BindInt32ByName(NS_LITERAL_CSTRING("frecency"), frecency);
   NS_ENSURE_SUCCESS(rv, rv);
   rv = stmt->BindInt32ByName(NS_LITERAL_CSTRING("hidden"), aPlace.hidden);
   NS_ENSURE_SUCCESS(rv, rv);
   nsAutoCString guid(aPlace.guid);
   if (aPlace.guid.IsVoid()) {
     rv = GenerateGUID(guid);
     NS_ENSURE_SUCCESS(rv, rv);
   }
@@ -2412,17 +2421,17 @@ History::VisitURI(nsIURI* aURI,
   }
 
   place.SetTransitionType(transitionType);
   place.hidden = GetHiddenState(aFlags & IHistory::REDIRECT_SOURCE,
                                 transitionType);
 
   // Error pages should never be autocompleted.
   if (aFlags & IHistory::UNRECOVERABLE_ERROR) {
-    place.frecency = 0;
+    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.
   if (place.transitionType == nsINavHistoryService::TRANSITION_EMBED) {
     StoreAndNotifyEmbedVisit(place);
   }
   else {
--- a/toolkit/components/places/tests/browser/Makefile.in
+++ b/toolkit/components/places/tests/browser/Makefile.in
@@ -18,16 +18,17 @@ MOCHITEST_BROWSER_FILES = \
 	browser_bug646422.js \
 	browser_bug680727.js \
 	browser_colorAnalyzer.js \
 	browser_favicon_privatebrowsing_perwindowpb.js \
 	browser_favicon_setAndFetchFaviconForPage.js \
 	browser_favicon_setAndFetchFaviconForPage_failures.js \
 	browser_notfound.js \
 	browser_redirect.js \
+	browser_visited_notfound.js \
 	browser_visituri.js \
 	browser_visituri_nohistory.js \
 	browser_visituri_privatebrowsing_perwindowpb.js \
 	browser_settitle.js \
 	colorAnalyzer/category-discover.png \
 	colorAnalyzer/dictionaryGeneric-16.png \
 	colorAnalyzer/extensionGeneric-16.png \
 	colorAnalyzer/localeGeneric.png \
new file mode 100644
--- /dev/null
+++ b/toolkit/components/places/tests/browser/browser_visited_notfound.js
@@ -0,0 +1,51 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+const TEST_URI = NetUtil.newURI("http://mochi.test:8888/notFoundPage.html");
+
+function test() {
+  waitForExplicitFinish();
+
+  gBrowser.selectedTab = gBrowser.addTab();
+  registerCleanupFunction(function() {
+    gBrowser.removeCurrentTab();
+  });
+
+  // First add a visit to the page, this will ensure that later we skip
+  // updating the frecency for a newly not-found page.
+  addVisits({ uri: TEST_URI }, window, () => {
+    info("Added visit");
+    fieldForUrl(TEST_URI, "frecency", aFrecency => {
+      ok(aFrecency > 0, "Frecency should be > 0");
+      continueTest(aFrecency);
+    });
+  });
+}
+
+function continueTest(aOldFrecency) {
+  // Used to verify errors are not marked as typed.
+  PlacesUtils.history.markPageAsTyped(TEST_URI);
+  gBrowser.selectedTab.linkedBrowser.loadURI(TEST_URI.spec);
+
+  // Create and add history observer.
+  let historyObserver = {
+    __proto__: NavHistoryObserver.prototype,
+    onVisit: function (aURI, aVisitID, aTime, aSessionID, aReferringID,
+                      aTransitionType) {
+      PlacesUtils.history.removeObserver(historyObserver);
+      info("Received onVisit: " + aURI.spec);
+      fieldForUrl(aURI, "frecency", function (aFrecency) {
+        is(aFrecency, aOldFrecency, "Frecency should be unchanged");
+        fieldForUrl(aURI, "hidden", function (aHidden) {
+          is(aHidden, 0, "Page should not be hidden");
+          fieldForUrl(aURI, "typed", function (aTyped) {
+            is(aTyped, 0, "page should not be marked as typed");
+            promiseClearHistory().then(finish);
+          });
+        });
+      });
+    }
+  };
+  PlacesUtils.history.addObserver(historyObserver, false);
+}
new file mode 100644
--- /dev/null
+++ b/toolkit/components/places/tests/unit/test_frecency_zero_updated.js
@@ -0,0 +1,30 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+// Tests a zero frecency is correctly updated when inserting new valid visits.
+
+function run_test()
+{
+  run_next_test()
+}
+
+add_task(function ()
+{
+  const TEST_URI = NetUtil.newURI("http://example.com/");
+  let id = PlacesUtils.bookmarks.insertBookmark(PlacesUtils.unfiledBookmarksFolderId,
+                                                TEST_URI,
+                                                PlacesUtils.bookmarks.DEFAULT_INDEX,
+                                                "A title");
+  yield promiseAsyncUpdates();
+  do_check_true(frecencyForUrl(TEST_URI) > 0);
+
+  // Removing the bookmark should leave an orphan page with zero frecency.
+  // Note this would usually be expired later by expiration.
+  PlacesUtils.bookmarks.removeItem(id);
+  yield promiseAsyncUpdates();
+  do_check_eq(frecencyForUrl(TEST_URI), 0);
+
+  // Now add a valid visit to the page, frecency should increase.
+  yield promiseAddVisits({ uri: TEST_URI });
+  do_check_true(frecencyForUrl(TEST_URI) > 0);
+});
--- a/toolkit/components/places/tests/unit/xpcshell.ini
+++ b/toolkit/components/places/tests/unit/xpcshell.ini
@@ -67,16 +67,17 @@ skip-if = os == "android"
 [test_bug636917_isLivemark.js]
 [test_childlessTags.js]
 [test_crash_476292.js]
 [test_database_replaceOnStartup.js]
 [test_download_history.js]
 # Bug 676989: test fails consistently on Android
 fail-if = os == "android"
 [test_frecency.js]
+[test_frecency_zero_updated.js]
 # Bug 676989: test hangs consistently on Android
 skip-if = os == "android"
 [test_getChildIndex.js]
 [test_history.js]
 [test_history_autocomplete_tags.js]
 [test_history_catobs.js]
 [test_history_notifications.js]
 [test_history_observer.js]