Bug 376706 - "Can't Delete History by Date when grouped by 'Date' or by 'Date and Site'" [p=mak77@supereva.it (Marco Bonardo [mak77]) r=dietrich a=blocking-firefox3+]

browser/components/places/content/controller.js

@@ -943,6 +943,8 @@ PlacesController.prototype = {
     var URIs = [];
     var bhist = PlacesUtils.history.QueryInterface(Ci.nsIBrowserHistory);
     var resultView = this._view.getResultView();
+    var root = this._view.getResultNode();
+
     for (var i = 0; i < nodes.length; ++i) {
       var node = nodes[i];
       if (PlacesUtils.nodeIsHost(node))
@@ -954,6 +956,26 @@ PlacesController.prototype = {
           URIs.push(uri);
         }
       }
+      else if (PlacesUtils.nodeIsDay(node)) {
+        // this is the oldest date
+        // for the last node endDate is end of epoch
+        var beginDate = 0;
+        // this is the newest date
+        // day nodes have time property set to the last day in the interval
+        var endDate = node.time;
+
+        // if this is not the last day container, then beginDate
+        // is the time property of the next day container
+        for (var j = 0; j < root.childCount-1 && !beginDate; ++j) {
+          if (root.getChild(j) != node)
+            continue;
+          var nextNode = root.getChild(j+1);
+          beginDate = nextNode.time
+        }
+
+        // we want to exclude beginDate from the removal
+        bhist.removePagesByTimeframe(beginDate+1, endDate);
+      }
     }
 
     // if we have to delete a lot of urls RemovePage will be slow, it's better

browser/components/places/content/utils.js

