Last Comment Bug 838874 - Stop implementing nsIGlobalHistory2
: Stop implementing nsIGlobalHistory2
Status: RESOLVED FIXED
: addon-compat, dev-doc-needed
Product: Toolkit
Classification: Components
Component: Places (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla22
Assigned To: Marco Bonardo [::mak]
:
: Marco Bonardo [::mak]
Mentors:
Depends on: 739218
Blocks: 834495 852948 834457 834498 838872 856208
  Show dependency treegraph
 
Reported: 2013-02-06 15:44 PST by Marco Bonardo [::mak]
Modified: 2013-04-18 13:17 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1.0 (19.36 KB, patch)
2013-02-06 17:07 PST, Marco Bonardo [::mak]
no flags Details | Diff | Splinter Review
patch v1.1 (20.51 KB, patch)
2013-02-06 17:09 PST, Marco Bonardo [::mak]
no flags Details | Diff | Splinter Review
patch v1.2 (20.51 KB, patch)
2013-02-06 17:10 PST, Marco Bonardo [::mak]
no flags Details | Diff | Splinter Review
patch v1.3 (27.42 KB, patch)
2013-02-07 03:45 PST, Marco Bonardo [::mak]
gavin.sharp: superreview+
asaf: feedback+
Details | Diff | Splinter Review
patch v1.4 (51.70 KB, patch)
2013-02-27 17:02 PST, Marco Bonardo [::mak]
asaf: review+
Details | Diff | Splinter Review

Description Marco Bonardo [::mak] 2013-02-06 15:44:47 PST
the interface will survive but Places won't implement it anymore, that basically means any Places-based browser won't have an nsIGlobalHistory2 implementation.
Comment 1 Marco Bonardo [::mak] 2013-02-06 17:07:13 PST
Created attachment 711091 [details] [diff] [review]
patch v1.0
Comment 2 Marco Bonardo [::mak] 2013-02-06 17:09:45 PST
Created attachment 711099 [details] [diff] [review]
patch v1.1
Comment 3 Marco Bonardo [::mak] 2013-02-06 17:10:34 PST
Created attachment 711100 [details] [diff] [review]
patch v1.2
Comment 4 Marco Bonardo [::mak] 2013-02-07 03:45:50 PST
Created attachment 711244 [details] [diff] [review]
patch v1.3

more changes
Comment 5 Marco Bonardo [::mak] 2013-02-07 05:19:29 PST
Comment on attachment 711244 [details] [diff] [review]
patch v1.3

Ok, Try is happy
Comment 6 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2013-02-09 09:08:02 PST
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?
Comment 7 Marco Bonardo [::mak] 2013-02-26 11:52:57 PST
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
Comment 8 Marco Bonardo [::mak] 2013-02-26 11:53:54 PST
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 9 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-02-26 13:55:55 PST
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?
Comment 10 Marco Bonardo [::mak] 2013-02-26 13:57:57 PST
(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)
Comment 11 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-02-26 14:02:35 PST
dev-doc-needed: once this lands, nsIGlobalHistory2 is no longer implemented in Firefox
Comment 12 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-02-26 14:03:37 PST
(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.
Comment 13 Marco Bonardo [::mak] 2013-02-26 14:06:22 PST
(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.
Comment 14 Marco Bonardo [::mak] 2013-02-26 14:07:09 PST
notice that other history implementations may still implement nsIGlobalHistory2, the docshell supports both kind of history.
Comment 15 Marco Bonardo [::mak] 2013-02-27 16:59:18 PST
(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.
Comment 16 Marco Bonardo [::mak] 2013-02-27 17:02:51 PST
Created attachment 719257 [details] [diff] [review]
patch v1.4

I think I addressed all of the comments
Comment 17 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2013-03-18 03:44:56 PDT
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.
Comment 18 Marco Bonardo [::mak] 2013-03-20 08:27:26 PDT
(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
Comment 20 Ed Morley [:emorley] 2013-03-21 06:05:11 PDT
https://hg.mozilla.org/mozilla-central/rev/4d5bd5014ce6
Comment 22 Marco Bonardo [::mak] 2013-04-18 13:17:53 PDT
(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

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