From 516123d72fccb07442bc7bcab782f7e8097e2f1a Mon Sep 17 00:00:00 2001 From: "reed%reedloden.com" Date: Fri, 22 Feb 2008 11:46:30 +0000 Subject: [PATCH] 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+] --- .../components/places/content/controller.js | 22 +++++ browser/components/places/content/utils.js | 24 +++-- .../places/public/nsIBrowserHistory.idl | 15 +++- .../components/places/src/nsNavHistory.cpp | 51 +++++++++++ .../places/tests/unit/test_browserhistory.js | 88 ++++++++++++++++--- 5 files changed, 178 insertions(+), 22 deletions(-) diff --git a/browser/components/places/content/controller.js b/browser/components/places/content/controller.js index 766c6209882..4b8061e7997 100755 --- a/browser/components/places/content/controller.js +++ b/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 diff --git a/browser/components/places/content/utils.js b/browser/components/places/content/utils.js index 686d27a46b4..386a5edccf2 100644 --- a/browser/components/places/content/utils.js +++ b/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 diff --git a/toolkit/components/places/public/nsIBrowserHistory.idl b/toolkit/components/places/public/nsIBrowserHistory.idl index d9d239286d3..149b6b8fd59 100644 --- a/toolkit/components/places/public/nsIBrowserHistory.idl +++ b/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 * diff --git a/toolkit/components/places/src/nsNavHistory.cpp b/toolkit/components/places/src/nsNavHistory.cpp index ef451d29edb..d82251d63f9 100644 --- a/toolkit/components/places/src/nsNavHistory.cpp +++ b/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 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. diff --git a/toolkit/components/places/tests/unit/test_browserhistory.js b/toolkit/components/places/tests/unit/test_browserhistory.js index 039bdfbd171..d2949f249fb 100644 --- a/toolkit/components/places/tests/unit/test_browserhistory.js +++ b/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);