235915 - RemovePage method is no longer part of nsIBrowserHistory interface

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Son Le]

User-Agent:       
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7b) Gecko/20040226 Firefox/0.8.0+

The checkin for bug 224829 deleted the RemovePage method from the
nsIBrowserHistory interface and broke extensions that rely on this method (eg.
LinkVisitor).

The aim of this bug is to reverse the parts of the 224829 checkin that removed
the method declaration and add it back into the nsIBrowserHistory interface.

Reproducible: Always
Steps to Reproduce:

Comment 1

[Benjamin Smedberg]

who's gonna make a patch? I can very simply make a patch to move
nsGlobalHistory::RemovePage back into the ffox nsIBrowserHistory interface. If
someone wants to make it use nsIURI instead of strings, go ahead.

Comment 2

[Alec Flett]

I'll review the patch and then we can leave this open - after your stuff has
landed I can try to make another patch to make it use nsIURI.. 

can we un-fork this interface in preparation for freezing?

Comment 3

[Benjamin Smedberg]

Attachment: Put removePage() back into ffox nsIBrowserHistory

I don't think we should freeze nsIBrowserHistory, and I don't think we need to
unfork it.

Comment 4

[Alec Flett]

hmm... its not that it would be part of an embedded application, but instead
part of nsGlobalHistory, frozen so that privacy extensions to mozilla/firefox
can actually take advantage of the interface without breaking all the time...
there are a few methods that would obviously move into a non-frozen interface,
such as lastPageVisited, maybe count, and maybe markPagesAsTyped...

Comment 5

[Son Le]

The patch looks nice and simple. Alex, can you do a quick review?

Comment 6

[Benjamin Smedberg]

I'm not sure these blocking flags do any good, but...

Comment 7

[Alec Flett]

will do, sonia.

Comment 8

[Son Le]

:-)

Comment 9

[Son Le]

Using djeter's Win32 2004-04-16 trunk build with patch applied
(http://forums.mozillazine.org/viewtopic.php?t=69620) and updated LinkVisitor
code (1 line changed) works like a charm.

Comment 10

[lamekisser]

(In reply to comment #9)
> Using djeter's Win32 2004-04-16 trunk build with patch applied
> (http://forums.mozillazine.org/viewtopic.php?t=69620) and updated LinkVisitor
> code (1 line changed) works like a charm.

I tried djeter's win32 2004-04-16 but nsIBrowserHistory.removePage did not
return expected result. I entered these in Venkman:

var bh = Components.classes["@mozilla.org/browser/global-history;2"];
bh = bh.getService(Components.interfaces.nsIBrowserHistory);

var gh = Components.classes['@mozilla.org/browser/global-history;2'];
gh = gh.getService(Components.interfaces.nsIGlobalHistory2);

var uf = Components.classes["@mozilla.org/docshell/urifixup;1"];
uf = uf.getService(Components.interfaces.nsIURIFixup);

var uri1 = uf.createFixupURI('http://www.abcd1234.com/', 0);

gh.isVisited(uri1); // false, okay

gh.addURI(uri1, false, true);

gh.isVisited(uri1); // true, okay

bh.removePage(uri1); // remove the newly added page

gh.isVisited(uri1); // returns true, should be false

Please advise, thanks.

Comment 11

[Son Le]

(In reply to comment #10)

> I tried djeter's win32 2004-04-16 but nsIBrowserHistory.removePage did not
> return expected result. I entered these in Venkman:
>
> bh.removePage(uri1); // remove the newly added page

The confusion is here. The removePage() function is declared as 

void removePage(in string aURI);

and takes a string url and not aURI as the parameter name suggests. When I
changed the function call to bh.removePage(uri1.spec), it works as expected.

Comment 12

[Asa Dotzler [:asa]]

We should get this. Adding blocking flag and targeting. 

Comment 13

[lamekisser]

(In reply to comment #11)
> The confusion is here. The removePage() function is declared as 
> void removePage(in string aURI);

Sorry, my mistake.
I tried it and now it is working :)
Thanks.

Comment 14

[Mike Connor [:mconnor]]

Comment on attachment 142671 [details] [diff] [review]
Put removePage() back into ffox nsIBrowserHistory

looks good, r=[e-mail], please get this in on the branch

Comment 15

[Mike Connor [:mconnor]]

checked in branch and trunk, 2004-05-05 16:43