@@ -249,7 +249,7 @@ var PlacesUtils = {
   },
 
   /**
-   * Determines whether or not a ResultNode is a Bookmark folder or not.
+   * Determines whether or not a ResultNode is a Bookmark folder.
    * @param   aNode
    *          A result node
    * @returns true if the node is a Bookmark folder, false otherwise
@@ -285,7 +285,7 @@ var PlacesUtils = {
   },
 
   /**
-   * Determines whether or not a ResultNode is a visit item or not
+   * Determines whether or not a ResultNode is a visit item.
    * @param   aNode
    *          A result node
    * @returns true if the node is a visit item, false otherwise
@@ -300,7 +300,7 @@ var PlacesUtils = {
   },
 
   /**
-   * Determines whether or not a ResultNode is a URL item or not
+   * Determines whether or not a ResultNode is a URL item.
    * @param   aNode
    *          A result node
    * @returns true if the node is a URL item, false otherwise
@@ -314,7 +314,7 @@ var PlacesUtils = {
   },
 
   /**
-   * Determines whether or not a ResultNode is a Query item or not
+   * Determines whether or not a ResultNode is a Query item.
    * @param   aNode
    *          A result node
    * @returns true if the node is a Query item, false otherwise
@@ -342,10 +342,10 @@ var PlacesUtils = {
   },
 
   /**
-   * Determines whether or not a ResultNode is a host folder or not
+   * Determines whether or not a ResultNode is a host container.
    * @param   aNode
    *          A result node
-   * @returns true if the node is a host item, false otherwise
+   * @returns true if the node is a host container, false otherwise
    */
   nodeIsHost: function PU_nodeIsHost(aNode) {
     NS_ASSERT(aNode, "null node");
@@ -353,7 +353,17 @@ var PlacesUtils = {
   },
 
   /**
-   * Determines whether or not a ResultNode is a container item or not
+   * Determines whether or not a ResultNode is a day container.
+   * @param   node
+   *          A NavHistoryResultNode
+   * @returns true if the node is a day container, false otherwise
+   */
+  nodeIsDay: function PU_nodeIsDay(aNode) {
+    return aNode.type == Ci.nsINavHistoryResultNode.RESULT_TYPE_DAY;
+  },
+
+  /**
+   * Determines whether or not a ResultNode is a container.
    * @param   aNode
    *          A result node
    * @returns true if the node is a container item, false otherwise

toolkit/components/places/public/nsIBrowserHistory.idl

@@ -42,7 +42,7 @@
 #include "nsISupports.idl"
 #include "nsIGlobalHistory2.idl"
 
-[scriptable, uuid(ef807ae6-4ae9-4d1f-8ade-3ee75e24d18c)]
+[scriptable, uuid(96602bf3-de2a-42ed-812f-a83b130e6299)]
 interface nsIBrowserHistory : nsIGlobalHistory2
 {
     /**
@@ -63,7 +63,8 @@ interface nsIBrowserHistory : nsIGlobalHistory2
     /**
      * count
      *
-     * The number of entries in global history
+     * Indicate if there are entries in global history
+     * For performance reasons this does not return the real number of entries
      */
     readonly attribute PRUint32 count;
 
@@ -95,6 +96,16 @@ interface nsIBrowserHistory : nsIGlobalHistory2
      */
     void removePagesFromHost(in AUTF8String aHost, in boolean aEntireDomain);
 
+    /**
+     * removePagesByTimeframe
+     *
+     * Remove all pages for a given timeframe.
+     * Limits are included: aBeginTime <= timeframe <= aEndTime
+     * Notice that this does not call observers for single deleted uris,
+     * instead it will send Begin/EndUpdateBatch
+     */
+    void removePagesByTimeframe(in long long aBeginTime, in long long aEndTime);
+
     /**
      * removeAllPages
      *

toolkit/components/places/src/nsNavHistory.cpp

@@ -3341,6 +3341,57 @@ nsNavHistory::RemovePagesFromHost(const nsACString& aHost, PRBool aEntireDomain)
 }
 
 
+// nsNavHistory::RemovePagesByTimeframe
+//
+//    This function will delete all history information about
+//    pages for a given timeframe.
+//    Limits are included: aBeginTime <= timeframe <= aEndTime
+//
+//    This method sends onBeginUpdateBatch/onEndUpdateBatch to observers
+
+NS_IMETHODIMP
+nsNavHistory::RemovePagesByTimeframe(PRTime aBeginTime, PRTime aEndTime)
+{
+  nsresult rv;
+  // build a list of place ids to delete
+  nsCString deletePlaceIdsQueryString;
+
+  // we only need to know if a place has a visit into the given timeframe
+  // this query is faster than actually selecting in moz_historyvisits
+  nsCOMPtr<mozIStorageStatement> selectByTime;
+  rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
+      "SELECT h.id FROM moz_places h WHERE "
+      "EXISTS (SELECT id FROM moz_historyvisits v WHERE v.place_id = h.id "
+      " AND v.visit_date >= ?1 AND v.visit_date <= ?2 LIMIT 1)"),
+    getter_AddRefs(selectByTime));
+  NS_ENSURE_SUCCESS(rv, rv);
+  rv = selectByTime->BindInt64Parameter(0, aBeginTime);
+  NS_ENSURE_SUCCESS(rv, rv);
+  rv = selectByTime->BindInt64Parameter(1, aEndTime);
+  NS_ENSURE_SUCCESS(rv, rv);
+
+  PRBool hasMore = PR_FALSE;
+  while (NS_SUCCEEDED(selectByTime->ExecuteStep(&hasMore)) && hasMore) {
+    PRInt64 placeId;
+    rv = selectByTime->GetInt64(0, &placeId);
+    NS_ENSURE_SUCCESS(rv, rv);
+    if (placeId != 0) {
+      if (!deletePlaceIdsQueryString.IsEmpty())
+        deletePlaceIdsQueryString.AppendLiteral(",");
+      deletePlaceIdsQueryString.AppendInt(placeId);
+    }
+  }
+
+  rv = RemovePagesInternal(deletePlaceIdsQueryString);
+  NS_ENSURE_SUCCESS(rv, rv);
+
+  // force a full refresh calling onEndUpdateBatch (will call Refresh())
+  UpdateBatchScoper batch(*this); // sends Begin/EndUpdateBatch to observers
+
+  return NS_OK;
+}
+
+
 // nsNavHistory::RemoveAllPages
 //
 //    This function is used to clear history.

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

@@ -41,33 +41,63 @@
 
 // Get global history service
 try {
-  var bhist = Cc["@mozilla.org/browser/global-history;2"].getService(Ci.nsIBrowserHistory);
+  var bhist = Cc["@mozilla.org/browser/global-history;2"].
+                getService(Ci.nsIBrowserHistory);
 } catch(ex) {
   do_throw("Could not get history service\n");
-} 
+}
 
 // Get annotation service
 try {
-  var annosvc= Cc["@mozilla.org/browser/annotation-service;1"].getService(Ci.nsIAnnotationService);
+  var annosvc= Cc["@mozilla.org/browser/annotation-service;1"].
+                 getService(Ci.nsIAnnotationService);
 } catch(ex) {
   do_throw("Could not get annotation service\n");
-} 
+}
 
 // Get bookmark service
 try {
-  var bmsvc = Cc["@mozilla.org/browser/nav-bookmarks-service;1"].getService(Ci.nsINavBookmarksService);
+  var bmsvc = Cc["@mozilla.org/browser/nav-bookmarks-service;1"].
+                getService(Ci.nsINavBookmarksService);
 } catch(ex) {
   do_throw("Could not get nav-bookmarks-service\n");
 }
 
+// Get history service
+try {
+  var histsvc = Cc["@mozilla.org/browser/nav-history-service;1"].
+                  getService(Ci.nsINavHistoryService);
+} catch(ex) {
+  do_throw("Could not get history service\n");
+}
+
+/**
+ * Checks to see that a URI is in the database.
+ *
+ * @param aURI
+ *        The URI to check.
+ * @returns true if the URI is in the DB, false otherwise.
+ */
+function uri_in_db(aURI) {
+  var options = histsvc.getNewQueryOptions();
+  options.maxResults = 1;
+  options.resultType = options.RESULTS_AS_URI;
+  var query = histsvc.getNewQuery();
+  query.uri = aURI;
+  var result = histsvc.executeQuery(query, options);
+  var root = result.root;
+  root.containerOpen = true;
+  return (root.childCount == 1);
+}
+
 // main
 function run_test() {
   var testURI = uri("http://mozilla.com");
 
-  /** 
+  /**
    * addPageWithDetails
-   * Adds a page to history with specific time stamp information. This is used in
-   * the History migrator. 
+   * Adds a page to history with specific time stamp information.
+   * This is used in the History migrator.
    */
   try {
     bhist.addPageWithDetails(testURI, "testURI", Date.now() * 1000);
@@ -83,7 +113,7 @@ function run_test() {
 
   /**
    * count
-   * The number of entries in global history
+   * Indicate if there are entries in global history
    */
   do_check_eq(1, bhist.count);
 
@@ -113,7 +143,8 @@ function run_test() {
 
   try {
     for (var i = 0; i < deletedPages.length ; ++i)
-      bhist.addPageWithDetails(deletedPages[i], "testURI" + (i+1), Date.now() * 1000);
+      bhist.addPageWithDetails(deletedPages[i], "testURI" + (i+1),
+                               Date.now() * 1000);
   } catch(ex) {
     do_throw("addPageWithDetails failed");
   }
@@ -137,13 +168,43 @@ function run_test() {
   do_check_eq(0, bhist.count);
   do_check_eq("", bhist.lastPageVisited);
   // check that bookmark and annotation still exist
-  do_check_eq(bmsvc.getBookmarkURI(bookmark).spec, deletedPages[bookmarkIndex].spec);
-  do_check_eq(annosvc.getPageAnnotation(deletedPages[annoIndex], annoName), annoValue);
+  do_check_eq(bmsvc.getBookmarkURI(bookmark).spec,
+              deletedPages[bookmarkIndex].spec);
+  do_check_eq(annosvc.getPageAnnotation(deletedPages[annoIndex], annoName),
+              annoValue);
   // remove annotation and bookmark
   annosvc.removePageAnnotation(deletedPages[annoIndex], annoName);
   bmsvc.removeItem(bookmark);
   bhist.removeAllPages();
 
+  /**
+   * removePagesByTimeframe
+   * Remove all pages for a given timeframe.
+   */
+  // PRTime is microseconds while JS time is milliseconds
+  var startDate = Date.now() * 1000;
+  try {
+    for (var i = 0; i < 10; ++i) {
+      let testURI = uri("http://mirror" + i + ".mozilla.com");
+      bhist.addPageWithDetails(testURI, "testURI" + i, startDate + i);
+    }
+  } catch(ex) {
+    do_throw("addPageWithDetails failed");
+  }
+  // delete all pages except the first and the last
+  bhist.removePagesByTimeframe(startDate+1, startDate+8);
+  // check that we have removed correct pages
+  for (var i = 0; i < 10; ++i) {
+    let testURI = uri("http://mirror" + i + ".mozilla.com");
+    if (i > 0 && i < 9)
+      do_check_false(uri_in_db(testURI));
+    else
+      do_check_true(uri_in_db(testURI));
+  }
+  // clear remaining items and check that all pages have been removed
+  bhist.removePagesByTimeframe(startDate, startDate+9);
+  do_check_eq(0, bhist.count);
+
   /**
    * removePagesFromHost
    * Remove all pages from the given host.
@@ -156,7 +217,8 @@ function run_test() {
 
   // test aEntireDomain
   bhist.addPageWithDetails(testURI, "testURI", Date.now() * 1000);
-  bhist.addPageWithDetails(uri("http://foobar.mozilla.com"), "testURI2", Date.now() * 1000);
+  var testURI2 = uri("http://foobar.mozilla.com");
+  bhist.addPageWithDetails(testURI2, "testURI2", Date.now() * 1000);
   bhist.removePagesFromHost("mozilla.com", false);
   do_check_eq(1, bhist.count);