42001 - [FEATURE]Clear History button in preferences doesn't work

Status: VERIFIED FIXED

(Core :: DOM: Navigation, defect, P2)

Description

[Phil Peterson]

Using 2000-06-08-08 on NT

1. Open Preferences dialog
2. Click Navigator item if necessary
3. Click on Clear History button and OK the dialog
4. Tasks | Tools | History
5. History window comes up, contents not cleared

I looked for a dup of this and amazingly couldn't find one.

Comment 1

[Radha on family leave (not reading bugmail)]

Ben, Did you mention yesterday that you got a patch from someone for this?

Comment 2

[Radha on family leave (not reading bugmail)]

Over to slamm, 

Comment 3

[Chris McAfee]

ben, you gotum patchum?

Comment 4

[Claudius Gayle]

submitting for nsbeta3, we minused a related bug b/c this was supposed to be in and working. If it's not then it needs to. People 
need to be able to clear their history.

Comment 5

[Chris McAfee]

confirming on linux

Comment 6

[Paul Chen]

nav triage team: congratulations, Steve, you are the owner of another bug. Enjoy.

Comment 7

[Steve Lamm]

I talked to waterson about this. The global history datasource does not have a 
"clear" method. First, we need to figure out how to clear the history items from 
the mork database. Second, we need to have that reflected in the history 
datasource. Finally, we need to make sure the history observers get notified of 
the change (i.e. if the history window is open, you will see all the items 
disappear).

Waterson also mentioned that the mork method, CompressCommit(), should be called 
after deleting the history items.

Comment 8

[Jason Eager]

Slamm, can I take this off your hands?

Comment 9

[Steve Lamm]

Sure, that would be great. I have not had a chance to work on this any more.

Comment 10

[Jason Eager]

Accepting bug. (unless anyone else has been working on it in the meantime).

Comment 11

[Phil Peterson]

PDT agrees P2 since no workaround. You can't even delete from the History window

Comment 12

[Blake Ross]

[e-mail], did you mean to reassign this bug to yourself (rather than 
just accepting it for slamm)?

Comment 13

[Jason Eager]

YES, I meant to reassign this bug to myself. Read the exchange above. :P

Comment 14

[Blake Ross]

Are you actually doing work on this nsbeta3+ bug?   If not, reassign it to 
someone who is.

Comment 15

[Jason Eager]

I'm doing work on this, but will happily hand the bug over to anyone who
understands the mork database api (which is what the clear history routine will
need to use). I took the bug from Steve because he couldn't do anything with it.

Comment 16

[David :Bienvenu]

It would probably be easier to just delete the mork database file (after closing
it, of course) and create a new one. Chris, is there any information that is
stored here that we don't want cleared out?

I imagine the sequence of events would be:

1. Clear the datasource, which would get rid of any objects holding onto mork rows.
2. Close the history db
3. delete history.dat, or whatever it's called
4. Create a new, empty history db using the code that creates the initial
history db, and open it.

Comment 17

[David :Bienvenu]

adding self to cc list.

Comment 18

[Jason Eager]

Why does this bug keep getting set back to "new"? I keep accepting it. I'll
GLADLY hand it over to someone who understands mork so I don't have to
desperately take a look at mork in my spare free time in order to figure out how
this patch should work. But in the meantime, no one else is willing to do work
on this, so I am doing work on it. PLEASE stop resetting the bug to "new".

Comment 19

[David :Bienvenu]

Judging from the bug activity, it's been you that's been setting it back to
"New". Don't know why that would be. Maybe a browser bug.

Anyway, I don't believe you need to know anything about mork to fix this, as I
previously explained.

Comment 20

[Jason Eager]

Would deleting the file really be anything more then an ugly hack? Would all of
the state be cleared properly?

Comment 21

[David :Bienvenu]

It's not an ugly hack - it's a clever optimization! It's going to be faster than 
deleting every mork row one by one. It's trivial to write code to delete every 
row by hand and I can tell you how to do it if you want, but it would be faster 
to close the file, delete it, and start over.

Here are the mork api's you would use to delete all the rows:

get the history table (of type nsIMdbTable)

