Closed Bug 224829 Opened 21 years ago Closed 21 years ago

nsIGlobalHistory/nsIBrowserHistory introduces bad dependencies between toolkits and docshell

Categories

(Core Graveyard :: Embedding: GRE Core, defect, P1)

Product:
Core Graveyard
Component:
Embedding: GRE Core
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla1.7alpha

People

(Reporter: benjamin, Assigned: benjamin)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

Updated with review comments, nsDocShellCID.h, and properly hiding frames and redirects
118.44 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
grr, more bad dependencies between various toolkits and core gecko stuff. This time it's nsIGlobalHistory, which is forked between the toolkits but referenced by docshell/uriloader. I propose moving nsIGlobalHistory.idl into docshell/base. Can I get r/sr=bz and assent from some *bird person (Pierre)? I will get leaf or someone to do the CVS-move-thing to preserve blame and history. --BDS
>which is forked between the toolkits actually the content of the file is the same in both. even the REQUIRES line is the same, iirc. and note that doing this would not help much, because docshell still depends on nsIBrowserHistory.idl
oh yeah, I forgot to mention that nsIGlobalHistory is @status FROZEN, so this is merely a build-location cleanup. nsIBrowserHistory is more complex: pch? this file seems already to be changed in the new toolkit. Options: 1) make docshell not need nsIBrowserHistory (I don't know whether this is feasible) 2) make a core nsIBrowserHistory that docshell uses and extend it with seamonkey/firebird inherited interfaces as appropriate or something else? --BDS
Summary: nsIGlobalHistory introduces bad dependencies between toolkits and docshell → nsIGlobalHistory/nsIBrowserHistory introduces bad dependencies between toolkits and docshell
> 1) make docshell not need nsIBrowserHistory (I don't know whether this is > feasible) Not easily. bsmedberg is right. The IDL should live in gecko and be frozen (once we decide on a reasonable interface that all embeddors could implement); if embeddors need other history functionality they should put it on other interfaces specific to their embedding apps.
I am not sure that nsIGlobalHistory and nsIBrowserHistory should belong the Gecko (and thus the GRE). Thunderbird, the standalone Composer, calendar don't need it. I would rather see them at the app level and the autocomplete stuff (another bad dependency) at the toolkit level. But since the former is frozen, that would create some headaches to ourselves and embeddors.
nsIGlobalHistory is needed for core gecko services like :visited links, so it has to be part of gecko, without question. browser history is more nebulous... here are the functions of nsIBrowserHistory that docshell uses: .setPageTitle http://lxr.mozilla.org/mozilla/source/docshell/base/nsDocShell.cpp#3376 .hidePage #6547 #6575 .setLastPageVisited #6568 Pierre, do you have a specific plan for making docshell not need these methods (remembering the docshell is used by embedding clients as well)? Could we just fork off these methods onto a nsIGlobalHistory2 and let the rest nsIBrowserHistory be forked and in flux? --BDS
The dochshell has to hook into history somehow and somewhere, as does the style resolution code. So there needs to be a history API in the core (with all implementations of it outside the core and the core able to handle the lack of a service implementing that interface, which will be the case with tbird, eg).
To apply this patch, first move nsIGlobalHistory.idl from xpfe/components/history/public to docshell/base. (This will be done using a CVS move to preserve history). You can also then remove toolkit/components/public/nsIGlobalHistory.idl
Comment on attachment 135192 [details] [diff] [review] Separate nsIGlobalHistoryEx and (forked) nsIBrowserHistory hrm, it seems that the aviary-review email doesn't work yet?
Attachment #135192 - Flags: superreview?(bz-vacation)
Attachment #135192 - Flags: review?(p_ch)
It'll take me at least a few days to get to this...
Comment on attachment 135192 [details] [diff] [review] Separate nsIGlobalHistoryEx and (forked) nsIBrowserHistory removing myself from the reviewer field. My last comment clearly disqualify me :) Establishing interfaces must be dealt with a lot of care, so pick up a more knowledgeable reviewer than me. I will post some comments within few days and then give an approval for the toolkit changes. though.
Attachment #135192 - Flags: review?(p_ch)
Comment on attachment 135192 [details] [diff] [review] Separate nsIGlobalHistoryEx and (forked) nsIBrowserHistory +SDK_XPIDLSRCS = \ + nsIGlobalHistory.idl \ + nsIGlobalHistoryEx.idl \ I don't think you're supposed to put non-frozen interfaces into the sdk, and the *Ex one isn't frozen. also, I dislike *Ex as an interface name. maybe nsIGlobalHistory2 would be better... + * This interface was extracted from xpfe/components/history/public/nsIBrowserHistory.idl not sure if this historic information is of value to users of this interface... + * setPageTitle well, that's kinda redundant... especially if you do use doxygen to extract the documentation.
> not sure if this historic information is of value to users of this interface... Sure as heck helps look up CVS blame... ;)
Comment on attachment 135192 [details] [diff] [review] Separate nsIGlobalHistoryEx and (forked) nsIBrowserHistory I used "ex" because that's what I'm familiar with as a Microsoft-ism. "2" is fine if that's what you and bz desire. I would like to keep the historic reference because I didn't write the interface from scratch, but copy/pasted verbatim from the old file (which is where the weird javadoc comments come from). --BDS
Attachment #135192 - Flags: review?(cbiesinger)
Comment on attachment 135192 [details] [diff] [review] Separate nsIGlobalHistoryEx and (forked) nsIBrowserHistory ok... so while I can't find any reference on this, I'm pretty sure you should not put non-frozen interfaces in the SDK. Hence, please remove nsIGlobalHistoryEx.idl from the SDK_XPIDLSRCS line. I'd still prefer you to change the javadoc comments to remove the function name in the comment ifdef MOZ_PHOENIX REQUIRES += toolkitcomps endif is that still needed? for the name, I'd prefer nsIGlobalHistory2. per bz's comment, feel free to leave the reference to nsIBrowserHistory in the file. r=biesi with that
Attachment #135192 - Flags: review?(cbiesinger) → review+
Comment on attachment 135192 [details] [diff] [review] Separate nsIGlobalHistoryEx and (forked) nsIBrowserHistory >Index: Makefile.in >-ifdef MOZ_XUL_APP >-tier_9_dirs += toolkit/components/history/public chrome >-else >-tier_9_dirs += xpfe/components/history/public >-endif No need for "chrome" anymore? Why not? >Index: docshell/base/Makefile.in >+SDK_XPIDLSRCS = \ >+ nsIGlobalHistory.idl \ >+ nsIGlobalHistoryEx.idl \ Biesi is right -- don't put non-frozen interfaces in the SDK. >Index: docshell/base/nsIGlobalHistoryEx.idl Biesi is again right. Make that nsIGlobalHistory2.idl. The "Ex" approach does not scale well to screwing up more than one revision of an interface, and I'm sure we will. >Index: uriloader/exthandler/Makefile.in >- history \ >+ docshell \ uriloader is linked into docshell. Do you still need that requires? sr=bzbarsky with those changes and nits.
Attachment #135192 - Flags: superreview?(bz-vacation) → superreview+
I am not sure we're heading to the right direction, here. I am not familiar with this code, that's why it took some time. > Pierre, do you have a specific plan for making docshell not need these methods? I'd like to submit one, at least for hidePage and setLastVisitedPage methods. These two methods are only called in nsDocShell::AddToGlobalHistory. In fact, the call to these two methods should be moved in nsGlobalHistory::addPage. That's where the embeddor should decide what to do with a new entry: dealing with javascript url, flag it as the last visited page, calculate weights for the autocomplete entries etc. That's none of Gecko business. Concerning setPageTitle, I am not sure what to do, I should think a bit more about it.
> In fact, the call to these two methods should be moved in > nsGlobalHistory::addPage What does it mean for nsIGlobalHistory to be frozen? Does that mean that Gecko will not be changing the way it accesses it? But yes, in general I think we should move all that logic into the history impl. In fact, the ShouldAddToGlobalHistory logic should probably move into the history impl. Again, that's an implicit API change from an embeddor's point of view, since suddenly their product's history will break (will be showing all sorts of stuff they may not want shown)....
> the ShouldAddToGlobalHistory logic should probably move into the history impl. I don't think that ShouldAddToGlobalHistory should be moved to the global history implementation (even if you are not yet proposing to do so, if I understand correctly) Basically, this method rejects the protocols that are not handled by gecko. That's really a gecko-level information. Embeddors still have the possibility to reject protocols that are handled by gecko or any url in addPage. It makes not sense for them to reject a protocol that gecko can not handle itself.
> What does it mean for nsIGlobalHistory to be frozen? Does that mean that Gecko > will not be changing the way it accesses it? The problem is that the proposed nsIGlobalHistory2 interface contains random methods (lastPageVisited: only used by the browser, hidePage: only used by the autocomplete widget). I don't see how it could pretend one day to the frozen status. I think we should stick with the definition of API: gecko won't change the way it accesses it. But we should also annouce the changes. Imo, the long term benefits outperforms the pains as it will provide simpler (not laughed at) interface for new embeddors. Note that adding a new interface is also a pain for embeddors.
> Basically, this method rejects the protocols that are not handled by gecko. On the contrary. Every single one of these protocols is handled by Gecko; the ones that are not never make it down into this code. The only reason this code is there is because nsGlobalHistory did not do such filtering itself, as far as I can tell (the code predates niceties like bugzilla bug numbers in checkin comments, so I can't be sure, of course); chances are, it was because we didn't want those URLs in the autocomplete dropwdown. For example, why don't we add view-source: urls to global history? What if an embeddor wants to have :visited working on such? Right now that is impossible. My point is that we're drawing an artificial boudnary between the places where hidePage() is called and the places where ShouldAddToGlobalHistory is called -- they are really no different. Here is my proposal, for what it's worth (or at least the direction my thoughts are running in right now... I'm not completely sure how feasible or desirable this is yet): 1) Create nsIGlobalHistory2. Make it properly use nsIURI instead of string as the URL key. Make it clear that all URLs that docshell sees will be passed to it. People implementing it are expected to do their own filtering, hiding, whatever. 2) Create a small class that wraps an nsIGlobalHistory impl and implements nsIGlobalHistory2. If desired, this can even be exposed via contractid. 3) Have docshell QI whatever nsIGlobalHistory it gets to nsIGlobalHistory2. If that fails, wrap it in above class. 4) Have docshell call nsIGlobalHistory2 methods only. The ShouldAdd* logic can then move into the helper class. Embeddors that implement nsIGlobalHistory will see no behavior change. Embeddors that want more control over what history does should implement nsIGlobalHistory2. At the same time, perhaps history should move out of "xpfe" or wherever it lives now and become an embedding component that embeddors can just use in their apps as-is if desired... Thoughts? Another option, of course, is to have a "history filter" component that does what ShouldAddToGlobalHistory/HidePage/whatever do right now and to provide one by default with Gecko but allow embeddors to override it.
this new interface is not cohesive. why are we seperating the existing interface?
Doug, the goal here is to figure out what parts of the history interface are of interest to embeddors in general (as in, Gecko will be calling those methods) and what parts are only of interest to the particular firebird impl (as in, the UI will be calling those methods). That's the basis for the separation.
> At the same time, perhaps history should move out of "xpfe" or wherever it lives > now and become an embedding component that embeddors can just use in their apps > as-is if desired... Well, that's pretty much the way it is... currently gecko doesn't provide any default history impl at all. There's a super-simple history provider in embedding/, and there are the more complex history providers in xpfe/ and toolkit/ (just pretend that xpfe/ and toolkit/ are embedding applications like any other). --BDS
Yeah, that's the way I think of them. I meant that perhaps we should consider having a not-quite-so-simple history provider in embedding/ ;) In any case, that's orthogonal to this bug.
> xpfe/ and toolkit/ are embedding applications like any other yes they are. I am still confused that with all these presshell, webshell and docshell layers, none has to be implemented by the client. To add a bit of confusion, but also to try to get a broader view, I'd like to bring to your attention that the docshell depends also on the back/forward history (nsISHEntry etc.) whose idl files are also in mozilla/xpfe. Is it impossible to let some of the docshell implementation up to the embeddor ? I think that unless the GRE is kept simple, there won't be any chances to freeze its interfaces.
> Is it impossible to let some of the docshell implementation up to the embeddor ? What parts? Since the history impl is already supplied by the embedding app, we only need to unfork the interfaces, not the implementations. Or has this discussion morphed? nsISHistory.idl is almost exactly like this case. The nsISHistory interface and nsISHistoryListener are frozen and just need to be moved into gecko. However, the nsISHistoryInternal interface is not frozen and is used by docshell. --BDS
> whose idl files are also in mozilla/xpfe. They need to move too. > Is it impossible to let some of the docshell implementation up to the > embeddor? That's what we're doing. ;) We're pushing the history part of the docshell impl up to the embeddor. Interfaces in the GRE are of two kinds. There are the ones that the GRE implements. And there are the ones the GRE expects the embeddor to implement. Both types of IDL files have to live in the GRE. History is in the latter category. We could push the history impl into the GRE and make all the history interfaces internal (and not interfaces, as desired). But I doubt that's wanted. ;)
By the way, as far as session history goes I would almost say the impl should also move into the docshell library.... I dunno how well we handle a nonexistent or "nonstandard" session history impl, but I doubt we do well.
I've gone back and rethought the global history a bit... I'm liking bz's idea more and more... create a new comprehensive nsIGlobalHistory2 using the new string classes and nsIURI, and make a little wrapper so that the frozen nsIGlobalHistory still works.
And the other way around? How about cutting off the tree a bit more? Why couldn't we move the parts of the docshell involving the history out of the GRE? That would imply to let the clients define, only if they need it, their own nsIBrowserHistory and history sessions implementation *and* interfaces or whatever according to how they want to organize their app. The only (in addition to the docshell subset) mandatory implementation would still be the good old nsIGlobalHistory that only contains what the layout needs. That would also give them the possibility to add cpp hooks not specifically related to history (in loadURI esp.), without compromizing the GRE permanence and giving us a chance to freeze these API.
> Why couldn't we move the parts of the docshell involving the history out of the > GRE? This is about session history, right? As you noted, global history has these style system dependencies... It could maybe be done, replacing it with an observer-type system and lots and lots of testing of all sorts of stuff to make sure it didn't break. Think partial loads, aborted loads, error pages, etc, etc. I would rather we not try to rewrite all of session history immediately (a multi-year project) under the aegis of this bug...
This is a work-in-progress patch for my new plan: replace nsIGlobalHistory with an entirely new interface, and have a small adapter-class so that we don't break the frozen embedding interface.
Attachment #135192 - Attachment is obsolete: true
Comment on attachment 138044 [details] [diff] [review] replace nsIGlobalHistory with something better, and use an adapter bz, this isn't complete, but before I go whacking at my entire tree can you look at this and see if the API is ok?
Attachment #138044 - Flags: review?(bz-vacation)
Comment on attachment 138044 [details] [diff] [review] replace nsIGlobalHistory with something better, and use an adapter + adapter->AddRef(); why not NS_ADDREF? + * @param aRedirect whether the URI was redirected to another location So what happens if uri A gets redirected to uri B? Will addURI be called twice? if yes, will aRedirect in both cases? if no, which one? If it will be called once, which URI will be given to addURI? it would be great to document that. +#define NS_GLOBALHISTORY2_CONTRACTID \ + "@mozilla.org/browser/global-history;2" contract ids should not be put into IDL files. hm... docshell still depends on nsIBrowserHistory right? At least for lastPageVisited. shouldn't that be fixed as well?
I am not likely to get to this before the 15th.
Comment on attachment 138044 [details] [diff] [review] replace nsIGlobalHistory with something better, and use an adapter Don't bother reviewing this... the final patch will be huge and need detailed review anyway. I wouldn't mind validation of the nsIGlobalHistory2 interface.
Attachment #138044 - Flags: review?(bz-vacation)
Big honky patch, I'm vaguely sorry. There is a new nsIGlobalHistory2 interface which is designed to replace nsIGlobalHistory. In order to maintain cross-compatibility with the old interface, there are two adapter classes nsIGlobalHistory->nsIGlobalHistory2 (nsGlobalHistoryAdapter, for use by gecko when an old-style embedding app provides nsIGlobalHistory but we need to use the new interface). nsIGlobalHistory2->nsIGlobalHistory (nsGlobalHistory2Adapter, for use by extensions such as chatzilla which want nsIGlobalHistory)
Attachment #138044 - Attachment is obsolete: true
Attachment #138424 - Flags: review?(bz-vacation)
Comment on attachment 138424 [details] [diff] [review] nsIGlobalHistory2, plus adapters and seamonkey/aviary fixup + PRBool isJavascript; + rv = aURI->SchemeIs("javascript", &isJavascript); + NS_ENSURE_SUCCESS(rv, rv); + + if (!isJavascript) { + // if this is a JS url, hide it in global history so that it doesn't show Oops, this test should be (isJavascript) instead of (!isJavascript) in both xpfe/ and toolkit/. Took me a damn long time to figure out why the history sidebar was so sparse.
Priority: -- → P1
can someone give me the 10-words-or-less version of what i have to change in camino for this?
Mike, it looks like camino is using the seamonkey implementation of global history (I'm not sure, though)... if that is the case, I think the nsIBrowserHistory parts of this patch will work without problems in camino, except perhaps for include paths. Code that uses nsIDocShellHistory will have to be updated.
> Basically, this method rejects the protocols that are not handled by gecko. Originally all doc shells had global history. Nowadays we can disable global history by adding disablehistory="true" to the <browser> element, this has been done for mail and venkman to name but two. This means we should be able to get rid of most of those tests.
Comment on attachment 138424 [details] [diff] [review] nsIGlobalHistory2, plus adapters and seamonkey/aviary fixup >Index: docshell/base/nsDocShell.cpp >+nsDocShell::SetUseGlobalHistory(PRBool aUseGlobalHistory) >+ mGlobalHistory = do_GetService(NS_GLOBALHISTORY2_CONTRACTID); Shouldn't OOM here propagate out or something? >-NS_IMETHODIMP >-nsDocShell::GetGlobalHistory(nsIGlobalHistory ** aGlobalHistory) >+nsresult >+nsDocShell::GetUseGlobalHistory(PRBool *aUseGlobalHistory) Shouldn't that still be an NS_IMETHODIMP? >+ NS_ENSURE_ARG_POINTER(aUseGlobalHistory); Don't null-check XPCOM out params. Just assert that they're not null or something. >@@ -5960,17 +5959,17 @@ nsDocShell::OnNewURI(nsIURI * aURI, nsIC > // Update Global history >- AddToGlobalHistory(aURI, IsFrame()); >+ AddToGlobalHistory(aURI, PR_FALSE); That's a behavior change that will cause subframe URIs to appear in the history dropdown and whatnot, if I understand this code correctly? Is that desirable? >+nsDocShell::AddToGlobalHistory(nsIURI * aURI, PRBool aRedirect) >+ NS_ENSURE_STATE(mGlobalHistory); That seems wrong. Operating without a history is definitely a supported mode of operation; we should just return NS_OK if the mGlobalHistory is null, no? As long as we have a warning on OOM at the place where it's allocated, things should be happy... >Index: docshell/base/nsGlobalHistory2Adapter.cpp >+ * Portions created by the Initial Developer are Copyright (C) 2003 2004 at this point? ;) >+nsGlobalHistory2Adapter::Create(nsISupports *aOuter, >+ if (aOuter) { >+ rv = NS_ERROR_NO_AGGREGATION; >+ return rv; >+ } Is there a good reason to do that rather than just return NS_ERROR_NO_AGGREGATION? Similar in various other places in this file... Are you trying to reduce the codesize gcc outputs or something? Iirc that problem is solved in 3.5 and I would rather not bend code around it.... >+ nsGlobalHistory2Adapter* adapter = new nsGlobalHistory2Adapter(); >+ rv = adapter->Init(); >+ if (NS_SUCCEEDED(rv)) { >+ rv = adapter->QueryInterface(aIID, aResult); >+ } >+ NS_RELEASE(adapter); Um... where did you addref adapter? I assume you were going to do that right before the Init() call? >+nsGlobalHistory2Adapter::RegisterSelf(nsIComponentManager* aCompMgr, So we're relying on the embeddor registering for the global history contractid before this is called? Is this a good assumption? I don't know much about the registration process... >+nsGlobalHistory2Adapter::Init() >+ do { More working around gcc issues? Again, I'd really rather not do that.... >+ mHistory = do_GetService(NS_GLOBALHISTORY_CONTRACTID, &rv); NS_GLOBALHISTORY2_CONTRACTID, no? >+nsresult >+nsGlobalHistory2Adapter::AddPage(const char* aURI) NS_IMETHODIMP? >+nsresult >+nsGlobalHistory2Adapter::IsVisited(const char* aURI, PRBool* aRetval) Same. + NS_ENSURE_ARG_POINTER(aRetval); It's an out param; just assert. >Index: docshell/base/nsGlobalHistory2Adapter.h >+ * Portions created by the Initial Developer are Copyright (C) 2003 Again, 2004? >+ * A compatibility wrapper for the nsIGlobalHistory2 interface. It provides >+ * the old nsIGlobalHistory interface for extensions who still use it. "which still use it" >Index: docshell/base/nsGlobalHistoryAdapter.cpp >+ * Portions created by the Initial Developer are Copyright (C) 2003 The usual. >+nsGlobalHistoryAdapter::Create(nsISupports *aOuter, This has all the same issues as nsGlobalHistory2Adapter::Create >+nsGlobalHistoryAdapter::Init() Again, the "do" weirdness. >+nsresult >+nsGlobalHistoryAdapter::AddURI(nsIURI* aURI, PRBool aRedirect, NS_IMETHODIMP >+ // The model is really if we don't know differently then add which basically >+ // means we are suppose "supposed" (and yes, I know you just copied this). This function doesn't do quite the same thing as nsDocShell::AddToGlobalHistory used to, does it? In particular, it fails to do the hidepage stuff.... Won't that break embeddors who implemented nsIBrowserHistory and assumed that the docshell would handle all this for them? This seems like a major problem to me.... >+nsresult >+nsGlobalHistoryAdapter::IsVisited(nsIURI* aURI, PRBool* aRetval) NS_IMETHODIMP >+ NS_ENSURE_ARG_POINTER(aRetval); It's an out param. Just assert. >+nsresult >+nsGlobalHistoryAdapter::SetPageTitle(nsIURI* aURI, const nsAString& aTitle) NS_IMETHODIMP >+{ >+ return NS_ERROR_NOT_IMPLEMENTED; >+} Again, this breaks embeddors who actually implemented nsIBrowserHistory, no? >Index: docshell/base/nsGlobalHistoryAdapter.h >+ * Portions created by the Initial Developer are Copyright (C) 2003 The usual. >+ * A compatibility wrapper for the old nsIGlobalHistory interface. It provides >+ * the new nsIGlobalHistory2 interface for embedders who haven't implemented it >+ * themself. "themselves" >Index: docshell/base/nsIDocShellHistory.idl What's the motivation for the change to this file? I assume the idea is that we're not likely to have two different history impls in one app and that there is no reason to support that? >Index: docshell/base/nsIGlobalHistory2.idl >+ * Version: NPL 1.1/GPL 2.0/LGPL 2.1 Why? This is your code, no? It should be MPL'd. >+ * The Initial Developer of the Original Code is >+ * Netscape Communications Corporation. Nope. >+ * Portions created by the Initial Developer are Copyright (C) 1998 Again, nope. >+ * @param aURI the URI of the page >+ * @param aRedirect whether the URI was redirected to another location Would this be the pre-redirect URI or the post-redirect one? That's not clear. >+ * @param aToplevel whether the URI is loaded in a top-level window >+ void addURI(in nsIURI aURI, in boolean aRedirect, in boolean aFrame); Comments and code don't agree.... Further, docshell code and nsGlobalHistoryAdapter code don't agree. Please fix those issues! >+ * Checks to see if the given URI is in history "whether" >+ * @return true if a page has been passed into addPage(). >+ */ >+ boolean isVisited(in nsIURI aURI); That comment is very confusing.... "a page"? I'd say "@return whether the URI has ever been passed to addPage()" or something. Except we have an addURI(), not an addPage(). >+ * Set the page title for the given url. "given URI". >+ * URIs that are not already in >+ * global history should not be added. "You should not do this unless you have done addURI() with that URI already" There should probably be a @throws annotation with the error that will be thrown if you _do_ call this on a URI that's not in the history.... >+%{ C++ >+#define NS_GLOBALHISTORY2_CONTRACTID \ >+ "@mozilla.org/browser/global-history;2" >+%} It's really bad form to put this in interfaces... any way to avoid this, please? >Index: xpfe/components/autocomplete/resources/content/autocomplete.xml These changes seem extraneous. >Index: xpfe/components/history/public/nsIBrowserHistory.idl >+ readonly attribute PRUint32 count; Just make this a "long" while you're here? >Index: xpfe/components/history/public/nsIGlobalHistory.idl > %{ C++ >-// {9491C382-E3C4-11D2-BDBE-0050040A9B44} >-#define NS_GLOBALHISTORY_CID \ >-{ 0x9491c382, 0xe3c4, 0x11d2, { 0xbd, 0xbe, 0x0, 0x50, 0x4, 0xa, 0x9b, 0x44} I'm not sure we can remove this crud from here, really... people may be depending on it. But I'm not sure what our story is on removing such things from frozen interfaces. >Index: xpfe/components/history/src/nsGlobalHistory.cpp >- nsresult rv = AddPageToDatabase(aURL, GetNow()); Was there a good reason to remove the separate AddPageToDatabase function? >+ if (!isJavascript) { >+ // if this is a JS url, hide it in global history so that it doesn't show Isn't that test backwards? >+ if (aToplevel) { Make sure to make this agree with the idl and all.... >+nsGlobalHistory::SetPageTitle(nsIURI *aURI, const nsAString& aTitle) >+ // if the row doesn't exist, we silently succeed >+ if (rv == NS_ERROR_NOT_AVAILABLE) return NS_OK; This seems ill-matched to the idl, which insisted people not do this... >-NS_IMETHODIMP >+nsresult > nsGlobalHistory::RemovePage(const char *aURL) Why?? >+nsGlobalHistory::IsVisited(nsIURI* aURI, PRBool *_retval) >+ NS_ENSURE_ARG_POINTER(_retval); Don't null-check out params. >Index: xpfe/global/resources/content/bindings/button.xml More extraneous changes.... >Index: xpfe/global/resources/content/bindings/dialog.xml And this >Index: xpfe/global/resources/content/bindings/wizard.xml And this. >Index: toolkit/components/history/src/nsGlobalHistory.cpp I assume your changes here are identical to the xpfe changes? >Index: browser/components/prefwindow/content/pref-privacy.js >+ var globalHistory = Components.classes["@mozilla.org/browser/global-history;s"] ";s" ?? You mean ";2", right? Marking r- due to the issues with the adapter I brought up and the issues with the toplevel/frame/whatever boolean in the new interface....
Attachment #138424 - Flags: review?(bz-vacation) → review-
>>+ nsGlobalHistory2Adapter* adapter = new nsGlobalHistory2Adapter(); >>+ rv = adapter->Init(); >>+ if (NS_SUCCEEDED(rv)) { >>+ rv = adapter->QueryInterface(aIID, aResult); >>+ } >>+ NS_RELEASE(adapter); >Um... where did you addref adapter? I assume you were going to do that right >before the Init() call? Or use an nsRefPtr perhaps?
>>+ readonly attribute PRUint32 count; >Just make this a "long" while you're here? "unsigned long", surely?
> "unsigned long", surely? Er, yes.
> >@@ -5960,17 +5959,17 @@ nsDocShell::OnNewURI(nsIURI * aURI, nsIC > > // Update Global history > >- AddToGlobalHistory(aURI, IsFrame()); > >+ AddToGlobalHistory(aURI, PR_FALSE); > > That's a behavior change that will cause subframe URIs to appear in the history > dropdown and whatnot, if I understand this code correctly? Is that desirable? No, the AddToGlobalHistory param is just for redirects. Frames get their own param. > >+ if (aOuter) { > >+ rv = NS_ERROR_NO_AGGREGATION; > >+ return rv; > >+ } > > Is there a good reason to do that rather than just return > NS_ERROR_NO_AGGREGATION? Similar in various other places in this file... This was copied from the generic factory. I would like to keep the rv=blah; return rv; format. I've junked a lot of the nested-if and dorky do{} madness. > >+nsGlobalHistory2Adapter::RegisterSelf(nsIComponentManager* aCompMgr, > > So we're relying on the embeddor registering for the global history contractid > before this is called? Is this a good assumption? No, we're not relying on that. The RegisterSelf performs a check "if this contractID is not registered by someone else, register it." > >+nsGlobalHistory2Adapter::Init() > >+ mHistory = do_GetService(NS_GLOBALHISTORY_CONTRACTID, &rv); > > NS_GLOBALHISTORY2_CONTRACTID, no? No. In order to prevent an infinite loop between the two history adapters, this method checks for the other to make sure it's wrapping a real implementation. > This function doesn't do quite the same thing as nsDocShell::AddToGlobalHistory > used to, does it? In particular, it fails to do the hidepage stuff.... Won't > that break embeddors who implemented nsIBrowserHistory and assumed that the > docshell would handle all this for them? This seems like a major problem to > me.... Well, that's all non-frozen interface, so I'm much less worried about breaking it. The only embedders I'm aware of which actually implemented nsIBrowserHistory used the seamonkey implementation (that's what camino does, and I believe epiphany as well), so they will still work. > >Index: docshell/base/nsIDocShellHistory.idl > > What's the motivation for the change to this file? I assume the idea is that > we're not likely to have two different history impls in one app and that there > is no reason to support that? Yes, and it was necessary to avoid passing adapters around when we shouldn't have been. > >+ * @param aURI the URI of the page > >+ * @param aRedirect whether the URI was redirected to another location > > Would this be the pre-redirect URI or the post-redirect one? That's not clear. pre-redirect, and I have updated the docs. > >+ * @param aToplevel whether the URI is loaded in a top-level window > > >+ void addURI(in nsIURI aURI, in boolean aRedirect, in boolean aFrame); > > Comments and code don't agree.... Further, docshell code and > nsGlobalHistoryAdapter code don't agree. Please fix those issues! aToplevel is correct, all places fixed. > >+ * URIs that are not already in > >+ * global history should not be added. > > "You should not do this unless you have done addURI() with that URI already" > > There should probably be a @throws annotation with the error that will be > thrown if you _do_ call this on a URI that's not in the history.... actually, wrong way round. You *may* call this for pages not in history... but they won't be added. The new comment says "URIs that are not already in global history will not be added." > >Index: xpfe/components/history/public/nsIGlobalHistory.idl > > %{ C++ > >-// {9491C382-E3C4-11D2-BDBE-0050040A9B44} > >-#define NS_GLOBALHISTORY_CID \ > >-{ 0x9491c382, 0xe3c4, 0x11d2, { 0xbd, 0xbe, 0x0, 0x50, 0x4, 0xa, 0x9b, 0x44} > > I'm not sure we can remove this crud from here, really... people may be > depending on it. But I'm not sure what our story is on removing such things > from frozen interfaces. They cannot be depending on the CID, and we cannot have frozen it. We need different CIDs for the seamonkey impl and the FB impl, in addition to the adapter. > > >Index: xpfe/components/history/src/nsGlobalHistory.cpp > > >- nsresult rv = AddPageToDatabase(aURL, GetNow()); > > Was there a good reason to remove the separate AddPageToDatabase function? Yes, and it had something to do with RDF notifications. > >Index: toolkit/components/history/src/nsGlobalHistory.cpp > > I assume your changes here are identical to the xpfe changes? no... the core ideas are the same, but the code was slightly restructured when I got to it, so I kept within that framework. All other nits picked. Patch coming up again imminently.
Fixed everything per review comments, plus a bonus hiding of frames and redirects again.
Attachment #138424 - Attachment is obsolete: true
Comment on attachment 140724 [details] [diff] [review] Updated with review comments, nsDocShellCID.h, and properly hiding frames and redirects Brian: bz went over everything except some minor differences in toolkit/components/history/src/nsGlobalHistory.cpp Could you review that file, and sr the whole shebang?
Attachment #140724 - Flags: superreview?(bryner)
Attachment #140724 - Flags: review?(bzbarsky)
Comment on attachment 140724 [details] [diff] [review] Updated with review comments, nsDocShellCID.h, and properly hiding frames and redirects >Index: docshell/base/nsDocShell.cpp >+nsDocShell::GetUseGlobalHistory(PRBool *aUseGlobalHistory) >+ NS_ENSURE_ARG_POINTER(aUseGlobalHistory); Like I said, don't do that. Just assert. >+nsDocShell::AddToGlobalHistory(nsIURI * aURI, PRBool aRedirect) >+ NS_ENSURE_STATE(mGlobalHistory); Again, like I said, don't do that. Just return NS_OK if it's not there. >-#define DIALOG_STRING_URI "chrome://global/locale/appstrings.properties" >+#define DIALOG_STRING_URI "resource://embed-locale/docshell/appstrings.properties" Is that really part of this patch? >Index: xpfe/browser/src/nsBrowserInstance.cpp >+ nsCOMPtr<nsIBrowserHistory> history(do_GetService(NS_GLOBALHISTORY_CONTRACTID)); Not HISTORY2? Why not? With those fixed, r=bzbarsky.
Attachment #140724 - Flags: review?(bzbarsky) → review+
Attachment #140724 - Flags: superreview?(bryner) → superreview?(alecf)
Comment on attachment 140724 [details] [diff] [review] Updated with review comments, nsDocShellCID.h, and properly hiding frames and redirects in nsDocShell::SetUseGlobalHistory(PRBool if (!mGlobalHistory) return NS_ERROR_OUT_OF_MEMORY; I'd rather you use the two-parameter form of do_GetService and return the rv of that. that way, you can avoid the if completely, and just return rv. NS_ENSURE_ARG_POINTER(aUseGlobalHistory); no need to check xpcom return values for null so.. around Lines 6525-6602 in nDocShell.cpp... why are you sure that this is not a redirect? #define DIALOG_STRING_URI "chrome://global/locale/appstrings.properties" #define DIALOG_STRING_URI is that change supposed to be in this patch? nsDocShellCID.h is pretty empty. should other contractids move here? nsIBrowserHistory.idl: readonly attribute string lastPageVisited; string? why not nsIURI? hm, an issue for most functions on that interface. removePagesFromHost also should consider another type for aHost - non-ascii hostnames happen... nsGlobalHistory::AddURI(nsIURI PRInt64 now = GetNow(); why not PRTime? above the block that does the "set last page visited" stuff, could you add a comment what it does? like "Store last visited page if we have the pref set accordingly" in SetPageTitle: // skip about:blank to avoid reading in the db this comment is wrong. you actually skip all about: pages. but should you? and wouldn't it be better to check if aTitle is empty and only skip setting the title then? in nsBrowserInstance: nsCOMPtr<nsIBrowserHistory> history(do_GetService(NS_GLOBALHISTORY_CONTRACTID)); not GLOBALHISTORY2? my review is not valid for any of the toolkit or browser/ files. didn't look at them, too. r=me with these changes
> so.. around Lines 6525-6602 in nDocShell.cpp... why are you sure that this is > not a redirect? Because OnLoadingSite does the proper checks for a redirected page. > #define DIALOG_STRING_URI "chrome://global/locale/appstrings.properties" > is that change supposed to be in this patch? Yes, that was contamination from another bug. > nsDocShellCID.h is pretty empty. should other contractids move here? Yes, but not yet. > nsIBrowserHistory.idl: > readonly attribute string lastPageVisited; > string? why not nsIURI? hm, an issue for most functions on that interface. > removePagesFromHost also should consider another type for aHost - non-ascii > hostnames happen... All of that is true, but it requires significant work updating callers and implementation; I'm trying to be as non-invasive as possible ;) > nsGlobalHistory::AddURI(nsIURI > PRInt64 now = GetNow(); > > why not PRTime? Because that's the signature of GetNow(). Which should be fixed, but not here.
ok, can you file bugs on the things I mentioned that won't be done in this bug? (please cc me)
You can take my review as sr=bzbarsky if you want to use biesi's r=.
alecf's review will suffice for the required peer review for Firefox. Please send him an e-mail and see if you can get him to take a look. If not, send mail to aviary-review@mozilla.org.
CVS move of xpfe/components/history/public/nsIGlobalHistory.idl -> docshell/base/nsIGlobalHistory.idl has been completed. You'll still need to cvs remove the old version as part of your patch. The file should magically show up in the new location with the same rev number on your next cvs update.
Fix checked in, with two changes: I reordered the root Makefile.in build seqyence so it wasn't so zany (components/history/public is now tier_50 where it belongs), and I put removePage() back into nsIBrowserHistory.idl, because camino needed it. --BDS Bugs from review nits will be filed separately.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.7alpha
Attachment #140724 - Flags: superreview?(alecf)
This patch ignored comment 54 and checked in without the mandatory Firefox peer review (the one exception on the requirements page does not apply here). As a result, this patch caused numerous bugs, like bug 234136. Nearly one week after the checkin, we are still cleaning up the mess. Please be more careful in the future and abide by the checkin requirements or such patches will be backed out.
blake: as my checkin comment indicated, I got approval from mscott AND talked with Ben Goodger before landing this patch. I am aware that these API patches might cause odd regressions and am available to fix them if I'm notified. But I'm not telepathic, so somebody should actually say "Benjamin, you caused bug such-and-such" instead of complaining after the fact that I broke it.
Blocks: 234636
I really would have liked to have been CC'ed on this bug.
Blocks: 234695
Blocks: 234697
Standalone composer failing to build because of this checkin... I can fix it.
Daniel, why did it fail to build? The goal was to not break any embeddors, and we apparently failed... if you can tell us what the build errors were, we should try to fix them.
I'm trying to get LinkVisitor (a firefox extension) to work after this checkin and am having problems. LinkVisitor uses the nsIBrowserHistory.removePage() function to mark links as unvisited, but after updating to use global-history;2 I get an error that "removePage is not a function" whenever it is called. Am I missing something?
Son Le: the "removePage" method is no longer part of the firefox nsIBrowserHistory interface, although adding it back wouldn't be that hard. I removed it because there were no callers in the tree. If you really need it back, file a new bug and reverse the little bit of this patch that moved the method from toolkit/components/history/public/nsIBrowserHistory.idl to toolkit/components/history/src/nsBrowserHistory.h and get blake to review it.
How does the history window remove pages from history?
probably through RDF.. wow! :( I put removePage into the API a long time ago, specifically to allow for extensions like LinkVisitor... Perhaps we want to start considering freezing a subset of nsIBrowserHistory so that we don't break cool extensions? (and move the non frozen stuff into nsIBrowserHistoryInternal)
I should have commented earlier, but better later that never :) I am worried about the inconsistencies of the argument types in the 2 following interfaces: nsIGlobalHistory2 -> nsIURI only nsIBrowserHistory -> string only except hidePage (nsIURI) shouldn't hidePage be reverted to use string, instead of nsIURI? At least, there would be a consistency inside the interfaces. Also, the naming is confusing: because of addURI(in nsIURI aURI...), markPageAsTyped(in string aURL) and addPageWithDetails(in string aURL...) I would expect the following rule: ...Page... -> string ...URI.... -> nsIURI No? That would imply changing setPageTitle(in nsIURI aURI...) to setTitle() or setURITitle()...
pch: see comment 50... I also commented on the use of string in that interface. I hope bsmedberg filed a bug on that as promised.
I've filed bug 235915 for the adding of the RemovePage method back to nsIBrowserHistory.idl. I'm tried looking for the file nsBrowserHistory.h using LXR and couldn't find it, but did find another copy of nsIBrowserHistory.idl (in mozilla/xpfe/components/history/public/) which still has the RemovePage declaration. Needless to say I'm a little confused.
Welcome to the bright new world of forked apis.
oh jeez, they forked this? sometimes I want to strangle firefox. I like the idea of being consistent with the use of nsIURI though. let's make removePage() use that. We'll continue this in 235915
(In reply to comment #51) > > nsDocShellCID.h is pretty empty. should other contractids move here? > > Yes, but not yet. bug 234697 > > nsIBrowserHistory.idl: > > readonly attribute string lastPageVisited; > > string? why not nsIURI? hm, an issue for most functions on that interface. > > removePagesFromHost also should consider another type for aHost - non-ascii > > hostnames happen... > > All of that is true, but it requires significant work updating callers and > implementation; I'm trying to be as non-invasive as possible ;) can't find a bug for that, did you file it? > > nsGlobalHistory::AddURI(nsIURI > > PRInt64 now = GetNow(); > > > > why not PRTime? > > Because that's the signature of GetNow(). Which should be fixed, but not here. bug 234695
(In reply to comment #71) > > > nsIBrowserHistory.idl: > > > readonly attribute string lastPageVisited; > > > string? why not nsIURI? hm, an issue for most functions on that interface. > > > removePagesFromHost also should consider another type for aHost - non-ascii > > > hostnames happen... bug 254332
This "fix" is causing problems for an embedder (camino) that wants to implement its own global history. Specifically, this code: rv = compReg->IsContractIDRegistered(NS_GLOBALHISTORY_CONTRACTID, &registered); if (NS_FAILED(rv)) return rv; // If the embedder has already registered the contractID, we don't want to // register ourself. Ideally the component manager would handle this for us. is bogus because nsGlobalHistory2Adapter::RegisterSelf() is called at NS_InitEmbedding time, which is before an embedder has had a chance to register its own contractID for global history.
> is bogus because nsGlobalHistory2Adapter::RegisterSelf() is called at NS_InitEmbedding time, which is > before an embedder has had a chance to register its own contractID for global history. That's ok. You can override the registration at any time.
Yeah, this was a red herring in my debugging. The real problem is bug 276956.
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: