Closed Bug 838874 Opened 11 years ago Closed 11 years ago

Stop implementing nsIGlobalHistory2

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: mak, Assigned: mak)

References

(Blocks 1 open bug)

Details

(Keywords: addon-compat, dev-doc-needed)

Attachments

(1 file, 4 obsolete files)

the interface will survive but Places won't implement it anymore, that basically means any Places-based browser won't have an nsIGlobalHistory2 implementation.
Blocks: 838872
Attached patch patch v1.0 (obsolete) — Splinter Review
Attached patch patch v1.1 (obsolete) — Splinter Review
Attachment #711091 - Attachment is obsolete: true
Attached patch patch v1.2 (obsolete) — Splinter Review
Attachment #711099 - Attachment is obsolete: true
Attached patch patch v1.3 (obsolete) — Splinter Review
more changes
Attachment #711100 - Attachment is obsolete: true
Depends on: 739218
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+
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)
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+
(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)
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.
(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.
notice that other history implementations may still implement nsIGlobalHistory2, the docshell supports both kind of history.
(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.
Attached patch patch v1.4Splinter Review
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+
Blocks: 852948
(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
Blocks: 834498, 834495
https://hg.mozilla.org/mozilla-central/rev/4d5bd5014ce6
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Blocks: 856208
(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.