From Bugzilla Helper: User-Agent: Mozilla/5.0 (Macintosh; U; PPC; en-US; rv:0.9.8+) Gecko/20020228 BuildID: 2002022808 The referrer column is empty for every bookmarks in sidebar and History window. Reproducible: Always

also on Linux. Resolving as new.

*** Bug 130261 has been marked as a duplicate of this bug. ***

Alec: did this ever work?

no, I don't think so. .. I think the UI was added (before me, I think) just for consistency with the database.. but nothing is ever added to the database in this column.

This would be really handy for offering a non-flattened history interface.

I would love a "Go to the page that referred me here the first time I visited this URL" button, because that question accounts for most of the time I spend in the History window or sidebar. (I use the History window for "what was that window I just closed accidentally" more often, but that takes less time and is bug 39165.) If storing referers in global history would hurt startup performance and memory usage, maybe this should be a hidden pref that extensions can turn on. Opening the Referer column in the History window for the first time could also turn it on.

I've had a look at this, and I think it's doable. There are two things to do, add support to the history code to store the referrer (easy) and then create an interface to it (also fairly easy), then get all callers of the interface to add referrer information (harder). To start with, there's two interfaces to the history, wrapped by GlobalHistoryAdapter and GlobalHistory2Adapter. Since this is a feature unlikely to be used by embedders, it can go into GlobalHistory2Adapter (but is changing the interface ok? And I'm not sure which one Firefox uses.) The interface in GH2A is AddURI(sIURI *aURI, PRBool aRedirect, PRBool aTopLevel) (implemented in {toolkit,xpfe}/components/history/src/nsGlobalHistory.cpp). From http://lxr.mozilla.org/mozilla/search?string=adduri we see it's used by browser/base/content/contentAreaUtils.js and docshell/base/nsDocShell.cpp. In javascript, we can just use getReferrer(document). in nsDocShell.dpp, AddToGlobalHistory is called from two functions, OnStateChange(nsIWebProgress * aProgress, nsIRequest * aRequest, PRUint32 aStateFlags, nsresult aStatus) and OnNewURI(nsIURI * aURI, nsIChannel * aChannel, PRUint32 aLoadType). nsIRequest might be a nsIChannel, which might be a nsIHttpChannel, which has a nsIURI referrer attribute. So we just test if the nsIRequest (for OnStateChange) or nsIChannel (for OnNewURI) is actually a nsIHttpChannel, and if so, add it to the call to AddToGlobalHistory. If it's not a nsIHttpChannel, well, nothing apart from HTTP has the concept of referrer anyway, so nothing lost. As for the actual history code, it should be fairly simple, since there's already nsIRDFResource* nsGlobalHistory::kNC_Referrer in the source. Logic-wise, I think it should just record the first non-blank referrer, since that's what I'm always wanting to know. Hmm, there's also the question of the UI, but I guess it already supports it. It shouldn't be a performance issue, since the referrer column is already in the history database (but never used currently).

Ok, I've implemented it myself. It compiles and runs fine under linux gtk2. This is my first mozilla patch, so there's probably something stupid in there.

Comment on attachment 155547 [details] [diff] [review] patch to set referrers in history Asking timeless for review since he's done stuff with nsDocShell before. Also, this patch doesn't touch firefox, since you can't even view the referrer in the firefox panel. Although, the firefox code looks very similar, so it shouldn't be hard for me to port it if wanted.

Regarding firefox, this underlying data provides opportunities for a whole range of new UI options; so I'd love to see ffox get tackled after the code gets reviewed.

