615219 - [ChatZilla] Start using nsIGlobalHistory2, maybe stop support for nsIGlobalHistory

Status: RESOLVED FIXED

(Other Applications :: ChatZilla, defect)

Description

[Serge Gautherie (:sgautherie)]

(Noticed while investigating bug 603031.)


http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsIGlobalHistory.idl
{
42  * @status DEPRECATED. This interface is still accepted, but new
43  *         implementations should use nsIGlobalHistory2.

52     /**
53      * addPage
54      * Add a page to the history
55      *
56      * @param aURL the url to the page
57      */
58 
59     void addPage(in string aURL);
}

http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsIGlobalHistory2.idl
{
58     /**
59      * Add a URI to global history
60      *
61      * @param aURI      the URI of the page
62      * @param aRedirect whether the URI was redirected to another location;
63      *                  this is 'true' for the original URI which is
64      *                  redirected.
65      * @param aToplevel whether the URI is loaded in a top-level window
66      * @param aReferrer the URI of the referring page
67      *
68      * @note  Docshell will not filter out URI schemes like chrome: data:
69      *        about: and view-source:.  Embedders should consider filtering out
70      *        these schemes and others, e.g. mailbox: for the main URI and the
71      *        referrer.
72      */
73     void addURI(in nsIURI aURI, in boolean aRedirect, in boolean aToplevel, in nsIURI aReferrer);
}

I don't know whether nsIGlobalHistory support needs to be kept for the older application versions we care about now.

Comment 1

[Serge Gautherie (:sgautherie)]

Per bug 615213 comment 2, this is now major :-<

Comment 2

[Serge Gautherie (:sgautherie)]

"blocking-seamonkey2.1=?":
if not blocking then surely wanted...

Comment 3

[Philip Chee]

According to MXR nsIGlobalHistory2 is available all the way back to Mozilla 1.7 and the Aviary branch.

http://mxr.mozilla.org/mozilla1.7/find?string=nsIglobalHistory&tree=mozilla1.7&hint=
http://mxr.mozilla.org/aviary101branch/find?string=nsIglobalHistory&tree=aviary101branch&hint=

nsIGlobalHistory2 checked in 2004-02-10 18:09
https://bugzilla.mozilla.org/show_bug.cgi?id=224829#c37
According to Bug 224829 Comment 37 there was a backward compatibility adapter that existed only because of Chatzilla which chatzilla apparently never used :P

> nsIGlobalHistory2->nsIGlobalHistory
>   (nsGlobalHistory2Adapter, for use by extensions such as chatzilla which want
>    nsIGlobalHistory)

But grepping though the Chatzilla source code it looks like just a SMOP to change the callers.

Comment 4

[Serge Gautherie (:sgautherie)]

Robert, can it be expected that you work on this bug or is it helpwanted?

Comment 5

[Justin Wood (:Callek)]

As long as nsIGlobalHistory is still present in 2.1, I'm not even marking this as wanted; since it is a 3rd party extension, we happen to ship with.

But I would like to see this as well.

Comment 6

[:Gijs (he/him)]

Attachment: Patch

nsIGlobalHistory2 has been around since 2007. As we've updated compat requirements to beyond that date, I believe it's safe to just switch wholesale. This will also allow us to release a Fx15-compatible version to AMO.

Comment 7

[:Gijs (he/him)]

(In reply to Gijs Kruitbosch from comment #6)
> since 2007.

Excuse me, that was supposed to be *at least* 2007 - I didn't bother checking CVS as it was in the original hg import.

Comment 8

[Serge Gautherie (:sgautherie)]

Comment on attachment 658241 [details] [diff] [review]
Patch

Does this patch actually work? I mean "global-history;1" and "addPage()"...

Comment 9

[:Gijs (he/him)]

Comment on attachment 658241 [details] [diff] [review]
Patch

(In reply to Serge Gautherie (:sgautherie) from comment #8)
> Comment on attachment 658241 [details] [diff] [review]
> Patch
> 
> Does this patch actually work? I mean "global-history;1" and "addPage()"...

Fair point. I totally blame the AMO warnings that claim "You can use nsIGlobalHistory2 instead". Sigh. Proper patch coming tonight.

Comment 10

[:Gijs (he/him)]

Attachment: Patch [checked in]

This has the benefit of actually being tested and stuff. AFAICT this should work on everything we still support.

Comment 11

[James Ross]

Comment on attachment 658568 [details] [diff] [review]
Patch [checked in]

Review of attachment 658568 [details] [diff] [review]:
-----------------------------------------------------------------

::: xul/content/static.js
@@ +945,5 @@
>      destroyPrefs();
>  }
>  
> +
> +function addURLToHistory(url, referer) {

Nit: Braces go on new lines (and elsewhere). :)

@@ +947,5 @@
>  
> +
> +function addURLToHistory(url, referer) {
> +    if (client.globalHistory) {
> +        referer = referer ? client.iosvc.newURI(referer, "UTF-8", null) : null;

The referer is never used, though I guess it might be handy; did you have anything in mind here or just future utility?

Comment 12

[:Gijs (he/him)]

Checked in with nits, http://hg.mozilla.org/chatzilla/rev/8e70231eca3d .

(In reply to James Ross from comment #11)
> The referer is never used, though I guess it might be handy; did you have
> anything in mind here or just future utility?

Pretty much just futureproofing (I know, I know).