128398 - Referrer column is empty for the History

Status: RESOLVED FIXED

(Core Graveyard :: History: Global, defect)

Description

[Benjamin Mucci]

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

Comment 1

[R.K.Aa.]

also on Linux. Resolving as new.

Comment 2

[Robin Green]

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

Comment 3

[Blake Ross]

Alec: did this ever work?

Comment 4

[Alec Flett]

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.

Comment 5

[Andy Edmonds]

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

Comment 6

[Jesse Ruderman]

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.

Comment 7

[James Andrewartha]

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).

Comment 8

[James Andrewartha]

Attachment: patch to set referrers in history

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 9

[James Andrewartha]

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.

Comment 10

[Andy Edmonds]

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.

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

(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.

Comment 12

[James Andrewartha]

Attachment: patches firefox and mozilla

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 13

[James Andrewartha]

Comment on attachment 155793 [details] [diff] [review]
patches firefox and mozilla

Asking biesi for review for when he gets back from vacation.

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 15

[James Andrewartha]

(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 16

[James Andrewartha]

Attachment: cleaner patch

Comment 17

[James Andrewartha]

Comment on attachment 157788 [details] [diff] [review]
cleaner patch

oops, wrong match

Comment 18

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 19

[James Andrewartha]

(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 20

[James Andrewartha]

Attachment: patch with ifs

Comment 21

[Christian :Biesinger (don't email me, ping me on IRC)]

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

thanks.

Comment 22

[Stefan Borggraefe]

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 23

[James Andrewartha]

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

#developers suggested neil for sr, asking

Comment 24

[[e-mail]]

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?

Comment 25

[James Andrewartha]

(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 26

[James Andrewartha]

Attachment: better ifs and nsnull

Comment 27

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 28

[Christian :Biesinger (don't email me, ping me on IRC)]

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 :)

Comment 29

[Christian :Biesinger (don't email me, ping me on IRC)]

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

checked in.

Comment 30

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 31

[Ian Neal]

Relanding relevant parts of patch following aviary branch landing

Comment 32

[Jesse Ruderman]

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