(In reply to comment #7) > To start with, there's two interfaces to the history, wrapped by > GlobalHistoryAdapter and GlobalHistory2Adapter. Since this is a feature unlikely > to be used by embedders, it can go into GlobalHistory2Adapter (but is changing > the interface ok? GlobalHistory2Adapter is not the (real) implementation of nsIGlobalHistory2... changing non-frozen interfaces is ok. > And I'm not sure which one Firefox uses.) nsIGlobalHistory2 I hope, but it has a forked implementation. > The interface in GH2A is AddURI(sIURI *aURI, PRBool aRedirect, PRBool aTopLevel) > (implemented in {toolkit,xpfe}/components/history/src/nsGlobalHistory.cpp). GH2A and GHA are really just for convenience of embeddors... > Also, this patch doesn't touch firefox you must touch firefox, or it will fail to compile. xpfe/components/history/src/nsGlobalHistory.cpp + nsCAutoString referrerSpec; + if (aReferrer) + rv = aReferrer->GetSpec(referrerSpec); + else + referrerSpec = ""; no need for the else, since nsCAutoString's default ctor inits the string to be empty. nsDocShell.cpp + nsCOMPtr<nsIURI> referrer; + nsCOMPtr<nsIHttpChannel> httpchannel(do_QueryInterface(aChannel)); + if (httpchannel) + httpchannel->GetReferrer(getter_AddRefs(referrer)); + else referrer = nsnull; no need for the else here either + nsCOMPtr<nsIHttpChannel> httpchannel(do_QueryInterface(aRequest)); + if (httpchannel) + httpchannel->GetReferrer(getter_AddRefs(referrer)); + else referrer = nsnull; and here.

Ok, this patch includes firefox as well, although I can't tell if it's working since there's no UI and history.dat is rather opaque.

Comment on attachment 155793 [details] [diff] [review] patches firefox and mozilla Asking biesi for review for when he gets back from vacation.

Comment on attachment 155793 [details] [diff] [review] patches firefox and mozilla Index: browser/base/content/contentAreaUtils.js + globalHistory.addURI(uri, false, true, getReferrer(document)); Hmm... so this will lie in some cases (i.e. will not match what is sent to the server). on the other hand... I do not think that this function should always use document, since it can be called for any document... either pass the document to it, or use null in the adduri call. (I do not understand why this function is used at all, but ok...) Index: docshell/base/nsIGlobalHistory2.idl you must change the IID of this interface maybe this should document that the implementation does not make sure that file: etc referrers will not be filtered out by the history impl? Index: toolkit/components/history/src/nsGlobalHistory.cpp This would have benefitted from a bit more context and the -p option to diff... Index: toolkit/components/history/src/nsGlobalHistory.h nsresult AddExistingPageToDatabase(nsIMdbRow *row, PRInt64 aDate, PRInt64 *aOldDate, - PRInt32 *aOldCount); + PRInt32 *aOldCount, + nsIURI *aReferrer); please don't add in parameters after out parameters. I.e. please move the referrer arg after aDate. Similarly, in AddNewPageToDatabase, move it after aTopLevel. Same in xpfe/components/history/src/nsGlobalHistory.h Note: alecf must review or super-review changes this patch

