The default bug view has changed. See this FAQ.

Stop implementing nsIGlobalHistory2

RESOLVED FIXED in mozilla22

Status

()

Toolkit
Places
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mak, Assigned: mak)

Tracking

(Blocks: 2 bugs, {addon-compat, dev-doc-needed})

Trunk
mozilla22
addon-compat, dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

4 years ago
the interface will survive but Places won't implement it anymore, that basically means any Places-based browser won't have an nsIGlobalHistory2 implementation.
(Assignee)

Updated

4 years ago
Blocks: 838872
(Assignee)

Comment 1

4 years ago
Created attachment 711091 [details] [diff] [review]
patch v1.0
(Assignee)

Comment 2

4 years ago
Created attachment 711099 [details] [diff] [review]
patch v1.1
Attachment #711091 - Attachment is obsolete: true
(Assignee)

Comment 3

4 years ago
Created attachment 711100 [details] [diff] [review]
patch v1.2
Attachment #711099 - Attachment is obsolete: true
(Assignee)

Comment 4

4 years ago
Created attachment 711244 [details] [diff] [review]
patch v1.3

more changes
Attachment #711100 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Depends on: 739218
(Assignee)

Comment 5

4 years ago
Comment on attachment 711244 [details] [diff] [review]
patch v1.3

Ok, Try is happy
Attachment #711244 - Flags: review?(mano)
Comment on attachment 711244 [details] [diff] [review]
patch v1.3

>diff --git a/browser/base/content/sanitize.js b/browser/base/content/sanitize.js

>-        var globalHistory = Components.classes["@mozilla.org/browser/global-history;2"]
>-                                      .getService(Components.interfaces.nsIBrowserHistory);
>+        var history = Components.classes["@mozilla.org/browser/nav-history-service;1"]
>+                                .getService(Components.interfaces.nsIBrowserHistory);

Let's use PlacesUtils.

...but not PlacesUtils.bhistory. Instead, let's fix PlacesUtils.history to also QI to nsIBrowserHistory (I guess we've to keep the bhistory getter for backwards compatibility).


>diff --git a/mobile/xul/chrome/content/sanitize.js b/mobile/xul/chrome/content/sanitize.js

>+        var history = Cc["@mozilla.org/browser/nav-history-service;1"].getService(Ci.nsIBrowserHistory);
>+        history.removeAllPages();
>         

ditto, and remove the trailing spaces while you're there.


>diff --git a/toolkit/components/places/PlacesUtils.jsm b/toolkit/components/places/PlacesUtils.jsm

> 
>-XPCOMUtils.defineLazyGetter(PlacesUtils, "ghistory2", function() {
>-  return PlacesUtils.history.QueryInterface(Ci.nsIGlobalHistory2);
>-});
>-

It was nice to find out no addons are using this getter :)


>diff --git a/toolkit/components/places/nsIBrowserHistory.idl b/toolkit/components/places/nsIBrowserHistory.idl

>+[scriptable, uuid(20d31479-38de-49f4-9300-566d6e834c66)]
>+interface nsIBrowserHistory : nsISupports

Please file a follow up for a possible merge of this interface to the nsINavHistory.


>diff --git a/toolkit/components/places/nsPlacesAutoComplete.js b/toolkit/components/places/nsPlacesAutoComplete.js


>   XPCOMUtils.defineLazyServiceGetter(this, "_bh",
>-                                     "@mozilla.org/browser/global-history;2",
>+                                     "@mozilla.org/browser/nav-history-service;1",
>                                      "nsIBrowserHistory");
> 

Any reason PlacesUtils isn't used here?



>diff --git a/toolkit/components/places/tests/bookmarks/test_savedsearches.js b/toolkit/components/places/tests/bookmarks/test_savedsearches.js

