Backing out bug 402880

browser/app/profile/firefox.js

@@ -258,8 +258,7 @@ pref("browser.history.grouping", "day");
 pref("browser.history.showSessions", false);
 pref("browser.sessionhistory.max_entries", 50);
 pref("browser.history_expire_days", 180);
-pref("browser.history_expire_days_min", 90);
+pref("browser.history_expire_visits", 20000);
-pref("browser.history_expire_sites", 40000);
 
 // handle external links
 // 0=default window, 1=current window/tab, 2=new window, 3=new tab in most recent window

browser/components/preferences/privacy.js

@@ -56,20 +56,11 @@ var gPrivacyPane = {
   /*
    * Preferences:
    *
-   * NOTE: These first two are no longer shown in the UI. They're controlled
-   *       via the checkbox, which uses the zero state of the pref to turn
-   *       history off.
    * browser.history_expire_days
    * - the number of days of history to remember
    * browser.history_expire_days.mirror
    * - a preference whose value mirrors that of browser.history_expire_days, to
    *   make the "days of history" checkbox easier to code
-   *
-   * browser.history_expire_days_min
-   * - the mininum number of days of history to remember
-   * browser.history_expire_days_min.mirror
-   * - a preference whose value mirrors that of browser.history_expire_days_min
-   *   to make the "days of history" checkbox easier to code
    * browser.formfill.enable
    * - true if entries in forms and the search bar should be saved, false
    *   otherwise
@@ -91,7 +82,6 @@ var gPrivacyPane = {
   {
     var pref = document.getElementById("browser.history_expire_days");
     var mirror = document.getElementById("browser.history_expire_days.mirror");
-    var pref_min = document.getElementById("browser.history_expire_days_min");
     var textbox = document.getElementById("historyDays");
     var checkbox = document.getElementById("rememberHistoryDays");
 
@@ -101,6 +91,25 @@ var gPrivacyPane = {
 
     checkbox.checked = (pref.value > 0);
     textbox.disabled = !checkbox.checked;
+
+    // hook up textbox to mirror preference and force a preference read
+    textbox.setAttribute("onsynctopreference", "return gPrivacyPane._writeHistoryDaysMirror();");
+    textbox.setAttribute("preference", "browser.history_expire_days.mirror");
+    mirror.updateElements();
+  },
+
+  /**
+   * Stores the days of history to the actual days-of-history preference and
+   * returns that value, to be stored in the mirror preference.
+   */
+  _writeHistoryDaysMirror: function ()
+  {
+    var pref = document.getElementById("browser.history_expire_days");
+    var textbox = document.getElementById("historyDays");
+    pref.value = textbox.value;
+
+    // don't override the value in the textbox
+    return undefined;
   },
 
   /**
@@ -119,19 +128,6 @@ var gPrivacyPane = {
     textbox.disabled = !checkbox.checked;
   },
 
-  /**
-   * Responds to changes in the days-of-history textbox,
-   * unchecking the history-enabled checkbox if the days
-   * value is zero.
-   */
-  onkeyupHistoryDaysText: function ()
-  {
-    var textbox = document.getElementById("historyDays");
-    var checkbox = document.getElementById("rememberHistoryDays");
-    
-    checkbox.checked = textbox.value != 0;
-  },
-
   /**
    * Converts the value of the browser.download.manager.retention preference
    * into a Boolean value.  "remove on close" and "don't remember" both map

browser/components/preferences/privacy.xul

@@ -70,9 +70,6 @@
       <preference id="browser.history_expire_days.mirror"
                   name="browser.history_expire_days.mirror"
                   type="int"/>
-      <preference id="browser.history_expire_days_min"
-                  name="browser.history_expire_days_min"
-                  type="int"/>
       <preference id="browser.formfill.enable"
                   name="browser.formfill.enable"
                   type="bool"/>
@@ -107,15 +104,13 @@
 
       <hbox align="center">
         <checkbox id="rememberHistoryDays"
-                  label="&rememberDaysBefore.label;"
+                  label="&rememberBefore.label;"
-                  accesskey="&rememberDaysBefore.accesskey;"
+                  accesskey="&rememberBefore.accesskey;"
                   oncommand="gPrivacyPane.onchangeHistoryDaysCheck();"
                   aaa:labelledby="rememberHistoryDays historyDays rememberAfter"/>
         <textbox id="historyDays" type="number" size="3"
-                 aaa:labelledby="rememberHistoryDays historyDays rememberAfter"
+                 aaa:labelledby="rememberHistoryDays historyDays rememberAfter"/>
-                 onkeyup="gPrivacyPane.onkeyupHistoryDaysText();"
+        <label id="rememberAfter"> &rememberAfter.label;</label>
-                 preference="browser.history_expire_days_min"/>
-        <label id="rememberAfter"> &rememberDaysAfter.label;</label>
       </hbox>
       <checkbox id="rememberForms"
                 label="&rememberForms.label;"

browser/locales/en-US/chrome/browser/preferences/privacy.dtd

@@ -1,14 +1,14 @@
 <!ENTITY  history.label "History">
 
 <!-- LOCALIZATION NOTE:
-  The entities rememberDaysBefore.label and rememberDaysAfter.label appear on a single
+  The entities rememberBefore.label and rememberAfter.label appear on a single
   line in preferences as follows:
 
-  &rememberDaysBefore.label  [ textbox for number of days ]   &rememberDaysAfter.label;
+  &rememberBefore.label  [ textbox for number of days ]   &rememberAfter.label;
 -->
-<!ENTITY  rememberDaysBefore.label      "Keep my history for at least">
+<!ENTITY  rememberBefore.label          "Remember visited pages for the last">
-<!ENTITY  rememberDaysBefore.accesskey  "h">
+<!ENTITY  rememberBefore.accesskey      "v">
-<!ENTITY  rememberDaysAfter.label       "days.">
+<!ENTITY  rememberAfter.label           "days.">
 
 <!ENTITY  rememberForms.label           "Remember what I enter in forms and the search bar">
 <!ENTITY  rememberForms.accesskey       "f">

toolkit/components/places/src/nsNavHistory.cpp

@@ -104,7 +104,6 @@
 
 // preference ID strings
 #define PREF_BRANCH_BASE                        "browser."
-#define PREF_BROWSER_HISTORY_EXPIRE_DAYS_MIN    "history_expire_days_min"
 #define PREF_BROWSER_HISTORY_EXPIRE_DAYS        "history_expire_days"
 #define PREF_BROWSER_HISTORY_EXPIRE_VISITS      "history_expire_visits"
 #define PREF_AUTOCOMPLETE_ONLY_TYPED            "urlbar.matchOnlyTyped"
@@ -173,7 +172,7 @@
 #define MAX_EXPIRE_RECORDS_ON_IDLE 200
 
 // Limit the number of items in the history for performance reasons
-#define EXPIRATION_CAP_SITES 40000
+#define EXPIRATION_CAP_VISITS 20000
 
 NS_IMPL_ADDREF(nsNavHistory)
 NS_IMPL_RELEASE(nsNavHistory)
@@ -272,9 +271,8 @@ nsNavHistory* nsNavHistory::gHistoryService;
 nsNavHistory::nsNavHistory() : mNowValid(PR_FALSE),
                                mExpireNowTimer(nsnull),
                                mExpire(this),
-                               mExpireDaysMin(0),
+                               mExpireDays(0),
-                               mExpireDaysMax(0),
+                               mExpireVisits(0),
-                               mExpireSites(0),
                                mAutoCompleteOnlyTyped(PR_FALSE),
                                mBatchLevel(0),
                                mLock(nsnull),
@@ -412,9 +410,8 @@ nsNavHistory::Init()
   nsCOMPtr<nsIPrefBranch2> pbi = do_QueryInterface(mPrefBranch);
   if (pbi) {
     pbi->AddObserver(PREF_AUTOCOMPLETE_ONLY_TYPED, this, PR_FALSE);
-    pbi->AddObserver(PREF_BROWSER_HISTORY_EXPIRE_DAYS_MAX, this, PR_FALSE);
+    pbi->AddObserver(PREF_BROWSER_HISTORY_EXPIRE_DAYS, this, PR_FALSE);
-    pbi->AddObserver(PREF_BROWSER_HISTORY_EXPIRE_DAYS_MIN, this, PR_FALSE);
+    pbi->AddObserver(PREF_BROWSER_HISTORY_EXPIRE_VISITS, this, PR_FALSE);
-    pbi->AddObserver(PREF_BROWSER_HISTORY_EXPIRE_SITES, this, PR_FALSE);
   }
 
   observerService->AddObserver(this, gQuitApplicationMessage, PR_FALSE);
@@ -1420,11 +1417,10 @@ nsNavHistory::LoadPrefs()
   if (! mPrefBranch)
     return NS_OK;
 
-  mPrefBranch->GetIntPref(PREF_BROWSER_HISTORY_EXPIRE_DAYS_MAX, &mExpireDaysMax);
+  mPrefBranch->GetIntPref(PREF_BROWSER_HISTORY_EXPIRE_DAYS, &mExpireDays);
-  mPrefBranch->GetIntPref(PREF_BROWSER_HISTORY_EXPIRE_DAYS_MIN, &mExpireDaysMin);
+  if (NS_FAILED(mPrefBranch->GetIntPref(PREF_BROWSER_HISTORY_EXPIRE_VISITS,
-  if (NS_FAILED(mPrefBranch->GetIntPref(PREF_BROWSER_HISTORY_EXPIRE_SITES,
+                                        &mExpireVisits)))
-                                        &mExpireSites)))
+    mExpireVisits = EXPIRATION_CAP_VISITS;
-    mExpireSites = EXPIRATION_CAP_SITES;
   
   PRBool oldCompleteOnlyTyped = mAutoCompleteOnlyTyped;
   mPrefBranch->GetBoolPref(PREF_AUTOCOMPLETE_ONLY_TYPED,
@@ -1940,6 +1936,7 @@ nsNavHistory::AddVisit(nsIURI* aURI, PRTime aTime, PRInt64 aReferringVisit,
   PRBool newItem = PR_FALSE; // used to send out notifications at the end
   if (alreadyVisited) {
     // Update the existing entry...
+
     rv = mDBGetPageVisitStats->GetInt64(0, &pageID);
     NS_ENSURE_SUCCESS(rv, rv);
 
@@ -2613,6 +2610,8 @@ nsNavHistory::GetHistoryDisabled(PRBool *_retval)
 //
 //    Note that this always adds the page with one visit and no parent, which
 //    is appropriate for imported URIs.
+//
+//    UNTESTED
 
 NS_IMETHODIMP
 nsNavHistory::AddPageWithDetails(nsIURI *aURI, const PRUnichar *aTitle,
@@ -3521,12 +3520,10 @@ nsNavHistory::Observe(nsISupports *aSubject, const char *aTopic,
     observerService->RemoveObserver(this, gXpcomShutdown);
     observerService->RemoveObserver(this, gQuitApplicationMessage);
   } else if (nsCRT::strcmp(aTopic, "nsPref:changed") == 0) {
-    PRInt32 oldDaysMin = mExpireDaysMin;
+    PRInt32 oldDays = mExpireDays;
-    PRInt32 oldDaysMax = mExpireDaysMax;
+    PRInt32 oldVisits = mExpireVisits;
-    PRInt32 oldVisits = mExpireSites;
     LoadPrefs();
-    if (oldDaysMin != mExpireDaysMin || oldDaysMax != mExpireDaysMax ||
+    if (oldDays != mExpireDays || oldVisits != mExpireVisits)
-        oldVisits != mExpireSites)
       mExpire.OnExpirationChanged();
   }
 
@@ -4564,7 +4561,7 @@ nsNavHistory::FilterResultSet(nsNavHistoryQueryResultNode* aQueryNode,
 //    Sees if this URL happened "recently."
 //
 //    It is always removed from our recent list no matter what. It only counts
-//    as "recent" if the event happened more recently than our event
+//    as "recent" if the event happend more recently than our event
 //    threshold ago.
 
 PRBool
@@ -5308,9 +5305,9 @@ GetSimpleBookmarksQueryFolder(const nsCOMArray<nsNavHistoryQuery>& aQueries,
 //
 //    Construct a matrix of search terms from the given queries array.
 //    All of the query objects are ORed together. Within a query, all the terms
-//    are ANDed together. See nsINavHistoryService.idl.
+//    are ANDed together. See nsINavHistory.idl.
 //
-//    This just breaks the query up into words. We don't do anything fancy,
+//    This just breaks the quer up into words. We don't do anything fancy,
 //    not even quoting. We do, however, strip quotes, because people might
 //    try to input quotes expecting them to do something and get no results
 //    back.

toolkit/components/places/src/nsNavHistory.h

@@ -203,7 +203,7 @@ public:
     { return mDateFormatter; }
 
   // returns true if history has been disabled
-  PRBool IsHistoryDisabled() { return mExpireDaysMax == 0; }
+  PRBool IsHistoryDisabled() { return mExpireDays == 0; }
 
   // get the statement for selecting a history row by URL
   mozIStorageStatement* DBGetURLPageInfo() { return mDBGetURLPageInfo; }
@@ -608,9 +608,8 @@ protected:
   static void AutoCompleteTimerCallback(nsITimer* aTimer, void* aClosure);
   void DoneSearching();
 
-  PRInt32 mExpireDaysMin;
+  PRInt32 mExpireDays;
-  PRInt32 mExpireDaysMax;
+  PRInt32 mExpireVisits;
-  PRInt32 mExpireSites;
 
   // in nsNavHistoryQuery.cpp
   nsresult TokensToQueries(const nsTArray<QueryKeyValuePair>& aTokens,

toolkit/components/places/src/nsNavHistoryExpire.cpp

@@ -98,7 +98,7 @@ const PRTime EXPIRATION_POLICY_WEEKS = ((PRTime)30 * 86400 * PR_USEC_PER_SEC);
 const PRTime EXPIRATION_POLICY_MONTHS = ((PRTime)180 * 86400 * PR_USEC_PER_SEC);
 
 // Expiration policy for embedded links (bug #401722)
-const PRTime EMBEDDED_LINK_LIFETIME = ((PRTime)1 * 86400 * PR_USEC_PER_SEC);
+const PRTime EMBEDDED_LINK_LIFETIME = ((PRTime)10 * 86400 * PR_USEC_PER_SEC);
 
 // Expiration cap for embedded visits
 #define EXPIRATION_CAP_EMBEDDED 500
@@ -118,11 +118,13 @@ const PRTime EMBEDDED_LINK_LIFETIME = ((PRTime)1 * 86400 * PR_USEC_PER_SEC);
 
 nsNavHistoryExpire::nsNavHistoryExpire(nsNavHistory* aHistory) :
     mHistory(aHistory),
+    mSequentialRuns(0),
     mTimerSet(PR_FALSE),
     mAnyEmptyRuns(PR_FALSE),
     mNextExpirationTime(0),
     mAddCount(0),
-    mExpiredItems(0)
+    mExpiredItems(0),
+    mExpireRuns(0)
 {
 
 }
@@ -149,6 +151,7 @@ void
 nsNavHistoryExpire::OnAddURI(PRTime aNow)
 {
   mAddCount ++;
+  mSequentialRuns = 0;
 
   if (mTimer && mTimerSet) {
     mTimer->Cancel();
@@ -238,10 +241,11 @@ nsNavHistoryExpire::OnQuit()
 nsresult
 nsNavHistoryExpire::ClearHistory()
 {
+  PRBool keepGoing;
+
   mozIStorageConnection* connection = mHistory->GetStorageConnection();
   NS_ENSURE_TRUE(connection, NS_ERROR_OUT_OF_MEMORY);
 
-  PRBool keepGoing;
   nsresult rv = ExpireItems(0, &keepGoing);
   if (NS_FAILED(rv))
     NS_WARNING("ExpireItems failed.");
@@ -285,13 +289,15 @@ nsNavHistoryExpire::OnExpirationChanged()
 nsresult
 nsNavHistoryExpire::DoPartialExpiration()
 {
+  mSequentialRuns ++;
+
   // expire history items
   PRBool keepGoing;
   nsresult rv = ExpireItems(EXPIRATION_COUNT_PER_RUN, &keepGoing);
   if (NS_FAILED(rv))
     NS_WARNING("ExpireItems failed.");
 
-  if (keepGoing)
+  if (keepGoing && mSequentialRuns < MAX_SEQUENTIAL_RUNS)
     StartTimer(SUBSEQUENT_EXPIRATION_TIMEOUT);
   return NS_OK;
 }
@@ -310,6 +316,9 @@ nsNavHistoryExpire::DoPartialExpiration()
 nsresult
 nsNavHistoryExpire::ExpireItems(PRUint32 aNumToExpire, PRBool* aKeepGoing)
 {
+  // mark how many times we've been able to run
+  mExpireRuns ++;
+
   mozIStorageConnection* connection = mHistory->GetStorageConnection();
   NS_ENSURE_TRUE(connection, NS_ERROR_OUT_OF_MEMORY);
 
@@ -411,12 +420,8 @@ nsNavHistoryExpireRecord::nsNavHistoryExpireRecord(
 
 // nsNavHistoryExpire::FindVisits
 //
-//    Find visits to expire, meeting the following criteria:
+//    Find visits to expire, up to the cap in browser.history_expire_visits
-//
+//    then the age in browser.history_expire_days.
-//    * With a visit date greater than (now - browser.history_expire_days_min)
-//    * With a visit date less than (now - browser.history_expire_days)
-//    * With a visit date greater than the minimum, and less than the maximum,
-//      and over the visit cap of browser.history_expire_sites.
 //
 //    aExpireThreshold is the time at which we will delete visits before.
 //    If it is zero, we will not use a threshold and will match everything.
@@ -429,67 +434,70 @@ nsNavHistoryExpire::FindVisits(PRTime aExpireThreshold, PRUint32 aNumToExpire,
                                mozIStorageConnection* aConnection,
                                nsTArray<nsNavHistoryExpireRecord>& aRecords)
 {
-  printf("FindVisits(%lld, %d)\n", aExpireThreshold, aNumToExpire);
+  // get default browser.history_expire_days value
-  // Select moz_places records, including whether visited and whether the URI
+  nsresult rv;
-  // is bookmarked or not.
+  nsCOMPtr<nsIPrefService> prefService =
+    do_GetService(NS_PREFSERVICE_CONTRACTID, &rv);
+  NS_ENSURE_SUCCESS(rv, rv);
+  nsCOMPtr<nsIPrefBranch> defaultPrefBranch;
+  rv = prefService->GetDefaultBranch(PREF_BRANCH_BASE,
+    getter_AddRefs(defaultPrefBranch));
+  NS_ENSURE_SUCCESS(rv, rv);
+  PRInt32 defaultExpireDays;
+  rv = defaultPrefBranch->GetIntPref(PREF_BROWSER_HISTORY_EXPIRE_DAYS, &defaultExpireDays);
+  NS_ENSURE_SUCCESS(rv, rv);
+
+  // base sql
   nsCAutoString sqlBase;
   sqlBase.AssignLiteral(
     "SELECT v.id, v.place_id, v.visit_date, h.url, h.favicon_id, h.hidden, b.fk "
-    "FROM moz_places h LEFT OUTER JOIN moz_historyvisits v ON h.id = v.place_id "
+    "FROM moz_historyvisits v LEFT JOIN moz_places h ON v.place_id = h.id "
-    "LEFT OUTER JOIN moz_bookmarks b on h.id = b.fk ");
+    "LEFT OUTER JOIN moz_bookmarks b on v.place_id = b.fk ");
-
-  // 1. Expire records older than the max-age cap
-  nsCAutoString sqlMaxAge;
-  sqlMaxAge.Assign(sqlBase);
 
+  // If history_expire_days is the default then go ahead and expire up to the
+  // visit cap, else we respect the user's value, or if we're clearing history.
+  if (defaultExpireDays == mHistory->mExpireDays || !aNumToExpire) {
+    // build capped query
+    nsCOMPtr<mozIStorageStatement> visitsStatement;
+    nsCAutoString sqlVisits;
+    sqlVisits.Assign(sqlBase);
     if (aNumToExpire) {
-    // Select records older than the max-age cap
+      // Setting the visit cap as the OFFSET value selects the next aNumToExpire
-    sqlMaxAge.AppendLiteral("WHERE v.visit_date < ?1 "
+      // records above the cap.
-                            "ORDER BY v.visit_date DESC LIMIT ?2");
+      sqlVisits.AppendLiteral("ORDER BY v.visit_date DESC LIMIT ?1 OFFSET ?2 ");
     }
-
+    rv = aConnection->CreateStatement(sqlVisits, getter_AddRefs(visitsStatement));
-  nsCOMPtr<mozIStorageStatement> selectStatement;
-  nsresult rv = aConnection->CreateStatement(sqlMaxAge, getter_AddRefs(selectStatement));
     NS_ENSURE_SUCCESS(rv, rv);
 
     if (aNumToExpire) {
-    rv = selectStatement->BindInt64Parameter(0, aExpireThreshold);
+      rv = visitsStatement->BindInt64Parameter(0, aNumToExpire);
       NS_ENSURE_SUCCESS(rv, rv);
-    rv = selectStatement->BindInt32Parameter(1, aNumToExpire);
+      rv = visitsStatement->BindInt32Parameter(1, mHistory->mExpireVisits);
       NS_ENSURE_SUCCESS(rv, rv);
     }
 
     PRBool hasMore = PR_FALSE;
-  while (NS_SUCCEEDED(selectStatement->ExecuteStep(&hasMore)) && hasMore) {
+    while (NS_SUCCEEDED(visitsStatement->ExecuteStep(&hasMore)) && hasMore) {
-    nsNavHistoryExpireRecord record(selectStatement);
+      nsNavHistoryExpireRecord record(visitsStatement);
       aRecords.AppendElement(record);
     }
+  }
 
-  // 2. if no over-max-age records are found, select records older than the min-age cap AND over the sites cap.
+  if (aExpireThreshold && aRecords.Length() < aNumToExpire) {
-  if (!aRecords.Length()) {
+    nsCOMPtr<mozIStorageStatement> selectStatement;
-    nsCAutoString sqlMinAge;
+    nsCAutoString sqlDate;
-    sqlMinAge.Assign(sqlBase);
+    sqlDate.Assign(sqlBase);
-
+    sqlDate.AppendLiteral("WHERE v.visit_date < ?1 LIMIT ?2");
-    // Select records older than the max-age cap
+    rv = aConnection->CreateStatement(sqlDate, getter_AddRefs(selectStatement));
-    // Setting the visit cap as the OFFSET value selects the next aNumToExpire
+    NS_ENSURE_SUCCESS(rv, rv);
-    // records above the cap.
+    rv = selectStatement->BindInt64Parameter(0, aExpireThreshold);
-    sqlMinAge.AppendLiteral("WHERE v.visit_date < ?1 "
+    NS_ENSURE_SUCCESS(rv, rv);
-                            "ORDER BY v.visit_date DESC LIMIT ?2 OFFSET ?3");
+    rv = selectStatement->BindInt32Parameter(1, aNumToExpire - aRecords.Length());
-
-    nsCOMPtr<mozIStorageStatement> selectMinStatement;
-    nsresult rv = aConnection->CreateStatement(sqlMinAge, getter_AddRefs(selectMinStatement));
     NS_ENSURE_SUCCESS(rv, rv);
 
-    rv = selectMinStatement->BindInt64Parameter(0, aExpireThreshold);
+    PRBool hasMore = PR_FALSE;
-    NS_ENSURE_SUCCESS(rv, rv);
+    while (NS_SUCCEEDED(selectStatement->ExecuteStep(&hasMore)) && hasMore) {
-    rv = selectMinStatement->BindInt32Parameter(1, aNumToExpire);
+      nsNavHistoryExpireRecord record(selectStatement);
-    NS_ENSURE_SUCCESS(rv, rv);
-    rv = selectMinStatement->BindInt32Parameter(2, mHistory->mExpireSites);
-    NS_ENSURE_SUCCESS(rv, rv);
-
-    hasMore = PR_FALSE;
-    while (NS_SUCCEEDED(selectMinStatement->ExecuteStep(&hasMore)) && hasMore) {
-      nsNavHistoryExpireRecord record(selectMinStatement);
       aRecords.AppendElement(record);
     }
   }
@@ -796,16 +804,11 @@ nsNavHistoryExpire::ExpireAnnotationsParanoid(mozIStorageConnection* aConnection
   NS_ENSURE_SUCCESS(rv, rv);
 
   // delete all uri annos w/o a corresponding place id
-  // or without any visits *and* not EXPIRE_NEVER.
   rv = aConnection->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
     "DELETE FROM moz_annos WHERE id IN "
       "(SELECT a.id FROM moz_annos a "
       "LEFT OUTER JOIN moz_places p ON a.place_id = p.id "
-      "LEFT OUTER JOIN moz_historyvisits v ON a.place_id = v.place_id "
+      "WHERE p.id IS NULL)"));
-      "WHERE p.id IS NULL "
-      "OR (v.id IS NULL AND a.expiration != ") +
-      nsPrintfCString("%d", nsIAnnotationService::EXPIRE_NEVER) +
-      NS_LITERAL_CSTRING("))"));
   NS_ENSURE_SUCCESS(rv, rv);
 
   // delete item annos w/o a corresponding item id
@@ -918,7 +921,7 @@ nsNavHistoryExpire::TimerCallback(nsITimer* aTimer, void* aClosure)
 PRTime
 nsNavHistoryExpire::GetExpirationTimeAgo()
 {
-  PRInt64 expireDays = mHistory->mExpireDaysMax;
+  PRInt64 expireDays = mHistory->mExpireDays;
 
   // Prevent Int64 overflow for people that type in huge numbers.
   // This number is 2^63 / 24 / 60 / 60 / 1000000 (reversing the math below)

toolkit/components/places/src/nsNavHistoryExpire.h

@@ -63,6 +63,9 @@ protected:
 
   nsNavHistory* mHistory;
 
+  // Number of partial expirations since last AddURI call.
+  PRUint32 mSequentialRuns;
+
   nsCOMPtr<nsITimer> mTimer;
   PRBool mTimerSet;
 
@@ -80,6 +83,7 @@ protected:
   // global statistics
   PRUint32 mAddCount;
   PRUint32 mExpiredItems;
+  PRUint32 mExpireRuns;
 
   nsresult DoPartialExpiration();
 

toolkit/components/places/tests/unit/test_expiration.js

@@ -109,7 +109,7 @@ var dbConnection = dbService.openDatabase(dbFile);
 var testURI = uri("http://mozilla.com");
 var testAnnoName = "tests/expiration/history";
 var testAnnoVal = "foo";
-var bookmark = bmsvc.insertBookmark(bmsvc.bookmarksMenuFolder, testURI, bmsvc.DEFAULT_INDEX, "foo");
+var bookmark = bmsvc.insertBookmark(bmsvc.bookmarksRoot, testURI, bmsvc.DEFAULT_INDEX, "foo");
 var triggerURI = uri("http://foobar.com");
 
 // main
@@ -118,7 +118,7 @@ function run_test() {
   test that nsIBrowserHistory.removePagesFromHost does remove expirable annotations
   but doesn't remove bookmarks or EXPIRE_NEVER annotations.
   */
-  histsvc.addVisit(testURI, Date.now() * 1000, 0, histsvc.TRANSITION_TYPED, false, 0);
+  histsvc.addVisit(testURI, Date.now(), 0, histsvc.TRANSITION_TYPED, false, 0);
   annosvc.setPageAnnotation(testURI, testAnnoName + "Hist", testAnnoVal, 0, annosvc.EXPIRE_WITH_HISTORY);
   annosvc.setPageAnnotation(testURI, testAnnoName + "Never", testAnnoVal, 0, annosvc.EXPIRE_NEVER);
   bhist.removePagesFromHost("mozilla.com", false);
@@ -136,9 +136,9 @@ function run_test() {
   */
   var removeAllTestURI = uri("http://removeallpages.com");
   var removeAllTestURINever = uri("http://removeallpagesnever.com");
-  histsvc.addVisit(removeAllTestURI, Date.now() * 1000, 0, histsvc.TRANSITION_TYPED, false, 0);
+  histsvc.addVisit(removeAllTestURI, Date.now(), 0, histsvc.TRANSITION_TYPED, false, 0);
   var bmURI = uri("http://bookmarked");
-  bmsvc.insertBookmark(bmsvc.bookmarksMenuFolder, bmURI, bmsvc.DEFAULT_INDEX, "foo");
+  bmsvc.insertBookmark(bmsvc.bookmarksRoot, bmURI, bmsvc.DEFAULT_INDEX, "foo");
   //bhist.addPageWithDetails(placeURI, "place uri", Date.now());
   var placeURI = uri("place:folder=23");
   bhist.addPageWithDetails(placeURI, "place uri", Date.now());
@@ -167,7 +167,7 @@ function run_test() {
   /*
   test anno expiration (expire never)
   */
-  histsvc.addVisit(testURI, Date.now() * 1000, 0, histsvc.TRANSITION_TYPED, false, 0);
+  histsvc.addVisit(testURI, Date.now(), 0, histsvc.TRANSITION_TYPED, false, 0);
   annosvc.setPageAnnotation(testURI, testAnnoName, testAnnoVal, 0, annosvc.EXPIRE_NEVER);
   annosvc.setItemAnnotation(bookmark, testAnnoName, testAnnoVal, 0, annosvc.EXPIRE_NEVER);
   histsvc.removeAllPages();
@@ -179,7 +179,7 @@ function run_test() {
   /*
   test anno expiration (expire with history)
   */
-  histsvc.addVisit(testURI, Date.now() * 1000, 0, histsvc.TRANSITION_TYPED, false, 0);
+  histsvc.addVisit(testURI, Date.now(), 0, histsvc.TRANSITION_TYPED, false, 0);
   annosvc.setPageAnnotation(testURI, testAnnoName, testAnnoVal, 0, annosvc.EXPIRE_WITH_HISTORY);
   histsvc.removeAllPages();
   try {
@@ -195,7 +195,7 @@ function run_test() {
     - try to get the anno (should fail. maybe race here? is there a way to determine
       if the page has been added, so we know that expiration is done?)
   */
-  histsvc.addVisit(testURI, Date.now() * 1000, 0, histsvc.TRANSITION_TYPED, false, 0);
+  histsvc.addVisit(testURI, Date.now(), 0, histsvc.TRANSITION_TYPED, false, 0);
 
   // these annotations should be removed (after manually tweaking their dateAdded)
   annosvc.setPageAnnotation(testURI, testAnnoName, testAnnoVal, 0, annosvc.EXPIRE_DAYS);
@@ -211,7 +211,7 @@ function run_test() {
   annosvc.setItemAnnotation(bookmark, testAnnoName + "NotExpired", testAnnoVal, 0, annosvc.EXPIRE_DAYS);
 
   // add a uri and then remove it, to trigger expiration
-  histsvc.addVisit(triggerURI, Date.now() * 1000, 0, histsvc.TRANSITION_TYPED, false, 0);
+  histsvc.addVisit(triggerURI, Date.now(), 0, histsvc.TRANSITION_TYPED, false, 0);
   bhist.removePage(triggerURI);
 
   // test for unexpired annos
@@ -239,7 +239,7 @@ function run_test() {
   } catch(ex) {}
 
   // test anno expiration (days) removes annos annos 6 days old
-  histsvc.addVisit(testURI, Date.now() * 1000, 0, histsvc.TRANSITION_TYPED, false, 0);
+  histsvc.addVisit(testURI, Date.now(), 0, histsvc.TRANSITION_TYPED, false, 0);
   annosvc.setPageAnnotation(testURI, testAnnoName, testAnnoVal, 0, annosvc.EXPIRE_DAYS);
   annosvc.setItemAnnotation(bookmark, testAnnoName, testAnnoVal, 0, annosvc.EXPIRE_DAYS);
   // these annotations should remain as they are only 6 days old
@@ -248,7 +248,7 @@ function run_test() {
   dbConnection.executeSimpleSQL("UPDATE moz_items_annos SET dateAdded = " + expirationDate);
 
   // add a uri and then remove it, to trigger expiration
-  histsvc.addVisit(triggerURI, Date.now() * 1000, 0, histsvc.TRANSITION_TYPED, false, 0);
+  histsvc.addVisit(triggerURI, Date.now(), 0, histsvc.TRANSITION_TYPED, false, 0);
   bhist.removePage(triggerURI);
 
   // test for unexpired annos
@@ -267,7 +267,7 @@ function run_test() {
 
 
   // test anno expiration (weeks) removes annos 31 days old
-  histsvc.addVisit(testURI, Date.now() * 1000, 0, histsvc.TRANSITION_TYPED, false, 0);
+  histsvc.addVisit(testURI, Date.now(), 0, histsvc.TRANSITION_TYPED, false, 0);
   annosvc.setPageAnnotation(testURI, testAnnoName, testAnnoVal, 0, annosvc.EXPIRE_WEEKS);
   annosvc.setItemAnnotation(bookmark, testAnnoName, testAnnoVal, 0, annosvc.EXPIRE_WEEKS);
   // these annotations should not remain as they are 31 days old
@@ -279,7 +279,7 @@ function run_test() {
   annosvc.setItemAnnotation(bookmark, testAnnoName + "NotExpired", testAnnoVal, 0, annosvc.EXPIRE_WEEKS);
 
   // add a uri and then remove it, to trigger expiration
-  histsvc.addVisit(triggerURI, Date.now() * 1000, 0, histsvc.TRANSITION_TYPED, false, 0);
+  histsvc.addVisit(triggerURI, Date.now(), 0, histsvc.TRANSITION_TYPED, false, 0);
   bhist.removePage(triggerURI);
 
   // test for unexpired annos
@@ -306,7 +306,7 @@ function run_test() {
   } catch(ex) {}
 
   // test anno expiration (weeks) does not remove annos 29 days old
-  histsvc.addVisit(testURI, Date.now() * 1000, 0, histsvc.TRANSITION_TYPED, false, 0);
+  histsvc.addVisit(testURI, Date.now(), 0, histsvc.TRANSITION_TYPED, false, 0);
   annosvc.setPageAnnotation(testURI, testAnnoName, testAnnoVal, 0, annosvc.EXPIRE_WEEKS);
   annosvc.setItemAnnotation(bookmark, testAnnoName, testAnnoVal, 0, annosvc.EXPIRE_WEEKS);
   // these annotations should remain as they are only 29 days old
@@ -315,7 +315,7 @@ function run_test() {
   dbConnection.executeSimpleSQL("UPDATE moz_items_annos SET dateAdded = " + expirationDate);
 
   // add a uri and then remove it, to trigger expiration
-  histsvc.addVisit(triggerURI, Date.now() * 1000, 0, histsvc.TRANSITION_TYPED, false, 0);
+  histsvc.addVisit(triggerURI, Date.now(), 0, histsvc.TRANSITION_TYPED, false, 0);
   bhist.removePage(triggerURI);
 
   // test for unexpired annos
@@ -343,7 +343,7 @@ function run_test() {
   annosvc.setItemAnnotation(bookmark, testAnnoName + "NotExpired", testAnnoVal, 0, annosvc.EXPIRE_MONTHS);
 
   // add a uri and then remove it, to trigger expiration
-  histsvc.addVisit(triggerURI, Date.now() * 1000, 0, histsvc.TRANSITION_TYPED, false, 0);
+  histsvc.addVisit(triggerURI, Date.now(), 0, histsvc.TRANSITION_TYPED, false, 0);
   bhist.removePage(triggerURI);
 
   // test for unexpired annos
@@ -370,7 +370,7 @@ function run_test() {
   } catch(ex) {}
 
   // test anno expiration (months) does not remove annos 179 days old
-  histsvc.addVisit(testURI, Date.now() * 1000, 0, histsvc.TRANSITION_TYPED, false, 0);
+  histsvc.addVisit(testURI, Date.now(), 0, histsvc.TRANSITION_TYPED, false, 0);
   annosvc.setPageAnnotation(testURI, testAnnoName, testAnnoVal, 0, annosvc.EXPIRE_MONTHS);
   annosvc.setItemAnnotation(bookmark, testAnnoName, testAnnoVal, 0, annosvc.EXPIRE_MONTHS);
   // these annotations should remain as they are only 179 days old
@@ -379,7 +379,7 @@ function run_test() {
   dbConnection.executeSimpleSQL("UPDATE moz_items_annos SET dateAdded = " + expirationDate);
 
   // add a uri and then remove it, to trigger expiration
-  histsvc.addVisit(triggerURI, Date.now() * 1000, 0, histsvc.TRANSITION_TYPED, false, 0);
+  histsvc.addVisit(triggerURI, Date.now(), 0, histsvc.TRANSITION_TYPED, false, 0);
   bhist.removePage(triggerURI);
 
   // test for unexpired annos
@@ -411,7 +411,7 @@ function run_test() {
   annosvc.setPageAnnotation(testURI, testAnnoName, "mod", 0, annosvc.EXPIRE_DAYS);
   annosvc.setItemAnnotation(bookmark, testAnnoName, "mod", 0, annosvc.EXPIRE_DAYS);
   // add a uri and then remove it, to trigger expiration
-  histsvc.addVisit(triggerURI, Date.now() * 1000, 0, histsvc.TRANSITION_TYPED, false, 0);
+  histsvc.addVisit(triggerURI, Date.now(), 0, histsvc.TRANSITION_TYPED, false, 0);
   bhist.removePage(triggerURI);
   // anno should still be there
   try {
@@ -430,7 +430,7 @@ function run_test() {
   dbConnection.executeSimpleSQL("UPDATE moz_annos SET lastModified = " + expirationDate);
   dbConnection.executeSimpleSQL("UPDATE moz_items_annos SET lastModified = " + expirationDate);
   // add a uri and then remove it, to trigger expiration
-  histsvc.addVisit(triggerURI, Date.now() * 1000, 0, histsvc.TRANSITION_TYPED, false, 0);
+  histsvc.addVisit(triggerURI, Date.now(), 0, histsvc.TRANSITION_TYPED, false, 0);
   bhist.removePage(triggerURI);
   // anno should have been deleted
   try {
@@ -449,180 +449,87 @@ function run_test() {
 // run async, chained
 
 function startIncrementalExpirationTests() {
-  startExpireNeither();
+  startExpireByVisitsTest();
 }
 
 var prefs = Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefBranch);
 var ghist = Cc["@mozilla.org/browser/global-history;2"].getService(Ci.nsIGlobalHistory2);
 
 /*
-test 1: NO EXPIRATION CRITERIA MET
+test visit-count-based history expiration via the browser.history_expire_visits pref.
-
-1. zero visits > {browser.history_expire_days}
-2. zero visits > {browser.history_expire_days_min}
-   AND total visited site count < {browser.history_expire_sites}
-
 steps:
-  - clear history
   - reset observer
-  - add a visit, w/ current date
+  - clear history
-  - set browser.history_expire_days to 3
+  - add 6 visits
-  - set browser.history_expire_days_min to 2
+  - set browser.history_expire_visits pref to 1 visit
-  - set browser.history_expire_sites to 2
+  - kick off incremental expiration via addURI
-  - kick off incremental expiration
+  - check onPageExpired for the older visit
-
-confirmation:
-  - check onPageExpired, confirm nothing was expired
-  - query for the visit, confirm it's there
-
 */
-function startExpireNeither() {
+function startExpireByVisitsTest() {
-  dump("startExpireNeither()\n");
+  dump("starting history_expire_visits test\n");
-  // setup
-  histsvc.removeAllPages();
   observer.expiredURI = null;
-
+  histsvc.removeAllPages();
-  // add data
+  var fillerURI = uri("http://blah.com");
-  histsvc.addVisit(testURI, Date.now() * 1000, 0, histsvc.TRANSITION_TYPED, false, 0);
+  for (var i = 0; i < 5; i++)
+    histsvc.addVisit(uri("http://filler.com/" + i), Date.now(), 0, histsvc.TRANSITION_TYPED, false, 0);
+  // 6th visit (incremental expiration does chunks of 6, see EXPIRATION_COUNT_PER_RUN)
+  // distinguish it from the other visits, and make it the oldest
+  histsvc.addVisit(testURI, Date.now() - (86400 * 2), 0, histsvc.TRANSITION_TYPED, false, 0);
   annosvc.setPageAnnotation(testURI, testAnnoName, testAnnoVal, 0, annosvc.EXPIRE_WITH_HISTORY);
-
+  prefs.setIntPref("browser.history_expire_visits", 1);
-  // set visit cap to 2
+  // addURI triggers incremental expiration, and is also the 7th visit
-  prefs.setIntPref("browser.history_expire_sites", 2);
+  ghist.addURI(uri("http://fizz.com"), false, true, triggerURI); 
-  // set date minimum to 2
-  prefs.setIntPref("browser.history_expire_days_min", 2);
-  // set date maximum to 3
-  prefs.setIntPref("browser.history_expire_days", 3);
-
-  // trigger expiration
-  ghist.addURI(triggerURI, false, true, null); 
-
-  // setup confirmation
   do_test_pending();
-  do_timeout(3600, "checkExpireNeither();"); // incremental expiration timer is 3500
+  do_timeout(3600, "checkExpireByVisitsTest();"); // incremental expiration timer is 3500, see PARTIAL_EXPIRATION_TIMEOUT 
 }
 
-function checkExpireNeither() {
+function checkExpireByVisitsTest() {
-  dump("checkExpireNeither()\n");
   try {
-    do_check_eq(observer.expiredURI, null);
+    do_check_eq(testURI.spec, observer.expiredURI);
-    do_check_eq(annosvc.getPageAnnotationNames(testURI, {}).length, 1);
-  } catch(ex) {
-    do_throw(ex);
-  }
-  dump("done incremental expiration test 1\n");
-  startExpireDaysOnly();
-}
-
-/*
-test 2: MAX-AGE DATE CRITERIA MET
-
-1. some visits > {browser.history_expire_days}
-2. total visited sites count < {browser.history_expire_sites}
-
-steps:
-  - clear history
-  - reset observer
-  - add a visit, 4 days old
-  - set browser.history_expire_days to 3
-  - set browser.history_expire_days_min to 2
-  - set browser.history_expire_sites to 2
-  - kick off incremental expiration
-
-confirmation:
-  - check onPageExpired, confirm nothing was expired
-  - query for the visit, confirm it's there
-*/
-function startExpireDaysOnly() {
-  dump("startExpireDaysOnly()\n");
-  // setup
-  histsvc.removeAllPages();
-  observer.expiredURI = null;
-
-  // add expirable visit
-  histsvc.addVisit(testURI, (Date.now() - (86400 * 2 * 1000)) * 1000, 0, histsvc.TRANSITION_TYPED, false, 0);
-  annosvc.setPageAnnotation(testURI, testAnnoName, testAnnoVal, 0, annosvc.EXPIRE_WITH_HISTORY);
-
-  // add un-expirable visit
-  histsvc.addVisit(uri("http://unexpirable.com"), (Date.now() - (86400 * 1000)) * 1000, 0, histsvc.TRANSITION_TYPED, false, 0);
-
-  // set visit cap to 2
-  prefs.setIntPref("browser.history_expire_sites", 2);
-  // set date minimum to 2
-  prefs.setIntPref("browser.history_expire_days", 2);
-
-  // trigger expiration
-  ghist.addURI(triggerURI, false, true, null); 
-
-  // setup confirmation
-  do_timeout(3600, "checkExpireDaysOnly();"); // incremental expiration timer is 3500
-}
-
-function checkExpireDaysOnly() {
-  try {
-    // test expired record
-    do_check_eq(observer.expiredURI, testURI.spec);
     do_check_eq(annosvc.getPageAnnotationNames(testURI, {}).length, 0);
-    // test unexpired record
+    do_check_eq(histsvc.getPageTitle(uri("http://fizz.com")), "fizz.com");
-    do_check_neq(histsvc.getPageTitle(uri("http://unexpirable.com")), null);
   } catch(ex) {}
-  dump("done expiration test 2\n");
+  dump("done history_expire_visits test\n");
-  startExpireBoth();
+  startExpireByDaysTest();
 }
 
-/*
-test 3: MIN-AGE+VISIT-CAP CRITERIA MET
 
-1. zero visits > {browser.history_expire_days}
+/*
-2. some visits > {browser.history_expire_days_min}
+test fallback to age-based history expiration via the browser.history_expire_days pref.
-   AND total visited sites count > {browser.history_expire_sites}
+
+ie: assuming there are less than browser.history_expire_visits visits, expire any visits
+over browser.history_expire_days old.
 
 steps:
-  - clear history
   - reset observer
-  - add a visit, 2 days old
+  - clear history
-  - add a visit, 2 days old
+  - add 6 visits, each 2 days old
-  - set browser.history_expire_days to 3
+  - browser.history_expire_visits is at 1
-  - set browser.history_expire_days_min to 1
+  - set browser.history_expire_days pref to 1 day
-  - set browser.history_expire_sites to 1
   - kick off incremental expiration
-
+  - check onPageExpired for the older visit
-confirmation:
-  - check onPageExpired, confirm our visit was expired
-  - query for the visit, confirm it's not there
 */
-function startExpireBoth() {
+function startExpireByDaysTest() {
-  dump("starting expiration test 3: both criteria met\n");
+  dump("starting history_expire_days test\n");
-  // setup
-  histsvc.removeAllPages();
   observer.expiredURI = null;
-
+  histsvc.removeAllPages();
-  // add visits
+  histsvc.addVisit(uri("http://blah.com"), Date.now() - (86400 * 2), 0, histsvc.TRANSITION_TYPED, false, 0);
-  // 2 days old, in microseconds
+  histsvc.addVisit(uri("http://bleh.com"), Date.now() - (86400 * 2), 0, histsvc.TRANSITION_TYPED, false, 0);
-  var age = (Date.now() - (86400 * 2 * 1000)) * 1000;
+  histsvc.addVisit(testURI, Date.now() - (86400 * 2), 0, histsvc.TRANSITION_TYPED, false, 0);
-  dump("AGE: " + age + "\n");
-  histsvc.addVisit(testURI, age, 0, histsvc.TRANSITION_TYPED, false, 0);
-  histsvc.addVisit(testURI, age, 0, histsvc.TRANSITION_TYPED, false, 0);
   annosvc.setPageAnnotation(testURI, testAnnoName, testAnnoVal, 0, annosvc.EXPIRE_WITH_HISTORY);
-
+  var prefs = Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefBranch);
-  // set visit cap to 1
+  // history_expire_visits is set to 1 already
-  prefs.setIntPref("browser.history_expire_sites", 1);
+  // setting history_expire_days to 1 will cause all of our 2 day-old visits to expire
-  // set date max to 3
-  prefs.setIntPref("browser.history_expire_days", 3);
-  // set date minimum to 1
   prefs.setIntPref("browser.history_expire_days", 1);
-
+  ghist.addURI(testURI, false, true, triggerURI); 
-  // trigger expiration
+  do_timeout(3600, "checkExpireByDaysTest();"); // incremental expiration timer is 3500
-  ghist.addURI(triggerURI, false, true, null);
-
-  // setup confirmation
-  do_timeout(3600, "checkExpireBoth();"); // incremental expiration timer is 3500
 }
 
-function checkExpireBoth() {
+function checkExpireByDaysTest() {
   try {
-    do_check_eq(observer.expiredURI, testURI.spec);
+    do_check_eq(testURI.spec, observer.expiredURI);
     do_check_eq(annosvc.getPageAnnotationNames(testURI, {}).length, 0);
   } catch(ex) {}
-  dump("done expiration test 3\n");
+  dump("done history_expire_days test\n");
   do_test_finished();
 }