(In reply to comment #14) > (From update of attachment 155793 [details] [diff] [review]) > Index: browser/base/content/contentAreaUtils.js > + globalHistory.addURI(uri, false, true, getReferrer(document)); > > Hmm... so this will lie in some cases (i.e. will not match what is sent to the > server). > > on the other hand... I do not think that this function should always use > document, since it can be called for any document... either pass the document > to it, or use null in the adduri call. > > (I do not understand why this function is used at all, but ok...) I think this is the fix for marking a link as visited as soon as it's clicked on (I can't find the bug for this, but bug 78510 is the more general case). Passing null is fine since the real referrer should be set later by the docshell when the page actually loads. > Index: docshell/base/nsIGlobalHistory2.idl > > you must change the IID of this interface New uuid: cf777d42-1270-4b34-be7b-2931c93feda5 > maybe this should document that the implementation does not make sure that > file: etc referrers will not be filtered out by the history impl? Done > Index: toolkit/components/history/src/nsGlobalHistory.cpp > > This would have benefitted from a bit more context and the -p option to diff... > > Index: toolkit/components/history/src/nsGlobalHistory.h > nsresult AddExistingPageToDatabase(nsIMdbRow *row, > PRInt64 aDate, > PRInt64 *aOldDate, > - PRInt32 *aOldCount); > + PRInt32 *aOldCount, > + nsIURI *aReferrer); > > please don't add in parameters after out parameters. I.e. please move the > referrer arg after aDate. > > Similarly, in AddNewPageToDatabase, move it after aTopLevel. > > > Same in xpfe/components/history/src/nsGlobalHistory.h Done, see new patch. > Note: alecf must review or super-review changes this patch I'm getting asserts on display of the history window: ###!!! ASSERTION: URI is empty: '!aURI.IsEmpty()', file nsRDFService.cpp, line 1022 The number of these is equal to the number of history entries with an empty referrer. A backtrace indicates that it's in GetTarget, nsGlobalHistory.cpp line 1815 during what I guess is construction of the history window. Full backtrace available on request. Anyway, I'm not sure if I should care about these, since I don't have an unpatched debug build to see if they happen when the (empty without this patch) referrer column is displayed.

Comment on attachment 157788 [details] [diff] [review] cleaner patch oops, wrong match

Comment on attachment 157788 [details] [diff] [review] cleaner patch docshell/base/nsDocShell.cpp + // Get the referrrer uri from the channel It looks like there's one 'r' too much here :-) toolkit/components/history/src/nsGlobalHistory.cpp + // Set the referrer. + SetRowValue(row, kToken_ReferrerColumn, referrerSpec.get()); hm... maybe you should only do this if (aReferer)? xpfe/components/history/src/nsGlobalHistory.cpp + // no referrer? Now there is! + rv = GetRowValue(row, kToken_ReferrerColumn, oldReferrer); + if (NS_FAILED(rv) || oldReferrer.IsEmpty()) and here, only if aReferrer && *aReferrer? + // Set the referrer. + SetRowValue(row, kToken_ReferrerColumn, aReferrer); and here

(In reply to comment #18) > (From update of attachment 157788 [details] [diff] [review]) > docshell/base/nsDocShell.cpp > + // Get the referrrer uri from the channel > > It looks like there's one 'r' too much here :-) [snip] > hm... maybe you should only do this if (aReferer)? and one 'r' too few here :-) > xpfe/components/history/src/nsGlobalHistory.cpp > + // no referrer? Now there is! > + rv = GetRowValue(row, kToken_ReferrerColumn, oldReferrer); > + if (NS_FAILED(rv) || oldReferrer.IsEmpty()) > > and here, only if aReferrer && *aReferrer? > > + // Set the referrer. > + SetRowValue(row, kToken_ReferrerColumn, aReferrer); > > and here I checked with a vanilla build and one with the above ifs, and I get the same asserts I did as in comment #15 when I show the referrer column with empty referrers, so the ifs don't fix the asserts, but the asserts aren't my fault either. I'll attach the patch with the ifs anyway, since there's no point writing to history unless we've got something to put there.

Comment on attachment 158707 [details] [diff] [review] patch with ifs thanks.

Alecf currently has changed his Bugzilla name to "Alec Flett (on vacation until 1/1/2005)". So you probably want to choose another super-reviewer.

Comment on attachment 158707 [details] [diff] [review] patch with ifs #developers suggested neil for sr, asking

Comment on attachment 158707 [details] [diff] [review] patch with ifs >+ // no referrer? Now there is! >+ rv = GetRowValue(row, kToken_ReferrerColumn, oldReferrer); >+ if ((NS_FAILED(rv) || oldReferrer.IsEmpty()) && aReferrer && *aReferrer) >+ SetRowValue(row, kToken_ReferrerColumn, aReferrer); Wouldn't it be better to wrap this block in an if (aReferrer && *aReferrer) so we don't try to update the row if we know there's nothing to update with? >- rv = AddNewPageToDatabase(spec.get(), GetNow(), getter_AddRefs(row)); >+ rv = AddNewPageToDatabase(spec.get(), GetNow(), "", getter_AddRefs(row)); Should be nsnull instead of "" surely? sr=me with those nits fixed. Is there a bug filed on the assert?

(In reply to comment #24) > (From update of attachment 158707 [details] [diff] [review]) > >+ // no referrer? Now there is! > >+ rv = GetRowValue(row, kToken_ReferrerColumn, oldReferrer); > >+ if ((NS_FAILED(rv) || oldReferrer.IsEmpty()) && aReferrer && *aReferrer) > >+ SetRowValue(row, kToken_ReferrerColumn, aReferrer); > Wouldn't it be better to wrap this block in an if (aReferrer && *aReferrer) so > we don't try to update the row if we know there's nothing to update with? Yes, that's even better. > >- rv = AddNewPageToDatabase(spec.get(), GetNow(), getter_AddRefs(row)); > >+ rv = AddNewPageToDatabase(spec.get(), GetNow(), "", getter_AddRefs(row)); > Should be nsnull instead of "" surely? AddNewPageToDatabase takes a char* for aURL, so I matched this for aReferrer. This is xpfe only, toolkit takes nsIURIs for both. There's no actual difference between them, it's just a question of whether aReferrer->GetSpec(referrerSpec) and referrerSpec.get() are called outside or inside Add{New,Existing}PageToDatebase . > sr=me with those nits fixed. Do I need to submit a new patch? Also, how do I get someone to commit it? > Is there a bug filed on the assert? Not that I can see, searched on "history referrer" and "history assert".

Comment on attachment 159298 [details] [diff] [review] better ifs and nsnull in the toolkit globalhistory: if ((NS_FAILED(rv) || oldReferrer.IsEmpty())) you could remove one set of parens

so hm, sorry for only thinking about this now... but this only sets the referrer if one is not known previously. is this "correct"? consider I load the page again, with a different referrer. last visited date will be updated, page title, but not referrer? I was about to check this in, but then that thought crossed my mind :)

hmm, Neil convinced me, since when you browse around a site, you would otherwise overwrite the referrer. checked in.

looks like contentAreaUtil's change in browser/ needs to be relanded

Relanding relevant parts of patch following aviary branch landing

*** Bug 274636 has been marked as a duplicate of this bug. ***