Closed
Bug 838874
Opened 12 years ago
Closed 12 years ago
Stop implementing nsIGlobalHistory2
Categories
(Toolkit :: Places, defect)
Toolkit
Places
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)
(deleted),
patch
|
asaf
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Comment 1•12 years ago
|
||
Assignee | ||
Comment 2•12 years ago
|
||
Attachment #711091 -
Attachment is obsolete: true
Assignee | ||
Comment 3•12 years ago
|
||
Attachment #711099 -
Attachment is obsolete: true
Assignee | ||
Comment 5•12 years ago
|
||
Comment on attachment 711244 [details] [diff] [review]
patch v1.3
Ok, Try is happy
Attachment #711244 -
Flags: review?(mano)
Comment 6•12 years ago
|
||
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•12 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•12 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 9•12 years ago
|
||
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•12 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)
Updated•12 years ago
|
Keywords: addon-compat
Comment 11•12 years ago
|
||
dev-doc-needed: once this lands, nsIGlobalHistory2 is no longer implemented in Firefox
Keywords: dev-doc-needed
Comment 12•12 years ago
|
||
(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•12 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•12 years ago
|
||
notice that other history implementations may still implement nsIGlobalHistory2, the docshell supports both kind of history.
Assignee | ||
Comment 15•12 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•12 years ago
|
||
I think I addressed all of the comments
Attachment #711244 -
Attachment is obsolete: true
Attachment #719257 -
Flags: review?(mano)
Comment 17•12 years ago
|
||
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 | ||
Comment 18•12 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•12 years ago
|
Assignee | ||
Comment 19•12 years ago
|
||
Target Milestone: --- → mozilla22
Comment 20•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 21•12 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•12 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.
Description
•