Redirects show up as individual entries. They should be hidden.

Also, do the smart thing with respect to favicons and custom titles. The destination page should be used if it has a favicon/custom title, but we should fall through to the source page if we don't have the information. Favicons associated with the destionation page should also be pushed up to the source page if it has no favicon. This will be important for bookmarks.

Also, do the right thing for smart queries. We have the URL http://www.google.com/search?q=%s in history for the bookmark. This should be associated with the expanded URL that you end up using so that the bookmark gets the proper favicon.

See also http://wiki.mozilla.org/Browser_History:Redirects

Comments would be appreciated.

In case it wasn't clear, the previous patch implements the plan in the wiki: http://wiki.mozilla.org/Browser_History:Redirects#Proposal Basically: * redirect function on nsIGlobalHistory2. * New virtual function OnRedirectStateChange in docloader. * Move redirect code from OnStateChange to OnRedirectStateChange. * Change docshell to use new function in nsIGlobalHistory2 * Implement nsIWritablePropertyBag on the docshell.

Brett, The docshell/docloader changes in this patch look good to me. Usually it is best to separate firefox and gecko patches into two because the reviewers are often different. There is also a "Core: Embedding: docshell" where changes to docshell/docloader are typically filed. Nits: +/*NS_IMPL_THREADSAFE_ADDREF(nsDocLoader) +NS_IMPL_THREADSAFE_RELEASE(nsDocLoader)*/ Delete this commented out code. In OnRedirectStateChange, consider adding an early return if the state flags are not interesting. That way you reduce indenting for the main part of the function. r=me on the patch.

A fewcomments on very brief skim of patch (I won't be able to do more until I get back, probably): 1) It looks to me, based on this patch and the subject of some mail I recall, that we are basically trying to unify our global and session history architectures more. If so, is there a design document somewhere? I'd really rather not do something like that in piecemeal changes. If that's not what we're doing, then what _are_ we doing? That is, what is the actual plan in terms of the docshell and history API changes? 2) I'd like to see the core patches (for this bug and any other work of this sort) in core bugs filed in the right component and with the module owners/peers actually cced. 3) This patch is clearly not acceptable for the 1.8 branch (API change). Does that matter?

should this new nsIGlobalHistory2 redirect function also get the redirect flags? That might be useful - history impls might not want to store the pre-redirect URL for permanent redirects.

This patch is somewhat simpler. It moves the new interface to nsIGlobalHistory3, dispenses with all the property bags, and uses channels instead of URIs which are potentially more useful to a history implementation.

so now that that interface is identical to nsIChannelEventSink, might it make sense to just qi the history impl to that?

(In reply to comment #10) > so now that that interface is identical to nsIChannelEventSink, might it make > sense to just qi the history impl to that? Well, we could. I think a bunch of other stuff will be added to GloablHistory3, like GeckoFlags from trunk, and the number of interfaces implemented by history is already out of control. Further, I thought we might add additional information to this function, or add corresponding new SiteVisited functions to this interface. I thought it seemed better to keep it all together.

If this is ready to go, feel free to pull into f2a1.

Comment on attachment 209783 [details] [diff] [review] New simpler patch For what it's worth, it'd be nice to put the Gecko stuff in a bug in the right component and product. ;) >Index: docshell/base/nsDocShell.cpp >- // Is the document stop notification for this document? >- if (aProgress == webProgress.get()) { It looks like this check got lost. That's not good. We don't want to lose this check. That said, it could be changed to use SameCOMIdentity or something to make it clearer what's really being checked, if desired. >Index: docshell/base/nsDocShell.h >+ // overridden from nsIDocLoader, this provides more information than the There is no nsIDocLoader. Should that be nsDocLoader? >Index: docshell/base/nsIGlobalHistory3.idl >+interface nsIGlobalHistory3 : nsISupports Would it make sense to inherit from nsIGlobalHistory2 here? I think it would... darin? Thoughts? >+ * Notifies the history system that page 'old' redirected to page 'new'. Maybe "that the page loading via aOldChannel redirected to aNewChannel"? >+ * The old page will be added to history, but will not normally be visible. It's not clear to me whether this is part of the interface contract or not... If this is meant to be advisory, then perhaps: "History implementations are advised to add the URI for aOldChannel to history for link coloring purposes but not to expose it in the history user interface." Or something. I'm not even sure that's true, depending on the redirect flags. >+ * This will be called, if nsIGlobalHistory3 is available, instead of >+ * nsIGlobalHistory2.addURI with redirect=true. "If the global history implements nsIGlobalHistory3, this function will be called when redirects happen. Otherwise, nsIGlobalHistory2.addURI with aRedirect == true will be called." > This function is superior >+ * because it will give the redirect desination. "This function is preferred to nsIGlobalHistory2.addURI because it provides more information (including the redirect destination, channels involved, and redirect flags) to the history implementation." >+ * This function takes identical areguments as >+ * nsIChannelEventSink::OnChannelRedirect "This function takes the same arguments as ..." (note spelling change for "arguments"). >+ void addToplevelRedirect(in nsIChannel oldURI, in nsIChannel newURI, aOldChannel and aNewChannel, perhaps? >+ in PRInt32 flags); And aFlags. sr=bzbarsky with those nits picked.