> // Get global history service
> try {
>-  var bhist = Cc["@mozilla.org/browser/global-history;2"].getService(Ci.nsIBrowserHistory);
>+  var bhist = Cc["@mozilla.org/browser/nav-history-service;1"].getService(Ci.nsIBrowserHistory);

Please fix the comment.

>diff --git a/toolkit/components/places/tests/unit/test_415757.js b/toolkit/components/places/tests/unit/test_415757.js

> try {
>-  var bhist = Cc["@mozilla.org/browser/global-history;2"]
>+  var bhist = Cc["@mozilla.org/browser/nav-history-service;1"]
>                 .getService(Ci.nsIBrowserHistory);
> } catch(ex) {
>   do_throw("Could not get history service\n");
> }

ditto.

> // Get history service
> try {
>   var histsvc = Cc["@mozilla.org/browser/nav-history-service;1"]

Hrm, well, it would be nice to avoid one of them.

>diff --git a/toolkit/forgetaboutsite/ForgetAboutSite.jsm b/toolkit/forgetaboutsite/ForgetAboutSite.jsm
>--- a/toolkit/forgetaboutsite/ForgetAboutSite.jsm
>+++ b/toolkit/forgetaboutsite/ForgetAboutSite.jsm

>-    let (bh = Cc["@mozilla.org/browser/global-history;2"].
>+    let (bh = Cc["@mozilla.org/browser/nav-history-service;1"].
>               getService(Ci.nsIBrowserHistory)) {

PlacesUtils?
Attachment #711244 - Flags: review?(mano) → feedback+
(Assignee)

Comment 7

4 years ago
Comment on attachment 711244 [details] [diff] [review]
patch v1.3

since the review comments don't involve changes to the deprecation we can proceed with the SR in parallel
Attachment #711244 - Flags: superreview?(gavin.sharp)
(Assignee)

Comment 8

4 years ago
Note that this patch doesn't actually do anything to nsIGlobalHistory2, the change involved here is that Firefox won't have anymore a nsIGlobalHistory2 implementation, trying to get one will throw.
Comment on attachment 711244 [details] [diff] [review]
patch v1.3

Looks like this will need some browser/metro/base/content/sanitize.js changes as well, now that that has landed in m-c.

Is there a bug tracking getting rid of the contract ID/interface/nsDownloadHistory entirely?

What implementation is test_nsIDownloadHistory.js testing if not this one? Wouldn't that test be failing with this patch?
Attachment #711244 - Flags: superreview?(gavin.sharp) → superreview+
(Assignee)

Comment 10

4 years ago
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #9)
> Comment on attachment 711244 [details] [diff] [review]
> patch v1.3
> 
> Looks like this will need some browser/metro/base/content/sanitize.js
> changes as well, now that that has landed in m-c.

ugh.

> Is there a bug tracking getting rid of the contract
> ID/interface/nsDownloadHistory entirely?
> What implementation is test_nsIDownloadHistory.js testing if not this one?
> Wouldn't that test be failing with this patch?

nsDownloadHistory is not something we care about, it's just a basic implementation of nsIDownloadHistory, but Places has its own implementation of nsIDownloadHistory that overrides that one (and is already async)
Keywords: addon-compat
dev-doc-needed: once this lands, nsIGlobalHistory2 is no longer implemented in Firefox
Keywords: dev-doc-needed
(In reply to Marco Bonardo [:mak] (Away Mar 1) from comment #10)
> nsDownloadHistory is not something we care about, it's just a basic
> implementation of nsIDownloadHistory, but Places has its own implementation
> of nsIDownloadHistory that overrides that one (and is already async)

Since it delegates to nsIGlobalHistory2, it seems useless if we never implement nsIGlobalHistory2, so it seems like we should remove it.
(Assignee)

Comment 13

4 years ago
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #12)
> Since it delegates to nsIGlobalHistory2, it seems useless if we never
> implement nsIGlobalHistory2, so it seems like we should remove it.

Well, I'm not sure if others not using Places rely on having a nsIDownloadHistory implementation, that was existing before us and it's definitely possible to build without Places, I may file a bug in docshell to investigate removing it.
(Assignee)

Comment 14

4 years ago
notice that other history implementations may still implement nsIGlobalHistory2, the docshell supports both kind of history.
(Assignee)

Comment 15

4 years ago
(In reply to Mano from comment #6)
> >diff --git a/toolkit/components/places/nsIBrowserHistory.idl b/toolkit/components/places/nsIBrowserHistory.idl
> 
> >+[scriptable, uuid(20d31479-38de-49f4-9300-566d6e834c66)]
> >+interface nsIBrowserHistory : nsISupports
> 
> Please file a follow up for a possible merge of this interface to the
> nsINavHistory.

Even better, we will completely remove it in bug 739219. Just matter of writing new remove methods, replace and deprecate the old ones, and then remove.
(Assignee)

Comment 16

4 years ago
Created attachment 719257 [details] [diff] [review]
patch v1.4

I think I addressed all of the comments
Attachment #711244 - Attachment is obsolete: true
Attachment #719257 - Flags: review?(mano)
Comment on attachment 719257 [details] [diff] [review]
patch v1.4

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

::: toolkit/components/places/PlacesUtils.jsm
@@ +2124,5 @@
>                                     "@mozilla.org/browser/history;1",
>                                     "mozIAsyncHistory");
>  
>  XPCOMUtils.defineLazyGetter(PlacesUtils, "bhistory", function() {
> +  return PlacesUtils.history;

Please file a bug for removing this getter.

::: toolkit/components/places/nsPlacesAutoComplete.js
@@ +305,5 @@
>      // Get a cloned, read-only version of the database.  We'll only ever write
>      // to our own in-memory temp table, and having a cloned copy means we do not
>      // run the risk of our queries taking longer due to the main database
>      // connection performing a long-running task.
> +    let db = PlacesUtils.history

missing dot.

::: toolkit/components/places/nsPlacesModule.cpp
@@ -44,5 @@
>  };
>  
>  const mozilla::Module::ContractIDEntry kPlacesContracts[] = {
>    { NS_NAVHISTORYSERVICE_CONTRACTID, &kNS_NAVHISTORYSERVICE_CID },
> -  { NS_GLOBALHISTORY2_CONTRACTID, &kNS_NAVHISTORYSERVICE_CID },

You could probably remove the nsDocShellCID.h inclusion.
Attachment #719257 - Flags: review?(mano) → review+
(Assignee)

Updated

4 years ago
Blocks: 852948
(Assignee)

Comment 18

4 years ago
(In reply to Mano from comment #17)
> >  XPCOMUtils.defineLazyGetter(PlacesUtils, "bhistory", function() {
> > +  return PlacesUtils.history;
> 
> Please file a bug for removing this getter.

bug 852948

> ::: toolkit/components/places/nsPlacesAutoComplete.js
> > +    let db = PlacesUtils.history
> 
> missing dot.

I actually fixed this locally when making PlacesUtils.history QI to nsPIPlacesDatabase

> ::: toolkit/components/places/nsPlacesModule.cpp
> @@ -44,5 @@
> >  };
> >  
> >  const mozilla::Module::ContractIDEntry kPlacesContracts[] = {
> >    { NS_NAVHISTORYSERVICE_CONTRACTID, &kNS_NAVHISTORYSERVICE_CID },
> > -  { NS_GLOBALHISTORY2_CONTRACTID, &kNS_NAVHISTORYSERVICE_CID },
> 
> You could probably remove the nsDocShellCID.h inclusion.

Nope. it's still used for IHistory
(Assignee)

Updated

4 years ago
Blocks: 834498, 834495
(Assignee)

Comment 19

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d5bd5014ce6
Target Milestone: --- → mozilla22
https://hg.mozilla.org/mozilla-central/rev/4d5bd5014ce6
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED

Updated

4 years ago
Blocks: 856208

Comment 21

4 years ago
At least these pages will need to be updated:

https://developer.mozilla.org/en-US/docs/Using_the_Places_history_service
https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIGlobalHistory2

What's the right API to use now instead of isVisited?  https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/mozIAsyncHistory ?
(Assignee)

Comment 22

4 years ago
(In reply to Jesse Ruderman from comment #21)
> What's the right API to use now instead of isVisited? 
> https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/
> mozIAsyncHistory ?

isURIVisited
You need to log in before you can comment on or make changes to this bug.