342591 - set the referrer header when downloading a generator linked by a page

Status: RESOLVED WONTFIX

(Firefox Graveyard :: Microsummaries, defect)

Description

[Myk Melez [:myk] [@mykmelez]]

When a page links to a microsummary generator via <link rel="microsummary">, we should set the referrer header to the URL of the page whenever we download the generator.

This will be easier to do once bug 339543 lands, so making this bug depend on that one.

Comment 1

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Patch

This patch gives generators an ownerDocument property (set for any instantiated by MicrosummarySet()) and sends a Referer (not a typo: http://tools.ietf.org/html/rfc2616#section-14.36 ;)) header with the uri of that property if it's present.

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 257482 [details] [diff] [review]
Patch

>Index: nsMicrosummaryService.js

>       var resource = new MicrosummaryResource(this.generator.uri);
>+      resource.referrer = this.generator.ownerDocument;

I wonder if it would be better to add a "referrerURI" parameter to the MicrosummaryResource constructor and pass the referrer URI (if any) as the value of that parameter.


>+  // For generators specified via a link element, ownerDocument is the uri of
>+  // the page which included it.
>+  _ownerDocument: null,
>+  get ownerDocument() { return this._ownerDocument },
>+  set ownerDocument(uri) { this._ownerDocument = uri },

ownerDocument has a very specific meaning elsewhere in the Mozilla codebase, and I think its usage here will be misleading, so let's call this something else, f.e. referrerURI.


>+  _referrer: null,
>+  get referrer() { return this._referrer },
>+  set referrer(uri) { this._referrer = uri },

This should probably be referrerURI rather than simply referrer.


>+    if (this._referrer)
>+      request.setRequestHeader("Referer", this._referrer.spec);

This should use the getter (this.referrer) rather than referencing the internal element directly.

Otherwise this looks good except for one last issue, which I'll mention in a followup comment.

Comment 3

[Myk Melez [:myk] [@mykmelez]]

The last issue is that this only sends a Referer header when the remote generator is first extracted from a page, i.e. when creating a list of possible microsummaries for an Add Bookmark or Bookmark Properties dialog.

Once the user has set the bookmark to use the microsummary generator, the service no longer sets the Referer header when subsequently downloading the remote generator.

The original intent of this bug, as I recall, is for the service to always set the Referer header every time it downloads the generator in order to refresh the microsummary.  And I think that's probably the way it should work, but just to be sure, I'm cc:ing bz and biesi on this bug.

bz and biesi: is that the right thing to do when repeatedly downloading a remote microsummary generator to refresh a microsummary?

Ryan: if so, then I think you can fix this by setting the referrer every time the generator URI is not a urn:.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> bz and biesi: is that the right thing to do 

It's not exactly like the HTTP spec says...  It seems like a reasonable thing to do, I guess.

Comment 5

[Phil Ringnalda (:philor)]

Feels wrong to me, analogous to sending mozilla.org as a referrer to mozillazine.org with every Livemark fetch of their RSS feed, just because it was added from there. But then, sending it the first time feels worse, since I don't much like sites having a way to know whether or not I bookmark them, even if I don't use the microsummary they link to. At least sending it every time would generate enough noise to cover that up.

Comment 6

[Marco Bonardo [:mak]]

- BUGSPAM -
Wontfixing all Microsummaries bugs, since the feature has been removed from the core product and previous versions won't get further fixes for it.
If interested in supporting Microsummaries in your add-on, you're free to use our old microsummaries code and to search all previously open bugs by looking for [microsummaries-feature-removal] in the status whiteboard field.