Stop using global-history;2 in SeaMonkey code

RESOLVED FIXED in seamonkey2.19

Status

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: mcsmurf, Assigned: mcsmurf)

Tracking

Trunk
seamonkey2.19
Dependency tree / graph

SeaMonkey Tracking Flags

(seamonkey2.26 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

6 years ago
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).
(Assignee)

Comment 1

6 years ago
Posted patch Patch (obsolete) — Splinter Review
Fix looked quite easy, I hope this is ok :) pref pane and Sanitize dialog work as expected again.
Assignee: nobody → bugzilla
Status: NEW → ASSIGNED
Attachment #731350 - Flags: review?(neil)
(Assignee)

Comment 2

6 years ago
Posted patch Patch (obsolete) — Splinter Review
Forgot to remove an unused var.
Attachment #731350 - Attachment is obsolete: true
Attachment #731350 - Flags: review?(neil)
Attachment #731351 - Flags: review?(neil)
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.
(Assignee)

Comment 4

6 years ago
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.
Summary: Stop using nsIGlobalHistory2 in SeaMonkey code → Stop using global-history;2 in SeaMonkey code
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.
(Assignee)

Comment 6

6 years ago
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).
(Assignee)

Comment 7

6 years ago
Posted patch PatchSplinter Review
Ok, I think the old method is actually the better one ;) (one level less where things can break in the future and defined interface).
Attachment #731351 - Attachment is obsolete: true
Attachment #731351 - Flags: review?(neil)
(Assignee)

Updated

6 years ago
Attachment #731592 - Flags: review?(neil)
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 ;-)
Attachment #731592 - Flags: review?(neil) → review+
(Assignee)

Comment 9

6 years ago
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 ;)
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.19
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.
(Assignee)

Comment 11

6 years ago
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.
Attachment #809135 - Flags: review?(neil)
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.)
Attachment #809135 - Flags: review?(neil) → review-
(Assignee)

Comment 13

6 years ago
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.
Attachment #809135 - Attachment is obsolete: true
Attachment #809218 - Flags: review?(neil)
Attachment #809218 - Flags: review?(neil) → review+
(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).
(Assignee)

Comment 15

5 years ago
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

Updated

5 years ago
Blocks: 1018792
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 <mcsmurf@mcsmurf.de>
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
You need to log in before you can comment on or make changes to this bug.