376706 - Can't Delete History by Date when grouped by 'Date' or by 'Date and Site'

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P2)

Description

[tabmix.onemen]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a4pre) Gecko/20070405 Minefield/3.0a4pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a4pre) Gecko/20070405 Minefield/3.0a4pre

1. set view to Date and site
2. try to delete site

result nothing was deleted error message:

Error: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIBrowserHistory.removePagesFromHost]
Source file: chrome://browser/content/places/controller.js
Line: 1057

3. set view to Date and Site or to Date
4. try to delete day

result nothing was deleted NO error message

also if you reduce number of days to remember from Tools > Options
no days was deleted

Reproducible: Always

Comment 1

[(not reading, please use [e-mail] instead)]

the first part is a dup of an existing bug.  I'm not sure about the second part being a dup.

Comment 2

[tabmix.onemen]

Attachment: patch

removePagesFromHost [nsNavHistory::RemovePagesFromHost] is not working at the moment, but anyhow the function is designed to delete all pages for host.
what about the case for deleting host pages for one day in history ?

this patch fix the delete from history sidebar for any view as well as bug #376581

currently there is no code to change history from options dialog
after user changed the preference from Tools > Options > Privacy "Remember visited pages..." this is done only when Firefox Shutdown (as far as i can tell :)) 
don't you think its need to be done when on dialog accept ?

Comment 3

[Arie Paap [:wildmyron]]

I guess comment 1 is referring to bug 363621.

Comment 4

[Asaf Romano (gone)]

Comment on attachment 260991 [details] [diff] [review]
patch

Please patch bug 376581 separately.

Comment 5

[Noah (NoahSUMO/SolidSnake) [:Noah]]

Maybe this has been partially resolved due to other Places checkins as I can now delete entries from underneath folders in 'Date & Site' view. I have not tried other views tho. The remaining problem for me is I still can not delete an individual site's folder of history. Can anyone confirm?

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a5pre) Gecko/20070530 Minefield/3.0a5pre

Comment 6

[Dietrich Ayala (:dietrich)]

(In reply to comment #5)
> Maybe this has been partially resolved due to other Places checkins as I can
> now delete entries from underneath folders in 'Date & Site' view. I have not
> tried other views tho. The remaining problem for me is I still can not delete
> an individual site's folder of history. Can anyone confirm?
> 
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a5pre) Gecko/20070530
> Minefield/3.0a5pre
> 

Confirmed:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a7pre) Gecko/2007080217 Minefield/3.0a7pre

Deleting by site, when grouped by 'Date & Site', works.

Deleting by day, when grouped by 'Date & Site', does not work. In a debug build, there are no errors, and it instead expands the selection to include the container prior to it (in the visible folder order).

I'm changing the summary to reflect this.

Comment 7

[Dietrich Ayala (:dietrich)]

pushing to M10.

Comment 8

[Dietrich Ayala (:dietrich)]

un-assigning seth's places/autocomplete bugs.

Comment 9

[Marco Bonardo [:mak]]

taking for now

Comment 10

[Marco Bonardo [:mak]]

Attachment: wip

A little explanation of the idea behind this:

we actually have day containers in the sidebar and we are grouping them as "today, yesterday, 2days ago, ..., more than 6 days ago".
Every day container has the time property set to the min(visit_date) that it contains (i've contacted Ondrej to put that in into his new history sidebar patch), we could delete from that visit_date to the next day adding a consistent number of microseconds (like in original Seth's patch attached to Bug 363621).
But now that we have 90 days of history we should move to an exponential division like "today, yesterday, one week ago, ...", so that approach should be updated everytime we change the groupings.

We could instead delete between the time of the actual day container and the time of the next day container, this way if we want to change the grouping we can do that and delete should still work.

I've added a new RemovePagesByTimeframe to globalHistory, that calls RemovePagesInternal, so we still have a single delete method for all remove functions.

When deleting a day folder, more of them could disappear, that is because we delete ALL visits for a page (deleting an item from history should delete it completely from all hostory views), so if i delete today, and i've visited the same pages as yesterday, also yesterday will disappear, this is a limit test case that could happen (and should be considered correct behaviour)

i still need to write a test for the new api and check the code that search for the next day container, i hope there's a better way of doing that.

Comment 11

[Marco Bonardo [:mak]]

well sorry ther's an error in my comment:

Every day container has the time property set to the max(visit_date) that it contains

Comment 12

[Marco Bonardo [:mak]]

Attachment: patch + test

added the testcase, cleaned-up a little the test file, fixed a comment in idl about count (is not really eturning the actual number of entries in history, see Bug 416832)

Comment 13

[Marco Bonardo [:mak]]

Attachment: patch + test

Done some comment cleanup & functionality test. And we are ready for review.

Comment 14

[Dietrich Ayala (:dietrich)]

Comment on attachment 302800 [details] [diff] [review]
patch + test


> // 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);

nit: style is usually dot at the end.

r=me

Comment 15

[Marco Bonardo [:mak]]

Attachment: for check-in

addressed comment, unit tests passed, functionality test passed.
ready for check-in

Comment 16

[Reed Loden [:reed]]

Checking in toolkit/components/places/public/nsIBrowserHistory.idl;
/cvsroot/mozilla/toolkit/components/places/public/nsIBrowserHistory.idl,v  <--  nsIBrowserHistory.idl
new revision: 1.13; previous revision: 1.12
done
Checking in toolkit/components/places/src/nsNavHistory.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v  <--  nsNavHistory.cpp
new revision: 1.258; previous revision: 1.257
done
Checking in toolkit/components/places/tests/unit/test_browserhistory.js;
/cvsroot/mozilla/toolkit/components/places/tests/unit/test_browserhistory.js,v  <--  test_browserhistory.js
new revision: 1.5; previous revision: 1.4
done
Checking in browser/components/places/content/controller.js;
/cvsroot/mozilla/browser/components/places/content/controller.js,v  <--  controller.js
new revision: 1.205; previous revision: 1.204
done
Checking in browser/components/places/content/utils.js;
/cvsroot/mozilla/browser/components/places/content/utils.js,v  <--  utils.js
new revision: 1.103; previous revision: 1.102
done

Comment 18

[Tracy Walker [:tracy]]

verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre)
Gecko/2008022704 Minefield/3.0b4pre and Mozilla/5.0 (Macintosh; U; Intel Mac OS
X 10.4; en-US; rv:1.9b4pre) Gecko/2008022704 Minefield/3.0b4pre

Comment 19

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv