202636 - document.referrer has no properties for XML documents

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Pratik]

Spun off from bug 192366. Looking at page info for any XML document always shows
"no referrer" as the referrer field. It should show the correct referrer.

Comment 1

[Pratik]

jst:

You had some thoughts on how to do this. Can you tell me what you said on IRC
(since I've forgotten what you said)? If we can come up with a design no how to
do this, then I can write up the code without much effort.

Comment 2

[Johnny Stenback (:jst)]

Simply add a "readonly attribute DOMString referrer" attribute to
nsIDOMNSDocument, and implement that in nsDocument, and you should be all set.
This attribute will be shadowed by nsIDOMHTMLDocument::referrer on HTML
documents, but that's ok, they're the same thing so it doesn't matter through
which interface you get the referrer.

Comment 3

[Johnny Stenback (:jst)]

Oh, and move the referrer code (except the getter) from nsHTMLDocument to
nsDocument.

Comment 4

[Pratik]

Attachment: patch, v1

Here's my first stab at this patch. I tested it with XML and XUL URLS and it
works.

Comment 5

[Johnny Stenback (:jst)]

Comment on attachment 139400 [details] [diff] [review]
patch, v1

- In content/base/public/nsIDocument.h:

+  nsCString mReferrer;

This new member should go into nsDocument, not into nsIDocument.

+NS_IMETHODIMP
+nsDocument::GetReferrer(nsAString& aReferrer)
+{
+  CopyUTF8toUTF16(mReferrer, aReferrer);
+
+  return NS_OK;
+}
+
+nsresult
+nsDocument::SetReferrer(const nsAString& aReferrer)
+{
+  CopyUTF16toUTF8(aReferrer, mReferrer);
+  return NS_OK;
+}
+
[...]
+    // The misspelled key 'referer' is as per the HTTP spec
+    rv = httpChannel->GetRequestHeader(NS_LITERAL_CSTRING("referer"),
+					 mReferrer);
(fix the indentation of the second line in that call)
+    if (NS_FAILED(rv)) {
+      mReferrer.Truncate();
+    }
+    

Now that you're adding this code to nsDocument, you can remove all the referrer
code from nsHTMLDocument (since that now inherits the code from nsDocument).
IOW, remove mReferrer from nsHTMLDocument, and the code that's not needed there
any more.

Fix that, and I'll have one more look.

Comment 6

[Pratik]

Thanks for the comments. I'll fix up my patch over the weekend.

Comment 7

[Pratik]

Attachment: patch, v2

Second try. I got rid of some of the referrer code from nsHTMLDocument but
can't seem to remove Get/SetReferrer. So I'm just calling
nsDocument::Get/SetReferrer().

Questions
1. Should nsDocument::SetRefferer be declared as an NS_IMETHOD?
2. My previous patch used CopyUTF16toUTF8/CopyUTF8toUTF16 but somehow I started
geting errors with those functions so I'm now using
CopyUCS2toASCII/CopyASCIItoUCS2. Let me know if which is the right function to
call.

Comment 8

[Johnny Stenback (:jst)]

Comment on attachment 139875 [details] [diff] [review]
patch, v2

+nsDocument::GetReferrer(nsAString& aReferrer)
+{
+//  CopyUTF8toUTF16(mReferrer, aReferrer);
+  CopyASCIItoUCS2(mReferrer, aReferrer);

Use the UTF8 version (it should compile, I can't see why it wouldn't here.

+NS_IMETHODIMP
+nsDocument::SetReferrer(const nsAString& aReferrer)
+{
+//  CopyUTF8toUTF16(aReferrer, mReferrer);
+  CopyUCS2toASCII(aReferrer, mReferrer);
+  return NS_OK;
+}

Seems like this method isn't needed, I don't see anyone calling it (except
nsHTMLDocument::SetReferrer, which can probably be removed too, see below).

- In nsDocument.h:

+  /**
+   * Set the referrer of this document.
+   */
+  NS_IMETHOD SetReferrer(const nsAString& aReferrer);

This should be removed too, unless it turns out to be needed.

 NS_IMETHODIMP
 nsHTMLDocument::SetReferrer(const nsAString& aReferrer)
 {
-  mReferrer.Assign(aReferrer);
-
-  return NS_OK;
+  return nsDocument::SetReferrer(aReferrer);
 }

The only reason this is needed here is that the SetReferrer() method is defined
in the interface nsIHTMLDocument, but I don't see anyone calling that method,
so it seems like you should be able to remove the method from nsIHTMLDocument,
and then eliminate it from nsHTMLDocument as well, and thus also from
nsDocument.

- In nsHTMLDocument.h:

-  nsString mReferrer;
+  //nsString mReferrer;

Just remove this alltogether, don't just comment it out.

Fix that, and this should be ready to go (assuming I'm correct on that
nsIHTMLDocument::SetReferrer() is not needed any more).

Comment 9

[Pratik]

Attachment: patch, v3

Third try.

Comment 10

[Johnny Stenback (:jst)]

Comment on attachment 139950 [details] [diff] [review]
patch, v3

Very nice! r+sr=jst

Comment 11

[Johnny Stenback (:jst)]

Fix checked in.

Comment 12

[Pratik]

Now that we got rid of RetrieveRelevantHeaders() from nsHTMLDocument, we don't
need to declare it as a virtual function in nsDocument. Can we just drop the
virtual attribute? No one else overrides it (according to lxr).

Comment 13

[Johnny Stenback (:jst)]

Yup, good catch! Fix checked in.

Comment 14

[Boris Zbarsky [:bzbarsky]]

This caused bug 233197 -- some of the code removed from
nsHTMLDocument::RetrieveRelevantHeaders was not placed anywhere else...

Comment 15

[Pratik]

Oops.. Gotta be more careful. Thanks for taking care of this bz.