Comment on attachment 209783 [details] [diff] [review] New simpler patch >Index: docshell/base/nsDocShell.cpp >+void >+nsDocShell::OnRedirectStateChange(nsIChannel* aOldChannel, >+ nsIChannel* aNewChannel, >+ PRUint32 aRedirectFlags, >+ PRUint32 aStateFlags) >+{ >+ if ((~aStateFlags & (STATE_IS_DOCUMENT | STATE_REDIRECTING)) == 0) { >+ nsCOMPtr<nsIGlobalHistory3> history3(do_QueryInterface(mGlobalHistory)); nit: I recommend returning early if this condition is not met. That way you can reduce overall indenting of this function by one level. >Index: docshell/base/nsDocShell.h >+ // overridden from nsIDocLoader, this provides more information than the s/nsIDocLoader/nsDocLoader/ >Index: docshell/base/nsIGlobalHistory3.idl >+/** >+ * Provides information about global history to gecko, extending GlobalHistory2 >+ */ >+ >+#include "nsISupports.idl" >+interface nsIChannel; >+ >+[scriptable, uuid(40e9613e-1742-4b4d-a858-053e6237d04d)] nit: I recommend moving the comment about this interface down next to the interface itself. That way automatic documentation generation tools like doxygen will know how to associate the comment with the interface: /** * Blah, blah, blah... */ [scriptable, uuid(...)] interface nsIFoo : nsISupports { ... r=me with those nits picked

> Would it make sense to inherit from nsIGlobalHistory2 here? I think it > would... darin? Thoughts? Yeah, that sounds like a good idea to me. That will help avoid an additional vtable pointer in the nsIGlobalHistory3 implementation, and since nsIGlobalHistory3 is not a wholesale replacement for nsIGlobalHistory2, it makes sense for the two interfaces to be tightly coupled.

> >Index: docshell/base/nsDocShell.cpp > >- // Is the document stop notification for this document? > >- if (aProgress == webProgress.get()) { After talking with Darin, we decided this check isn't needed. This new redirect code is called from OnChannelRedirect which only happens on the exact docshell that the request happened on and is not passed up. Therefore, this message will always be for us. Also, it looks like this check can be removed as well: "if ((~aStateFlags & (STATE_IS_DOCUMENT | STATE_REDIRECTING)) == 0)" since (a) we know for sure we're redirecting here and (b) the state will always be for the main document since that what we've registered the listener for. bz: Do you agree with these conclusions? If so, I'll remove the checks before I submit.

> After talking with Darin, we decided this check isn't needed. OK. I see that. Make it an assert instead of a check, ok? > Also, it looks like this check can be removed as well: > "if ((~aStateFlags & (STATE_IS_DOCUMENT | STATE_REDIRECTING)) == 0)" > since (a) we know for sure we're redirecting here and (b) the state will > always be for the main document since that what we've registered the listener > for. I agree with (a); that part of the check should be an assert. But I see nothing that ensures STATE_IS_DOCUMENT here. See http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/uriloader/base/nsDocLoader.cpp&rev=3.289&mark=1425-1427#1423 That is, I would expect this function to be called any time any channel in the loadgroup is redirected. Note that it doesn't matter what we registered for, because we're calling OnChannelRedirect directly and that doesn't check the listener registration stuff, unlike FireOnStateChange. So I think we still need to check for STATE_IS_DOCUMENT and we can assert STATE_REDIRECTING. Am I making sense?

> OK. I see that. Make it an assert instead of a check, ok? Actually, I guess this is irrelevant since there's nothing to assert... ;) We still need an assert on STATE_REDIRECTING and a check on the rest. And I'd still like either this bug to move to Core:Docshell or the patch to move to a bug there.

I couldn't find a Core:Docshell, feel free to recategorize this. I'll make a new bug for the places implementation that uses this feature.

-> Core: Embedding: Docshell ;-)

This was already approved, but can either Darin or Boris double check my OnRedirectStateChange implementation to make sure it's what we discussed?

Comment on attachment 210619 [details] [diff] [review] Final patch with comments addressed >Index: docshell/base/nsDocShell.cpp >+nsDocShell::OnRedirectStateChange(nsIChannel* aOldChannel, >+ NS_ASSERTION(aStateFlags | STATE_REDIRECTING, You want '&', not '|'. >+ if (!(aStateFlags | STATE_IS_DOCUMENT)) >+ return; // not a toplevel document And same here. Other than that, looks great.

I was actually implementing this in my history system and found an issue. nsIBrowserHistory inherits from nsIGlobalHistory2. Now we're making nsIGlobalHistory3 inherit from nsIGlobalHistory2. Do you think we should: [1] Leave as-is (and deal with occational annoying implementation weirdness), [2] Inherit nsBrowserHistory from nsIGlobalHistory3 (and probably force other implementors to change), or [3] just make nsIGlobalHistory3 not inherit from anything?

> nsIBrowserHistory inherits from nsIGlobalHistory2. Um... WHY? :( I guess given that our only option is to make nsIGlobalHistory3 inherit from nsISupports with a nice XXX comment about being screwed over by non-core interfaces inheriting from core ones and hence messing up versioning. :( The other option is to change nsIBrowserHistory on the branch. If we're doing that anyway, I guess that's ok. But if we're not we really shouldn't.

Hrm... that's a tough one. If we weren't trying to preserve API compat between Gecko 1.8 and 1.8.1, then I'd say change nsIBrowserHistory to inherit from nsIGlobalHistory3 (or even just nsISupports). However, given that we can't touch nsIBrowserHistory for FF2, we might then prefer to have nsIGlobalHistory3 inherit from nsISupports. The only reason for making it extend nsIGlobalHistory2 was to avoid an extra vtable pointer per "global history" instance, but that's a singleton anyways, so it probably doesn't matter ;-)

Comment on attachment 210619 [details] [diff] [review] Final patch with comments addressed I'll make nsIGlobalHistory3 just inherit from nsISupports. We aren't touching nsBrowserHistory at all. All the new frontend stuff is going in a new interface (nsINavHistory).

> I'll make nsIGlobalHistory3 just inherit from nsISupports. We aren't touching > nsBrowserHistory at all. All the new frontend stuff is going in a new interface > (nsINavHistory). Wait... that means that nsIBrowserHistory will be un-implemented in FF2, right? That is sort of equivalent to API breakage. More to the point, changing an interface is equivalent to removing an interface and adding a different one, so if we are okay removing nsIBrowserHistory in FF2, then we could also just as well change it.

We support nsIBrowserHistory, we don't change it. The history implentation is super confusing with all the functions randomly spewed about nsIGlobalHistory2, nsIGlobalHistory3, nsIBrowserHistory, and nsINavHistory.

wait a second, wasn't there a second reason, namely that nsIGlobalHistory3 isn't a "standalone" interface, and that it would therefore make sense from that point of view? isn't the only issue here that casting to nsIGlobalHistory2 will be ambiguous?

That's one issue, but the other is just multiple inheritance bloat. We can take that hit if we think that nsIGlobalHistory3 should inherit from #2.

So wait. nsIBrowserHistory is a front-end-only interface. It's not used by Gecko. Will your new history impl implement nsIBrowserHistory? If not, then we're "changing" it alright (like removing it altogether).

(In reply to comment #31) > So wait. nsIBrowserHistory is a front-end-only interface. It's not used by > Gecko. Will your new history impl implement nsIBrowserHistory? If not, then > we're "changing" it alright (like removing it altogether). Yes, the new history implementation implements nsIBrowserHistory and supports all of its functionality.

Er, just saw comment 28. So I think that nsIGlobalHistory3 should absolutely inherit from nsIGlobalHistory2 -- it reduces QIing in general and is the way the API works. I don't think we should be making the Gecko history APIs harder to use and less clear because some particular Gecko embeddor made a bad decision in the past and actually extended a Gecko api (!). On trunk, I suspect we'll want to just make nsIBrowserHistory inherit from nsISupports. Or something. On branch, I guess we keep things as is and deal in the impl.

bz: agreed. that sounds like the best plan.

On branch and trunk.