Get a row cursor with GetTableRowCursor()
use NextRow to get each row.
Probably should call CutAllColumns on the row.
Then, use nsIMdbTable::CutRow to cut that row from the table.

Comment 22

[Phil Peterson]

I guess the question is what does the history window look like (and how does
autocomplete behave, etc.) after deleting the file? Do we have any stale history
state hanging around? Seems easy enough to try it out, but if it's enough to
delete the file, that seems pretty attractive.

Comment 23

[David :Bienvenu]

Please don't misunderstand me - there's more involved than just deleting the
history database and creating a blank one. I'm sorry if I gave that impression.
I was just suggesting a way to deal with the mork part of this problem.
Obviously, the rdf state has to be cleaned up, the tree widget needs to be
notified that all the rows are gone, etc.

Another way of handling this would just be to "pretend" the user did a select
all followed by a delete - this would cause everything to get cleaned up - and
presumably, deleting a single history entry deletes the corresponding mork row,
so you wouldn't have to know any mork api calls - just the call that the history
front end already makes when deleting a single entry.

Comment 24

[Phil Peterson]

Not holding PR3 for this. Marking nsbeta3- and adding rtm nomination since I 
think we'll either have to make this work or take out the UI.

Comment 25

[Jason Eager]

And we'll need to impliment this feature, because otherwise people won't really
want to use Netscape (and they won't be honest about why either).

I'm SO sorry about the delay on getting this done. I'm moving flats right now. :-(

Comment 26

[R.K.Aa.]

*** Bug 55294 has been marked as a duplicate of this bug. ***

Comment 27

[ratman]

using a hack that would simulate a select all and delete won't work - because
doing that doesn't work either.  no manipulation to remove any part of the
history within the browser works.

Comment 28

[Chris Waterson]

Attachment: implement "clear history" in prefs

Comment 29

[Chris Waterson]

bienvenu, could I get you to look over the changes to nsGlobalHistory.cpp in the
above patch to implement removal from history? I iterate through the rows in the
history back to front, using the PosToRow() API; for each row that's removed, I
cut the row, and then notify via RDF APIs.

ben, could you review the changes to prefutilities.js?

Comment 30

[Blake Ross]

reassigning to waterson...

Comment 31

[Ben Goodger (use ben at mozilla dot org for email)]

r=ben for the changes to js.

but: shouldn't the method name have a lower case first letter? :P 
(removeAllPages)

Comment 32

[Brendan Eich [:brendan]]

Ben: waterson is doing "When in Rome" and minimizing changes.  I'd rather he or 
someone else change the .idl file to use Mozilla XPIDL style on the trunk, and 
later, than to mix styles here in this patch, which hopes to get [rtm++]'d.

I can give an r=, but nominate shaver to give the a= (he knows RDF, if not MDB).

/be

Comment 33

[Mike Shaver (:shaver emeritus)]

I'm no RDF wizard, and that MDB stuff makes me cold in my heart, but it looks
pretty good.

I don't like the explicit comparisons to 0 in the conditionals
  if (err != 0)
and they're not consistent with the pointer checks below, but other than that I
can find no real nits to pick. sr=shaver

Comment 34

[Blake Ross]

So we are going to try for rtm for this patch?  It's going to be a tough 
argument.  Brendan, can you officially r= (now that we have sr=shaver), and 
then + it once you do (if there's no concerns)?

Comment 35

[Phil Peterson]

I'd like bienvenu to review the MDB usage. He's on vacation today, but should be
available tomorrow.

Comment 36

[Phil Peterson]

Never mind -- bienvenu is out until Monday. He can review ex post facto if PDT
agrees to take this.

Comment 37

[Brendan Eich [:brendan]]

I meant to give my r= -- sorry for any ambiguity.

/be

Comment 38

[Blake Ross]

OK, r=brendan sr=shaver.  +'ing, Phil says David B. won't be back until Monday 
so we should try for ++ without waiting for his review.

Comment 39

[Chris Waterson]

I am not going to check this in on the branch until bienvenu has looked at it.

Comment 40

[Chris Waterson]

patch checked in on trunk

Comment 41

[ratman]

the "clear history" button works in build 2000102004 (win98), but the delete
function in the edit menu still doesn't work.  does this need a new bug report?

Comment 42

[Chris Waterson]

ratman: I think that's covered by bug 37078

Comment 43

[David :Bienvenu]

I don't know about the batch stuff - I suspect it does nothing. I'll check.

After you cut the row, you should call row->CutAllColumns(morkEnv) - this may
fix the leaks you mentioned.

Comment 44

[David :Bienvenu]

your useage of postorow looks fine - pos is just a 0-based index, so you are
using it correctly. The batch hint stuff does nothing corrently, but if mork
supports sorting, then the hint stuff will be useful to tell it not to keep the
sort tables up to date while the batch stuff is going on. r=bienvenu

Comment 45

[Chris Waterson]

bienvenu: Do I need to add a "CutAllColumns()" call? Or is the code ok as is...

Comment 46

[David :Bienvenu]

I would strongly recommend adding the CutAllColumns call - if you don't, I think
the row won't really fully go away. If you don't, at best you'll have memory
leaks, and at worst, the rows will still be around. It's some mork bug that I've
long since forgotten the details of, except that it might involve the fact that
the same row can appear in multiple tables, and I don't think mork does the
right thing when a row is removed from the last table, or something like that,
unless you CutAllColumns.

Comment 47

[Chris Waterson]

Attachment: cleanup to last patch

Comment 48

[Chris Waterson]

Add "CutAllColumns()" call per bienvenu's suggestion; change "LargeCommit()" to
"CompressCommit()" to force file to be nuked.

Comment 49

[Brendan Eich [:brendan]]

Wow, who knew those mork methods had such necessary effects!  Their names do not 
connote what they do (but you noticed that already, I'm sure).

/be

Comment 50

[David :Bienvenu]

looks good, r=bienvenu

Comment 51

[David :Bienvenu]

It's just a bug; adding this call works around the bug - the method names *do*
do what they say they do (I assume you're commenting about the fact that you
need to CutAllColumns after removing the row). I think I explained before that
it was a bug, so maybe you're talking about something else.

Comment 52

[Brendan Eich [:brendan]]

Sorry, I missed your explanation citing a mork bug.  But I also was commenting 
(hence my use of plural) on CompressCommit -- Compress to my reading does not 
connote "force file to be nuked", but maybe now I'm misunderstanding waterson's 
words.

Who should give sr=?

/be

Comment 53

[David :Bienvenu]

I suspect you just misunderstood Chris's comment - compress commit on an empty
database will essentially nuke the file. Compress commit in mork gets rid of all
the incremental updates (hence the name Compress). For example, when you delete
a row from a mork database, it doesn't rewrite the whole file; it just adds a
record at the end that says "remove row foo", and row foo remains in the db on
disk. This makes incremental updates of large db's faster. Compress commit
writes out the db from scratch.

I'm not sure who should do the sr - no one on the reviewers list has more
experience with mork than Chris - in fact, no one else on the reviewers list
probably has any experience with mork, the lucky bastards!

Comment 54

[Brendan Eich [:brendan]]

I'm delegating mork review to bienvenu and waterson, which makes it seem like 
this patch is under-reviewed, but I looked at the rest of it and gave an r=.  So 
consider this my sr=.

/be

Comment 55

[Jason Eager]

We have a r=, and a sr=, this needs to go to rtm+ immediately.

Comment 56

[Chris Waterson]

nominating rtm+

Comment 57

[Jim Roskind]

rtm double plus
As I understand this, the modified (new) code is all restricted to the codepath
associated with clearing history.  AS such, it should (worst case) induce
problems only when used, and should not leave us with much in the way of
regressions (if any) unless we exercise the feature.  As such, the risk seems
low enough to land on the branch asap.
Thanks for jumping all over this bug so quickly!!!!! Nice work.
Please land asap on branch.

Comment 58

[Chris Waterson]

Checked in on branch.

Comment 59

[Claudius Gayle]

WooHoo!
VERIFIED Fixed on 2000102509 builds (Win98,MacOS 8.5.1, and Linux RH6)
adding vtrunk keyword so this can be looked at on the trunk as well

Comment 60

[John Unruh]

Verified fixed on Mac, Linux and Win 10/31 trunk builds.