856208 - Stop using global-history;2 in SeaMonkey code

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[Frank Wein [:mcsmurf]]

See Bug 838874 and http://hg.mozilla.org/mozilla-central/rev/4d5bd5014ce6, nsIGlobalHistory2 has been removed. Code like that still gets used in a few places in SeaMonkey (search for "global-history;2" in mxr).

Comment 1

[Frank Wein [:mcsmurf]]

Attachment: Patch

Fix looked quite easy, I hope this is ok :) pref pane and Sanitize dialog work as expected again.

Comment 2

[Frank Wein [:mcsmurf]]

Attachment: Patch

Forgot to remove an unused var.

Comment 3

[[e-mail]]

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

>-      gGlobalHistory = Components.classes["@mozilla.org/browser/global-history;2"]
>-                                 .getService(Components.interfaces.nsIBrowserHistory);
Any reason not to just replace this with nsINavHistoryService?

>-  var globalHistory = Components.classes["@mozilla.org/browser/global-history;2"]
>-                                .getService(Components.interfaces.nsIBrowserHistory);
These ones are still the correct interface, just need to change the contract.

Comment 4

[Frank Wein [:mcsmurf]]

Neil: All new/changed code seems to use PlacesUtils these days now (and using that also results in shorter code), that is why I used it.

Comment 5

[[e-mail]]

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

>diff --git a/suite/common/contentAreaClick.js b/suite/common/contentAreaClick.js
>--- a/suite/common/contentAreaClick.js
>+++ b/suite/common/contentAreaClick.js
>@@ -1,13 +1,17 @@
> // /* -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> /* This Source Code Form is subject to the terms of the Mozilla Public
>  * License, v. 2.0. If a copy of the MPL was not distributed with this
>  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
>+Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
>+XPCOMUtils.defineLazyModuleGetter(this, "PlacesUtils",
>+                                  "resource://gre/modules/PlacesUtils.jsm");
Use PlacesUIUtils instead, it's already imported.

>diff --git a/suite/common/pref/pref-history.js b/suite/common/pref/pref-history.js
>--- a/suite/common/pref/pref-history.js
>+++ b/suite/common/pref/pref-history.js
>@@ -1,12 +1,16 @@
> /* This Source Code Form is subject to the terms of the Mozilla Public
> * License, v. 2.0. If a copy of the MPL was not distributed with this
> * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
>+Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
>+XPCOMUtils.defineLazyModuleGetter(this, "PlacesUtils",
>+                                  "resource://gre/modules/PlacesUtils.jsm");
No need to load lazily, we need it for startup.

Comment 6

[Frank Wein [:mcsmurf]]

I must admit though that using interfaces directly might be better. One layer less to deal with (and an defined interface, though this interface might also change at any time).

Comment 7

[Frank Wein [:mcsmurf]]

Attachment: Patch

Ok, I think the old method is actually the better one ;) (one level less where things can break in the future and defined interface).

Comment 8

[[e-mail]]

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

>+      gGlobalHistory = Components.classes["@mozilla.org/browser/nav-history-service;1"]
>                                  .getService(Components.interfaces.nsIBrowserHistory);
This is the one bit I forgot to test, but it looks as if markPageAsTyped is a function on nsINavHistoryService, not nsIBrowserHistory. r=me with that fixed, or you could switch to PlacesUIUtils ;-)

Comment 9

[Frank Wein [:mcsmurf]]

Pushed: https://hg.mozilla.org/comm-central/rev/b8bc5996f9ba

Neil: You were right about the markPageAsTyped method. I did test by typing in an URL, but I guess the try...catch prevented any errors from appearing in JS Console (should have checked in JS debugger)? Mainly this happened because I used the nsIBrowserHistory and nsINavHistoryService documentation from devmo, which was out-of-date (markPageAsTyped was moved to nsINavHistoryService one month ago). I also fixed the devmo documentation then ;)

Comment 10

[[e-mail]]

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

>     if (!gGlobalHistory)
>-      gGlobalHistory = Components.classes["@mozilla.org/browser/global-history;2"]
>+      gGlobalHistory = Components.classes["@mozilla.org/browser/nav-history-service;1"]
>                                  .getService(Components.interfaces.nsIBrowserHistory);
As per bug 845895 this should probably use PlacesUIUtils instead.

Comment 11

[Frank Wein [:mcsmurf]]

Attachment: Use PlacesUtils and PlacesUIUtils

Neil: This is a follow-up to fix what you have mentioned. I've tested that "Clear History" in pref window and "Clear Private Data" still works as expected. The change in contentAreaClick.js also seems to work fine. The call to createFixupURI is actually a bit redundant as PlacesUIUtils.markPageAsTyped also calls createFixupURI on its argument. Or should I just call PlacesUtils.history.markPageAsTyped here to avoid that redundant call? Not sure if there is any convention on that.

Comment 12

[[e-mail]]

Comment on attachment 809135 [details] [diff] [review]
Use PlacesUtils and PlacesUIUtils

>-        gGlobalHistory.markPageAsTyped(fixedUpURI);
>+        PlacesUIUtils.markPageAsTyped(fixedUpURI);
fixedUpURI is an nsIURI but PlacesUIUtils wants a string.
If we know that markPageAsTyped ignores data: then we can pass url directly, otherwise go with your other suggestion of PlacesUtils.history instead.

>+Components.utils.import("resource://gre/modules/PlacesUtils.jsm")
Can we do these imports in the functions themselves, to avoid importing when we're not going to use it? (e.g. I don't want seamonkey -preferences to import all of Places.)

Comment 13

[Frank Wein [:mcsmurf]]

Attachment: Use PlacesUtils and PlacesUIUtils, take 2

All fixed, everything seems to work fine.
It looks like markPageAsTyped does not ignore data: URLs, so I used PlacesUtils (I tested it by removing the if check). Firefox does not seem to check for data: URLs though when writing; but their code does not display data: URLs either (I tested it), code is at http://hg.mozilla.org/mozilla-central/annotate/57d160eda301/browser/base/content/browser.js#l3231. So not really sure why their code does not store/display data: URLs, maybe they add data: URLs to urlbar history, but don't display them.

Comment 14

[[e-mail]]

(In reply to Frank Wein from comment #13)
> maybe they add data: URLs to urlbar history, but don't display them.

Might be used for their location bar history (which is a custom query).

Comment 15

[Frank Wein [:mcsmurf]]

Comment on attachment 809218 [details] [diff] [review]
Use PlacesUtils and PlacesUIUtils, take 2

Totally forgot to push this patch, done now: https://hg.mozilla.org/comm-central/rev/edb20fdb7c41

Comment 16

[Justin Wood (:Callek)]

translanted the latter patch on comm-release for SeaMonkey 2.26.1

$ hg tip
changeset:   20173:c9876da9d1ff
branch:      SEA_2_26_1_RELBRANCH
tag:         tip
user:        Frank Wein <[e-mail]>
date:        Fri Mar 07 17:49:01 2014 +0100
summary:     Bug 856208 - Stop using global-history;2 in SeaMonkey code, followup to move more Places modules code, r=Neil